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

[PATCH 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>
---
Joshua Crofts (2):
      iio: light: veml3328: add devicetree binding for new sensor
      iio: light: veml3328: add support for new device

 .../bindings/iio/light/vishay,veml3328.yaml        |  45 +++
 MAINTAINERS                                        |   6 +
 drivers/iio/light/Kconfig                          |  11 +
 drivers/iio/light/Makefile                         |   1 +
 drivers/iio/light/veml3328.c                       | 405 +++++++++++++++++++++
 5 files changed, 468 insertions(+)
---
base-commit: 74d173f29572951629d1e0b7456b424006e51b87
change-id: 20260426-veml3328-5bf93f85198e

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

[PATCH 1/2] iio: light: veml3328: add devicetree binding for new sensor

[Joshua Crofts]

Add devicetree binding for the Vishay VEML3328 RGB/IR light sensor
connected via I2C (SMBus compatible).

Additionally, add an entry in MAINTAINERS.

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 .../bindings/iio/light/vishay,veml3328.yaml        | 45 ++++++++++++++++++++++
 MAINTAINERS                                        |  6 +++
 2 files changed, 51 insertions(+)

diff --git a/Documentation/devicetree/bindings/iio/light/vishay,veml3328.yaml b/Documentation/devicetree/bindings/iio/light/vishay,veml3328.yaml
new file mode 100644
index 000000000000..1a30e8470fc6
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/vishay,veml3328.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/vishay,veml3328.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Vishay VEML3328 RGBCIR Light Sensor
+
+maintainers:
+  - Joshua Crofts <joshua.crofts1@gmail.com>
+
+description:
+  Vishay VEML3328 color RGBCIR light sensor.
+
+  The VEML3328 datasheet can be found at https://www.vishay.com/docs/84968/veml3328.pdf
+
+properties:
+  compatible:
+    enum:
+      - vishay,veml3328
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        light-sensor@10 {
+            compatible = "vishay,veml3328";
+            reg = <0x10>;
+            vdd-supply = <&vcc_3v3>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index d6c3c7d22403..134690aa9866 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -28386,6 +28386,12 @@ 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:	Documentation/devicetree/bindings/iio/light/vishay,veml3328.yaml
+F:	drivers/iio/light/veml3328.c
+
 VISHAY VEML6030 AMBIENT LIGHT SENSOR DRIVER
 M:	Javier Carrasco <javier.carrasco.cruz@gmail.com>
 S:	Maintained

-- 
2.34.1

Re: [PATCH 1/2] iio: light: veml3328: add devicetree binding for new sensor

[Krzysztof Kozlowski]

On 16/05/2026 23:50, Joshua Crofts wrote:
> Add devicetree binding for the Vishay VEML3328 RGB/IR light sensor
> connected via I2C (SMBus compatible).

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

And then:
drop second/last, redundant "devicetree binding for new". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> Additionally, add an entry in MAINTAINERS.

Drop, no need to describe what you did. We see that in the diff.

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

...


> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        light-sensor@10 {
> +            compatible = "vishay,veml3328";
> +            reg = <0x10>;
> +            vdd-supply = <&vcc_3v3>;
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index d6c3c7d22403..134690aa9866 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -28386,6 +28386,12 @@ 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:	Documentation/devicetree/bindings/iio/light/vishay,veml3328.yaml
> +F:	drivers/iio/light/veml3328.c

There is no such file at this point.


Best regards,
Krzysztof

Re: [PATCH 1/2] iio: light: veml3328: add devicetree binding for new sensor

[Joshua Crofts]

On Sun, 17 May 2026 at 10:38, Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 16/05/2026 23:50, Joshua Crofts wrote:
> > Add devicetree binding for the Vishay VEML3328 RGB/IR light sensor
> > connected via I2C (SMBus compatible).
>
> Please use subject prefixes matching the subsystem. You can get them for
> example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> your patch is touching. For bindings, the preferred subjects are
> explained here:
> https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
>
> And then:
> drop second/last, redundant "devicetree binding for new". The
> "dt-bindings" prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> >
> > Additionally, add an entry in MAINTAINERS.
>
> Drop, no need to describe what you did. We see that in the diff.
>
> >
> > Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> > ---
>
> ...
>
>
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        light-sensor@10 {
> > +            compatible = "vishay,veml3328";
> > +            reg = <0x10>;
> > +            vdd-supply = <&vcc_3v3>;
> > +        };
> > +    };
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index d6c3c7d22403..134690aa9866 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -28386,6 +28386,12 @@ 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:   Documentation/devicetree/bindings/iio/light/vishay,veml3328.yaml
> > +F:   drivers/iio/light/veml3328.c
>
> There is no such file at this point.

Thanks for the review, will address these in v2.

-- 
Kind regards

CJD

Re: [PATCH 1/2] iio: light: veml3328: add devicetree binding for new sensor

[Jonathan Cameron]

On Sun, 17 May 2026 12:38:40 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> On Sun, 17 May 2026 at 10:38, Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On 16/05/2026 23:50, Joshua Crofts wrote:  
> > > Add devicetree binding for the Vishay VEML3328 RGB/IR light sensor
> > > connected via I2C (SMBus compatible).  
> >
> > Please use subject prefixes matching the subsystem. You can get them for
> > example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
> > your patch is touching. For bindings, the preferred subjects are
> > explained here:
> > https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters
> >
> > And then:
> > drop second/last, redundant "devicetree binding for new". The
> > "dt-bindings" prefix is already stating that these are bindings.
> > See also:
> > https://elixir.bootlin.com/linux/v6.17-rc3/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
> >  
> > >
> > > Additionally, add an entry in MAINTAINERS.  
> >
> > Drop, no need to describe what you did. We see that in the diff.
> >  
> > >
> > > Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> > > ---  
> >
> > ...
> >
> >  
> > > +examples:
> > > +  - |
> > > +    i2c {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        light-sensor@10 {
> > > +            compatible = "vishay,veml3328";
> > > +            reg = <0x10>;
> > > +            vdd-supply = <&vcc_3v3>;
> > > +        };
> > > +    };
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index d6c3c7d22403..134690aa9866 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -28386,6 +28386,12 @@ 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:   Documentation/devicetree/bindings/iio/light/vishay,veml3328.yaml
> > > +F:   drivers/iio/light/veml3328.c  
> >
> > There is no such file at this point.  
> 
> Thanks for the review, will address these in v2.
> 
Hi Joshua,

One process thing - don't reply to say you'll address everything.
Just put your thanks under the --- for instance in the change log for v2.

A reply like this is just noise that several people need to open and
scroll down to see if discussion needs to continue!

Note this is one of my most common bits of feedback to contributors who
are moving on to more significant contributions!

Jonathan

Re: [PATCH 1/2] iio: light: veml3328: add devicetree binding for new sensor

[Jonathan Cameron]

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

> Add devicetree binding for the Vishay VEML3328 RGB/IR light sensor
> connected via I2C (SMBus compatible).
> 
> Additionally, add an entry in MAINTAINERS.
> 
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>

This is very standard looking. Have you considered if any similar
bindings can be easily extended to cover this device?

Note that there is no direct relationship between bindings and
drivers, there are single bindings with multiple associated drivers
and multiple bindings covered by a single driver.
Also dt-bindings get used in multiple projects and some of those
will do different splits in what code covers what parts.

Jonathan

Re: [PATCH 1/2] iio: light: veml3328: add devicetree binding for new sensor

[Joshua Crofts]

On Sun, 17 May 2026 at 15:11, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 16 May 2026 23:50:53 +0200
> Joshua Crofts <joshua.crofts1@gmail.com> wrote:
>
> > Add devicetree binding for the Vishay VEML3328 RGB/IR light sensor
> > connected via I2C (SMBus compatible).
> >
> > Additionally, add an entry in MAINTAINERS.
> >
> > Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
>
> This is very standard looking. Have you considered if any similar
> bindings can be easily extended to cover this device?
>
> Note that there is no direct relationship between bindings and
> drivers, there are single bindings with multiple associated drivers
> and multiple bindings covered by a single driver.
> Also dt-bindings get used in multiple projects and some of those
> will do different splits in what code covers what parts.

Hmm, now that I think about it, the VEML6030 binding could be easily
extended to support this device - oh well, should've checked earlier
and it would've saved me a headache.

-- 
Kind regards

CJD

Re: [PATCH 1/2] iio: light: veml3328: add devicetree binding for new sensor

[Joshua Crofts]

On Sun, 17 May 2026 at 16:26, Joshua Crofts <joshua.crofts1@gmail.com> wrote:
>
> On Sun, 17 May 2026 at 15:11, Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > On Sat, 16 May 2026 23:50:53 +0200
> > Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> >
> > > Add devicetree binding for the Vishay VEML3328 RGB/IR light sensor
> > > connected via I2C (SMBus compatible).
> > >
> > > Additionally, add an entry in MAINTAINERS.
> > >
> > > Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> >
> > This is very standard looking. Have you considered if any similar
> > bindings can be easily extended to cover this device?
> >
> > Note that there is no direct relationship between bindings and
> > drivers, there are single bindings with multiple associated drivers
> > and multiple bindings covered by a single driver.
> > Also dt-bindings get used in multiple projects and some of those
> > will do different splits in what code covers what parts.
>
> Hmm, now that I think about it, the VEML6030 binding could be easily
> extended to support this device - oh well, should've checked earlier
> and it would've saved me a headache.

Before I forget, any idea if I should add an F: reference in the MAINTAINERS
file if I'm just expanding an existing .yaml file? Given I'm not the maintainer
of it, I'd expect to just have a reference to the actual driver file and not the
binding.

Thanks.

-- 
Kind regards

CJD

Re: [PATCH 1/2] iio: light: veml3328: add devicetree binding for new sensor

[Jonathan Cameron]

On Mon, 18 May 2026 16:18:37 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> On Sun, 17 May 2026 at 16:26, Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> >
> > On Sun, 17 May 2026 at 15:11, Jonathan Cameron <jic23@kernel.org> wrote:  
> > >
> > > On Sat, 16 May 2026 23:50:53 +0200
> > > Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> > >  
> > > > Add devicetree binding for the Vishay VEML3328 RGB/IR light sensor
> > > > connected via I2C (SMBus compatible).
> > > >
> > > > Additionally, add an entry in MAINTAINERS.
> > > >
> > > > Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>  
> > >
> > > This is very standard looking. Have you considered if any similar
> > > bindings can be easily extended to cover this device?
> > >
> > > Note that there is no direct relationship between bindings and
> > > drivers, there are single bindings with multiple associated drivers
> > > and multiple bindings covered by a single driver.
> > > Also dt-bindings get used in multiple projects and some of those
> > > will do different splits in what code covers what parts.  
> >
> > Hmm, now that I think about it, the VEML6030 binding could be easily
> > extended to support this device - oh well, should've checked earlier
> > and it would've saved me a headache.  
> 
> Before I forget, any idea if I should add an F: reference in the MAINTAINERS
> file if I'm just expanding an existing .yaml file? Given I'm not the maintainer
> of it, I'd expect to just have a reference to the actual driver file and not the
> binding.

I wouldn't bother.  This one is simple enough a breaking change is unlikely!

J
> 
> Thanks.
> 

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

Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
---
 drivers/iio/light/Kconfig    |  11 ++
 drivers/iio/light/Makefile   |   1 +
 drivers/iio/light/veml3328.c | 405 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 417 insertions(+)

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..9eb5429c813d
--- /dev/null
+++ b/drivers/iio/light/veml3328.c
@@ -0,0 +1,405 @@
+// 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/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/mutex.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.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_IT_MASK		GENMASK(5, 4)
+#define VEML3328_GAIN_MASK		GENMASK(11, 10)
+
+#define VEML3328_ID_VAL			0x28
+
+#define VEML3328_CONF_SD0		BIT(0)
+#define VEML3328_CONF_SD1		BIT(15)
+#define VEML3328_SHUTDOWN		(VEML3328_CONF_SD0 | VEML3328_CONF_SD1)
+
+struct veml3328_data {
+	struct regmap *regmap;
+	struct device *dev;
+	struct mutex lock;
+};
+
+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[] = {
+	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, 50000 },
+	{ 0, 100000 },
+	{ 0, 200000 },
+	{ 0, 400000 },
+};
+
+static int veml3328_power_down(struct veml3328_data *data)
+{
+	return regmap_update_bits(data->regmap, VEML3328_REG_CONF,
+				  VEML3328_SHUTDOWN, VEML3328_SHUTDOWN);
+}
+
+static int veml3328_power_up(struct veml3328_data *data)
+{
+	int ret;
+
+	ret = regmap_update_bits(data->regmap, VEML3328_REG_CONF,
+				 VEML3328_SHUTDOWN, 0);
+	if (ret < 0)
+		return ret;
+
+	fsleep(veml3328_it_times[3][1]);
+
+	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;
+	unsigned int reg_val;
+	int ret;
+	int reg;
+
+	guard(mutex)(&data->lock);
+
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret < 0)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_read(regmap, chan->address, &reg_val);
+		if (ret < 0)
+			goto exit;
+
+		*val = reg_val;
+		ret = IIO_VAL_INT;
+		break;
+
+	case IIO_CHAN_INFO_INT_TIME:
+		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
+		if (ret < 0)
+			goto exit;
+
+		reg = FIELD_GET(VEML3328_IT_MASK, reg_val);
+		if (reg >= ARRAY_SIZE(veml3328_it_times)) {
+			ret = -EINVAL;
+			goto exit;
+		}
+
+		*val = veml3328_it_times[reg][0];
+		*val2 = veml3328_it_times[reg][1];
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
+		if (ret < 0)
+			goto exit;
+
+		reg = FIELD_GET(VEML3328_GAIN_MASK, reg_val);
+		if (reg >= ARRAY_SIZE(veml3328_scale_vals)) {
+			ret = -EINVAL;
+			goto exit;
+		}
+
+		*val = veml3328_scale_vals[reg][0];
+		*val2 = veml3328_scale_vals[reg][1];
+		ret = IIO_VAL_INT_PLUS_MICRO;
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+exit:
+	pm_runtime_put_autosuspend(data->dev);
+
+	return ret;
+}
+
+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;
+	int ret;
+	int i;
+
+	guard(mutex)(&data->lock);
+
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret < 0)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		if (val != 0) {
+			ret = -EINVAL;
+			goto exit;
+		}
+
+		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)) {
+			ret = -EINVAL;
+			goto exit;
+		}
+
+		ret = regmap_update_bits(regmap, VEML3328_REG_CONF,
+					 VEML3328_IT_MASK,
+					 FIELD_PREP(VEML3328_IT_MASK, i));
+		break;
+
+	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)) {
+			ret = -EINVAL;
+			goto exit;
+		}
+
+		ret = regmap_update_bits(regmap, VEML3328_REG_CONF,
+					 VEML3328_GAIN_MASK,
+					 FIELD_PREP(VEML3328_GAIN_MASK, i));
+
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+exit:
+	pm_runtime_put_autosuspend(data->dev);
+
+	return ret;
+}
+
+static int veml3328_write_raw_get_fmt(struct iio_dev *indio_dev,
+				      struct iio_chan_spec const *chan,
+				      long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_SCALE:
+	case IIO_CHAN_INFO_INT_TIME:
+		return IIO_VAL_INT_PLUS_MICRO;
+	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,
+	.write_raw_get_fmt = veml3328_write_raw_get_fmt,
+};
+
+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->dev = 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_mutex_init(dev, &data->lock);
+	if (ret)
+		return ret;
+
+	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 < 0)
+		return dev_err_probe(dev, ret, "Failed to read ID register\n");
+
+	if ((reg_val & 0xff) != VEML3328_ID_VAL)
+		return dev_err_probe(dev, -ENODEV, "Invalid device ID\n");
+
+	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 < 0)
+		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 < 0)
+		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[] = {
+	{ "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 RGB Light Sensor");
+MODULE_LICENSE("GPL");

-- 
2.34.1

Re: [PATCH 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:
- [Medium] The device will not enter runtime suspend until it is accessed at least once by userspace, wasting power indefinitely.
- [Medium] Mixing `goto` with `guard()` cleanup in the same function.
- [Medium] Unconditional 400ms blocking sleep on runtime resume.
--

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

    iio: light: veml3328: add support for new device

This commit introduces a new driver for the Vishay VEML3328 RGB/IR light sensor
communicating via I2C. It includes runtime PM support and basic IIO channel
configurations.

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

[ ... ]

> +static int veml3328_power_up(struct veml3328_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap, VEML3328_REG_CONF,
> +				 VEML3328_SHUTDOWN, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	fsleep(veml3328_it_times[3][1]);

Does this unconditional 400ms sleep cause a latency regression when the device
is resuming?

Because this function is called from the runtime resume callback, this adds a
400ms latency to every operation that wakes the device from suspend, including
simple configuration reads or writes via sysfs.

It also unconditionally sleeps for the maximum possible integration time
instead of the currently configured one.

Should the measurement delay be handled in veml3328_read_raw() when actually
acquiring data, rather than in the general resume callback?

> +
> +	return 0;
> +}

[ ... ]

> +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;
> +	unsigned int reg_val;
> +	int ret;
> +	int reg;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(regmap, chan->address, &reg_val);
> +		if (ret < 0)
> +			goto exit;

Is it intended to mix goto-based error handling with guard() in the same
function?

The kernel cleanup guidelines explicitly state that mixing goto-based cleanup
labels with guard() declarations in the same function creates confusing
ownership semantics.

Could the goto statements inside the switch blocks be trivially replaced with
break to eliminate the exit label and comply with the guidelines?

> +
> +		*val = reg_val;
> +		ret = IIO_VAL_INT;
> +		break;

[ ... ]

> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +exit:
> +	pm_runtime_put_autosuspend(data->dev);
> +
> +	return ret;
> +}

[ ... ]

> +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;
> +	int ret;
> +	int i;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (val != 0) {
> +			ret = -EINVAL;
> +			goto exit;

This has the same issue with mixing goto and guard() as veml3328_read_raw().

> +		}
> +
> +		for (i = 0; i < ARRAY_SIZE(veml3328_it_times); i++) {
> +			if (veml3328_it_times[i][1] == val2)
> +				break;
> +		}

[ ... ]

> +static int veml3328_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct veml3328_data *data;

[ ... ]

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

Will this device fail to enter runtime suspend until it is accessed at least
once by userspace?

Because the usage count starts at 0 and no idle request is explicitly queued,
the PM core does not automatically start the autosuspend timer. The device
might remain fully powered on indefinitely until the first userspace access
triggers a get/put cycle.

Should the driver queue an idle check by calling pm_request_idle(dev) or
perform a pm_runtime_get_noresume() and pm_runtime_put_autosuspend() sequence
at the end of probe to initiate autosuspend?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260516-veml3328-v1-0-1d4b663e2fe3@gmail.com?part=2

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

[Joshua Crofts]

On Sat, 16 May 2026 at 23:51, 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.
>
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>

Oops, Sashiko pointed out that I'm mixing goto with guard, will remove these in
V2.

-- 
Kind regards

CJD

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

[Andy Shevchenko]

On Sat, May 16, 2026 at 11:50:54PM +0200, Joshua Crofts 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.

...

IWYU.

+ 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/mutex.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>

Most likely + types.h.

...

> +struct veml3328_data {
> +	struct regmap *regmap;
> +	struct device *dev;

Dup? We may derive one from the other (in case if regmap is registered with
the same dev, otherwise needs a good comment explaining why not).

> +	struct mutex lock;
> +};

...

> +/* integration times in microseconds */
> +static const int veml3328_it_times[][2] = {
> +	{ 0, 50000 },
> +	{ 0, 100000 },
> +	{ 0, 200000 },
> +	{ 0, 400000 },

USEC_PER_MSEC in all of them.

> +};

...

> +	return regmap_update_bits(data->regmap, VEML3328_REG_CONF,
> +				  VEML3328_SHUTDOWN, VEML3328_SHUTDOWN);

Here and everywhere else, use regmap_set_bits()/regmap_clear_bits() when
appropriate.

...

> +static int veml3328_power_up(struct veml3328_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap, VEML3328_REG_CONF,
> +				 VEML3328_SHUTDOWN, 0);
> +	if (ret < 0)
> +		return ret;

> +	fsleep(veml3328_it_times[3][1]);

This is not good. Why do we have a table from which we are using only one
value? Define it properly and use here.

> +	return 0;
> +}

