[PATCH v3 0/3] iio:light: add driver for veml6046x00 RGBIR color sensor

[PATCH v3 0/3] iio:light: add driver for veml6046x00 RGBIR color sensor

[Andreas Klinger]

This patchset adds an IIO driver for Vishay veml6046x00 RGBIR color sensor

Changes in v3:
- implement a lot of feedback from Jonathan
- change scale value to real factor of lux per raw count instead of hardware
  gain
- optimize code by using more lookup tables
- remove unimplemented threshold functionality

Changes in v2:
- fix missing include for example in vishay,veml6046x00.yaml

Andreas Klinger (3):
  dt-bindings: iio: light: veml6046x00: add color sensor
  iio: light: add support for veml6046x00 RGBIR color sensor
  MAINTAINER: add maintainer for veml6046x00

 .../iio/light/vishay,veml6046x00.yaml         |  51 +
 MAINTAINERS                                   |   6 +
 drivers/iio/light/Kconfig                     |  13 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/iio/light/veml6046x00.c               | 889 ++++++++++++++++++
 5 files changed, 960 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
 create mode 100644 drivers/iio/light/veml6046x00.c

-- 
2.39.5

[PATCH v3 1/3] dt-bindings: iio: light: veml6046x00: add color sensor

[Andreas Klinger]

Add a new compatible for Vishay high accuracy RGBIR color sensor
veml6046x00.

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 .../iio/light/vishay,veml6046x00.yaml         | 51 +++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml

diff --git a/Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml b/Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
new file mode 100644
index 000000000000..112d448ff0bf
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/vishay,veml6046x00.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Vishay VEML6046X00 High accuracy RGBIR color sensor
+
+maintainers:
+  - Andreas Klinger <ak@it-klinger.de>
+
+description:
+  VEML6046X00 datasheet at https://www.vishay.com/docs/80173/veml6046x00.pdf
+
+properties:
+  compatible:
+    enum:
+      - vishay,veml6046x00
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        color-sensor@29 {
+            compatible = "vishay,veml6046x00";
+            reg = <0x29>;
+            vdd-supply = <&vdd_reg>;
+            interrupt-parent = <&gpio2>;
+            interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
+        };
+    };
+...
-- 
2.39.5

[PATCH v3 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Andreas Klinger]

Add Vishay VEML6046X00 high accuracy RGBIR color sensor.

This sensor provides three colour (red, green and blue) as well as one
infrared (IR) channel through I2C.

Support direct and buffered mode.

An optional interrupt for signaling green colour threshold underflow or
overflow is not supported so far.

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/light/Kconfig       |  13 +
 drivers/iio/light/Makefile      |   1 +
 drivers/iio/light/veml6046x00.c | 889 ++++++++++++++++++++++++++++++++
 3 files changed, 903 insertions(+)
 create mode 100644 drivers/iio/light/veml6046x00.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 4a7d983c9cd4..ac1408d374c9 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -724,6 +724,19 @@ config VEML6040
 	  To compile this driver as a module, choose M here: the
 	  module will be called veml6040.
 
+config VEML6046X00
+	tristate "VEML6046X00 RGBIR color sensor"
+	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	depends on I2C
+	help
+	  Say Y here if you want to build a driver for the Vishay VEML6046X00
+	  high accuracy RGBIR color sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called veml6046x00.
+
 config VEML6070
 	tristate "VEML6070 UV A light sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 8229ebe6edc4..c0048e0d5ca8 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_VCNL4035)		+= vcnl4035.o
 obj-$(CONFIG_VEML3235)		+= veml3235.o
 obj-$(CONFIG_VEML6030)		+= veml6030.o
 obj-$(CONFIG_VEML6040)		+= veml6040.o
+obj-$(CONFIG_VEML6046X00)	+= veml6046x00.o
 obj-$(CONFIG_VEML6070)		+= veml6070.o
 obj-$(CONFIG_VEML6075)		+= veml6075.o
 obj-$(CONFIG_VL6180)		+= vl6180.o
