ACPI output/lcd/auxdisplay mess

ACPI output/lcd/auxdisplay mess

[James Simmons]

I have noticed a bunch of patches coming in dealing with the same problem. How to handle displays 
i.e LCD, CRT etc. We have a lcd class in video/backlight which no one is using. Because no one was
aware of it auxdisplay was created instead. Then we have the acpi code which wants a output class
to handle powering down the display device. Plus the acpi layer does something very similar to the
framebuffer with getting edids and data relating to the display device. We really should place all
this handling in one standard place. To do this I need to know what all of you require.

Re: ACPI output/lcd/auxdisplay mess

[Miguel Ojeda]

On 11/14/06, James Simmons <jsimmons@infradead.org> wrote:
>
> I have noticed a bunch of patches coming in dealing with the same problem. How to handle displays
> i.e LCD, CRT etc. We have a lcd class in video/backlight which no one is using. Because no one was
> aware of it auxdisplay was created instead. Then we have the acpi code which wants a output class

Well, we were aware of video/backlight/* (read below). Anyway,
auxdisplay doesn't create a class; it did in first versions, but right
now it behaves just like a framebuffer, no classes in the playground
(maybe you read a old version?).

> to handle powering down the display device. Plus the acpi layer does something very similar to the
> framebuffer with getting edids and data relating to the display device. We really should place all
> this handling in one standard place. To do this I need to know what all of you require.
>

auxdisplay was discussed and created because lcd/backlight mean a primary LCD:

"Backlight & LCD device support"
	  Enable this to be able to choose the drivers for controlling the
	  backlight and the LCD panel on some platforms, for example on PDAs.

However, auxdisplay means "auxiliary display device drivers", not _the
display_. In such folder we can put every
auxiliar-optional-secundary-rare display (not just LCDs, framebuffers,
...) who has special requirements (like parport wiring, fixed refresh
rate, different properties...). Also, things like "set_contrast",
"max_constrast", "set_power"... didn't seem very appropriate.

There was a long discussion about this stuff and someone pointed out
the existence of video/backlight/*, so, well, we were aware of such
it. I think people prefered drivers/auxdisplay.

As people wish,

       Miguel Ojeda

-- 
Miguel Ojeda
http://maxextreme.googlepages.com/index.htm

Re: ACPI output/lcd/auxdisplay mess

[James Simmons]

> Well, we were aware of video/backlight/* (read below). Anyway,
> auxdisplay doesn't create a class; it did in first versions, but right
> now it behaves just like a framebuffer, no classes in the playground
> (maybe you read a old version?).
...
> However, auxdisplay means "auxiliary display device drivers", not _the
> display_. In such folder we can put every
> auxiliar-optional-secundary-rare display (not just LCDs, framebuffers,
> ...) who has special requirements (like parport wiring, fixed refresh
> rate, different properties...). Also, things like "set_contrast",
> "max_constrast", "set_power"... didn't seem very appropriate.

Is it a framebuffer device ? The framebuffer layer is abstracted to work 
with such devices.

Re: ACPI output/lcd/auxdisplay mess

[Miguel Ojeda]

On 11/15/06, James Simmons <jsimmons@infradead.org> wrote:
>
> > Well, we were aware of video/backlight/* (read below). Anyway,
> > auxdisplay doesn't create a class; it did in first versions, but right
> > now it behaves just like a framebuffer, no classes in the playground
> > (maybe you read a old version?).
> ...
> > However, auxdisplay means "auxiliary display device drivers", not _the
> > display_. In such folder we can put every
> > auxiliar-optional-secundary-rare display (not just LCDs, framebuffers,
> > ...) who has special requirements (like parport wiring, fixed refresh
> > rate, different properties...). Also, things like "set_contrast",
> > "max_constrast", "set_power"... didn't seem very appropriate.
>
> Is it a framebuffer device ? The framebuffer layer is abstracted to work
> with such devices.
>

cfag12864bcfb is a "fbdev" (actually, it is a "fb wrapper" for
cfag12864b, so it behaves like a framebuffer, although it is not an
usual framebuffer. f.e. it has asynchronous refresh rate, a mmaped
page to appear to be a fb...).

Still, it is not the front panel lcd of any specific device like PDA,
so people that expects only their primary video/ displays may be
confused if it appears at such section. So we decided to go away from
video/. Maybe we can change the description, as right now it only
refers to front panel lcds.

-- 
Miguel Ojeda
http://maxextreme.googlepages.com/index.htm

Re: ACPI output/lcd/auxdisplay mess

[James Simmons]

> > Is it a framebuffer device ? The framebuffer layer is abstracted to work
> > with such devices.
> > 
> 
> cfag12864bcfb is a "fbdev" (actually, it is a "fb wrapper" for
> cfag12864b, so it behaves like a framebuffer, although it is not an
> usual framebuffer. f.e. it has asynchronous refresh rate, a mmaped
> page to appear to be a fb...).

BTW to use it as a fb you need to set the FILLRECT etc. See Kconfig in the 
drivers/video directory and look at one of the graphic card examples.
 
> Still, it is not the front panel lcd of any specific device like PDA,
> so people that expects only their primary video/ displays may be
> confused if it appears at such section. So we decided to go away from
> video/. Maybe we can change the description, as right now it only
> refers to front panel lcds.

  Neither is a monitor for a PC desktop. That is why we have ddc. If I 
take a desktop with more than one video card and swap the lcd monitors 
the lcd monitor data remains the same. As soon as the display device is 
attached to the graphics card the graphics card will then communicate 
with the monitor to retrieve data. For example if the mode of the 
graphics card is set to 1900x1080 which is supported by the current 
monitor. Then we swap it for a CRT that supports only 1280x1024 then in 
that case when the graphics card probes the CRT it will change the 
resolution to the maximum that is supported by the CRT. 
  Currently the fbdev layer handles all this with struct fb_monspecs. Now
I know that structure doesn't cover everything. Nor does it handle 
multiple displays attached to one piece of hardware. These where things I 
was hoping to fix. Now that there are display devices that can handle 
there own power management I have no problem having another sysfs device
to handle it. A representation that is more generic than lcd in the 
backlight directory. Like the output device suggested by Yu. Of course I'm 
not fond of that name. Display would be better. 

Re: ACPI output/lcd/auxdisplay mess

[Miguel Ojeda]

On 11/16/06, James Simmons <jsimmons@infradead.org> wrote:
>
> >
> > cfag12864bcfb is a "fbdev" (actually, it is a "fb wrapper" for
> > cfag12864b, so it behaves like a framebuffer, although it is not an
> > usual framebuffer. f.e. it has asynchronous refresh rate, a mmaped
> > page to appear to be a fb...).
>
> BTW to use it as a fb you need to set the FILLRECT etc. See Kconfig in the
> drivers/video directory and look at one of the graphic card examples.

I see, thanks you. I will take care of that.

>
> > Still, it is not the front panel lcd of any specific device like PDA,
> > so people that expects only their primary video/ displays may be
> > confused if it appears at such section. So we decided to go away from
> > video/. Maybe we can change the description, as right now it only
> > refers to front panel lcds.
>
>   Neither is a monitor for a PC desktop. That is why we have ddc. If I
> take a desktop with more than one video card and swap the lcd monitors
> the lcd monitor data remains the same. As soon as the display device is
> attached to the graphics card the graphics card will then communicate
> with the monitor to retrieve data. For example if the mode of the
> graphics card is set to 1900x1080 which is supported by the current
> monitor. Then we swap it for a CRT that supports only 1280x1024 then in
> that case when the graphics card probes the CRT it will change the
> resolution to the maximum that is supported by the CRT.
>   Currently the fbdev layer handles all this with struct fb_monspecs. Now
> I know that structure doesn't cover everything. Nor does it handle
> multiple displays attached to one piece of hardware. These where things I
> was hoping to fix. Now that there are display devices that can handle
> there own power management I have no problem having another sysfs device
> to handle it. A representation that is more generic than lcd in the
> backlight directory. Like the output device suggested by Yu. Of course I'm
> not fond of that name. Display would be better.
>

Yep, indeed it would be better to have both generic place and sysfs
device to put all these generic displays, however, I think they
shouldn't be at video/* (maybe video/something/* or outside). I mean,
we can do something like:

1. First, we move auxiliary-special displays (including cfag12864b,
[*]"Arc Monochrome LCD board" and stuff like that: they aren't really
primary video displays) to another folder outside video/, maybe
"drivers/display/" ("drivers/auxdisplay/"...).
2. Then we create a sysfs device, called "display" ("auxdisplay"...)
for all of them, which must be generic as you suggested, not just lcds
or "front panels".

So finally we will have usual-primary fbdevs and video drivers at
"drivers/video/*", and all the other stuff at "drivers/display/*" or
some similar place.

Is that a good suggestion?

[*] I saw fbdevs like "Arc Monochorme LCD board support" that can be
put there the same way like auxdisplay/cfag12864b, as they look pretty
similar.

-- 
Miguel Ojeda
http://maxextreme.googlepages.com/index.htm

Display class

[James Simmons]

This is the pass at a display class to meet the needs of the output class 
of Mr. Yu for acpi. Also this class could in time replace the lcd class 
located in the backlight directory since a lcd is a type of display.
The final hope is that the purpose auxdisplay could fall under this 
catergory.

P.S
   I know the edid parsing would have to be pulled out of the fbdev layer.


diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/acpi/video.c fbdev-2.6/drivers/acpi/video.c
--- linus-2.6/drivers/acpi/video.c	2006-11-07 05:38:34.000000000 -0500
+++ fbdev-2.6/drivers/acpi/video.c	2006-12-04 10:40:48.000000000 -0500
@@ -30,6 +30,9 @@
 #include <linux/list.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#if defined(CONFIG_DISPLAY_DDC)
+#include <linux/display.h> 
+#endif
 
 #include <asm/uaccess.h>
 
@@ -154,6 +157,20 @@
 	int *levels;
 };
 
+#if defined(CONFIG_DISPLAY_DDC)
+static int
+acpi_display_get_power(struct display_device *dev)
+{
+	return 0;
+}
+
+static int
+acpi_display_set_power(struct display_device *dev)
+{
+	return 0;
+}
+#endif
+
 struct acpi_video_device {
 	unsigned long device_id;
 	struct acpi_video_device_flags flags;
@@ -162,6 +179,10 @@
 	struct acpi_video_bus *video;
 	struct acpi_device *dev;
 	struct acpi_video_device_brightness *brightness;
+#if defined(CONFIG_DISPLAY_DDC)
+	struct display_properties acpi_display_properties;
+	struct display_device *display;
+#endif
 };
 
 /* bus */
@@ -399,6 +420,27 @@
 	return status;
 }
 
+#if defined(CONFIG_DISPLAY_DDC)
+static void*
+acpi_display_get_EDID(struct acpi_video_device *dev)
+{
+	union acpi_object *edid = NULL;
+	void *data = NULL;
+	int status;
+
+	status = acpi_video_device_EDID(dev, &edid, 128);
+	if (ACPI_FAILURE(status))
+		status = acpi_video_device_EDID(dev, &edid, 256);
+
+	if (ACPI_SUCCESS(status)) {
+		if (edid && edid->type == ACPI_TYPE_BUFFER) {
+			data = edid->buffer.pointer;
+		}
+	}
+	return data;
+}
+#endif
+
 /* bus */
 
 static int
@@ -1255,7 +1297,6 @@
 	int status;
 	struct acpi_video_device *data;
 
-
 	if (!device || !video)
 		return -EINVAL;
 
