[PATCH v4 0/4] iio: light: add support for veml6031x00 ALS series

[PATCH v4 0/4] iio: light: add support for veml6031x00 ALS series

[Javier Carrasco]

These ambient light sensors with I2C interface provide two light
channels (ALS and IR), high/low threshold alarms with configurable
persistence, and a data ready signal.

The devices covered by this driver have the same resolution, and they
share most of their functionality. These are the differences between
them (note that the x belongs to their names, and it is not a wildcard):

 - Device ID: accessible via two 8-byte registers, different values for
   veml6031x00/veml6031x01 and veml60311x00/veml60311x01.
 - I2C address: same grouping, 0x29 and 0x10 I2C addresses.
 - AEC qualification: AEC-Q100 for veml6031x00/veml60311x00 and
   AEC-Q101 for veml6031x01/veml60311x01.

The alarms and the data ready signals share the interrupt pin, and an
interrupt status register must be accessed to identify the source. Such
multiplexing is not new in IIO, and I have followed existing examples
for it. The persistence setting (own attribute) to trigger the alarms
uses the pattern that has already been used for the veml6030.

The device configuration is in general documented in the datasheet and
the application note. There is an exception, though: the activation of
the "active force" mode that is required for the data ready signal must
be carried out in two steps even though the affected bits are located in
the same register: first ALS_AF (active force mode enable) must be set,
and then ALS_TRIG (active force trigger setting) must be enabled. I have
added a brief commentary in the code to explain this behavior, which has
been confirmed by the manufacturer.

The only functionality that has not been implemented yet is the x0.66
gain (and its x0.165 counterpart when PD_DIV=1), which makes the gts
helpers less usable due to the conversions required. It is indeed an
uncommon gain to use (there are x0.5 and x0.125 gains) with no known
use-case at the moment that justifies making adjustments to the gts
helpers or adding artificial conversions to make it work.

This driver has been tested with the four supported devices separately
as well as in pairs where the I2C addresses don't overlap.

To: Jonathan Cameron <jic23@kernel.org>
To: Lars-Peter Clausen <lars@metafoo.de>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Rishi Gupta <gupt21@gmail.com>
To: David Lechner <dlechner@baylibre.com>
To: Nuno Sá <nuno.sa@analog.com>
To: Andy Shevchenko <andy@kernel.org>
To: Matti Vaittinen <mazziesaccount@gmail.com>
Cc: linux-iio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

Changes in v4:
- [1/4] Entry in alphabetical order for MAINTAINERS.
- [2/4] Fix style for device ID tables.
- [2/4] Add missing header mod_devicetable.h and move iio/sysfs.h to
  [4/4] for the persistence attribute.
- [2/4] Move IIO_DEV_ACQUIRE_DIRECT_MODE to [3/4] where buffers are
  added.
- [3/4] Use test_bit() to check the active scan mask.
- [2/4] Add mutex for operations on scattered register fields.
- [2/4] Add shutdown action after turning the device on.
- Link to v3: https://lore.kernel.org/r/20260524-veml6031x00-v3-0-29165609b2b5@gmail.com

Changes in v3:
- Move veml6030 fixes to a separate patch stack.
- Use C99 initializers for i2c_device_id.
- Split driver code into multiple patches to ease its review.
- Rework locking to get rid of atomic increment/decrement ops.
- Fix error paths in pm_runtime operations.
- Use IIO_DEV_ACQUIRE_DIRECT_MODE for single read/write ops.
- Link to v2: https://lore.kernel.org/r/20260513-veml6031x00-v2-0-4703ca661a1d@gmail.com

Changes in v2:
- Add commit to fix bug in veml6030.c (channel type when pushing
  events) and remove dead code.
- Use gts helpers to simplify operations.
- Drop unused gain_idx.
- Build INT_MASK as an OR operation of the involved bits.
- Format arrays to follow the desired standard for IIO.
- Directly return function result as the last operation within another
  function instead of 'ret = x; if (ret) return ret; return 0;'.
- Fix some spacing (double space, tab for alignemnt in info struct).
- Use sizeof() for __le16 reg instead of 2.
- Return an error if the part ID could not be read.
- Spell out sd -> shutdown.
- Use devm_mutex_init() instead of mutex_init().
- Avoid using conditional guard, use claim/release instead.
- Access integration times from the global array to get and set the
  integration time instead of using a switch.
- Simplify read of available periods (persistence).
- Drop IRQF_TRIGGER_FALLING in the threaded irq request.
- Add regmap ranges.
- Link to v1: https://lore.kernel.org/r/20241126-veml6031x00-v1-0-4affa62bfefd@gmail.com

---
Javier Carrasco (4):
      dt-bindings: iio: light: veml6030: add veml6031x00 ALS series
      iio: light: add support for veml6031x00 ALS series
      iio: light: veml6031x00: add support for triggered buffers
      iio: light: veml6031x00: add support for events and trigger

 .../bindings/iio/light/vishay,veml6030.yaml        |   23 +-
 MAINTAINERS                                        |    6 +
 drivers/iio/light/Kconfig                          |   14 +
 drivers/iio/light/Makefile                         |    1 +
 drivers/iio/light/veml6031x00.c                    | 1228 ++++++++++++++++++++
 5 files changed, 1271 insertions(+), 1 deletion(-)
---
base-commit: 254f49634ee16a731174d2ae34bc50bd5f45e731
change-id: 20241109-veml6031x00-aa9463da064a

Best regards,
-- 
Javier Carrasco <javier.carrasco.cruz@gmail.com>

[PATCH v4 1/4] dt-bindings: iio: light: veml6030: add veml6031x00 ALS series

[Javier Carrasco]

These ambient light sensors share their properties with the ones
from the same manufacturer that are supported by this bindings.

Note that only two datasheets are provided as every one of them covers
two devices (veml6031x00/veml60311x00 and veml6031x01/veml60311x01).

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 .../bindings/iio/light/vishay,veml6030.yaml        | 23 +++++++++++++++++++++-
 MAINTAINERS                                        |  5 +++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml b/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
index 4ea69f1fdd63..e01e8747e47c 100644
--- a/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
+++ b/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
@@ -4,7 +4,9 @@
 $id: http://devicetree.org/schemas/iio/light/vishay,veml6030.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: VEML3235, VEML6030, VEML6035 and VEML7700 Ambient Light Sensors (ALS)
+title:
+  VEML3235, VEML6030, VEML6031x00 series, VEML6035 and VEML7700 Ambient
+  Light Sensors (ALS)
 
 maintainers:
   - Rishi Gupta <gupt21@gmail.com>
@@ -22,12 +24,18 @@ description: |
   Specifications about the sensors can be found at:
     https://www.vishay.com/docs/80131/veml3235.pdf
     https://www.vishay.com/docs/84366/veml6030.pdf
+    https://www.vishay.com/docs/80007/veml6031x00.pdf
+    https://www.vishay.com/docs/80008/veml6031x01.pdf
     https://www.vishay.com/docs/84889/veml6035.pdf
     https://www.vishay.com/docs/84286/veml7700.pdf
 
 properties:
   compatible:
     enum:
+      - vishay,veml6031x00
+      - vishay,veml6031x01
+      - vishay,veml60311x00
+      - vishay,veml60311x01
       - vishay,veml3235
       - vishay,veml6030
       - vishay,veml6035
@@ -67,6 +75,8 @@ allOf:
       properties:
         compatible:
           enum:
+            - vishay,veml6031x00
+            - vishay,veml6031x01
             - vishay,veml6035
     then:
       properties:
@@ -79,12 +89,23 @@ allOf:
         compatible:
           enum:
             - vishay,veml3235
+            - vishay,veml60311x00
+            - vishay,veml60311x01
             - vishay,veml7700
     then:
       properties:
         reg:
           enum:
             - 0x10
+
+  - if:
+      properties:
+        compatible:
+          enum:
+            - vishay,veml3235
+            - vishay,veml7700
+    then:
+      properties:
         interrupts: false
 
 additionalProperties: false
diff --git a/MAINTAINERS b/MAINTAINERS
index 2fb1c75afd16..921da7584963 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -28375,6 +28375,11 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
 F:	drivers/iio/light/veml6030.c
 
+VISHAY VEML6031X00 AMBIENT LIGHT SENSOR DRIVER
+M:	Javier Carrasco <javier.carrasco.cruz@gmail.com>
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
+
 VISHAY VEML6046X00 RGBIR COLOR SENSOR DRIVER
 M:	Andreas Klinger <ak@it-klinger.de>
 S:	Maintained

-- 
2.43.0

[PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Javier Carrasco]

These sensors provide two light channels (ALS and IR), I2C communication
and a multiplexed interrupt line to signal data ready and configurable
threshold alarms.

This first implementation provides basic functionality (measurement
configuration, raw reads and ID validation) and defines the different
register regions in preparation for extended features in the subsequent
patches of the series.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 MAINTAINERS                     |   1 +
 drivers/iio/light/Kconfig       |  12 +
 drivers/iio/light/Makefile      |   1 +
 drivers/iio/light/veml6031x00.c | 680 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 694 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 921da7584963..db800537aefc 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -28379,6 +28379,7 @@ VISHAY VEML6031X00 AMBIENT LIGHT SENSOR DRIVER
 M:	Javier Carrasco <javier.carrasco.cruz@gmail.com>
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
+F:	drivers/iio/light/veml6031x00.c
 
 VISHAY VEML6046X00 RGBIR COLOR SENSOR DRIVER
 M:	Andreas Klinger <ak@it-klinger.de>
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index eff33e456c70..99a6ed80c7db 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -713,6 +713,18 @@ config VEML6030
 	  To compile this driver as a module, choose M here: the
 	  module will be called veml6030.
 
+config VEML6031X00
+	tristate "VEML6031X00 ambient light sensor series"
+	select REGMAP_I2C
+	select IIO_GTS_HELPER
+	depends on I2C
+	help
+	  Say Y here if you want to build a driver for the Vishay VEML6031X00
+	  ambient light sensor series.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called veml6031x00.
+
 config VEML6040
 	tristate "VEML6040 RGBW light sensor"
 	select REGMAP_I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index c0048e0d5ca8..a8cc03cfb6c2 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_VCNL4000)		+= vcnl4000.o
 obj-$(CONFIG_VCNL4035)		+= vcnl4035.o
 obj-$(CONFIG_VEML3235)		+= veml3235.o
 obj-$(CONFIG_VEML6030)		+= veml6030.o
+obj-$(CONFIG_VEML6031X00)	+= veml6031x00.o
 obj-$(CONFIG_VEML6040)		+= veml6040.o
 obj-$(CONFIG_VEML6046X00)	+= veml6046x00.o
 obj-$(CONFIG_VEML6070)		+= veml6070.o
diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
new file mode 100644
index 000000000000..6f9a7bad44d4
--- /dev/null
+++ b/drivers/iio/light/veml6031x00.c
@@ -0,0 +1,680 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * VEML6031X00 Ambient Light Sensor
+ *
+ * Copyright (c) 2026, Javier Carrasco <javier.carrasco.cruz@gmail.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/mutex.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/units.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/iio-gts-helper.h>
+
+/* Device registers */
+#define VEML6031X00_REG_CONF0       0x00
+#define VEML6031X00_REG_CONF1       0x01
+#define VEML6031X00_REG_ALS_L       0x10
+#define VEML6031X00_REG_ALS_H       0x11
+#define VEML6031X00_REG_IR_L        0x12
+#define VEML6031X00_REG_IR_H        0x13
+#define VEML6031X00_REG_ID_L        0x14
+#define VEML6031X00_REG_ID_H        0x15
+
+/* Bit masks for specific functionality */
+#define VEML6031X00_CONF0_SD        BIT(0)
+#define VEML6031X00_CONF1_IR_SD     BIT(7)
+
+struct veml6031x00_rf {
+	struct regmap_field *gain;
+	struct regmap_field *it;
+	struct regmap_field *pd_div4;
+};
+
+struct veml6031x00_chip {
+	const char *name;
+	const int part_id;
+};
+
+struct veml6031x00_data {
+	struct device *dev;
+	struct iio_gts gts;
+	struct regmap *regmap;
+	struct veml6031x00_rf rf;
+	const struct veml6031x00_chip *chip;
+	/*
+	 * Serialize access to scale register fields scattered across multiple
+	 * registers (rf.gain, rf.pd_div4, rf.it) to read and write them as a
+	 * consistent set.
+	 */
+	struct mutex scale_lock;
+};
+
+static const struct iio_itime_sel_mul veml6031x00_it_sel[] = {
+	GAIN_SCALE_ITIME_US(3125, 0, 1),
+	GAIN_SCALE_ITIME_US(6250, 1, 2),
+	GAIN_SCALE_ITIME_US(12500, 2, 4),
+	GAIN_SCALE_ITIME_US(25000, 3, 8),
+	GAIN_SCALE_ITIME_US(50000, 4, 16),
+	GAIN_SCALE_ITIME_US(100000, 5, 32),
+	GAIN_SCALE_ITIME_US(200000, 6, 64),
+	GAIN_SCALE_ITIME_US(400000, 7, 128),
+};
+
+/*
+ * The gain selector encodes (PD_D4 << 2) | GAIN to identify each gain setting.
+ * Gains are multiplied by 8 to work with integers. The values in the iio-gts
+ * tables don't need corrections because the maximum value of the scale refers
+ * to GAIN = x1, and the rest of the values are obtained from the resulting
+ * linear function.
+ * TODO: add support for MILLI_GAIN_X165 and MILLI_GAIN_X660
+ */
+#define VEML6031X00_SEL_MILLI_GAIN_X125  0x07
+#define VEML6031X00_SEL_MILLI_GAIN_X250  0x04
+#define VEML6031X00_SEL_MILLI_GAIN_X500  0x03
+#define VEML6031X00_SEL_MILLI_GAIN_X1000 0x00
+#define VEML6031X00_SEL_MILLI_GAIN_X2000 0x01
+static const struct iio_gain_sel_pair veml6031x00_gain_sel[] = {
+	GAIN_SCALE_GAIN(1, VEML6031X00_SEL_MILLI_GAIN_X125),
+	GAIN_SCALE_GAIN(2, VEML6031X00_SEL_MILLI_GAIN_X250),
+	GAIN_SCALE_GAIN(4, VEML6031X00_SEL_MILLI_GAIN_X500),
+	GAIN_SCALE_GAIN(8, VEML6031X00_SEL_MILLI_GAIN_X1000),
+	GAIN_SCALE_GAIN(16, VEML6031X00_SEL_MILLI_GAIN_X2000),
+};
+
+/*
+ * Two shutdown bits (SD and ALS_IR_SD) must be cleared to power on
+ * the device.
+ */
+static int veml6031x00_als_power_on(struct veml6031x00_data *data)
+{
+	int ret;
+
+	ret = regmap_clear_bits(data->regmap, VEML6031X00_REG_CONF0,
+				VEML6031X00_CONF0_SD);
+	if (ret)
+		return ret;
+
+	return regmap_clear_bits(data->regmap, VEML6031X00_REG_CONF1,
+				 VEML6031X00_CONF1_IR_SD);
+}
+
+/*
+ * Two shutdown bits (SD and ALS_IR_SD) must be set to power off
+ * the device.
+ */
+static int veml6031x00_als_shutdown(struct veml6031x00_data *data)
+{
+	int ret;
+
+	ret = regmap_set_bits(data->regmap, VEML6031X00_REG_CONF0,
+			      VEML6031X00_CONF0_SD);
+	if (ret)
+		return ret;
+
+	return regmap_set_bits(data->regmap, VEML6031X00_REG_CONF1,
+			       VEML6031X00_CONF1_IR_SD);
+}
+
+static void veml6031x00_als_shutdown_action(void *data)
+{
+	veml6031x00_als_shutdown(data);
+}
+
+static const struct iio_chan_spec veml6031x00_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.address = VEML6031X00_REG_ALS_L,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
+	},
+	{
+		.type = IIO_INTENSITY,
+		.address = VEML6031X00_REG_IR_L,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_IR,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
+		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
+	},
+};
+
+static const struct regmap_range veml6031x00_readable_ranges[] = {
+	regmap_reg_range(VEML6031X00_REG_CONF0, VEML6031X00_REG_CONF1),
+	regmap_reg_range(VEML6031X00_REG_ALS_L, VEML6031X00_REG_ID_H),
+};
+
+static const struct regmap_access_table veml6031x00_readable_table = {
+	.yes_ranges = veml6031x00_readable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(veml6031x00_readable_ranges),
+};
+
+static const struct regmap_range veml6031x00_writable_ranges[] = {
+	regmap_reg_range(VEML6031X00_REG_CONF0, VEML6031X00_REG_CONF1),
+};
+
+static const struct regmap_access_table veml6031x00_writable_table = {
+	.yes_ranges = veml6031x00_writable_ranges,
+	.n_yes_ranges = ARRAY_SIZE(veml6031x00_writable_ranges),
+};
+
+static const struct regmap_range veml6031x00_volatile_ranges[] = {
+	regmap_reg_range(VEML6031X00_REG_ALS_L, VEML6031X00_REG_IR_H),
+};
+
+static const struct regmap_access_table veml6031x00_volatile_table = {
+	.yes_ranges = veml6031x00_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(veml6031x00_volatile_ranges),
+};
+
+static const struct regmap_config veml6031x00_regmap_config = {
+	.name = "veml6031x00_regmap",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.rd_table = &veml6031x00_readable_table,
+	.wr_table = &veml6031x00_writable_table,
+	.volatile_table = &veml6031x00_volatile_table,
+	.max_register = VEML6031X00_REG_ID_H,
+	.cache_type = REGCACHE_MAPLE,
+};
+
+static const struct reg_field veml6031x00_rf_it =
+	REG_FIELD(VEML6031X00_REG_CONF0, 4, 6);
+
+static const struct reg_field veml6031x00_rf_gain =
+	REG_FIELD(VEML6031X00_REG_CONF1, 3, 4);
+
+static const struct reg_field veml6031x00_rf_pd_div4 =
+	REG_FIELD(VEML6031X00_REG_CONF1, 6, 6);
+
+static int veml6031x00_regfield_init(struct veml6031x00_data *data)
+{
+	struct regmap *regmap = data->regmap;
+	struct device *dev = data->dev;
+	struct regmap_field *rm_field;
+	struct veml6031x00_rf *rf = &data->rf;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_gain);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->gain = rm_field;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_it);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->it = rm_field;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_pd_div4);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->pd_div4 = rm_field;
+
+	return 0;
+}
+
+static int veml6031x00_get_it(struct veml6031x00_data *data, int *val2)
+{
+	int ret, it_idx;
+
+	scoped_guard(mutex, &data->scale_lock) {
+		ret = regmap_field_read(data->rf.it, &it_idx);
+		if (ret)
+			return ret;
+	}
+
+	ret = iio_gts_find_int_time_by_sel(&data->gts, it_idx);
+	if (ret < 0)
+		return ret;
+
+	*val2 = ret;
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int veml6031x00_set_it(struct iio_dev *iio, int val, int val2)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+	int ret, gain_sel, gain_reg, pd_div4, it_idx, new_gain, prev_gain, prev_it;
+	bool in_range;
+
+	if (val || !iio_gts_valid_time(&data->gts, val2))
+		return -EINVAL;
+
+	guard(mutex)(&data->scale_lock);
+
+	ret = regmap_field_read(data->rf.it, &it_idx);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_read(data->rf.gain, &gain_reg);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_read(data->rf.pd_div4, &pd_div4);
+	if (ret)
+		return ret;
+
+	prev_it = iio_gts_find_int_time_by_sel(&data->gts, it_idx);
+	if (prev_it < 0)
+		return prev_it;
+
+	if (prev_it == val2)
+		return 0;
+
+	prev_gain = iio_gts_find_gain_by_sel(&data->gts, (pd_div4 << 2) | gain_reg);
+	if (prev_gain < 0)
+		return prev_gain;
+
+	ret = iio_gts_find_new_gain_by_gain_time_min(&data->gts, prev_gain, prev_it,
+						     val2, &new_gain, &in_range);
+	if (ret)
+		return ret;
+
+	if (!in_range)
+		dev_dbg(data->dev, "Optimal gain out of range\n");
+
+	ret = iio_gts_find_sel_by_int_time(&data->gts, val2);
+	if (ret < 0)
+		return ret;
+
+	ret = regmap_field_write(data->rf.it, ret);
+	if (ret)
+		return ret;
+
+	gain_sel = iio_gts_find_sel_by_gain(&data->gts, new_gain);
+	if (gain_sel < 0)
+		return gain_sel;
+
+	ret = regmap_field_write(data->rf.pd_div4, gain_sel >> 2);
+	if (ret)
+		return ret;
+
+	return regmap_field_write(data->rf.gain, gain_sel & 0x03);
+}
+
+static int veml6031x00_set_scale(struct iio_dev *iio, int val, int val2)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+	int gain_sel, it_sel, ret;
+
+	ret = iio_gts_find_gain_time_sel_for_scale(&data->gts, val, val2,
+						   &gain_sel, &it_sel);
+	if (ret)
+		return ret;
+
+	guard(mutex)(&data->scale_lock);
+
+	ret = regmap_field_write(data->rf.it, it_sel);
+	if (ret)
+		return ret;
+
+	ret = regmap_field_write(data->rf.pd_div4, gain_sel >> 2);
+	if (ret)
+		return ret;
+
+	return regmap_field_write(data->rf.gain, gain_sel & 0x03);
+}
+
+static int veml6031x00_get_scale(struct veml6031x00_data *data, int *val,
+				 int *val2)
+{
+	int gain, it, gain_reg, pd_div4, it_reg, ret, sel;
+
+	scoped_guard(mutex, &data->scale_lock) {
+		ret = regmap_field_read(data->rf.gain, &gain_reg);
+		if (ret)
+			return ret;
+
+		ret = regmap_field_read(data->rf.pd_div4, &pd_div4);
+		if (ret)
+			return ret;
+
+		sel = (pd_div4 << 2) | gain_reg;
+		gain = iio_gts_find_gain_by_sel(&data->gts, sel);
+		if (gain < 0)
+			return gain;
+
+		ret = regmap_field_read(data->rf.it, &it_reg);
+		if (ret)
+			return ret;
+	}
+
+	it = iio_gts_find_int_time_by_sel(&data->gts, it_reg);
+	if (it < 0)
+		return it;
+
+	ret = iio_gts_get_scale(&data->gts, gain, it, val, val2);
+	if (ret)
+		return ret;
+
+	return IIO_VAL_INT_PLUS_NANO;
+}
+
+static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
+				   int *val)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+	int addr, it_usec, ret;
+	__le16 reg;
+
+	switch (type) {
+	case IIO_LIGHT:
+		addr = VEML6031X00_REG_ALS_L;
+		break;
+	case IIO_INTENSITY:
+		addr = VEML6031X00_REG_IR_L;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(data->dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
+	ret = veml6031x00_get_it(data, &it_usec);
+	if (ret < 0)
+		return ret;
+
+	/* integration time + 10 % to ensure completion */
+	fsleep(it_usec + (it_usec / 10));
+
+	ret = regmap_bulk_read(data->regmap, addr, &reg, sizeof(reg));
+	if (ret)
+		return ret;
+
+	*val = le16_to_cpu(reg);
+	return IIO_VAL_INT;
+}
+
+static int veml6031x00_read_raw(struct iio_dev *iio,
+				struct iio_chan_spec const *chan, int *val,
+				int *val2, long mask)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		return veml6031x00_single_read(iio, chan->type, val);
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = 0;
+		return veml6031x00_get_it(data, val2);
+	case IIO_CHAN_INFO_SCALE:
+		return veml6031x00_get_scale(data, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml6031x00_read_avail(struct iio_dev *iio,
+				  struct iio_chan_spec const *chan,
+				  const int **vals, int *type, int *length,
+				  long mask)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return iio_gts_avail_times(&data->gts, vals, type, length);
+	case IIO_CHAN_INFO_SCALE:
+		return iio_gts_all_avail_scales(&data->gts, vals, type, length);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml6031x00_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 veml6031x00_set_it(iio, val, val2);
+	case IIO_CHAN_INFO_SCALE:
+		return veml6031x00_set_scale(iio, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml6031x00_write_raw_get_fmt(struct iio_dev *indio_dev,
+					 struct iio_chan_spec const *chan,
+					 long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+		return IIO_VAL_INT_PLUS_NANO;
+	case IIO_CHAN_INFO_INT_TIME:
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info veml6031x00_info = {
+	.read_raw = veml6031x00_read_raw,
+	.read_avail = veml6031x00_read_avail,
+	.write_raw = veml6031x00_write_raw,
+	.write_raw_get_fmt = veml6031x00_write_raw_get_fmt,
+};
+
+static int veml6031x00_validate_part_id(struct veml6031x00_data *data)
+{
+	int part_id, ret;
+	__le16 reg;
+
+	ret = regmap_bulk_read(data->regmap, VEML6031X00_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 != data->chip->part_id)
+		dev_warn(data->dev, "Unknown ID %04x\n", part_id);
+
+	return 0;
+}
+
+static int veml6031x00_hw_init(struct iio_dev *iio)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+	struct device *dev = data->dev;
+	int ret;
+
+	/* Max resolution = 6.9632 lx/cnt for gain = 0.125 and IT = 3.125ms */
+	ret = devm_iio_init_iio_gts(dev, 6, 963200000,
+				    veml6031x00_gain_sel,
+				    ARRAY_SIZE(veml6031x00_gain_sel),
+				    veml6031x00_it_sel,
+				    ARRAY_SIZE(veml6031x00_it_sel),
+				    &data->gts);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to init iio gts\n");
+
+	return 0;
+}
+
+static int veml6031x00_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct veml6031x00_data *data;
+	struct iio_dev *iio;
+	struct regmap *regmap;
+	int ret;
+
+	regmap = devm_regmap_init_i2c(i2c, &veml6031x00_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 = devm_mutex_init(dev, &data->scale_lock);
+	if (ret)
+		return ret;
+
+	ret = veml6031x00_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");
+
+	data->chip = i2c_get_match_data(i2c);
+	if (!data->chip)
+		return dev_err_probe(dev, -EINVAL, "Failed to get chip data\n");
+
+	/* The device starts in power down mode by default */
+	ret = veml6031x00_als_power_on(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to power on the device\n");
+
+	ret = devm_add_action_or_reset(dev, veml6031x00_als_shutdown_action, data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to add shutdown action\n");
+
+	pm_runtime_set_autosuspend_delay(dev, 2000);
+	pm_runtime_use_autosuspend(dev);
+	ret = devm_pm_runtime_set_active_enabled(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
+
+	ret = devm_pm_runtime_get_noresume(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to get runtime PM\n");
+
+	ret = veml6031x00_validate_part_id(data);
+	if (ret)
+		return ret;
+
+	iio->name = data->chip->name;
+	iio->channels = veml6031x00_channels;
+	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
+	iio->modes = INDIO_DIRECT_MODE;
+	iio->info = &veml6031x00_info;
+
+	ret = veml6031x00_hw_init(iio);
+	if (ret)
+		return ret;
+
+	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\n");
+
+	return 0;
+}
+
+static int veml6031x00_runtime_suspend(struct device *dev)
+{
+	struct veml6031x00_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return veml6031x00_als_shutdown(data);
+}
+
+static int veml6031x00_runtime_resume(struct device *dev)
+{
+	struct veml6031x00_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return veml6031x00_als_power_on(data);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(veml6031x00_pm_ops, veml6031x00_runtime_suspend,
+				 veml6031x00_runtime_resume, NULL);
+
+static const struct veml6031x00_chip veml6031x00_chip = {
+	.name = "veml6031x00",
+	.part_id = 0x0001,
+};
+
+static const struct veml6031x00_chip veml6031x01_chip = {
+	.name = "veml6031x01",
+	.part_id = 0x0001,
+};
+
+static const struct veml6031x00_chip veml60311x00_chip = {
+	.name = "veml60311x00",
+	.part_id = 0x1001,
+};
+
+static const struct veml6031x00_chip veml60311x01_chip = {
+	.name = "veml60311x01",
+	.part_id = 0x1001,
+};
+
+static const struct of_device_id veml6031x00_of_match[] = {
+	{
+		.compatible = "vishay,veml6031x00",
+		.data = &veml6031x00_chip,
+	},
+	{
+		.compatible = "vishay,veml6031x01",
+		.data = &veml6031x01_chip,
+	},
+	{
+		.compatible = "vishay,veml60311x00",
+		.data = &veml60311x00_chip,
+	},
+	{
+		.compatible = "vishay,veml60311x01",
+		.data = &veml60311x01_chip,
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, veml6031x00_of_match);
+
+static const struct i2c_device_id veml6031x00_id[] = {
+	{
+		.name = "veml6031x00",
+		.driver_data = (kernel_ulong_t)&veml6031x00_chip
+	},
+	{
+		.name = "veml6031x01",
+		.driver_data = (kernel_ulong_t)&veml6031x01_chip },
+	{
+		.name = "veml60311x00",
+		.driver_data = (kernel_ulong_t)&veml60311x00_chip
+	},
+	{
+		.name = "veml60311x01",
+		.driver_data = (kernel_ulong_t)&veml60311x01_chip
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, veml6031x00_id);
+
+static struct i2c_driver veml6031x00_driver = {
+	.driver = {
+		.name = "veml6031x00",
+		.of_match_table = veml6031x00_of_match,
+		.pm = pm_ptr(&veml6031x00_pm_ops),
+	},
+	.probe = veml6031x00_probe,
+	.id_table = veml6031x00_id,
+};
+module_i2c_driver(veml6031x00_driver);
+
+MODULE_AUTHOR("Javier Carrasco <javier.carrasco.cruz@gmail.com>");
+MODULE_DESCRIPTION("VEML6031X00 Ambient Light Sensor");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("IIO_GTS_HELPER");

-- 
2.43.0

[PATCH v4 3/4] iio: light: veml6031x00: add support for triggered buffers

[Javier Carrasco]

Add triggered buffer functionality for the two channels the device
provides (ALS and IR).

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/Kconfig       |   2 +
 drivers/iio/light/veml6031x00.c | 114 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+)

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index 99a6ed80c7db..ff71de8454bd 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -717,6 +717,8 @@ config VEML6031X00
 	tristate "VEML6031X00 ambient light sensor series"
 	select REGMAP_I2C
 	select IIO_GTS_HELPER
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
 	depends on I2C
 	help
 	  Say Y here if you want to build a driver for the Vishay VEML6031X00
diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
index 6f9a7bad44d4..facb1b8e4241 100644
--- a/drivers/iio/light/veml6031x00.c
+++ b/drivers/iio/light/veml6031x00.c
@@ -16,6 +16,8 @@
 #include <linux/units.h>
 #include <linux/iio/iio.h>
 #include <linux/iio/iio-gts-helper.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
 
 /* Device registers */
 #define VEML6031X00_REG_CONF0       0x00
@@ -31,6 +33,12 @@
 #define VEML6031X00_CONF0_SD        BIT(0)
 #define VEML6031X00_CONF1_IR_SD     BIT(7)
 
+enum veml6031x00_scan {
+	VEML6031X00_SCAN_ALS,
+	VEML6031X00_SCAN_IR,
+	VEML6031X00_SCAN_TIMESTAMP,
+};
+
 struct veml6031x00_rf {
 	struct regmap_field *gain;
 	struct regmap_field *it;
@@ -136,6 +144,13 @@ static const struct iio_chan_spec veml6031x00_channels[] = {
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
 		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
+		.scan_index = VEML6031X00_SCAN_ALS,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
 	},
 	{
 		.type = IIO_INTENSITY,
@@ -146,7 +161,15 @@ static const struct iio_chan_spec veml6031x00_channels[] = {
 				      BIT(IIO_CHAN_INFO_SCALE),
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
+		.scan_index = VEML6031X00_SCAN_IR,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
 	},
+	IIO_CHAN_SOFT_TIMESTAMP(VEML6031X00_SCAN_TIMESTAMP),
 };
 
 static const struct regmap_range veml6031x00_readable_ranges[] = {
@@ -378,6 +401,10 @@ static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
 		return -EINVAL;
 	}
 
+	IIO_DEV_ACQUIRE_DIRECT_MODE(iio, claim);
+	if (IIO_DEV_ACQUIRE_FAILED(claim))
+		return -EBUSY;
+
 	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(data->dev, pm);
 	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
 	if (ret)
@@ -438,6 +465,10 @@ static int veml6031x00_write_raw(struct iio_dev *iio,
 				 struct iio_chan_spec const *chan,
 				 int val, int val2, long mask)
 {
+	IIO_DEV_ACQUIRE_DIRECT_MODE(iio, claim);
+	if (IIO_DEV_ACQUIRE_FAILED(claim))
+		return -EBUSY;
+
 	switch (mask) {
 	case IIO_CHAN_INFO_INT_TIME:
 		return veml6031x00_set_it(iio, val, val2);
@@ -469,6 +500,82 @@ static const struct iio_info veml6031x00_info = {
 	.write_raw_get_fmt = veml6031x00_write_raw_get_fmt,
 };
 
+static int veml6031x00_buffer_preenable(struct iio_dev *iio)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+	int ret, it_usec;
+
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret)
+		return ret;
+
+	ret = veml6031x00_get_it(data, &it_usec);
+	if (ret < 0) {
+		pm_runtime_put_autosuspend(data->dev);
+		return ret;
+	}
+
+	/*
+	 * Wait one integration period + 10% margin so the first triggered
+	 * read does not race with the sensor completing its first conversion
+	 * after power-on.
+	 */
+	fsleep(it_usec + (it_usec / 10));
+
+	return 0;
+}
+
+static int veml6031x00_buffer_postdisable(struct iio_dev *iio)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+
+	pm_runtime_put_autosuspend(data->dev);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops veml6031x00_buffer_setup_ops = {
+	.preenable = veml6031x00_buffer_preenable,
+	.postdisable = veml6031x00_buffer_postdisable,
+};
+
+static irqreturn_t veml6031x00_trig_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *iio = pf->indio_dev;
+	struct veml6031x00_data *data = iio_priv(iio);
+	int ch, ret, i = 0;
+	struct {
+		__le16 chans[2];
+		aligned_s64 timestamp;
+	} scan = { };
+
+	if (test_bit(VEML6031X00_SCAN_ALS, iio->active_scan_mask) &&
+	    test_bit(VEML6031X00_SCAN_IR, iio->active_scan_mask)) {
+		ret = regmap_bulk_read(data->regmap,
+				       VEML6031X00_REG_ALS_L,
+				       &scan.chans, sizeof(scan.chans));
+		if (ret)
+			goto done;
+	} else {
+		iio_for_each_active_channel(iio, ch) {
+			ret = regmap_bulk_read(data->regmap,
+					       iio->channels[ch].address,
+					       &scan.chans[i++],
+					       sizeof(*scan.chans));
+			if (ret)
+				goto done;
+		}
+	}
+
+	iio_push_to_buffers_with_ts(iio, &scan, sizeof(scan), pf->timestamp);
+
+done:
+	iio_trigger_notify_done(iio->trig);
+
+	return IRQ_HANDLED;
+}
+
 static int veml6031x00_validate_part_id(struct veml6031x00_data *data)
 {
 	int part_id, ret;
@@ -576,6 +683,13 @@ static int veml6031x00_probe(struct i2c_client *i2c)
 	if (ret)
 		return ret;
 
+	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
+					      veml6031x00_trig_handler,
+					      &veml6031x00_buffer_setup_ops);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register triggered buffer\n");
+
 	pm_runtime_put_autosuspend(dev);
 
 	ret = devm_iio_device_register(dev, iio);

-- 
2.43.0

[PATCH v4 4/4] iio: light: veml6031x00: add support for events and trigger

[Javier Carrasco]

The device provides a shared interrupt line for to notify events and
data ready, which can be used as a trigger. The interrupt line is not a
requirement for the device to work. Implement variants for the cases
whether the interrupt line is provided or not.

Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
---
 drivers/iio/light/veml6031x00.c | 442 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 438 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
index facb1b8e4241..5e79eb942f09 100644
--- a/drivers/iio/light/veml6031x00.c
+++ b/drivers/iio/light/veml6031x00.c
@@ -8,6 +8,7 @@
 #include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/i2c.h>
+#include <linux/interrupt.h>
 #include <linux/module.h>
 #include <linux/mod_devicetable.h>
 #include <linux/mutex.h>
@@ -15,6 +16,9 @@
 #include <linux/regmap.h>
 #include <linux/units.h>
 #include <linux/iio/iio.h>
+#include <linux/iio/events.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
 #include <linux/iio/iio-gts-helper.h>
 #include <linux/iio/triggered_buffer.h>
 #include <linux/iio/trigger_consumer.h>
@@ -22,16 +26,29 @@
 /* Device registers */
 #define VEML6031X00_REG_CONF0       0x00
 #define VEML6031X00_REG_CONF1       0x01
+#define VEML6031X00_REG_WH_L        0x04
+#define VEML6031X00_REG_WH_H        0x05
+#define VEML6031X00_REG_WL_L        0x06
+#define VEML6031X00_REG_WL_H        0x07
 #define VEML6031X00_REG_ALS_L       0x10
 #define VEML6031X00_REG_ALS_H       0x11
 #define VEML6031X00_REG_IR_L        0x12
 #define VEML6031X00_REG_IR_H        0x13
 #define VEML6031X00_REG_ID_L        0x14
 #define VEML6031X00_REG_ID_H        0x15
+#define VEML6031X00_REG_INT         0x17
 
 /* Bit masks for specific functionality */
 #define VEML6031X00_CONF0_SD        BIT(0)
+#define VEML6031X00_CONF0_AF_TRIG   BIT(2)
+#define VEML6031X00_CONF0_AF        BIT(3)
 #define VEML6031X00_CONF1_IR_SD     BIT(7)
+#define VEML6031X00_INT_TH_H        BIT(1)
+#define VEML6031X00_INT_TH_L        BIT(2)
+#define VEML6031X00_INT_DRDY        BIT(3)
+#define VEML6031X00_INT_MASK        (VEML6031X00_INT_TH_L | \
+				     VEML6031X00_INT_TH_H | \
+				     VEML6031X00_INT_DRDY)
 
 enum veml6031x00_scan {
 	VEML6031X00_SCAN_ALS,
@@ -41,8 +58,10 @@ enum veml6031x00_scan {
 
 struct veml6031x00_rf {
 	struct regmap_field *gain;
+	struct regmap_field *int_en;
 	struct regmap_field *it;
 	struct regmap_field *pd_div4;
+	struct regmap_field *pers;
 };
 
 struct veml6031x00_chip {
@@ -54,6 +73,7 @@ struct veml6031x00_data {
 	struct device *dev;
 	struct iio_gts gts;
 	struct regmap *regmap;
+	struct iio_trigger *trig;
 	struct veml6031x00_rf rf;
 	const struct veml6031x00_chip *chip;
 	/*
@@ -62,6 +82,11 @@ struct veml6031x00_data {
 	 * consistent set.
 	 */
 	struct mutex scale_lock;
+	/* serialize access to irq enable/disable by events and trigger */
+	struct mutex irq_lock;
+	int int_users;
+	bool ev_en;
+	bool trig_en;
 };
 
 static const struct iio_itime_sel_mul veml6031x00_it_sel[] = {
@@ -96,6 +121,17 @@ static const struct iio_gain_sel_pair veml6031x00_gain_sel[] = {
 	GAIN_SCALE_GAIN(16, VEML6031X00_SEL_MILLI_GAIN_X2000),
 };
 
+static IIO_CONST_ATTR(in_illuminance_thresh_either_period_available, "1 2 4 8");
+
+static struct attribute *veml6031x00_event_attributes[] = {
+	&iio_const_attr_in_illuminance_thresh_either_period_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group veml6031x00_event_attr_group = {
+	.attrs = veml6031x00_event_attributes,
+};
+
 /*
  * Two shutdown bits (SD and ALS_IR_SD) must be cleared to power on
  * the device.
@@ -135,6 +171,23 @@ static void veml6031x00_als_shutdown_action(void *data)
 	veml6031x00_als_shutdown(data);
 }
 
+static const struct iio_event_spec veml6031x00_event_spec[] = {
+	{
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_RISING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_FALLING,
+		.mask_separate = BIT(IIO_EV_INFO_VALUE),
+	}, {
+		.type = IIO_EV_TYPE_THRESH,
+		.dir = IIO_EV_DIR_EITHER,
+		.mask_separate = BIT(IIO_EV_INFO_PERIOD) |
+				 BIT(IIO_EV_INFO_ENABLE),
+	},
+};
+
 static const struct iio_chan_spec veml6031x00_channels[] = {
 	{
 		.type = IIO_LIGHT,
@@ -144,6 +197,8 @@ static const struct iio_chan_spec veml6031x00_channels[] = {
 		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
 		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
 		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
+		.event_spec = veml6031x00_event_spec,
+		.num_event_specs = ARRAY_SIZE(veml6031x00_event_spec),
 		.scan_index = VEML6031X00_SCAN_ALS,
 		.scan_type = {
 			.sign = 'u',
@@ -174,7 +229,9 @@ static const struct iio_chan_spec veml6031x00_channels[] = {
 
 static const struct regmap_range veml6031x00_readable_ranges[] = {
 	regmap_reg_range(VEML6031X00_REG_CONF0, VEML6031X00_REG_CONF1),
+	regmap_reg_range(VEML6031X00_REG_WH_L, VEML6031X00_REG_WL_H),
 	regmap_reg_range(VEML6031X00_REG_ALS_L, VEML6031X00_REG_ID_H),
+	regmap_reg_range(VEML6031X00_REG_INT, VEML6031X00_REG_INT),
 };
 
 static const struct regmap_access_table veml6031x00_readable_table = {
@@ -183,7 +240,7 @@ static const struct regmap_access_table veml6031x00_readable_table = {
 };
 
 static const struct regmap_range veml6031x00_writable_ranges[] = {
-	regmap_reg_range(VEML6031X00_REG_CONF0, VEML6031X00_REG_CONF1),
+	regmap_reg_range(VEML6031X00_REG_CONF0, VEML6031X00_REG_WL_H),
 };
 
 static const struct regmap_access_table veml6031x00_writable_table = {
@@ -193,6 +250,7 @@ static const struct regmap_access_table veml6031x00_writable_table = {
 
 static const struct regmap_range veml6031x00_volatile_ranges[] = {
 	regmap_reg_range(VEML6031X00_REG_ALS_L, VEML6031X00_REG_IR_H),
+	regmap_reg_range(VEML6031X00_REG_INT, VEML6031X00_REG_INT),
 };
 
 static const struct regmap_access_table veml6031x00_volatile_table = {
@@ -200,6 +258,15 @@ static const struct regmap_access_table veml6031x00_volatile_table = {
 	.n_yes_ranges = ARRAY_SIZE(veml6031x00_volatile_ranges),
 };
 
+static const struct regmap_range veml6031x00_precious_ranges[] = {
+	regmap_reg_range(VEML6031X00_REG_INT, VEML6031X00_REG_INT),
+};
+
+static const struct regmap_access_table veml6031x00_precious_table = {
+	.yes_ranges = veml6031x00_precious_ranges,
+	.n_yes_ranges = ARRAY_SIZE(veml6031x00_precious_ranges),
+};
+
 static const struct regmap_config veml6031x00_regmap_config = {
 	.name = "veml6031x00_regmap",
 	.reg_bits = 8,
@@ -207,13 +274,20 @@ static const struct regmap_config veml6031x00_regmap_config = {
 	.rd_table = &veml6031x00_readable_table,
 	.wr_table = &veml6031x00_writable_table,
 	.volatile_table = &veml6031x00_volatile_table,
-	.max_register = VEML6031X00_REG_ID_H,
+	.precious_table = &veml6031x00_precious_table,
+	.max_register = VEML6031X00_REG_INT,
 	.cache_type = REGCACHE_MAPLE,
 };
 
+static const struct reg_field veml6031x00_rf_int_en =
+	REG_FIELD(VEML6031X00_REG_CONF0, 1, 1);
+
 static const struct reg_field veml6031x00_rf_it =
 	REG_FIELD(VEML6031X00_REG_CONF0, 4, 6);
 
+static const struct reg_field veml6031x00_rf_pers =
+	REG_FIELD(VEML6031X00_REG_CONF1, 1, 2);
+
 static const struct reg_field veml6031x00_rf_gain =
 	REG_FIELD(VEML6031X00_REG_CONF1, 3, 4);
 
@@ -232,6 +306,11 @@ static int veml6031x00_regfield_init(struct veml6031x00_data *data)
 		return PTR_ERR(rm_field);
 	rf->gain = rm_field;
 
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_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, veml6031x00_rf_it);
 	if (IS_ERR(rm_field))
 		return PTR_ERR(rm_field);
@@ -242,6 +321,11 @@ static int veml6031x00_regfield_init(struct veml6031x00_data *data)
 		return PTR_ERR(rm_field);
 	rf->pd_div4 = rm_field;
 
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_pers);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->pers = rm_field;
+
 	return 0;
 }
 
@@ -325,6 +409,30 @@ static int veml6031x00_set_it(struct iio_dev *iio, int val, int val2)
 	return regmap_field_write(data->rf.gain, gain_sel & 0x03);
 }
 
+static int veml6031x00_read_period(struct iio_dev *iio, int *val)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+	int ret, reg;
+
+	ret = regmap_field_read(data->rf.pers, &reg);
+	if (ret)
+		return ret;
+
+	*val = 1 << reg;
+
+	return IIO_VAL_INT;
+}
+
+static int veml6031x00_write_period(struct iio_dev *iio, int val)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+
+	if (val < 0 || val > 8 || hweight8(val) != 1)
+		return -EINVAL;
+
+	return regmap_field_write(data->rf.pers, ffs(val) - 1);
+}
+
 static int veml6031x00_set_scale(struct iio_dev *iio, int val, int val2)
 {
 	struct veml6031x00_data *data = iio_priv(iio);
@@ -383,6 +491,51 @@ static int veml6031x00_get_scale(struct veml6031x00_data *data, int *val,
 	return IIO_VAL_INT_PLUS_NANO;
 }
 
+static int veml6031x00_read_th(struct iio_dev *iio, int *val, int *val2, int dir)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+	__le16 reg;
+	int ret;
+
+	if (dir == IIO_EV_DIR_RISING)
+		ret = regmap_bulk_read(data->regmap, VEML6031X00_REG_WH_L,
+				       &reg, sizeof(reg));
+	else
+		ret = regmap_bulk_read(data->regmap, VEML6031X00_REG_WL_L,
+				       &reg, sizeof(reg));
+	if (ret)
+		return ret;
+
+	*val = le16_to_cpu(reg);
+
+	return IIO_VAL_INT;
+}
+
+static int veml6031x00_write_th(struct iio_dev *iio, int val, int val2, int dir)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+	struct device *dev = data->dev;
+	__le16 reg = cpu_to_le16(val);
+	int ret;
+
+	if (val < 0 || val > U16_MAX || val2)
+		return -EINVAL;
+
+	if (dir == IIO_EV_DIR_RISING) {
+		ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WH_L,
+					&reg, sizeof(reg));
+		if (ret)
+			dev_dbg(dev, "Failed to set high threshold %d\n", ret);
+	} else {
+		ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WL_L,
+					&reg, sizeof(reg));
+		if (ret)
+			dev_dbg(dev, "Failed to set low threshold %d\n", ret);
+	}
+
+	return ret;
+}
+
 static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
 				   int *val)
 {
@@ -493,13 +646,189 @@ static int veml6031x00_write_raw_get_fmt(struct iio_dev *indio_dev,
 	}
 }
 
+static int veml6031x00_set_interrupt(struct veml6031x00_data *data, bool state)
+	__must_hold(&data->irq_lock)
+{
+	int ret;
+
+	if (state) {
+		data->int_users++;
+		if (data->int_users > 1)
+			return 0;
+	} else {
+		data->int_users--;
+		if (data->int_users > 0)
+			return 0;
+	}
+
+	ret = regmap_field_write(data->rf.int_en, state);
+	if (ret) {
+		if (state)
+			data->int_users--;
+		else
+			data->int_users++;
+	}
+
+	return ret;
+}
+
+static int veml6031x00_read_event_val(struct iio_dev *iio,
+				      const struct iio_chan_spec *chan,
+				      enum iio_event_type type,
+				      enum iio_event_direction dir,
+				      enum iio_event_info info,
+				      int *val, int *val2)
+{
+	switch (type) {
+	case IIO_EV_TYPE_THRESH:
+		if (dir == IIO_EV_DIR_EITHER && info == IIO_EV_INFO_PERIOD)
+			return veml6031x00_read_period(iio, val);
+
+		return veml6031x00_read_th(iio, val, val2, dir);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml6031x00_write_event_val(struct iio_dev *iio,
+				       const struct iio_chan_spec *chan,
+				       enum iio_event_type type,
+				       enum iio_event_direction dir,
+				       enum iio_event_info info,
+				       int val, int val2)
+{
+	switch (info) {
+	case IIO_EV_INFO_VALUE:
+		return veml6031x00_write_th(iio, val, val2, dir);
+	case IIO_EV_INFO_PERIOD:
+		return veml6031x00_write_period(iio, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml6031x00_read_event_config(struct iio_dev *iio,
+					 const struct iio_chan_spec *chan,
+					 enum iio_event_type type,
+					 enum iio_event_direction dir)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+
+	guard(mutex)(&data->irq_lock);
+
+	return data->ev_en;
+}
+
+static int veml6031x00_write_event_config(struct iio_dev *iio,
+					  const struct iio_chan_spec *chan,
+					  enum iio_event_type type,
+					  enum iio_event_direction dir,
+					  bool state)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+	int ret;
+
+	guard(mutex)(&data->irq_lock);
+
+	/* avoid multiple increments/decrements from one source */
+	if (state == data->ev_en)
+		return 0;
+
+	if (state) {
+		ret = pm_runtime_resume_and_get(data->dev);
+		if (ret)
+			return ret;
+	}
+
+	ret = veml6031x00_set_interrupt(data, state);
+	if (ret) {
+		if (state)
+			pm_runtime_put_autosuspend(data->dev);
+		return ret;
+	}
+
+	data->ev_en = state;
+
+	if (!state)
+		pm_runtime_put_autosuspend(data->dev);
+
+	return 0;
+}
+
 static const struct iio_info veml6031x00_info = {
 	.read_raw = veml6031x00_read_raw,
 	.read_avail = veml6031x00_read_avail,
 	.write_raw = veml6031x00_write_raw,
 	.write_raw_get_fmt = veml6031x00_write_raw_get_fmt,
+	.read_event_value = veml6031x00_read_event_val,
+	.write_event_value = veml6031x00_write_event_val,
+	.read_event_config = veml6031x00_read_event_config,
+	.write_event_config = veml6031x00_write_event_config,
+	.event_attrs = &veml6031x00_event_attr_group,
+};
+
+static const struct iio_info veml6031x00_info_no_irq = {
+	.read_raw = veml6031x00_read_raw,
+	.read_avail = veml6031x00_read_avail,
+	.write_raw = veml6031x00_write_raw,
+	.write_raw_get_fmt = veml6031x00_write_raw_get_fmt,
 };
 
+/* AF_TRIG is reset by hardware, but the rest of the fields are persistent */
+static int veml6031x00_set_af_trig(struct veml6031x00_data *data, bool state)
+{
+	regcache_drop_region(data->regmap, VEML6031X00_REG_CONF0,
+			     VEML6031X00_REG_CONF0);
+
+	return regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
+				  VEML6031X00_CONF0_AF_TRIG,
+				  FIELD_PREP(VEML6031X00_CONF0_AF_TRIG, state));
+}
+
+static irqreturn_t veml6031x00_interrupt(int irq, void *private)
+{
+	struct iio_dev *iio = private;
+	struct veml6031x00_data *data = iio_priv(iio);
+	s64 timestamp;
+	int ret, reg;
+
+	ret = regmap_read(data->regmap, VEML6031X00_REG_INT, &reg);
+	if (ret) {
+		dev_err(data->dev,
+			"Failed to read interrupt register %d\n", ret);
+		return IRQ_NONE;
+	}
+
+	if (!(reg & VEML6031X00_INT_MASK))
+		return IRQ_NONE;
+
+	guard(mutex)(&data->irq_lock);
+
+	if ((reg & (VEML6031X00_INT_TH_H | VEML6031X00_INT_TH_L)) && data->ev_en) {
+		timestamp = iio_get_time_ns(iio);
+
+		if (reg & VEML6031X00_INT_TH_H)
+			iio_push_event(iio, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
+								 IIO_EV_TYPE_THRESH,
+								 IIO_EV_DIR_RISING),
+				       timestamp);
+		if (reg & VEML6031X00_INT_TH_L)
+			iio_push_event(iio, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
+								 IIO_EV_TYPE_THRESH,
+								 IIO_EV_DIR_FALLING),
+				       timestamp);
+	}
+
+	if ((reg & VEML6031X00_INT_DRDY) && data->trig_en) {
+		iio_trigger_poll_nested(data->trig);
+		ret = veml6031x00_set_af_trig(data, true);
+		if (ret)
+			dev_err(data->dev, "Failed to set trigger %d\n", ret);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static int veml6031x00_buffer_preenable(struct iio_dev *iio)
 {
 	struct veml6031x00_data *data = iio_priv(iio);
@@ -534,11 +863,54 @@ static int veml6031x00_buffer_postdisable(struct iio_dev *iio)
 	return 0;
 }
 
+static int veml6031x00_set_trigger_state(struct iio_trigger *trig, bool state)
+{
+	struct iio_dev *iio = iio_trigger_get_drvdata(trig);
+	struct veml6031x00_data *data = iio_priv(iio);
+	int ret;
+
+	guard(mutex)(&data->irq_lock);
+
+	if (state == data->trig_en)
+		return 0;
+
+	ret = veml6031x00_set_interrupt(data, state);
+	if (ret)
+		return ret;
+
+	/* The AF bit must be set before setting AF_TRIG */
+	ret = regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
+				 VEML6031X00_CONF0_AF,
+				 FIELD_PREP(VEML6031X00_CONF0_AF, state));
+	if (ret)
+		goto err_disable_interrupt;
+
+	ret = veml6031x00_set_af_trig(data, state);
+	if (ret)
+		goto err_clear_af;
+
+	data->trig_en = state;
+
+	return 0;
+
+err_clear_af:
+	regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
+			   VEML6031X00_CONF0_AF,
+			   FIELD_PREP(VEML6031X00_CONF0_AF, !state));
+err_disable_interrupt:
+	veml6031x00_set_interrupt(data, !state);
+	return ret;
+}
+
 static const struct iio_buffer_setup_ops veml6031x00_buffer_setup_ops = {
 	.preenable = veml6031x00_buffer_preenable,
 	.postdisable = veml6031x00_buffer_postdisable,
 };
 
+static const struct iio_trigger_ops veml6031x00_trigger_ops = {
+	.set_trigger_state = veml6031x00_set_trigger_state,
+};
+
 static irqreturn_t veml6031x00_trig_handler(int irq, void *p)
 {
 	struct iio_poll_func *pf = p;
@@ -597,7 +969,8 @@ static int veml6031x00_hw_init(struct iio_dev *iio)
 {
 	struct veml6031x00_data *data = iio_priv(iio);
 	struct device *dev = data->dev;
-	int ret;
+	int ret, val;
+	__le16 reg;
 
 	/* Max resolution = 6.9632 lx/cnt for gain = 0.125 and IT = 3.125ms */
 	ret = devm_iio_init_iio_gts(dev, 6, 963200000,
@@ -609,6 +982,54 @@ static int veml6031x00_hw_init(struct iio_dev *iio)
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to init iio gts\n");
 
+	reg = 0;
+	ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WL_L, &reg, sizeof(reg));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set low threshold\n");
+
+	reg = cpu_to_le16(U16_MAX);
+	ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WH_L, &reg, sizeof(reg));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set high threshold\n");
+
+	ret = regmap_field_write(data->rf.int_en, 0);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(data->regmap, VEML6031X00_REG_INT, &val);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to clear interrupts\n");
+
+	return 0;
+}
+
+static int veml6031x00_setup_irq(struct i2c_client *i2c, struct iio_dev *iio)
+{
+	struct veml6031x00_data *data = iio_priv(iio);
+	struct device *dev = data->dev;
+	int ret;
+
+	data->trig = devm_iio_trigger_alloc(dev, "%s-drdy%d", iio->name,
+					    iio_device_id(iio));
+	if (!data->trig)
+		return -ENOMEM;
+
+	data->trig->ops = &veml6031x00_trigger_ops;
+	iio_trigger_set_drvdata(data->trig, iio);
+
+	ret = devm_iio_trigger_register(dev, data->trig);
+	if (ret)
+		return ret;
+
+	iio->trig = iio_trigger_get(data->trig);
+	ret = devm_request_threaded_irq(dev, i2c->irq, NULL,
+					veml6031x00_interrupt,
+					IRQF_ONESHOT,
+					iio->name, iio);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to request irq %d\n",
+				     i2c->irq);
+
 	return 0;
 }
 
@@ -638,6 +1059,10 @@ static int veml6031x00_probe(struct i2c_client *i2c)
 	if (ret)
 		return ret;
 
+	ret = devm_mutex_init(dev, &data->irq_lock);
+	if (ret)
+		return ret;
+
 	ret = veml6031x00_regfield_init(data);
 	if (ret)
 		return dev_err_probe(dev, ret, "Failed to init regfield\n");
@@ -677,12 +1102,21 @@ static int veml6031x00_probe(struct i2c_client *i2c)
 	iio->channels = veml6031x00_channels;
 	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
 	iio->modes = INDIO_DIRECT_MODE;
-	iio->info = &veml6031x00_info;
 
 	ret = veml6031x00_hw_init(iio);
 	if (ret)
 		return ret;
 
+	if (i2c->irq) {
+		ret = veml6031x00_setup_irq(i2c, iio);
+		if (ret)
+			return ret;
+
+		iio->info = &veml6031x00_info;
+	} else {
+		iio->info = &veml6031x00_info_no_irq;
+	}
+
 	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
 					      veml6031x00_trig_handler,
 					      &veml6031x00_buffer_setup_ops);

-- 
2.43.0

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Runtime PM usage counter underflow due to unbalanced puts.
- [Medium] The IR intensity channel does not expose its available scales to userspace.
- [Medium] Missing lock in raw read path allows concurrent configuration changes to corrupt sensor readings.
--

commit 5e5c3dc820c07a34f2519b89a89cabf3c88212a1
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

iio: light: add support for veml6031x00 ALS series

These sensors provide two light channels (ALS and IR), I2C communication
and a multiplexed interrupt line to signal data ready and configurable
threshold alarms.

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> new file mode 100644
> index 0000000000000..6f9a7bad44d44
> --- /dev/null
> +++ b/drivers/iio/light/veml6031x00.c

[ ... ]

> +static const struct iio_chan_spec veml6031x00_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.address = VEML6031X00_REG_ALS_L,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +	{
> +		.type = IIO_INTENSITY,
> +		.address = VEML6031X00_REG_IR_L,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_LIGHT_IR,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +	},

[Severity: Medium]
Will userspace be able to discover the valid scale multipliers for the IR
channel?

The ALS channel includes BIT(IIO_CHAN_INFO_SCALE) in its
info_mask_separate_available bitmask, but it appears to be omitted here
for the IR channel.

Because of this omission, the IIO core won't create the
in_intensity_ir_scale_available sysfs attribute.

[ ... ]

> +static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
> +				   int *val)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int addr, it_usec, ret;
> +	__le16 reg;

[ ... ]

> +	ret = veml6031x00_get_it(data, &it_usec);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* integration time + 10 % to ensure completion */
> +	fsleep(it_usec + (it_usec / 10));

[Severity: Medium]
Can a concurrent sysfs write corrupt this sensor reading?

This read sequence sleeps here using fsleep() without holding a lock to
serialize the read against concurrent sysfs writes.

If a user thread concurrently modifies the integration time or scale via
veml6031x00_write_raw(), the hardware's measurement cycle might be reset
mid-measurement. When single_read() wakes up, it could read the data register
before the new measurement has finished, returning stale or corrupted data.

> +
> +	ret = regmap_bulk_read(data->regmap, addr, &reg, sizeof(reg));
> +	if (ret)
> +		return ret;

[ ... ]

> +static int veml6031x00_probe(struct i2c_client *i2c)
> +{

[ ... ]

> +	ret = devm_pm_runtime_set_active_enabled(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> +	ret = devm_pm_runtime_get_noresume(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get runtime PM\n");
> +
> +	ret = veml6031x00_validate_part_id(data);
> +	if (ret)
> +		return ret;

[ ... ]

> +	ret = veml6031x00_hw_init(iio);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_put_autosuspend(dev);

[Severity: High]
Could this lead to a Runtime PM usage count underflow on driver unbind?

Earlier in veml6031x00_probe(), the driver acquires a PM runtime reference
using the devres helper:

ret = devm_pm_runtime_get_noresume(dev);

This automatically registers a cleanup action to decrement the usage count upon
driver removal or probe failure. 

However, manually dropping the reference here with pm_runtime_put_autosuspend()
allows the device to autosuspend, but leaves the devres cleanup action queued.
When the driver is unbound (or if devm_iio_device_register() fails below),
the devres cleanup action will execute and decrement the usage counter again.

> +
> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531-veml6031x00-v4-0-e64f7fdce38d@gmail.com?part=2

Re: [PATCH v4 3/4] iio: light: veml6031x00: add support for triggered buffers

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Spurious I2C read of configuration register when the soft timestamp channel is enabled without both ALS and IR.
- [High] Uninitialized padding bytes in the `scan` structure leak kernel stack memory to user-space.

Pre-existing issues:
- [High] Mixed devm-managed PM runtime GET and manual autosuspend PUT causes the usage counter to underflow on driver unbind.
--

commit a608e278d687a935cb2bc7ba944ea15e822ef8ce
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

iio: light: veml6031x00: add support for triggered buffers

Add triggered buffer functionality for the two channels the device
provides (ALS and IR).

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> index 6f9a7bad44d44..facb1b8e42418 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c
[ ... ]
> @@ -469,6 +500,82 @@ static const struct iio_info veml6031x00_info = {
[ ... ]
> +static irqreturn_t veml6031x00_trig_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio = pf->indio_dev;
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ch, ret, i = 0;
> +	struct {
> +		__le16 chans[2];
> +		aligned_s64 timestamp;
> +	} scan = { };

[Severity: High]
Does this initialization leave compiler padding bytes uninitialized?

The scan struct has 4 bytes of compiler padding between the chans array (4
bytes) and the timestamp field (aligned to 8 bytes). Using = { }
zero-initializes the defined fields but leaves the padding bytes uninitialized
in C.

Since iio_push_to_buffers_with_ts() copies the entire 16-byte struct to the
IIO ring buffer, could this leak uninitialized kernel stack padding to
user-space? An explicit memset(&scan, 0, sizeof(scan)) is typically used to
prevent this.

> +
> +	if (test_bit(VEML6031X00_SCAN_ALS, iio->active_scan_mask) &&
> +	    test_bit(VEML6031X00_SCAN_IR, iio->active_scan_mask)) {
> +		ret = regmap_bulk_read(data->regmap,
> +				       VEML6031X00_REG_ALS_L,
> +				       &scan.chans, sizeof(scan.chans));
> +		if (ret)
> +			goto done;
> +	} else {
> +		iio_for_each_active_channel(iio, ch) {
> +			ret = regmap_bulk_read(data->regmap,
> +					       iio->channels[ch].address,
> +					       &scan.chans[i++],
> +					       sizeof(*scan.chans));

[Severity: High]
Does this loop fail to skip the software-only timestamp channel?

The iio_for_each_active_channel() macro iterates over all set bits in
active_scan_mask, including the timestamp channel bit
(VEML6031X00_SCAN_TIMESTAMP). 

Because the timestamp channel doesn't have an address,
iio->channels[ch].address will default to 0x00 (VEML6031X00_REG_CONF0).

Will this result in a spurious I2C read from the configuration register when
the soft timestamp channel is enabled without both ALS and IR, wasting I2C
bandwidth and populating the data array with unintended hardware data?

> +			if (ret)
> +				goto done;
> +		}
> +	}
> +
> +	iio_push_to_buffers_with_ts(iio, &scan, sizeof(scan), pf->timestamp);
> +
> +done:
> +	iio_trigger_notify_done(iio->trig);
> +
> +	return IRQ_HANDLED;
> +}
[ ... ]
> @@ -576,6 +683,13 @@ static int veml6031x00_probe(struct i2c_client *i2c)
[ ... ]
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
> +					      veml6031x00_trig_handler,
> +					      &veml6031x00_buffer_setup_ops);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register triggered buffer\n");
> +
>  	pm_runtime_put_autosuspend(dev);

[Severity: High]
This is a pre-existing issue, but does mixing devm-managed PM runtime GET and
manual autosuspend PUT cause the usage counter to underflow on driver unbind?

Earlier in veml6031x00_probe(), devm_pm_runtime_get_noresume(dev)
increments the PM usage counter and queues a devres action to decrement it on
module unbind.

At the end of the probe function, the driver manually calls
pm_runtime_put_autosuspend(dev), decrementing the usage counter back to 0.

Upon driver unbind, the queued devres action will execute, dropping the usage
counter a second time. Could this underflow the PM usage counter to -1,
causing a WARN splat and corrupting the device's PM state? Should this be
using the non-devm pm_runtime_get_noresume(dev) if it manually drops the
reference in probe?

>  
>  	ret = devm_iio_device_register(dev, iio);
>  	if (ret)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531-veml6031x00-v4-0-e64f7fdce38d@gmail.com?part=3

Re: [PATCH v4 4/4] iio: light: veml6031x00: add support for events and trigger

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [High] Missing `.validate_device` in trigger operations allows other devices to attach to a sensor-specific trigger.
- [High] The trigger handler performs an unintended I2C read from address 0 for the software timestamp channel.
- [High] PM runtime usage count is leaked if the device is unbound while events are enabled.
- [High] Stale values in the regmap cache for the hardware-cleared `AF_TRIG` bit cause unintended triggers.
- [Medium] Mixing `goto` error handling with `guard()` scope-based cleanup in the same function.
- [Medium] Non-functional event sysfs attributes are exposed to userspace when the device lacks an IRQ.

Pre-existing issues:
- [High] Unbalanced PM runtime usage counter due to mixing devm action with manual put.
--

commit c0654b1fbfc516c85d27a7c169844b649c2a1077
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>

iio: light: veml6031x00: add support for events and trigger
    
The device provides a shared interrupt line for to notify events and
data ready, which can be used as a trigger. The interrupt line is not a
requirement for the device to work. Implement variants for the cases
whether the interrupt line is provided or not.

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> index facb1b8e42418..5e79eb942f09c 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c

[ ... ]

> @@ -144,6 +197,8 @@ static const struct iio_chan_spec veml6031x00_channels[] = {
>  		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
>  		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
>  		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
> +		.event_spec = veml6031x00_event_spec,
> +		.num_event_specs = ARRAY_SIZE(veml6031x00_event_spec),

[Severity: Medium]
Will this unconditionally expose event sysfs attributes even when the device
lacks an IRQ? 

If i2c->irq is 0, the driver uses veml6031x00_info_no_irq to omit event
callbacks. However, the IIO core checks num_event_specs during
iio_check_for_dynamic_events() and creates sysfs attributes anyway. If these
attributes are accessed from userspace without the callbacks, will they
return -EINVAL?

[ ... ]

> +static int veml6031x00_set_af_trig(struct veml6031x00_data *data, bool state)
> +{
> +	regcache_drop_region(data->regmap, VEML6031X00_REG_CONF0,
> +			     VEML6031X00_REG_CONF0);
> +
> +	return regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				  VEML6031X00_CONF0_AF_TRIG,
> +				  FIELD_PREP(VEML6031X00_CONF0_AF_TRIG, state));
> +}

[Severity: High]
Does this leave a stale AF_TRIG value in the non-volatile regmap cache?

The AF_TRIG bit is cleared automatically by hardware. While this function
drops the cache region before setting the bit, VEML6031X00_REG_CONF0 is not
marked as volatile in veml6031x00_volatile_ranges. Because of this,
regmap_update_bits() writes the new value to hardware and also caches it. 

Would any subsequent updates to CONF0 (such as changing AF or SD bits) read
the cached AF_TRIG=1 and write it back, causing an unintended measurement
trigger?

[ ... ]

> +static int veml6031x00_write_event_config(struct iio_dev *iio,
> +					  const struct iio_chan_spec *chan,
> +					  enum iio_event_type type,
> +					  enum iio_event_direction dir,
> +					  bool state)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret;
> +
> +	guard(mutex)(&data->irq_lock);
> +
> +	/* avoid multiple increments/decrements from one source */
> +	if (state == data->ev_en)
> +		return 0;
> +
> +	if (state) {
> +		ret = pm_runtime_resume_and_get(data->dev);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
Is the PM runtime usage count leaked if the device is unbound while events
are enabled? 

The pm_runtime_resume_and_get() call keeps the device powered on, but there
does not appear to be a custom devres action or remove callback to disable
the event and drop this reference if the driver is unbound. Could this
prevent the parent I2C controller from ever entering runtime suspend?

[ ... ]

> +static int veml6031x00_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *iio = iio_trigger_get_drvdata(trig);
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret;
> +
> +	guard(mutex)(&data->irq_lock);
> +
> +	if (state == data->trig_en)
> +		return 0;
> +
> +	ret = veml6031x00_set_interrupt(data, state);
> +	if (ret)
> +		return ret;
> +
> +	/* The AF bit must be set before setting AF_TRIG */
> +	ret = regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				 VEML6031X00_CONF0_AF,
> +				 FIELD_PREP(VEML6031X00_CONF0_AF, state));
> +	if (ret)
> +		goto err_disable_interrupt;
> +
> +	ret = veml6031x00_set_af_trig(data, state);
> +	if (ret)
> +		goto err_clear_af;

[Severity: Medium]
Is it safe to mix goto-based error handling with guard() scope-based cleanup
in this function? 

According to the cleanup subsystem guidelines, mixing goto with guard() or
__free() in the same function can create confusing ownership semantics.
Should this function either use goto entirely or be fully converted to
scoped cleanups?

[ ... ]

> +static const struct iio_trigger_ops veml6031x00_trigger_ops = {
> +	.set_trigger_state = veml6031x00_set_trigger_state,
> +};

[Severity: High]
Is a .validate_device callback needed here to prevent other devices from
attaching to this sensor-specific trigger? 

Without setting .validate_device to iio_trigger_validate_own_device, another
IIO device could attach to it. If that happens, veml6031x00_set_trigger_state()
would enable the DRDY interrupt while the sensor is PM runtime suspended
(since its own buffer_preenable isn't called). Would this cause the dependent
device's buffer captures to hang?

[ ... ]

> +static irqreturn_t veml6031x00_trig_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio = pf->indio_dev;
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ch, ret, i = 0;
> +	struct {
> +		__le16 chans[2];
> +		aligned_s64 timestamp;
> +	} scan = { };
> +
> +	if (test_bit(VEML6031X00_SCAN_ALS, iio->active_scan_mask) &&
> +	    test_bit(VEML6031X00_SCAN_IR, iio->active_scan_mask)) {
> +		ret = regmap_bulk_read(data->regmap,
> +				       VEML6031X00_REG_ALS_L,
> +				       &scan.chans, sizeof(scan.chans));
> +		if (ret)
> +			goto done;
> +	} else {
> +		iio_for_each_active_channel(iio, ch) {
> +			ret = regmap_bulk_read(data->regmap,
> +					       iio->channels[ch].address,
> +					       &scan.chans[i++],
> +					       sizeof(*scan.chans));

[Severity: High]
Does this loop inadvertently perform an I2C read from address 0 for the
software timestamp channel? 

When IIO_CHAN_SOFT_TIMESTAMP is active and the fast-path isn't taken, it
iterates over all active channels. Since the timestamp channel does not define
a hardware address, its .address is 0. 

Would regmap_bulk_read() read VEML6031X00_REG_CONF0 instead of data,
wasting I2C bandwidth and writing the configuration register's value into
the scan buffer just before the timestamp?

[ ... ]

> @@ -677,12 +1102,21 @@ static int veml6031x00_probe(struct i2c_client *i2c)
>  	iio->channels = veml6031x00_channels;
>  	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
>  	iio->modes = INDIO_DIRECT_MODE;
>  
>  	ret = veml6031x00_hw_init(iio);
>  	if (ret)
>  		return ret;
>  
> +	if (i2c->irq) {
> +		ret = veml6031x00_setup_irq(i2c, iio);
> +		if (ret)
> +			return ret;
> +
> +		iio->info = &veml6031x00_info;
> +	} else {
> +		iio->info = &veml6031x00_info_no_irq;
> +	}
> +
>  	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
>  					      veml6031x00_trig_handler,
>  					      &veml6031x00_buffer_setup_ops);

[Severity: High]
This isn't a bug introduced by this patch, but is the PM runtime usage
counter unbalanced at the end of veml6031x00_probe()?

The driver acquires a reference using devm_pm_runtime_get_noresume() which
associates a devres cleanup action. If pm_runtime_put_autosuspend() is
called manually at the end of probe (immediately after this buffer setup),
the devres cleanup will still fire when the device is unbound, performing a
second put. Could this cause the PM runtime usage counter to underflow
to -1?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260531-veml6031x00-v4-0-e64f7fdce38d@gmail.com?part=4

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Jonathan Cameron]

On Sun, 31 May 2026 21:58:22 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> These sensors provide two light channels (ALS and IR), I2C communication
> and a multiplexed interrupt line to signal data ready and configurable
> threshold alarms.
> 
> This first implementation provides basic functionality (measurement
> configuration, raw reads and ID validation) and defines the different
> register regions in preparation for extended features in the subsequent
> patches of the series.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
A few comments from sashiko and a couple from me based on a fresh read.


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

> +
> +static const struct iio_chan_spec veml6031x00_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +		.address = VEML6031X00_REG_ALS_L,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
> +	},
> +	{
> +		.type = IIO_INTENSITY,
> +		.address = VEML6031X00_REG_IR_L,
> +		.modified = 1,
> +		.channel2 = IIO_MOD_LIGHT_IR,
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),

Can we move scale to shared_by_type?

Thinking on some of the others, they should probably be shared_by_type as well
rather than shared_by_all. If we ever add buffered support and a timestamp
the integration_time doesn't apply to that. 

shared_by_all tends to only include things that are truely universal like
sampling_frequency.


> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> +	},
> +};

> +
> +static int veml6031x00_get_it(struct veml6031x00_data *data, int *val2)
> +{
> +	int ret, it_idx;
> +
> +	scoped_guard(mutex, &data->scale_lock) {

Given below I suggest you need an unlocked variant of this, maybe
think about whether we care if the scope includes the table search.

I'd just take the lock for the whole function.

> +		ret = regmap_field_read(data->rf.it, &it_idx);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = iio_gts_find_int_time_by_sel(&data->gts, it_idx);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val2 = ret;
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}

> +
> +static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
> +				   int *val)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int addr, it_usec, ret;
> +	__le16 reg;
> +
> +	switch (type) {
> +	case IIO_LIGHT:
> +		addr = VEML6031X00_REG_ALS_L;
> +		break;
> +	case IIO_INTENSITY:
> +		addr = VEML6031X00_REG_IR_L;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(data->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	ret = veml6031x00_get_it(data, &it_usec);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* integration time + 10 % to ensure completion */
> +	fsleep(it_usec + (it_usec / 10));
> +
> +	ret = regmap_bulk_read(data->regmap, addr, &reg, sizeof(reg));
> +	if (ret)
> +		return ret;

https://sashiko.dev/#/patchset/20260531-veml6031x00-v4-0-e64f7fdce38d%40gmail.com

Hmm. Sashiko shouts race here. It is kind of correct in that we don't know
if we have a matching pair of integration time and reading. Thus we might
have
Thread 1                           Thread 2
Power on.                           
Get small integration time           
                                   Set large integration time
read before new integration done.

Whether this is bug kind of depends on the device when the integration time
is updated.  Does it finish current integration with old one, or does it just
update some register against which a comparator is running. So if we are
already integrating, do we just integrate for longer before stopping?

I haven't checked but this smells like kind of thing a datasheet won't tell
us.  Hence I'd be tempted to throw use of data->mutex to serialize single
reads and integration time updates + anything related to that).  It's
harmless at worst and makes it easier to reason about this code.
You'll need an unlocked  __veml6031x00_get_it() variant to call though
given that takes the same lock. 



 


> +
> +	*val = le16_to_cpu(reg);
> +	return IIO_VAL_INT;
> +}

> +
> +static int veml6031x00_validate_part_id(struct veml6031x00_data *data)
> +{
> +	int part_id, ret;
> +	__le16 reg;
> +
> +	ret = regmap_bulk_read(data->regmap, VEML6031X00_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 != data->chip->part_id)
> +		dev_warn(data->dev, "Unknown ID %04x\n", part_id);

Maybe relax to dev_info() given we expect this to happen if fallback
compatibles get used in future. Warn is a little strong.

> +
> +	return 0;
> +}

> +static int veml6031x00_probe(struct i2c_client *i2c)
> +{

> +	pm_runtime_set_autosuspend_delay(dev, 2000);
> +	pm_runtime_use_autosuspend(dev);
> +	ret = devm_pm_runtime_set_active_enabled(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> +	ret = devm_pm_runtime_get_noresume(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get runtime PM\n");
> +
> +	ret = veml6031x00_validate_part_id(data);
> +	if (ret)
> +		return ret;
> +
> +	iio->name = data->chip->name;
> +	iio->channels = veml6031x00_channels;
> +	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->info = &veml6031x00_info;
This block doesn't need to happen in the runtime pm region.
> +
> +	ret = veml6031x00_hw_init(iio);
> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_put_autosuspend(dev);

As sashiko shouts, this looks like runtime pm will underflow on remove.
Check it by removing your driver.  It doesn't actually result in any 
problem, as the runtime pm subsystem just saturates at 0 on decrement.
Given that's tear down anyway maybe we don't care.  However, it's easy
enough to fix by using pm_runtime_get_no_resume() and a couple of
explicit calls to put it in error paths.





> +
> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
> +
> +	return 0;
> +}

> 

Re: [PATCH v4 3/4] iio: light: veml6031x00: add support for triggered buffers

[Jonathan Cameron]

On Sun, 31 May 2026 21:58:23 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> Add triggered buffer functionality for the two channels the device
> provides (ALS and IR).
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>

One trivial thing from me. I think all the feedback remaining on this one
from Sashiko is false positives.

> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> index 6f9a7bad44d4..facb1b8e4241 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c

>  
> +static int veml6031x00_buffer_preenable(struct iio_dev *iio)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret, it_usec;
> +
> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = veml6031x00_get_it(data, &it_usec);
> +	if (ret < 0) {
> +		pm_runtime_put_autosuspend(data->dev);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Wait one integration period + 10% margin so the first triggered
> +	 * read does not race with the sensor completing its first conversion
> +	 * after power-on.
> +	 */
> +	fsleep(it_usec + (it_usec / 10));
> +
> +	return 0;
> +}

> +
> +static irqreturn_t veml6031x00_trig_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio = pf->indio_dev;
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ch, ret, i = 0;
> +	struct {
> +		__le16 chans[2];
> +		aligned_s64 timestamp;
> +	} scan = { };

In case anyone wonders about the sashiko feedback, this is fine. The kernel
is carefully built with options (plus self tests) to ensure that padding is initialized
by doing this.  Sashiko is correct that the C spec (until recently?) doesn't require
that to be the case.	

> +
> +	if (test_bit(VEML6031X00_SCAN_ALS, iio->active_scan_mask) &&
> +	    test_bit(VEML6031X00_SCAN_IR, iio->active_scan_mask)) {
> +		ret = regmap_bulk_read(data->regmap,
> +				       VEML6031X00_REG_ALS_L,
> +				       &scan.chans, sizeof(scan.chans));
> +		if (ret)
> +			goto done;
> +	} else {
> +		iio_for_each_active_channel(iio, ch) {
> +			ret = regmap_bulk_read(data->regmap,
> +					       iio->channels[ch].address,
> +					       &scan.chans[i++],
> +					       sizeof(*scan.chans));
> +			if (ret)
> +				goto done;
> +		}
> +	}
> +
> +	iio_push_to_buffers_with_ts(iio, &scan, sizeof(scan), pf->timestamp);
> +
> +done:
> +	iio_trigger_notify_done(iio->trig);
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int veml6031x00_validate_part_id(struct veml6031x00_data *data)
>  {
>  	int part_id, ret;
> @@ -576,6 +683,13 @@ static int veml6031x00_probe(struct i2c_client *i2c)
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
> +					      veml6031x00_trig_handler,
> +					      &veml6031x00_buffer_setup_ops);

Why is this in the region in which the device is forced to be powered up?  

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register triggered buffer\n");
> +
>  	pm_runtime_put_autosuspend(dev);

I would have thought here was fine.

>  
>  	ret = devm_iio_device_register(dev, iio);
> 

Re: [PATCH v4 4/4] iio: light: veml6031x00: add support for events and trigger

[Jonathan Cameron]

On Sun, 31 May 2026 21:58:24 +0200
Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:

> The device provides a shared interrupt line for to notify events and
> data ready, which can be used as a trigger. The interrupt line is not a
> requirement for the device to work. Implement variants for the cases
> whether the interrupt line is provided or not.
> 
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
A few more things inline. Some from sashiko.

> +static int veml6031x00_write_event_config(struct iio_dev *iio,
> +					  const struct iio_chan_spec *chan,
> +					  enum iio_event_type type,
> +					  enum iio_event_direction dir,
> +					  bool state)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret;
> +
> +	guard(mutex)(&data->irq_lock);
> +
> +	/* avoid multiple increments/decrements from one source */
> +	if (state == data->ev_en)
> +		return 0;
> +
> +	if (state) {
> +		ret = pm_runtime_resume_and_get(data->dev);

Sashiko points out that, unlike buffered capture there is no core
based disabling of events in remove path.  Hence if events are on
we may not get the runtime pm put that we need to actually power down
the device and the stuff above it in the system.  You need
to do your own tracking for whether it's needed or not.

For background, we can't do a general disable from the IIO core because
the core doesn't actually know what events are enabled as we don't model
the complex interactions that can occur between different events - i.e.
enabling one can disable another.

> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = veml6031x00_set_interrupt(data, state);
> +	if (ret) {
> +		if (state)
> +			pm_runtime_put_autosuspend(data->dev);
> +		return ret;
> +	}
> +
> +	data->ev_en = state;
> +
> +	if (!state)
> +		pm_runtime_put_autosuspend(data->dev);
> +
> +	return 0;
> +}

>  
> +/* AF_TRIG is reset by hardware, but the rest of the fields are persistent */

https://sashiko.dev/#/patchset/20260531-veml6031x00-v4-0-e64f7fdce38d%40gmail.com
comment seems valid to me. I think you have to treat this register as volatile
and rewrite the whole thing every time. Maybe a reorder would work. Drop the region
after the update.  However then next access has to do a read so maybe just
do your own sub register caching for this one.


> +static int veml6031x00_set_af_trig(struct veml6031x00_data *data, bool state)
> +{
> +	regcache_drop_region(data->regmap, VEML6031X00_REG_CONF0,
> +			     VEML6031X00_REG_CONF0);
> +
> +	return regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				  VEML6031X00_CONF0_AF_TRIG,
> +				  FIELD_PREP(VEML6031X00_CONF0_AF_TRIG, state));
> +}
> +
> +static irqreturn_t veml6031x00_interrupt(int irq, void *private)
> +{
> +	struct iio_dev *iio = private;
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	s64 timestamp;
> +	int ret, reg;

regval. Reg is often used for register address.

> +
> +	ret = regmap_read(data->regmap, VEML6031X00_REG_INT, &reg);
> +	if (ret) {
> +		dev_err(data->dev,
> +			"Failed to read interrupt register %d\n", ret);
> +		return IRQ_NONE;
> +	}
> +
> +	if (!(reg & VEML6031X00_INT_MASK))
> +		return IRQ_NONE;
> +
> +	guard(mutex)(&data->irq_lock);

I'm not really sure what this lock is protecting against.  It doesn't cover
the register read above, so the rest may be based on stale value of reg.
Perhaps some comments?

> +
> +	if ((reg & (VEML6031X00_INT_TH_H | VEML6031X00_INT_TH_L)) && data->ev_en) {
> +		timestamp = iio_get_time_ns(iio);
> +
> +		if (reg & VEML6031X00_INT_TH_H)
> +			iio_push_event(iio, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +								 IIO_EV_TYPE_THRESH,
> +								 IIO_EV_DIR_RISING),
> +				       timestamp);
> +		if (reg & VEML6031X00_INT_TH_L)
> +			iio_push_event(iio, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +								 IIO_EV_TYPE_THRESH,
> +								 IIO_EV_DIR_FALLING),
> +				       timestamp);
> +	}
> +
> +	if ((reg & VEML6031X00_INT_DRDY) && data->trig_en) {
> +		iio_trigger_poll_nested(data->trig);
> +		ret = veml6031x00_set_af_trig(data, true);

This superficially seems like something that maybe belongs in the trigger
reenable callback?  That will get called in much the same place, but has
the advantage that it documents this is about reenabling that trigger.

> +		if (ret)
> +			dev_err(data->dev, "Failed to set trigger %d\n", ret);
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
>  static int veml6031x00_buffer_preenable(struct iio_dev *iio)
>  {
>  	struct veml6031x00_data *data = iio_priv(iio);
> @@ -534,11 +863,54 @@ static int veml6031x00_buffer_postdisable(struct iio_dev *iio)
>  	return 0;
>  }
>  
> +static int veml6031x00_set_trigger_state(struct iio_trigger *trig, bool state)
> +{
> +	struct iio_dev *iio = iio_trigger_get_drvdata(trig);
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret;
> +
> +	guard(mutex)(&data->irq_lock);

Correct report form sashiko here. You should not be mixing guard and gotos.
It's fine to just use traditional mutex_lock / unlock in some
functions where guard() is not appropriate.

Note i don't think there is a bug here but this pattern is considered
fragile as future code might see the gotos and add some more and one of
those might cross the declaration hidden in guard()


> +
> +	if (state == data->trig_en)
> +		return 0;
> +
> +	ret = veml6031x00_set_interrupt(data, state);
> +	if (ret)
> +		return ret;
> +
> +	/* The AF bit must be set before setting AF_TRIG */
> +	ret = regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +				 VEML6031X00_CONF0_AF,
> +				 FIELD_PREP(VEML6031X00_CONF0_AF, state));
> +	if (ret)
> +		goto err_disable_interrupt;
> +
> +	ret = veml6031x00_set_af_trig(data, state);
> +	if (ret)
> +		goto err_clear_af;
> +
> +	data->trig_en = state;
> +
> +	return 0;
> +
> +err_clear_af:
> +	regmap_update_bits(data->regmap, VEML6031X00_REG_CONF0,
> +			   VEML6031X00_CONF0_AF,
> +			   FIELD_PREP(VEML6031X00_CONF0_AF, !state));
> +err_disable_interrupt:
> +	veml6031x00_set_interrupt(data, !state);
> +	return ret;
> +}
> +
>  static const struct iio_buffer_setup_ops veml6031x00_buffer_setup_ops = {
>  	.preenable = veml6031x00_buffer_preenable,
>  	.postdisable = veml6031x00_buffer_postdisable,
>  };
>  
> +static const struct iio_trigger_ops veml6031x00_trigger_ops = {
> +	.set_trigger_state = veml6031x00_set_trigger_state,
Another plausible sashiko comment. Is this trigger usable by another device?
That in theory at least includes when this device is not using it.  Might
be easier to set the validation callback to restrict it to this device.

> +};


>  
> @@ -638,6 +1059,10 @@ static int veml6031x00_probe(struct i2c_client *i2c)
>  	if (ret)
>  		return ret;
>  
> +	ret = devm_mutex_init(dev, &data->irq_lock);
> +	if (ret)
> +		return ret;
> +
>  	ret = veml6031x00_regfield_init(data);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "Failed to init regfield\n");
> @@ -677,12 +1102,21 @@ static int veml6031x00_probe(struct i2c_client *i2c)
>  	iio->channels = veml6031x00_channels;
>  	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);

Sashiko caught that you need separate channel definitions as well for the
no irq vs irq cases.  Otherwise a bunch of sysfs files that don't do anything
will be created.

>  	iio->modes = INDIO_DIRECT_MODE;
> -	iio->info = &veml6031x00_info;
>  
>  	ret = veml6031x00_hw_init(iio);
>  	if (ret)
>  		return ret;
>  
> +	if (i2c->irq) {
> +		ret = veml6031x00_setup_irq(i2c, iio);
> +		if (ret)
> +			return ret;
> +
> +		iio->info = &veml6031x00_info;
> +	} else {
> +		iio->info = &veml6031x00_info_no_irq;
> +	}
> +
>  	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
>  					      veml6031x00_trig_handler,
>  					      &veml6031x00_buffer_setup_ops);
> 

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Javier Carrasco]

Hi Jonathan, thanks again for your review.

On Mon Jun 1, 2026 at 12:21 PM CEST, Jonathan Cameron wrote:
> On Sun, 31 May 2026 21:58:22 +0200
> Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
>
>> These sensors provide two light channels (ALS and IR), I2C communication
>> and a multiplexed interrupt line to signal data ready and configurable
>> threshold alarms.
>>
>> This first implementation provides basic functionality (measurement
>> configuration, raw reads and ID validation) and defines the different
>> register regions in preparation for extended features in the subsequent
>> patches of the series.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> A few comments from sashiko and a couple from me based on a fresh read.
>
>
>> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
>> new file mode 100644
>> index 000000000000..6f9a7bad44d4
>> --- /dev/null
>> +++ b/drivers/iio/light/veml6031x00.c
>
>> +
>> +static const struct iio_chan_spec veml6031x00_channels[] = {
>> +	{
>> +		.type = IIO_LIGHT,
>> +		.address = VEML6031X00_REG_ALS_L,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +				      BIT(IIO_CHAN_INFO_SCALE),
>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
>> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
>> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
>> +	},
>> +	{
>> +		.type = IIO_INTENSITY,
>> +		.address = VEML6031X00_REG_IR_L,
>> +		.modified = 1,
>> +		.channel2 = IIO_MOD_LIGHT_IR,
>> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> +				      BIT(IIO_CHAN_INFO_SCALE),
>
> Can we move scale to shared_by_type?
>
> Thinking on some of the others, they should probably be shared_by_type as well
> rather than shared_by_all. If we ever add buffered support and a timestamp
> the integration_time doesn't apply to that.
>
> shared_by_all tends to only include things that are truely universal like
> sampling_frequency.
>

I am not sure if I get this point. This device has a single IIO_LIGHT
channel, and the scale only applies to it. Are info_mask_separate and
info_mask_shared_by_type not the same in that case? I have seen that
some drivers use both info_mask_separate for INFO_RAW, and
info_mask_shared_by_type for INFO_INT_TIME and/or INFO_SCALE, but that
could make more sense if there were multiple channels of the same type.
What am I missing here?

On the other hand, the integration time applies to both the IIO_LIGHT
and IIO_INTENSITY channels, so I guess you are suggesting to add it
to both channels as info_mask_shared_by_type because the timestamp
is a channel itself. Moving it form shared_by_all to shared_by_type is
alright, and I will add it to V5.

>> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
>> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
>> +	},
>> +};
>
>> +
>> +static int veml6031x00_get_it(struct veml6031x00_data *data, int *val2)
>> +{
>> +	int ret, it_idx;
>> +
>> +	scoped_guard(mutex, &data->scale_lock) {
>
> Given below I suggest you need an unlocked variant of this, maybe
> think about whether we care if the scope includes the table search.
>
> I'd just take the lock for the whole function.
>
>> +		ret = regmap_field_read(data->rf.it, &it_idx);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	ret = iio_gts_find_int_time_by_sel(&data->gts, it_idx);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*val2 = ret;
>> +
>> +	return IIO_VAL_INT_PLUS_MICRO;
>> +}
>
>> +
>> +static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
>> +				   int *val)
>> +{
>> +	struct veml6031x00_data *data = iio_priv(iio);
>> +	int addr, it_usec, ret;
>> +	__le16 reg;
>> +
>> +	switch (type) {
>> +	case IIO_LIGHT:
>> +		addr = VEML6031X00_REG_ALS_L;
>> +		break;
>> +	case IIO_INTENSITY:
>> +		addr = VEML6031X00_REG_IR_L;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(data->dev, pm);
>> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = veml6031x00_get_it(data, &it_usec);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* integration time + 10 % to ensure completion */
>> +	fsleep(it_usec + (it_usec / 10));
>> +
>> +	ret = regmap_bulk_read(data->regmap, addr, &reg, sizeof(reg));
>> +	if (ret)
>> +		return ret;
>
> https://sashiko.dev/#/patchset/20260531-veml6031x00-v4-0-e64f7fdce38d%40gmail.com
>
> Hmm. Sashiko shouts race here. It is kind of correct in that we don't know
> if we have a matching pair of integration time and reading. Thus we might
> have
> Thread 1                           Thread 2
> Power on.
> Get small integration time
>                                    Set large integration time
> read before new integration done.
>
> Whether this is bug kind of depends on the device when the integration time
> is updated.  Does it finish current integration with old one, or does it just
> update some register against which a comparator is running. So if we are
> already integrating, do we just integrate for longer before stopping?
>
> I haven't checked but this smells like kind of thing a datasheet won't tell
> us.  Hence I'd be tempted to throw use of data->mutex to serialize single
> reads and integration time updates + anything related to that).  It's
> harmless at worst and makes it easier to reason about this code.
> You'll need an unlocked  __veml6031x00_get_it() variant to call though
> given that takes the same lock.
>

You are right assuming that the datasheet will not give us an answer
about that. I suspect by my measurements that the measurement is carried
out with the first integration time, which would make this issue
harmless. But as I am not 100% sure, I contacted the manufacturer to
have a reliable answer. I will wait for a few days while I fix all the
stuff for V5, and if I don't get an answer, or the new integration time
is used, I will use the mutexes as you suggested. I don't like using
mutexes to protect sections of code "just in case" without a real reason
behind, but if there is no way to know, then it will have to be that
way...

>> +
>> +	*val = le16_to_cpu(reg);
>> +	return IIO_VAL_INT;
>> +}
>
>> +
>> +static int veml6031x00_validate_part_id(struct veml6031x00_data *data)
>> +{
>> +	int part_id, ret;
>> +	__le16 reg;
>> +
>> +	ret = regmap_bulk_read(data->regmap, VEML6031X00_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 != data->chip->part_id)
>> +		dev_warn(data->dev, "Unknown ID %04x\n", part_id);
>
> Maybe relax to dev_info() given we expect this to happen if fallback
> compatibles get used in future. Warn is a little strong.
>

Ack.

>> +
>> +	return 0;ä
>> +}
>
>> +static int veml6031x00_probe(struct i2c_client *i2c)
>> +{
>
>> +	pm_runtime_set_autosuspend_delay(dev, 2000);
>> +	pm_runtime_use_autosuspend(dev);
>> +	ret = devm_pm_runtime_set_active_enabled(dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
>> +
>> +	ret = devm_pm_runtime_get_noresume(dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to get runtime PM\n");
>> +
>> +	ret = veml6031x00_validate_part_id(data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	iio->name = data->chip->name;
>> +	iio->channels = veml6031x00_channels;
>> +	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
>> +	iio->modes = INDIO_DIRECT_MODE;
>> +	iio->info = &veml6031x00_info;
> This block doesn't need to happen in the runtime pm region.

Ack.

>> +
>> +	ret = veml6031x00_hw_init(iio);
>> +	if (ret)
>> +		return ret;
>> +
>> +	pm_runtime_put_autosuspend(dev);
>
> As sashiko shouts, this looks like runtime pm will underflow on remove.
> Check it by removing your driver.  It doesn't actually result in any
> problem, as the runtime pm subsystem just saturates at 0 on decrement.
> Given that's tear down anyway maybe we don't care.  However, it's easy
> enough to fix by using pm_runtime_get_no_resume() and a couple of
> explicit calls to put it in error paths.
>

I took some time to audit this in detail, because although my
expectations are that atomic_add_unless(usage_count, -1, 0) should make
this shout a false positive. Expectations don't always meet reality. My
expectations were based on the code, so I have added tracepoints to know
exactly what's going on.

Scenario 1: Successful probe -> unbind driver.

devm_pm_runtime_get_noresume() increases usage_count, and the call to
pm_runtime_put_autosuspend() decreases it. That is balanced, giving us
usage_count = 0 and putting the device in power down mode as desired. If
the device is then unbound, the devres action (a call to
pm_runtime_put_noidle_action, which only calls pm_runtime_put_noidle)
triggers a call to atomic_add_unless(&dev->power.usage_count, -1, 0).
Given that the usage count is 0, nothing happens and there is no
underflow. In fact, the underflow is not possible this way, and adding a
remove function to check if usage_count is 0 and calling
pm_runtime_put() is basically repeating what the devres action already
offers for free.

Scenario 2: write_event_config -> unbind driver.

Sashiko says that pm_runtime_resume_and_get() increases usage_count, and
there is no devres action associated to it. That is only partially
correct, because the devres action from devm_pm_runtime_get_noresume()
will be triggered when the driver is unbound. Actually, this is great
because then pm_runtime_resume_and_get() gets balanced, and there is no
need for a remove function to check again if usage_count is 0 or not. In
this case, usage_count = 1 before unbinding the driver, and then the
devres action is triggered when it gets unbound. Exactly what we want to
have usage_count = 0.

>> +
>> +	ret = devm_iio_device_register(dev, iio);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
>> +
>> +	return 0;
>> +}
>
>>

Best regards,
Javier

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Andy Shevchenko]

On Sun, May 31, 2026 at 09:58:22PM +0200, Javier Carrasco wrote:
> These sensors provide two light channels (ALS and IR), I2C communication
> and a multiplexed interrupt line to signal data ready and configurable
> threshold alarms.
> 
> This first implementation provides basic functionality (measurement
> configuration, raw reads and ID validation) and defines the different
> register regions in preparation for extended features in the subsequent
> patches of the series.

...

+ array_size.h

> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/i2c.h>

> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>

In C locale it seems wrong order.

> +#include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>

+ types.h

> +#include <linux/units.h>

+ Blank line.

> +#include <linux/iio/iio.h>
> +#include <linux/iio/iio-gts-helper.h>

...

> +struct veml6031x00_data {

Have you run `pahole`? Does it agree with your layout?

> +	struct device *dev;
> +	struct iio_gts gts;
> +	struct regmap *regmap;

Do you need dev and regmap? One may be derived from the other in case regmap
uses the same dev on initialisation as the one stored here.

> +	struct veml6031x00_rf rf;
> +	const struct veml6031x00_chip *chip;
> +	/*
> +	 * Serialize access to scale register fields scattered across multiple
> +	 * registers (rf.gain, rf.pd_div4, rf.it) to read and write them as a
> +	 * consistent set.
> +	 */
> +	struct mutex scale_lock;
> +};

...

> +/*
> + * The gain selector encodes (PD_D4 << 2) | GAIN to identify each gain setting.
> + * Gains are multiplied by 8 to work with integers. The values in the iio-gts
> + * tables don't need corrections because the maximum value of the scale refers
> + * to GAIN = x1, and the rest of the values are obtained from the resulting
> + * linear function.
> + * TODO: add support for MILLI_GAIN_X165 and MILLI_GAIN_X660
> + */
> +#define VEML6031X00_SEL_MILLI_GAIN_X125  0x07
> +#define VEML6031X00_SEL_MILLI_GAIN_X250  0x04
> +#define VEML6031X00_SEL_MILLI_GAIN_X500  0x03
> +#define VEML6031X00_SEL_MILLI_GAIN_X1000 0x00
> +#define VEML6031X00_SEL_MILLI_GAIN_X2000 0x01

Not sure if these one-time use definitions improve or not the readability
of the code. Up to Jonathan.

> +static const struct iio_gain_sel_pair veml6031x00_gain_sel[] = {
> +	GAIN_SCALE_GAIN(1, VEML6031X00_SEL_MILLI_GAIN_X125),
> +	GAIN_SCALE_GAIN(2, VEML6031X00_SEL_MILLI_GAIN_X250),
> +	GAIN_SCALE_GAIN(4, VEML6031X00_SEL_MILLI_GAIN_X500),
> +	GAIN_SCALE_GAIN(8, VEML6031X00_SEL_MILLI_GAIN_X1000),
> +	GAIN_SCALE_GAIN(16, VEML6031X00_SEL_MILLI_GAIN_X2000),
> +};

...

> +{
> +	struct regmap *regmap = data->regmap;
> +	struct device *dev = data->dev;

In case you really need a 'dev' here, pass via function parameter, no need to
keep it in the 'data'.

> +	struct regmap_field *rm_field;
> +	struct veml6031x00_rf *rf = &data->rf;
> +
> +	rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_gain);
> +	if (IS_ERR(rm_field))
> +		return PTR_ERR(rm_field);
> +	rf->gain = rm_field;
> +
> +	rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_it);
> +	if (IS_ERR(rm_field))
> +		return PTR_ERR(rm_field);
> +	rf->it = rm_field;
> +
> +	rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_pd_div4);
> +	if (IS_ERR(rm_field))
> +		return PTR_ERR(rm_field);
> +	rf->pd_div4 = rm_field;
> +
> +	return 0;
> +}

...

> +static int veml6031x00_get_it(struct veml6031x00_data *data, int *val2)
> +{
> +	int ret, it_idx;

Why is 'it_idx' signed?

> +
> +	scoped_guard(mutex, &data->scale_lock) {
> +		ret = regmap_field_read(data->rf.it, &it_idx);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ret = iio_gts_find_int_time_by_sel(&data->gts, it_idx);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val2 = ret;
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}

...

> +static int veml6031x00_set_it(struct iio_dev *iio, int val, int val2)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret, gain_sel, gain_reg, pd_div4, it_idx, new_gain, prev_gain, prev_it;

Similar question here and so on...

> +	bool in_range;

> +}

...

> +	ret = regmap_field_write(data->rf.pd_div4, gain_sel >> 2);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_field_write(data->rf.gain, gain_sel & 0x03);

Looks like repetitive piece of code, shouldn't be a helper?

...

> +static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
> +				   int *val)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int addr, it_usec, ret;
> +	__le16 reg;
> +
> +	switch (type) {
> +	case IIO_LIGHT:
> +		addr = VEML6031X00_REG_ALS_L;
> +		break;
> +	case IIO_INTENSITY:
> +		addr = VEML6031X00_REG_IR_L;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(data->dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	ret = veml6031x00_get_it(data, &it_usec);
> +	if (ret < 0)
> +		return ret;

> +	/* integration time + 10 % to ensure completion */

fsleep() adds up to 25%, isn't it enough?

> +	fsleep(it_usec + (it_usec / 10));
> +
> +	ret = regmap_bulk_read(data->regmap, addr, &reg, sizeof(reg));
> +	if (ret)
> +		return ret;
> +
> +	*val = le16_to_cpu(reg);
> +	return IIO_VAL_INT;
> +}

...

> +static int veml6031x00_validate_part_id(struct veml6031x00_data *data)
> +{
> +	int part_id, ret;
> +	__le16 reg;
> +
> +	ret = regmap_bulk_read(data->regmap, VEML6031X00_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 != data->chip->part_id)
> +		dev_warn(data->dev, "Unknown ID %04x\n", part_id);

dev_warn_probe() for the sake of consistency?

> +	return 0;
> +}

...

> +static int veml6031x00_hw_init(struct iio_dev *iio)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	struct device *dev = data->dev;
> +	int ret;
> +
> +	/* Max resolution = 6.9632 lx/cnt for gain = 0.125 and IT = 3.125ms */
> +	ret = devm_iio_init_iio_gts(dev, 6, 963200000,
> +				    veml6031x00_gain_sel,
> +				    ARRAY_SIZE(veml6031x00_gain_sel),
> +				    veml6031x00_it_sel,
> +				    ARRAY_SIZE(veml6031x00_it_sel),
> +				    &data->gts);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "failed to init iio gts\n");

