* Git backlight subsystem tree @ 2007-02-08 2:30 Richard Purdie 2007-02-08 3:01 ` Andrew Morton 2007-02-08 15:28 ` James Simmons 0 siblings, 2 replies; 11+ messages in thread From: Richard Purdie @ 2007-02-08 2:30 UTC (permalink / raw) To: LKML, akpm; +Cc: Marcin Juszkiewicz 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Git backlight subsystem tree 2007-02-08 2:30 Git backlight subsystem tree Richard Purdie @ 2007-02-08 3:01 ` Andrew Morton 2007-02-08 15:28 ` James Simmons 1 sibling, 0 replies; 11+ messages in thread From: Andrew Morton @ 2007-02-08 3:01 UTC (permalink / raw) To: Richard Purdie; +Cc: LKML, Marcin Juszkiewicz 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! ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Git backlight subsystem tree 2007-02-08 2:30 Git backlight subsystem tree Richard Purdie 2007-02-08 3:01 ` Andrew Morton @ 2007-02-08 15:28 ` James Simmons 2007-02-08 17:59 ` Richard Purdie 1 sibling, 1 reply; 11+ messages in thread From: James Simmons @ 2007-02-08 15:28 UTC (permalink / raw) To: Richard Purdie; +Cc: LKML, akpm, Marcin Juszkiewicz 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, ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: Git backlight subsystem tree 2007-02-08 15:28 ` James Simmons @ 2007-02-08 17:59 ` Richard Purdie 2007-02-08 18:32 ` James Simmons 0 siblings, 1 reply; 11+ messages in thread From: Richard Purdie @ 2007-02-08 17:59 UTC (permalink / raw) To: James Simmons; +Cc: LKML, akpm, Marcin Juszkiewicz 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Git backlight subsystem tree 2007-02-08 17:59 ` Richard Purdie @ 2007-02-08 18:32 ` James Simmons 2007-02-08 19:37 ` Richard Purdie 2007-02-08 21:23 ` Greg KH 0 siblings, 2 replies; 11+ messages in thread From: James Simmons @ 2007-02-08 18:32 UTC (permalink / raw) To: Richard Purdie; +Cc: LKML, akpm, Marcin Juszkiewicz, Greg KH 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. ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Git backlight subsystem tree 2007-02-08 18:32 ` James Simmons @ 2007-02-08 19:37 ` Richard Purdie 2007-02-08 21:23 ` Greg KH 1 sibling, 0 replies; 11+ messages in thread From: Richard Purdie @ 2007-02-08 19:37 UTC (permalink / raw) To: James Simmons; +Cc: LKML, akpm, Marcin Juszkiewicz, Greg KH 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Git backlight subsystem tree 2007-02-08 18:32 ` James Simmons 2007-02-08 19:37 ` Richard Purdie @ 2007-02-08 21:23 ` Greg KH 2007-02-08 21:54 ` James Simmons 2007-02-08 23:50 ` Richard Purdie 1 sibling, 2 replies; 11+ messages in thread From: Greg KH @ 2007-02-08 21:23 UTC (permalink / raw) To: James Simmons; +Cc: Richard Purdie, LKML, akpm, Marcin Juszkiewicz 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Git backlight subsystem tree 2007-02-08 21:23 ` Greg KH @ 2007-02-08 21:54 ` James Simmons 2007-02-08 23:41 ` Greg KH 2007-02-08 23:50 ` Richard Purdie 1 sibling, 1 reply; 11+ messages in thread From: James Simmons @ 2007-02-08 21:54 UTC (permalink / raw) To: Greg KH; +Cc: Richard Purdie, LKML, akpm, Marcin Juszkiewicz > 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, ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: Git backlight subsystem tree 2007-02-08 21:54 ` James Simmons @ 2007-02-08 23:41 ` Greg KH 0 siblings, 0 replies; 11+ messages in thread From: Greg KH @ 2007-02-08 23:41 UTC (permalink / raw) To: James Simmons; +Cc: Richard Purdie, LKML, akpm, Marcin Juszkiewicz 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Git backlight subsystem tree 2007-02-08 21:23 ` Greg KH 2007-02-08 21:54 ` James Simmons @ 2007-02-08 23:50 ` Richard Purdie 2007-02-09 0:23 ` Greg KH 1 sibling, 1 reply; 11+ messages in thread From: Richard Purdie @ 2007-02-08 23:50 UTC (permalink / raw) To: Greg KH; +Cc: James Simmons, LKML, akpm, Marcin Juszkiewicz 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: Git backlight subsystem tree 2007-02-08 23:50 ` Richard Purdie @ 2007-02-09 0:23 ` Greg KH 0 siblings, 0 replies; 11+ messages in thread From: Greg KH @ 2007-02-09 0:23 UTC (permalink / raw) To: Richard Purdie; +Cc: James Simmons, LKML, akpm, Marcin Juszkiewicz 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2007-02-09 0:24 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2007-02-08 2:30 Git backlight subsystem tree Richard Purdie 2007-02-08 3:01 ` Andrew Morton 2007-02-08 15:28 ` James Simmons 2007-02-08 17:59 ` Richard Purdie 2007-02-08 18:32 ` James Simmons 2007-02-08 19:37 ` Richard Purdie 2007-02-08 21:23 ` Greg KH 2007-02-08 21:54 ` James Simmons 2007-02-08 23:41 ` Greg KH 2007-02-08 23:50 ` Richard Purdie 2007-02-09 0:23 ` Greg KH
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.