Backlight device class redesign

Backlight device class redesign

[Rafał Miłecki]

As discussed in http://marc.info/?t=124947671300008&r=1&w=2 we need to
redesign our backlight device class. If you wish you can read whole
thread and even http://bugs.freedesktop.org/show_bug.cgi?id=20963 but
I'll try to summarize everything anyway.

1) One display can be controlled (I mean backlight) in more than one
way. For example: ACPI, something platform-specific and GPU encoder.
2) Exporting many ways of controlling one display at the same time
drives us to inconsistencies.
3) We have to decide some priority like: ACPI > platform > GPU
4) We have to keep list of all registered controllers for one display.
We can not just unregister lower prioritised as we will hit problem
after (for example) unloading acpi module.
5) We should not export more than one way and let userspace decide.

Currently we keep all registered controllers in /sys/class/backlgiht/
without any grouping per display or per anything. We can not use
grouping per GPU (as somewhere proposed) because one GPU can control
(for example) two PANELS and TV - 3 displays needing backlight
management. The most obvious and stable solution seems to be grouping
per display.

So staying with idea of grouping per display I tried to imagine the
most crazy notebook with external displays configuration we should
handle. Let's consider following displays in notebook (yes, two
panel-notebook!):
1) PANEL#A controlled by ACPI + platform-specific + GPU
2) PANEL#B controlled by ACPI + platform-specific
3) TV connected by S-VIDEO controlled by GPU
4) LCD#A connected by USB controlled by LCD-specific driver
5) LCD#B connected by USB controlled by LCD-specific driver (same
model as LCD#A, driven by the same driver)

Let's do reverse order:
a) For two USB-LCDs driver should generate two other names. Depends on
properties received from LCD that could be serial number or usb port
number. So we would get /sys/class/lcd-vendor-0000:00:1d.2 and
/sys/class/lcd-vendor-0000:00:a3.5
b) For TV gpu kernel module would use name with GPU card number and
output name. Let's say /sys/class/card0-svideo
c) The most complicated case is for PANELs. We have to pick up same
names in: ACPI, platform-specific and GPU driver. For most cases,
which means notebooks with one panel, we could just use "panel".
However I do not want to redesign this after year when we get more
notebooks with two PANELs. So do you have any idea about this? How we
could generate correct names in 3 almost unrelated modules (ACPI,
platform, GPU driver)? We should end with something like
/sys/class/internal-panel-1 and /sys/class/internal-panel-2 but that
won't be so easy. Any ideas?

Please use "Reply to all", as I do cross-ML-post and I am not
subscribed to LKML.

-- 
Rafał

Re: Backlight device class redesign

[Rafał Miłecki]

W dniu 28 listopada 2009 01:30 użytkownik Rafał Miłecki
<zajec5@gmail.com> napisał:
> Currently we keep all registered controllers in /sys/class/backlgiht/
> without any grouping per display or per anything. We can not use
> grouping per GPU (as somewhere proposed) because one GPU can control
> (for example) two PANELS and TV - 3 displays needing backlight
> management. The most obvious and stable solution seems to be grouping
> per display.

Just in case I was not clear enough. We should make ACPI, platform and
GPU drivers register same panel backlight control under same name like
"panel-0". Backlight class device would create
/sys/class/backlight/panel-0 only. When touching files in this
backlight/panel-0/, blacklight class would call functions from the
most proper (ACPI > platform > GPU) driver.

I thought if we could use card number for anything. I recalled that
notebooks with hybrid gpu, where system should switch between weaker
and more powerful GPU watching load. In case of that notebooks PANEL
can be controlled by both: card0 and card1.

So finally I think we should just register panel0 by default
everywhere. And in future we could add some quirks in drivers (ACPI,
platform, GPU) that would register panel1, panel2... for such devices.

Any comments? Should I work on something like specified above?

-- 
Rafał

Re: Backlight device class redesign

[Henrique de Moraes Holschuh]

Backlight maintainer added to CC.

On Sat, 28 Nov 2009, Rafa?? Mi??ecki wrote:
> W dniu 28 listopada 2009 01:30 u??ytkownik Rafa?? Mi??ecki
> <zajec5@gmail.com> napisa??:
> > Currently we keep all registered controllers in /sys/class/backlgiht/
> > without any grouping per display or per anything. We can not use
> > grouping per GPU (as somewhere proposed) because one GPU can control
> > (for example) two PANELS and TV - 3 displays needing backlight
> > management. The most obvious and stable solution seems to be grouping
> > per display.
> 
> Just in case I was not clear enough. We should make ACPI, platform and
> GPU drivers register same panel backlight control under same name like
> "panel-0". Backlight class device would create
> /sys/class/backlight/panel-0 only. When touching files in this
> backlight/panel-0/, blacklight class would call functions from the
> most proper (ACPI > platform > GPU) driver.
> 

