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 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@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? >> 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. ...
