* [PATCH v3 1/4] dt-bindings: iio: light: veml6030: add veml6031x00 ALS series
2026-05-24 21:53 [PATCH v3 0/4] iio: light: add support for veml6031x00 ALS series Javier Carrasco
@ 2026-05-24 21:53 ` Javier Carrasco
2026-05-24 22:11 ` sashiko-bot
2026-05-24 21:53 ` [PATCH v3 2/4] iio: light: add support for " Javier Carrasco
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Javier Carrasco @ 2026-05-24 21:53 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Rishi Gupta, David Lechner,
Nuno Sá, Andy Shevchenko, Matti Vaittinen
Cc: linux-iio, devicetree, linux-kernel, Javier Carrasco,
Jonathan Cameron, Krzysztof Kozlowski
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..94a23bac568d 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -28381,6 +28381,11 @@ S: Maintained
F: Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
F: drivers/iio/light/veml6046x00.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 VEML6075 UVA AND UVB LIGHT SENSOR DRIVER
M: Javier Carrasco <javier.carrasco.cruz@gmail.com>
S: Maintained
--
2.43.0
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 1/4] dt-bindings: iio: light: veml6030: add veml6031x00 ALS series
2026-05-24 21:53 ` [PATCH v3 1/4] dt-bindings: iio: light: veml6030: add " Javier Carrasco
@ 2026-05-24 22:11 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-05-24 22:11 UTC (permalink / raw)
To: Javier Carrasco; +Cc: devicetree, conor+dt, robh
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] The new entry in the MAINTAINERS file breaks alphabetical sorting.
--
commit c75afbde2cfc8b7f8986709a43e0d1e7b6d9ebe5
Author: Javier Carrasco <javier.carrasco.cruz@gmail.com>
dt-bindings: iio: light: veml6030: add veml6031x00 ALS series
This commit updates the veml6030 bindings to include the veml6031x00 ambient
light sensor series. It adds the new compatible strings and creates a new
entry in the MAINTAINERS file for the driver.
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2fb1c75afd163..94a23bac568d3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -28381,6 +28381,11 @@ S: Maintained
> F: Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
> F: drivers/iio/light/veml6046x00.c
>
> +VISHAY VEML6031X00 AMBIENT LIGHT SENSOR DRIVER
[Severity: Low]
This isn't a bug, but does this insertion break the alphabetical sorting of
the MAINTAINERS file?
Since "VEML6031" comes before "VEML6046", should this new block be placed
above the VISHAY VEML6046X00 entry to maintain the standard lexicographical
ordering?
> +M: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> +S: Maintained
> +F: Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
> +
> VISHAY VEML6075 UVA AND UVB LIGHT SENSOR DRIVER
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260524-veml6031x00-v3-0-29165609b2b5@gmail.com?part=1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 2/4] iio: light: add support for veml6031x00 ALS series
2026-05-24 21:53 [PATCH v3 0/4] iio: light: add support for veml6031x00 ALS series Javier Carrasco
2026-05-24 21:53 ` [PATCH v3 1/4] dt-bindings: iio: light: veml6030: add " Javier Carrasco
@ 2026-05-24 21:53 ` Javier Carrasco
2026-05-24 22:53 ` sashiko-bot
2026-05-24 21:53 ` [PATCH v3 3/4] iio: light: veml6031x00: add support for triggered buffers Javier Carrasco
2026-05-24 21:53 ` [PATCH v3 4/4] iio: light: veml6031x00: add support for events and trigger Javier Carrasco
3 siblings, 1 reply; 11+ messages in thread
From: Javier Carrasco @ 2026-05-24 21:53 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Rishi Gupta, David Lechner,
Nuno Sá, Andy Shevchenko, Matti Vaittinen
Cc: linux-iio, devicetree, linux-kernel, Javier Carrasco,
Jonathan Cameron
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 | 657 ++++++++++++++++++++++++++++++++++++++++
4 files changed, 671 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index 94a23bac568d..47da46717c16 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -28385,6 +28385,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 VEML6075 UVA AND UVB LIGHT SENSOR DRIVER
M: Javier Carrasco <javier.carrasco.cruz@gmail.com>
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..50979d239230
--- /dev/null
+++ b/drivers/iio/light/veml6031x00.c
@@ -0,0 +1,657 @@
+// 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/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/units.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.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;
+};
+
+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;
+
+ 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;
+
+ 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;
+
+ 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;
+
+ 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;
+ }
+
+ 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)
+ 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, ®, 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)
+{
+ 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);
+ 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, ®,
+ 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 = 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");
+
+ 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");
+
+ /* 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");
+
+ 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 2/4] iio: light: add support for veml6031x00 ALS series
2026-05-24 21:53 ` [PATCH v3 2/4] iio: light: add support for " Javier Carrasco
@ 2026-05-24 22:53 ` sashiko-bot
2026-05-25 0:29 ` Javier Carrasco
0 siblings, 1 reply; 11+ messages in thread
From: sashiko-bot @ 2026-05-24 22:53 UTC (permalink / raw)
To: Javier Carrasco; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Mixing `devm_pm_runtime_get_noresume()` with manual `pm_runtime_put_autosuspend()` causes a PM usage counter underflow.
- [High] `veml6031x00_write_raw()` modifies hardware registers over I2C without acquiring a runtime PM reference.
- [High] The shutdown devres action performs I2C writes without ensuring the device and its parent are resumed.
- [Medium] Missing locks in `veml6031x00_read_raw()` for `IIO_CHAN_INFO_SCALE` and `IIO_CHAN_INFO_INT_TIME` allow torn reads of configuration fields.
- [Medium] Incorrect scale attribute exposure for the IR channel and failure to mark shared hardware attributes.
- [Medium] The driver unconditionally sleeps for the integration time on every raw read, introducing unnecessary latency.
--
commit 02aa1fda2e77e393230904b3d122bdbb99445c9f
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.
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.
> diff --git a/drivers/iio/light/veml6031x00.c b/drivers/iio/light/veml6031x00.c
> new file mode 100644
> index 0000000000000..50979d2392301
> --- /dev/null
> +++ b/drivers/iio/light/veml6031x00.c
> @@ -0,0 +1,657 @@
[ ... ]
> +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),
[Severity: Medium]
Does this code expose hardware gain globally or independently per channel?
If hardware gain (gain and pd_div4) is globally applied to the entire chip,
writing to a separate attribute must not silently alter another channel's
state. Should shared hardware settings be exposed via info_mask_shared_by_all
instead?
Additionally, the scale value returned for the IR channel is computed using a
lux-to-count base which might be physically meaningless for raw IR intensity.
[ ... ]
> +static int veml6031x00_single_read(struct iio_dev *iio, enum iio_chan_type type,
> + int *val)
> +{
[ ... ]
> + 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)
> + 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));
[Severity: Medium]
Does this unconditionally sleep for the integration time on every raw read?
Since the device measures continuously when active, this delay is likely only
needed if the device was just resumed from sleep. Forcing this delay on every
read unnecessarily throttles the maximum polling read rate.
Could this be optimized by manually calling pm_runtime_get_sync() and checking
its return value (which returns 1 if already active) to apply the sleep only
when strictly necessary?
[ ... ]
> +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);
[Severity: Medium]
Could concurrent reads and writes cause torn reads of configuration fields
here?
When handling IIO_CHAN_INFO_SCALE or IIO_CHAN_INFO_INT_TIME, this function
does not acquire the IIO direct mode lock or any other mutex. It performs
multiple non-atomic regmap_field_read() operations (e.g., reading both
gain and pd_div4 in veml6031x00_get_scale()).
If a sysfs read interleaves with a write, the read might fetch a transient,
inconsistent mix of old and new register bits.
[ ... ]
> +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);
> + case IIO_CHAN_INFO_SCALE:
> + return veml6031x00_set_scale(iio, val, val2);
[Severity: High]
Does this code modify hardware registers over I2C without acquiring a runtime
PM reference?
Unlike veml6031x00_single_read(), this function directly writes to the
sensor's configuration registers without a PM reference. Because the regmap is
not configured with .pm_runtime = true, I2C transactions may be attempted
while the client device (or its parent controller) is runtime suspended, which
can trigger bus transfer errors.
[ ... ]
> +static int veml6031x00_probe(struct i2c_client *i2c)
> +{
[ ... ]
> + 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");
[Severity: High]
Can this shutdown devres action perform I2C writes while the device is
suspended?
Because this devres action is registered before
devm_pm_runtime_set_active_enabled(), on driver unbind, PM is disabled and the
device state is forced to suspended before the shutdown action executes due to
devres LIFO execution order.
The shutdown action then attempts to perform I2C writes to the shutdown
register. Accessing the I2C bus while the device state is suspended can cause
bus errors or hangs.
[ ... ]
> + 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_hw_init(iio);
> + if (ret)
> + return ret;
> +
> + pm_runtime_put_autosuspend(dev);
[Severity: High]
Does mixing devm_pm_runtime_get_noresume() with manual
pm_runtime_put_autosuspend() cause a PM usage counter underflow?
The devm_pm_runtime_get_noresume(dev) call increments the PM usage counter
and registers a devres action to decrement it on driver unbind.
However, the explicit pm_runtime_put_autosuspend(dev) decrements the usage
counter to its pre-get state. When the driver is subsequently removed (or if
probe fails after the put), the devres action will blindly decrement the
counter a second time, resulting in an underflow.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260524-veml6031x00-v3-0-29165609b2b5@gmail.com?part=2
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 2/4] iio: light: add support for veml6031x00 ALS series
2026-05-24 22:53 ` sashiko-bot
@ 2026-05-25 0:29 ` Javier Carrasco
0 siblings, 0 replies; 11+ messages in thread
From: Javier Carrasco @ 2026-05-25 0:29 UTC (permalink / raw)
To: sashiko-reviews, Javier Carrasco; +Cc: robh, conor+dt, devicetree
On Mon May 25, 2026 at 12:53 AM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] Mixing `devm_pm_runtime_get_noresume()` with manual `pm_runtime_put_autosuspend()` causes a PM usage counter underflow.
> - [High] `veml6031x00_write_raw()` modifies hardware registers over I2C without acquiring a runtime PM reference.
> - [High] The shutdown devres action performs I2C writes without ensuring the device and its parent are resumed.
> - [Medium] Missing locks in `veml6031x00_read_raw()` for `IIO_CHAN_INFO_SCALE` and `IIO_CHAN_INFO_INT_TIME` allow torn reads of configuration fields.
> - [Medium] Incorrect scale attribute exposure for the IR channel and failure to mark shared hardware attributes.
> - [Medium] The driver unconditionally sleeps for the integration time on every raw read, introducing unnecessary latency.
> --
...
>> +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);
>> + case IIO_CHAN_INFO_SCALE:
>> + return veml6031x00_set_scale(iio, val, val2);
>
> [Severity: High]
> Does this code modify hardware registers over I2C without acquiring a runtime
> PM reference?
>
> Unlike veml6031x00_single_read(), this function directly writes to the
> sensor's configuration registers without a PM reference. Because the regmap is
> not configured with .pm_runtime = true, I2C transactions may be attempted
> while the client device (or its parent controller) is runtime suspended, which
> can trigger bus transfer errors.
>
The registers are always accessible as long as the device is powered,
both in normal operation and power down. Therefore, this function cannot
be compared to veml6031x00_single_read() that acquires a PM reference to
carry out a measurement, and the write operations work fine.
>> +static int veml6031x00_probe(struct i2c_client *i2c)
>> +{
>
> [ ... ]
>
>> + 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");
>
> [Severity: High]
> Can this shutdown devres action perform I2C writes while the device is
> suspended?
>
> Because this devres action is registered before
> devm_pm_runtime_set_active_enabled(), on driver unbind, PM is disabled and the
> device state is forced to suspended before the shutdown action executes due to
> devres LIFO execution order.
>
> The shutdown action then attempts to perform I2C writes to the shutdown
> register. Accessing the I2C bus while the device state is suspended can cause
> bus errors or hangs.
>
> [ ... ]
>
>> + 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_hw_init(iio);
>> + if (ret)
>> + return ret;
>> +
>> + pm_runtime_put_autosuspend(dev);
>
> [Severity: High]
> Does mixing devm_pm_runtime_get_noresume() with manual
> pm_runtime_put_autosuspend() cause a PM usage counter underflow?
>
> The devm_pm_runtime_get_noresume(dev) call increments the PM usage counter
> and registers a devres action to decrement it on driver unbind.
>
> However, the explicit pm_runtime_put_autosuspend(dev) decrements the usage
> counter to its pre-get state. When the driver is subsequently removed (or if
> probe fails after the put), the devres action will blindly decrement the
> counter a second time, resulting in an underflow.
I believe Sashiko is misinterpreting how this pattern works (not
introduced by me, it is already available upstream in
drivers/spi/atmel-quadspi.c). Decrementing the usage counter is fine
because the devres action uses pm_runtime_put_noidle(), which in turns
uses atomic_add_unless(usage_count, -1, 0) to avoid decrementing the
usage_count if it is already 0. Sashiko has reported the same issue in
other patches of this series.
Best,
Javier
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 3/4] iio: light: veml6031x00: add support for triggered buffers
2026-05-24 21:53 [PATCH v3 0/4] iio: light: add support for veml6031x00 ALS series Javier Carrasco
2026-05-24 21:53 ` [PATCH v3 1/4] dt-bindings: iio: light: veml6030: add " Javier Carrasco
2026-05-24 21:53 ` [PATCH v3 2/4] iio: light: add support for " Javier Carrasco
@ 2026-05-24 21:53 ` Javier Carrasco
2026-05-24 23:29 ` sashiko-bot
2026-05-24 21:53 ` [PATCH v3 4/4] iio: light: veml6031x00: add support for events and trigger Javier Carrasco
3 siblings, 1 reply; 11+ messages in thread
From: Javier Carrasco @ 2026-05-24 21:53 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Rishi Gupta, David Lechner,
Nuno Sá, Andy Shevchenko, Matti Vaittinen
Cc: linux-iio, devicetree, linux-kernel, Javier Carrasco,
Jonathan Cameron
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 | 106 ++++++++++++++++++++++++++++++++++++++++
2 files changed, 108 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 50979d239230..9968d4414dc9 100644
--- a/drivers/iio/light/veml6031x00.c
+++ b/drivers/iio/light/veml6031x00.c
@@ -14,6 +14,8 @@
#include <linux/iio/iio.h>
#include <linux/iio/sysfs.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
@@ -29,6 +31,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;
@@ -128,6 +136,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,
@@ -138,7 +153,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[] = {
@@ -461,6 +484,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 (*iio->active_scan_mask == (BIT(VEML6031X00_SCAN_ALS) |
+ BIT(VEML6031X00_SCAN_IR))) {
+ 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;
@@ -564,6 +663,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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 3/4] iio: light: veml6031x00: add support for triggered buffers
2026-05-24 21:53 ` [PATCH v3 3/4] iio: light: veml6031x00: add support for triggered buffers Javier Carrasco
@ 2026-05-24 23:29 ` sashiko-bot
0 siblings, 0 replies; 11+ messages in thread
From: sashiko-bot @ 2026-05-24 23:29 UTC (permalink / raw)
To: Javier Carrasco; +Cc: conor+dt, robh, devicetree
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The trigger handler does not skip the soft timestamp channel, leading to an out-of-bounds array write and an invalid I2C read.
- [Medium] Runtime PM usage counter underflow on device unbind due to mixing devm_pm_runtime_get_noresume() with a manual pm_runtime_put_autosuspend().
--
commit 7b07348868ecc638382ae3a2ca8a3bea1a0c8680
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 50979d2392301..9968d4414dc96 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c
[ ... ]
> @@ -461,6 +484,82 @@ static const struct iio_info veml6031x00_info = {
> .write_raw_get_fmt = veml6031x00_write_raw_get_fmt,
> };
[ ... ]
> +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 (*iio->active_scan_mask == (BIT(VEML6031X00_SCAN_ALS) |
> + BIT(VEML6031X00_SCAN_IR))) {
> + 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 properly handle the timestamp channel?
If the user enables the timestamp channel alongside ALS and IR, the timestamp
bit is included in iio->active_scan_mask. The optimization check above will
evaluate to false, falling back to this else block.
Inside this loop, the timestamp channel isn't skipped. Since the soft
timestamp channel has no assigned address, does it default to 0 and
cause an unintended I2C read of the configuration register?
Additionally, scan.chans is defined as an array of 2 elements. Could this
cause an out-of-bounds write to scan.chans[2] as i increments, corrupting
the struct padding or timestamp field?
> + 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;
> +}
[ ... ]
> @@ -564,6 +663,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: Medium]
Can this manual pm_runtime_put_autosuspend() cause a usage counter underflow
on device unbind?
The driver acquires a runtime PM reference earlier using
devm_pm_runtime_get_noresume(), which registers a devres cleanup action to
decrement the usage counter on unbind.
With this manual put in probe, will the devres unbind action drop the
counter below zero and trigger a warning in the PM core?
>
> ret = devm_iio_device_register(dev, iio);
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260524-veml6031x00-v3-0-29165609b2b5@gmail.com?part=3
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 4/4] iio: light: veml6031x00: add support for events and trigger
2026-05-24 21:53 [PATCH v3 0/4] iio: light: add support for veml6031x00 ALS series Javier Carrasco
` (2 preceding siblings ...)
2026-05-24 21:53 ` [PATCH v3 3/4] iio: light: veml6031x00: add support for triggered buffers Javier Carrasco
@ 2026-05-24 21:53 ` Javier Carrasco
2026-05-25 0:13 ` sashiko-bot
3 siblings, 1 reply; 11+ messages in thread
From: Javier Carrasco @ 2026-05-24 21:53 UTC (permalink / raw)
To: Jonathan Cameron, Lars-Peter Clausen, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Rishi Gupta, David Lechner,
Nuno Sá, Andy Shevchenko, Matti Vaittinen
Cc: linux-iio, devicetree, linux-kernel, Javier Carrasco,
Jonathan Cameron
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 9968d4414dc9..c57aaa29abf3 100644
--- a/drivers/iio/light/veml6031x00.c
+++ b/drivers/iio/light/veml6031x00.c
@@ -7,12 +7,16 @@
#include <linux/bitfield.h>
#include <linux/i2c.h>
+#include <linux/interrupt.h>
#include <linux/module.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/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>
@@ -20,16 +24,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,
@@ -39,8 +56,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 {
@@ -52,8 +71,14 @@ 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;
+ /* serialize access to irq enable/disable by events and trigger */
+ struct mutex lock;
+ int int_users;
+ bool ev_en;
+ bool trig_en;
};
static const struct iio_itime_sel_mul veml6031x00_it_sel[] = {
@@ -88,6 +113,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.
@@ -127,6 +163,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,
@@ -136,6 +189,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',
@@ -166,7 +221,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 = {
@@ -175,7 +232,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 = {
@@ -185,6 +242,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 = {
@@ -192,6 +250,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,
@@ -199,13 +266,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);
@@ -224,6 +298,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);
@@ -234,6 +313,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;
}
@@ -313,6 +397,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, ®);
+ 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);
@@ -367,6 +475,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,
+ ®, sizeof(reg));
+ else
+ ret = regmap_bulk_read(data->regmap, VEML6031X00_REG_WL_L,
+ ®, 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,
+ ®, 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,
+ ®, 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)
{
@@ -477,13 +630,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->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->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->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, ®);
+ 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->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);
@@ -518,11 +847,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->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;
@@ -581,7 +953,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,
@@ -593,6 +966,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, ®, 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, ®, 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;
}
@@ -618,6 +1039,10 @@ static int veml6031x00_probe(struct i2c_client *i2c)
data->dev = dev;
data->regmap = regmap;
+ ret = devm_mutex_init(dev, &data->lock);
+ if (ret)
+ return ret;
+
ret = veml6031x00_regfield_init(data);
if (ret)
return dev_err_probe(dev, ret, "Failed to init regfield\n");
@@ -657,12 +1082,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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v3 4/4] iio: light: veml6031x00: add support for events and trigger
2026-05-24 21:53 ` [PATCH v3 4/4] iio: light: veml6031x00: add support for events and trigger Javier Carrasco
@ 2026-05-25 0:13 ` sashiko-bot
2026-05-25 8:59 ` Javier Carrasco
0 siblings, 1 reply; 11+ messages in thread
From: sashiko-bot @ 2026-05-25 0:13 UTC (permalink / raw)
To: Javier Carrasco; +Cc: robh, conor+dt, devicetree
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
New issues:
- [High] Cache pollution of AF_TRIG causes unintended conversions and unnecessary I2C reads.
- [High] The trigger reference acquired via iio_trigger_get() is leaked on probe error paths and driver unbind.
- [High] PM runtime usage counter leaks if the driver unbinds while an event is enabled.
- [High] Sysfs read/write functions and trigger operations access hardware without acquiring a PM runtime reference.
- [High] Out-of-bounds array write in veml6031x00_trig_handler when the timestamp channel is enabled.
- [Medium] Dead event sysfs attributes are exposed when no IRQ is present.
Pre-existing issues:
- [High] Unbalanced double-put of the runtime PM usage counter on driver unbind.
- [High] The shutdown action accesses I2C registers after PM runtime has been disabled.
--
commit cbdd69d1454af4788d5d18a23d3fb6854c9981e9
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 9968d4414dc96..c57aaa29abf30 100644
> --- a/drivers/iio/light/veml6031x00.c
> +++ b/drivers/iio/light/veml6031x00.c
[ ... ]
> @@ -136,6 +189,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 expose dead event sysfs attributes when no IRQ is present?
When i2c->irq is 0, the driver registers veml6031x00_info_no_irq which lacks
event callbacks. However, this channel array unconditionally defines
event_spec and num_event_specs. The IIO core will parse these and expose
event sysfs files. Reading or writing these files will fail with -EINVAL
because the callbacks are NULL, which violates the sysfs API contract.
[ ... ]
> +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)
> +{
[Severity: High]
Are PM runtime references needed here before accessing hardware?
This function, as well as veml6031x00_write_event_val() and
veml6031x00_set_trigger_state(), performs I2C accesses via regmap without
first calling pm_runtime_resume_and_get(). If the device is
runtime-suspended and its PM domain has powered it off, will these I2C
accesses fail or time out?
[ ... ]
> +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->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);
[Severity: High]
Could this leak the PM runtime usage counter if the driver unbinds?
Enabling an event increments the PM runtime usage counter here. If the
driver is unbound via sysfs while an event is enabled, the IIO core does
not automatically disable the event. Since there is no unbind cleanup
mechanism to release the pending PM reference when data->ev_en is true,
does this keep the device powered on indefinitely?
[ ... ]
> +/* 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));
> +}
[Severity: High]
Can this cause unintentional active force conversions due to cache pollution?
This sets the self-clearing AF_TRIG bit using regmap_update_bits().
Because CONF0 is not marked as a volatile register, regmap_update_bits()
writes AF_TRIG=1 and caches it. Any subsequent read-modify-write on CONF0
(e.g., toggling the interrupt or changing integration time) will read the
cached AF_TRIG=1 and write it back to hardware, unintentionally triggering
an active force conversion and causing hardware state corruption.
[ ... ]
> 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 (*iio->active_scan_mask == (BIT(VEML6031X00_SCAN_ALS) |
> BIT(VEML6031X00_SCAN_IR))) {
> 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++],
[Severity: High]
Does this loop cause an out-of-bounds array write if the timestamp
channel is enabled?
The iio_for_each_active_channel() loop does not exclude the soft timestamp
channel. If the user enables ALS, IR, and TIMESTAMP channels, the loop
iterates 3 times. However, the scan.chans array is declared as
__le16 chans[2]. Processing the timestamp channel increments the index i
to 2 and writes to scan.chans[2], causing an out-of-bounds memory write.
Furthermore, because the timestamp channel's address is 0, this incorrectly
attempts a bogus I2C read from VEML6031X00_REG_CONF0.
[ ... ]
> +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);
[Severity: High]
Is the trigger reference acquired here leaked on driver unbind?
Calling iio_trigger_get() increments the trigger device's refcount and the
driver module's refcount. If the driver fails probe afterwards or is unbound
later, iio_trigger_put() is never called because there is no devres action
registered for it and no custom remove callback. Does this permanently leak
the trigger device reference and prevent the module from being unloaded?
[ ... ]
> @@ -618,6 +1039,10 @@ static int veml6031x00_probe(struct i2c_client *i2c)
[ ... ]
> ret = devm_add_action_or_reset(dev, veml6031x00_als_shutdown_action, data);
[Severity: High]
This is a pre-existing issue, but does this shutdown action access I2C
registers after PM runtime has been disabled?
This action is registered via devm_add_action_or_reset() before PM runtime
is enabled. On teardown, devm actions run in reverse, meaning PM runtime is
disabled before the shutdown action runs. The shutdown action then attempts
I2C writes while PM runtime is disabled. If the device was suspended, it
cannot be resumed, potentially causing I2C transfer failures.
[ ... ]
> pm_runtime_put_autosuspend(dev);
[Severity: High]
This is a pre-existing issue, but is there an unbalanced double-put of the
runtime PM usage counter on driver unbind?
The probe function registers a cleanup action to drop the PM reference via
devm_pm_runtime_get_noresume(dev). However, this explicit put at the end
of probe means the devm action will drop the reference a second time on
unbind, causing a PM usage counter underflow that breaks the PM runtime
state machine.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260524-veml6031x00-v3-0-29165609b2b5@gmail.com?part=4
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH v3 4/4] iio: light: veml6031x00: add support for events and trigger
2026-05-25 0:13 ` sashiko-bot
@ 2026-05-25 8:59 ` Javier Carrasco
0 siblings, 0 replies; 11+ messages in thread
From: Javier Carrasco @ 2026-05-25 8:59 UTC (permalink / raw)
To: sashiko-reviews, Javier Carrasco; +Cc: robh, conor+dt, devicetree
On Mon May 25, 2026 at 2:13 AM CEST, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
>
> New issues:
> - [High] Cache pollution of AF_TRIG causes unintended conversions and unnecessary I2C reads.
> - [High] The trigger reference acquired via iio_trigger_get() is leaked on probe error paths and driver unbind.
> - [High] PM runtime usage counter leaks if the driver unbinds while an event is enabled.
> - [High] Sysfs read/write functions and trigger operations access hardware without acquiring a PM runtime reference.
> - [High] Out-of-bounds array write in veml6031x00_trig_handler when the timestamp channel is enabled.
> - [Medium] Dead event sysfs attributes are exposed when no IRQ is present.
>
> Pre-existing issues:
> - [High] Unbalanced double-put of the runtime PM usage counter on driver unbind.
> - [High] The shutdown action accesses I2C registers after PM runtime has been disabled.
> --
>
> commit cbdd69d1454af4788d5d18a23d3fb6854c9981e9
> 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 9968d4414dc96..c57aaa29abf30 100644
>> --- a/drivers/iio/light/veml6031x00.c
>> +++ b/drivers/iio/light/veml6031x00.c
>
> [ ... ]
>
>> @@ -136,6 +189,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 expose dead event sysfs attributes when no IRQ is present?
>
> When i2c->irq is 0, the driver registers veml6031x00_info_no_irq which lacks
> event callbacks. However, this channel array unconditionally defines
> event_spec and num_event_specs. The IIO core will parse these and expose
> event sysfs files. Reading or writing these files will fail with -EINVAL
> because the callbacks are NULL, which violates the sysfs API contract.
>
> [ ... ]
>
@Jonathan: is this how it really works? How can I then dynamically
define even_spec to account for it?
>> +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)
>> +{
>
> [Severity: High]
> Are PM runtime references needed here before accessing hardware?
>
> This function, as well as veml6031x00_write_event_val() and
> veml6031x00_set_trigger_state(), performs I2C accesses via regmap without
> first calling pm_runtime_resume_and_get(). If the device is
> runtime-suspended and its PM domain has powered it off, will these I2C
> accesses fail or time out?
>
> [ ... ]
>
As I said for [2/4], reading registers in shutdown mode is absolutely
fine.
>> +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->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);
>
> [Severity: High]
> Could this leak the PM runtime usage counter if the driver unbinds?
>
> Enabling an event increments the PM runtime usage counter here. If the
> driver is unbound via sysfs while an event is enabled, the IIO core does
> not automatically disable the event. Since there is no unbind cleanup
> mechanism to release the pending PM reference when data->ev_en is true,
> does this keep the device powered on indefinitely?
>
> [ ... ]
>
@Jonathan: Does the IIO core let the event enabled? What is the
approach to clear it then?
>> +/* 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));
>> +}
>
> [Severity: High]
> Can this cause unintentional active force conversions due to cache pollution?
>
> This sets the self-clearing AF_TRIG bit using regmap_update_bits().
> Because CONF0 is not marked as a volatile register, regmap_update_bits()
> writes AF_TRIG=1 and caches it. Any subsequent read-modify-write on CONF0
> (e.g., toggling the interrupt or changing integration time) will read the
> cached AF_TRIG=1 and write it back to hardware, unintentionally triggering
> an active force conversion and causing hardware state corruption.
>
> [ ... ]
>
I will define the CONF0 register as volatile to account for this.
>> 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 (*iio->active_scan_mask == (BIT(VEML6031X00_SCAN_ALS) |
>> BIT(VEML6031X00_SCAN_IR))) {
>> 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++],
>
> [Severity: High]
> Does this loop cause an out-of-bounds array write if the timestamp
> channel is enabled?
>
> The iio_for_each_active_channel() loop does not exclude the soft timestamp
> channel. If the user enables ALS, IR, and TIMESTAMP channels, the loop
> iterates 3 times. However, the scan.chans array is declared as
> __le16 chans[2]. Processing the timestamp channel increments the index i
> to 2 and writes to scan.chans[2], causing an out-of-bounds memory write.
>
> Furthermore, because the timestamp channel's address is 0, this incorrectly
> attempts a bogus I2C read from VEML6031X00_REG_CONF0.
>
> [ ... ]
>
Not sure if this is a real issue, and the address for channel 0 is not
CONF0 because it is defined in iio_chan_spec
veml6031x00_channels.address.
>> +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);
>
> [Severity: High]
> Is the trigger reference acquired here leaked on driver unbind?
>
> Calling iio_trigger_get() increments the trigger device's refcount and the
> driver module's refcount. If the driver fails probe afterwards or is unbound
> later, iio_trigger_put() is never called because there is no devres action
> registered for it and no custom remove callback. Does this permanently leak
> the trigger device reference and prevent the module from being unloaded?
>
> [ ... ]
>
Same here. I believe that only industrialio-trigger.c makes use of
iio_trigger_put().
>> @@ -618,6 +1039,10 @@ static int veml6031x00_probe(struct i2c_client *i2c)
>
> [ ... ]
>
>> ret = devm_add_action_or_reset(dev, veml6031x00_als_shutdown_action, data);
>
> [Severity: High]
> This is a pre-existing issue, but does this shutdown action access I2C
> registers after PM runtime has been disabled?
>
> This action is registered via devm_add_action_or_reset() before PM runtime
> is enabled. On teardown, devm actions run in reverse, meaning PM runtime is
> disabled before the shutdown action runs. The shutdown action then attempts
> I2C writes while PM runtime is disabled. If the device was suspended, it
> cannot be resumed, potentially causing I2C transfer failures.
>
> [ ... ]
>
>> pm_runtime_put_autosuspend(dev);
>
> [Severity: High]
> This is a pre-existing issue, but is there an unbalanced double-put of the
> runtime PM usage counter on driver unbind?
>
> The probe function registers a cleanup action to drop the PM reference via
> devm_pm_runtime_get_noresume(dev). However, this explicit put at the end
> of probe means the devm action will drop the reference a second time on
> unbind, causing a PM usage counter underflow that breaks the PM runtime
> state machine.
As I said for [2/4], there won't be any counter underflow that way.
Best,
Javier
^ permalink raw reply [flat|nested] 11+ messages in thread