[PATCH v14 03/10] qcom: spm: Add Subsystem Power Manager driver

[Arnd Bergmann]

On Thursday 04 December 2014 09:28:34 Lina Iyer wrote:
> On Thu, Dec 04 2014 at 02:02 -0700, Arnd Bergmann wrote:
> >On Thursday 04 December 2014 09:52:39 Daniel Lezcano wrote:
> >> On 12/03/2014 09:35 PM, Arnd Bergmann wrote:
> >> > On Wednesday 03 December 2014 07:31:22 Lina Iyer wrote:
> >> >>>>> +static int __init qcom_spm_init(void)
> >> >>>>> +{
> >> >>>>> +    int ret;
> >> >>>>> +
> >> >>>>> +    /*
> >> >>>>> +     * cpuidle driver need to registered before the cpuidle device
> >> >>>>> +     * for any cpu. Register the device for the the cpuidle driver.
> >> >>>>> +     */
> >> >>>>> +    ret = platform_device_register(&qcom_cpuidle_drv);
> >> >>>>> +    if (ret)
> >> >>>>> +        return ret;
> >> >>>> Stephen pointed out that we would have the platform device lying around
> >> >>>> on a non-QCOM device when using multi_v7_defconfig.
> >> >>>
> >> >>> Perhaps I am missing the point, but this is not supposed to happen, no ?
> >> >>>
> >> >> This would happen, since the file would compile on multi_v7 and we would
> >> >> initialize and register this device regardless. The cpuidle-qcom.c
> >> >> driver probe would bail out looking for a matching compatible property.
> >> >> So we would not register a cpuidle driver but the device would lay
> >> >> around.
> >> >
> >> > I think the problem is registering a platform_device. I've complained
> >> > about this before, but it still seems to get copied all over the
> >> > place. Please don't do this but have a driver that looks at DT to
> >> > figure out whether to access hardware or not.
> >>
> >> We did this approach but, I can remember why, someone was complaining
> >> about it also
> >>
> >> The platform device/driver paradigm allowed us to split the arch
> >> specific parts by passing the pm ops through the platform data.
> >>
> >> Would make sense to have a single common place for the ARM arch where we
> >> initialize the platform device for cpuidle ?
> >
> >No. It's really not a device, and if you pretend that it is, you get
> >into problems like this.
> 
> Arnd, the problem is that the provides function pointers to the SoC code
> that the cpuilde driver uses to call based on the idle state.
> 
> After much discussions, we came down to using function pointers from
> translating from DT strings, other than using that again, I dont see a
> good way of achieving the ability of cpuidle driver staying a separate
> driver from the SPM driver.
> 
> Daniel, thoughts?

Maybe the problem is trying too hard to separate two things that really
belong together then. In general, the strategy of coming up with subsystems
for a class of devices and them turning platform code into drivers for
that subsystem has worked out really well, but I think for cpufreq, cpuidle
and smp, it really hasn't, and in the third case we haven't even tried
coming up with a subsystem for that reason.

Having all cpuidle code generally live in drivers/cpuidle is still a good
idea IMO, but using a platform device just for the purpose of passing
data between some platform specific code and another platform specific
driver hasn't worked out that well here.

	Arnd