Re: [PATCH v2 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO

[Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>]

Linus Walleij writes:

> On Thu, Sep 3, 2020 at 3:35 PM Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx> wrote:
>
>> This adds a pinctrl driver for the Microsemi/Microchip Serial GPIO
>> (SGPIO) device used in various SoC's.
>>
>> Signed-off-by: Lars Povlsen <lars.povlsen@xxxxxxxxxxxxx>
> (...)
>
>> diff --git a/drivers/pinctrl/pinctrl-mchp-sgpio.c b/drivers/pinctrl/pinctrl-mchp-sgpio.c
>
> Can we just spell it out
> pinctrl-microchip-sgpio.c ?
>
> The abbreviation seems arbitrary and unnecessary.

Well, not completely arbitrary, but maybe unnecessary... I'll change
that. I'll also change that for any symbols/defines off course.

>
> I do see that this chip is using the pinctrl abstractions (very nicely)
> and should be under drivers/pinctrl/*.
>
> Sadly it doesn't mean the bindings need to be in pinctrl ... unless you
> plan to add pinctrl bindings later.
>
>> +config PINCTRL_MCHP_SGPIO
>> +       bool "Pinctrl driver for Microsemi/Microchip Serial GPIO"
>> +       depends on OF
>> +       depends on HAS_IOMEM
>> +       select GPIOLIB
>> +       select GENERIC_PINCONF
>> +       select GENERIC_PINCTRL_GROUPS
>> +       select GENERIC_PINMUX_FUNCTIONS
>
> Nice use of these abstractions!

Thanks!

>
>> +// SPDX-License-Identifier: (GPL-2.0 OR MIT)
>
> What's up with this OR MIT? I'd like Kate to OK this.
>
>> +#define MCHP_SGPIOS_PER_BANK   32
>> +#define MCHP_SGPIO_BANK_DEPTH  4
>> +
>> +#define PIN_NAM_SZ     (sizeof("SGPIO_D_pXXbY")+1)
>> +
>> +enum {
>> +       REG_INPUT_DATA,
>> +       REG_PORT_CONFIG,
>> +       REG_PORT_ENABLE,
>> +       REG_SIO_CONFIG,
>> +       REG_SIO_CLOCK,
>> +       MAXREG
>> +};
>> +
>> +struct mchp_sgpio_props {
>
> Just call it struct microchip_gpio_variant it is easier to read and
> does not abbreviate randomly, also it is a per-variant set of properties
> so calling it variant is more to the point.
>

Fine.

>> +struct mchp_sgpio_priv {
>
> I would just spell it out struct microchip_sgpio, it is implicit that
> the struct is private since it is defined in a c file.
>

Fine.

>> +struct mchp_sgpio_port_addr {
>
> struct microchip_sgpio_port_addr
>
> (Admittedly this is a bit about taste.)
>
>> +static inline void sgpio_writel(struct mchp_sgpio_priv *priv,
>> +                               u32 val, u32 rno, u32 off)
>> +{
>> +       u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off];
>> +
>> +       writel(val, reg);
>> +}
>> +
>> +static inline void sgpio_clrsetbits(struct mchp_sgpio_priv *priv,
>> +                                   u32 rno, u32 off, u32 clear, u32 set)
>> +{
>> +       u32 __iomem *reg = &priv->regs[priv->props->regoff[rno] + off];
>> +       u32 val = readl(reg);
>> +
>> +       val &= ~clear;
>> +       val |= set;
>> +
>> +       writel(val, reg);
>> +}
>
> This looks like a reimplementation of regmap_update_bits for a bit,
> have you considered just using regmap? It's pretty simple.
>

Well, the registers are not in a regmap, so I did not consider that. The
utility function also serves to abstract the variant register address.

>> +static int mchp_sgpio_direction_input(struct gpio_chip *gc, unsigned int gpio)
>> +{
>> +       struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
>> +
>> +       /* Fixed-position function */
>> +       return sgpio_is_input(priv, gpio) ? 0 : -EINVAL;
>> +}
>> +
>> +static int mchp_sgpio_direction_output(struct gpio_chip *gc,
>> +                                      unsigned int gpio, int value)
>> +{
>> +       struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
>> +       struct mchp_sgpio_port_addr addr;
>> +
>> +       sgpio_pin_to_addr(priv, gpio, &addr);
>> +
>> +       /* Fixed-position function */
>> +       if (addr.input)
>> +               return -EINVAL;
>> +
>> +       sgpio_output_set(priv, &addr, value);
>> +
>> +       return 0;
>> +}
>
> This looks like the right way to handle this!

I'm glad you think so...

>
>> +static int mchp_sgpio_of_xlate(struct gpio_chip *gc,
>> +                              const struct of_phandle_args *gpiospec,
>> +                              u32 *flags)
>> +{
>> +       struct mchp_sgpio_priv *priv = gpiochip_get_data(gc);
>> +       int pin, base;
>> +
>> +       if (gpiospec->args[0] > MCHP_SGPIOS_PER_BANK ||
>> +           gpiospec->args[1] > priv->bitcount)
>> +               return -EINVAL;
>> +       base = priv->bitcount * gpiospec->args[0];
>> +       pin = base + gpiospec->args[1];
>> +       /* Add to 2nd half of output range if output */
>> +       if (gpiospec->args[2] == PIN_OUTPUT)
>> +               pin += (priv->ngpios / 2);
>> +
>> +       if (pin > gc->ngpio)
>> +               return -EINVAL;
>> +
>> +       if (flags)
>> +               *flags = gpiospec->args[3];
>> +
>> +       return pin;
>> +}
>
> I don't like this. I would certainly prefer the driver to just use standard
> GPIO bindings. I do not understand why this is necessary.
>
> If for nothing else, there should be a big comment explaining this.
>
> The only real problem I have with the driver is this extra flag tagged onto
> all the GPIOs, this seems unnecessary, and something the hardware
> driver should already know from the compatible string.

I hope my previous comments have cleared this up.

>
> Yours,
> Linus Walleij

Thank you for your time and comments!

---Lars

-- 
Lars Povlsen,
Microchip