Re: [PATCH 2/2] iio: light: Add support for the TI OPT4048 color sensor

[Paul Kocialkowski <paulk@sys-base.io>]

[-- Attachment #1: Type: text/plain, Size: 3552 bytes --]

Hi Mehdi,

Glad to see you active here :)

Le Mon 02 Dec 24, 15:55, Mehdi Djait a écrit :
> 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@sys-base.io> 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@sys-base.io> 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@sys-base.io>  
> > > > 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/

Thanks for the hint! I'll definitely look it up for details.

Cheers,

Paul

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

-- 
Paul Kocialkowski,

Independent contractor - sys-base - https://www.sys-base.io/
Free software developer - https://www.paulk.fr/

Expert in multimedia, graphics and embedded hardware support with Linux.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]