Date:	Fri, 18 May 2012 18:34:01 +0100
From:	Jonathan Cameron <jic23@...nel.org>
To:	Johan Hovold <jhovold@...il.com>
CC:	Jonathan Cameron <jic23@....ac.uk>, Rob Landley <rob@...dley.net>,
	Richard Purdie <rpurdie@...ys.net>,
	Samuel Ortiz <sameo@...ux.intel.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Florian Tobias Schandinat <FlorianSchandinat@....de>,
	Arnd Bergmann <arnd@...db.de>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Mark Brown <broonie@...nsource.wolfsonmicro.com>,
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-iio@...r.kernel.org, devel@...verdev.osuosl.org,
	linux-fbdev@...r.kernel.org
Subject: Re: [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver

On 05/18/2012 01:27 PM, Johan Hovold wrote:
> On Wed, May 16, 2012 at 03:21:14PM +0100, Jonathan Cameron wrote:
>> On 5/16/2012 2:05 PM, Johan Hovold wrote:
>>> On Tue, May 15, 2012 at 09:00:46PM +0100, Jonathan Cameron wrote:
>>>> On 05/15/2012 05:44 PM, Johan Hovold wrote:
>>>>> On Tue, May 08, 2012 at 02:47:19PM +0100, Jonathan Cameron wrote:
>>>>>> On 5/3/2012 5:36 PM, Johan Hovold wrote:
>>>>>>> On Thu, May 03, 2012 at 12:40:10PM +0100, Jonathan Cameron wrote:
>>>>>>>> On 5/3/2012 11:26 AM, Johan Hovold wrote:
>>>>>>>>> Add sub-driver for the ambient light sensor interface on National
>>>>>>>>> Semiconductor / TI LM3533 lighting power chips.
>>>>>>>>>
>>>>>>>>> The sensor interface can be used to control the LEDs and backlights of
>>>>>>>>> the chip through defining five light zones and three sets of
>>>>>>>>> corresponding brightness target levels.
>>>>>>>>>
>>>>>>>>> The driver provides raw and mean adc readings along with the current
>>>>>>>>> light zone through sysfs. A threshold event can be generated on zone
>>>>>>>>> changes.
>>>>>>>> Code is fine.  Pretty much all my comments are to do with the interface.
>>>>>>> [...]
>>>>>>>
>>>>>>>>> diff --git a/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
>>>>>>>>> new file mode 100644
>>>>>>>>> index 0000000..9849d14
>>>>>>>>> --- /dev/null
>>>>>>>>> +++ b/drivers/staging/iio/Documentation/sysfs-bus-iio-light-lm3533-als
>>>>>>>>> @@ -0,0 +1,62 @@
>>>>>>>>> +What:		/sys/bus/iio/devices/iio:deviceX/gain
>>>>>>>>> +Date:		April 2012
>>>>>>>>> +KernelVersion:	3.5
>>>>>>>>> +Contact:	Johan Hovold<jhovold@...il.com>
>>>>>>>>> +Description:
>>>>>>>>> +		Set the ALS gain-resistor setting (0..127) for analog input
>>>>>>>>> +		mode, where
>>>>>>>>> +
>>>>>>>>> +		0000000 - ALS input is high impedance
>>>>>>>>> +		0000001 - 200kOhm (10uA at 2V full-scale)
>>>>>>>>> +		0000010 - 100kOhm (20uA at 2V full-scale)
>>>>>>>>> +		...
>>>>>>>>> +		1111110 - 1.587kOhm (1.26mA at 2V full-scale)
>>>>>>>>> +		1111111 - 1.575kOhm (1.27mA at 2V full-scale)
>>>>>>>>> +
>>>>>>>>> +		R_als = 2V / (10uA * gain)	(gain>    0)
>>>>>>>> Firstly, no magic numbers.  These are definitely magic.
>>>>>>> Not that magic as they're clearly documented (in code and public
>>>>>>> datasheets), right? What would you prefer instead?
>>>>>> The numbers on the right of the - look good to me though then this isn't
>>>>>> a gain.  (200kohm) and the infinite element is annoying.  Why not
>>>>>> compute the actual gains?
>>>>>> Gain = (Rals*10e-6)/2  and use those values?  Yes you will have to do
>>>>>> a bit of fixed point maths in the driver but the advantage is you'll
>>>>>> have real values that are standardizable across multiple devices
>>>>>> and hence allow your device to be operated by generic userspace
>>>>>> code.  Welcome to standardising interfaces - my favourite occupation ;)
>>>>>>
>>>>>>>> Secondly see in_illuminance0_scale for a suitable existing attribute.
>>>>>>> I didn't consider scale to be appropriate given the following
>>>>>>> documentation (e.g, for in_voltageY_scale):
>>>>>> sorry  I just did this to someone else in another review (so I'm
>>>>>> consistently wrong!)
>>>>>>
>>>>>> in_voltageY_calibscale is what I should have said.  That one applies a
>>>>>> scaling before the raw reading is generated (so in hardware).
>>>>>
>>>>> Ok, then calibscale is the appropriate attribute for the resistor
>>>>> setting. But as this is a device-specific hardware-calibration setting
>>>>> I would suggest using the following interface:
>>>>>
>>>>> What:		/sys/bus/iio/devices/iio:deviceX/in_illuminance_calibscale
>>>>> Description:
>>>>> 		Set the ALS calibration scale (internal resistors) for
>>>>> 		analog input mode, where the scale factor is the current in uA
>>>>> 		at 2V full-scale (10..1270, 10uA step), that is,
>>>>>
>>>>> 		R_als = 2V / in_illuminance_calibscale
>>>>>
>>>>> 		This setting is ignored in PWM mode.
>>>> This is a generic element that really ought to just fit in with the
>>>> equivalent in sysfs-bus-iio for calibscan.  It's a ratio, so it should
>>>> be unit free for starters.
>>>
>>> I'm starting to doubt that calibscale is really appropriate in this case.
>>>
>>> For starters, the description in sysfs-bus-iio doesn't really apply:
>>>
>>> 	"Hardware applied calibration scale factor. (assumed to fix
>>> 	 production inaccuracies)."
>> Hmm.. if you really don't like this, Michael Hennerich had a case
>> where this made even less sense, so we now have hardwaregain.
>> Use that if you like...
> 
> I really think that this should remain a device specific attribute as I
> originally suggested. It's an integration parameter that needs to be set
> precisely depending on the actual hardware setup (which analog light
> sensor and other external components). 
Then it shouldn't be exposed to userspace.  If there is reason to vary
it from userspace then it is a calibration parameter and should be
treated like the other ones we have, if not it should be done from
dt or platform data.
> 
> The lm3533 also supports two types of light sensors: pwm- and analog-
> output ones. The resistor select settings only applies when in analog
> mode as the input is always high impedance otherwise. Thus a generic
> attribute (such as calibscale or hardware gain) shouldn't be used as it
> will have no effect whatsoever in PWM-mode.
> 
> I'm thus back at my original proposal, albeit with a different name (I
> think a lot of this discussion could have been avoided had I not
> misnamed the parameter "gain"): 
> 
> What:		/sys/bus/iio/devices/iio:deviceX/r_select
> Description:
> 		Set the ALS internal pull-down resistor for analog input mode
> 		(1..127), such that,
> 
> 		R_als = 200000 / r_select	(ohm)
> 
> 		This setting is ignored in PWM-mode (input is always high
> 		impedance in PWM-mode).
> 
> I don't think much is gained from using ohm as the unit: it just adds
> complexity and the selected resistor setting will likely not match the
> input value anyway. It's better that the chip integrators have full
> control over which resistor setting is actually used so that it matches
> external components.
This smacks of something that should never be exposed to users.
I'd hide it away in platform data.
> 
> 
>>> The resistor setting of the lm3533 is about fitting an external analog
>>> light sensor to the lm3533 als interface (which is basically just an adc
>>> with some extra logic), that is, it is used to match the output current
>>> of the chosen sensor so that the ADC measures 2V at full LUX.
>>>
>>> It's not a setting to calibrate "inaccuracies", but rather an
>>> integration parameter that is set once when the characteristics of the
>>> light sensor is known. (Sure, it could be used later to increase
>>> sensitivity as well, but the main purpose is to fit a new light sensor
>>> to a generic input interface.)
>>>
>>>>> [...]
>>>>>
>>>>>>>>> +What:		/sys/bus/iio/devices/iio:deviceX/target[m]_[n]
>>>>>>>>> +Date:		April 2012
>>>>>>>>> +KernelVersion:	3.5
>>>>>>>>> +Contact:	Johan Hovold<jhovold@...il.com>
>>>>>>>>> +Description:
>>>>>>>>> +		Set the target brightness for ALS-mapper m in light zone n
>>>>>>>>> +		(0..255), where m in 1..3 and n in 0..4.
>>>>>>>> Don't suppose you could do a quick summary of what these zones are
>>>>>>>> and why there are 3 ALS-mappers?  I'm not getting terribly far on a
>>>>>>>> quick look at the datasheet!
>>>>>>> Of course. The average adc readings are mapped to five light zones using
>>>>>>> eight zone boundary registers (4 boundaries with hysteresis) and a set
>>>>>>> of rules.
>>>>>> This is going to be fun.  We'll need the boundaries and attached
>>>>>> hysteresis attributes to fully specify these (nothing would indicate
>>>>>> that hysterisis is involved otherwise).
>>>>>
>>>>> You can't define the hysteresis explicitly with the lm3533 register
>>>>> interface, rather it's is defined implicitly in case threshY_falling is
>>>>> less than threshY_rasising.
>>>>>
>>>>> So the raising/falling attributes should be enough, right?
>>>> Nope, because they don't tell a general userspace application what is
>>>> going on.  Without hysterisis attributes it has no way of knowing there
>>>> is hysterisis present.
>>>
>>> Well an application could simply look at the difference between raising
>>> and falling to determine the hysteresis?
>> Only if it knows it has your sensor.  For other sensors it could be 
>> completely separate or not present.  If the parameter is missing 
>> assumption is that there is no hysterisis.
>>>
>>> It gets more complicated as the lm3533 allow the raising threshold to
>>> be lower than the falling. It appears the device is using whichever
>>> register is lower for the falling threshold. I guess I should compensate
>>> for this in the driver.
>> That's nasty.
>>>
>>> Furthermore, you can define threshold 1 to be lower than threshold 0,
>>> effectively preventing zone 1 to be reached. In this case, dropping
>>> below thres1_falling gives zone 0, and raising above thres1_raising gives
>>> zone 2. In particular, no threshold event is generated when
>>> thres0_{falling/raising} is passed in either direction. But perhaps this
>>> should just be documented as a feature/quirk of the device.
>> Seems sensible...
>>>
>>>> Feel free to make them read only though.
>>>
>>> So you're suggesting something like:
>>>
>>> 	events/in_illuminance0_threshY_falling_value
>>> 	events/in_illuminance0_threshY_raising_value
>>> 	events/in_illuminance0_threshY_hysteresis
>>>
>>> where hysteresis is a read-only attribute whose value is
>>> 	
>>> 	threshY_raising_value - threshY_falling_value
>> yes.  Annoying it may be but it matches existing interface.
> 
> I'm posting a v4 which includes the above proposal for resistor select.
> I've also added the hysteresis attributes as requested and fixed the
> device threshold quirkiness mentioned above (the device is using
> whichever register value is smaller as the falling threshold).
cool.  I'll take a look soon.
> 
> Thanks,
> Johan

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