two patches - request for comments

two patches - request for comments

[Andrew Zabolotny]

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

Hello!

I'm going to submit the class_find_device() patch (attached for your
convenience) as a pre-requisite for the backlight/lcd device class patch
(also included so that you can take at it as well) via Russel King (the
backlight/lcd patch is needed for our ARM-based handhelds framebuffer
devices). Any comments/objections are welcome.

The LCD and backlight device classes were implemented with the following in
mind:

a) Allow user-mode code to control backlight and lcd (notably for
handhelds this is a must). This is done via sysfs (/sys/class/backlight/XXX/
and /sys/class/lcd/XXX/ where XXX is something like mq1100fb0).

b) Allow framebuffer devices to find and contact the respective backlight and
lcd devices (they are using the same name as the framebuffer device). This is
needed to implement the respective calls to backlight and lcd devices for
power management.

--
Greetings,
   Andrew

P.S. I've posted a similar message on linux-fbdev mailing listt; forgive me if
you seen it twice.

[-- Attachment #2: class.diff --]
[-- Type: application/octet-stream, Size: 2520 bytes --]

--- linux-2.6.6/drivers/base/class.c	2004-05-10 06:33:21.000000000 +0400
+++ linux-2.6.6-hh0/drivers/base/class.c	2004-05-25 15:17:57.000000000 +0400
@@ -359,7 +359,7 @@
 	class_device_put(class_dev);
 }
 
-int class_device_rename(struct class_device *class_dev, char *new_name)
+int class_device_rename(struct class_device *class_dev, const char *new_name)
 {
 	class_dev = class_device_get(class_dev);
 	if (!class_dev)
@@ -377,6 +377,33 @@
 	return 0;
 }
 
+/**
+ * class_device_find - find a struct class_device in a specific class
+ * @class: the class to search
+ * @class_id: the class_id to search for
+ *
+ * Iterates through the list of all class devices registered to a class. If
+ * the class_id is found, its reference count is incremented and returned to
+ * the caller. If the class_id does not match any existing struct class_device
+ * registered to this struct class, then NULL is returned.
+ */
+struct class_device * class_device_find(struct class *class, const char *class_id)
+{
+	struct class_device *class_dev;
+	struct class_device *found = NULL;
+
+	down_read(&class->subsys.rwsem);
+	list_for_each_entry(class_dev, &class->children, node) {
+		if (strcmp(class_dev->class_id, class_id) == 0) {
+			found = class_device_get(class_dev);
+			break;
+		}
+	}
+	up_read(&class->subsys.rwsem);
+
+	return found;
+}
+
 struct class_device * class_device_get(struct class_device *class_dev)
 {
 	if (class_dev)
@@ -389,7 +416,6 @@
 	kobject_put(&class_dev->kobj);
 }
 