@@ -1263,12 +1304,10 @@
 	    acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
 	if (ACPI_SUCCESS(status)) {
 
-		data = kmalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
+		data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
 		if (!data)
 			return -ENOMEM;
 
-		memset(data, 0, sizeof(struct acpi_video_device));
-
 		strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
 		strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
 		acpi_driver_data(device) = data;
@@ -1295,6 +1334,13 @@
 		acpi_video_device_bind(video, data);
 		acpi_video_device_find_cap(data);
 
+#ifdef CONFIG_DISPLAY_DDC
+		data->acpi_display_properties.get_power = acpi_display_get_power;
+		data->acpi_display_properties.set_power = acpi_display_set_power;
+		data->acpi_display_properties.probe = probe_edid;
+		data->display = display_device_register(NULL, acpi_display_get_EDID(data),
+							&data->acpi_display_properties);		
+#endif
 		status = acpi_install_notify_handler(device->handle,
 						     ACPI_DEVICE_NOTIFY,
 						     acpi_video_device_notify,
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/display-ddc.c fbdev-2.6/drivers/video/display/display-ddc.c
--- linus-2.6/drivers/video/display/display-ddc.c	1969-12-31 19:00:00.000000000 -0500
+++ fbdev-2.6/drivers/video/display/display-ddc.c	2006-12-03 05:27:00.000000000 -0500
@@ -0,0 +1,45 @@
+/*
+ *  display-ddc.c - EDID paring code
+ *
+ *  Copyright (C) 2006 James Simmons <jsimmons@infradead.org>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or (at
+ *  your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+#include <linux/module.h>
+#include <linux/display.h>
+#include <linux/err.h>
+#include <linux/ctype.h>
+
+#include <linux/fb.h>
+
+int probe_edid(struct display_device *dev, void *data)
+{
+	struct fb_monspecs spec;
+	ssize_t size = 45;
+
+	dev->name = kzalloc(size, GFP_KERNEL);
+	fb_edid_to_monspecs((unsigned char *) data, &spec);
+	strcpy(dev->name, spec.manufacturer);
+	return snprintf(dev->name, size, "%s %s %s\n", spec.manufacturer, spec.monitor, spec.ascii);
+}
+EXPORT_SYMBOL(probe_edid);
+
+MODULE_DESCRIPTION("Display Hardware handling");
+MODULE_AUTHOR("James Simmons <jsimmons@infradead.org>");
+MODULE_LICENSE("GPL");
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/display-sysfs.c fbdev-2.6/drivers/video/display/display-sysfs.c
--- linus-2.6/drivers/video/display/display-sysfs.c	1969-12-31 19:00:00.000000000 -0500
+++ fbdev-2.6/drivers/video/display/display-sysfs.c	2006-12-04 07:48:34.000000000 -0500
@@ -0,0 +1,164 @@
+/*
+ *  display.c - Display output driver
+ *
+ *  Copyright (C) 2006 James Simmons <jsimmons@infradead.org>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or (at
+ *  your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+#include <linux/module.h>
+#include <linux/display.h>
+#include <linux/err.h>
+#include <linux/ctype.h>
+
+static ssize_t display_show_name(struct class_device *dev, char *buf)
+{
+	struct display_device *dsp = to_display_device(dev);
+	return snprintf(buf, PAGE_SIZE, "%s\n", dsp->name);
+}
+
+static ssize_t display_show_power(struct class_device *dev, char *buf)
+{
+	struct display_device *dsp = to_display_device(dev);
+	ssize_t ret = -ENXIO;
+
+	mutex_lock(&dsp->lock);
+	if (likely(dsp->driver->get_power))
+		ret = sprintf(buf,"%.8x\n", dsp->driver->get_power(dsp));
+	mutex_unlock(&dsp->lock);
+	return ret;
+}
+
+static ssize_t display_store_power(struct class_device *dev,
+	const char *buf,size_t count)
+{
+	struct display_device *dsp = to_display_device(dev);
+	size_t size;
+	char *endp;
+	int power;
+
+	power = simple_strtoul(buf, &endp, 0);
+	size = endp - buf;
+	if (*endp && isspace(*endp))
+		size++;
+	if (size != count)
+		return -EINVAL;
+
+	mutex_lock(&dsp->lock);
+	if (likely(dsp->driver->set_power)) {
+		dsp->request_state = power;
+		dsp->driver->set_power(dsp);
+	}
+	mutex_unlock(&dsp->lock);
+	return count;
+}
+
+static void display_class_release(struct class_device *dev)
+{
+	struct display_device *dsp = to_display_device(dev);
+	kfree(dsp);
+}
+
+static struct class_device_attribute display_attributes[] = {
+	__ATTR(name, S_IRUGO, display_show_name, NULL),
+	__ATTR(power, S_IRUGO | S_IWUSR, display_show_power, display_store_power),
+};
+
+static struct class display_class = {
+	.name = "display",
+	.release = display_class_release,
+	.class_dev_attrs = display_attributes,
+};
+
+static int index;
+
+struct display_device *display_device_register(struct display_driver *driver,
+						struct device *dev, void *devdata)
+{
+	struct display_device *new_dev;
+	int ret = -ENOMEM, i;
+
+	if (unlikely(!driver))
+		return ERR_PTR(-EINVAL);
+
+	new_dev = kzalloc(sizeof(struct display_device), GFP_KERNEL);
+	if (likely(new_dev)) {
+		driver->probe(new_dev, devdata);
+
+		mutex_init(&new_dev->lock);
+		new_dev->driver = driver;
+		new_dev->class_dev.class = &display_class;
+		new_dev->class_dev.dev = dev;
+		sprintf(new_dev->class_dev.class_id, "display%d", index++);
+		class_set_devdata(&new_dev->class_dev, devdata);
+		ret = class_device_register(&new_dev->class_dev);
+
+		if (likely(ret)) {
+			for (i = 0; i < ARRAY_SIZE(display_attributes); i++) {
+				ret = class_device_create_file(&new_dev->class_dev,
+								&display_attributes[i]);
+				if (unlikely(ret)) {
+					while (--i >= 0)
+						class_device_remove_file(&new_dev->class_dev,
+								&display_attributes[i]);
+					class_device_unregister(&new_dev->class_dev);
+				}
+			}
+		}
+	}
+	if (unlikely(ret)) {
+		printk("failed to register display device\n");
+		kfree(new_dev);
+		new_dev = ERR_PTR(ret);
+	}
+	return new_dev;
+}
+EXPORT_SYMBOL(display_device_register);
+
+void display_device_unregister(struct display_device *dev)
+{
+	int i;
+
+	if (!dev)
+		return;
+	mutex_lock(&dev->lock);
+	dev->driver = NULL;
+	for (i = 0; i < ARRAY_SIZE(display_attributes); i++)
+		class_device_remove_file(&dev->class_dev,
+					&display_attributes[i]);
+	mutex_unlock(&dev->lock);
+	class_device_unregister(&dev->class_dev);
+}
+EXPORT_SYMBOL(display_device_unregister);
+
+static void __exit display_class_exit(void)
+{
+	class_unregister(&display_class);
+}
+
+static int __init display_class_init(void)
+{
+	return class_register(&display_class);
+}
+
+postcore_initcall(display_class_init);
+module_exit(display_class_exit);
+
+MODULE_DESCRIPTION("Display Hardware handling");
+MODULE_AUTHOR("James Simmons <jsimmons@infradead.org>");
+MODULE_LICENSE("GPL");
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/Kconfig fbdev-2.6/drivers/video/display/Kconfig
--- linus-2.6/drivers/video/display/Kconfig	1969-12-31 19:00:00.000000000 -0500
+++ fbdev-2.6/drivers/video/display/Kconfig	2006-12-04 11:41:03.000000000 -0500
@@ -0,0 +1,29 @@
+#
+# Display drivers configuration
+#
+
+menu "Display device support"
+
+config DISPLAY_SUPPORT
+	tristate "Display panel/monitor support"
+	---help---
+	  This framework adds support for low-level control of a display. 
+	  This includes support for power.
+
+	  Enable this to be able to choose the drivers for controlling the
+	  physical display panel/monitor on some platforms. This not only
+	  covers LCD displays for PDAs but also other types of displays
+	  such as CRT, TVout etc.
+
+	  To have support for your specific display panel you will have to
+	  select the proper drivers which depend on this option.
+
+config DISPLAY_DDC
+	tristate "Enable EDID parsing"
+	---help---
+	  Enable EDID parsing
+
+comment "Display hardware drivers"
+	depends on DISPLAY_SUPPORT
+
+endmenu
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/Makefile fbdev-2.6/drivers/video/display/Makefile
--- linus-2.6/drivers/video/display/Makefile	1969-12-31 19:00:00.000000000 -0500
+++ fbdev-2.6/drivers/video/display/Makefile	2006-12-03 05:02:07.000000000 -0500
@@ -0,0 +1,7 @@
+# Display drivers
+
+display-objs				:= display-sysfs.o 
+
+obj-$(CONFIG_DISPLAY_SUPPORT)		+= display.o
+
+obj-$(CONFIG_DISPLAY_DDC)		+= display-ddc.o
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/Kconfig fbdev-2.6/drivers/video/Kconfig
--- linus-2.6/drivers/video/Kconfig	2006-11-30 05:06:13.000000000 -0500
+++ fbdev-2.6/drivers/video/Kconfig	2006-12-04 10:40:48.000000000 -0500
@@ -4,21 +4,6 @@
 
 menu "Graphics support"
 
-config FIRMWARE_EDID
-       bool "Enable firmware EDID"
-       default y
-       ---help---
-         This enables access to the EDID transferred from the firmware.
-	 On the i386, this is from the Video BIOS. Enable this if DDC/I2C
-	 transfers do not work for your driver and if you are using
-	 nvidiafb, i810fb or savagefb.
-
-	 In general, choosing Y for this option is safe.  If you
-	 experience extremely long delays while booting before you get
-	 something on your display, try setting this to N.  Matrox cards in
-	 combination with certain motherboards and monitors are known to
-	 suffer from this problem.
-
 config FB
 	tristate "Support for frame buffer devices"
 	---help---
@@ -53,6 +38,22 @@
 	  (e.g. an accelerated X server) and that are not frame buffer
 	  device-aware may cause unexpected results. If unsure, say N.
 
+config FIRMWARE_EDID
+       bool "Enable firmware EDID"
+       depends on FB
+       default n 
+       ---help---
+         This enables access to the EDID transferred from the firmware.
+	 On the i386, this is from the Video BIOS. Enable this if DDC/I2C
+	 transfers do not work for your driver and if you are using
+	 nvidiafb, i810fb or savagefb.
+
+	 In general, choosing Y for this option is safe.  If you
+	 experience extremely long delays while booting before you get
+	 something on your display, try setting this to N.  Matrox cards in
+	 combination with certain motherboards and monitors are known to
+	 suffer from this problem.
+
 config FB_DDC
        tristate
        depends on FB && I2C && I2C_ALGOBIT
@@ -126,6 +127,9 @@
 	 This is particularly important to one driver, matroxfb.  If
 	 unsure, say N.
 
+comment "Frambuffer hardware drivers"
+	depends on FB
+
 config FB_CIRRUS
 	tristate "Cirrus Logic support"
 	depends on FB && (ZORRO || PCI)
@@ -1641,6 +1647,7 @@
 endif
 
 if SYSFS
+	source "drivers/video/display/Kconfig"
 	source "drivers/video/backlight/Kconfig"
 endif
 
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/Makefile fbdev-2.6/drivers/video/Makefile
--- linus-2.6/drivers/video/Makefile	2006-11-07 05:38:36.000000000 -0500
+++ fbdev-2.6/drivers/video/Makefile	2006-12-04 10:40:48.000000000 -0500
@@ -12,7 +12,7 @@
 
 obj-$(CONFIG_VT)		  += console/
 obj-$(CONFIG_LOGO)		  += logo/
-obj-$(CONFIG_SYSFS)		  += backlight/
+obj-$(CONFIG_SYSFS)		  += backlight/ display/
 
 obj-$(CONFIG_FB_CFB_FILLRECT)  += cfbfillrect.o
 obj-$(CONFIG_FB_CFB_COPYAREA)  += cfbcopyarea.o
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/include/linux/display.h fbdev-2.6/include/linux/display.h
--- linus-2.6/include/linux/display.h	1969-12-31 19:00:00.000000000 -0500
+++ fbdev-2.6/include/linux/display.h	2006-12-04 07:42:29.000000000 -0500
@@ -0,0 +1,55 @@
+/*
+ *  Copyright (C) 2006 James Simmons <jsimmons@infradead.org>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or (at
+ *  your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#ifndef _LINUX_DISPLAY_H
+#define _LINUX_DISPLAY_H
+
+#include <linux/device.h>
+
+struct display_device;
+
+/* This structure defines all the properties of a Display. */
+struct display_driver {
+	int (*set_power)(struct display_device *);
+	int (*get_power)(struct display_device *);
+	int (*probe)(struct display_device *, void *);
+	int (*remove)(struct display_device *);
+};
+
+struct display_device {
+	struct module *owner;	/* Owner module */
+	char *name;
+	struct mutex lock;	
+	int request_state;
+	struct display_driver *driver;
+	struct class_device class_dev;		/* The class device structure */
+};
+
+extern struct display_device *display_device_register(struct display_driver *driver,
+					struct device *dev, void *devdata);
+extern void display_device_unregister(struct display_device *dev);
+
+extern int probe_edid(struct display_device *dev, void *devdata);
+
+#define to_display_device(obj) container_of(obj, struct display_device, class_dev)
+
+#endif
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/MAINTAINERS fbdev-2.6/MAINTAINERS
--- linus-2.6/MAINTAINERS	2006-12-04 04:11:38.000000000 -0500
+++ fbdev-2.6/MAINTAINERS	2006-12-04 10:40:48.000000000 -0500
@@ -911,6 +923,12 @@
 L:	linux-kernel@vger.kernel.org
 S:	Maintained
 
+DISPLAY SUBSYSTEM
+P:	James Simmons
+M:	jsimmons@infradead.org
+L:	linux-kernel@vger.kernel.org
+S:	Maintained
+
 DISTRIBUTED LOCK MANAGER
 P:	Patrick Caulfield
 M:	pcaulfie@redhat.com

Re: Display class

[Randy Dunlap]

On Tue, 5 Dec 2006 18:03:39 +0000 (GMT) James Simmons wrote:

> This is the pass at a display class to meet the needs of the output class 
> of Mr. Yu for acpi. Also this class could in time replace the lcd class 
> located in the backlight directory since a lcd is a type of display.
> The final hope is that the purpose auxdisplay could fall under this 
> catergory.
> 
> P.S
>    I know the edid parsing would have to be pulled out of the fbdev layer.

Hi,

Where is "struct display_properties" defined?

  CC [M]  drivers/acpi/video.o
drivers/acpi/video.c:183: error: field 'acpi_display_properties' has incomplete type
make[2]: *** [drivers/acpi/video.o] Error 1

Lots of style issues (see below).

> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/acpi/video.c fbdev-2.6/drivers/acpi/video.c
> --- linus-2.6/drivers/acpi/video.c	2006-11-07 05:38:34.000000000 -0500
> +++ fbdev-2.6/drivers/acpi/video.c	2006-12-04 10:40:48.000000000 -0500
> @@ -30,6 +30,9 @@
>  #include <linux/list.h>
>  #include <linux/proc_fs.h>
>  #include <linux/seq_file.h>
> +#if defined(CONFIG_DISPLAY_DDC)
> +#include <linux/display.h> 
> +#endif
>  
>  #include <asm/uaccess.h>
>  
> @@ -154,6 +157,20 @@
>  	int *levels;
>  };
>  
> +#if defined(CONFIG_DISPLAY_DDC)
> +static int
> +acpi_display_get_power(struct display_device *dev)

Combine those 2 lines into 1 line.

> +{
> +	return 0;
> +}
> +
> +static int
> +acpi_display_set_power(struct display_device *dev)

Ditto.

> +{
> +	return 0;
> +}
> +#endif
> +
>  struct acpi_video_device {
>  	unsigned long device_id;
>  	struct acpi_video_device_flags flags;
> @@ -162,6 +179,10 @@
>  	struct acpi_video_bus *video;
>  	struct acpi_device *dev;
>  	struct acpi_video_device_brightness *brightness;
> +#if defined(CONFIG_DISPLAY_DDC)
> +	struct display_properties acpi_display_properties;
> +	struct display_device *display;
> +#endif
>  };
>  
>  /* bus */
> @@ -399,6 +420,27 @@
>  	return status;
>  }
>  
> +#if defined(CONFIG_DISPLAY_DDC)
> +static void*
> +acpi_display_get_EDID(struct acpi_video_device *dev)

Combine and place '*' immediately before the function name,
not immediately after "void".