diff --git a/drivers/iio/light/veml6046x00.c b/drivers/iio/light/veml6046x00.c
new file mode 100644
index 000000000000..5bde3b35ada4
--- /dev/null
+++ b/drivers/iio/light/veml6046x00.c
@@ -0,0 +1,889 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * VEML6046X00 High Accuracy RGBIR Color Sensor
+ *
+ * Copyright (c) 2025 Andreas Klinger <ak@it-klinger.de>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+#include <linux/units.h>
+#include <linux/pm_runtime.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+/* Device registers */
+#define VEML6046X00_REG_CONF0       0x00
+#define VEML6046X00_REG_CONF1       0x01
+#define VEML6046X00_REG_THDH_L      0x04
+#define VEML6046X00_REG_THDH_H      0x05
+#define VEML6046X00_REG_THDL_L      0x06
+#define VEML6046X00_REG_THDL_H      0x07
+#define VEML6046X00_REG_R_L         0x10
+#define VEML6046X00_REG_R_H         0x11
+#define VEML6046X00_REG_G_L         0x12
+#define VEML6046X00_REG_G_H         0x13
+#define VEML6046X00_REG_B_L         0x14
+#define VEML6046X00_REG_B_H         0x15
+#define VEML6046X00_REG_IR_L        0x16
+#define VEML6046X00_REG_IR_H        0x17
+#define VEML6046X00_REG_ID_L        0x18
+#define VEML6046X00_REG_ID_H        0x19
+#define VEML6046X00_REG_INT_L       0x1A
+#define VEML6046X00_REG_INT_H       0x1B
+#define VEML6046X00_REG_DATA(ch)    (VEML6046X00_REG_R_L + (ch))
+
+/* Bit masks for specific functionality */
+#define VEML6046X00_CONF0_ON_0      BIT(0)
+#define VEML6046X00_CONF0_INT       BIT(1)
+#define VEML6046X00_CONF0_AF_TRIG   BIT(2)
+#define VEML6046X00_CONF0_AF        BIT(3)
+#define VEML6046X00_CONF0_IT        GENMASK(6, 4)
+#define VEML6046X00_CONF1_CAL       BIT(0)
+#define VEML6046X00_CONF1_PERS      GENMASK(2, 1)
+#define VEML6046X00_CONF1_GAIN      GENMASK(4, 3)
+#define VEML6046X00_CONF1_PD_D2     BIT(6)
+#define VEML6046X00_CONF1_ON_1      BIT(7)
+#define VEML6046X00_INT_TH_H        BIT(1)
+#define VEML6046X00_INT_TH_L        BIT(2)
+#define VEML6046X00_INT_DRDY        BIT(3)
+#define VEML6046X00_INT_MASK        (VEML6046X00_INT_TH_H | \
+				VEML6046X00_INT_TH_L | VEML6046X00_INT_DRDY)
+
+#define	VEML6046X00_GAIN_1          0x0
+#define	VEML6046X00_GAIN_2          0x1
+#define	VEML6046X00_GAIN_0_66       0x2
+#define	VEML6046X00_GAIN_0_5        0x3
+
+#define VEML6046X00_PD_2_2          0x0
+#define VEML6046X00_PD_1_2          BIT(6)
+
+/* Autosuspend delay */
+#define VEML6046X00_AUTOSUSPEND_MS  3000
+
+enum veml6046x00_scan {
+	VEML6046X00_SCAN_R,
+	VEML6046X00_SCAN_G,
+	VEML6046X00_SCAN_B,
+	VEML6046X00_SCAN_IR,
+	VEML6046X00_SCAN_TIMESTAMP,
+};
+
+/*
+ * regmap fields
+ */
+struct veml6046x00_rf {
+	struct regmap_field *int_en;
+	struct regmap_field *mode;
+	struct regmap_field *trig;
+	struct regmap_field *it;
+	struct regmap_field *pers;
+};
+
+struct veml6046x00_data {
+	struct device *dev;
+	struct regmap *regmap;
+	struct iio_trigger *trig;
+	struct veml6046x00_rf rf;
+};
+
+struct veml6046x00_scan_buf {
+	__le16 chans[4];
+	aligned_s64 timestamp;
+};
+
+/*
+ * Integration times
+ * Register value on veml6046x00 is identical with array index
+ * --> no separate table needed
+ */
+static const int veml6046x00_it[][2] = {
+	{ 0, 3125 },
+	{ 0, 6250 },
+	{ 0, 12500 },
+	{ 0, 25000 },
+	{ 0, 50000 },
+	{ 0, 100000 },
+	{ 0, 200000 },
+	{ 0, 400000 },
+};
+
+/*
+ * Gains here in the driver are not exactly the same as in the datasheet of the
+ * sensor. The gain in the driver is a combination of the gain of the sensor
+ * with the photodiode size (PD).
+ * The following combinations are possible:
+ *   gain(driver) = gain(sensor) * PD
+ *           0.25 = x0.5  * 1/2
+ *           0.33 = x0.66 * 1/2
+ *           0.5  = x0.5  * 2/2
+ *           0.66 = x0.66 * 2/2
+ *           1    = x1    * 2/2
+ *           2    = x2    * 2/2
+ */
+
+/*
+ * veml6046x00_gain_pd - translation from gain index (used in the driver) to
+ * gain (sensor) and PD
+ * @gain_sen:	Gain used in the sensor as described in the datasheet of the
+ *		sensor
+ * @pd:		Photodiode size in the sensor
+ */
+struct veml6046x00_gain_pd {
+	int gain_sen;
+	int pd;
+};
+
+#define VEML6046X00_GAIN_PD(_gain_sen, _pd)				\
+{									\
+	.gain_sen = (_gain_sen),					\
+	.pd = (_pd),							\
+}
+
+static const struct veml6046x00_gain_pd veml6046x00_gain_pd[] = {
+	VEML6046X00_GAIN_PD(VEML6046X00_GAIN_0_5, VEML6046X00_PD_1_2),
+	VEML6046X00_GAIN_PD(VEML6046X00_GAIN_0_66, VEML6046X00_PD_1_2),
+	VEML6046X00_GAIN_PD(VEML6046X00_GAIN_0_5, VEML6046X00_PD_2_2),
+	VEML6046X00_GAIN_PD(VEML6046X00_GAIN_0_66, VEML6046X00_PD_2_2),
+	VEML6046X00_GAIN_PD(VEML6046X00_GAIN_1, VEML6046X00_PD_2_2),
+	VEML6046X00_GAIN_PD(VEML6046X00_GAIN_2, VEML6046X00_PD_2_2),
+};
+
+/*
+ * Factors for lux / raw count in dependency of integration time (IT) as rows
+ * and driver gain in columns
+ * Columns:
+ * x0.25 x0.33 x0.5 x0.66 x1 x2
+ * Rows:
+ * 3.125 6.25 12.5 25 50 100 200 400ms
+ */
+static const u32 veml6046x00_it_gains[][6][2] = {
+{{5, 376000}, {4,  72700}, {2, 688000}, {2,  36400}, {1, 344000}, {0, 672000}},
+{{2, 688000}, {2,  36350}, {1, 344000}, {1,  18200}, {0, 672000}, {0, 336000}},
+{{1, 344000}, {1,  18175}, {0, 672000}, {0, 509100}, {0, 336000}, {0, 168000}},
+{{0, 672000}, {0, 509087}, {0, 336000}, {0, 254550}, {0, 168000}, {0,  84000}},
+{{0, 336000}, {0, 254543}, {0, 168000}, {0, 127275}, {0,  84000}, {0,  42000}},
+{{0, 168000}, {0, 127271}, {0,  84000}, {0,  63637}, {0,  42000}, {0,  21000}},
+{{0,  84000}, {0,  63635}, {0,  42000}, {0,  31818}, {0,  21000}, {0,  10500}},
+{{0,  42000}, {0,  31817}, {0,  21000}, {0,  15909}, {0,  10500}, {0,   5250}},
+};
+
+/*
+ * Two bits (RGB_ON_0 and RGB_ON_1) must be cleared to power on the device.
+ */
+static int veml6046x00_power_on(struct veml6046x00_data *data)
+{
+	int ret;
+
+	ret = regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF0,
+						VEML6046X00_CONF0_ON_0);
+	if (ret) {
+		dev_err(data->dev, "Failed to set bit for power on %d\n", ret);
+		return ret;
+	}
+
+	return regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF1,
+						VEML6046X00_CONF1_ON_1);
+}
+
+/*
+ * Two bits (RGB_ON_0 and RGB_ON_1) must be set to power off the device.
+ */
+static int veml6046x00_shutdown(struct veml6046x00_data *data)
+{
+	int ret;
+
+	ret = regmap_set_bits(data->regmap, VEML6046X00_REG_CONF0,
+			      VEML6046X00_CONF0_ON_0);
+	if (ret) {
+		dev_err(data->dev, "Failed to set bit for shutdown %d\n", ret);
+		return ret;
+	}
+
+	return regmap_set_bits(data->regmap, VEML6046X00_REG_CONF1,
+			       VEML6046X00_CONF1_ON_1);
+}
+
+static void veml6046x00_shutdown_action(void *data)
+{
+	veml6046x00_shutdown(data);
+}
+
+static const struct iio_chan_spec veml6046x00_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.address = VEML6046X00_REG_R_L,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_RED,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
+					   BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+						     BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = VEML6046X00_SCAN_R,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
+	},
+	{
+		.type = IIO_LIGHT,
+		.address = VEML6046X00_REG_G_L,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_GREEN,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
+					   BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+						     BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = VEML6046X00_SCAN_G,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
+	},
+	{
+		.type = IIO_LIGHT,
+		.address = VEML6046X00_REG_B_L,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_BLUE,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
+					   BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+						     BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = VEML6046X00_SCAN_B,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
+	},
+	{
+		.type = IIO_LIGHT,
+		.address = VEML6046X00_REG_IR_L,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_IR,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME) |
+					   BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) |
+						     BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = VEML6046X00_SCAN_IR,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(VEML6046X00_SCAN_TIMESTAMP),
+};
+
+static const struct regmap_config veml6046x00_regmap_config = {
+	.name = "veml6046x00_regmap",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = VEML6046X00_REG_INT_H,
+};
+
+static const struct reg_field veml6046x00_rf_int_en =
+	REG_FIELD(VEML6046X00_REG_CONF0, 1, 1);
+
+static const struct reg_field veml6046x00_rf_trig =
+	REG_FIELD(VEML6046X00_REG_CONF0, 2, 2);
+
+static const struct reg_field veml6046x00_rf_mode =
+	REG_FIELD(VEML6046X00_REG_CONF0, 3, 3);
+
+static const struct reg_field veml6046x00_rf_it =
+	REG_FIELD(VEML6046X00_REG_CONF0, 4, 6);
+
+static const struct reg_field veml6046x00_rf_pers =
+	REG_FIELD(VEML6046X00_REG_CONF1, 1, 2);
+
+static int veml6046x00_regfield_init(struct veml6046x00_data *data)
+{
+	struct regmap *regmap = data->regmap;
+	struct device *dev = data->dev;
+	struct regmap_field *rm_field;
+	struct veml6046x00_rf *rf = &data->rf;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6046x00_rf_int_en);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->int_en = rm_field;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6046x00_rf_mode);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->mode = rm_field;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6046x00_rf_trig);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->trig = rm_field;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6046x00_rf_it);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->it = rm_field;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6046x00_rf_pers);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->pers = rm_field;
+
+	return 0;
+}
+
+static int veml6046x00_get_it_index(struct veml6046x00_data *data)
+{
+	int ret, reg;
+
+	ret = regmap_field_read(data->rf.it, &reg);
+	if (ret)
+		return ret;
+
+	/* register value is identical with index of array */
+	if ((reg >= 0) && (reg < ARRAY_SIZE(veml6046x00_it)))
+		return reg;
+
+	return -EINVAL;
+}
+
+static int veml6046x00_get_it_usec(struct veml6046x00_data *data, int *it_usec)
+{
+	int ret, reg;
+
+	ret = regmap_field_read(data->rf.it, &reg);
+	if (ret)
+		return ret;
+
+	if ((reg >= 0) && (reg < ARRAY_SIZE(veml6046x00_it)))
+		*it_usec = veml6046x00_it[reg][1];
+	else
+		return -EINVAL;
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int veml6046x00_set_it(struct iio_dev *iio, int val, int val2)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(veml6046x00_it); i++) {
+		if ((veml6046x00_it[i][0] == val) &&
+		    (veml6046x00_it[i][1] == val2))
+			return regmap_field_write(data->rf.it, i);
+	}
+
+	return -EINVAL;
+}
+
+static int veml6046x00_get_val_gain_idx(struct veml6046x00_data *data, int val,
+								int val2)
+{
+	int i;
+	int it_idx;
+
+	it_idx = veml6046x00_get_it_index(data);
+	if (it_idx < 0)
+		return it_idx;
+
+	for (i = 0; i < ARRAY_SIZE(veml6046x00_it_gains[it_idx]); i++) {
+		if ((veml6046x00_it_gains[it_idx][i][0] == val) &&
+		    (veml6046x00_it_gains[it_idx][i][1] == val2)) {
+			return i;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int veml6046x00_get_gain_idx(struct veml6046x00_data *data)
+{
+	int ret, reg;
+	int i;
+	int reg_gain;
+	int reg_pd;
+
+	ret = regmap_read(data->regmap, VEML6046X00_REG_CONF1, &reg);
+	if (ret)
+		return ret;
+
+	reg_gain = FIELD_GET(VEML6046X00_CONF1_GAIN, reg);
+	reg_pd = reg & VEML6046X00_CONF1_PD_D2;
+
+	for (i = 0; i < ARRAY_SIZE(veml6046x00_gain_pd); i++) {
+		if ((veml6046x00_gain_pd[i].gain_sen == reg_gain) &&
+		    (veml6046x00_gain_pd[i].pd == reg_pd)) {
+			return i;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int veml6046x00_set_scale(struct iio_dev *iio, int val, int val2)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	int new_scale;
+	int gain_idx;
+
+	gain_idx = veml6046x00_get_val_gain_idx(data, val, val2);
+	if (gain_idx < 0)
+		return gain_idx;
+
+	new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN,
+			veml6046x00_gain_pd[gain_idx].gain_sen) |
+			veml6046x00_gain_pd[gain_idx].pd;
+
+	return regmap_update_bits(data->regmap, VEML6046X00_REG_CONF1,
+				 VEML6046X00_CONF1_GAIN |
+				 VEML6046X00_CONF1_PD_D2,
+				 new_scale);
+}
+
+static int veml6046x00_get_scale(struct veml6046x00_data *data,
+							int *val, int *val2)
+{
+	int gain_idx;
+	int it_idx;
+
+	gain_idx = veml6046x00_get_gain_idx(data);
+	if (gain_idx < 0)
+		return gain_idx;
+
+	it_idx = veml6046x00_get_it_index(data);
+	if (it_idx < 0)
+		return it_idx;
+
+	*val = veml6046x00_it_gains[it_idx][gain_idx][0];
+	*val2 = veml6046x00_it_gains[it_idx][gain_idx][1];
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+/*
+ * veml6046x00_wait_data_available: Wait until data is available in the sensor
+ * returns:
+ * 1	if data is available (AF_DATA_READY is set)
+ * 0	if no data is available
+ * -EIO	in case of error
+ */
+static int veml6046x00_wait_data_available(struct iio_dev *iio, int usecs)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	int ret, reg;
+	int cnt = 2;
+	int i;
+
+	for (i = 0; i < cnt; i++) {
+		ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, &reg);
+		if (ret) {
+			dev_err(data->dev,
+					"Failed to read interrupt register %d\n", ret);
+			return -EIO;
+		}
+
+		if (reg & VEML6046X00_INT_DRDY)
+			return 1;
+
+		if (i < cnt)
+			fsleep(usecs);
+	}
+
+	return 0;
+}
+
+static int veml6046x00_single_read(struct iio_dev *iio,
+					enum iio_modifier modifier, int *val)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	int addr, it_usec, ret;
+	__le16 reg;
+
+	switch (modifier) {
+	case IIO_MOD_LIGHT_RED:
+		addr = VEML6046X00_REG_R_L;
+		break;
+	case IIO_MOD_LIGHT_GREEN:
+		addr = VEML6046X00_REG_G_L;
+		break;
+	case IIO_MOD_LIGHT_BLUE:
+		addr = VEML6046X00_REG_B_L;
+		break;
+	case IIO_MOD_LIGHT_IR:
+		addr = VEML6046X00_REG_IR_L;
+		break;
+	default:
+		return -EINVAL;
+	}
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret)
+		return ret;
+
+	ret = veml6046x00_get_it_usec(data, &it_usec);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_field_write(data->rf.mode, 1);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(data->rf.trig, 1);
+	if (ret)
+		return ret;
+
+	/* integration time + 10 % to ensure completion */
+	fsleep(it_usec + (it_usec / 10));
+
+	ret = veml6046x00_wait_data_available(iio, it_usec * 10);
+	if (ret == 1) {
+		dev_dbg(data->dev, "data ready\n");
+	} else {
+		dev_warn(data->dev, "no data ready ret: %d\n", ret);
+		goto no_data;
+	}
+
+	if (!iio_device_claim_direct(iio))
+		return -EBUSY;
+
+	ret = regmap_bulk_read(data->regmap, addr, &reg, sizeof(reg));
+	iio_device_release_direct(iio);
+	if (ret)
+		return ret;
+
+	pm_runtime_mark_last_busy(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
+
+	*val = le16_to_cpu(reg);
+
+	return IIO_VAL_INT;
+
+no_data:
+	pm_runtime_mark_last_busy(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
+
+	return -EINVAL;
+}
+
+static int veml6046x00_read_raw(struct iio_dev *iio,
+				struct iio_chan_spec const *chan, int *val,
+				int *val2, long mask)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->type != IIO_LIGHT)
+			return -EINVAL;
+		return veml6046x00_single_read(iio, chan->channel2, val);
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = 0;
+		return veml6046x00_get_it_usec(data, val2);
+	case IIO_CHAN_INFO_SCALE:
+		return veml6046x00_get_scale(data, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml6046x00_read_avail(struct iio_dev *iio,
+				  struct iio_chan_spec const *chan,
+				  const int **vals, int *type, int *length,
+				  long mask)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	int it_idx;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		*vals = (int *)&veml6046x00_it;
+		*length = 2 * ARRAY_SIZE(veml6046x00_it);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SCALE:
+		it_idx = veml6046x00_get_it_index(data);
+		if (it_idx < 0)
+			return it_idx;
+		*vals = (int *)&veml6046x00_it_gains[it_idx];
+		*length = 2 * ARRAY_SIZE(veml6046x00_it_gains[it_idx]);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml6046x00_write_raw(struct iio_dev *iio,
+				 struct iio_chan_spec const *chan,
+				 int val, int val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return veml6046x00_set_it(iio, val, val2);
+	case IIO_CHAN_INFO_SCALE:
+		return veml6046x00_set_scale(iio, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info veml6046x00_info_no_irq = {
+	.read_raw = veml6046x00_read_raw,
+	.read_avail = veml6046x00_read_avail,
+	.write_raw = veml6046x00_write_raw,
+};
+
+static int veml6046x00_buffer_preenable(struct iio_dev *iio)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	struct device *dev = data->dev;
+	int ret;
+
+	ret = regmap_field_write(data->rf.mode, 0);
+	if (ret) {
+		dev_err(data->dev, "Failed to set mode %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_field_write(data->rf.trig, 0);
+	if (ret) {
+		dev_err(data->dev, "Failed to set trigger %d\n", ret);
+		return ret;
+	}
+
+	return pm_runtime_resume_and_get(dev);
+}
+
+static int veml6046x00_buffer_postdisable(struct iio_dev *iio)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	struct device *dev = data->dev;
+	int ret;
+
+	ret = regmap_field_write(data->rf.mode, 1);
+	if (ret) {
+		dev_err(data->dev, "Failed to set mode %d\n", ret);
+		return ret;
+	}
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops veml6046x00_buffer_setup_ops = {
+	.preenable = veml6046x00_buffer_preenable,
+	.postdisable = veml6046x00_buffer_postdisable,
+};
+
+static irqreturn_t veml6046x00_trig_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *iio = pf->indio_dev;
+	struct veml6046x00_data *data = iio_priv(iio);
+	int ret;
+	struct veml6046x00_scan_buf scan;
+
+	memset(&scan, 0, sizeof(scan));
+
+	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_R_L, &scan.chans,
+						sizeof(scan.chans));
+	if (ret)
+		goto done;
+
+	iio_push_to_buffers_with_timestamp(iio, &scan, iio_get_time_ns(iio));
+
+done:
+	iio_trigger_notify_done(iio->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int veml6046x00_validate_part_id(struct veml6046x00_data *data)
+{
+	int part_id, ret;
+	__le16 reg;
+
+	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_ID_L, &reg, sizeof(reg));
+	if (ret)
+		return dev_err_probe(data->dev, ret, "Failed to read ID\n");
+
+	part_id = le16_to_cpu(reg);
+	if (part_id != 0x0001)
+		dev_info(data->dev, "Unknown ID %#02x\n", part_id);
+
+	return 0;
+}
+
+static int veml6046x00_setup_device(struct iio_dev *iio)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	struct device *dev = data->dev;
+	int ret, val;
+	__le16 reg16;
+	uint8_t reg[2];
+
+	reg[0] = VEML6046X00_CONF0_AF;
+	reg[1] = 0x00;
+	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_CONF0, reg, sizeof(reg));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set configuration\n");
+
+	reg16 = cpu_to_le16(0);
+	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_THDL_L, &reg16, sizeof(reg16));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set low threshold\n");
+
+	reg16 = cpu_to_le16(U16_MAX);
+	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_THDH_L, &reg16, sizeof(reg16));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set high threshold\n");
+
+	ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, &val);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to clear interrupts\n");
+
+	return 0;
+}
+
+static int veml6046x00_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct veml6046x00_data *data;
+	struct iio_dev *iio;
+	struct regmap *regmap;
+	int ret;
+
+	regmap = devm_regmap_init_i2c(i2c, &veml6046x00_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to set regmap\n");
+
+	iio = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!iio)
+		return -ENOMEM;
+
+	data = iio_priv(iio);
+	i2c_set_clientdata(i2c, iio);
+	data->dev = dev;
+	data->regmap = regmap;
+
+	ret = veml6046x00_regfield_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to init regfield\n");
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulator\n");
+
+	/* bring device in a known state and switch device on */
+	ret = veml6046x00_setup_device(iio);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, veml6046x00_shutdown_action, data);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to add shut down action\n");
+
+	ret = pm_runtime_set_active(dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to activate PM runtime\n");
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable PM runtime\n");
+
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_autosuspend_delay(dev, VEML6046X00_AUTOSUSPEND_MS);
+	pm_runtime_use_autosuspend(dev);
+
+	ret = veml6046x00_validate_part_id(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to validate device ID\n");
+
+	iio->name = "veml6046x00";
+	iio->channels = veml6046x00_channels;
+	iio->num_channels = ARRAY_SIZE(veml6046x00_channels);
+	iio->modes = INDIO_DIRECT_MODE;
+
+	iio->info = &veml6046x00_info_no_irq;
+
+	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
+					      veml6046x00_trig_handler,
+					      &veml6046x00_buffer_setup_ops);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register triggered buffer");
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	ret = devm_iio_device_register(dev, iio);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register iio device");
+
+	return 0;
+}
+
+static int veml6046x00_runtime_suspend(struct device *dev)
+{
+	struct veml6046x00_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return veml6046x00_shutdown(data);
+}
+
+static int veml6046x00_runtime_resume(struct device *dev)
+{
+	struct veml6046x00_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return veml6046x00_power_on(data);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(veml6046x00_pm_ops, veml6046x00_runtime_suspend,
+				 veml6046x00_runtime_resume, NULL);
+
+static const struct of_device_id veml6046x00_of_match[] = {
+	{
+		.compatible = "vishay,veml6046x00",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, veml6046x00_of_match);
+
+static const struct i2c_device_id veml6046x00_id[] = {
+	{ "veml6046x00" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, veml6046x00_id);
+
+static struct i2c_driver veml6046x00_driver = {
+	.driver = {
+		.name = "veml6046x00",
+		.of_match_table = veml6046x00_of_match,
+		.pm = pm_ptr(&veml6046x00_pm_ops),
+	},
+	.probe = veml6046x00_probe,
+	.id_table = veml6046x00_id,
+};
+module_i2c_driver(veml6046x00_driver);
+
+MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
+MODULE_DESCRIPTION("VEML6046X00 RGBIR Color Sensor");
+MODULE_LICENSE("GPL");
-- 
2.39.5

Re: [PATCH v3 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Andy Shevchenko]

On Mon, May 05, 2025 at 10:23:12PM +0200, Andreas Klinger wrote:
> Add Vishay VEML6046X00 high accuracy RGBIR color sensor.
> 
> This sensor provides three colour (red, green and blue) as well as one
> infrared (IR) channel through I2C.
> 
> Support direct and buffered mode.
> 
> An optional interrupt for signaling green colour threshold underflow or
> overflow is not supported so far.

...

> +#include <linux/bitfield.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +#include <linux/pm_runtime.h>

+ blank line

and keep each group sorted.

There are missing inclusions as well.

> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>

> +/* Bit masks for specific functionality */
> +#define VEML6046X00_CONF0_ON_0      BIT(0)
> +#define VEML6046X00_CONF0_INT       BIT(1)
> +#define VEML6046X00_CONF0_AF_TRIG   BIT(2)
> +#define VEML6046X00_CONF0_AF        BIT(3)
> +#define VEML6046X00_CONF0_IT        GENMASK(6, 4)
> +#define VEML6046X00_CONF1_CAL       BIT(0)
> +#define VEML6046X00_CONF1_PERS      GENMASK(2, 1)
> +#define VEML6046X00_CONF1_GAIN      GENMASK(4, 3)
> +#define VEML6046X00_CONF1_PD_D2     BIT(6)
> +#define VEML6046X00_CONF1_ON_1      BIT(7)
> +#define VEML6046X00_INT_TH_H        BIT(1)
> +#define VEML6046X00_INT_TH_L        BIT(2)
> +#define VEML6046X00_INT_DRDY        BIT(3)

+ bits.h

...

> +#define VEML6046X00_INT_MASK        (VEML6046X00_INT_TH_H | \
> +				VEML6046X00_INT_TH_L | VEML6046X00_INT_DRDY)

Better formatting is

#define VEML6046X00_INT_MASK						     \
	(VEML6046X00_INT_TH_H | VEML6046X00_INT_TH_L | VEML6046X00_INT_DRDY)


> +/* Autosuspend delay */
> +#define VEML6046X00_AUTOSUSPEND_MS  3000

(3 * MSEC_PER_SEC)

(will require time.h to be included)

...

> +struct veml6046x00_data {
> +	struct device *dev;
> +	struct regmap *regmap;

Isn't dev the same as in regmap?
Why to have both?

> +	struct iio_trigger *trig;
> +	struct veml6046x00_rf rf;
> +};

...

> +struct veml6046x00_scan_buf {
> +	__le16 chans[4];
> +	aligned_s64 timestamp;
> +};

This is only used in one function, move it there like other drivers do.

...

> + * Columns:
> + * x0.25 x0.33 x0.5 x0.66 x1 x2
> + * Rows:
> + * 3.125 6.25 12.5 25 50 100 200 400ms

Better to have these as comments

...

> +static const u32 veml6046x00_it_gains[][6][2] = {

Also indentation is wrong.

> +{{5, 376000}, {4,  72700}, {2, 688000}, {2,  36400}, {1, 344000}, {0, 672000}},

	/* 3.125 */
	{
		{ 5, 376000 },		/* 0.25 */
		{ 4,  72700 },
		...
	},


> +{{2, 688000}, {2,  36350}, {1, 344000}, {1,  18200}, {0, 672000}, {0, 336000}},
> +{{1, 344000}, {1,  18175}, {0, 672000}, {0, 509100}, {0, 336000}, {0, 168000}},
> +{{0, 672000}, {0, 509087}, {0, 336000}, {0, 254550}, {0, 168000}, {0,  84000}},
> +{{0, 336000}, {0, 254543}, {0, 168000}, {0, 127275}, {0,  84000}, {0,  42000}},
> +{{0, 168000}, {0, 127271}, {0,  84000}, {0,  63637}, {0,  42000}, {0,  21000}},
> +{{0,  84000}, {0,  63635}, {0,  42000}, {0,  31818}, {0,  21000}, {0,  10500}},
> +{{0,  42000}, {0,  31817}, {0,  21000}, {0,  15909}, {0,  10500}, {0,   5250}},
> +};

Yes, it will be much longer, but much easier to read and check.

...

> +static int veml6046x00_power_on(struct veml6046x00_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF0,
> +						VEML6046X00_CONF0_ON_0);

Wrong indentation. Same applies to more places in the code.

> +	if (ret) {
> +		dev_err(data->dev, "Failed to set bit for power on %d\n", ret);
> +		return ret;
> +	}
> +
> +	return regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF1,
> +						VEML6046X00_CONF1_ON_1);
> +}

