[PATCH V6 1/2] introduce ALS sysfs class

[PATCH V6 1/2] introduce ALS sysfs class

[Zhang Rui]

Introduce ALS sysfs class.

ALS sysfs class provides a standard sysfs interface for
Ambient Light Sensor devices.

please read Documentation/ABI/testing/sysfs-class-als for
detailed sysfs designs.

CC: Pavel Machek <pavel@ucw.cz>
Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Zhang Rui <rui.zhang@intel.com>
---
 Documentation/ABI/testing/sysfs-class-als |   23 +++
 MAINTAINERS                               |    6 
 drivers/Kconfig                           |    2 
 drivers/Makefile                          |    1 
 drivers/als/Kconfig                       |   10 +
 drivers/als/Makefile                      |    5 
 drivers/als/als_sys.c                     |  200 ++++++++++++++++++++++++++++++
 include/linux/als_sys.h                   |   51 +++++++
 8 files changed, 298 insertions(+)

Index: linux-2.6/drivers/Kconfig
===================================================================
--- linux-2.6.orig/drivers/Kconfig
+++ linux-2.6/drivers/Kconfig
@@ -62,6 +62,8 @@ source "drivers/power/Kconfig"
 
 source "drivers/hwmon/Kconfig"
 
+source "drivers/als/Kconfig"
+
 source "drivers/thermal/Kconfig"
 
 source "drivers/watchdog/Kconfig"
Index: linux-2.6/drivers/Makefile
===================================================================
--- linux-2.6.orig/drivers/Makefile
+++ linux-2.6/drivers/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_PPS)		+= pps/
 obj-$(CONFIG_W1)		+= w1/
 obj-$(CONFIG_POWER_SUPPLY)	+= power/
 obj-$(CONFIG_HWMON)		+= hwmon/
+obj-$(CONFIG_ALS)		+= als/
 obj-$(CONFIG_THERMAL)		+= thermal/
 obj-$(CONFIG_WATCHDOG)		+= watchdog/
 obj-$(CONFIG_PHONE)		+= telephony/
