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