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 <[email protected]>
Acked-by: Greg Kroah-Hartman <[email protected]>
Signed-off-by: Zhang Rui <[email protected]>
---
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 <[email protected]>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * 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 <[email protected]>
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ * 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 <[email protected]>
+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 <[email protected]>
+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: [email protected]
F: arch/alpha/
+AMBIENT LIGHT SENSOR
+M: Zhang Rui <[email protected]>
+S: Supported
+F: include/linux/als_sys.h
+F: drivers/als/
+
AMD GEODE CS5536 USB DEVICE CONTROLLER DRIVER
M: Thomas Dahlmann <[email protected]>
L: [email protected] (moderated for non-subscribers)
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
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
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
> > > 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
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
On 9/1/09, Zhang Rui <[email protected]> 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 <[email protected]>
> Acked-by: Greg Kroah-Hartman <[email protected]>
> Signed-off-by: Zhang Rui <[email protected]>
> ---
> +++ 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
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
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
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
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
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
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
On Thu, 2009-09-03 at 03:22 +0800, Alan Jenkins wrote:
> On 9/1/09, Zhang Rui <[email protected]> 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 <[email protected]>
> > Acked-by: Greg Kroah-Hartman <[email protected]>
> > Signed-off-by: Zhang Rui <[email protected]>
> > ---
>
> > +++ 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
On 9/3/09, Zhang Rui <[email protected]> wrote:
> On Thu, 2009-09-03 at 03:22 +0800, Alan Jenkins wrote:
>> On 9/1/09, Zhang Rui <[email protected]> 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 <[email protected]>
>> > Acked-by: Greg Kroah-Hartman <[email protected]>
>> > Signed-off-by: Zhang Rui <[email protected]>
>> > ---
>>
>> > +++ 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
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
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
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
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