Index: linux-2.6/drivers/als/Kconfig
===================================================================
--- /dev/null
+++ linux-2.6/drivers/als/Kconfig
@@ -0,0 +1,10 @@
+#
+# Ambient Light Sensor sysfs device configuration
+#
+
+menuconfig ALS
+	tristate "Ambient Light Sensor sysfs device"
+	help
+	  This framework provides a generic sysfs interface for
+	  Ambient Light Sensor devices.
+	  If you want this support, you should say Y or M here.
Index: linux-2.6/drivers/als/Makefile
===================================================================
--- /dev/null
+++ linux-2.6/drivers/als/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for Ambient Light Sensor device drivers.
+#
+
+obj-$(CONFIG_ALS)		+= als_sys.o
Index: linux-2.6/drivers/als/als_sys.c
===================================================================
--- /dev/null
+++ linux-2.6/drivers/als/als_sys.c
@@ -0,0 +1,200 @@
+/*
+ *  als_sys.c - Ambient Light Sensor Sysfs support.
+ *
+ *  Copyright (C) 2009 Intel Corp
+ *  Copyright (C) 2009 Zhang Rui <rui.zhang@intel.com>
+ *
+ *  ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/als_sys.h>
+
+MODULE_AUTHOR("Zhang Rui");
+MODULE_DESCRIPTION("Ambient Light Sensor sysfs support");
+MODULE_LICENSE("GPL");
+
+static int als_get_adjustment(struct als_device *, int, int *);
+
+/* sys I/F for Ambient Light Sensor */
+
+#define to_als_device(dev) container_of(dev, struct als_device, device)
+
+static ssize_t
+illuminance_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct als_device *als = to_als_device(dev);
+	int illuminance;
+	int result;
+
+	result = als->ops->get_illuminance(als, &illuminance);
+	if (result)
+		return result;
+
+	if (!illuminance)
+		return sprintf(buf, "Illuminance below the supported range\n");
+	else if (illuminance == -1)
+		return sprintf(buf, "Illuminance above the supported range\n");
+	else if (illuminance < -1)
+		return -ERANGE;
+	else
+		return sprintf(buf, "%d\n", illuminance);
+}
+
+static ssize_t
+adjustment_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct als_device *als = to_als_device(dev);
+	int illuminance, adjustment;
+	int result;
+
+	result = als->ops->get_illuminance(als, &illuminance);
+	if (result)
+		return result;
+
+	if (illuminance < 0 && illuminance != -1)
+		return sprintf(buf, "Current illuminance invalid\n");
+
+	result = als_get_adjustment(als, illuminance, &adjustment);
+	if (result)
+		return result;
+
+	return sprintf(buf, "%d%%\n", adjustment);
+}
+
+static struct device_attribute als_attrs[] = {
+	__ATTR(illuminance, 0444, illuminance_show, NULL),
+	__ATTR(display_adjustment, 0444, adjustment_show, NULL),
+	__ATTR_NULL,
+};
+
+static void als_release(struct device *dev)
+{
+	struct als_device *als = to_als_device(dev);
+
+	kfree(als);
+}
+
+static struct class als_class = {
+	.name = "als",
+	.dev_release = als_release,
+	.dev_attrs = als_attrs,
+};
+
+static int als_get_adjustment(struct als_device *als, int illuminance,
+			      int *adjustment)
+{
+	int lux_high, lux_low, adj_high, adj_low;
+	int i;
+
+	if (!als->mappings)
+		return -EINVAL;
+
+	if (illuminance == -1
+	    || illuminance > als->mappings[als->count - 1].illuminance)
+		illuminance = als->mappings[als->count - 1].illuminance;
+	else if (illuminance < als->mappings[0].illuminance)
+		illuminance = als->mappings[0].illuminance;
+
+	for (i = 0; i < als->count; i++) {
+		if (illuminance == als->mappings[i].illuminance) {
+			*adjustment = als->mappings[i].adjustment;
+			return 0;
+		}
+
+		if (illuminance > als->mappings[i].illuminance)
+			continue;
+
+		lux_high = als->mappings[i].illuminance;
+		lux_low = als->mappings[i - 1].illuminance;
+		adj_high = als->mappings[i].adjustment;
+		adj_low = als->mappings[i - 1].adjustment;
+
+		*adjustment =
+		    ((adj_high - adj_low) * (illuminance - lux_low)) /
+			(lux_high - lux_low) + adj_low;
+		return 0;
+	}
+	return -EINVAL;
+}
+
+/**
+ * als_device_register - register a new Ambient Light Sensor class device
+ * @ops:	standard ALS devices callbacks.
+ * @devdata:	device private data.
+ */
+struct als_device *als_device_register(struct als_device_ops *ops,
+				       char *name, void *devdata)
+{
+	struct als_device *als;
+	int result = -ENOMEM;
+
+	if (!ops || !ops->get_illuminance || !name)
+		return ERR_PTR(-EINVAL);
+
+	als = kzalloc(sizeof(struct als_device), GFP_KERNEL);
+	if (!als)
+		return ERR_PTR(-ENOMEM);
+
+	als->ops = ops;
+	als->device.class = &als_class;
+	dev_set_name(&als->device, name);
+	dev_set_drvdata(&als->device, devdata);
+	result = device_register(&als->device);
+	if (result)
+		goto err;
+
+	if (ops->update_mappings) {
+		result = ops->update_mappings(als);
+		if (result) {
+			device_unregister(&als->device);
+			goto err;
+		}
+	}
+	return als;
+
+err:
+	kfree(als);
+	return ERR_PTR(result);
+}
+EXPORT_SYMBOL(als_device_register);
+
+/**
+ * als_device_unregister - removes the registered ALS device
+ * @als:	the ALS device to remove.
+ */
+void als_device_unregister(struct als_device *als)
+{
+	device_unregister(&als->device);
+}
+EXPORT_SYMBOL(als_device_unregister);
+
+static int __init als_init(void)
+{
+	return class_register(&als_class);
+}
+
+static void __exit als_exit(void)
+{
+	class_unregister(&als_class);
+}
+
+subsys_initcall(als_init);
+module_exit(als_exit);
Index: linux-2.6/include/linux/als_sys.h
===================================================================
--- /dev/null
+++ linux-2.6/include/linux/als_sys.h
@@ -0,0 +1,51 @@
+/*
+ *  als.h
+ *
+ *  Copyright (C) 2009  Intel Corp
+ *  Copyright (C) 2009  Zhang Rui <rui.zhang@intel.com>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; version 2 of the License.
+ *
+ *  This program is distributed in the hope that it will be useful, but
+ *  WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ *  General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#ifndef __ALS_SYS_H__
+#define __ALS_SYS_H__
+
+#include <linux/device.h>
+
+struct als_device;
+
+struct als_device_ops {
+	int (*get_illuminance) (struct als_device *, int *);
+	int (*update_mappings) (struct als_device *);
+};
+
+struct als_mapping {
+	int illuminance;
+	int adjustment;
+};
+
+struct als_device {
+	struct device device;
+	struct als_device_ops *ops;
+	int count;
+	struct als_mapping *mappings;
+};
+
+struct als_device *als_device_register(struct als_device_ops *, char *, void *);
+void als_device_unregister(struct als_device *);
+
+#endif /* __ALS_SYS_H__ */
Index: linux-2.6/Documentation/ABI/testing/sysfs-class-als
===================================================================
--- /dev/null
+++ linux-2.6/Documentation/ABI/testing/sysfs-class-als
@@ -0,0 +1,23 @@
+What:		/sys/class/als/.../illuminance
+Date:		Aug. 2009
+KernelVersion:	2.6.32
+Contact:	Zhang Rui <rui.zhang@intel.com>
+Description:	Current Ambient Light Illuminance reported by
+		native ALS driver
+		Unit: lux (lumens per square meter)
+		RO
+
+What:		/sys/class/als/.../display_adjustment
+Date:		Aug. 2009
+KernelVersion:	2.6.32
+Contact:	Zhang Rui <rui.zhang@intel.com>
+Description:	a relative percentages in order to simplify the means
+		by which these adjustments are applied in lieu of
+		changes to the user’s display brightness preference.
+		A value of 100% is used to indicate no display
+		brightness adjustment.
+		Values less than 100% indicate a negative adjustment
+		(dimming); values greater than 100% indicate a positive
+		adjustment (brightening).
+		RO
+
Index: linux-2.6/MAINTAINERS
===================================================================
--- linux-2.6.orig/MAINTAINERS
+++ linux-2.6/MAINTAINERS
@@ -399,6 +399,12 @@ S:	Maintained for 2.4; PCI support for 2
 L:	linux-alpha@vger.kernel.org
 F:	arch/alpha/
 
+AMBIENT LIGHT SENSOR
+M:	Zhang Rui <rui.zhang@intel.com>
+S:	Supported
+F:	include/linux/als_sys.h
+F:	drivers/als/
+
 AMD GEODE CS5536 USB DEVICE CONTROLLER DRIVER
 M:	Thomas Dahlmann <dahlmann.thomas@arcor.de>
 L:	linux-geode@lists.infradead.org (moderated for non-subscribers)

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Pavel Machek]

Hi!

> Introduce ALS sysfs class.
> 
> ALS sysfs class provides a standard sysfs interface for
> Ambient Light Sensor devices.
> 
> please read Documentation/ABI/testing/sysfs-class-als for
> detailed sysfs designs.

Thanks for fixing the interface!

> +static ssize_t
> +illuminance_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct als_device *als = to_als_device(dev);
> +	int illuminance;
> +	int result;
> +
> +	result = als->ops->get_illuminance(als, &illuminance);
> +	if (result)
> +		return result;
> +
> +	if (!illuminance)
> +		return sprintf(buf, "Illuminance below the supported range\n");
> +	else if (illuminance == -1)
> +		return sprintf(buf, "Illuminance above the supported range\n");
> +	else if (illuminance < -1)
> +		return -ERANGE;
> +	else
> +		return sprintf(buf, "%d\n", illuminance);
> +}

