Backlight and LCD module patches [1]

Backlight and LCD module patches [1]

[Andrew Zabolotny]

Hello!

I've tried to fulfill all the requirements that various people presented to
the previous version of this patch; this and the following letter contains
the fixed version of the patch that is proposed to be included into the
mainstream kernel.

--
Greetings,
   Andrew

This patch adds the class_device_find() function, which can be used
to find a class_device by its name. The function was originally
written by Greg Kroah-Hartman.

Also it fixes the class_rename() function to accept 'const char *'
argument as the new class device name.

Signed-off-by: Andrew Zabolotny <zap@homelink.ru>

--- linux-2.6.7-rc3/drivers/base/class.c	2004-06-11 02:39:19.000000000 +0400
+++ linux/drivers/base/class.c	2004-06-17 21:51:11.000000000 +0400
@@ -359,7 +359,7 @@
 	class_device_put(class_dev);
 }
 
-int class_device_rename(struct class_device *class_dev, char *new_name)
+int class_device_rename(struct class_device *class_dev, const char *new_name)
 {
 	int error = 0;
 
@@ -379,6 +379,33 @@
 	return error;
 }
 
+/**
+ * class_device_find - find a struct class_device in a specific class
+ * @class: the class to search
+ * @class_id: the class_id to search for
+ *
+ * Iterates through the list of all class devices registered to a class. If
+ * the class_id is found, its reference count is incremented and returned to
+ * the caller. If the class_id does not match any existing struct class_device
+ * registered to this struct class, then NULL is returned.
+ */
+struct class_device * class_device_find(struct class *class, const char *class_id)
+{
+	struct class_device *class_dev;
+	struct class_device *found = NULL;
+
+	down_read(&class->subsys.rwsem);
+	list_for_each_entry(class_dev, &class->children, node) {
+		if (strcmp(class_dev->class_id, class_id) == 0) {
+			found = class_device_get(class_dev);
+			break;
+		}
+	}
+	up_read(&class->subsys.rwsem);
+
+	return found;
+}
+
 struct class_device * class_device_get(struct class_device *class_dev)
 {
 	if (class_dev)
@@ -391,7 +418,6 @@
 	kobject_put(&class_dev->kobj);
 }
 
