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

[Andrea della Porta <andrea.porta@suse.com>]

Hi Linus,

On 11:37 Tue 19 Aug     , Linus Walleij wrote:
> 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

I'm trying to find a suitable way to do that. The difficulty is
that AFAIK this is the only chipset using this driver so I'm
not sure what parts are generic and what are specific to BCM2712.
I'll do my best.

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

Sure, provided we have a clear separation of common vs specific,
like stated above.

> 
> On Mon, Aug 11, 2025 at 4:45 PM Andrea della Porta
> <andrea.porta@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.

Ack.

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

If no one else has something against that, I'll leaving those as they are,
then. 

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

Sure, moreover there's just a small bunch of invokation for those functions...

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

Ack.

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

Sure, very nice!

> 
> > +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()

Ack.

Many thanks,
Andrea

> 
> Yours,
> Linus Walleij