IIO GTS

> +	return 0;
> +}

...

> +static int veml6031x00_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct veml6031x00_data *data;
> +	struct iio_dev *iio;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	regmap = devm_regmap_init_i2c(i2c, &veml6031x00_regmap_config);
> +	if (IS_ERR(regmap))

> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "Failed to set regmap\n");

One line is okay.

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

As I said, one of these two is redundant.

> +	ret = devm_mutex_init(dev, &data->scale_lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = veml6031x00_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");

> +	data->chip = i2c_get_match_data(i2c);
> +	if (!data->chip)
> +		return dev_err_probe(dev, -EINVAL, "Failed to get chip data\n");

I would move this closer to the point when we have data allocated. This is
a cheap check and it's better to boil out without need to allocate resources,
touch regulators (that might be undesired from power consumption and physical
processes due to the dragging them on and off), et cetera.

> +	/* The device starts in power down mode by default */
> +	ret = veml6031x00_als_power_on(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to power on the device\n");
> +
> +	ret = devm_add_action_or_reset(dev, veml6031x00_als_shutdown_action, data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to add shutdown action\n");
> +
> +	pm_runtime_set_autosuspend_delay(dev, 2000);
> +	pm_runtime_use_autosuspend(dev);
> +	ret = devm_pm_runtime_set_active_enabled(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> +	ret = devm_pm_runtime_get_noresume(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to get runtime PM\n");
> +
> +	ret = veml6031x00_validate_part_id(data);
> +	if (ret)
> +		return ret;
> +
> +	iio->name = data->chip->name;
> +	iio->channels = veml6031x00_channels;
> +	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
> +	iio->modes = INDIO_DIRECT_MODE;
> +	iio->info = &veml6031x00_info;
> +
> +	ret = veml6031x00_hw_init(iio);
> +	if (ret)
> +		return ret;

> +	pm_runtime_put_autosuspend(dev);

Hmm... But why? Wouldn't this be problematic with reference count on the failed
devm_iio_device_register() below?

> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
> +
> +	return 0;
> +}

...

> +static DEFINE_RUNTIME_DEV_PM_OPS(veml6031x00_pm_ops, veml6031x00_runtime_suspend,
> +				 veml6031x00_runtime_resume, NULL);

Wrap this logically:

static DEFINE_RUNTIME_DEV_PM_OPS(veml6031x00_pm_ops,
				 veml6031x00_runtime_suspend,
				 veml6031x00_runtime_resume,
				 NULL);

OR

static DEFINE_RUNTIME_DEV_PM_OPS(veml6031x00_pm_ops,
				 veml6031x00_runtime_suspend, veml6031x00_runtime_resume, NULL);

...

> +static const struct i2c_device_id veml6031x00_id[] = {
> +	{
> +		.name = "veml6031x00",
> +		.driver_data = (kernel_ulong_t)&veml6031x00_chip

In the similar (to OF ID table) way, leave trailing commas.

> +	},
> +	{
> +		.name = "veml6031x01",
> +		.driver_data = (kernel_ulong_t)&veml6031x01_chip },

Broken indentation, should be a new line somewhere.

> +	{
> +		.name = "veml60311x00",
> +		.driver_data = (kernel_ulong_t)&veml60311x00_chip
> +	},
> +	{
> +		.name = "veml60311x01",
> +		.driver_data = (kernel_ulong_t)&veml60311x01_chip
> +	},

> +};

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 3/4] iio: light: veml6031x00: add support for triggered buffers

[Andy Shevchenko]

On Sun, May 31, 2026 at 09:58:23PM +0200, Javier Carrasco wrote:
> Add triggered buffer functionality for the two channels the device
> provides (ALS and IR).

...

> +static int veml6031x00_buffer_preenable(struct iio_dev *iio)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret, it_usec;

Can it_usec be negative?

> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret)
> +		return ret;

Wouldn't be better to use respective ACQUIRE() macros from pm_runtime.h?

> +	ret = veml6031x00_get_it(data, &it_usec);
> +	if (ret < 0) {
> +		pm_runtime_put_autosuspend(data->dev);
> +		return ret;
> +	}
> +
> +	/*
> +	 * Wait one integration period + 10% margin so the first triggered
> +	 * read does not race with the sensor completing its first conversion
> +	 * after power-on.
> +	 */
> +	fsleep(it_usec + (it_usec / 10));

fsleep() adds up to 25% of margin, wouldn't be that enough?

> +	return 0;
> +}

...

> +static irqreturn_t veml6031x00_trig_handler(int irq, void *p)
> +{
> +	struct iio_poll_func *pf = p;
> +	struct iio_dev *iio = pf->indio_dev;
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ch, ret, i = 0;

Make 'i' unsigned and split the assignment so it goes closer to the user, makes
maintenance easier.

> +	struct {
> +		__le16 chans[2];
> +		aligned_s64 timestamp;
> +	} scan = { };
> +
> +	if (test_bit(VEML6031X00_SCAN_ALS, iio->active_scan_mask) &&
> +	    test_bit(VEML6031X00_SCAN_IR, iio->active_scan_mask)) {

I hope everyone understands that this testing may be immediately false here
(without any additional protection added). Even more, the _ALS bit may be
cleared just before checking the _IR one.

> +		ret = regmap_bulk_read(data->regmap,
> +				       VEML6031X00_REG_ALS_L,
> +				       &scan.chans, sizeof(scan.chans));
> +		if (ret)
> +			goto done;
> +	} else {
> +		iio_for_each_active_channel(iio, ch) {
> +			ret = regmap_bulk_read(data->regmap,
> +					       iio->channels[ch].address,
> +					       &scan.chans[i++],
> +					       sizeof(*scan.chans));
> +			if (ret)
> +				goto done;
> +		}
> +	}
> +
> +	iio_push_to_buffers_with_ts(iio, &scan, sizeof(scan), pf->timestamp);
> +
> +done:
> +	iio_trigger_notify_done(iio->trig);
> +
> +	return IRQ_HANDLED;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 4/4] iio: light: veml6031x00: add support for events and trigger

[Andy Shevchenko]

On Sun, May 31, 2026 at 09:58:24PM +0200, Javier Carrasco wrote:
> The device provides a shared interrupt line for to notify events and
> data ready, which can be used as a trigger. The interrupt line is not a
> requirement for the device to work. Implement variants for the cases
> whether the interrupt line is provided or not.

...

>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
>  #include <linux/i2c.h>
> +#include <linux/interrupt.h>

Previous patch uses irqreturn_t already.

...

> +#define VEML6031X00_INT_MASK        (VEML6031X00_INT_TH_L | \
> +				     VEML6031X00_INT_TH_H | \
> +				     VEML6031X00_INT_DRDY)

