Re: [PATCH v3 04/21] pinctrl: starfive: Add StarFive JHB100 sys0 controller driver

Hi, Linus

> On Sat, Jul 25, 2026 at 12:25 PM Changhuang Liang
> <[email protected]> wrote:
> > > On Wed, Jun 3, 2026 at 7:54 AM Changhuang Liang
> > > <[email protected]> wrote:
> 
> > > > +static unsigned int jhb100_gpio_to_pin(struct gpio_chip *gc,
> > > > +unsigned int gpio) {
> > > > +       struct jhb100_gpio_bank *bank = jhb100_gc_to_bank(gc);
> > > > +
> > > > +       return bank->id * JHB100_NR_GPIOS_PER_BANK + gpio; }
> > >
> > > This usually tells me that GPIO_GENERIC can be used but maybe this
> > > has been discussed before...
> >
> > Here, the main purpose is to convert GPIOs into pins for configuring
> > certain pinctrl settings. I haven't found the part where GPIO_GENERIC can
> assist with this.
> > Can you point it out for me? Thank you very much.
> 
> Normally GPIO chips use the .config() callback when you want to set pin
> config for a certain pin in a pinctrl backend.
> 
> See Documentation/driver-api/gpio/driver.rst:
> 
> "The .set_config() callback uses the same enumerators and configuration
> semantics as the generic pin control drivers. This is not a coincidence: it is
> possible to assign the .set_config() to the function gpiochip_generic_config()
> which will result in pinctrl_gpio_set_config() being called and eventually
> ending up in the pin control back-end "behind" the GPIO controller, usually
> closer to the actual pins. This way the pin controller can manage the below
> listed GPIO configurations.
> 
> If a pin controller back-end is used, the GPIO controller or hardware
> description needs to provide "GPIO ranges" mapping the GPIO line offsets to
> pin numbers on the pin controller so they can properly cross-reference each
> other."

I tried this change, but it doesn't work. In my new version, the GPIO direction is set 
via the `struct pinmux_ops .gpio_set_direction` callback, which is executed after 
`mutex_lock(&pctldev->mutex);`. I need to configure some pinconf settings while 
setting the GPIO direction inside `.gpio_set_direction`, for example:

    config = pinconf_to_config_packed(PIN_CONFIG_INPUT_ENABLE, 0);
    ret = pinctrl_gpio_set_config(gc, gpio, config);

However, `pinctrl_gpio_set_config` will again acquire the lock with 
`mutex_lock(&pctldev->mutex);`.

So this approach may no longer work?

> Maybe this is what you're looking for?
> 
> If you define gpio-ranges in your device tree, the GPIO and pin control core
> deals with the cross-referencing for you.
> 
> > > > +       sfp->num_banks = DIV_ROUND_UP(sfp->ngpios,
> > > > + JHB100_NR_GPIOS_PER_BANK);
> > > > +
> > > > +       for (unsigned int i = 0; i < sfp->num_banks; i++) {
> > > > +               if (sfp->ngpios > (i + 1) *
> > > JHB100_NR_GPIOS_PER_BANK)
> > > > +                       sfp->banks[i].gc.ngpio = (i + 1) *
> > > JHB100_NR_GPIOS_PER_BANK;
> > > > +               else
> > > > +                       sfp->banks[i].gc.ngpio = sfp->ngpios - i *
> > > > + JHB100_NR_GPIOS_PER_BANK;
> > >
> > > This looks completely bananas, shouldn't this be simply:
> > >
> > > sfp->banks[i].gc.ngpio = JHB100_NR_GPIOS_PER_BANK;
> >
> > The JHB100 GPIO domain is not fully aligned to 32. For example, in the
> > per0 domain, there are 60 GPIOs, so I split it into:
> > bank0: 32 GPIOs
> > bank1: 28 GPIOs.
> 
> At least add that, what you just wrote, as a comment right above this loop.
> 
> There is something with this math I think can be made simpler with bitwise
> artithmetic. Think about it.
> 
> > > > +               sfp->banks[i].gc.request = pinctrl_gpio_request;
> > >
> > > Use
> > > gpiochip_generic_request
> > >
> > > > +               sfp->banks[i].gc.free = pinctrl_gpio_free;
> > >
> > > Use
> > > gpiochip_generic_free
> > >
> > > These calls will do what you want, and also check that the right
> > > gpio ranges are available.
> > >
> > > Make sure you add GPIO ranges (the mapping between pin control pins
> > > and corresponding GPIO offsets) for this to work properly.
> > >
> > > I'm pretty sure you can have a generic pin config backend as well.
> > >
> > > sfp->banks[i].gc.set_config = gpiochip_generic_config;
> 
> This is pertaining to what I said above about using the gpio ranges and .config
> etc.
> 
> > > This will make config calls to the gpiochip call into the pinctrl
> > > backend = what you want.
> 
> I still think this is what you want.
> 
> Yours,
> Linus Walleij

Best Regards,
Changhuang
_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv