Re: [RFC PATCH 5/5] ACPI: processor: do update when maximum cooling state changed

From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: Zhang Rui <rui.zhang@intel.com>, linux-pm@vger.kernel.org
Cc: tiwai@suse.de, viresh.kumar@linaro.org
Subject: Re: [RFC PATCH 5/5] ACPI: processor: do update when maximum cooling state changed
Date: Sun, 12 Apr 2020 12:07:48 +0200	[thread overview]
Message-ID: <e264d4a2-b0d9-8adf-b44f-7501c4407029@linaro.org> (raw)
In-Reply-To: <fee68438572796c71089571e0ff0add6cbbd3e0c.camel@intel.com>

Hi Rui,

On 12/04/2020 08:13, Zhang Rui wrote:
> On Fri, 2020-04-10 at 14:10 +0200, Daniel Lezcano wrote:

[ ... ]

>>> And to fix this, it's better to
>>> a. unregister and re-register the cooling device as you suggested.
>>> or
>>> b. introduce an API that updates the cooling device entirely
>>> instead of
>>> statistics table only.
>>>
>>> For either of the above solutions, we'd better to cleanup the code
>>> to 
>>> invoke .get_max_state() during registration/max_state_reset phase,
>>> once, and then always use cached value later.
>>> And plus, if we want to follow solution a), it's better to remove
>>> .get_max_state() callback and use an integer instead so that every
>>> driver knows this limitation.
>>> I'd vote for solution a) if there is no soc thermal driver that may
>>> return dynamic max_states.
>>>
>>> Do I still miss something?
>>
>> I agree for the a) solution too.
>>
>> But regarding the get_max_state() callback being converted to a
>> integer,
>> the driver int3406_thermal.c computes the upper and lower limits
>> which
>> are updated on a INT3406_BRIGHTNESS_LIMITS_CHANGED notification and
>> get_max_state() does uppper - lower.
> 
> Right, this is another case shows that it's better to support dynamic
> max_state.
> IMO, this is not difficult to do. We just need to introduce a new API,
> which reuses the current cdev device, and reset its every thermal
> instance, and update all the thermal zones the cdev is involved.
> what do you think?

I like how the thermal framework is designed but I think there are too
many API for the thermal framework and it deserves a simplification
rather than adding more of them.

There is no place where the get_max_state is cached except in the stats
structure.

In the function thermal_cooling_device_stats_update():

  Is it possible to just compare the 'new_state' parameter with
stats->max_state and if it is greater increase the stats table and
update max_state to the new_state ?

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog