[PATCH v3 2/3] pinctrl: bcm: Add STB family pin controller driver

[Linus Walleij]

Hi Andrea/Ivan,

thanks for your patch!

I'll make a bit of detailed review below, the big question I have
is if it is possible to split the files a bit, like:

pinctrl-brcmstb.c  <- STB core
pinctrl-brcmstb.h  <- STB API
pinctrl-brcmstb-bcm2717.c <- All BCM2712 specifics

This would make it easier to reuse the base file with other STB
chips, right?

On Mon, Aug 11, 2025 at 4:45 PM Andrea della Porta
<andrea.porta at suse.com> wrote:

> +#define FUNC(f) \
> +       [func_##f] = #f
> +
> +#define PIN(i, f1, f2, f3, f4, f5, f6, f7, f8) \
> +       [i] = { \
> +               .funcs = { \
> +                       func_##f1, \
> +                       func_##f2, \
> +                       func_##f3, \
> +                       func_##f4, \
> +                       func_##f5, \
> +                       func_##f6, \
> +                       func_##f7, \
> +                       func_##f8, \
> +               }, \
> +       }

These macros have a bit too generic names. Prefix with BRCMSTB_* or
something please.

> +#define MUX_BIT_VALID          0x8000
> +#define PAD_BIT_INVALID                0xffff
> +
> +#define BIT_TO_REG(b)          (((b) >> 5) << 2)
> +#define BIT_TO_SHIFT(b)                ((b) & 0x1f)
> +
> +#define MUX_BIT(muxreg, muxshift) \
> +       (MUX_BIT_VALID + ((muxreg) << 5) + ((muxshift) << 2))
> +#define PAD_BIT(padreg, padshift) \
> +       (((padreg) << 5) + ((padshift) << 1))
> +
> +#define GPIO_REGS(n, muxreg, muxshift, padreg, padshift) \
> +       [n] = { MUX_BIT(muxreg, muxshift), PAD_BIT(padreg, padshift) }
> +
> +#define EMMC_REGS(n, padreg, padshift) \
> +       [n] = { 0, PAD_BIT(padreg, padshift) }
> +
> +#define AGPIO_REGS(n, muxreg, muxshift, padreg, padshift) \
> +       GPIO_REGS(n, muxreg, muxshift, padreg, padshift)
> +
> +#define SGPIO_REGS(n, muxreg, muxshift) \
> +       [(n) + 32] = { MUX_BIT(muxreg, muxshift), PAD_BIT_INVALID }
> +
> +#define GPIO_PIN(n)            PINCTRL_PIN(n, "gpio" #n)
> +#define AGPIO_PIN(n)           PINCTRL_PIN(n, "aon_gpio" #n)
> +#define SGPIO_PIN(n)           PINCTRL_PIN((n) + 32, "aon_sgpio" #n)

These are also pretty generically named, but this is OK because they
don't intrude on the pinctrl namespace as much.

> +static inline u32 brcmstb_reg_rd(struct brcmstb_pinctrl *pc, unsigned int reg)
> +{
> +       return readl(pc->base + reg);
> +}
> +
> +static inline void brcmstb_reg_wr(struct brcmstb_pinctrl *pc, unsigned int reg,
> +                                 u32 val)
> +{
> +       writel(val, pc->base + reg);
> +}

This looks like unnecessary indirection. Can't you just use readl/writel?

> +static int brcmstb_pinctrl_fsel_set(struct brcmstb_pinctrl *pc,
> +                                   unsigned int pin, enum brcmstb_funcs func)
> +{
> +       u32 bit = pc->pin_regs[pin].mux_bit, val;
> +       const u8 *pin_funcs;
> +       unsigned long flags;
> +       int fsel;
> +       int cur;
> +       int i;
> +
> +       if (!bit || func >= func_count)
> +               return -EINVAL;
> +
> +       bit &= ~MUX_BIT_VALID;
> +
> +       fsel = BRCMSTB_FSEL_COUNT;
> +
> +       if (func >= BRCMSTB_FSEL_COUNT) {
> +               /* Convert to an fsel number */
> +               pin_funcs = pc->pin_funcs[pin].funcs;
> +               for (i = 1; i < BRCMSTB_FSEL_COUNT; i++) {
> +                       if (pin_funcs[i - 1] == func) {
> +                               fsel = i;
> +                               break;
> +                       }
> +               }
> +       } else {
> +               fsel = (enum brcmstb_funcs)func;
> +       }
> +
> +       if (fsel >= BRCMSTB_FSEL_COUNT)
> +               return -EINVAL;
> +
> +       spin_lock_irqsave(&pc->fsel_lock, flags);

Please use lock guards instead, we do that in all new code:

#include <linux/cleanup.h>

guard(spinlock_irqsave)(&pc->fsel_lock);

The framework handles the flags variable and the freeing,
look at other drivers using guard() for guidance.

> +static int brcmstb_pmx_gpio_request_enable(struct pinctrl_dev *pctldev,
> +                                          struct pinctrl_gpio_range *range,
> +                                          unsigned int pin)
> +{
> +       struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +
> +       return brcmstb_pinctrl_fsel_set(pc, pin, func_gpio);
> +}
> +
> +static void brcmstb_pmx_gpio_disable_free(struct pinctrl_dev *pctldev,
> +                                         struct pinctrl_gpio_range *range,
> +                                         unsigned int offset)
> +{
> +       struct brcmstb_pinctrl *pc = pinctrl_dev_get_drvdata(pctldev);
> +
> +       /* disable by setting to GPIO */
> +       (void)brcmstb_pinctrl_fsel_set(pc, offset, func_gpio);
> +}
> +
> +static const struct pinmux_ops brcmstb_pmx_ops = {
> +       .free = brcmstb_pmx_free,
> +       .get_functions_count = brcmstb_pmx_get_functions_count,
> +       .get_function_name = brcmstb_pmx_get_function_name,
> +       .get_function_groups = brcmstb_pmx_get_function_groups,
> +       .set_mux = brcmstb_pmx_set,
> +       .gpio_request_enable = brcmstb_pmx_gpio_request_enable,
> +       .gpio_disable_free = brcmstb_pmx_gpio_disable_free,
> +};

With regards to the GPIO "shotcut" functions:
please familiarize yourself with Bartosz recent patch set:
https://lore.kernel.org/linux-gpio/20250815-pinctrl-gpio-pinfuncs-v5-0-955de9fd91db@linaro.org/T/#t

This makes it possible for the pinctrl core to know about
functions that are used for GPIO, so you can mark your
pin controller as "strict". using the new .function_is_gpio()
callback.

I plan to merge Bartosz series soon and if your pin controller
is aware about which functions are GPIO functions, this makes
things better.

> +static int brcmstb_pull_config_set(struct brcmstb_pinctrl *pc,
> +                                  unsigned int pin, unsigned int arg)
> +{
> +       u32 bit = pc->pin_regs[pin].pad_bit, val;
> +       unsigned long flags;
> +
> +       if (bit == PAD_BIT_INVALID) {
> +               dev_warn(pc->dev, "Can't set pulls for %s\n",
> +                        pc->gpio_groups[pin]);
> +               return -EINVAL;
> +       }
> +
> +       spin_lock_irqsave(&pc->fsel_lock, flags);

Use a guard()

Yours,
Linus Walleij