that's nor particulary clean. One value per file and all that. Could
we simply return errnos in _all_ the error cases? (Docs would suggest
this contains integer so string is definitely unexpected).


> +static ssize_t
> +adjustment_show(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct als_device *als = to_als_device(dev);
> +	int illuminance, adjustment;
> +	int result;
> +
> +	result = als->ops->get_illuminance(als, &illuminance);
> +	if (result)
> +		return result;
> +
> +	if (illuminance < 0 && illuminance != -1)
> +		return sprintf(buf, "Current illuminance invalid\n");
> +
> +	result = als_get_adjustment(als, illuminance, &adjustment);
> +	if (result)
> +		return result;
> +
> +	return sprintf(buf, "%d%%\n", adjustment);
> +}

You should not return strings... and in this case it is not clear how
the code works. You fill the buf, but then return...? Better stick to
integers.
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Zhang Rui]

On Tue, 2009-09-01 at 16:11 +0800, Pavel Machek wrote:
> Hi!
> 
> > Introduce ALS sysfs class.
> > 
> > ALS sysfs class provides a standard sysfs interface for
> > Ambient Light Sensor devices.
> > 
> > please read Documentation/ABI/testing/sysfs-class-als for
> > detailed sysfs designs.
> 
> Thanks for fixing the interface!
> 
> > +static ssize_t
> > +illuminance_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct als_device *als = to_als_device(dev);
> > +	int illuminance;
> > +	int result;
> > +
> > +	result = als->ops->get_illuminance(als, &illuminance);
> > +	if (result)
> > +		return result;
> > +
> > +	if (!illuminance)
> > +		return sprintf(buf, "Illuminance below the supported range\n");
> > +	else if (illuminance == -1)
> > +		return sprintf(buf, "Illuminance above the supported range\n");
> > +	else if (illuminance < -1)
> > +		return -ERANGE;
> > +	else
> > +		return sprintf(buf, "%d\n", illuminance);
> > +}
> 
> that's nor particulary clean. One value per file and all that. Could
> we simply return errnos in _all_ the error cases? (Docs would suggest
> this contains integer so string is definitely unexpected).
> 
IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
illuminance is beyond the device support range, while the device is
still working normally.
what about exporting these values (0 and -1) to user space directly?

> 
> > +static ssize_t
> > +adjustment_show(struct device *dev, struct device_attribute *attr, char *buf)
> > +{
> > +	struct als_device *als = to_als_device(dev);
> > +	int illuminance, adjustment;
> > +	int result;
> > +
> > +	result = als->ops->get_illuminance(als, &illuminance);
> > +	if (result)
> > +		return result;
> > +
> > +	if (illuminance < 0 && illuminance != -1)
> > +		return sprintf(buf, "Current illuminance invalid\n");
> > +
> > +	result = als_get_adjustment(als, illuminance, &adjustment);
> > +	if (result)
> > +		return result;
> > +
> > +	return sprintf(buf, "%d%%\n", adjustment);
> > +}
> 
> You should not return strings... and in this case it is not clear how
> the code works. You fill the buf, but then return...? 

As the adjustment is a percentage value, I added a '%' postfix so that
users won't be confused.
yes, it's okay to just export the integer, e.g. "100" instead of "100%".

thanks,
rui

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Pavel Machek]

On Tue 2009-09-01 16:30:44, Zhang Rui wrote:
> On Tue, 2009-09-01 at 16:11 +0800, Pavel Machek wrote:
> > Hi!
> > 
> > > Introduce ALS sysfs class.
> > > 
> > > ALS sysfs class provides a standard sysfs interface for
> > > Ambient Light Sensor devices.
> > > 
> > > please read Documentation/ABI/testing/sysfs-class-als for
> > > detailed sysfs designs.
> > 
> > Thanks for fixing the interface!
> > 
> > > +static ssize_t
> > > +illuminance_show(struct device *dev, struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct als_device *als = to_als_device(dev);
> > > +	int illuminance;
> > > +	int result;
> > > +
> > > +	result = als->ops->get_illuminance(als, &illuminance);
> > > +	if (result)
> > > +		return result;
> > > +
> > > +	if (!illuminance)
> > > +		return sprintf(buf, "Illuminance below the supported range\n");
> > > +	else if (illuminance == -1)
> > > +		return sprintf(buf, "Illuminance above the supported range\n");
> > > +	else if (illuminance < -1)
> > > +		return -ERANGE;
> > > +	else
> > > +		return sprintf(buf, "%d\n", illuminance);
> > > +}
> > 
> > that's nor particulary clean. One value per file and all that. Could
> > we simply return errnos in _all_ the error cases? (Docs would suggest
> > this contains integer so string is definitely unexpected).
> > 
> IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> illuminance is beyond the device support range, while the device is
> still working normally.
> what about exporting these values (0 and -1) to user space directly?

Returning 0 for "below" range and 99999999 for "above" range would be
nice, yes. 

> > 
> > > +static ssize_t
> > > +adjustment_show(struct device *dev, struct device_attribute *attr, char *buf)
> > > +{
> > > +	struct als_device *als = to_als_device(dev);
> > > +	int illuminance, adjustment;
> > > +	int result;
> > > +
> > > +	result = als->ops->get_illuminance(als, &illuminance);
> > > +	if (result)
> > > +		return result;
> > > +
> > > +	if (illuminance < 0 && illuminance != -1)
> > > +		return sprintf(buf, "Current illuminance invalid\n");
> > > +
> > > +	result = als_get_adjustment(als, illuminance, &adjustment);
> > > +	if (result)
> > > +		return result;
> > > +
> > > +	return sprintf(buf, "%d%%\n", adjustment);
> > > +}
> > 
> > You should not return strings... and in this case it is not clear how
> > the code works. You fill the buf, but then return...? 
> 
> As the adjustment is a percentage value, I added a '%' postfix so that
> users won't be confused.
> yes, it's okay to just export the integer, e.g. "100" instead of "100%".

