Re: [PATCH 4/4] pinctrl: renesas: pinctrl-rzg2l: Add support to get/set drive-strength and output-impedance-ohms

[Lad, Prabhakar]

Hi Geert,

Thank you for the review.

On Thu, Oct 28, 2021 at 8:56 AM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Wed, Oct 27, 2021 at 3:45 PM Lad Prabhakar
> <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx> wrote:
> > Add support to get/set drive-strength and output-impedance-ohms
> > for the supported pins.
> >
> > While at it also renamed the below macros to match the HW manual,
> > PIN_CFG_IOLH_SD0 -> PIN_CFG_IO_VMC_SD0
> > PIN_CFG_IOLH_SD1 -> PIN_CFG_IO_VMC_SD1
> > PIN_CFG_IOLH_QSPI -> PIN_CFG_IO_VMC_QSPI
> > PIN_CFG_IOLH_ETH0 -> PIN_CFG_IO_VMC_ETH0
> > PIN_CFG_IOLH_ETH1 -> PIN_CFG_IO_VMC_ETH1
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > Reviewed-by: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
>
> Thanks for the update!
>
> > ---
> > RFC->v1
> >  * Renamed macros to match HW manual
>
> You may want to split that off into a separate patch, as not all lines
> changed are touched for other reasons.
Sure will do.

> BTW, where do I find these "VMC" names in the HW manual?
>
I have shortened VOLTAGE MODE CONTROL to VMC from ETH_ch0/1, SD_ch0/1
and QSPI registers.

> >  * Added PIN_CFG_IOLH_A/B macros to differentiate Group A/B
> >  * Added helper function to read/rmw pin config
> >  * Included RB tags
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
>
> > +static u32 rzg2l_read_pin_config(void __iomem *addr,
> > +                                u8 bit, u32 mask)
>
> The above fits on a single line.
>
> > +{
> > +       void __iomem *addr_adjust = addr;
> > +       u8 bit_adjust = bit;
>
> No need for these, just operate on addr and bit directly.
>
will do.

> > +       u32 reg;
> > +
> > +       if (bit >= 4) {
> > +               bit_adjust -= 4;
> > +               addr_adjust += 4;
> > +       }
> > +
> > +       reg = readl(addr_adjust) & (mask << (bit_adjust * 8));
> > +       return (reg >> (bit_adjust * 8));
> > +}
> > +
> > +static void rzg2l_rmw_pin_config(void __iomem *addr,
> > +                                u8 bit, u32 mask, u32 val)
> > +{
>
> The above fits on a single line.
>
will adjust that.

> > +       void __iomem *addr_adjust = addr;
> > +       u8 bit_adjust = bit;
>
> No need for these, just operate on addr and bit directly.
>
OK.

> > +       u32 reg;
> > +
> > +       if (bit >= 4) {
> > +               bit_adjust -= 4;
> > +               addr_adjust += 4;
> > +       }
> > +
> > +       reg = readl(addr_adjust) & ~(mask << (bit_adjust * 8));
> > +
> > +       writel(reg | val, addr_adjust);
>
> I think you should handle "val << (bit * 8)" here, instead of in
> all callers.
>
Agreed.

> > +}
>
> Please split the introduction of these helpers (and add conversion
> of the existing PIN_CONFIG_INPUT_ENABLE handling) off into a separate
> patch.
>
Agreed, will do.

