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: <3e5e7761887e7624c2c225b105105864@manjaro.org>
Date: Sun, 26 Jan 2025 07:09:54 +0100
From: Dragan Simic <dsimic@...jaro.org>
To: Alexey Charkov <alchark@...il.com>
Cc: Alexander Shiyan <eagle.alexander923@...il.com>, 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, linux-rockchip@...ts.infradead.org, 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 Alexey,

On 2025-01-24 18:23, Alexey Charkov wrote:
> On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@...jaro.org> 
> wrote:
>> On 2025-01-24 11:25, Alexey Charkov wrote:
>> > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@...jaro.org>
>> > wrote:
>> >> On 2025-01-24 09:33, Alexey Charkov wrote:
>> >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
>> >> > <eagle.alexander923@...il.com> 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".
>> >> >
>> >> > This looks reasonable, however I've tried it on my Radxa Rock 5C and
>> >> > the driver still doesn't claim GPIO0 RK_PA1 even with this change. As
>> >> > a result, a simulated thermal runaway condition (I've changed the
>> >> > tshut temperature to 65000 and tshut mode to 1) doesn't trigger a PMIC
>> >> > reset, even though a direct `gpioset 0 1=0` does.
>> >> >
>> >> > Are any additional changes needed to the driver itself?
>> >>
>> >> I've been digging through this patch the whole TSADC/OTP thing in the
>> >> last couple of hours, and AFAIK some parts of the upstream driver are
>> >> still missing, in comparison with the downstream driver.
>> >>
>> >> I've got some small suggestions for the patch itself, but the issue
>> >> you observed is obviously of higher priority, and I've singled it out
>> >> as well while digging through the code.
>> >>
>> >> Could you, please, try the patch below quickly, to see is it going to
>> >> fix the issue you observed?  I've got some "IRL stuff" to take care of
>> >> today, so I can't test it myself, and it would be great to know is it
>> >> the right path to the proper fix.
>> >>
>> >> diff --git i/drivers/thermal/rockchip_thermal.c
>> >> w/drivers/thermal/rockchip_thermal.c
>> >> index f551df48eef9..62f0e14a8d98 100644
>> >> --- i/drivers/thermal/rockchip_thermal.c
>> >> +++ w/drivers/thermal/rockchip_thermal.c
>> >> @@ -1568,6 +1568,11 @@ static int rockchip_thermal_probe(struct
>> >> platform_device *pdev)
>> >>          thermal->chip->initialize(thermal->grf, thermal->regs,
>> >>                                    thermal->tshut_polarity);
>> >>
>> >> +       if (thermal->tshut_mode == TSHUT_MODE_GPIO)
>> >> +               pinctrl_select_default_state(dev);
>> >> +       else
>> >> +               pinctrl_select_sleep_state(dev);
>> >
>> > I believe no 'else' block is needed here, because if tshut_mode is not
>> > TSHUT_MODE_GPIO then the TSADC doesn't use this pin at all, so there's
>> > no reason for the driver to mess with its pinctrl state. I'd rather
>> > put a mirroring block to put the pin back to its 'sleep' state in the
>> > removal function for the TSHUT_MODE_GPIO case.
>> 
>> You're right, but the "else block" is what the downstream driver does,
> 
> Does it though? It only handles the TSHUT_MODE_GPIO case as far as I
> can tell (or TSHUT_MODE_OTP in downstream driver lingo) [1]
> 
> [1] 
> https://github.com/radxa/kernel/blob/edb3eeeaa4643ecac6f4185d6d391c574735fca1/drivers/thermal/rockchip_thermal.c#L2564

Ah, you're right.  Somehow I saw something that actually wasn't
there, so the else block would indeed be redundant.

>> so I think it's better to simply stay on the safe side and follow that
>> logic in the upstream driver.  Is it really needed?  Perhaps not, but
>> it also shouldn't hurt.