-
 int class_interface_register(struct class_interface *class_intf)
 {
 	struct class * parent;
@@ -470,6 +496,8 @@
 EXPORT_SYMBOL(class_device_put);
 EXPORT_SYMBOL(class_device_create_file);
 EXPORT_SYMBOL(class_device_remove_file);
+EXPORT_SYMBOL(class_device_rename);
+EXPORT_SYMBOL(class_device_find);
 
 EXPORT_SYMBOL(class_interface_register);
 EXPORT_SYMBOL(class_interface_unregister);
--- linux-2.6.7-rc3/include/linux/device.h	2004-06-11 02:39:37.000000000 +0400
+++ linux/include/linux/device.h	2004-06-17 21:52:32.000000000 +0400
@@ -212,8 +212,9 @@
 extern int class_device_add(struct class_device *);
 extern void class_device_del(struct class_device *);
 
-extern int class_device_rename(struct class_device *, char *);
+extern int class_device_rename(struct class_device *, const char *);
 
+extern struct class_device * class_device_find(struct class *class, const char *class_id);
 extern struct class_device * class_device_get(struct class_device *);
 extern void class_device_put(struct class_device *);
 

Re: Backlight and LCD module patches [1]

[Greg KH]

On Thu, Jun 17, 2004 at 10:35:14PM +0400, Andrew Zabolotny wrote:
> Hello!
> 
> I've tried to fulfill all the requirements that various people presented to
> the previous version of this patch; this and the following letter contains
> the fixed version of the patch that is proposed to be included into the
> mainstream kernel.

You didn't fulfill my requirement that this patch is not needed at all :)

So no, I'm not going to accept this, you need to change your lcd code to
pass around pointers to the proper structures, instead of trying to rely
on the name of a device.  Because of this, I'm not going to apply your
second patch.

thanks,

greg k-h

Re: Backlight and LCD module patches [1]

[Andrew Zabolotny]

On Thu, 17 Jun 2004 12:47:39 -0700
Greg KH <greg@kroah.com> wrote:

> So no, I'm not going to accept this, you need to change your lcd code to
> pass around pointers to the proper structures, instead of trying to rely
> on the name of a device.  Because of this, I'm not going to apply your
> second patch.
I think you missed something. It doesn't rely on the name of the device while
registering/unregistering, I've changed this, look:

extern int lcd_device_register(const char *name, void *devdata,
                              struct lcd_properties *lp,
                              struct lcd_device **alloc_ld);
extern void lcd_device_unregister(struct lcd_device *ld);

So the `register` function returns a pointer (fourth argument) to the created
and registered device, and in module_unload it calls something like
lcd_device_unregister (my_lcd_device). The name is passed during registration
because it has to be copied to the allocated struct device (and so that
find_device_by_name() can find the device by name).

Now this:

extern struct lcd_device *lcd_device_find(const char *name);

It needs a char* argument because there's no other easy way to find the
correspondence between framebuffer devices and lcd/backlight devices
corresponding to that framebuffer device. So the conventionality is that the
lcd/backlight devices bear the same name as the framebuffer they correspond
to, so the framebuffer device can easily find them.

Same reasons apply to backlight devices, they are quite similar.

--
Greetings,
   Andrew

Re: Backlight and LCD module patches [1]

[Greg KH]

On Fri, Jun 18, 2004 at 01:55:04AM +0400, Andrew Zabolotny wrote:
> On Thu, 17 Jun 2004 12:47:39 -0700
> Greg KH <greg@kroah.com> wrote:
> 
> > So no, I'm not going to accept this, you need to change your lcd code to
> > pass around pointers to the proper structures, instead of trying to rely
> > on the name of a device.  Because of this, I'm not going to apply your
> > second patch.
> I think you missed something. It doesn't rely on the name of the device while
> registering/unregistering, I've changed this, look:

No, I saw your change.

> extern int lcd_device_register(const char *name, void *devdata,
>                               struct lcd_properties *lp,
>                               struct lcd_device **alloc_ld);

That function should be:
struct lcd_device lcd_device_register(const char *name, void *devdata,
					struct lcd_properties *lp);

instead.  Then return an ERR_PTR() if you have an error.

> Now this:
> 
> extern struct lcd_device *lcd_device_find(const char *name);
> 
> It needs a char* argument because there's no other easy way to find the
> correspondence between framebuffer devices and lcd/backlight devices
> corresponding to that framebuffer device.

Then you need to have a way to corrispond those devices together,
becides just a name.  Use the pointer that you have provided to link
them together some way.

thanks,

greg k-h

Re: Backlight and LCD module patches [1]

[Andrew Zabolotny]

On Thu, 17 Jun 2004 15:05:10 -0700
Greg KH <greg@kroah.com> wrote:

> That function should be:
> struct lcd_device lcd_device_register(const char *name, void *devdata,
> 					struct lcd_properties *lp);
> instead.  Then return an ERR_PTR() if you have an error.

Okay, that was the easy part. Fixed.

> > extern struct lcd_device *lcd_device_find(const char *name);
> > 
> > It needs a char* argument because there's no other easy way to find the
> > correspondence between framebuffer devices and lcd/backlight devices
> > corresponding to that framebuffer device.
> Then you need to have a way to corrispond those devices together,
> becides just a name.  Use the pointer that you have provided to link
> them together some way.

There's no place to stuff that pointer into, because the load order of the
framebuffer and lcd/backlight modules are not important (that's the reason for
the notification chain), and at the time l/b modules are loaded there can be
even no corresponding platform device (on my PDA for example, where platform
device is also registered from a module).

How about passing a pointer to struct dev, and a pointer to struct fbinfo to
every l/b driver and asking them if they are for this device or not? The
lcd/backlight device then could check anything they like inside the structs
and make their decision - they are bound to this fb device or not. This way,
the class_find_device() patch would not be needed anymore, and
lcd_find_device replaced instead by something like this:

struct lcd_device *lcd_device_find (struct device *ld, struct fbinfo *fb)
{
	struct class_device *class_dev;
	struct lcd_device *found = NULL;

	down_read (&lcd_class->subsys.rwsem);
	list_for_each_entry (class_dev, &lcd_class->children, node) {
		struct lcd_device *ld = to_lcd_device (class_dev);
		if (ld->props && (ld->props->check_device (ld, fb) == 0)) {
			found = lcd_device_get (ld);
			break;
		}
	}
	up_read (&lcd_class->subsys.rwsem);

	return found;
}

--
Greetings,
   Andrew

Re: Backlight and LCD module patches [1]

[Greg KH]

On Fri, Jun 18, 2004 at 09:55:59AM +0400, Andrew Zabolotny wrote:
> On Thu, 17 Jun 2004 15:05:10 -0700
> Greg KH <greg@kroah.com> wrote:
> > Then you need to have a way to corrispond those devices together,
> > becides just a name.  Use the pointer that you have provided to link
> > them together some way.
> 
> There's no place to stuff that pointer into, because the load order of the
> framebuffer and lcd/backlight modules are not important (that's the reason for
> the notification chain), and at the time l/b modules are loaded there can be
> even no corresponding platform device (on my PDA for example, where platform
> device is also registered from a module).
> 
> How about passing a pointer to struct dev, and a pointer to struct fbinfo to
> every l/b driver and asking them if they are for this device or not?

Ick, no.

How about just having every l/b driver containing a pointer to the
fbinfo that it is associated with?  Isn't there some way you can keep
the pointer that you need around within the place that you need to use
it from eventually?

thanks,

greg k-h

Re: Backlight and LCD module patches [1]

[Andrew Zabolotny]

On Thu, 24 Jun 2004 14:34:52 -0700
Greg KH <greg@kroah.com> wrote:

> How about just having every l/b driver containing a pointer to the
> fbinfo that it is associated with?  Isn't there some way you can keep
> the pointer that you need around within the place that you need to use
> it from eventually?
It's not a question of b/l driver needing the framebuffer driver; it's the
other way around: the framebuffer driver needs the b/l drivers (needs so
much that it can fail initialization in some cases if it doesn't find the
corresponding b/l device). Framebuffer interface has some functionality that
is directly related to b/l, notably the 'un/blank screen' method, and also
at initialization it would be very nice to turn on lcd and the backlight,
otherwise the user wouldn't see anything.

One possible solution would be to place the b/l pointers in the
'platform_data' field of the framebuffer device, but it's usable only for
platform devices, and even there not very handy (because the code registering
the framebuffer platform device would need to know about the b/l drivers, what
if b/l is a module that's not yet loaded?). In the case where devices are
registered by the bus enumerator the things are even worse.

After some discussion on IRC here's a slightly different proposal: During
framebuffer driver initialization it calls lcd_find_device(struct device*),
passing him the 'struct device' of the framebuffer device. The lcd or the
backlight driver looks inside the struct device (notably at the bus, bus_id
and other related fields) and compares them with whatever it expects them to
be (for example, a platform device-based lcd driver could compare
dev->bus_type with &platform_bus_type, a PCI driver could study the PCI device
ids and so on). And it returns either 0 (success) or -ENODEV (failure).

Reasoning: There isn't any static way to find out which b/l device corresponds
to a given framebuffer device. This is something that only the b/l driver
knows. For example, the b/l controls for a PCI video card could be embedded
into the PCI card; for palmtops they are implemented absolutely differently
(e.g. through auxiliary GPIOs, controlled by unrelated ASICs and so on). So
deciding the correspondence between the b/l driver and the framebuffer device
is the responsability of b/l driver: there is simply no other driver that
knows that.

If you'll ask why not embed the b/l controls directly into the framebuffer
drivers, the reason is simple: some video controllers just don't have a
predefined way of controlling the b/l, so in every implementation it's
different. Example: many PDAs use the PXA2xx CPU embedded video controller
(drivers/video/pxafb.c), but every PDA has his own way of controlling the
backlight and lcd. So the only solution here is to make pxafb ask "who knows
how to handle the backlight/lcd for *this* device" and get a pointer to the
b/l drivers.

--
Greetings,
   Andrew

Re: Backlight and LCD module patches [1]

[Greg KH]

On Sun, Jun 27, 2004 at 12:21:52AM +0400, Andrew Zabolotny wrote:
> On Thu, 24 Jun 2004 14:34:52 -0700
> Greg KH <greg@kroah.com> wrote:
> 
> > How about just having every l/b driver containing a pointer to the
> > fbinfo that it is associated with?  Isn't there some way you can keep
> > the pointer that you need around within the place that you need to use
> > it from eventually?
> It's not a question of b/l driver needing the framebuffer driver; it's the
> other way around: the framebuffer driver needs the b/l drivers (needs so
> much that it can fail initialization in some cases if it doesn't find the
> corresponding b/l device).

Ok, then put a pointer in the fb driver to the backlight.
And a pointer in the backlight to the fb.  What's wrong with that?

> If you'll ask why not embed the b/l controls directly into the framebuffer
> drivers, the reason is simple: some video controllers just don't have a
> predefined way of controlling the b/l, so in every implementation it's
> different.

Just do it for the ones that you do know, what's wrong with that?

thanks,

greg k-h

Re: Backlight and LCD module patches [1]

[Andrew Zabolotny]

On Tue, 13 Jul 2004 23:11:38 -0700
Greg KH <greg@kroah.com> wrote:

> > It's not a question of b/l driver needing the framebuffer driver; it's the
> > other way around: the framebuffer driver needs the b/l drivers (needs so
> > much that it can fail initialization in some cases if it doesn't find the
> > corresponding b/l device).
> Ok, then put a pointer in the fb driver to the backlight.
> And a pointer in the backlight to the fb.  What's wrong with that?
Then arises the same question, but upside down. How the backlight driver will
find the corresponding framebuffer device to put a pointer to himself into?

For example, mediaq 11xx chip can be connected to the PCI bus (apart from the
fact that it can be connected to embedded CPUs directly), so suppose there are
several PCI cards, every card has a LCD connected to it. Now you have a bl/lcd
driver that can drive those LCDs; how you can know which LCD is connected to
which framebuffer? You will have to do the same: examine the device structure
and find the PCI device id, slot number and such - there are simply no other
ways.

So I basically propose the same way - but unified from the bl/lcd driver
perspective: bl/lcd looks at the framebuffer device structure and decides
if it corresponds to the respective device or not. But this operation is
initiated by the framebuffer device, not by bl/lcd, since the bl/lcd
infrastructure already has a list of all bl/lcd drivers.

--
Greetings,
   Andrew