[PATCH v1 02/20] pinctrl: starfive: Add StarFive JHB100 sys0 controller driver

[Changhuang Liang]

Hi Linus,

Thanks for the review.

> Hi Changhuang,
> 
> thanks for your patch!
> 
> On Fri, Apr 24, 2026 at 1:13 PM Changhuang Liang
> <changhuang.liang at starfivetech.com> wrote:
> 
> > Add pinctrl driver for StarFive JHB100 SoC System-0(sys0) pinctrl
> > controller.
> >
> > Signed-off-by: Lianfeng Ouyang <lianfeng.ouyang at starfivetech.com>
> > Signed-off-by: Changhuang Liang <changhuang.liang at starfivetech.com>
> 
> (...)
> 
> > +config PINCTRL_STARFIVE_JHB100
> > +       bool
> > +       select GENERIC_PINCONF
> > +       select GENERIC_PINCTRL_GROUPS
> > +       select GENERIC_PINMUX_FUNCTIONS
> 
> Neat that you use the generic stuff!
> 
> > +#include <linux/pinctrl/consumer.h>
> 
> Do you really need the consumer header?

Will drop it

> > +/* Custom pinconf parameters */
> > +#define STARFIVE_PIN_CONFIG_GMAC_VSEL
> (PIN_CONFIG_END + 1)
> > +#define STARFIVE_PIN_CONFIG_DEBOUNCE_WIDTH
> (PIN_CONFIG_END + 2)
> 
> Hm I wonder about this...
> 
> > +#define STARFIVE_PIN_DRIVE_I2C_FAST_MODE
> (PIN_CONFIG_END + 3)
> > +#define STARFIVE_PIN_DRIVE_I2C_FAST_MODE_PLUS
> (PIN_CONFIG_END + 4)
> > +#define STARFIVE_PIN_OPEN_DRAIN_PULLUP_SELECT
> (PIN_CONFIG_END + 5)
> 
> But the existing pullup already takes an argument. This looks wrong.
> 
> > +static const struct pinconf_generic_params jhb100_custom_bindings[] = {
> > +       { "starfive,gmac-vsel", STARFIVE_PIN_CONFIG_GMAC_VSEL, 0 },
> 
> Can't you use the existing "power-source" instead? It's fine if it's only
> applicable to a few pins. This is overly specific.
> 
> > +       { "starfive,debounce-width",
> > + STARFIVE_PIN_CONFIG_DEBOUNCE_WIDTH, 0 },
> 
> Don't know about this... sounds like the argument to the existing
> input-debounce which is expressed in microseconds. Just recalculate that
> value to your "width"?
> 
> > +       { "starfive,drive-i2c-fast-mode",
> STARFIVE_PIN_DRIVE_I2C_FAST_MODE, 0 },
> > +       { "starfive,drive-i2c-fast-mode-plus",
> > + STARFIVE_PIN_DRIVE_I2C_FAST_MODE_PLUS, 0 },
> 
> It's not special that things are for i2c. Use the generic slew-rate for these two,
> it describes how fast something is.
> 
> > +       { "starfive,i2c-open-drain-pull-up-ohm",
> > + STARFIVE_PIN_OPEN_DRAIN_PULLUP_SELECT, 0 },
> 
> Use the existing drive-open-drain; with the exitsing bias-pull-up = <ohms>;
> two properties. No need to be fancy and create a new property for this.
> 
> > +       { "starfive,vga-rte", STARFIVE_PIN_VGA_RTE_SELECT, 0 },
> 
> No idea what this is...

This is the retention signal bus used for configuring the VGA domain. However, after our review, we found that VGA does not use this property, so we have decided to remove this configuration in the next version.

> 
> > +static int jhb100_dt_node_to_map(struct pinctrl_dev *pctldev,
> > +                                struct device_node *np,
> > +                                struct pinctrl_map **maps,
> > +                                unsigned int *num_maps)
> 
> Long complicated function. Try to use the generic helper instead and extend it
> if need be.
> 
> > +static void jhb100_dt_free_map(struct pinctrl_dev *pctldev, struct
> pinctrl_map *map,
> > +                              unsigned int num_maps)
> 
> Use the generic helper instead.
> 
> > +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   = jhb100_dt_node_to_map,
> > +       .dt_free_map      = jhb100_dt_free_map,
> 
> Maybe the need for this goes away if you just use the generic properties
> instead?
> 
> > +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);
> 
> All of these are signs that the GPIOs are "banked" into 32 GPIOs per bank, and
> I think the chips get simpler and easier to use if they get split into threecell DT
> phandles.
> 
> > +static int jhb100_gpio_get_direction(struct gpio_chip *gc,
> > +                                    unsigned int gpio) {
> > +       struct jhb100_pinctrl *sfp = container_of(gc, struct jhb100_pinctrl,
> gc);
> > +       const struct jhb100_pinctrl_domain_info *info = sfp->info;
> > +       unsigned int offset = 4 * (gpio / 32);
> > +       unsigned int shift = 1 * (gpio % 32);
> 
> Here too.
> 
> > +static void jhb100_gpio_irq_handler(struct irq_desc *desc) {
> > +       struct jhb100_pinctrl *sfp = jhb100_from_irq_desc(desc);
> > +       struct irq_chip *chip = irq_desc_get_chip(desc);
> > +       struct gpio_irq_chip *girq = &sfp->gc.irq;
> > +       struct starfive_pinctrl_regs *pinctrl_regs = sfp->info->regs;
> > +       unsigned long is;
> > +       unsigned int pin;
> > +       unsigned int total, size, remain = sfp->npins;
> > +
> > +       chained_irq_enter(chip, desc);
> > +
> > +       for (total = 0, size = 0; total < sfp->npins; total += 32, remain -=
> size) {
> > +               is = readl_relaxed(sfp->base +
> pinctrl_regs->irq_status.reg +
> > +                                  (total >> 3));
> > +               size = umin(remain, 32);
> > +
> > +               for_each_set_bit(pin, &is, size) {
> > +                       if (sfp->gpio_func_sel_arr[pin] >= 0)
> > +
> generic_handle_domain_irq(girq->domain, pin);
> > +               }
> > +       }
> > +
> > +       chained_irq_exit(chip, desc);
> > +}
> 
> And since the IRQs are groups per 32 pins I think this will become much easier
> if you break the GPIOs into banks.
> 
> (...)
> > +++ b/drivers/pinctrl/starfive/pinctrl-starfive-jhb100.h
> 
> > +struct starfive_pinctrl_regs {
> > +       struct pinvref_reg vref;
> > +       struct gpio_irq_reg config;
> > +       struct gpio_irq_reg output;
> > +       struct gpio_irq_reg output_en;
> > +       struct gpio_irq_reg func_sel;
> > +       struct gpio_irq_reg gpio_status;
> > +       struct gpio_irq_reg irq_en;
> > +       struct gpio_irq_reg irq_status;
> > +       struct gpio_irq_reg irq_clr;
> > +       struct gpio_irq_reg irq_trigger;
> > +       struct gpio_irq_reg irq_level;
> > +       struct gpio_irq_reg irq_both_edge;
> > +       struct gpio_irq_reg irq_edge;
> > +};
> 
> If you want to keep a cache of all registers around, use regmap-mmio.

If I'm not mistaken, this is not about keeping a cache of all registers around. it's 
just a record of register offsets for the same functions across different pinctrl 
domains.

Best Regards,
Changhuang