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

Hi, Linus

Thanks for the review.
I'm very sorry for replying so late.

> Hi Changhuang,
> 
> thanks for your patch!
> 
> On Wed, Jun 3, 2026 at 7:54 AM Changhuang Liang
> <[email protected]> wrote:
> 
> > Add pinctrl driver for StarFive JHB100 SoC System-0(sys0) pinctrl
> > controller.
> >
> > Co-developed-by: Lianfeng Ouyang <[email protected]>
> > Signed-off-by: Lianfeng Ouyang <[email protected]>
> > Signed-off-by: Changhuang Liang <[email protected]>
> 
> This patch adds generic infrastructure "JHB100" that is then used by several
> drivers does it not?
> 
> Write something about that and some about the design in the commit
> message.
> 
> > +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jhb100-sys0.c
> > @@ -0,0 +1,123 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +/*
> > + * Pinctrl / GPIO driver for StarFive JHB100 SoC System-0 domain
> > + *
> > + * Copyright (C) 2024 StarFive Technology Co., Ltd.
> > + * Author: Alex Soo <[email protected]>
> 
> Shouldn't this person be in the Signed-off-by?
> 
> I guess it's not legally necessary but feels appropriate.
> 
> > +static struct config_reg_layout_desc jhb100_sys0_pinctrl_crl_desc[] = {
> > +       {
> > +               .pin_start                      = 0,
> > +               .pin_cnt                        = 4,
> > +               .drive_strength_2bit            = { .shift = 0, .width
> = 2 },
> > +               .input_enable                   = { .shift =
> 2, .width = 1 },
> > +               .pull_down                      = { .shift =
> 3, .width = 1 },
> > +               .pull_up                        = { .shift =
> 4, .width = 1 },
> > +               .slew_rate                      = { .shift =
> 5, .width = 1 },
> > +               .schmitt_trigger_select         = { .shift = 6, .width =
> 1 },
> > +               .reserved                       = { .shift =
> 7, .width = 8 },
> > +               .debounce_width                 = { .shift =
> 15, .width = 17 },
> > +       },
> > +       {
> > +               .pin_start                      = 4,
> > +               .pin_cnt                        = 5,
> > +               .schmitt_trigger_select         = { .shift = 0, .width =
> 1 },
> > +               .reserved                       = { .shift =
> 1, .width = 31 },
> > +       },
> > +       {
> > +               .pin_start                      = 9,
> > +               .pin_cnt                        = 1,
> > +               .drive_strength_2bit            = { .shift = 0, .width
> = 2 },
> > +               .slew_rate                      = { .shift =
> 2, .width = 1 },
> > +               .reserved                       = { .shift =
> 3, .width = 29 },
> > +       },
> > +       {
> > +               .pin_start                      = 10,
> > +               .pin_cnt                        = 1,
> > +               .drive_strength_2bit            = { .shift = 0, .width
> = 2 },
> > +               .input_enable                   = { .shift =
> 2, .width = 1 },
> > +               .pull_down                      = { .shift =
> 3, .width = 1 },
> > +               .pull_up                        = { .shift =
> 4, .width = 1 },
> > +               .slew_rate                      = { .shift =
> 5, .width = 1 },
> > +               .schmitt_trigger_select         = { .shift = 6, .width =
> 1 },
> > +               .reserved                       = { .shift =
> 7, .width = 25 },
> > +       },
> > +       { 0xff },
> > +};
> 
> Would it be appropriate to index the different register variants with a enum
> with a good name so it is easy to understand which variant each entry in the
> array is?
> 
> > +#include <linux/string.h>
> > +#include <linux/sort.h>
> 
> Hm why... I guess I will see.
> 
> > +#define JHB100_DEBOUNCE_WIDTH_STAGES_MAX       0x1FFFFU
> 
> Is that a GENMASK(16,0)?
> 
> Since it seems to have something to do with bitfield widths.
> 
> > +/* i2c open-drain pull-up select */
> > +#define JHB100_I2C_OPEN_DRAIN_PU_600_OHMS      0
> > +#define JHB100_I2C_OPEN_DRAIN_PU_900_OHMS      1
> > +#define JHB100_I2C_OPEN_DRAIN_PU_1200_OHMS     2
> > +#define JHB100_I2C_OPEN_DRAIN_PU_2000_OHMS     3
> 
> Very nice and to the point! It's easy to read and understand drivers that are
> writing things out explicitly like this!
> 
> > +#define JHB100_NR_GPIOS_PER_BANK               32
> (...)
> > +static inline struct jhb100_gpio_bank *jhb100_gc_to_bank(struct
> > +gpio_chip *gc) {
> > +       return container_of(gc, struct jhb100_gpio_bank, gc); }
> > +
> > +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.