...

> +		ret = regmap_read(regmap, chan->address, &reg_val);
> +		if (ret < 0)

Do all these ' < 0' for regmap API make any sense?

> +			goto exit;

...

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

There are respective PM_RUNTIME_*() macros.

...

> +	if ((reg_val & 0xff) != VEML3328_ID_VAL)

Do you need the & 0xff? Do you have register width > 8 bits?

> +		return dev_err_probe(dev, -ENODEV, "Invalid device ID\n");

We don't fail on that, now we just warn.

...

> +static DEFINE_RUNTIME_DEV_PM_OPS(veml3328_pm_ops, veml3328_runtime_suspend,
> +				 veml3328_runtime_resume, NULL);

Split logically.

...

> +static const struct i2c_device_id veml3328_id[] = {
> +	{ "veml3328" },

Use .name.
This is a new development due to some Uwe's ongoing (re)work on ID tables.

> +	{ }
> +};

...

Was this AI-assisted? If so, don't forget to add tags.

-- 
With Best Regards,
Andy Shevchenko

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

[Joshua Crofts]

On Sun, 17 May 2026 at 09:53, Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Sat, May 16, 2026 at 11:50:54PM +0200, Joshua Crofts 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.
>
> ...
>
> IWYU.
>
> + array_size.h

Ah, missed that one.

