Re: [PATCH 2/4] cpuidle: define the enter function in the driver structure

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 07/05/2012 10:38 PM, Rafael J. Wysocki wrote:
> On Thursday, July 05, 2012, Daniel Lezcano wrote:
>> We have the state index passed as parameter to the 'enter' function.
>> Most of the drivers assign their 'enter' functions several times in
>> the cpuidle_state structure, as we have the index, we can delegate
>> to the driver to handle their own callback array.
>>
>> That will have the benefit of removing multiple lines of code in the
>> different drivers.
> 
> Hmm. I suppose the cpuidle subsystem was designed the way it was for a reason.
> Among other things, this was to avoid recurrence in callbacks - please see
> acpi_idle_enter_bm() for example.
> 
> Now, if .enter() is moved to the driver structure, it will have to be an
> all-purpose complicated routine calling itself recursively at least in
> some cases.  I'm not quite convinced that would be an improvement.
> 
> On the other hand, I don't see anything wrong with setting several callback
> pointers to the same routine.

Deepthi sent a few months ago a patch moving the per-cpu cpuidle_state
to a single structure stored in the driver. The drivers were modified
and cleanup to take into account this modification.

We saw some regressions like for example the 'disable' which were not
per cpu and has been moved to the cpuidle_state_usage (and IMHO it is a
workaround more than a real fix). And now we have new architectures
(tegra3, big.LITTLE) with different latencies per cpu. Logically we
should revert Deepthi's patches but from my POV, going back and forth is
not a good solution (also we have to undo all modifications done in the
different drivers).

The main purpose of all these cleanup patches are to move out all
non-data information from the cpuidle_state structure in order to add a
new api which could be 'cpuidle_register_cpu_latency(int cpu, struct
cpuidle_latencies latencies)'.

For this specific patch, the 'enter' function for all the drivers is not
used [1] and one of the driver is about to use a single function [2]. So
we have only one driver is a couple of functions for this which can be
replaced by an array of callbacks in the driver itself as we have the index.

[1] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012355.html
[2] http://lists.linaro.org/pipermail/linaro-dev/2012-June/012399.html


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

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html