| Message ID | 20230209143702.44408-1-hal.feng@starfivetech.com |
|---|---|
| Headers | show |
| Series | Basic pinctrl support for StarFive JH7110 RISC-V SoC | expand |
On Thu, Feb 9, 2023 at 3:37 PM Hal Feng <hal.feng@starfivetech.com> wrote:
> This patch series adds basic pinctrl support for StarFive JH7110 SoC.
This v4 version applied, the driver is in good shape and all bindings ACKed,
nice work on this driver!
Yours,
Linus Walleij
On Fri, 10 Feb 2023 23:45:05 +0100, Linus Walleij wrote: > On Thu, Feb 9, 2023 at 3:37 PM Hal Feng <hal.feng@starfivetech.com> wrote: > > > This patch series adds basic pinctrl support for StarFive JH7110 SoC. > > This v4 version applied, the driver is in good shape and all bindings ACKed, > nice work on this driver! v4? Is this a typo? This series is v5. Anyway, thank you so much! Best regards, Hal
On Sat, 11 Feb 2023 at 09:25, Hal Feng <hal.feng@starfivetech.com> wrote: > On Fri, 10 Feb 2023 23:45:05 +0100, Linus Walleij wrote: > > On Thu, Feb 9, 2023 at 3:37 PM Hal Feng <hal.feng@starfivetech.com> wrote: > > > > > This patch series adds basic pinctrl support for StarFive JH7110 SoC. > > > > This v4 version applied, the driver is in good shape and all bindings ACKed, > > nice work on this driver! > > v4? Is this a typo? This series is v5. > Anyway, thank you so much! Hi Hal and Linus I'm curious if there is a plan to address Icenowy's concerns here: https://lore.kernel.org/linux-gpio/52dcbe48dbf5f2561713a9642943353216fef15a.camel@icenowy.me/ The problem is that input from "GPIO" pins is configured a little differently on the StarFive SoCs. Instead of having a register pr. pin(grroup) there is a register pr. control line to the peripherals, and into these you write the pin number + 2 of the pin you want the peripheral to react to. Why +2? That's because 0 is a special "always low" signal and similarly 1 is a special "always high" signal. With the current bindings one hacky way to solve this is to treat those two special values as kind of "virtual pins" that will always be high or low. So that would be something like pinmux = <GPIOMUX(GPIO_ALWAYS_LOW, GPOUT_IGNORED, GPOEN_DISABLE, GPI_SYS_USB_OVERCURRENT)>; ..but this means we might need to mux these two virtual pins to multiple peripherals. I'm not sure the pinmux framework is prepared for that. /Emil > Best regards, > Hal
On Mon, Feb 20, 2023 at 11:16 AM Emil Renner Berthing <emil.renner.berthing@canonical.com> wrote: > Hi Hal and Linus > > I'm curious if there is a plan to address Icenowy's concerns here: > https://lore.kernel.org/linux-gpio/52dcbe48dbf5f2561713a9642943353216fef15a.camel@icenowy.me/ > > The problem is that input from "GPIO" pins is configured a little > differently on the StarFive SoCs. Instead of having a register pr. > pin(grroup) there is a register pr. control line to the peripherals, > and into these you write the pin number + 2 of the pin you want the > peripheral to react to. Why +2? That's because 0 is a special "always > low" signal and similarly 1 is a special "always high" signal. > > With the current bindings one hacky way to solve this is to treat > those two special values as kind of "virtual pins" that will always be > high or low. So that would be something like > > pinmux = <GPIOMUX(GPIO_ALWAYS_LOW, GPOUT_IGNORED, GPOEN_DISABLE, > GPI_SYS_USB_OVERCURRENT)>; > > ..but this means we might need to mux these two virtual pins to > multiple peripherals. I'm not sure the pinmux framework is prepared > for that. I am not a fan of the magic numbers approach to pin muxing at all and I often point out that I prefer that you use strings and connect groups with functions using strings. The above mentioned problems is one of the reasons. As maintainer I have been repeatedly hammered down by maintainers who think magic numbers is good for them, usually because it saves space in the DTS file and is easier to code a driver for. So this alternative approach is acceptable, as a compromise. My apologies if I didn't point this out as sternly as I often do for StarFive. Really sorry. I guess I have just started to give up on insisting, as so many people like their magic numbers. If you switch to stop using the "pinmux" property and instead use string arrays function and groups as defined in Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml this should fix it, the old pinmux device trees can be supported as fallback, if pinmux is present but function and groups are missing. So that is my suggestion. Usually people don't like this suggestion, so I suspect you might not like it either and come up with something else to work around it. Yours, Linus Walleij