[PATCH] arm64: dts: rockchip: Fix broken tsadc pinctrl binding for rk3588

[PATCH] arm64: dts: rockchip: Fix broken tsadc pinctrl binding for rk3588

[Alexander Shiyan]

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@vger.kernel.org
Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.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@fec00000 {
 		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";
 	};
-- 
2.39.1

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

[Alexey Charkov]

Hi Alexander,

On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
<eagle.alexander923@gmail.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?

Best regards,
Alexey

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

[Dragan Simic]

Hello Alexey,

On 2025-01-24 09:33, Alexey Charkov wrote:
> On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
> <eagle.alexander923@gmail.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);
+
         for (i = 0; i < thermal->chip->chn_num; i++) {
                 error = rockchip_thermal_register_sensor(pdev, thermal,
                                                 &thermal->sensors[i],

If you could test it, please, it would be great, and I'd prepare the
proper patch tomorrow or so.

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

[Alexey Charkov]

On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Alexey,
>
> On 2025-01-24 09:33, Alexey Charkov wrote:
> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
> > <eagle.alexander923@gmail.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.

Will try and revert.

P.S. Just looked at the downstream driver, and it actually calls
TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
not a typo in the first place. So maybe the right approach here is not
to change the device tree but rather fix the "gpio" / "otpout" pinctrl
state handling in the driver.

Best,
Alexey

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

[Dragan Simic]

On 2025-01-24 11:25, Alexey Charkov wrote:
> On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2025-01-24 09:33, Alexey Charkov wrote:
>> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
>> > <eagle.alexander923@gmail.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,
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.

> Will try and revert.

Awesome, thanks!

> P.S. Just looked at the downstream driver, and it actually calls
> TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
> not a typo in the first place. So maybe the right approach here is not
> to change the device tree but rather fix the "gpio" / "otpout" pinctrl
> state handling in the driver.

Indeed, "otpout" wasn't a typo, and I've already addressed that in my
comments to Alexander's patch.  Will send that response a bit later.

I think it's actually better to accept the approach in Alexander's
patch, because the whole thing applies to other Rockchip SoCs as well,
not just to the RK3588(S).

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

[Dragan Simic]

On 2025-01-24 11:37, Dragan Simic wrote:
> On 2025-01-24 11:25, Alexey Charkov wrote:
>> On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org> 
>> wrote:
>>> On 2025-01-24 09:33, Alexey Charkov wrote:
>>> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
>>> > <eagle.alexander923@gmail.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,
> 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.
> 
>> Will try and revert.
> 
> Awesome, thanks!

Actually...  Revert or report? :)

>> P.S. Just looked at the downstream driver, and it actually calls
>> TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
>> not a typo in the first place. So maybe the right approach here is not
>> to change the device tree but rather fix the "gpio" / "otpout" pinctrl
>> state handling in the driver.
> 
> Indeed, "otpout" wasn't a typo, and I've already addressed that in my
> comments to Alexander's patch.  Will send that response a bit later.
> 
> I think it's actually better to accept the approach in Alexander's
> patch, because the whole thing applies to other Rockchip SoCs as well,
> not just to the RK3588(S).

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

[Alexey Charkov]

On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@manjaro.org> wrote:
>
> On 2025-01-24 11:25, Alexey Charkov wrote:
> > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org>
> > wrote:
> >> On 2025-01-24 09:33, Alexey Charkov wrote:
> >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
> >> > <eagle.alexander923@gmail.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

> 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.
>
> > Will try and revert.
>
> Awesome, thanks!
>
> > P.S. Just looked at the downstream driver, and it actually calls
> > TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
> > not a typo in the first place. So maybe the right approach here is not
> > to change the device tree but rather fix the "gpio" / "otpout" pinctrl
> > state handling in the driver.
>
> Indeed, "otpout" wasn't a typo, and I've already addressed that in my
> comments to Alexander's patch.  Will send that response a bit later.
>
> I think it's actually better to accept the approach in Alexander's
> patch, because the whole thing applies to other Rockchip SoCs as well,
> not just to the RK3588(S).

Anyway, I've just tried it after including the changes below, and
while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
pinctrls under tsadc, the driver still doesn't seem to be triggering a
PMIC reset. Weird. Any thoughts welcome.

Best regards,
Alexey

rock-5c ~ # cat /sys/kernel/debug/pinctrl/pinctrl-handles
Requested pin control handlers their pinmux maps:
[...]
device: fec00000.tsadc current state: default
 state: default
   type: MUX_GROUP controller rockchip-pinctrl group: tsadc-shut (84)
function: tsadc (84)
   type: CONFIGS_PIN controller rockchip-pinctrl pin gpio0-1 (1)config 00000001
 state: sleep
   type: MUX_GROUP controller rockchip-pinctrl group: tsadc-gpio-func
(95) function: gpio-func (97)
   type: CONFIGS_PIN controller rockchip-pinctrl pin gpio0-1 (1)config 00000001
[...]

rock-5c ~ # gpioinfo 0
gpiochip0 - 32 lines:
       line   0:      unnamed "regulator-vdd-3v3" output active-high [used]
       line   1:      unnamed       unused   input  active-high
       line   2:      unnamed       unused   input  active-high
       line   3:      unnamed       unused   input  active-high
       line   4:      unnamed       unused   input  active-high
       line   5:      unnamed       unused   input  active-high
       line   6:      unnamed       unused   input  active-high
       line   7:      unnamed       unused   input  active-high
       line   8:      unnamed       unused   input  active-high
       line   9:      unnamed       unused   input  active-high
       line  10:      unnamed       unused   input  active-high
       line  11:      unnamed       unused   input  active-high
       line  12:      unnamed       unused   input  active-high
       line  13:      unnamed       unused   input  active-high
       line  14:      unnamed       unused   input  active-high
       line  15:      unnamed       unused   input  active-high
       line  16:      unnamed       unused   input  active-high
       line  17:      unnamed       unused   input  active-high
       line  18:      unnamed       unused   input  active-high
       line  19:      unnamed       unused   input  active-high
       line  20:      unnamed       unused   input  active-high
       line  21:      unnamed "regulator-pcie2x1l2-3v3" output
active-high [used]
       line  22:      unnamed       unused   input  active-high
       line  23:      unnamed       unused   input  active-high
       line  24:      unnamed       unused   input  active-high
       line  25:      unnamed       unused   input  active-high
       line  26:      unnamed       unused   input  active-high
       line  27:      unnamed       unused   input  active-high
       line  28:      unnamed "regulator-vcc5v0-usb-otg0" output
active-high [used]
       line  29:      unnamed       unused   input  active-high
       line  30:      unnamed       unused   input  active-high
       line  31:      unnamed       unused   input  active-high

(note line 1: unused above)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
index 6e56d7704cbe..e8c4d9b3f828 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
@@ -873,6 +873,9 @@ regulator-state-mem {
};

&tsadc {
+       rockchip,hw-tshut-temp = <65000>;
+       rockchip,hw-tshut-mode = <1>; /* tshut mode 0:CRU 1:GPIO */
+       rockchip,hw-tshut-polarity = <0>; /* tshut polarity 0:LOW 1:HIGH */
       status = "okay";
};

diff --git a/drivers/thermal/rockchip_thermal.c
b/drivers/thermal/rockchip_thermal.c
index f551df48eef9..4f474906b2b0 100644
--- a/drivers/thermal/rockchip_thermal.c
+++ b/drivers/thermal/rockchip_thermal.c
@@ -1568,6 +1568,9 @@ 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(&pdev->dev);
+
       for (i = 0; i < thermal->chip->chn_num; i++) {
               error = rockchip_thermal_register_sensor(pdev, thermal,
                                               &thermal->sensors[i],
@@ -1614,6 +1617,9 @@ static void rockchip_thermal_remove(struct
platform_device *pdev)
       }

       thermal->chip->control(thermal->regs, false);
+
+       if (thermal->tshut_mode == TSHUT_MODE_GPIO)
+               pinctrl_pm_select_sleep_state(&pdev->dev);
}

static int __maybe_unused rockchip_thermal_suspend(struct device *dev)
@@ -1629,7 +1635,8 @@ static int __maybe_unused
rockchip_thermal_suspend(struct device *dev)
       clk_disable(thermal->pclk);
       clk_disable(thermal->clk);

-       pinctrl_pm_select_sleep_state(dev);
+       if (thermal->tshut_mode == TSHUT_MODE_GPIO)
+               pinctrl_pm_select_sleep_state(dev);

       return 0;
}
@@ -1674,7 +1681,8 @@ static int __maybe_unused
rockchip_thermal_resume(struct device *dev)
       for (i = 0; i < thermal->chip->chn_num; i++)
               rockchip_thermal_toggle_sensor(&thermal->sensors[i], true);

