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

From: Linus Walleij
Date: Wed Oct 07 2020 - 09:30:23 EST


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