The "%" postfix is okay, but returning "Current illuminance invalid"
is ugly. Better return -EINVAL or -EIO or something.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Pavel Machek]

> > > IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> > > illuminance is beyond the device support range, while the device is
> > > still working normally.
> > > what about exporting these values (0 and -1) to user space directly?
> > 
> > Returning 0 for "below" range and 99999999 for "above" range would be
> > nice, yes. 
> 
> Why not 0 and "all ones" or 0 and -1.
> 
> Is there anything wrong with -1 in particular?

Normal people expect -1 to be less than 123, and output is in ascii. If
you make it ((unsigned) ~0) I guess that becomes acceptable.
								Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Rafael J. Wysocki]

On Tuesday 01 September 2009, Pavel Machek wrote:
> On Tue 2009-09-01 16:30:44, Zhang Rui wrote:
> > On Tue, 2009-09-01 at 16:11 +0800, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > Introduce ALS sysfs class.
> > > > 
> > > > ALS sysfs class provides a standard sysfs interface for
> > > > Ambient Light Sensor devices.
> > > > 
> > > > please read Documentation/ABI/testing/sysfs-class-als for
> > > > detailed sysfs designs.
> > > 
> > > Thanks for fixing the interface!
> > > 
> > > > +static ssize_t
> > > > +illuminance_show(struct device *dev, struct device_attribute *attr, char *buf)
> > > > +{
> > > > +	struct als_device *als = to_als_device(dev);
> > > > +	int illuminance;
> > > > +	int result;
> > > > +
> > > > +	result = als->ops->get_illuminance(als, &illuminance);
> > > > +	if (result)
> > > > +		return result;
> > > > +
> > > > +	if (!illuminance)
> > > > +		return sprintf(buf, "Illuminance below the supported range\n");
> > > > +	else if (illuminance == -1)
> > > > +		return sprintf(buf, "Illuminance above the supported range\n");
> > > > +	else if (illuminance < -1)
> > > > +		return -ERANGE;
> > > > +	else
> > > > +		return sprintf(buf, "%d\n", illuminance);
> > > > +}
> > > 
> > > that's nor particulary clean. One value per file and all that. Could
> > > we simply return errnos in _all_ the error cases? (Docs would suggest
> > > this contains integer so string is definitely unexpected).
> > > 
> > IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> > illuminance is beyond the device support range, while the device is
> > still working normally.
> > what about exporting these values (0 and -1) to user space directly?
> 
> Returning 0 for "below" range and 99999999 for "above" range would be
> nice, yes. 

Why not 0 and "all ones" or 0 and -1.

Is there anything wrong with -1 in particular?

Best,
Rafael

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Alan Jenkins]

On 9/1/09, Zhang Rui <rui.zhang@intel.com> wrote:
>
> Introduce ALS sysfs class.
>
> ALS sysfs class provides a standard sysfs interface for
> Ambient Light Sensor devices.
>
> please read Documentation/ABI/testing/sysfs-class-als for
> detailed sysfs designs.
>
> CC: Pavel Machek <pavel@ucw.cz>
> Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> ---

> +++ linux-2.6/drivers/als/Kconfig
> @@ -0,0 +1,10 @@
> +#
> +# Ambient Light Sensor sysfs device configuration
> +#
> +
> +menuconfig ALS
> +	tristate "Ambient Light Sensor sysfs device"
> +	help
> +	  This framework provides a generic sysfs interface for
> +	  Ambient Light Sensor devices.
> +	  If you want this support, you should say Y or M here.

Gratuitous nitpick:
Can this option be made non-visible (or hidden behind CONFIG_EMBEDDED)?

Maybe you copied the pattern of the thermal sysfs interface - but the
ACPI thermal driver is useful to keep your computer cool even without
the sysfs interface.  ALS drivers are useless without the sysfs
interface, no?

Regards
Alan

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Rafael J. Wysocki]

On Tuesday 01 September 2009, Pavel Machek wrote:
> 
> > > > IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> > > > illuminance is beyond the device support range, while the device is
> > > > still working normally.
> > > > what about exporting these values (0 and -1) to user space directly?
> > > 
> > > Returning 0 for "below" range and 99999999 for "above" range would be
> > > nice, yes. 
> > 
> > Why not 0 and "all ones" or 0 and -1.
> > 
> > Is there anything wrong with -1 in particular?
> 
> Normal people expect -1 to be less than 123, and output is in ascii. If
> you make it ((unsigned) ~0) I guess that becomes acceptable.

Well, "-1" is a perfectly valid alphanumerical representation of an int.
I don't really see the problem with the "-", unless we're talking about some
broken user space, that is.

Rafael

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Pavel Machek]

On Wed 2009-09-02 23:12:58, Rafael J. Wysocki wrote:
> On Tuesday 01 September 2009, Pavel Machek wrote:
> > 
> > > > > IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> > > > > illuminance is beyond the device support range, while the device is
> > > > > still working normally.
> > > > > what about exporting these values (0 and -1) to user space directly?
> > > > 
> > > > Returning 0 for "below" range and 99999999 for "above" range would be
> > > > nice, yes. 
> > > 
> > > Why not 0 and "all ones" or 0 and -1.
> > > 
> > > Is there anything wrong with -1 in particular?
> > 
> > Normal people expect -1 to be less than 123, and output is in ascii. If
> > you make it ((unsigned) ~0) I guess that becomes acceptable.
> 
> Well, "-1" is a perfectly valid alphanumerical representation of an int.
> I don't really see the problem with the "-", unless we're talking about some
> broken user space, that is.

No. But if you see illumination value of -1 lumen, do you really
expect a *lot* of light?
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Rafael J. Wysocki]

