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@xxxxxxxxx>
>>>>> +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.
>
> [...]
>
>>>>> +What: /sys/bus/iio/devices/iio:deviceX/target[m]_[n]
>>>>> +Date: April 2012
>>>>> +KernelVersion: 3.5
>>>>> +Contact: Johan Hovold<jhovold@xxxxxxxxx>
>>>>> +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. Feel free to make them read only though.
>
>>> To simplify somewhat (by ignoring some of the rules): If the average
>>> adc input drops below boundary0_low, the zone register reads 0; if it
>>> drops below boundary1_low, it reads 1, and so on. If the input it
>>> increases over boundary3_high, the zone register return 4; if it
>>> increases passed boundary2_high, it returns zone 3, etc.
>>>
>>> That is, roughly something like (we get 8-bits of input from the ADC):
>>>
>>> zone 0
>>>
>>> boundary0_low 51
>>> boundary0_high 53
>>>
>>> zone 1
>>>
>>> boundary1_low 102
>>> boundary1_high 106
>>>
>>> zone 2
>>>
>>> boundary2_low 153
>>> boundary2_high 161
>>>
>>> zone 3
>>>
>>> boundary3_low 204
>>> boundary3_high 220
>>>
>>> zone 4
>>>
>>> [ Figure 6 on page 20 in the datasheets should make it clear. ]
>>>
>>> The ALS interface and it's zone concept can then be used to control the
>>> LEDs and backlights of the chip, by determining the target brightness for
>>> each zone, e.g., set brightness to 52 when in zone 0.
>>>
>>> To complicate things further (and it is complicated), there are three
>>> such sets of target brightness values: ALSM1, ALSM2, ALSM3.
>>>
>>> So for each LED or backlight you can set ALS-input control mode, by
>>> saying that the device should get it's brightness levels from target set
>>> 1, 2, or 3.
>>>
>>> [ And it gets even more complicated, as ALSM1 can only control
>>> backlight0, where as ALSM2 and ALSM3 can control any of the remaining
>>> devices, but that's irrelevant here. ]
>>>
>>> Initially, I thought this interface to be too esoteric to be worth
>>> generalising, but it sort of fits with event thresholds so I gave it a
>>> try.
>> Glad you did and it pretty much fits, be it with a few extensions being
>> necessary.
>>> The biggest conceptual problem, I think, is that the zone
>>> boundaries can be used to control the other devices, even when the event
>>> is not enabled (or even an irq line not configured). That is, I find it
>>> a bit awkward that the event thresholds also defines the zones (a sort of
>>> discrete scaling factor).
>> That is indeed awkward. I'm not sure how we handle this either. If we
>> need to control these from the other devices (e.g. the back light
>> driver) then we'll have to get them into chan_spec and use the
>> inkernel interfaces to do it. Not infeasible but I was hoping to
>> avoid that until we have had a few months to see what similar devices
>> show up (on basis nothing in this world is a one off for long ;)
>
> I don't think the control bits can or should be generalised at this
> point. The same ALS-target values may be used to control more than one
> device, so they need to be set from the als rather from the controlled
> device (otherwise, changing the target value of led1 could change that
> of the other three leds without the user realising that this can be a
> side effect).
Good point. Nasty little device to write an interface for :)
>
>>> Perhaps simply keeping the attributes outside of events (e.g. named
>>> boundary[n]_{low,high}) and having a custom event enabled (e.g.
>>> in_illuminance_zone_change_en) is the best solution?
>> Maybe, but it's ugly and as you have said, they do correspond pretty well to
>> thresholds so I'd rather you went with that.
>> The core stuff for registering events clearly needs a rethink.... For
>> now doing it as you describe above (with the addition fo hysteresis
>> attributes) should be fine. Just document the 'quirks'.
>
> Ok, I'll keep the event/zone interface as it stands for now and we'll
> see if it can be generalised later. [ See my comment on the hysteresis
> above: there are only the rising/falling thresholds (low/high
> boundaries) and no boundary or hysteresis settings. ]
On that, just to reiterate, to have anything generalizable, userspace
needs to know that hysterisis exists on the individual thresholds
(though it is clearly a function of the neighbouring one).
>
> Thanks,
> Johan
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