> 
> > +static const struct pinctrl_ops jhb100_pinctrl_ops = {
> > +       .get_groups_count = pinctrl_generic_get_group_count,
> > +       .get_group_name   = pinctrl_generic_get_group_name,
> > +       .get_group_pins   = pinctrl_generic_get_group_pins,
> > +       .dt_node_to_map   =
> pinctrl_generic_pins_function_dt_node_to_map,
> > +       .dt_free_map      = pinctrl_utils_free_map,
> > +};
> 
> Nice use of the generic helpers!
> 
> > +static void jhb100_set_gpioval(struct jhb100_pinctrl *sfp, unsigned int pin,
> > +                              unsigned int val) {
> > +       const struct jhb100_pinctrl_domain_info *info = sfp->info;
> > +       unsigned int offset = 4 * (pin / 32);
> > +       unsigned int shift = 1 * (pin % 32);
> > +       unsigned int fs_offset = 4 * (pin / 16);
> > +       unsigned int fs_shift = 2 * (pin % 16);
> > +       u32 func_sel_mask;
> > +       u32 dout, doen, fs;
> > +       void __iomem *reg_gpio_o;
> > +       void __iomem *reg_gpio_oen;
> > +       void __iomem *reg_gpio_func_sel;
> > +       unsigned long flags;
> > +
> > +       reg_gpio_o = sfp->base + info->regs->output + offset;
> > +       reg_gpio_oen = sfp->base + info->regs->output_en + offset;
> > +       reg_gpio_func_sel = sfp->base + info->regs->func_sel.reg +
> > + fs_offset;
> 
> The part from here:
> 
> > +       func_sel_mask = GENMASK(info->regs->func_sel.width_per_pin -
> > + 1, 0) << fs_shift;
> (...)
> > +
> > +       raw_spin_lock_irqsave(&sfp->lock, flags);
> > +       fs = readl_relaxed(reg_gpio_func_sel);
> > +       if (fs & func_sel_mask) {
> > +               fs &= ~func_sel_mask;
> > +               writel_relaxed(fs, reg_gpio_func_sel);
> > +       }
> 
> ..to here seems to reimplement the shortcut
> .gpio_request_enable() in struct pinmux_ops.
> 
> Then this:
> 
> > +       dout = val << shift;
> > +       doen = 0;
> 
> > +       dout |= readl_relaxed(reg_gpio_o) & ~BIT(shift);
> > +       writel_relaxed(dout, reg_gpio_o);
> > +       doen |= readl_relaxed(reg_gpio_oen) & ~BIT(shift);
> > +       writel_relaxed(doen, reg_gpio_oen);
> 
> Seems more like the actual code that should be here.
> 
> > +       raw_spin_unlock_irqrestore(&sfp->lock, flags);
> 
> Please use guards for these spinlocks. They make for less bugs.
> 
> guard(raw_spinlock_irqsave)(&sfp->lock);
> 
> > +static const struct pinmux_ops jhb100_pinmux_ops = {
> > +       .get_functions_count = pinmux_generic_get_function_count,
> > +       .get_function_name   = pinmux_generic_get_function_name,
> > +       .get_function_groups = pinmux_generic_get_function_groups,
> > +       .set_mux             = jhb100_set_mux,
> > +};
> 
> Implement .gpio_request_enable() (see above) and
> .gpio_set_direction() see below.
> 
> Maybe also .gpio_disable_free() if you need to deconfigure stuff when a pin is
> release from GPIO.
> 
> > +static const struct pinconf_ops jhb100_pinconf_ops = {
> > +       .pin_config_get         = jhb100_pinconf_get,
> > +       .pin_config_set         = jhb100_pinconf_set,
> > +       .pin_config_group_get   = jhb100_pinconf_group_get,
> > +       .pin_config_group_set   = jhb100_pinconf_group_set,
> > +       .is_generic             = true,
> > +};
> 
> Overall this looks nice, good use of the group config!
> 
> > +static int jhb100_gpio_get_direction(struct gpio_chip *gc,
> > +                                    unsigned int gpio) {
> > +       struct jhb100_gpio_bank *bank = jhb100_gc_to_bank(gc);
> > +       struct jhb100_pinctrl *sfp = gpiochip_get_data(gc);
> > +       const struct jhb100_pinctrl_domain_info *info = sfp->info;
> > +       unsigned int offset = 4 * bank->id;
> > +       u32 doen;
> > +       void __iomem *reg_gpio_oen;
> > +
> > +       reg_gpio_oen = sfp->base + info->regs->output_en + offset;
> > +
> > +       doen = (readl_relaxed(reg_gpio_oen) & BIT(gpio)) >> gpio;
> > +
> > +       return doen == GPOEN_ENABLE ? GPIO_LINE_DIRECTION_OUT :
> > +GPIO_LINE_DIRECTION_IN; }
> > +
> > +static int jhb100_gpio_direction_input(struct gpio_chip *gc,
> > +                                      unsigned int gpio) {
> > +       struct jhb100_pinctrl *sfp = gpiochip_get_data(gc);
> > +       struct device *dev = sfp->dev;
> > +       struct config_reg_layout_desc *crl_desc;
> > +       unsigned int pin = jhb100_gpio_to_pin(gc, gpio);
> > +
> > +       crl_desc = get_crl_desc_by_pin(sfp, pin);
> > +       if (!crl_desc) {
> > +               dev_err(dev, "pin %d can't not found reg layout
> descriptor\n",
> > +                       pin);
> > +               return -EINVAL;
> > +       }
> > +
> > +       jhb100_padcfg_rmw(sfp, pin,
> > +                         RL_DESC_GENMASK(crl_desc,
> input_enable) |
> > +                         RL_DESC_GENMASK(crl_desc,
> schmitt_trigger_select),
> > +                         RL_DESC_GENMASK(crl_desc,
> input_enable) |
> > +                         RL_DESC_GENMASK(crl_desc,
> > + schmitt_trigger_select));
> 
> Instead of doing these writes directly into the config registers, implement
> .gpio_set_direction() in struct pinmux_ops and call the pinmux generic
> back-end.
> 
> > +static int jhb100_gpio_direction_output(struct gpio_chip *gc,
> > +                                       unsigned int gpio, int
> value)
> > +{
> > +       struct jhb100_pinctrl *sfp = gpiochip_get_data(gc);
> > +       struct device *dev = sfp->dev;
> > +       struct config_reg_layout_desc *crl_desc;
> > +       unsigned int pin = jhb100_gpio_to_pin(gc, gpio);
> > +
> > +       jhb100_set_one_pin_mux(sfp, pin, 0,
> > +                              value ? GPOUT_HIGH :
> GPOUT_LOW);
> > +
> > +       crl_desc = get_crl_desc_by_pin(sfp, pin);
> > +       if (!crl_desc) {
> > +               dev_err(dev, "pin %d can't not found reg layout
> descriptor\n",
> > +                       pin);
> > +               return -EINVAL;
> > +       }
> > +
> > +       jhb100_padcfg_rmw(sfp, pin,
> > +                         RL_DESC_GENMASK(crl_desc,
> input_enable) |
> > +                         RL_DESC_GENMASK(crl_desc,
> schmitt_trigger_select) |
> > +                         RL_DESC_GENMASK(crl_desc, pull_down) |
> > +                         RL_DESC_GENMASK(crl_desc, pull_up),
> > +                         0);
> 
> Dito.
> 
> > +static int jhb100_gpio_get(struct gpio_chip *gc, unsigned int gpio) {
> > +       struct jhb100_gpio_bank *bank = jhb100_gc_to_bank(gc);
> > +       struct jhb100_pinctrl *sfp = gpiochip_get_data(gc);
> > +       const struct jhb100_pinctrl_domain_info *info = sfp->info;
> > +       unsigned int offset = 4 * bank->id;
> > +       u32 doen = 0;
> > +       void __iomem *reg_gpio_oen;
> > +       void __iomem *reg;
> > +       unsigned long flags;
> > +
> > +       reg_gpio_oen = sfp->base + info->regs->output_en + offset;
> > +       reg = sfp->base + info->regs->gpio_status + offset;
> > +
> > +       raw_spin_lock_irqsave(&sfp->lock, flags);
> > +       doen = readl_relaxed(reg_gpio_oen) | BIT(gpio);
> > +       writel_relaxed(doen, reg_gpio_oen);
> > +       raw_spin_unlock_irqrestore(&sfp->lock, flags);
> 
> Why *on* *earth* are you read-modify-writing the output enable register in
> the *get* function? Is this a copy-on-paste error??
> 
> > +       return !!(readl_relaxed(reg) & BIT(gpio % 32));
> 
> Also you never actuall read reg .... ehhhh this is a glaring bug.

Will Fix.

> 
> > +static int jhb100_gpio_set(struct gpio_chip *gc, unsigned int gpio,
> > +int value) {
> > +       struct jhb100_gpio_bank *bank = jhb100_gc_to_bank(gc);
> > +       struct jhb100_pinctrl *sfp = gpiochip_get_data(gc);
> > +       const struct jhb100_pinctrl_domain_info *info = sfp->info;
> > +       unsigned int offset = 4 * bank->id;
> > +       void __iomem *reg_dout;
> > +       u32 dout;
> > +       unsigned long flags;
> > +
> > +       reg_dout = sfp->base + info->regs->output + offset;
> > +       dout = (value ? GPOUT_HIGH : GPOUT_LOW) << gpio;
> > +
> > +       raw_spin_lock_irqsave(&sfp->lock, flags);
> > +       dout |= readl_relaxed(reg_dout) & ~BIT(gpio);
> > +       writel_relaxed(dout, reg_dout);
> > +       raw_spin_unlock_irqrestore(&sfp->lock, flags);
> > +
> > +       return 0;
> > +}
> 
> This looks right, did you only test output and not input..?
> 
> > +static const struct irq_chip jhb100_irq_chip = {
> > +       .irq_ack        = jhb100_irq_ack,
> > +       .irq_mask       = jhb100_irq_mask,
> > +       .irq_mask_ack   = jhb100_irq_mask_ack,
> > +       .irq_unmask     = jhb100_irq_unmask,
> > +       .irq_set_type   = jhb100_irq_set_type,
> > +       .irq_set_wake   = jhb100_irq_set_wake,
> > +       .irq_print_chip = jhb100_irq_print_chip,
> > +       .flags          = IRQCHIP_SET_TYPE_MASKED |
> > +                         IRQCHIP_IMMUTABLE |
> > +                         IRQCHIP_ENABLE_WAKEUP_ON_SUSPEND
> |
> > +                         IRQCHIP_MASK_ON_SUSPEND |
> > +                         IRQCHIP_SKIP_SET_WAKE,
> > +       GPIOCHIP_IRQ_RESOURCE_HELPERS, };
> 
> The irqchip looks good!
> 
> > +static int field_compare(const void *a, const void *b) {
> > +       const struct field_info *fa = (const struct field_info *)a;
> > +       const struct field_info *fb = (const struct field_info *)b;
> > +
> > +       if (fa->shift < fb->shift)
> > +               return -1;
> > +
> > +       if (fa->shift > fb->shift)
> > +               return 1;
> > +
> > +       return 0;
> > +}
> 
> Are you sure the kernel doesn't already have a helper like this...
> 
> > +       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.

> 
> ???
> 
> What is getting assigned to ngpios looks like a gpiochip base, and have all the
> signs of a real bad AI hallucination.
> 
> > +
> > +               sfp->banks[i].id = i;
> > +
> > +               sfp->banks[i].gc.parent = dev;
> > +               sfp->banks[i].gc.label = dev_name(dev);
> > +               sfp->banks[i].gc.owner = THIS_MODULE;
> > +               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 will make config calls to the gpiochip call into the pinctrl backend = what
> you want.
> 
> > +               sfp->banks[i].gc.get_direction =
> jhb100_gpio_get_direction;
> > +               sfp->banks[i].gc.direction_input =
> jhb100_gpio_direction_input;
> > +               sfp->banks[i].gc.direction_output =
> jhb100_gpio_direction_output;
> > +               sfp->banks[i].gc.get = jhb100_gpio_get;
> > +               sfp->banks[i].gc.set = jhb100_gpio_set;
> > +               sfp->banks[i].gc.set_config = gpiochip_generic_config;
> > +               sfp->banks[i].gc.base = -1;
> > +               sfp->banks[i].gc.of_gpio_n_cells = 3;
> > +               sfp->banks[i].gc.of_node_instance_match =
> > + starfive_of_node_instance_match;
> 
> Since you have a threecell scheme with 32 gpios
> (JHB100_NR_GPIOS_PER_BANK)  per instance (right? the ngpios code above
> made me really confused....) you should be able so select GPIO_GENERIC,
> #include <linux/gpio/generic.h> and use the generic GPIO pretty much the
> same way drivers/gpio/gpio-spacemit-k1.c does it, check that driver out
> (especially spacemit_gpio_add_bank()).
> 

Best Regards,
Changhuang