...

> +	/* register value is identical with index of array */
> +	if ((reg >= 0) && (reg < ARRAY_SIZE(veml6046x00_it)))
> +		return reg;
> +
> +	return -EINVAL;

Invert the conditional to check for errors.

...

> +static int veml6046x00_get_it_usec(struct veml6046x00_data *data, int *it_usec)
> +{
> +	int ret, reg;
> +
> +	ret = regmap_field_read(data->rf.it, &reg);
> +	if (ret)
> +		return ret;
> +
> +	if ((reg >= 0) && (reg < ARRAY_SIZE(veml6046x00_it)))
> +		*it_usec = veml6046x00_it[reg][1];

> +	else

Redundant. (See above how to fix)

> +		return -EINVAL;
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}

...

> +static int veml6046x00_set_it(struct iio_dev *iio, int val, int val2)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	int i;

Why signed?

> +	for (i = 0; i < ARRAY_SIZE(veml6046x00_it); i++) {
> +		if ((veml6046x00_it[i][0] == val) &&
> +		    (veml6046x00_it[i][1] == val2))
> +			return regmap_field_write(data->rf.it, i);
> +	}
> +
> +	return -EINVAL;
> +}
> +
> +static int veml6046x00_get_val_gain_idx(struct veml6046x00_data *data, int val,
> +								int val2)
> +{
> +	int i;

Ditto. And so on...

> +	int it_idx;
> +
> +	it_idx = veml6046x00_get_it_index(data);
> +	if (it_idx < 0)
> +		return it_idx;
> +
> +	for (i = 0; i < ARRAY_SIZE(veml6046x00_it_gains[it_idx]); i++) {
> +		if ((veml6046x00_it_gains[it_idx][i][0] == val) &&
> +		    (veml6046x00_it_gains[it_idx][i][1] == val2)) {
> +			return i;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}

...

> +static int veml6046x00_wait_data_available(struct iio_dev *iio, int usecs)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	int ret, reg;
> +	int cnt = 2;
> +	int i;
> +
> +	for (i = 0; i < cnt; i++) {
> +		ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, &reg);
> +		if (ret) {
> +			dev_err(data->dev,
> +					"Failed to read interrupt register %d\n", ret);
> +			return -EIO;
> +		}
> +
> +		if (reg & VEML6046X00_INT_DRDY)
> +			return 1;

> +		if (i < cnt)

Always true, no?

> +			fsleep(usecs);
> +	}
> +
> +	return 0;
> +}

