Support for Texas Instruments OPT4060 RGBW Color sensor.

Hi Per-Daniel,

kernel test robot noticed the following build errors:

[auto build test ERROR on jic23-iio/togreg]
[also build test ERROR on robh/for-next linus/master v6.12-rc6 next-20241106] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Per-Daniel-Olsson/dt-bindings-iio-light-Document-TI-OPT4060-RGBW-sensor/20241106-200407
base:   https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git togreg
patch link:    https://lore.kernel.org/r/20241106120036.986755-3-perdaniel.olsson%40axis.com
patch subject: [PATCH v5 2/2] iio: light: Add support for TI OPT4060 color sensor
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20241107/[email protected]/config)
compiler: clang version 19.1.3 (https://github.com/llvm/llvm-project ab51eccf88f5321e7c60591c5546b254b6afab99)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241107/[email protected]/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

All error/warnings (new ones prefixed by >>):

In file included from drivers/iio/light/opt4060.c:11:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2213:
include/linux/vmstat.h:504:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]

     504 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     505 |                            item];
         |                            ~~~~
   include/linux/vmstat.h:511:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     511 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     512 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
   include/linux/vmstat.h:518:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     518 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
   include/linux/vmstat.h:524:43: warning: arithmetic between different enumeration types ('enum zone_stat_item' and 'enum numa_stat_item') [-Wenum-enum-conversion]
     524 |         return vmstat_text[NR_VM_ZONE_STAT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~ ^
     525 |                            NR_VM_NUMA_EVENT_ITEMS +
         |                            ~~~~~~~~~~~~~~~~~~~~~~

Quoted 1 line

Quoted 1 line

drivers/iio/light/opt4060.c:847:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]

     847 |                 u32 th_lo, th_hi;
         |                 ^
   drivers/iio/light/opt4060.c:884:3: warning: label followed by a declaration is a C23 extension [-Wc23-extensions]
     884 |                 u32 th_lo, th_hi;
         |                 ^

Quoted 1 line

Quoted 1 line

drivers/iio/light/opt4060.c:977:24: error: incompatible function pointer types initializing 'int (*)(struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type, enum iio_event_direction, bool)' (aka 'int (*)(struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type, enum iio_event_direction, _Bool)') with an expression of type 'int (struct iio_dev *, const struct iio_chan_spec *, enum iio_event_type, enum iio_event_direction, int)' [-Wincompatible-function-pointer-types]

     977 |         .write_event_config = opt4060_write_event_config,
         |                               ^~~~~~~~~~~~~~~~~~~~~~~~~~

6 warnings and 1 error generated.

