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: <10d6bba4-4d25-4ee0-877e-48a27c622bde@axis.com>
Date: Wed, 23 Oct 2024 09:29:08 +0200
From: Per-Daniel Olsson <perdaniel.olsson@...s.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: 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

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?

> 
> 
...

>> +	if (processed) {
>> +		/*
>> +		 * The processed value are multiplied by factors that correspond to optical
>> +		 * parameters in the sensor. The processed values gives more correct comparison
>> +		 * between channels.
>> +		 */
> 
> No to these.  We discussed this in review thread of earlier version. There is no
> standard unit of comparison defined so this ABI is not something that userspace
> can currently understand.
> 
> I'm not against introducing one, though it would need a bunch of investigation
> into what the appropriate way to define this is. TI have chosen an option that
> I don't recall seeing used by anyone else but maybe I'm wrong on that.
> There are several competing RGB standards. https://en.wikipedia.org/wiki/RGB_color_spaces
> provides some info
> 

See my remarks and question above.

...

>> +	/*
>> +	 * The threshold trigger allows for sample capture on threshold interrupts.
>> +	 * Just using events does not enable a way to get the samples that actually
>> +	 * triggered the threshold interrupt.
>> +	 */
>> +	chip->threshold_trig = threshold_trigger;
>> +	threshold_trigger->ops = &opt4060_trigger_ops;
> 
> Whilst I understand your usecase for this, I'd rather not have it in the initial
> driver.  The use of events to trigger data capture should be a generic feature
> not one buried in a specific driver.  Best bet for initial merge would be
> to just stick to the dataready trigger alone.
> + I'd also encourage you to look at if your usespace code can be made less sensitive
> to slightly late read back and hence potentially not a precise match with the
> trigger event.
> 

I will remove the threshold trigger in the next patch. I have started looking at the
generic feature that you described in a previous email but it will take some time and
it will have to be another set of patches. The team here working on the userspace app
will have to adapt until this is done.

...