On Wednesday 02 September 2009, Pavel Machek wrote:
> On Wed 2009-09-02 23:12:58, Rafael J. Wysocki wrote:
> > On Tuesday 01 September 2009, Pavel Machek wrote:
> > > 
> > > > > > IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> > > > > > illuminance is beyond the device support range, while the device is
> > > > > > still working normally.
> > > > > > what about exporting these values (0 and -1) to user space directly?
> > > > > 
> > > > > Returning 0 for "below" range and 99999999 for "above" range would be
> > > > > nice, yes. 
> > > > 
> > > > Why not 0 and "all ones" or 0 and -1.
> > > > 
> > > > Is there anything wrong with -1 in particular?
> > > 
> > > Normal people expect -1 to be less than 123, and output is in ascii. If
> > > you make it ((unsigned) ~0) I guess that becomes acceptable.
> > 
> > Well, "-1" is a perfectly valid alphanumerical representation of an int.
> > I don't really see the problem with the "-", unless we're talking about some
> > broken user space, that is.
> 
> No. But if you see illumination value of -1 lumen, do you really
> expect a *lot* of light?

Not really.  I'd rather intrepret it as "the number is not to be trusted",
which is what it means.

The problem with "all ones" is that it depends on the size of the underlying
data type, which is not nice.  Also, if you want that to be a "big number",
there's no clear rule to tell what the number should actually be.

Anyway, this really is a matter of definition.  If we document the attribute
to read as "-1" in specific circumstances, the user space will have to take
that into account.

Thanks,
Rafael

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Pavel Machek]

On Wed 2009-09-02 23:46:11, Rafael J. Wysocki wrote:
> On Wednesday 02 September 2009, Pavel Machek wrote:
> > On Wed 2009-09-02 23:12:58, Rafael J. Wysocki wrote:
> > > On Tuesday 01 September 2009, Pavel Machek wrote:
> > > > 
> > > > > > > IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> > > > > > > illuminance is beyond the device support range, while the device is
> > > > > > > still working normally.
> > > > > > > what about exporting these values (0 and -1) to user space directly?
> > > > > > 
> > > > > > Returning 0 for "below" range and 99999999 for "above" range would be
> > > > > > nice, yes. 
> > > > > 
> > > > > Why not 0 and "all ones" or 0 and -1.
> > > > > 
> > > > > Is there anything wrong with -1 in particular?
> > > > 
> > > > Normal people expect -1 to be less than 123, and output is in ascii. If
> > > > you make it ((unsigned) ~0) I guess that becomes acceptable.
> > > 
> > > Well, "-1" is a perfectly valid alphanumerical representation of an int.
> > > I don't really see the problem with the "-", unless we're talking about some
> > > broken user space, that is.
> > 
> > No. But if you see illumination value of -1 lumen, do you really
> > expect a *lot* of light?
> 
> Not really.  I'd rather intrepret it as "the number is not to be trusted",
> which is what it means.
> 
> The problem with "all ones" is that it depends on the size of the underlying
> data type, which is not nice.  Also, if you want that to be a "big number",
> there's no clear rule to tell what the number should actually be.
> 
> Anyway, this really is a matter of definition.  If we document the attribute
> to read as "-1" in specific circumstances, the user space will have to take
> that into account.

Well, I'd prefer to specify -1 as "underflow" and 1000000000 as
"overflow". Any numbers should work, but ... lets make the interface
logical if we can.

									Pavel 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Rafael J. Wysocki]

On Wednesday 02 September 2009, Pavel Machek wrote:
> On Wed 2009-09-02 23:46:11, Rafael J. Wysocki wrote:
> > On Wednesday 02 September 2009, Pavel Machek wrote:
> > > On Wed 2009-09-02 23:12:58, Rafael J. Wysocki wrote:
> > > > On Tuesday 01 September 2009, Pavel Machek wrote:
> > > > > 
> > > > > > > > IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> > > > > > > > illuminance is beyond the device support range, while the device is
> > > > > > > > still working normally.
> > > > > > > > what about exporting these values (0 and -1) to user space directly?
> > > > > > > 
> > > > > > > Returning 0 for "below" range and 99999999 for "above" range would be
> > > > > > > nice, yes. 
> > > > > > 
> > > > > > Why not 0 and "all ones" or 0 and -1.
> > > > > > 
> > > > > > Is there anything wrong with -1 in particular?
> > > > > 
> > > > > Normal people expect -1 to be less than 123, and output is in ascii. If
> > > > > you make it ((unsigned) ~0) I guess that becomes acceptable.
> > > > 
> > > > Well, "-1" is a perfectly valid alphanumerical representation of an int.
> > > > I don't really see the problem with the "-", unless we're talking about some
> > > > broken user space, that is.
> > > 
> > > No. But if you see illumination value of -1 lumen, do you really
> > > expect a *lot* of light?
> > 
> > Not really.  I'd rather intrepret it as "the number is not to be trusted",
> > which is what it means.
> > 
> > The problem with "all ones" is that it depends on the size of the underlying
> > data type, which is not nice.  Also, if you want that to be a "big number",
> > there's no clear rule to tell what the number should actually be.
> > 
> > Anyway, this really is a matter of definition.  If we document the attribute
> > to read as "-1" in specific circumstances, the user space will have to take
> > that into account.
> 
> Well, I'd prefer to specify -1 as "underflow" and 1000000000 as
> "overflow". Any numbers should work, but ... lets make the interface
> logical if we can.

The interface is already defined, isn't it?  And we're now considering whether
or not to pass the values defined by the interface directly to the user space,
which I think is the right thing to do, because we have no reason _whatsoever_
to change them to anything else.

Thanks,
Rafael

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Zhang Rui]