...

> +	/* integration time + 10 % to ensure completion */
> +	fsleep(it_usec + (it_usec / 10));

Unneeded parentheses.

...

> +	ret = veml6046x00_wait_data_available(iio, it_usec * 10);
> +	if (ret == 1) {
> +		dev_dbg(data->dev, "data ready\n");

> +	} else {

Redundant 'else'.

> +		dev_warn(data->dev, "no data ready ret: %d\n", ret);
> +		goto no_data;
> +	}

...

> +static irqreturn_t veml6046x00_trig_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio = pf->indio_dev;
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	int ret;
> +	struct veml6046x00_scan_buf scan;
> +
> +	memset(&scan, 0, sizeof(scan));
> +
> +	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_R_L, &scan.chans,
> +						sizeof(scan.chans));

Wrong indentation.

> +	if (ret)
> +		goto done;
> +
> +	iio_push_to_buffers_with_timestamp(iio, &scan, iio_get_time_ns(iio));
> +
> +done:
> +	iio_trigger_notify_done(iio->trig);
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int veml6046x00_validate_part_id(struct veml6046x00_data *data)
> +{
> +	int part_id, ret;
> +	__le16 reg;
> +
> +	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_ID_L, &reg, sizeof(reg));

Remove _L from the definitions for the registers that you access with bulk IO.

