Message-ID: <4AA4F194.1070507@cam.ac.uk>
Date:	Mon, 07 Sep 2009 12:42:12 +0100
From:	Jonathan Cameron <jic23@....ac.uk>
To:	Zhang Rui <rui.zhang@...el.com>
CC:	LKML <linux-kernel@...r.kernel.org>,
	Jean Delvare <khali@...ux-fr.org>,
	"alan@...ux.intel.com" <alan@...ux.intel.com>,
	"lenb@...nel.org" <lenb@...nel.org>, "pavel@....cz" <pavel@....cz>,
	"Cory T. Tusar" <ctusar@...eon-central.com>,
	"Trisal, Kalhan" <kalhan.trisal@...el.com>,
	linux-acpi <linux-acpi@...r.kernel.org>
Subject: Re: RFC: Light sensors, unifying current options?

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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists