[PATCH v3 0/2] iio: light: veml3328: add support for new sensor

[PATCH v3 0/2] iio: light: veml3328: add support for new sensor

[Joshua Crofts]

This patch series adds support for the Vishay VEML3328 RGBCIR light
sensor. The sensor communicates via I2C (SMBus compatible) and provides
5 types of 16-bit measurements: red, green, blue, clear and infrared.

Reasons for adding a new driver:
- Existing Vishay drivers in the kernel do not cover sensors that
  handle RGBC and IR simultaneously.
- The register map and configuration differ from other Vishay light
  sensors currently supported by IIO.

Testing:
- Tested on a Raspberry Pi 4 using a VEML3328 breakout board.

Datasheet:
https://www.vishay.com/docs/84968/veml3328.pdf

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
Changes in v2:
- Add additional IIO_LIGHT channel for ambient light sensing
- Remove separate dt binding file and added veml3328 entry to veml6030
  yaml
- Move driver to PM_RUNTIME_ACQUIRE_AUTOSUSPEND() macro
- Add missing headers
- Remov redundant mutex as regmap handles it itself
- Use regmap_set/clear_bits() instead of regmap_update_bits()
- Removed redundant dev pointer
- Edit commit messages
- Various code style cleanups
- Link to v1: https://patch.msgid.link/20260516-veml3328-v1-0-1d4b663e2fe3@gmail.com

Changes in v3:
- Add 2D array of precomputed scale values based on integration time
- Fix chan_spec masks
- Simplify read/write/avail callbacks

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>

---
Joshua Crofts (2):
      dt-bindings: iio: light: veml6030: add veml3328
      iio: light: veml3328: add support for new device

 .../bindings/iio/light/vishay,veml6030.yaml        |   5 +-
 MAINTAINERS                                        |   5 +
 drivers/iio/light/Kconfig                          |  11 +
 drivers/iio/light/Makefile                         |   1 +
 drivers/iio/light/veml3328.c                       | 413 +++++++++++++++++++++
 5 files changed, 434 insertions(+), 1 deletion(-)
---
base-commit: 7b84b1e9dd850a5c9b55e27daa4ecdc2dd5b3431
change-id: 20260530-veml3328-d2ccb26d02c7

Best regards,
-- 
Joshua Crofts <joshua.crofts1@gmail.com>

[PATCH v3 1/2] dt-bindings: iio: light: veml6030: add veml3328

[Joshua Crofts]

The Vishay VEML3328 is an RGBCIR light sensor that shares similar
devicetree properties as other existing VEMLxxxx sensors in the
kernel.

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml | 5 ++++-
 1 file changed, 4 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..0041e1db6838 100644
--- a/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
+++ b/Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
@@ -4,7 +4,7 @@
 $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, VEML3328, VEML6030, VEML6035 and VEML7700 Ambient Light Sensors (ALS)
 
 maintainers:
   - Rishi Gupta <gupt21@gmail.com>
@@ -21,6 +21,7 @@ description: |
 
   Specifications about the sensors can be found at:
     https://www.vishay.com/docs/80131/veml3235.pdf
+    https://www.vishay.com/docs/84968/veml3328.pdf
     https://www.vishay.com/docs/84366/veml6030.pdf
     https://www.vishay.com/docs/84889/veml6035.pdf
     https://www.vishay.com/docs/84286/veml7700.pdf
@@ -29,6 +30,7 @@ properties:
   compatible:
     enum:
       - vishay,veml3235
+      - vishay,veml3328
       - vishay,veml6030
       - vishay,veml6035
       - vishay,veml7700
@@ -79,6 +81,7 @@ allOf:
         compatible:
           enum:
             - vishay,veml3235
+            - vishay,veml3328
             - vishay,veml7700
     then:
       properties:

-- 
2.54.0

[PATCH v3 2/2] iio: light: veml3328: add support for new device

[Joshua Crofts]

Add support for the Vishay VEML3328 RGB/IR light sensor communicating
via I2C (SMBus compatible).

Also add a new entry for said driver into Kconfig and Makefile.

