On Thursday 11 of October 2012 00:21:28 Daniel Lezcano wrote:

The function needs the cpuidle_device which is initially passed to the caller.

The current code gets the struct device from the struct cpuidle_device, pass it the cpuidle_add_sysfs function. This function calls per_cpu(cpuidle_devices, cpu) to get the cpuidle_device.

This patch pass the cpuidle_device instead and simplify the code.

Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org

I like this one. I'll take it for v3.8.

Thanks, Rafael

---

drivers/cpuidle/cpuidle.c | 8 +++----- drivers/cpuidle/cpuidle.h | 6 ++---- drivers/cpuidle/sysfs.c | 12 +++--------- 3 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index e28f6ea..07c6637 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -394,7 +394,6 @@ EXPORT_SYMBOL_GPL(cpuidle_disable_device); static int __cpuidle_register_device(struct cpuidle_device *dev) { int ret;

• struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu); struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();

if (!try_module_get(cpuidle_driver->owner)) @@ -404,7 +403,7 @@ static int __cpuidle_register_device(struct cpuidle_device *dev) per_cpu(cpuidle_devices, dev->cpu) = dev; list_add(&dev->device_list, &cpuidle_detected_devices);

• ret = cpuidle_add_sysfs(cpu_dev);

• ret = cpuidle_add_sysfs(dev); if (ret) goto err_sysfs;

@@ -416,7 +415,7 @@ static int __cpuidle_register_device(struct cpuidle_device *dev) return 0; err_coupled:

• cpuidle_remove_sysfs(cpu_dev);

• cpuidle_remove_sysfs(dev); wait_for_completion(&dev->kobj_unregister);

err_sysfs: list_del(&dev->device_list); @@ -460,7 +459,6 @@ EXPORT_SYMBOL_GPL(cpuidle_register_device); */ void cpuidle_unregister_device(struct cpuidle_device *dev) {

• struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu); struct cpuidle_driver *cpuidle_driver = cpuidle_get_driver();

if (dev->registered == 0) @@ -470,7 +468,7 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) cpuidle_disable_device(dev);

• cpuidle_remove_sysfs(cpu_dev);

• cpuidle_remove_sysfs(dev); list_del(&dev->device_list); wait_for_completion(&dev->kobj_unregister); per_cpu(cpuidle_devices, dev->cpu) = NULL;

diff --git a/drivers/cpuidle/cpuidle.h b/drivers/cpuidle/cpuidle.h index 76e7f69..a5bbd1c 100644 --- a/drivers/cpuidle/cpuidle.h +++ b/drivers/cpuidle/cpuidle.h @@ -5,8 +5,6 @@ #ifndef __DRIVER_CPUIDLE_H #define __DRIVER_CPUIDLE_H -#include <linux/device.h>

/* For internal use only */ extern struct cpuidle_governor *cpuidle_curr_governor; extern struct list_head cpuidle_governors; @@ -29,8 +27,8 @@ extern int cpuidle_add_interface(struct device *dev); extern void cpuidle_remove_interface(struct device *dev); extern int cpuidle_add_state_sysfs(struct cpuidle_device *device); extern void cpuidle_remove_state_sysfs(struct cpuidle_device *device); -extern int cpuidle_add_sysfs(struct device *dev); -extern void cpuidle_remove_sysfs(struct device *dev); +extern int cpuidle_add_sysfs(struct cpuidle_device *dev); +extern void cpuidle_remove_sysfs(struct cpuidle_device *dev); #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED bool cpuidle_state_is_coupled(struct cpuidle_device *dev, diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index 5f809e3..84e6285 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -408,13 +408,11 @@ void cpuidle_remove_state_sysfs(struct cpuidle_device *device)

• cpuidle_add_sysfs - creates a sysfs instance for the target device
• @dev: the target device

*/ -int cpuidle_add_sysfs(struct device *cpu_dev) +int cpuidle_add_sysfs(struct cpuidle_device *dev) {

• int cpu = cpu_dev->id;
• struct cpuidle_device *dev;

• struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu); int error;

• dev = per_cpu(cpuidle_devices, cpu); error = kobject_init_and_add(&dev->kobj, &ktype_cpuidle, &cpu_dev->kobj, "cpuidle"); if (!error)

@@ -426,11 +424,7 @@ int cpuidle_add_sysfs(struct device *cpu_dev)

• cpuidle_remove_sysfs - deletes a sysfs instance on the target device
• @dev: the target device

