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