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

[PATCH v4 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 v4:
- implement feedback from Andy and Jonathan
- implement feedback from vendor (reading interrupt register as bulk read)

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               | 953 ++++++++++++++++++
 5 files changed, 1024 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
 create mode 100644 drivers/iio/light/veml6046x00.c

-- 
2.39.5

[PATCH v4 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 v4 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 | 953 ++++++++++++++++++++++++++++++++
 3 files changed, 967 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..d45fda49692d
--- /dev/null
+++ b/drivers/iio/light/veml6046x00.c
@@ -0,0 +1,953 @@
+// 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/bits.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/time.h>
+#include <linux/types.h>
+#include <linux/units.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
+ * Those which are accessed as bulk io are omitted
+ */
+#define VEML6046X00_REG_CONF0       0x00
+#define VEML6046X00_REG_CONF1       0x01
+#define VEML6046X00_REG_THDH        0x04
+#define VEML6046X00_REG_THDL        0x06
+#define VEML6046X00_REG_R           0x10
+#define VEML6046X00_REG_G           0x12
+#define VEML6046X00_REG_B           0x14
+#define VEML6046X00_REG_IR          0x16
+#define VEML6046X00_REG_ID          0x18
+#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  (3 * MSEC_PER_SEC)
+
+enum veml6046x00_scan {
+	VEML6046X00_SCAN_R,
+	VEML6046X00_SCAN_G,
+	VEML6046X00_SCAN_B,
+	VEML6046X00_SCAN_IR,
+	VEML6046X00_SCAN_TIMESTAMP,
+};
+
+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 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[] = {
+	{.gain_sen = VEML6046X00_GAIN_0_5, .pd = VEML6046X00_PD_1_2},
+	{.gain_sen = VEML6046X00_GAIN_0_66, .pd = VEML6046X00_PD_1_2},
+	{.gain_sen = VEML6046X00_GAIN_0_5, .pd = VEML6046X00_PD_2_2},
+	{.gain_sen = VEML6046X00_GAIN_0_66, .pd = VEML6046X00_PD_2_2},
+	{.gain_sen = VEML6046X00_GAIN_1, .pd = VEML6046X00_PD_2_2},
+	{.gain_sen = VEML6046X00_GAIN_2, .pd = VEML6046X00_PD_2_2},
+};
+
+/*
+ * Factors for lux / raw count in dependency of integration time (IT) as rows
+ * and driver gain in columns
+ */
+static const u32 veml6046x00_it_gains[][6][2] = {
+	/* integration time: 3.125 ms */
+	{
+		{ 5, 376000 },	/* gain: x0.25 */
+		{ 4,  72700 },	/* gain: x0.33 */
+		{ 2, 688000 },	/* gain: x0.5 */
+		{ 2,  36400 },	/* gain: x0.66 */
+		{ 1, 344000 },	/* gain: x1 */
+		{ 0, 672000 }	/* gain: x2 */
+	},
+	/* integration time: 6.25 ms */
+	{
+		{ 2, 688000 },	/* gain: x0.25 */
+		{ 2,  36350 },	/* gain: x0.33 */
+		{ 1, 344000 },	/* gain: x0.5 */
+		{ 1,  18200 },	/* gain: x0.66 */
+		{ 0, 672000 },	/* gain: x1 */
+		{ 0, 336000 }	/* gain: x2 */
+	},
+	/* integration time: 12.5 ms */
+	{
+		{ 1, 344000 },	/* gain: x0.25 */
+		{ 1,  18175 },	/* gain: x0.33 */
+		{ 0, 672000 },	/* gain: x0.5 */
+		{ 0, 509100 },	/* gain: x0.66 */
+		{ 0, 336000 },	/* gain: x1 */
+		{ 0, 168000 }	/* gain: x2 */
+	},
+	/* integration time: 25 ms */
+	{
+		{ 0, 672000 },	/* gain: x0.25 */
+		{ 0, 509087 },	/* gain: x0.33 */
+		{ 0, 336000 },	/* gain: x0.5 */
+		{ 0, 254550 },	/* gain: x0.66 */
+		{ 0, 168000 },	/* gain: x1 */
+		{ 0,  84000 }	/* gain: x2 */
+	},
+	/* integration time: 50 ms */
+	{
+		{ 0, 336000 },	/* gain: x0.25 */
+		{ 0, 254543 },	/* gain: x0.33 */
+		{ 0, 168000 },	/* gain: x0.5 */
+		{ 0, 127275 },	/* gain: x0.66 */
+		{ 0,  84000 },	/* gain: x1 */
+		{ 0,  42000 }	/* gain: x2 */
+	},
+	/* integration time: 100 ms */
+	{
+		{ 0, 168000 },	/* gain: x0.25 */
+		{ 0, 127271 },	/* gain: x0.33 */
+		{ 0,  84000 },	/* gain: x0.5 */
+		{ 0,  63637 },	/* gain: x0.66 */
+		{ 0,  42000 },	/* gain: x1 */
+		{ 0,  21000 }	/* gain: x2 */
+	},
+	/* integration time: 200 ms */
+	{
+		{ 0,  84000 },	/* gain: x0.25 */
+		{ 0,  63635 },	/* gain: x0.33 */
+		{ 0,  42000 },	/* gain: x0.5 */
+		{ 0,  31818 },	/* gain: x0.66 */
+		{ 0,  21000 },	/* gain: x1 */
+		{ 0,  10500 }	/* gain: x2 */
+	},
+	/* integration time: 400 ms */
+	{
+		{ 0,  42000 },	/* gain: x0.25 */
+		{ 0,  31817 },	/* gain: x0.33 */
+		{ 0,  21000 },	/* gain: x0.5 */
+		{ 0,  15909 },	/* gain: x0.66 */
+		{ 0,  10500 },	/* gain: x1 */
+		{ 0,   5250 }	/* gain: x2 */
+	},
+};
+
+/*
+ * 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;
+	struct device *dev = regmap_get_device(data->regmap);
+
+	ret = regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF0,
+							VEML6046X00_CONF0_ON_0);
+	if (ret) {
+		dev_err(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;
+	struct device *dev = regmap_get_device(data->regmap);
+
+	ret = regmap_set_bits(data->regmap, VEML6046X00_REG_CONF0,
+							VEML6046X00_CONF0_ON_0);
+	if (ret) {
+		dev_err(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,
+		.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,
+		.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,
+		.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,
+		.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_regm",
+	.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 = regmap_get_device(data->regmap);
+	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;
+	int 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 -EINVAL;
+
+	return reg;
+}
+
+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)))
+		return -EINVAL;
+
+	*it_usec = veml6046x00_it[reg][1];
+
+	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);
+	u32 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)
+{
+	u32 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, reg_gain, reg_pd;
+	u32 i;
+
+	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);
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret, i, cnt = 2;
+	u8 reg[2];
+
+	for (i = 0; i < cnt; i++) {
+		/*
+		 * Note from the vendor, but not explicitly in the datasheet: we
+		 * should always read both registers together
+		 */
+		ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_INT_L,
+							&reg, sizeof(reg));
+		if (ret) {
+			dev_err(dev,
+				"Failed to read interrupt register %d\n", ret);
+			return -EIO;
+		}
+
+		if (reg[1] & VEML6046X00_INT_DRDY)
+			return 1;
+
+		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);
+	struct device *dev = regmap_get_device(data->regmap);
+	int addr, it_usec, ret;
+	__le16 reg;
+
+	switch (modifier) {
+	case IIO_MOD_LIGHT_RED:
+		addr = VEML6046X00_REG_R;
+		break;
+	case IIO_MOD_LIGHT_GREEN:
+		addr = VEML6046X00_REG_G;
+		break;
+	case IIO_MOD_LIGHT_BLUE:
+		addr = VEML6046X00_REG_B;
+		break;
+	case IIO_MOD_LIGHT_IR:
+		addr = VEML6046X00_REG_IR;
+		break;
+	default:
+		return -EINVAL;
+	}
+	ret = pm_runtime_resume_and_get(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)
+		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(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	*val = le16_to_cpu(reg);
+
+	return IIO_VAL_INT;
+
+no_data:
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return -EAGAIN;
+}
+
+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 = regmap_get_device(data->regmap);
+	int ret;
+
+	ret = regmap_field_write(data->rf.mode, 0);
+	if (ret) {
+		dev_err(dev, "Failed to set mode %d\n", ret);
+		return ret;
+	}
+
+	ret = regmap_field_write(data->rf.trig, 0);
+	if (ret) {
+		dev_err(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 = regmap_get_device(data->regmap);
+	int ret;
+
+	ret = regmap_field_write(data->rf.mode, 1);
+	if (ret) {
+		dev_err(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 {
+		__le16 chans[4];
+		aligned_s64 timestamp;
+	} scan;
+
+	memset(&scan, 0, sizeof(scan));
+
+	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_R,
+					&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)
+{
+	struct device *dev = regmap_get_device(data->regmap);
+	int part_id, ret;
+	__le16 reg;
+
+	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_ID,
+							&reg, sizeof(reg));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read ID\n");
+
+	part_id = le16_to_cpu(reg);
+	if (part_id != 0x0001)
+		dev_info(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 = regmap_get_device(data->regmap);
+	int ret;
+	__le16 reg16;
+	u8 reg[2] = { VEML6046X00_CONF0_AF, 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,
+							&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,
+							&reg16, sizeof(reg16));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set high threshold\n");
+
+	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_INT_L,
+							&reg16, sizeof(reg16));
+	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->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

[PATCH v4 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 f21f1dabb5fe..872273ee6bf6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25859,6 +25859,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

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

[Andy Shevchenko]

On Mon, May 19, 2025 at 08:08:03AM +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/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/time.h>
> +#include <linux/types.h>
> +#include <linux/units.h>

...

> +/*
> + * 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

This is made to look like kernel-doc, but it's not marked as a such, why?

> + */
> +struct veml6046x00_gain_pd {
> +	int gain_sen;
> +	int pd;
> +};

...

> +/*
> + * Factors for lux / raw count in dependency of integration time (IT) as rows
> + * and driver gain in columns

Missing period at the end. Please, fix all your multi-line comments
accordingly.

> + */

...

> +	ret = regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF0,
> +							VEML6046X00_CONF0_ON_0);

Something wrong with the indentation. Please, fix all places like this...

> +	if (ret) {
> +		dev_err(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);

...or like this.

> +}

...

> +static int veml6046x00_get_it_index(struct veml6046x00_data *data)
> +{
> +	int ret;
> +	int reg;

Why the 'reg' is signed? regmap API doesn't operate on signed values. Please
fix all places in your code.

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

in_range() ?

> +		return -EINVAL;
> +
> +	return reg;
> +}

...

> +static int veml6046x00_get_it_usec(struct veml6046x00_data *data, int *it_usec)

Same comments as per above function.

...

> +static int veml6046x00_get_val_gain_idx(struct veml6046x00_data *data, int val,
> +								int val2)
> +{
> +	u32 i;

Why fixed-width type? Wouldn't unsigned int i work?
Please, fix in all places. The rule of thumb is to use fixed-width types either
when it's HW / protocol specific, or when the respective API uses the same type.
Otherwise use PODs.

> +	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);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret, i, cnt = 2;
> +	u8 reg[2];
> +
> +	for (i = 0; i < cnt; i++) {
> +		/*
> +		 * Note from the vendor, but not explicitly in the datasheet: we
> +		 * should always read both registers together
> +		 */
> +		ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_INT_L,

Please, drop _L if not used as a single byte access.

> +							&reg, sizeof(reg));
> +		if (ret) {
> +			dev_err(dev,
> +				"Failed to read interrupt register %d\n", ret);
> +			return -EIO;
> +		}
> +
> +		if (reg[1] & VEML6046X00_INT_DRDY)
> +			return 1;
> +
> +		fsleep(usecs);
> +	}
> +
> +	return 0;
> +}

...

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

I would suggest  / 8 as it gives much better code generation. Divisions are
slow and hard.

> +	ret = veml6046x00_wait_data_available(iio, it_usec * 10);

Also it won't mess with semantics of '10' here.

> +	if (ret != 1)

Can it return negative error? If not, why is error code shadowed?

> +		goto no_data;

...

> +static int veml6046x00_validate_part_id(struct veml6046x00_data *data)
> +{
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int part_id, ret;
> +	__le16 reg;
> +
> +	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_ID,
> +							&reg, sizeof(reg));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to read ID\n");
> +
> +	part_id = le16_to_cpu(reg);
> +	if (part_id != 0x0001)

Here you put 4 digits...

> +		dev_info(dev, "Unknown ID %#02x\n", part_id);

...and here you are expecting that it may be two only. Please, make these two
consistent.

> +
> +	return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

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

[Andreas Klinger]

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

Hi Andy,

thanks for the review. I have a question and a comment below.

Andy Shevchenko <andriy.shevchenko@linux.intel.com> schrieb am Mo, 19. Mai 14:04:
> > +/*
> > + * 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
> 
> This is made to look like kernel-doc, but it's not marked as a such, why?

I'll remove the '@'

...

> > +	ret = regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF0,
> > +							VEML6046X00_CONF0_ON_0);
> 
> Something wrong with the indentation. Please, fix all places like this...
> 
> > +	if (ret) {
> > +		dev_err(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);
> 
> ...or like this.
> 
> > +}

I don't get the point what is wrong with the indentation. In the coding-style it
says the decendant line should be placed to the right.
Did i miss something?

Best regards,

Andreas


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

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

[Jonathan Cameron]

On Sun, 25 May 2025 09:34:52 +0200
Andreas Klinger <ak@it-klinger.de> wrote:

> Hi Andy,
> 
> thanks for the review. I have a question and a comment below.
> 
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> schrieb am Mo, 19. Mai 14:04:
> > > +/*
> > > + * 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  
> > 
> > This is made to look like kernel-doc, but it's not marked as a such, why?  
> 
> I'll remove the '@'

Better to make it kernel doc!  That is add /** and check for
errors using scripts/kernel-doc 


> 
> ...
> 
> > > +	ret = regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF0,
> > > +							VEML6046X00_CONF0_ON_0);  
> > 
> > Something wrong with the indentation. Please, fix all places like this...
> >   
> > > +	if (ret) {
> > > +		dev_err(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);  
> > 
> > ...or like this.
> >   
> > > +}  
> 
> I don't get the point what is wrong with the indentation. In the coding-style it
> says the decendant line should be placed to the right.
> Did i miss something?

Where there are no other constraints it should be aligned under the first parameter
on the line above.

	return regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF1,
				 VEML6046X00_CONF1_ON_1);  

This is one of those things that has never been added explicitly to the coding style
doc because there lots of subtle corner cases where it isn't appropriate.
But all significantly to the right usually means is 1 tab or more.
Where possible most people prefer the above style.

Jonathan

> 
> Best regards,
> 
> Andreas
> 

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

[Jonathan Cameron]

On Mon, 19 May 2025 08:08:03 +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>
Hi Andreas

A few little additions from me.

The one about iio_push_to_buffers_with_ts() is entirely up to you.
When I introduce new features like that I tend to give a window before insisting
on them being done by driver authors. It's unfair to insist when they crossed
with your code and the extra cost of one more driver on a big conversion is
trivial.

Jonathan


> diff --git a/drivers/iio/light/veml6046x00.c b/drivers/iio/light/veml6046x00.c
> new file mode 100644
> index 000000000000..d45fda49692d
> --- /dev/null
> +++ b/drivers/iio/light/veml6046x00.c

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

You moved the definition inline but forgot to delete this one I think.
So drop it.


> +/*
Might as well be kernel-doc

/**

> + * 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),							\
> +}

Macro not used any more so drop it.

> +
> +static const struct veml6046x00_gain_pd veml6046x00_gain_pd[] = {
> +	{.gain_sen = VEML6046X00_GAIN_0_5, .pd = VEML6046X00_PD_1_2},
{ .gain 

So add spaces after { and before }
> +	{.gain_sen = VEML6046X00_GAIN_0_66, .pd = VEML6046X00_PD_1_2},
> +	{.gain_sen = VEML6046X00_GAIN_0_5, .pd = VEML6046X00_PD_2_2},
> +	{.gain_sen = VEML6046X00_GAIN_0_66, .pd = VEML6046X00_PD_2_2},
> +	{.gain_sen = VEML6046X00_GAIN_1, .pd = VEML6046X00_PD_2_2},
> +	{.gain_sen = VEML6046X00_GAIN_2, .pd = VEML6046X00_PD_2_2},
> +};
> +
> +/*
> + * Factors for lux / raw count in dependency of integration time (IT) as rows
> + * and driver gain in columns
> + */
> +static const u32 veml6046x00_it_gains[][6][2] = {
> +	/* integration time: 3.125 ms */

> +	/* integration time: 400 ms */
> +	{
> +		{ 0,  42000 },	/* gain: x0.25 */
> +		{ 0,  31817 },	/* gain: x0.33 */
> +		{ 0,  21000 },	/* gain: x0.5 */
> +		{ 0,  15909 },	/* gain: x0.66 */
> +		{ 0,  10500 },	/* gain: x1 */
> +		{ 0,   5250 }	/* gain: x2 */

Convention is trailing comma for anything that isn't an explicit
terminator.  That tends to apply even when w know there will never
be more elements like here.  Same for earlier sub arrays.

> +	},
> +};

> +
> +static int veml6046x00_single_read(struct iio_dev *iio,
> +					enum iio_modifier modifier, int *val)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int addr, it_usec, ret;
> +	__le16 reg;
> +
> +	switch (modifier) {
> +	case IIO_MOD_LIGHT_RED:
> +		addr = VEML6046X00_REG_R;
> +		break;
> +	case IIO_MOD_LIGHT_GREEN:
> +		addr = VEML6046X00_REG_G;
> +		break;
> +	case IIO_MOD_LIGHT_BLUE:
> +		addr = VEML6046X00_REG_B;
> +		break;
> +	case IIO_MOD_LIGHT_IR:
> +		addr = VEML6046X00_REG_IR;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = veml6046x00_get_it_usec(data, &it_usec);
> +	if (ret < 0)
> +		return ret;

Why is suspending not appropriate if this fails?
Sure comms may well be dead, but maybe we should try?
If not a comment is needed on why not.

> +
> +	ret = regmap_field_write(data->rf.mode, 1);
> +	if (ret)
> +		return ret;

As above.

> +
> +	ret = regmap_field_write(data->rf.trig, 1);
> +	if (ret)
> +		return ret;

As above.

> +
> +	/* integration time + 10 % to ensure completion */
> +	fsleep(it_usec + it_usec / 10);
> +
> +	ret = veml6046x00_wait_data_available(iio, it_usec * 10);
> +	if (ret != 1)
> +		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(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	*val = le16_to_cpu(reg);
> +
> +	return IIO_VAL_INT;
> +
> +no_data:
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	return -EAGAIN;
> +}

> +
> +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 {
> +		__le16 chans[4];
> +		aligned_s64 timestamp;
> +	} scan;
> +
> +	memset(&scan, 0, sizeof(scan));

Can use
	} scan = {};

as a shorter way to ensure it's zeroed.
The scattering of memset date back to me not being sure if {} zero's
holes. The C spec recent versions say that it does, and the kernel build options
should ensure it does.


> +
> +	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_R,
> +					&scan.chans, sizeof(scan.chans));
> +	if (ret)
> +		goto done;
> +
> +	iio_push_to_buffers_with_timestamp(iio, &scan, iio_get_time_ns(iio));

I've recently merged 
iio_push_buffers_with_ts() that in addition takes sizeof(scan) as a parameter
and allows the core code to run a sanity check that we haven't gotten the
size wrong. As it looks like you are respinning anyway, please consider
using that. I won't insist on it as it is very new and there are lots
of drivers to convert anyway so one more doesn't matter much!

> +
> +done:
> +	iio_trigger_notify_done(iio->trig);
> +
> +	return IRQ_HANDLED;
> +}