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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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


-- 
 <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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel