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
[off-list ref] 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 (local)

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