[patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Richard Hughes]

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

On Thu, 2007-05-31 at 00:24 -0700, Greg KH wrote: 
> On Wed, May 30, 2007 at 05:19:31PM +0100, Richard Purdie wrote:
> > On Wed, 2007-05-30 at 16:34 +0100, Richard Hughes wrote:
> > > I've talked with other kernel guys here at red hat and they don't think
> > > putting properties in the handle is a good idea.
> > > 
> > > I've cc'd Kay, Greg KH and a few other developers for comments.
> > > 
> > > To summarize, the LED device class creates a device with a ":" delimited
> > > handle, where properties that are not going to changed get included in
> > > the handle name for userspace to parse with sscanf. For
> > > example, /sys/class/leds/thinklight:blue:keyboard_backlight/brightness
> > > 
> > > I think this breaks the one-value-per sysfs file rule and sure makes it
> > > more difficult to extract information in HAL. The backlight class should
> > > have an attribute "color" and "function" so new properties can be added
> > > (or ones that make no sense removed) without breaking API, but the
> > > maintainer doesn't like that idea.
> > 
> > I will put my side (as the above maintainer) across. When deciding on
> > the naming for the LEDs in /sys/class/leds/ we had several choices.
> > Taking my handheld as an example, some choices were:
> > 
> > /sys/class/leds/led0/
> > /sys/class/leds/led1/
> 
> Yes.
> 
> > /sys/class/leds/spitz0/
> > /sys/class/leds/spitz1/
> 
> Yes.
> 
> > /sys/class/leds/spitz:amber/
> > /sys/class/leds/spitz:green/
> 
> No way!  Don't do that.
> 
> > The first contains no useful information, the second one is only
> > fractionally better, the third is actually quite useful. Faced with the
> > third name, a person can actually point to the right LED on the device.
> 
> So?  You need to read the attributes in the sysfs device directory to
> find out exactly what type of device this is, what it does, and all
> sorts of other information.
> 
> The first two examples above are correct.  They use a "bus id" type
> naming scheme, like ALL OTHER DEVICES IN THE KERNEL.  The only
> requirement is that it is unique.
> 
> Userspace programs, like HAL, can handle the fact that spitz0 is really
> a gree LED and it means that the device is charging.  That all comes
> from the individual sysfs files.
> 
> So please, don't imbend sysfs attributes into the bus id for the device,
> that's just wrong on so many levels...
> 
> > As far as the class is concerned, LED colour is a static property (it
> > could handle multi coloured through multiple LED devices).
> 
> Yes, and as a property, it needs to be an attribute, not the bus id.
> 
> Do you really want to see /sys/bus/usb/devices/usb:hub23 or
> /sys/bus/usb/devices/usb:nerf_rocket_launcher3 ?  No, just read the
> attributes to determine the type of the device, like all other devices.
> 
> Please, if this is the way that the code is currently working, change it
> now.

Kay,

Patch attached corrects all the brokenness with the led class (encoding
some attributes in the device handle).

 Documentation/leds-class.txt    |   13 --------
 drivers/leds/led-class.c        |   62 ++++++++++++++++++++++++++++++++++++++--
 drivers/leds/leds-ams-delta.c   |   18 +++++++----
 drivers/leds/leds-corgi.c       |    8 +++--
 drivers/leds/leds-h1940.c       |   12 +++++--
 drivers/leds/leds-locomo.c      |    8 +++--
 drivers/leds/leds-net48xx.c     |    3 +
 drivers/leds/leds-spitz.c       |    8 +++--
 drivers/leds/leds-tosa.c        |    8 +++--
 drivers/leds/leds-wrap.c        |    6 ++-
 drivers/macintosh/via-pmu-led.c |    1 
 drivers/misc/asus-laptop.c      |    3 +
 include/linux/leds.h            |    2 +
 13 files changed, 115 insertions(+), 37 deletions(-)

Signed-off-by: Richard Hughes <richard@hughsie.com>

Please review attached patch, thanks.


[-- Attachment #2: leds-add-colour-and-description.patch --]
[-- Type: text/x-patch, Size: 12141 bytes --]

diff --git a/Documentation/leds-class.txt b/Documentation/leds-class.txt
index 8c35c04..083222b 100644
--- a/Documentation/leds-class.txt
+++ b/Documentation/leds-class.txt
@@ -34,19 +34,6 @@ and the aim is to keep a small amount of code giving as much functionality
 as possible.  Please keep this in mind when suggesting enhancements.
 
 
-LED Device Naming
-=================
-
-Is currently of the form:
-
-"devicename:colour"
-
-There have been calls for LED properties such as colour to be exported as
-individual led class attributes. As a solution which doesn't incur as much
-overhead, I suggest these become part of the device name. The naming scheme
-above leaves scope for further attributes should they be needed.
-
-
 Known Issues
 ============
 
diff --git a/drivers/leds/led-class.c b/drivers/leds/led-class.c
index 3c17112..42b7355 100644
--- a/drivers/leds/led-class.c
+++ b/drivers/leds/led-class.c
@@ -36,6 +36,30 @@ static ssize_t led_brightness_show(struct class_device *dev, char *buf)
 	return ret;
 }
 
