Re: [PATCH v2 0/2] Support for Texas Instruments OPT4060 RGBW Color sensor.

[Per-Daniel Olsson]

On 10/8/24 19:11, Jonathan Cameron wrote:
> On Mon, 7 Oct 2024 15:37:07 +0200
> Per-Daniel Olsson <perdaniel.olsson@xxxxxxxx> wrote:
> 
>> On 10/6/24 17:24, Jonathan Cameron wrote:
>>> On Sat, 5 Oct 2024 18:51:17 +0200
>>> Per-Daniel Olsson <perdaniel.olsson@xxxxxxxx> wrote:
>>>   
>>>> This patch series adds support for Texas Instruments OPT4060 RGBW Color sensor
>>>> using the i2c interface.
>>>>
>>>> The driver exposes both raw adc values and calculated lux values though sysfs.
>>>> Integration time can be configured though sysfs as well.  
>>>
>>> Lux is a unit with a particular light curve.  It has no defined meaning for
>>> color channels.  As a result we usually only have colors as intensity channels
>>> (unit free).  If it is possible to compute an estimate of the illuminance then
>>> that can be a separate IIO_LIGHT channel.  
>>
>> The thing with lux is not actually something I know much about,
> 
> https://en.wikipedia.org/wiki/Illuminance is a decent description
> (though I haven't reread it today!)
> 
> Key thing is a brightness measure adjusted to take into account an
> approximation of the human eye sensitivity to various wavelengths.
>>> I just read the
>> data sheet (https://www.ti.com/lit/gpn/opt4060). According to chapter 8.4.5.2
>> (page 17), lux can be calculated this way:
>>
>> lux = GREEN_ADC_RAW * 2.15e-3
> ouch.  
>>
>> It is also stated that R=G=B for a D65 standard white light source if red is
>> multiplied by 2.4 and blue is multiplied with 1.3. I interpreted this as if
>> IIO_LIGHT was the best fit and that's why I didn't use IIO_INTENSITY. Should I
>> change to IIO_INTENSITY?
> 
> Yes.  Light isn't typically a d65 light unfortunately (reference lights
> are expensive!)
> 

I have switched to IIO_INTENSITY in patch v3.

> Mind you I guess if was, we'd live in a blank and white world as there
> would be no colors, just shades of gray...> 
> 
>>
>>>   
>>>> The OPT4060 sensor
>>>> supports both rising and falling threshold interrupts. These interrupts are
>>>> exposed as IIO events. The driver also implements an IIO triggered buffer with
>>>> two triggers, one trigger for conversion ready interrupts and one trigger for
>>>> threshold interrupts. The typical use case for this is to define a threshold and
>>>> listen for the events, and at the same time enable the triggered buffer with the
>>>> threshold trigger. Once the application gets the threshold event, the values
>>>> from the time of the event will be available in the triggered buffer. This
>>>> limits the number of interrupts between sensor and host and also the the usage
>>>> of sysfs for reading values after events.  
>>>
>>> We have had various discussions of threshold triggers in the past, but I don't
>>> think we ever merged any (maybe there is one somewhere?) The reasons for that are:
>>> 1) They are hard for generic userspace to understand.
>>> 2) For light sensors the input tends to be slow moving - grabbing one reading
>>>    on a threshold interrupt is rarely that informative (or are you grabbing them
>>>    on dataready once the threshold is breached?)  
>>
>> When the sensor triggers an interrupt for a threshold, it will also have the bit for
>> dataready set. So the values available at that point in time are the values that
>> triggered the threshold interrupt.
>>
>>> 3) If we want to do threshold triggers we should really add generic infrastructure
>>>    for them based on adding an in kernel events consumer path and a way to
>>>    instantiate a trigger based on any event.  Doing it in a single driver creates
>>>    an ABI we won't want to retain long term.
>>>
>>> So how important is this feature to you and why?  Maybe it is time to finally
>>> take a close look at option 3.  
>>
>> Our userspace application needs the values after getting the threshold event. Before
>> I implemented the threshold trigger and the triggered buffer, the application would
>> read the values from sysfs right after the event. In that case the values will not be
>> the ones that actually triggered the event. When the light condition is close to the
>> threshold, the value from sysfs might even be on the wrong side of the threshold which
>> can be confusing for the state machine in userspace. I would say that this feature is
>> fairly important to us, this is the way our userspace is using the sensor.
> 
> Brutal answer is fix your state machine to drop that assumption. I'd try to clamp
> the nearest to threshold to the threshold value in your userspace app. Any error
> that introduces should be lost in the noise.
> 
>>
>> If I understand you correctly, the problem you see with threshold triggers is that
>> it creates an implicit dependency between events and triggers. For the trigger to
>> function, userspace will first have to enable the event and set the threshold. I can
>> understand this issue, I think. Your suggestion with a way to instantiate triggers
>> from events sounds like a potential way forward. Do you have any more thoughts on how
>> that could be implemented? How can we proceed? How can I help?
> 
> Step one would be to add a general in kernel interface to get hold of events.
> That would have to look a little like the in kernel access to buffers (see inkern.c)
> We might be able to get away with different consumers just having to accept
> they may get events they didn't ask for.  So make the consumers filter them
> and the interface would just allow requesting 'more' events from a device.
> That device could say no if it doesn't support the requested events in addition
> to what it already has. 
> 
> That interface has a bunch of other uses such as trip points for thermal etc.
> 
> After that was done we'd also need a way to instantiate a trigger on a particular
> devices' event stream + filter.  Maybe we could do it for all devices, though that is
> going to be a little ugly as a lot of new triggers would turn up as in theory
> we should register one for every possible event each device can create.
> (imagine we want a trigger on a rising threshold and a different one to capture
> something else on the falling threshold).
> 
> Alternative would be to use configfs to provide a creation path for such triggers.
> 
> So not a small job, but not really breaking any new ground, just filling in
> a couple of long known to be missing features.
> 
> We might need some example tooling + a bunch of docs on how to use this as well.
> 
> Jonathan
>