> > +#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/mutex.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.h>
>
> Most likely + types.h.
>
> ...
>
> > +struct veml3328_data {
> > +     struct regmap *regmap;
> > +     struct device *dev;
>
> Dup? We may derive one from the other (in case if regmap is registered with
> the same dev, otherwise needs a good comment explaining why not).
>
> > +     struct mutex lock;
> > +};

Okay, regmap is registered with the same device.

> ...
>
> > +/* integration times in microseconds */
> > +static const int veml3328_it_times[][2] = {
> > +     { 0, 50000 },
> > +     { 0, 100000 },
> > +     { 0, 200000 },
> > +     { 0, 400000 },
>
> USEC_PER_MSEC in all of them.

Good catch.

> > +};
>
> ...
>
> > +     return regmap_update_bits(data->regmap, VEML3328_REG_CONF,
> > +                               VEML3328_SHUTDOWN, VEML3328_SHUTDOWN);
>
> Here and everywhere else, use regmap_set_bits()/regmap_clear_bits() when
> appropriate.

No problem.

> ...
>
> > +static int veml3328_power_up(struct veml3328_data *data)
> > +{
> > +     int ret;
> > +
> > +     ret = regmap_update_bits(data->regmap, VEML3328_REG_CONF,
> > +                              VEML3328_SHUTDOWN, 0);
> > +     if (ret < 0)
> > +             return ret;
>
> > +     fsleep(veml3328_it_times[3][1]);
>
> This is not good. Why do we have a table from which we are using only one
> value? Define it properly and use here.

My bad, will use a macro instead.

> > +     return 0;
> > +}
>
> ...
>
> > +             ret = regmap_read(regmap, chan->address, &reg_val);
> > +             if (ret < 0)
>
> Do all these ' < 0' for regmap API make any sense?