Besides mixture of tabs and spaces (the indentation in the first line made only
using spaces) this is less readable than

#define VEML6031X00_INT_MASK						     \
	(VEML6031X00_INT_TH_L | VEML6031X00_INT_TH_H | VEML6031X00_INT_DRDY)

...

> +static int veml6031x00_read_period(struct iio_dev *iio, int *val)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	int ret, reg;
> +
> +	ret = regmap_field_read(data->rf.pers, &reg);
> +	if (ret)
> +		return ret;

> +	*val = 1 << reg;

UB for reg == 31. What's wrong with BIT() here?

> +	return IIO_VAL_INT;
> +}

...

> +static int veml6031x00_write_period(struct iio_dev *iio, int val)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +
> +	if (val < 0 || val > 8 || hweight8(val) != 1)

This gives upper bits out of considerations, while...

> +		return -EINVAL;
> +
> +	return regmap_field_write(data->rf.pers, ffs(val) - 1);

...this one not. It's not a problem per se, but confusing for the reader, needs
more brain power to get it.

Instead I propose to make it clearer

	u8 period = val;
	...

Also, wouldn't __ffs() suffice instead of ffs() - 1? I don't remember, please
double check that.

> +}

...

> +static int veml6031x00_write_th(struct iio_dev *iio, int val, int val2, int dir)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	struct device *dev = data->dev;
> +	__le16 reg = cpu_to_le16(val);
> +	int ret;
> +
> +	if (val < 0 || val > U16_MAX || val2)
> +		return -EINVAL;
> +
> +	if (dir == IIO_EV_DIR_RISING) {
> +		ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WH_L,
> +					&reg, sizeof(reg));
> +		if (ret)
> +			dev_dbg(dev, "Failed to set high threshold %d\n", ret);
> +	} else {
> +		ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WL_L,
> +					&reg, sizeof(reg));
> +		if (ret)
> +			dev_dbg(dev, "Failed to set low threshold %d\n", ret);
> +	}

