Re: [PATCH 2/2] pinctrl: renesas: Add RZ/V2M pin and gpio controller driver

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Phil,

On Tue, Jun 14, 2022 at 5:00 PM Phil Edworthy <phil.edworthy@xxxxxxxxxxx> wrote:
> On 14 June 2022 13:43 Geert Uytterhoeven wrote:
> > On Fri, May 20, 2022 at 5:41 PM Phil Edworthy wrote:
> > > Add support for pin and gpio controller driver for RZ/V2M SoC.
> > > Based on the RZ/G2L driver.
> > >
> > > Note that the DETDO and DETMS dedicated pins are currently not
> > > documented in the HW manual as to which pin group they are in.
> > > HW team have since said that the output level of V1.8V I/O group 4
> > > (for MD0-7, and debugger) is the same as the 1.8V IO group 3.
> >
> > Thank you, I rediscovered this explanation just before pressing send ;-)
> >
> > >
> > > Signed-off-by: Phil Edworthy <phil.edworthy@xxxxxxxxxxx>
> > > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > > --- a/drivers/pinctrl/renesas/Kconfig
> > > +++ b/drivers/pinctrl/renesas/Kconfig
> > > @@ -193,6 +194,18 @@ config PINCTRL_RZG2L
> > >           This selects GPIO and pinctrl driver for Renesas
> > RZ/{G2L,G2UL,V2L}
> > >           platforms.
> > >
> > > +config PINCTRL_RZV2M
> > > +       bool "pin control support for RZ/V2M"
> > > +       depends on OF
> > > +       depends on ARCH_R9A09G011 || COMPILE_TEST
> > > +       select GPIOLIB
> > > +       select GENERIC_PINCTRL_GROUPS
> > > +       select GENERIC_PINMUX_FUNCTIONS
> > > +       select GENERIC_PINCONF
> > > +       help
> > > +         This selects GPIO and pinctrl driver for Renesas RZ/V2M
> > > +         platforms.
> > > +
> >
> > Please preserve sort order.
> For a while I couldn't see what's wrong here. It should be ordered on
> the text, not the Kconfig symbol, right?

Exactly, cfr. commit d89a08f52b0dd30d ("pinctrl: sh-pfc: Tidy up
driver description title").

> > > +       case PIN_CONFIG_DRIVE_STRENGTH_UA:
> > > +               if (!(cfg & PIN_CFG_DRV))
> > > +                       return -EINVAL;
> > > +
> > > +               /* DRV uses 2-bits per pin */
> > > +               bit <<= 1;
> > > +
> > > +               /* Dedicated port is irregularly located to the others
> > */
> > > +               if (port_offset == RZV2M_DEDICATED_PORT_IDX)
> > > +                       val = (readl(pctrl->base + DRV_DEDICATED) >>
> > bit) & DRV_MASK;
> > > +               else
> > > +                       val = (readl(pctrl->base + DRV(port_offset))
> > > + >> bit) & DRV_MASK;
> >
> > You can simplify this, by handling the dedicated port offset in the
> > definition of the DRV() macro.  Same for SR().
> Do you mean put conditional code in the DRV() macro?

Indeed. Cfr. the IMUCTR(n) in drivers/iommu/ipmmu-vmsa.c.

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