Thank you for your thoughts and directions. I will try to find time to prototype what
you're suggesting. It will probably take a while, both since it's "not a small job" and
also because I'm not yet that familiar with the code.

/ P-D
 
>>
>> Thank you for you comments so far, looking forward to hearing your thoughts on a way
>> forward.
>>
>> / P-D
>>
>>>
>>> Jonathan
>>>   
>>>>
>>>> Changes in v2:
>>>> - dt-bindings: Removed incorrect allOf.
>>>> - dt-bindings: Changed to generic node name.
>>>> - Correction in opt4060_trigger_one_shot(...) for continuous mode.
>>>> - Correction in opt4060_power_down(...), wrong register was read.
>>>> - Corrected usage of active_scan_mask in opt4060_trigger_handler(...).
>>>> - Clean-up of various comments.
>>>> - Link to V1: https://lore.kernel.org/lkml/20241003164932.1162049-1-perdaniel.olsson@xxxxxxxx/
>>>>
>>>> Per-Daniel Olsson (2):
>>>>   dt-bindings: iio: light: Document TI OPT4060 RGBW sensor
>>>>   iio: light: Add support for TI OPT4060 color sensor
>>>>
>>>>  .../bindings/iio/light/ti,opt4060.yaml        |   51 +
>>>>  drivers/iio/light/Kconfig                     |   13 +
>>>>  drivers/iio/light/Makefile                    |    1 +
>>>>  drivers/iio/light/opt4060.c                   | 1216 +++++++++++++++++
>>>>  4 files changed, 1281 insertions(+)
>>>>  create mode 100644 Documentation/devicetree/bindings/iio/light/ti,opt4060.yaml
>>>>  create mode 100644 drivers/iio/light/opt4060.c
>>>>
>>>>
>>>> base-commit: 0c559323bbaabee7346c12e74b497e283aaafef5  
>>>   
>>
>>
>