You can deduplicate the message by:

	bool rising = dir == IIO_EV_DIR_RISING;
	...
	if (rising)
		ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WH_L,
					&reg, sizeof(reg));
	else
		ret = regmap_bulk_write(data->regmap, VEML6031X00_REG_WL_L,
					&reg, sizeof(reg));
	if (ret)
		dev_dbg(dev, "Failed to set %s threshold %d\n", str_high_low(rising), ret);

Will require string_choices.h.

> +	return ret;
> +}

...

> +static int veml6031x00_set_interrupt(struct veml6031x00_data *data, bool state)
> +	__must_hold(&data->irq_lock)

This is an annotation for sparse. If you really want to make it sure...

> +{
> +	int ret;

...use lockdep annotation here (as well).

> +	if (state) {
> +		data->int_users++;
> +		if (data->int_users > 1)
> +			return 0;
> +	} else {
> +		data->int_users--;
> +		if (data->int_users > 0)
> +			return 0;
> +	}
> +
> +	ret = regmap_field_write(data->rf.int_en, state);
> +	if (ret) {
> +		if (state)
> +			data->int_users--;
> +		else
> +			data->int_users++;
> +	}
> +
> +	return ret;
> +}

...

> +static irqreturn_t veml6031x00_interrupt(int irq, void *private)
> +{
> +	struct iio_dev *iio = private;
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	s64 timestamp;
> +	int ret, reg;
> +
> +	ret = regmap_read(data->regmap, VEML6031X00_REG_INT, &reg);
> +	if (ret) {
> +		dev_err(data->dev,
> +			"Failed to read interrupt register %d\n", ret);

It will flood the log very quickly under a heavy system load that may not
service interrupt in time. I'm actually not sure how useful this message
is (only for debug?).

> +		return IRQ_NONE;
> +	}
> +
> +	if (!(reg & VEML6031X00_INT_MASK))
> +		return IRQ_NONE;
> +
> +	guard(mutex)(&data->irq_lock);
> +
> +	if ((reg & (VEML6031X00_INT_TH_H | VEML6031X00_INT_TH_L)) && data->ev_en) {
> +		timestamp = iio_get_time_ns(iio);
> +
> +		if (reg & VEML6031X00_INT_TH_H)
> +			iio_push_event(iio, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +								 IIO_EV_TYPE_THRESH,
> +								 IIO_EV_DIR_RISING),
> +				       timestamp);
> +		if (reg & VEML6031X00_INT_TH_L)
> +			iio_push_event(iio, IIO_UNMOD_EVENT_CODE(IIO_LIGHT, 0,
> +								 IIO_EV_TYPE_THRESH,
> +								 IIO_EV_DIR_FALLING),
> +				       timestamp);
> +	}
> +
> +	if ((reg & VEML6031X00_INT_DRDY) && data->trig_en) {
> +		iio_trigger_poll_nested(data->trig);
> +		ret = veml6031x00_set_af_trig(data, true);
> +		if (ret)
> +			dev_err(data->dev, "Failed to set trigger %d\n", ret);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

...

> +static int veml6031x00_setup_irq(struct i2c_client *i2c, struct iio_dev *iio)
> +{
> +	struct veml6031x00_data *data = iio_priv(iio);
> +	struct device *dev = data->dev;
> +	int ret;
> +
> +	data->trig = devm_iio_trigger_alloc(dev, "%s-drdy%d", iio->name,
> +					    iio_device_id(iio));
> +	if (!data->trig)
> +		return -ENOMEM;
> +
> +	data->trig->ops = &veml6031x00_trigger_ops;
> +	iio_trigger_set_drvdata(data->trig, iio);
> +
> +	ret = devm_iio_trigger_register(dev, data->trig);
> +	if (ret)
> +		return ret;
> +
> +	iio->trig = iio_trigger_get(data->trig);
> +	ret = devm_request_threaded_irq(dev, i2c->irq, NULL,
> +					veml6031x00_interrupt,
> +					IRQF_ONESHOT,
> +					iio->name, iio);
> +	if (ret)

> +		return dev_err_probe(dev, ret, "Failed to request irq %d\n",
> +				     i2c->irq);

Dup message, please remove. return ret; will suffice.

>  	return 0;
>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Javier Carrasco]

Hi Andy, thanks for your review.

On Tue Jun 2, 2026 at 12:01 PM CEST, Andy Shevchenko wrote:
> On Sun, May 31, 2026 at 09:58:22PM +0200, Javier Carrasco wrote:
>> These sensors provide two light channels (ALS and IR), I2C communication
>> and a multiplexed interrupt line to signal data ready and configurable
>> threshold alarms.
>>
>> This first implementation provides basic functionality (measurement
>> configuration, raw reads and ID validation) and defines the different
>> register regions in preparation for extended features in the subsequent
>> patches of the series.
>
> ...
>
> + array_size.h
>
>> +#include <linux/bitfield.h>
>> +#include <linux/bits.h>
>> +#include <linux/i2c.h>
>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>
> In C locale it seems wrong order.
>
>> +#include <linux/mutex.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/regmap.h>
>
> + types.h
>

Do you know any tool to automate this beyond asking an AI? Manual
auditing is not very reliablo and it is not that difficult to miss a
header that has been indirectly included. Building with W=1 and similar
stuff did not help.

>> +#include <linux/units.h>
>
> + Blank line.
>
>> +#include <linux/iio/iio.h>
>> +#include <linux/iio/iio-gts-helper.h>
>
> ...
>
>> +struct veml6031x00_data {
>
> Have you run `pahole`? Does it agree with your layout?
>
>> +	struct device *dev;
>> +	struct iio_gts gts;
>> +	struct regmap *regmap;
>
> Do you need dev and regmap? One may be derived from the other in case regmap
> uses the same dev on initialisation as the one stored here.
>
Ack.
>> +	struct veml6031x00_rf rf;
>> +	const struct veml6031x00_chip *chip;
>> +	/*
>> +	 * Serialize access to scale register fields scattered across multiple
>> +	 * registers (rf.gain, rf.pd_div4, rf.it) to read and write them as a
>> +	 * consistent set.
>> +	 */
>> +	struct mutex scale_lock;
>> +};
>
> ...
>
>> +/*
>> + * The gain selector encodes (PD_D4 << 2) | GAIN to identify each gain setting.
>> + * Gains are multiplied by 8 to work with integers. The values in the iio-gts
>> + * tables don't need corrections because the maximum value of the scale refers
>> + * to GAIN = x1, and the rest of the values are obtained from the resulting
>> + * linear function.
>> + * TODO: add support for MILLI_GAIN_X165 and MILLI_GAIN_X660
>> + */
>> +#define VEML6031X00_SEL_MILLI_GAIN_X125  0x07
>> +#define VEML6031X00_SEL_MILLI_GAIN_X250  0x04
>> +#define VEML6031X00_SEL_MILLI_GAIN_X500  0x03
>> +#define VEML6031X00_SEL_MILLI_GAIN_X1000 0x00
>> +#define VEML6031X00_SEL_MILLI_GAIN_X2000 0x01
>
> Not sure if these one-time use definitions improve or not the readability
> of the code. Up to Jonathan.
>

I prefer these definitions, and a similar pattern is used in multiple
drivers in IIO, but I have no strong feelings about it.

>> +static const struct iio_gain_sel_pair veml6031x00_gain_sel[] = {
>> +	GAIN_SCALE_GAIN(1, VEML6031X00_SEL_MILLI_GAIN_X125),
>> +	GAIN_SCALE_GAIN(2, VEML6031X00_SEL_MILLI_GAIN_X250),
>> +	GAIN_SCALE_GAIN(4, VEML6031X00_SEL_MILLI_GAIN_X500),
>> +	GAIN_SCALE_GAIN(8, VEML6031X00_SEL_MILLI_GAIN_X1000),
>> +	GAIN_SCALE_GAIN(16, VEML6031X00_SEL_MILLI_GAIN_X2000),
>> +};
>
> ...
>
>> +{
>> +	struct regmap *regmap = data->regmap;
>> +	struct device *dev = data->dev;
>
> In case you really need a 'dev' here, pass via function parameter, no need to
> keep it in the 'data'.
>
Ack.
>> +	struct regmap_field *rm_field;
>> +	struct veml6031x00_rf *rf = &data->rf;
>> +
>> +	rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_gain);
>> +	if (IS_ERR(rm_field))
>> +		return PTR_ERR(rm_field);
>> +	rf->gain = rm_field;
>> +
>> +	rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_it);
>> +	if (IS_ERR(rm_field))
>> +		return PTR_ERR(rm_field);
>> +	rf->it = rm_field;
>> +
>> +	rm_field = devm_regmap_field_alloc(dev, regmap, veml6031x00_rf_pd_div4);
>> +	if (IS_ERR(rm_field))
>> +		return PTR_ERR(rm_field);
>> +	rf->pd_div4 = rm_field;
>> +
>> +	return 0;
>> +}
>
> ...
>
>> +static int veml6031x00_get_it(struct veml6031x00_data *data, int *val2)
>> +{
>> +	int ret, it_idx;
>
> Why is 'it_idx' signed?
>
I'll make it unsigned int.
>> +
>> +	scoped_guard(mutex, &data->scale_lock) {
>> +		ret = regmap_field_read(data->rf.it, &it_idx);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	ret = iio_gts_find_int_time_by_sel(&data->gts, it_idx);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	*val2 = ret;
>> +
>> +	return IIO_VAL_INT_PLUS_MICRO;
>> +}
>
> ...
>
>> +static int veml6031x00_set_it(struct iio_dev *iio, int val, int val2)
>> +{
>> +	struct veml6031x00_data *data = iio_priv(iio);
>> +	int ret, gain_sel, gain_reg, pd_div4, it_idx, new_gain, prev_gain, prev_it;
>
> Similar question here and so on...
>
Ack.
>> +	bool in_range;
>
>> +}
>
> ...
>
>> +	ret = regmap_field_write(data->rf.pd_div4, gain_sel >> 2);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return regmap_field_write(data->rf.gain, gain_sel & 0x03);
>
> Looks like repetitive piece of code, shouldn't be a helper?
>
> ...
>
Ack.
>> +static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
>> +				   int *val)
>> +{
>> +	struct veml6031x00_data *data = iio_priv(iio);
>> +	int addr, it_usec, ret;
>> +	__le16 reg;
>> +
>> +	switch (type) {
>> +	case IIO_LIGHT:
>> +		addr = VEML6031X00_REG_ALS_L;
>> +		break;
>> +	case IIO_INTENSITY:
>> +		addr = VEML6031X00_REG_IR_L;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(data->dev, pm);
>> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = veml6031x00_get_it(data, &it_usec);
>> +	if (ret < 0)
>> +		return ret;
>
>> +	/* integration time + 10 % to ensure completion */
>
> fsleep() adds up to 25%, isn't it enough?
>

The problem is that it adds UP to 25%, but that is not guaranteed. If
almost no extra delay is added, it will be below the margin I added,
which was necessary when I tested this delay with real hardware.

>> +	fsleep(it_usec + (it_usec / 10));
>> +
>> +	ret = regmap_bulk_read(data->regmap, addr, &reg, sizeof(reg));
>> +	if (ret)
>> +		return ret;
>> +
>> +	*val = le16_to_cpu(reg);
>> +	return IIO_VAL_INT;
>> +}
>
> ...
>
>> +static int veml6031x00_validate_part_id(struct veml6031x00_data *data)
>> +{
>> +	int part_id, ret;
>> +	__le16 reg;
>> +
>> +	ret = regmap_bulk_read(data->regmap, VEML6031X00_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 != data->chip->part_id)
>> +		dev_warn(data->dev, "Unknown ID %04x\n", part_id);
>
> dev_warn_probe() for the sake of consistency?
>

Jonathan would like this to be a dev_info().

>> +	return 0;
>> +}
>
> ...
>
>> +static int veml6031x00_hw_init(struct iio_dev *iio)
>> +{
>> +	struct veml6031x00_data *data = iio_priv(iio);
>> +	struct device *dev = data->dev;
>> +	int ret;
>> +
>> +	/* Max resolution = 6.9632 lx/cnt for gain = 0.125 and IT = 3.125ms */
>> +	ret = devm_iio_init_iio_gts(dev, 6, 963200000,
>> +				    veml6031x00_gain_sel,
>> +				    ARRAY_SIZE(veml6031x00_gain_sel),
>> +				    veml6031x00_it_sel,
>> +				    ARRAY_SIZE(veml6031x00_it_sel),
>> +				    &data->gts);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "failed to init iio gts\n");
>
> IIO GTS
>
Ack.
>> +	return 0;
>> +}
>
> ...
>
>> +static int veml6031x00_probe(struct i2c_client *i2c)
>> +{
>> +	struct device *dev = &i2c->dev;
>> +	struct veml6031x00_data *data;
>> +	struct iio_dev *iio;
>> +	struct regmap *regmap;
>> +	int ret;
>> +
>> +	regmap = devm_regmap_init_i2c(i2c, &veml6031x00_regmap_config);
>> +	if (IS_ERR(regmap))
>
>> +		return dev_err_probe(dev, PTR_ERR(regmap),
>> +				     "Failed to set regmap\n");
>
> One line is okay.
>
Ack.
>> +	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;
>
> As I said, one of these two is redundant.
>
Ack.
>> +	ret = devm_mutex_init(dev, &data->scale_lock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = veml6031x00_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");
>
>> +	data->chip = i2c_get_match_data(i2c);
>> +	if (!data->chip)
>> +		return dev_err_probe(dev, -EINVAL, "Failed to get chip data\n");
>
> I would move this closer to the point when we have data allocated. This is
> a cheap check and it's better to boil out without need to allocate resources,
> touch regulators (that might be undesired from power consumption and physical
> processes due to the dragging them on and off), et cetera.
>

Ack.

>> +	/* The device starts in power down mode by default */
>> +	ret = veml6031x00_als_power_on(data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to power on the device\n");
>> +
>> +	ret = devm_add_action_or_reset(dev, veml6031x00_als_shutdown_action, data);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to add shutdown action\n");
>> +
>> +	pm_runtime_set_autosuspend_delay(dev, 2000);
>> +	pm_runtime_use_autosuspend(dev);
>> +	ret = devm_pm_runtime_set_active_enabled(dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
>> +
>> +	ret = devm_pm_runtime_get_noresume(dev);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to get runtime PM\n");
>> +
>> +	ret = veml6031x00_validate_part_id(data);
>> +	if (ret)
>> +		return ret;
>> +
>> +	iio->name = data->chip->name;
>> +	iio->channels = veml6031x00_channels;
>> +	iio->num_channels = ARRAY_SIZE(veml6031x00_channels);
>> +	iio->modes = INDIO_DIRECT_MODE;
>> +	iio->info = &veml6031x00_info;
>> +
>> +	ret = veml6031x00_hw_init(iio);
>> +	if (ret)
>> +		return ret;
>
>> +	pm_runtime_put_autosuspend(dev);
>
> Hmm... But why? Wouldn't this be problematic with reference count on the failed
> devm_iio_device_register() below?
>

I could move this a bit further down after devm_iio_device_register(),
but as I replied to a Sashiko complaint, the reference count will never
underflow in this configuration.

>> +	ret = devm_iio_device_register(dev, iio);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
>> +
>> +	return 0;
>> +}
>
> ...
>
>> +static DEFINE_RUNTIME_DEV_PM_OPS(veml6031x00_pm_ops, veml6031x00_runtime_suspend,
>> +				 veml6031x00_runtime_resume, NULL);
>
> Wrap this logically:
>
> static DEFINE_RUNTIME_DEV_PM_OPS(veml6031x00_pm_ops,
> 				 veml6031x00_runtime_suspend,
> 				 veml6031x00_runtime_resume,
> 				 NULL);
>
> OR
>
> static DEFINE_RUNTIME_DEV_PM_OPS(veml6031x00_pm_ops,
> 				 veml6031x00_runtime_suspend, veml6031x00_runtime_resume, NULL);
>
> ...
>

Ack.

>> +static const struct i2c_device_id veml6031x00_id[] = {
>> +	{
>> +		.name = "veml6031x00",
>> +		.driver_data = (kernel_ulong_t)&veml6031x00_chip
>
> In the similar (to OF ID table) way, leave trailing commas.
>
Ack.
>> +	},
>> +	{
>> +		.name = "veml6031x01",
>> +		.driver_data = (kernel_ulong_t)&veml6031x01_chip },
>
> Broken indentation, should be a new line somewhere.
>
Ack.
>> +	{
>> +		.name = "veml60311x00",
>> +		.driver_data = (kernel_ulong_t)&veml60311x00_chip
>> +	},
>> +	{
>> +		.name = "veml60311x01",
>> +		.driver_data = (kernel_ulong_t)&veml60311x01_chip
>> +	},
>
>> +};

Best regards,
Javier

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Andy Shevchenko]

On Tue, Jun 02, 2026 at 12:45:35PM +0200, Javier Carrasco wrote:
> On Tue Jun 2, 2026 at 12:01 PM CEST, Andy Shevchenko wrote:
> > On Sun, May 31, 2026 at 09:58:22PM +0200, Javier Carrasco wrote:

...

> > + array_size.h
> >
> >> +#include <linux/bitfield.h>
> >> +#include <linux/bits.h>
> >> +#include <linux/i2c.h>
> >
> >> +#include <linux/module.h>
> >> +#include <linux/mod_devicetable.h>
> >
> > In C locale it seems wrong order.
> >
> >> +#include <linux/mutex.h>
> >> +#include <linux/pm_runtime.h>
> >> +#include <linux/regmap.h>
> >
> > + types.h
> 
> Do you know any tool to automate this beyond asking an AI? Manual
> auditing is not very reliablo and it is not that difficult to miss a
> header that has been indirectly included. Building with W=1 and similar
> stuff did not help.

`iwyu`, but it needs a custom configuration. Even with that it's quite far from ideal.
The custom config had been shared in the linux-iio@ mailing list this year.

> >> +#include <linux/units.h>

...

> >> +	/* integration time + 10 % to ensure completion */
> >
> > fsleep() adds up to 25%, isn't it enough?
> 
> The problem is that it adds UP to 25%, but that is not guaranteed. If
> almost no extra delay is added, it will be below the margin I added,
> which was necessary when I tested this delay with real hardware.

Noted. Can it_usec be precalculated already to include that 10%?

> >> +	fsleep(it_usec + (it_usec / 10));

...

> >> +	pm_runtime_put_autosuspend(dev);
> >
> > Hmm... But why? Wouldn't this be problematic with reference count on the failed
> > devm_iio_device_register() below?
> 
> I could move this a bit further down after devm_iio_device_register(),
> but as I replied to a Sashiko complaint, the reference count will never
> underflow in this configuration.

Please, add a comment elaborating on that, it's not clear at glance.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Joshua Crofts]

On Tue, 2 Jun 2026 at 13:17, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> > Do you know any tool to automate this beyond asking an AI? Manual
> > auditing is not very reliablo and it is not that difficult to miss a
> > header that has been indirectly included. Building with W=1 and similar
> > stuff did not help.
>
> `iwyu`, but it needs a custom configuration. Even with that it's quite far from ideal.
> The custom config had been shared in the linux-iio@ mailing list this year.

Let me help with that.

https://lore.kernel.org/all/20260512073505.1310-1-joshua.crofts1@gmail.com/

-- 
Kind regards

CJD

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Javier Carrasco]

On Tue Jun 2, 2026 at 1:21 PM CEST, Joshua Crofts wrote:
> On Tue, 2 Jun 2026 at 13:17, Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
>> > Do you know any tool to automate this beyond asking an AI? Manual
>> > auditing is not very reliablo and it is not that difficult to miss a
>> > header that has been indirectly included. Building with W=1 and similar
>> > stuff did not help.
>>
>> `iwyu`, but it needs a custom configuration. Even with that it's quite far from ideal.
>> The custom config had been shared in the linux-iio@ mailing list this year.
>
> Let me help with that.
>
> https://lore.kernel.org/all/20260512073505.1310-1-joshua.crofts1@gmail.com/

Thank you, Joshua. One question, though: maybe you remember that your
last driver was missing one header (which I am not criticizing at all,
as you can see I missed some too!), and I am wondering if it was because
iwyu missed it and if so, how that could be avoided.

Best regards,
Javier

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Javier Carrasco]

