[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 at gmail.com> wrote:
>
> On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic at manjaro.org> wrote:
> >
> > On 2025-01-24 11:25, Alexey Charkov wrote:
> > > On Fri, Jan 24, 2025 at 2:06 PM Dragan Simic <dsimic at 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 at 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