On 11/23/24 15:58, Jonathan Cameron wrote: > On Tue, 12 Nov 2024 16:35:18 +0100 > Per-Daniel Olsson <perdaniel.olsson@xxxxxxxx> wrote: > >> On 11/9/24 16:09, Jonathan Cameron wrote: >>> On Wed, 6 Nov 2024 13:00:36 +0100 >>> Per-Daniel Olsson <perdaniel.olsson@xxxxxxxx> wrote: >>> >>>> Add support for Texas Instruments OPT4060 RGBW Color sensor. >>>> >>>> Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@xxxxxxxx> >>> 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 > > xyz are color spaces coordinates for CIE XYZ color space. Which admittedly gets > confusing when we use XYZ for index numbers in this file. > >> 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... > No problem. There are often no 'right' rules for this, just better and worse > options! >> >> 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? > yes. >> Should the _scale ones be added to the group starting on line 469? > yes. >> >>>> +KernelVersion: >>>> +Contact: linux-iio@xxxxxxxxxxxxxxx >>>> +Description: >>>> + Unit-less raw value for red intensity. >>>> + >>>> +What: /sys/bus/iio/devices/iio:deviceX/in_intensity_red_scale >>>> +KernelVersion: >>>> +Contact: linux-iio@xxxxxxxxxxxxxxx >>>> +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? > Hmm. A divide by the sum is unfortunate. The only common case for this is > quarternions (where the 4 elements are normalized). > > I suggest you don't divide through, just leave that for userspace to do > if it wishes. Fine to document that procedure in an rst file though if you like. > >> >>>> 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@xxxxxxxxxxxxxxx >>>> +Description: >>>> + Unit-less raw value for green intensity. >>>> + >>>> +What: /sys/bus/iio/devices/iio:deviceX/in_intensity_green_scale >>>> +KernelVersion: >>>> +Contact: linux-iio@xxxxxxxxxxxxxxx >>>> +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@xxxxxxxxxxxxxxx >>>> +Description: >>>> + Unit-less raw value for blue intensity. >>>> + >>>> +What: /sys/bus/iio/devices/iio:deviceX/in_intensity_blue_scale >>>> +KernelVersion: >>>> +Contact: linux-iio@xxxxxxxxxxxxxxx >>>> +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@xxxxxxxxxxxxxxx >>>> +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@xxxxxxxxxxxxxxx >>>> +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. > > Great, > > Sorry for slow reply. Things got a bit busy for a week or so! > > Jonathan > Hi Jonathan, Thank you for your answers and clarifications. I will remove the normalized scaled values in the next version of the patch and explain the usage in the rst file. I will also update the sysfs-bus-iio file according to your answer. I will also update some small comments from Javier Carrasco. Hopefully I will send the next version very soon, so if you haven't already looked at v6 it might be best to wait for v7. Best regards / P-D >> >> ... >