On Tue Jun 2, 2026 at 1:12 PM CEST, Andy Shevchenko wrote:
> On Tue, Jun 02, 2026 at 12:45:35PM +0200, Javier Carrasco wrote:
>> On Tue Jun 2, 2026 at 12:01 PM CEST, Andy Shevchenko wrote:
>> > On Sun, May 31, 2026 at 09:58:22PM +0200, Javier Carrasco wrote:
>
> ...
>
>> > + array_size.h
>> >
>> >> +#include <linux/bitfield.h>
>> >> +#include <linux/bits.h>
>> >> +#include <linux/i2c.h>
>> >
>> >> +#include <linux/module.h>
>> >> +#include <linux/mod_devicetable.h>
>> >
>> > In C locale it seems wrong order.
>> >
>> >> +#include <linux/mutex.h>
>> >> +#include <linux/pm_runtime.h>
>> >> +#include <linux/regmap.h>
>> >
>> > + types.h
>>
>> Do you know any tool to automate this beyond asking an AI? Manual
>> auditing is not very reliablo and it is not that difficult to miss a
>> header that has been indirectly included. Building with W=1 and similar
>> stuff did not help.
>
> `iwyu`, but it needs a custom configuration. Even with that it's quite far from ideal.
> The custom config had been shared in the linux-iio@ mailing list this year.
>

