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

[Per-Daniel Olsson]

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

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?

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

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?

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
>