+static ssize_t led_colour_show(struct class_device *dev, char *buf)
+{
+	struct led_classdev *led_cdev = class_get_devdata(dev);
+	ssize_t ret = 0;
+
+	/* no lock needed for this */
+	sprintf(buf, "%s\n", led_cdev->colour);
+	ret = strlen(buf) + 1;
+
+	return ret;
+}
+
+static ssize_t led_description_show(struct class_device *dev, char *buf)
+{
+	struct led_classdev *led_cdev = class_get_devdata(dev);
+	ssize_t ret = 0;
+
+	/* no lock needed for this */
+	sprintf(buf, "%s\n", led_cdev->description);
+	ret = strlen(buf) + 1;
+
+	return ret;
+}
+
 static ssize_t led_brightness_store(struct class_device *dev,
 				const char *buf, size_t size)
 {
@@ -58,6 +82,8 @@ static ssize_t led_brightness_store(struct class_device *dev,
 
 static CLASS_DEVICE_ATTR(brightness, 0644, led_brightness_show,
 			led_brightness_store);
+static CLASS_DEVICE_ATTR(colour, 0644, led_colour_show, NULL);
+static CLASS_DEVICE_ATTR(description, 0644, led_description_show, NULL);
 #ifdef CONFIG_LEDS_TRIGGERS
 static CLASS_DEVICE_ATTR(trigger, 0644, led_trigger_show, led_trigger_store);
 #endif
@@ -106,6 +132,19 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 	if (rc)
 		goto err_out;
 
+	/* these are optional */
+	if (led_cdev->colour)
+		rc = class_device_create_file(led_cdev->class_dev,
+					      &class_device_attr_colour);
+		if (rc)
+			goto err_out_brightness;
+	if (led_cdev->description)
+		rc = class_device_create_file(led_cdev->class_dev,
+					      &class_device_attr_description);
+		if (rc)
+			goto err_out_colour;
+
+
 	/* add to the list of leds */
 	write_lock(&leds_list_lock);
 	list_add_tail(&led_cdev->node, &leds_list);
@@ -129,12 +168,23 @@ int led_classdev_register(struct device *parent, struct led_classdev *led_cdev)
 
 #ifdef CONFIG_LEDS_TRIGGERS
 err_out_led_list:
-	class_device_remove_file(led_cdev->class_dev,
-				&class_device_attr_brightness);
-	list_del(&led_cdev->node);
+	if (led_cdev->description)
+		class_device_remove_file(led_cdev->class_dev,
+					 &class_device_attr_description);
 #endif
+
+err_out_colour:
+	if (led_cdev->colour)
+		class_device_remove_file(led_cdev->class_dev,
+					 &class_device_attr_colour);
+err_out_brightness:
+	class_device_remove_file(led_cdev->class_dev,
+				 &class_device_attr_brightness);
 err_out:
 	class_device_unregister(led_cdev->class_dev);
+
+	list_del(&led_cdev->node);
+
 	return rc;
 }
 EXPORT_SYMBOL_GPL(led_classdev_register);
@@ -149,6 +199,12 @@ void led_classdev_unregister(struct led_classdev *led_cdev)
 {
 	class_device_remove_file(led_cdev->class_dev,
 				&class_device_attr_brightness);
+	if (led_cdev->colour)
+		class_device_remove_file(led_cdev->class_dev,
+					 &class_device_attr_colour);
+	if (led_cdev->description)
+		class_device_remove_file(led_cdev->class_dev,
+					 &class_device_attr_description);
 #ifdef CONFIG_LEDS_TRIGGERS
 	class_device_remove_file(led_cdev->class_dev,
 				&class_device_attr_trigger);
diff --git a/drivers/leds/leds-ams-delta.c b/drivers/leds/leds-ams-delta.c
index 599878c..21b1bef 100644
--- a/drivers/leds/leds-ams-delta.c
+++ b/drivers/leds/leds-ams-delta.c
@@ -37,42 +37,48 @@ static void ams_delta_led_set(struct led_classdev *led_cdev,
 static struct ams_delta_led ams_delta_leds[] = {
 	{
 		.cdev		= {
-			.name		= "ams-delta:camera",
+			.name		= "ams-delta-camera",
+			.description	= "camera",
 			.brightness_set = ams_delta_led_set,
 		},
 		.bitmask	= AMS_DELTA_LATCH1_LED_CAMERA,
 	},
 	{
 		.cdev		= {
-			.name		= "ams-delta:advert",
+			.name		= "ams-delta-advert",
+			.description	= "advert",
 			.brightness_set = ams_delta_led_set,
 		},
 		.bitmask	= AMS_DELTA_LATCH1_LED_ADVERT,
 	},
 	{
 		.cdev		= {
-			.name		= "ams-delta:email",
+			.name		= "ams-delta-email",
+			.description	= "email",
 			.brightness_set = ams_delta_led_set,
 		},
 		.bitmask	= AMS_DELTA_LATCH1_LED_EMAIL,
 	},
 	{
 		.cdev		= {
-			.name		= "ams-delta:handsfree",
+			.name		= "ams-delta-handsfree",
+			.description	= "handsfree",
 			.brightness_set = ams_delta_led_set,
 		},
 		.bitmask	= AMS_DELTA_LATCH1_LED_HANDSFREE,
 	},
 	{
 		.cdev		= {
-			.name		= "ams-delta:voicemail",
+			.name		= "ams-delta-voicemail",
+			.description	= "voicemail",
 			.brightness_set = ams_delta_led_set,
 		},
 		.bitmask	= AMS_DELTA_LATCH1_LED_VOICEMAIL,
 	},
 	{
 		.cdev		= {
-			.name		= "ams-delta:voice",
+			.name		= "ams-delta-voice",
+			.description	= "voice",
 			.brightness_set = ams_delta_led_set,
 		},
 		.bitmask	= AMS_DELTA_LATCH1_LED_VOICE,
diff --git a/drivers/leds/leds-corgi.c b/drivers/leds/leds-corgi.c
index cf1dcd7..eeb5a7c 100644
--- a/drivers/leds/leds-corgi.c
+++ b/drivers/leds/leds-corgi.c
@@ -38,13 +38,17 @@ static void corgiled_green_set(struct led_classdev *led_cdev, enum led_brightnes
 }
 
 static struct led_classdev corgi_amber_led = {
-	.name			= "corgi:amber",
+	.name			= "corgi_charge",
+	.colour			= "amber",
+	.description		= "charge",
 	.default_trigger	= "sharpsl-charge",
 	.brightness_set		= corgiled_amber_set,
 };
 
 static struct led_classdev corgi_green_led = {
-	.name			= "corgi:green",
+	.name			= "corgi_disk",
+	.colour			= "green",
+	.description		= "disk",
 	.default_trigger	= "nand-disk",
 	.brightness_set		= corgiled_green_set,
 };
diff --git a/drivers/leds/leds-h1940.c b/drivers/leds/leds-h1940.c
index 677c993..d134c79 100644
--- a/drivers/leds/leds-h1940.c
+++ b/drivers/leds/leds-h1940.c
@@ -44,7 +44,9 @@ void h1940_greenled_set(struct led_classdev *led_dev, enum led_brightness value)
 }
 
 static struct led_classdev h1940_greenled = {
-	.name			= "h1940:green",
+	.name			= "h1940_green",
+	.colour			= "green",
+	.description		= "charger",
 	.brightness_set		= h1940_greenled_set,
 	.default_trigger	= "h1940-charger",
 };
@@ -73,7 +75,9 @@ void h1940_redled_set(struct led_classdev *led_dev, enum led_brightness value)
 }
 
 static struct led_classdev h1940_redled = {
-	.name			= "h1940:red",
+	.name			= "h1940_charger",
+	.colour			= "red",
+	.description		= "charger",
 	.brightness_set		= h1940_redled_set,
 	.default_trigger	= "h1940-charger",
 };
@@ -96,7 +100,9 @@ void h1940_blueled_set(struct led_classdev *led_dev, enum led_brightness value)
 }
 
 static struct led_classdev h1940_blueled = {
-	.name			= "h1940:blue",
+	.name			= "h1940_bluetooth",
+	.colour			= "blue",
+	.description		= "bluetooth",
 	.brightness_set		= h1940_blueled_set,
 	.default_trigger	= "h1940-bluetooth",
 };
diff --git a/drivers/leds/leds-locomo.c b/drivers/leds/leds-locomo.c
index 6f2d449..0e297d4 100644
--- a/drivers/leds/leds-locomo.c
+++ b/drivers/leds/leds-locomo.c
@@ -43,13 +43,17 @@ static void locomoled_brightness_set1(struct led_classdev *led_cdev,
 }
 
 static struct led_classdev locomo_led0 = {
-	.name			= "locomo:amber",
+	.name			= "locomo_charge",
+	.colour			= "amber",
+	.description		= "charge",
 	.default_trigger	= "sharpsl-charge",
 	.brightness_set		= locomoled_brightness_set0,
 };
 
 static struct led_classdev locomo_led1 = {
-	.name			= "locomo:green",
+	.name			= "locomo_disk",
+	.colour			= "green",
+	.description		= "disk",
 	.default_trigger	= "nand-disk",
 	.brightness_set		= locomoled_brightness_set1,
 };
diff --git a/drivers/leds/leds-net48xx.c b/drivers/leds/leds-net48xx.c
index 45ba3d4..5e21553 100644
--- a/drivers/leds/leds-net48xx.c
+++ b/drivers/leds/leds-net48xx.c
@@ -31,7 +31,8 @@ static void net48xx_error_led_set(struct led_classdev *led_cdev,
 }
 
 static struct led_classdev net48xx_error_led = {
-	.name		= "net48xx:error",
+	.name		= "net48xx_error",
+	.description	= "error",
 	.brightness_set	= net48xx_error_led_set,
 };
 
diff --git a/drivers/leds/leds-spitz.c b/drivers/leds/leds-spitz.c
index 126d09c..b8340ab 100644
--- a/drivers/leds/leds-spitz.c
+++ b/drivers/leds/leds-spitz.c
@@ -38,13 +38,17 @@ static void spitzled_green_set(struct led_classdev *led_cdev, enum led_brightnes
 }
 
 static struct led_classdev spitz_amber_led = {
-	.name			= "spitz:amber",
+	.name			= "spitz_charge",
+	.colour			= "amber",
+	.description		= "charge",
 	.default_trigger	= "sharpsl-charge",
 	.brightness_set		= spitzled_amber_set,
 };
 
 static struct led_classdev spitz_green_led = {
-	.name			= "spitz:green",
+	.name			= "spitz_disk",
+	.colour			= "green",
+	.description		= "disk",
 	.default_trigger	= "ide-disk",
 	.brightness_set		= spitzled_green_set,
 };
diff --git a/drivers/leds/leds-tosa.c b/drivers/leds/leds-tosa.c
index fb2416a..6e2fb39 100644
--- a/drivers/leds/leds-tosa.c
+++ b/drivers/leds/leds-tosa.c
@@ -45,13 +45,17 @@ static void tosaled_green_set(struct led_classdev *led_cdev,
 }
 
 static struct led_classdev tosa_amber_led = {
-	.name			= "tosa:amber",
+	.name			= "tosa_charge",
+	.colour			= "amber",
+	.description		= "charge",
 	.default_trigger	= "sharpsl-charge",
 	.brightness_set		= tosaled_amber_set,
 };
 
 static struct led_classdev tosa_green_led = {
-	.name			= "tosa:green",
+	.name			= "tosa_disk",
+	.colour			= "green",
+	.description		= "disk",
 	.default_trigger	= "nand-disk",
 	.brightness_set		= tosaled_green_set,
 };
diff --git a/drivers/leds/leds-wrap.c b/drivers/leds/leds-wrap.c
index 27fb2d8..1daa13e 100644
--- a/drivers/leds/leds-wrap.c
+++ b/drivers/leds/leds-wrap.c
@@ -43,12 +43,14 @@ static void wrap_extra_led_set(struct led_classdev *led_cdev,
 }
 
 static struct led_classdev wrap_error_led = {
-	.name		= "wrap:error",
+	.name		= "wrap_error",
+	.description	= "error",
 	.brightness_set	= wrap_error_led_set,
 };
 
 static struct led_classdev wrap_extra_led = {
-	.name           = "wrap:extra",
+	.name           = "wrap_extra",
+	.description    = "extra",
 	.brightness_set = wrap_extra_led_set,
 };
 
diff --git a/drivers/macintosh/via-pmu-led.c b/drivers/macintosh/via-pmu-led.c
index 55ad956..b67a95a 100644
--- a/drivers/macintosh/via-pmu-led.c
+++ b/drivers/macintosh/via-pmu-led.c
@@ -73,6 +73,7 @@ static void pmu_led_set(struct led_classdev *led_cdev,
 
 static struct led_classdev pmu_led = {
 	.name = "pmu-front-led",
+	.description = "front-led",
 #ifdef CONFIG_ADB_PMU_LED_IDE
 	.default_trigger = "ide-disk",
 #endif
diff --git a/drivers/misc/asus-laptop.c b/drivers/misc/asus-laptop.c
index 4f9060a..d7f1175 100644
--- a/drivers/misc/asus-laptop.c
+++ b/drivers/misc/asus-laptop.c
@@ -235,7 +235,8 @@ static struct workqueue_struct *led_workqueue;
 	static int object##_led_wk;					\
 	static DECLARE_WORK(object##_led_work, object##_led_update);	\
 	static struct led_classdev object##_led = {			\
-		.name           = "asus:" ledname,			\
+		.name           = "asus_" ledname,			\
+		.description    = ledname,				\
 		.brightness_set = object##_led_set,			\
 	}
 
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 88afcef..c9cb394 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -41,6 +41,8 @@ struct led_classdev {
 	struct class_device	*class_dev;
 	struct list_head	 node;			/* LED Device list */
 	char			*default_trigger;	/* Trigger to use */
+	char			*colour;		/* Colour of the LED */
+	char			*description;		/* Description of purpose */
 
 #ifdef CONFIG_LEDS_TRIGGERS
 	/* Protects the trigger data below */

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Richard Purdie]

On Fri, 2007-06-01 at 16:04 +0100, Richard Hughes wrote:
> Patch attached corrects all the brokenness with the led class (encoding
> some attributes in the device handle).

For the benefit of LKML, there has been some discussion of this
elsewhere. My arguments do not particularly come across in Richard's
choice of quoting and I'm a little annoyed he's chosen to do things this
way, particularly before any further feedback from Greg/Kay but so be
it.

Greg said that the LEDs class should export any property data as a class
attribute rather than this just being available in the device name. I
added the patch in
http://git.o-hand.com/?p=linux-rpurdie-leds;a=shortlog;h=for-mm to deal
with this, without breaking the existing LED naming and documented API.
Greg said that the only requirement on bus id naming was that it needed
to be unique so this should therefore be compatible. HAL could then go
ahead and ignore the device name, all the attributes are there in the
fashion it needs which was the original complaint.

I accept this is basically out of my hands now. Greg/Kay have the
appropriate emails and if they'll let me know which approach they want
to take, I'll apply an appropriate patch. I'd suggest not applying this
patch directly as it introduces a race in the error path it alters.

Richard

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Richard Hughes]

On Fri, 2007-06-01 at 16:43 +0100, Richard Purdie wrote:
> On Fri, 2007-06-01 at 16:04 +0100, Richard Hughes wrote:
> > Patch attached corrects all the brokenness with the led class (encoding
> > some attributes in the device handle).
> 
> For the benefit of LKML, there has been some discussion of this
> elsewhere. My arguments do not particularly come across in Richard's
> choice of quoting and I'm a little annoyed he's chosen to do things this
> way, particularly before any further feedback from Greg/Kay but so be
> it.

To be honest, I didn't want to do this, but you would not accept my
argument - and you wanted to add _more_ properties into the device
handle.

> Greg said that the LEDs class should export any property data as a class
> attribute rather than this just being available in the device name. I
> added the patch in
> http://git.o-hand.com/?p=linux-rpurdie-leds;a=shortlog;h=for-mm to deal
> with this, without breaking the existing LED naming and documented API.

Yes, but as you'll notice with my patch, lots of the users that use the
led class do not use the handle:colour name, and so stripping off
second : parameter for the color attribute was just wrong. And ugly.

> Greg said that the only requirement on bus id naming was that it needed
> to be unique so this should therefore be compatible. HAL could then go
> ahead and ignore the device name, all the attributes are there in the
> fashion it needs which was the original complaint.

But when I was investigating adding led class support to thinkpad_acpi I
couldn't use the existing LED class, as some of the LED's did not have a
pre-defined colour, but did have a description. Again, adding support
into HAL was made more difficult until you proposed screenscaping the
led colour from the device name. This isn't very nice.

> I accept this is basically out of my hands now. Greg/Kay have the
> appropriate emails and if they'll let me know which approach they want
> to take, I'll apply an appropriate patch. I'd suggest not applying this
> patch directly as it introduces a race in the error path it alters.

Sure, I really wanted to convert the class to struct device (which I'm
more familiar with) but just tried to do minimal changes. What changes
need to be made to avoid a race? Thanks.

Richard.

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Richard Purdie]

On Fri, 2007-06-01 at 16:59 +0100, Richard Hughes wrote:
> On Fri, 2007-06-01 at 16:43 +0100, Richard Purdie wrote:
> > I accept this is basically out of my hands now. Greg/Kay have the
> > appropriate emails and if they'll let me know which approach they want
> > to take, I'll apply an appropriate patch. I'd suggest not applying this
> > patch directly as it introduces a race in the error path it alters.
> 
> Sure, I really wanted to convert the class to struct device (which I'm
> more familiar with) but just tried to do minimal changes. What changes
> need to be made to avoid a race? Thanks.

It has nothing to do with the struct device vs. class devices, you need
to think more carefully about the placement of the list_del().

My other concerns with this patch are that it exports incorrect
information on spitz (which I had warned about and I will get bug
reports about) and that "description" is not really a suitable name for
a sysfs attribute, "function" might give a better idea of what it
represents to developers.

I still question whether either colour or function properties are
actually particularly useful to userspace other than for naming purposes
which is why they were part of the device name.

Anyhow, I'm trying not to say too much more as it will just go in
circles. I'll await a reply from Greg.

Regards,

Richard

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Greg KH]

On Fri, Jun 01, 2007 at 05:23:24PM +0100, Richard Purdie wrote:
> On Fri, 2007-06-01 at 16:59 +0100, Richard Hughes wrote:
> > On Fri, 2007-06-01 at 16:43 +0100, Richard Purdie wrote:
> > > I accept this is basically out of my hands now. Greg/Kay have the
> > > appropriate emails and if they'll let me know which approach they want
> > > to take, I'll apply an appropriate patch. I'd suggest not applying this
> > > patch directly as it introduces a race in the error path it alters.
> > 
> > Sure, I really wanted to convert the class to struct device (which I'm
> > more familiar with) but just tried to do minimal changes. What changes
> > need to be made to avoid a race? Thanks.
> 
> It has nothing to do with the struct device vs. class devices, you need
> to think more carefully about the placement of the list_del().
> 
> My other concerns with this patch are that it exports incorrect
> information on spitz (which I had warned about and I will get bug
> reports about) and that "description" is not really a suitable name for
> a sysfs attribute, "function" might give a better idea of what it
> represents to developers.
> 
> I still question whether either colour or function properties are
> actually particularly useful to userspace other than for naming purposes
> which is why they were part of the device name.
> 
> Anyhow, I'm trying not to say too much more as it will just go in
> circles. I'll await a reply from Greg.

Hm, I thought I made my opinion pretty clear in the beginning.

Why not just do a simple:
	led01
	led02
	led03
	...

and so on?

And use the 'name' field to put the name of your device (disk,
bluetooth, etc.)  This is the way all other busses and devices work, and
I don't think that LEDs are anything more "special" than anything else
in the kernel, right?

So the original patch in this thread is close, but not quite there.

thanks,

greg k-h

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Richard Purdie]