I once suggested some sort of multiplexer backlight device that would do
exactly that.  Various drivers (platform, ACPI, display) would register
backends and specify some sort of dynamic preference so that the kernel
would have a good bet on the default one, and userspace could switch the
active backend later if it wanted.

In this case, we'd have to specify properties or some other way to group the
backends properly and to easily let userspace find the backlight device it
needs to drive a specific device.

> I thought if we could use card number for anything. I recalled that
> notebooks with hybrid gpu, where system should switch between weaker
> and more powerful GPU watching load. In case of that notebooks PANEL
> can be controlled by both: card0 and card1.

That would require backends to be able to runtime register and unregister
themselves with the main backlight device, and that they hook somehow on the
hotplug stuff, to know when to bind/unbind along with the GPU.

> So finally I think we should just register panel0 by default
> everywhere. And in future we could add some quirks in drivers (ACPI,
> platform, GPU) that would register panel1, panel2... for such devices.

If you're going to design something, do it properly from day one.  Address
all the main issues: multiple backends for each device, auto-selection of
the default backend based on data reported by the backend drivers (e.g. a
platform driver usually knows very well if it should be preferred over ACPI
and GPU or not...), *and* multiple devices with hotplug.

These are not, after all, overengineering for the future.  It is a problem
we have _now_ with real world devices that are shipping.

There are other ways to solve this issue, of course, and they're easier:
publish MORE information on sysfs to properly bind backlight devices to the
real device they control, as well as more data to help userspace decide
which one it should use.   It is a cop-out, as we are only solving half the
problem space in-kernel.  But it works, and it is not a hack, so it is also
a viable solution.

I prefer the full kernel solution where we export just one backlight
interface, but when I propesed it, the others made it clear they'd prefer
the less complex "userspace chooses which backlight backend to drive"
currently employed.

Meanwhile, I'd like a LOT to know how I could parent the backlight device I
export in a platform driver to the correct PCI or ACPI device for the main
GPU...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: Backlight device class redesign

[Rafał Miłecki]

2009/11/28 Henrique de Moraes Holschuh <hmh@hmh.eng.br>:
> Backlight maintainer added to CC.

Thanks, I should have done that.

> On Sat, 28 Nov 2009, Rafa?? Mi??ecki wrote:
>> W dniu 28 listopada 2009 01:30 u??ytkownik Rafa?? Mi??ecki
>> <zajec5@gmail.com> napisa??:
>> > Currently we keep all registered controllers in /sys/class/backlgiht/
>> > without any grouping per display or per anything. We can not use
>> > grouping per GPU (as somewhere proposed) because one GPU can control
>> > (for example) two PANELS and TV - 3 displays needing backlight
>> > management. The most obvious and stable solution seems to be grouping
>> > per display.
>>
>> Just in case I was not clear enough. We should make ACPI, platform and
>> GPU drivers register same panel backlight control under same name like
>> "panel-0". Backlight class device would create
>> /sys/class/backlight/panel-0 only. When touching files in this
>> backlight/panel-0/, blacklight class would call functions from the
>> most proper (ACPI > platform > GPU) driver.
>>
>
> I once suggested some sort of multiplexer backlight device that would do
> exactly that.  Various drivers (platform, ACPI, display) would register
> backends and specify some sort of dynamic preference so that the kernel
> would have a good bet on the default one, and userspace could switch the
> active backend later if it wanted.
>
> In this case, we'd have to specify properties or some other way to group the
> backends properly and to easily let userspace find the backlight device it
> needs to drive a specific device.

I described my idea of solution at end (cat/echo method).


>> I thought if we could use card number for anything. I recalled that
>> notebooks with hybrid gpu, where system should switch between weaker
>> and more powerful GPU watching load. In case of that notebooks PANEL
>> can be controlled by both: card0 and card1.
>
> That would require backends to be able to runtime register and unregister
> themselves with the main backlight device, and that they hook somehow on the
> hotplug stuff, to know when to bind/unbind along with the GPU.

Don't see anything problematic here. First of course we would have to
support GPU switching, but backlight should not be any issue. When
switching from ATI do Intel just unregister backlight device in ATI
and register backlight device in Intel. Both drivers should register
under the same name like "panel0" or sth.


>> So finally I think we should just register panel0 by default
>> everywhere. And in future we could add some quirks in drivers (ACPI,
>> platform, GPU) that would register panel1, panel2... for such devices.
>
> If you're going to design something, do it properly from day one.  Address
> all the main issues: multiple backends for each device, auto-selection of
> the default backend based on data reported by the backend drivers (e.g. a
> platform driver usually knows very well if it should be preferred over ACPI
> and GPU or not...), *and* multiple devices with hotplug.
>
> These are not, after all, overengineering for the future.  It is a problem
> we have _now_ with real world devices that are shipping.

