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