> +	if (ret)
> +		return dev_err_probe(data->dev, ret, "Failed to read ID\n");
> +
> +	part_id = le16_to_cpu(reg);
> +	if (part_id != 0x0001)
> +		dev_info(data->dev, "Unknown ID %#02x\n", part_id);
> +
> +	return 0;
> +}

...

> +static int veml6046x00_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct veml6046x00_data *data;
> +	struct iio_dev *iio;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	regmap = devm_regmap_init_i2c(i2c, &veml6046x00_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "Failed to set regmap\n");

One line.

> +	iio = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	data = iio_priv(iio);
> +	i2c_set_clientdata(i2c, iio);
> +	data->dev = dev;
> +	data->regmap = regmap;
> +
> +	ret = veml6046x00_regfield_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to init regfield\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulator\n");
> +
> +	/* bring device in a known state and switch device on */
> +	ret = veml6046x00_setup_device(iio);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, veml6046x00_shutdown_action, data);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to add shut down action\n");
> +
> +	ret = pm_runtime_set_active(dev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to activate PM runtime\n");
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable PM runtime\n");
> +
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_autosuspend_delay(dev, VEML6046X00_AUTOSUSPEND_MS);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	ret = veml6046x00_validate_part_id(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to validate device ID\n");
> +
> +	iio->name = "veml6046x00";
> +	iio->channels = veml6046x00_channels;
> +	iio->num_channels = ARRAY_SIZE(veml6046x00_channels);
> +	iio->modes = INDIO_DIRECT_MODE;
> +
> +	iio->info = &veml6046x00_info_no_irq;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
> +					      veml6046x00_trig_handler,
> +					      &veml6046x00_buffer_setup_ops);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register triggered buffer");
> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register iio device");
> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Jonathan Cameron]