Assisted-by: Gemini:3.1-Pro
Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 MAINTAINERS                  |   5 +
 drivers/iio/light/Kconfig    |  11 ++
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/veml3328.c | 413 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 430 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9e9457c7bba6..a893a1a1181e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -28402,6 +28402,11 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
 F:	drivers/iio/light/veml3235.c
 
+VISHAY VEML3328 RGB IR LIGHT SENSOR DRIVER
+M:	Joshua Crofts <joshua.crofts1@gmail.com>
+S:	Maintained
+F:	drivers/iio/light/veml3328.c
+
 VISHAY VEML6030 AMBIENT LIGHT SENSOR DRIVER
 M:	Javier Carrasco <javier.carrasco.cruz@gmail.com>
 S:	Maintained
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index da4807a3fd3d..ef36824f312f 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -713,6 +713,17 @@ config VEML3235
 	  To compile this driver as a module, choose M here: the
 	  module will be called veml3235.
 
+config VEML3328
+	tristate "VEML3328 RGBCIR light sensor"
+	select REGMAP_I2C
+	depends on I2C
+	help
+	  Say Y here if you want to build a driver for the Vishay VEML3328
+	  RGB IR light sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called veml3328
+
 config VEML6030
 	tristate "VEML6030 and VEML6035 ambient light sensors"
 	select REGMAP_I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 39e62dfc10c7..64e354c49ed8 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -66,6 +66,7 @@ obj-$(CONFIG_US5182D)		+= us5182d.o
 obj-$(CONFIG_VCNL4000)		+= vcnl4000.o
 obj-$(CONFIG_VCNL4035)		+= vcnl4035.o
 obj-$(CONFIG_VEML3235)		+= veml3235.o
+obj-$(CONFIG_VEML3328)		+= veml3328.o
 obj-$(CONFIG_VEML6030)		+= veml6030.o
 obj-$(CONFIG_VEML6040)		+= veml6040.o
 obj-$(CONFIG_VEML6046X00)	+= veml6046x00.o