Thanks, I'll take a look at it in case it finds some more missing
headers.

>> >> +#include <linux/units.h>
>
> ...
>
>> >> +	/* integration time + 10 % to ensure completion */
>> >
>> > fsleep() adds up to 25%, isn't it enough?
>>
>> The problem is that it adds UP to 25%, but that is not guaranteed. If
>> almost no extra delay is added, it will be below the margin I added,
>> which was necessary when I tested this delay with real hardware.
>
> Noted. Can it_usec be precalculated already to include that 10%?
>

Yes, that is possible. I'll add it to V5.

>> >> +	fsleep(it_usec + (it_usec / 10));
>
> ...
>
>> >> +	pm_runtime_put_autosuspend(dev);
>> >
>> > Hmm... But why? Wouldn't this be problematic with reference count on the failed
>> > devm_iio_device_register() below?
>>
>> I could move this a bit further down after devm_iio_device_register(),
>> but as I replied to a Sashiko complaint, the reference count will never
>> underflow in this configuration.
>
> Please, add a comment elaborating on that, it's not clear at glance.

I will provide the link to the message where I explained this instead of
copying the explanation to keep the discussion in one place. The mail
belongs to the discussion thread for [2/4] (this patch) anyway:

https://lore.kernel.org/all/20260531-veml6031x00-v4-0-e64f7fdce38d@gmail.com/

Best regards,
Javier

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Javier Carrasco]

On Tue Jun 2, 2026 at 1:42 PM CEST, Javier Carrasco wrote:
>>> > Hmm... But why? Wouldn't this be problematic with reference count on the failed
>>> > devm_iio_device_register() below?
>>>
>>> I could move this a bit further down after devm_iio_device_register(),
>>> but as I replied to a Sashiko complaint, the reference count will never
>>> underflow in this configuration.
>>
>> Please, add a comment elaborating on that, it's not clear at glance.
>
> I will provide the link to the message where I explained this instead of
> copying the explanation to keep the discussion in one place. The mail
> belongs to the discussion thread for [2/4] (this patch) anyway:
>
> https://lore.kernel.org/all/20260531-veml6031x00-v4-0-e64f7fdce38d@gmail.com/
>
> Best regards,
> Javier

Sorry, I think I gave you the link to the cover letter, this is the
right one:

https://lore.kernel.org/all/DIXZFV822HRI.2SBIT7ADW9LUK@gmail.com/

The relevant information is at the very end.

Best regards,
Javier

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Joshua Crofts]

On Tue, 2 Jun 2026 at 13:35, Javier Carrasco
<javier.carrasco.cruz@gmail.com> wrote:
> Thank you, Joshua. One question, though: maybe you remember that your
> last driver was missing one header (which I am not criticizing at all,
> as you can see I missed some too!), and I am wondering if it was because
> iwyu missed it and if so, how that could be avoided.

About that - I didn't actually run iwyu on my driver, I recently migrated to
Fedora and forgot to set up LLVM and iywu-tool :-)

-- 
Kind regards

CJD

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Javier Carrasco]

On Tue Jun 2, 2026 at 1:47 PM CEST, Joshua Crofts wrote:
> On Tue, 2 Jun 2026 at 13:35, Javier Carrasco
> <javier.carrasco.cruz@gmail.com> wrote:
>> Thank you, Joshua. One question, though: maybe you remember that your
>> last driver was missing one header (which I am not criticizing at all,
>> as you can see I missed some too!), and I am wondering if it was because
>> iwyu missed it and if so, how that could be avoided.
>
> About that - I didn't actually run iwyu on my driver, I recently migrated to
> Fedora and forgot to set up LLVM and iywu-tool :-)

I installed IWYU and added your mapping for a quick test, and it wants
to add a million headers or so:

python3 ~/iwyu/include-what-you-use/iwyu_tool.py \
    -p . \
    drivers/iio/light/veml6031x00.c \
    -- \
    -Xiwyu --no_default_mappings \
    -Xiwyu --mapping_file=tools/iio/iio.imp

drivers/iio/light/veml6031x00.c should add these lines:
#include <asm/byteorder.h>                     // for le16_to_cpu, cpu_to_le16
#include <linux/bitops.h>                      // for BIT, const_test_bit
#include <linux/device.h>                      // for dev_get_drvdata, devic...
#include <linux/errno.h>                       // for EINVAL, EBUSY, ENOMEM
#include <linux/limits.h>                      // for U16_MAX
#include <linux/pm.h>                          // for pm_ptr
#include <linux/stddef.h>                      // for NULL, true
#include <linux/sysfs.h>                       // for attribute_group
#include <linux/types.h>                       // for __le16, bool, aligned_s64
#include "asm-generic/bitops/builtin-ffs.h"    // for ffs
#include "asm-generic/bitops/const_hweight.h"  // for hweight8
#include "linux/array_size.h"                  // for ARRAY_SIZE
#include "linux/cleanup.h"                     // for guard, scoped_guard
#include "linux/compiler-context-analysis.h"   // for __must_hold
#include "linux/delay.h"                       // for fsleep
#include "linux/dev_printk.h"                  // for dev_err_probe, dev_dbg
#include "linux/device/devres.h"               // for devm_add_action_or_reset
#include "linux/err.h"                         // for IS_ERR, PTR_ERR
#include "linux/iio/buffer.h"                  // for iio_push_to_buffers_wi...
#include "linux/irqreturn.h"                   // for irqreturn, irqreturn_t
#include "linux/kconfig.h"                     // for __ARG_PLACEHOLDER_1
#include "linux/regulator/consumer.h"          // for devm_regulator_get_enable

drivers/iio/light/veml6031x00.c should remove these lines:
- #include <linux/bits.h>  // lines 9-9
- #include <linux/units.h>  // lines 17-17

The full include-list for drivers/iio/light/veml6031x00.c:
#include <asm/byteorder.h>                     // for le16_to_cpu, cpu_to_le16
#include <linux/bitfield.h>                    // for FIELD_PREP
#include <linux/bitops.h>                      // for BIT, const_test_bit
#include <linux/device.h>                      // for dev_get_drvdata, devic...
#include <linux/errno.h>                       // for EINVAL, EBUSY, ENOMEM
#include <linux/i2c.h>                         // for i2c_client, i2c_get_ma...
#include <linux/iio/events.h>                  // for IIO_UNMOD_EVENT_CODE
#include <linux/iio/iio-gts-helper.h>          // for GAIN_SCALE_ITIME_US
#include <linux/iio/iio.h>                     // for iio_chan_info_enum
#include <linux/iio/sysfs.h>                   // for iio_const_attr, IIO_CO...
#include <linux/iio/trigger.h>                 // for devm_iio_trigger_register
#include <linux/iio/trigger_consumer.h>        // for iio_poll_func, iio_tri...
#include <linux/iio/triggered_buffer.h>        // for devm_iio_triggered_buf...
#include <linux/interrupt.h>                   // for devm_request_threaded_irq
#include <linux/limits.h>                      // for U16_MAX
#include <linux/mod_devicetable.h>             // for kernel_ulong_t, i2c_de...
#include <linux/module.h>                      // for MODULE_DEVICE_TABLE
#include <linux/mutex.h>                       // for class_mutex_constructor
#include <linux/pm.h>                          // for pm_ptr
#include <linux/pm_runtime.h>                  // for pm_runtime_put_autosus...
#include <linux/regmap.h>                      // for regmap_field_write
#include <linux/stddef.h>                      // for NULL, true
#include <linux/sysfs.h>                       // for attribute_group
#include <linux/types.h>                       // for __le16, bool, aligned_s64
#include "asm-generic/bitops/builtin-ffs.h"    // for ffs
#include "asm-generic/bitops/const_hweight.h"  // for hweight8
#include "linux/array_size.h"                  // for ARRAY_SIZE
#include "linux/cleanup.h"                     // for guard, scoped_guard
#include "linux/compiler-context-analysis.h"   // for __must_hold
#include "linux/delay.h"                       // for fsleep
#include "linux/dev_printk.h"                  // for dev_err_probe, dev_dbg
#include "linux/device/devres.h"               // for devm_add_action_or_reset
#include "linux/err.h"                         // for IS_ERR, PTR_ERR
#include "linux/iio/buffer.h"                  // for iio_push_to_buffers_wi...
#include "linux/irqreturn.h"                   // for irqreturn, irqreturn_t
#include "linux/kconfig.h"                     // for __ARG_PLACEHOLDER_1
#include "linux/regulator/consumer.h"          // for devm_regulator_get_enable
---

