[v2] Support for Texas Instruments OPT4060 RGBW Color sensor.

[PATCH v2 2/2] iio: light: Add support for TI OPT4060 color sensor

Posted by Per-Daniel Olsson 1 month, 3 weeks ago

Add support for Texas Instruments OPT4060 RGBW Color sensor.

Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.com>
---
 drivers/iio/light/Kconfig   |   13 +
 drivers/iio/light/Makefile  |    1 +
 drivers/iio/light/opt4060.c | 1216 +++++++++++++++++++++++++++++++++++
 3 files changed, 1230 insertions(+)
 create mode 100644 drivers/iio/light/opt4060.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 515ff46b5b82..0e2059512157 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -490,6 +490,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 321010fc0b93..55902b21ec0c 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..a917b59cd500
--- /dev/null
+++ b/drivers/iio/light/opt4060.c
@@ -0,0 +1,1216 @@
+// 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>
+
+#define OPT_4060_DRV_NAME	"opt4060"
+
+/* 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_NUM_CHANS:	Number of channel types.
+ */
+enum opt4060_chan_type {
+	OPT4060_RED,
+	OPT4060_GREEN,
+	OPT4060_BLUE,
+	OPT4060_CLEAR,
+	OPT4060_NUM_CHANS
+};
+
+struct opt4060_chip {
+	struct regmap *regmap;
+	struct device *dev;
+	struct iio_trigger *data_trig;
+	bool data_trigger_active;
+	struct iio_trigger *threshold_trig;
+	bool threshold_trigger_active;
+	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];
+	s64 ts;
+};
+
+static const struct opt4060_channel_factor opt4060_channel_factors[] = {
+	{
+		/* RED 2.4 * 2.15e-3 */
+		.mul = 516,
+		.div = 100000,
+	}, {
+		/* GREEN 1.0 * 2.15e-3 */
+		.mul = 215,
+		.div = 100000,
+	}, {
+		/* BLUE 1.3 * 2.15e-3 */
+		.mul = 258,
+		.div = 100000,
+	}, {
+		/* CLEAR 1.0 * 2.15e-3 */
+		.mul = 215,
+		.div = 100000,
+	},
+};
+
+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;
+
+	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, oper_mode;
+
+	/* 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;
+	}
+
+	oper_mode = FIELD_GET(OPT4060_CTRL_OPER_MODE_MASK, ctrl_reg);
+
+	/* If the device is already in continuous mode, one-shot is not needed. */
+	if (oper_mode != OPT4060_CTRL_OPER_MODE_CONTINUOUS) {
+		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;
+	u32 result;
+	u16 msb;
+	u16 lsb;
+	u8 exp;
+	u8 count;
+	u8 crc;
+	u8 calc_crc;
+	u32 mantissa_raw;
+
+	ret = regmap_bulk_read(chip->regmap, address, &result, 2);
+	if (ret)
+		dev_err(chip->dev, "Reading channel data failed\n");
+	else {
+		count = FIELD_GET(OPT4060_COUNTER_MASK, result >> 16);
+		crc = FIELD_GET(OPT4060_CRC_MASK, result >> 16);
+		lsb = FIELD_GET(OPT4060_LSB_MASK, result >> 16);
+		exp = FIELD_GET(OPT4060_EXPONENT_MASK, result);
+		msb = FIELD_GET(OPT4060_MSB_MASK, result);
+		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 ret;
+}
+
+static int opt4060_read_chan_value(struct iio_dev *indio_dev,
+				   struct iio_chan_spec const *chan,
+				   int *val, int *val2, bool processed)
+{
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+	u32 adc_raw;
+	int ret;
+	/* Set timeout to two times the integration time multiplied by channel count. */
+	unsigned int timeout_us = 2 * OPT4060_NUM_CHANS *
+				  opt4060_int_time_reg[chip->int_time][0];
+
+	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_info(chip->dev, "Completion timed out.\n");
+	}
+
+	ret = opt4060_read_raw_value(chip, chan->address, &adc_raw);
+	if (ret) {
+		dev_err(chip->dev, "Reading raw channel data failed\n");
+		return ret;
+	}
+	if (processed) {
+		u32 lux_raw;
+		u32 rem;
+		u32 mul = opt4060_channel_factors[chan->scan_index].mul;
+		u32 div = opt4060_channel_factors[chan->scan_index].div;
+
+		lux_raw = adc_raw * mul;
+		*val = div_u64_rem(lux_raw, div, &rem);
+		*val2 = rem * div_u64(div, 10);
+		return IIO_VAL_INT_PLUS_NANO;
+	}
+	*val = adc_raw;
+	*val2 = 0;
+	return IIO_VAL_INT;
+}
+
+static int opt4060_set_conf(struct opt4060_chip *chip)
+{
+	u16 reg;
+	int ret;
+
+	reg = FIELD_PREP(OPT4060_CTRL_RANGE_MASK,
+			 OPT4060_CTRL_LIGHT_SCALE_AUTO);
+	reg |= FIELD_PREP(OPT4060_CTRL_CONV_TIME_MASK, chip->int_time);
+	if (chip->irq)
+		reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
+				  OPT4060_CTRL_OPER_MODE_ONE_SHOT);
+	else
+		reg |= FIELD_PREP(OPT4060_CTRL_OPER_MODE_MASK,
+				  OPT4060_CTRL_OPER_MODE_CONTINUOUS);
+	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 int opt4060_power_down(struct opt4060_chip *chip)
+{
+	int ret;
+	unsigned int reg;
+
+	ret = regmap_read(chip->regmap, OPT4060_CTRL, &reg);
+	if (ret) {
+		dev_err(chip->dev, "Failed to read configuration\n");
+		return ret;
+	}
+
+	/* MODE_OFF is 0x0 so just set bits to 0 */
+	reg &= ~OPT4060_CTRL_OPER_MODE_MASK;
+
+	ret = regmap_write(chip->regmap, OPT4060_CTRL, reg);
+	if (ret)
+		dev_err(chip->dev, "Failed to power down\n");
+
+	return ret;
+}
+
+static void opt4060_chip_off_action(void *data)
+{
+	struct opt4060_chip *chip = data;
+
+	opt4060_power_down(chip);
+}
+
+#define OPT4060_CHAN(color)							\
+{										\
+	.type = IIO_LIGHT,							\
+	.modified = 1,								\
+	.channel2 = IIO_MOD_LIGHT_##color,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |			\
+			      BIT(IIO_CHAN_INFO_RAW),				\
+	.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_LE,						\
+	},									\
+	.event_spec = opt4060_event_spec,					\
+	.num_event_specs = ARRAY_SIZE(opt4060_event_spec),			\
+}
+
+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_CHAN(RED),
+	OPT4060_CHAN(GREEN),
+	OPT4060_CHAN(BLUE),
+	OPT4060_CHAN(CLEAR),
+	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_value(indio_dev, chan, val, val2,
+					       false);
+	case IIO_CHAN_INFO_PROCESSED:
+		return opt4060_read_chan_value(indio_dev, chan, val, val2,
+					       true);
+	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_conf(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;
+	u32 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 opt4060_chip *chip, bool state)
+{
+	int ret = 0;
+
+	if (state)
+		ret = opt4060_set_continous_mode(chip, true);
+	else if (!chip->data_trigger_active && chip->irq)
+		ret = opt4060_set_continous_mode(chip, false);
+
+	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 (indio_dev->trig) {
+		iio_trigger_put(indio_dev->trig);
+		indio_dev->trig = NULL;
+	}
+
+	if (state) {
+		indio_dev->trig = iio_trigger_get(trig);
+		ret = opt4060_set_continous_mode(chip, true);
+	} else if (!opt4060_event_active(chip) && chip->irq)
+		ret = opt4060_set_continous_mode(chip, false);
+
+	if (trig == chip->data_trig) {
+		chip->data_trigger_active = state;
+		chip->threshold_trigger_active = !state;
+	} else if (trig == chip->threshold_trig) {
+		chip->data_trigger_active = !state;
+		chip->threshold_trigger_active = state;
+	}
+
+	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");
+	else
+		*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 = -EINVAL;
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) {
+			u32 th_lo, th_hi;
+
+			if (opt4060_get_thresholds(chip, &th_lo, &th_hi))
+				return -EFAULT;
+			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;
+			}
+		}
+		break;
+	case IIO_EV_INFO_PERIOD:
+		return opt4060_read_ev_period(chip, val, val2);
+	default:
+		return -EINVAL;
+	}
+	return ret;
+}
+
+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)
+{
+	int ret = -EINVAL;
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) {
+			u32 th_lo, th_hi;
+
+			if (opt4060_get_thresholds(chip, &th_lo, &th_hi))
+				return -EFAULT;
+			if (dir == IIO_EV_DIR_FALLING)
+				th_lo = val;
+			else if (dir == IIO_EV_DIR_RISING)
+				th_hi = val;
+			if (opt4060_set_thresholds(chip, th_lo, th_hi))
+				return -EFAULT;
+			ret = 0;
+		}
+		break;
+	case IIO_EV_INFO_PERIOD:
+		return opt4060_write_ev_period(chip, val, val2);
+	default:
+		return -EINVAL;
+	}
+	return ret;
+}
+
+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 ret = -EINVAL;
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+
+	if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) {
+		int ch_idx = chan->scan_index;
+		int ch_sel;
+
+		if (opt4060_get_channel_sel(chip, &ch_sel))
+			return -EFAULT;
+
+		if (((dir == IIO_EV_DIR_FALLING) && chip->thresh_event_lo_active) ||
+		    ((dir == IIO_EV_DIR_RISING) && chip->thresh_event_hi_active))
+			ret = (ch_sel == ch_idx);
+		else
+			ret = FALSE;
+	}
+	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 ret = -EINVAL;
+	struct opt4060_chip *chip = iio_priv(indio_dev);
+
+	if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) {
+		int ch_idx = chan->scan_index;
+		int ch_sel;
+
+		if (opt4060_get_channel_sel(chip, &ch_sel))
+			return -EFAULT;
+
+		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;
+			if (opt4060_set_channel_sel(chip, ch_idx))
+				return -EFAULT;
+			ret = 0;
+		} 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;
+			}
+			ret = 0;
+		}
+
+		if (opt4060_event_set_state(chip, chip->thresh_event_hi_active |
+					    chip->thresh_event_lo_active))
+			return -EFAULT;
+	}
+	return ret;
+}
+
+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)
+{
+	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;
+
+	return opt4060_set_conf(chip);
+}
+
+static bool opt4060_volatile_reg(struct device *dev, unsigned int reg)
+{
+	if ((reg >= OPT4060_RED_MSB && reg <= OPT4060_CLEAR_LSB) ||
+	    reg == OPT4060_RES_CTRL)
+		return true;
+	return false;
+}
+
+static bool opt4060_writable_reg(struct device *dev, unsigned int reg)
+{
+	if (reg >= OPT4060_THRESHOLD_LOW || reg >= OPT4060_INT_CTRL)
+		return true;
+	return false;
+}
+
+static bool opt4060_readonly_reg(struct device *dev, unsigned int reg)
+{
+	if (reg == OPT4060_DEVICE_ID)
+		return true;
+	return false;
+}
+
+static bool opt4060_readable_reg(struct device *dev, unsigned int reg)
+{
+	/* Volatile, writable and read-only registers are readable. */
+	if (opt4060_volatile_reg(dev, reg) || opt4060_writable_reg(dev, reg) ||
+	    opt4060_readonly_reg(dev, reg))
+		return TRUE;
+	return false;
+}
+
+static const struct regmap_config opt4060_regmap_config = {
+	.name = OPT_4060_DRV_NAME,
+	.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;
+	int i = 0;
+
+	memset(&raw, 0, sizeof(raw));
+
+	for_each_set_bit(chan, idev->active_scan_mask, idev->masklength) {
+		ret = opt4060_read_raw_value(chip,
+					     opt4060_channels[chan].address,
+					     &raw.chan[i++]);
+		if (ret) {
+			dev_err(chip->dev, "Reading raw 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 int opt4060_buffer_preenable(struct iio_dev *idev)
+{
+	struct opt4060_chip *chip = iio_priv(idev);
+
+	return opt4060_trigger_one_shot(chip);
+}
+
+static const struct iio_buffer_setup_ops opt4060_buffer_ops = {
+	.preenable = opt4060_buffer_preenable,
+};
+
+static irqreturn_t opt4060_irq_thread(int irq, void *private)
+{
+	struct iio_dev *idev = private;
+	struct opt4060_chip *chip = iio_priv(idev);
+	int ret, chan, 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;
+	}
+
+	if (!chip->data_trigger_active) {
+		/* Only trigger interrupts on thresholds */
+		ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_THRESHOLD);
+		if (ret) {
+			dev_err(chip->dev, "Failed to set interrupt state\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;
+
+		/* Check if the interrupt is from the lower threshold */
+		if (int_res & OPT4060_RES_CTRL_FLAG_L) {
+			code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
+							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_LIGHT,
+							chan,
+							IIO_EV_TYPE_THRESH,
+							IIO_EV_DIR_RISING);
+			iio_push_event(idev, code,
+					iio_get_time_ns(idev));
+		}
+		/* Handle threshold triggers */
+		if (chip->threshold_trigger_active && iio_buffer_enabled(idev))
+			iio_trigger_poll_nested(chip->threshold_trig);
+	}
+
+	/* 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 (chip->data_trigger_active && iio_buffer_enabled(idev))
+			iio_trigger_poll_nested(chip->data_trig);
+	}
+	return IRQ_HANDLED;
+}
+
+static int opt4060_setup_triggers(struct opt4060_chip *chip, struct iio_dev *idev)
+{
+	struct iio_trigger *data_trigger;
+	struct iio_trigger *threshold_trigger;
+	char *name;
+	int ret;
+
+	ret = devm_iio_triggered_buffer_setup(chip->dev, idev,
+					      &iio_pollfunc_store_time,
+					      opt4060_trigger_handler,
+					      &opt4060_buffer_ops);
+	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;
+	chip->data_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");
+
+	threshold_trigger = devm_iio_trigger_alloc(chip->dev, "%s-threshold-dev%d",
+				       idev->name, iio_device_id(idev));
+	if (!threshold_trigger)
+		return -ENOMEM;
+	chip->threshold_trig = threshold_trigger;
+	threshold_trigger->ops = &opt4060_trigger_ops;
+	iio_trigger_set_drvdata(threshold_trigger, idev);
+	ret = devm_iio_trigger_register(chip->dev, threshold_trigger);
+	if (ret)
+		return dev_err_probe(chip->dev, ret,
+				     "Threshold trigger registration failed\n");
+
+
+	name = devm_kasprintf(chip->dev, GFP_KERNEL, "%s-opt4060",
+			      dev_name(chip->dev));
+
+	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);
+
+	/* Request regulator */
+	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_warn(dev, "Device ID: %#04x unknown\n", dev_id);
+
+	indio_dev->channels = opt4060_channels;
+	indio_dev->num_channels = ARRAY_SIZE(opt4060_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->name = OPT_4060_DRV_NAME;
+
+	/* Initialize device with default settings */
+	ret = opt4060_load_defaults(chip);
+	if (ret < 0)
+		return dev_err_probe(dev, ret,
+				     "Failed to set sensor defaults\n");
+
+	/* If irq is enabled, initialize triggers */
+	if (chip->irq) {
+		ret = opt4060_setup_triggers(chip, indio_dev);
+		if (ret)
+			return ret;
+	} else {
+		dev_info(chip->dev, "No IRQ, events and triggers disabled\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");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+/*
+ * The compatible string determines which constants to use depending on
+ * opt4060 packaging
+ */
+static const struct i2c_device_id opt4060_id[] = {
+	{ OPT_4060_DRV_NAME, 0 },
+	{}
+};
+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 = OPT_4060_DRV_NAME,
+		.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");
-- 
2.39.5

Re: [PATCH v2 2/2] iio: light: Add support for TI OPT4060 color sensor

Posted by kernel test robot 1 month, 3 weeks ago

Hi Per-Daniel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 0c559323bbaabee7346c12e74b497e283aaafef5]

url:    https://github.com/intel-lab-lkp/linux/commits/Per-Daniel-Olsson/dt-bindings-iio-light-Document-TI-OPT4060-RGBW-sensor/20241006-005244
base:   0c559323bbaabee7346c12e74b497e283aaafef5
patch link:    https://lore.kernel.org/r/20241005165119.3549472-3-perdaniel.olsson%40axis.com
patch subject: [PATCH v2 2/2] iio: light: Add support for TI OPT4060 color sensor
config: csky-randconfig-r122-20241007 (https://download.01.org/0day-ci/archive/20241007/202410072240.s3wGb17S-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 14.1.0
reproduce: (https://download.01.org/0day-ci/archive/20241007/202410072240.s3wGb17S-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/202410072240.s3wGb17S-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/iio/light/opt4060.c:963:9: sparse: sparse: dereference of noderef expression
>> drivers/iio/light/opt4060.c:963:9: sparse: sparse: dereference of noderef expression
>> drivers/iio/light/opt4060.c:963:9: sparse: sparse: dereference of noderef expression

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

   951	
   952	static irqreturn_t opt4060_trigger_handler(int irq, void *p)
   953	{
   954		struct iio_poll_func *pf = p;
   955		struct iio_dev *idev = pf->indio_dev;
   956		struct opt4060_chip *chip = iio_priv(idev);
   957		struct opt4060_buffer raw;
   958		int ret, chan;
   959		int i = 0;
   960	
   961		memset(&raw, 0, sizeof(raw));
   962	
 > 963		for_each_set_bit(chan, idev->active_scan_mask, idev->masklength) {
   964			ret = opt4060_read_raw_value(chip,
   965						     opt4060_channels[chan].address,
   966						     &raw.chan[i++]);
   967			if (ret) {
   968				dev_err(chip->dev, "Reading raw channel data failed\n");
   969				goto err_read;
   970			}
   971		}
   972	
   973		iio_push_to_buffers_with_timestamp(idev, &raw, pf->timestamp);
   974	err_read:
   975		iio_trigger_notify_done(idev->trig);
   976		return IRQ_HANDLED;
   977	}
   978	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/2] iio: light: Add support for TI OPT4060 color sensor

Posted by kernel test robot 1 month, 3 weeks ago

Hi Per-Daniel,

kernel test robot noticed the following build warnings:

[auto build test WARNING on 0c559323bbaabee7346c12e74b497e283aaafef5]

url:    https://github.com/intel-lab-lkp/linux/commits/Per-Daniel-Olsson/dt-bindings-iio-light-Document-TI-OPT4060-RGBW-sensor/20241006-005244
base:   0c559323bbaabee7346c12e74b497e283aaafef5
patch link:    https://lore.kernel.org/r/20241005165119.3549472-3-perdaniel.olsson%40axis.com
patch subject: [PATCH v2 2/2] iio: light: Add support for TI OPT4060 color sensor
config: um-allmodconfig (https://download.01.org/0day-ci/archive/20241007/202410071407.8Vp5xp85-lkp@intel.com/config)
compiler: clang version 20.0.0git (https://github.com/llvm/llvm-project fef3566a25ff0e34fb87339ba5e13eca17cec00f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241007/202410071407.8Vp5xp85-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/202410071407.8Vp5xp85-lkp@intel.com/

All 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:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:8:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:548:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     548 |         val = __raw_readb(PCI_IOBASE + addr);
         |                           ~~~~~~~~~~ ^
   include/asm-generic/io.h:561:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     561 |         val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
      37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
         |                                                   ^
   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:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:8:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:574:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     574 |         val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
         |                                                         ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
      35 | #define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
         |                                                   ^
   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:13:
   In file included from include/linux/cgroup.h:26:
   In file included from include/linux/kernel_stat.h:8:
   In file included from include/linux/interrupt.h:11:
   In file included from include/linux/hardirq.h:11:
   In file included from arch/um/include/asm/hardirq.h:5:
   In file included from include/asm-generic/hardirq.h:17:
   In file included from include/linux/irq.h:20:
   In file included from include/linux/io.h:14:
   In file included from arch/um/include/asm/io.h:24:
   include/asm-generic/io.h:585:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     585 |         __raw_writeb(value, PCI_IOBASE + addr);
         |                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:595:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     595 |         __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:605:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     605 |         __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
         |                                                       ~~~~~~~~~~ ^
   include/asm-generic/io.h:693:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     693 |         readsb(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:701:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     701 |         readsw(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:709:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     709 |         readsl(PCI_IOBASE + addr, buffer, count);
         |                ~~~~~~~~~~ ^
   include/asm-generic/io.h:718:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     718 |         writesb(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:727:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     727 |         writesw(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   include/asm-generic/io.h:736:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
     736 |         writesl(PCI_IOBASE + addr, buffer, count);
         |                 ~~~~~~~~~~ ^
   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: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_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> drivers/iio/light/opt4060.c:1026:8: warning: variable 'chan' is uninitialized when used here [-Wuninitialized]
    1026 |                                                         chan,
         |                                                         ^~~~
   include/linux/iio/events.h:54:51: note: expanded from macro 'IIO_UNMOD_EVENT_CODE'
      54 |         IIO_EVENT_CODE(chan_type, 0, 0, direction, type, number, 0, 0)
         |                                                          ^~~~~~
   include/linux/iio/events.h:29:9: note: expanded from macro 'IIO_EVENT_CODE'
      29 |          ((u16)chan))
         |                ^~~~
   drivers/iio/light/opt4060.c:994:15: note: initialize the variable 'chan' to silence this warning
     994 |         int ret, chan, dummy;
         |                      ^
         |                       = 0
   14 warnings generated.

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for MODVERSIONS
   Depends on [n]: MODULES [=y] && !COMPILE_TEST [=y]
   Selected by [y]:
   - RANDSTRUCT_FULL [=y] && (CC_HAS_RANDSTRUCT [=y] || GCC_PLUGINS [=n]) && MODULES [=y]
   WARNING: unmet direct dependencies detected for GET_FREE_REGION
   Depends on [n]: SPARSEMEM [=n]
   Selected by [m]:
   - RESOURCE_KUNIT_TEST [=m] && RUNTIME_TESTING_MENU [=y] && KUNIT [=m]


vim +/chan +1026 drivers/iio/light/opt4060.c

   989	
   990	static irqreturn_t opt4060_irq_thread(int irq, void *private)
   991	{
   992		struct iio_dev *idev = private;
   993		struct opt4060_chip *chip = iio_priv(idev);
   994		int ret, chan, dummy;
   995		unsigned int int_res;
   996	
   997		ret = regmap_read(chip->regmap, OPT4060_RES_CTRL, &int_res);
   998		if (ret < 0) {
   999			dev_err(chip->dev, "Failed to read interrupt reasons.\n");
  1000			return IRQ_NONE;
  1001		}
  1002	
  1003		if (!chip->data_trigger_active) {
  1004			/* Only trigger interrupts on thresholds */
  1005			ret = opt4060_set_int_state(chip, OPT4060_INT_CTRL_THRESHOLD);
  1006			if (ret) {
  1007				dev_err(chip->dev, "Failed to set interrupt state\n");
  1008				return IRQ_NONE;
  1009			}
  1010		}
  1011	
  1012		/* Read OPT4060_CTRL to clear interrupt */
  1013		ret = regmap_read(chip->regmap, OPT4060_CTRL, &dummy);
  1014		if (ret < 0) {
  1015			dev_err(chip->dev, "Failed to clear interrupt\n");
  1016			return IRQ_NONE;
  1017		}
  1018	
  1019		/* Handle events */
  1020		if (int_res & (OPT4060_RES_CTRL_FLAG_H | OPT4060_RES_CTRL_FLAG_L)) {
  1021			u64 code;
  1022	
  1023			/* Check if the interrupt is from the lower threshold */
  1024			if (int_res & OPT4060_RES_CTRL_FLAG_L) {
  1025				code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
> 1026								chan,
  1027								IIO_EV_TYPE_THRESH,
  1028								IIO_EV_DIR_FALLING);
  1029				iio_push_event(idev, code,
  1030						iio_get_time_ns(idev));
  1031			}
  1032			/* Check if the interrupt is from the upper threshold */
  1033			if (int_res & OPT4060_RES_CTRL_FLAG_H) {
  1034				code = IIO_UNMOD_EVENT_CODE(IIO_LIGHT,
  1035								chan,
  1036								IIO_EV_TYPE_THRESH,
  1037								IIO_EV_DIR_RISING);
  1038				iio_push_event(idev, code,
  1039						iio_get_time_ns(idev));
  1040			}
  1041			/* Handle threshold triggers */
  1042			if (chip->threshold_trigger_active && iio_buffer_enabled(idev))
  1043				iio_trigger_poll_nested(chip->threshold_trig);
  1044		}
  1045	
  1046		/* Handle conversion ready */
  1047		if (int_res & OPT4060_RES_CTRL_CONV_READY) {
  1048			/* Signal completion for potentially waiting reads */
  1049			complete(&chip->completion);
  1050	
  1051			/* Handle data ready triggers */
  1052			if (chip->data_trigger_active && iio_buffer_enabled(idev))
  1053				iio_trigger_poll_nested(chip->data_trig);
  1054		}
  1055		return IRQ_HANDLED;
  1056	}
  1057	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 2/2] iio: light: Add support for TI OPT4060 color sensor

Posted by Jonathan Cameron 1 month, 3 weeks ago

On Sat, 5 Oct 2024 18:51:19 +0200
Per-Daniel Olsson <perdaniel.olsson@axis.com> wrote:

> Add support for Texas Instruments OPT4060 RGBW Color sensor.
Hi Per-Daniel.

Thanks for your driver.  Various comments inline.
The two bigger things I'd like to understand are:
1) Why we have a threshold trigger?  That is unusual so I'd like to fully understand
  the use case.  I see an explanation in your cover letter so I'll comment on that here
  but I would like one here as well.

2) Why do we need processed and raw? That may make sense but it is also unusual
   so I'd like a comment in this patch description on your reasoning.

I've only raised some style issues in a few places. Please check to see if those
apply more broadly.

Thanks,

Jonathan


> 
> Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.com>
> ---
>  drivers/iio/light/Kconfig   |   13 +
>  drivers/iio/light/Makefile  |    1 +
>  drivers/iio/light/opt4060.c | 1216 +++++++++++++++++++++++++++++++++++
>  3 files changed, 1230 insertions(+)
>  create mode 100644 drivers/iio/light/opt4060.c
> 
> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
> index 515ff46b5b82..0e2059512157 100644
> --- a/drivers/iio/light/Kconfig
> +++ b/drivers/iio/light/Kconfig
> @@ -490,6 +490,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 321010fc0b93..55902b21ec0c 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..a917b59cd500
> --- /dev/null
> +++ b/drivers/iio/light/opt4060.c
> @@ -0,0 +1,1216 @@
> +// 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>
> +
> +#define OPT_4060_DRV_NAME	"opt4060"
I'm generally against these DRV_NAME type defines as they tend to imply there
is a reason various strings in the driver match when it is an arbitrary choice.
I'd rather see the string inline.

> +
> +struct opt4060_buffer {
> +	u32 chan[OPT4060_NUM_CHANS];
> +	s64 ts;
aligned_s64
Which is needed to cover the delights of 32 bit x86 where an s64 is 32bit aligned
and this whole structure can end up aligned differently from the obvious.


> +};

...
> +
> +static u8 opt4060_calculate_crc(u8 exp, u32 mantissa, u8 count)

Good to include the formula here. I'm assuming this doesn't correspond
to any of the standard crc codes for which we have implementations already.

> +{
> +	u8 crc;
> +
> +	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_read_raw_value(struct opt4060_chip *chip,
> +				  unsigned long address, u32 *raw)
> +{
> +	int ret;
> +	u32 result;
> +	u16 msb;
> +	u16 lsb;
> +	u8 exp;
> +	u8 count;
> +	u8 crc;

Combine entrees of same type to save a few lines here

> +	u8 calc_crc;
> +	u32 mantissa_raw;
> +
> +	ret = regmap_bulk_read(chip->regmap, address, &result, 2);
> +	if (ret)
> +		dev_err(chip->dev, "Reading channel data failed\n");
		return ret; and drop the else.

Also kernel coding style requires {} for all legs if any need it.

> +	else {
> +		count = FIELD_GET(OPT4060_COUNTER_MASK, result >> 16);
> +		crc = FIELD_GET(OPT4060_CRC_MASK, result >> 16);
> +		lsb = FIELD_GET(OPT4060_LSB_MASK, result >> 16);
> +		exp = FIELD_GET(OPT4060_EXPONENT_MASK, result);
> +		msb = FIELD_GET(OPT4060_MSB_MASK, result);
> +		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 ret;
> +}
> +
> +static int opt4060_read_chan_value(struct iio_dev *indio_dev,
> +				   struct iio_chan_spec const *chan,
> +				   int *val, int *val2, bool processed)
> +{
> +	struct opt4060_chip *chip = iio_priv(indio_dev);
> +	u32 adc_raw;
> +	int ret;
> +	/* Set timeout to two times the integration time multiplied by channel count. */
> +	unsigned int timeout_us = 2 * OPT4060_NUM_CHANS *
> +				  opt4060_int_time_reg[chip->int_time][0];
> +
> +	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_info(chip->dev, "Completion timed out.\n");

It's an error dev_err() appropriate.

> +	}
> +
> +	ret = opt4060_read_raw_value(chip, chan->address, &adc_raw);
> +	if (ret) {
> +		dev_err(chip->dev, "Reading raw channel data failed\n");
> +		return ret;
> +	}
> +	if (processed) {

Having both processed and raw for a channel needs an explanation.
It's not helped by processed being meaningless for color channels as there
are not really an well defined units.


> +		u32 lux_raw;
> +		u32 rem;
> +		u32 mul = opt4060_channel_factors[chan->scan_index].mul;
> +		u32 div = opt4060_channel_factors[chan->scan_index].div;
> +
> +		lux_raw = adc_raw * mul;
> +		*val = div_u64_rem(lux_raw, div, &rem);
> +		*val2 = rem * div_u64(div, 10);
> +		return IIO_VAL_INT_PLUS_NANO;
> +	}
> +	*val = adc_raw;
> +	*val2 = 0;

No need to set val2 if IIO_VAL_INT
The core code guarantees never to use it.

> +	return IIO_VAL_INT;
> +}

> +static int opt4060_power_down(struct opt4060_chip *chip)
> +{
> +	int ret;
> +	unsigned int reg;
> +
> +	ret = regmap_read(chip->regmap, OPT4060_CTRL, &reg);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to read configuration\n");
> +		return ret;
> +	}
> +
> +	/* MODE_OFF is 0x0 so just set bits to 0 */
> +	reg &= ~OPT4060_CTRL_OPER_MODE_MASK;

regmap_clear_bits() maybe?

> +
> +	ret = regmap_write(chip->regmap, OPT4060_CTRL, reg);
> +	if (ret)
> +		dev_err(chip->dev, "Failed to power down\n");
> +
> +	return ret;
> +}
> +
> +static void opt4060_chip_off_action(void *data)
> +{
> +	struct opt4060_chip *chip = data;
No need for the local variable.  Just rename data to chip is enough.
> +
> +	opt4060_power_down(chip);
> +}
> +
> +#define OPT4060_CHAN(color)							\
> +{										\
> +	.type = IIO_LIGHT,							\
> +	.modified = 1,								\
> +	.channel2 = IIO_MOD_LIGHT_##color,					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |			\
> +			      BIT(IIO_CHAN_INFO_RAW),				\
> +	.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_LE,						\
> +	},									\
> +	.event_spec = opt4060_event_spec,					\
where you have an optional irq like here, have two sets of chan_spec and pick
between them dependent on the availability of that irq.

If there is no irq, there should be no event attributes.

> +	.num_event_specs = ARRAY_SIZE(opt4060_event_spec),			\
> +}
> +
> +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 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;
> +	}
> +}
> +
One blank line is enough. Check for other instances.

> +
> +static int opt4060_event_set_state(struct opt4060_chip *chip, bool state)
> +{
> +	int ret = 0;
> +
> +	if (state)
> +		ret = opt4060_set_continous_mode(chip, true);
		return opt...
> +	else if (!chip->data_trigger_active && chip->irq)
> +		ret = opt4060_set_continous_mode(chip, false);
		return opt...

> +
> +	return ret;
Not an error?

> +}
> +
> +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 (indio_dev->trig) {
> +		iio_trigger_put(indio_dev->trig);
> +		indio_dev->trig = NULL;
> +	}
> +
> +	if (state) {
> +		indio_dev->trig = iio_trigger_get(trig);
> +		ret = opt4060_set_continous_mode(chip, true);
	return opt...

> +	} else if (!opt4060_event_active(chip) && chip->irq)
> +		ret = opt4060_set_continous_mode(chip, false);
		return opt
> +
> +	if (trig == chip->data_trig) {
> +		chip->data_trigger_active = state;
> +		chip->threshold_trigger_active = !state;
> +	} else if (trig == chip->threshold_trig) {
> +		chip->data_trigger_active = !state;
> +		chip->threshold_trigger_active = state;
> +	}
> +
> +	return ret;
> +}

> +
> +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");
		return ret;
> +	else
no else needed.
> +		*ch_sel = FIELD_GET(OPT4060_INT_CTRL_THRESH_SEL, regval);
> +	return ret;
return 0;
Check for other cases that look like this. Early returns on error almost
always make for more readable code than keeping the return for the end
(After doing nothing useful extra)
> +}
> +
> +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;
	return 0;
> +}
> +
> +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 = -EINVAL;
> +	struct opt4060_chip *chip = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) {
> +			u32 th_lo, th_hi;
> +
> +			if (opt4060_get_thresholds(chip, &th_lo, &th_hi))
> +				return -EFAULT;
> +			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;
> +			}
> +		}
> +		break;
Similar refactor to do early returns as suggested for the next function (see below)

