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

[Linus Walleij]

Hi Lars!

Thanks for the update, this looks much improved!

Some further hints at things I saw here:

On Tue, Oct 6, 2020 at 4:25 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>

> +       /* 2 banks: IN/OUT */
> +       struct {
> +               struct gpio_chip gpio;
> +               struct pinctrl_desc pctl_desc;
> +               struct pinctrl_dev *pctl_dev;
> +       } bank[2];

Is it really good to use index [0,1] to refer to these?
Isnt it easier to do something like:

struct sgpio_bank {
         struct gpio_chip gpio;
         struct pinctrl_desc pctl_desc;
         struct pinctrl_dev *pctl_dev;
};

struct sgpio_priv {
        (...)
        struct sgpio_bank in;
        struct sgpio_bank out;
        (...)
};

This way you I think the code becomes clearer.

> +static inline bool sgpio_pctl_is_input(struct sgpio_priv *priv,
> +                                      struct pinctrl_dev *pctl)
> +{
> +       /* 1st index is input */
> +       return pctl == priv->bank[0].pctl_dev;
> +}
> +
> +static inline bool sgpio_gpio_is_input(struct sgpio_priv *priv,
> +                                      struct gpio_chip *gc)
> +{
> +       /* 1st index is input */
> +       return gc == &priv->bank[0].gpio;
> +}

Isn't it easier to just add a

bool is_input;

to the struct gpio_bank?

> +static inline u32 sgpio_readl(struct sgpio_priv *priv, u32 rno, u32 off)
> +{
> +       u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off];
> +
> +       return readl(reg);
> +}
> +
> +static inline void sgpio_writel(struct sgpio_priv *priv,
> +                               u32 val, u32 rno, u32 off)
> +{
> +       u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off];
> +
> +       writel(val, reg);
> +}
> +
> +static inline void sgpio_clrsetbits(struct sgpio_priv *priv,
> +                                   u32 rno, u32 off, u32 clear, u32 set)
> +{
> +       u32 __iomem *reg = &priv->regs[priv->properties->regoff[rno] + off];
> +       u32 val = readl(reg);
> +
> +       val &= ~clear;
> +       val |= set;
> +
> +       writel(val, reg);
> +}

These accessors are somewhat re-implementing regmap-mmio, especially
the clrsetbits. I would consider just creating a regmap for each
struct sgpio_bank and manipulate the register through that.
(Not any requirement just a suggestion.)

> +static void sgpio_output_set(struct sgpio_priv *priv,
> +                            struct sgpio_port_addr *addr,
> +                            int value)
> +{
> +       u32 mask = 3 << (3 * addr->bit);
> +       value = (value & 3) << (3 * addr->bit);

I would spell it out a bit so it becomes clear what is going in
and use the bits helpers.

value & 3 doesn't make much sense since value here is always
0 or 1. Thus if value is 1 it in effect becomes value = 1 << (3 * addr->bit)
so with the above helper bit:

unsigned int bit = 3 * addr->bit;
u32 mask = GENMASK(bit+1, bit);
u32 val = BIT(bit);

This way it becomes clear that you set the output usin the lower
of the two bits.

Also note that I use val rather than value to avoid overwriting
the parameter: it is legal but confusing. Let the compiler optimize
register use.

> +static int sgpio_output_get(struct sgpio_priv *priv,
> +                           struct sgpio_port_addr *addr)
> +{
> +       u32 portval = sgpio_readl(priv, REG_PORT_CONFIG, addr->port);
> +       int ret;
> +
> +       ret = SGPIO_X_PORT_CFG_BIT_SOURCE(priv, portval);
> +       ret = !!(ret & (3 << (3 * addr->bit)));

Is the output direction really controlled by both bits?

ret = !!(ret & (BIT(3 * addr->bit))); ?

> +static int microchip_sgpio_get_direction(struct gpio_chip *gc, unsigned int gpio)
> +{
> +       struct sgpio_priv *priv = gpiochip_get_data(gc);
> +
> +       return sgpio_gpio_is_input(priv, gc); /* 0=out, 1=in */

You can use the #defines from <linux/gpio/driver.h> if you want to be explicit:

return sgpio_gpio_is_input(priv, gc) ? GPIO_LINE_DIRECTION_IN :
GPIO_LINE_DIRECTION_OUT;

> +static int microchip_sgpio_of_xlate(struct gpio_chip *gc,
> +                              const struct of_phandle_args *gpiospec,
> +                              u32 *flags)
> +{
> +       struct sgpio_priv *priv = gpiochip_get_data(gc);
> +       int pin;
> +
> +       if (gpiospec->args[0] > SGPIO_BITS_PER_WORD ||
> +           gpiospec->args[1] > priv->bitcount)
> +               return -EINVAL;
> +
> +       pin = gpiospec->args[1] + (gpiospec->args[0] * priv->bitcount);
> +
> +       if (pin > gc->ngpio)
> +               return -EINVAL;
> +
> +       if (flags)
> +               *flags = gpiospec->args[2];
> +
> +       return pin;
> +}

I'm still not convinced that you need the flags in args[2].

Just using the default of_gpio_simple_xlate() with one flag
should be fine. But I will go and review the bindings to figure
this out.

> +       gc->of_xlate            = microchip_sgpio_of_xlate;
> +       gc->of_gpio_n_cells     = 3;

So I'm sceptical to this.

Why can't you just use the pin index in cell 0 directly
and avoid cell 1?

Yours,
Linus Walleij