Re: [PATCH v5 7/8] drivers: cpuidle: initialize Exynos driver through DT

[Mark Rutland <mark.rutland@arm.com>]

On Wed, Jun 25, 2014 at 03:10:20PM +0100, Lorenzo Pieralisi wrote:
> With the introduction of DT based idle states, CPUidle drivers for
> ARM can now initialize idle states data through properties in the device
> tree.
> 
> This patch adds code to the Exynos CPUidle driver to dynamically
> initialize idle states data through the updated device tree source
> files.
> 
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Tomasz Figa <t.figa@samsung.com>
> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> ---
> Compile tested, I am not sure I patched the right dts files, please check.
> 
>  .../devicetree/bindings/arm/exynos/idle-states.txt | 27 ++++++++++++++++++++
>  arch/arm/boot/dts/exynos3250.dtsi                  | 16 ++++++++++++
>  arch/arm/boot/dts/exynos5250.dtsi                  | 15 +++++++++++
>  arch/arm/boot/dts/exynos5410.dtsi                  | 17 +++++++++++++
>  drivers/cpuidle/Kconfig.arm                        |  1 +
>  drivers/cpuidle/cpuidle-exynos.c                   | 29 +++++++++++++---------
>  6 files changed, 93 insertions(+), 12 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/exynos/idle-states.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/exynos/idle-states.txt b/Documentation/devicetree/bindings/arm/exynos/idle-states.txt
> new file mode 100644
> index 0000000..342ecb4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/exynos/idle-states.txt
> @@ -0,0 +1,27 @@
> +idle-states node
> +----------------
> +
> +On Exynos platforms with power management capabilities, the device
> +tree source file must contain the idle-states node[1]. As defined in [1] the
> +idle-states node must contain an entry-method property that for Exynos
> +platforms can be one of:
> +
> +	- "samsung,exynos"

Similarly to the TC2 binding, what does this mean?

What is a kernel expected to do when it sees this entry-method?

Using "samsung,exynos" as the entry-method feels like something that's
going to bite us; it sounds far too wide-reaching.

>  static int exynos_cpuidle_probe(struct platform_device *pdev)
>  {
> -	int ret;
> +	int ret, i;
> +	struct cpuidle_driver *drv = &exynos_idle_driver;
>  
>  	exynos_enter_aftr = (void *)(pdev->dev.platform_data);
>  
> -	ret = cpuidle_register(&exynos_idle_driver, NULL);
> +	drv->cpumask = (struct cpumask *) cpu_possible_mask;

This assignment looks scary to me. Why do we need to do this, and why
are we throwing away the constness of cpu_possible_mask?

Mark.