[PATCH v3 04/21] pinctrl: starfive: Add StarFive JHB100 sys0 controller driver
Linus Walleij
linusw at kernel.org
Thu Jun 11 04:37:01 PDT 2026
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