> +{
> +	union acpi_object *edid = NULL;
> +	void *data = NULL;
> +	int status;
> +
> +	status = acpi_video_device_EDID(dev, &edid, 128);
> +	if (ACPI_FAILURE(status))
> +		status = acpi_video_device_EDID(dev, &edid, 256);
> +
> +	if (ACPI_SUCCESS(status)) {
> +		if (edid && edid->type == ACPI_TYPE_BUFFER) {
> +			data = edid->buffer.pointer;

No braces for one-statement "blocks".

> +		}
> +	}
> +	return data;
> +}
> +#endif
> +
>  /* bus */
>  
>  static int

> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/display-ddc.c fbdev-2.6/drivers/video/display/display-ddc.c
> --- linus-2.6/drivers/video/display/display-ddc.c	1969-12-31 19:00:00.000000000 -0500
> +++ fbdev-2.6/drivers/video/display/display-ddc.c	2006-12-03 05:27:00.000000000 -0500
> @@ -0,0 +1,45 @@
> +/*
> + *  display-ddc.c - EDID paring code

typo:  parsing

> +#include <linux/module.h>
> +#include <linux/display.h>
> +#include <linux/err.h>
> +#include <linux/ctype.h>
> +
> +#include <linux/fb.h>
> +
> +int probe_edid(struct display_device *dev, void *data)
> +{
> +	struct fb_monspecs spec;
> +	ssize_t size = 45;

Where does the magic # 45 come from?
Can you use a #define or sizeof(X) for it instead?

> +
> +	dev->name = kzalloc(size, GFP_KERNEL);
> +	fb_edid_to_monspecs((unsigned char *) data, &spec);
> +	strcpy(dev->name, spec.manufacturer);
> +	return snprintf(dev->name, size, "%s %s %s\n", spec.manufacturer, spec.monitor, spec.ascii);

Try to limit source lines to < 80 columns.

> +}
> +EXPORT_SYMBOL(probe_edid);
> +
> +MODULE_DESCRIPTION("Display Hardware handling");
> +MODULE_AUTHOR("James Simmons <jsimmons@infradead.org>");
> +MODULE_LICENSE("GPL");

> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/display-sysfs.c fbdev-2.6/drivers/video/display/display-sysfs.c
> --- linus-2.6/drivers/video/display/display-sysfs.c	1969-12-31 19:00:00.000000000 -0500
> +++ fbdev-2.6/drivers/video/display/display-sysfs.c	2006-12-04 07:48:34.000000000 -0500
> @@ -0,0 +1,164 @@
> +#include <linux/module.h>
> +#include <linux/display.h>
> +#include <linux/err.h>
> +#include <linux/ctype.h>
> +
> +struct display_device *display_device_register(struct display_driver *driver,
> +						struct device *dev, void *devdata)
> +{
> +	struct display_device *new_dev;
> +	int ret = -ENOMEM, i;
> +
> +	if (unlikely(!driver))
> +		return ERR_PTR(-EINVAL);
> +
> +	new_dev = kzalloc(sizeof(struct display_device), GFP_KERNEL);
> +	if (likely(new_dev)) {
> +		driver->probe(new_dev, devdata);
> +
> +		mutex_init(&new_dev->lock);
> +		new_dev->driver = driver;
> +		new_dev->class_dev.class = &display_class;
> +		new_dev->class_dev.dev = dev;
> +		sprintf(new_dev->class_dev.class_id, "display%d", index++);
> +		class_set_devdata(&new_dev->class_dev, devdata);
> +		ret = class_device_register(&new_dev->class_dev);
> +
> +		if (likely(ret)) {
> +			for (i = 0; i < ARRAY_SIZE(display_attributes); i++) {
> +				ret = class_device_create_file(&new_dev->class_dev,
> +								&display_attributes[i]);
> +				if (unlikely(ret)) {
> +					while (--i >= 0)
> +						class_device_remove_file(&new_dev->class_dev,
> +								&display_attributes[i]);
> +					class_device_unregister(&new_dev->class_dev);

Several lines > 80 columns there.

> +				}
> +			}
> +		}
> +	}
> +	if (unlikely(ret)) {
> +		printk("failed to register display device\n");

*			KERN_ERR or some such printk log level

> +		kfree(new_dev);
> +		new_dev = ERR_PTR(ret);
> +	}
> +	return new_dev;
> +}
> +EXPORT_SYMBOL(display_device_register);
> +
> +void display_device_unregister(struct display_device *dev)
> +{
> +	int i;
> +
> +	if (!dev)
> +		return;
> +	mutex_lock(&dev->lock);
> +	dev->driver = NULL;
> +	for (i = 0; i < ARRAY_SIZE(display_attributes); i++)
> +		class_device_remove_file(&dev->class_dev,
> +					&display_attributes[i]);
> +	mutex_unlock(&dev->lock);
> +	class_device_unregister(&dev->class_dev);
> +}
> +EXPORT_SYMBOL(display_device_unregister);
> +
> +static void __exit display_class_exit(void)
> +{
> +	class_unregister(&display_class);
> +}
> +
> +static int __init display_class_init(void)
> +{
> +	return class_register(&display_class);
> +}
> +
> +postcore_initcall(display_class_init);
> +module_exit(display_class_exit);
> +
> +MODULE_DESCRIPTION("Display Hardware handling");
> +MODULE_AUTHOR("James Simmons <jsimmons@infradead.org>");
> +MODULE_LICENSE("GPL");

> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/Kconfig fbdev-2.6/drivers/video/display/Kconfig
> --- linus-2.6/drivers/video/display/Kconfig	1969-12-31 19:00:00.000000000 -0500
> +++ fbdev-2.6/drivers/video/display/Kconfig	2006-12-04 11:41:03.000000000 -0500
> @@ -0,0 +1,29 @@
> +#
> +# Display drivers configuration
> +#
> +
> +menu "Display device support"
> +
> +config DISPLAY_SUPPORT
> +	tristate "Display panel/monitor support"
> +	---help---
> +	  This framework adds support for low-level control of a display. 
> +	  This includes support for power.
> +
> +	  Enable this to be able to choose the drivers for controlling the
> +	  physical display panel/monitor on some platforms. This not only
> +	  covers LCD displays for PDAs but also other types of displays
> +	  such as CRT, TVout etc.
> +
> +	  To have support for your specific display panel you will have to
> +	  select the proper drivers which depend on this option.
> +
> +config DISPLAY_DDC
> +	tristate "Enable EDID parsing"
> +	---help---
> +	  Enable EDID parsing

"Enable" makes it sound like a boolean, not a tristate....

> +
> +comment "Display hardware drivers"
> +	depends on DISPLAY_SUPPORT
> +
> +endmenu
> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/Makefile fbdev-2.6/drivers/video/display/Makefile
> --- linus-2.6/drivers/video/display/Makefile	1969-12-31 19:00:00.000000000 -0500
> +++ fbdev-2.6/drivers/video/display/Makefile	2006-12-03 05:02:07.000000000 -0500
> @@ -0,0 +1,7 @@
> +# Display drivers
> +
> +display-objs				:= display-sysfs.o 
> +
> +obj-$(CONFIG_DISPLAY_SUPPORT)		+= display.o
> +
> +obj-$(CONFIG_DISPLAY_DDC)		+= display-ddc.o
> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/Kconfig fbdev-2.6/drivers/video/Kconfig
> --- linus-2.6/drivers/video/Kconfig	2006-11-30 05:06:13.000000000 -0500
> +++ fbdev-2.6/drivers/video/Kconfig	2006-12-04 10:40:48.000000000 -0500
> @@ -4,21 +4,6 @@
>  
>  menu "Graphics support"
>  
> -config FIRMWARE_EDID
> -       bool "Enable firmware EDID"
> -       default y
> -       ---help---
> -         This enables access to the EDID transferred from the firmware.
> -	 On the i386, this is from the Video BIOS. Enable this if DDC/I2C
> -	 transfers do not work for your driver and if you are using
> -	 nvidiafb, i810fb or savagefb.
> -
> -	 In general, choosing Y for this option is safe.  If you
> -	 experience extremely long delays while booting before you get
> -	 something on your display, try setting this to N.  Matrox cards in
> -	 combination with certain motherboards and monitors are known to
> -	 suffer from this problem.
> -
>  config FB
>  	tristate "Support for frame buffer devices"
>  	---help---
> @@ -53,6 +38,22 @@
>  	  (e.g. an accelerated X server) and that are not frame buffer
>  	  device-aware may cause unexpected results. If unsure, say N.
>  
> +config FIRMWARE_EDID
> +       bool "Enable firmware EDID"
> +       depends on FB
> +       default n 
> +       ---help---
> +         This enables access to the EDID transferred from the firmware.
> +	 On the i386, this is from the Video BIOS. Enable this if DDC/I2C
> +	 transfers do not work for your driver and if you are using
> +	 nvidiafb, i810fb or savagefb.
> +

All of the help text (above & below) should be indented like the
first line above is:  one tab + 2 spaces.

> +	 In general, choosing Y for this option is safe.  If you
> +	 experience extremely long delays while booting before you get
> +	 something on your display, try setting this to N.  Matrox cards in
> +	 combination with certain motherboards and monitors are known to
> +	 suffer from this problem.
> +
>  config FB_DDC
>         tristate
>         depends on FB && I2C && I2C_ALGOBIT

---
~Randy

Re: Display class

[James Simmons]

> > of Mr. Yu for acpi. Also this class could in time replace the lcd class 
> > located in the backlight directory since a lcd is a type of display.
> > The final hope is that the purpose auxdisplay could fall under this 
> > catergory.
> > 
> > P.S
> >    I know the edid parsing would have to be pulled out of the fbdev layer.

That patch was rought draft for feedback. I applied your comments. This 
patch actually works. It includes my backlight fix as well.

diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/acpi/video.c fbdev-2.6/drivers/acpi/video.c
--- linus-2.6/drivers/acpi/video.c	2006-11-07 05:38:34.000000000 -0500
+++ fbdev-2.6/drivers/acpi/video.c	2006-12-06 05:01:22.000000000 -0500
@@ -30,6 +30,7 @@
 #include <linux/list.h>
 #include <linux/proc_fs.h>
 #include <linux/seq_file.h>
+#include <linux/display.h> 
 
 #include <asm/uaccess.h>
 
@@ -154,6 +155,19 @@
 	int *levels;
 };
 
+ 
+#if defined(CONFIG_DISPLAY_DDC) || defined(CONFIG_DISPLAY_DDC_MODULE)
+static int acpi_display_get_power(struct display_device *dev)
+{
+	return 0;
+}
+
+static int acpi_display_set_power(struct display_device *dev)
+{
+	return 0;
+}
+#endif
+
 struct acpi_video_device {
 	unsigned long device_id;
 	struct acpi_video_device_flags flags;
@@ -162,6 +176,10 @@
 	struct acpi_video_bus *video;
 	struct acpi_device *dev;
 	struct acpi_video_device_brightness *brightness;
+#if defined(CONFIG_DISPLAY_DDC) || defined(CONFIG_DISPLAY_DDC_MODULE)
+	struct display_driver acpi_display_driver;
+	struct display_device *display;
+#endif
 };
 
 /* bus */
@@ -399,6 +417,24 @@
 	return status;
 }
 
+#if defined(CONFIG_DISPLAY_DDC) || defined(CONFIG_DISPLAY_DDC_MODULE)
+static void *acpi_display_get_EDID(struct acpi_video_device *dev)
+{
+	union acpi_object *edid = NULL;
+	void *data = NULL;
+	int status;
+
+	status = acpi_video_device_EDID(dev, &edid, 128);
+	if (ACPI_FAILURE(status))
+		status = acpi_video_device_EDID(dev, &edid, 256);
+
+	if (ACPI_SUCCESS(status))
+		if (edid && edid->type == ACPI_TYPE_BUFFER)
+			data = edid->buffer.pointer;
+	return data;
+}
+#endif
+
 /* bus */
 
 static int
@@ -1255,7 +1291,6 @@
 	int status;
 	struct acpi_video_device *data;
 
-
 	if (!device || !video)
 		return -EINVAL;
 