diff --git a/drivers/iio/light/veml3328.c b/drivers/iio/light/veml3328.c
new file mode 100644
index 000000000000..1def67fd9b51
--- /dev/null
+++ b/drivers/iio/light/veml3328.c
@@ -0,0 +1,413 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Vishay VEML3328 RGBCIR light sensor driver
+ *
+ * Copyright (c) 2026 Joshua Crofts <joshua.crofts1@gmail.com>
+ *
+ * Datasheet: https://www.vishay.com/docs/84968/veml3328.pdf
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/time.h>
+#include <linux/types.h>
+
+#include <linux/iio/iio.h>
+
+#define VEML3328_REG_CONF		0x00
+#define VEML3328_REG_ID			0x0c
+#define VEML3328_REG_DATA_C		0x04
+#define VEML3328_REG_DATA_R		0x05
+#define VEML3328_REG_DATA_G		0x06
+#define VEML3328_REG_DATA_B		0x07
+#define VEML3328_REG_DATA_IR		0x08
+
+#define VEML3328_CONF_IT_MASK		GENMASK(5, 4)
+#define VEML3328_CONF_GAIN_MASK		GENMASK(11, 10)
+
+#define VEML3328_MAX_IT_TIME		(400 * USEC_PER_MSEC)
+
+#define VEML3328_ID_VAL			0x28
+
+#define VEML3328_SHUTDOWN		(BIT(0) | BIT(15))
+
+struct veml3328_data {
+	struct regmap *regmap;
+};
+
+static const struct regmap_config veml3328_regmap_config = {
+	.name = "veml3328",
+	.reg_bits = 8,
+	.val_bits = 16,
+	.max_register = VEML3328_REG_ID,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+};
+
+#define VEML3328_CHAN_SPEC(_color, _addr) { \
+	.type = IIO_INTENSITY, \
+	.modified = 1, \
+	.channel2 = IIO_MOD_LIGHT_##_color, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+	.info_mask_shared_by_type_available = 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), \
+	.address = _addr, \
+}
+
+static const struct iio_chan_spec veml3328_channels[] = {
+	{
+		.type = IIO_LIGHT,
+
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_type_available = 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),
+		.address = VEML3328_REG_DATA_G,
+	},
+	VEML3328_CHAN_SPEC(CLEAR, VEML3328_REG_DATA_C),
+	VEML3328_CHAN_SPEC(RED, VEML3328_REG_DATA_R),
+	VEML3328_CHAN_SPEC(GREEN, VEML3328_REG_DATA_G),
+	VEML3328_CHAN_SPEC(BLUE, VEML3328_REG_DATA_B),
+	VEML3328_CHAN_SPEC(IR, VEML3328_REG_DATA_IR),
+};
+
+/*
+ * Precomputed scale values (micro units).
+ * Formula for calculation: 0.384 * (50000 / IT_us) * (1 / Gain)
+ * Gain indexes: 0 (x0.5), 1 (x1), 2 (x2), 3 (x4)
+ * IT indexes: 0 (50ms), 1 (100ms), 2 (200ms), 3 (400ms)
+ */
+static const int veml3328_scale_vals[4][8] = {
+	{ 0, 768000, 0, 384000, 0, 192000, 0, 96000 },
+	{ 0, 384000, 0, 192000, 0, 96000,  0, 48000 },
+	{ 0, 192000, 0, 96000,  0, 48000,  0, 24000 },
+	{ 0, 96000,  0, 48000,  0, 24000,  0, 12000 },
+};
+
+/* integration times in microseconds */
+static const int veml3328_it_times[][2] = {
+	{ 0, 50 * USEC_PER_MSEC },
+	{ 0, 100 * USEC_PER_MSEC },
+	{ 0, 200 * USEC_PER_MSEC },
+	{ 0, 400 * USEC_PER_MSEC },
+};
+
+static int veml3328_power_down(struct veml3328_data *data)
+{
+	return regmap_set_bits(data->regmap, VEML3328_REG_CONF,
+			       VEML3328_SHUTDOWN);
+}
+
+static int veml3328_power_up(struct veml3328_data *data)
+{
+	int ret;
+
+	ret = regmap_clear_bits(data->regmap, VEML3328_REG_CONF,
+				VEML3328_SHUTDOWN);
+	if (ret)
+		return ret;
+
+	/*
+	 * Sleep for maximum integration time to ensure sensor is powered on
+	 * correctly.
+	 */
+	fsleep(VEML3328_MAX_IT_TIME);
+
+	return 0;
+}
+
+static void veml3328_power_down_action(void *data)
+{
+	veml3328_power_down(data);
+}
+
+static int veml3328_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val, int *val2, long mask)
+{
+	struct veml3328_data *data = iio_priv(indio_dev);
+	struct regmap *regmap = data->regmap;
+	struct device *dev = regmap_get_device(regmap);
+	unsigned int reg_val;
+	int it_inx, gain_inx;
+	int ret;
+
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_read(regmap, chan->address, &reg_val);
+		if (ret)
+			return ret;
+
+		*val = reg_val;
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
+		if (ret)
+			return ret;
+
+		it_inx = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);
+		if (it_inx >= ARRAY_SIZE(veml3328_it_times))
+			return -EINVAL;
+
+		*val = veml3328_it_times[it_inx][0];
+		*val2 = veml3328_it_times[it_inx][1];
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
+		if (ret)
+			return ret;
+
+		it_inx = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);
+		gain_inx = FIELD_GET(VEML3328_CONF_GAIN_MASK, reg_val);
+
+		if (it_inx >= ARRAY_SIZE(veml3328_it_times) || gain_inx >= 4)
+			return -EINVAL;
+
+		/* Stride by 2 through the flattened array to match (val, val2) */
+		*val = veml3328_scale_vals[it_inx][gain_inx * 2];
+		*val2 = veml3328_scale_vals[it_inx][gain_inx * 2 + 1];
+
+		return IIO_VAL_INT_PLUS_MICRO;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml3328_read_avail(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       const int **vals, int *type, int *length,
+			       long mask)
+{
+	struct veml3328_data *data = iio_priv(indio_dev);
+	struct regmap *regmap = data->regmap;
+	struct device *dev = regmap_get_device(data->regmap);
+	unsigned int reg_val;
+	int ret, it_inx;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		*length = ARRAY_SIZE(veml3328_it_times) * 2;
+		*vals = (const int *)veml3328_it_times;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+
+	case IIO_CHAN_INFO_SCALE:
+		PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm);
+		ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+		if (ret)
+			return ret;
+
+		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
+		if (ret)
+			return ret;
+
+		it_inx = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);
+		if (it_inx >= ARRAY_SIZE(veml3328_it_times))
+			return -EINVAL;
+
+		*length = 8;
+		*vals = (const int *)veml3328_scale_vals[it_inx];
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml3328_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val, int val2, long mask)
+{
+	struct veml3328_data *data = iio_priv(indio_dev);
+	struct regmap *regmap = data->regmap;
+	struct device *dev = regmap_get_device(regmap);
+	unsigned int reg_val;
+	int i, it_inx;
+	int ret;
+
+	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		if (val != 0)
+			return -EINVAL;
+
+		for (i = 0; i < ARRAY_SIZE(veml3328_it_times); i++) {
+			if (veml3328_it_times[i][1] == val2)
+				break;
+		}
+
+		if (i == ARRAY_SIZE(veml3328_it_times))
+			return -EINVAL;
+
+		return regmap_update_bits(regmap, VEML3328_REG_CONF,
+					  VEML3328_CONF_IT_MASK,
+					  FIELD_PREP(VEML3328_CONF_IT_MASK, i));
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
+		if (ret)
+			return ret;
+
+		it_inx = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);
+		if (it_inx >= ARRAY_SIZE(veml3328_it_times))
+			return -EINVAL;
+
+		for (i = 0; i < 4; i++) {
+			if (val == veml3328_scale_vals[it_inx][i * 2] &&
+			    val2 == veml3328_scale_vals[it_inx][i * 2 + 1])
+				break;
+		}
+
+		if (i == 4)
+			return -EINVAL;
+
+		return regmap_update_bits(regmap, VEML3328_REG_CONF,
+					  VEML3328_CONF_GAIN_MASK,
+					  FIELD_PREP(VEML3328_CONF_GAIN_MASK, i));
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info veml3328_info = {
+	.read_raw = veml3328_read_raw,
+	.write_raw = veml3328_write_raw,
+	.read_avail = veml3328_read_avail,
+};
+
+static int veml3328_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct veml3328_data *data;
+	struct iio_dev *indio_dev;
+	unsigned int reg_val;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+
+	data->regmap = devm_regmap_init_i2c(client, &veml3328_regmap_config);
+	if (IS_ERR(data->regmap))
+		return dev_err_probe(dev, PTR_ERR(data->regmap),
+				     "Failed to initialize regmap\n");
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulator\n");
+
+	ret = regmap_read(data->regmap, VEML3328_REG_ID, &reg_val);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to read ID register\n");
+
+	if ((reg_val & 0xff) != VEML3328_ID_VAL)
+		dev_warn(dev, "Unknown device ID: 0x%02x\n", reg_val & 0xff);
+
+	ret = veml3328_power_up(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to power on sensor\n");
+
+	ret = devm_add_action_or_reset(dev, veml3328_power_down_action, data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register teardown\n");
+
+	indio_dev->name = "veml3328";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &veml3328_info;
+	indio_dev->channels = veml3328_channels;
+	indio_dev->num_channels = ARRAY_SIZE(veml3328_channels);
+
+	pm_runtime_set_active(dev);
+	pm_runtime_set_autosuspend_delay(dev, 2000);
+	pm_runtime_use_autosuspend(dev);
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static int veml3328_runtime_suspend(struct device *dev)
+{
+	struct veml3328_data *data = iio_priv(dev_get_drvdata(dev));
+	int ret;
+
+	ret = veml3328_power_down(data);
+	if (ret)
+		dev_err(dev, "Failed to suspend: %d\n", ret);
+
+	return ret;
+}
+
+static int veml3328_runtime_resume(struct device *dev)
+{
+	struct veml3328_data *data = iio_priv(dev_get_drvdata(dev));
+	int ret;
+
+	ret = veml3328_power_up(data);
+	if (ret)
+		dev_err(dev, "Failed to resume: %d\n", ret);
+
+	return ret;
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(veml3328_pm_ops,
+				 veml3328_runtime_suspend,
+				 veml3328_runtime_resume,
+				 NULL);
+
+static const struct of_device_id veml3328_of_match[] = {
+	{ .compatible = "vishay,veml3328" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, veml3328_of_match);
+
+static const struct i2c_device_id veml3328_id[] = {
+	{ .name = "veml3328" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, veml3328_id);
+
+static struct i2c_driver veml3328_driver = {
+	.driver = {
+		.name = "veml3328",
+		.of_match_table = veml3328_of_match,
+		.pm = pm_ptr(&veml3328_pm_ops),
+	},
+	.probe = veml3328_probe,
+	.id_table = veml3328_id,
+};
+module_i2c_driver(veml3328_driver);
+
+MODULE_AUTHOR("Joshua Crofts <joshua.crofts1@gmail.com>");
+MODULE_DESCRIPTION("VEML3328 RGBCIR Light Sensor");
+MODULE_LICENSE("GPL");

-- 
2.54.0

Re: [PATCH v3 2/2] iio: light: veml3328: add support for new device

[Joshua Crofts]

On Sat, 30 May 2026 at 19:07, Joshua Crofts <joshua.crofts1@gmail.com> wrote:
>
> Add support for the Vishay VEML3328 RGB/IR light sensor communicating
> via I2C (SMBus compatible).
>
> Also add a new entry for said driver into Kconfig and Makefile.
>
> Assisted-by: Gemini:3.1-Pro
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> ---

Sashiko had a few remarks, two are bogus IMO, but it does mention
that the device isn't put to sleep after probing...

https://sashiko.dev/#/patchset/20260530-veml3328-v3-0-dd562eaee8d9%40gmail.com

-- 
Kind regards

CJD

Re: [PATCH v3 2/2] iio: light: veml3328: add support for new device

[Jonathan Cameron]

On Sat, 30 May 2026 19:23:53 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> On Sat, 30 May 2026 at 19:07, Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> >
> > Add support for the Vishay VEML3328 RGB/IR light sensor communicating
> > via I2C (SMBus compatible).
> >
> > Also add a new entry for said driver into Kconfig and Makefile.
> >
> > Assisted-by: Gemini:3.1-Pro
> > Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> > ---  
> 
> Sashiko had a few remarks, two are bogus IMO, but it does mention
> that the device isn't put to sleep after probing...
> 
> https://sashiko.dev/#/patchset/20260530-veml3328-v3-0-dd562eaee8d9%40gmail.com
> 

I vaguely recall looking into this years ago and being convinced that it
would autosuspend. Easiest is probably to verify if it does by just testing
the device.

However that might rely on turning runtime pm on before calling use_autosuspend
whereas you have it in a different order here.

One of the other sashiko comments is correct.. I'll reply to the patch.

Re: [PATCH v3 2/2] iio: light: veml3328: add support for new device

[Jonathan Cameron]

On Sat, 30 May 2026 19:06:47 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> Add support for the Vishay VEML3328 RGB/IR light sensor communicating
> via I2C (SMBus compatible).
> 
> Also add a new entry for said driver into Kconfig and Makefile.
> 
> Assisted-by: Gemini:3.1-Pro
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
Hi Joshua,

I didn't find anything extras so this is just some more explanation for
the Sashiko comments.

Give we are approaching the end of the cycle, feel free to do a new version
quicker than normal if you are sure on what needs changing.

Jonathan

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

> +static int veml3328_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{
> +	struct veml3328_data *data = iio_priv(indio_dev);
> +	struct regmap *regmap = data->regmap;
> +	struct device *dev = regmap_get_device(regmap);
> +	unsigned int reg_val;
> +	int it_inx, gain_inx;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(regmap, chan->address, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		*val = reg_val;
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_INT_TIME:
> +		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		it_inx = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);
> +		if (it_inx >= ARRAY_SIZE(veml3328_it_times))
> +			return -EINVAL;
> +
> +		*val = veml3328_it_times[it_inx][0];
> +		*val2 = veml3328_it_times[it_inx][1];
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		it_inx = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);
> +		gain_inx = FIELD_GET(VEML3328_CONF_GAIN_MASK, reg_val);
> +
> +		if (it_inx >= ARRAY_SIZE(veml3328_it_times) || gain_inx >= 4)
> +			return -EINVAL;
> +
> +		/* Stride by 2 through the flattened array to match (val, val2) */
> +		*val = veml3328_scale_vals[it_inx][gain_inx * 2];
> +		*val2 = veml3328_scale_vals[it_inx][gain_inx * 2 + 1];
> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int veml3328_read_avail(struct iio_dev *indio_dev,
> +			       struct iio_chan_spec const *chan,
> +			       const int **vals, int *type, int *length,
> +			       long mask)
> +{
> +	struct veml3328_data *data = iio_priv(indio_dev);
> +	struct regmap *regmap = data->regmap;
> +	struct device *dev = regmap_get_device(data->regmap);
> +	unsigned int reg_val;
> +	int ret, it_inx;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		*length = ARRAY_SIZE(veml3328_it_times) * 2;
> +		*vals = (const int *)veml3328_it_times;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;
> +
> +	case IIO_CHAN_INFO_SCALE:

Under the hood, like all the cleanup.h magic, this instantiates a local
variable - the cleanup scope is tided to that and scope for switch statements
is a funny thing - it's not per case, but rather the whole switch.
Hence need to add {} to define the scope for this case block.

Otherwise you enter this switch via another path and the local variable
is unassigned, but the __free() that cleans it up is set. Hence you get
a use of undefined variable.

> +		PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm);
> +		ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		it_inx = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);
> +		if (it_inx >= ARRAY_SIZE(veml3328_it_times))
> +			return -EINVAL;
> +
> +		*length = 8;
> +		*vals = (const int *)veml3328_scale_vals[it_inx];
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		return IIO_AVAIL_LIST;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int veml3328_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{
> +	struct veml3328_data *data = iio_priv(indio_dev);
> +	struct regmap *regmap = data->regmap;
> +	struct device *dev = regmap_get_device(regmap);
> +	unsigned int reg_val;
> +	int i, it_inx;
> +	int ret;
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED_AUTOSUSPEND(dev, pm);
> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (val != 0)
> +			return -EINVAL;
> +
> +		for (i = 0; i < ARRAY_SIZE(veml3328_it_times); i++) {
> +			if (veml3328_it_times[i][1] == val2)
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(veml3328_it_times))
> +			return -EINVAL;
> +
> +		return regmap_update_bits(regmap, VEML3328_REG_CONF,
> +					  VEML3328_CONF_IT_MASK,
> +					  FIELD_PREP(VEML3328_CONF_IT_MASK, i));
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);

Hmm. The comment made by sashiko on this is also possibly correct.
The write to INT_TIME can race with the write to INFO_SCALE.  Whilst that's
not a normal use model (one thread would typically set up all the parameters)
you could end up with picking a scale based on an int time that isn't the current
one. I'd just throw a guard(mutex)(&data->lock) above this whole switch so we don't have
to think about it. (and a mutex in data, plus initialization etc).


> +		if (ret)
> +			return ret;
> +
> +		it_inx = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);
> +		if (it_inx >= ARRAY_SIZE(veml3328_it_times))
> +			return -EINVAL;
> +
> +		for (i = 0; i < 4; i++) {
> +			if (val == veml3328_scale_vals[it_inx][i * 2] &&
> +			    val2 == veml3328_scale_vals[it_inx][i * 2 + 1])
> +				break;
> +		}
> +
> +		if (i == 4)
> +			return -EINVAL;
> +
> +		return regmap_update_bits(regmap, VEML3328_REG_CONF,
> +					  VEML3328_CONF_GAIN_MASK,
> +					  FIELD_PREP(VEML3328_CONF_GAIN_MASK, i));
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_info veml3328_info = {
> +	.read_raw = veml3328_read_raw,
> +	.write_raw = veml3328_write_raw,
> +	.read_avail = veml3328_read_avail,
> +};
> +
> +static int veml3328_probe(struct i2c_client *client)
> +{

> +
> +	pm_runtime_set_active(dev);
> +	pm_runtime_set_autosuspend_delay(dev, 2000);
> +	pm_runtime_use_autosuspend(dev);

As per other thread check this results in power down.
If it doesn't, try moving autosupend calls after devm_pm_runtime_enable()
and see if that is enough.  Otherwise, you can either manipulate the counters
or I think just call pm_runtime_idle() to force it off immediately.


> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable runtime PM\n");
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

Re: [PATCH v3 2/2] iio: light: veml3328: add support for new device

[Joshua Crofts]

On Sun, 31 May 2026 at 11:19, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 30 May 2026 19:06:47 +0200
> Joshua Crofts <joshua.crofts1@gmail.com> wrote:
>
> > Add support for the Vishay VEML3328 RGB/IR light sensor communicating
> > via I2C (SMBus compatible).
> >
> > Also add a new entry for said driver into Kconfig and Makefile.
> >
> > Assisted-by: Gemini:3.1-Pro
> > Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> Hi Joshua,
>
> I didn't find anything extras so this is just some more explanation for
> the Sashiko comments.
>
> Give we are approaching the end of the cycle, feel free to do a new version
> quicker than normal if you are sure on what needs changing.

Considering these are relatively simple fixes, I'll probably send a v4 today
(and hope that it will be the final iteration).

> > +     case IIO_CHAN_INFO_SCALE:
>
> Under the hood, like all the cleanup.h magic, this instantiates a local
> variable - the cleanup scope is tided to that and scope for switch statements
> is a funny thing - it's not per case, but rather the whole switch.
> Hence need to add {} to define the scope for this case block.
>
> Otherwise you enter this switch via another path and the local variable
> is unassigned, but the __free() that cleans it up is set. Hence you get
> a use of undefined variable.

Okay, thanks for the clarification.

> > +     case IIO_CHAN_INFO_SCALE:
> > +             ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
>
> Hmm. The comment made by sashiko on this is also possibly correct.
> The write to INT_TIME can race with the write to INFO_SCALE.  Whilst that's
> not a normal use model (one thread would typically set up all the parameters)
> you could end up with picking a scale based on an int time that isn't the current
> one. I'd just throw a guard(mutex)(&data->lock) above this whole switch so we don't have
> to think about it. (and a mutex in data, plus initialization etc).

Heh, my first iteration used guard, so we've come full circle :)

> > +
> > +     pm_runtime_set_active(dev);
> > +     pm_runtime_set_autosuspend_delay(dev, 2000);
> > +     pm_runtime_use_autosuspend(dev);
>
> As per other thread check this results in power down.
> If it doesn't, try moving autosupend calls after devm_pm_runtime_enable()
> and see if that is enough.  Otherwise, you can either manipulate the counters
> or I think just call pm_runtime_idle() to force it off immediately.

Sure, that seems reasonable. Testing pm_runtime stuff on a Pi 4 is a bit more
painful though, as the LTS kernel doesn't support the PM_RUNTIME_ACQUIRE
macros - they were introduced in kernel version 6.19 (which I found out the hard
way!)

Thanks

-- 
Kind regards

CJD

Re: [PATCH v3 2/2] iio: light: veml3328: add support for new device

[Joshua Crofts]

On Sun, 31 May 2026 at 11:19, Jonathan Cameron <jic23@kernel.org> wrote:
> > +     pm_runtime_set_active(dev);
> > +     pm_runtime_set_autosuspend_delay(dev, 2000);
> > +     pm_runtime_use_autosuspend(dev);
>
> As per other thread check this results in power down.
> If it doesn't, try moving autosupend calls after devm_pm_runtime_enable()
> and see if that is enough.  Otherwise, you can either manipulate the counters
> or I think just call pm_runtime_idle() to force it off immediately.

Just to confirm, I've tested this on the actual hardware and it did autosuspend
after probe, so I'm going to say that Sashiko is a bit off for once.

-- 
Kind regards

CJD

Re: [PATCH v3 2/2] iio: light: veml3328: add support for new device

[Jonathan Cameron]

On Sun, 31 May 2026 18:44:43 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> On Sun, 31 May 2026 at 11:19, Jonathan Cameron <jic23@kernel.org> wrote:
> > > +     pm_runtime_set_active(dev);
> > > +     pm_runtime_set_autosuspend_delay(dev, 2000);
> > > +     pm_runtime_use_autosuspend(dev);  
> >
> > As per other thread check this results in power down.
> > If it doesn't, try moving autosupend calls after devm_pm_runtime_enable()
> > and see if that is enough.  Otherwise, you can either manipulate the counters
> > or I think just call pm_runtime_idle() to force it off immediately.  
> 
> Just to confirm, I've tested this on the actual hardware and it did autosuspend
> after probe, so I'm going to say that Sashiko is a bit off for once.

If you have time to figure out the path that causes that, it would be excellent
to have it recorded. I spent a few minutes the other day on this and couldn't
find it.  Maybe ask AI to figure it out :)

J
> 

Re: [PATCH v3 2/2] iio: light: veml3328: add support for new device

[Joshua Crofts]

On Mon, 1 Jun 2026 at 11:17, Jonathan Cameron <jic23@kernel.org> wrote:
>
> If you have time to figure out the path that causes that, it would be excellent
> to have it recorded. I spent a few minutes the other day on this and couldn't
> find it.  Maybe ask AI to figure it out :)

Maybe, just maybe if I'm not lazy enough to do it :)

-- 
Kind regards

CJD

Re: [PATCH v3 2/2] iio: light: veml3328: add support for new device

[Joshua Crofts]

On Mon, 1 Jun 2026 at 16:43, Joshua Crofts <joshua.crofts1@gmail.com> wrote:
>
> On Mon, 1 Jun 2026 at 11:17, Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > If you have time to figure out the path that causes that, it would be excellent
> > to have it recorded. I spent a few minutes the other day on this and couldn't
> > find it.  Maybe ask AI to figure it out :)

Briefly analyzing this with AI (Gemini 3.1 Pro, just like Sashiko). I
noticed that
in drivers/base/dd.c the specific driver probe function is called and then
pm_request_idle() is called after it in _driver_probe_device(), which
is probably
the reason the sensor is suspended after probe even though I didn't
explicitly call
pm_runtime_put_autosuspend() (odd that this behaviour in the actual
driver base).

To err on the side of caution, I'd add a pm_runtime_get_noresume() call before
setting up autosuspend and a
pm_runtime_mark_last_busy()/pm_runtime_get_autosupend()
combo before registering the device. Technically Sashiko is correct, this isn't
explicitly done in the driver, nevertheless the driver base actually
handles this
if pm_runtime is correctly set up and callbacks are defined.

Jonathan - up to you if you want to fix up the driver since it's
already committed
or if I should send a patch (I guess it isn't that critical, but it
keeps Sashiko quiet
in future patches).

-- 
Kind regards

CJD

Re: [PATCH v3 2/2] iio: light: veml3328: add support for new device

[Jonathan Cameron]

On Tue, 2 Jun 2026 10:14:59 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> On Mon, 1 Jun 2026 at 16:43, Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> >
> > On Mon, 1 Jun 2026 at 11:17, Jonathan Cameron <jic23@kernel.org> wrote:  
> > >
> > > If you have time to figure out the path that causes that, it would be excellent
> > > to have it recorded. I spent a few minutes the other day on this and couldn't
> > > find it.  Maybe ask AI to figure it out :)  
> 
> Briefly analyzing this with AI (Gemini 3.1 Pro, just like Sashiko). I
> noticed that
> in drivers/base/dd.c the specific driver probe function is called and then
> pm_request_idle() is called after it in _driver_probe_device(), which
> is probably
> the reason the sensor is suspended after probe even though I didn't
> explicitly call
> pm_runtime_put_autosuspend() (odd that this behaviour in the actual
> driver base).

It kind of makes sense as it's basically a now you are finished, should this
be going to sleep call.  I never thought of looking there.
I did some archaeology and via 3 refactors / bug fixes that call goes all the
way back to the introduction of runtime PM 17 years ago.


> 
> To err on the side of caution, I'd add a pm_runtime_get_noresume() call before
> setting up autosuspend and a
> pm_runtime_mark_last_busy()/pm_runtime_get_autosupend()
> combo before registering the device. Technically Sashiko is correct, this isn't
> explicitly done in the driver, nevertheless the driver base actually
> handles this
> if pm_runtime is correctly set up and callbacks are defined.
> 
> Jonathan - up to you if you want to fix up the driver since it's
> already committed
> or if I should send a patch (I guess it isn't that critical, but it
> keeps Sashiko quiet
> in future patches).
It is fine as it stands - lets just keep replying to tell sashiko it is wrong
until it learns :)

Arguably we should be ripping out some of the dance that is going on in current
drivers so as to remove unnecessary complexity.  I'm nervous to do that on anything
we can't test.

Thanks for looking into this!


Jonathan

>