Re: RFC: Light sensors, unifying current options?

Re: RFC: Light sensors, unifying current options?

[Zhang Rui]

Hi, Jonathan,

On Thu, 2009-09-03 at 21:51 +0800, Jonathan Cameron wrote:
> Dear All,
> 
> This thread is a follow up to (amongst others)
> 
> [lm-sensors] Ambient Light sensor for Intersil-ISL29020 device
> 
> Currently there are a number of light sensor drivers either in the
> mainline kernel, posted to various mailing lists or sitting in various
> testing trees.
> 
> For example.
> 
> Intersil ISL29020
> http://www.intersil.com/products/deviceinfo.asp?pn=ISL29020 driver
> posted by Kalan Trisal to lm-sensors (as hwmon device rejected for
> being out of subsystem scope)
> http://lists.lm-sensors.org/pipermail/lm-sensors/2009-September/026575.html
> 
> ALS_sysfs class and als_acpi driver (V6 posted to lkml earlier this week).
> http://lkml.org/lkml/2009/9/1/38
> 
> TSL2561 under the industrial I/O Framework. (in Greg KH's tree, will
> being in staging after merge window - there due to lack of review
> more than any known problems.)
> http://www.farnell.com/datasheets/49661.pdf
> http://lkml.org/lkml/2009/7/2/189
> 
> TSL2550 under i2c/chips/ which as a location is going away.
> http://www.farnell.com/datasheets/48715.pdf
> 
> (any others people know of?)
> 
> Two big questions:
> 
> * Are there sufficient shared characteristics between these devices to
> all for a unified framework? (would certainly be nice!)
> 
> * What applications are they used for? This will drive the question
> of what functionality is desirable. (particularly do we need an event
> infrastructure or not).
> 
> To sumarize the functionality currently provided by the above drivers
> (or that should probably be added)
> 
> ISL29020:
> * sensing_range
> * lux_level
> * power state (should probably move over to the new power management
>   framework)
> 
> ALS:
> * illuminance (equivalent of lux_level)
> * adjustment (I don't follow the purpose of this, but then I don't know
> anything about how this is being used!)
> 
adjustment is a percentage value used by userspace to adjust the display
backlight.

According to the ACPI spec, ACPI ALS device has "ambient light
illuminance to display luminance" mappings that can be used by an OS to
calibrate its ambient light policy for a given sensor configuration. The
OS can use this information to extrapolate an ALS response curve. i.e.
ACPI ALS knows what to do when ambient light illuminance changes, but it
won't change the backlight. Instead, it exports this info to user space
via the "adjustment" attribute.
user space applications can get this value and change the display
brightness via the backlight sysfs I/F.

IMO, the ALS device should do the following work:
1. catch the ambient light illuminance change.
2. tell the userspace what to do with this change.
isn't this true for the other ALS devices in this thread?

> TSL2561
> * infrared (raw value)
> * broad spectrum (raw value)
> (I'm of the view any derived values should probably be done in userspace)
> This one is under IIO at the moment for two reasons.
> 1) I hit the same issue of no suitable subsystem, but for a much larger
> class of sensor devices. Light sensors are just one example (that's not
> to say I mind hiving this lot off to a system of their own).
> 2) To provide an event interface (which I haven't yet done)
> Driver should also include:
> * integration time
> * gain control 
> 
> TSL2550
> * power state
> * operating mode
> * lux (actually calculated from two separate readings as
> per the tsl2561 but the are not available to userspace)
> 
> Applications:
> 
> 1) Backlight intensity type apps (guessing that covers most people)
> 2) Environmental monitoring apps (the crossbow imb400 imote2 daughter
> board I'm using doesn't have any screen or other direct interface, its
> simply a lightweight sensor platform).
> 3) High speed apps (all current sensors are pretty slow so this isn't
> yet relevant).
> 
> My personal feelings is that the IIO is overkill for these types
> of sensors (slow update rate, tsl2550 takes 400ms, tsl2561 12-400ms)
> unless we want the event handling infrastructure. I'm inclined to
> say it is unecessary given the same result could be obtained by
> polling only a few times a second.
> 
agree.
this is not a problem to ACPI ALS device.
ACPI sends a notification to the ACPI ALS device when illuminance is
changed.

