lkml.org 
[lkml]   [2006]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: Display class
    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
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/
    \
     
     \ /
      Last update: 2006-12-06 02:17    [W:0.048 / U:88.312 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site