Re: [PATCH v5 3/5] pinctrl: renesas: rzg2l: Unify the power source handling

[Geert Uytterhoeven]

Hi Claudiu,

On Wed, 19 Aug 2026 at 16:36, Claudiu Beznea <claudiu.beznea@xxxxxxxxx> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> The previous code handled power sources using a mixture of power
> source specific definitions and lookups in the available_ps[] array.
> Unify the power source handling by introducing
> struct rzg2l_pinctrl_ps_desc, whose purpose is to describe a power
> source through its power source value, associated register value,
> associated capabilities (e.g. Ethernet), and associated IOLH index.
>
> Introduce two new functions, rzg2l_ps_to_desc() and
> rzg2l_pwr_reg_val_to_desc(), using the
> RZG2L_PINCTRL_PS_DESC_MEMBER_TO_DESC_FUNC() macro, as their
> implementations are similar.
>
> These functions retrieve a power source descriptor based on either a
> power source value or a power source register value. Other functions
> that need to perform power source specific operations can call them to
> retrieve the corresponding power source descriptor.
>
> 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

> @@ -440,7 +439,65 @@ struct rzg2l_pinctrl {
>         u32                             clone_offset;
>  };
>
> -static const u16 available_ps[] = { 1800, 2500, 3300 };
> +/**
> + * struct rzg2l_pinctrl_ps_desc - RZ/G2L power source descriptor
> + * @caps: Capabilities that applies to the power source
> + * @iolh_index: IOLH index
> + * @ps: Power source value
> + * @pwr_reg_val: Power source register value
> + */
> +struct rzg2l_pinctrl_ps_desc {
> +       u32 caps;
> +       enum rzg2l_iolh_index iolh_index;
> +       u16 ps;
> +       u16 pwr_reg_val;
> +};
> +
> +#define RZG2L_PINCTRL_PS_DESC(_ps, _pwr_reg_val, _caps, _iolh_index) \
> +       { \
> +               .ps = _ps, \
> +               .pwr_reg_val = _pwr_reg_val, \
> +               .caps = _caps, \
> +               .iolh_index = _iolh_index, \
> +       }
> +
> +/* Keep the entries with .caps set in the first positions. */
> +static const struct rzg2l_pinctrl_ps_desc available_ps[] = {
> +       /* Ethernet I/O domain voltage 2.5V */
> +       RZG2L_PINCTRL_PS_DESC(2500, 2, PIN_CFG_IO_VMC_ETH0 | PIN_CFG_IO_VMC_ETH1,
> +                             RZG2L_IOLH_IDX_2V5),
> +       /* I/O domain voltage 1.8V */
> +       RZG2L_PINCTRL_PS_DESC(1800, 1, 0, RZG2L_IOLH_IDX_1V8),
> +       /* I/O domain voltage 3.3V */
> +       RZG2L_PINCTRL_PS_DESC(3300, 0, 0, RZG2L_IOLH_IDX_3V3),
> +};
> +
> +#define RZG2L_PINCTRL_PS_DESC_MEMBER_TO_DESC_FUNC(_name, _desc_member, _caps)  \
> +static const struct rzg2l_pinctrl_ps_desc *_name(u16 _desc_member, u32 _caps)  \
> +{                                                                              \
> +       const struct rzg2l_pinctrl_ps_desc *desc = NULL;                        \
> +                                                                               \
> +       for (unsigned int i = 0; i < ARRAY_SIZE(available_ps); i++) {           \
> +               if (available_ps[i]._desc_member == _desc_member) {             \
> +                       if (available_ps[i].caps) {                             \
> +                               if (available_ps[i].caps & caps) {              \

"_caps", as pointed out by Sashiko.

> +                                       desc = &available_ps[i];                \
> +                                       break;                                  \
> +                               } else {                                        \
> +                                       continue;                               \
> +                               }                                               \
> +                       } else {                                                \
> +                               desc = &available_ps[i];                        \
> +                               break;                                          \
> +                       }                                                       \
> +               }                                                               \
> +       }                                                                       \

You can reduce indentation, and simplify the code, by restructuring
the checks:

    for (unsigned int i = 0; i < ARRAY_SIZE(available_ps); i++) {
            if (!available_ps[i]._desc_member == _desc_member)
                    continue;

            if (!available_ps[i].caps) {
                    desc = &available_ps[i];
                    break;
            }

            if (available_ps[i].caps & caps) {
                    desc = &available_ps[i];
                    break;
            }
    }

> +                                                                               \
> +       return desc;                                                            \
> +}
> +
> +RZG2L_PINCTRL_PS_DESC_MEMBER_TO_DESC_FUNC(rzg2l_ps_to_desc, ps, caps)
> +RZG2L_PINCTRL_PS_DESC_MEMBER_TO_DESC_FUNC(rzg2l_pwr_reg_val_to_desc, pwr_reg_val, caps)
>
>  static u64 rzg2l_pinctrl_get_variable_pin_cfg(struct rzg2l_pinctrl *pctrl,
>                                               u64 pincfg,

> @@ -1191,32 +1240,21 @@ static int rzg2l_set_power_source(struct rzg2l_pinctrl *pctrl, u32 pin, u32 caps
>         return 0;
>  }
>
> -static bool rzg2l_ps_is_supported(u16 ps)
> +static bool rzg2l_ps_is_supported(u16 ps, u32 caps)
>  {
> -       unsigned int i;
> +       const struct rzg2l_pinctrl_ps_desc *desc = rzg2l_ps_to_desc(ps, caps);
>
> -       for (i = 0; i < ARRAY_SIZE(available_ps); i++) {
> -               if (available_ps[i] == ps)
> -                       return true;
> -       }
> -
> -       return false;
> +       return !!desc;

No need for the "!!".
Then this function becomes very simple, and you can just call
rzg2l_ps_to_desc(ps, caps) in the callers instead.

>  }
>
> -static enum rzg2l_iolh_index rzg2l_ps_to_iolh_idx(u16 ps)
> +static enum rzg2l_iolh_index rzg2l_ps_to_iolh_idx(u16 ps, u32 caps)
>  {
> -       unsigned int i;
> +       const struct rzg2l_pinctrl_ps_desc *desc = rzg2l_ps_to_desc(ps, caps);
>
> -       for (i = 0; i < ARRAY_SIZE(available_ps); i++) {
> -               if (available_ps[i] == ps)
> -                       break;
> -       }
> +       if (!desc)
> +               return RZG2L_IOLH_IDX_NA;
>
> -       /*
> -        * We multiply with RZG2L_IOLH_MAX_DS_ENTRIES as we have
> -        * RZG2L_IOLH_MAX_DS_ENTRIES DS values per power source
> -        */
> -       return i * RZG2L_IOLH_MAX_DS_ENTRIES;
> +       return desc->iolh_index;

Perhaps

    return desc ? desc->iolh_index : RZG2L_IOLH_IDX_NA;

?

>  }
>
>  static u16 rzg2l_iolh_val_to_ua(const struct rzg2l_hwcfg *hwcfg, u32 caps, u8 val)

> @@ -1797,7 +1839,7 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
>
>         /* Apply power source. */
>         if (settings.power_source != pctrl->settings[_pin].power_source) {
> -               ret = rzg2l_ps_is_supported(settings.power_source);
> +               ret = rzg2l_ps_is_supported(settings.power_source, cfg);
>                 if (!ret)
>                         return -EINVAL;

As rzg2l_ps_is_supported() returns a bool, there is no need to keep its
return value in "ret" for later use.

After eliminating rzg2l_ps_is_supported(), this can just become

    if (!rzg2l_ps_to_desc(settings.power_source, cfg)
            return -EINVAL;

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