On Fri, 2007-06-08 at 11:57 -0700, Greg KH wrote:
> On Fri, Jun 01, 2007 at 05:23:24PM +0100, Richard Purdie wrote:
> > I still question whether either colour or function properties are
> > actually particularly useful to userspace other than for naming purposes
> > which is why they were part of the device name.
> > 
> > Anyhow, I'm trying not to say too much more as it will just go in
> > circles. I'll await a reply from Greg.
> 
> Hm, I thought I made my opinion pretty clear in the beginning.
> 
> Why not just do a simple:
> 	led01
> 	led02
> 	led03
> 	...
> 
> and so on?

If we do this, my point is we're exporting something to userspace which
is totally devoid of information. 

sysfs is for userspace and "led01" means absolutely nothing to it. The
most you can summarise from it is it happens to have registered first
and its an LED. Since its in class/leds/ we can summarise the latter
without help from the prefix. I'd hate to think userspace cares about
the former.

> And use the 'name' field to put the name of your device (disk,
> bluetooth, etc.)  This is the way all other busses and devices work, and
> I don't think that LEDs are anything more "special" than anything else
> in the kernel, right?

Since the "busid" field means absolutely nothing, why not give it a
sensible id and save the overhead of a separate name?

If kernel policy is that we should have useless data in sysfs, so be it,
I just make sure that is on record and then break the defined LED class
API.


