diff mbox series

[v5,2/2] iio: light: Add support for TI OPT4060 color sensor

Message ID 20241106120036.986755-3-perdaniel.olsson@axis.com (mailing list archive)
State Changes Requested
Headers show
Series Support for Texas Instruments OPT4060 RGBW Color sensor. | expand

Commit Message

Per-Daniel Olsson Nov. 6, 2024, noon UTC
Add support for Texas Instruments OPT4060 RGBW Color sensor.

Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.com>
---
 .../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

Comments

kernel test robot Nov. 7, 2024, 12:39 a.m. UTC | #1
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/202411070820.GxSGRFR6-lkp@intel.com/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/202411070820.GxSGRFR6-lkp@intel.com/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 <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411070820.GxSGRFR6-lkp@intel.com/

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 +
         |                            ~~~~~~~~~~~~~~~~~~~~~~
>> 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;
         |                 ^
>> 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:
 > 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,
 > 977		.write_event_config = opt4060_write_event_config,
   978	};
   979
Jonathan Cameron Nov. 9, 2024, 2:44 p.m. UTC | #2
On Thu, 7 Nov 2024 08:39:56 +0800
kernel test robot <lkp@intel.com> wrote:

> 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/202411070820.GxSGRFR6-lkp@intel.com/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/202411070820.GxSGRFR6-lkp@intel.com/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 <lkp@intel.com>
> | Closes: https://lore.kernel.org/oe-kbuild-all/202411070820.GxSGRFR6-lkp@intel.com/

As you have probably realized, your series crossed with a change of parameter type.

If there is nothing else to warrant a respin I can fix this up whilst applying.

Jonathan

> 
> 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 +
>          |                            ~~~~~~~~~~~~~~~~~~~~~~
> >> 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;
>          |                 ^
> >> 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:
>  > 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,
>  > 977		.write_event_config = opt4060_write_event_config,  
>    978	};
>    979	
>
Jonathan Cameron Nov. 9, 2024, 3:09 p.m. UTC | #3
On Wed, 6 Nov 2024 13:00:36 +0100
Per-Daniel Olsson <perdaniel.olsson@axis.com> wrote:

> Add support for Texas Instruments OPT4060 RGBW Color sensor.
> 
> Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.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


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

> +KernelVersion:
> +Contact:	linux-iio@vger.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@vger.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."

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

> diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
> new file mode 100644
> index 000000000000..218ca3fd74f5
> --- /dev/null
> +++ b/drivers/iio/light/opt4060.c
> @@ -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;
> +		ret = opt4060_set_continous_mode(chip, true);
> +	} else {
> +		if (!opt4060_event_active(chip) && chip->irq) {
> +			ret = opt4060_set_continous_mode(chip, false);
> +			if (ret)
> +				return ret;
> +		}
> +		ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_THRESHOLD);
> +	}
> +	return ret;
return in the if / else as nothing common to do down ehre.

> +}
> +
> +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;
> +		break;
> +	case OPT4060_CTRL_FAULT_COUNT_4:
> +		pers = 4;
> +		break;
> +	case OPT4060_CTRL_FAULT_COUNT_8:
> +		pers = 8;
> +		break;
> +
> +	default:
> +		pers = 1;
break from here too.  Not strictly necessary of course.

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

> +
> +static irqreturn_t opt4060_trigger_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *idev = pf->indio_dev;
> +	struct opt4060_chip *chip = iio_priv(idev);
> +	struct opt4060_buffer raw;

Type only used in here, bring the definition down here as well.

> +	int ret, chan = 0, i = 0;
I'd prefer you don't mix assigned and unassign declarations on one line.
	int chan = 0, i = 0;
	int ret;

> +
> +	memset(&raw, 0, sizeof(raw));
> +
> +	iio_for_each_active_channel(idev, chan) {
Fairly sure that will init chan for you so shouldn't be needed above.
> +		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 */
> +		if (int_res & OPT4060_RES_CTRL_FLAG_L) {
> +			code = IIO_UNMOD_EVENT_CODE(IIO_INTENSITY,
> +						    chan,
> +						    IIO_EV_TYPE_THRESH,
> +						    IIO_EV_DIR_FALLING);
> +			iio_push_event(idev, code, iio_get_time_ns(idev));
> +		}
> +		/* Check if the interrupt is from the upper threshold */
> +		if (int_res & OPT4060_RES_CTRL_FLAG_H) {
> +			code = IIO_UNMOD_EVENT_CODE(IIO_INTENSITY,
Unmodified?  Your channels are modified so this should be as well I think.



> +						    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)
> +{
> +	struct iio_trigger *data_trigger;
> +	char *name;
> +	int ret;
> +
> +	ret = devm_iio_triggered_buffer_setup(chip->dev, idev,
> +					      &iio_pollfunc_store_time,
> +					      opt4060_trigger_handler, NULL);
> +	if (ret)
> +		return dev_err_probe(chip->dev, ret,
> +				     "iio_triggered_buffer_setup_ext FAIL\n");
>
Note using the ext form. Also I'd talk about what failed (buffer setup) rather
than the function that failed.

> +
> +static int opt4060_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct opt4060_chip *chip;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +	uint dev_id;
uint usages is unusual. and regmap takes an unsigned int
so spell it out fully.


> +
> +	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)
> +		return dev_err_probe(dev, ret, "Failed to enable vdd supply\n");
> +
> +	chip->regmap = devm_regmap_init_i2c(client, &opt4060_regmap_config);
> +	if (IS_ERR(chip->regmap))
> +		return dev_err_probe(dev, PTR_ERR(chip->regmap),
> +				     "regmap initialization failed\n");
> +
> +	chip->dev = dev;
> +	chip->irq = client->irq;
> +	init_completion(&chip->completion);

Perhaps move to be dependent on the irq actually being available as I don't
think it is used otherwise.

> +
> +	indio_dev->info = &opt4060_info;
> +
> +	ret = regmap_reinit_cache(chip->regmap, &opt4060_regmap_config);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "failed to reinit regmap cache\n");
> +
> +	ret = regmap_read(chip->regmap, OPT4060_DEVICE_ID, &dev_id);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +			"Failed to read the device ID register\n");
> +
> +	dev_id = FIELD_GET(OPT4060_DEVICE_ID_MASK, dev_id);

Use a different variable for the register value and the field.
It is less confusing to read that way.

