Re: Backlight device class redesign

["Rafał Miłecki" <zajec5@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1396 bytes --]

W dniu 28 listopada 2009 01:30 użytkownik Rafał Miłecki
<zajec5@gmail.com> napisał:
> As discussed in http://marc.info/?t=124947671300008&r=1&w=2 we need to
> redesign our backlight device class.

I was trying to make it work this way:
1. Everytime register request comes, look for backlight_display with given name.
1. A. If needed, create new backlight_display
2. Add new device to devices list of backlight_display
3. Find best device on devices list of backlight_display
4. Register if under /sys/class/backlight/name_of_display

This didn't work because of registering only the best device. Look at
this code in drivers/acpi/video.c:

device->backlight = backlight_device_register(name,
    NULL, device, &acpi_backlight_ops);
(...)
result = sysfs_create_link(&device->backlight->dev.kobj,
    &device->dev->dev.kobj, "device");

It expects device to be registered (have dev.kobj) after calling
backlight_device_register.

So in summary we just can not do lazy registration. Every device
passed to backlight_device_register has to be registered in that
function or never.

So I think we have to register every device and just keep symlink
/sys/class/backlight/display_name pointing best device. We could
register devices in
1) /sys/class/backlight/.devicename (hidden)
2) some /sys/class/internal-usage/devicename

Do you agree on this?

-- 
Rafał

[-- Attachment #2: backlight2.patch --]
[-- Type: application/octet-stream, Size: 5652 bytes --]

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 6615ac7..2236b38 100644
--- a/drivers/video/backlight/backlight.c
+++ b/drivers/video/backlight/backlight.c
@@ -13,11 +13,46 @@
 #include <linux/ctype.h>
 #include <linux/err.h>
 #include <linux/fb.h>
+#include <linux/string.h>
 
 #ifdef CONFIG_PMAC_BACKLIGHT
 #include <asm/backlight.h>
 #endif
 
+static struct class *backlight_class;
+static struct list_head displays;
+
+static struct backlight_display *backlight_find_display(const char *name)
+{
+	struct list_head *ptr;
+	struct backlight_display *entry;
+
+	list_for_each(ptr, &displays) {
+		entry = list_entry(ptr, struct backlight_display, list);
+		if (strcmp(entry->name, name) == 0) {
+			return entry;
+		}
+	}
+
+	return NULL;
+}
+
+static struct backlight_device *backlight_best_device(struct backlight_display *display)
+{
+	struct list_head *ptr;
+	struct backlight_device *entry;
+
+	printk(KERN_WARNING "[DBG] get_device called for %s\n", display->name);
+	list_for_each(ptr, &display->devices) {
+		/* FIXME: return best, not first */
+		entry = list_entry(ptr, struct backlight_device, list);
+		return entry;
+	}
+
+	printk(KERN_WARNING "[DBG] ohhhhh noooo, list empty?\n");
+	return NULL;
+}
+
 #if defined(CONFIG_FB) || (defined(CONFIG_FB_MODULE) && \
 			   defined(CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE))
 /* This callback gets called when something important happens inside a
@@ -73,6 +108,53 @@ static inline void backlight_unregister_fb(struct backlight_device *bd)
 }
 #endif /* CONFIG_FB */
 
