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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Regression preventing software-configured (`PIN_CFG_SOFT_PS`) pins from using a 2.5V power source.
- [Medium] Wildcard capability matching in `RZG2L_PINCTRL_PS_DESC_MEMBER_TO_DESC_FUNC` can cause silent hardware misconfiguration.
- [Low] Macro hygiene violation in `RZG2L_PINCTRL_PS_DESC_MEMBER_TO_DESC_FUNC` forces callers to use a specific identifier.

Pre-existing issues:
- [High] Drive strength hardware register (`IOLH`) is not updated when the pin's power source changes.
--

--- Patch [3]: [PATCH v5 3/5] pinctrl: renesas: rzg2l: Unify the power source handling ---
commit 359b67b0750f9cab682964830c50121ec1ab0ad9
Author: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
Subject: pinctrl: renesas: rzg2l: Unify the power source handling

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.

> diff --git a/drivers/pinctrl/renesas/pinctrl-rzg2l.c b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> index 9c935dc475557..bab7bd73a4328 100644
> --- a/drivers/pinctrl/renesas/pinctrl-rzg2l.c
> +++ b/drivers/pinctrl/renesas/pinctrl-rzg2l.c
[ ... ]
> @@ -440,7 +439,65 @@ struct rzg2l_pinctrl {
>  	u32				clone_offset;
>  };
>  
[ ... ]
> +#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) {		\

[Severity: Low]
This isn't a bug, but does this use the wrong variable? The macro takes
_caps as a parameter, but caps is used here. Will this cause compilation
failures if callers use a different variable name?

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

[Severity: Medium]
Does this else block act as a wildcard for capabilities equal to 0? If a
caller requests an unsupported voltage for a restricted domain (like an
anticipated I3C domain that does not support 3.3V), will the lookup
falsely match the generic descriptor and return its register value
(e.g., 0)? Could this silently misconfigure the hardware to an incorrect
voltage instead of safely returning -EINVAL?

> +			}							\
> +		}								\
> +	}									\
> +										\
> +	return desc;								\
> +}
[ ... ]
> @@ -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;

[Severity: High]
Does this check inadvertently break pins configured via software
(PIN_CFG_SOFT_PS) that require a 2.5V power source?

The only available 2.5V descriptor explicitly requires Ethernet capabilities:

    RZG2L_PINCTRL_PS_DESC(2500, 2, PIN_CFG_IO_VMC_ETH0 | PIN_CFG_IO_VMC_ETH1,
                          RZG2L_IOLH_IDX_2V5)

Would a PIN_CFG_SOFT_PS pin requesting 2500 fail this capability check and
be rejected with -EINVAL, breaking backward compatibility?

[ ... ]
> @@ -1812,7 +1854,10 @@ static int rzg2l_pinctrl_pinconf_set(struct pinctrl_dev *pctldev,
>  		enum rzg2l_iolh_index iolh_idx;
>  		int val;
>  
> -		iolh_idx = rzg2l_ps_to_iolh_idx(settings.power_source);
> +		iolh_idx = rzg2l_ps_to_iolh_idx(settings.power_source, cfg);
> +		if (iolh_idx == RZG2L_IOLH_IDX_NA)
> +			return -EINVAL;
> +
>  		ret = rzg2l_ds_is_supported(pctrl, cfg, iolh_idx,
>  					    settings.drive_strength_ua);
>  		if (!ret)

[Severity: High]
This is a pre-existing issue, but is the drive strength hardware register
(IOLH) left out of sync if only the power source changes?

Looking at rzg2l_pinctrl_pinconf_set(), the hardware register update is
skipped if the requested drive strength has not changed:

    if (settings.drive_strength_ua != pctrl->settings[_pin].drive_strength_ua) {
        ...
        iolh_idx = rzg2l_ps_to_iolh_idx(settings.power_source, cfg);

Because the physical mapping between the IOLH register index and the actual
microamp output depends heavily on the selected power source, will keeping
the old IOLH index at a new voltage yield an incorrect drive strength?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260819143553.2478984-1-claudiu.beznea+renesas@tuxon.dev?part=3