Re: [PATCH 05/18] cpuidle: make a single register function for all

On 04/10/2013 06:55 PM, Andrew Lunn wrote:

...

+/**

• • cpuidle_register: registers the driver and the cpu devices with the
• • coupled_cpus passed as parameter. This function is used for all common
• • initialization pattern there are in the arch specific drivers. The
• • devices is globally defined in this file.
• • @drv : a valid pointer to a struct cpuidle_driver
• • @coupled_cpus: a cpumask for the coupled states
• • Returns 0 on success, < 0 otherwise
• */

+int cpuidle_register(struct cpuidle_driver *drv,

•      const struct cpumask *const coupled_cpus)
  

+{

• int ret, cpu;
• struct cpuidle_device *device;
• ret = cpuidle_register_driver(drv);
• if (ret) {

• printk(KERN_ERR "failed to register cpuidle driver\n");
  

pr_err()

Ok.

...

• return ret;
  

• }
• for_each_possible_cpu(cpu) {

• device = &per_cpu(cpuidle_dev, cpu);
  

• device->cpu = cpu;
  

+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED

• device->coupled_cpus = *coupled_cpus;
  

+#endif

• ret = cpuidle_register_device(device);
  

• if (!ret)
  

• 	continue;
  

• printk(KERN_ERR "Failed to register cpuidle "
  

•        "device for cpu%d\n", cpu);
  

pr_err() and don't split the message over two lines, it makes it harder for somebody to find with

grep -r "Failed to register cpuidle device for cpu" *

Ok if the line length is under 80 chars.

...

• cpuidle_unregister(drv);
  

• break;
  

• }
• return 0;

You should return an error code, so that the caller can also return an error code. If you look at cpuidle-kirkwood and cpuidle-calxeda, and maybe others, if the registration fails, the probe function returns an error code, as it should. With your change, its always going to return 0, even if it fails.

Right, right :)

it should be 'return ret;'

Thanks for pointing this.

-- Daniel