Re: [PATCH 3/3] pinctrl: rockchip: fix pinmux reset in rockchip_pmx_set

[Heiko Stübner <heiko@xxxxxxxxx>]

Am Donnerstag, 6. Juni 2024, 08:04:35 CEST schrieb Huang-Huang Bao:
> rockchip_pmx_set reset all pinmuxs in group to 0 in the case of error,
> add missing bank data retrieval in that code to avoid setting mux on
> unexpected pins.
> 
> Fixes: 14797189b35e ("pinctrl: rockchip: add return value to rockchip_set_mux")
> Signed-off-by: Huang-Huang Bao <i@xxxxxx>
> ---
>  drivers/pinctrl/pinctrl-rockchip.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pinctrl/pinctrl-rockchip.c b/drivers/pinctrl/pinctrl-rockchip.c
> index 24ee88863ce3..3f56991f5b89 100644
> --- a/drivers/pinctrl/pinctrl-rockchip.c
> +++ b/drivers/pinctrl/pinctrl-rockchip.c
> @@ -2751,8 +2751,10 @@ static int rockchip_pmx_set(struct pinctrl_dev *pctldev, unsigned selector,
>  
>  	if (ret) {
>  		/* revert the already done pin settings */
> -		for (cnt--; cnt >= 0; cnt--)
> +		for (cnt--; cnt >= 0; cnt--) {
> +			bank = pin_to_bank(info, pins[cnt]);
>  			rockchip_set_mux(bank, pins[cnt] - bank->pin_base, 0);
> +		}
>  
>  		return ret;
>  	}
> 

Oh, nice find - and wow that original code is actually 10 years old :-)
For context, original mistake is in the error handling (probably the case
it never turned up)

The first loop counts upwards doing pinmuxing and in the error case,
wants to reset everything back to the "standard" gpio muxing.
The first loop correctly retrieves the bank for each group, but the
error handling will always operate on that last retrieved bank:

 	for (cnt = 0; cnt < info->groups[group].npins; cnt++) {
 		bank = pin_to_bank(info, pins[cnt]);
		ret = rockchip_set_mux(bank, pins[cnt] - bank->pin_base,
				       data[cnt].func);
		if (ret)
			break;
	}

	if (ret) {
		/* revert the already done pin settings */
		for (cnt--; cnt >= 0; cnt--)
			rockchip_set_mux(bank, pins[cnt] - bank->pin_base, 0);

		return ret;
 	}

So, TL;DR:

Reviewed-by: Heiko Stuebner <heiko@xxxxxxxxx>