-       pinctrl_pm_select_default_state(dev);
+       if (thermal->tshut_mode == TSHUT_MODE_GPIO)
+               pinctrl_pm_select_default_state(dev);

       return 0;
}

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

[Alexey Charkov]

On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov <alchark@gmail.com> wrote:
>
> On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@manjaro.org> wrote:
> >
> > On 2025-01-24 11:25, Alexey Charkov wrote:
> > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org>
> > > wrote:
> > >> On 2025-01-24 09:33, Alexey Charkov wrote:
> > >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
> > >> > <eagle.alexander923@gmail.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
>
> > 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.
> >
> > > Will try and revert.
> >
> > Awesome, thanks!
> >
> > > P.S. Just looked at the downstream driver, and it actually calls
> > > TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
> > > not a typo in the first place. So maybe the right approach here is not
> > > to change the device tree but rather fix the "gpio" / "otpout" pinctrl
> > > state handling in the driver.
> >
> > Indeed, "otpout" wasn't a typo, and I've already addressed that in my
> > comments to Alexander's patch.  Will send that response a bit later.
> >
> > I think it's actually better to accept the approach in Alexander's
> > patch, because the whole thing applies to other Rockchip SoCs as well,
> > not just to the RK3588(S).
>
> Anyway, I've just tried it after including the changes below, and
> while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
> pinctrls under tsadc, the driver still doesn't seem to be triggering a
> PMIC reset. Weird. Any thoughts welcome.

I found the culprit. "otpout" (or "default" if we follow Alexander's
suggested approach) pinctrl state should refer to the &tsadc_shut_org
config instead of &tsadc_shut - then the PMIC reset works.

Best,
Alexey

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

[Dragan Simic]

On 2025-01-24 20:44, Alexey Charkov wrote:
> On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov <alchark@gmail.com> 
> wrote:
>> On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@manjaro.org> 
>> wrote:
>> > On 2025-01-24 11:25, Alexey Charkov wrote:
>> > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org>
>> > > wrote:
>> > >> On 2025-01-24 09:33, Alexey Charkov wrote:
>> > >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
>> > >> > <eagle.alexander923@gmail.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
>> 
>> > 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.
>> >
>> > > Will try and revert.
>> >
>> > Awesome, thanks!
>> >
>> > > P.S. Just looked at the downstream driver, and it actually calls
>> > > TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
>> > > not a typo in the first place. So maybe the right approach here is not
>> > > to change the device tree but rather fix the "gpio" / "otpout" pinctrl
>> > > state handling in the driver.
>> >
>> > Indeed, "otpout" wasn't a typo, and I've already addressed that in my
>> > comments to Alexander's patch.  Will send that response a bit later.
>> >
>> > I think it's actually better to accept the approach in Alexander's
>> > patch, because the whole thing applies to other Rockchip SoCs as well,
>> > not just to the RK3588(S).
>> 
>> Anyway, I've just tried it after including the changes below, and
>> while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
>> pinctrls under tsadc, the driver still doesn't seem to be triggering a
>> PMIC reset. Weird. Any thoughts welcome.
> 
> I found the culprit. "otpout" (or "default" if we follow Alexander's
> suggested approach) pinctrl state should refer to the &tsadc_shut_org
> config instead of &tsadc_shut - then the PMIC reset works.

Huh, thanks for debugging, but this is quite confusing.  Let me dig
through everything again later today.

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

[Alexander Shiyan]

Hello.

> > > I think it's actually better to accept the approach in Alexander's
> > > patch, because the whole thing applies to other Rockchip SoCs as well,
> > > not just to the RK3588(S).
> >
> > Anyway, I've just tried it after including the changes below, and
> > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
> > pinctrls under tsadc, the driver still doesn't seem to be triggering a
> > PMIC reset. Weird. Any thoughts welcome.
>
> I found the culprit. "otpout" (or "default" if we follow Alexander's
> suggested approach) pinctrl state should refer to the &tsadc_shut_org
> config instead of &tsadc_shut - then the PMIC reset works.

Great, I'll use this in v2.

Thanks!

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

[Dragan Simic]

Hello Alexander,

On 2025-01-26 15:25, Alexander Shiyan wrote:
>> > > I think it's actually better to accept the approach in Alexander's
>> > > patch, because the whole thing applies to other Rockchip SoCs as well,
>> > > not just to the RK3588(S).
>> >
>> > Anyway, I've just tried it after including the changes below, and
>> > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
>> > pinctrls under tsadc, the driver still doesn't seem to be triggering a
>> > PMIC reset. Weird. Any thoughts welcome.
>> 
>> I found the culprit. "otpout" (or "default" if we follow Alexander's
>> suggested approach) pinctrl state should refer to the &tsadc_shut_org
>> config instead of &tsadc_shut - then the PMIC reset works.
> 
> Great, I'll use this in v2.

Please, let's wait with the v2 until I go through the whole thing again,
which I expected to have done already, but had some other "IRL stuff" 
that
introduced a delay.

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

[Alexey Charkov]

On Tue, Jan 28, 2025 at 1:24 PM Dragan Simic <dsimic@manjaro.org> wrote:
>
> Hello Alexander,
>
> On 2025-01-26 15:25, Alexander Shiyan wrote:
> >> > > I think it's actually better to accept the approach in Alexander's
> >> > > patch, because the whole thing applies to other Rockchip SoCs as well,
> >> > > not just to the RK3588(S).
> >> >
> >> > Anyway, I've just tried it after including the changes below, and
> >> > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
> >> > pinctrls under tsadc, the driver still doesn't seem to be triggering a
> >> > PMIC reset. Weird. Any thoughts welcome.
> >>
> >> I found the culprit. "otpout" (or "default" if we follow Alexander's
> >> suggested approach) pinctrl state should refer to the &tsadc_shut_org
> >> config instead of &tsadc_shut - then the PMIC reset works.
> >
> > Great, I'll use this in v2.
>
> Please, let's wait with the v2 until I go through the whole thing again

I, for one, would welcome a v2 that could be tested and confirmed
working with and without driver changes. Especially given that:
 - the changes are pretty small
 - hardware docs say nothing about the difference between TSADC_SHUT
