Re: [PATCH v4 2/2] iio: light: Add support for TI OPT4060 color sensor

[Per-Daniel Olsson <perdaniel.olsson@axis.com>]

On 10/23/24 15:27, Jonathan Cameron wrote:
> On Wed, 23 Oct 2024 09:29:08 +0200
> Per-Daniel Olsson <perdaniel.olsson@axis.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@axis.com> wrote:
>>>   
>>>> Add support for Texas Instruments OPT4060 RGBW Color sensor.
>>>>
>>>> Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.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
> 
> 

Ok, great. Thank you for responding so quickly. I will try to implement it
according to your suggestions in the next patch and also patch the
documentation.

/ P-D