On Thu, 2009-09-03 at 08:16 +0800, Rafael J. Wysocki wrote:
> On Wednesday 02 September 2009, Pavel Machek wrote:
> > On Wed 2009-09-02 23:46:11, Rafael J. Wysocki wrote:
> > > On Wednesday 02 September 2009, Pavel Machek wrote:
> > > > On Wed 2009-09-02 23:12:58, Rafael J. Wysocki wrote:
> > > > > On Tuesday 01 September 2009, Pavel Machek wrote:
> > > > > > 
> > > > > > > > > IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> > > > > > > > > illuminance is beyond the device support range, while the device is
> > > > > > > > > still working normally.
> > > > > > > > > what about exporting these values (0 and -1) to user space directly?
> > > > > > > > 
> > > > > > > > Returning 0 for "below" range and 99999999 for "above" range would be
> > > > > > > > nice, yes. 
> > > > > > > 
> > > > > > > Why not 0 and "all ones" or 0 and -1.
> > > > > > > 
> > > > > > > Is there anything wrong with -1 in particular?
> > > > > > 
> > > > > > Normal people expect -1 to be less than 123, and output is in ascii. If
> > > > > > you make it ((unsigned) ~0) I guess that becomes acceptable.
> > > > > 
> > > > > Well, "-1" is a perfectly valid alphanumerical representation of an int.
> > > > > I don't really see the problem with the "-", unless we're talking about some
> > > > > broken user space, that is.
> > > > 
> > > > No. But if you see illumination value of -1 lumen, do you really
> > > > expect a *lot* of light?
> > > 
> > > Not really.  I'd rather intrepret it as "the number is not to be trusted",
> > > which is what it means.
> > > 
> > > The problem with "all ones" is that it depends on the size of the underlying
> > > data type, which is not nice.  Also, if you want that to be a "big number",
> > > there's no clear rule to tell what the number should actually be.
> > > 
> > > Anyway, this really is a matter of definition.  If we document the attribute
> > > to read as "-1" in specific circumstances, the user space will have to take
> > > that into account.
> > 
> > Well, I'd prefer to specify -1 as "underflow" and 1000000000 as
> > "overflow". Any numbers should work, but ... lets make the interface
> > logical if we can.
> 
> The interface is already defined, isn't it?  And we're now considering whether
> or not to pass the values defined by the interface directly to the user space,
> which I think is the right thing to do, because we have no reason _whatsoever_
> to change them to anything else.
> 
I agree.
For environment illuminance, "-1" is surely an invalid value.
IMO, users would rather look for what it actually means than interpret
it to a value lower than 0.

thanks,
rui

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Zhang Rui]

On Thu, 2009-09-03 at 03:22 +0800, Alan Jenkins wrote:
> On 9/1/09, Zhang Rui <rui.zhang@intel.com> wrote:
> >
> > Introduce ALS sysfs class.
> >
> > ALS sysfs class provides a standard sysfs interface for
> > Ambient Light Sensor devices.
> >
> > please read Documentation/ABI/testing/sysfs-class-als for
> > detailed sysfs designs.
> >
> > CC: Pavel Machek <pavel@ucw.cz>
> > Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
> > ---
> 
> > +++ linux-2.6/drivers/als/Kconfig
> > @@ -0,0 +1,10 @@
> > +#
> > +# Ambient Light Sensor sysfs device configuration
> > +#
> > +
> > +menuconfig ALS
> > +	tristate "Ambient Light Sensor sysfs device"
> > +	help
> > +	  This framework provides a generic sysfs interface for
> > +	  Ambient Light Sensor devices.
> > +	  If you want this support, you should say Y or M here.
> 
> Gratuitous nitpick:
> Can this option be made non-visible (or hidden behind CONFIG_EMBEDDED)?
> 
> Maybe you copied the pattern of the thermal sysfs interface - but the
> ACPI thermal driver is useful to keep your computer cool even without
> the sysfs interface.  ALS drivers are useless without the sysfs
> interface, no?
> 
ACPI ALS device driver is the first user of this sysfs class.
I think there will be more native ALS device drivers in the future,
which locate at drivers/als/ and depends on CONFIG_ALS.

thanks,
rui

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Alan Jenkins]

On 9/3/09, Zhang Rui <rui.zhang@intel.com> wrote:
> On Thu, 2009-09-03 at 03:22 +0800, Alan Jenkins wrote:
>> On 9/1/09, Zhang Rui <rui.zhang@intel.com> wrote:
>> >
>> > Introduce ALS sysfs class.
>> >
>> > ALS sysfs class provides a standard sysfs interface for
>> > Ambient Light Sensor devices.
>> >
>> > please read Documentation/ABI/testing/sysfs-class-als for
>> > detailed sysfs designs.
>> >
>> > CC: Pavel Machek <pavel@ucw.cz>
>> > Acked-by: Greg Kroah-Hartman <gregkh@suse.de>
>> > Signed-off-by: Zhang Rui <rui.zhang@intel.com>
>> > ---
>>
>> > +++ linux-2.6/drivers/als/Kconfig
>> > @@ -0,0 +1,10 @@
>> > +#
>> > +# Ambient Light Sensor sysfs device configuration
>> > +#
>> > +
>> > +menuconfig ALS
>> > +	tristate "Ambient Light Sensor sysfs device"
>> > +	help
>> > +	  This framework provides a generic sysfs interface for
>> > +	  Ambient Light Sensor devices.
>> > +	  If you want this support, you should say Y or M here.
>>
>> Gratuitous nitpick:
>> Can this option be made non-visible (or hidden behind CONFIG_EMBEDDED)?
>>
>> Maybe you copied the pattern of the thermal sysfs interface - but the
>> ACPI thermal driver is useful to keep your computer cool even without
>> the sysfs interface.  ALS drivers are useless without the sysfs
>> interface, no?
>>
> ACPI ALS device driver is the first user of this sysfs class.
> I think there will be more native ALS device drivers in the future,
> which locate at drivers/als/ and depends on CONFIG_ALS.
>
> thanks,
> rui

Oh!  So it may become a menu like e.g. hwmon.

Thanks for the explanation
Alan

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Pavel Machek]

