Message-ID: <20241008181151.00006abd@Huawei.com>
Date: Tue, 8 Oct 2024 18:11:51 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Per-Daniel Olsson <perdaniel.olsson@...s.com>
CC: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen <lars@...afoo.de>,
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, <linux-iio@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
	<rickard.andersson@...s.com>, <kernel@...s.com>
Subject: Re: [PATCH v2 0/2] Support for Texas Instruments OPT4060 RGBW Color
 sensor.

On Mon, 7 Oct 2024 15:37:07 +0200
Per-Daniel Olsson <perdaniel.olsson@...s.com> wrote:

> On 10/6/24 17:24, Jonathan Cameron wrote:
> > On Sat, 5 Oct 2024 18:51:17 +0200
> > Per-Daniel Olsson <perdaniel.olsson@...s.com> 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!)

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 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@axis.com/
> >>
> >> 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  
> >   
> 
> 

Powered by blists - more mailing lists