Message-ID: <87ft6px9wc.fsf@soft-dev15.microsemi.net>
Date:   Thu, 8 Oct 2020 12:57:39 +0200
From:   Lars Povlsen <lars.povlsen@...rochip.com>
To:     Linus Walleij <linus.walleij@...aro.org>
CC:     Lars Povlsen <lars.povlsen@...rochip.com>,
        Microchip Linux Driver Support <UNGLinuxDriver@...rochip.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Alexandre Belloni <alexandre.belloni@...tlin.com>
Subject: Re: [RESEND PATCH v3 2/3] pinctrl: pinctrl-mchp-sgpio: Add pinctrl driver for Microsemi Serial GPIO


Linus Walleij writes:

> Hi Lars!
>
> Thanks for the update, this looks much improved!

Glad you like it! It's been a difficult birth...

>
> Some further hints at things I saw here:
>
> On Tue, Oct 6, 2020 at 4:25 PM Lars Povlsen <lars.povlsen@...rochip.com> 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@...rochip.com>
>
>> +       /* 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.
>

I have done the change as suggested, and I think your right - looks
better. Also helped loose the "is_input" helper functions.

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

Yes, did that.

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

Humm, not sure if these few functions warrants using regmap. I'll have a
closer look.

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

I will change as suggested.

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

The 3 bits are actually "mode" not direction. The direction is fixed as
described earlier.

0: Forced 0
1: Forced 1
2: Blink mode 0
3: Blink mode 1
4: Link activity blink mode 0
5: Link activity blink mode 1

But you are still right the (forced) _value_ can still be read using
just the first bit. I will change to using just the first 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;
>

Yes, good suggestion. Also using bank->is_input now.

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

Ok, I just assumed the std GPIO flags in args[2] were kind of mandatory,
I mean polarity would be needed?

F.ex. a GPIO (led) looks like this now:

    led@0 {
            label = "eth60:yellow";
            gpios = <&sgpio_out1 28 1 GPIO_ACTIVE_LOW>;
            default-state = "off";
    };

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

You scepticism has surfaced before :-). The (now) 2 indices relates to
how the hardware address signals.

Each signal/pin is addressed by port, bit number and direction. We now
have the direction encoded in the bank/phandle.

While it would be possible to just produce a linear range of (32 *
width), hardware designs and documentation use pXXbY (as "p28b1" above),
making the cross reference much better using the 2 indices when
specifying a pin (as opposed to using f.ex. value "60" in the example).

Hope this helps.

Thank you for your comments, I will refresh later today.

---Lars

> Yours,
> Linus Walleij

-- 
Lars Povlsen,
Microchip

Powered by blists - more mailing lists