I wasn't clear, sorry. I want to address all current and future
combinations. I tried to make that clear in my mails. I just mean we
should modify all panel backlight devices to register for "panel0"
display as we currently don't know/support notebooks with two PANELs.
In future if someone put hands on notebook with two PANELs he can
modify module to register both backlight devices: panel0 and panel1
(2, 3, ...).

I didn't know we should prefer platform over ACPI. But maybe you're
right, really, don't know. Eventually we can even add enum { LOWEST,
GPU, PLATFORM_LOW, ACPI, PLATFORM_HIGH } or sth.


> There are other ways to solve this issue, of course, and they're easier:
> publish MORE information on sysfs to properly bind backlight devices to the
> real device they control, as well as more data to help userspace decide
> which one it should use.   It is a cop-out, as we are only solving half the
> problem space in-kernel.  But it works, and it is not a hack, so it is also
> a viable solution.
>
> I prefer the full kernel solution where we export just one backlight
> interface, but when I propesed it, the others made it clear they'd prefer
> the less complex "userspace chooses which backlight backend to drive"
> currently employed.

I think we can stay with proposed kernel solution with eventual
choosing backlight controller for display (overwriting kernel's
choice). For example
$ cat /sys/class/backlight/panel0/method
acpi
platform
$ echo "platform" > /sys/class/backlight/panel0/method


> Meanwhile, I'd like a LOT to know how I could parent the backlight device I
> export in a platform driver to the correct PCI or ACPI device for the main
> GPU...

Is actually "parent" used for something? I can see it used in many
modules, but not sure where we use that pointer (if we do).

-- 
Rafał

Re: Backlight device class redesign

[Henrique de Moraes Holschuh]

On Sat, 28 Nov 2009, Rafa?? Mi??ecki wrote:
> > Meanwhile, I'd like a LOT to know how I could parent the backlight device I
> > export in a platform driver to the correct PCI or ACPI device for the main
> > GPU...
> 
> Is actually "parent" used for something? I can see it used in many
> modules, but not sure where we use that pointer (if we do).

For one, you get symlinks in sysfs, which would let userspace know what
backlight is related to what PCI/video/whatever device.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: Backlight device class redesign

[Rafał Miłecki]

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

Re: Backlight device class redesign

[Rafał Miłecki]

W dniu 29 listopada 2009 21:09 użytkownik Rafał Miłecki
<zajec5@gmail.com> napisał:
> 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?

Please, see attached patch. TODO:
1) Add METHOD (enum { ACPI, GPU, PLATFORM} ) as
backlight_device_register parameter
2) Add display name as backlight_device_register parameter
3) Use given METHODs in backlight_best_device
4) Fix symlinks to be created in /sys/class/backlight/ instead of
/sys/dev/char/ (how?!)

What about proposed solution?

-- 
Rafał

Re: Backlight device class redesign

[Rafał Miłecki]

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

W dniu 29 listopada 2009 22:51 użytkownik Rafał Miłecki
<zajec5@gmail.com> napisał:
> W dniu 29 listopada 2009 21:09 użytkownik Rafał Miłecki
> <zajec5@gmail.com> napisał:
>> 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?
>
> Please, see attached patch. TODO:

Attached...

-- 
Rafał

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

diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
index 6615ac7..43aae31 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,42 @@ static inline void backlight_unregister_fb(struct backlight_device *bd)
 }
 #endif /* CONFIG_FB */
 
+static int backlight_pick_up_device(struct backlight_display *display)
+{
+	int result;
+	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 symlink %s for old device\n", display->name);
+		sysfs_remove_link(backlight_class->dev_kobj, display->name);
+		display->curr = NULL;
+	}
+
+	printk(KERN_WARNING "[DBG] registering new symlink %s for new device\n", display->name);
+
+	result = sysfs_create_link(backlight_class->dev_kobj,
+					   &device->dev.kobj, display->name);
+	if (result) {
+		printk(KERN_WARNING "[DBG] failed to symlink\n");
+		return result;
+	}
+
+	display->curr = device;
+
+	return 0;
+}
+
 static void backlight_generate_event(struct backlight_device *bd,
 				     enum backlight_update_reason reason)
 {
@@ -190,8 +261,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 +340,26 @@ 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;
+	char *mod_name;
 	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)