*/ -void cpuidle_remove_sysfs(struct device *cpu_dev) +void cpuidle_remove_sysfs(struct cpuidle_device *dev) {

• int cpu = cpu_dev->id;
• struct cpuidle_device *dev;
• dev = per_cpu(cpuidle_devices, cpu); kobject_put(&dev->kobj);

}

On Thursday 11 of October 2012 00:21:29 Daniel Lezcano wrote:

Move the kobj initialization and completion in the sysfs.c and encapsulate the code more.

Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org

I like this one too and I'm going to take it for v3.8 as well.

Thanks, Rafael

---

drivers/cpuidle/cpuidle.c | 4 ---- drivers/cpuidle/sysfs.c | 7 +++++-- 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 07c6637..7cf8388 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -399,8 +399,6 @@ static int __cpuidle_register_device(struct cpuidle_device *dev) if (!try_module_get(cpuidle_driver->owner)) return -EINVAL;

• init_completion(&dev->kobj_unregister);
• per_cpu(cpuidle_devices, dev->cpu) = dev; list_add(&dev->device_list, &cpuidle_detected_devices); ret = cpuidle_add_sysfs(dev);

@@ -416,7 +414,6 @@ static int __cpuidle_register_device(struct cpuidle_device *dev) err_coupled: cpuidle_remove_sysfs(dev);

• wait_for_completion(&dev->kobj_unregister);

err_sysfs: list_del(&dev->device_list); per_cpu(cpuidle_devices, dev->cpu) = NULL; @@ -470,7 +467,6 @@ void cpuidle_unregister_device(struct cpuidle_device *dev) cpuidle_remove_sysfs(dev); list_del(&dev->device_list);

• wait_for_completion(&dev->kobj_unregister); per_cpu(cpuidle_devices, dev->cpu) = NULL;

cpuidle_coupled_unregister_device(dev); diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index 84e6285..ed87399 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -374,8 +374,8 @@ int cpuidle_add_state_sysfs(struct cpuidle_device *device) kobj->state_usage = &device->states_usage[i]; init_completion(&kobj->kobj_unregister);

• ret = kobject_init_and_add(&kobj->kobj, &ktype_state_cpuidle, &device->kobj,
  

• 			   "state%d", i);
  

• ret = kobject_init_and_add(&kobj->kobj, &ktype_state_cpuidle,
  

• 			   &device->kobj, "state%d", i);
  

  if (ret) { kfree(kobj); goto error_state;

@@ -413,6 +413,8 @@ int cpuidle_add_sysfs(struct cpuidle_device *dev) struct device *cpu_dev = get_cpu_device((unsigned long)dev->cpu); int error;

• init_completion(&dev->kobj_unregister);
• error = kobject_init_and_add(&dev->kobj, &ktype_cpuidle, &cpu_dev->kobj, "cpuidle"); if (!error)

@@ -427,4 +429,5 @@ int cpuidle_add_sysfs(struct cpuidle_device *dev) void cpuidle_remove_sysfs(struct cpuidle_device *dev) { kobject_put(&dev->kobj);

• wait_for_completion(&dev->kobj_unregister);

}

On Thursday 11 of October 2012 00:21:30 Daniel Lezcano wrote:

The next patchset providing the multiple drivers support will add to the per cpu sysfs driver.

In order to prepare the place this patch creates a single entry function when a cpuidle device is added or removed.

Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org

This is a simple rename, as far as I can say, and it's better to include it into the patchset introducing the support for multiple drivers.

Thanks, Rafael

---

drivers/cpuidle/cpuidle.c | 7 ++++--- drivers/cpuidle/cpuidle.h | 4 ++-- drivers/cpuidle/sysfs.c | 26 ++++++++++++++++++++++---- 3 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 7cf8388..847500b 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -331,7 +331,8 @@ int cpuidle_enable_device(struct cpuidle_device *dev) poll_idle_init(drv);

• if ((ret = cpuidle_add_state_sysfs(dev)))

• ret = cpuidle_add_device_sysfs(dev);
• if (ret) return ret;

if (cpuidle_curr_governor->enable && @@ -352,7 +353,7 @@ int cpuidle_enable_device(struct cpuidle_device *dev) return 0; fail_sysfs:

• cpuidle_remove_state_sysfs(dev);

• cpuidle_remove_device_sysfs(dev);

return ret; } @@ -378,7 +379,7 @@ void cpuidle_disable_device(struct cpuidle_device *dev) if (cpuidle_curr_governor->disable) cpuidle_curr_governor->disable(cpuidle_get_driver(), dev);

• cpuidle_remove_state_sysfs(dev);

• cpuidle_remove_device_sysfs(dev); enabled_devices--;

} diff --git a/drivers/cpuidle/cpuidle.h b/drivers/cpuidle/cpuidle.h index a5bbd1c..1ba1168 100644 --- a/drivers/cpuidle/cpuidle.h +++ b/drivers/cpuidle/cpuidle.h @@ -25,8 +25,8 @@ extern int cpuidle_switch_governor(struct cpuidle_governor *gov); /* sysfs */ extern int cpuidle_add_interface(struct device *dev); extern void cpuidle_remove_interface(struct device *dev); -extern int cpuidle_add_state_sysfs(struct cpuidle_device *device); -extern void cpuidle_remove_state_sysfs(struct cpuidle_device *device); +extern int cpuidle_add_device_sysfs(struct cpuidle_device *device); +extern void cpuidle_remove_device_sysfs(struct cpuidle_device *device); extern int cpuidle_add_sysfs(struct cpuidle_device *dev); extern void cpuidle_remove_sysfs(struct cpuidle_device *dev); diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index ed87399..c25e26e 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -356,10 +356,10 @@ static inline void cpuidle_free_state_kobj(struct cpuidle_device *device, int i) } /**

• • cpuidle_add_driver_sysfs - adds driver-specific sysfs attributes

• • cpuidle_add_state_sysfs - adds cpuidle states sysfs attributes
  • @device: the target device
  */