vs. TSADC_SHUT_ORG, except that one is config #2 and the other is
config #1
 - none of the source trees I looked at seem to enable PMIC based
resets on any RK3588-based boards, so these pinctrl configs appear to
have never been tested in the wild for RK3588*

So trying and testing seems to be the only way to understand the best
way forward. Unless, of course, someone from Rockchip can comment on
how the hardware works with TSADC_SHUT vs. TSADC_SHUT_ORG.

Best regards,
Alexey

> which I expected to have done already, but had some other "IRL stuff"
> that
> introduced a delay.

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

[Dragan Simic]

Hello Alexey,

On 2025-01-28 11:30, Alexey Charkov wrote:
> On Tue, Jan 28, 2025 at 1:24 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2025-01-26 15:25, Alexander Shiyan wrote:
>> >> > > I think it's actually better to accept the approach in Alexander's
>> >> > > patch, because the whole thing applies to other Rockchip SoCs as well,
>> >> > > not just to the RK3588(S).
>> >> >
>> >> > Anyway, I've just tried it after including the changes below, and
>> >> > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
>> >> > pinctrls under tsadc, the driver still doesn't seem to be triggering a
>> >> > PMIC reset. Weird. Any thoughts welcome.
>> >>
>> >> I found the culprit. "otpout" (or "default" if we follow Alexander's
>> >> suggested approach) pinctrl state should refer to the &tsadc_shut_org
>> >> config instead of &tsadc_shut - then the PMIC reset works.
>> >
>> > Great, I'll use this in v2.
>> 
>> Please, let's wait with the v2 until I go through the whole thing 
>> again
> 
> I, for one, would welcome a v2 that could be tested and confirmed
> working with and without driver changes. Especially given that:
>  - the changes are pretty small
>  - hardware docs say nothing about the difference between TSADC_SHUT
> vs. TSADC_SHUT_ORG, except that one is config #2 and the other is
> config #1
>  - none of the source trees I looked at seem to enable PMIC based
> resets on any RK3588-based boards, so these pinctrl configs appear to
> have never been tested in the wild for RK3588*
> 
> So trying and testing seems to be the only way to understand the best
> way forward. Unless, of course, someone from Rockchip can comment on
> how the hardware works with TSADC_SHUT vs. TSADC_SHUT_ORG.

Perhaps the best approach would be to have the v2 that works without
the driver changes, so it can be propagated into the stable kernels.

Then, a separate series, which I'll volunteer for, :) would introduce
the cleanups and any needed driver changes, which wouldn't be propagated
into the stable kernels.  That way, we'd have the minimal bugfixes in
stable kernels, and the nice cleanups in the latest kernel version.

Of course, detailed testing is mandatory.

>> which I expected to have done already, but had some other "IRL stuff"
>> that introduced a delay.

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

[Laurent Pinchart]

On Fri, Jan 24, 2025 at 11:44:34PM +0400, Alexey Charkov wrote:
> On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov <alchark@gmail.com> wrote:
> > On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@manjaro.org> wrote:
> > > On 2025-01-24 11:25, Alexey Charkov wrote:
> > > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org>
> > > > wrote:
> > > >> On 2025-01-24 09:33, Alexey Charkov wrote:
> > > >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
> > > >> > <eagle.alexander923@gmail.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
> >
> > > 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.
> > >
> > > > Will try and revert.
> > >
> > > Awesome, thanks!
> > >
> > > > P.S. Just looked at the downstream driver, and it actually calls
> > > > TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
> > > > not a typo in the first place. So maybe the right approach here is not
> > > > to change the device tree but rather fix the "gpio" / "otpout" pinctrl
> > > > state handling in the driver.
> > >
> > > Indeed, "otpout" wasn't a typo, and I've already addressed that in my
> > > comments to Alexander's patch.  Will send that response a bit later.
> > >
> > > I think it's actually better to accept the approach in Alexander's
> > > patch, because the whole thing applies to other Rockchip SoCs as well,
> > > not just to the RK3588(S).
> >
> > Anyway, I've just tried it after including the changes below, and
> > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
> > pinctrls under tsadc, the driver still doesn't seem to be triggering a
> > PMIC reset. Weird. Any thoughts welcome.
> 
> I found the culprit. "otpout" (or "default" if we follow Alexander's
> suggested approach) pinctrl state should refer to the &tsadc_shut_org
> config instead of &tsadc_shut - then the PMIC reset works.

I've recently brought up an RK3588S-based Orange Pi CM5 Base board, made
of a compute module (CM5, see [1]) and a carrier board (Base, see [2]).
The carrier board has a reset button which pulls the PMIC_RESET_L signal
of the CM5 to GND (see page 3 of the schematics in [3]).

With &tsadc_shut_org the reset button has absolutely no effect. With
&tsadc_shut it resets the board as expected.

Unfortunately the schematics of the CM5 is not available so it's not
immediately clear what the reset button is connected exactly. The same
manufacturer sells another board based on the same SoC, the Orange Pi 5B
([4]) whose full schematics is available ([5]). The board also has a
reset button pulling a PMIC_RESET_L signal to GND. The signal is pulled
up to VCC_1V8_S3 and connected to

- RESETB on the PMIC
- NPOR on the RK3588S
- TSADC_SHUT (GPIO0_A1) on the RK3588S

[1] http://www.orangepi.org/html/hardWare/computerAndMicrocontrollers/details/Orange-Pi-CM5.html
[2] http://www.orangepi.org/html/hardWare/computerAndMicrocontrollers/details/Orange-Pi-CM5-Board.html
[3] https://drive.google.com/file/d/1t4WmXGWed8NnS0m2PWE7p5YLiR1dbnvP/view
[4] http://www.orangepi.org/html/hardWare/computerAndMicrocontrollers/details/Orange-Pi-5B.html
[5] https://drive.google.com/file/d/19iPdpAXhA1vFkVOgG5Sz7L6cV8BB6004/view

-- 
Regards,

Laurent Pinchart

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

[Alexey Charkov]

Hi Laurent,

On Fri, Oct 3, 2025 at 5:33 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Fri, Jan 24, 2025 at 11:44:34PM +0400, Alexey Charkov wrote:
> > On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov <alchark@gmail.com> wrote:
> > > On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@manjaro.org> wrote:
> > > > On 2025-01-24 11:25, Alexey Charkov wrote:
> > > > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org>
> > > > > wrote:
> > > > >> On 2025-01-24 09:33, Alexey Charkov wrote:
> > > > >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
> > > > >> > <eagle.alexander923@gmail.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
> > >
> > > > 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.
> > > >
> > > > > Will try and revert.
> > > >
> > > > Awesome, thanks!
> > > >
> > > > > P.S. Just looked at the downstream driver, and it actually calls
> > > > > TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
> > > > > not a typo in the first place. So maybe the right approach here is not
> > > > > to change the device tree but rather fix the "gpio" / "otpout" pinctrl
> > > > > state handling in the driver.
> > > >
> > > > Indeed, "otpout" wasn't a typo, and I've already addressed that in my
> > > > comments to Alexander's patch.  Will send that response a bit later.
> > > >
> > > > I think it's actually better to accept the approach in Alexander's
> > > > patch, because the whole thing applies to other Rockchip SoCs as well,
> > > > not just to the RK3588(S).
> > >
> > > Anyway, I've just tried it after including the changes below, and
> > > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
> > > pinctrls under tsadc, the driver still doesn't seem to be triggering a
> > > PMIC reset. Weird. Any thoughts welcome.
> >
> > I found the culprit. "otpout" (or "default" if we follow Alexander's
> > suggested approach) pinctrl state should refer to the &tsadc_shut_org
> > config instead of &tsadc_shut - then the PMIC reset works.
>
> I've recently brought up an RK3588S-based Orange Pi CM5 Base board, made
> of a compute module (CM5, see [1]) and a carrier board (Base, see [2]).
> The carrier board has a reset button which pulls the PMIC_RESET_L signal
> of the CM5 to GND (see page 3 of the schematics in [3]).
>
> With &tsadc_shut_org the reset button has absolutely no effect. With
> &tsadc_shut it resets the board as expected.