<serious mode>
Ok, so I make the point above with a sledge hammer. There are more
subtle issues here too. People are asking for a ton of extra attributes
for the LED class. We can have a name, a colour, an LED "function", a
location on the device and probably 101 other things.

As I understand it, sysfs was put there to pass information *the kernel
knows* to userspace. The kernel doesn't actually care about the location
of an LED or its colour. All the kernel cares is we have an LED and it
has a certain brightness.

Yes, we can teach the kernel all this extra info but it is simply to
pass to userspace. Why should my kernel be bloated with all that extra
information which it doesn't need itself?

My conclusion is that there should be something in userspace
supplementing this information from the kernel. Going back the the
problem at hand, HAL is already equipped to do this, all it needs is
some kind of unique ID so it can identify the LED. This would be the
busid.

So I do stand by what the LED class does as being sane. Yes, the class
defined a way of writing its busids which isn't like any other but there
is a good reason for it. We could make it machine readable to provide
hints to userspace and I never saw any reason why we shouldn't have
done. It was discussed on LKML and no objections were raised (it was in
fact requested). The choice of field separator is unfortunate since its
developed other meanings after it was used in the LED class, such is
life.

So there are various options:

1. We keep 'useful' busids and the LED class continues as is. We accept
classes can provide a busid naming policy if it makes sense.

