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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2yc2igv2lxh3u4kmkz73httg3sp24ziagcoaa7unfupagji7zk@ezaue3umwe44>
Date: Mon, 2 Dec 2024 15:55:00 +0100
From: Mehdi Djait <mehdi.djait@...ux.intel.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: Paul Kocialkowski <paulk@...-base.io>, 
	Jonathan Cameron <jic23@...nel.org>, linux-iio@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Lars-Peter Clausen <lars@...afoo.de>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, 
	Conor Dooley <conor+dt@...nel.org>
Subject: Re: [PATCH 2/2] iio: light: Add support for the TI OPT4048 color
 sensor

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

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