Re: [PATCH V1 08/10] thermal: tegra: add PM support

[Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 2016年01月13日 23:55, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Wed, Jan 13, 2016 at 04:00:43PM +0800, Wei Ni wrote:
>> Add suspend/resume function in soctherm driver.
>> And enable it for Tegra124 and Tegra210.
>>
>> Signed-off-by: Wei Ni <wni-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/thermal/tegra/tegra124_soctherm.c |   9 ++
>>  drivers/thermal/tegra/tegra210_soctherm.c |   9 ++
>>  drivers/thermal/tegra/tegra_soctherm.c    | 154 +++++++++++++++++++++---------
>>  drivers/thermal/tegra/tegra_soctherm.h    |   5 +
>>  4 files changed, 130 insertions(+), 47 deletions(-)
> 
> This all looks like it'd be a better candidate for runtime PM. The
> implementation would be mostly the same, except that you'd need to
> add a few more things to tegra_soctherm_probe() and use the runtime
> PM API to enable/disable the device (clocks, resets, ...). System
> sleep should then be taken care of automatically.

We can not handle it as runtime PM, because this is used to monitor the
temperatures, once they reach to critical value, it need to trigger operations
to protect system, such as shutdown or throttle. If this device suspend in
runtime, then it can't handle these events.

> 
> Another comment irrespective of that...
> 
>> diff --git a/drivers/thermal/tegra/tegra124_soctherm.c b/drivers/thermal/tegra/tegra124_soctherm.c
>> index 13d7f4e3a3d7..04cb419dbabc 100644
>> --- a/drivers/thermal/tegra/tegra124_soctherm.c
>> +++ b/drivers/thermal/tegra/tegra124_soctherm.c
>> @@ -205,6 +205,14 @@ static const struct of_device_id tegra124_soctherm_of_match[] = {
>>  };
>>  MODULE_DEVICE_TABLE(of, tegra_soctherm_of_match);
>>  
>> +#ifdef CONFIG_PM_SLEEP
>> +static SIMPLE_DEV_PM_OPS(tegra124_soctherm_pm,
>> +			 soctherm_suspend, soctherm_resume);
>> +#define TEGRA124_SOC_THERM_PM      (&tegra124_soctherm_pm)
>> +#else
>> +#define TEGRA124_SOC_THERM_PM      NULL
>> +#endif
> 
> I wouldn't bother with the #ifdef, just always define the structure.

Ok, will remove them.

> 
> Thierry
> 
> * Unknown Key
> * 0x7F3EB3A1
>