[PATCH v3 04/21] pinctrl: starfive: Add StarFive JHB100 sys0 controller driver
Changhuang Liang
changhuang.liang at starfivetech.com
Fri Jun 19 01:41:29 PDT 2026
Hi, Linus
Thanks for the review.
I will first try making revisions according to your suggestions below. It may take some time for
the next version. If I run into any issues during the subsequent revisions, I may bother you then.
Best Regards,
Changhuang
> Hi Changhuang,
>
> thanks for your patch!
>
> On Wed, Jun 3, 2026 at 7:54 AM Changhuang Liang
> <changhuang.liang at starfivetech.com> wrote:
>
> > Add pinctrl driver for StarFive JHB100 SoC System-0(sys0) pinctrl
> > controller.
> >
> > Co-developed-by: Lianfeng Ouyang <lianfeng.ouyang at starfivetech.com>
> > Signed-off-by: Lianfeng Ouyang <lianfeng.ouyang at starfivetech.com>
> > Signed-off-by: Changhuang Liang <changhuang.liang at starfivetech.com>
>
> 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 <yuklin.soo at starfivetech.com>
>
> 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...
>
> > +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.
>
> > +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;
>
> ???
>
> 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()).
>
> Yours,
> Linus Walleij
More information about the linux-riscv
mailing list