> > @@ -484,6 +544,34 @@ static int rzg2l_pinctrl_pinconf_get(struct pinctrl_dev *pctldev,
> >                 break;
> >         }
> >
> > +       case PIN_CONFIG_DRIVE_STRENGTH: {
> > +               static const unsigned int mA[4] = { 2, 4, 8, 12 };
> > +
> > +               if (!(cfg & PIN_CFG_IOLH_A))
> > +                       return -EINVAL;
> > +
> > +               spin_lock_irqsave(&pctrl->lock, flags);
> > +               addr = pctrl->base + IOLH(port);
> > +               reg = rzg2l_read_pin_config(addr, bit, IOLH_MASK);
> > +               arg = mA[reg];
> > +               spin_unlock_irqrestore(&pctrl->lock, flags);
>
> Do you need the spinlock for reading?
>
Yes, to avoid any mishaps, I also looked at pinctrl.c which uses
spinlock while reading.

> > +               break;
> > +       }
> > +
> > +       case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS: {
> > +               static const unsigned int oi[4] = { 100, 66, 50, 33 };
> > +
> > +               if (!(cfg & PIN_CFG_IOLH_B))
> > +                       return -EINVAL;
> > +
> > +               spin_lock_irqsave(&pctrl->lock, flags);
> > +               addr = pctrl->base + IOLH(port);
> > +               reg = rzg2l_read_pin_config(addr, bit, IOLH_MASK);
> > +               arg = oi[reg];
> > +               spin_unlock_irqrestore(&pctrl->lock, flags);
>
> Likewise.
>
> > +               break;
> > +       }
> > +
> >         default:
> >                 return -ENOTSUPP;
> >         }
>
> > @@ -564,6 +659,49 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
> >                         spin_unlock_irqrestore(&pctrl->lock, flags);
> >                         break;
> >                 }
> > +
> > +               case PIN_CONFIG_DRIVE_STRENGTH: {
> > +                       unsigned int arg = pinconf_to_config_argument(_configs[i]);
> > +                       static const unsigned int mA[4] = { 2, 4, 8, 12 };
>
> Duplicate, move to file scope?
>
Ok will do.

> > +
> > +                       if (!(cfg & PIN_CFG_IOLH_A))
> > +                               return -EINVAL;
> > +
> > +                       for (i = 0; i < ARRAY_SIZE(mA); i++) {
> > +                               if (arg == mA[i])
> > +                                       break;
> > +                       }
> > +                       if (i >= ARRAY_SIZE(mA))
> > +                               return -EINVAL;
> > +
> > +                       spin_lock_irqsave(&pctrl->lock, flags);
> > +                       addr = pctrl->base + IOLH(port);
> > +                       rzg2l_rmw_pin_config(addr, bit, IOLH_MASK, (i << (bit * 8)));
>
> Pass pctrl and offset instead of addr (also for rzg2l_read_pin_config,
> for symmetry), and move locking into rzg2l_rmw_pin_config()?
> Taking all of the above into account, that would become:
>
Agreed.

>     rzg2l_rmw_pin_config(pctrl, IOLH(port), bit, IOLH_MASK, i);
>
That looks much cleaner.

> > +                       spin_unlock_irqrestore(&pctrl->lock, flags);
> > +                       break;
> > +               }
> > +
> > +               case PIN_CONFIG_OUTPUT_IMPEDANCE_OHMS: {
> > +                       unsigned int arg = pinconf_to_config_argument(_configs[i]);
> > +                       static const unsigned int oi[4] = { 100, 66, 50, 33 };
>
> Duplicate, move to file scope?
>
Agreed.

> > +
> > +                       if (!(cfg & PIN_CFG_IOLH_B))
> > +                               return -EINVAL;
> > +
> > +                       for (i = 0; i < ARRAY_SIZE(oi); i++) {
> > +                               if (arg == oi[i])
> > +                                       break;
> > +                       }
> > +                       if (i >= ARRAY_SIZE(oi))
> > +                               return -EINVAL;
> > +
> > +                       spin_lock_irqsave(&pctrl->lock, flags);
> > +                       addr = pctrl->base + IOLH(port);
> > +                       rzg2l_rmw_pin_config(addr, bit, IOLH_MASK, (i << (bit * 8)));
>
> Likewise.
>
OK.

> > +                       spin_unlock_irqrestore(&pctrl->lock, flags);
> > +                       break;
> > +               }
> > +
> >                 default:
> >                         return -EOPNOTSUPP;
> >                 }
>
> The rest looks good to me!
>
Thanks, I'll post a v2 just with patches 3 and 4 only. Hope that is OK with you.

Cheers,
Prabhakar

> Gr{oetje,eeting}s,
>
>                         Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds