Git backlight subsystem tree

Git backlight subsystem tree

[Richard Purdie]

I'm thinking of more officially volunteering to maintain the Linux
backlight class/subsystem and have been trying to sort out the various
pending patches. I've made a backlight tree available as:

http://git.o-hand.com/?p=linux-rpurdie-backlight;a=shortlog;h=for-mm

(or git://git.o-hand.com/linux-rpurdie-backlight)

This has a for-mm branch containing the backlight patches I've sorted
out so far. I still have some others left to deal with which are
probably more invasive and will need discussion and testing which I'll
send some mails about soon.

I'm planning to setup an LED drivers/class tree too, I have a couple of
pending patches for that.

Cheers,

Richard

Re: Git backlight subsystem tree

[Andrew Morton]

On Thu, 08 Feb 2007 02:30:26 +0000 Richard Purdie <rpurdie@rpsys.net> wrote:

> I'm thinking of more officially volunteering to maintain the Linux
> backlight class/subsystem and have been trying to sort out the various
> pending patches. I've made a backlight tree available as:
> 
> http://git.o-hand.com/?p=linux-rpurdie-backlight;a=shortlog;h=for-mm
> 
> (or git://git.o-hand.com/linux-rpurdie-backlight)
> 
> This has a for-mm branch containing the backlight patches I've sorted
> out so far. I still have some others left to deal with which are
> probably more invasive and will need discussion and testing which I'll
> send some mails about soon.

No probs,  I added git://git.o-hand.com/linux-rpurdie-backlight#for-mm to
-mm as git-backlight.patch.

> I'm planning to setup an LED drivers/class tree too, I have a couple of
> pending patches for that.

ooh, patches!

Re: Git backlight subsystem tree

[James Simmons]

I have some patches that move the backlight away from using the class 
stuff. The only problem is the patch requires all backlight devices 
to be linked to a real struct device. Right now the acpi backligths are 
not.

Signed-Off: James Simmons <jsimmons@infradead.org>

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 9601bfe..b56fc33 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -14,6 +14,8 @@
 #include <linux/err.h>
 #include <linux/fb.h>
 
+struct class *backlight_class;
+static int index;
 
 #if defined(CONFIG_FB) || (defined(CONFIG_FB_MODULE) && \
 			   defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE))
@@ -67,10 +69,11 @@ static inline void backlight_unregister_fb(struct backlight_device *bd)
 }
 #endif /* CONFIG_FB */
 
-static ssize_t backlight_show_power(struct class_device *cdev, char *buf)
+static ssize_t backlight_show_power(struct device *dev, struct device_attribute *attr,
+					char *buf)
 {
 	int rc = -ENXIO;
-	struct backlight_device *bd = to_backlight_device(cdev);
+	struct backlight_device *bd = dev_get_drvdata(dev);
 
 	down(&bd->sem);
 	if (likely(bd->props))
@@ -80,11 +83,12 @@ static ssize_t backlight_show_power(struct class_device *cdev, char *buf)
 	return rc;
 }
 
