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

[PATCH v2 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

To: Javier Carrasco <javier.carrasco.cruz@gmail.com>
To: Joshua Crofts <joshua.crofts1@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
To: David Lechner <dlechner@baylibre.com>
To: Nuno Sá <nuno.sa@analog.com>
To: Andy Shevchenko <andy@kernel.org>
To: Rob Herring <robh@kernel.org>
To: Krzysztof Kozlowski <krzk+dt@kernel.org>
To: Conor Dooley <conor+dt@kernel.org>
To: Rishi Gupta <gupt21@gmail.com>
Cc: linux-iio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
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                       | 416 +++++++++++++++++++++
 5 files changed, 437 insertions(+), 1 deletion(-)
---
base-commit: 74d173f29572951629d1e0b7456b424006e51b87
change-id: 20260426-veml3328-5bf93f85198e

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

[PATCH v2 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.

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.34.1

[PATCH v2 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 | 416 +++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 433 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index d6c3c7d22403..8236ba0c41ca 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -28386,6 +28386,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 eff33e456c70..1c00bb0de6c8 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -699,6 +699,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 c0048e0d5ca8..e03c6e23a9a4 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -65,6 +65,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..fdfa8fd1666b
--- /dev/null
+++ b/drivers/iio/light/veml3328.c
@@ -0,0 +1,416 @@
+// 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_all = BIT(IIO_CHAN_INFO_INT_TIME) | \
+				   BIT(IIO_CHAN_INFO_SCALE), \
+	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \
+					     BIT(IIO_CHAN_INFO_SCALE), \
+	.address = _addr, \
+}
+
+static const struct iio_chan_spec veml3328_channels[] = {
+	{
+		.type = IIO_LIGHT,
+
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+				      BIT(IIO_CHAN_INFO_SCALE),
+		.info_mask_shared_by_all = 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),
+};
+
+/* scale values per datasheet */
+static const int veml3328_scale_vals[][2] = {
+	{ 0, 500000 },
+	{ 1, 0 },
+	{ 2, 0 },
+	{ 4, 0 },
+};
+
+/* 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 ret;
+	int reg;
+
+	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;
+
+		reg = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);
+		if (reg >= ARRAY_SIZE(veml3328_it_times))
+			return -EINVAL;
+
+		*val = veml3328_it_times[reg][0];
+		*val2 = veml3328_it_times[reg][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;
+
+		if (chan->type == IIO_LIGHT) {
+			int it_inx, gain_inx;
+			int it_us, scale_u;
+
+			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 >= ARRAY_SIZE(veml3328_scale_vals))
+				return -EINVAL;
+
+			it_us = veml3328_it_times[it_inx][1];
+
+			/*
+			 * Equation for calculating ambient light scale:
+			 * Scale = 0.384 * (50000 / Current_IT) * (1x / Current_Gain)
+			 */
+			scale_u = 384 * USEC_PER_MSEC;
+			scale_u = scale_u  * 50000 / it_us;
+
+			switch (gain_inx) {
+			case 0:
+				scale_u *= 2;
+				break;
+			case 1:
+				break;
+			case 2:
+				scale_u /= 2;
+				break;
+			case 3:
+				scale_u /= 4;
+				break;
+			default:
+				return -EINVAL;
+			}
+
+			*val = 0;
+			*val2 = scale_u;
+
+			return IIO_VAL_INT_PLUS_MICRO;
+		}
+
+		reg = FIELD_GET(VEML3328_CONF_GAIN_MASK, reg_val);
+		if (reg >= ARRAY_SIZE(veml3328_scale_vals))
+			return -EINVAL;
+
+		*val = veml3328_scale_vals[reg][0];
+		*val2 = veml3328_scale_vals[reg][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)
+{
+	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:
+		*length = ARRAY_SIZE(veml3328_scale_vals) * 2;
+		*vals = (const int *)veml3328_scale_vals;
+		*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);
+	int ret;
+	int i;
+
+	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:
+		for (i = 0; i < ARRAY_SIZE(veml3328_scale_vals); i++) {
+			if (val == veml3328_scale_vals[i][0] &&
+			    val2 == veml3328_scale_vals[i][1])
+				break;
+		}
+
+		if (i == ARRAY_SIZE(veml3328_scale_vals))
+			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.34.1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Signed 32-bit integer overflow in the lux scale calculation due to large multiplication.
- [Medium] Inconsistent handling of the IIO_LIGHT channel scale across read, write, and available attributes breaks the IIO ABI.
- [Medium] The scale attribute for IIO_INTENSITY channels incorrectly returns the hardware gain multiplier instead of the true IIO scale factor.
--

commit 9edb449d4e1325159429ae7e39210b37d222a175
Author: Joshua Crofts <joshua.crofts1@gmail.com>

iio: light: veml3328: add support for new device

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

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

[ ... ]

> +static const struct iio_chan_spec veml3328_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),
> +		.address = VEML3328_REG_DATA_G,
> +	},

