Received-SPF: pass (google.com: domain of linux-kernel+bounces-204056-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
From: Heiko =?ISO-8859-1?Q?St=FCbner?= <heiko@sntech.de>
To: Linus Walleij <linus.walleij@linaro.org>, Huang-Huang Bao <i@eh5.me>
Cc: linux-gpio@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
 linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org,
 Huang-Huang Bao <i@eh5.me>
Subject:
 Re: [PATCH 3/3] pinctrl: rockchip: fix pinmux reset in rockchip_pmx_set
Date: Thu, 06 Jun 2024 11:51:03 +0200
Message-ID: <2308120.IFkqi6BYcA@diego>
In-Reply-To: <20240606060435.765716-4-i@mail.eh5.me>
References:
 <20240606060435.765716-1-i@eh5.me> <20240606060435.765716-4-i@mail.eh5.me>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"

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@eh5.me>
> ---
>  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@sntech.de>