vim +977 drivers/iio/light/opt4060.c

   829	
   830	static int opt4060_read_event(struct iio_dev *indio_dev,
   831				      const struct iio_chan_spec *chan,
   832				      enum iio_event_type type,
   833				      enum iio_event_direction dir,
   834				      enum iio_event_info info,
   835				      int *val, int *val2)
   836	{
   837		int ret;
   838		struct opt4060_chip *chip = iio_priv(indio_dev);
   839	
   840		if (chan->type != IIO_INTENSITY)
   841			return -EINVAL;
   842		if (type != IIO_EV_TYPE_THRESH)
   843			return -EINVAL;
   844	
   845		switch (info) {
   846		case IIO_EV_INFO_VALUE:

Quoted 1 line

847 u32 th_lo, th_hi;

848

   849			ret = opt4060_get_thresholds(chip, &th_lo, &th_hi);
   850			if (ret)
   851				return ret;
   852			if (dir == IIO_EV_DIR_FALLING) {
   853				*val = th_lo;
   854				ret = IIO_VAL_INT;
   855			} else if (dir == IIO_EV_DIR_RISING) {
   856				*val = th_hi;
   857				ret = IIO_VAL_INT;
   858			}
   859			return ret;
   860		case IIO_EV_INFO_PERIOD:
   861			return opt4060_read_ev_period(chip, val, val2);
   862		default:
   863			return -EINVAL;
   864		}
   865	}
   866	
   867	static int opt4060_write_event(struct iio_dev *indio_dev,
   868				       const struct iio_chan_spec *chan,
   869				       enum iio_event_type type,
   870				       enum iio_event_direction dir,
   871				       enum iio_event_info info,
   872				       int val, int val2)
   873	{
   874		struct opt4060_chip *chip = iio_priv(indio_dev);
   875		int ret;
   876	
   877		if (chan->type != IIO_INTENSITY)
   878			return -EINVAL;
   879		if (type != IIO_EV_TYPE_THRESH)
   880			return -EINVAL;
   881	
   882		switch (info) {
   883		case IIO_EV_INFO_VALUE:
   884			u32 th_lo, th_hi;
   885	
   886			ret = opt4060_get_thresholds(chip, &th_lo, &th_hi);
   887			if (ret)
   888				return ret;
   889			if (dir == IIO_EV_DIR_FALLING)
   890				th_lo = val;
   891			else if (dir == IIO_EV_DIR_RISING)
   892				th_hi = val;
   893			return opt4060_set_thresholds(chip, th_lo, th_hi);
   894		case IIO_EV_INFO_PERIOD:
   895			return opt4060_write_ev_period(chip, val, val2);
   896		default:
   897			return -EINVAL;
   898		}
   899	}
   900	
   901	static int opt4060_read_event_config(struct iio_dev *indio_dev,
   902					     const struct iio_chan_spec *chan,
   903					     enum iio_event_type type,
   904					     enum iio_event_direction dir)
   905	{
   906		int ch_sel, ch_idx = chan->scan_index;
   907		struct opt4060_chip *chip = iio_priv(indio_dev);
   908		int ret;
   909	
   910		if (chan->type != IIO_INTENSITY)
   911			return -EINVAL;
   912		if (type != IIO_EV_TYPE_THRESH)
   913			return -EINVAL;
   914	
   915		ret = opt4060_get_channel_sel(chip, &ch_sel);
   916		if (ret)
   917			return ret;
   918	
   919		if (((dir == IIO_EV_DIR_FALLING) && chip->thresh_event_lo_active) ||
   920		    ((dir == IIO_EV_DIR_RISING) && chip->thresh_event_hi_active))
   921			return ch_sel == ch_idx;
   922	
   923		return ret;
   924	}
   925	
   926	static int opt4060_write_event_config(struct iio_dev *indio_dev,
   927					      const struct iio_chan_spec *chan,
   928					      enum iio_event_type type,
   929					      enum iio_event_direction dir, int state)
   930	{
   931		int ch_sel, ch_idx = chan->scan_index;
   932		struct opt4060_chip *chip = iio_priv(indio_dev);
   933		int ret;
   934	
   935		if (chan->type != IIO_INTENSITY)
   936			return -EINVAL;
   937		if (type != IIO_EV_TYPE_THRESH)
   938			return -EINVAL;
   939	
   940		ret = opt4060_get_channel_sel(chip, &ch_sel);
   941		if (ret)
   942			return ret;
   943	
   944		if (state) {
   945			/* Only one channel can be active at the same time */
   946			if ((chip->thresh_event_lo_active ||
   947				chip->thresh_event_hi_active) && (ch_idx != ch_sel))
   948				return -EBUSY;
   949			if (dir == IIO_EV_DIR_FALLING)
   950				chip->thresh_event_lo_active = true;
   951			else if (dir == IIO_EV_DIR_RISING)
   952				chip->thresh_event_hi_active = true;
   953			ret = opt4060_set_channel_sel(chip, ch_idx);
   954			if (ret)
   955				return ret;
   956		} else {
   957			if (ch_idx == ch_sel) {
   958				if (dir == IIO_EV_DIR_FALLING)
   959					chip->thresh_event_lo_active = false;
   960				else if (dir == IIO_EV_DIR_RISING)
   961					chip->thresh_event_hi_active = false;
   962			}
   963		}
   964	
   965		return opt4060_event_set_state(indio_dev, chip->thresh_event_hi_active |
   966					       chip->thresh_event_lo_active);
   967	}
   968	
   969	static const struct iio_info opt4060_info = {
   970		.read_raw = opt4060_read_raw,
   971		.write_raw = opt4060_write_raw,
   972		.write_raw_get_fmt = opt4060_write_raw_get_fmt,
   973		.read_avail = opt4060_read_available,
   974		.read_event_value = opt4060_read_event,
   975		.write_event_value = opt4060_write_event,
   976		.read_event_config = opt4060_read_event_config,

Quoted 1 line

977 .write_event_config = opt4060_write_event_config,

   978	};
   979	

Add support for Texas Instruments OPT4060 RGBW Color sensor.

Signed-off-by Per-Daniel Olsson <[email protected]>

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

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +0 -0

new file mode 100644
index 000000000000..187e750602ee

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +34 -0

+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_green_raw
+KernelVersion:
+Contact:	[email protected]
+Description:
+		Unit-less raw value for green intensity.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_green_scale
+KernelVersion:
+Contact:	[email protected]
+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:	[email protected]
+Description:
+		Unit-less raw value for blue intensity.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_blue_scale
+KernelVersion:
+Contact:	[email protected]
+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

drivers/iio/light/opt4060.c new file +11 -0

new file mode 100644
index 000000000000..218ca3fd74f5
@@ -0,0 +1,1282 @@
 
+
+static int opt4060_trigger_set_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig);
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+	int ret = 0;
+
+	if (state) {
+		ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_ALL_CH);
+		if (ret)
+			return ret;