> +	case IIO_EV_INFO_PERIOD:
> +		return opt4060_read_ev_period(chip, val, val2);
> +	default:
> +		return -EINVAL;
> +	}
> +	return ret;
> +}
> +
> +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)
> +{
> +	int ret = -EINVAL;
> +	struct opt4060_chip *chip = iio_priv(indio_dev);
> +
> +	switch (info) {
> +	case IIO_EV_INFO_VALUE:
> +		if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) {
> +			u32 th_lo, th_hi;
> +
> +			if (opt4060_get_thresholds(chip, &th_lo, &th_hi))
> +				return -EFAULT;
> +			if (dir == IIO_EV_DIR_FALLING)
> +				th_lo = val;
> +			else if (dir == IIO_EV_DIR_RISING)
> +				th_hi = val;
> +			if (opt4060_set_thresholds(chip, th_lo, th_hi))
> +				return -EFAULT;
> +			ret = 0;
			return 0;
> +		}
		return -EINVAL;
> +		break;
> +	case IIO_EV_INFO_PERIOD:
> +		return opt4060_write_ev_period(chip, val, val2);
> +	default:
> +		return -EINVAL;
> +	}
> +	return ret;
Drop this as with above always returned already.

> +}
> +
> +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 ret = -EINVAL;
> +	struct opt4060_chip *chip = iio_priv(indio_dev);
> +
> +	if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) {
As below. Consider flipping the condition for an early return
Check all similar cases for whether it improves readability.


> +		int ch_idx = chan->scan_index;
> +		int ch_sel;
> +
> +		if (opt4060_get_channel_sel(chip, &ch_sel))
> +			return -EFAULT;
> +
> +		if (((dir == IIO_EV_DIR_FALLING) && chip->thresh_event_lo_active) ||
> +		    ((dir == IIO_EV_DIR_RISING) && chip->thresh_event_hi_active))
> +			ret = (ch_sel == ch_idx);
> +		else
> +			ret = FALSE;
> +	}
> +	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 ret = -EINVAL;
> +	struct opt4060_chip *chip = iio_priv(indio_dev);
> +
> +	if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) {
Maybe flip the condition to reduce effective indent.
	if (chan->type != IIO_LIGHT)
		return -EINVAL;
	if (type != IIO_EV_TYPE_THRESH)
		reuturn -EINVAL;

etc
> +		int ch_idx = chan->scan_index;
> +		int ch_sel;
> +
> +		if (opt4060_get_channel_sel(chip, &ch_sel))
> +			return -EFAULT;
> +
> +		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;
> +			if (opt4060_set_channel_sel(chip, ch_idx))
> +				return -EFAULT;
> +			ret = 0;
> +		} else {
> +			if (ch_idx == ch_sel) {
> +				if (dir == IIO_EV_DIR_FALLING)
> +					chip->thresh_event_lo_active = FALSE;
false etc

> +				else if (dir == IIO_EV_DIR_RISING)
> +					chip->thresh_event_hi_active = FALSE;
> +			}
> +			ret = 0;
> +		}
> +
> +		if (opt4060_event_set_state(chip, chip->thresh_event_hi_active |
> +					    chip->thresh_event_lo_active))
> +			return -EFAULT;
> +	}
> +	return ret;
> +}