> My comments on ALS may be wrong or misleading as they are based on a
> brief read of the code (please correct me!)  A lot of the
> infrastructure is only necessary if we have in kernel users
>  (and at
> the moment the functionality doesn't appear to be there for any such
> users to acquire access to these sensors in the first place.  For
> example, the approach used by hwmon of letting drivers define their
> own attributes seems to me to be more easily extendable than ALS' use
> of an ops structure.

I agree that the ops structure is unnecessary.
To make the als_sys class more generic, we just need to
1. defines the generic attributes that the native ALS driver must
follow.
2. registers an als device in the als_sys class.
and the native driver should be responsible for the sysfs attributes.

Because my approach is made by reading the ACPI spec, I'm not sure what
should be done in the native driver and what should be done in the
generic driver at the beginning.

thanks for pointing this out.

>  For example, I'm not convinced it makes sense for
> drivers to have to have a get_adjustment attribute or indeed even
> necessarily have a direct illuminance attribute (deriving the relevant
> value may be a case of userspace combining several associated
> readings).

what these associated readings are?
I think we can define some optional attributes besides the required
attributes.
but we should make clear what is necessary for an ALS device, and what
optional features it may support.

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

Re: RFC: Light sensors, unifying current options?

[Corentin Chary]

On Mon, Sep 7, 2009 at 9:32 AM, Zhang Rui<rui.zhang@intel.com> wrote:
> Hi, Jonathan,
>
> On Thu, 2009-09-03 at 21:51 +0800, Jonathan Cameron wrote:
>> Dear All,
>>
>> This thread is a follow up to (amongst others)
>>
>> [lm-sensors] Ambient Light sensor for Intersil-ISL29020 device
>>
>> Currently there are a number of light sensor drivers either in the
>> mainline kernel, posted to various mailing lists or sitting in various
>> testing trees.
>>
>> For example.
>>
>> Intersil ISL29020
>> http://www.intersil.com/products/deviceinfo.asp?pn=ISL29020 driver
>> posted by Kalan Trisal to lm-sensors (as hwmon device rejected for
>> being out of subsystem scope)
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2009-September/026575.html
>>
>> ALS_sysfs class and als_acpi driver (V6 posted to lkml earlier this week).
>> http://lkml.org/lkml/2009/9/1/38
>>
>> TSL2561 under the industrial I/O Framework. (in Greg KH's tree, will
>> being in staging after merge window - there due to lack of review
>> more than any known problems.)
>> http://www.farnell.com/datasheets/49661.pdf
>> http://lkml.org/lkml/2009/7/2/189
>>
>> TSL2550 under i2c/chips/ which as a location is going away.
>> http://www.farnell.com/datasheets/48715.pdf
>>
>> (any others people know of?)

asus-laptop have one, it exports power switch and sensitivity.
Josh Green did the patch, not me, and I don't have the hardware, so I
don't know exactly how sensitivy works.

>> Two big questions:
>>
>> * Are there sufficient shared characteristics between these devices to
>> all for a unified framework? (would certainly be nice!)
>>
>> * What applications are they used for? This will drive the question
>> of what functionality is desirable. (particularly do we need an event
>> infrastructure or not).
>>
>> To sumarize the functionality currently provided by the above drivers
>> (or that should probably be added)
>>
>> ISL29020:
>> * sensing_range
>> * lux_level
>> * power state (should probably move over to the new power management
>>   framework)
>>
>> ALS:
>> * illuminance (equivalent of lux_level)
>> * adjustment (I don't follow the purpose of this, but then I don't know
>> anything about how this is being used!)
>>
> adjustment is a percentage value used by userspace to adjust the display
> backlight.
>
> According to the ACPI spec, ACPI ALS device has "ambient light
> illuminance to display luminance" mappings that can be used by an OS to
> calibrate its ambient light policy for a given sensor configuration. The
> OS can use this information to extrapolate an ALS response curve. i.e.
> ACPI ALS knows what to do when ambient light illuminance changes, but it
> won't change the backlight. Instead, it exports this info to user space
> via the "adjustment" attribute.
> user space applications can get this value and change the display
> brightness via the backlight sysfs I/F.
>
> IMO, the ALS device should do the following work:
> 1. catch the ambient light illuminance change.
> 2. tell the userspace what to do with this change.
> isn't this true for the other ALS devices in this thread?
>
>> TSL2561
>> * infrared (raw value)
>> * broad spectrum (raw value)
>> (I'm of the view any derived values should probably be done in userspace)
>> This one is under IIO at the moment for two reasons.
>> 1) I hit the same issue of no suitable subsystem, but for a much larger
>> class of sensor devices. Light sensors are just one example (that's not
>> to say I mind hiving this lot off to a system of their own).
>> 2) To provide an event interface (which I haven't yet done)
>> Driver should also include:
>> * integration time
>> * gain control
>>
>> TSL2550
>> * power state
>> * operating mode
>> * lux (actually calculated from two separate readings as
>> per the tsl2561 but the are not available to userspace)
>>
>> Applications:
>>
>> 1) Backlight intensity type apps (guessing that covers most people)
>> 2) Environmental monitoring apps (the crossbow imb400 imote2 daughter
>> board I'm using doesn't have any screen or other direct interface, its
>> simply a lightweight sensor platform).
>> 3) High speed apps (all current sensors are pretty slow so this isn't
>> yet relevant).
>>
>> My personal feelings is that the IIO is overkill for these types
>> of sensors (slow update rate, tsl2550 takes 400ms, tsl2561 12-400ms)
>> unless we want the event handling infrastructure. I'm inclined to
>> say it is unecessary given the same result could be obtained by
>> polling only a few times a second.
>>
> agree.
> this is not a problem to ACPI ALS device.
> ACPI sends a notification to the ACPI ALS device when illuminance is
> changed.
>
>> My comments on ALS may be wrong or misleading as they are based on a
>> brief read of the code (please correct me!)  A lot of the
>> infrastructure is only necessary if we have in kernel users
>>  (and at
>> the moment the functionality doesn't appear to be there for any such
>> users to acquire access to these sensors in the first place.  For
>> example, the approach used by hwmon of letting drivers define their
>> own attributes seems to me to be more easily extendable than ALS' use
>> of an ops structure.
>
> I agree that the ops structure is unnecessary.
> To make the als_sys class more generic, we just need to
> 1. defines the generic attributes that the native ALS driver must
> follow.
> 2. registers an als device in the als_sys class.
> and the native driver should be responsible for the sysfs attributes.
>
> Because my approach is made by reading the ACPI spec, I'm not sure what
> should be done in the native driver and what should be done in the
> generic driver at the beginning.
>
> thanks for pointing this out.
>
>>  For example, I'm not convinced it makes sense for
>> drivers to have to have a get_adjustment attribute or indeed even
>> necessarily have a direct illuminance attribute (deriving the relevant
>> value may be a case of userspace combining several associated
>> readings).
>
> what these associated readings are?
> I think we can define some optional attributes besides the required
> attributes.
> but we should make clear what is necessary for an ALS device, and what
> optional features it may support.
>
> 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
>



-- 
Corentin Chary
http://xf.iksaif.net
--
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

Re: RFC: Light sensors, unifying current options?

[Jonathan Cameron]

Zhang Rui wrote:
> Hi, Jonathan,
> 
> On Thu, 2009-09-03 at 21:51 +0800, Jonathan Cameron wrote:
>> Dear All,
>>
>> This thread is a follow up to (amongst others)
>>
>> [lm-sensors] Ambient Light sensor for Intersil-ISL29020 device
>>
>> Currently there are a number of light sensor drivers either in the
>> mainline kernel, posted to various mailing lists or sitting in various
>> testing trees.
>>
>> For example.
>>
>> Intersil ISL29020
>> http://www.intersil.com/products/deviceinfo.asp?pn=ISL29020 driver
>> posted by Kalan Trisal to lm-sensors (as hwmon device rejected for
>> being out of subsystem scope)
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2009-September/026575.html
>>
>> ALS_sysfs class and als_acpi driver (V6 posted to lkml earlier this week).
>> http://lkml.org/lkml/2009/9/1/38
>>
>> TSL2561 under the industrial I/O Framework. (in Greg KH's tree, will
>> being in staging after merge window - there due to lack of review
>> more than any known problems.)
>> http://www.farnell.com/datasheets/49661.pdf
>> http://lkml.org/lkml/2009/7/2/189
>>
>> TSL2550 under i2c/chips/ which as a location is going away.
>> http://www.farnell.com/datasheets/48715.pdf
>>
>> (any others people know of?)
>>
>> Two big questions:
>>
>> * Are there sufficient shared characteristics between these devices to
>> all for a unified framework? (would certainly be nice!)
>>
>> * What applications are they used for? This will drive the question
>> of what functionality is desirable. (particularly do we need an event
>> infrastructure or not).
>>
>> To sumarize the functionality currently provided by the above drivers
>> (or that should probably be added)
>>
>> ISL29020:
>> * sensing_range
>> * lux_level
>> * power state (should probably move over to the new power management
>>   framework)
>>
>> ALS:
>> * illuminance (equivalent of lux_level)
>> * adjustment (I don't follow the purpose of this, but then I don't know
>> anything about how this is being used!)
>>
> adjustment is a percentage value used by userspace to adjust the display
> backlight.
> 
> According to the ACPI spec, ACPI ALS device has "ambient light
> illuminance to display luminance" mappings that can be used by an OS to
> calibrate its ambient light policy for a given sensor configuration. The
> OS can use this information to extrapolate an ALS response curve. i.e.
> ACPI ALS knows what to do when ambient light illuminance changes, but it
> won't change the backlight. Instead, it exports this info to user space
> via the "adjustment" attribute.
> user space applications can get this value and change the display
> brightness via the backlight sysfs I/F.
Is this conversion entirely independent of the physical configuration of
the sensor?  I can sort of imagine cases where some direct pickup from
the backlight occurs alongside the ambient and some where none does.
Fair enough if not.

> IMO, the ALS device should do the following work:
> 1. catch the ambient light illuminance change.
Sometimes this is more complex with the ability to separately read light
levels in different frequency ranges (e.g. infrared and visible)  Still
this value can usually be derived.
> 2. tell the userspace what to do with this change.
> isn't this true for the other ALS devices in this thread?
None of the others (other than the additional asus one mentioned
later in this thread - which I haven't looked at) have any concept of what userspace
should do with the value. They simply measure it (and supply appropriate
threshold interrupts etc)
> 
>> TSL2561
>> * infrared (raw value)
>> * broad spectrum (raw value)
>> (I'm of the view any derived values should probably be done in userspace)
>> This one is under IIO at the moment for two reasons.
>> 1) I hit the same issue of no suitable subsystem, but for a much larger
>> class of sensor devices. Light sensors are just one example (that's not
>> to say I mind hiving this lot off to a system of their own).
>> 2) To provide an event interface (which I haven't yet done)
>> Driver should also include:
>> * integration time
>> * gain control 
>>
>> TSL2550
>> * power state
>> * operating mode
>> * lux (actually calculated from two separate readings as
>> per the tsl2561 but the are not available to userspace)
>>
>> Applications:
>>
>> 1) Backlight intensity type apps (guessing that covers most people)
>> 2) Environmental monitoring apps (the crossbow imb400 imote2 daughter
>> board I'm using doesn't have any screen or other direct interface, its
>> simply a lightweight sensor platform).
>> 3) High speed apps (all current sensors are pretty slow so this isn't
>> yet relevant).
>>
>> My personal feelings is that the IIO is overkill for these types
>> of sensors (slow update rate, tsl2550 takes 400ms, tsl2561 12-400ms)
>> unless we want the event handling infrastructure. I'm inclined to
>> say it is unecessary given the same result could be obtained by
>> polling only a few times a second.
>>
> agree.
> this is not a problem to ACPI ALS device.
> ACPI sends a notification to the ACPI ALS device when illuminance is
> changed.
> 
>> My comments on ALS may be wrong or misleading as they are based on a
>> brief read of the code (please correct me!)  A lot of the
>> infrastructure is only necessary if we have in kernel users
>>  (and at
>> the moment the functionality doesn't appear to be there for any such
>> users to acquire access to these sensors in the first place.  For
>> example, the approach used by hwmon of letting drivers define their
>> own attributes seems to me to be more easily extendable than ALS' use
>> of an ops structure.
> 
> I agree that the ops structure is unnecessary.
> To make the als_sys class more generic, we just need to
> 1. defines the generic attributes that the native ALS driver must
> follow.
> 2. registers an als device in the als_sys class.
> and the native driver should be responsible for the sysfs attributes.
Yup. 
> Because my approach is made by reading the ACPI spec, I'm not sure what
> should be done in the native driver and what should be done in the
> generic driver at the beginning.
For now I'd be tempted to keep as little as possible in the generic
driver and start moving stuff in only once a particular element
is verified to be relevant to almost every device.
> 
> thanks for pointing this out.
> 
>>  For example, I'm not convinced it makes sense for
>> drivers to have to have a get_adjustment attribute or indeed even
>> necessarily have a direct illuminance attribute (deriving the relevant
>> value may be a case of userspace combining several associated
>> readings).
> 
> what these associated readings are?
> I think we can define some optional attributes besides the required
> attributes.
Agreed.
> but we should make clear what is necessary for an ALS device, and what
> optional features it may support.
Yes,

For now I'd be inclined to stick to the ability to read illuminance 
in some specified unit.  Perhaps some other flag to specify something
about the frequency range of the sensor?

Maybe similar to hwmon approach allowing for multiple readings of a given
type?

illumiance[n]
illuminmance_type[n]

Everything else optional.  Actually I'm personally of the view everything
should be optional as long as any close matches in functionality are given
the same names.  It's up to userspace to figure out of the device supports
what it wants to use.  Things that I can envisioned not meeting the above
but still being appropriately placed within als:

* Device that simply tells you whether ambient is greater or less than
backlight value + some offset (perhaps with controllable offset).

* Weird and wonderful sensor types we can't even envision at this point in
time!

Perhaps the trick is to document the 'required' parameters as being those
required without consulting the maintainer / mailing list then they can be
adjusted over time as more device drivers are written.

This is definitely the sort of driver
where the fine grained power management stuff should be encouraged.  After
all not a lot of point in having them powered up if the screen is off!
As ever whether people put this stuff in is up to them.  Others can always
submit patches adding it drivers at a later date.

> 
> thanks,
> rui

Thanks for your work on this. Will certainly be nice to clean up the current
mess with light sensors all over the place!

Jonathan
 

--
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

Re: RFC: Light sensors, unifying current options?

[Zhang Rui]

On Mon, 2009-09-07 at 19:42 +0800, Jonathan Cameron wrote:
> Zhang Rui wrote:
> > Hi, Jonathan,
> > 
> > On Thu, 2009-09-03 at 21:51 +0800, Jonathan Cameron wrote:
> >> Dear All,
> >>
> >> This thread is a follow up to (amongst others)
> >>
> >> [lm-sensors] Ambient Light sensor for Intersil-ISL29020 device
> >>
> >> Currently there are a number of light sensor drivers either in the
> >> mainline kernel, posted to various mailing lists or sitting in various
> >> testing trees.
> >>
> >> For example.
> >>
> >> Intersil ISL29020
> >> http://www.intersil.com/products/deviceinfo.asp?pn=ISL29020 driver
> >> posted by Kalan Trisal to lm-sensors (as hwmon device rejected for
> >> being out of subsystem scope)
> >> http://lists.lm-sensors.org/pipermail/lm-sensors/2009-September/026575.html
> >>
> >> ALS_sysfs class and als_acpi driver (V6 posted to lkml earlier this week).
> >> http://lkml.org/lkml/2009/9/1/38
> >>
> >> TSL2561 under the industrial I/O Framework. (in Greg KH's tree, will
> >> being in staging after merge window - there due to lack of review
> >> more than any known problems.)
> >> http://www.farnell.com/datasheets/49661.pdf
> >> http://lkml.org/lkml/2009/7/2/189
> >>
> >> TSL2550 under i2c/chips/ which as a location is going away.
> >> http://www.farnell.com/datasheets/48715.pdf
> >>
> >> (any others people know of?)
> >>
> >> Two big questions:
> >>
> >> * Are there sufficient shared characteristics between these devices to
> >> all for a unified framework? (would certainly be nice!)
> >>
> >> * What applications are they used for? This will drive the question
> >> of what functionality is desirable. (particularly do we need an event
> >> infrastructure or not).
> >>
> >> To sumarize the functionality currently provided by the above drivers
> >> (or that should probably be added)
> >>
> >> ISL29020:
> >> * sensing_range
> >> * lux_level
> >> * power state (should probably move over to the new power management
> >>   framework)
> >>
> >> ALS:
> >> * illuminance (equivalent of lux_level)
> >> * adjustment (I don't follow the purpose of this, but then I don't know
> >> anything about how this is being used!)
> >>
> > adjustment is a percentage value used by userspace to adjust the display
> > backlight.
> > 
> > According to the ACPI spec, ACPI ALS device has "ambient light
> > illuminance to display luminance" mappings that can be used by an OS to
> > calibrate its ambient light policy for a given sensor configuration. The
> > OS can use this information to extrapolate an ALS response curve. i.e.
> > ACPI ALS knows what to do when ambient light illuminance changes, but it
> > won't change the backlight. Instead, it exports this info to user space
> > via the "adjustment" attribute.
> > user space applications can get this value and change the display
> > brightness via the backlight sysfs I/F.
> Is this conversion entirely independent of the physical configuration of
> the sensor?

yes.

>   I can sort of imagine cases where some direct pickup from
> the backlight occurs alongside the ambient and some where none does.
> Fair enough if not.
> 
do you mean that on some platforms, als may change the backlight
directly when ambient light changes?

> > IMO, the ALS device should do the following work:
> > 1. catch the ambient light illuminance change.
> Sometimes this is more complex with the ability to separately read light
> levels in different frequency ranges (e.g. infrared and visible)  Still
> this value can usually be derived.

so, "illuminance" should be a required attribute, right?

> > 2. tell the userspace what to do with this change.
> > isn't this true for the other ALS devices in this thread?
> None of the others (other than the additional asus one mentioned
> later in this thread - which I haven't looked at) have any concept of what userspace
> should do with the value. They simply measure it (and supply appropriate
> threshold interrupts etc)

then what does OS do upon these interrupts? nothing?
who is responsible for changing the backlight, BIOS?

> > 
> >> TSL2561
> >> * infrared (raw value)
> >> * broad spectrum (raw value)
> >> (I'm of the view any derived values should probably be done in userspace)
> >> This one is under IIO at the moment for two reasons.
> >> 1) I hit the same issue of no suitable subsystem, but for a much larger
> >> class of sensor devices. Light sensors are just one example (that's not
> >> to say I mind hiving this lot off to a system of their own).
> >> 2) To provide an event interface (which I haven't yet done)
> >> Driver should also include:
> >> * integration time
> >> * gain control 
> >>
> >> TSL2550
> >> * power state
> >> * operating mode
> >> * lux (actually calculated from two separate readings as
> >> per the tsl2561 but the are not available to userspace)
> >>
> >> Applications:
> >>
> >> 1) Backlight intensity type apps (guessing that covers most people)
> >> 2) Environmental monitoring apps (the crossbow imb400 imote2 daughter
> >> board I'm using doesn't have any screen or other direct interface, its
> >> simply a lightweight sensor platform).
> >> 3) High speed apps (all current sensors are pretty slow so this isn't
> >> yet relevant).
> >>
> >> My personal feelings is that the IIO is overkill for these types
> >> of sensors (slow update rate, tsl2550 takes 400ms, tsl2561 12-400ms)
> >> unless we want the event handling infrastructure. I'm inclined to
> >> say it is unecessary given the same result could be obtained by
> >> polling only a few times a second.
> >>
> > agree.
> > this is not a problem to ACPI ALS device.
> > ACPI sends a notification to the ACPI ALS device when illuminance is
> > changed.
> > 
> >> My comments on ALS may be wrong or misleading as they are based on a
> >> brief read of the code (please correct me!)  A lot of the
> >> infrastructure is only necessary if we have in kernel users
> >>  (and at
> >> the moment the functionality doesn't appear to be there for any such
> >> users to acquire access to these sensors in the first place.  For
> >> example, the approach used by hwmon of letting drivers define their
> >> own attributes seems to me to be more easily extendable than ALS' use
> >> of an ops structure.
> > 
> > I agree that the ops structure is unnecessary.
> > To make the als_sys class more generic, we just need to
> > 1. defines the generic attributes that the native ALS driver must
> > follow.
> > 2. registers an als device in the als_sys class.
> > and the native driver should be responsible for the sysfs attributes.
> Yup. 
> > Because my approach is made by reading the ACPI spec, I'm not sure what
> > should be done in the native driver and what should be done in the
> > generic driver at the beginning.
> For now I'd be tempted to keep as little as possible in the generic
> driver and start moving stuff in only once a particular element
> is verified to be relevant to almost every device.
> > 
> > thanks for pointing this out.
> > 
> >>  For example, I'm not convinced it makes sense for
> >> drivers to have to have a get_adjustment attribute or indeed even
> >> necessarily have a direct illuminance attribute (deriving the relevant
> >> value may be a case of userspace combining several associated
> >> readings).
> > 
> > what these associated readings are?
> > I think we can define some optional attributes besides the required
> > attributes.
> Agreed.
> > but we should make clear what is necessary for an ALS device, and what
> > optional features it may support.
> Yes,
> 
> For now I'd be inclined to stick to the ability to read illuminance 
> in some specified unit.  Perhaps some other flag to specify something
> about the frequency range of the sensor?
> 
ACPI ALS doesn't support this.
what's this frequency range used for?
is there some reason that userspace should know this?

> Maybe similar to hwmon approach allowing for multiple readings of a given
> type?
> 
> illumiance[n]
> illuminmance_type[n]

what's the value in illuminance_type, infrared/visible/ultraviolet?

> 
> Everything else optional.  Actually I'm personally of the view everything
> should be optional as long as any close matches in functionality are given
> the same names.  It's up to userspace to figure out of the device supports
> what it wants to use.  Things that I can envisioned not meeting the above
> but still being appropriately placed within als:
> 
> * Device that simply tells you whether ambient is greater or less than
> backlight value + some offset (perhaps with controllable offset).
> 
sorry, I don't understand this.
IMO, ambient and backlight are two different concepts.
that's why ACPI spec defines "ambient light illuminance to display
luminance" mappings.

> * Weird and wonderful sensor types we can't even envision at this point in
> time!
> 
> Perhaps the trick is to document the 'required' parameters as being those
> required without consulting the maintainer / mailing list then they can be
> adjusted over time as more device drivers are written.
> 
> This is definitely the sort of driver
> where the fine grained power management stuff should be encouraged.  After
> all not a lot of point in having them powered up if the screen is off!
> As ever whether people put this stuff in is up to them.  Others can always
> submit patches adding it drivers at a later date.
> 
agree.

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

Re: RFC: Light sensors, unifying current options?

[Jonathan Cameron]

Zhang Rui wrote:
> On Mon, 2009-09-07 at 19:42 +0800, Jonathan Cameron wrote:
>> Zhang Rui wrote:
>>> Hi, Jonathan,
>>>
>>> On Thu, 2009-09-03 at 21:51 +0800, Jonathan Cameron wrote:
>>>> Dear All,
>>>>
>>>> This thread is a follow up to (amongst others)
>>>>
>>>> [lm-sensors] Ambient Light sensor for Intersil-ISL29020 device
>>>>
>>>> Currently there are a number of light sensor drivers either in the
>>>> mainline kernel, posted to various mailing lists or sitting in various
>>>> testing trees.
>>>>
>>>> For example.
>>>>
>>>> Intersil ISL29020
>>>> http://www.intersil.com/products/deviceinfo.asp?pn=ISL29020 driver
>>>> posted by Kalan Trisal to lm-sensors (as hwmon device rejected for
>>>> being out of subsystem scope)
>>>> http://lists.lm-sensors.org/pipermail/lm-sensors/2009-September/026575.html
>>>>
>>>> ALS_sysfs class and als_acpi driver (V6 posted to lkml earlier this week).
>>>> http://lkml.org/lkml/2009/9/1/38
>>>>
>>>> TSL2561 under the industrial I/O Framework. (in Greg KH's tree, will
>>>> being in staging after merge window - there due to lack of review
>>>> more than any known problems.)
>>>> http://www.farnell.com/datasheets/49661.pdf
>>>> http://lkml.org/lkml/2009/7/2/189
>>>>
>>>> TSL2550 under i2c/chips/ which as a location is going away.
>>>> http://www.farnell.com/datasheets/48715.pdf
>>>>
>>>> (any others people know of?)
>>>>
>>>> Two big questions:
>>>>
>>>> * Are there sufficient shared characteristics between these devices to
>>>> all for a unified framework? (would certainly be nice!)
>>>>
>>>> * What applications are they used for? This will drive the question
>>>> of what functionality is desirable. (particularly do we need an event
>>>> infrastructure or not).
>>>>
>>>> To sumarize the functionality currently provided by the above drivers
>>>> (or that should probably be added)
>>>>
>>>> ISL29020:
>>>> * sensing_range
>>>> * lux_level
>>>> * power state (should probably move over to the new power management
>>>>   framework)
>>>>
>>>> ALS:
>>>> * illuminance (equivalent of lux_level)
>>>> * adjustment (I don't follow the purpose of this, but then I don't know
>>>> anything about how this is being used!)
>>>>
>>> adjustment is a percentage value used by userspace to adjust the display
>>> backlight.
>>>
>>> According to the ACPI spec, ACPI ALS device has "ambient light
>>> illuminance to display luminance" mappings that can be used by an OS to
>>> calibrate its ambient light policy for a given sensor configuration. The
>>> OS can use this information to extrapolate an ALS response curve. i.e.
>>> ACPI ALS knows what to do when ambient light illuminance changes, but it
>>> won't change the backlight. Instead, it exports this info to user space
>>> via the "adjustment" attribute.
>>> user space applications can get this value and change the display
>>> brightness via the backlight sysfs I/F.
>> Is this conversion entirely independent of the physical configuration of
>> the sensor?
> 
> yes.
> 
>>   I can sort of imagine cases where some direct pickup from
>> the backlight occurs alongside the ambient and some where none does.
>> Fair enough if not.
>>
> do you mean that on some platforms, als may change the backlight
> directly when ambient light changes?
Not really, more a question of where the sensor is.  In some cases
it might be near enough the screen as to pick up direct lighting from
it and others it might be no where near and hence that doesn't occur.
I guess that may be something for a future occasion if it actually occurs!
No point in implementing code for something hypothetical. 
> 
>>> IMO, the ALS device should do the following work:
>>> 1. catch the ambient light illuminance change.
>> Sometimes this is more complex with the ability to separately read light
>> levels in different frequency ranges (e.g. infrared and visible)  Still
>> this value can usually be derived.
> 
> so, "illuminance" should be a required attribute, right?
Yes,
> 
>>> 2. tell the userspace what to do with this change.
>>> isn't this true for the other ALS devices in this thread?
>> None of the others (other than the additional asus one mentioned
>> later in this thread - which I haven't looked at) have any concept of what userspace
>> should do with the value. They simply measure it (and supply appropriate
>> threshold interrupts etc)
> 
> then what does OS do upon these interrupts? nothing?
Depends on the application.  Worth remembering not all such sensors are
used purely for back light control.  Down the line we might want a means
of passing these up to userspace for any apps that are interested.
I'd be inclined to leave them out for now though. I don't think any of
the current drivers implement them.

> who is responsible for changing the backlight, BIOS?
Back to the point that not all these sensors have anything to do with
back lights.  They are ultimately things sensing light levels. Whilst
using that to control back lights is one major application it isn't
the only one.
 
>>>> TSL2561
>>>> * infrared (raw value)
>>>> * broad spectrum (raw value)
>>>> (I'm of the view any derived values should probably be done in userspace)
>>>> This one is under IIO at the moment for two reasons.
>>>> 1) I hit the same issue of no suitable subsystem, but for a much larger
>>>> class of sensor devices. Light sensors are just one example (that's not
>>>> to say I mind hiving this lot off to a system of their own).
>>>> 2) To provide an event interface (which I haven't yet done)
>>>> Driver should also include:
>>>> * integration time
>>>> * gain control 
>>>>
>>>> TSL2550
>>>> * power state
>>>> * operating mode
>>>> * lux (actually calculated from two separate readings as
>>>> per the tsl2561 but the are not available to userspace)
>>>>
>>>> Applications:
>>>>
>>>> 1) Backlight intensity type apps (guessing that covers most people)
>>>> 2) Environmental monitoring apps (the crossbow imb400 imote2 daughter
>>>> board I'm using doesn't have any screen or other direct interface, its
>>>> simply a lightweight sensor platform).
>>>> 3) High speed apps (all current sensors are pretty slow so this isn't
>>>> yet relevant).
>>>>
>>>> My personal feelings is that the IIO is overkill for these types
>>>> of sensors (slow update rate, tsl2550 takes 400ms, tsl2561 12-400ms)
>>>> unless we want the event handling infrastructure. I'm inclined to
>>>> say it is unecessary given the same result could be obtained by
>>>> polling only a few times a second.
>>>>
>>> agree.
>>> this is not a problem to ACPI ALS device.
>>> ACPI sends a notification to the ACPI ALS device when illuminance is
>>> changed.
>>>
>>>> My comments on ALS may be wrong or misleading as they are based on a
>>>> brief read of the code (please correct me!)  A lot of the
>>>> infrastructure is only necessary if we have in kernel users
>>>>  (and at
>>>> the moment the functionality doesn't appear to be there for any such
>>>> users to acquire access to these sensors in the first place.  For
>>>> example, the approach used by hwmon of letting drivers define their
>>>> own attributes seems to me to be more easily extendable than ALS' use
>>>> of an ops structure.
>>> I agree that the ops structure is unnecessary.
>>> To make the als_sys class more generic, we just need to
>>> 1. defines the generic attributes that the native ALS driver must
>>> follow.
>>> 2. registers an als device in the als_sys class.
>>> and the native driver should be responsible for the sysfs attributes.
>> Yup. 
>>> Because my approach is made by reading the ACPI spec, I'm not sure what
>>> should be done in the native driver and what should be done in the
>>> generic driver at the beginning.
>> For now I'd be tempted to keep as little as possible in the generic
>> driver and start moving stuff in only once a particular element
>> is verified to be relevant to almost every device.
>>> thanks for pointing this out.
>>>
>>>>  For example, I'm not convinced it makes sense for
>>>> drivers to have to have a get_adjustment attribute or indeed even
>>>> necessarily have a direct illuminance attribute (deriving the relevant
>>>> value may be a case of userspace combining several associated
>>>> readings).
>>> what these associated readings are?
>>> I think we can define some optional attributes besides the required
>>> attributes.
>> Agreed.
>>> but we should make clear what is necessary for an ALS device, and what
>>> optional features it may support.
>> Yes,
>>
>> For now I'd be inclined to stick to the ability to read illuminance 
>> in some specified unit.  Perhaps some other flag to specify something
>> about the frequency range of the sensor?
>>
> ACPI ALS doesn't support this.
> what's this frequency range used for?
Typically a device has 2 sensors, one in infrared one covering infrared and visible.
You can use them to produce a single reading of visible light level (probably
corresponds to your ALS illuminance) but if the functionality is there to read
them independently I'd like to support it.
> is there some reason that userspace should know this?
Depends on application.  If you are looking at an environmental monitoring case
then sometimes yes.
> 
>> Maybe similar to hwmon approach allowing for multiple readings of a given
>> type?
>>
>> illumiance[n]
>> illuminmance_type[n]
> 
> what's the value in illuminance_type, infrared/visible/ultraviolet?
Probably a mask allowing for a given sensor to cover several of these. 
Hence if you have an infrared and an visible + infrared, userspace can
pull the visible out.  Or we could do that separation in kernel, so
in this case export

visible, visible+infrared and infrared.

Perhaps we might do it via sysfs naming instead.

illuminance[n] - defined to be visible (which is what term means anyway)

infrared[n] - hmm.. will be in different units to illuminance - perhaps we leave
these raw with suitable documentation?

illuminance_and_infrared[n]

On the tsl2561, if we were to implement threshold interrupts the interestingly
don't apply on illuminance but rather on the illuminance + infrared sensor,
which will make setting it from userspace 'interesting'.
> 
>> Everything else optional.  Actually I'm personally of the view everything
>> should be optional as long as any close matches in functionality are given
>> the same names.  It's up to userspace to figure out of the device supports
>> what it wants to use.  Things that I can envisioned not meeting the above
>> but still being appropriately placed within als:
>>
>> * Device that simply tells you whether ambient is greater or less than
>> backlight value + some offset (perhaps with controllable offset).
>>
> sorry, I don't understand this.
> IMO, ambient and backlight are two different concepts.
> that's why ACPI spec defines "ambient light illuminance to display
> luminance" mappings.
Agreed, though that doesn't mean every chip will allow you to read illuminance.
You may well get things that effectively provide you with a value part way through
the calculations you are doing in userspace to then adjust the display brightness.

The only point of these examples, is we don't want to enforce any attribute in the
code, rather we want to imply which are required in documentation.  Thus if something
comes along we haven't considered, it is possible to support it and merely change
documentation to reflect this rather than needing a change to the core.
> 
>> * Weird and wonderful sensor types we can't even envision at this point in
>> time!
>>
>> Perhaps the trick is to document the 'required' parameters as being those
>> required without consulting the maintainer / mailing list then they can be
>> adjusted over time as more device drivers are written.
>>
>> This is definitely the sort of driver
>> where the fine grained power management stuff should be encouraged.  After
>> all not a lot of point in having them powered up if the screen is off!
>> As ever whether people put this stuff in is up to them.  Others can always
>> submit patches adding it drivers at a later date.
>>
> agree.
>
So in summary, I think we are agreed that what we need is similar to hwmon,
with all drivers exporting illuminance.  If they are capable of
exporting other things all well and good.  The naming can be decided as and
when people post drivers with new functionality.

The other obvious question is whether it makes sense to cache values from sensors?
(as per many hwmon drivers)  These things are pretty slow, so there is no point in
taking a new reading unless there has been sufficient time for it to update.
Still this is again a documentation issue rather than core code, as it would be
down to the individual drivers.

Jonathan
--
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

Re: RFC: Light sensors, unifying current options?

[Zhang Rui]

On Wed, 2009-09-09 at 19:28 +0800, Jonathan Cameron wrote:
> Zhang Rui wrote:
> > On Mon, 2009-09-07 at 19:42 +0800, Jonathan Cameron wrote:
> >> Zhang Rui wrote:

> >
> >> Maybe similar to hwmon approach allowing for multiple readings of a given
> >> type?
> >>
> >> illumiance[n]
> >> illuminmance_type[n]
> >
> > what's the value in illuminance_type, infrared/visible/ultraviolet?
> Probably a mask allowing for a given sensor to cover several of these.
> Hence if you have an infrared and an visible + infrared, userspace can
> pull the visible out.  Or we could do that separation in kernel, so
> in this case export
> 
> visible, visible+infrared and infrared.
> 
> Perhaps we might do it via sysfs naming instead.
> 
> illuminance[n] - defined to be visible (which is what term means anyway)
> 
> infrared[n] - hmm.. will be in different units to illuminance - perhaps we leave
> these raw with suitable documentation?
> 
> illuminance_and_infrared[n]
> 
what's the unit of this?
what does the value of this file mean?
I think we should make sure that there are more sensors need this and
the same "visible + infrared" reading on different sensors stands for
the same ambient light environment.
Or else we should convert it to something generic. what do you think?

>   If they are capable of
> exporting other things all well and good.  The naming can be decided as and
> when people post drivers with new functionality.
> 

> The other obvious question is whether it makes sense to cache values from sensors?
> (as per many hwmon drivers)  These things are pretty slow, so there is no point in
> taking a new reading unless there has been sufficient time for it to update.
> Still this is again a documentation issue rather than core code, as it would be
> down to the individual drivers.
> 

I think this depends and it's the individual drivers responsibility to
keep these values valid.

For example, ACPI ALS can get a notification "whenever the lux reading
changes more than 10% (from the last reading that resulted in a
notification)". So it can use cache values because they're refreshed
from time to time.

But maybe there are some other sensors that don't have this asynchronous
notification so that the cached values don't have a chance to get
updated.
then we need to read the sensor every time the sysfs I/F is poked.

thanks,
rui

Re: RFC: Light sensors, unifying current options?

[Pavel Machek]

Hi!

> > Perhaps we might do it via sysfs naming instead.
> > 
> > illuminance[n] - defined to be visible (which is what term means anyway)
> > 
> > infrared[n] - hmm.. will be in different units to illuminance - perhaps we leave
> > these raw with suitable documentation?
> > 
> > illuminance_and_infrared[n]
> > 
> what's the unit of this?
> what does the value of this file mean?
> I think we should make sure that there are more sensors need this and
> the same "visible + infrared" reading on different sensors stands for
> the same ambient light environment.

Are you sure that makes sense? And is that even possible? I'd be
afraid that the sensors are not nearly accurate enough to give precise
values, and each sensor is going to take different parts of spectrum,
etc.

Yes, we should aim at having similar interface with similar values on
all the sensors, but lets not overdo it.

> But maybe there are some other sensors that don't have this asynchronous
> notification so that the cached values don't have a chance to get
> updated.
> then we need to read the sensor every time the sysfs I/F is poked.

I bet most sensors are like that. And no, we don't have to reread from
hw every time, we can just read and cache it once per second or
something.
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: RFC: Light sensors, unifying current options?

[Jonathan Cameron]

Pavel Machek wrote:
> Hi!
> 
>>> Perhaps we might do it via sysfs naming instead.
>>>
>>> illuminance[n] - defined to be visible (which is what term means anyway)
>>>
>>> infrared[n] - hmm.. will be in different units to illuminance - perhaps we leave
>>> these raw with suitable documentation?
>>>
>>> illuminance_and_infrared[n]
>>>
>> what's the unit of this?
>> what does the value of this file mean?
>> I think we should make sure that there are more sensors need this and
>> the same "visible + infrared" reading on different sensors stands for
>> the same ambient light environment.
> 
> Are you sure that makes sense? And is that even possible? I'd be
> afraid that the sensors are not nearly accurate enough to give precise
> values, and each sensor is going to take different parts of spectrum,
> etc.
> 
> Yes, we should aim at having similar interface with similar values on
> all the sensors, but lets not overdo it.
Agreed, if we match on the illuminance, pretty much everything else can
initially be chip dependent as long as they are sufficiently documented.
If certain things keep repeating then they can be standardized as and when
this occurs.
 
>> But maybe there are some other sensors that don't have this asynchronous
>> notification so that the cached values don't have a chance to get
>> updated.
>> then we need to read the sensor every time the sysfs I/F is poked.
> 
> I bet most sensors are like that. And no, we don't have to reread from
> hw every time, we can just read and cache it once per second or
> something.
The ones I've seen in embedded devices tend to have some sort of interrupt
system, but it's reasonably complicated to configure as you have to read the
sensor then write threshold values back to get the same sort of notifications
as acpi is doing.  Also, it's far from unusual for people not to bother connecting
the interrupt pin up in the first place so we can't have those drivers relying on its
presence.

I think we are getting to the point where the easiest way to work out further
issues is going to be in code as drivers get converted to the new subsystem.
I'm happy to do the two tsl drivers once the core as discussed in this thread
is in place.

Jonathan

Re: RFC: Light sensors, unifying current options?

[Jonathan Cameron]

Zhang Rui wrote:
> On Wed, 2009-09-09 at 19:28 +0800, Jonathan Cameron wrote:
>> Zhang Rui wrote:
>>> On Mon, 2009-09-07 at 19:42 +0800, Jonathan Cameron wrote:
>>>> Zhang Rui wrote:
> 
>>>> Maybe similar to hwmon approach allowing for multiple readings of a given
>>>> type?
>>>>
>>>> illumiance[n]
>>>> illuminmance_type[n]
>>> what's the value in illuminance_type, infrared/visible/ultraviolet?
>> Probably a mask allowing for a given sensor to cover several of these.
>> Hence if you have an infrared and an visible + infrared, userspace can
>> pull the visible out.  Or we could do that separation in kernel, so
>> in this case export
>>
>> visible, visible+infrared and infrared.
>>
>> Perhaps we might do it via sysfs naming instead.
>>
>> illuminance[n] - defined to be visible (which is what term means anyway)
>>
>> infrared[n] - hmm.. will be in different units to illuminance - perhaps we leave
>> these raw with suitable documentation?
>>
>> illuminance_and_infrared[n]
>>
> what's the unit of this?
Indeed, there in lies a problem.  I guess we export any we that don't fit
within the standard set (e.g. that infrared sensor etc) as raw values with
suitable documentation.  If people want to use them it is up to them, but they
shouldn't assume that two different sensors give equivalent readings.
> what does the value of this file mean?
It's really just an adc count.
> I think we should make sure that there are more sensors need this and
> the same "visible + infrared" reading on different sensors stands for
> the same ambient light environment.
>From what I've encountered it is a fairly common way of getting illuminance,
the question is merely whether the relevant processing is done on the sensor
or in software.  If in software the two values are available, in hardware
they probably only export illuminance.
> Or else we should convert it to something generic. what do you think?
Would be nice to be generic, but given they exact frequency ranges will change
between different sensors even if they do use this approach, I'm not sure what
it would be.
> 
>>   If they are capable of
>> exporting other things all well and good.  The naming can be decided as and
>> when people post drivers with new functionality.
>>
> 
>> The other obvious question is whether it makes sense to cache values from sensors?
>> (as per many hwmon drivers)  These things are pretty slow, so there is no point in
>> taking a new reading unless there has been sufficient time for it to update.
>> Still this is again a documentation issue rather than core code, as it would be
>> down to the individual drivers.
>>
> 
> I think this depends and it's the individual drivers responsibility to
> keep these values valid.
Agreed.
> 
> For example, ACPI ALS can get a notification "whenever the lux reading
> changes more than 10% (from the last reading that resulted in a
> notification)". So it can use cache values because they're refreshed
> from time to time.
> 
> But maybe there are some other sensors that don't have this asynchronous
> notification so that the cached values don't have a chance to get
> updated.
> then we need to read the sensor every time the sysfs I/F is poked.
Some sensors, e.g. tsl2561 use an internal clock to do sampling and hence
have some predefined period in which the reading taken doesn't change.
Perhaps exporting said period would be useful to userspace?

Jonathan

Re: RFC: Light sensors, unifying current options?

[Zhang Rui]

On Thu, 2009-09-10 at 18:05 +0800, Jonathan Cameron wrote:
> Zhang Rui wrote:
> > On Wed, 2009-09-09 at 19:28 +0800, Jonathan Cameron wrote:
> >> Zhang Rui wrote:
> >>> On Mon, 2009-09-07 at 19:42 +0800, Jonathan Cameron wrote:
> >>>> Zhang Rui wrote:
> > 
> >>>> Maybe similar to hwmon approach allowing for multiple readings of a given
> >>>> type?
> >>>>
> >>>> illumiance[n]
> >>>> illuminmance_type[n]
> >>> what's the value in illuminance_type, infrared/visible/ultraviolet?
> >> Probably a mask allowing for a given sensor to cover several of these.
> >> Hence if you have an infrared and an visible + infrared, userspace can
> >> pull the visible out.  Or we could do that separation in kernel, so
> >> in this case export
> >>
> >> visible, visible+infrared and infrared.
> >>
> >> Perhaps we might do it via sysfs naming instead.
> >>
> >> illuminance[n] - defined to be visible (which is what term means anyway)
> >>
> >> infrared[n] - hmm.. will be in different units to illuminance - perhaps we leave
> >> these raw with suitable documentation?
> >>
> >> illuminance_and_infrared[n]
> >>
> > what's the unit of this?
> Indeed, there in lies a problem.  I guess we export any we that don't fit
> within the standard set (e.g. that infrared sensor etc) as raw values with
> suitable documentation.  If people want to use them it is up to them, but they
> shouldn't assume that two different sensors give equivalent readings.

> > what does the value of this file mean?
> It's really just an adc count.
> > I think we should make sure that there are more sensors need this and
> > the same "visible + infrared" reading on different sensors stands for
> > the same ambient light environment.
> From what I've encountered it is a fairly common way of getting illuminance,
> the question is merely whether the relevant processing is done on the sensor
> or in software.  If in software the two values are available, in hardware
> they probably only export illuminance.
> > Or else we should convert it to something generic. what do you think?
> Would be nice to be generic, but given they exact frequency ranges will change
> between different sensors even if they do use this approach, I'm not sure what
> it would be.

so, if there are two sensors that support visible and infrared,
sensor 1:
illuminance:	xxx
infrared:	yyy
sensor 2:
illuminance:	xxx
infrared:	yyy

does this mean the ambient light of these two sensors are the same?

If no, I can't image how the userspace app uses these attributes?

> > 
> >>   If they are capable of
> >> exporting other things all well and good.  The naming can be decided as and
> >> when people post drivers with new functionality.
> >>
> > 
> >> The other obvious question is whether it makes sense to cache values from sensors?
> >> (as per many hwmon drivers)  These things are pretty slow, so there is no point in
> >> taking a new reading unless there has been sufficient time for it to update.
> >> Still this is again a documentation issue rather than core code, as it would be
> >> down to the individual drivers.
> >>
> > 
> > I think this depends and it's the individual drivers responsibility to
> > keep these values valid.
> Agreed.
> > 
> > For example, ACPI ALS can get a notification "whenever the lux reading
> > changes more than 10% (from the last reading that resulted in a
> > notification)". So it can use cache values because they're refreshed
> > from time to time.
> > 
> > But maybe there are some other sensors that don't have this asynchronous
> > notification so that the cached values don't have a chance to get
> > updated.
> > then we need to read the sensor every time the sysfs I/F is poked.
> Some sensors, e.g. tsl2561 use an internal clock to do sampling and hence
> have some predefined period in which the reading taken doesn't change.
> Perhaps exporting said period would be useful to userspace?
> 
I don't think so.
IMO, the tsl2561 driver should handle this, and there is no need to
annoy the user space.
i.e. users always assume the content of "illuminance" attribute is
valid, and it's the driver's job to make this true. We don't need to
document this.
what do you think?

thanks,
rui

Re: RFC: Light sensors, unifying current options?

[Jean Delvare]

On Fri, 11 Sep 2009 09:55:01 +0800, Zhang Rui wrote:
> On Thu, 2009-09-10 at 18:05 +0800, Jonathan Cameron wrote:
> > Some sensors, e.g. tsl2561 use an internal clock to do sampling and hence
> > have some predefined period in which the reading taken doesn't change.
> > Perhaps exporting said period would be useful to userspace?
> > 
> I don't think so.
> IMO, the tsl2561 driver should handle this, and there is no need to
> annoy the user space.
> i.e. users always assume the content of "illuminance" attribute is
> valid, and it's the driver's job to make this true. We don't need to
> document this.

I agree with Rui on this. Let's not delegate everything to user-space,
otherwise it's pointless to write kernel drivers in the first place.

-- 
Jean Delvare

Re: RFC: Light sensors, unifying current options?

[Jonathan Cameron]

Jean Delvare wrote:
> On Fri, 11 Sep 2009 09:55:01 +0800, Zhang Rui wrote:
>> On Thu, 2009-09-10 at 18:05 +0800, Jonathan Cameron wrote:
>>> Some sensors, e.g. tsl2561 use an internal clock to do sampling and hence
>>> have some predefined period in which the reading taken doesn't change.
>>> Perhaps exporting said period would be useful to userspace?
>>>
>> I don't think so.
>> IMO, the tsl2561 driver should handle this, and there is no need to
>> annoy the user space.
>> i.e. users always assume the content of "illuminance" attribute is
>> valid, and it's the driver's job to make this true. We don't need to
>> document this.
> 
> I agree with Rui on this. Let's not delegate everything to user-space,
> otherwise it's pointless to write kernel drivers in the first place.
Agreed, if anyone has an application they can add the relevant hooks when
they need them.

Re: RFC: Light sensors, unifying current options?

[Jonathan Cameron]

> 
> so, if there are two sensors that support visible and infrared,
> sensor 1:
> illuminance:	xxx
> infrared:	yyy
> sensor 2:
> illuminance:	xxx
> infrared:	yyy
> 
> does this mean the ambient light of these two sensors are the same?
If the values are the same, then probably up to variations in the
sensitivity of the two sensors (and things like calibration etc
afterall we don't really have a ready consistent measure for infrared
in the same way as the perception scaled illuminance)

It's entirely possible that two sensors on a given device might get
different readings, both of which are useful. e.g. one of those
phones with a screen on the outside then a flip lid with another one
underneath for example. 

> 
> If no, I can't image how the userspace app uses these attributes?
That depends if your aim is to use them to control the brightness of
a screen and similar, or for example to use them for environmental
monitoring.

I agree with Jean (next email) in many ways that it is useful to
hide some complexity within a given driver (so as to provide
consistent interfaces etc to userspace.) However, if we can
also provide device specific access to other features of the device
I see no reason not to make them available.  As already stated, there
are several applications where that infrared measurement is useful
even if only available as a raw adc count. (typically you'd have a
calibration function from measured values of what ever you care about
anyway).

As long as we have illuminance (the generally useful one that all such
sensors seem to provide) then any other values the driver wants to export
are fine as long as we maintain decent documentation of what they are.

Jonathan