drivers/iio/light/opt4060.c new file +18 -0

+}
+
+static ssize_t opt4060_read_ev_period(struct opt4060_chip *chip, int *val,
+				     int *val2)
+{
+	int ret, pers, fault_count, int_time;
+	u64 uval;
+
+	int_time = opt4060_int_time_reg[chip->int_time][0];
+
+	ret = regmap_read(chip->regmap, OPT4060_CTRL, &fault_count);
+	if (ret < 0)
+		return ret;
+
+	fault_count = fault_count & OPT4060_CTRL_FAULT_COUNT_MASK;
+	switch (fault_count) {
+	case OPT4060_CTRL_FAULT_COUNT_2:
+		pers = 2;

drivers/iio/light/opt4060.c new file +5 -0

+	}
+
+	uval = mul_u32_u32(int_time, pers);
+	*val = div_u64_rem(uval, MICRO, val2);
+

drivers/iio/light/opt4060.c new file +50 -0

+		if (chan == OPT4060_ILLUM)
+			ret = opt4060_calc_illuminance(chip, &raw.chan[i++]);
+		else
+			ret = opt4060_read_raw_value(chip,
+						     idev->channels[chan].address,
+						     &raw.chan[i++]);
+		if (ret) {
+			dev_err(chip->dev, "Reading channel data failed\n");
+			goto err_read;
+		}
+	}
+
+	iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp);
+err_read:
+	iio_trigger_notify_done(idev->trig);
+	return IRQ_HANDLED;
+}
+
+static irqreturn_t opt4060_irq_thread(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct opt4060_chip *chip = iio_priv(idev);
+	int ret, dummy;
+	unsigned int int_res;
+
+	ret = regmap_read(chip->regmap, OPT4060_RES_CTRL, &int_res);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to read interrupt reasons.\n");
+		return IRQ_NONE;
+	}
+
+	/* Read OPT4060_CTRL to clear interrupt */
+	ret = regmap_read(chip->regmap, OPT4060_CTRL, &dummy);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to clear interrupt\n");
+		return IRQ_NONE;
+	}
+
+	/* Handle events */
+	if (int_res & (OPT4060_RES_CTRL_FLAG_H | OPT4060_RES_CTRL_FLAG_L)) {
+		u64 code;
+		int chan = 0;
+
+		ret = opt4060_get_channel_sel(chip, &chan);
+		if (ret) {
+			dev_err(chip->dev, "Failed to read threshold channel.\n");
+			return IRQ_NONE;
+		}
+
+		/* Check if the interrupt is from the lower threshold */

drivers/iio/light/opt4060.c new file +21 -0

+						    chan,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_RISING);
+			iio_push_event(idev, code, iio_get_time_ns(idev));
+		}
+	}
+
+	/* Handle conversion ready */
+	if (int_res & OPT4060_RES_CTRL_CONV_READY) {
+		/* Signal completion for potentially waiting reads */
+		complete(&chip->completion);
+
+		/* Handle data ready triggers */
+		if (iio_buffer_enabled(idev))
+			iio_trigger_poll_nested(chip->trig);
+	}
+	return IRQ_HANDLED;
+}
+
+static int opt4060_setup_triggers(struct opt4060_chip *chip, struct iio_dev *idev)
+{

drivers/iio/light/opt4060.c new file +9 -0

+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	chip = iio_priv(indio_dev);
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)

drivers/iio/light/opt4060.c new file +4 -0

+
+	indio_dev->info = &opt4060_info;
+
+	ret = regmap_reinit_cache(chip->regmap, &opt4060_regmap_config);

drivers/iio/light/opt4060.c new file +15 -0

+	if (dev_id != OPT4060_DEVICE_ID_VAL)
+		dev_info(dev, "Device ID: %#04x unknown\n", dev_id);
+
+	if (chip->irq) {
+		indio_dev->channels = opt4060_channels;
+		indio_dev->num_channels = ARRAY_SIZE(opt4060_channels);
+	} else {
+		indio_dev->channels = opt4060_channels_no_events;
+		indio_dev->num_channels = ARRAY_SIZE(opt4060_channels_no_events);
+	}
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->name = "opt4060";
+
+	ret = opt4060_load_defaults(chip);
+	if (ret < 0)

drivers/iio/light/opt4060.c new file +6 -0

+		if (ret)
+			return ret;
+	}
+
+	return devm_iio_device_register(dev, indio_dev);
+}

Quoted 3 lines · On Wed, 6 Nov 2024 13:00:36 +0100 Per-Daniel Olsson…

Add support for Texas Instruments OPT4060 RGBW Color sensor.

Signed-off-by Per-Daniel Olsson <[email protected]>

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

