linux-kernel - Re: [PATCH v4 2/2] iio: light: Add support for TI OPT4060 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: <20241023142735.000018cb@Huawei.com>
Date: Wed, 23 Oct 2024 14:27:35 +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 v4 2/2] iio: light: Add support for TI OPT4060 color
 sensor

On Wed, 23 Oct 2024 09:29:08 +0200
Per-Daniel Olsson <perdaniel.olsson@...s.com> wrote:

> Hi Jonathan,
> 
> Thank you for your feedback, much appreciated. I have added questions and
> comments inline below regarding channels and triggers. I will address the other
> comments in the next patch.
> 
> Best regards / Per-Daniel
> 
> On 10/20/24 14:51, Jonathan Cameron wrote:
> > On Wed, 16 Oct 2024 23:34:09 +0200
> > Per-Daniel Olsson <perdaniel.olsson@...s.com> wrote:
> >   
> >> Add support for Texas Instruments OPT4060 RGBW Color sensor.
> >>
> >> Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@...s.com>  
> > 
> > Hi Per-Daniel,
> > 
> > Comments inline.
> > 
> > Jonathan
> >   
> >> diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
> >> new file mode 100644
> >> index 000000000000..2c3761ec423a
> >> --- /dev/null
> >> +++ b/drivers/iio/light/opt4060.c
> >> @@ -0,0 +1,1259 @@  
> > 
> > ...
> >   
> >> +
> >> +struct opt4060_buffer {
> >> +	u32 chan[OPT4060_NUM_CHANS];
> >> +	s64 ts __aligned(8);  
> > 
> > aligned_s64 is now available in linux-next + the IIO tree.
> >   
> >> +};
> >> +
> >> +static const struct opt4060_channel_factor opt4060_channel_factors[] = {
> >> +	{
> >> +		/* RED 2.4 * 2.15e-3 */  
> > This needs more details on wrt to what standard etc.
> > 
> > The datasheet is a little vague, but it seems to me like TI invented their
> > own standard. To use this stuff in a consistent ABI we need to have
> > a common standard or at least an approximation of one.
> > The illuminance estimates from some devices are bad approximations, but they
> > are at least attempting to approximate a well defined standard.  
> 
> I have read the datasheet again to try to figure out what TI means. When I read
> it now with your remarks from this email and previous emails in mind, I think I'm
> starting to understand more.
> 
> I think we should expose the data from the sensor in the following way:
> - Four raw channels (R, G, B and Clear)
> - Three processed IIO_INTENSITY channels with normalized values (R, G, B)
>   to get the relative color components independent of light intensity.
> - One IIO_LIGHT channel giving the lux value.
> 
> This is basically what TI is stating in chapter 8.4.5.2. I know that you don't
> like how TI are calculating the lux value using the green channel. But after
> reading the description and detailed description parts of the datasheet again,
> I think it sort of makes sense. Looking at the spectral response curves on the
> first page, the green curve covers the whole visible spectrum. It seems like this
> is what the sensor is actually designed for, measuring light intensity in lux and
> color independent of the light intensity.
> 
> Does this sound like a way forward you think?
Not keen on the colour part.

As far as I can tell TI made up a colour standard.  If it were
CIE 1931 RGB or then 'maybe' we could consider presenting them as processed,
though as they are linear scales even then should present _raw and _scale, not
_input (processed).  We would still need to figure out if we needed to handle
multiple colour space definitions.
As it is, if we have two different colour sensors, there is no way to compare the
values.  In particular that Green is way too broad for the colour standards
I quickly compared this with.

The green curve does (based on eyeballing it rather than anything formal)
look much closer to the luminosity function (one used for illuminance)
than I was assuming (given it's called green!)

So not ideal but that one feels ok (with comments in the code explaining
this) to use for illuminance.


For the color channels maybe we could present with _scale provided
if we add suitable documentation to say that the scaling is to arbitrary
datasheet specified normalization and that the resulting _raw * _scale
values cannot be compared across different sensors. I don't like that
but it does seem silly to not present the scaling if it might be useful
to someone.  So if you want to do this, propose some additions
to Documentation/testing/ABI/sysfs-bus-iio
to cover this for in_intensity_red_scale
etc.

Jonathan