Hello Alexey,
On 2025-01-24 18:23, Alexey Charkov wrote:
On Fri, Jan 24, 2025 at 2:37 PM Dragan Simic <dsimic@xxxxxxxxxxx>
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,
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.