On Mon,  5 May 2025 22:23:12 +0200
Andreas Klinger <ak@it-klinger.de> wrote:

> Add Vishay VEML6046X00 high accuracy RGBIR color sensor.
> 
> This sensor provides three colour (red, green and blue) as well as one
> infrared (IR) channel through I2C.
> 
> Support direct and buffered mode.
> 
> An optional interrupt for signaling green colour threshold underflow or
> overflow is not supported so far.
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>

A few minor additional comments from me.

> +
> +/*
> + * regmap fields
> + */

Not sure that comment adds anything as pretty obvious what these are.

> +struct veml6046x00_rf {
> +	struct regmap_field *int_en;
> +	struct regmap_field *mode;
> +	struct regmap_field *trig;
> +	struct regmap_field *it;
> +	struct regmap_field *pers;
> +};
> +
> +struct veml6046x00_data {
> +	struct device *dev;
> +	struct regmap *regmap;
> +	struct iio_trigger *trig;
> +	struct veml6046x00_rf rf;
> +};
> +
> +struct veml6046x00_scan_buf {
> +	__le16 chans[4];
> +	aligned_s64 timestamp;
This structure is only used on one place. Maybe just define it there?
> +};
> +
> +/*
> + * Integration times
> + * Register value on veml6046x00 is identical with array index
> + * --> no separate table needed
> + */
> +static const int veml6046x00_it[][2] = {
> +	{ 0, 3125 },
> +	{ 0, 6250 },
> +	{ 0, 12500 },
> +	{ 0, 25000 },
> +	{ 0, 50000 },
> +	{ 0, 100000 },
> +	{ 0, 200000 },
> +	{ 0, 400000 },
> +};
> +
> +/*
> + * Gains here in the driver are not exactly the same as in the datasheet of the
> + * sensor. The gain in the driver is a combination of the gain of the sensor
> + * with the photodiode size (PD).
> + * The following combinations are possible:
> + *   gain(driver) = gain(sensor) * PD
> + *           0.25 = x0.5  * 1/2
> + *           0.33 = x0.66 * 1/2
> + *           0.5  = x0.5  * 2/2
> + *           0.66 = x0.66 * 2/2
> + *           1    = x1    * 2/2
> + *           2    = x2    * 2/2
> + */
> +
> +/*
> + * veml6046x00_gain_pd - translation from gain index (used in the driver) to
> + * gain (sensor) and PD
> + * @gain_sen:	Gain used in the sensor as described in the datasheet of the
> + *		sensor
> + * @pd:		Photodiode size in the sensor
> + */
> +struct veml6046x00_gain_pd {
> +	int gain_sen;
> +	int pd;
> +};
> +
> +#define VEML6046X00_GAIN_PD(_gain_sen, _pd)				\
> +{									\
> +	.gain_sen = (_gain_sen),					\
> +	.pd = (_pd),							\
> +}
> +
> +static const struct veml6046x00_gain_pd veml6046x00_gain_pd[] = {
> +	VEML6046X00_GAIN_PD(VEML6046X00_GAIN_0_5, VEML6046X00_PD_1_2),
	{ .gain_sel = VEML6046X00_GAIN_0_5, .pd = VEML6046X00_PD_1_2 },
is probably fine - that is I'm not sure the macro helps much over
just doing 
	{ VEML6046X00_GAIN_0_5, VEML6046X00_PD_1_2 },
etc but laying out which parameter is which as above is nice to have.

> +	VEML6046X00_GAIN_PD(VEML6046X00_GAIN_0_66, VEML6046X00_PD_1_2),
> +	VEML6046X00_GAIN_PD(VEML6046X00_GAIN_0_5, VEML6046X00_PD_2_2),
> +	VEML6046X00_GAIN_PD(VEML6046X00_GAIN_0_66, VEML6046X00_PD_2_2),
> +	VEML6046X00_GAIN_PD(VEML6046X00_GAIN_1, VEML6046X00_PD_2_2),
> +	VEML6046X00_GAIN_PD(VEML6046X00_GAIN_2, VEML6046X00_PD_2_2),
> +};
> +
> +/*
> + * Factors for lux / raw count in dependency of integration time (IT) as rows
> + * and driver gain in columns
> + * Columns:
> + * x0.25 x0.33 x0.5 x0.66 x1 x2
> + * Rows:
> + * 3.125 6.25 12.5 25 50 100 200 400ms
> + */
> +static const u32 veml6046x00_it_gains[][6][2] = {
> +{{5, 376000}, {4,  72700}, {2, 688000}, {2,  36400}, {1, 344000}, {0, 672000}},
> +{{2, 688000}, {2,  36350}, {1, 344000}, {1,  18200}, {0, 672000}, {0, 336000}},
> +{{1, 344000}, {1,  18175}, {0, 672000}, {0, 509100}, {0, 336000}, {0, 168000}},
> +{{0, 672000}, {0, 509087}, {0, 336000}, {0, 254550}, {0, 168000}, {0,  84000}},
> +{{0, 336000}, {0, 254543}, {0, 168000}, {0, 127275}, {0,  84000}, {0,  42000}},
> +{{0, 168000}, {0, 127271}, {0,  84000}, {0,  63637}, {0,  42000}, {0,  21000}},
> +{{0,  84000}, {0,  63635}, {0,  42000}, {0,  31818}, {0,  21000}, {0,  10500}},
> +{{0,  42000}, {0,  31817}, {0,  21000}, {0,  15909}, {0,  10500}, {0,   5250}},
I'd prefer
   { { 0,  42000 }, { 0,   31817 }, etc for this formatting.
Don't worry about going a little over 80 chars to do so - I think the readability
makes it worth while.


> +};

> +static int veml6046x00_get_gain_idx(struct veml6046x00_data *data)
> +{
> +	int ret, reg;
> +	int i;
> +	int reg_gain;
> +	int reg_pd;
> +

I'd combine a few more of these on one line.  Either all as one line
or i on the line with ret then the two reg* together.


> +static int veml6046x00_single_read(struct iio_dev *iio,
> +					enum iio_modifier modifier, int *val)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	int addr, it_usec, ret;
> +	__le16 reg;
> +
> +	switch (modifier) {
> +	case IIO_MOD_LIGHT_RED:
> +		addr = VEML6046X00_REG_R_L;
> +		break;
> +	case IIO_MOD_LIGHT_GREEN:
> +		addr = VEML6046X00_REG_G_L;
> +		break;
> +	case IIO_MOD_LIGHT_BLUE:
> +		addr = VEML6046X00_REG_B_L;
> +		break;
> +	case IIO_MOD_LIGHT_IR:
> +		addr = VEML6046X00_REG_IR_L;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = veml6046x00_get_it_usec(data, &it_usec);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = regmap_field_write(data->rf.mode, 1);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_field_write(data->rf.trig, 1);
> +	if (ret)
> +		return ret;
> +
> +	/* integration time + 10 % to ensure completion */
> +	fsleep(it_usec + (it_usec / 10));
> +
> +	ret = veml6046x00_wait_data_available(iio, it_usec * 10);
> +	if (ret == 1) {
> +		dev_dbg(data->dev, "data ready\n");
> +	} else {
> +		dev_warn(data->dev, "no data ready ret: %d\n", ret);
> +		goto no_data;
> +	}
> +
> +	if (!iio_device_claim_direct(iio))
> +		return -EBUSY;
> +
> +	ret = regmap_bulk_read(data->regmap, addr, &reg, sizeof(reg));
> +	iio_device_release_direct(iio);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_mark_last_busy(data->dev);
> +	pm_runtime_put_autosuspend(data->dev);
> +
> +	*val = le16_to_cpu(reg);
> +
> +	return IIO_VAL_INT;
> +
> +no_data:
> +	pm_runtime_mark_last_busy(data->dev);
> +	pm_runtime_put_autosuspend(data->dev);
> +
> +	return -EINVAL;

Why -EINVAL for not yet?  EAGAIN maybe if expectation is it will succeed later.

> +}



> +static int veml6046x00_setup_device(struct iio_dev *iio)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	struct device *dev = data->dev;
> +	int ret, val;
> +	__le16 reg16;
> +	uint8_t reg[2];

kernel types preferred.  Doesn't really matter but they are more common in IIO.

> +
> +	reg[0] = VEML6046X00_CONF0_AF;
> +	reg[1] = 0x00;
Given you are respinning anyway, might as well do
	u8 reg[2] = { VEMDL6046X00_CONF0_AF, 0x00 };

and save a few lines.

> +	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_CONF0, reg, sizeof(reg));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set configuration\n");
> +
> +	reg16 = cpu_to_le16(0);
> +	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_THDL_L, &reg16, sizeof(reg16));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set low threshold\n");
> +
> +	reg16 = cpu_to_le16(U16_MAX);
> +	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_THDH_L, &reg16, sizeof(reg16));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set high threshold\n");
> +
> +	ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, &val);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to clear interrupts\n");
> +
> +	return 0;
> +}