-int cpuidle_add_state_sysfs(struct cpuidle_device *device) +static int cpuidle_add_state_sysfs(struct cpuidle_device *device) { int i, ret = -ENOMEM; struct cpuidle_state_kobj *kobj; @@ -393,10 +393,10 @@ error_state: } /**

• • cpuidle_remove_driver_sysfs - removes driver-specific sysfs attributes

• • cpuidle_remove_driver_sysfs - removes the cpuidle states sysfs attributes
  • @device: the target device
  */

-void cpuidle_remove_state_sysfs(struct cpuidle_device *device) +static void cpuidle_remove_state_sysfs(struct cpuidle_device *device) { int i; @@ -405,6 +405,24 @@ void cpuidle_remove_state_sysfs(struct cpuidle_device *device) } /**

• • cpuidle_add_device_sysfs - adds device specific sysfs attributes
• • @device: the target device
• */

+int cpuidle_add_device_sysfs(struct cpuidle_device *device) +{

• return cpuidle_add_state_sysfs(device);

+}

+/**

• • cpuidle_remove_device_sysfs : removes device specific sysfs attributes
• • @device : the target device
• */

+void cpuidle_remove_device_sysfs(struct cpuidle_device *device) +{

• cpuidle_remove_state_sysfs(device);

+}

+/**

• cpuidle_add_sysfs - creates a sysfs instance for the target device
• @dev: the target device

*/

On Thursday 11 of October 2012 00:21:31 Daniel Lezcano wrote:

The structure cpuidle_state_kobj is not used anywhere except in the sysfs.c file. The definition of this function is not

^^^^

Surely structure?

needed in the cpuidle header file.

Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org

drivers/cpuidle/sysfs.c | 7 +++++++ include/linux/cpuidle.h | 7 ------- 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/cpuidle/sysfs.c b/drivers/cpuidle/sysfs.c index c25e26e..c756ba4 100644 --- a/drivers/cpuidle/sysfs.c +++ b/drivers/cpuidle/sysfs.c @@ -297,6 +297,13 @@ static struct attribute *cpuidle_state_default_attrs[] = { NULL }; +struct cpuidle_state_kobj {

• struct cpuidle_state *state;
• struct cpuidle_state_usage *state_usage;
• struct completion kobj_unregister;
• struct kobject kobj;

+};

#define kobj_to_state_obj(k) container_of(k, struct cpuidle_state_kobj, kobj) #define kobj_to_state(k) (kobj_to_state_obj(k)->state) #define kobj_to_state_usage(k) (kobj_to_state_obj(k)->state_usage) diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h index 279b1ea..7daf0e3 100644 --- a/include/linux/cpuidle.h +++ b/include/linux/cpuidle.h @@ -82,13 +82,6 @@ cpuidle_set_statedata(struct cpuidle_state_usage *st_usage, void *data) st_usage->driver_data = data; } -struct cpuidle_state_kobj {

• struct cpuidle_state *state;
• struct cpuidle_state_usage *state_usage;
• struct completion kobj_unregister;
• struct kobject kobj;

-};

struct cpuidle_device { unsigned int registered:1; unsigned int enabled:1;