If that is correct, I bet there is almost no driver prior to IWYU that
has all the headers :D

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Joshua Crofts]

On Tue, 2 Jun 2026 at 14:22, Javier Carrasco
<javier.carrasco.cruz@gmail.com> wrote:
>
> On Tue Jun 2, 2026 at 1:47 PM CEST, Joshua Crofts wrote:
> > On Tue, 2 Jun 2026 at 13:35, Javier Carrasco
> > <javier.carrasco.cruz@gmail.com> wrote:
> >> Thank you, Joshua. One question, though: maybe you remember that your
> >> last driver was missing one header (which I am not criticizing at all,
> >> as you can see I missed some too!), and I am wondering if it was because
> >> iwyu missed it and if so, how that could be avoided.
> >
> > About that - I didn't actually run iwyu on my driver, I recently migrated to
> > Fedora and forgot to set up LLVM and iywu-tool :-)
>
> I installed IWYU and added your mapping for a quick test, and it wants
> to add a million headers or so:
> If that is correct, I bet there is almost no driver prior to IWYU that
> has all the headers :D

Well, as Andy said - it's still far from ideal. It generates unnecessary noise.

Question: did you run `make compile_commands.json` before running iwyu?

iwyu_tool -p compile_commands.json drivers/iio/magnetometer/ak8975.c -- \
-Xiwyu --no_default_mappings -Xiwyu --mapping_file=/home/josh/iio.imp

This was the command I used when I did some cleaning up on the ak8975.

-- 
Kind regards

CJD

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Jonathan Cameron]

On Mon, 01 Jun 2026 22:07:42 +0200
"Javier Carrasco" <javier.carrasco.cruz@gmail.com> wrote:

> Hi Jonathan, thanks again for your review.
> 
> On Mon Jun 1, 2026 at 12:21 PM CEST, Jonathan Cameron wrote:
> > On Sun, 31 May 2026 21:58:22 +0200
> > Javier Carrasco <javier.carrasco.cruz@gmail.com> wrote:
> >  
> >> These sensors provide two light channels (ALS and IR), I2C communication
> >> and a multiplexed interrupt line to signal data ready and configurable
> >> threshold alarms.
> >>
> >> This first implementation provides basic functionality (measurement
> >> configuration, raw reads and ID validation) and defines the different
> >> register regions in preparation for extended features in the subsequent
> >> patches of the series.
> >>
> >> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>  
> > A few comments from sashiko and a couple from me based on a fresh read.
> >
> >  
> >> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> >> new file mode 100644
> >> index 000000000000..6f9a7bad44d4
> >> --- /dev/null
> >> +++ b/drivers/iio/light/veml6031x00.c  
> >  
> >> +
> >> +static const struct iio_chan_spec veml6031x00_channels[] = {
> >> +	{
> >> +		.type = IIO_LIGHT,
> >> +		.address = VEML6031X00_REG_ALS_L,
> >> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >> +				      BIT(IIO_CHAN_INFO_SCALE),
> >> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> >> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> >> +		.info_mask_separate_available = BIT(IIO_CHAN_INFO_SCALE),
> >> +	},
> >> +	{
> >> +		.type = IIO_INTENSITY,
> >> +		.address = VEML6031X00_REG_IR_L,
> >> +		.modified = 1,
> >> +		.channel2 = IIO_MOD_LIGHT_IR,
> >> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> >> +				      BIT(IIO_CHAN_INFO_SCALE),  
> >
> > Can we move scale to shared_by_type?
> >
> > Thinking on some of the others, they should probably be shared_by_type as well
> > rather than shared_by_all. If we ever add buffered support and a timestamp
> > the integration_time doesn't apply to that.
> >
> > shared_by_all tends to only include things that are truely universal like
> > sampling_frequency.
> >  
> 
> I am not sure if I get this point. This device has a single IIO_LIGHT
> channel, and the scale only applies to it. Are info_mask_separate and
> info_mask_shared_by_type not the same in that case? I have seen that
> some drivers use both info_mask_separate for INFO_RAW, and
> info_mask_shared_by_type for INFO_INT_TIME and/or INFO_SCALE, but that
> could make more sense if there were multiple channels of the same type.
> What am I missing here?

Ah. I've been reading too many drivers. For some reason I thought this
had more intensity channels. My comment clearly garbage as you point out.
I maybe got thrown by how ALS sensors used to work.  They used IR only
and a clear window that covered both IR and the frequencies we need for
illumiance measurement. The light channel was then some combination of
the two. I guess they've figured out how to filter that IR out of that
these days.  That leaves me a little curious as to what applications actually
use the IR channel.

> 
> On the other hand, the integration time applies to both the IIO_LIGHT
> and IIO_INTENSITY channels, so I guess you are suggesting to add it
> to both channels as info_mask_shared_by_type because the timestamp
> is a channel itself. Moving it form shared_by_all to shared_by_type is
> alright, and I will add it to V5.

Yes, that one was correct.

> 
> >> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> >> +		.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME),
> >> +	},
> >> +};  

> 
> >> +
> >> +	ret = veml6031x00_hw_init(iio);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	pm_runtime_put_autosuspend(dev);  
> >
> > As sashiko shouts, this looks like runtime pm will underflow on remove.
> > Check it by removing your driver.  It doesn't actually result in any
> > problem, as the runtime pm subsystem just saturates at 0 on decrement.
> > Given that's tear down anyway maybe we don't care.  However, it's easy
> > enough to fix by using pm_runtime_get_no_resume() and a couple of
> > explicit calls to put it in error paths.
> >  
> 
> I took some time to audit this in detail, because although my
> expectations are that atomic_add_unless(usage_count, -1, 0) should make
> this shout a false positive. Expectations don't always meet reality. My
> expectations were based on the code, so I have added tracepoints to know
> exactly what's going on.
> 
> Scenario 1: Successful probe -> unbind driver.
> 
> devm_pm_runtime_get_noresume() increases usage_count, and the call to
> pm_runtime_put_autosuspend() decreases it. That is balanced, giving us
> usage_count = 0 and putting the device in power down mode as desired. If
> the device is then unbound, the devres action (a call to
> pm_runtime_put_noidle_action, which only calls pm_runtime_put_noidle)
> triggers a call to atomic_add_unless(&dev->power.usage_count, -1, 0).
> Given that the usage count is 0, nothing happens and there is no
> underflow. In fact, the underflow is not possible this way, and adding a
> remove function to check if usage_count is 0 and calling
> pm_runtime_put() is basically repeating what the devres action already
> offers for free.

Agreed on this analysis. From a readability point of view it is nice
to have balanced calls but as long as we only underflow / get clamped
in a path where we never increment again that's fine.  I'd add a comment
somewhere to say that's intentional.

> 
> Scenario 2: write_event_config -> unbind driver.
> 
> Sashiko says that pm_runtime_resume_and_get() increases usage_count, and
> there is no devres action associated to it. That is only partially
> correct, because the devres action from devm_pm_runtime_get_noresume()
> will be triggered when the driver is unbound. Actually, this is great
> because then pm_runtime_resume_and_get() gets balanced, and there is no
> need for a remove function to check again if usage_count is 0 or not. In
> this case, usage_count = 1 before unbinding the driver, and then the
> devres action is triggered when it gets unbound. Exactly what we want to
> have usage_count = 0.

This one I'm more dubious on.  Basically you are saying on remove we happen
to have an extra decrement for largely unrelated reasons so we are fine.
That to me smells fragile even if it works.  For instance someone tidying
up the imbalance in scenario 1 (which would seem reasonable to do from
a code understandability point of view) would break scenario 2.
I'd rather we had something explicit for this. Probably an devm action
that just checks for events being enabled at exit and disables them as part of
the tear down.

So not bugs as such, but fragile which is not good from maintainability
point of view.

Jonathan


> 
> >> +
> >> +	ret = devm_iio_device_register(dev, iio);
> >> +	if (ret)
> >> +		return dev_err_probe(dev, ret, "Failed to register iio device\n");
> >> +
> >> +	return 0;
> >> +}  
> >  
> >>  
> 
> Best regards,
> Javier

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Javier Carrasco]

On Tue Jun 2, 2026 at 2:33 PM CEST, Joshua Crofts wrote:
> On Tue, 2 Jun 2026 at 14:22, Javier Carrasco
> <javier.carrasco.cruz@gmail.com> wrote:
>>
>> On Tue Jun 2, 2026 at 1:47 PM CEST, Joshua Crofts wrote:
>> > On Tue, 2 Jun 2026 at 13:35, Javier Carrasco
>> > <javier.carrasco.cruz@gmail.com> wrote:
>> >> Thank you, Joshua. One question, though: maybe you remember that your
>> >> last driver was missing one header (which I am not criticizing at all,
>> >> as you can see I missed some too!), and I am wondering if it was because
>> >> iwyu missed it and if so, how that could be avoided.
>> >
>> > About that - I didn't actually run iwyu on my driver, I recently migrated to
>> > Fedora and forgot to set up LLVM and iywu-tool :-)
>>
>> I installed IWYU and added your mapping for a quick test, and it wants
>> to add a million headers or so:
>> If that is correct, I bet there is almost no driver prior to IWYU that
>> has all the headers :D
>
> Well, as Andy said - it's still far from ideal. It generates unnecessary noise.
>
> Question: did you run `make compile_commands.json` before running iwyu?
>
> iwyu_tool -p compile_commands.json drivers/iio/magnetometer/ak8975.c -- \
> -Xiwyu --no_default_mappings -Xiwyu --mapping_file=/home/josh/iio.imp
>
> This was the command I used when I did some cleaning up on the ak8975.

I used the compile_commands.json generated by
scripts/clang-tools/gen_compile_commands.py, which I always use to have
a better experience with clangd and Neovim.

Should it make any difference?

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Jonathan Cameron]

> >> +/*
> >> + * The gain selector encodes (PD_D4 << 2) | GAIN to identify each gain setting.
> >> + * Gains are multiplied by 8 to work with integers. The values in the iio-gts
> >> + * tables don't need corrections because the maximum value of the scale refers
> >> + * to GAIN = x1, and the rest of the values are obtained from the resulting
> >> + * linear function.
> >> + * TODO: add support for MILLI_GAIN_X165 and MILLI_GAIN_X660
> >> + */
> >> +#define VEML6031X00_SEL_MILLI_GAIN_X125  0x07
> >> +#define VEML6031X00_SEL_MILLI_GAIN_X250  0x04
> >> +#define VEML6031X00_SEL_MILLI_GAIN_X500  0x03
> >> +#define VEML6031X00_SEL_MILLI_GAIN_X1000 0x00
> >> +#define VEML6031X00_SEL_MILLI_GAIN_X2000 0x01  
> >
> > Not sure if these one-time use definitions improve or not the readability
> > of the code. Up to Jonathan.
> >  
> 
> I prefer these definitions, and a similar pattern is used in multiple
> drivers in IIO, but I have no strong feelings about it.

Looking again at this, what do the numbers in the defines actually mean?
Seems a bit odd to have the base gain of 1 being called X125.
Maybe a comment on that would be useful.  I don't mind either way
on defines for this but if that number is useful to have I'd rather
have a define than a comment on each line.

> 
> >> +static const struct iio_gain_sel_pair veml6031x00_gain_sel[] = {
> >> +	GAIN_SCALE_GAIN(1, VEML6031X00_SEL_MILLI_GAIN_X125),
> >> +	GAIN_SCALE_GAIN(2, VEML6031X00_SEL_MILLI_GAIN_X250),
> >> +	GAIN_SCALE_GAIN(4, VEML6031X00_SEL_MILLI_GAIN_X500),
> >> +	GAIN_SCALE_GAIN(8, VEML6031X00_SEL_MILLI_GAIN_X1000),
> >> +	GAIN_SCALE_GAIN(16, VEML6031X00_SEL_MILLI_GAIN_X2000),
> >> +};  
> >
> > ...

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Javier Carrasco]

On Tue Jun 2, 2026 at 2:38 PM CEST, Jonathan Cameron wrote:
>> >> +/*
>> >> + * The gain selector encodes (PD_D4 << 2) | GAIN to identify each gain setting.
>> >> + * Gains are multiplied by 8 to work with integers. The values in the iio-gts
>> >> + * tables don't need corrections because the maximum value of the scale refers
>> >> + * to GAIN = x1, and the rest of the values are obtained from the resulting
>> >> + * linear function.
>> >> + * TODO: add support for MILLI_GAIN_X165 and MILLI_GAIN_X660
>> >> + */
>> >> +#define VEML6031X00_SEL_MILLI_GAIN_X125  0x07
>> >> +#define VEML6031X00_SEL_MILLI_GAIN_X250  0x04
>> >> +#define VEML6031X00_SEL_MILLI_GAIN_X500  0x03
>> >> +#define VEML6031X00_SEL_MILLI_GAIN_X1000 0x00
>> >> +#define VEML6031X00_SEL_MILLI_GAIN_X2000 0x01
>> >
>> > Not sure if these one-time use definitions improve or not the readability
>> > of the code. Up to Jonathan.
>> >
>>
>> I prefer these definitions, and a similar pattern is used in multiple
>> drivers in IIO, but I have no strong feelings about it.
>
> Looking again at this, what do the numbers in the defines actually mean?
> Seems a bit odd to have the base gain of 1 being called X125.
> Maybe a comment on that would be useful.  I don't mind either way
> on defines for this but if that number is useful to have I'd rather
> have a define than a comment on each line.
>

I thought that MILLI_GAIN was already documenting what x125 is: 0.125 =
125 milli. More than the base gain of 1, it is the lowest gain you can
configure.

>>
>> >> +static const struct iio_gain_sel_pair veml6031x00_gain_sel[] = {
>> >> +	GAIN_SCALE_GAIN(1, VEML6031X00_SEL_MILLI_GAIN_X125),
>> >> +	GAIN_SCALE_GAIN(2, VEML6031X00_SEL_MILLI_GAIN_X250),
>> >> +	GAIN_SCALE_GAIN(4, VEML6031X00_SEL_MILLI_GAIN_X500),
>> >> +	GAIN_SCALE_GAIN(8, VEML6031X00_SEL_MILLI_GAIN_X1000),
>> >> +	GAIN_SCALE_GAIN(16, VEML6031X00_SEL_MILLI_GAIN_X2000),
>> >> +};
>> >
>> > ...

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Jonathan Cameron]

On Tue, 02 Jun 2026 14:40:47 +0200
"Javier Carrasco" <javier.carrasco.cruz@gmail.com> wrote:

> On Tue Jun 2, 2026 at 2:38 PM CEST, Jonathan Cameron wrote:
> >> >> +/*
> >> >> + * The gain selector encodes (PD_D4 << 2) | GAIN to identify each gain setting.
> >> >> + * Gains are multiplied by 8 to work with integers. The values in the iio-gts
> >> >> + * tables don't need corrections because the maximum value of the scale refers
> >> >> + * to GAIN = x1, and the rest of the values are obtained from the resulting
> >> >> + * linear function.
> >> >> + * TODO: add support for MILLI_GAIN_X165 and MILLI_GAIN_X660
> >> >> + */
> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X125  0x07
> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X250  0x04
> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X500  0x03
> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X1000 0x00
> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X2000 0x01  
> >> >
> >> > Not sure if these one-time use definitions improve or not the readability
> >> > of the code. Up to Jonathan.
> >> >  
> >>
> >> I prefer these definitions, and a similar pattern is used in multiple
> >> drivers in IIO, but I have no strong feelings about it.  
> >
> > Looking again at this, what do the numbers in the defines actually mean?
> > Seems a bit odd to have the base gain of 1 being called X125.
> > Maybe a comment on that would be useful.  I don't mind either way
> > on defines for this but if that number is useful to have I'd rather
> > have a define than a comment on each line.
> >  
> 
> I thought that MILLI_GAIN was already documenting what x125 is: 0.125 =
> 125 milli. More than the base gain of 1, it is the lowest gain you can
> configure.

Ah. The X location maybe what meant I didn't figure it out.
For similar the past we've done _0_125 .... _2_000 
which might be less confusing?

There isn't really a perfect answer for this stuff.

J
> 
> >>  
> >> >> +static const struct iio_gain_sel_pair veml6031x00_gain_sel[] = {
> >> >> +	GAIN_SCALE_GAIN(1, VEML6031X00_SEL_MILLI_GAIN_X125),
> >> >> +	GAIN_SCALE_GAIN(2, VEML6031X00_SEL_MILLI_GAIN_X250),
> >> >> +	GAIN_SCALE_GAIN(4, VEML6031X00_SEL_MILLI_GAIN_X500),
> >> >> +	GAIN_SCALE_GAIN(8, VEML6031X00_SEL_MILLI_GAIN_X1000),
> >> >> +	GAIN_SCALE_GAIN(16, VEML6031X00_SEL_MILLI_GAIN_X2000),
> >> >> +};  
> >> >
> >> > ...  
> 
> 

Re: [PATCH v4 2/4] iio: light: add support for veml6031x00 ALS series

[Javier Carrasco]

On Tue Jun 2, 2026 at 4:29 PM CEST, Jonathan Cameron wrote:
> On Tue, 02 Jun 2026 14:40:47 +0200
> "Javier Carrasco" <javier.carrasco.cruz@gmail.com> wrote:
>
>> On Tue Jun 2, 2026 at 2:38 PM CEST, Jonathan Cameron wrote:
>> >> >> +/*
>> >> >> + * The gain selector encodes (PD_D4 << 2) | GAIN to identify each gain setting.
>> >> >> + * Gains are multiplied by 8 to work with integers. The values in the iio-gts
>> >> >> + * tables don't need corrections because the maximum value of the scale refers
>> >> >> + * to GAIN = x1, and the rest of the values are obtained from the resulting
>> >> >> + * linear function.
>> >> >> + * TODO: add support for MILLI_GAIN_X165 and MILLI_GAIN_X660
>> >> >> + */
>> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X125  0x07
>> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X250  0x04
>> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X500  0x03
>> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X1000 0x00
>> >> >> +#define VEML6031X00_SEL_MILLI_GAIN_X2000 0x01
>> >> >
>> >> > Not sure if these one-time use definitions improve or not the readability
>> >> > of the code. Up to Jonathan.
>> >> >
>> >>
>> >> I prefer these definitions, and a similar pattern is used in multiple
>> >> drivers in IIO, but I have no strong feelings about it.
>> >
>> > Looking again at this, what do the numbers in the defines actually mean?
>> > Seems a bit odd to have the base gain of 1 being called X125.
>> > Maybe a comment on that would be useful.  I don't mind either way
>> > on defines for this but if that number is useful to have I'd rather
>> > have a define than a comment on each line.
>> >
>>
>> I thought that MILLI_GAIN was already documenting what x125 is: 0.125 =
>> 125 milli. More than the base gain of 1, it is the lowest gain you can
>> configure.
>
> Ah. The X location maybe what meant I didn't figure it out.
> For similar the past we've done _0_125 .... _2_000
> which might be less confusing?
>
> There isn't really a perfect answer for this stuff.
>
> J

If that has been done in the past, I am fine with dropping _MILLI_ and
using _0_125 and so on for V5.

Best regards,
Javier