Re: [PATCH v3 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Andreas Klinger]

[-- Attachment #1: Type: text/plain, Size: 1431 bytes --]

Hi Jonathan,

thanks for the review.

Jonathan Cameron <jic23@kernel.org> schrieb am Do, 15. Mai 17:37:
> > +/*
> > + * Factors for lux / raw count in dependency of integration time (IT) as rows
> > + * and driver gain in columns
> > + * Columns:
> > + * x0.25 x0.33 x0.5 x0.66 x1 x2
> > + * Rows:
> > + * 3.125 6.25 12.5 25 50 100 200 400ms
> > + */
> > +static const u32 veml6046x00_it_gains[][6][2] = {
> > +{{5, 376000}, {4,  72700}, {2, 688000}, {2,  36400}, {1, 344000}, {0, 672000}},
> > +{{2, 688000}, {2,  36350}, {1, 344000}, {1,  18200}, {0, 672000}, {0, 336000}},
> > +{{1, 344000}, {1,  18175}, {0, 672000}, {0, 509100}, {0, 336000}, {0, 168000}},
> > +{{0, 672000}, {0, 509087}, {0, 336000}, {0, 254550}, {0, 168000}, {0,  84000}},
> > +{{0, 336000}, {0, 254543}, {0, 168000}, {0, 127275}, {0,  84000}, {0,  42000}},
> > +{{0, 168000}, {0, 127271}, {0,  84000}, {0,  63637}, {0,  42000}, {0,  21000}},
> > +{{0,  84000}, {0,  63635}, {0,  42000}, {0,  31818}, {0,  21000}, {0,  10500}},
> > +{{0,  42000}, {0,  31817}, {0,  21000}, {0,  15909}, {0,  10500}, {0,   5250}},
> I'd prefer
>    { { 0,  42000 }, { 0,   31817 }, etc for this formatting.
> Don't worry about going a little over 80 chars to do so - I think the readability
> makes it worth while.

I tried it and ended up with 101 characters in a line. I'll pick up the
suggestion of Andy for the next version.

Andreas

-- 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v3 3/3] MAINTAINER: add maintainer for veml6046x00

[Andreas Klinger]

Add maintainer for Vishay veml6046x00 RGBIR color sensor driver and dt
binding.

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 69511c3b2b76..297c01e2357f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25800,6 +25800,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
 F:	drivers/iio/light/veml6030.c
 
+VISHAY VEML6046X00 RGBIR COLOR SENSOR DRIVER
+M:	Andreas Klinger <ak@it-klinger.de>
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
+F:	drivers/iio/light/veml6046x00.c
+
 VISHAY VEML6075 UVA AND UVB LIGHT SENSOR DRIVER
 M:	Javier Carrasco <javier.carrasco.cruz@gmail.com>
 S:	Maintained
-- 
2.39.5