Re: [PATCH 2/6] ARM: cpuidle: Add a cpuidle ops structure to be used for DT

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 03/16/2015 07:16 PM, Lorenzo Pieralisi wrote:
On Tue, Mar 03, 2015 at 12:29:33PM +0000, Daniel Lezcano wrote:
The current state of the different cpuidle drivers is the different PM

Nit: "The current state of cpuidle drivers is such that different ..."

Ok.

operations are passed via the platform_data using the platform driver
paradigm.

This approach allowed to split the low level PM code from the arch specific
and the generic cpuidle code.

Unfortunately there are complains about this approach as, in the context of the

Nit: s/complains/complaints

Ok.

[ ... ]

@@ -27,4 +27,14 @@ static inline int arm_cpuidle_simple_enter(struct cpuidle_device *dev,
   */
  #define ARM_CPUIDLE_WFI_STATE ARM_CPUIDLE_WFI_STATE_PWR(UINT_MAX)

+struct cpuidle_ops {
+	const char *name;
+	int (*suspend)(int cpu, unsigned long arg);
+	int (*init)(struct device_node *, int cpu);
+};
+
+extern int arm_cpuidle_suspend(int index);
+
+extern int arm_cpuidle_init(int cpu);

idle_cpu_suspend()
idle_cpu_init()

?

I am really not fussed about the naming.

To make this and x86 driver name compliant (well, function signatures
are a bit different) we could use:

arm_idle()
arm_idle_cpu_init()

even though I think the arch prefix is useless.

Side note: why is the x86 driver in drivers/idle ? To have another dir :) ?

I believe it is there for historical reasons.

[ ... ]

+static struct cpuidle_ops cpuidle_ops[NR_CPUS];

That's because you want platform cpuidle_ops to be __initdata ?

Yes.

It should not be a big overhead on arm32 to have a number of
structs equal to NR_CPUS, on arm64 it is the other way around
there are few cpu_ops, but number of CPUs can be high so it
is an array of pointers.

I think it is ok to leave it as it is (or probably make cpuidle_ops
a single struct, I expect enable-method to be common across cpus).

I prefer to keep per cpu because I am not sure of this assumption.

[ ... ]

+	cpuidle_ops[cpu] = *ops; /* structure copy */

See above.

+
+	pr_notice("cpuidle: enable-method property '%s'"
+		  " found operations\n", ops->name);
+
+	return 0;
+}
+
+int __init arm_cpuidle_init(int cpu)
+{
+	int ret = -EOPNOTSUPP;

Nit: You always assign ret, so there is no point in initializing it.

Ok, I will fix it.

Thanks for reviewing.

  -- Daniel



--
 <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 devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux