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

[Linus Walleij]

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?

> +/* 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...

> +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.

> +int jhb100_pinctrl_probe(struct platform_device *pdev);
> +
> +void pinctrl_utils_free_map(struct pinctrl_dev *pctldev,
> +                           struct pinctrl_map *map, unsigned int num_maps);
> +int pinmux_generic_get_function_count(struct pinctrl_dev *pctldev);
> +const char *pinmux_generic_get_function_name(struct pinctrl_dev *pctldev,
> +                                            unsigned int selector);
> +int pinmux_generic_get_function_groups(struct pinctrl_dev *pctldev,
> +                                      unsigned int selector,
> +                                      const char * const **groups,
> +                                      unsigned int * const num_groups);
> +int pinmux_generic_add_function(struct pinctrl_dev *pctldev,
> +                               const char *name,
> +                               const char * const *groups,
> +                               unsigned int const num_groups,
> +                               void *data);
> +
> +#if defined(CONFIG_GENERIC_PINCONF) && defined(CONFIG_OF)
> +int pinconf_generic_parse_dt_config(struct device_node *np,
> +                                   struct pinctrl_dev *pctldev,
> +                                   unsigned long **configs,
> +                                   unsigned int *nconfigs);
> +#endif

Why is there a copy of the generic function headers here?
It looks wrong.

Yours,
Linus Walleij