I guess not, will change these to use (ret)
>
> > +                     goto exit;
>
> ...
>
> > +     ret = pm_runtime_resume_and_get(data->dev);
> > +     if (ret < 0)
> > +             return ret;
>
> There are respective PM_RUNTIME_*() macros.
>
> ...
>
> > +     if ((reg_val & 0xff) != VEML3328_ID_VAL)
>
> Do you need the & 0xff? Do you have register width > 8 bits?

The datasheet says that the high byte is reserved (silicon revision imo),
so it does contain data, therefore it has to be masked.

>
> > +             return dev_err_probe(dev, -ENODEV, "Invalid device ID\n");
>
> We don't fail on that, now we just warn.

Alright.

> ...
>
> > +static DEFINE_RUNTIME_DEV_PM_OPS(veml3328_pm_ops, veml3328_runtime_suspend,
> > +                              veml3328_runtime_resume, NULL);
>
> Split logically.

Easy enough.

>
> ...
>
> > +static const struct i2c_device_id veml3328_id[] = {
> > +     { "veml3328" },
>
> Use .name.
> This is a new development due to some Uwe's ongoing (re)work on ID tables.

Sure.

> > +     { }
> > +};
>
> ...
>
> Was this AI-assisted? If so, don't forget to add tags.

My bad, forgot to add it. Thanks for the detailed review!