@@ -1263,12 +1298,10 @@
 	    acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
 	if (ACPI_SUCCESS(status)) {
 
-		data = kmalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
+		data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
 		if (!data)
 			return -ENOMEM;
 
-		memset(data, 0, sizeof(struct acpi_video_device));
-
 		strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
 		strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
 		acpi_driver_data(device) = data;
@@ -1295,6 +1328,13 @@
 		acpi_video_device_bind(video, data);
 		acpi_video_device_find_cap(data);
 
+#if defined(CONFIG_DISPLAY_DDC) || defined(CONFIG_DISPLAY_DDC_MODULE)
+		data->acpi_display_driver.get_power = acpi_display_get_power;
+		data->acpi_display_driver.set_power = acpi_display_set_power;
+		data->acpi_display_driver.probe = probe_edid;
+		data->display = display_device_register(&data->acpi_display_driver,
+						NULL, acpi_display_get_EDID(data));
+#endif
 		status = acpi_install_notify_handler(device->handle,
 						     ACPI_DEVICE_NOTIFY,
 						     acpi_video_device_notify,
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/macintosh/Kconfig fbdev-2.6/drivers/macintosh/Kconfig
--- linus-2.6/drivers/macintosh/Kconfig	2006-12-05 05:07:18.000000000 -0500
+++ fbdev-2.6/drivers/macintosh/Kconfig	2006-12-06 05:01:22.000000000 -0500
@@ -123,7 +123,7 @@
 config PMAC_BACKLIGHT
 	bool "Backlight control for LCD screens"
 	depends on ADB_PMU && FB = y && (BROKEN || !PPC64)
-	select FB_BACKLIGHT
+	select BACKLIGHT_CLASS_DEVICE
 	help
 	  Say Y here to enable Macintosh specific extensions of the generic
 	  backlight code. With this enabled, the brightness keys on older
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/display-ddc.c fbdev-2.6/drivers/video/display/display-ddc.c
--- linus-2.6/drivers/video/display/display-ddc.c	1969-12-31 19:00:00.000000000 -0500
+++ fbdev-2.6/drivers/video/display/display-ddc.c	2006-12-06 04:31:24.000000000 -0500
@@ -0,0 +1,49 @@
+/*
+ *  display-ddc.c - EDID parsing code
+ *
+ *  Copyright (C) 2006 James Simmons <jsimmons@infradead.org>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or (at
+ *  your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+#include <linux/module.h>
+#include <linux/display.h>
+#include <linux/err.h>
+#include <linux/ctype.h>
+
+#include <linux/fb.h>
+
+int probe_edid(struct display_device *dev, void *data)
+{
+	struct fb_monspecs spec;
+	ssize_t size;
+
+	if (!data)
+		return 0;
+	size = sizeof(spec.manufacturer) + sizeof(spec.monitor) + sizeof(spec.ascii);
+	dev->name = kzalloc(size, GFP_KERNEL);
+	fb_edid_to_monspecs((unsigned char *) data, &spec);
+	strcpy(dev->name, spec.manufacturer);
+	return snprintf(dev->name, size, "%s %s %s\n", spec.manufacturer,
+			spec.monitor, spec.ascii);
+}
+EXPORT_SYMBOL(probe_edid);
+
+MODULE_DESCRIPTION("Display Hardware handling");
+MODULE_AUTHOR("James Simmons <jsimmons@infradead.org>");
+MODULE_LICENSE("GPL");
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/display-sysfs.c fbdev-2.6/drivers/video/display/display-sysfs.c
--- linus-2.6/drivers/video/display/display-sysfs.c	1969-12-31 19:00:00.000000000 -0500
+++ fbdev-2.6/drivers/video/display/display-sysfs.c	2006-12-06 04:32:15.000000000 -0500
@@ -0,0 +1,161 @@
+/*
+ *  display.c - Display output driver
+ *
+ *  Copyright (C) 2006 James Simmons <jsimmons@infradead.org>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or (at
+ *  your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+#include <linux/module.h>
+#include <linux/display.h>
+#include <linux/err.h>
+#include <linux/ctype.h>
+
+static ssize_t display_show_name(struct class_device *dev, char *buf)
+{
+	struct display_device *dsp = to_display_device(dev);
+	return snprintf(buf, PAGE_SIZE, "%s\n", dsp->name);
+}
+
+static ssize_t display_show_power(struct class_device *dev, char *buf)
+{
+	struct display_device *dsp = to_display_device(dev);
+	ssize_t ret = -ENXIO;
+
+	mutex_lock(&dsp->lock);
+	if (likely(dsp->driver->get_power))
+		ret = sprintf(buf,"%.8x\n", dsp->driver->get_power(dsp));
+	mutex_unlock(&dsp->lock);
+	return ret;
+}
+
+static ssize_t display_store_power(struct class_device *dev,
+	const char *buf,size_t count)
+{
+	struct display_device *dsp = to_display_device(dev);
+	size_t size;
+	char *endp;
+	int power;
+
+	power = simple_strtoul(buf, &endp, 0);
+	size = endp - buf;
+	if (*endp && isspace(*endp))
+		size++;
+	if (size != count)
+		return -EINVAL;
+
+	mutex_lock(&dsp->lock);
+	if (likely(dsp->driver->set_power)) {
+		dsp->request_state = power;
+		dsp->driver->set_power(dsp);
+	}
+	mutex_unlock(&dsp->lock);
+	return count;
+}
+
+static void display_class_release(struct class_device *dev)
+{
+	struct display_device *dsp = to_display_device(dev);
+	kfree(dsp);
+}
+
+static struct class_device_attribute display_attributes[] = {
+	__ATTR(name, S_IRUGO, display_show_name, NULL),
+	__ATTR(power, S_IRUGO | S_IWUSR, display_show_power, display_store_power),
+};
+
+static struct class display_class = {
+	.name = "display",
+	.release = display_class_release,
+	.class_dev_attrs = display_attributes,
+};
+
+static int index;
+
+struct display_device *display_device_register(struct display_driver *driver,
+						struct device *dev, void *devdata)
+{
+	struct display_device *new_dev;
+	int ret = -ENOMEM, i;
+
+	if (unlikely(!driver))
+		return ERR_PTR(-EINVAL);
+
+	new_dev = kzalloc(sizeof(struct display_device), GFP_KERNEL);
+	if (likely(new_dev) && unlikely(driver->probe(new_dev, devdata))) {
+		mutex_init(&new_dev->lock);
+		new_dev->driver = driver;
+		new_dev->class_dev.class = &display_class;
+		new_dev->class_dev.dev = dev;
+		sprintf(new_dev->class_dev.class_id, "display%d", index++);
+		class_set_devdata(&new_dev->class_dev, devdata);
+		ret = class_device_register(&new_dev->class_dev);
+
+		if (likely(ret)) {
+			for (i = 0; i < ARRAY_SIZE(display_attributes); i++) {
+				ret = class_device_create_file(&new_dev->class_dev,
+							&display_attributes[i]);
+				if (unlikely(ret)) {
+					while (--i >= 0)
+						class_device_remove_file(&new_dev->class_dev,
+							&display_attributes[i]);
+					class_device_unregister(&new_dev->class_dev);
+				}
+			}
+		}
+	}
+	if (unlikely(ret)) {
+		kfree(new_dev);
+		new_dev = ERR_PTR(ret);
+	}
+	return new_dev;
+}
+EXPORT_SYMBOL(display_device_register);
+
+void display_device_unregister(struct display_device *dev)
+{
+	int i;
+
+	if (!dev)
+		return;
+	mutex_lock(&dev->lock);
+	dev->driver = NULL;
+	for (i = 0; i < ARRAY_SIZE(display_attributes); i++)
+		class_device_remove_file(&dev->class_dev,
+					&display_attributes[i]);
+	mutex_unlock(&dev->lock);
+	class_device_unregister(&dev->class_dev);
+}
+EXPORT_SYMBOL(display_device_unregister);
+
+static void __exit display_class_exit(void)
+{
+	class_unregister(&display_class);
+}
+
+static int __init display_class_init(void)
+{
+	return class_register(&display_class);
+}
+
+postcore_initcall(display_class_init);
+module_exit(display_class_exit);
+
+MODULE_DESCRIPTION("Display Hardware handling");
+MODULE_AUTHOR("James Simmons <jsimmons@infradead.org>");
+MODULE_LICENSE("GPL");
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/Kconfig fbdev-2.6/drivers/video/display/Kconfig
--- linus-2.6/drivers/video/display/Kconfig	1969-12-31 19:00:00.000000000 -0500
+++ fbdev-2.6/drivers/video/display/Kconfig	2006-12-06 04:50:40.000000000 -0500
@@ -0,0 +1,29 @@
+#
+# Display drivers configuration
+#
+
+menu "Display device support"
+
+config DISPLAY_SUPPORT
+	tristate "Display panel/monitor support"
+	---help---
+	  This framework adds support for low-level control of a display. 
+	  This includes support for power.
+
+	  Enable this to be able to choose the drivers for controlling the
+	  physical display panel/monitor on some platforms. This not only
+	  covers LCD displays for PDAs but also other types of displays
+	  such as CRT, TVout etc.
+
+	  To have support for your specific display panel you will have to
+	  select the proper drivers which depend on this option.
+
+config DISPLAY_DDC
+	tristate "Enable EDID parsing"
+	---help---
+	  Enable EDID parsing
+
+comment "Display hardware drivers"
+	depends on DISPLAY_SUPPORT
+
+endmenu
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/Makefile fbdev-2.6/drivers/video/display/Makefile
--- linus-2.6/drivers/video/display/Makefile	1969-12-31 19:00:00.000000000 -0500
+++ fbdev-2.6/drivers/video/display/Makefile	2006-12-06 04:54:08.000000000 -0500
@@ -0,0 +1,7 @@
+# Display drivers
+
+display-objs				:= display-sysfs.o 
+
+obj-$(CONFIG_DISPLAY_SUPPORT)		+= display.o
+
+obj-$(CONFIG_DISPLAY_DDC)		+= display-ddc.o
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/Kconfig fbdev-2.6/drivers/video/Kconfig
--- linus-2.6/drivers/video/Kconfig	2006-11-30 05:06:13.000000000 -0500
+++ fbdev-2.6/drivers/video/Kconfig	2006-12-06 05:01:22.000000000 -0500
@@ -4,20 +4,10 @@
 
 menu "Graphics support"
 
-config FIRMWARE_EDID
-       bool "Enable firmware EDID"
-       default y
-       ---help---
-         This enables access to the EDID transferred from the firmware.
-	 On the i386, this is from the Video BIOS. Enable this if DDC/I2C
-	 transfers do not work for your driver and if you are using
-	 nvidiafb, i810fb or savagefb.
-
-	 In general, choosing Y for this option is safe.  If you
-	 experience extremely long delays while booting before you get
-	 something on your display, try setting this to N.  Matrox cards in
-	 combination with certain motherboards and monitors are known to
-	 suffer from this problem.
+if SYSFS
+	source "drivers/video/backlight/Kconfig"
+	source "drivers/video/display/Kconfig"
+endif
 
 config FB
 	tristate "Support for frame buffer devices"
@@ -53,6 +43,22 @@
 	  (e.g. an accelerated X server) and that are not frame buffer
 	  device-aware may cause unexpected results. If unsure, say N.
 
+config FIRMWARE_EDID
+       bool "Enable firmware EDID"
+       depends on FB
+       default n 
+       ---help---
+         This enables access to the EDID transferred from the firmware.
+	 On the i386, this is from the Video BIOS. Enable this if DDC/I2C
+	 transfers do not work for your driver and if you are using
+	 nvidiafb, i810fb or savagefb.
+
+	 In general, choosing Y for this option is safe.  If you
+	 experience extremely long delays while booting before you get
+	 something on your display, try setting this to N.  Matrox cards in
+	 combination with certain motherboards and monitors are known to
+	 suffer from this problem.
+
 config FB_DDC
        tristate
        depends on FB && I2C && I2C_ALGOBIT
@@ -90,13 +96,6 @@
        depends on FB
        default n
 
-config FB_BACKLIGHT
-	bool
-	depends on FB
-	select BACKLIGHT_LCD_SUPPORT
-	select BACKLIGHT_CLASS_DEVICE
-	default n
-
 config FB_MODE_HELPERS
         bool "Enable Video Mode Handling Helpers"
         depends on FB
@@ -126,6 +125,9 @@
 	 This is particularly important to one driver, matroxfb.  If
 	 unsure, say N.
 
+comment "Frambuffer hardware drivers"
+	depends on FB
+
 config FB_CIRRUS
 	tristate "Cirrus Logic support"
 	depends on FB && (ZORRO || PCI)
@@ -551,6 +553,7 @@
 	select FB_CFB_FILLRECT
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
+	select VIDEO_SELECT
 	help
 	  This is the frame buffer device driver for generic VESA 2.0
 	  compliant graphic cards. The older VESA 1.2 cards are not supported.
@@ -728,8 +731,7 @@
 
 config FB_NVIDIA_BACKLIGHT
 	bool "Support for backlight control"
-	depends on FB_NVIDIA && PMAC_BACKLIGHT
-	select FB_BACKLIGHT
+	depends on FB_NVIDIA && BACKLIGHT_CLASS_DEVICE
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -775,8 +777,7 @@
 
 config FB_RIVA_BACKLIGHT
 	bool "Support for backlight control"
-	depends on FB_RIVA && PMAC_BACKLIGHT
-	select FB_BACKLIGHT
+	depends on FB_RIVA && BACKLIGHT_CLASS_DEVICE
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -1049,8 +1050,7 @@
 
 config FB_RADEON_BACKLIGHT
 	bool "Support for backlight control"
-	depends on FB_RADEON && PMAC_BACKLIGHT
-	select FB_BACKLIGHT
+	depends on FB_RADEON && BACKLIGHT_CLASS_DEVICE 
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -1081,8 +1081,7 @@
 
 config FB_ATY128_BACKLIGHT
 	bool "Support for backlight control"
-	depends on FB_ATY128 && PMAC_BACKLIGHT
-	select FB_BACKLIGHT
+	depends on FB_ATY128 && BACKLIGHT_CLASS_DEVICE 
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -1131,8 +1130,7 @@
 
 config FB_ATY_BACKLIGHT
 	bool "Support for backlight control"
-	depends on FB_ATY && PMAC_BACKLIGHT
-	select FB_BACKLIGHT
+	depends on FB_ATY && BACKLIGHT_CLASS_DEVICE 
 	default y
 	help
 	  Say Y here if you want to control the backlight of your display.
@@ -1632,6 +1630,7 @@
 	  the vfb_enable=1 option.
 
 	  If unsure, say N.
+
 if VT
 	source "drivers/video/console/Kconfig"
 endif
@@ -1640,9 +1639,5 @@
 	source "drivers/video/logo/Kconfig"
 endif
 
-if SYSFS
-	source "drivers/video/backlight/Kconfig"
-endif
-
 endmenu
 
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/Makefile fbdev-2.6/drivers/video/Makefile
--- linus-2.6/drivers/video/Makefile	2006-11-07 05:38:36.000000000 -0500
+++ fbdev-2.6/drivers/video/Makefile	2006-12-06 05:01:22.000000000 -0500
@@ -12,7 +12,7 @@
 
 obj-$(CONFIG_VT)		  += console/
 obj-$(CONFIG_LOGO)		  += logo/
-obj-$(CONFIG_SYSFS)		  += backlight/
+obj-$(CONFIG_SYSFS)		  += backlight/ display/
 
 obj-$(CONFIG_FB_CFB_FILLRECT)  += cfbfillrect.o
 obj-$(CONFIG_FB_CFB_COPYAREA)  += cfbcopyarea.o
diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/include/linux/display.h fbdev-2.6/include/linux/display.h
--- linus-2.6/include/linux/display.h	1969-12-31 19:00:00.000000000 -0500
+++ fbdev-2.6/include/linux/display.h	2006-12-05 14:43:54.000000000 -0500
@@ -0,0 +1,55 @@
+/*
+ *  Copyright (C) 2006 James Simmons <jsimmons@infradead.org>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or (at
+ *  your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#ifndef _LINUX_DISPLAY_H
+#define _LINUX_DISPLAY_H
+
+#include <linux/device.h>
+
+struct display_device;
+
+/* This structure defines all the properties of a Display. */
+struct display_driver {
+	int (*set_power)(struct display_device *);
+	int (*get_power)(struct display_device *);
+	int (*probe)(struct display_device *, void *);
+	int (*remove)(struct display_device *);
+};
+
+struct display_device {
+	struct module *owner;	/* Owner module */
+	char *name;
+	struct mutex lock;	
+	int request_state;
+	struct display_driver *driver;
+	struct class_device class_dev;		/* The class device structure */
+};
+
+extern struct display_device *display_device_register(struct display_driver *driver,
+					struct device *dev, void *devdata);
+extern void display_device_unregister(struct display_device *dev);
+
+extern int probe_edid(struct display_device *dev, void *devdata);
+
+#define to_display_device(obj) container_of(obj, struct display_device, class_dev)
+
+#endif

Re: Display class

[Randy Dunlap]

On Wed, 6 Dec 2006 15:10:44 +0000 (GMT) James Simmons wrote:

> 
> > > of Mr. Yu for acpi. Also this class could in time replace the lcd class 
> > > located in the backlight directory since a lcd is a type of display.
> > > The final hope is that the purpose auxdisplay could fall under this 
> > > catergory.
> > > 
> > > P.S
> > >    I know the edid parsing would have to be pulled out of the fbdev layer.
> 
> That patch was rought draft for feedback. I applied your comments. This 
> patch actually works. It includes my backlight fix as well.

Glad to hear it.  I had to make the following changes
in order for it to build.
However, I still have build errors for aty.

---
From: Randy Dunlap <randy.dunlap@oracle.com>

Replace CONFIG_FB_BACKLIGHT with CONFIG_BACKLIGHT_CLASS_DEVICE
in include/linux/fb.h and drivers/video/fbsysfs.c
to match Kconfig changes.

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 drivers/video/fbsysfs.c |    8 ++++----
 include/linux/fb.h      |    4 ++--
 2 files changed, 6 insertions(+), 6 deletions(-)

--- linux-2.6.19-git7.orig/include/linux/fb.h
+++ linux-2.6.19-git7/include/linux/fb.h
@@ -367,7 +367,7 @@ struct fb_cursor {
 	struct fb_image	image;	/* Cursor image */
 };
 
-#ifdef CONFIG_FB_BACKLIGHT
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 /* Settings for the generic backlight code */
 #define FB_BACKLIGHT_LEVELS	128
 #define FB_BACKLIGHT_MAX	0xFF
@@ -759,7 +759,7 @@ struct fb_info {
 	struct list_head modelist;      /* mode list */
 	struct fb_videomode *mode;	/* current mode */
 
-#ifdef CONFIG_FB_BACKLIGHT
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 	/* Lock ordering:
 	 * bl_mutex (protects bl_dev and bl_curve)
 	 *   bl_dev->sem (backlight class)
--- linux-2.6.19-git7.orig/drivers/video/fbsysfs.c
+++ linux-2.6.19-git7/drivers/video/fbsysfs.c
@@ -58,7 +58,7 @@ struct fb_info *framebuffer_alloc(size_t
 
 	info->device = dev;
 
-#ifdef CONFIG_FB_BACKLIGHT
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 	mutex_init(&info->bl_mutex);
 #endif
 
@@ -411,7 +411,7 @@ static ssize_t show_fbstate(struct devic
 	return snprintf(buf, PAGE_SIZE, "%d\n", fb_info->state);
 }
 
-#ifdef CONFIG_FB_BACKLIGHT
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 static ssize_t store_bl_curve(struct device *device,
 			      struct device_attribute *attr,
 			      const char *buf, size_t count)
@@ -500,7 +500,7 @@ static struct device_attribute device_at
 	__ATTR(stride, S_IRUGO, show_stride, NULL),
 	__ATTR(rotate, S_IRUGO|S_IWUSR, show_rotate, store_rotate),
 	__ATTR(state, S_IRUGO|S_IWUSR, show_fbstate, store_fbstate),
-#ifdef CONFIG_FB_BACKLIGHT
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 	__ATTR(bl_curve, S_IRUGO|S_IWUSR, show_bl_curve, store_bl_curve),
 #endif
 };
@@ -541,7 +541,7 @@ void fb_cleanup_device(struct fb_info *f
 	}
 }
 
-#ifdef CONFIG_FB_BACKLIGHT
+#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
 /* This function generates a linear backlight curve
  *
  *     0: off

Re: Display class

[James Simmons]

> > That patch was rought draft for feedback. I applied your comments. This 
> > patch actually works. It includes my backlight fix as well.
> 
> Glad to hear it.  I had to make the following changes
> in order for it to build.
> However, I still have build errors for aty.

Ug. I see another problem. I had backlight completly compiled as a 
module! Thus it hid these compile errors. So we need also a 
CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE check as well. Can sysfs handle this 
well or would it be better the the backlight class be a boolean instead?

 
> ---
> From: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Replace CONFIG_FB_BACKLIGHT with CONFIG_BACKLIGHT_CLASS_DEVICE
> in include/linux/fb.h and drivers/video/fbsysfs.c
> to match Kconfig changes.
> 
> Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
> ---
>  drivers/video/fbsysfs.c |    8 ++++----
>  include/linux/fb.h      |    4 ++--
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> --- linux-2.6.19-git7.orig/include/linux/fb.h
> +++ linux-2.6.19-git7/include/linux/fb.h
> @@ -367,7 +367,7 @@ struct fb_cursor {
>  	struct fb_image	image;	/* Cursor image */
>  };
>  
> -#ifdef CONFIG_FB_BACKLIGHT
> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>  /* Settings for the generic backlight code */
>  #define FB_BACKLIGHT_LEVELS	128
>  #define FB_BACKLIGHT_MAX	0xFF
> @@ -759,7 +759,7 @@ struct fb_info {
>  	struct list_head modelist;      /* mode list */
>  	struct fb_videomode *mode;	/* current mode */
>  
> -#ifdef CONFIG_FB_BACKLIGHT
> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>  	/* Lock ordering:
>  	 * bl_mutex (protects bl_dev and bl_curve)
>  	 *   bl_dev->sem (backlight class)
> --- linux-2.6.19-git7.orig/drivers/video/fbsysfs.c
> +++ linux-2.6.19-git7/drivers/video/fbsysfs.c
> @@ -58,7 +58,7 @@ struct fb_info *framebuffer_alloc(size_t
>  
>  	info->device = dev;
>  
> -#ifdef CONFIG_FB_BACKLIGHT
> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>  	mutex_init(&info->bl_mutex);
>  #endif
>  
> @@ -411,7 +411,7 @@ static ssize_t show_fbstate(struct devic
>  	return snprintf(buf, PAGE_SIZE, "%d\n", fb_info->state);
>  }
>  
> -#ifdef CONFIG_FB_BACKLIGHT
> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>  static ssize_t store_bl_curve(struct device *device,
>  			      struct device_attribute *attr,
>  			      const char *buf, size_t count)
> @@ -500,7 +500,7 @@ static struct device_attribute device_at
>  	__ATTR(stride, S_IRUGO, show_stride, NULL),
>  	__ATTR(rotate, S_IRUGO|S_IWUSR, show_rotate, store_rotate),
>  	__ATTR(state, S_IRUGO|S_IWUSR, show_fbstate, store_fbstate),
> -#ifdef CONFIG_FB_BACKLIGHT
> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>  	__ATTR(bl_curve, S_IRUGO|S_IWUSR, show_bl_curve, store_bl_curve),
>  #endif
>  };
> @@ -541,7 +541,7 @@ void fb_cleanup_device(struct fb_info *f
>  	}
>  }
>  
> -#ifdef CONFIG_FB_BACKLIGHT
> +#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>  /* This function generates a linear backlight curve
>   *
>   *     0: off
> 

Re: Display class

[Randy Dunlap]

On Wed, 6 Dec 2006 18:24:08 +0000 (GMT) James Simmons wrote:

> 
> > > That patch was rought draft for feedback. I applied your comments. This 
> > > patch actually works. It includes my backlight fix as well.
> > 
> > Glad to hear it.  I had to make the following changes
> > in order for it to build.
> > However, I still have build errors for aty.
> 
> Ug. I see another problem. I had backlight completly compiled as a 
> module! Thus it hid these compile errors. So we need also a 
> CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE check as well. Can sysfs handle this 
> well or would it be better the the backlight class be a boolean instead?

SCSI works as a module and it uses sysfs.
See drivers/scsi/scsi_sysfs.c.
Does that answer your question?  I wasn't quite sure what
the question was.

Next question, based on:
drivers/built-in.o: In function `probe_edid':
(.text.probe_edid+0x42): undefined reference to `fb_edid_to_monspecs'

Should backlight and/or display support depend on
CONFIG_FB?  Right now they don't, so the above can happen...

---
~Randy

Re: Display class

[James Simmons]

> > > > That patch was rought draft for feedback. I applied your comments. This 
> > > > patch actually works. It includes my backlight fix as well.
> > > 
> > > Glad to hear it.  I had to make the following changes
> > > in order for it to build.
> > > However, I still have build errors for aty.
> > 
> > Ug. I see another problem. I had backlight completly compiled as a 
> > module! Thus it hid these compile errors. So we need also a 
> > CONFIG_BACKLIGHT_CLASS_DEVICE_MODULE check as well. Can sysfs handle this 
> > well or would it be better the the backlight class be a boolean instead?
> 
> SCSI works as a module and it uses sysfs.
> See drivers/scsi/scsi_sysfs.c.
> Does that answer your question?  I wasn't quite sure what
> the question was.

I'm scratching my head on how to configure a modular driver and 
two modular sysfs classes. 
 
> Next question, based on:
> drivers/built-in.o: In function `probe_edid':
> (.text.probe_edid+0x42): undefined reference to `fb_edid_to_monspecs'
> 
> Should backlight and/or display support depend on
> CONFIG_FB?  Right now they don't, so the above can happen...

I already sent a patch to Andrew to make backlight/lcd work independent of
CONFIG_FB. Display is still in the alpha stage. In time it will work 
independent of CONFIG_FB.

Re: Display class

[Randy Dunlap]

On Wed, 6 Dec 2006 15:10:44 +0000 (GMT) James Simmons wrote:

> 
> > > of Mr. Yu for acpi. Also this class could in time replace the lcd class 
> > > located in the backlight directory since a lcd is a type of display.
> > > The final hope is that the purpose auxdisplay could fall under this 
> > > catergory.
> > > 
> > > P.S
> > >    I know the edid parsing would have to be pulled out of the fbdev layer.
> 
> That patch was rought draft for feedback. I applied your comments. This 
> patch actually works. It includes my backlight fix as well.

BTW, this patch version contains trailing whitespace
which should be cleaned up:

Warning: trailing whitespace in lines 33,158 of drivers/acpi/video.c
Warning: trailing whitespace in line 10 of drivers/video/display/Kconfig
Warning: trailing whitespace in line 41 of include/linux/display.h
Warning: trailing whitespace in lines 49,1053,1084,1133 of drivers/video/Kconfig
Warning: trailing whitespace in line 3 of drivers/video/display/Makefile

---
~Randy

Re: Display class

[Miguel Ojeda]

On 12/5/06, James Simmons <jsimmons@infradead.org> wrote:
>
> This is the pass at a display class to meet the needs of the output class
> of Mr. Yu for acpi. Also this class could in time replace the lcd class
> located in the backlight directory since a lcd is a type of display.
> The final hope is that the purpose auxdisplay could fall under this
> catergory.
>
> P.S
>    I know the edid parsing would have to be pulled out of the fbdev layer.
>

Few comments about EXPORT_SYMBOL_GPL:

>
> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/acpi/video.c fbdev-2.6/drivers/acpi/video.c
> --- linus-2.6/drivers/acpi/video.c      2006-11-07 05:38:34.000000000 -0500
> +++ fbdev-2.6/drivers/acpi/video.c      2006-12-04 10:40:48.000000000 -0500
> @@ -30,6 +30,9 @@
>  #include <linux/list.h>
>  #include <linux/proc_fs.h>
>  #include <linux/seq_file.h>
> +#if defined(CONFIG_DISPLAY_DDC)
> +#include <linux/display.h>
> +#endif
>
>  #include <asm/uaccess.h>
>
> @@ -154,6 +157,20 @@
>         int *levels;
>  };
>
> +#if defined(CONFIG_DISPLAY_DDC)
> +static int
> +acpi_display_get_power(struct display_device *dev)
> +{
> +       return 0;
> +}
> +
> +static int
> +acpi_display_set_power(struct display_device *dev)
> +{
> +       return 0;
> +}
> +#endif
> +
>  struct acpi_video_device {
>         unsigned long device_id;
>         struct acpi_video_device_flags flags;
> @@ -162,6 +179,10 @@
>         struct acpi_video_bus *video;
>         struct acpi_device *dev;
>         struct acpi_video_device_brightness *brightness;
> +#if defined(CONFIG_DISPLAY_DDC)
> +       struct display_properties acpi_display_properties;
> +       struct display_device *display;
> +#endif
>  };
>
>  /* bus */
> @@ -399,6 +420,27 @@
>         return status;
>  }
>
> +#if defined(CONFIG_DISPLAY_DDC)
> +static void*
> +acpi_display_get_EDID(struct acpi_video_device *dev)
> +{
> +       union acpi_object *edid = NULL;
> +       void *data = NULL;
> +       int status;
> +
> +       status = acpi_video_device_EDID(dev, &edid, 128);
> +       if (ACPI_FAILURE(status))
> +               status = acpi_video_device_EDID(dev, &edid, 256);
> +
> +       if (ACPI_SUCCESS(status)) {
> +               if (edid && edid->type == ACPI_TYPE_BUFFER) {
> +                       data = edid->buffer.pointer;
> +               }
> +       }
> +       return data;
> +}
> +#endif
> +
>  /* bus */
>
>  static int
> @@ -1255,7 +1297,6 @@
>         int status;
>         struct acpi_video_device *data;
>
> -
>         if (!device || !video)
>                 return -EINVAL;
>
> @@ -1263,12 +1304,10 @@
>             acpi_evaluate_integer(device->handle, "_ADR", NULL, &device_id);
>         if (ACPI_SUCCESS(status)) {
>
> -               data = kmalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
> +               data = kzalloc(sizeof(struct acpi_video_device), GFP_KERNEL);
>                 if (!data)
>                         return -ENOMEM;
>
> -               memset(data, 0, sizeof(struct acpi_video_device));
> -
>                 strcpy(acpi_device_name(device), ACPI_VIDEO_DEVICE_NAME);
>                 strcpy(acpi_device_class(device), ACPI_VIDEO_CLASS);
>                 acpi_driver_data(device) = data;
> @@ -1295,6 +1334,13 @@
>                 acpi_video_device_bind(video, data);
>                 acpi_video_device_find_cap(data);
>
> +#ifdef CONFIG_DISPLAY_DDC
> +               data->acpi_display_properties.get_power = acpi_display_get_power;
> +               data->acpi_display_properties.set_power = acpi_display_set_power;
> +               data->acpi_display_properties.probe = probe_edid;
> +               data->display = display_device_register(NULL, acpi_display_get_EDID(data),
> +                                                       &data->acpi_display_properties);
> +#endif
>                 status = acpi_install_notify_handler(device->handle,
>                                                      ACPI_DEVICE_NOTIFY,
>                                                      acpi_video_device_notify,
> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/display-ddc.c fbdev-2.6/drivers/video/display/display-ddc.c
> --- linus-2.6/drivers/video/display/display-ddc.c       1969-12-31 19:00:00.000000000 -0500
> +++ fbdev-2.6/drivers/video/display/display-ddc.c       2006-12-03 05:27:00.000000000 -0500
> @@ -0,0 +1,45 @@
> +/*
> + *  display-ddc.c - EDID paring code
> + *
> + *  Copyright (C) 2006 James Simmons <jsimmons@infradead.org>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or (at
> + *  your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +#include <linux/module.h>
> +#include <linux/display.h>
> +#include <linux/err.h>
> +#include <linux/ctype.h>
> +
> +#include <linux/fb.h>
> +
> +int probe_edid(struct display_device *dev, void *data)
> +{
> +       struct fb_monspecs spec;
> +       ssize_t size = 45;
> +
> +       dev->name = kzalloc(size, GFP_KERNEL);
> +       fb_edid_to_monspecs((unsigned char *) data, &spec);
> +       strcpy(dev->name, spec.manufacturer);
> +       return snprintf(dev->name, size, "%s %s %s\n", spec.manufacturer, spec.monitor, spec.ascii);
> +}
> +EXPORT_SYMBOL(probe_edid);

Shouldn't be EXPORT_SYMBOL_GPL?

> +
> +MODULE_DESCRIPTION("Display Hardware handling");
> +MODULE_AUTHOR("James Simmons <jsimmons@infradead.org>");
> +MODULE_LICENSE("GPL");
> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/display-sysfs.c fbdev-2.6/drivers/video/display/display-sysfs.c
> --- linus-2.6/drivers/video/display/display-sysfs.c     1969-12-31 19:00:00.000000000 -0500
> +++ fbdev-2.6/drivers/video/display/display-sysfs.c     2006-12-04 07:48:34.000000000 -0500
> @@ -0,0 +1,164 @@
> +/*
> + *  display.c - Display output driver
> + *
> + *  Copyright (C) 2006 James Simmons <jsimmons@infradead.org>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or (at
> + *  your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +#include <linux/module.h>
> +#include <linux/display.h>
> +#include <linux/err.h>
> +#include <linux/ctype.h>
> +
> +static ssize_t display_show_name(struct class_device *dev, char *buf)
> +{
> +       struct display_device *dsp = to_display_device(dev);
> +       return snprintf(buf, PAGE_SIZE, "%s\n", dsp->name);
> +}
> +
> +static ssize_t display_show_power(struct class_device *dev, char *buf)
> +{
> +       struct display_device *dsp = to_display_device(dev);
> +       ssize_t ret = -ENXIO;
> +
> +       mutex_lock(&dsp->lock);
> +       if (likely(dsp->driver->get_power))
> +               ret = sprintf(buf,"%.8x\n", dsp->driver->get_power(dsp));
> +       mutex_unlock(&dsp->lock);
> +       return ret;
> +}
> +
> +static ssize_t display_store_power(struct class_device *dev,
> +       const char *buf,size_t count)
> +{
> +       struct display_device *dsp = to_display_device(dev);
> +       size_t size;
> +       char *endp;
> +       int power;
> +
> +       power = simple_strtoul(buf, &endp, 0);
> +       size = endp - buf;
> +       if (*endp && isspace(*endp))
> +               size++;
> +       if (size != count)
> +               return -EINVAL;
> +
> +       mutex_lock(&dsp->lock);
> +       if (likely(dsp->driver->set_power)) {
> +               dsp->request_state = power;
> +               dsp->driver->set_power(dsp);
> +       }
> +       mutex_unlock(&dsp->lock);
> +       return count;
> +}
> +
> +static void display_class_release(struct class_device *dev)
> +{
> +       struct display_device *dsp = to_display_device(dev);
> +       kfree(dsp);
> +}
> +
> +static struct class_device_attribute display_attributes[] = {
> +       __ATTR(name, S_IRUGO, display_show_name, NULL),
> +       __ATTR(power, S_IRUGO | S_IWUSR, display_show_power, display_store_power),
> +};
> +
> +static struct class display_class = {
> +       .name = "display",
> +       .release = display_class_release,
> +       .class_dev_attrs = display_attributes,
> +};
> +
> +static int index;
> +
> +struct display_device *display_device_register(struct display_driver *driver,
> +                                               struct device *dev, void *devdata)
> +{
> +       struct display_device *new_dev;
> +       int ret = -ENOMEM, i;
> +
> +       if (unlikely(!driver))
> +               return ERR_PTR(-EINVAL);
> +
> +       new_dev = kzalloc(sizeof(struct display_device), GFP_KERNEL);
> +       if (likely(new_dev)) {
> +               driver->probe(new_dev, devdata);
> +
> +               mutex_init(&new_dev->lock);
> +               new_dev->driver = driver;
> +               new_dev->class_dev.class = &display_class;
> +               new_dev->class_dev.dev = dev;
> +               sprintf(new_dev->class_dev.class_id, "display%d", index++);
> +               class_set_devdata(&new_dev->class_dev, devdata);
> +               ret = class_device_register(&new_dev->class_dev);
> +
> +               if (likely(ret)) {
> +                       for (i = 0; i < ARRAY_SIZE(display_attributes); i++) {
> +                               ret = class_device_create_file(&new_dev->class_dev,
> +                                                               &display_attributes[i]);
> +                               if (unlikely(ret)) {
> +                                       while (--i >= 0)
> +                                               class_device_remove_file(&new_dev->class_dev,
> +                                                               &display_attributes[i]);
> +                                       class_device_unregister(&new_dev->class_dev);
> +                               }
> +                       }
> +               }
> +       }
> +       if (unlikely(ret)) {
> +               printk("failed to register display device\n");
> +               kfree(new_dev);
> +               new_dev = ERR_PTR(ret);
> +       }
> +       return new_dev;
> +}
> +EXPORT_SYMBOL(display_device_register);

Shouldn't be EXPORT_SYMBOL_GPL?

> +
> +void display_device_unregister(struct display_device *dev)
> +{
> +       int i;
> +
> +       if (!dev)
> +               return;
> +       mutex_lock(&dev->lock);
> +       dev->driver = NULL;
> +       for (i = 0; i < ARRAY_SIZE(display_attributes); i++)
> +               class_device_remove_file(&dev->class_dev,
> +                                       &display_attributes[i]);
> +       mutex_unlock(&dev->lock);
> +       class_device_unregister(&dev->class_dev);
> +}
> +EXPORT_SYMBOL(display_device_unregister);

Shouldn't be EXPORT_SYMBOL_GPL?

> +
> +static void __exit display_class_exit(void)
> +{
> +       class_unregister(&display_class);
> +}
> +
> +static int __init display_class_init(void)
> +{
> +       return class_register(&display_class);
> +}
> +
> +postcore_initcall(display_class_init);
> +module_exit(display_class_exit);
> +
> +MODULE_DESCRIPTION("Display Hardware handling");
> +MODULE_AUTHOR("James Simmons <jsimmons@infradead.org>");
> +MODULE_LICENSE("GPL");
> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/Kconfig fbdev-2.6/drivers/video/display/Kconfig
> --- linus-2.6/drivers/video/display/Kconfig     1969-12-31 19:00:00.000000000 -0500
> +++ fbdev-2.6/drivers/video/display/Kconfig     2006-12-04 11:41:03.000000000 -0500
> @@ -0,0 +1,29 @@
> +#
> +# Display drivers configuration
> +#
> +
> +menu "Display device support"
> +
> +config DISPLAY_SUPPORT
> +       tristate "Display panel/monitor support"
> +       ---help---
> +         This framework adds support for low-level control of a display.
> +         This includes support for power.
> +
> +         Enable this to be able to choose the drivers for controlling the
> +         physical display panel/monitor on some platforms. This not only
> +         covers LCD displays for PDAs but also other types of displays
> +         such as CRT, TVout etc.
> +
> +         To have support for your specific display panel you will have to
> +         select the proper drivers which depend on this option.
> +
> +config DISPLAY_DDC
> +       tristate "Enable EDID parsing"
> +       ---help---
> +         Enable EDID parsing
> +
> +comment "Display hardware drivers"
> +       depends on DISPLAY_SUPPORT
> +
> +endmenu
> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/display/Makefile fbdev-2.6/drivers/video/display/Makefile
> --- linus-2.6/drivers/video/display/Makefile    1969-12-31 19:00:00.000000000 -0500
> +++ fbdev-2.6/drivers/video/display/Makefile    2006-12-03 05:02:07.000000000 -0500
> @@ -0,0 +1,7 @@
> +# Display drivers
> +
> +display-objs                           := display-sysfs.o
> +
> +obj-$(CONFIG_DISPLAY_SUPPORT)          += display.o
> +
> +obj-$(CONFIG_DISPLAY_DDC)              += display-ddc.o
> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/Kconfig fbdev-2.6/drivers/video/Kconfig
> --- linus-2.6/drivers/video/Kconfig     2006-11-30 05:06:13.000000000 -0500
> +++ fbdev-2.6/drivers/video/Kconfig     2006-12-04 10:40:48.000000000 -0500
> @@ -4,21 +4,6 @@
>
>  menu "Graphics support"
>
> -config FIRMWARE_EDID
> -       bool "Enable firmware EDID"
> -       default y
> -       ---help---
> -         This enables access to the EDID transferred from the firmware.
> -        On the i386, this is from the Video BIOS. Enable this if DDC/I2C
> -        transfers do not work for your driver and if you are using
> -        nvidiafb, i810fb or savagefb.
> -
> -        In general, choosing Y for this option is safe.  If you
> -        experience extremely long delays while booting before you get
> -        something on your display, try setting this to N.  Matrox cards in
> -        combination with certain motherboards and monitors are known to
> -        suffer from this problem.
> -
>  config FB
>         tristate "Support for frame buffer devices"
>         ---help---
> @@ -53,6 +38,22 @@
>           (e.g. an accelerated X server) and that are not frame buffer
>           device-aware may cause unexpected results. If unsure, say N.
>
> +config FIRMWARE_EDID
> +       bool "Enable firmware EDID"
> +       depends on FB
> +       default n
> +       ---help---
> +         This enables access to the EDID transferred from the firmware.
> +        On the i386, this is from the Video BIOS. Enable this if DDC/I2C
> +        transfers do not work for your driver and if you are using
> +        nvidiafb, i810fb or savagefb.
> +
> +        In general, choosing Y for this option is safe.  If you
> +        experience extremely long delays while booting before you get
> +        something on your display, try setting this to N.  Matrox cards in
> +        combination with certain motherboards and monitors are known to
> +        suffer from this problem.
> +
>  config FB_DDC
>         tristate
>         depends on FB && I2C && I2C_ALGOBIT
> @@ -126,6 +127,9 @@
>          This is particularly important to one driver, matroxfb.  If
>          unsure, say N.
>
> +comment "Frambuffer hardware drivers"
> +       depends on FB
> +
>  config FB_CIRRUS
>         tristate "Cirrus Logic support"
>         depends on FB && (ZORRO || PCI)
> @@ -1641,6 +1647,7 @@
>  endif
>
>  if SYSFS
> +       source "drivers/video/display/Kconfig"
>         source "drivers/video/backlight/Kconfig"
>  endif
>
> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/drivers/video/Makefile fbdev-2.6/drivers/video/Makefile
> --- linus-2.6/drivers/video/Makefile    2006-11-07 05:38:36.000000000 -0500
> +++ fbdev-2.6/drivers/video/Makefile    2006-12-04 10:40:48.000000000 -0500
> @@ -12,7 +12,7 @@
>
>  obj-$(CONFIG_VT)                 += console/
>  obj-$(CONFIG_LOGO)               += logo/
> -obj-$(CONFIG_SYSFS)              += backlight/
> +obj-$(CONFIG_SYSFS)              += backlight/ display/
>
>  obj-$(CONFIG_FB_CFB_FILLRECT)  += cfbfillrect.o
>  obj-$(CONFIG_FB_CFB_COPYAREA)  += cfbcopyarea.o
> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/include/linux/display.h fbdev-2.6/include/linux/display.h
> --- linus-2.6/include/linux/display.h   1969-12-31 19:00:00.000000000 -0500
> +++ fbdev-2.6/include/linux/display.h   2006-12-04 07:42:29.000000000 -0500
> @@ -0,0 +1,55 @@
> +/*
> + *  Copyright (C) 2006 James Simmons <jsimmons@infradead.org>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or (at
> + *  your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#ifndef _LINUX_DISPLAY_H
> +#define _LINUX_DISPLAY_H
> +
> +#include <linux/device.h>
> +
> +struct display_device;
> +
> +/* This structure defines all the properties of a Display. */
> +struct display_driver {
> +       int (*set_power)(struct display_device *);
> +       int (*get_power)(struct display_device *);
> +       int (*probe)(struct display_device *, void *);
> +       int (*remove)(struct display_device *);
> +};
> +
> +struct display_device {
> +       struct module *owner;   /* Owner module */
> +       char *name;
> +       struct mutex lock;
> +       int request_state;
> +       struct display_driver *driver;
> +       struct class_device class_dev;          /* The class device structure */
> +};
> +
> +extern struct display_device *display_device_register(struct display_driver *driver,
> +                                       struct device *dev, void *devdata);
> +extern void display_device_unregister(struct display_device *dev);
> +
> +extern int probe_edid(struct display_device *dev, void *devdata);
> +
> +#define to_display_device(obj) container_of(obj, struct display_device, class_dev)
> +
> +#endif
> diff -urN -X fbdev-2.6/Documentation/dontdiff linus-2.6/MAINTAINERS fbdev-2.6/MAINTAINERS
> --- linus-2.6/MAINTAINERS       2006-12-04 04:11:38.000000000 -0500
> +++ fbdev-2.6/MAINTAINERS       2006-12-04 10:40:48.000000000 -0500
> @@ -911,6 +923,12 @@
>  L:     linux-kernel@vger.kernel.org
>  S:     Maintained
>
> +DISPLAY SUBSYSTEM
> +P:     James Simmons
> +M:     jsimmons@infradead.org
> +L:     linux-kernel@vger.kernel.org
> +S:     Maintained
> +
>  DISTRIBUTED LOCK MANAGER
>  P:     Patrick Caulfield
>  M:     pcaulfie@redhat.com
>


-- 
Miguel Ojeda
http://maxextreme.googlepages.com/index.htm

Re: Display class

[Dmitry Torokhov]

Hi,

On Tuesday 05 December 2006 13:03, James Simmons wrote:
> +int probe_edid(struct display_device *dev, void *data)
> +{
> +       struct fb_monspecs spec;
> +       ssize_t size = 45;

const ssize_t size = 45? 

> +
> +       dev->name = kzalloc(size, GFP_KERNEL);

Why do you need kzalloc here?

> +       fb_edid_to_monspecs((unsigned char *) data, &spec);
> +       strcpy(dev->name, spec.manufacturer);

You seem to be overwriting dev->name in the very next line?

> +       return snprintf(dev->name, size, "%s %s %s\n", spec.manufacturer, spec.monitor, spec.ascii);
> 

Is result of snprintf interesting to the callers?

-- 
Dmitry

Re: Display class

[James Simmons]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 11557 bytes --]


> Hi,
> 
> On Tuesday 05 December 2006 13:03, James Simmons wrote:
> > +int probe_edid(struct display_device *dev, void *data)
> > +{
> > +       struct fb_monspecs spec;
> > +       ssize_t size = 45;

That code was only for testing. I do have new core code. Andrew could 
you merge this patch as it is against the -mm tree.

This new class provides a way common interface for various types of 
displays such as LCD, CRT, LVDS etc. It is a expansion of the lcd
class to include other types of displays.
  
diff -urN linux-2.6.20-rc3/drivers/video/display/display-sysfs.c linux-2.6.20-rc3-display/drivers/video/display/display-sysfs.c
--- linux-2.6.20-rc3/drivers/video/display/display-sysfs.c	1969-12-31 19:00:00.000000000 -0500
+++ linux-2.6.20-rc3-display/drivers/video/display/display-sysfs.c	2007-01-13 16:22:54.000000000 -0500
@@ -0,0 +1,208 @@
+/*
+ *  display.c - Display output driver
+ *
+ *  Copyright (C) 2007 James Simmons <jsimmons@infradead.org>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or (at
+ *  your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+#include <linux/module.h>
+#include <linux/display.h>
+#include <linux/err.h>
+#include <linux/ctype.h>
+
+static ssize_t display_show_name(struct class_device *cdev, char *buf)
+{
+	struct display_device *dsp = to_display_device(cdev);
+	return snprintf(buf, PAGE_SIZE, "%s\n", dsp->name);
+}
+
+static ssize_t display_show_type(struct class_device *cdev, char *buf)
+{
+	struct display_device *dsp = to_display_device(cdev);
+	return snprintf(buf, PAGE_SIZE, "%s\n", dsp->driver->type);
+}
+
+static ssize_t display_show_power(struct class_device *cdev, char *buf)
+{
+	struct display_device *dsp = to_display_device(cdev);
+	ssize_t ret = -ENXIO;
+
+	mutex_lock(&dsp->lock);
+	if (likely(dsp->driver->get_power))
+		ret = sprintf(buf,"%.8x\n", dsp->driver->get_power(dsp));
+	mutex_unlock(&dsp->lock);
+	return ret;
+}
+
+static ssize_t display_store_power(struct class_device *cdev,
+	const char *buf, size_t count)
+{
+	struct display_device *dsp = to_display_device(cdev);
+	ssize_t size;
+	char *endp;
+	int power;
+
+	power = simple_strtoul(buf, &endp, 0);
+	size = endp - buf;
+	if (*endp && isspace(*endp))
+		size++;
+	if (size != count)
+		return -EINVAL;
+
+	mutex_lock(&dsp->lock);
+	if (likely(dsp->driver->set_power)) {
+		dsp->request_state = power;
+		dsp->driver->set_power(dsp);
+	}
+	mutex_unlock(&dsp->lock);
+	return count;
+}
+
+static ssize_t display_show_contrast(struct class_device *cdev, char *buf)
+{
+	struct display_device *dsp = to_display_device(cdev);
+	ssize_t rc = -ENXIO;
+
+	mutex_lock(&dsp->lock);
+	if (likely(dsp->driver) && dsp->driver->get_contrast)
+		rc = sprintf(buf, "%d\n", dsp->driver->get_contrast(dsp));
+	mutex_unlock(&dsp->lock);
+	return rc;
+}
+
+static ssize_t display_store_contrast(struct class_device *cdev, const char *buf, size_t count)
+{
+	
+	struct display_device *dsp = to_display_device(cdev);
+	ssize_t ret = -EINVAL, size;
+	int contrast;
+	char *endp;
+
+	contrast = simple_strtoul(buf, &endp, 0);
+	size = endp - buf;
+
+	if (*endp && isspace(*endp))
+		size++;
+
+	if (size != count)
+		return ret;
+
+	mutex_lock(&dsp->lock);
+	if (likely(dsp->driver && dsp->driver->set_contrast)) {
+		pr_debug("display: set contrast to %d\n", contrast);
+		dsp->driver->set_contrast(dsp, contrast);
+		ret = count;
+	}
+	mutex_unlock(&dsp->lock);
+	return ret;
+}
+
+static ssize_t display_show_max_contrast(struct class_device *cdev, char *buf)
+{
+	struct display_device *dsp = to_display_device(cdev);
+	ssize_t rc = -ENXIO;
+
+	mutex_lock(&dsp->lock);
+	if (likely(dsp->driver))
+		rc = sprintf(buf, "%d\n", dsp->driver->max_contrast);
+	mutex_unlock(&dsp->lock);
+	return rc;
+}
+
+static void display_class_release(struct class_device *dev)
+{
+	struct display_device *dsp = to_display_device(dev);
+	kfree(dsp);
+}
+
+static struct class_device_attribute display_attributes[] = {
+	__ATTR(name, S_IRUGO, display_show_name, NULL),
+	__ATTR(type, S_IRUGO, display_show_type, NULL),
+	__ATTR(power, S_IRUGO | S_IWUSR, display_show_power, display_store_power),
+	__ATTR(contrast, S_IRUGO | S_IWUSR, display_show_contrast, display_store_contrast),
+	__ATTR(max_contrast, S_IRUGO, display_show_max_contrast, NULL),
+};
+
+static struct class display_class = {
+	.name = "display",
+	.release = display_class_release,
+	.class_dev_attrs = display_attributes,
+};
+
+static int index;
+
+struct display_device *display_device_register(struct display_driver *driver,
+						struct device *dev, void *devdata)
+{
+	struct display_device *new_dev;
+	int ret = -ENOMEM;
+
+	if (unlikely(!driver))
+		return ERR_PTR(-EINVAL);
+
+	new_dev = kzalloc(sizeof(struct display_device), GFP_KERNEL);
+	if (likely(new_dev) && unlikely(driver->probe(new_dev, devdata))) {
+		mutex_init(&new_dev->lock);
+
+		new_dev->class_dev.groups = &driver->type_prop; 
+		new_dev->class_dev.class = &display_class;
+		new_dev->class_dev.dev = dev;
+		new_dev->driver = driver;
+
+		sprintf(new_dev->class_dev.class_id, "display%d", index++);
+		class_set_devdata(&new_dev->class_dev, devdata);
+		ret = class_device_register(&new_dev->class_dev);
+	}
+	if (unlikely(ret)) {
+		kfree(new_dev);
+		new_dev = ERR_PTR(ret);
+	}
+	return new_dev;
+}
+EXPORT_SYMBOL(display_device_register);
+
+void display_device_unregister(struct display_device *dev)
+{
+	if (!dev)
+		return;
+	mutex_lock(&dev->lock);
+	class_device_unregister(&dev->class_dev);
+	dev->driver = NULL;
+	index--;
+	mutex_unlock(&dev->lock);
+	kfree(dev);
+}
+EXPORT_SYMBOL(display_device_unregister);
+
+static void __exit display_class_exit(void)
+{
+	class_unregister(&display_class);
+}
+
+static int __init display_class_init(void)
+{
+	return class_register(&display_class);
+}
+
+postcore_initcall(display_class_init);
+module_exit(display_class_exit);
+
+MODULE_DESCRIPTION("Display Hardware handling");
+MODULE_AUTHOR("James Simmons <jsimmons@infradead.org>");
+MODULE_LICENSE("GPL");
diff -urN linux-2.6.20-rc3/drivers/video/display/Kconfig linux-2.6.20-rc3-display/drivers/video/display/Kconfig
--- linux-2.6.20-rc3/drivers/video/display/Kconfig	1969-12-31 19:00:00.000000000 -0500
+++ linux-2.6.20-rc3-display/drivers/video/display/Kconfig	2007-01-13 16:00:51.000000000 -0500
@@ -0,0 +1,24 @@
+#
+# Display drivers configuration
+#
+
+menu "Display device support"
+
+config DISPLAY_SUPPORT
+	tristate "Display panel/monitor support"
+	---help---
+	  This framework adds support for low-level control of a display.
+	  This includes support for power.
+
+	  Enable this to be able to choose the drivers for controlling the
+	  physical display panel/monitor on some platforms. This not only
+	  covers LCD displays for PDAs but also other types of displays
+	  such as CRT, TVout etc.
+
+	  To have support for your specific display panel you will have to
+	  select the proper drivers which depend on this option.
+
+comment "Display hardware drivers"
+	depends on DISPLAY_SUPPORT
+
+endmenu
diff -urN linux-2.6.20-rc3/drivers/video/display/Makefile linux-2.6.20-rc3-display/drivers/video/display/Makefile
--- linux-2.6.20-rc3/drivers/video/display/Makefile	1969-12-31 19:00:00.000000000 -0500
+++ linux-2.6.20-rc3-display/drivers/video/display/Makefile	2007-01-13 16:01:03.000000000 -0500
@@ -0,0 +1,6 @@
+# Display drivers
+
+display-objs				:= display-sysfs.o
+
+obj-$(CONFIG_DISPLAY_SUPPORT)		+= display.o
+
diff -urN linux-2.6.20-rc3/drivers/video/Kconfig linux-2.6.20-rc3-display/drivers/video/Kconfig
--- linux-2.6.20-rc3/drivers/video/Kconfig	2007-01-08 14:00:26.000000000 -0500
+++ linux-2.6.20-rc3-display/drivers/video/Kconfig	2007-01-13 09:23:03.000000000 -0500
@@ -6,6 +6,7 @@
 
 if SYSFS
 	source "drivers/video/backlight/Kconfig"
+	source "drivers/video/display/Kconfig"
 endif
 
 config FB
diff -urN linux-2.6.20-rc3/drivers/video/Makefile linux-2.6.20-rc3-display/drivers/video/Makefile
--- linux-2.6.20-rc3/drivers/video/Makefile	2007-01-08 14:00:26.000000000 -0500
+++ linux-2.6.20-rc3-display/drivers/video/Makefile	2007-01-13 09:22:40.000000000 -0500
@@ -12,7 +12,7 @@
 
 obj-$(CONFIG_VT)		  += console/
 obj-$(CONFIG_LOGO)		  += logo/
-obj-$(CONFIG_SYSFS)		  += backlight/
+obj-$(CONFIG_SYSFS)		  += backlight/ display/
 
 obj-$(CONFIG_FB_CFB_FILLRECT)  += cfbfillrect.o
 obj-$(CONFIG_FB_CFB_COPYAREA)  += cfbcopyarea.o
diff -urN linux-2.6.20-rc3/include/linux/display.h linux-2.6.20-rc3-display/include/linux/display.h
--- linux-2.6.20-rc3/include/linux/display.h	1969-12-31 19:00:00.000000000 -0500
+++ linux-2.6.20-rc3-display/include/linux/display.h	2007-01-13 16:23:51.000000000 -0500
@@ -0,0 +1,60 @@
+/*
+ *  Copyright (C) 2006 James Simmons <jsimmons@infradead.org>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or (at
+ *  your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#ifndef _LINUX_DISPLAY_H
+#define _LINUX_DISPLAY_H
+
+#include <linux/device.h>
+
+struct display_device;
+
+/* This structure defines all the properties of a Display. */
+struct display_driver {
+	int (*set_power)(struct display_device *);
+	int (*get_power)(struct display_device *);
+	int (*set_contrast)(struct display_device *, unsigned int);
+	int (*get_contrast)(struct display_device *);
+	int (*probe)(struct display_device *, void *);
+	int (*remove)(struct display_device *);
+	struct attribute_group *type_prop;
+	int max_contrast;
+	char type[16];
+};
+
+struct display_device {
+	struct module *owner;	/* Owner module */
+	char *name;
+	struct mutex lock;
+	int request_state;
+	struct display_driver *driver;
+	struct class_device class_dev;		/* The class device structure */
+};
+
+extern struct display_device *display_device_register(struct display_driver *driver,
+					struct device *dev, void *devdata);
+extern void display_device_unregister(struct display_device *dev);
+
+extern int display_probe_edid(struct display_device *dev, void *devdata);
+
+#define to_display_device(obj) container_of(obj, struct display_device, class_dev)
+
+#endif

Re: [Linux-fbdev-devel] Display class

[James Simmons]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 12354 bytes --]


Andrew please apply this patch.

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

> 
> > Hi,
> > 
> > On Tuesday 05 December 2006 13:03, James Simmons wrote:
> > > +int probe_edid(struct display_device *dev, void *data)
> > > +{
> > > +       struct fb_monspecs spec;
> > > +       ssize_t size = 45;
> 
> That code was only for testing. I do have new core code. Andrew could 
> you merge this patch as it is against the -mm tree.
> 
> This new class provides a way common interface for various types of 
> displays such as LCD, CRT, LVDS etc. It is a expansion of the lcd
> class to include other types of displays.
>   
> diff -urN linux-2.6.20-rc3/drivers/video/display/display-sysfs.c linux-2.6.20-rc3-display/drivers/video/display/display-sysfs.c
> --- linux-2.6.20-rc3/drivers/video/display/display-sysfs.c	1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.20-rc3-display/drivers/video/display/display-sysfs.c	2007-01-13 16:22:54.000000000 -0500
> @@ -0,0 +1,208 @@
> +/*
> + *  display.c - Display output driver
> + *
> + *  Copyright (C) 2007 James Simmons <jsimmons@infradead.org>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or (at
> + *  your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +#include <linux/module.h>
> +#include <linux/display.h>
> +#include <linux/err.h>
> +#include <linux/ctype.h>
> +
> +static ssize_t display_show_name(struct class_device *cdev, char *buf)
> +{
> +	struct display_device *dsp = to_display_device(cdev);
> +	return snprintf(buf, PAGE_SIZE, "%s\n", dsp->name);
> +}
> +
> +static ssize_t display_show_type(struct class_device *cdev, char *buf)
> +{
> +	struct display_device *dsp = to_display_device(cdev);
> +	return snprintf(buf, PAGE_SIZE, "%s\n", dsp->driver->type);
> +}
> +
> +static ssize_t display_show_power(struct class_device *cdev, char *buf)
> +{
> +	struct display_device *dsp = to_display_device(cdev);
> +	ssize_t ret = -ENXIO;
> +
> +	mutex_lock(&dsp->lock);
> +	if (likely(dsp->driver->get_power))
> +		ret = sprintf(buf,"%.8x\n", dsp->driver->get_power(dsp));
> +	mutex_unlock(&dsp->lock);
> +	return ret;
> +}
> +
> +static ssize_t display_store_power(struct class_device *cdev,
> +	const char *buf, size_t count)
> +{
> +	struct display_device *dsp = to_display_device(cdev);
> +	ssize_t size;
> +	char *endp;
> +	int power;
> +
> +	power = simple_strtoul(buf, &endp, 0);
> +	size = endp - buf;
> +	if (*endp && isspace(*endp))
> +		size++;
> +	if (size != count)
> +		return -EINVAL;
> +
> +	mutex_lock(&dsp->lock);
> +	if (likely(dsp->driver->set_power)) {
> +		dsp->request_state = power;
> +		dsp->driver->set_power(dsp);
> +	}
> +	mutex_unlock(&dsp->lock);
> +	return count;
> +}
> +
> +static ssize_t display_show_contrast(struct class_device *cdev, char *buf)
> +{
> +	struct display_device *dsp = to_display_device(cdev);
> +	ssize_t rc = -ENXIO;
> +
> +	mutex_lock(&dsp->lock);
> +	if (likely(dsp->driver) && dsp->driver->get_contrast)
> +		rc = sprintf(buf, "%d\n", dsp->driver->get_contrast(dsp));
> +	mutex_unlock(&dsp->lock);
> +	return rc;
> +}
> +
> +static ssize_t display_store_contrast(struct class_device *cdev, const char *buf, size_t count)
> +{
> +	
> +	struct display_device *dsp = to_display_device(cdev);
> +	ssize_t ret = -EINVAL, size;
> +	int contrast;
> +	char *endp;
> +
> +	contrast = simple_strtoul(buf, &endp, 0);
> +	size = endp - buf;
> +
> +	if (*endp && isspace(*endp))
> +		size++;
> +
> +	if (size != count)
> +		return ret;
> +
> +	mutex_lock(&dsp->lock);
> +	if (likely(dsp->driver && dsp->driver->set_contrast)) {
> +		pr_debug("display: set contrast to %d\n", contrast);
> +		dsp->driver->set_contrast(dsp, contrast);
> +		ret = count;
> +	}
> +	mutex_unlock(&dsp->lock);
> +	return ret;
> +}
> +
> +static ssize_t display_show_max_contrast(struct class_device *cdev, char *buf)
> +{
> +	struct display_device *dsp = to_display_device(cdev);
> +	ssize_t rc = -ENXIO;
> +
> +	mutex_lock(&dsp->lock);
> +	if (likely(dsp->driver))
> +		rc = sprintf(buf, "%d\n", dsp->driver->max_contrast);
> +	mutex_unlock(&dsp->lock);
> +	return rc;
> +}
> +
> +static void display_class_release(struct class_device *dev)
> +{
> +	struct display_device *dsp = to_display_device(dev);
> +	kfree(dsp);
> +}
> +
> +static struct class_device_attribute display_attributes[] = {
> +	__ATTR(name, S_IRUGO, display_show_name, NULL),
> +	__ATTR(type, S_IRUGO, display_show_type, NULL),
> +	__ATTR(power, S_IRUGO | S_IWUSR, display_show_power, display_store_power),
> +	__ATTR(contrast, S_IRUGO | S_IWUSR, display_show_contrast, display_store_contrast),
> +	__ATTR(max_contrast, S_IRUGO, display_show_max_contrast, NULL),
> +};
> +
> +static struct class display_class = {
> +	.name = "display",
> +	.release = display_class_release,
> +	.class_dev_attrs = display_attributes,
> +};
> +
> +static int index;
> +
> +struct display_device *display_device_register(struct display_driver *driver,
> +						struct device *dev, void *devdata)
> +{
> +	struct display_device *new_dev;
> +	int ret = -ENOMEM;
> +
> +	if (unlikely(!driver))
> +		return ERR_PTR(-EINVAL);
> +
> +	new_dev = kzalloc(sizeof(struct display_device), GFP_KERNEL);
> +	if (likely(new_dev) && unlikely(driver->probe(new_dev, devdata))) {
> +		mutex_init(&new_dev->lock);
> +
> +		new_dev->class_dev.groups = &driver->type_prop; 
> +		new_dev->class_dev.class = &display_class;
> +		new_dev->class_dev.dev = dev;
> +		new_dev->driver = driver;
> +
> +		sprintf(new_dev->class_dev.class_id, "display%d", index++);
> +		class_set_devdata(&new_dev->class_dev, devdata);
> +		ret = class_device_register(&new_dev->class_dev);
> +	}
> +	if (unlikely(ret)) {
> +		kfree(new_dev);
> +		new_dev = ERR_PTR(ret);
> +	}
> +	return new_dev;
> +}
> +EXPORT_SYMBOL(display_device_register);
> +
> +void display_device_unregister(struct display_device *dev)
> +{
> +	if (!dev)
> +		return;
> +	mutex_lock(&dev->lock);
> +	class_device_unregister(&dev->class_dev);
> +	dev->driver = NULL;
> +	index--;
> +	mutex_unlock(&dev->lock);
> +	kfree(dev);
> +}
> +EXPORT_SYMBOL(display_device_unregister);
> +
> +static void __exit display_class_exit(void)
> +{
> +	class_unregister(&display_class);
> +}
> +
> +static int __init display_class_init(void)
> +{
> +	return class_register(&display_class);
> +}
> +
> +postcore_initcall(display_class_init);
> +module_exit(display_class_exit);
> +
> +MODULE_DESCRIPTION("Display Hardware handling");
> +MODULE_AUTHOR("James Simmons <jsimmons@infradead.org>");
> +MODULE_LICENSE("GPL");
> diff -urN linux-2.6.20-rc3/drivers/video/display/Kconfig linux-2.6.20-rc3-display/drivers/video/display/Kconfig
> --- linux-2.6.20-rc3/drivers/video/display/Kconfig	1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.20-rc3-display/drivers/video/display/Kconfig	2007-01-13 16:00:51.000000000 -0500
> @@ -0,0 +1,24 @@
> +#
> +# Display drivers configuration
> +#
> +
> +menu "Display device support"
> +
> +config DISPLAY_SUPPORT
> +	tristate "Display panel/monitor support"
> +	---help---
> +	  This framework adds support for low-level control of a display.
> +	  This includes support for power.
> +
> +	  Enable this to be able to choose the drivers for controlling the
> +	  physical display panel/monitor on some platforms. This not only
> +	  covers LCD displays for PDAs but also other types of displays
> +	  such as CRT, TVout etc.
> +
> +	  To have support for your specific display panel you will have to
> +	  select the proper drivers which depend on this option.
> +
> +comment "Display hardware drivers"
> +	depends on DISPLAY_SUPPORT
> +
> +endmenu
> diff -urN linux-2.6.20-rc3/drivers/video/display/Makefile linux-2.6.20-rc3-display/drivers/video/display/Makefile
> --- linux-2.6.20-rc3/drivers/video/display/Makefile	1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.20-rc3-display/drivers/video/display/Makefile	2007-01-13 16:01:03.000000000 -0500
> @@ -0,0 +1,6 @@
> +# Display drivers
> +
> +display-objs				:= display-sysfs.o
> +
> +obj-$(CONFIG_DISPLAY_SUPPORT)		+= display.o
> +
> diff -urN linux-2.6.20-rc3/drivers/video/Kconfig linux-2.6.20-rc3-display/drivers/video/Kconfig
> --- linux-2.6.20-rc3/drivers/video/Kconfig	2007-01-08 14:00:26.000000000 -0500
> +++ linux-2.6.20-rc3-display/drivers/video/Kconfig	2007-01-13 09:23:03.000000000 -0500
> @@ -6,6 +6,7 @@
>  
>  if SYSFS
>  	source "drivers/video/backlight/Kconfig"
> +	source "drivers/video/display/Kconfig"
>  endif
>  
>  config FB
> diff -urN linux-2.6.20-rc3/drivers/video/Makefile linux-2.6.20-rc3-display/drivers/video/Makefile
> --- linux-2.6.20-rc3/drivers/video/Makefile	2007-01-08 14:00:26.000000000 -0500
> +++ linux-2.6.20-rc3-display/drivers/video/Makefile	2007-01-13 09:22:40.000000000 -0500
> @@ -12,7 +12,7 @@
>  
>  obj-$(CONFIG_VT)		  += console/
>  obj-$(CONFIG_LOGO)		  += logo/
> -obj-$(CONFIG_SYSFS)		  += backlight/
> +obj-$(CONFIG_SYSFS)		  += backlight/ display/
>  
>  obj-$(CONFIG_FB_CFB_FILLRECT)  += cfbfillrect.o
>  obj-$(CONFIG_FB_CFB_COPYAREA)  += cfbcopyarea.o
> diff -urN linux-2.6.20-rc3/include/linux/display.h linux-2.6.20-rc3-display/include/linux/display.h
> --- linux-2.6.20-rc3/include/linux/display.h	1969-12-31 19:00:00.000000000 -0500
> +++ linux-2.6.20-rc3-display/include/linux/display.h	2007-01-13 16:23:51.000000000 -0500
> @@ -0,0 +1,60 @@
> +/*
> + *  Copyright (C) 2006 James Simmons <jsimmons@infradead.org>
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + *
> + *  This program is free software; you can redistribute it and/or modify
> + *  it under the terms of the GNU General Public License as published by
> + *  the Free Software Foundation; either version 2 of the License, or (at
> + *  your option) any later version.
> + *
> + *  This program is distributed in the hope that it will be useful, but
> + *  WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + *  General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
> + *
> + * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> + */
> +
> +#ifndef _LINUX_DISPLAY_H
> +#define _LINUX_DISPLAY_H
> +
> +#include <linux/device.h>
> +
> +struct display_device;
> +
> +/* This structure defines all the properties of a Display. */
> +struct display_driver {
> +	int (*set_power)(struct display_device *);
> +	int (*get_power)(struct display_device *);
> +	int (*set_contrast)(struct display_device *, unsigned int);
> +	int (*get_contrast)(struct display_device *);
> +	int (*probe)(struct display_device *, void *);
> +	int (*remove)(struct display_device *);
> +	struct attribute_group *type_prop;
> +	int max_contrast;
> +	char type[16];
> +};
> +
> +struct display_device {
> +	struct module *owner;	/* Owner module */
> +	char *name;
> +	struct mutex lock;
> +	int request_state;
> +	struct display_driver *driver;
> +	struct class_device class_dev;		/* The class device structure */
> +};
> +
> +extern struct display_device *display_device_register(struct display_driver *driver,
> +					struct device *dev, void *devdata);
> +extern void display_device_unregister(struct display_device *dev);
> +
> +extern int display_probe_edid(struct display_device *dev, void *devdata);
> +
> +#define to_display_device(obj) container_of(obj, struct display_device, class_dev)
> +
> +#endif
> 

Re: Display class

[Greg KH]

On Sat, Jan 13, 2007 at 10:40:55PM +0000, James Simmons wrote:
> 
> > Hi,
> > 
> > On Tuesday 05 December 2006 13:03, James Simmons wrote:
> > > +int probe_edid(struct display_device *dev, void *data)
> > > +{
> > > +???????struct fb_monspecs spec;
> > > +???????ssize_t size = 45;
> 
> That code was only for testing. I do have new core code. Andrew could 
> you merge this patch as it is against the -mm tree.
> 
> This new class provides a way common interface for various types of 
> displays such as LCD, CRT, LVDS etc. It is a expansion of the lcd
> class to include other types of displays.

Any chance you can change this to use "struct device" instead of "struct
class_device" so I don't have to convert it myself in the next few weeks
if it ever gets merged?  :)

The functionality will be the same, if not, please let me know what
problems you have with the change.

thanks,

greg k-h

Re: Display class

[Greg KH]

On Sat, Jan 13, 2007 at 10:40:55PM +0000, James Simmons wrote:
> 
> > Hi,
> > 
> > On Tuesday 05 December 2006 13:03, James Simmons wrote:
> > > +int probe_edid(struct display_device *dev, void *data)
> > > +{
> > > +???????struct fb_monspecs spec;
> > > +???????ssize_t size = 45;
> 
> That code was only for testing. I do have new core code. Andrew could 
> you merge this patch as it is against the -mm tree.
> 
> This new class provides a way common interface for various types of 
> displays such as LCD, CRT, LVDS etc. It is a expansion of the lcd
> class to include other types of displays.

Have you worked with the DRM developers who also need to tie into this
CRT class somehow?

thanks,

greg k-h