> +	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)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to set sensor defaults\n");
> +
> +	ret = devm_add_action_or_reset(dev, opt4060_chip_off_action, chip);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to setup power off action\n");
> +
> +	if (chip->irq) {
> +		ret = opt4060_setup_triggers(chip, indio_dev);
Currently you only restrict the trigger to definitely be used with this device
but the device can be used with other triggers.  If that's intentional
you should setup the buffer whether or not the irq is available.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
Per-Daniel Olsson Nov. 12, 2024, 3:35 p.m. UTC | #4
On 11/9/24 16:09, Jonathan Cameron wrote:
> On Wed, 6 Nov 2024 13:00:36 +0100
> Per-Daniel Olsson <perdaniel.olsson@axis.com> wrote:
> 
>> Add support for Texas Instruments OPT4060 RGBW Color sensor.
>>
>> Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.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@vger.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@vger.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@vger.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@vger.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@vger.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@vger.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@vger.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@vger.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.
 
...
Jonathan Cameron Nov. 23, 2024, 2:58 p.m. UTC | #5
On Tue, 12 Nov 2024 16:35:18 +0100
Per-Daniel Olsson <perdaniel.olsson@axis.com> wrote:

> On 11/9/24 16:09, Jonathan Cameron wrote:
> > On Wed, 6 Nov 2024 13:00:36 +0100
> > Per-Daniel Olsson <perdaniel.olsson@axis.com> wrote:
> >   
> >> Add support for Texas Instruments OPT4060 RGBW Color sensor.
> >>
> >> Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.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

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

Great,

Sorry for slow reply. Things got a bit busy for a week or so!

Jonathan

>  
> ...
diff mbox series

Patch

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
+KernelVersion:
+Contact:	linux-iio@vger.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@vger.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. 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.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_intensity_green_raw
+KernelVersion:
+Contact:	linux-iio@vger.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@vger.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@vger.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@vger.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@vger.kernel.org
+Description:
+		Unit-less raw value for clear intensity.
+
+What:		/sys/bus/iio/devices/iio:deviceX/in_illuminance_input
+KernelVersion:
+Contact:	linux-iio@vger.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.
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 29ffa8491927..96cd26d95af9 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -491,6 +491,19 @@  config OPT4001
 	  If built as a dynamically linked module, it will be called
 	  opt4001.
 
+config OPT4060
+	tristate "Texas Instruments OPT4060 RGBW Color Sensor"
+	depends on I2C
+	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	help
+	  If you say Y or M here, you get support for Texas Instruments
+	  OPT4060 RGBW Color Sensor.
+
+	  If built as a dynamically linked module, it will be called
+	  opt4060.
+
 config PA12203001
 	tristate "TXC PA12203001 light and proximity sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index f14a37442712..0ea5e6348878 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -42,6 +42,7 @@  obj-$(CONFIG_MAX44009)		+= max44009.o
 obj-$(CONFIG_NOA1305)		+= noa1305.o
 obj-$(CONFIG_OPT3001)		+= opt3001.o
 obj-$(CONFIG_OPT4001)		+= opt4001.o
+obj-$(CONFIG_OPT4060)		+= opt4060.o
 obj-$(CONFIG_PA12203001)	+= pa12203001.o
 obj-$(CONFIG_ROHM_BU27008)	+= rohm-bu27008.o
 obj-$(CONFIG_ROHM_BU27034)	+= rohm-bu27034.o
diff --git a/drivers/iio/light/opt4060.c b/drivers/iio/light/opt4060.c
new file mode 100644
index 000000000000..218ca3fd74f5
--- /dev/null
+++ b/drivers/iio/light/opt4060.c
@@ -0,0 +1,1282 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 Axis Communications AB
+ *
+ * Datasheet: https://www.ti.com/lit/gpn/opt4060
+ *
+ * Device driver for the Texas Instruments OPT4060 RGBW Color Sensor.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/math64.h>
+#include <linux/units.h>
+#include <linux/limits.h>
+#include <linux/module.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/iio/events.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+/* OPT4060 register set */
+#define OPT4060_RED_MSB				0x00
+#define OPT4060_RED_LSB				0x01
+#define OPT4060_GREEN_MSB			0x02
+#define OPT4060_GREEN_LSB			0x03
+#define OPT4060_BLUE_MSB			0x04
+#define OPT4060_BLUE_LSB			0x05
+#define OPT4060_CLEAR_MSB			0x06
+#define OPT4060_CLEAR_LSB			0x07
+#define OPT4060_THRESHOLD_LOW			0x08
+#define OPT4060_THRESHOLD_HIGH			0x09
+#define OPT4060_CTRL				0x0a
+#define OPT4060_INT_CTRL			0x0b
+#define OPT4060_RES_CTRL			0x0c
+#define OPT4060_DEVICE_ID			0x11
+
+/* OPT4060 register mask */
+#define OPT4060_EXPONENT_MASK			GENMASK(15, 12)
+#define OPT4060_MSB_MASK			GENMASK(11, 0)
+#define OPT4060_LSB_MASK			GENMASK(15, 8)
+#define OPT4060_COUNTER_MASK			GENMASK(7, 4)
+#define OPT4060_CRC_MASK			GENMASK(3, 0)
+
+/* OPT4060 device id mask */
+#define OPT4060_DEVICE_ID_MASK			GENMASK(11, 0)
+
+/* OPT4060 control register masks */
+#define OPT4060_CTRL_QWAKE_MASK			BIT(15)
+#define OPT4060_CTRL_RANGE_MASK			GENMASK(13, 10)
+#define OPT4060_CTRL_CONV_TIME_MASK		GENMASK(9, 6)
+#define OPT4060_CTRL_OPER_MODE_MASK		GENMASK(5, 4)
+#define OPT4060_CTRL_LATCH_MASK			BIT(3)
+#define OPT4060_CTRL_INT_POL_MASK		BIT(2)
+#define OPT4060_CTRL_FAULT_COUNT_MASK		GENMASK(1, 0)
+
+/* OPT4060 interrupt control register masks */
+#define OPT4060_INT_CTRL_THRESH_SEL		GENMASK(6, 5)
+#define OPT4060_INT_CTRL_OUTPUT			BIT(4)
+#define OPT4060_INT_CTRL_INT_CFG		GENMASK(3, 2)
+#define OPT4060_INT_CTRL_THRESHOLD		0x0
+#define OPT4060_INT_CTRL_NEXT_CH		0x1
+#define OPT4060_INT_CTRL_ALL_CH			0x3
+
+/* OPT4060 result control register masks */
+#define OPT4060_RES_CTRL_OVERLOAD		BIT(3)
+#define OPT4060_RES_CTRL_CONV_READY		BIT(2)
+#define OPT4060_RES_CTRL_FLAG_H			BIT(1)
+#define OPT4060_RES_CTRL_FLAG_L			BIT(0)
+
+/* OPT4060 constants */
+#define OPT4060_DEVICE_ID_VAL			0x821
+
+/* OPT4060 operating modes */
+#define OPT4060_CTRL_OPER_MODE_OFF		0x0
+#define OPT4060_CTRL_OPER_MODE_FORCED		0x1
+#define OPT4060_CTRL_OPER_MODE_ONE_SHOT		0x2
+#define OPT4060_CTRL_OPER_MODE_CONTINUOUS	0x3
+
+/* OPT4060 conversion control register definitions */
+#define OPT4060_CTRL_CONVERSION_0_6MS		0x0
+#define OPT4060_CTRL_CONVERSION_1MS		0x1
+#define OPT4060_CTRL_CONVERSION_1_8MS		0x2
+#define OPT4060_CTRL_CONVERSION_3_4MS		0x3
+#define OPT4060_CTRL_CONVERSION_6_5MS		0x4
+#define OPT4060_CTRL_CONVERSION_12_7MS		0x5
+#define OPT4060_CTRL_CONVERSION_25MS		0x6
+#define OPT4060_CTRL_CONVERSION_50MS		0x7
+#define OPT4060_CTRL_CONVERSION_100MS		0x8
+#define OPT4060_CTRL_CONVERSION_200MS		0x9
+#define OPT4060_CTRL_CONVERSION_400MS		0xa
+#define OPT4060_CTRL_CONVERSION_800MS		0xb
+
+/* OPT4060 fault count control register definitions */
+#define OPT4060_CTRL_FAULT_COUNT_1		0x0
+#define OPT4060_CTRL_FAULT_COUNT_2		0x1
+#define OPT4060_CTRL_FAULT_COUNT_4		0x2
+#define OPT4060_CTRL_FAULT_COUNT_8		0x3
+
+/* OPT4060 scale light level range definitions */
+#define OPT4060_CTRL_LIGHT_SCALE_AUTO		12
+
+/* OPT4060 default values */
+#define OPT4060_DEFAULT_CONVERSION_TIME OPT4060_CTRL_CONVERSION_50MS
+
+/*
+ * enum opt4060_chan_type - OPT4060 channel types
+ * @OPT4060_RED:	Red channel.
+ * @OPT4060_GREEN:	Green channel.
+ * @OPT4060_BLUE:	Blue channel.
+ * @OPT4060_CLEAR:	Clear (white) channel.
+ * @OPT4060_ILLUM:	Calculated illuminance channel.
+ * @OPT4060_NUM_CHANS:	Number of channel types.
+ */
+enum opt4060_chan_type {
+	OPT4060_RED,
+	OPT4060_GREEN,
+	OPT4060_BLUE,
+	OPT4060_CLEAR,
+	OPT4060_ILLUM,
+	OPT4060_NUM_CHANS
+};
+
+struct opt4060_chip {
+	struct regmap *regmap;
+	struct device *dev;
+	struct iio_trigger *trig;
+	u8 int_time;
+	int irq;
+	struct completion completion;
+	bool thresh_event_lo_active;
+	bool thresh_event_hi_active;
+};
+
+struct opt4060_channel_factor {
+	u32 mul;
+	u32 div;
+};
+
+struct opt4060_buffer {
+	u32 chan[OPT4060_NUM_CHANS];
+	aligned_s64 ts;
+};
+
+static const int opt4060_int_time_available[][2] = {
+	{ 0,    600 },
+	{ 0,   1000 },
+	{ 0,   1800 },
+	{ 0,   3400 },
+	{ 0,   6500 },
+	{ 0,  12700 },
+	{ 0,  25000 },
+	{ 0,  50000 },
+	{ 0, 100000 },
+	{ 0, 200000 },
+	{ 0, 400000 },
+	{ 0, 800000 },
+};
+
+/*
+ * Conversion time is integration time + time to set register
+ * this is used as integration time.
+ */
+static const int opt4060_int_time_reg[][2] = {
+	{    600,  OPT4060_CTRL_CONVERSION_0_6MS  },
+	{   1000,  OPT4060_CTRL_CONVERSION_1MS    },
+	{   1800,  OPT4060_CTRL_CONVERSION_1_8MS  },
+	{   3400,  OPT4060_CTRL_CONVERSION_3_4MS  },
+	{   6500,  OPT4060_CTRL_CONVERSION_6_5MS  },
+	{  12700,  OPT4060_CTRL_CONVERSION_12_7MS },
+	{  25000,  OPT4060_CTRL_CONVERSION_25MS   },
+	{  50000,  OPT4060_CTRL_CONVERSION_50MS   },
+	{ 100000,  OPT4060_CTRL_CONVERSION_100MS  },
+	{ 200000,  OPT4060_CTRL_CONVERSION_200MS  },
+	{ 400000,  OPT4060_CTRL_CONVERSION_400MS  },
+	{ 800000,  OPT4060_CTRL_CONVERSION_800MS  },
+};
+
+static int opt4060_als_time_to_index(const u32 als_integration_time)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(opt4060_int_time_available); i++) {
+		if (als_integration_time == opt4060_int_time_available[i][1])
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static u8 opt4060_calculate_crc(u8 exp, u32 mantissa, u8 count)
+{
+	u8 crc;
+
+	/*
+	 * Calculates a 4-bit CRC from a 20-bit mantissa, 4-bit exponent and a 4-bit counter.
+	 * crc[0] = XOR(mantissa[19:0], exp[3:0], count[3:0])
+	 * crc[1] = XOR(mantissa[1,3,5,7,9,11,13,15,17,19], exp[1,3], count[1,3])
+	 * crc[2] = XOR(mantissa[3,7,11,15,19], exp[3], count[3])
+	 * crc[3] = XOR(mantissa[3,11,19])
+	 */
+	crc = (hweight32(mantissa) + hweight32(exp) + hweight32(count)) % 2;
+	crc |= ((hweight32(mantissa & 0xAAAAA) + hweight32(exp & 0xA)
+		 + hweight32(count & 0xA)) % 2) << 1;
+	crc |= ((hweight32(mantissa & 0x88888) + hweight32(exp & 0x8)
+		 + hweight32(count & 0x8)) % 2) << 2;
+	crc |= (hweight32(mantissa & 0x80808) % 2) << 3;
+
+	return crc;
+}
+
+static int opt4060_set_int_state(struct opt4060_chip *chip, u32 state)
+{
+	int ret;
+	unsigned int regval;
+
+	regval = FIELD_PREP(OPT4060_INT_CTRL_INT_CFG, state);
+	ret = regmap_update_bits(chip->regmap, OPT4060_INT_CTRL,
+				 OPT4060_INT_CTRL_INT_CFG, regval);
+	if (ret)
+		dev_err(chip->dev, "Failed to set interrupt config\n");
+	return ret;
+}
+
+static int opt4060_trigger_one_shot(struct opt4060_chip *chip)
+{
+	int ret;
+	unsigned int ctrl_reg;
+
+	/* Enable interrupt for conversion ready of all channels */
+	ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_ALL_CH);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(chip->regmap, OPT4060_CTRL, &ctrl_reg);
+	if (ret < 0) {
+		dev_err(chip->dev, "Failed to read ctrl register\n");
+		return ret;
+	}
+
+	/* If the device is already in continuous mode, one-shot is not needed. */
+	if (FIELD_GET(OPT4060_CTRL_OPER_MODE_MASK, ctrl_reg) ==
+	    OPT4060_CTRL_OPER_MODE_CONTINUOUS)
+		return 0;
+
+	ctrl_reg &= ~OPT4060_CTRL_OPER_MODE_MASK;
+	ctrl_reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
+				OPT4060_CTRL_OPER_MODE_ONE_SHOT);
+
+	/* Trigger a new conversion by writing to CRTL register. */
+	ret = regmap_write(chip->regmap, OPT4060_CTRL, ctrl_reg);
+	if (ret)
+		dev_err(chip->dev, "Failed to set ctrl register\n");
+	return ret;
+}
+
+static int opt4060_set_continous_mode(struct opt4060_chip *chip,
+				      bool continuous)
+{
+	u16 reg;
+	int ret;
+
+	if (continuous)
+		reg = FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
+				 OPT4060_CTRL_OPER_MODE_CONTINUOUS);
+	else
+		reg = FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
+				 OPT4060_CTRL_OPER_MODE_ONE_SHOT);
+
+	ret = regmap_update_bits(chip->regmap, OPT4060_CTRL,
+				 OPT4060_CTRL_OPER_MODE_MASK, reg);
+	if (ret)
+		dev_err(chip->dev, "Failed to set configuration\n");
+
+	return ret;
+}
+
+static int opt4060_read_raw_value(struct opt4060_chip *chip,
+				  unsigned long address, u32 *raw)
+{
+	int ret;
+	u16 result[2];
+	u32 mantissa_raw;
+	u16 msb, lsb;
+	u8 exp, count, crc, calc_crc;
+
+	ret = regmap_bulk_read(chip->regmap, address, result, 2);
+	if (ret) {
+		dev_err(chip->dev, "Reading channel data failed\n");
+		return ret;
+	}
+	exp = FIELD_GET(OPT4060_EXPONENT_MASK, result[0]);
+	msb = FIELD_GET(OPT4060_MSB_MASK, result[0]);
+	count = FIELD_GET(OPT4060_COUNTER_MASK, result[1]);
+	crc = FIELD_GET(OPT4060_CRC_MASK, result[1]);
+	lsb = FIELD_GET(OPT4060_LSB_MASK, result[1]);
+	mantissa_raw = (msb << 8) + lsb;
+	calc_crc = opt4060_calculate_crc(exp, mantissa_raw, count);
+	if (calc_crc != crc)
+		return -EIO;
+	*raw = mantissa_raw << exp;
+	return 0;
+}
+
+static int opt4060_trigger_new_samples(struct iio_dev *indio_dev)
+{
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+	int ret;
+
+	/*
+	 * The conversion time should be 500us startup time plus the integration time
+	 * times the number of channels. An exact timeout isn't critical, it's better
+	 * not to get incorrect errors in the log. Setting the timeout to double the
+	 * theoretical time plus and extra 100ms margin.
+	 */
+	unsigned int timeout_us = (500 + OPT4060_NUM_CHANS *
+				  opt4060_int_time_reg[chip->int_time][0]) * 2 + 100000;
+
+	if (chip->irq) {
+		reinit_completion(&chip->completion);
+		ret = opt4060_trigger_one_shot(chip);
+		if (ret)
+			return ret;
+		if (wait_for_completion_timeout(&chip->completion,
+						usecs_to_jiffies(timeout_us)) == 0) {
+			dev_err(chip->dev, "Completion timed out.\n");
+			return -ETIME;
+		}
+		/*
+		 * The opt4060_trigger_one_shot() function will enable irq on
+		 * every conversion. If the buffer isn't enabled, irq should
+		 * only be enabled for thresholds.
+		 */
+		if (!iio_buffer_enabled(indio_dev)) {
+			ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_THRESHOLD);
+			if (ret)
+				return ret;
+		}
+	} else {
+		unsigned int ready;
+
+		ret = opt4060_trigger_one_shot(chip);
+		if (ret)
+			return ret;
+
+		ret = regmap_read_poll_timeout(chip->regmap, OPT4060_RES_CTRL,
+					       ready, (ready & OPT4060_RES_CTRL_CONV_READY),
+					       1000, timeout_us);
+		if (ret)
+			dev_err(chip->dev, "Conversion ready did not finish within timeout.\n");
+	}
+	return ret;
+}
+
+static int opt4060_read_chan_raw(struct iio_dev *indio_dev,
+				 struct iio_chan_spec const *chan, int *val)
+{
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+	u32 adc_raw;
+	int ret;
+
+	ret = opt4060_trigger_new_samples(indio_dev);
+	if (ret) {
+		dev_err(chip->dev, "Failed to trigger new samples.\n");
+		return ret;
+	}
+
+	ret = opt4060_read_raw_value(chip, chan->address, &adc_raw);
+	if (ret) {
+		dev_err(chip->dev, "Reading raw channel data failed.\n");
+		return ret;
+	}
+	*val = adc_raw;
+	return IIO_VAL_INT;
+}
+
+/*
+ * Function for calculating color components independent of light intensity. The
+ * raw values are multiplied by individual factors that correspond to the
+ * sensitivity of the different color filters. The returned value is normalized.
+ */
+static int opt4060_read_chan_scale(struct iio_dev *indio_dev,
+				   struct iio_chan_spec const *chan,
+				   int *val, int *val2)
+{
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+	static const u32 color_factors[] = { 24, 10, 13 };
+	u32 adc_raw[3], sum = 0, rem;
+	int ret;
+
+	ret = opt4060_trigger_new_samples(indio_dev);
+	if (ret) {
+		dev_err(chip->dev, "Failed to trigger new samples.\n");
+		return ret;
+	}
+
+	for (int color = OPT4060_RED; color <= OPT4060_BLUE; color++) {
+		ret = opt4060_read_raw_value(chip, indio_dev->channels[color].address,
+					     &(adc_raw[color]));
+		if (ret) {
+			dev_err(chip->dev, "Reading raw channel data failed\n");
+			return ret;
+		}
+		adc_raw[color] *= color_factors[color];
+		sum += adc_raw[color];
+	}
+	*val = div_u64_rem((u64)adc_raw[chan->scan_index], sum, &rem);
+	*val2 = DIV_U64_ROUND_CLOSEST((u64)(rem * MICRO), sum);
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int opt4060_calc_illuminance(struct opt4060_chip *chip, int *val)
+{
+	u32 lux_raw;
+	int ret;
+
+	/* The green wide spectral channel is used for illuminance. */
+	ret = opt4060_read_raw_value(chip, OPT4060_GREEN_MSB, &lux_raw);
+	if (ret) {
+		dev_err(chip->dev, "Reading raw channel data failed\n");
+		return ret;
+	}
+
+	/* Illuminance is calculated by ADC_RAW * 2.15e-3. */
+	*val = DIV_U64_ROUND_CLOSEST((u64)(lux_raw * 215), 1000);
+	return ret;
+}
+
+static int opt4060_read_illuminance(struct iio_dev *indio_dev,
+				    struct iio_chan_spec const *chan,
+				    int *val)
+{
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+	int ret;
+
+	ret = opt4060_trigger_new_samples(indio_dev);
+	if (ret) {
+		dev_err(chip->dev, "Failed to trigger new samples.\n");
+		return ret;
+	}
+	ret = opt4060_calc_illuminance(chip, val);
+	if (ret) {
+		dev_err(chip->dev, "Failed to calculate illuminance.\n");
+		return ret;
+	}
+
+	return IIO_VAL_INT;
+}
+
+static int opt4060_set_int_time(struct opt4060_chip *chip)
+{
+	unsigned int regval;
+	int ret;
+
+	regval = FIELD_PREP(OPT4060_CTRL_CONV_TIME_MASK, chip->int_time);
+	ret = regmap_update_bits(chip->regmap, OPT4060_CTRL,
+				 OPT4060_CTRL_CONV_TIME_MASK, regval);
+	if (ret)
+		dev_err(chip->dev, "Failed to set integration time.\n");
+
+	return ret;
+}
+
+static int opt4060_power_down(struct opt4060_chip *chip)
+{
+	int ret;
+
+	ret = regmap_clear_bits(chip->regmap, OPT4060_CTRL, OPT4060_CTRL_OPER_MODE_MASK);
+	if (ret)
+		dev_err(chip->dev, "Failed to power down\n");
+
+	return ret;
+}
+
+static void opt4060_chip_off_action(void *chip)
+{
+	opt4060_power_down(chip);
+}
+
+#define _OPT4060_COLOR_CHANNEL(_color, _mask, _ev_spec, _num_ev_spec)		\
+{										\
+	.type = IIO_INTENSITY,							\
+	.modified = 1,								\
+	.channel2 = IIO_MOD_LIGHT_##_color,					\
+	.info_mask_separate = _mask,						\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),			\
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),	\
+	.address = OPT4060_##_color##_MSB,					\
+	.scan_index = OPT4060_##_color,						\
+	.scan_type = {								\
+		.sign = 'u',							\
+		.realbits = 32,							\
+		.storagebits = 32,						\
+		.endianness = IIO_CPU,						\
+	},									\
+	.event_spec = _ev_spec,							\
+	.num_event_specs = _num_ev_spec,					\
+}
+
+#define OPT4060_COLOR_CHANNEL(_color, _mask)					\
+	_OPT4060_COLOR_CHANNEL(_color, _mask, opt4060_event_spec,		\
+			       ARRAY_SIZE(opt4060_event_spec))			\
+
+#define OPT4060_COLOR_CHANNEL_NO_EVENTS(_color, _mask)				\
+	_OPT4060_COLOR_CHANNEL(_color, _mask, NULL, 0)				\
+
+#define OPT4060_LIGHT_CHANNEL(_channel)						\
+{										\
+	.type = IIO_LIGHT,							\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED),			\
+	.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),			\
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),	\
+	.scan_index = OPT4060_##_channel,					\
+	.scan_type = {								\
+		.sign = 'u',							\
+		.realbits = 32,							\
+		.storagebits = 32,						\
+		.endianness = IIO_CPU,						\
+	},									\
+}
+
+static const struct iio_event_spec opt4060_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_PERIOD),
+	},
+};
+
+static const struct iio_chan_spec opt4060_channels[] = {
+	OPT4060_COLOR_CHANNEL(RED, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)),
+	OPT4060_COLOR_CHANNEL(GREEN, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)),
+	OPT4060_COLOR_CHANNEL(BLUE, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)),
+	OPT4060_COLOR_CHANNEL(CLEAR, BIT(IIO_CHAN_INFO_RAW)),
+	OPT4060_LIGHT_CHANNEL(ILLUM),
+	IIO_CHAN_SOFT_TIMESTAMP(OPT4060_NUM_CHANS),
+};
+
+static const struct iio_chan_spec opt4060_channels_no_events[] = {
+	OPT4060_COLOR_CHANNEL_NO_EVENTS(RED, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)),
+	OPT4060_COLOR_CHANNEL_NO_EVENTS(GREEN, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)),
+	OPT4060_COLOR_CHANNEL_NO_EVENTS(BLUE, BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE)),
+	OPT4060_COLOR_CHANNEL_NO_EVENTS(CLEAR, BIT(IIO_CHAN_INFO_RAW)),
+	OPT4060_LIGHT_CHANNEL(ILLUM),
+	IIO_CHAN_SOFT_TIMESTAMP(OPT4060_NUM_CHANS),
+};
+
+static int opt4060_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return opt4060_read_chan_raw(indio_dev, chan, val);
+	case IIO_CHAN_INFO_SCALE:
+		return opt4060_read_chan_scale(indio_dev, chan, val, val2);
+	case IIO_CHAN_INFO_PROCESSED:
+		return opt4060_read_illuminance(indio_dev, chan, val);
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = 0;
+		*val2 = opt4060_int_time_reg[chip->int_time][0];
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int opt4060_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+	int int_time;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		int_time = opt4060_als_time_to_index(val2);
+		if (int_time < 0)
+			return int_time;
+		chip->int_time = int_time;
+		return opt4060_set_int_time(chip);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int opt4060_write_raw_get_fmt(struct iio_dev *indio_dev,
+				     struct iio_chan_spec const *chan,
+				     long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static u32 opt4060_calc_th_reg(u32 adc_val)
+{
+	u32 th_val, th_exp, bits;
+	/*
+	 * The threshold registers take 4 bits of exponent and 12 bits of data
+	 * ADC = TH_VAL << (8 + TH_EXP)
+	 */
+	bits = fls(adc_val);
+
+	if (bits > 31)
+		th_exp = 11; /* Maximum exponent */
+	else if (bits > 20)
+		th_exp = bits - 20;
+	else
+		th_exp = 0;
+	th_val = (adc_val >> (8 + th_exp)) & 0xfff;
+
+	return (th_exp << 12) + th_val;
+}
+
+static u32 opt4060_calc_val_from_th_reg(u32 th_reg)
+{
+	/*
+	 * The threshold registers take 4 bits of exponent and 12 bits of data
+	 * ADC = TH_VAL << (8 + TH_EXP)
+	 */
+	u32 th_val, th_exp;
+
+	th_exp = (th_reg >> 12) & 0xf;
+	th_val = th_reg & 0xfff;
+
+	return th_val << (8 + th_exp);
+}
+
+static bool opt4060_event_active(struct opt4060_chip *chip)
+{
+	return chip->thresh_event_lo_active || chip->thresh_event_hi_active;
+}
+
+static int opt4060_read_available(struct iio_dev *indio_dev,
+				  struct iio_chan_spec const *chan,
+				  const int **vals, int *type, int *length,
+				  long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		*length = ARRAY_SIZE(opt4060_int_time_available) * 2;
+		*vals = (const int *)opt4060_int_time_available;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int opt4060_event_set_state(struct iio_dev *indio_dev, bool state)
+{
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+	int ret = 0;
+
+	if (state)
+		ret = opt4060_set_continous_mode(chip, true);
+	else if (!iio_buffer_enabled(indio_dev) && chip->irq)
+		ret = opt4060_set_continous_mode(chip, false);
+
+	if (ret)
+		dev_err(chip->dev, "Failed to set event state.\n");
+
+	return ret;
+}
+
+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;
+		ret = opt4060_set_continous_mode(chip, true);
+	} else {
+		if (!opt4060_event_active(chip) && chip->irq) {
+			ret = opt4060_set_continous_mode(chip, false);
+			if (ret)
+				return ret;
+		}
+		ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_THRESHOLD);
+	}
+	return ret;
+}
+
+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;
+		break;
+	case OPT4060_CTRL_FAULT_COUNT_4:
+		pers = 4;
+		break;
+	case OPT4060_CTRL_FAULT_COUNT_8:
+		pers = 8;
+		break;
+
+	default:
+		pers = 1;
+	}
+
+	uval = mul_u32_u32(int_time, pers);
+	*val = div_u64_rem(uval, MICRO, val2);
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static ssize_t opt4060_write_ev_period(struct opt4060_chip *chip, int val,
+				      int val2)
+{
+	u64 uval, int_time;
+	unsigned int regval, fault_count_val;
+
+	uval = mul_u32_u32(val, MICRO) + val2;
+	int_time = opt4060_int_time_reg[chip->int_time][0];
+
+	/* Check if the period is closest to 1, 2, 4 or 8 times integration time.*/
+	if (uval <= int_time)
+		fault_count_val = OPT4060_CTRL_FAULT_COUNT_1;
+	else if (uval <= int_time * 2)
+		fault_count_val = OPT4060_CTRL_FAULT_COUNT_2;
+	else if (uval <= int_time * 4)
+		fault_count_val = OPT4060_CTRL_FAULT_COUNT_4;
+	else
+		fault_count_val = OPT4060_CTRL_FAULT_COUNT_8;
+
+	regval = FIELD_PREP(OPT4060_CTRL_FAULT_COUNT_MASK, fault_count_val);
+	return regmap_update_bits(chip->regmap, OPT4060_CTRL,
+				 OPT4060_CTRL_FAULT_COUNT_MASK, regval);
+}
+
+static int opt4060_get_channel_sel(struct opt4060_chip *chip, int *ch_sel)
+{
+	int ret;
+	u32 regval;
+
+	ret = regmap_read(chip->regmap, OPT4060_INT_CTRL, &regval);
+	if (ret)
+		dev_err(chip->dev, "Failed to get channel selection.\n");
+	*ch_sel = FIELD_GET(OPT4060_INT_CTRL_THRESH_SEL, regval);
+	return ret;
+}
+
+static int opt4060_set_channel_sel(struct opt4060_chip *chip, int ch_sel)
+{
+	int ret;
+	u32 regval;
+
+	regval = FIELD_PREP(OPT4060_INT_CTRL_THRESH_SEL, ch_sel);
+	ret = regmap_update_bits(chip->regmap, OPT4060_INT_CTRL,
+				 OPT4060_INT_CTRL_THRESH_SEL, regval);
+	if (ret)
+		dev_err(chip->dev, "Failed to set channel selection.\n");
+	return ret;
+}
+
+static int opt4060_get_thresholds(struct opt4060_chip *chip, u32 *th_lo, u32 *th_hi)
+{
+	int ret;
+	u32 regval;
+
+	ret = regmap_read(chip->regmap, OPT4060_THRESHOLD_LOW, &regval);
+	if (ret) {
+		dev_err(chip->dev, "Failed to read THRESHOLD_LOW.\n");
+		return ret;
+	}
+	*th_lo = opt4060_calc_val_from_th_reg(regval);
+
+	ret = regmap_read(chip->regmap, OPT4060_THRESHOLD_HIGH, &regval);
+	if (ret) {
+		dev_err(chip->dev, "Failed to read THRESHOLD_LOW.\n");
+		return ret;
+	}
+	*th_hi = opt4060_calc_val_from_th_reg(regval);
+
+	return ret;
+}
+
+static int opt4060_set_thresholds(struct opt4060_chip *chip, u32 th_lo, u32 th_hi)
+{
+	int ret;
+
+	ret = regmap_write(chip->regmap, OPT4060_THRESHOLD_LOW, opt4060_calc_th_reg(th_lo));
+	if (ret) {
+		dev_err(chip->dev, "Failed to write THRESHOLD_LOW.\n");
+		return ret;
+	}
+
+	ret = regmap_write(chip->regmap, OPT4060_THRESHOLD_HIGH, opt4060_calc_th_reg(th_hi));
+	if (ret)
+		dev_err(chip->dev, "Failed to write THRESHOLD_HIGH.\n");
+
+	return ret;
+}
+
+static int opt4060_read_event(struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan,
+			      enum iio_event_type type,
+			      enum iio_event_direction dir,
+			      enum iio_event_info info,
+			      int *val, int *val2)
+{
+	int ret;
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+
+	if (chan->type != IIO_INTENSITY)
+		return -EINVAL;
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		u32 th_lo, th_hi;
+
+		ret = opt4060_get_thresholds(chip, &th_lo, &th_hi);
+		if (ret)
+			return ret;
+		if (dir == IIO_EV_DIR_FALLING) {
+			*val = th_lo;
+			ret = IIO_VAL_INT;
+		} else if (dir == IIO_EV_DIR_RISING) {
+			*val = th_hi;
+			ret = IIO_VAL_INT;
+		}
+		return ret;
+	case IIO_EV_INFO_PERIOD:
+		return opt4060_read_ev_period(chip, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int opt4060_write_event(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       enum iio_event_type type,
+			       enum iio_event_direction dir,
+			       enum iio_event_info info,
+			       int val, int val2)
+{
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_INTENSITY)
+		return -EINVAL;
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		u32 th_lo, th_hi;
+
+		ret = opt4060_get_thresholds(chip, &th_lo, &th_hi);
+		if (ret)
+			return ret;
+		if (dir == IIO_EV_DIR_FALLING)
+			th_lo = val;
+		else if (dir == IIO_EV_DIR_RISING)
+			th_hi = val;
+		return opt4060_set_thresholds(chip, th_lo, th_hi);
+	case IIO_EV_INFO_PERIOD:
+		return opt4060_write_ev_period(chip, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int opt4060_read_event_config(struct iio_dev *indio_dev,
+				     const struct iio_chan_spec *chan,
+				     enum iio_event_type type,
+				     enum iio_event_direction dir)
+{
+	int ch_sel, ch_idx = chan->scan_index;
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_INTENSITY)
+		return -EINVAL;
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	ret = opt4060_get_channel_sel(chip, &ch_sel);
+	if (ret)
+		return ret;
+
+	if (((dir == IIO_EV_DIR_FALLING) && chip->thresh_event_lo_active) ||
+	    ((dir == IIO_EV_DIR_RISING) && chip->thresh_event_hi_active))
+		return ch_sel == ch_idx;
+
+	return ret;
+}
+
+static int opt4060_write_event_config(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir, int state)
+{
+	int ch_sel, ch_idx = chan->scan_index;
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+	int ret;
+
+	if (chan->type != IIO_INTENSITY)
+		return -EINVAL;
+	if (type != IIO_EV_TYPE_THRESH)
+		return -EINVAL;
+
+	ret = opt4060_get_channel_sel(chip, &ch_sel);
+	if (ret)
+		return ret;
+
+	if (state) {
+		/* Only one channel can be active at the same time */
+		if ((chip->thresh_event_lo_active ||
+			chip->thresh_event_hi_active) && (ch_idx != ch_sel))
+			return -EBUSY;
+		if (dir == IIO_EV_DIR_FALLING)
+			chip->thresh_event_lo_active = true;
+		else if (dir == IIO_EV_DIR_RISING)
+			chip->thresh_event_hi_active = true;
+		ret = opt4060_set_channel_sel(chip, ch_idx);
+		if (ret)
+			return ret;
+	} else {
+		if (ch_idx == ch_sel) {
+			if (dir == IIO_EV_DIR_FALLING)
+				chip->thresh_event_lo_active = false;
+			else if (dir == IIO_EV_DIR_RISING)
+				chip->thresh_event_hi_active = false;
+		}
+	}
+
+	return opt4060_event_set_state(indio_dev, chip->thresh_event_hi_active |
+				       chip->thresh_event_lo_active);
+}
+
+static const struct iio_info opt4060_info = {
+	.read_raw = opt4060_read_raw,
+	.write_raw = opt4060_write_raw,
+	.write_raw_get_fmt = opt4060_write_raw_get_fmt,
+	.read_avail = opt4060_read_available,
+	.read_event_value = opt4060_read_event,
+	.write_event_value = opt4060_write_event,
+	.read_event_config = opt4060_read_event_config,
+	.write_event_config = opt4060_write_event_config,
+};
+
+static int opt4060_load_defaults(struct opt4060_chip *chip)
+{
+	u16 reg;
+	int ret;
+
+	chip->int_time = OPT4060_DEFAULT_CONVERSION_TIME;
+
+	/* Set initial MIN/MAX thresholds */
+	ret = opt4060_set_thresholds(chip, 0, UINT_MAX);
+	if (ret)
+		return ret;
+
+	/*
+	 * Setting auto-range, latched window for thresholds, one-shot conversion
+	 * and quick wake-up mode as default.
+	 */
+	reg = FIELD_PREP(OPT4060_CTRL_RANGE_MASK,
+			 OPT4060_CTRL_LIGHT_SCALE_AUTO);
+	reg |= FIELD_PREP(OPT4060_CTRL_CONV_TIME_MASK, chip->int_time);
+	reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
+				OPT4060_CTRL_OPER_MODE_ONE_SHOT);
+	reg |= OPT4060_CTRL_QWAKE_MASK | OPT4060_CTRL_LATCH_MASK;
+
+	ret = regmap_write(chip->regmap, OPT4060_CTRL, reg);
+	if (ret)
+		dev_err(chip->dev, "Failed to set configuration\n");
+
+	return ret;
+}
+
+static bool opt4060_volatile_reg(struct device *dev, unsigned int reg)
+{
+	return reg <= OPT4060_CLEAR_LSB || reg == OPT4060_RES_CTRL;
+}
+
+static bool opt4060_writable_reg(struct device *dev, unsigned int reg)
+{
+	return reg >= OPT4060_THRESHOLD_LOW || reg >= OPT4060_INT_CTRL;
+}
+
+static bool opt4060_readonly_reg(struct device *dev, unsigned int reg)
+{
+	return reg == OPT4060_DEVICE_ID;
+}
+
+static bool opt4060_readable_reg(struct device *dev, unsigned int reg)
+{
+	/* Volatile, writable and read-only registers are readable. */
+	return opt4060_volatile_reg(dev, reg) || opt4060_writable_reg(dev, reg) ||
+	       opt4060_readonly_reg(dev, reg);
+}
+
+static const struct regmap_config opt4060_regmap_config = {
+	.name = "opt4060",
+	.reg_bits = 8,
+	.val_bits = 16,
+	.cache_type = REGCACHE_RBTREE,
+	.max_register = OPT4060_DEVICE_ID,
+	.readable_reg = opt4060_readable_reg,
+	.writeable_reg = opt4060_writable_reg,
+	.volatile_reg = opt4060_volatile_reg,
+	.val_format_endian = REGMAP_ENDIAN_BIG,
+};
+
+static const struct iio_trigger_ops opt4060_trigger_ops = {
+	.validate_device = iio_trigger_validate_own_device,
+	.set_trigger_state = opt4060_trigger_set_state,
+};
+
+static irqreturn_t opt4060_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *idev = pf->indio_dev;
+	struct opt4060_chip *chip = iio_priv(idev);
+	struct opt4060_buffer raw;
+	int ret, chan = 0, i = 0;
+
+	memset(&raw, 0, sizeof(raw));
+
+	iio_for_each_active_channel(idev, chan) {
+		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 */
+		if (int_res & OPT4060_RES_CTRL_FLAG_L) {
+			code = IIO_UNMOD_EVENT_CODE(IIO_INTENSITY,
+						    chan,
+						    IIO_EV_TYPE_THRESH,
+						    IIO_EV_DIR_FALLING);
+			iio_push_event(idev, code, iio_get_time_ns(idev));
+		}
+		/* Check if the interrupt is from the upper threshold */
+		if (int_res & OPT4060_RES_CTRL_FLAG_H) {
+			code = IIO_UNMOD_EVENT_CODE(IIO_INTENSITY,
+						    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)
+{
+	struct iio_trigger *data_trigger;
+	char *name;
+	int ret;
+
+	ret = devm_iio_triggered_buffer_setup(chip->dev, idev,
+					      &iio_pollfunc_store_time,
+					      opt4060_trigger_handler, NULL);
+	if (ret)
+		return dev_err_probe(chip->dev, ret,
+				     "iio_triggered_buffer_setup_ext FAIL\n");
+
+	data_trigger = devm_iio_trigger_alloc(chip->dev, "%s-data-ready-dev%d",
+					      idev->name, iio_device_id(idev));
+	if (!data_trigger)
+		return -ENOMEM;
+
+	/* The data trigger allows for sample capture on each new conversion ready interrupt. */
+	chip->trig = data_trigger;
+	data_trigger->ops = &opt4060_trigger_ops;
+	iio_trigger_set_drvdata(data_trigger, idev);
+	ret = devm_iio_trigger_register(chip->dev, data_trigger);
+	if (ret)
+		return dev_err_probe(chip->dev, ret,
+				     "Data ready trigger registration failed\n");
+
+	name = devm_kasprintf(chip->dev, GFP_KERNEL, "%s-opt4060",
+			      dev_name(chip->dev));
+	if (!name)
+		return dev_err_probe(chip->dev, -ENOMEM, "Failed to alloc chip name\n");
+
+	ret = devm_request_threaded_irq(chip->dev, chip->irq, NULL, opt4060_irq_thread,
+					IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING |
+					IRQF_ONESHOT, name, idev);
+	if (ret)
+		return dev_err_probe(chip->dev, ret, "Could not request IRQ\n");
+
+	ret = regmap_write_bits(chip->regmap, OPT4060_INT_CTRL,
+				OPT4060_INT_CTRL_OUTPUT,
+				OPT4060_INT_CTRL_OUTPUT);
+	if (ret)
+		return dev_err_probe(chip->dev, ret,
+				     "Failed to set interrupt as output\n");
+
+	return 0;
+}
+
+static int opt4060_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct opt4060_chip *chip;
+	struct iio_dev *indio_dev;
+	int ret;
+	uint dev_id;
+
+	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)
+		return dev_err_probe(dev, ret, "Failed to enable vdd supply\n");
+
+	chip->regmap = devm_regmap_init_i2c(client, &opt4060_regmap_config);
+	if (IS_ERR(chip->regmap))
+		return dev_err_probe(dev, PTR_ERR(chip->regmap),
+				     "regmap initialization failed\n");
+
+	chip->dev = dev;
+	chip->irq = client->irq;
+	init_completion(&chip->completion);
+
+	indio_dev->info = &opt4060_info;
+
+	ret = regmap_reinit_cache(chip->regmap, &opt4060_regmap_config);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "failed to reinit regmap cache\n");
+
+	ret = regmap_read(chip->regmap, OPT4060_DEVICE_ID, &dev_id);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+			"Failed to read the device ID register\n");
+
+	dev_id = FIELD_GET(OPT4060_DEVICE_ID_MASK, dev_id);
+	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)
+		return dev_err_probe(dev, ret,
+				     "Failed to set sensor defaults\n");
+
+	ret = devm_add_action_or_reset(dev, opt4060_chip_off_action, chip);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Failed to setup power off action\n");
+
+	if (chip->irq) {
+		ret = opt4060_setup_triggers(chip, indio_dev);
+		if (ret)
+			return ret;
+	}
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct i2c_device_id opt4060_id[] = {
+	{ "opt4060", },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, opt4060_id);
+
+static const struct of_device_id opt4060_of_match[] = {
+	{ .compatible = "ti,opt4060" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, opt4060_of_match);
+
+static struct i2c_driver opt4060_driver = {
+	.driver = {
+		.name = "opt4060",
+		.of_match_table = opt4060_of_match,
+	},
+	.probe = opt4060_probe,
+	.id_table = opt4060_id,
+};
+module_i2c_driver(opt4060_driver);
+
+MODULE_DESCRIPTION("Texas Instruments OPT4060 RGBW color sensor driver");
+MODULE_LICENSE("GPL");