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

[Dragan Simic <dsimic@manjaro.org>]

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).

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-rockchip