Re: [RFC PATCH 1/8] LED: Add LED Class

[Greg KH <greg@kroah.com>]

On Mon, Dec 05, 2005 at 01:09:26PM +0000, Richard Purdie wrote:
> +static void leds_class_release(struct class_device *dev)
> +{
> +	kfree(dev);
> +}
> +
> +static struct class leds_class = {
> +	.name		= "leds",
> +	.release	= leds_class_release,
> +};

Instead of this, just use class_create().  Then you don't need to
specify a release function at all.

> +/**
> + * leds_device_register - register a new object of led_device class.
> + * @dev: The device to register.
> + * @led_dev: the led_device structure for this device.
> + */
> +int leds_device_register(struct device *dev, struct led_device *led_dev)
> +{
> +	int rc;
> +
> +	led_dev->class_dev = kzalloc (sizeof (struct class_device), GFP_KERNEL);
> +	if (unlikely (!led_dev->class_dev))
> +		return -ENOMEM;
> +
> +	rwlock_init(&led_dev->lock);
> +
> +	led_dev->class_dev->class = &leds_class;
> +	led_dev->class_dev->dev = dev;
> +	led_dev->class_dev->class_data = led_dev;
> +
> +	/* assign this led its name */
> +	strncpy(led_dev->class_dev->class_id, led_dev->name, sizeof(led_dev->class_dev->class_id));
> +
> +	rc = class_device_register (led_dev->class_dev);

Use class_device_create() instead, it's simpler, and will work better
than creating your own class device in the future.

> +	if (unlikely (rc)) {
> +		kfree (led_dev->class_dev);
> +		return rc;
> +	}
> +
> +	/* register the attributes */
> +	class_device_create_file(led_dev->class_dev, &class_device_attr_brightness);
> +
> +	/* add to the list of leds */
> +	write_lock(&leds_list_lock);
> +	list_add_tail(&led_dev->node, &leds_list);
> +	write_unlock(&leds_list_lock);
> +
> +	printk(KERN_INFO "Registered led device: %s\n", led_dev->class_dev->class_id);
> +
> +	return 0;
> +}
> +
> +/**
> + * leds_device_unregister - unregisters a object of led_properties class.
> + * @led_dev: the led device to unreigister
> + *
> + * Unregisters a previously registered via leds_device_register object.
> + */
> +void leds_device_unregister(struct led_device *led_dev)
> +{
> +	class_device_remove_file (led_dev->class_dev, &class_device_attr_brightness);
> +
> +	class_device_unregister(led_dev->class_dev);
> +
> +	write_lock(&leds_list_lock);
> +	list_del(&led_dev->node);
> +	write_unlock(&leds_list_lock);
> +}
> +
> +EXPORT_SYMBOL(leds_device_suspend);
> +EXPORT_SYMBOL(leds_device_resume);
> +EXPORT_SYMBOL(leds_device_register);
> +EXPORT_SYMBOL(leds_device_unregister);

EXPORT_SYMBOL_GPL() perhaps?


> +
> +static int __init leds_init(void)
> +{
> +	return class_register(&leds_class);
> +}
> +
> +static void __exit leds_exit(void)
> +{
> +	class_unregister(&leds_class);

class_destroy() instead, if you use class_create().

Other than these minor things, looks good to me.

thanks,

greg k-h