+static int backlight_pick_up_device(struct backlight_display *display)
+{
+	int rc;
+	struct backlight_device *device = backlight_best_device(display);
+
+	if (!device) {
+		printk(KERN_WARNING "[DBG] can't find best device for %s\n", display->name);
+		return -1;
+	}
+
+	if (device == display->curr) {
+		/* Currently used device is still the best choice */
+		printk(KERN_WARNING "[DBG] current is the best\n");
+		return 0;
+	}
+
+	if (display->curr) {
+		printk(KERN_WARNING "[DBG] unregistering old (current)\n");
+		backlight_unregister_fb(display->curr);
+		device_unregister(&display->curr->dev);
+		display->curr = NULL;
+	}
+
+	printk(KERN_WARNING "[DBG] registering new\n");
+
+	rc = device_register(&device->dev);
+	if (rc) {
+		return rc;
+	}
+
+	rc = backlight_register_fb(device);
+	if (rc) {
+		return rc;
+	}
+
+	display->curr = device;
+
+#ifdef CONFIG_PMAC_BACKLIGHT
+	mutex_lock(&pmac_backlight_mutex);
+	if (!pmac_backlight)
+		pmac_backlight = new_bd;
+	mutex_unlock(&pmac_backlight_mutex);
+#endif
+
+	return 0;
+}
+
 static void backlight_generate_event(struct backlight_device *bd,
 				     enum backlight_update_reason reason)
 {
@@ -190,8 +272,6 @@ static ssize_t backlight_show_actual_brightness(struct device *dev,
 	return rc;
 }
 
-static struct class *backlight_class;
-
 static int backlight_suspend(struct device *dev, pm_message_t state)
 {
 	struct backlight_device *bd = to_backlight_device(dev);
@@ -271,10 +351,24 @@ EXPORT_SYMBOL(backlight_force_update);
 struct backlight_device *backlight_device_register(const char *name,
 		struct device *parent, void *devdata, struct backlight_ops *ops)
 {
+	struct backlight_display *display;
 	struct backlight_device *new_bd;
-	int rc;
 
 	pr_debug("backlight_device_register: name=%s\n", name);
+	printk(KERN_WARNING "[DBG] registering new backlight: %s\n", name);
+
+	display = backlight_find_display("panel0");
+	if (!display) {
+		display = kzalloc(sizeof(struct backlight_display), GFP_KERNEL);
+		if (!display)
+			return ERR_PTR(-ENOMEM);
+
+		INIT_LIST_HEAD(&display->devices);
+		display->name = kstrdup("panel0", GFP_KERNEL);
+		list_add_tail(&display->list, &displays);
+
+		printk(KERN_WARNING "[DBG] we have new display %s\n", display->name);
+	}
 
 	new_bd = kzalloc(sizeof(struct backlight_device), GFP_KERNEL);
 	if (!new_bd)
@@ -289,26 +383,11 @@ struct backlight_device *backlight_device_register(const char *name,
 	dev_set_name(&new_bd->dev, name);
 	dev_set_drvdata(&new_bd->dev, devdata);
 
-	rc = device_register(&new_bd->dev);
-	if (rc) {
-		kfree(new_bd);
-		return ERR_PTR(rc);
-	}
-
-	rc = backlight_register_fb(new_bd);
-	if (rc) {
-		device_unregister(&new_bd->dev);
-		return ERR_PTR(rc);
-	}
-
 	new_bd->ops = ops;
 
-#ifdef CONFIG_PMAC_BACKLIGHT
-	mutex_lock(&pmac_backlight_mutex);
-	if (!pmac_backlight)
-		pmac_backlight = new_bd;
-	mutex_unlock(&pmac_backlight_mutex);
-#endif
+	list_add_tail(&new_bd->list, &display->devices);
+
+	backlight_pick_up_device(display);
 
 	return new_bd;
 }
@@ -347,6 +426,8 @@ static void __exit backlight_class_exit(void)
 
 static int __init backlight_class_init(void)
 {
+	INIT_LIST_HEAD(&displays);
+
 	backlight_class = class_create(THIS_MODULE, "backlight");
 	if (IS_ERR(backlight_class)) {
 		printk(KERN_WARNING "Unable to create backlight class; errno = %ld\n",
diff --git a/include/linux/backlight.h b/include/linux/backlight.h
index 0f5f578..5b148f4 100644
--- a/include/linux/backlight.h
+++ b/include/linux/backlight.h
@@ -35,6 +35,18 @@ enum backlight_update_reason {
 struct backlight_device;
 struct fb_info;
 
+struct backlight_display {
+	/* List of displays */
+	struct list_head list;
+
+	char *name;
+
+	/* Devices controlling display's backlight */
+	struct list_head devices;
+
+	struct backlight_device *curr;
+};
+
 struct backlight_ops {
 	unsigned int options;
 
@@ -76,6 +88,11 @@ struct backlight_properties {
 };
 
 struct backlight_device {
+	/* List of devices */
+	struct list_head list;
+
+	struct backlight_display *display;
+
 	/* Backlight properties */
 	struct backlight_properties props;