| Message ID | 1251789947.3483.215.camel@rzhang-dt (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
> > > 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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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)