-
 int class_interface_register(struct class_interface *class_intf)
 {
 	struct class * parent;
@@ -468,6 +494,8 @@
 EXPORT_SYMBOL(class_device_put);
 EXPORT_SYMBOL(class_device_create_file);
 EXPORT_SYMBOL(class_device_remove_file);
+EXPORT_SYMBOL(class_device_rename);
+EXPORT_SYMBOL(class_device_find);
 
 EXPORT_SYMBOL(class_interface_register);
 EXPORT_SYMBOL(class_interface_unregister);
--- linux-2.6.6/include/linux/device.h	2004-05-10 06:33:19.000000000 +0400
+++ linux-2.6.6-hh0/include/linux/device.h	2004-05-25 01:54:30.000000000 +0400
@@ -212,8 +212,9 @@
 extern int class_device_add(struct class_device *);
 extern void class_device_del(struct class_device *);
 
-extern int class_device_rename(struct class_device *, char *);
+extern int class_device_rename(struct class_device *, const char *);
 
+extern struct class_device * class_device_find(struct class *class, const char *class_id);
 extern struct class_device * class_device_get(struct class_device *);
 extern void class_device_put(struct class_device *);
 

[-- Attachment #3: bl-lcd.diff --]
[-- Type: application/octet-stream, Size: 23553 bytes --]

--- linux-2.6.6/include/linux/lcd.h	1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6.6-hh0/include/linux/lcd.h	2004-05-27 22:48:18.000000000 +0400
@@ -0,0 +1,60 @@
+/*
+ * LCD Lowlevel Control Abstraction
+ * 
+ * Copyright (C) 2003,2004 Hewlett-Packard Company
+ * 
+ */
+
+#ifndef _LINUX_LCD_H
+#define _LINUX_LCD_H
+
+#include <linux/device.h>
+
+struct lcd_device;
+
+/* This structure defines all the properties of a LCD flat panel. */
+struct lcd_properties {
+	/* Owner module */
+	struct module *owner;
+	/* Get the power enabled status (0/1) */
+	int (*get_power) (struct lcd_device *);
+	/* Enable or disable power to the LCD */
+	int (*set_power) (struct lcd_device *, int power);
+	/* Get the LCD controller enabled status (0/1) */
+	int (*get_enable) (struct lcd_device *);
+	/* Enable or disable the LCD controller */
+	int (*set_enable) (struct lcd_device *, int enable);
+	/* The maximum value for contrast (read-only) */
+	int max_contrast;
+	/* Get the current contrast setting (0-max_contrast) */
+	int (*get_contrast) (struct lcd_device *);
+	/* Set LCD panel contrast */
+	int (*set_contrast) (struct lcd_device *, int contrast);
+};
+
+struct lcd_device {
+	/* This protects the 'props' field. If 'props' is NULL, the driver that
+	   registered this device has been unloaded, and if class_devdata()
+	   points to something in the body of that driver, it is also invalid. */
+	struct semaphore sem;
+	/* If this is NULL, the backing module is unloaded */
+	struct lcd_properties *props;
+	/* The class device structure */
+	struct class_device class_dev;
+};
+
+extern int lcd_device_register(const char *name, void *devdata,
+	struct lcd_properties *lp);
+extern void lcd_device_unregister(const char *name);
+
+extern struct lcd_device *lcd_device_find(const char *name);
+extern struct lcd_device *lcd_device_get(struct lcd_device *ld);
+extern void lcd_device_put(struct lcd_device *ld);
+
+#define to_lcd_device(obj) container_of(obj, struct lcd_device, class_dev)
+
+/* The registered clients of this notifier chain will be called every time
+   a new lcd class_device is registered */
+extern struct notifier_block *lcd_device_chain;
+
+#endif
--- linux-2.6.6/include/linux/backlight.h	1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6.6-hh0/include/linux/backlight.h	2004-05-27 22:48:27.000000000 +0400
@@ -0,0 +1,57 @@
+/*
+ * Backlight Lowlevel Control Abstraction
+ * 
+ * Copyright (C) 2003,2004 Hewlett-Packard Company
+ * 
+ */
+
+#ifndef _LINUX_BACKLIGHT_H
+#define _LINUX_BACKLIGHT_H
+
+#include <linux/device.h>
+
+struct backlight_device;
+
+/* This structure defines all the properties of a backlight
+   (usually attached to a LCD). */
+struct backlight_properties {
+	/* Owner module */
+	struct module *owner;
+	/* Get the power enabled status (0/1) */
+	int (*get_power) (struct backlight_device *);
+	/* Enable or disable power to the LCD */
+	int (*set_power) (struct backlight_device *, int power);
+	/* Maximal value for brightness (read-only) */
+	int max_brightness;
+	/* Get current backlight brightness */
+	int (*get_brightness) (struct backlight_device *);
+	/* Set backlight brightness (0..max_brightness) */
+	int (*set_brightness) (struct backlight_device *, int brightness);
+};
+
+struct backlight_device {
+	/* This protects the 'props' field. If 'props' is NULL, the driver that
+	   registered this device has been unloaded, and if class_devdata()
+	   points to something in the body of that driver, it is also invalid. */
+	struct semaphore sem;
+	/* If this is NULL, the backing module is unloaded */
+	struct backlight_properties *props;
+	/* The class device structure */
+	struct class_device class_dev;
+};
+
+extern int backlight_device_register(const char *name, void *devdata,
+				     struct backlight_properties *bp);
+extern void backlight_device_unregister(const char *name);
+
+extern struct backlight_device *backlight_device_find(const char *name);
+extern struct backlight_device *backlight_device_get(struct backlight_device *bd);
+extern void backlight_device_put(struct backlight_device *bd);
+
+#define to_backlight_device(obj) container_of(obj, struct backlight_device, class_dev)
+
+/* The registered clients of this notifier chain will be called every time
+   a new backlight class_device is registered */
+extern struct notifier_block *backlight_device_chain;
+
+#endif
--- linux-2.6.6/drivers/video/lcd.c	1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6.6-hh0/drivers/video/lcd.c	2004-05-29 01:00:52.000000000 +0400
@@ -0,0 +1,339 @@
+/*
+ * LCD Lowlevel Control Abstraction
+ * 
+ * Copyright (C) 2003,2004 Hewlett-Packard Company
+ * 
+ */
+
+#include <linux/version.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/lcd.h>
+#include <linux/notifier.h>
+#include <linux/ctype.h>
+#include <asm/bug.h>
+
+struct notifier_block *lcd_device_chain;
+EXPORT_SYMBOL (lcd_device_chain);
+
+static ssize_t lcd_show_power(struct class_device *cdev, char *buf)
+{
+	int rc;
+	struct lcd_device *ld = to_lcd_device(cdev);
+
+	down (&ld->sem);
+	if (likely (ld->props && ld->props->get_power))
+		rc = sprintf (buf, "%d\n", ld->props->get_power (ld));
+	else
+		rc = -ENXIO;
+	up (&ld->sem);
+
+	return rc;
+}
+
+static ssize_t lcd_store_power(struct class_device *cdev, const char *buf, size_t count)
+{
+	int rc, power;
+	char *endp;
+	struct lcd_device *ld = to_lcd_device(cdev);
+
+	power = simple_strtoul(buf, &endp, 0);
+	if (*endp && !isspace (*endp))
+		return -EINVAL;
+
+	down (&ld->sem);
+	if (likely (ld->props && ld->props->set_power)) {
+		pr_debug("lcd: set power to %d\n", power);
+		ld->props->set_power(ld, power);
+		rc = count;
+	} else
+		rc = -ENXIO;
+	up (&ld->sem);
+
+	return rc;
+}
+
+static ssize_t lcd_show_enable(struct class_device *cdev, char *buf)
+{
+	int rc;
+	struct lcd_device *ld = to_lcd_device(cdev);
+
+	down (&ld->sem);
+	if (likely(ld->props && ld->props->get_enable))
+		rc = sprintf (buf, "%d\n", ld->props->get_enable(ld));
+	else
+		rc = -ENXIO;
+	up (&ld->sem);
+
+	return rc;
+}
+
+static ssize_t lcd_store_enable(struct class_device *cdev, const char *buf, size_t count)
+{
+	int rc, enable;
+	char *endp;
+	struct lcd_device *ld = to_lcd_device(cdev);
+
+	enable = simple_strtoul(buf, &endp, 0);
+	if (*endp && !isspace (*endp))
+		return -EINVAL;
+
+	down (&ld->sem);
+	if (likely (ld->props && ld->props->set_enable)) {
+		pr_debug("lcd: set enable to %d\n", enable);
+		ld->props->set_enable(ld, enable);
+		rc = count;
+	} else
+		rc = -ENXIO;
+	up (&ld->sem);
+
+	return rc;
+}
+
+static ssize_t lcd_show_contrast(struct class_device *cdev, char *buf)
+{
+	int rc;
+	struct lcd_device *ld = to_lcd_device(cdev);
+
+	down (&ld->sem);
+	if (likely (ld->props && ld->props->get_contrast))
+		rc = sprintf (buf, "%d\n", ld->props->get_contrast(ld));
+	else
+		rc = -ENXIO;
+	up (&ld->sem);
+
+	return rc;
+}
+
+static ssize_t lcd_store_contrast(struct class_device *cdev, const char *buf, size_t count)
+{
+	int rc, contrast;
+	char *endp;
+	struct lcd_device *ld = to_lcd_device(cdev);
+
+	contrast = simple_strtoul(buf, &endp, 0);
+	if (*endp && !isspace (*endp))
+		return -EINVAL;
+
+	down (&ld->sem);
+	if (likely (ld->props && ld->props->set_contrast)) {
+		pr_debug("lcd: set contrast to %d\n", contrast);
+		ld->props->set_contrast(ld, contrast);
+		rc = count;
+	} else
+		rc = -ENXIO;
+	up (&ld->sem);
+
+	return rc;
+}
+
+static ssize_t lcd_show_max_contrast(struct class_device *cdev, char *buf)
+{
+	int rc;
+	struct lcd_device *ld = to_lcd_device(cdev);
+
+	down (&ld->sem);
+	if (likely (ld->props))
+		rc = sprintf (buf, "%d\n", ld->props->max_contrast);
+	else
+		rc = -ENXIO;
+	up (&ld->sem);
+
+	return rc;
+}
+
+static void lcd_class_release(struct class_device *dev)
+{
+	struct lcd_device *ld = to_lcd_device (dev);
+	kfree (ld);
+}
+
+struct class lcd_class = {
+	.name = "lcd",
+	.release = lcd_class_release,
+};
+
+#define DECLARE_ATTR(_name,_mode,_show,_store)			\
+{							 	\
+	.attr	= { .name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },	\
+	.show	= _show,					\
+	.store	= _store,					\
+}
+
+static struct class_device_attribute lcd_class_device_attributes[] = {
+	DECLARE_ATTR (power, 0644, lcd_show_power, lcd_store_power),
+	DECLARE_ATTR (enable, 0644, lcd_show_enable, lcd_store_enable),
+	DECLARE_ATTR (contrast, 0644, lcd_show_contrast, lcd_store_contrast),
+	DECLARE_ATTR (max_contrast, 0444, lcd_show_max_contrast, NULL),
+};
+
+/**
+ * lcd_device_register - register a new object of lcd_device class.
+ * @name: the name of the new object (must be the same as the name of the
+ *   respective framebuffer device).
+ * @devdata: an optional pointer to be stored in the class_device. The
+ *   methods may retrieve it by using class_get_devdata (ld->class_dev).
+ * @lp: the lcd properties structure.
+ *
+ * Creates a new lcd class_device and copies data from the received ld
+ * structure into the new object. Returns after registering the new object.
+ */
+int lcd_device_register (const char *name, void *devdata,
+			 struct lcd_properties *lp)
+{
+	int i, rc;
+	struct lcd_device *new_ld;
+
+	pr_debug("lcd_device_register: name=%s\n", name);
+
+	new_ld = kmalloc (sizeof (struct lcd_device), GFP_KERNEL);
+	if (unlikely (!new_ld))
+		return -ENOMEM;
+
+	init_MUTEX (&new_ld->sem);
+	new_ld->props = lp;
+	new_ld->class_dev.class = &lcd_class;
+	strlcpy (new_ld->class_dev.class_id, name, KOBJ_NAME_LEN);
+	class_set_devdata (&new_ld->class_dev, devdata);
+	rc = class_device_register (&new_ld->class_dev);
+	if (unlikely (rc)) {
+		kfree (new_ld);
+		return rc;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(lcd_class_device_attributes); i++) {
+		rc = class_device_create_file(&new_ld->class_dev,
+					      &lcd_class_device_attributes[i]);
+		if (unlikely (rc)) {
+			while (--i >= 0)
+				class_device_remove_file(&new_ld->class_dev,
+							 &lcd_class_device_attributes[i]);
+			class_device_unregister(&new_ld->class_dev);
+			return rc;
+		}
+	}
+
+	notifier_call_chain (&lcd_device_chain, 0, new_ld);
+
+	return 0;
+}
+EXPORT_SYMBOL(lcd_device_register);
+
+/**
+ * lcd_device_unregister - unregisters a object of lcd_device class.
+ * @name: the name of the object (must be the same as the name of the
+ *   respective framebuffer device).
+ *
+ * Unregisters a previously registered via lcd_device_register object.
+ */
+void lcd_device_unregister(const char *name)
+{
+	int i;
+	struct lcd_device *ld;
+	struct class_device *class_dev =
+		class_device_find (&lcd_class, name);
+
+	if (!class_dev)
+		return;
+
+	pr_debug("lcd_device_unregister: name=%s\n", name);
+
+	for (i = 0; i < ARRAY_SIZE (lcd_class_device_attributes); i++)
+		class_device_remove_file (class_dev,
+					  &lcd_class_device_attributes[i]);
+
+	ld = to_lcd_device (class_dev);
+	down (&ld->sem);
+	ld->props = NULL;
+	up (&ld->sem);
+
+	class_device_unregister(class_dev);
+	/* We got the reference counter incremented by class_device_find() */
+	class_device_put(class_dev);
+}
+EXPORT_SYMBOL(lcd_device_unregister);
+
+/**
+ * lcd_device_find - find a LCD device by name.
+ * @name: the name of the LCD object to find.
+ *
+ * The reference counter of the returned object as well as on the module
+ * that implements the backlight functions are incremented.
+ */
+struct lcd_device *lcd_device_find(const char *name)
+{
+	struct lcd_device *ld;
+	struct class_device *class_dev =
+		class_device_find (&lcd_class, name);
+
+	if (unlikely (!class_dev))
+		return NULL;
+
+	ld = lcd_device_get (to_lcd_device (class_dev));
+	class_device_put (class_dev);
+	return ld;
+}
+EXPORT_SYMBOL(lcd_device_find);
+
+/**
+ * lcd_device_get - increment reference counter of a LCD device
+ *   and on the module that implements the LCD device methods.
+ * @ld: the LCD device object.
+ *
+ * Increments the reference counter on both the LCD device and on the
+ * module that implements this LCD device.
+ */
+struct lcd_device *lcd_device_get(struct lcd_device *ld)
+{
+	struct class_device *class_dev = class_device_get (&ld->class_dev);
+
+	if (likely (class_dev)) {
+		down (&ld->sem);
+		if (likely (ld->props && try_module_get (ld->props->owner))) {
+			up (&ld->sem);
+			return ld;
+		}
+		up (&ld->sem);
+
+		class_device_put(class_dev);
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(lcd_device_get);
+
+/**
+ * lcd_device_put - decrement reference counter of a LCD device.
+ * @ld: the LCD device object.
+ *
+ * Decrements the reference counter on both the LCD device and on the
+ * module that implements this LCD device.
+ */
+void lcd_device_put(struct lcd_device *ld)
+{
+	if (ld) {
+		down (&ld->sem);
+		if (ld->props)
+			module_put(ld->props->owner);
+		up (&ld->sem);
+		class_device_put (&ld->class_dev);
+	}
+}
+EXPORT_SYMBOL(lcd_device_put);
+
+static void __exit lcd_class_exit(void)
+{
+	class_unregister(&lcd_class);
+}
+
+static int __init lcd_class_init(void)
+{
+	return class_register(&lcd_class);
+}
+
+/*
+ * if this is compiled into the kernel, we need to ensure that the
+ * class is registered before users of the class try to register lcd's
+ */
+postcore_initcall(lcd_class_init);
+module_exit(lcd_class_exit);
--- linux-2.6.6/drivers/video/backlight.c	1970-01-01 03:00:00.000000000 +0300
+++ linux-2.6.6-hh0/drivers/video/backlight.c	2004-05-29 00:56:53.000000000 +0400
@@ -0,0 +1,301 @@
+/*
+ * Backlight Lowlevel Control Abstraction
+ * 
+ * Copyright (C) 2003,2004 Hewlett-Packard Company
+ * 
+ */
+
+#include <linux/version.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/device.h>
+#include <linux/backlight.h>
+#include <linux/notifier.h>
+#include <linux/ctype.h>
+#include <asm/bug.h>
+
+struct notifier_block *backlight_device_chain;
+EXPORT_SYMBOL (backlight_device_chain);
+
+static ssize_t backlight_show_power (struct class_device *cdev, char *buf)
+{
+	int rc;
+	struct backlight_device *bd = to_backlight_device (cdev);
+
+	down (&bd->sem);
+	if (likely (bd->props && bd->props->get_power))
+		rc = sprintf (buf, "%d\n", bd->props->get_power (bd));
+	else
+		rc = -ENXIO;
+	up (&bd->sem);
+
+	return rc;
+}
+
+static ssize_t backlight_store_power(struct class_device *cdev, const char *buf, size_t count)
+{
+	int rc, power;
+	char *endp;
+	struct backlight_device *bd = to_backlight_device (cdev);
+
+	power = simple_strtoul (buf, &endp, 0);
+	if (*endp && !isspace (*endp))
+		return -EINVAL;
+
+	down (&bd->sem);
+	if (likely (bd->props && bd->props->set_power)) {
+		pr_debug("backlight: set power to %d\n", power);
+		bd->props->set_power(bd, power);
+		rc = count;
+	} else
+		rc = -ENXIO;
+	up (&bd->sem);
+
+	return rc;
+}
+
+static ssize_t backlight_show_brightness(struct class_device *cdev, char *buf)
+{
+	int rc;
+	struct backlight_device *bd = to_backlight_device (cdev);
+
+	down (&bd->sem);
+	if (likely (bd->props && bd->props->get_brightness))
+		rc = sprintf (buf, "%d\n", bd->props->get_brightness (bd));
+	else
+		rc = -ENXIO;
+	up (&bd->sem);
+
+	return rc;
+}
+
+static ssize_t backlight_store_brightness(struct class_device *cdev, const char *buf, size_t count)
+{
+	int rc, brightness;
+	char *endp;
+	struct backlight_device *bd = to_backlight_device (cdev);
+
+	brightness = simple_strtoul (buf, &endp, 0);
+	if (*endp && !isspace (*endp))
+		return -EINVAL;
+
+	down (&bd->sem);
+	if (likely (bd->props && bd->props->set_brightness)) {
+		pr_debug("backlight: set brightness to %d\n", brightness);
+		bd->props->set_brightness(bd, brightness);
+		rc = count;
+	} else
+		rc = -ENXIO;
+	up (&bd->sem);
+
+	return rc;
+}
+
+static ssize_t backlight_show_max_brightness(struct class_device *cdev, char *buf)
+{
+	int rc;
+	struct backlight_device *bd = to_backlight_device (cdev);
+
+	down (&bd->sem);
+	if (likely (bd->props))
+		rc = sprintf(buf, "%d\n", bd->props->max_brightness);
+	else
+		rc = -ENXIO;
+	up (&bd->sem);
+
+	return rc;
+}
+
+static void backlight_class_release(struct class_device *dev)
+{
+	struct backlight_device *bd = to_backlight_device (dev);
+	kfree (bd);
+}
+
+struct class backlight_class = {
+	.name = "backlight",
+	.release = backlight_class_release,
+};
+
+#define DECLARE_ATTR(_name,_mode,_show,_store)			\
+{							 	\
+	.attr	= { .name = __stringify(_name), .mode = _mode, .owner = THIS_MODULE },	\
+	.show	= _show,					\
+	.store	= _store,					\
+}
+
+static struct class_device_attribute bl_class_device_attributes[] = {
+	DECLARE_ATTR (power, 0644, backlight_show_power, backlight_store_power),
+	DECLARE_ATTR (brightness, 0644, backlight_show_brightness, backlight_store_brightness),
+	DECLARE_ATTR (max_brightness, 0444, backlight_show_max_brightness, NULL),
+};
+
+/**
+ * backlight_device_register - register a new object of backlight_device class.
+ * @name: the name of the new object (must be the same as the name of the
+ *   respective framebuffer device).
+ * @devdata: an optional pointer to be stored in the class_device. The
+ *   methods may retrieve it by using class_get_devdata (&bd->class_dev).
+ * @bp: the backlight properties structure.
+ *
+ * Creates a new backlight class_device. Returns after registering the
+ * new object.
+ */
+int backlight_device_register(const char *name, void *devdata,
+			      struct backlight_properties *bp)
+{
+	int i, rc;
+	struct backlight_device *new_bd;
+
+	pr_debug("backlight_device_register: name=%s\n", name);
+
+	new_bd = kmalloc (sizeof (struct backlight_device), GFP_KERNEL);
+	if (unlikely (!new_bd))
+		return -ENOMEM;
+
+	init_MUTEX (&new_bd->sem);
+	new_bd->props = bp;
+	new_bd->class_dev.class = &backlight_class;
+	strlcpy (new_bd->class_dev.class_id, name, KOBJ_NAME_LEN);
+	class_set_devdata (&new_bd->class_dev, devdata);
+	rc = class_device_register (&new_bd->class_dev);
+	if (unlikely (rc)) {
+		kfree (new_bd);
+		return rc;
+	}
+
+	for (i = 0; i < ARRAY_SIZE(bl_class_device_attributes); i++) {
+		rc = class_device_create_file(&new_bd->class_dev,
+					      &bl_class_device_attributes[i]);
+		if (unlikely (rc)) {
+			while (--i >= 0)
+				class_device_remove_file(&new_bd->class_dev,
+							 &bl_class_device_attributes[i]);
+			class_device_unregister(&new_bd->class_dev);
+			return rc;
+		}
+	}
+
+	notifier_call_chain (&backlight_device_chain, 0, new_bd);
+
+	return 0;
+}
+EXPORT_SYMBOL(backlight_device_register);
+
+/**
+ * backlight_device_unregister - unregisters a object of backlight_device class.
+ * @name: the name of the object (must be the same as the name of the
+ *   respective framebuffer device).
+ *
+ * Unregisters a previously registered via backlight_device_register object.
+ */
+void backlight_device_unregister(const char *name)
+{
+	int i;
+	struct backlight_device *bd;
+	struct class_device *class_dev =
+		class_device_find (&backlight_class, name);
+
+	if (!class_dev)
+		return;
+
+	pr_debug("backlight_device_unregister: name=%s\n", name);
+
+	for (i = 0; i < ARRAY_SIZE(bl_class_device_attributes); i++)
+		class_device_remove_file (class_dev,
+                                          &bl_class_device_attributes[i]);
+
+	bd = to_backlight_device (class_dev);
+	down (&bd->sem);
+	bd->props = NULL;
+	up (&bd->sem);
+
+	class_device_unregister(class_dev);
+	/* We got the reference counter incremented by class_device_find() */
+	class_device_put(class_dev);
+}
+EXPORT_SYMBOL(backlight_device_unregister);
+
+/**
+ * backlight_device_find - find a backlight device by name.
+ * @name: the name of the backlight object to find.
+ *
+ * The reference counter of the returned object as well as on the module
+ * that implements the backlight functions are incremented.
+ */
+struct backlight_device *backlight_device_find(const char *name)
+{
+	struct backlight_device *bd;
+	struct class_device *class_dev =
+		class_device_find (&backlight_class, name);
+
+	if (unlikely (!class_dev))
+		return NULL;
+
+	bd = backlight_device_get (to_backlight_device (class_dev));
+	class_device_put (class_dev);
+	return bd;
+}
+EXPORT_SYMBOL(backlight_device_find);
+
+/**
+ * backlight_device_get - increment reference counter of a backlight device
+ *   and on the module that implements the backlight device methods.
+ * @bd: the backlight device object.
+ *
+ * Increments the reference counter on both the backlight device and on the
+ * module that implements this backlight device.
+ */
+struct backlight_device *backlight_device_get(struct backlight_device *bd)
+{
+	struct class_device *class_dev = class_device_get (&bd->class_dev);
+
+	if (likely (class_dev)) {
+		down (&bd->sem);
+		if (likely (bd->props && try_module_get (bd->props->owner))) {
+			up (&bd->sem);
+			return bd;
+		}
+                up (&bd->sem);
+
+		class_device_put(class_dev);
+	}
+	return NULL;
+}
+EXPORT_SYMBOL(backlight_device_get);
+
+/**
+ * backlight_device_put - decrement reference counter of a backlight device.
+ * @ld: the backlight device object.
+ *
+ * Decrements the reference counter on both the backlight device and on the
+ * module that implements this backlight device.
+ */
+void backlight_device_put(struct backlight_device *bd)
+{
+	if (bd) {
+		down (&bd->sem);
+		if (bd->props)
+			module_put(bd->props->owner);
+		up (&bd->sem);
+		class_device_put (&bd->class_dev);
+	}
+}
+EXPORT_SYMBOL(backlight_device_put);
+
+static void __exit backlight_class_exit(void)
+{
+	class_unregister(&backlight_class);
+}
+
+static int __init backlight_class_init(void)
+{
+	return class_register(&backlight_class);
+}
+
+/*
+ * if this is compiled into the kernel, we need to ensure that the
+ * class is registered before users of the class try to register lcd's
+ */
+postcore_initcall(backlight_class_init);
+module_exit(backlight_class_exit);
--- linux-2.6.6/drivers/video/Kconfig	2004-05-10 06:31:59.000000000 +0400
+++ replacement/drivers/video/Kconfig	2004-05-28 23:19:45.000000000 +0400
@@ -38,6 +38,38 @@
 	  (e.g. an accelerated X server) and that are not frame buffer
 	  device-aware may cause unexpected results. If unsure, say N.
 
+config LCD_CLASS_DEVICE
+        tristate "Lowlevel LCD controls"
+	depends on FB
+	help
+	  This framework adds support for low-level control of LCD.
+	  Some framebuffer devices connect to platform-specific LCD modules
+	  in order to have a platform-specific way to control the flat panel
+	  (contrast and applying power to the LCD (not to the backlight!)).
+
+	  To have support for your specific LCD panel you will have to
+	  select the proper drivers which depend on this option.
+
+config LCD_DEVICE
+	bool
+	depends on LCD_CLASS_DEVICE
+	default FB
+
+config BACKLIGHT_CLASS_DEVICE
+        tristate "Lowlevel Backlight controls"
+	depends on FB
+	help
+	  This framework adds support for low-level control of the LCD
+          backlight. This includes support for brightness and power.
+
+	  To have support for your specific LCD panel you will have to
+	  select the proper drivers which depend on this option.
+
+config BACKLIGHT_DEVICE
+	bool
+	depends on BACKLIGHT_CLASS_DEVICE
+	default FB
+
 config FB_CIRRUS
 	tristate "Cirrus Logic support"
 	depends on FB && (AMIGA || PCI) && BROKEN
--- linux-2.6.6/drivers/video/Makefile	2004-05-10 06:32:00.000000000 +0400
+++ replacement/drivers/video/Makefile	2004-05-25 21:29:26.000000000 +0400
@@ -13,6 +13,9 @@
 obj-$(CONFIG_PPC)                 += macmodes.o
 endif
 
+obj-$(CONFIG_LCD_DEVICE)          += lcd.o
+obj-$(CONFIG_BACKLIGHT_DEVICE)    += backlight.o
+
 obj-$(CONFIG_FB_ACORN)            += acornfb.o cfbfillrect.o cfbcopyarea.o cfbimgblt.o
 obj-$(CONFIG_FB_AMIGA)            += amifb.o c2p.o
 obj-$(CONFIG_FB_PM2)              += pm2fb.o cfbfillrect.o cfbcopyarea.o cfbimgblt.o

Re: two patches - request for comments

[Todd Poynor]

Hi, you're adding new interfaces for power management of LCD and 
backlight devices.  Since there's already LDM/sysfs interfaces for 
reading and writing power state of generic devices, is it necessary to 
add ones particular to these devices or device classes?  In other words, 
is /sys/devices/<bus>/<device>/power/state not suitable for these purposes?

And if a PM interface for device classes is needed that ties into the 
device driver suspend/resume callbacks, perhaps it can be modeled more 
closely on the existing interfaces?  These new interfaces seem to be 
intended to define: 0 == power off, 1 ==  power on.  The existing 
ACPI-inspired interfaces use: 0 == power on/full-power, 1/2/3/4 == 
low-power/off state.

New files don't have GPL license comments.

-- 
Todd

Re: two patches - request for comments

[Greg KH]

On Sat, May 29, 2004 at 01:20:30AM +0400, Andrew Zabolotny wrote:
> Hello!
> 
> I'm going to submit the class_find_device() patch (attached for your
> convenience) as a pre-requisite for the backlight/lcd device class patch
> (also included so that you can take at it as well) via Russel King (the
> backlight/lcd patch is needed for our ARM-based handhelds framebuffer
> devices). Any comments/objections are welcome.
> 
> The LCD and backlight device classes were implemented with the following in
> mind:

Becides the comments that Todd had about the power management stuff, I
have the following comments:
	- please inline your patches, I can't quote them :(
	- you create the DEVICE_ATTR macro, why not use the one already
	  created for you (CLASS_DEVICE_ATTR will work I think.)
	- Don't do a unregister function by passing a string to it.
	  Explicitly pass the pointer of the object that you want to
	  unregister, like all other kernel interfaces do.  With that
	  change you no longer need the class_find_device() patch,
	  right?
	- How about some drivers that actually use this interface?
	  Again, you are creating interfaces with no examples of users
	  of the interface, which isn't acceptable.

thanks,

greg k-h

Re: two patches - request for comments

[Andrew Zabolotny]

On Fri, 28 May 2004 14:59:53 -0700
Todd Poynor <tpoynor@mvista.com> wrote:

> Hi, you're adding new interfaces for power management of LCD and 
> backlight devices.  Since there's already LDM/sysfs interfaces for 
> reading and writing power state of generic devices, is it necessary to 
> add ones particular to these devices or device classes?  In other words, 
> is /sys/devices/<bus>/<device>/power/state not suitable for these purposes?
Well, first liquid crystal displays and backlight are not connected to any
specific bus. They could, on some platforms, and could not on other. For
example, on some PDAs backlight is controlled via the programmed I/O CPU pins
(GPIO), on other they are connected to some weird companion chips, on third
they could be controlled via the PCI bus (on large PCs, I mean) and so on. So
there will be no easy way for an application to find the backlight devices
and control them. In our case that's easy: opendir ("/sys/class/backlight")
and you found all of them.

On the other hand, the lcd and backlight modules will be used by the
framebuffer device, which in most cases will be itself on a certain bus. And
when the framebuffer device will be powered off, it contacts the corresponding
lcd and backlight devices to power them off as well.

On the third hand (:-) the lcd device class, for example, actually has *two*
power switches: the 'power' and the 'enable' attribute. The first powers off
the liquid crystal display, and seconds powers off the lcd controller. These
are different things and it would be wise to leave them apart, as having finer
grained control is always better. Also they have attributes that are in no way
part of power management: contrast, brightness etc. So these attributes would
have to be spreaded out across the entire sysfs.

On the fourth hand (oh no), what would be the code sequence for a framebuffer
device to find the corresponding lcd and backlight devices? For now that's
done by searching the 'backlight' and 'lcd' device classes for a device with
the same name as the framebuffer device; if we place every device on its own
bus, the framebuffer device will have to search all the buses in the system.
Not speaking that the code will be substantially larger (current code is
already 2-3k for each class, which is alot if we're speaking of embedded
systems).

> And if a PM interface for device classes is needed that ties into the 
> device driver suspend/resume callbacks, perhaps it can be modeled more 
> closely on the existing interfaces?  These new interfaces seem to be 
> intended to define: 0 == power off, 1 ==  power on.  The existing 
> ACPI-inspired interfaces use: 0 == power on/full-power, 1/2/3/4 == 
> low-power/off state.
Um, well, this could be implemented. Although I don't see much reason for a
backlight to be in a "semi-enabled state"; besides if it will remain a
separate class device, it doesn't matter much. But if someone cares, it could
be changed now since there is not much code which depends on that.

> New files don't have GPL license comments.
Indeed, I forgot to add them. Initially they weren't designed to be compiled
as modules.

--
Greetings,
   Andrew

Re: two patches - request for comments

[Andrew Zabolotny]

On Fri, 28 May 2004 15:10:06 -0700
Greg KH <greg@kroah.com> wrote:

> 	- you create the DEVICE_ATTR macro, why not use the one already
> 	  created for you (CLASS_DEVICE_ATTR will work I think.)
Because it would involve unneeded extra garbage into data segment. See:

CLASS_DEVICE_ATTR(_name,_mode,_show,_store) is:
struct class_device_attribute class_device_attr_##_name = {
  ...
}

DECLARE_ATTR is just:
{ ... }

This way, I cannot use CLASS_DEVICE_ATTR for things like:

static struct class_device_attribute bl_class_device_attributes[] = {
	DECLARE_ATTR(...),
	DECLARE_ATTR(...),
	...
};

Instead, I must declare it this way:

CLASS_DEVICE_ATTR(blah1)
CLASS_DEVICE_ATTR(blah2)
CLASS_DEVICE_ATTR(blah3)

and after that:

struct class_device_attribut *blah_ptr [] = {
	blah1, blah2, blah3
};

The second array is absolutely unneeded here (one garbage pointer for every
device attribute plus alignment), since the array is absolutely static. And
even worse, the array cannot be declared __initdata since devices can
register and unregister at any time.

> 	- Don't do a unregister function by passing a string to it.
> 	  Explicitly pass the pointer of the object that you want to
> 	  unregister, like all other kernel interfaces do.  With that
> 	  change you no longer need the class_find_device() patch,
> 	  right?
No. class_find_device was written for lcd_find_device() and
backlight_find_device() (framebuffer devices use them). On the other hand,
the driver that registers the backlight device doesn't have a pointer to the
class device (well, the lcd_register_device could return it). Since the
lcd/backlight names are unique anyway, I don't see any problems with that, and
moreover, it is *registered* by giving it a name, why it should be
unregistered in a different way?

In any case, if you have strong objections against that, this could be changed
of course. But again, it will result in more useless code/data (the driver
will have to store somewhere the device it has registered, or alternatively,
use lcd/backlight_device_find()).

> 	- How about some drivers that actually use this interface?
> 	  Again, you are creating interfaces with no examples of users
> 	  of the interface, which isn't acceptable.
There are already four drivers that implement the lcd/backlight devices (for
Dell Axim X5, iPAQ 2210, Jornada 560, Rover P5), and three framebuffer devices
that were modified to use it (sa1100fb, pxafb and mq1100fb). I would paste
them here but they are quite large, and I wouldn't like to pollute this list,
it's so high-traffic already.

Instead, you can download them from
http://zap.eltrast.ru/data/backlight-lcd-class-devices.tar.bz2 (~33k).

The full sources are in handhelds.org' public CVS
(:pserver:anoncvs@cvs.handhelds.org:/cvs, repository linux/kernel26).

--
Greetings,
   Andrew

Re: two patches - request for comments

[Denis Vlasenko]

On Saturday 29 May 2004 00:20, Andrew Zabolotny wrote:
> Hello!
>
> I'm going to submit the class_find_device() patch (attached for your
> convenience) as a pre-requisite for the backlight/lcd device class patch
> (also included so that you can take at it as well) via Russel King (the

Danger Will Robinson. Try to do the same to Rusty Russell and you're toast ;)
--
vda

Re: two patches - request for comments

[Todd Poynor]

Andrew Zabolotny wrote:
> On Fri, 28 May 2004 14:59:53 -0700
> Todd Poynor <tpoynor@mvista.com> wrote:
> 
> 
>>Hi, you're adding new interfaces for power management of LCD and 
>>backlight devices.  Since there's already LDM/sysfs interfaces for 
>>reading and writing power state of generic devices, is it necessary to 
>>add ones particular to these devices or device classes?  In other words, 
>>is /sys/devices/<bus>/<device>/power/state not suitable for these purposes?
> 
> Well, first liquid crystal displays and backlight are not connected to any
> specific bus. They could, on some platforms, and could not on other. For
> example, on some PDAs backlight is controlled via the programmed I/O CPU pins
> (GPIO), on other they are connected to some weird companion chips, on third
> they could be controlled via the PCI bus (on large PCs, I mean) and so on. So
> there will be no easy way for an application to find the backlight devices
> and control them. In our case that's easy: opendir ("/sys/class/backlight")
> and you found all of them.

I'm not questioning the usefulness of the other aspects of the patch, 
such as adding an LCD/backlight class for framebuffer access and adding 
attributes for the unique features of LCD/backlight devices.  My 
questions are specific to the power management interfaces, since there's 
already interfaces for this, with different semantics than the new class 
interfaces, and there's some value in sticking with a single consistent 
interface for it.

If I understand correctly, the LCD device is registered on a bus (either 
a platform-specific bus or the generic "platform" bus), and the device 
therefore already has a power/state attribute; the class entry could 
refer back to that device if needed.  So I'm interested in discussing 
whether the existing PM interface suffices for LCD/backlight devices, or 
if not, should the existing interfaces be improved (rather than working 
around deficiencies via device-specific interfaces)?

 > [...]
> On the third hand (:-) the lcd device class, for example, actually has *two*
> power switches: the 'power' and the 'enable' attribute. The first powers off
> the liquid crystal display, and seconds powers off the lcd controller. These
> are different things and it would be wise to leave them apart, as having finer
> grained control is always better. [...]

But it also sounds like the single LDM registration for an LCD device 
could be better handled by registering the LCD display, LCD controller, 
and backlight as separate devices (which they probably are), allowing 
individual control through the standard interfaces.

>>And if a PM interface for device classes is needed that ties into the 
>>device driver suspend/resume callbacks, perhaps it can be modeled more 
>>closely on the existing interfaces?  These new interfaces seem to be 
>>intended to define: 0 == power off, 1 ==  power on.  The existing 
>>ACPI-inspired interfaces use: 0 == power on/full-power, 1/2/3/4 == 
>>low-power/off state.
> 
> Um, well, this could be implemented. Although I don't see much reason for a
> backlight to be in a "semi-enabled state"; besides if it will remain a
> separate class device, it doesn't matter much. 

The main problem is that existing interfaces write zero for on and 
non-zero for off, while the new interface reverses things.  I realize 
that multiple power states are not implemented by all devices; if 
there's interest in tailoring device states to more closely match the 
hardware capabilities of non-ACPI systems then I'd be happy to talk more 
about that.

So none of my objections are terribly crucial, and if Greg et al don't 
have a problem with device-class-specific PM interfaces that have 
different semantics and/or capabilities than those of the device 
power/state attributes then I don't have much of a problem with it 
either.  Just seems worthwhile to check whether there's improvements 
needed in the existing PM interfaces instead.


-- 
Todd

Re: two patches - request for comments

[Andrew Zabolotny]

On Tue, 01 Jun 2004 13:09:46 -0700
Todd Poynor <tpoynor@mvista.com> wrote:

> I'm not questioning the usefulness of the other aspects of the patch, 
> such as adding an LCD/backlight class for framebuffer access and adding 
> attributes for the unique features of LCD/backlight devices.  My 
> questions are specific to the power management interfaces, since there's 
> already interfaces for this, with different semantics than the new class 
> interfaces, and there's some value in sticking with a single consistent 
> interface for it.
Well, after thinking awhile, I have changed it so that 0 means power on,
1..3 intermediate values (although for now they are interpreted as poweroff by
existing drivers since there are no intermediate states) and 4 is 'off for
real'. Indeed, there is no much reason to have them use different semantics.

> If I understand correctly, the LCD device is registered on a bus (either 
> a platform-specific bus or the generic "platform" bus)
No, they are registered just as a class device. There is no corresponding
device on any other bus, this would mean a lot of unneeded overhead.

> therefore already has a power/state attribute; the class entry could 
> refer back to that device if needed.  So I'm interested in discussing 
> whether the existing PM interface suffices for LCD/backlight devices, or 
> if not, should the existing interfaces be improved (rather than working 
> around deficiencies via device-specific interfaces)?
Um, well, the LCD device actually has two power controls, like I said
before: one toggles the power to the LCD itself, another one (the 'enable'
attribute) controls power to the LCD controller. Not that this explicit
separate control is required very much, but it would be nice to have a degree
of freedom close to that allowed by hardware.

In theory, if we would use the standard power interface, it could use the
different levels of power saving, e.g. 0 - controller and LCD on, 1,2 - LCD
off, controller on, 3,4 - both off.

> But it also sounds like the single LDM registration for an LCD device 
> could be better handled by registering the LCD display, LCD controller, 
> and backlight as separate devices (which they probably are), allowing 
> individual control through the standard interfaces.
Well, the LCD panel is controllable only through the LCD controller, so for
the most part they are the same. The only thing is that the LCD controller has
a one-bit switch to disable the power to the panel. I don't think it makes
sense to separate that bit into a separate device.

> So none of my objections are terribly crucial, and if Greg et al don't 
> have a problem with device-class-specific PM interfaces that have 
> different semantics and/or capabilities than those of the device 
> power/state attributes then I don't have much of a problem with it 
> either.  Just seems worthwhile to check whether there's improvements 
> needed in the existing PM interfaces instead.
Well, the power interface under drivers/ is available for framebuffer.
If it would handle it properly (the framebuffer drivers I've tried
don't, alas), they would toggle the attached (to the framebuffer) LCD
and backlight power state according to its own state (which is not so fine
grained, but is logical). In any case, one of reasons this backlight/lcd patch
has been written was to avoid that mess with callbacks that lately begun to
appear in ARM-specific framebuffer devices (and I shudder at the thought that
MIPS people should be doing something similar).

--
Greetings,
   Andrew

Re: two patches - request for comments

[Jeff Garzik]

Andrew Zabolotny wrote:
> No. class_find_device was written for lcd_find_device() and
> backlight_find_device() (framebuffer devices use them). On the other hand,
> the driver that registers the backlight device doesn't have a pointer to the
> class device (well, the lcd_register_device could return it). Since the
> lcd/backlight names are unique anyway, I don't see any problems with that, and
> moreover, it is *registered* by giving it a name, why it should be
> unregistered in a different way?


Typical Linux usage to an item being registered is

	ptr = alloc_foo()
	register_foo(ptr)
	unregister_foo(ptr)
	free_foo()

It is quite unusual to unregister based on name.  Pointers are far more 
likely to be unique, and the programmer is far less likely to screw up 
the unregister operation.

	Jeff

Re: two patches - request for comments

[Andrew Zabolotny]

On Tue, 01 Jun 2004 17:23:11 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> Typical Linux usage to an item being registered is
> 	ptr = alloc_foo()
> 	register_foo(ptr)
> 	unregister_foo(ptr)
> 	free_foo()
In this case it is:

register_lcd_device("foo", ...);
...
unregister_lcd_device("foo");

The name is guaranteed to be unique by sysfs design during the whole
device lifetime, and calling unregister_xxx() outside the lifetime brackets
is clearly an error.

> It is quite unusual to unregister based on name.  Pointers are far more 
> likely to be unique, and the programmer is far less likely to screw up 
> the unregister operation.
I understand this, I see why it looks unusual. I'll fix this if it matters.
It'll be something like:

lcd_device = register_lcd_device ("foo", ...);
...
unregister_lcd_device (lcd_device);

However, this extra pointer will take some memory; not much of course.

--
Greetings,
   Andrew

Re: two patches - request for comments

[Greg KH]

On Wed, Jun 02, 2004 at 01:57:40AM +0400, Andrew Zabolotny wrote:
> On Tue, 01 Jun 2004 17:23:11 -0400
> Jeff Garzik <jgarzik@pobox.com> wrote:
> 
> > Typical Linux usage to an item being registered is
> > 	ptr = alloc_foo()
> > 	register_foo(ptr)
> > 	unregister_foo(ptr)
> > 	free_foo()
> In this case it is:
> 
> register_lcd_device("foo", ...);
> ...
> unregister_lcd_device("foo");
> 
> The name is guaranteed to be unique by sysfs design during the whole
> device lifetime, and calling unregister_xxx() outside the lifetime brackets
> is clearly an error.
> 
> > It is quite unusual to unregister based on name.  Pointers are far more 
> > likely to be unique, and the programmer is far less likely to screw up 
> > the unregister operation.
> I understand this, I see why it looks unusual. I'll fix this if it matters.

It matters, please fix it.

> It'll be something like:
> 
> lcd_device = register_lcd_device ("foo", ...);
> ...
> unregister_lcd_device (lcd_device);

What about:
	lcd_device = alloc_lcd_device("foo", ...);
	error = register_lcd_device(lcd_device);
	...
	unregister_lcd_device(lcd_device);

thanks,

greg k-h

Re: two patches - request for comments

[Greg KH]

On Wed, Jun 02, 2004 at 01:00:36AM +0400, Andrew Zabolotny wrote:
> 
> In theory, if we would use the standard power interface, it could use the
> different levels of power saving, e.g. 0 - controller and LCD on, 1,2 - LCD
> off, controller on, 3,4 - both off.

Please use the standard power interface, and use the standard levels of
power state.  That's why we _have_ this driver model in the first
place...

> > So none of my objections are terribly crucial, and if Greg et al don't 
> > have a problem with device-class-specific PM interfaces that have 
> > different semantics and/or capabilities than those of the device 
> > power/state attributes then I don't have much of a problem with it 
> > either.  Just seems worthwhile to check whether there's improvements 
> > needed in the existing PM interfaces instead.

I do have a problem with device-class-specific PM interfaces that have
different semantics from the whole rest of the system.

> Well, the power interface under drivers/ is available for framebuffer.
> If it would handle it properly (the framebuffer drivers I've tried
> don't, alas)

Then they need to be fixed to do so.

thanks,

greg k-h

Re: two patches - request for comments

[Andrew Zabolotny]

On Wed, 2 Jun 2004 10:15:42 -0700
Greg KH <greg@kroah.com> wrote:

> > In theory, if we would use the standard power interface, it could use the
> > different levels of power saving, e.g. 0 - controller and LCD on, 1,2 -
> > LCD off, controller on, 3,4 - both off.
> Please use the standard power interface, and use the standard levels of
> power state.  That's why we _have_ this driver model in the first
> place...
The problem is that I cannot use the standard power interface because I don't
know how. The patch in question implements class devices, not regular devices.
Class devices don't use the standard power management scheme. Here:

/sys # ls devices/platform/
detach_state    power           pxa2xx_udc0
mq11xx0         pxa2xx-pcmcia0  pxafb0
mq11xx_udc0     pxa2xx_serial0  pxamci0

None of the above devices implement the backlight and LCD; they use bits from
different devices; in my case several GPIO pins from the mq11xx0 device, a
couple of CPU' GPIO pins (which aren't present in sysfs at all).

Now the mq11xx0 device (which is a System-On-Chip, e.g. a single chip
encomprising a lot of somehow-independent-functions) has a lot of subdevices:

/sys # ls devices/platform/mq11xx0/
detach_state  mq11xx_i2s0   mq11xx_spi0   mq11xx_uhc0
mq11xx_fb0    mq11xx_lcd0   mq11xx_udc0   power

The backlight is controlled by a few spare bits from the mq11xx_spi0 device
(which is a SPI bus controller). I don't think it makes sense to create the
backlight device node under /sys/devices/platform/mq11xx0/mq11xx_spi0/;
besides that's on my PDA; on other PDAs they could be located anywhere else,
thus the software will have a major headache to find the backlight/lcd power
controls.

Right now it works this way:

/sys # ls class/backlight/mq11xx_fb0/
brightness      max_brightness  power
/sys # ls class/lcd/mq11xx_fb0/
contrast      enable        max_contrast  power

I could unify lcd's 'power' and 'enable' attributes into one (they already use
the 'standard' 0-4 power levels), like I stated in one of my previous
messages, but it anyways would be quite different from the current power
management structure.

> > Well, the power interface under drivers/ is available for framebuffer.
> > If it would handle it properly (the framebuffer drivers I've tried
> > don't, alas)
> Then they need to be fixed to do so.
Doh, you don't want me to fix the whole kernel in one patch, don't you? :-)

--
Greetings,
   Andrew

Re: two patches - request for comments

[Russell King]

On Wed, Jun 02, 2004 at 10:15:42AM -0700, Greg KH wrote:
> On Wed, Jun 02, 2004 at 01:00:36AM +0400, Andrew Zabolotny wrote:
> > 
> > In theory, if we would use the standard power interface, it could use the
> > different levels of power saving, e.g. 0 - controller and LCD on, 1,2 - LCD
> > off, controller on, 3,4 - both off.
> 
> Please use the standard power interface, and use the standard levels of
> power state.  That's why we _have_ this driver model in the first
> place...

It /doesn't/ make any sense to in this case.  We're talking effectively
about the LCD panel attributes, not a device as such.

IOW:
- LCD backlight brightness
- LCD backlight on/off
- LCD panel power on/off

Typically, the last item needs to be closely controlled in relation to
the LCD controller itself, and it just does not make sense to separate
the LCD panel power control from the controller itself in the device
model.  In fact, exposing the LCD panel power control independent of
the LCD controller state can lead to cases where you end up destroying
your LCD panel - there is a very defined sequence to power up/down
these things to avoid degredation.

However, the issue is that all of the three things can be handled
god-knows-which-way on any platform, even when they're using the
same LCD controllers.  So we need a way for platform/machine specific
code to provide the information so that the LCD controller can
correctly handle these settings on blank, etc.

Hope this provides some extra reasoning why using the device model
for these attributes is wrong.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
                 2.6 Serial core

Re: two patches - request for comments

[Greg KH]

On Wed, Jun 02, 2004 at 10:32:19PM +0100, Russell King wrote:
> On Wed, Jun 02, 2004 at 10:15:42AM -0700, Greg KH wrote:
> > On Wed, Jun 02, 2004 at 01:00:36AM +0400, Andrew Zabolotny wrote:
> > > 
> > > In theory, if we would use the standard power interface, it could use the
> > > different levels of power saving, e.g. 0 - controller and LCD on, 1,2 - LCD
> > > off, controller on, 3,4 - both off.
> > 
> > Please use the standard power interface, and use the standard levels of
> > power state.  That's why we _have_ this driver model in the first
> > place...
> 
> It /doesn't/ make any sense to in this case.  We're talking effectively
> about the LCD panel attributes, not a device as such.

<snip>

> Hope this provides some extra reasoning why using the device model
> for these attributes is wrong.

Ok, this makes more sense now, thanks for explaining it.

greg k-h