Re: [PATCH v9 16/36] pinctrl: airoha: minor improvements

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

New issues:
- [Low] The newly added function-scoped `err` variable is shadowed by an existing local `err` variable inside the switch statement.

Pre-existing issues:
- [High] The commit adds missing error checks for PIN_CONFIG_BIAS_* settings but fails to do the same for PIN_CONFIG_DRIVE_STRENGTH and PIN_CONFIG_DRIVE_OPEN_DRAIN in the same switch statement.
--

--- Patch [16]: [PATCH v9 16/36] pinctrl: airoha: minor improvements ---
commit f4dfe61b91abddcba631650033db26be051c2e35
Author: Mikhail Kshevetskiy <[email protected]>

pinctrl: airoha: minor improvements

Changes:
 * use field_get() macro instead of direct bit operations
 * improve error handling

> diff --git a/drivers/pinctrl/airoha/pinctrl-airoha.c b/drivers/pinctrl/airoha/pinctrl-airoha.c
> index e4ccf357df892..f982a75d4253e 100644
> --- a/drivers/pinctrl/airoha/pinctrl-airoha.c
> +++ b/drivers/pinctrl/airoha/pinctrl-airoha.c
[ ... ]
> @@ -2996,7 +2996,7 @@ static int airoha_pinconf_set(struct pinctrl_dev *pctrl_dev,
>  			      unsigned int num_configs)
>  {
>  	struct airoha_pinctrl *pinctrl = pinctrl_dev_get_drvdata(pctrl_dev);
> -	int i;
> +	int i, err;

[Severity: Low]
Does the inner err declaration inside the PIN_CONFIG_OUTPUT_ENABLE block
shadow this newly added function-scoped err variable?

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_pinconf_set() {
    ...
		case PIN_CONFIG_OUTPUT_ENABLE:
		case PIN_CONFIG_INPUT_ENABLE:
		case PIN_CONFIG_LEVEL: {
			bool input = param == PIN_CONFIG_INPUT_ENABLE;
			int err;
    ...
}

This could trigger -Wshadow compiler warnings.

>  
>  	for (i = 0; i < num_configs; i++) {
>  		u32 param = pinconf_to_config_param(configs[i]);
[ ... ]
>  		case PIN_CONFIG_BIAS_PULL_DOWN:
> -			airoha_pinctrl_set_pulldown_conf(pinctrl, pin, 1);
> -			airoha_pinctrl_set_pullup_conf(pinctrl, pin, 0);
> +			err = airoha_pinctrl_set_pulldown_conf(pinctrl, pin, 1);
> +			if (err)
> +				return err;
> +
> +			err = airoha_pinctrl_set_pullup_conf(pinctrl, pin, 0);
> +			if (err)
> +				return err;
> +
>  			break;
>  		case PIN_CONFIG_DRIVE_STRENGTH: {
>  			u32 e2 = 0, e4 = 0;

[Severity: High]
This is a pre-existing issue, but since this patch improves error handling for
the PIN_CONFIG_BIAS_* settings, shouldn't we also check the return values for
PIN_CONFIG_DRIVE_STRENGTH and PIN_CONFIG_DRIVE_OPEN_DRAIN in the same switch
statement?

drivers/pinctrl/airoha/pinctrl-airoha.c:airoha_pinconf_set() {
    ...
			airoha_pinctrl_set_drive_e2_conf(pinctrl, pin, e2);
			airoha_pinctrl_set_drive_e4_conf(pinctrl, pin, e4);
			break;
		}
		case PIN_CONFIG_DRIVE_OPEN_DRAIN:
			airoha_pinctrl_set_pcie_rst_od_conf(pinctrl, pin, !!arg);
			break;
    ...
}

If a caller requests these configurations on a pin that does not support them,
the underlying airoha_pinctrl_get_conf_reg() returns NULL, causing
airoha_pinctrl_set_conf() to return -EINVAL.

Because the macro invocations ignore this return value, airoha_pinconf_set()
silently succeeds. Could this make board bring-up or device configuration bugs
hard to debug?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=16