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@xxxxxxxxxxx>
wrote:
On 2025-01-24 09:33, Alexey Charkov wrote:
> On Fri, Jan 24, 2025 at 9:26 AM Alexander Shiyan
> <eagle.alexander923@xxxxxxxxx> 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).
