[PATCH V5 18/20] ARM: exynos: cpuidle: Pass the AFTR callback to the platform_data

[daniel.lezcano@linaro.org (Daniel Lezcano)]

On 05/09/2014 02:02 PM, Tomasz Figa wrote:
> Hi Arnd,
>
> On 09.05.2014 12:56, Arnd Bergmann wrote:
>> On Friday 11 April 2014, Daniel Lezcano wrote:
>>> No more dependency on the arch code. The platform_data field is used to set the
>>> PM callback as the other cpuidle drivers.
>>>
>>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>
>> This has just shown up in linux-next and broken randconfig builds.
>>
>>> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
>>> index fe8dac8..d22f0e4 100644
>>> --- a/arch/arm/mach-exynos/exynos.c
>>> +++ b/arch/arm/mach-exynos/exynos.c
>>> @@ -221,8 +221,9 @@ void exynos_restart(enum reboot_mode mode, const char *cmd)
>>>   }
>>>
>>>   static struct platform_device exynos_cpuidle = {
>>> -       .name           = "exynos_cpuidle",
>>> -       .id             = -1,
>>> +       .name              = "exynos_cpuidle",
>>> +       .dev.platform_data = exynos_enter_aftr,
>>> +       .id                = -1,
>>>   };
>>>
>>
>> This is wrong on many levels, can we please do this properly?
>>
>> * The exynos_enter_aftr function is compiled conditionally, so you can't just
>>    reference it from generic code, or you get a link error.
>
> +1

That is true but still we have a link error without this patch. We 
shouldn't register and declare this structure if CONFIG_PM / 
CONFIG_CPU_IDLE are not set.

>> * 'static struct platform_device ...' has been deprecated for at least a decade,
>>    stop doing that. For any platform devices that get registered, there is
>>    platform_device_register_simple().
>
> +0.5
>
> The missing 0.5 is because you can't pass platform data using
> platform_device_register_simple(). There is
> platform_device_register_resndata(), though.
>
>> * There shouldn't need to be a platform_device to start with, this should all
>>    come from DT. We can't do this on arm64 anyway, so any code that may be
>>    shared between arm32 and arm64 should have proper abstractions.
>
> -1
>
> Exynos cpuidle is not a device on the SoC, so I don't think there is any
> way to represent it in DT. The only thing I could see this is matching
> root node with a central SoC driver that instantiates specific
> subdevices, such as cpufreq and cpuidle, but I don't see any available
> infrastructure for this.

There is a RFC for defining generic idle states [1].

The idea behind using the platform driver framework is to unify the code 
across the different drivers and separate the PM / cpuidle code.

By this way, we can move the different drivers to drivers/cpuidle and 
store them in a single place. That make easier the tracking, the review 
and the maintenance.

I am ok to by using platform_device_register_resndata() but I would 
prefer to do that a bit later by converting the other drivers too. That 
will be easier if we have them grouped in a single directory (this is 
what does this patchset at the end).

As there are some more work based on this patchset and the link error 
could be fixed as an independent patch, I would recommend to 
re-integrate it in the tree as asked by Bartlomiej.

Thanks
   -- Daniel


[1] http://www.spinics.net/lists/arm-kernel/msg328747.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