--
Kind regards

CJD

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

[Jonathan Cameron]

On Sat, 16 May 2026 23:50:54 +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.
> 
> Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
Hi Joshua,

Pretty nice for a v1.  A few comments inline to add to
the other reviews (I may well have duplicated some!)

Jonathan

> diff --git a/drivers/iio/light/veml3328.c b/drivers/iio/light/veml3328.c
> new file mode 100644
> index 000000000000..9eb5429c813d
> --- /dev/null
> +++ b/drivers/iio/light/veml3328.c
> @@ -0,0 +1,405 @@
> +// 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/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/mutex.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.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_IT_MASK		GENMASK(5, 4)
> +#define VEML3328_GAIN_MASK		GENMASK(11, 10)
Name these so it is obvious which register they are in.
I guess VEML3328_CONF_IT_MASK etc.


> +
> +#define VEML3328_ID_VAL			0x28
> +
> +#define VEML3328_CONF_SD0		BIT(0)
> +#define VEML3328_CONF_SD1		BIT(15)
These two don't add anything given always used together.
> +#define VEML3328_SHUTDOWN		(VEML3328_CONF_SD0 | VEML3328_CONF_SD1)

#define VEML3328_SHUTDOWN		BIT(0) | BIT(15)

is sufficient

> +
> +struct veml3328_data {
> +	struct regmap *regmap;
> +	struct device *dev;
The use of the one embedded in regmap got mentioned already in another review.

> +	struct mutex lock;
All locks need a comment saying what data they are protecting
(might well be in the device).

Mind you - I'm seeing quite a bit of locking around simple regmap calls.
Given there are locks in regmap, you may need to call out if there
is a particular readout sequence that must not be interrupted.

I'm not immediately seeing one and as such you might not need a local
lock.

> +};
> +
> +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[] = {
> +	VEML3328_CHAN_SPEC(CLEAR, VEML3328_REG_DATA_C),

Would be nice to have a an IIO_LUX channel (always an approximation)
as that tends to be what userspace wanges.

Hmm. I googled a bit. Seems there is a note that has more info.
https://www.vishay.com/docs/80010/designingveml3328.pdf

Interestingly suggests that green channel is nearer than clear.

If you can put the correct scaling in that would be a good thing
to support (keep the green as well).



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

> +
> +static int veml3328_power_up(struct veml3328_data *data)
> +{
> +	int ret;
> +
> +	ret = regmap_update_bits(data->regmap, VEML3328_REG_CONF,
> +				 VEML3328_SHUTDOWN, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	fsleep(veml3328_it_times[3][1]);

Add a comment on why we need to sleep maximum integration 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;
> +	unsigned int reg_val;
> +	int ret;
> +	int reg;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		ret = regmap_read(regmap, chan->address, &reg_val);
> +		if (ret < 0)
> +			goto exit;
> +
> +		*val = reg_val;
> +		ret = IIO_VAL_INT;
> +		break;
> +
> +	case IIO_CHAN_INFO_INT_TIME:
> +		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
> +		if (ret < 0)
> +			goto exit;
> +
> +		reg = FIELD_GET(VEML3328_IT_MASK, reg_val);
> +		if (reg >= ARRAY_SIZE(veml3328_it_times)) {
> +			ret = -EINVAL;
> +			goto exit;
> +		}
> +
> +		*val = veml3328_it_times[reg][0];
> +		*val2 = veml3328_it_times[reg][1];
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
> +		if (ret < 0)
> +			goto exit;
> +
> +		reg = FIELD_GET(VEML3328_GAIN_MASK, reg_val);
> +		if (reg >= ARRAY_SIZE(veml3328_scale_vals)) {
> +			ret = -EINVAL;
> +			goto exit;
> +		}
> +
> +		*val = veml3328_scale_vals[reg][0];
> +		*val2 = veml3328_scale_vals[reg][1];
> +		ret = IIO_VAL_INT_PLUS_MICRO;
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +exit:
> +	pm_runtime_put_autosuspend(data->dev);
See below for comment on how to handle this using stuff that
ultimately comes from cleanup.h

> +
> +	return ret;
> +}
> +

> +
> +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;
> +	int ret;
> +	int i;
> +
> +	guard(mutex)(&data->lock);
> +
> +	ret = pm_runtime_resume_and_get(data->dev);

PM_RUNTIME_ACQUIRE_AUTOSUSPEND() will mean you can rely on this
being auto suspended on exiting scope.  Will allow early returns
and get rid of your goto that you noted already.


> +	if (ret < 0)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_INT_TIME:
> +		if (val != 0) {
> +			ret = -EINVAL;
> +			goto exit;

With above, all these become direct returns.

> +		}
> +
> +		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)) {
> +			ret = -EINVAL;
> +			goto exit;
> +		}
> +
> +		ret = regmap_update_bits(regmap, VEML3328_REG_CONF,
> +					 VEML3328_IT_MASK,
> +					 FIELD_PREP(VEML3328_IT_MASK, i));
> +		break;
> +
> +	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)) {
> +			ret = -EINVAL;
> +			goto exit;
> +		}
> +
> +		ret = regmap_update_bits(regmap, VEML3328_REG_CONF,
> +					 VEML3328_GAIN_MASK,
> +					 FIELD_PREP(VEML3328_GAIN_MASK, i));
> +
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +exit:
> +	pm_runtime_put_autosuspend(data->dev);
> +
> +	return ret;
> +}
> +
> +static int veml3328_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				      struct iio_chan_spec const *chan,
> +				      long mask)
> +{
> +	switch (mask) {
> +	case IIO_CHAN_INFO_SCALE:
> +	case IIO_CHAN_INFO_INT_TIME:
> +		return IIO_VAL_INT_PLUS_MICRO;

That's the default - no need for the callback.

> +	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,
> +	.write_raw_get_fmt = veml3328_write_raw_get_fmt,
> +};
> +
> +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->dev = 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_mutex_init(dev, &data->lock);
> +	if (ret)
> +		return ret;
> +
> +	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 < 0)
> +		return dev_err_probe(dev, ret, "Failed to read ID register\n");
> +
> +	if ((reg_val & 0xff) != VEML3328_ID_VAL)
> +		return dev_err_probe(dev, -ENODEV, "Invalid device ID\n");