> +
> +static bool opt4060_volatile_reg(struct device *dev, unsigned int reg)
> +{
> +	if ((reg >= OPT4060_RED_MSB && reg <= OPT4060_CLEAR_LSB) ||
> +	    reg == OPT4060_RES_CTRL)
> +		return true;
> +	return false;

As below, just return the condition.

> +}
> +
> +static bool opt4060_writable_reg(struct device *dev, unsigned int reg)
> +{
> +	if (reg >= OPT4060_THRESHOLD_LOW || reg >= OPT4060_INT_CTRL)
> +		return true;
> +	return false;
return reg >=...
> +}
> +
> +static bool opt4060_readonly_reg(struct device *dev, unsigned int reg)
> +{
> +	if (reg == OPT4060_DEVICE_ID)
> +		return true;
> +	return false;
	return reg == ..

> +}
> +
> +static bool opt4060_readable_reg(struct device *dev, unsigned int reg)
> +{
> +	/* Volatile, writable and read-only registers are readable. */
> +	if (opt4060_volatile_reg(dev, reg) || opt4060_writable_reg(dev, reg) ||
> +	    opt4060_readonly_reg(dev, reg))
> +		return TRUE;
true

> +	return false;

return opt....

> +}
> +

> +
> +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;
> +	int i = 0;
> +
> +	memset(&raw, 0, sizeof(raw));
> +
> +	for_each_set_bit(chan, idev->active_scan_mask, idev->masklength) {
Try compiling this on latest togreg branch.  We have removed direct
access to masklength

There is a iio_for_each_active_channel() macro that handles this usage
more cleanly.

> +		ret = opt4060_read_raw_value(chip,
> +					     opt4060_channels[chan].address,
> +					     &raw.chan[i++]);
> +		if (ret) {
> +			dev_err(chip->dev, "Reading raw 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 int opt4060_setup_triggers(struct opt4060_chip *chip, struct iio_dev *idev)
> +{
> +	struct iio_trigger *data_trigger;
> +	struct iio_trigger *threshold_trigger;
> +	char *name;
> +	int ret;
> +
> +	ret = devm_iio_triggered_buffer_setup(chip->dev, idev,
> +					      &iio_pollfunc_store_time,
> +					      opt4060_trigger_handler,
> +					      &opt4060_buffer_ops);
> +	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));

Where it doesn't result in very long lines, align to just after (

That may mean you have more line breaks. If so that is fine.


> +	if (!data_trigger)
> +		return -ENOMEM;
Slight preference for a blank line after error checks like this.

> +	chip->data_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");
> +
> +	threshold_trigger = devm_iio_trigger_alloc(chip->dev, "%s-threshold-dev%d",
> +				       idev->name, iio_device_id(idev));

This needs some explanation. Why a threshold trigger rather than just using
events?  Superficially looks like you should just use the event interfaces.

> +	if (!threshold_trigger)
> +		return -ENOMEM;
> +	chip->threshold_trig = threshold_trigger;
> +	threshold_trigger->ops = &opt4060_trigger_ops;
> +	iio_trigger_set_drvdata(threshold_trigger, idev);
> +	ret = devm_iio_trigger_register(chip->dev, threshold_trigger);
> +	if (ret)
> +		return dev_err_probe(chip->dev, ret,
> +				     "Threshold trigger registration failed\n");
> +
One blank line is plenty.
> +
> +	name = devm_kasprintf(chip->dev, GFP_KERNEL, "%s-opt4060",
> +			      dev_name(chip->dev));

Check for failure to allocate.

> +
> +	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;

unsigned int to match the regmap parameter type.

> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	chip = iio_priv(indio_dev);
> +
> +	/* Request regulator */

very obvious. Drop the comment.

> +	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_warn(dev, "Device ID: %#04x unknown\n", dev_id);

Prefer dev_info() for this as it may well be a valid fallback
compatible.

> +
> +	indio_dev->channels = opt4060_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(opt4060_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->name = OPT_4060_DRV_NAME;
> +
> +	/* Initialize device with default settings */

As below. Comment is (rightly) obvious from the code, so don't have
a comment.

> +	ret = opt4060_load_defaults(chip);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to set sensor defaults\n");
> +
> +	/* If irq is enabled, initialize triggers */
The comment doesn't add anything not obvious from the code.  I'd drop it
and any similar ones.
> +	if (chip->irq) {
> +		ret = opt4060_setup_triggers(chip, indio_dev);
> +		if (ret)
> +			return ret;
> +	} else {
> +		dev_info(chip->dev, "No IRQ, events and triggers disabled\n");

Too noisy.  At most dev_dbg.  Probably not useful at all as it is fairly easy
to see no irq was registered.

> +	}
> +
> +	ret = devm_add_action_or_reset(dev, opt4060_chip_off_action, chip);

This needs to be alongside whatever turned it on. Or if that is a side effect
of later accesses (i.e. it is powered up only channel read) then add
a comment to that affect.

> +	if (ret < 0)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to setup power off action\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +/*
> + * The compatible string determines which constants to use depending on
> + * opt4060 packaging
There is only one. So this comment is currently meaningless.

> + */
> +static const struct i2c_device_id opt4060_id[] = {
> +	{ OPT_4060_DRV_NAME, 0 },
Put the string directly in here. There is no reason why this
should match the DRV_NAME.  Also drop the 0 as it is currently
pointless and when you do introduce some data it should be a pointer
anyway.

> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, opt4060_id);
> +
> +static const struct of_device_id opt4060_of_match[] = {
> +	{ .compatible = "ti,opt4060" },
> +	{}

Trivial but I'm trying to get consistency in IIO (slowly) on 
{ } for these terminators (so with the space).
I'd prefer to not introduce more instances of {}

Thanks,

Jonathan


> +};
> +MODULE_DEVICE_TABLE(of, opt4060_of_match);

Re: [PATCH v2 2/2] iio: light: Add support for TI OPT4060 color sensor

Posted by Per-Daniel Olsson 1 month, 2 weeks ago

On 10/6/24 17:18, Jonathan Cameron wrote:
> On Sat, 5 Oct 2024 18:51:19 +0200
> Per-Daniel Olsson <perdaniel.olsson@axis.com> wrote:
> 
>> Add support for Texas Instruments OPT4060 RGBW Color sensor.
> Hi Per-Daniel.
> 
> Thanks for your driver.  Various comments inline.

Hi Jonathan.

Thank you for your comments. I will address and correct the inline comments in the next
version and I will try to answer your two bigger questions here. I replied to your
comments in the cover letter earlier but I forgot to answer them here also, as you
requested.

> The two bigger things I'd like to understand are:
> 1) Why we have a threshold trigger?  That is unusual so I'd like to fully understand
>   the use case.  I see an explanation in your cover letter so I'll comment on that here
>   but I would like one here as well.

Our userspace application needs the values after getting the threshold event. Before
I implemented the threshold trigger and the triggered buffer, the application would
read the values from sysfs right after the event. In that case the values will not be
the ones that actually triggered the event. When the light condition is close to the
threshold, the value from sysfs might even be on the wrong side of the threshold which
can be confusing for the state machine in userspace. I would say that this feature is
fairly important to us, this is the way our userspace is using the sensor.

> 
> 2) Why do we need processed and raw? That may make sense but it is also unusual
>    so I'd like a comment in this patch description on your reasoning.
> 

The raw values are the actual ADC values from the sensor. Those values are the ones
that are needed when setting thresholds, so the raw values are definitely needed. The
processed values are a conversion to something that should be close to lux. This is not
something I know much about, I just read the data sheet (https://www.ti.com/lit/gpn/opt4060).
According to chapter 8.4.5.2 (page 17), lux can be calculated this way:

lux = GREEN_ADC_RAW * 2.15e-3

It is also stated that R=G=B for a D65 standard white light source if red is multiplied
by 2.4 and blue is multiplied with 1.3. We currently don't use the processed values in
our use of the sensor, but I assume that other users will expect this functionality.

> I've only raised some style issues in a few places. Please check to see if those
> apply more broadly.

I'll try to address the style issues broadly in the next version.

Best regards / P-D

> 
> Thanks,
> 
> Jonathan
> 
> 
>>
>> Signed-off-by: Per-Daniel Olsson <perdaniel.olsson@axis.com>
>> ---
>>  drivers/iio/light/Kconfig   |   13 +
>>  drivers/iio/light/Makefile  |    1 +
>>  drivers/iio/light/opt4060.c | 1216 +++++++++++++++++++++++++++++++++++
>>  3 files changed, 1230 insertions(+)
>>  create mode 100644 drivers/iio/light/opt4060.c
>>
>> diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
>> index 515ff46b5b82..0e2059512157 100644
>> --- a/drivers/iio/light/Kconfig
>> +++ b/drivers/iio/light/Kconfig
>> @@ -490,6 +490,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 321010fc0b93..55902b21ec0c 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..a917b59cd500
>> --- /dev/null
>> +++ b/drivers/iio/light/opt4060.c
>> @@ -0,0 +1,1216 @@
>> +// 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>
>> +
>> +#define OPT_4060_DRV_NAME	"opt4060"
> I'm generally against these DRV_NAME type defines as they tend to imply there
> is a reason various strings in the driver match when it is an arbitrary choice.
> I'd rather see the string inline.
> 
>> +
>> +struct opt4060_buffer {
>> +	u32 chan[OPT4060_NUM_CHANS];
>> +	s64 ts;
> aligned_s64
> Which is needed to cover the delights of 32 bit x86 where an s64 is 32bit aligned
> and this whole structure can end up aligned differently from the obvious.
> 
> 
>> +};
> 
> ...
>> +
>> +static u8 opt4060_calculate_crc(u8 exp, u32 mantissa, u8 count)
> 
> Good to include the formula here. I'm assuming this doesn't correspond
> to any of the standard crc codes for which we have implementations already.
> 
>> +{
>> +	u8 crc;
>> +
>> +	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_read_raw_value(struct opt4060_chip *chip,
>> +				  unsigned long address, u32 *raw)
>> +{
>> +	int ret;
>> +	u32 result;
>> +	u16 msb;
>> +	u16 lsb;
>> +	u8 exp;
>> +	u8 count;
>> +	u8 crc;
> 
> Combine entrees of same type to save a few lines here
> 
>> +	u8 calc_crc;
>> +	u32 mantissa_raw;
>> +
>> +	ret = regmap_bulk_read(chip->regmap, address, &result, 2);
>> +	if (ret)
>> +		dev_err(chip->dev, "Reading channel data failed\n");
> 		return ret; and drop the else.
> 
> Also kernel coding style requires {} for all legs if any need it.
> 
>> +	else {
>> +		count = FIELD_GET(OPT4060_COUNTER_MASK, result >> 16);
>> +		crc = FIELD_GET(OPT4060_CRC_MASK, result >> 16);
>> +		lsb = FIELD_GET(OPT4060_LSB_MASK, result >> 16);
>> +		exp = FIELD_GET(OPT4060_EXPONENT_MASK, result);
>> +		msb = FIELD_GET(OPT4060_MSB_MASK, result);
>> +		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 ret;
>> +}
>> +
>> +static int opt4060_read_chan_value(struct iio_dev *indio_dev,
>> +				   struct iio_chan_spec const *chan,
>> +				   int *val, int *val2, bool processed)
>> +{
>> +	struct opt4060_chip *chip = iio_priv(indio_dev);
>> +	u32 adc_raw;
>> +	int ret;
>> +	/* Set timeout to two times the integration time multiplied by channel count. */
>> +	unsigned int timeout_us = 2 * OPT4060_NUM_CHANS *
>> +				  opt4060_int_time_reg[chip->int_time][0];
>> +
>> +	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_info(chip->dev, "Completion timed out.\n");
> 
> It's an error dev_err() appropriate.
> 
>> +	}
>> +
>> +	ret = opt4060_read_raw_value(chip, chan->address, &adc_raw);
>> +	if (ret) {
>> +		dev_err(chip->dev, "Reading raw channel data failed\n");
>> +		return ret;
>> +	}
>> +	if (processed) {
> 
> Having both processed and raw for a channel needs an explanation.
> It's not helped by processed being meaningless for color channels as there
> are not really an well defined units.
> 
> 
>> +		u32 lux_raw;
>> +		u32 rem;
>> +		u32 mul = opt4060_channel_factors[chan->scan_index].mul;
>> +		u32 div = opt4060_channel_factors[chan->scan_index].div;
>> +
>> +		lux_raw = adc_raw * mul;
>> +		*val = div_u64_rem(lux_raw, div, &rem);
>> +		*val2 = rem * div_u64(div, 10);
>> +		return IIO_VAL_INT_PLUS_NANO;
>> +	}
>> +	*val = adc_raw;
>> +	*val2 = 0;
> 
> No need to set val2 if IIO_VAL_INT
> The core code guarantees never to use it.
> 
>> +	return IIO_VAL_INT;
>> +}
> 
>> +static int opt4060_power_down(struct opt4060_chip *chip)
>> +{
>> +	int ret;
>> +	unsigned int reg;
>> +
>> +	ret = regmap_read(chip->regmap, OPT4060_CTRL, &reg);
>> +	if (ret) {
>> +		dev_err(chip->dev, "Failed to read configuration\n");
>> +		return ret;
>> +	}
>> +
>> +	/* MODE_OFF is 0x0 so just set bits to 0 */
>> +	reg &= ~OPT4060_CTRL_OPER_MODE_MASK;
> 
> regmap_clear_bits() maybe?
> 
>> +
>> +	ret = regmap_write(chip->regmap, OPT4060_CTRL, reg);
>> +	if (ret)
>> +		dev_err(chip->dev, "Failed to power down\n");
>> +
>> +	return ret;
>> +}
>> +
>> +static void opt4060_chip_off_action(void *data)
>> +{
>> +	struct opt4060_chip *chip = data;
> No need for the local variable.  Just rename data to chip is enough.
>> +
>> +	opt4060_power_down(chip);
>> +}
>> +
>> +#define OPT4060_CHAN(color)							\
>> +{										\
>> +	.type = IIO_LIGHT,							\
>> +	.modified = 1,								\
>> +	.channel2 = IIO_MOD_LIGHT_##color,					\
>> +	.info_mask_separate = BIT(IIO_CHAN_INFO_PROCESSED) |			\
>> +			      BIT(IIO_CHAN_INFO_RAW),				\
>> +	.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_LE,						\
>> +	},									\
>> +	.event_spec = opt4060_event_spec,					\
> where you have an optional irq like here, have two sets of chan_spec and pick
> between them dependent on the availability of that irq.
> 
> If there is no irq, there should be no event attributes.
> 
>> +	.num_event_specs = ARRAY_SIZE(opt4060_event_spec),			\
>> +}
>> +
>> +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 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;
>> +	}
>> +}
>> +
> One blank line is enough. Check for other instances.
> 
>> +
>> +static int opt4060_event_set_state(struct opt4060_chip *chip, bool state)
>> +{
>> +	int ret = 0;
>> +
>> +	if (state)
>> +		ret = opt4060_set_continous_mode(chip, true);
> 		return opt...
>> +	else if (!chip->data_trigger_active && chip->irq)
>> +		ret = opt4060_set_continous_mode(chip, false);
> 		return opt...
> 
>> +
>> +	return ret;
> Not an error?
> 
>> +}
>> +
>> +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 (indio_dev->trig) {
>> +		iio_trigger_put(indio_dev->trig);
>> +		indio_dev->trig = NULL;
>> +	}
>> +
>> +	if (state) {
>> +		indio_dev->trig = iio_trigger_get(trig);
>> +		ret = opt4060_set_continous_mode(chip, true);
> 	return opt...
> 
>> +	} else if (!opt4060_event_active(chip) && chip->irq)
>> +		ret = opt4060_set_continous_mode(chip, false);
> 		return opt
>> +
>> +	if (trig == chip->data_trig) {
>> +		chip->data_trigger_active = state;
>> +		chip->threshold_trigger_active = !state;
>> +	} else if (trig == chip->threshold_trig) {
>> +		chip->data_trigger_active = !state;
>> +		chip->threshold_trigger_active = state;
>> +	}
>> +
>> +	return ret;
>> +}
> 
>> +
>> +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");
> 		return ret;
>> +	else
> no else needed.
>> +		*ch_sel = FIELD_GET(OPT4060_INT_CTRL_THRESH_SEL, regval);
>> +	return ret;
> return 0;
> Check for other cases that look like this. Early returns on error almost
> always make for more readable code than keeping the return for the end
> (After doing nothing useful extra)
>> +}
>> +
>> +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;
> 	return 0;
>> +}
>> +
>> +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 = -EINVAL;
>> +	struct opt4060_chip *chip = iio_priv(indio_dev);
>> +
>> +	switch (info) {
>> +	case IIO_EV_INFO_VALUE:
>> +		if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) {
>> +			u32 th_lo, th_hi;
>> +
>> +			if (opt4060_get_thresholds(chip, &th_lo, &th_hi))
>> +				return -EFAULT;
>> +			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;
>> +			}
>> +		}
>> +		break;
> Similar refactor to do early returns as suggested for the next function (see below)
> 
>> +	case IIO_EV_INFO_PERIOD:
>> +		return opt4060_read_ev_period(chip, val, val2);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +	return ret;
>> +}
>> +
>> +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)
>> +{
>> +	int ret = -EINVAL;
>> +	struct opt4060_chip *chip = iio_priv(indio_dev);
>> +
>> +	switch (info) {
>> +	case IIO_EV_INFO_VALUE:
>> +		if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) {
>> +			u32 th_lo, th_hi;
>> +
>> +			if (opt4060_get_thresholds(chip, &th_lo, &th_hi))
>> +				return -EFAULT;
>> +			if (dir == IIO_EV_DIR_FALLING)
>> +				th_lo = val;
>> +			else if (dir == IIO_EV_DIR_RISING)
>> +				th_hi = val;
>> +			if (opt4060_set_thresholds(chip, th_lo, th_hi))
>> +				return -EFAULT;
>> +			ret = 0;
> 			return 0;
>> +		}
> 		return -EINVAL;
>> +		break;
>> +	case IIO_EV_INFO_PERIOD:
>> +		return opt4060_write_ev_period(chip, val, val2);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +	return ret;
> Drop this as with above always returned already.
> 
>> +}
>> +
>> +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 ret = -EINVAL;
>> +	struct opt4060_chip *chip = iio_priv(indio_dev);
>> +
>> +	if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) {
> As below. Consider flipping the condition for an early return
> Check all similar cases for whether it improves readability.
> 
> 
>> +		int ch_idx = chan->scan_index;
>> +		int ch_sel;
>> +
>> +		if (opt4060_get_channel_sel(chip, &ch_sel))
>> +			return -EFAULT;
>> +
>> +		if (((dir == IIO_EV_DIR_FALLING) && chip->thresh_event_lo_active) ||
>> +		    ((dir == IIO_EV_DIR_RISING) && chip->thresh_event_hi_active))
>> +			ret = (ch_sel == ch_idx);
>> +		else
>> +			ret = FALSE;
>> +	}
>> +	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 ret = -EINVAL;
>> +	struct opt4060_chip *chip = iio_priv(indio_dev);
>> +
>> +	if (chan->type == IIO_LIGHT && type == IIO_EV_TYPE_THRESH) {
> Maybe flip the condition to reduce effective indent.
> 	if (chan->type != IIO_LIGHT)
> 		return -EINVAL;
> 	if (type != IIO_EV_TYPE_THRESH)
> 		reuturn -EINVAL;
> 
> etc
>> +		int ch_idx = chan->scan_index;
>> +		int ch_sel;
>> +
>> +		if (opt4060_get_channel_sel(chip, &ch_sel))
>> +			return -EFAULT;
>> +
>> +		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;
>> +			if (opt4060_set_channel_sel(chip, ch_idx))
>> +				return -EFAULT;
>> +			ret = 0;
>> +		} else {
>> +			if (ch_idx == ch_sel) {
>> +				if (dir == IIO_EV_DIR_FALLING)
>> +					chip->thresh_event_lo_active = FALSE;
> false etc
> 
>> +				else if (dir == IIO_EV_DIR_RISING)
>> +					chip->thresh_event_hi_active = FALSE;
>> +			}
>> +			ret = 0;
>> +		}
>> +
>> +		if (opt4060_event_set_state(chip, chip->thresh_event_hi_active |
>> +					    chip->thresh_event_lo_active))
>> +			return -EFAULT;
>> +	}
>> +	return ret;
>> +}
> 
>> +
>> +static bool opt4060_volatile_reg(struct device *dev, unsigned int reg)
>> +{
>> +	if ((reg >= OPT4060_RED_MSB && reg <= OPT4060_CLEAR_LSB) ||
>> +	    reg == OPT4060_RES_CTRL)
>> +		return true;
>> +	return false;
> 
> As below, just return the condition.
> 
>> +}
>> +
>> +static bool opt4060_writable_reg(struct device *dev, unsigned int reg)
>> +{
>> +	if (reg >= OPT4060_THRESHOLD_LOW || reg >= OPT4060_INT_CTRL)
>> +		return true;
>> +	return false;
> return reg >=...
>> +}
>> +
>> +static bool opt4060_readonly_reg(struct device *dev, unsigned int reg)
>> +{
>> +	if (reg == OPT4060_DEVICE_ID)
>> +		return true;
>> +	return false;
> 	return reg == ..
> 
>> +}
>> +
>> +static bool opt4060_readable_reg(struct device *dev, unsigned int reg)
>> +{
>> +	/* Volatile, writable and read-only registers are readable. */
>> +	if (opt4060_volatile_reg(dev, reg) || opt4060_writable_reg(dev, reg) ||
>> +	    opt4060_readonly_reg(dev, reg))
>> +		return TRUE;
> true
> 
>> +	return false;
> 
> return opt....
> 
>> +}
>> +
> 
>> +
>> +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;
>> +	int i = 0;
>> +
>> +	memset(&raw, 0, sizeof(raw));
>> +
>> +	for_each_set_bit(chan, idev->active_scan_mask, idev->masklength) {
> Try compiling this on latest togreg branch.  We have removed direct
> access to masklength
> 
> There is a iio_for_each_active_channel() macro that handles this usage
> more cleanly.
> 
>> +		ret = opt4060_read_raw_value(chip,
>> +					     opt4060_channels[chan].address,
>> +					     &raw.chan[i++]);
>> +		if (ret) {
>> +			dev_err(chip->dev, "Reading raw 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 int opt4060_setup_triggers(struct opt4060_chip *chip, struct iio_dev *idev)
>> +{
>> +	struct iio_trigger *data_trigger;
>> +	struct iio_trigger *threshold_trigger;
>> +	char *name;
>> +	int ret;
>> +
>> +	ret = devm_iio_triggered_buffer_setup(chip->dev, idev,
>> +					      &iio_pollfunc_store_time,
>> +					      opt4060_trigger_handler,
>> +					      &opt4060_buffer_ops);
>> +	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));
> 
> Where it doesn't result in very long lines, align to just after (
> 
> That may mean you have more line breaks. If so that is fine.
> 
> 
>> +	if (!data_trigger)
>> +		return -ENOMEM;
> Slight preference for a blank line after error checks like this.
> 
>> +	chip->data_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");
>> +
>> +	threshold_trigger = devm_iio_trigger_alloc(chip->dev, "%s-threshold-dev%d",
>> +				       idev->name, iio_device_id(idev));
> 
> This needs some explanation. Why a threshold trigger rather than just using
> events?  Superficially looks like you should just use the event interfaces.
> 
>> +	if (!threshold_trigger)
>> +		return -ENOMEM;
>> +	chip->threshold_trig = threshold_trigger;
>> +	threshold_trigger->ops = &opt4060_trigger_ops;
>> +	iio_trigger_set_drvdata(threshold_trigger, idev);
>> +	ret = devm_iio_trigger_register(chip->dev, threshold_trigger);
>> +	if (ret)
>> +		return dev_err_probe(chip->dev, ret,
>> +				     "Threshold trigger registration failed\n");
>> +
> One blank line is plenty.
>> +
>> +	name = devm_kasprintf(chip->dev, GFP_KERNEL, "%s-opt4060",
>> +			      dev_name(chip->dev));
> 
> Check for failure to allocate.
> 
>> +
>> +	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;
> 
> unsigned int to match the regmap parameter type.
> 
>> +
>> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*chip));
>> +	if (!indio_dev)
>> +		return -ENOMEM;
>> +
>> +	chip = iio_priv(indio_dev);
>> +
>> +	/* Request regulator */
> 
> very obvious. Drop the comment.
> 
>> +	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_warn(dev, "Device ID: %#04x unknown\n", dev_id);
> 
> Prefer dev_info() for this as it may well be a valid fallback
> compatible.
> 
>> +
>> +	indio_dev->channels = opt4060_channels;
>> +	indio_dev->num_channels = ARRAY_SIZE(opt4060_channels);
>> +	indio_dev->modes = INDIO_DIRECT_MODE;
>> +	indio_dev->name = OPT_4060_DRV_NAME;
>> +
>> +	/* Initialize device with default settings */
> 
> As below. Comment is (rightly) obvious from the code, so don't have
> a comment.
> 
>> +	ret = opt4060_load_defaults(chip);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret,
>> +				     "Failed to set sensor defaults\n");
>> +
>> +	/* If irq is enabled, initialize triggers */
> The comment doesn't add anything not obvious from the code.  I'd drop it
> and any similar ones.
>> +	if (chip->irq) {
>> +		ret = opt4060_setup_triggers(chip, indio_dev);
>> +		if (ret)
>> +			return ret;
>> +	} else {
>> +		dev_info(chip->dev, "No IRQ, events and triggers disabled\n");
> 
> Too noisy.  At most dev_dbg.  Probably not useful at all as it is fairly easy
> to see no irq was registered.
> 
>> +	}
>> +
>> +	ret = devm_add_action_or_reset(dev, opt4060_chip_off_action, chip);
> 
> This needs to be alongside whatever turned it on. Or if that is a side effect
> of later accesses (i.e. it is powered up only channel read) then add
> a comment to that affect.
> 
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret,
>> +				     "Failed to setup power off action\n");
>> +
>> +	return devm_iio_device_register(dev, indio_dev);
>> +}
>> +
>> +/*
>> + * The compatible string determines which constants to use depending on
>> + * opt4060 packaging
> There is only one. So this comment is currently meaningless.
> 
>> + */
>> +static const struct i2c_device_id opt4060_id[] = {
>> +	{ OPT_4060_DRV_NAME, 0 },
> Put the string directly in here. There is no reason why this
> should match the DRV_NAME.  Also drop the 0 as it is currently
> pointless and when you do introduce some data it should be a pointer
> anyway.
> 
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(i2c, opt4060_id);
>> +
>> +static const struct of_device_id opt4060_of_match[] = {
>> +	{ .compatible = "ti,opt4060" },
>> +	{}
> 
> Trivial but I'm trying to get consistency in IIO (slowly) on 
> { } for these terminators (so with the space).
> I'd prefer to not introduce more instances of {}
> 
> Thanks,
> 
> Jonathan
> 
> 
>> +};
>> +MODULE_DEVICE_TABLE(of, opt4060_of_match);

[PATCH v2 1/2] dt-bindings: iio: light: Document TI OPT4060 RGBW sensor
[PATCH v2 2/2] iio: light: Add support for TI OPT4060 color sensor