linux-kernel - Re: [PATCH] arm64: dts: rockchip: Fix broken tsadc pinctrl binding for rk3588

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8fe92764f7f3df9b25cd832045d28ad5@manjaro.org>
Date: Fri, 24 Jan 2025 11:38:54 +0100
From: Dragan Simic <dsimic@...jaro.org>
To: Alexander Shiyan <eagle.alexander923@...il.com>
Cc: linux-rockchip@...ts.infradead.org, Rob Herring <robh@...nel.org>, Conor
 Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>,
 devicetree@...r.kernel.org, Sebastian Reichel
 <sebastian.reichel@...labora.com>, stable@...r.kernel.org,
 linux-kernel@...r.kernel.org, Alexey Charkov <alchark@...il.com>, Krzysztof
 Kozlowski <krzk+dt@...nel.org>, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] arm64: dts: rockchip: Fix broken tsadc pinctrl binding
 for rk3588

Hello Alexander,

On 2025-01-24 06:26, Alexander Shiyan wrote:
> There is no pinctrl "gpio" and "otpout" (probably designed as "output")
> handling in the tsadc driver.
> Let's use proper binding "default" and "sleep".
> 
> Fixes: 32641b8ab1a5 ("arm64: dts: rockchip: add rk3588 thermal sensor")
> Cc: stable@...r.kernel.org
> Signed-off-by: Alexander Shiyan <eagle.alexander923@...il.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> index a337f3fb8377..f141065eb69d 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
> @@ -2667,9 +2667,9 @@ tsadc: tsadc@...00000 {
>  		rockchip,hw-tshut-temp = <120000>;
>  		rockchip,hw-tshut-mode = <0>; /* tshut mode 0:CRU 1:GPIO */
>  		rockchip,hw-tshut-polarity = <0>; /* tshut polarity 0:LOW 1:HIGH */
> -		pinctrl-0 = <&tsadc_gpio_func>;
> -		pinctrl-1 = <&tsadc_shut>;
> -		pinctrl-names = "gpio", "otpout";
> +		pinctrl-0 = <&tsadc_shut>;
> +		pinctrl-1 = <&tsadc_gpio_func>;
> +		pinctrl-names = "default", "sleep";
>  		#thermal-sensor-cells = <1>;
>  		status = "disabled";
>  	};

Thanks for the patch, it's looking good to me.  The old values
for the pinctrl names are leftovers back from the import of the
downstream kernel code, while the new values follow the expected
pinctrl naming scheme.  The resulting behavior follows, almost
entirely, the behavior found in the downstream kernel code.

Actually, there's some rather critical discrepancy between the
upstream TSADC driver and it's downstream cousin, as already
described in earlier responses from Alexey and me.  However,
those issues have to be addressed in a separate patch, while
this patch, to me, remains fine on its own.

My only suggestions would be to adjust both the patch summary
and the description not to use word "binding", because that
technically isn't fixed here, but to use "pinctrl names" instead.
Also, please note that the downstream kernel uses "otpout" as
a pinctrl name, [1] so the assumption about "output" in the
patch description should be removed.

With the suggestions from above addressed in the v2, please feel
free to include my

Reviewed-by: Dragan Simic <dsimic@...jaro.org>

[1] 
https://raw.githubusercontent.com/rockchip-linux/kernel/refs/heads/develop-5.10/drivers/thermal/rockchip_thermal.c