Quoted 16 lines

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

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +0 -0

new file mode 100644
index 000000000000..187e750602ee

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 line 1

Quoted 1 line

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +1 -0

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

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060

Quoted 12 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +12 -0

+KernelVersion:
+Contact:	[email protected]
+Description:
+		Unit-less raw value for red intensity.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_red_scale
+KernelVersion:
+Contact:	[email protected]
+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."

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 line 16

Quoted 4 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +3 -0

 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.

Quoted 34 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +34 -0

+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_green_raw
+KernelVersion:
+Contact:	[email protected]
+Description:
+		Unit-less raw value for green intensity.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_green_scale
+KernelVersion:
+Contact:	[email protected]
+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:	[email protected]
+Description:
+		Unit-less raw value for blue intensity.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_blue_scale
+KernelVersion:
+Contact:	[email protected]
+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

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 line 60

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +10 -0

+		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:	[email protected]
+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.

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 line 66

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +6 -0

+KernelVersion:
+Contact:	[email protected]
+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.

Quoted 16 lines · Jonathan Cameron

Quoted 3 lines · On Wed, 6 Nov 2024 13:00:36 +0100 Per-Daniel Olsson…

Add support for Texas Instruments OPT4060 RGBW Color sensor.

Signed-off-by Per-Daniel Olsson <[email protected]>

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.

Quoted 2 lines

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

Quoted 22 lines

Quoted 16 lines

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

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +0 -0

new file mode 100644
index 000000000000..187e750602ee

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 line 1

Quoted 1 line

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +1 -0

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

Quoted 3 lines

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?

Quoted 17 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060

Quoted 13 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +12 -0

+KernelVersion:
+Contact:	[email protected]
+Description:
+		Unit-less raw value for red intensity.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_red_scale
+KernelVersion:
+Contact:	[email protected]
+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:

Quoted 3 lines

"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?

Quoted 56 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 line 16

Quoted 5 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +3 -0

 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.

Quoted 34 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +34 -0

+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_green_raw
+KernelVersion:
+Contact:	[email protected]
+Description:
+		Unit-less raw value for green intensity.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_green_scale
+KernelVersion:
+Contact:	[email protected]
+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:	[email protected]
+Description:
+		Unit-less raw value for blue intensity.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_blue_scale
+KernelVersion:
+Contact:	[email protected]
+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

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 line 60

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +10 -0

+		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:	[email protected]
+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.

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 line 66

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +6 -0

+KernelVersion:
+Contact:	[email protected]
+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.

...

Quoted 54 lines · On Tue, 12 Nov 2024 16:35:18 +0100 Per-Daniel Olsson…

Quoted 16 lines · Jonathan Cameron

Quoted 3 lines · On Wed, 6 Nov 2024 13:00:36 +0100 Per-Daniel Olsson…

Add support for Texas Instruments OPT4060 RGBW Color sensor.

Signed-off-by Per-Daniel Olsson <[email protected]>

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.

Quoted 2 lines

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

Quoted 22 lines

Quoted 16 lines

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

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +0 -0

new file mode 100644
index 000000000000..187e750602ee

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 line 1

Quoted 1 line

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +1 -0

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

Quoted 3 lines

Please add them there instead of in a device specific file. Also group the 3 colors together as done for intensity_x, _y, _z

Quoted 7 lines

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.

Quoted 2 lines

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!

Quoted 11 lines

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

Quoted 1 line

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

yes.

Quoted 1 line

Should the _scale ones be added to the group starting on line 469?

yes.

Quoted 21 lines

Quoted 17 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060

Quoted 13 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +12 -0

+KernelVersion:
+Contact:	[email protected]
+Description:
+		Unit-less raw value for red intensity.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_red_scale
+KernelVersion:
+Contact:	[email protected]
+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:

Quoted 3 lines

"Scales the raw value so that for a particular test light source, typically white, the measurement intensity is the same across different colour channels."

Quoted 6 lines

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.

Quoted 67 lines

Quoted 66 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 line 16

Quoted 5 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +3 -0

 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.

Quoted 34 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +34 -0

+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_green_raw
+KernelVersion:
+Contact:	[email protected]
+Description:
+		Unit-less raw value for green intensity.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_green_scale
+KernelVersion:
+Contact:	[email protected]
+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:	[email protected]
+Description:
+		Unit-less raw value for blue intensity.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_blue_scale
+KernelVersion:
+Contact:	[email protected]
+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

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 line 60

Quoted 10 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +10 -0

+		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:	[email protected]
+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.

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 line 66

Quoted 6 lines

Documentation/ABI/testing/sysfs-bus-iio-light-opt4060 new file +6 -0

+KernelVersion:
+Contact:	[email protected]
+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.

Quoted 1 line

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

Quoted 2 lines

...