Hello Paul :) On Mon, Dec 02, 2024 at 11:06:59AM +0000, Jonathan Cameron wrote: > On Sun, 1 Dec 2024 18:56:30 +0100 > Paul Kocialkowski <paulk@xxxxxxxxxxx> wrote: > > > Hi Jonathan, > > > > Le Sun 01 Dec 24, 11:55, Jonathan Cameron a écrit : > > > On Sat, 30 Nov 2024 18:42:12 +0100 > > > Paul Kocialkowski <paulk@xxxxxxxxxxx> wrote: > > > > > > > The Texas Instruments OPT4048 is a XYZ tristimulus color sensor, > > > > with an additional wide (visible + IR) channel. > > > > > > > > This driver implements support for all channels, with configurable > > > > integration time and auto-gain. Both direct reading and > > > > triggered-buffer modes are supported. > > > > > > > > Note that the Y channel is also reported as a separate illuminance > > > > channel, for which a scale is provided (following the datasheet) to > > > > convert it to lux units. Falling and rising thresholds are supported > > > > for this channel. > > > > > > > > The device's interrupt can be used to sample all channels at the end > > > > of conversion and is optional. > > > > > > > > Signed-off-by: Paul Kocialkowski <paulk@xxxxxxxxxxx> > > > Hi Paul, > > > > > > Various comments inline. Most significant is that this seems to be > > > suitable for a simple dataready trigger that will make your various > > > interrupt and non interrupt flows more similar. > > > > And thanks for the fast review and insightful comments! > > > > I considered implementing a trigger in the driver, but the issue I found > > is that the trigger is expected to be called from hard irq context, > > while the new values are read in the bottom half. > > The trigger can be called from either the hard irq context or from > a thread. See iio_trigger_poll_nested() > There is a quirk that you then don't end up calling the registered > hard irq handler for the trigger so sometimes a bit of fiddly code > is needed to ensure timestamps etc are grabbed. Not sure that matters > here. > If the timestamps do matter: here is a (maybe relevant?) discussion for an issue I faced with timestamps for a driver that supports both FIFO and triggered buffer mode Please note that iio_trigger_poll_nested() was called iio_trigger_poll_chained() back in that discussion. https://lore.kernel.org/linux-iio/Y+6QoBLh1k82cJVN@carbian/ > > I understand the triggered > > buffer callbacks are executed as a thread as well, so there would be race > > between the two which could result in previous values being returned. > > With the above nested call it is all run in the same thread > See handle_nested_irq() in particular the function docs. > https://elixir.bootlin.com/linux/v6.12.1/source/kernel/irq/chip.c#L459 > > > So I concluded that it was more beneficial to preserve the synchronous reading > > mechanism over implementing the trigger. > > Definite preference for a trigger approach, but I may well still be missing > a detail. -- Kind Regards Mehdi Djait