Interesting. The TSADC shouldn't affect the physical button operation
at all, if it's really wired to the PMIC as the signal name implies.
There isn't even any default pull value associated with the TSHUT pin
config.

What if you switch the GPIO0_A1 pin to GPIO output mode with no pull?
Does the button work then? Does the board reset if you toggle the GPIO
value with `gpioset`?

Best regards,
Alexey

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

[Alexey Charkov]

On Fri, Oct 3, 2025 at 6:13 PM Alexey Charkov <alchark@gmail.com> wrote:
>
> Hi Laurent,
>
> On Fri, Oct 3, 2025 at 5:33 PM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > On Fri, Jan 24, 2025 at 11:44:34PM +0400, Alexey Charkov wrote:
> > > On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov <alchark@gmail.com> wrote:
> > > > On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@manjaro.org> wrote:
> > > > > On 2025-01-24 11:25, Alexey Charkov wrote:
> > > > > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org>
> > > > > > wrote:
> > > > > >> On 2025-01-24 09:33, Alexey Charkov wrote:
> > > > > >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
> > > > > >> > <eagle.alexander923@gmail.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
> > > >
> > > > > 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.
> > > > >
> > > > > > Will try and revert.
> > > > >
> > > > > Awesome, thanks!
> > > > >
> > > > > > P.S. Just looked at the downstream driver, and it actually calls
> > > > > > TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
> > > > > > not a typo in the first place. So maybe the right approach here is not
> > > > > > to change the device tree but rather fix the "gpio" / "otpout" pinctrl
> > > > > > state handling in the driver.
> > > > >
> > > > > Indeed, "otpout" wasn't a typo, and I've already addressed that in my
> > > > > comments to Alexander's patch.  Will send that response a bit later.
> > > > >
> > > > > I think it's actually better to accept the approach in Alexander's
> > > > > patch, because the whole thing applies to other Rockchip SoCs as well,
> > > > > not just to the RK3588(S).
> > > >
> > > > Anyway, I've just tried it after including the changes below, and
> > > > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
> > > > pinctrls under tsadc, the driver still doesn't seem to be triggering a
> > > > PMIC reset. Weird. Any thoughts welcome.
> > >
> > > I found the culprit. "otpout" (or "default" if we follow Alexander's
> > > suggested approach) pinctrl state should refer to the &tsadc_shut_org
> > > config instead of &tsadc_shut - then the PMIC reset works.
> >
> > I've recently brought up an RK3588S-based Orange Pi CM5 Base board, made
> > of a compute module (CM5, see [1]) and a carrier board (Base, see [2]).
> > The carrier board has a reset button which pulls the PMIC_RESET_L signal
> > of the CM5 to GND (see page 3 of the schematics in [3]).
> >
> > With &tsadc_shut_org the reset button has absolutely no effect. With
> > &tsadc_shut it resets the board as expected.
>
> Interesting. The TSADC shouldn't affect the physical button operation
> at all, if it's really wired to the PMIC as the signal name implies.
> There isn't even any default pull value associated with the TSHUT pin
> config.

On a second thought, I've got another hypothesis. Your baseboard only
pulls the reset line through  a 100 Ohm resistor when the button is
pressed. So if the TSHUT pin is in its default push-pull mode and
stays high when no thermal runaway reset is requested, the reset
button won't pull the line fully to zero, as the TSHUT line pulls it
high at the same time.

If you switch it from &tsadc_shut_org to &tsadc_shut, then it stops
working properly as the thermal protection reset, and GPIO0_A1 remains
high-impendance, thus allowing the reset button to function even
though its pull is too weak.

So maybe change the pin configuration of &tsadc_shut_org in
rk3588-base-pinctrl.dtsi to open drain and retry?

Best regards,
Alexey

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

[Laurent Pinchart]

Hi Alexey,

On Fri, Oct 03, 2025 at 06:55:26PM +0400, Alexey Charkov wrote:
> On Fri, Oct 3, 2025 at 6:13 PM Alexey Charkov wrote:
> > On Fri, Oct 3, 2025 at 5:33 PM Laurent Pinchart wrote:
> > > On Fri, Jan 24, 2025 at 11:44:34PM +0400, Alexey Charkov wrote:
> > > > On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov <alchark@gmail.com> wrote:
> > > > > On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@manjaro.org> wrote:
> > > > > > On 2025-01-24 11:25, Alexey Charkov wrote:
> > > > > > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org>
> > > > > > > wrote:
> > > > > > >> On 2025-01-24 09:33, Alexey Charkov wrote:
> > > > > > >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
> > > > > > >> > <eagle.alexander923@gmail.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
> > > > >
> > > > > > 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.
> > > > > >
> > > > > > > Will try and revert.
> > > > > >
> > > > > > Awesome, thanks!
> > > > > >
> > > > > > > P.S. Just looked at the downstream driver, and it actually calls
> > > > > > > TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
> > > > > > > not a typo in the first place. So maybe the right approach here is not
> > > > > > > to change the device tree but rather fix the "gpio" / "otpout" pinctrl
> > > > > > > state handling in the driver.
> > > > > >
> > > > > > Indeed, "otpout" wasn't a typo, and I've already addressed that in my
> > > > > > comments to Alexander's patch.  Will send that response a bit later.
> > > > > >
> > > > > > I think it's actually better to accept the approach in Alexander's
> > > > > > patch, because the whole thing applies to other Rockchip SoCs as well,
> > > > > > not just to the RK3588(S).
> > > > >
> > > > > Anyway, I've just tried it after including the changes below, and
> > > > > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
> > > > > pinctrls under tsadc, the driver still doesn't seem to be triggering a
> > > > > PMIC reset. Weird. Any thoughts welcome.
> > > >
> > > > I found the culprit. "otpout" (or "default" if we follow Alexander's
> > > > suggested approach) pinctrl state should refer to the &tsadc_shut_org
> > > > config instead of &tsadc_shut - then the PMIC reset works.
> > >
> > > I've recently brought up an RK3588S-based Orange Pi CM5 Base board, made
> > > of a compute module (CM5, see [1]) and a carrier board (Base, see [2]).
> > > The carrier board has a reset button which pulls the PMIC_RESET_L signal
> > > of the CM5 to GND (see page 3 of the schematics in [3]).
> > >
> > > With &tsadc_shut_org the reset button has absolutely no effect. With
> > > &tsadc_shut it resets the board as expected.
> >
> > Interesting. The TSADC shouldn't affect the physical button operation
> > at all, if it's really wired to the PMIC as the signal name implies.
> > There isn't even any default pull value associated with the TSHUT pin
> > config.
> 
> On a second thought, I've got another hypothesis. Your baseboard only
> pulls the reset line through  a 100 Ohm resistor when the button is
> pressed. So if the TSHUT pin is in its default push-pull mode and
> stays high when no thermal runaway reset is requested, the reset
> button won't pull the line fully to zero, as the TSHUT line pulls it
> high at the same time.