On Thu 2009-09-03 02:16:04, Rafael J. Wysocki wrote:
> On Wednesday 02 September 2009, Pavel Machek wrote:
> > On Wed 2009-09-02 23:46:11, Rafael J. Wysocki wrote:
> > > On Wednesday 02 September 2009, Pavel Machek wrote:
> > > > On Wed 2009-09-02 23:12:58, Rafael J. Wysocki wrote:
> > > > > On Tuesday 01 September 2009, Pavel Machek wrote:
> > > > > > 
> > > > > > > > > IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> > > > > > > > > illuminance is beyond the device support range, while the device is
> > > > > > > > > still working normally.
> > > > > > > > > what about exporting these values (0 and -1) to user space directly?
> > > > > > > > 
> > > > > > > > Returning 0 for "below" range and 99999999 for "above" range would be
> > > > > > > > nice, yes. 
> > > > > > > 
> > > > > > > Why not 0 and "all ones" or 0 and -1.
> > > > > > > 
> > > > > > > Is there anything wrong with -1 in particular?
> > > > > > 
> > > > > > Normal people expect -1 to be less than 123, and output is in ascii. If
> > > > > > you make it ((unsigned) ~0) I guess that becomes acceptable.
> > > > > 
> > > > > Well, "-1" is a perfectly valid alphanumerical representation of an int.
> > > > > I don't really see the problem with the "-", unless we're talking about some
> > > > > broken user space, that is.
> > > > 
> > > > No. But if you see illumination value of -1 lumen, do you really
> > > > expect a *lot* of light?
> > > 
> > > Not really.  I'd rather intrepret it as "the number is not to be trusted",
> > > which is what it means.
> > > 
> > > The problem with "all ones" is that it depends on the size of the underlying
> > > data type, which is not nice.  Also, if you want that to be a "big number",
> > > there's no clear rule to tell what the number should actually be.
> > > 
> > > Anyway, this really is a matter of definition.  If we document the attribute
> > > to read as "-1" in specific circumstances, the user space will have to take
> > > that into account.
> > 
> > Well, I'd prefer to specify -1 as "underflow" and 1000000000 as
> > "overflow". Any numbers should work, but ... lets make the interface
> > logical if we can.
> 
> The interface is already defined, isn't it?  And we're now
> considering whether

No, I don't think it is "already defined". Yes the patches float
around, but as nothing is merged we can still fix it easily.
								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Pavel Machek]

On Thu 2009-09-03 10:35:39, Zhang Rui wrote:
> On Thu, 2009-09-03 at 08:16 +0800, Rafael J. Wysocki wrote:
> > On Wednesday 02 September 2009, Pavel Machek wrote:
> > > On Wed 2009-09-02 23:46:11, Rafael J. Wysocki wrote:
> > > > On Wednesday 02 September 2009, Pavel Machek wrote:
> > > > > On Wed 2009-09-02 23:12:58, Rafael J. Wysocki wrote:
> > > > > > On Tuesday 01 September 2009, Pavel Machek wrote:
> > > > > > > 
> > > > > > > > > > IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> > > > > > > > > > illuminance is beyond the device support range, while the device is
> > > > > > > > > > still working normally.
> > > > > > > > > > what about exporting these values (0 and -1) to user space directly?
> > > > > > > > > 
> > > > > > > > > Returning 0 for "below" range and 99999999 for "above" range would be
> > > > > > > > > nice, yes. 
> > > > > > > > 
> > > > > > > > Why not 0 and "all ones" or 0 and -1.
> > > > > > > > 
> > > > > > > > Is there anything wrong with -1 in particular?
> > > > > > > 
> > > > > > > Normal people expect -1 to be less than 123, and output is in ascii. If
> > > > > > > you make it ((unsigned) ~0) I guess that becomes acceptable.
> > > > > > 
> > > > > > Well, "-1" is a perfectly valid alphanumerical representation of an int.
> > > > > > I don't really see the problem with the "-", unless we're talking about some
> > > > > > broken user space, that is.
> > > > > 
> > > > > No. But if you see illumination value of -1 lumen, do you really
> > > > > expect a *lot* of light?
> > > > 
> > > > Not really.  I'd rather intrepret it as "the number is not to be trusted",
> > > > which is what it means.
> > > > 
> > > > The problem with "all ones" is that it depends on the size of the underlying
> > > > data type, which is not nice.  Also, if you want that to be a "big number",
> > > > there's no clear rule to tell what the number should actually be.
> > > > 
> > > > Anyway, this really is a matter of definition.  If we document the attribute
> > > > to read as "-1" in specific circumstances, the user space will have to take
> > > > that into account.
> > > 
> > > Well, I'd prefer to specify -1 as "underflow" and 1000000000 as
> > > "overflow". Any numbers should work, but ... lets make the interface
> > > logical if we can.
> > 
> > The interface is already defined, isn't it?  And we're now considering whether
> > or not to pass the values defined by the interface directly to the user space,
> > which I think is the right thing to do, because we have no reason _whatsoever_
> > to change them to anything else.
> > 
> I agree.
> For environment illuminance, "-1" is surely an invalid value.
> IMO, users would rather look for what it actually means than interpret
> it to a value lower than 0.

Yes, you'd have to look that up. With 1000000000 for overflow, you
would not have to do any lookups...

(Plus, if you use -1 for overflow... you need 0 for underflow, but 0
lumen is pretty valid value. Actually I'm thinking if this should
maybe not on the log scale or something).
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Rafael J. Wysocki]