2. We adopt free form busids and export a ton of attributes just useful
to userspace (bloating the kernel IMO).

3. As 2. but use 'useless' busids that mean nothing.

4. We adopt free form busids but let userspace fill in its own
information based on the busid (no policy on naming).

5. Other combinations thereof.


I'm very aware I'm isolated here and ultimately this is probably Greg's
decision which I will end up abiding by. If anyone else does have a
view, speak now ;-). I think there are some important issues here and
they do need clarification, one way or another.


Cheers,

Richard

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Greg KH]

On Sat, Jun 09, 2007 at 12:02:28AM +0100, Richard Purdie wrote:
> On Fri, 2007-06-08 at 11:57 -0700, Greg KH wrote:
> > On Fri, Jun 01, 2007 at 05:23:24PM +0100, Richard Purdie wrote:
> > > I still question whether either colour or function properties are
> > > actually particularly useful to userspace other than for naming purposes
> > > which is why they were part of the device name.
> > > 
> > > Anyhow, I'm trying not to say too much more as it will just go in
> > > circles. I'll await a reply from Greg.
> > 
> > Hm, I thought I made my opinion pretty clear in the beginning.
> > 
> > Why not just do a simple:
> > 	led01
> > 	led02
> > 	led03
> > 	...
> > 
> > and so on?
> 
> If we do this, my point is we're exporting something to userspace which
> is totally devoid of information. 

