Message-ID: <303bd71a-bb38-4afa-a9e2-046425084b69@axis.com>
Date: Tue, 12 Nov 2024 16:35:18 +0100
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 v5 2/2] iio: light: Add support for TI OPT4060 color
 sensor

On 11/9/24 16:09, Jonathan Cameron wrote:
> On Wed, 6 Nov 2024 13:00:36 +0100
> 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
> 
> Main comment in here is that the ABI is standard (though oddly
> missing in some cases from the main ABI doc). Annoyingly the
> docs build process (try make htmldocs) does not work if there
> are multiple entries for the same ABI, so we need to ensure that
> the documentation for common ABI is in just one place.
> That makes device specific ABI docs tricky, so instead we tend
> to use extra rst files in Documentation/iio/ to provide more details.
> 
> Jonathan
> 
Hi Jonathan,

Thank you for your code comments, I will fix those in the next version.
I have been trying to understand what I should do in
Documentation/ABI/testing/sysfs-bus-iio but I don't really get it. See
my questions below.

/ P-D

> 
>> ---
>>  .../ABI/testing/sysfs-bus-iio-light-opt4060   |   66 +
>>  drivers/iio/light/Kconfig                     |   13 +
>>  drivers/iio/light/Makefile                    |    1 +
>>  drivers/iio/light/opt4060.c                   | 1282 +++++++++++++++++
>>  4 files changed, 1362 insertions(+)
>>  create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-light-opt4060
>>  create mode 100644 drivers/iio/light/opt4060.c
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 b/Documentation/ABI/testing/sysfs-bus-iio-light-opt4060
>> new file mode 100644
>> index 000000000000..187e750602ee
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-iio-light-opt4060
>> @@ -0,0 +1,66 @@
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_red_raw
> 
> Huh... This is general ABI but not present in the sysfs-bus-iio
> where it should be.  There are some control parameters on these channels
> but not the actual channels.
> 
> Please add them there instead of in a device specific file.
> Also group the 3 colors together as done for intensity_x, _y, _z
>

So you want me to add 4 lines for in_intensity_X_raw where X is red, green,
blue and clear? Should I add those together with a description in the end of
the file or some place where I find similar definitions? The closest I can
find is in_intensityY_raw (line 1629 in the version of the file I'm looking at).
I also can't find the entries for in_intensity_red/green/blue_scale? I can find
in_intensity_x/y/z_scale but those were added in a commit for the as73211 driver
and as far as I can understand from the driver, x, y and z are coordinates and
not some kind of unknown variables in that context. I'm sorry for what I assume
are really stupid questions, but I just don't get it...

Basically I think I need to add the following 7 lines to the file:

What:    /sys/bus/iio/devices/iio:deviceX/in_intensity_red_raw
What:    /sys/bus/iio/devices/iio:deviceX/in_intensity_green_raw
What:    /sys/bus/iio/devices/iio:deviceX/in_intensity_blue_raw
What:    /sys/bus/iio/devices/iio:deviceX/in_intensity_clear_raw
What:    /sys/bus/iio/devices/iio:deviceX/in_intensity_red_scale
What:    /sys/bus/iio/devices/iio:deviceX/in_intensity_green_scale
What:    /sys/bus/iio/devices/iio:deviceX/in_intensity_blue_scale

Is that correct? Should the _raw ones be added in the groups starting on line 1629?
Should the _scale ones be added to the group starting on line 469?

>> +KernelVersion:
>> +Contact:	linux-iio@...r.kernel.org
>> +Description:
>> +		Unit-less raw value for red intensity.
>> +
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_red_scale
>> +KernelVersion:
>> +Contact:	linux-iio@...r.kernel.org
>> +Description:
>> +		Decimal value for the red component of the light. The value
>> +		is normalized to give the relative red component
>> +		independently of the light intensity.
> 
> I'm not sure I understand this text.   Also why Decimal?
> Maybe something like:
> 
> "Scales the raw value so that for a particular test light source, typically
> white, the measurement intensity is the same across different colour channels."
> 

Your text is also not totally correct, but probably better. The parameters are
first scaled the way you describe but then divided by the sum of the 3 RGB channels.
This to give an estimate of the color ratio between the three color components
independently of the light intensity. A decimal value between 1.0 and 0 will be
returned. Is this the type of oddity that should be documented in an rst file, the
way you described further down?

>>  The raw value for red
>> +		is multiplied by 2.4 before being normalized, this to adapt
>> +		to the relative sensitivity of the red filter of the sensor.
>> +		The factor for green is 1.0 and the factor for blue is 1.3.
> An unfortunately characteristic of the ABI docs is we can't have duplication so
> once this is moved to the general docs this detail will have to go in favour
> of generality.  You could add a little 'footnote' to the entry to say that
> for this particular device the meaning is this.
> 
> 
>> +
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_green_raw
>> +KernelVersion:
>> +Contact:	linux-iio@...r.kernel.org
>> +Description:
>> +		Unit-less raw value for green intensity.
>> +
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_green_scale
>> +KernelVersion:
>> +Contact:	linux-iio@...r.kernel.org
>> +Description:
>> +		Decimal value for the green component of the light. The
>> +		value is normalized to give the relative green component
>> +		independently of the light intensity. The raw value for
>> +		green is multiplied by 1.0 before being normalized, this to
>> +		adapt to the relative sensitivity of the green filter of
>> +		the sensor. The factor for red is 2.4 and the factor for
>> +		blue is 1.3.
>> +
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_blue_raw
>> +KernelVersion:
>> +Contact:	linux-iio@...r.kernel.org
>> +Description:
>> +		Unit-less raw value for blue intensity.
>> +
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_blue_scale
>> +KernelVersion:
>> +Contact:	linux-iio@...r.kernel.org
>> +Description:
>> +		Decimal value for the blue component of the light. The
>> +		value is normalized to give the relative blue component
>> +		independently of the light intensity. The raw value for
>> +		blue is multiplied by 1.3 before being normalized, this to
>> +		adapt to the relative sensitivity of the blue filter of the
>> +		sensor. The factor for red is 2.4 and the factor for green
>> +		is 1.0.
>> +
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_clear_raw
>> +KernelVersion:
>> +Contact:	linux-iio@...r.kernel.org
>> +Description:
>> +		Unit-less raw value for clear intensity.
>> +
>> +What:		/sys/bus/iio/devices/iio:deviceX/in_illuminance_input
> This is already in the main ABI doc. 
>> +KernelVersion:
>> +Contact:	linux-iio@...r.kernel.org
>> +Description:
>> +		Lux value for the light illuminance. The value is
>> +		calculated using the wide spectrum green channel and
>> +		multiplied by 2.15e-3.
> It may be worth capturing these details in an rst file under 
> Documentation/iio/  Just remember to add an entry in the index.rst file
> there so that they get included in the docs buidl.
>

Ok, I can describe this in an rst file.
 
...

Powered by blists - more mailing lists