On Thursday 03 September 2009, Pavel Machek wrote:
> On Thu 2009-09-03 02:16:04, Rafael J. Wysocki wrote:
> > On Wednesday 02 September 2009, Pavel Machek wrote:
> > > On Wed 2009-09-02 23:46:11, Rafael J. Wysocki wrote:
> > > > On Wednesday 02 September 2009, Pavel Machek wrote:
> > > > > On Wed 2009-09-02 23:12:58, Rafael J. Wysocki wrote:
> > > > > > On Tuesday 01 September 2009, Pavel Machek wrote:
> > > > > > > 
> > > > > > > > > > IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> > > > > > > > > > illuminance is beyond the device support range, while the device is
> > > > > > > > > > still working normally.
> > > > > > > > > > what about exporting these values (0 and -1) to user space directly?
> > > > > > > > > 
> > > > > > > > > Returning 0 for "below" range and 99999999 for "above" range would be
> > > > > > > > > nice, yes. 
> > > > > > > > 
> > > > > > > > Why not 0 and "all ones" or 0 and -1.
> > > > > > > > 
> > > > > > > > Is there anything wrong with -1 in particular?
> > > > > > > 
> > > > > > > Normal people expect -1 to be less than 123, and output is in ascii. If
> > > > > > > you make it ((unsigned) ~0) I guess that becomes acceptable.
> > > > > > 
> > > > > > Well, "-1" is a perfectly valid alphanumerical representation of an int.
> > > > > > I don't really see the problem with the "-", unless we're talking about some
> > > > > > broken user space, that is.
> > > > > 
> > > > > No. But if you see illumination value of -1 lumen, do you really
> > > > > expect a *lot* of light?
> > > > 
> > > > Not really.  I'd rather intrepret it as "the number is not to be trusted",
> > > > which is what it means.
> > > > 
> > > > The problem with "all ones" is that it depends on the size of the underlying
> > > > data type, which is not nice.  Also, if you want that to be a "big number",
> > > > there's no clear rule to tell what the number should actually be.
> > > > 
> > > > Anyway, this really is a matter of definition.  If we document the attribute
> > > > to read as "-1" in specific circumstances, the user space will have to take
> > > > that into account.
> > > 
> > > Well, I'd prefer to specify -1 as "underflow" and 1000000000 as
> > > "overflow". Any numbers should work, but ... lets make the interface
> > > logical if we can.
> > 
> > The interface is already defined, isn't it?  And we're now
> > considering whether
> 
> No, I don't think it is "already defined".

ACPI Specification 4.0, Section 9.2.2, p. 335, defines the interface quite
clearly.  It actually is defined as "Ones (-1)", but as I said previously, the
"all ones" version is not really convenient for sysfs.

I don't think the attribute should return anything other than the values
defined by the spec, because anyone searching for documentation will first look
into the spec.

As I said before, I don't think we have any reason _whatsoever_ to translate
the spec-defined values to anything else.

Thanks,
Rafael

Re: [PATCH V6 1/2] introduce ALS sysfs class

[Rafael J. Wysocki]

On Thursday 03 September 2009, Pavel Machek wrote:
> On Thu 2009-09-03 10:35:39, Zhang Rui wrote:
> > On Thu, 2009-09-03 at 08:16 +0800, Rafael J. Wysocki wrote:
> > > On Wednesday 02 September 2009, Pavel Machek wrote:
> > > > On Wed 2009-09-02 23:46:11, Rafael J. Wysocki wrote:
> > > > > On Wednesday 02 September 2009, Pavel Machek wrote:
> > > > > > On Wed 2009-09-02 23:12:58, Rafael J. Wysocki wrote:
> > > > > > > On Tuesday 01 September 2009, Pavel Machek wrote:
> > > > > > > > 
> > > > > > > > > > > IMO, 0 and -1 are not errors. they just suggest that the Ambient Light
> > > > > > > > > > > illuminance is beyond the device support range, while the device is
> > > > > > > > > > > still working normally.
> > > > > > > > > > > what about exporting these values (0 and -1) to user space directly?
> > > > > > > > > > 
> > > > > > > > > > Returning 0 for "below" range and 99999999 for "above" range would be
> > > > > > > > > > nice, yes. 
> > > > > > > > > 
> > > > > > > > > Why not 0 and "all ones" or 0 and -1.
> > > > > > > > > 
> > > > > > > > > Is there anything wrong with -1 in particular?
> > > > > > > > 
> > > > > > > > Normal people expect -1 to be less than 123, and output is in ascii. If
> > > > > > > > you make it ((unsigned) ~0) I guess that becomes acceptable.
> > > > > > > 
> > > > > > > Well, "-1" is a perfectly valid alphanumerical representation of an int.
> > > > > > > I don't really see the problem with the "-", unless we're talking about some
> > > > > > > broken user space, that is.
> > > > > > 
> > > > > > No. But if you see illumination value of -1 lumen, do you really
> > > > > > expect a *lot* of light?
> > > > > 
> > > > > Not really.  I'd rather intrepret it as "the number is not to be trusted",
> > > > > which is what it means.
> > > > > 
> > > > > The problem with "all ones" is that it depends on the size of the underlying
> > > > > data type, which is not nice.  Also, if you want that to be a "big number",
> > > > > there's no clear rule to tell what the number should actually be.
> > > > > 
> > > > > Anyway, this really is a matter of definition.  If we document the attribute
> > > > > to read as "-1" in specific circumstances, the user space will have to take
> > > > > that into account.
> > > > 
> > > > Well, I'd prefer to specify -1 as "underflow" and 1000000000 as
> > > > "overflow". Any numbers should work, but ... lets make the interface
> > > > logical if we can.
> > > 
> > > The interface is already defined, isn't it?  And we're now considering whether
> > > or not to pass the values defined by the interface directly to the user space,
> > > which I think is the right thing to do, because we have no reason _whatsoever_
> > > to change them to anything else.
> > > 
> > I agree.
> > For environment illuminance, "-1" is surely an invalid value.
> > IMO, users would rather look for what it actually means than interpret
> > it to a value lower than 0.
> 
> Yes, you'd have to look that up. With 1000000000 for overflow, you
> would not have to do any lookups...
> 
> (Plus, if you use -1 for overflow... you need 0 for underflow, but 0
> lumen is pretty valid value. Actually I'm thinking if this should
> maybe not on the log scale or something).

Let's make the "-1 corresponds to all ones" rule and stop losing time for
discussing that any more.  I don't think it's worth it. :-)

Thanks,
Rafael