Just like all other subsystems?

No, the only information the device name is that it shows a UNIQUE name
at that point in time in the kernel.  Heck, we could just use random
numbers that are unique like some other people have suggested, it would
mean the exact same thing.

> sysfs is for userspace and "led01" means absolutely nothing to it.

Wrong, the attributes in that directory mean everything.  The name means
nothing.

> The most you can summarise from it is it happens to have registered
> first and its an LED.

Exactly.

> Since its in class/leds/ we can summarise the latter without help from
> the prefix. I'd hate to think userspace cares about the former.

Ok, then use a random number please.  Start with 00000000000001 and just
increment from there, or use the idr subsystem.

I was trying to be nice and at least give you a prefix that looked kind
of sane to people, but if you want to be difficult... :)

> 
> > And use the 'name' field to put the name of your device (disk,
> > bluetooth, etc.)  This is the way all other busses and devices work, and
> > I don't think that LEDs are anything more "special" than anything else
> > in the kernel, right?
> 
> Since the "busid" field means absolutely nothing, why not give it a
> sensible id and save the overhead of a separate name?

Because that is not how things work, sorry.

> If kernel policy is that we should have useless data in sysfs, so be it,
> I just make sure that is on record and then break the defined LED class
> API.

Again, the bus id needs to be unique, for that specific class/bus that's
it.  The attributes in the directory let you figure out what specifics
are for the device.

Look at the PCI and USB devices.  There is some information you can
glean from those names, but they are primary a unique identifier, you
need to look at the attributes to get the real information about your
device.

LED devices are no different.

> <serious mode>
> Ok, so I make the point above with a sledge hammer. There are more
> subtle issues here too. People are asking for a ton of extra attributes
> for the LED class. We can have a name, a colour, an LED "function", a
> location on the device and probably 101 other things.

Great, the more the merrier.  Seriously, I have no problem with that.

> As I understand it, sysfs was put there to pass information *the kernel
> knows* to userspace. The kernel doesn't actually care about the location
> of an LED or its colour. All the kernel cares is we have an LED and it
> has a certain brightness.

If you know the color and location already, why not export that
information?  Unless you can determine it from userspace some other way
already?

> Yes, we can teach the kernel all this extra info but it is simply to
> pass to userspace. Why should my kernel be bloated with all that extra
> information which it doesn't need itself?

If there's no other way for userspace to determine it, then put it in
the kernel.  Otherwise leave it for userspace to handle.

> My conclusion is that there should be something in userspace
> supplementing this information from the kernel. Going back the the
> problem at hand, HAL is already equipped to do this, all it needs is
> some kind of unique ID so it can identify the LED. This would be the
> busid.

Well, if that is possible.  Just put it in an attribute.

> So I do stand by what the LED class does as being sane. Yes, the class
> defined a way of writing its busids which isn't like any other but there
> is a good reason for it. We could make it machine readable to provide
> hints to userspace and I never saw any reason why we shouldn't have
> done. It was discussed on LKML and no objections were raised (it was in
> fact requested). The choice of field separator is unfortunate since its
> developed other meanings after it was used in the LED class, such is
> life.

Sorry, I missed it with the other stuff on lkml, but now I'm trying to
get this fixed.  Just use an attribute like the whole rest of the
kernel and a number for a bus id.

> So there are various options:
> 
> 1. We keep 'useful' busids and the LED class continues as is. We accept
> classes can provide a busid naming policy if it makes sense.

Nope.

> 2. We adopt free form busids and export a ton of attributes just useful
> to userspace (bloating the kernel IMO).

Yup, and there's no real bloat at all.

> 3. As 2. but use 'useless' busids that mean nothing.

bus ids mean almost nothing today, see above.

> 4. We adopt free form busids but let userspace fill in its own
> information based on the busid (no policy on naming).

No, use an attribute please.

> I'm very aware I'm isolated here and ultimately this is probably Greg's
> decision which I will end up abiding by. If anyone else does have a
> view, speak now ;-). I think there are some important issues here and
> they do need clarification, one way or another.

I know that LEDs are special and unique, just like everyone else, so
please work like everyone else does :)

thanks,

greg k-h

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Richard Purdie]