-static ssize_t backlight_store_power(struct class_device *cdev, const char *buf, size_t count)
+static ssize_t backlight_store_power(struct device *dev, struct device_attribute *attr,
+					const char *buf, size_t count)
 {
 	int rc = -ENXIO;
 	char *endp;
-	struct backlight_device *bd = to_backlight_device(cdev);
+	struct backlight_device *bd = dev_get_drvdata(dev);
 	int power = simple_strtoul(buf, &endp, 0);
 	size_t size = endp - buf;
 
@@ -106,10 +110,11 @@ static ssize_t backlight_store_power(struct class_device *cdev, const char *buf,
 	return rc;
 }
 
-static ssize_t backlight_show_brightness(struct class_device *cdev, char *buf)
+static ssize_t backlight_show_brightness(struct device *dev, struct device_attribute *attr,
+						char *buf)
 {
 	int rc = -ENXIO;
-	struct backlight_device *bd = to_backlight_device(cdev);
+	struct backlight_device *bd = dev_get_drvdata(dev);
 
 	down(&bd->sem);
 	if (likely(bd->props))
@@ -119,11 +124,12 @@ static ssize_t backlight_show_brightness(struct class_device *cdev, char *buf)
 	return rc;
 }
 
-static ssize_t backlight_store_brightness(struct class_device *cdev, const char *buf, size_t count)
+static ssize_t backlight_store_brightness(struct device *dev, struct device_attribute *attr,
+						const char *buf, size_t count)
 {
 	int rc = -ENXIO;
 	char *endp;
-	struct backlight_device *bd = to_backlight_device(cdev);
+	struct backlight_device *bd = dev_get_drvdata(dev);
 	int brightness = simple_strtoul(buf, &endp, 0);
 	size_t size = endp - buf;
 
@@ -150,10 +156,11 @@ static ssize_t backlight_store_brightness(struct class_device *cdev, const char
 	return rc;
 }
 
-static ssize_t backlight_show_max_brightness(struct class_device *cdev, char *buf)
+static ssize_t backlight_show_max_brightness(struct device *dev, struct device_attribute *attr,
+						char *buf)
 {
 	int rc = -ENXIO;
-	struct backlight_device *bd = to_backlight_device(cdev);
+	struct backlight_device *bd = dev_get_drvdata(dev);
 
 	down(&bd->sem);
 	if (likely(bd->props))
@@ -163,11 +170,11 @@ static ssize_t backlight_show_max_brightness(struct class_device *cdev, char *bu
 	return rc;
 }
 
-static ssize_t backlight_show_actual_brightness(struct class_device *cdev,
+static ssize_t backlight_show_actual_brightness(struct device *dev, struct device_attribute *attr,
 						char *buf)
 {
+	struct backlight_device *bd = dev_get_drvdata(dev);
 	int rc = -ENXIO;
-	struct backlight_device *bd = to_backlight_device(cdev);
 
 	down(&bd->sem);
 	if (likely(bd->props && bd->props->get_brightness))
@@ -177,31 +184,12 @@ static ssize_t backlight_show_actual_brightness(struct class_device *cdev,
 	return rc;
 }
 
-static void backlight_class_release(struct class_device *dev)
-{
-	struct backlight_device *bd = to_backlight_device(dev);
-	kfree(bd);
-}
-
-static struct class backlight_class = {
-	.name = "backlight",
-	.release = backlight_class_release,
-};
-
-#define DECLARE_ATTR(_name,_mode,_show,_store)			\
-{							 	\
-	.attr	= { .name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },	\
-	.show	= _show,					\
-	.store	= _store,					\
-}
-
-static const struct class_device_attribute bl_class_device_attributes[] = {
-	DECLARE_ATTR(power, 0644, backlight_show_power, backlight_store_power),
-	DECLARE_ATTR(brightness, 0644, backlight_show_brightness,
-		     backlight_store_brightness),
-	DECLARE_ATTR(actual_brightness, 0444, backlight_show_actual_brightness,
-		     NULL),
-	DECLARE_ATTR(max_brightness, 0444, backlight_show_max_brightness, NULL),
+static struct device_attribute bl_class_device_attributes[] = {
+	__ATTR(power, S_IRUGO | S_IWUSR, backlight_show_power, backlight_store_power),
+	__ATTR(brightness, S_IRUGO | S_IWUSR, backlight_show_brightness,
+		backlight_store_brightness),
+	__ATTR(actual_brightness, S_IRUGO, backlight_show_actual_brightness, NULL),
+	__ATTR(max_brightness, S_IRUGO, backlight_show_max_brightness, NULL),
 };
 
 /**
@@ -217,12 +205,11 @@ static const struct class_device_attribute bl_class_device_attributes[] = {
  * ERR_PTR() or a pointer to the newly allocated device.
  */
 struct backlight_device *backlight_device_register(const char *name,
-	struct device *dev,
-	void *devdata,
+	struct device *parent, void *devdata,
 	struct backlight_properties *bp)
 {
-	int i, rc;
 	struct backlight_device *new_bd;
+	int rc;
 
 	pr_debug("backlight_device_alloc: name=%s\n", name);
 
@@ -230,37 +217,21 @@ struct backlight_device *backlight_device_register(const char *name,
 	if (unlikely(!new_bd))
 		return ERR_PTR(-ENOMEM);
 
+	new_bd->device = device_create(backlight_class, parent, 0,
+					name, index++);
+	if (IS_ERR(new_bd->device)) {
+		new_bd->device = NULL;
+		return ERR_PTR(-EINVAL);
+	}
+
 	init_MUTEX(&new_bd->sem);
+	dev_set_drvdata(new_bd->device, devdata);
 	new_bd->props = bp;
-	memset(&new_bd->class_dev, 0, sizeof(new_bd->class_dev));
-	new_bd->class_dev.class = &backlight_class;
-	new_bd->class_dev.dev = dev;
-	strlcpy(new_bd->class_dev.class_id, name, KOBJ_NAME_LEN);
-	class_set_devdata(&new_bd->class_dev, devdata);
 
-	rc = class_device_register(&new_bd->class_dev);
+	rc = backlight_register_fb(new_bd);
 	if (unlikely(rc)) {
-error:		kfree(new_bd);
-		return ERR_PTR(rc);
-	}
 
-	rc = backlight_register_fb(new_bd);
-	if (unlikely(rc))
-		goto error;
-
-	for (i = 0; i < ARRAY_SIZE(bl_class_device_attributes); i++) {
-		rc = class_device_create_file(&new_bd->class_dev,
-					      &bl_class_device_attributes[i]);
-		if (unlikely(rc)) {
-			while (--i >= 0)
-				class_device_remove_file(&new_bd->class_dev,
-							 &bl_class_device_attributes[i]);
-			class_device_unregister(&new_bd->class_dev);
-			/* No need to kfree(new_bd) since release() method was called */
-			return ERR_PTR(rc);
-		}
 	}
-
 	return new_bd;
 }
 EXPORT_SYMBOL(backlight_device_register);
@@ -273,35 +244,36 @@ EXPORT_SYMBOL(backlight_device_register);
  */
 void backlight_device_unregister(struct backlight_device *bd)
 {
-	int i;
-
 	if (!bd)
 		return;
 
-	pr_debug("backlight_device_unregister: name=%s\n", bd->class_dev.class_id);
-
-	for (i = 0; i < ARRAY_SIZE(bl_class_device_attributes); i++)
-		class_device_remove_file(&bd->class_dev,
-					 &bl_class_device_attributes[i]);
+	pr_debug("backlight_device_unregister\n");
 
 	down(&bd->sem);
 	bd->props = NULL;
 	up(&bd->sem);
 
 	backlight_unregister_fb(bd);
-
-	class_device_unregister(&bd->class_dev);
+	dev_set_drvdata(bd->device, NULL);
+	device_unregister(bd->device);
 }
 EXPORT_SYMBOL(backlight_device_unregister);
 
 static void __exit backlight_class_exit(void)
 {
-	class_unregister(&backlight_class);
+	class_destroy(backlight_class);
 }
 
 static int __init backlight_class_init(void)
 {
-	return class_register(&backlight_class);
+	backlight_class = class_create(THIS_MODULE, "backlight");
+	if (IS_ERR(backlight_class)) {
+		printk(KERN_WARNING "Unable to create backlight class; errno = %ld\n",
+			PTR_ERR(backlight_class));
+		backlight_class = NULL;
+	} else
+		backlight_class->dev_attrs = bl_class_device_attributes;
+	return 0;
 }
 
 /*
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index a5cf1be..d593266 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -49,8 +49,10 @@ struct backlight_device {
 	struct backlight_properties *props;
 	/* The framebuffer notifier block */
 	struct notifier_block fb_notif;
-	/* The class device structure */
-	struct class_device class_dev;
+	/* Parent device */
+	struct device *parent;
+	/* The device structure */
+	struct device *device;
 };
 
 extern struct backlight_device *backlight_device_register(const char *name,

Re: Git backlight subsystem tree

[Richard Purdie]

On Thu, 2007-02-08 at 15:28 +0000, James Simmons wrote:
> I have some patches that move the backlight away from using the class 
> stuff. The only problem is the patch requires all backlight devices 
> to be linked to a real struct device. Right now the acpi backligths are 
> not.

Why would you want to do that?

The whole point of having this is so that backlights appear as a
standard interface under /sys/class/backlight.

An example of why standardised interfaces are good would be someone
writing an applet for a handheld to control the backlight brightness.
With the class in place, the applet can easily work with any backlight.
Without it, it has to be written for each backlight.

So this is a very strong NAK but I'm curious why you'd want to do it...

Richard

Re: Git backlight subsystem tree

[James Simmons]

On Thu, 8 Feb 2007, Richard Purdie wrote:

> On Thu, 2007-02-08 at 15:28 +0000, James Simmons wrote:
> > I have some patches that move the backlight away from using the class 
> > stuff. The only problem is the patch requires all backlight devices 
> > to be linked to a real struct device. Right now the acpi backligths are 
> > not.
> 
> Why would you want to do that?
> 
> The whole point of having this is so that backlights appear as a
> standard interface under /sys/class/backlight.
> 
> An example of why standardised interfaces are good would be someone
> writing an applet for a handheld to control the backlight brightness.
> With the class in place, the applet can easily work with any backlight.
> Without it, it has to be written for each backlight.
> 
> So this is a very strong NAK but I'm curious why you'd want to do it...

I CC Greg to explain. The backlight class didn't go away. The way it is 
handled is different.

Re: Git backlight subsystem tree

[Richard Purdie]

On Thu, 2007-02-08 at 18:32 +0000, James Simmons wrote:
> On Thu, 8 Feb 2007, Richard Purdie wrote:
> 
> > On Thu, 2007-02-08 at 15:28 +0000, James Simmons wrote:
> > > I have some patches that move the backlight away from using the class 
> > > stuff. The only problem is the patch requires all backlight devices 
> > > to be linked to a real struct device. Right now the acpi backligths are 
> > > not.
> > 
> > Why would you want to do that?
[...]
>
> I CC Greg to explain. The backlight class didn't go away. The way it is 
> handled is different.

>From the changelog and a quick skim though the patch it looked like you
were just removing the class functionality and using the struct device
directly but I see what you're trying to do now.

I'm not sure I agree with it though as it results in two devices for
each backlight user as almost all users have an existing struct device.
The current approach allows you to attach the class to an existing
device and also means you can attach multiple classes to a given device
which gives more flexibility.

So the above question stills stands, why would you want to do this
(apart from removing some code)?

Richard

Re: Git backlight subsystem tree

[Greg KH]

On Thu, Feb 08, 2007 at 06:32:02PM +0000, James Simmons wrote:
> On Thu, 8 Feb 2007, Richard Purdie wrote:
> 
> > On Thu, 2007-02-08 at 15:28 +0000, James Simmons wrote:
> > > I have some patches that move the backlight away from using the class 
> > > stuff. The only problem is the patch requires all backlight devices 
> > > to be linked to a real struct device. Right now the acpi backligths are 
> > > not.
> > 
> > Why would you want to do that?
> > 
> > The whole point of having this is so that backlights appear as a
> > standard interface under /sys/class/backlight.
> > 
> > An example of why standardised interfaces are good would be someone
> > writing an applet for a handheld to control the backlight brightness.
> > With the class in place, the applet can easily work with any backlight.
> > Without it, it has to be written for each backlight.
> > 
> > So this is a very strong NAK but I'm curious why you'd want to do it...
> 
> I CC Greg to explain. The backlight class didn't go away. The way it is 
> handled is different.

Have a pointer to the patch so I can help explain better?

As a short summary, 'struct class_device' is going away.  Using a
'struct device' in its place is what the conversion should have just
done, no functionality change otherwise.

thanks,

greg k-h

Re: Git backlight subsystem tree

[James Simmons]

> On Thu, Feb 08, 2007 at 06:32:02PM +0000, James Simmons wrote:
> > On Thu, 8 Feb 2007, Richard Purdie wrote:
> > 
> > > On Thu, 2007-02-08 at 15:28 +0000, James Simmons wrote:
> > > > I have some patches that move the backlight away from using the class 
> > > > stuff. The only problem is the patch requires all backlight devices 
> > > > to be linked to a real struct device. Right now the acpi backligths are 
> > > > not.
> > > 
> > > Why would you want to do that?
> > > 
> > > The whole point of having this is so that backlights appear as a
> > > standard interface under /sys/class/backlight.
> > > 
> > > An example of why standardised interfaces are good would be someone
> > > writing an applet for a handheld to control the backlight brightness.
> > > With the class in place, the applet can easily work with any backlight.
> > > Without it, it has to be written for each backlight.
> > > 
> > > So this is a very strong NAK but I'm curious why you'd want to do it...
> > 
> > I CC Greg to explain. The backlight class didn't go away. The way it is 
> > handled is different.
> 
> Have a pointer to the patch so I can help explain better?
> 
> As a short summary, 'struct class_device' is going away.  Using a
> 'struct device' in its place is what the conversion should have just
> done, no functionality change otherwise.

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 9601bfe..b56fc33 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -14,6 +14,8 @@
 #include <linux/err.h>
 #include <linux/fb.h>
 
+struct class *backlight_class;
+static int index;
 
 #if defined(CONFIG_FB) || (defined(CONFIG_FB_MODULE) && \
 			   defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE))
@@ -67,10 +69,11 @@ static inline void backlight_unregister_fb(struct backlight_device *bd)
 }
 #endif /* CONFIG_FB */
 
-static ssize_t backlight_show_power(struct class_device *cdev, char *buf)
+static ssize_t backlight_show_power(struct device *dev, struct device_attribute *attr,
+					char *buf)
 {
 	int rc = -ENXIO;
-	struct backlight_device *bd = to_backlight_device(cdev);
+	struct backlight_device *bd = dev_get_drvdata(dev);
 
 	down(&bd->sem);
 	if (likely(bd->props))
@@ -80,11 +83,12 @@ static ssize_t backlight_show_power(struct class_device *cdev, char *buf)
 	return rc;
 }
 
-static ssize_t backlight_store_power(struct class_device *cdev, const char *buf, size_t count)
+static ssize_t backlight_store_power(struct device *dev, struct device_attribute *attr,
+					const char *buf, size_t count)
 {
 	int rc = -ENXIO;
 	char *endp;
-	struct backlight_device *bd = to_backlight_device(cdev);
+	struct backlight_device *bd = dev_get_drvdata(dev);
 	int power = simple_strtoul(buf, &endp, 0);
 	size_t size = endp - buf;
 
@@ -106,10 +110,11 @@ static ssize_t backlight_store_power(struct class_device *cdev, const char *buf,
 	return rc;
 }
 
-static ssize_t backlight_show_brightness(struct class_device *cdev, char *buf)
+static ssize_t backlight_show_brightness(struct device *dev, struct device_attribute *attr,
+						char *buf)
 {
 	int rc = -ENXIO;
-	struct backlight_device *bd = to_backlight_device(cdev);
+	struct backlight_device *bd = dev_get_drvdata(dev);
 
 	down(&bd->sem);
 	if (likely(bd->props))
@@ -119,11 +124,12 @@ static ssize_t backlight_show_brightness(struct class_device *cdev, char *buf)
 	return rc;
 }
 
-static ssize_t backlight_store_brightness(struct class_device *cdev, const char *buf, size_t count)
+static ssize_t backlight_store_brightness(struct device *dev, struct device_attribute *attr,
+						const char *buf, size_t count)
 {
 	int rc = -ENXIO;
 	char *endp;
-	struct backlight_device *bd = to_backlight_device(cdev);
+	struct backlight_device *bd = dev_get_drvdata(dev);
 	int brightness = simple_strtoul(buf, &endp, 0);
 	size_t size = endp - buf;
 
@@ -150,10 +156,11 @@ static ssize_t backlight_store_brightness(struct class_device *cdev, const char
 	return rc;
 }
 
-static ssize_t backlight_show_max_brightness(struct class_device *cdev, char *buf)
+static ssize_t backlight_show_max_brightness(struct device *dev, struct device_attribute *attr,
+						char *buf)
 {
 	int rc = -ENXIO;
-	struct backlight_device *bd = to_backlight_device(cdev);
+	struct backlight_device *bd = dev_get_drvdata(dev);
 
 	down(&bd->sem);
 	if (likely(bd->props))
@@ -163,11 +170,11 @@ static ssize_t backlight_show_max_brightness(struct class_device *cdev, char *bu
 	return rc;
 }
 
-static ssize_t backlight_show_actual_brightness(struct class_device *cdev,
+static ssize_t backlight_show_actual_brightness(struct device *dev, struct device_attribute *attr,
 						char *buf)
 {
+	struct backlight_device *bd = dev_get_drvdata(dev);
 	int rc = -ENXIO;
-	struct backlight_device *bd = to_backlight_device(cdev);
 
 	down(&bd->sem);
 	if (likely(bd->props && bd->props->get_brightness))
@@ -177,31 +184,12 @@ static ssize_t backlight_show_actual_brightness(struct class_device *cdev,
 	return rc;
 }
 
-static void backlight_class_release(struct class_device *dev)
-{
-	struct backlight_device *bd = to_backlight_device(dev);
-	kfree(bd);
-}
-
-static struct class backlight_class = {
-	.name = "backlight",
-	.release = backlight_class_release,
-};
-
-#define DECLARE_ATTR(_name,_mode,_show,_store)			\
-{							 	\
-	.attr	= { .name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },	\
-	.show	= _show,					\
-	.store	= _store,					\
-}
-
-static const struct class_device_attribute bl_class_device_attributes[] = {
-	DECLARE_ATTR(power, 0644, backlight_show_power, backlight_store_power),
-	DECLARE_ATTR(brightness, 0644, backlight_show_brightness,
-		     backlight_store_brightness),
-	DECLARE_ATTR(actual_brightness, 0444, backlight_show_actual_brightness,
-		     NULL),
-	DECLARE_ATTR(max_brightness, 0444, backlight_show_max_brightness, NULL),
+static struct device_attribute bl_class_device_attributes[] = {
+	__ATTR(power, S_IRUGO | S_IWUSR, backlight_show_power, backlight_store_power),
+	__ATTR(brightness, S_IRUGO | S_IWUSR, backlight_show_brightness,
+		backlight_store_brightness),
+	__ATTR(actual_brightness, S_IRUGO, backlight_show_actual_brightness, NULL),
+	__ATTR(max_brightness, S_IRUGO, backlight_show_max_brightness, NULL),
 };
 
 /**
@@ -217,12 +205,11 @@ static const struct class_device_attribute bl_class_device_attributes[] = {
  * ERR_PTR() or a pointer to the newly allocated device.
  */
 struct backlight_device *backlight_device_register(const char *name,
-	struct device *dev,
-	void *devdata,
+	struct device *parent, void *devdata,
 	struct backlight_properties *bp)
 {
-	int i, rc;
 	struct backlight_device *new_bd;
+	int rc;
 
 	pr_debug("backlight_device_alloc: name=%s\n", name);
 
@@ -230,37 +217,21 @@ struct backlight_device *backlight_device_register(const char *name,
 	if (unlikely(!new_bd))
 		return ERR_PTR(-ENOMEM);
 
+	new_bd->device = device_create(backlight_class, parent, 0,
+					name, index++);
+	if (IS_ERR(new_bd->device)) {
+		new_bd->device = NULL;
+		return ERR_PTR(-EINVAL);
+	}
+
 	init_MUTEX(&new_bd->sem);
+	dev_set_drvdata(new_bd->device, devdata);
 	new_bd->props = bp;
-	memset(&new_bd->class_dev, 0, sizeof(new_bd->class_dev));
-	new_bd->class_dev.class = &backlight_class;
-	new_bd->class_dev.dev = dev;
-	strlcpy(new_bd->class_dev.class_id, name, KOBJ_NAME_LEN);
-	class_set_devdata(&new_bd->class_dev, devdata);
 
-	rc = class_device_register(&new_bd->class_dev);
+	rc = backlight_register_fb(new_bd);
 	if (unlikely(rc)) {
-error:		kfree(new_bd);
-		return ERR_PTR(rc);
-	}
 
-	rc = backlight_register_fb(new_bd);
-	if (unlikely(rc))
-		goto error;
-
-	for (i = 0; i < ARRAY_SIZE(bl_class_device_attributes); i++) {
-		rc = class_device_create_file(&new_bd->class_dev,
-					      &bl_class_device_attributes[i]);
-		if (unlikely(rc)) {
-			while (--i >= 0)
-				class_device_remove_file(&new_bd->class_dev,
-							 &bl_class_device_attributes[i]);
-			class_device_unregister(&new_bd->class_dev);
-			/* No need to kfree(new_bd) since release() method was called */
-			return ERR_PTR(rc);
-		}
 	}
-
 	return new_bd;
 }
 EXPORT_SYMBOL(backlight_device_register);
@@ -273,35 +244,36 @@ EXPORT_SYMBOL(backlight_device_register);
  */
 void backlight_device_unregister(struct backlight_device *bd)
 {
-	int i;
-
 	if (!bd)
 		return;
 
-	pr_debug("backlight_device_unregister: name=%s\n", bd->class_dev.class_id);
-
-	for (i = 0; i < ARRAY_SIZE(bl_class_device_attributes); i++)
-		class_device_remove_file(&bd->class_dev,
-					 &bl_class_device_attributes[i]);
+	pr_debug("backlight_device_unregister\n");
 
 	down(&bd->sem);
 	bd->props = NULL;
 	up(&bd->sem);
 
 	backlight_unregister_fb(bd);
-
-	class_device_unregister(&bd->class_dev);
+	dev_set_drvdata(bd->device, NULL);
+	device_unregister(bd->device);
 }
 EXPORT_SYMBOL(backlight_device_unregister);
 
 static void __exit backlight_class_exit(void)
 {
-	class_unregister(&backlight_class);
+	class_destroy(backlight_class);
 }
 
 static int __init backlight_class_init(void)
 {
-	return class_register(&backlight_class);
+	backlight_class = class_create(THIS_MODULE, "backlight");
+	if (IS_ERR(backlight_class)) {
+		printk(KERN_WARNING "Unable to create backlight class; errno = %ld\n",
+			PTR_ERR(backlight_class));
+		backlight_class = NULL;
+	} else
+		backlight_class->dev_attrs = bl_class_device_attributes;
+	return 0;
 }
 
 /*
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index a5cf1be..d593266 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -49,8 +49,10 @@ struct backlight_device {
 	struct backlight_properties *props;
 	/* The framebuffer notifier block */
 	struct notifier_block fb_notif;
-	/* The class device structure */
-	struct class_device class_dev;
+	/* Parent device */
+	struct device *parent;
+	/* The device structure */
+	struct device *device;
 };
 
 extern struct backlight_device *backlight_device_register(const char *name,

Re: Git backlight subsystem tree

[Greg KH]

On Thu, Feb 08, 2007 at 09:54:21PM +0000, James Simmons wrote:
> 
> > On Thu, Feb 08, 2007 at 06:32:02PM +0000, James Simmons wrote:
> > > On Thu, 8 Feb 2007, Richard Purdie wrote:
> > > 
> > > > On Thu, 2007-02-08 at 15:28 +0000, James Simmons wrote:
> > > > > I have some patches that move the backlight away from using the class 
> > > > > stuff. The only problem is the patch requires all backlight devices 
> > > > > to be linked to a real struct device. Right now the acpi backligths are 
> > > > > not.
> > > > 
> > > > Why would you want to do that?
> > > > 
> > > > The whole point of having this is so that backlights appear as a
> > > > standard interface under /sys/class/backlight.
> > > > 
> > > > An example of why standardised interfaces are good would be someone
> > > > writing an applet for a handheld to control the backlight brightness.
> > > > With the class in place, the applet can easily work with any backlight.
> > > > Without it, it has to be written for each backlight.
> > > > 
> > > > So this is a very strong NAK but I'm curious why you'd want to do it...
> > > 
> > > I CC Greg to explain. The backlight class didn't go away. The way it is 
> > > handled is different.
> > 
> > Have a pointer to the patch so I can help explain better?
> > 
> > As a short summary, 'struct class_device' is going away.  Using a
> > 'struct device' in its place is what the conversion should have just
> > done, no functionality change otherwise.
> 
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c

<snip>

Looks good to me.  And it makes the code simpler too :)

thanks,

greg k-h

Re: Git backlight subsystem tree

[Richard Purdie]

Hi Greg,

On Thu, 2007-02-08 at 13:23 -0800, Greg KH wrote:
> On Thu, Feb 08, 2007 at 06:32:02PM +0000, James Simmons wrote:
> > I CC Greg to explain. The backlight class didn't go away. The way it is 
> > handled is different.
> 
> Have a pointer to the patch so I can help explain better?

You've been cc'd on the proposed patch a couple of times in this thread
so it should be in your mailbox now.

> As a short summary, 'struct class_device' is going away.  Using a
> 'struct device' in its place is what the conversion should have just
> done, no functionality change otherwise.

The backlight class is drivers/video/backlight/backlight.c and if I
understand things correctly what shows up in sysfs changes.

At the moment (as I understand the code which could be wrong), the
backlight functionality is tagged onto an existing device. Taking
drivers/video/backlight/corgi_bl.c as an example, the corgi_bl device
exists under /sys/devices/platform/corgi_bl with an associated struct
device. The backlight code then appends some magic to this to link in
some class attributes that show up under /sys/class/backlight. There is
only ever one device.

By replacing class_device_register() with device_create(), the proposed
patch appears to generate a second struct device with the original as
its parent. I'm not sure where it appears in /sys/devices? Having
another full blown struct device around makes me uneasy as wherever it
appears, we only have one device, not two.

If I had some kind of platform device which had an LED, backlight and
say battery component and registered with the three appropriate classes
would that mean four struct devices? Where under /sys/devices do they
each appear?

Also, it was mentioned that a parent struct device is a requirement and
this isn't the case for all backlight users. I think this constraint on
device_create has been removed in more recent code though? 

Richard

Re: Git backlight subsystem tree

[Greg KH]

On Thu, Feb 08, 2007 at 11:50:23PM +0000, Richard Purdie wrote:
> Hi Greg,
> 
> On Thu, 2007-02-08 at 13:23 -0800, Greg KH wrote:
> > On Thu, Feb 08, 2007 at 06:32:02PM +0000, James Simmons wrote:
> > > I CC Greg to explain. The backlight class didn't go away. The way it is 
> > > handled is different.
> > 
> > Have a pointer to the patch so I can help explain better?
> 
> You've been cc'd on the proposed patch a couple of times in this thread
> so it should be in your mailbox now.
> 
> > As a short summary, 'struct class_device' is going away.  Using a
> > 'struct device' in its place is what the conversion should have just
> > done, no functionality change otherwise.
> 
> The backlight class is drivers/video/backlight/backlight.c and if I
> understand things correctly what shows up in sysfs changes.
> 
> At the moment (as I understand the code which could be wrong), the
> backlight functionality is tagged onto an existing device. Taking
> drivers/video/backlight/corgi_bl.c as an example, the corgi_bl device
> exists under /sys/devices/platform/corgi_bl with an associated struct
> device. The backlight code then appends some magic to this to link in
> some class attributes that show up under /sys/class/backlight. There is
> only ever one device.
> 
> By replacing class_device_register() with device_create(), the proposed
> patch appears to generate a second struct device with the original as
> its parent. I'm not sure where it appears in /sys/devices? Having
> another full blown struct device around makes me uneasy as wherever it
> appears, we only have one device, not two.

No, your blacklight "device" is also a device now, not just a
class_device.  You want to show that heirachy properly for a variety of
different reasons (power management being one of the most important.)

> If I had some kind of platform device which had an LED, backlight and
> say battery component and registered with the three appropriate classes
> would that mean four struct devices? Where under /sys/devices do they
> each appear?

Under the main device itself, as they are children of it.

As an example, look at a sound device now on 2.6.20 with
CONFIG_SYSFS_DEPRECATED turned off:

$ tree /sys/class/sound/
/sys/class/sound/
|-- adsp -> ../../devices/pci0000:00/0000:00:1b.0/card0/adsp
|-- audio -> ../../devices/pci0000:00/0000:00:1b.0/card0/audio
|-- card0 -> ../../devices/pci0000:00/0000:00:1b.0/card0
|-- controlC0 -> ../../devices/pci0000:00/0000:00:1b.0/card0/controlC0
|-- dsp -> ../../devices/pci0000:00/0000:00:1b.0/card0/dsp
|-- mixer -> ../../devices/pci0000:00/0000:00:1b.0/card0/mixer
|-- pcmC0D0c -> ../../devices/pci0000:00/0000:00:1b.0/card0/pcmC0D0c
|-- pcmC0D0p -> ../../devices/pci0000:00/0000:00:1b.0/card0/pcmC0D0p
|-- pcmC0D1p -> ../../devices/pci0000:00/0000:00:1b.0/card0/pcmC0D1p
|-- seq -> ../../devices/virtual/sound/seq
|-- sequencer -> ../../devices/virtual/sound/sequencer
|-- sequencer2 -> ../../devices/virtual/sound/sequencer2
`-- timer -> ../../devices/virtual/sound/timer

There are a bunch of mixer and other interfaces, all now real devices
under the proper PCI device (sound added the "card0" intermediate level
also, but you will not have that for your devices.

> Also, it was mentioned that a parent struct device is a requirement and
> this isn't the case for all backlight users. I think this constraint on
> device_create has been removed in more recent code though? 

Yes, if NULL is passed in, it will be created in the
/sys/devices/virtual/CLASS_NAME/ directory.  See the above "seq" and
"timer" devices as an example of that.

Hope this helps,

greg k-h