This breaks fallback dt compatibles.
If some future part is released that backwards compatible with this one, then
the dt-binding will reflect that.  That future part will typically have a different
ID_VAL.  We need that case of new binding, older kernel to work so this can't
fail probe.  It is fine to print a message though to indicate we are seeing
a value that we don't know about.

Note sashiko moans about this sometimes. It's wrong so ignore it on this
particular thing!  Sadly it's seeing a lot of older drivers where we did this
wrong in the past and haven't adding the churn needed to fix them all.

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

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

[Joshua Crofts]

On Sun, 17 May 2026 at 15:34, Jonathan Cameron <jic23@kernel.org> wrote:
>
> On Sat, 16 May 2026 23:50:54 +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.
> >
> > Signed-off-by: Joshua Crofts <joshua.crofts1@gmail.com>
> Hi Joshua,
>
> Pretty nice for a v1.  A few comments inline to add to
> the other reviews (I may well have duplicated some!)
>
> Jonathan
>
> > diff --git a/drivers/iio/light/veml3328.c b/drivers/iio/light/veml3328.c
> > new file mode 100644
> > index 000000000000..9eb5429c813d
> > --- /dev/null
> > +++ b/drivers/iio/light/veml3328.c
> > @@ -0,0 +1,405 @@
> > +// 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/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/mutex.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/regulator/consumer.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_IT_MASK             GENMASK(5, 4)
> > +#define VEML3328_GAIN_MASK           GENMASK(11, 10)
> Name these so it is obvious which register they are in.
> I guess VEML3328_CONF_IT_MASK etc.
>
>
> > +
> > +#define VEML3328_ID_VAL                      0x28
> > +
> > +#define VEML3328_CONF_SD0            BIT(0)
> > +#define VEML3328_CONF_SD1            BIT(15)
> These two don't add anything given always used together.
> > +#define VEML3328_SHUTDOWN            (VEML3328_CONF_SD0 | VEML3328_CONF_SD1)
>
> #define VEML3328_SHUTDOWN               BIT(0) | BIT(15)
>
> is sufficient