That's the most likely cause, I agree.

> If you switch it from &tsadc_shut_org to &tsadc_shut, then it stops
> working properly as the thermal protection reset, and GPIO0_A1 remains
> high-impendance, thus allowing the reset button to function even
> though its pull is too weak.

By the way, what is the difference between tsadc_shut_org and tsadc_shut
? I haven't seen it being clearly documented in the TRM.

> So maybe change the pin configuration of &tsadc_shut_org in
> rk3588-base-pinctrl.dtsi to open drain and retry?

That's a good idea, but... how ? The pinctrl-rockchip driver doesn't
seem to support generic open-drain configuration.

-- 
Regards,

Laurent Pinchart

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

[Alexey Charkov]

On Sat, Oct 4, 2025 at 3:29 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Alexey,
>
> On Fri, Oct 03, 2025 at 06:55:26PM +0400, Alexey Charkov wrote:
> > On Fri, Oct 3, 2025 at 6:13 PM Alexey Charkov wrote:
> > > On Fri, Oct 3, 2025 at 5:33 PM Laurent Pinchart wrote:
> > > > On Fri, Jan 24, 2025 at 11:44:34PM +0400, Alexey Charkov wrote:
> > > > > On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov <alchark@gmail.com> wrote:
> > > > > > On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@manjaro.org> wrote:
> > > > > > > On 2025-01-24 11:25, Alexey Charkov wrote:
> > > > > > > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org>
> > > > > > > > wrote:
> > > > > > > >> On 2025-01-24 09:33, Alexey Charkov wrote:
> > > > > > > >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
> > > > > > > >> > <eagle.alexander923@gmail.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
> > > > > >
> > > > > > > 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.
> > > > > > >
> > > > > > > > Will try and revert.
> > > > > > >
> > > > > > > Awesome, thanks!
> > > > > > >
> > > > > > > > P.S. Just looked at the downstream driver, and it actually calls
> > > > > > > > TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
> > > > > > > > not a typo in the first place. So maybe the right approach here is not
> > > > > > > > to change the device tree but rather fix the "gpio" / "otpout" pinctrl
> > > > > > > > state handling in the driver.
> > > > > > >
> > > > > > > Indeed, "otpout" wasn't a typo, and I've already addressed that in my
> > > > > > > comments to Alexander's patch.  Will send that response a bit later.
> > > > > > >
> > > > > > > I think it's actually better to accept the approach in Alexander's
> > > > > > > patch, because the whole thing applies to other Rockchip SoCs as well,
> > > > > > > not just to the RK3588(S).
> > > > > >
> > > > > > Anyway, I've just tried it after including the changes below, and
> > > > > > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
> > > > > > pinctrls under tsadc, the driver still doesn't seem to be triggering a
> > > > > > PMIC reset. Weird. Any thoughts welcome.
> > > > >
> > > > > I found the culprit. "otpout" (or "default" if we follow Alexander's
> > > > > suggested approach) pinctrl state should refer to the &tsadc_shut_org
> > > > > config instead of &tsadc_shut - then the PMIC reset works.
> > > >
> > > > I've recently brought up an RK3588S-based Orange Pi CM5 Base board, made
> > > > of a compute module (CM5, see [1]) and a carrier board (Base, see [2]).
> > > > The carrier board has a reset button which pulls the PMIC_RESET_L signal
> > > > of the CM5 to GND (see page 3 of the schematics in [3]).
> > > >
> > > > With &tsadc_shut_org the reset button has absolutely no effect. With
> > > > &tsadc_shut it resets the board as expected.
> > >
> > > Interesting. The TSADC shouldn't affect the physical button operation
> > > at all, if it's really wired to the PMIC as the signal name implies.
> > > There isn't even any default pull value associated with the TSHUT pin
> > > config.
> >
> > On a second thought, I've got another hypothesis. Your baseboard only
> > pulls the reset line through  a 100 Ohm resistor when the button is
> > pressed. So if the TSHUT pin is in its default push-pull mode and
> > stays high when no thermal runaway reset is requested, the reset
> > button won't pull the line fully to zero, as the TSHUT line pulls it
> > high at the same time.
>
> That's the most likely cause, I agree.
>
> > If you switch it from &tsadc_shut_org to &tsadc_shut, then it stops
> > working properly as the thermal protection reset, and GPIO0_A1 remains
> > high-impendance, thus allowing the reset button to function even
> > though its pull is too weak.
>
> By the way, what is the difference between tsadc_shut_org and tsadc_shut
> ? I haven't seen it being clearly documented in the TRM.

No idea frankly. Looks like a half-finished design change to me, which
left the non-"org" version unconnected internally.

> > So maybe change the pin configuration of &tsadc_shut_org in
> > rk3588-base-pinctrl.dtsi to open drain and retry?
>
> That's a good idea, but... how ? The pinctrl-rockchip driver doesn't
> seem to support generic open-drain configuration.

