Re: [PATCH v2 03/13] pinctrl: renesas: pinctrl-rzg2l: Allow more bits for pin configuration

["Lad, Prabhakar" <prabhakar.csengg@xxxxxxxxx>]

Hi Geert,

Thank you for the review.

On Wed, May 22, 2024 at 11:19 AM Geert Uytterhoeven
<geert@xxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> On Tue, Apr 23, 2024 at 7:59 PM Prabhakar <prabhakar.csengg@xxxxxxxxx> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> >
> > The pin configuration bits have been growing for every new SoCs being
> > added for the pinctrl-rzg2l driver which would mean updating the macros
> > every time for each new configuration. To avoid this allocate additional
> > bits for pin configuration by relocating the known fixed bits to the very
> > end of the configuration.
> >
> > Also update the size of 'cfg' to 'u64' to allow more configuration bits in
> > the 'struct rzg2l_variable_pin_cfg'.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > RFC->v2
> > - Merged the macros and rzg2l_variable_pin_cfg changes into single patch
> > - Updated types for the config changes
>
> Thanks for the update!
>
> > --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> > @@ -78,9 +78,9 @@
> >                                          PIN_CFG_FILNUM | \
> >                                          PIN_CFG_FILCLKSEL)
> >
> > -#define PIN_CFG_PIN_MAP_MASK           GENMASK_ULL(35, 28)
> > -#define PIN_CFG_PIN_REG_MASK           GENMASK(27, 20)
> > -#define PIN_CFG_MASK                   GENMASK(19, 0)
> > +#define PIN_CFG_PIN_MAP_MASK           GENMASK_ULL(62, 55)
> > +#define PIN_CFG_PIN_REG_MASK           GENMASK_ULL(54, 47)
> > +#define PIN_CFG_MASK                   GENMASK_ULL(46, 0)
> >
> >  /*
> >   * m indicates the bitmap of supported pins, a is the register index
>
> > @@ -241,9 +241,9 @@ struct rzg2l_dedicated_configs {
> >   * @pin: port pin
> >   */
> >  struct rzg2l_variable_pin_cfg {
> > -       u32 cfg:20;
> > -       u32 port:5;
> > -       u32 pin:3;
> > +       u64 cfg:46;
>
> 47, to match PIN_CFG_MASK()?
>
Oops, I missed that.

> > +       u64 port:5;
> > +       u64 pin:3;
> >  };
>
> To avoid such mistakes, and to increase uniformity, I think it would
> be good to get rid of this structure, and replace it by masks, to be
> used with FIELD_GET() and FIELD_PREP_CONST().
>
Agreed, I will make a patch on top of this patch (so that its easier
for review).

Cheers,
Prabhakar