@@ -283,11 +368,16 @@ struct backlight_device *backlight_device_register(const char *name,
 	mutex_init(&new_bd->update_lock);
 	mutex_init(&new_bd->ops_lock);
 
+	mod_name = kzalloc(strlen(name) + 2, GFP_KERNEL);
+	if (!mod_name)
+		return ERR_PTR(-ENOMEM);
+	sprintf(mod_name, ".%s", name);
 	new_bd->dev.class = backlight_class;
 	new_bd->dev.parent = parent;
 	new_bd->dev.release = bl_device_release;
-	dev_set_name(&new_bd->dev, name);
+	dev_set_name(&new_bd->dev, mod_name);
 	dev_set_drvdata(&new_bd->dev, devdata);
+	kfree(mod_name);
 
 	rc = device_register(&new_bd->dev);
 	if (rc) {
@@ -301,6 +391,10 @@ struct backlight_device *backlight_device_register(const char *name,
 		return ERR_PTR(rc);
 	}
 
+	list_add_tail(&new_bd->list, &display->devices);
+
+	backlight_pick_up_device(display);
+
 	new_bd->ops = ops;
 
 #ifdef CONFIG_PMAC_BACKLIGHT
@@ -347,6 +441,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;
 

Re: Backlight device class redesign

[Henrique de Moraes Holschuh]

Please don't attach patches that are to be reviewed, especially if your MUA
is not going to mark them "text/plain".  Send them in the main body of the
email.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: Backlight device class redesign

[Zhang Rui]

On Mon, 2009-11-30 at 04:09 +0800, Rafał Miłecki wrote:
> 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");
> 
IMO, we should create one device for one display.
For example, device /sys/class/backlight/panel0 is created the first
time a backlight_ops for this display is registered.
And create the symbol link from the backlight class device to "physical
device" (device that invokes backlight_device_register) in the backlight
class device driver, if we really need it.
Remove and recreate a link pointing to the new physical device when the
backlight mode is changed.

thanks,
rui

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

Re: Backlight device class redesign

[Henrique de Moraes Holschuh]

On Thu, 03 Dec 2009, Zhang Rui wrote:
> For example, device /sys/class/backlight/panel0 is created the first
> time a backlight_ops for this display is registered.
> And create the symbol link from the backlight class device to "physical
> device" (device that invokes backlight_device_register) in the backlight
> class device driver, if we really need it.
> Remove and recreate a link pointing to the new physical device when the
> backlight mode is changed.

FWIW, I agree the above solution is simple enough and quite good enough.

I just have absolutely _NO_ idea whatsoever how a platform driver can ask
the PCI or APCI layer "where is the video device driving the LVDS panel so
that I can bind to it"...

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: Backlight device class redesign

[Matthew Garrett]

On Thu, Dec 03, 2009 at 04:01:14PM +0800, Zhang Rui wrote:

> IMO, we should create one device for one display.
> For example, device /sys/class/backlight/panel0 is created the first
> time a backlight_ops for this display is registered.

For anything other than the simple case (one display per machine), how 
do you know whether the firmware, platform and gpu are referring to the 
same display device?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: Backlight device class redesign

[Zhang Rui]

On Thu, 2009-12-03 at 18:52 +0800, Henrique de Moraes Holschuh wrote:
> On Thu, 03 Dec 2009, Zhang Rui wrote:
> > For example, device /sys/class/backlight/panel0 is created the first
> > time a backlight_ops for this display is registered.
> > And create the symbol link from the backlight class device to "physical
> > device" (device that invokes backlight_device_register) in the backlight
> > class device driver, if we really need it.
> > Remove and recreate a link pointing to the new physical device when the
> > backlight mode is changed.
> 
> FWIW, I agree the above solution is simple enough and quite good enough.
> 
> I just have absolutely _NO_ idea whatsoever how a platform driver can ask
> the PCI or APCI layer "where is the video device driving the LVDS panel so
> that I can bind to it"...
> 
well. this is the question I want to ask. :)

IMO, if we want to create one device for one display, we must have a
unique ID for each display and the ID must be generic enough so that
both ACPI & platform & graphics driver can understand it.

ACPI&platform drivers have no idea about the display info it's bind to.
But they're always used to control the integrated panel, no?
we can use something like DEFAULT_ACPI_DISPLAY/DEFAULT_PLATFORM_DISPLAY
which stands for the integrated LVDS panel, which can also be understood
by the graphics driver.

ACPI video driver -->ACPI_DISPLAY-->|
                                    |
platform driver->PLATFORM_DISPLAY-->|--->/sys/class/backlight/panel0
                                    |
          |-->integrated panel----->|
          |
graphics  |-->TV------------------->|--->/sys/class/backlight/tv0
          |
          |-->external LCD--------->|--->...

I'm not a graphics guy so I don't if there are any other ways to control
the TV/external LCD device, if yes, they can use something else as the
unique ID, say EDID, right?

thanks,
rui