linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jonathan Cameron <jic23@kernel.org>
To: Johan Hovold <jhovold@gmail.com>
Cc: Jonathan Cameron <jic23@cam.ac.uk>, Rob Landley <rob@landley.net>,
	Richard Purdie <rpurdie@rpsys.net>,
	Samuel Ortiz <sameo@linux.intel.com>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Florian Tobias Schandinat <FlorianSchandinat@gmx.de>,
	Arnd Bergmann <arnd@arndb.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Mark Brown <broonie@opensource.wolfsonmicro.com>,
	linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
	linux-iio@vger.kernel.org, devel@driverdev.osuosl.org,
	linux-fbdev@vger.kernel.org
Subject: Re: [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver
Date: Fri, 18 May 2012 18:34:01 +0100	[thread overview]
Message-ID: <4FB68809.2010009@kernel.org> (raw)
In-Reply-To: <20120518122725.GB20467@localhost>

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@gmail.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@gmail.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


  reply	other threads:[~2012-05-18 17:34 UTC|newest]

Thread overview: 83+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-04-20 15:30 [PATCH 0/4] mfd: add LM3533 lighting-power chip driver Johan Hovold
2012-04-20 15:30 ` [PATCH 1/4] mfd: add LM3533 lighting-power core driver Johan Hovold
2012-04-26 12:41   ` Mark Brown
2012-05-03 10:15     ` Johan Hovold
2012-05-03 10:22     ` Johan Hovold
2012-04-20 15:30 ` [PATCH 2/4] misc: add LM3533 ambient light sensor driver Johan Hovold
2012-04-20 15:57   ` Greg Kroah-Hartman
2012-04-20 17:28     ` Johan Hovold
2012-04-20 17:37       ` Greg Kroah-Hartman
2012-04-26 11:52         ` Johan Hovold
2012-04-20 15:30 ` [PATCH 3/4] leds: add LM3533 LED driver Johan Hovold
2012-04-20 16:10   ` Arnd Bergmann
2012-04-20 16:45     ` Johan Hovold
2012-04-20 15:30 ` [PATCH 4/4] backlight: add LM3533 backlight driver Johan Hovold
2012-05-03 10:26 ` [PATCH v2 0/4] mfd: add LM3533 lighting-power chip driver Johan Hovold
2012-05-03 10:26   ` [PATCH v2 1/4] mfd: add LM3533 lighting-power core driver Johan Hovold
2012-05-03 10:38     ` Mark Brown
2012-05-03 11:28       ` Johan Hovold
2012-05-03 11:38         ` Mark Brown
2012-05-03 15:00           ` Johan Hovold
2012-05-03 15:24             ` Mark Brown
2012-05-03 16:54               ` Johan Hovold
2012-05-03 16:57                 ` Mark Brown
2012-05-03 17:14                   ` Johan Hovold
2012-05-03 17:23                     ` Mark Brown
2012-05-03 17:31                       ` Johan Hovold
2012-05-09 14:42     ` Samuel Ortiz
2012-05-10 12:07       ` Johan Hovold
2012-05-10 12:11         ` [PATCH 1/2] mfd: lm3533: add boost frequency and ovp to platform data Johan Hovold
2012-05-10 12:11           ` [PATCH 2/2] mfd: lm3533: remove boost attributes Johan Hovold
2012-05-10 17:18         ` [PATCH 0/2] mfd: lm3533: update max-current interface Johan Hovold
2012-05-10 17:18           ` [PATCH 1/2] mfd: lm3533: remove unused max-current function Johan Hovold
2012-05-10 17:18           ` [PATCH 2/2] mfd: lm3533: use SI-units for max-current interface Johan Hovold
2012-05-11 13:32         ` [PATCH v2 1/4] mfd: add LM3533 lighting-power core driver Samuel Ortiz
2012-05-03 10:26   ` [PATCH v2 2/4] iio: add LM3533 ambient light sensor driver Johan Hovold
2012-05-03 11:40     ` Jonathan Cameron
2012-05-03 16:36       ` Johan Hovold
2012-05-08 13:47         ` Jonathan Cameron
2012-05-15 16:44           ` Johan Hovold
2012-05-15 20:00             ` Jonathan Cameron
2012-05-16 13:05               ` Johan Hovold
2012-05-16 14:21                 ` Jonathan Cameron
2012-05-18 12:27                   ` Johan Hovold
2012-05-18 17:34                     ` Jonathan Cameron [this message]
2012-05-18 17:57                       ` Johan Hovold
2012-05-19  8:04                         ` Jonathan Cameron
2012-05-15 16:46     ` [PATCH v3] iio: add LM3533 ambient-light-sensor driver Johan Hovold
2012-05-15 19:27       ` Andrew Morton
2012-05-15 20:00         ` Johan Hovold
2012-05-15 20:16           ` Jonathan Cameron
2012-05-18 13:07       ` [PATCH v4] " Johan Hovold
2012-05-19  8:48         ` Jonathan Cameron
2012-05-19 16:30           ` Johan Hovold
2012-05-19 13:26             ` Jonathan Cameron
2012-05-21  9:50           ` Johan Hovold
2012-05-21 16:37             ` Jonathan Cameron
2012-05-21 22:07               ` Johan Hovold
2012-05-22  7:13                 ` Jonathan Cameron
2012-05-22  9:09                   ` Johan Hovold
2012-05-22  9:15                     ` Jonathan Cameron
2012-05-22  7:45               ` Michael Hennerich
2012-05-22  7:49                 ` Jonathan Cameron
2012-05-22  8:11                   ` Michael Hennerich
2012-05-22  8:20                     ` Jonathan Cameron
2012-05-21 12:18         ` [PATCH v5] " Johan Hovold
2012-05-22  9:19           ` Jonathan Cameron
2012-05-22  9:40             ` Johan Hovold
2012-05-22 13:55               ` Greg Kroah-Hartman
2012-06-05  4:11               ` Greg Kroah-Hartman
2012-05-03 10:26   ` [PATCH v2 3/4] leds: add LM3533 LED driver Johan Hovold
2012-05-03 10:43     ` Mark Brown
2012-05-03 11:50       ` Johan Hovold
2012-05-03 14:51         ` Mark Brown
2012-05-03 16:46           ` Johan Hovold
2012-05-10 18:27     ` [PATCH v3] " Johan Hovold
2012-05-10 18:48       ` Andrew Morton
2012-05-11  9:54         ` Johan Hovold
2012-05-11 22:24           ` Andrew Morton
2012-05-14 10:25             ` Johan Hovold
2012-05-14 10:31       ` [PATCH v4] " Johan Hovold
2012-05-03 10:26   ` [PATCH v2 4/4] backlight: add LM3533 backlight driver Johan Hovold
2012-05-10 18:29     ` [PATCH v3] " Johan Hovold
2012-05-15 19:13       ` [PATCH v4] " Johan Hovold

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4FB68809.2010009@kernel.org \
    --to=jic23@kernel.org \
    --cc=FlorianSchandinat@gmx.de \
    --cc=akpm@linux-foundation.org \
    --cc=arnd@arndb.de \
    --cc=broonie@opensource.wolfsonmicro.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=jhovold@gmail.com \
    --cc=jic23@cam.ac.uk \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-fbdev@vger.kernel.org \
    --cc=linux-iio@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=rob@landley.net \
    --cc=rpurdie@rpsys.net \
    --cc=sameo@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).