On Fri, 2007-06-08 at 16:46 -0700, Greg KH wrote: 
> On Sat, Jun 09, 2007 at 12:02:28AM +0100, Richard Purdie wrote:
> > On Fri, 2007-06-08 at 11:57 -0700, Greg KH wrote:
> > > On Fri, Jun 01, 2007 at 05:23:24PM +0100, Richard Purdie wrote:
> > > Why not just do a simple:
> > > 	led01
> > > 	led02
> > > 	led03
> > > 	...
> > > 
> > > and so on?
> > 
> > If we do this, my point is we're exporting something to userspace which
> > is totally devoid of information. 
> 
> Just like all other subsystems?
>
> No, the only information the device name is that it shows a UNIQUE name
> at that point in time in the kernel.  Heck, we could just use random
> numbers that are unique like some other people have suggested, it would
> mean the exact same thing.
>
> > Since its in class/leds/ we can summarise the latter without help from
> > the prefix. I'd hate to think userspace cares about the former.
> 
> Ok, then use a random number please.  Start with 00000000000001 and just
> increment from there, or use the idr subsystem.
> 
> I was trying to be nice and at least give you a prefix that looked kind
> of sane to people, but if you want to be difficult... :)

This whole mess is because the LED class tried to be nice with sane
looking ids :/.

> > > And use the 'name' field to put the name of your device (disk,
> > > bluetooth, etc.)  This is the way all other busses and devices work, and
> > > I don't think that LEDs are anything more "special" than anything else
> > > in the kernel, right?
> > 
> > Since the "busid" field means absolutely nothing, why not give it a
> > sensible id and save the overhead of a separate name?
> 
> Because that is not how things work, sorry.

Most of your argument seems to read that this shouldn't be done because
nobody else does it which isn't a particularly good one. Everyone else
is jumping off a bridge, I should as well? You then point out that PCI
and USB busids do have some meaning and a quick glance at sysfs shows
others too.

> > If kernel policy is that we should have useless data in sysfs, so be it,
> > I just make sure that is on record and then break the defined LED class
> > API.
> 
> Again, the bus id needs to be unique, for that specific class/bus that's
> it.

and as I've said several times, the LED scheme *is* unique meeting your
criteria ;-).

>   The attributes in the directory let you figure out what specifics
> are for the device.
> 
> Look at the PCI and USB devices.  There is some information you can
> glean from those names, but they are primary a unique identifier, you
> need to look at the attributes to get the real information about your
> device.

So the PCI and USB subsystems need adjusting to use random numbers since
they're also breaking the rules and might have meaningful information in
their busids? Please fix them too ;-).

> > <serious mode>
> > Ok, so I make the point above with a sledge hammer. There are more
> > subtle issues here too. People are asking for a ton of extra attributes
> > for the LED class. We can have a name, a colour, an LED "function", a
> > location on the device and probably 101 other things.
> 
> Great, the more the merrier.  Seriously, I have no problem with that.

Where do you draw the line though?

> > As I understand it, sysfs was put there to pass information *the kernel
> > knows* to userspace. The kernel doesn't actually care about the location
> > of an LED or its colour. All the kernel cares is we have an LED and it
> > has a certain brightness.
> 
> If you know the color and location already, why not export that
> information?  Unless you can determine it from userspace some other way
> already?

The kernel doesn't know this and it doesn't care about this.

We could teach the kernel. Alternatively we could teach userspace.

> > Yes, we can teach the kernel all this extra info but it is simply to
> > pass to userspace. Why should my kernel be bloated with all that extra
> > information which it doesn't need itself?
> 
> If there's no other way for userspace to determine it, then put it in
> the kernel.  Otherwise leave it for userspace to handle.

Ok. The LED class can provide a name attribute which uniquely identifies
each LED. Userspace can then store and look up all the information it
wants against that "name".

So by this argument, colour, function and all these other attributes
should be left for userspace/HAL to deal with.

> > I'm very aware I'm isolated here and ultimately this is probably Greg's
> > decision which I will end up abiding by. If anyone else does have a
> > view, speak now ;-). I think there are some important issues here and
> > they do need clarification, one way or another.
> 
> I know that LEDs are special and unique, just like everyone else, so
> please work like everyone else does :)

I've never claimed that it was special and unique. Just because it does
something differently, that doesn't mean its wrong either. 

One last try to demonstrate this is grossly inefficient (and this kind
of inefficiency is one reason I'm beginning to dislike sysfs). On my
handheld, you can currently script something to light a specific LED
with something like:

echo "1" > /sys/class/leds/spitz\:green/brightness

With the busid changes, this becomes:

for busid in `ls /sys/class/leds`
do
  name=`cat /sys/class/leds/$busid/name`
  if [ "$name" = "spitz_green" ]; then
    break
  fi
done
echo "1" > /sys/class/leds/$busid/brightness

Even after accounting for my lack of l33t shell skills, the latter will
be much more complex. For what gain? It doesn't matter whether you do
this through shell or any other language. Even if you know the name of
the device you want to talk to you have to convert to some meaningless
busid first which is inefficient. Things like HAL are going to be forced
to read things after every boot. This has wider implications than the
LEDs which are just a tiny consideration in the grand sysfs scheme. If
we can make sysfs more efficient, wouldn't that be a good idea?


Anyhow, time to stop pretending I have any choice in this ;-). I will
have the LED class use random numbers for busids and add a name
attribute unless anyone else voices an opinion.

My current view is that notions of colour, function, location etc.
shouldn't be in the class since userspace can handle it in userspace and
these don't mean anything to the kernel.

Cheers,

Richard

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Pavel Machek]

Hi!