Ah, fair enough, that's a remnant of me using existing drivers
as inspiration.

> > +
> > +struct veml3328_data {
> > +     struct regmap *regmap;
> > +     struct device *dev;
> The use of the one embedded in regmap got mentioned already in another review.
> > +     struct mutex lock;
> All locks need a comment saying what data they are protecting
> (might well be in the device).
>
> Mind you - I'm seeing quite a bit of locking around simple regmap calls.
> Given there are locks in regmap, you may need to call out if there
> is a particular readout sequence that must not be interrupted.
>
> I'm not immediately seeing one and as such you might not need a local
> lock.

I was on the fence with this one - my understanding was that the locking
in regmap was just for i2c bus interactions, not actual value read/writes.
I've no problem with removing it though if I am mistaken.

> > +};
> > +
> > +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[] = {
> > +     VEML3328_CHAN_SPEC(CLEAR, VEML3328_REG_DATA_C),
>
> Would be nice to have a an IIO_LUX channel (always an approximation)
> as that tends to be what userspace wanges.
>
> Hmm. I googled a bit. Seems there is a note that has more info.
> https://www.vishay.com/docs/80010/designingveml3328.pdf
>
> Interestingly suggests that green channel is nearer than clear.
>
> If you can put the correct scaling in that would be a good thing
> to support (keep the green as well).

Sure, I was intending to do some extensions once this first version
gets polished and accepted, but I can do it now.

>
>
> > +     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),
> > +};
>
> > +
> > +static int veml3328_power_up(struct veml3328_data *data)
> > +{
> > +     int ret;
> > +
> > +     ret = regmap_update_bits(data->regmap, VEML3328_REG_CONF,
> > +                              VEML3328_SHUTDOWN, 0);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     fsleep(veml3328_it_times[3][1]);
>
> Add a comment on why we need to sleep maximum integration time.

Fair, I also made the value into a macro per Andy's comment.

> > +
> > +     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;
> > +     unsigned int reg_val;
> > +     int ret;
> > +     int reg;
> > +
> > +     guard(mutex)(&data->lock);
> > +
> > +     ret = pm_runtime_resume_and_get(data->dev);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_RAW:
> > +             ret = regmap_read(regmap, chan->address, &reg_val);
> > +             if (ret < 0)
> > +                     goto exit;
> > +
> > +             *val = reg_val;
> > +             ret = IIO_VAL_INT;
> > +             break;
> > +
> > +     case IIO_CHAN_INFO_INT_TIME:
> > +             ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
> > +             if (ret < 0)
> > +                     goto exit;
> > +
> > +             reg = FIELD_GET(VEML3328_IT_MASK, reg_val);
> > +             if (reg >= ARRAY_SIZE(veml3328_it_times)) {
> > +                     ret = -EINVAL;
> > +                     goto exit;
> > +             }
> > +
> > +             *val = veml3328_it_times[reg][0];
> > +             *val2 = veml3328_it_times[reg][1];
> > +             ret = IIO_VAL_INT_PLUS_MICRO;
> > +             break;
> > +
> > +     case IIO_CHAN_INFO_SCALE:
> > +             ret = regmap_read(regmap, VEML3328_REG_CONF, &reg_val);
> > +             if (ret < 0)
> > +                     goto exit;
> > +
> > +             reg = FIELD_GET(VEML3328_GAIN_MASK, reg_val);
> > +             if (reg >= ARRAY_SIZE(veml3328_scale_vals)) {
> > +                     ret = -EINVAL;
> > +                     goto exit;
> > +             }
> > +
> > +             *val = veml3328_scale_vals[reg][0];
> > +             *val2 = veml3328_scale_vals[reg][1];
> > +             ret = IIO_VAL_INT_PLUS_MICRO;
> > +             break;
> > +
> > +     default:
> > +             ret = -EINVAL;
> > +     }
> > +
> > +exit:
> > +     pm_runtime_put_autosuspend(data->dev);
> See below for comment on how to handle this using stuff that
> ultimately comes from cleanup.h
>
> > +
> > +     return ret;
> > +}
> > +
>
> > +
> > +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;
> > +     int ret;
> > +     int i;
> > +
> > +     guard(mutex)(&data->lock);
> > +
> > +     ret = pm_runtime_resume_and_get(data->dev);
>
> PM_RUNTIME_ACQUIRE_AUTOSUSPEND() will mean you can rely on this
> being auto suspended on exiting scope.  Will allow early returns
> and get rid of your goto that you noted already.

Oh, now that is interesting! It would simplify things greatly...

