Re: [PATCH v4 1/5] pinctrl: renesas: rzg2l: Generalize the power source code

[Geert Uytterhoeven]

Hi Claudiu,

On Fri, 10 Jul 2026 at 13:36, Claudiu Beznea
<claudiu.beznea+renesas@xxxxxxxxx> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> The current functions used to get/set the pin power source check the
> OTHER_POC register, which is specific to the RZ/G3L SoC only. To allow the
> code to be extended for other power source functionalities (e.g. I3C on
> RZ/G3S), generalize the functions used to get/set the pin power source.
>
> For this, introduce the struct rzg2l_register_masks data structure whose
> purpose is to store SoC specific register bit masks. The members of this
> structure are then used in rzg2l_caps_to_pwr_reg() to retrieve the bitmask
> corresponding to a SoC specific power source capability.
>
> The conversion between HW specific power source values and SW specific
> power source values is now handled through rzg2l_pwr_reg_val_to_ps() and
> rzg2l_ps_to_pwr_reg_val().
>
> Finally, to keep the code generic, the register update in
> rzg2l_set_power_source() was changed to a read-modify-write approach to
> cover all cases.
>
> Tested-by: Wolfram Sang <wsa+renesas@xxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>

Thanks for your patch!

> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c

>  static int rzg2l_caps_to_pwr_reg(const struct rzg2l_register_offsets *regs,
> -                                u32 caps, u8 *mask)
> +                                const struct rzg2l_register_masks *masks,
> +                                u32 caps, u16 *offset, u8 *mask)

Yes, always returning an offset and mask is what I had in mind before.

>  {
> -       if (caps & PIN_CFG_IO_VMC_SD0)
> -               return SD_CH(regs->sd_ch, 0);
> -       if (caps & PIN_CFG_IO_VMC_SD1)
> -               return SD_CH(regs->sd_ch, 1);
> -       if (caps & PIN_CFG_IO_VMC_ETH0)
> -               return ETH_POC(regs->eth_poc, 0);
> -       if (caps & PIN_CFG_IO_VMC_ETH1)
> -               return ETH_POC(regs->eth_poc, 1);
> -       if (caps & PIN_CFG_IO_VMC_QSPI)
> -               return QSPI;
> +       *mask = PVDD_MASK;
> +
> +       if (caps & PIN_CFG_IO_VMC_SD0) {
> +               *offset = SD_CH(regs->sd_ch, 0);
> +               return 0;
> +       }
> +       if (caps & PIN_CFG_IO_VMC_SD1) {
> +               *offset = SD_CH(regs->sd_ch, 1);
> +               return 0;
> +       }
> +       if (caps & PIN_CFG_IO_VMC_ETH0) {
> +               *offset = ETH_POC(regs->eth_poc, 0);
> +               return 0;
> +       }
> +       if (caps & PIN_CFG_IO_VMC_ETH1) {
> +               *offset = ETH_POC(regs->eth_poc, 1);
> +               return 0;
> +       }
> +       if (caps & PIN_CFG_IO_VMC_QSPI) {
> +               *offset = regs->qspi;
> +               return 0;
> +       }
>         if (caps & PIN_CFG_OTHER_POC_MASK) {
> +               *offset = regs->other_poc;
>                 if (caps & PIN_CFG_PVDD1833_OTH_AWO_POC)
> -                       *mask = BIT(0);
> +                       *mask = masks->other_poc_pvdd1833_oth_awo_poc;
>                 else if (caps & PIN_CFG_PVDD1833_OTH_ISO_POC)
> -                       *mask = BIT(1);
> +                       *mask = masks->other_poc_pvdd1833_oth_iso_poc;
>                 else
> -                       *mask = BIT(2);
> +                       *mask = masks->other_poc_wdtovf_n_poc;
> +               return 0;
> +       }
>
> -               return OTHER_POC;
> +       return -EINVAL;
> +}
> +
> +static int rzg2l_pwr_reg_val_to_ps(u8 val, u32 caps)
> +{
> +       switch (val) {
> +       case PVDD_1800:
> +               return 1800;
> +       case PVDD_2500:
> +               return 2500;
> +       case PVDD_3300:
> +               return 3300;
> +       }
> +
> +       return -EINVAL;
> +}

This function returns a valid value, or an error code...

> +
> +static int rzg2l_ps_to_pwr_reg_val(u8 *val, u32 ps, u32 caps)
> +{
> +       switch (ps) {
> +       case 1800:
> +               *val = PVDD_1800;
> +               return 0;
> +       case 2500:
> +               if (!(caps & (PIN_CFG_IO_VMC_ETH0 | PIN_CFG_IO_VMC_ETH1)))
> +                       return -EINVAL;
> +               *val = PVDD_2500;
> +               return 0;
> +       case 3300:
> +               *val = PVDD_3300;
> +               return 0;
>         }
>
>         return -EINVAL;

... while the inverse function stores the value in an output pointer
on succes. Perhaps this should be changed, for consistency?

Reviewed-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>

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