> > My other concerns with this patch are that it exports incorrect
> > information on spitz (which I had warned about and I will get bug
> > reports about) and that "description" is not really a suitable name for
> > a sysfs attribute, "function" might give a better idea of what it
> > represents to developers.
> > 
> > I still question whether either colour or function properties are
> > actually particularly useful to userspace other than for naming purposes
> > which is why they were part of the device name.
> > 
> > Anyhow, I'm trying not to say too much more as it will just go in
> > circles. I'll await a reply from Greg.
> 
> Hm, I thought I made my opinion pretty clear in the beginning.
> 
> Why not just do a simple:
> 	led01
> 	led02
> 	led03
> 	...
> 
> and so on?
> 
> And use the 'name' field to put the name of your device (disk,
> bluetooth, etc.)  This is the way all other busses and devices work, and
> I don't think that LEDs are anything more "special" than anything else
> in the kernel, right?

Can we keep the original naming? spitz:disk is as unique as led02, and
it is _way_ easier to use.

Come on, I want to use the led subsystem from the scripts... 
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Richard Hughes]

On Sun, 2007-06-10 at 12:11 +0200, Pavel Machek wrote:
> Can we keep the original naming? spitz:disk is as unique as led02, and
> it is _way_ easier to use.
> Come on, I want to use the led subsystem from the scripts... 

I don't see a problem with spitz_disk, which is just as easy to use in
scripts.

Richard.

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Henrique de Moraes Holschuh]

Hi Richard!

On Sun, 10 Jun 2007, Richard Hughes wrote:

> On Sun, 2007-06-10 at 12:11 +0200, Pavel Machek wrote:
> > Can we keep the original naming? spitz:disk is as unique as led02, and
> > it is _way_ easier to use.
> > Come on, I want to use the led subsystem from the scripts... 
> 
> I don't see a problem with spitz_disk, which is just as easy to use in
> scripts.

It is the old problem with mV in battery_level_mV.  How do you know for sure
what is the designator, and what is the unit (or in this case, color)?  You
could take the last _[^_]+, but what if there is no unit/color? etc.

There is a BIIIIG thread about it (or more than one, actually) in the
archives, and I don't recall a consensus being reached.

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

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Greg KH]

On Sat, Jun 09, 2007 at 10:25:16AM +0100, Richard Purdie wrote:
> Anyhow, time to stop pretending I have any choice in this ;-). I will
> have the LED class use random numbers for busids and add a name
> attribute unless anyone else voices an opinion.

Thank you, I really appreciate it, and so does the people who write the
tools that parse sysfs (like HAL.)

greg k-h

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Richard Purdie]

On Sun, 2007-06-24 at 22:06 -0700, Greg KH wrote:
> On Sat, Jun 09, 2007 at 10:25:16AM +0100, Richard Purdie wrote:
> > Anyhow, time to stop pretending I have any choice in this ;-). I will
> > have the LED class use random numbers for busids and add a name
> > attribute unless anyone else voices an opinion.
> 
> Thank you, I really appreciate it, and so does the people who write the
> tools that parse sysfs (like HAL.)

There were some other opinions voiced including one from the person who
started this discussion.

So no, the people who write the tools that parse sysfs (like HAL.) don't
appreciate this.

People who write tools that parse sysfs like shell scripts don't
appreciate it either, as I illustrated.

You've yet to give any technical reason why we can't have meaningful
busids rather than random numbers. Your entire argument seems to be that
its wrong because its a bit different and nobody else does it...

Richard

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Richard Hughes]

On Tue, 2007-06-26 at 11:02 +0100, Richard Purdie wrote:
> There were some other opinions voiced including one from the person
> who started this discussion.
> 
> So no, the people who write the tools that parse sysfs (like HAL.)
> don't appreciate this.
> 
> People who write tools that parse sysfs like shell scripts don't
> appreciate it either, as I illustrated.

>From a hal point of view, we don't care if the device name is 'led01' or
'light_to_dance_the_fandango' and from a shell point of view it's
probably best for the latter. I think the point Greg tried to make is
that it shouldn't matter, and HAL shouldn't export (nor parse) the
device name as anything sensible.

> You've yet to give any technical reason why we can't have meaningful
> busids rather than random numbers. Your entire argument seems to be
> that its wrong because its a bit different and nobody else does it...

If it's a trivial name then I think led_thinklight0 is perfectly okay, I
think Kay was talking more about the attribute vs. name-in-device
encoding.

Richard.

Re: [patch] Move led attributes out of device name and into sysfs attributes, was Re: LED devices

[Kay Sievers]

On Tue, 2007-06-26 at 11:46 +0100, Richard Hughes wrote:
> On Tue, 2007-06-26 at 11:02 +0100, Richard Purdie wrote:
> > There were some other opinions voiced including one from the person
> > who started this discussion.
> > 
> > So no, the people who write the tools that parse sysfs (like HAL.)
> > don't appreciate this.
> > 
> > People who write tools that parse sysfs like shell scripts don't
> > appreciate it either, as I illustrated.
> 
> >From a hal point of view, we don't care if the device name is 'led01' or
> 'light_to_dance_the_fandango' and from a shell point of view it's
> probably best for the latter. I think the point Greg tried to make is
> that it shouldn't matter, and HAL shouldn't export (nor parse) the
> device name as anything sensible.
> 
> > You've yet to give any technical reason why we can't have meaningful
> > busids rather than random numbers. Your entire argument seems to be
> > that its wrong because its a bit different and nobody else does it...
> 
> If it's a trivial name then I think led_thinklight0 is perfectly okay, I
> think Kay was talking more about the attribute vs. name-in-device
> encoding.

I see no problem to use the function name and add an enumeration number
to that name to be able to handle multiple instances with the same
function name. Like pointed out in earlier mail:
  http://lists.freedesktop.org/archives/hal/2007-May/008552.html

Thanks,
Kay