>
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_INT_TIME:
> > +             if (val != 0) {
> > +                     ret = -EINVAL;
> > +                     goto exit;
>
> With above, all these become direct returns.
>
> > +             }
> > +
> > +             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)) {
> > +                     ret = -EINVAL;
> > +                     goto exit;
> > +             }
> > +
> > +             ret = regmap_update_bits(regmap, VEML3328_REG_CONF,
> > +                                      VEML3328_IT_MASK,
> > +                                      FIELD_PREP(VEML3328_IT_MASK, i));
> > +             break;
> > +
> > +     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)) {
> > +                     ret = -EINVAL;
> > +                     goto exit;
> > +             }
> > +
> > +             ret = regmap_update_bits(regmap, VEML3328_REG_CONF,
> > +                                      VEML3328_GAIN_MASK,
> > +                                      FIELD_PREP(VEML3328_GAIN_MASK, i));
> > +
> > +             break;
> > +
> > +     default:
> > +             ret = -EINVAL;
> > +     }
> > +
> > +exit:
> > +     pm_runtime_put_autosuspend(data->dev);
> > +
> > +     return ret;
> > +}
> > +
> > +static int veml3328_write_raw_get_fmt(struct iio_dev *indio_dev,
> > +                                   struct iio_chan_spec const *chan,
> > +                                   long mask)
> > +{
> > +     switch (mask) {
> > +     case IIO_CHAN_INFO_SCALE:
> > +     case IIO_CHAN_INFO_INT_TIME:
> > +             return IIO_VAL_INT_PLUS_MICRO;
>
> That's the default - no need for the callback.

Good point.

>
> > +     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,
> > +     .write_raw_get_fmt = veml3328_write_raw_get_fmt,
> > +};
> > +
> > +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->dev = 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_mutex_init(dev, &data->lock);
> > +     if (ret)
> > +             return ret;
> > +
> > +     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 < 0)
> > +             return dev_err_probe(dev, ret, "Failed to read ID register\n");
> > +
> > +     if ((reg_val & 0xff) != VEML3328_ID_VAL)
> > +             return dev_err_probe(dev, -ENODEV, "Invalid device ID\n");
>
> This breaks fallback dt compatibles.
> If some future part is released that backwards compatible with this one, then
> the dt-binding will reflect that.  That future part will typically have a different
> ID_VAL.  We need that case of new binding, older kernel to work so this can't
> fail probe.  It is fine to print a message though to indicate we are seeing
> a value that we don't know about.
>
> Note sashiko moans about this sometimes. It's wrong so ignore it on this
> particular thing!  Sadly it's seeing a lot of older drivers where we did this
> wrong in the past and haven't adding the churn needed to fix them all.

Yes, Andy already pointed out to use dev_warn() instead. Good idea.

Overall I appreciate the review as this is my first attempt at a driver.

-- 
Kind regards

CJD

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

[Jonathan Cameron]

> > > +
> > > +struct veml3328_data {
> > > +     struct regmap *regmap;
> > > +     struct device *dev;  
> > The use of the one embedded in regmap got mentioned already in another review.  
> > > +     struct mutex lock;  
> > All locks need a comment saying what data they are protecting
> > (might well be in the device).
> >
> > Mind you - I'm seeing quite a bit of locking around simple regmap calls.
> > Given there are locks in regmap, you may need to call out if there
> > is a particular readout sequence that must not be interrupted.
> >
> > I'm not immediately seeing one and as such you might not need a local
> > lock.  
> 
> I was on the fence with this one - my understanding was that the locking
> in regmap was just for i2c bus interactions, not actual value read/writes.
> I've no problem with removing it though if I am mistaken.
> 
The regmap calls themselves are all safe against races.
E.g.
 */
int regmap_update_bits_base(struct regmap *map, unsigned int reg,
			    unsigned int mask, unsigned int val,
			    bool *change, bool async, bool force)
{
	int ret;

	map->lock(map->lock_arg);

	map->async = async;

	ret = _regmap_update_bits(map, reg, mask, val, change, force);

	map->async = false;

	map->unlock(map->lock_arg);

	return ret;
}
EXPORT_SYMBOL_GPL(regmap_update_bits_base);

is taking the lock - under the hood probably uses regmap_lock_mutex() but
I haven't checked this specific config.

So you only tend to need your own locking if:
1) read modify write cycles occur that aren't handled by the simple regmap calls.
2) need to maintain consistency between some internal state and a register.
3) need to ensure a sequence of regmap accesses aren't interrupted.

Jonathan

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

[Joshua Crofts]

On Sun, 17 May 2026 at 15:34, Jonathan Cameron <jic23@kernel.org> wrote:
> > +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;
> > +     int ret;
> > +     int i;
> > +
> > +     guard(mutex)(&data->lock);
> > +
> > +     ret = pm_runtime_resume_and_get(data->dev);
>
> PM_RUNTIME_ACQUIRE_AUTOSUSPEND() will mean you can rely on this
> being auto suspended on exiting scope.  Will allow early returns
> and get rid of your goto that you noted already.

Sorry if this is a silly request, however I can't seem to understand how the
macro mentioned above works... Could someone point me to documentation
on it or a functioning example in a driver? I tried analyzing this
with AI as well
and it keeps going in circles that the macro does not exist :(

-- 
Kind regards

CJD

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

[Andy Shevchenko]

On Sun, May 17, 2026 at 07:11:06PM +0200, Joshua Crofts wrote:
> On Sun, 17 May 2026 at 15:34, Jonathan Cameron <jic23@kernel.org> wrote:

...

> > > +     ret = pm_runtime_resume_and_get(data->dev);
> >
> > PM_RUNTIME_ACQUIRE_AUTOSUSPEND() will mean you can rely on this
> > being auto suspended on exiting scope.  Will allow early returns
> > and get rid of your goto that you noted already.
> 
> Sorry if this is a silly request, however I can't seem to understand how the
> macro mentioned above works... Could someone point me to documentation
> on it or a functioning example in a driver?

`git grep ...` is your everyday tool for most questions in the kernel :-)

In short it uses the guard()()-type approach beneath, it creates a scoped auto
variable on stack and then uses it at the function epilogue.

-- 
With Best Regards,
Andy Shevchenko