I thought I saw open-drain configurations here, but after reviewing
the TRM, bindings and the driver it turns out I must have been
daydreaming :( Sorry.

Looks like the best we can try is a lower drive strength while keeping
the push-pull mode, but I'm afraid this 100 Ohm pulldown is too weak,
because the lowest TSHUT drive strength Rockchip offers is 100 Ohm,
while the PMIC would only count anything below 30% reference voltage
as logical low. Maybe adding a pulldown to the pin config can help,
but most likely this board will require switching the pin to GPIO
input for high-z, and switching the TSHUT mode to CRU.

So how about something like this first:

&tsadc_shut_org {
        rockchip,pins = <0 RK_PA1 1 &pcfg_pull_down_drv_level_0>;
};

Best regards,
Alexey

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

[Laurent Pinchart]

On Sat, Oct 04, 2025 at 03:41:41PM +0400, Alexey Charkov wrote:
> On Sat, Oct 4, 2025 at 3:29 AM Laurent Pinchart wrote:
> > On Fri, Oct 03, 2025 at 06:55:26PM +0400, Alexey Charkov wrote:
> > > On Fri, Oct 3, 2025 at 6:13 PM Alexey Charkov wrote:
> > > > On Fri, Oct 3, 2025 at 5:33 PM Laurent Pinchart wrote:
> > > > > On Fri, Jan 24, 2025 at 11:44:34PM +0400, Alexey Charkov wrote:
> > > > > > On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov wrote:
> > > > > > > On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic wrote:
> > > > > > > > On 2025-01-24 11:25, Alexey Charkov wrote:
> > > > > > > > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic wrote:
> > > > > > > > >> On 2025-01-24 09:33, Alexey Charkov wrote:
> > > > > > > > >> > On Fri, Jan 24, 2025 at 9:26 AM 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".
> > > > > > > > >> >
> > > > > > > > >> > 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
> > > > > > >
> > > > > > > > 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.
> > > > > > > >
> > > > > > > > > Will try and revert.
> > > > > > > >
> > > > > > > > Awesome, thanks!
> > > > > > > >
> > > > > > > > > P.S. Just looked at the downstream driver, and it actually calls
> > > > > > > > > TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
> > > > > > > > > not a typo in the first place. So maybe the right approach here is not
> > > > > > > > > to change the device tree but rather fix the "gpio" / "otpout" pinctrl
> > > > > > > > > state handling in the driver.
> > > > > > > >
> > > > > > > > Indeed, "otpout" wasn't a typo, and I've already addressed that in my
> > > > > > > > comments to Alexander's patch.  Will send that response a bit later.
> > > > > > > >
> > > > > > > > I think it's actually better to accept the approach in Alexander's
> > > > > > > > patch, because the whole thing applies to other Rockchip SoCs as well,
> > > > > > > > not just to the RK3588(S).
> > > > > > >
> > > > > > > Anyway, I've just tried it after including the changes below, and
> > > > > > > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
> > > > > > > pinctrls under tsadc, the driver still doesn't seem to be triggering a
> > > > > > > PMIC reset. Weird. Any thoughts welcome.
> > > > > >
> > > > > > I found the culprit. "otpout" (or "default" if we follow Alexander's
> > > > > > suggested approach) pinctrl state should refer to the &tsadc_shut_org
> > > > > > config instead of &tsadc_shut - then the PMIC reset works.
> > > > >
> > > > > I've recently brought up an RK3588S-based Orange Pi CM5 Base board, made
> > > > > of a compute module (CM5, see [1]) and a carrier board (Base, see [2]).
> > > > > The carrier board has a reset button which pulls the PMIC_RESET_L signal
> > > > > of the CM5 to GND (see page 3 of the schematics in [3]).
> > > > >
> > > > > With &tsadc_shut_org the reset button has absolutely no effect. With
> > > > > &tsadc_shut it resets the board as expected.
> > > >
> > > > Interesting. The TSADC shouldn't affect the physical button operation
> > > > at all, if it's really wired to the PMIC as the signal name implies.
> > > > There isn't even any default pull value associated with the TSHUT pin
> > > > config.
> > >
> > > On a second thought, I've got another hypothesis. Your baseboard only
> > > pulls the reset line through  a 100 Ohm resistor when the button is
> > > pressed. So if the TSHUT pin is in its default push-pull mode and
> > > stays high when no thermal runaway reset is requested, the reset
> > > button won't pull the line fully to zero, as the TSHUT line pulls it
> > > high at the same time.
> >
> > That's the most likely cause, I agree.
> >
> > > If you switch it from &tsadc_shut_org to &tsadc_shut, then it stops
> > > working properly as the thermal protection reset, and GPIO0_A1 remains
> > > high-impendance, thus allowing the reset button to function even
> > > though its pull is too weak.
> >
> > By the way, what is the difference between tsadc_shut_org and tsadc_shut
> > ? I haven't seen it being clearly documented in the TRM.
> 
> No idea frankly. Looks like a half-finished design change to me, which
> left the non-"org" version unconnected internally.

:-/

> > > So maybe change the pin configuration of &tsadc_shut_org in
> > > rk3588-base-pinctrl.dtsi to open drain and retry?
> >
> > That's a good idea, but... how ? The pinctrl-rockchip driver doesn't
> > seem to support generic open-drain configuration.
> 
> I thought I saw open-drain configurations here, but after reviewing
> the TRM, bindings and the driver it turns out I must have been
> daydreaming :( Sorry.
> 
> Looks like the best we can try is a lower drive strength while keeping
> the push-pull mode, but I'm afraid this 100 Ohm pulldown is too weak,
> because the lowest TSHUT drive strength Rockchip offers is 100 Ohm,
> while the PMIC would only count anything below 30% reference voltage
> as logical low. Maybe adding a pulldown to the pin config can help,
> but most likely this board will require switching the pin to GPIO
> input for high-z, and switching the TSHUT mode to CRU.

I agree with you, going through the CRU seems the best solution for this
board. This is actually the default mode in
arch/arm64/boot/dts/rockchip/rk3588-base.dtsi:

	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_shut_org>;
	pinctrl-1 = <&tsadc_gpio_func>;

If hw-tshut-mode defaults to 0, why do we need to setup the GPIO0_A1 pin
to output the TSADC_SHUT signal ?

> So how about something like this first:
> 
> &tsadc_shut_org {
>         rockchip,pins = <0 RK_PA1 1 &pcfg_pull_down_drv_level_0>;
> };

I've tested that and it's indeed not enough, the reset button still
doesn't work.

-- 
Regards,

Laurent Pinchart

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

[Alexey Charkov]

On Sun, Oct 5, 2025 at 2:03 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Sat, Oct 04, 2025 at 03:41:41PM +0400, Alexey Charkov wrote:
> > On Sat, Oct 4, 2025 at 3:29 AM Laurent Pinchart wrote:
> > > On Fri, Oct 03, 2025 at 06:55:26PM +0400, Alexey Charkov wrote:
> > > > On Fri, Oct 3, 2025 at 6:13 PM Alexey Charkov wrote:
> > > > > On Fri, Oct 3, 2025 at 5:33 PM Laurent Pinchart wrote:
> > > > > > On Fri, Jan 24, 2025 at 11:44:34PM +0400, Alexey Charkov wrote:
> > > > > > > On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov wrote:
> > > > > > > > On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic wrote:
> > > > > > > > > On 2025-01-24 11:25, Alexey Charkov wrote:
> > > > > > > > > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic wrote:
> > > > > > > > > >> On 2025-01-24 09:33, Alexey Charkov wrote:
> > > > > > > > > >> > On Fri, Jan 24, 2025 at 9:26 AM 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".
> > > > > > > > > >> >
> > > > > > > > > >> > 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
> > > > > > > >
> > > > > > > > > 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.
> > > > > > > > >
> > > > > > > > > > Will try and revert.
> > > > > > > > >
> > > > > > > > > Awesome, thanks!
> > > > > > > > >
> > > > > > > > > > P.S. Just looked at the downstream driver, and it actually calls
> > > > > > > > > > TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
> > > > > > > > > > not a typo in the first place. So maybe the right approach here is not
> > > > > > > > > > to change the device tree but rather fix the "gpio" / "otpout" pinctrl
> > > > > > > > > > state handling in the driver.
> > > > > > > > >
> > > > > > > > > Indeed, "otpout" wasn't a typo, and I've already addressed that in my
> > > > > > > > > comments to Alexander's patch.  Will send that response a bit later.
> > > > > > > > >
> > > > > > > > > I think it's actually better to accept the approach in Alexander's
> > > > > > > > > patch, because the whole thing applies to other Rockchip SoCs as well,
> > > > > > > > > not just to the RK3588(S).
> > > > > > > >
> > > > > > > > Anyway, I've just tried it after including the changes below, and
> > > > > > > > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
> > > > > > > > pinctrls under tsadc, the driver still doesn't seem to be triggering a
> > > > > > > > PMIC reset. Weird. Any thoughts welcome.
> > > > > > >
> > > > > > > I found the culprit. "otpout" (or "default" if we follow Alexander's
> > > > > > > suggested approach) pinctrl state should refer to the &tsadc_shut_org
> > > > > > > config instead of &tsadc_shut - then the PMIC reset works.
> > > > > >
> > > > > > I've recently brought up an RK3588S-based Orange Pi CM5 Base board, made
> > > > > > of a compute module (CM5, see [1]) and a carrier board (Base, see [2]).
> > > > > > The carrier board has a reset button which pulls the PMIC_RESET_L signal
> > > > > > of the CM5 to GND (see page 3 of the schematics in [3]).
> > > > > >
> > > > > > With &tsadc_shut_org the reset button has absolutely no effect. With
> > > > > > &tsadc_shut it resets the board as expected.
> > > > >
> > > > > Interesting. The TSADC shouldn't affect the physical button operation
> > > > > at all, if it's really wired to the PMIC as the signal name implies.
> > > > > There isn't even any default pull value associated with the TSHUT pin
> > > > > config.
> > > >
> > > > On a second thought, I've got another hypothesis. Your baseboard only
> > > > pulls the reset line through  a 100 Ohm resistor when the button is
> > > > pressed. So if the TSHUT pin is in its default push-pull mode and
> > > > stays high when no thermal runaway reset is requested, the reset
> > > > button won't pull the line fully to zero, as the TSHUT line pulls it
> > > > high at the same time.
> > >
> > > That's the most likely cause, I agree.
> > >
> > > > If you switch it from &tsadc_shut_org to &tsadc_shut, then it stops
> > > > working properly as the thermal protection reset, and GPIO0_A1 remains
> > > > high-impendance, thus allowing the reset button to function even
> > > > though its pull is too weak.
> > >
> > > By the way, what is the difference between tsadc_shut_org and tsadc_shut
> > > ? I haven't seen it being clearly documented in the TRM.
> >
> > No idea frankly. Looks like a half-finished design change to me, which
> > left the non-"org" version unconnected internally.
>
> :-/
>
> > > > So maybe change the pin configuration of &tsadc_shut_org in
> > > > rk3588-base-pinctrl.dtsi to open drain and retry?
> > >
> > > That's a good idea, but... how ? The pinctrl-rockchip driver doesn't
> > > seem to support generic open-drain configuration.
> >
> > I thought I saw open-drain configurations here, but after reviewing
> > the TRM, bindings and the driver it turns out I must have been
> > daydreaming :( Sorry.
> >
> > Looks like the best we can try is a lower drive strength while keeping
> > the push-pull mode, but I'm afraid this 100 Ohm pulldown is too weak,
> > because the lowest TSHUT drive strength Rockchip offers is 100 Ohm,
> > while the PMIC would only count anything below 30% reference voltage
> > as logical low. Maybe adding a pulldown to the pin config can help,
> > but most likely this board will require switching the pin to GPIO
> > input for high-z, and switching the TSHUT mode to CRU.
>
> I agree with you, going through the CRU seems the best solution for this
> board. This is actually the default mode in
> arch/arm64/boot/dts/rockchip/rk3588-base.dtsi:
>
>         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_shut_org>;
>         pinctrl-1 = <&tsadc_gpio_func>;
>
> If hw-tshut-mode defaults to 0, why do we need to setup the GPIO0_A1 pin
> to output the TSADC_SHUT signal ?

I believe the thinking was along the lines of "it can't hurt, so let's
provide a default that's likely to work both for the boards where
TSHUT is routed to the PMIC and those where it's not, with an added
benefit of hogging the pin to prevent anyone from accidentally
triggering it to a low level from user space thus suddenly resetting
the board".

But this case of "TSHUT is routed, but with a deviation from the
reference schematic which makes it impossible to use as designed" was
likely never envisaged.

Technically, there is no reason to switch the pin to tsadc_shut_org
when CRU mode is used, and the boottime default for this pin is
high-impedance.

Heiko, shall we remove the pinctrl properties from the common .dtsi
and move them to board specific .dts for those boards that use
PMIC-assisted thermal resets? Happy to produce a patch to that effect.

Best regards,
Alexey

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

[Laurent Pinchart]

On Sun, Oct 05, 2025 at 02:55:07PM +0400, Alexey Charkov wrote:
> On Sun, Oct 5, 2025 at 2:03 AM Laurent Pinchart wrote:
> > On Sat, Oct 04, 2025 at 03:41:41PM +0400, Alexey Charkov wrote:
> > > On Sat, Oct 4, 2025 at 3:29 AM Laurent Pinchart wrote:
> > > > On Fri, Oct 03, 2025 at 06:55:26PM +0400, Alexey Charkov wrote:
> > > > > On Fri, Oct 3, 2025 at 6:13 PM Alexey Charkov wrote:
> > > > > > On Fri, Oct 3, 2025 at 5:33 PM Laurent Pinchart wrote:
> > > > > > > On Fri, Jan 24, 2025 at 11:44:34PM +0400, Alexey Charkov wrote:
> > > > > > > > On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov wrote:
> > > > > > > > > On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic wrote:
> > > > > > > > > > On 2025-01-24 11:25, Alexey Charkov wrote:
> > > > > > > > > > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic wrote:
> > > > > > > > > > >> On 2025-01-24 09:33, Alexey Charkov wrote:
> > > > > > > > > > >> > On Fri, Jan 24, 2025 at 9:26 AM 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".
> > > > > > > > > > >> >
> > > > > > > > > > >> > 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
> > > > > > > > >
> > > > > > > > > > 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.
> > > > > > > > > >
> > > > > > > > > > > Will try and revert.
> > > > > > > > > >
> > > > > > > > > > Awesome, thanks!
> > > > > > > > > >
> > > > > > > > > > > P.S. Just looked at the downstream driver, and it actually calls
> > > > > > > > > > > TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
> > > > > > > > > > > not a typo in the first place. So maybe the right approach here is not
> > > > > > > > > > > to change the device tree but rather fix the "gpio" / "otpout" pinctrl
> > > > > > > > > > > state handling in the driver.
> > > > > > > > > >
> > > > > > > > > > Indeed, "otpout" wasn't a typo, and I've already addressed that in my
> > > > > > > > > > comments to Alexander's patch.  Will send that response a bit later.
> > > > > > > > > >
> > > > > > > > > > I think it's actually better to accept the approach in Alexander's
> > > > > > > > > > patch, because the whole thing applies to other Rockchip SoCs as well,
> > > > > > > > > > not just to the RK3588(S).
> > > > > > > > >
> > > > > > > > > Anyway, I've just tried it after including the changes below, and
> > > > > > > > > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
> > > > > > > > > pinctrls under tsadc, the driver still doesn't seem to be triggering a
> > > > > > > > > PMIC reset. Weird. Any thoughts welcome.
> > > > > > > >
> > > > > > > > I found the culprit. "otpout" (or "default" if we follow Alexander's
> > > > > > > > suggested approach) pinctrl state should refer to the &tsadc_shut_org
> > > > > > > > config instead of &tsadc_shut - then the PMIC reset works.
> > > > > > >
> > > > > > > I've recently brought up an RK3588S-based Orange Pi CM5 Base board, made
> > > > > > > of a compute module (CM5, see [1]) and a carrier board (Base, see [2]).
> > > > > > > The carrier board has a reset button which pulls the PMIC_RESET_L signal
> > > > > > > of the CM5 to GND (see page 3 of the schematics in [3]).
> > > > > > >
> > > > > > > With &tsadc_shut_org the reset button has absolutely no effect. With
> > > > > > > &tsadc_shut it resets the board as expected.
> > > > > >
> > > > > > Interesting. The TSADC shouldn't affect the physical button operation
> > > > > > at all, if it's really wired to the PMIC as the signal name implies.
> > > > > > There isn't even any default pull value associated with the TSHUT pin
> > > > > > config.
> > > > >
> > > > > On a second thought, I've got another hypothesis. Your baseboard only
> > > > > pulls the reset line through  a 100 Ohm resistor when the button is
> > > > > pressed. So if the TSHUT pin is in its default push-pull mode and
> > > > > stays high when no thermal runaway reset is requested, the reset
> > > > > button won't pull the line fully to zero, as the TSHUT line pulls it
> > > > > high at the same time.
> > > >
> > > > That's the most likely cause, I agree.
> > > >
> > > > > If you switch it from &tsadc_shut_org to &tsadc_shut, then it stops
> > > > > working properly as the thermal protection reset, and GPIO0_A1 remains
> > > > > high-impendance, thus allowing the reset button to function even
> > > > > though its pull is too weak.
> > > >
> > > > By the way, what is the difference between tsadc_shut_org and tsadc_shut
> > > > ? I haven't seen it being clearly documented in the TRM.
> > >
> > > No idea frankly. Looks like a half-finished design change to me, which
> > > left the non-"org" version unconnected internally.
> >
> > :-/
> >
> > > > > So maybe change the pin configuration of &tsadc_shut_org in
> > > > > rk3588-base-pinctrl.dtsi to open drain and retry?
> > > >
> > > > That's a good idea, but... how ? The pinctrl-rockchip driver doesn't
> > > > seem to support generic open-drain configuration.
> > >
> > > I thought I saw open-drain configurations here, but after reviewing
> > > the TRM, bindings and the driver it turns out I must have been
> > > daydreaming :( Sorry.
> > >
> > > Looks like the best we can try is a lower drive strength while keeping
> > > the push-pull mode, but I'm afraid this 100 Ohm pulldown is too weak,
> > > because the lowest TSHUT drive strength Rockchip offers is 100 Ohm,
> > > while the PMIC would only count anything below 30% reference voltage
> > > as logical low. Maybe adding a pulldown to the pin config can help,
> > > but most likely this board will require switching the pin to GPIO
> > > input for high-z, and switching the TSHUT mode to CRU.
> >
> > I agree with you, going through the CRU seems the best solution for this
> > board. This is actually the default mode in
> > arch/arm64/boot/dts/rockchip/rk3588-base.dtsi:
> >
> >         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_shut_org>;
> >         pinctrl-1 = <&tsadc_gpio_func>;
> >
> > If hw-tshut-mode defaults to 0, why do we need to setup the GPIO0_A1 pin
> > to output the TSADC_SHUT signal ?
> 
> I believe the thinking was along the lines of "it can't hurt, so let's
> provide a default that's likely to work both for the boards where
> TSHUT is routed to the PMIC and those where it's not, with an added
> benefit of hogging the pin to prevent anyone from accidentally
> triggering it to a low level from user space thus suddenly resetting
> the board".
> 
> But this case of "TSHUT is routed, but with a deviation from the
> reference schematic which makes it impossible to use as designed" was
> likely never envisaged.
> 
> Technically, there is no reason to switch the pin to tsadc_shut_org
> when CRU mode is used, and the boottime default for this pin is
> high-impedance.

I tried to trigger an over-temperature condition by setting
rockchip,hw-tshut-temp to 40°C (yes, it's getting cold in Finland). With
pinctrl-0 set to <&tsadc_shut_org>, the system reset when the trip point
was reached, regardless of rockchip,hw-tshut-mode.

With pinctrl-0 set to <&tsadc_gpio_func>, reaching 40°C caused a reset
with rockchip,hw-tshut-mode set to 0 (CRU), and no action occurred when
rockchip,hw-tshut-mode was set to 1 (GPIO).

I've also tested <&tsadc_shut>. It resulted in a reset in CRU mode and
no action in GPIO mode.

> Heiko, shall we remove the pinctrl properties from the common .dtsi
> and move them to board specific .dts for those boards that use
> PMIC-assisted thermal resets? Happy to produce a patch to that effect.

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

On Fri, Oct 03, 2025 at 06:13:39PM +0400, Alexey Charkov wrote:
> On Fri, Oct 3, 2025 at 5:33 PM Laurent Pinchart wrote:
> > On Fri, Jan 24, 2025 at 11:44:34PM +0400, Alexey Charkov wrote:
> > > On Fri, Jan 24, 2025 at 9:23 PM Alexey Charkov <alchark@gmail.com> wrote:
> > > > On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@manjaro.org> wrote:
> > > > > On 2025-01-24 11:25, Alexey Charkov wrote:
> > > > > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org>
> > > > > > wrote:
> > > > > >> On 2025-01-24 09:33, Alexey Charkov wrote:
> > > > > >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
> > > > > >> > <eagle.alexander923@gmail.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
> > > >
> > > > > 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.
> > > > >
> > > > > > Will try and revert.
> > > > >
> > > > > Awesome, thanks!
> > > > >
> > > > > > P.S. Just looked at the downstream driver, and it actually calls
> > > > > > TSHUT_MODE_GPIO TSHUT_MODE_OTP instead, so it seems that "otpout" was
> > > > > > not a typo in the first place. So maybe the right approach here is not
> > > > > > to change the device tree but rather fix the "gpio" / "otpout" pinctrl
> > > > > > state handling in the driver.
> > > > >
> > > > > Indeed, "otpout" wasn't a typo, and I've already addressed that in my
> > > > > comments to Alexander's patch.  Will send that response a bit later.
> > > > >
> > > > > I think it's actually better to accept the approach in Alexander's
> > > > > patch, because the whole thing applies to other Rockchip SoCs as well,
> > > > > not just to the RK3588(S).
> > > >
> > > > Anyway, I've just tried it after including the changes below, and
> > > > while /sys/kernel/debug/pinctrl/pinctrl-handles shows the expected
> > > > pinctrls under tsadc, the driver still doesn't seem to be triggering a
> > > > PMIC reset. Weird. Any thoughts welcome.
> > >
> > > I found the culprit. "otpout" (or "default" if we follow Alexander's
> > > suggested approach) pinctrl state should refer to the &tsadc_shut_org
> > > config instead of &tsadc_shut - then the PMIC reset works.
> >
> > I've recently brought up an RK3588S-based Orange Pi CM5 Base board, made
> > of a compute module (CM5, see [1]) and a carrier board (Base, see [2]).
> > The carrier board has a reset button which pulls the PMIC_RESET_L signal
> > of the CM5 to GND (see page 3 of the schematics in [3]).
> >
> > With &tsadc_shut_org the reset button has absolutely no effect. With
> > &tsadc_shut it resets the board as expected.
> 
> Interesting. The TSADC shouldn't affect the physical button operation
> at all, if it's really wired to the PMIC as the signal name implies.
> There isn't even any default pull value associated with the TSHUT pin
> config.
> 
> What if you switch the GPIO0_A1 pin to GPIO output mode with no pull?
> Does the button work then? Does the board reset if you toggle the GPIO
> value with `gpioset`?

The button works when GPIO0_A1 is configured as a GPIO input. It doesn't
work anymore when the GPIO is set to 1, and setting the GPIO to 0 resets
the board. This confirms your hypothesis that the reset button
functionality conflicts with TSADC_SHUT being configured as a push-pull
output.

-- 
Regards,

Laurent Pinchart

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

[Dragan Simic]

Hello Alexey,

On 2025-01-24 18:23, Alexey Charkov wrote:
> On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2025-01-24 11:25, Alexey Charkov wrote:
>> > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic@manjaro.org>
>> > wrote:
>> >> On 2025-01-24 09:33, Alexey Charkov wrote:
>> >> > On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
>> >> > <eagle.alexander923@gmail.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.

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

[Dragan Simic]

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@vger.kernel.org
> Signed-off-by: Alexander Shiyan <eagle.alexander923@gmail.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@fec00000 {
>  		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@manjaro.org>

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