[Severity: Medium]
Is the in_illuminance_scale_available sysfs attribute missing here because
info_mask_shared_by_all_available does not include the scale bit for the
IIO_LIGHT channel?

[ ... ]

> +static int veml3328_read_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int *val, int *val2, long mask)
> +{

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
> +		if (ret)
> +			return ret;
> +
> +		if (chan->type == IIO_LIGHT) {
> +			int it_inx, gain_inx;
> +			int it_us, scale_u;
> +
> +			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 >= ARRAY_SIZE(veml3328_scale_vals))
> +				return -EINVAL;
> +
> +			it_us = veml3328_it_times[it_inx][1];
> +
> +			/*
> +			 * Equation for calculating ambient light scale:
> +			 * Scale = 0.384 * (50000 / Current_IT) * (1x / Current_Gain)
> +			 */
> +			scale_u = 384 * USEC_PER_MSEC;
> +			scale_u = scale_u  * 50000 / it_us;

[Severity: High]
Does this code overflow scale_u?

Since scale_u is initially set to 384,000 (384 * 1000) and is a 32-bit signed
integer, the multiplication by 50000 evaluates to 19,200,000,000. This exceeds
the maximum positive value of 2,147,483,647 for a signed 32-bit int.

> +			switch (gain_inx) {

[ ... ]

> +		}
> +
> +		reg = FIELD_GET(VEML3328_CONF_GAIN_MASK, reg_val);
> +		if (reg >= ARRAY_SIZE(veml3328_scale_vals))
> +			return -EINVAL;
> +
> +		*val = veml3328_scale_vals[reg][0];
> +		*val2 = veml3328_scale_vals[reg][1];
> +		return IIO_VAL_INT_PLUS_MICRO;

[Severity: Medium]
For IIO_INTENSITY channels, this returns the hardware gain multiplier directly
from veml3328_scale_vals. Will this instruct userspace to multiply an already
increased raw value by the hardware gain again?

Should the scale instead return the inverse (1 / gain) to correctly normalize
the readings?

[ ... ]

> +static int veml3328_write_raw(struct iio_dev *indio_dev,
> +			      struct iio_chan_spec const *chan,
> +			      int val, int val2, long mask)
> +{

[ ... ]

> +	case IIO_CHAN_INFO_SCALE:
> +		for (i = 0; i < ARRAY_SIZE(veml3328_scale_vals); i++) {
> +			if (val == veml3328_scale_vals[i][0] &&
> +			    val2 == veml3328_scale_vals[i][1])
> +				break;
> +		}

[Severity: Medium]
For the IIO_LIGHT channel, veml3328_read_raw() dynamically calculates a lux
scale, but this write function compares the input against the raw hardware
gain multipliers in veml3328_scale_vals without verifying the channel type.

Does this mean writing the read scale value back will fail with -EINVAL?

Writing a valid gain value here seems to succeed and modify the hardware
gain, which would change the lux scale to something entirely different.
Could this inconsistent behavior break the expected IIO ABI principle of
being able to write back the values that were read?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260520-veml3328-v2-0-a57f0d9e28d4@gmail.com?part=2

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

[Krzysztof Kozlowski]

On Wed, May 20, 2026 at 11:15:37PM +0200, Joshua Crofts wrote:
> The Vishay VEML3328 is an RGBCIR light sensor that shares similar
> devicetree properties as other existing VEMLxxxx sensors in the
> kernel.
> 
> 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(-)

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>

Best regards,
Krzysztof

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

[Jonathan Cameron]

On Wed, 20 May 2026 23:15:38 +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,

Sashiko did a really good job on this. I'm always amazed at how much it
understands of how these attributes are used.

https://sashiko.dev/#/patchset/20260520-veml3328-v2-0-a57f0d9e28d4%40gmail.com

My biggest comment is I'd look at making the scale from integration / gain
calculations something that is precomputed that you just look up.
You can have separate ones for light and intensity channels or be a little
cheeky and just use the light one for everything - given intensity doesn't
have fixed units as such we can scale by the extra factors in the illuminance
calculation and not worry about what they are.


> diff --git a/drivers/iio/light/veml3328.c b/drivers/iio/light/veml3328.c
> new file mode 100644
> index 000000000000..fdfa8fd1666b
> --- /dev/null
> +++ b/drivers/iio/light/veml3328.c
> @@ -0,0 +1,416 @@

> +
> +#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_all = BIT(IIO_CHAN_INFO_INT_TIME) | \
> +				   BIT(IIO_CHAN_INFO_SCALE), \
> +	.info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_INT_TIME) | \
> +					     BIT(IIO_CHAN_INFO_SCALE), \
> +	.address = _addr, \
> +}
> +
> +static const struct iio_chan_spec veml3328_channels[] = {
> +	{
> +		.type = IIO_LIGHT,
> +
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> +				      BIT(IIO_CHAN_INFO_SCALE),
> +		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_INT_TIME),

Sashiko is kind of right and kind of wrong on this.
As it is shared by all, BIT(IIO_CHAN_INFO_SCALE)
should be here.  It won't have a function affect as we don't verify
they all have it and as long as one does the file will get created.
So this is more about consistency than correctness.

Raises more questions though.  Is it correct scale? I think not.
Looks like you have scaling for the intensity channels returning
different values from the light one. 

So I think the above INTENSITY should move IIO_CHAN_INFO_SCALE
to .info_mask_shared_by_type and equivalent for the available.




> +		.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),
> +};
> +
> +/* scale values per datasheet */
> +static const int veml3328_scale_vals[][2] = {
Sashiko wins again. Whilst we don't get a nice standard unit for
IIO_INTENSITY channels for _raw * _scale, the expectation is that
for the same input light intensity this should remain constant.
I.e. If _scale value double _raw should halve.

I assume these are x0.5 x1 x2 and x4 gains applied. Those will
result in _raw changing by those factors, therefore to get a stable
output these should be inverted.  

	{ 2, 0 },
	{ 1, 0 },
	{ 0, 500000 },
	{ 0, 250000 },

Make sure you test _raw * _scale is stable as a result.

> +	{ 0, 500000 },
> +	{ 1, 0 },
> +	{ 2, 0 },
> +	{ 4, 0 },
> +};
> +
> +/* 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_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 ret;
> +	int reg;
> +
> +	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;
> +
> +		reg = FIELD_GET(VEML3328_CONF_IT_MASK, reg_val);
> +		if (reg >= ARRAY_SIZE(veml3328_it_times))
> +			return -EINVAL;
> +
> +		*val = veml3328_it_times[reg][0];
> +		*val2 = veml3328_it_times[reg][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;
> +
> +		if (chan->type == IIO_LIGHT) {
> +			int it_inx, gain_inx;
> +			int it_us, scale_u;
> +
> +			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 >= ARRAY_SIZE(veml3328_scale_vals))
> +				return -EINVAL;
> +
> +			it_us = veml3328_it_times[it_inx][1];
> +
> +			/*
> +			 * Equation for calculating ambient light scale:
> +			 * Scale = 0.384 * (50000 / Current_IT) * (1x / Current_Gain)

Good. This has the inverses missing for the intensity channels.

As I note below, there are quite a few accesses to what is basically a 4x4 array
of values based on selection of gain and selection of integration time.

I'd be tempted to precompute them and make all these paths look ups.


> +			 */
> +			scale_u = 384 * USEC_PER_MSEC;
> +			scale_u = scale_u  * 50000 / it_us;
> +
> +			switch (gain_inx) {
> +			case 0:
> +				scale_u *= 2;
> +				break;
> +			case 1:
> +				break;
> +			case 2:
> +				scale_u /= 2;
> +				break;
> +			case 3:
> +				scale_u /= 4;
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +
> +			*val = 0;
> +			*val2 = scale_u;
> +
> +			return IIO_VAL_INT_PLUS_MICRO;
> +		}
> +
> +		reg = FIELD_GET(VEML3328_CONF_GAIN_MASK, reg_val);
> +		if (reg >= ARRAY_SIZE(veml3328_scale_vals))
> +			return -EINVAL;
> +
> +		*val = veml3328_scale_vals[reg][0];
> +		*val2 = veml3328_scale_vals[reg][1];
> +		return IIO_VAL_INT_PLUS_MICRO;
I would  flip the logic and do this one out of line.  It's shorter!

Mind you it should still be taking integration time into account to
achieve a stable value whatever the integration time (assuming we don't
hit limits).


> +
> +	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)
> +{
> +	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:
As mentioned else where this needs to work for both light and intensity
channels and incorporate the affect of integration time.

> +		*length = ARRAY_SIZE(veml3328_scale_vals) * 2;
> +		*vals = (const int *)veml3328_scale_vals;
> +		*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);
> +	int ret;
> +	int i;
> +
> +	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:

So this is always a bit of a pain to handle, but you need to take the
integration time into account.

Also this currently applies to the IIO_LIGHT channel as well so you need
to handle that as well which means reversing the maths above to find the
right gain for the current integration time.  There are only 16 options so
I'd be tempted to precompute them all. 



> +		for (i = 0; i < ARRAY_SIZE(veml3328_scale_vals); i++) {
> +			if (val == veml3328_scale_vals[i][0] &&
> +			    val2 == veml3328_scale_vals[i][1])
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(veml3328_scale_vals))
> +			return -EINVAL;
> +
> +		return regmap_update_bits(regmap, VEML3328_REG_CONF,
> +					  VEML3328_CONF_GAIN_MASK,
> +					  FIELD_PREP(VEML3328_CONF_GAIN_MASK, i));
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}


>