[PATCH 0/3] iio:light: add driver for veml6046x00 RGBIR color sensor

[PATCH 0/3] iio:light: add driver for veml6046x00 RGBIR color sensor

[Andreas Klinger]

This patchset adds an IIO driver for Vishay veml6046x00 RGBIR color sensor

Andreas Klinger (3):
  dt-bindings: iio: light: veml6046x00: add color sensor
  iio: light: add support for veml6046x00 RGBIR color sensor
  MAINTAINER: add maintainer for veml6046x00

 .../iio/light/vishay,veml6046x00.yaml         |  49 +
 MAINTAINERS                                   |   6 +
 drivers/iio/light/Kconfig                     |  13 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/iio/light/veml6046x00.c               | 890 ++++++++++++++++++
 5 files changed, 959 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
 create mode 100644 drivers/iio/light/veml6046x00.c

-- 
2.39.5

[PATCH 1/3] dt-bindings: iio: light: veml6046x00: add color sensor

[Andreas Klinger]

Add a new compatible for Vishay high accuracy RGBIR color sensor
veml6046x00.

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 .../iio/light/vishay,veml6046x00.yaml         | 49 +++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml

diff --git a/Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml b/Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
new file mode 100644
index 000000000000..3207800fc539
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
@@ -0,0 +1,49 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/vishay,veml6046x00.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Vishay VEML6046X00 High accuracy RGBIR color sensor
+
+maintainers:
+  - Andreas Klinger <ak@it-klinger.de>
+
+description:
+  VEML6046X00 datasheet at https://www.vishay.com/docs/80173/veml6046x00.pdf
+
+properties:
+  compatible:
+    enum:
+      - vishay,veml6046x00
+
+  reg:
+    maxItems: 1
+
+  vdd-supply: true
+
+  interrupts:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        color-sensor@29 {
+            compatible = "vishay,veml6046x00";
+            reg = <0x29>;
+            vdd-supply = <&vdd_reg>;
+            interrupt-parent = <&gpio2>;
+            interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
+        };
+    };
+...
-- 
2.39.5

Re: [PATCH 1/3] dt-bindings: iio: light: veml6046x00: add color sensor

[Rob Herring (Arm)]

On Sun, 16 Mar 2025 12:31:29 +0100, Andreas Klinger wrote:
> Add a new compatible for Vishay high accuracy RGBIR color sensor
> veml6046x00.
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
>  .../iio/light/vishay,veml6046x00.yaml         | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.example.dts:33.33-34 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1518: dt_binding_check] Error 2
make: *** [Makefile:248: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250316113131.62884-2-ak@it-klinger.de

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/3] dt-bindings: iio: light: veml6046x00: add color sensor

[Jonathan Cameron]

On Sun, 16 Mar 2025 12:31:29 +0100
Andreas Klinger <ak@it-klinger.de> wrote:

> Add a new compatible for Vishay high accuracy RGBIR color sensor
> veml6046x00.
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> ---
>  .../iio/light/vishay,veml6046x00.yaml         | 49 +++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml b/Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
> new file mode 100644
> index 000000000000..3207800fc539
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/vishay,veml6046x00.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Vishay VEML6046X00 High accuracy RGBIR color sensor
> +
> +maintainers:
> +  - Andreas Klinger <ak@it-klinger.de>
> +
> +description:
> +  VEML6046X00 datasheet at https://www.vishay.com/docs/80173/veml6046x00.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - vishay,veml6046x00
> +
> +  reg:
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +  interrupts:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        color-sensor@29 {
> +            compatible = "vishay,veml6046x00";
> +            reg = <0x29>;
> +            vdd-supply = <&vdd_reg>;
> +            interrupt-parent = <&gpio2>;
> +            interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
Need an include for this I think.  Make sure to test build your
bindings following the instructions in the bot message.

Thanks,

Jonathan

> +        };
> +    };
> +...

Re: [PATCH 1/3] dt-bindings: iio: light: veml6046x00: add color sensor

[Andreas Klinger]

[-- Attachment #1: Type: text/plain, Size: 2427 bytes --]

Hi Jonathan,


Jonathan Cameron <jic23@kernel.org> schrieb am Mo, 17. Mär 11:00:
> On Sun, 16 Mar 2025 12:31:29 +0100
> Andreas Klinger <ak@it-klinger.de> wrote:
> 
> > Add a new compatible for Vishay high accuracy RGBIR color sensor
> > veml6046x00.
> > 
> > Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> > ---
> >  .../iio/light/vishay,veml6046x00.yaml         | 49 +++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml b/Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
> > new file mode 100644
> > index 000000000000..3207800fc539
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
> > @@ -0,0 +1,49 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/light/vishay,veml6046x00.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Vishay VEML6046X00 High accuracy RGBIR color sensor
> > +
> > +maintainers:
> > +  - Andreas Klinger <ak@it-klinger.de>
> > +
> > +description:
> > +  VEML6046X00 datasheet at https://www.vishay.com/docs/80173/veml6046x00.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - vishay,veml6046x00
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vdd-supply: true
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - vdd-supply
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +        #address-cells = <1>;
> > +        #size-cells = <0>;
> > +
> > +        color-sensor@29 {
> > +            compatible = "vishay,veml6046x00";
> > +            reg = <0x29>;
> > +            vdd-supply = <&vdd_reg>;
> > +            interrupt-parent = <&gpio2>;
> > +            interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
> Need an include for this I think.  Make sure to test build your
> bindings following the instructions in the bot message.

I already sent out an version 2 yesterday which is with the include, tested and
already reviewed by Krzysztof.

Andreas

> 
> Thanks,
> 
> Jonathan
> 
> > +        };
> > +    };
> > +...
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 1/3] dt-bindings: iio: light: veml6046x00: add color sensor

[Krzysztof Kozlowski]

On 17/03/2025 12:17, Andreas Klinger wrote:
>>> +  - |
>>> +    i2c {
>>> +        #address-cells = <1>;
>>> +        #size-cells = <0>;
>>> +
>>> +        color-sensor@29 {
>>> +            compatible = "vishay,veml6046x00";
>>> +            reg = <0x29>;
>>> +            vdd-supply = <&vdd_reg>;
>>> +            interrupt-parent = <&gpio2>;
>>> +            interrupts = <3 IRQ_TYPE_EDGE_FALLING>;
>> Need an include for this I think.  Make sure to test build your
>> bindings following the instructions in the bot message.
> 
> I already sent out an version 2 yesterday which is with the include, tested and
> already reviewed by Krzysztof.

The point is that you should test it, not rely on us and our tools (e.g.
my or Rob's machine). If our tools report such issues, it is already too
late because it means you did not follow the process.

Best regards,
Krzysztof

[PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Andreas Klinger]

Add Vishay VEML6046X00 high accuracy RGBIR color sensor.

This sensor provides three colour (red, green and blue) as well as one
infrared (IR) channel through I2C.

Support direct and buffered mode.

An optional interrupt for signaling green colour threshold underflow or
overflow is not supported so far.

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 drivers/iio/light/Kconfig       |  13 +
 drivers/iio/light/Makefile      |   1 +
 drivers/iio/light/veml6046x00.c | 890 ++++++++++++++++++++++++++++++++
 3 files changed, 904 insertions(+)
 create mode 100644 drivers/iio/light/veml6046x00.c

diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index e34e551eef3e..d79bc302afab 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -702,6 +702,19 @@ config VEML6040
 	  To compile this driver as a module, choose M here: the
 	  module will be called veml6040.
 
+config VEML6046X00
+	tristate "VEML6046X00 RGBIR color sensor"
+	select REGMAP_I2C
+	select IIO_BUFFER
+	select IIO_TRIGGERED_BUFFER
+	depends on I2C
+	help
+	  Say Y here if you want to build a driver for the Vishay VEML6046X00
+	  high accuracy RGBIR color sensor.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called veml6046x00.
+
 config VEML6070
 	tristate "VEML6070 UV A light sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 11a4041b918a..14c9dc7813d6 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -65,6 +65,7 @@ obj-$(CONFIG_VCNL4035)		+= vcnl4035.o
 obj-$(CONFIG_VEML3235)		+= veml3235.o
 obj-$(CONFIG_VEML6030)		+= veml6030.o
 obj-$(CONFIG_VEML6040)		+= veml6040.o
+obj-$(CONFIG_VEML6046X00)	+= veml6046x00.o
 obj-$(CONFIG_VEML6070)		+= veml6070.o
 obj-$(CONFIG_VEML6075)		+= veml6075.o
 obj-$(CONFIG_VL6180)		+= vl6180.o
diff --git a/drivers/iio/light/veml6046x00.c b/drivers/iio/light/veml6046x00.c
new file mode 100644
index 000000000000..8e6232e1ab70
--- /dev/null
+++ b/drivers/iio/light/veml6046x00.c
@@ -0,0 +1,890 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * VEML6046X00 High Accuracy RGBIR Color Sensor
+ *
+ * Copyright (c) 2025 Andreas Klinger <ak@it-klinger.de>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/types.h>
+#include <linux/units.h>
+#include <linux/pm_runtime.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+/* Device registers */
+#define VEML6046X00_REG_CONF0       0x00
+#define VEML6046X00_REG_CONF1       0x01
+#define VEML6046X00_REG_THDH_L      0x04
+#define VEML6046X00_REG_THDH_H      0x05
+#define VEML6046X00_REG_THDL_L      0x06
+#define VEML6046X00_REG_THDL_H      0x07
+#define VEML6046X00_REG_R_L         0x10
+#define VEML6046X00_REG_R_H         0x11
+#define VEML6046X00_REG_G_L         0x12
+#define VEML6046X00_REG_G_H         0x13
+#define VEML6046X00_REG_B_L         0x14
+#define VEML6046X00_REG_B_H         0x15
+#define VEML6046X00_REG_IR_L        0x16
+#define VEML6046X00_REG_IR_H        0x17
+#define VEML6046X00_REG_ID_L        0x18
+#define VEML6046X00_REG_ID_H        0x19
+#define VEML6046X00_REG_INT_L       0x1A
+#define VEML6046X00_REG_INT_H       0x1B
+#define VEML6046X00_REG_DATA(ch)    (VEML6046X00_REG_R_L + (ch))
+
+/* Bit masks for specific functionality */
+#define VEML6046X00_CONF0_ON_0      BIT(0)
+#define VEML6046X00_CONF0_INT       BIT(1)
+#define VEML6046X00_CONF0_AF_TRIG   BIT(2)
+#define VEML6046X00_CONF0_AF        BIT(3)
+#define VEML6046X00_CONF0_IT        GENMASK(6, 4)
+#define VEML6046X00_CONF1_CAL       BIT(0)
+#define VEML6046X00_CONF1_PERS      GENMASK(2, 1)
+#define VEML6046X00_CONF1_GAIN      GENMASK(4, 3)
+#define VEML6046X00_CONF1_PD_D2     BIT(6)
+#define VEML6046X00_CONF1_ON_1      BIT(7)
+#define VEML6046X00_INT_TH_H        BIT(1)
+#define VEML6046X00_INT_TH_L        BIT(2)
+#define VEML6046X00_INT_DRDY        BIT(3)
+#define VEML6046X00_INT_MASK        (VEML6046X00_INT_TH_H | \
+				VEML6046X00_INT_TH_L | VEML6046X00_INT_DRDY)
+
+#define	VEML6046X00_GAIN_1          0x0
+#define	VEML6046X00_GAIN_2          0x1
+#define	VEML6046X00_GAIN_0_66       0x2
+#define	VEML6046X00_GAIN_0_5        0x3
+
+/* Autosuspend delay */
+#define VEML6046X00_AUTOSUSPEND_MS  3000
+
+enum veml6046x00_scan {
+	VEML6046X00_SCAN_R,
+	VEML6046X00_SCAN_G,
+	VEML6046X00_SCAN_B,
+	VEML6046X00_SCAN_IR,
+	VEML6046X00_SCAN_TIMESTAMP,
+};
+
+/*
+ * regmap fields
+ */
+struct veml6046x00_rf {
+	struct regmap_field *int_en;
+	struct regmap_field *mode;
+	struct regmap_field *trig;
+	struct regmap_field *it;
+	struct regmap_field *pers;
+};
+
+struct veml6046x00_data {
+	struct device *dev;
+	struct regmap *regmap;
+	struct iio_trigger *trig;
+	struct veml6046x00_rf rf;
+};
+
+struct veml6046x00_scan_buf {
+	__le16 chans[4];
+
+	s64 timestamp __aligned(8);
+};
+
+static const int veml6046x00_it[][2] = {
+	{ 0, 3125 },
+	{ 0, 6250 },
+	{ 0, 12500 },
+	{ 0, 25000 },
+	{ 0, 50000 },
+	{ 0, 100000 },
+	{ 0, 200000 },
+	{ 0, 400000 },
+};
+
+static const int veml6046x00_gains[][2] = {
+	{ 0, 250000 },
+	{ 0, 330000 },
+	{ 0, 500000 },
+	{ 0, 660000 },
+	{ 1, 0 },
+	{ 2, 0 },
+};
+
+/*
+ * Persistence = 1/2/4/8 x integration time
+ * Minimum time for which light readings must stay above configured
+ * threshold to assert the interrupt.
+ */
+static const char * const period_values[] = {
+		"0.003125 0.00625 0.0125 0.025",
+		"0.00625 0.0125 0.025 0.05",
+		"0.0125 0.025 0.05 0.1",
+		"0.025 0.050 0.1 0.2",
+		"0.05 0.1 0.2 0.4",
+		"0.1 0.2 0.4 0.8",
+		"0.2 0.4 0.8 1.6",
+		"0.4 0.8 1.6 3.2"
+};
+
+/*
+ * Return list of valid period values in seconds corresponding to
+ * the currently active integration time.
+ */
+static ssize_t in_illuminance_period_available_show(struct device *dev,
+						    struct device_attribute *attr,
+						    char *buf)
+{
+	struct veml6046x00_data *data = iio_priv(dev_to_iio_dev(dev));
+	int ret, reg;
+
+	ret = regmap_field_read(data->rf.it, &reg);
+	if (ret)
+		return ret;
+
+	if (reg < 0 || reg >= ARRAY_SIZE(period_values))
+		return -EINVAL;
+
+	return sysfs_emit(buf, "%s\n", period_values[reg]);
+}
+
+static IIO_DEVICE_ATTR_RO(in_illuminance_period_available, 0);
+
+static struct attribute *veml6046x00_event_attributes[] = {
+	&iio_dev_attr_in_illuminance_period_available.dev_attr.attr,
+	NULL
+};
+
+static const struct attribute_group veml6046x00_event_attr_group = {
+	.attrs = veml6046x00_event_attributes,
+};
+
+/*
+ * Two bits (RGB_ON_0 and RGB_ON_1) must be cleared to power on the device.
+ */
+static int veml6046x00_power_on(struct veml6046x00_data *data)
+{
+	int ret;
+
+	ret = regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF0,
+			VEML6046X00_CONF0_ON_0);
+	if (ret) {
+		dev_err(data->dev, "Failed to set bit for power on %d\n", ret);
+		return ret;
+	}
+
+	return regmap_clear_bits(data->regmap, VEML6046X00_REG_CONF1,
+			VEML6046X00_CONF1_ON_1);
+}
+
+/*
+ * Two bits (RGB_ON_0 and RGB_ON_1) must be set to power off the device.
+ */
+static int veml6046x00_shutdown(struct veml6046x00_data *data)
+{
+	int ret;
+
+	ret = regmap_set_bits(data->regmap, VEML6046X00_REG_CONF0,
+			      VEML6046X00_CONF0_ON_0);
+	if (ret) {
+		dev_err(data->dev, "Failed to set bit for shutdown %d\n", ret);
+		return ret;
+	}
+
+	return regmap_set_bits(data->regmap, VEML6046X00_REG_CONF1,
+			       VEML6046X00_CONF1_ON_1);
+}
+
+static void veml6046x00_shutdown_action(void *data)
+{
+	veml6046x00_shutdown(data);
+}
+
+static const struct iio_chan_spec veml6046x00_channels[] = {
+	{
+		.type = IIO_LIGHT,
+		.address = VEML6046X00_REG_R_L,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_RED,
+		.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),
+		.scan_index = VEML6046X00_SCAN_R,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
+	},
+	{
+		.type = IIO_LIGHT,
+		.address = VEML6046X00_REG_G_L,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_GREEN,
+		.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),
+		.scan_index = VEML6046X00_SCAN_G,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
+	},
+	{
+		.type = IIO_LIGHT,
+		.address = VEML6046X00_REG_B_L,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_BLUE,
+		.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),
+		.scan_index = VEML6046X00_SCAN_B,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
+	},
+	{
+		.type = IIO_LIGHT,
+		.address = VEML6046X00_REG_IR_L,
+		.modified = 1,
+		.channel2 = IIO_MOD_LIGHT_IR,
+		.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),
+		.scan_index = VEML6046X00_SCAN_IR,
+		.scan_type = {
+			.sign = 'u',
+			.realbits = 16,
+			.storagebits = 16,
+			.endianness = IIO_LE,
+		},
+	},
+	IIO_CHAN_SOFT_TIMESTAMP(VEML6046X00_SCAN_TIMESTAMP),
+};
+
+static const struct regmap_config veml6046x00_regmap_config = {
+	.name = "veml6046x00_regmap",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = VEML6046X00_REG_INT_H,
+};
+
+static const struct reg_field veml6046x00_rf_int_en =
+	REG_FIELD(VEML6046X00_REG_CONF0, 1, 1);
+
+static const struct reg_field veml6046x00_rf_trig =
+	REG_FIELD(VEML6046X00_REG_CONF0, 2, 2);
+
+static const struct reg_field veml6046x00_rf_mode =
+	REG_FIELD(VEML6046X00_REG_CONF0, 3, 3);
+
+static const struct reg_field veml6046x00_rf_it =
+	REG_FIELD(VEML6046X00_REG_CONF0, 4, 6);
+
+static const struct reg_field veml6046x00_rf_pers =
+	REG_FIELD(VEML6046X00_REG_CONF1, 1, 2);
+
+static int veml6046x00_regfield_init(struct veml6046x00_data *data)
+{
+	struct regmap *regmap = data->regmap;
+	struct device *dev = data->dev;
+	struct regmap_field *rm_field;
+	struct veml6046x00_rf *rf = &data->rf;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6046x00_rf_int_en);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->int_en = rm_field;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6046x00_rf_mode);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->mode = rm_field;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6046x00_rf_trig);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->trig = rm_field;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6046x00_rf_it);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->it = rm_field;
+
+	rm_field = devm_regmap_field_alloc(dev, regmap, veml6046x00_rf_pers);
+	if (IS_ERR(rm_field))
+		return PTR_ERR(rm_field);
+	rf->pers = rm_field;
+
+	return 0;
+}
+
+static int veml6046x00_get_it_usec(struct veml6046x00_data *data, int *it_usec)
+{
+	int ret, reg;
+
+	ret = regmap_field_read(data->rf.it, &reg);
+	if (ret)
+		return ret;
+
+	switch (reg) {
+	case 0:
+		*it_usec = 3125;
+		break;
+	case 1:
+		*it_usec = 6250;
+		break;
+	case 2:
+		*it_usec = 12500;
+		break;
+	case 3:
+		*it_usec = 25000;
+		break;
+	case 4:
+		*it_usec = 50000;
+		break;
+	case 5:
+		*it_usec = 100000;
+		break;
+	case 6:
+		*it_usec = 200000;
+		break;
+	case 7:
+		*it_usec = 400000;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int veml6046x00_set_it(struct iio_dev *iio, int val, int val2)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	int ret, new_it;
+
+	if (val)
+		return -EINVAL;
+
+	switch (val2) {
+	case 3125:
+		new_it = 0x00;
+		break;
+	case 6250:
+		new_it = 0x01;
+		break;
+	case 12500:
+		new_it = 0x02;
+		break;
+	case 25000:
+		new_it = 0x03;
+		break;
+	case 50000:
+		new_it = 0x04;
+		break;
+	case 100000:
+		new_it = 0x05;
+		break;
+	case 200000:
+		new_it = 0x06;
+		break;
+	case 400000:
+		new_it = 0x07;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = regmap_field_write(data->rf.it, new_it);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int veml6046x00_set_scale(struct iio_dev *iio, int val, int val2)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	int new_scale;
+
+	if (val == 0 && val2 == 250000) {
+		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_5) |
+					VEML6046X00_CONF1_PD_D2;
+	} else if (val == 0 && val2 == 330000) {
+		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_66) |
+					VEML6046X00_CONF1_PD_D2;
+	} else if (val == 0 && val2 == 500000) {
+		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_5);
+	} else if (val == 0 && val2 == 660000) {
+		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_66);
+	} else if (val == 1 && val2 == 0) {
+		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_1);
+	} else if (val == 2 && val2 == 0) {
+		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_2);
+	} else {
+		return -EINVAL;
+	}
+
+	return regmap_update_bits(data->regmap, VEML6046X00_REG_CONF1,
+				 VEML6046X00_CONF1_GAIN |
+				 VEML6046X00_CONF1_PD_D2,
+				 new_scale);
+}
+
+static int veml6046x00_get_scale(struct veml6046x00_data *data,
+				 int *val, int *val2)
+{
+	int ret, reg;
+
+	ret = regmap_read(data->regmap, VEML6046X00_REG_CONF1, &reg);
+	if (ret)
+		return ret;
+
+	switch (FIELD_GET(VEML6046X00_CONF1_GAIN, reg)) {
+	case 0:
+		*val = 1;
+		*val2 = 0;
+		break;
+	case 1:
+		*val = 2;
+		*val2 = 0;
+		break;
+	case 2:
+		*val = 0;
+		*val2 = 660000;
+		break;
+	case 3:
+		*val = 0;
+		*val2 = 500000;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (reg & VEML6046X00_CONF1_PD_D2)
+		*val2 /= 2;
+
+	return IIO_VAL_INT_PLUS_MICRO;
+}
+
+static int veml6046x00_set_mode(struct veml6046x00_data *data, bool state)
+{
+	return regmap_field_write(data->rf.mode, state);
+}
+
+static int veml6046x00_set_trig(struct veml6046x00_data *data, bool state)
+{
+	return regmap_field_write(data->rf.trig, state);
+}
+
+static int veml6046x00_wait_data_available(struct iio_dev *iio, int usecs)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	int ret, reg;
+	int cnt = 2;
+	int i;
+
+	for (i = 0; i < cnt; i++) {
+		ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, &reg);
+		if (ret) {
+			dev_err(data->dev,
+					"Failed to read interrupt register %d\n", ret);
+			return -EIO;
+		}
+
+		if (reg & VEML6046X00_INT_DRDY)
+			return 1;
+
+		if (i < cnt)
+			fsleep(usecs);
+	}
+
+	return 0;
+}
+
+static int veml6046x00_single_read(struct iio_dev *iio,
+					enum iio_modifier modifier, int *val)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	int addr, it_usec, ret;
+	uint8_t reg[2];
+
+	switch (modifier) {
+	case IIO_MOD_LIGHT_RED:
+		addr = VEML6046X00_REG_R_L;
+	break;
+	case IIO_MOD_LIGHT_GREEN:
+		addr = VEML6046X00_REG_G_L;
+	break;
+	case IIO_MOD_LIGHT_BLUE:
+		addr = VEML6046X00_REG_B_L;
+	break;
+	case IIO_MOD_LIGHT_IR:
+		addr = VEML6046X00_REG_IR_L;
+	break;
+	default:
+		return -EINVAL;
+	}
+	ret = pm_runtime_resume_and_get(data->dev);
+	if (ret)
+		return ret;
+
+	ret = veml6046x00_get_it_usec(data, &it_usec);
+	if (ret < 0)
+		return ret;
+
+	veml6046x00_set_mode(data, 1);
+
+	veml6046x00_set_trig(data, 1);
+
+	/* integration time + 10 % to ensure completion */
+	fsleep(it_usec + (it_usec / 10));
+
+	ret = veml6046x00_wait_data_available(iio, it_usec * 10);
+	if (ret == 1) {
+		dev_dbg(data->dev, "data ready\n");
+	} else {
+		dev_warn(data->dev, "no data ready ret: %d\n", ret);
+		goto no_data;
+	}
+
+	ret = iio_device_claim_direct_mode(iio);
+	if (ret)
+		return ret;
+
+	ret = regmap_bulk_read(data->regmap, addr, reg, sizeof(reg));
+	iio_device_release_direct_mode(iio);
+	if (ret)
+		return ret;
+
+	pm_runtime_mark_last_busy(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
+
+	*val = reg[1] << 8 | reg[0];
+
+	return IIO_VAL_INT;
+
+no_data:
+	pm_runtime_mark_last_busy(data->dev);
+	pm_runtime_put_autosuspend(data->dev);
+
+	return -EINVAL;
+}
+
+static int veml6046x00_read_raw(struct iio_dev *iio,
+				struct iio_chan_spec const *chan, int *val,
+				int *val2, long mask)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (chan->type != IIO_LIGHT)
+			return -EINVAL;
+		return veml6046x00_single_read(iio, chan->channel2, val);
+	case IIO_CHAN_INFO_INT_TIME:
+		*val = 0;
+		return veml6046x00_get_it_usec(data, val2);
+	case IIO_CHAN_INFO_SCALE:
+		return veml6046x00_get_scale(data, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml6046x00_read_avail(struct iio_dev *iio,
+				  struct iio_chan_spec const *chan,
+				  const int **vals, int *type, int *length,
+				  long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		*vals = (int *)&veml6046x00_it;
+		*length = 2 * ARRAY_SIZE(veml6046x00_it);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_SCALE:
+		*vals = (int *)&veml6046x00_gains;
+		*length = 2 * ARRAY_SIZE(veml6046x00_gains);
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int veml6046x00_write_raw(struct iio_dev *iio,
+				 struct iio_chan_spec const *chan,
+				 int val, int val2, long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_INT_TIME:
+		return veml6046x00_set_it(iio, val, val2);
+	case IIO_CHAN_INFO_SCALE:
+		return veml6046x00_set_scale(iio, val, val2);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info veml6046x00_info_no_irq = {
+	.read_raw = veml6046x00_read_raw,
+	.read_avail = veml6046x00_read_avail,
+	.write_raw = veml6046x00_write_raw,
+};
+
+static int veml6046x00_buffer_preenable(struct iio_dev *iio)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	struct device *dev = data->dev;
+	int ret;
+
+	ret = veml6046x00_set_mode(data, 0);
+	if (ret)
+		dev_err(data->dev, "Failed to set mode %d\n", ret);
+
+	ret = veml6046x00_set_trig(data, 0);
+	if (ret)
+		dev_err(data->dev, "Failed to set trigger %d\n", ret);
+
+	return pm_runtime_resume_and_get(dev);
+}
+
+static int veml6046x00_buffer_postdisable(struct iio_dev *iio)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	struct device *dev = data->dev;
+	int ret;
+
+	ret = veml6046x00_set_mode(data, 1);
+	if (ret)
+		dev_err(data->dev, "Failed to set mode %d\n", ret);
+
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	return 0;
+}
+
+static const struct iio_buffer_setup_ops veml6046x00_buffer_setup_ops = {
+	.preenable = veml6046x00_buffer_preenable,
+	.postdisable = veml6046x00_buffer_postdisable,
+};
+
+static irqreturn_t veml6046x00_trig_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *iio = pf->indio_dev;
+	struct veml6046x00_data *data = iio_priv(iio);
+	int ret;
+	struct veml6046x00_scan_buf scan;
+
+	memset(&scan, 0, sizeof(scan));
+
+	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_R_L, &scan.chans,
+						sizeof(scan.chans));
+	if (ret)
+		goto done;
+
+	iio_push_to_buffers_with_timestamp(iio, &scan, iio_get_time_ns(iio));
+
+done:
+	iio_trigger_notify_done(iio->trig);
+
+	return IRQ_HANDLED;
+}
+
+static int veml6046x00_validate_part_id(struct veml6046x00_data *data)
+{
+	int part_id, ret;
+	__le16 reg;
+
+	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_ID_L, &reg, sizeof(reg));
+	if (ret) {
+		dev_info(data->dev, "Failed to read ID\n");
+		return -EIO;
+	}
+
+	part_id = le16_to_cpu(reg);
+	if (part_id != 0x0001)
+		dev_info(data->dev, "Unknown ID %#02x\n", part_id);
+
+	return 0;
+}
+
+static int veml6046x00_setup_device(struct iio_dev *iio)
+{
+	struct veml6046x00_data *data = iio_priv(iio);
+	struct device *dev = data->dev;
+	int ret, val;
+	__le16 reg16;
+	uint8_t reg[2];
+
+	reg[0] = VEML6046X00_CONF0_AF;
+	reg[1] = 0x00;
+	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_CONF0, reg, sizeof(reg));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set configuration\n");
+
+	reg16 = cpu_to_le16(0);
+	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_THDL_L, &reg16, sizeof(reg16));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set low threshold\n");
+
+	reg16 = cpu_to_le16(U16_MAX);
+	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_THDH_L, &reg16, sizeof(reg16));
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to set high threshold\n");
+
+	ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, &val);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to clear interrupts\n");
+
+	return 0;
+}
+
+static int veml6046x00_probe(struct i2c_client *i2c)
+{
+	struct device *dev = &i2c->dev;
+	struct veml6046x00_data *data;
+	struct iio_dev *iio;
+	struct regmap *regmap;
+	int ret;
+
+	regmap = devm_regmap_init_i2c(i2c, &veml6046x00_regmap_config);
+	if (IS_ERR(regmap))
+		return dev_err_probe(dev, PTR_ERR(regmap),
+				     "Failed to set regmap\n");
+
+	iio = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!iio)
+		return -ENOMEM;
+
+	data = iio_priv(iio);
+	i2c_set_clientdata(i2c, iio);
+	data->dev = dev;
+	data->regmap = regmap;
+
+	ret = veml6046x00_regfield_init(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to init regfield\n");
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable regulator\n");
+
+	ret = devm_add_action_or_reset(dev, veml6046x00_shutdown_action, data);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to add shut down action\n");
+
+	ret = pm_runtime_set_active(dev);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "Failed to activate PM runtime\n");
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to enable PM runtime\n");
+
+	pm_runtime_get_noresume(dev);
+	pm_runtime_set_autosuspend_delay(dev, VEML6046X00_AUTOSUSPEND_MS);
+	pm_runtime_use_autosuspend(dev);
+
+	ret = veml6046x00_validate_part_id(data);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to validate device ID\n");
+
+	iio->name = i2c->name;
+	iio->channels = veml6046x00_channels;
+	iio->num_channels = ARRAY_SIZE(veml6046x00_channels);
+	iio->modes = INDIO_DIRECT_MODE;
+
+	iio->info = &veml6046x00_info_no_irq;
+
+	ret = veml6046x00_setup_device(iio);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
+					      veml6046x00_trig_handler,
+					      &veml6046x00_buffer_setup_ops);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to register triggered buffer");
+
+	pm_runtime_mark_last_busy(dev);
+	pm_runtime_put_autosuspend(dev);
+
+	ret = devm_iio_device_register(dev, iio);
+	if (ret)
+		return dev_err_probe(dev, ret, "Failed to register iio device");
+
+	return 0;
+}
+
+static int veml6046x00_runtime_suspend(struct device *dev)
+{
+	struct veml6046x00_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return veml6046x00_shutdown(data);
+}
+
+static int veml6046x00_runtime_resume(struct device *dev)
+{
+	struct veml6046x00_data *data = iio_priv(dev_get_drvdata(dev));
+
+	return veml6046x00_power_on(data);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(veml6046x00_pm_ops, veml6046x00_runtime_suspend,
+				 veml6046x00_runtime_resume, NULL);
+
+static const struct of_device_id veml6046x00_of_match[] = {
+	{
+		.compatible = "vishay,veml6046x00",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, veml6046x00_of_match);
+
+static const struct i2c_device_id veml6046x00_id[] = {
+	{ "veml6046x00" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, veml6046x00_id);
+
+static struct i2c_driver veml6046x00_driver = {
+	.driver = {
+		.name = "veml6046x00",
+		.of_match_table = veml6046x00_of_match,
+		.pm = pm_ptr(&veml6046x00_pm_ops),
+	},
+	.probe = veml6046x00_probe,
+	.id_table = veml6046x00_id,
+};
+module_i2c_driver(veml6046x00_driver);
+
+MODULE_AUTHOR("Andreas Klinger <ak@it-klinger.de>");
+MODULE_DESCRIPTION("VEML6046X00 RGBIR Color Sensor");
+MODULE_LICENSE("GPL");
-- 
2.39.5

Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Jonathan Cameron]

On Sun, 16 Mar 2025 12:31:30 +0100
Andreas Klinger <ak@it-klinger.de> wrote:

> Add Vishay VEML6046X00 high accuracy RGBIR color sensor.
> 
> This sensor provides three colour (red, green and blue) as well as one
> infrared (IR) channel through I2C.
> 
> Support direct and buffered mode.
> 
> An optional interrupt for signaling green colour threshold underflow or
> overflow is not supported so far.
> 
> Signed-off-by: Andreas Klinger <ak@it-klinger.de>

Hi Andreas,

A nice clean driver.
A few comments and questions inline.

Jonathan

> diff --git a/drivers/iio/light/veml6046x00.c b/drivers/iio/light/veml6046x00.c
> new file mode 100644
> index 000000000000..8e6232e1ab70
> --- /dev/null
> +++ b/drivers/iio/light/veml6046x00.c
> @@ -0,0 +1,890 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * VEML6046X00 High Accuracy RGBIR Color Sensor
> + *
> + * Copyright (c) 2025 Andreas Klinger <ak@it-klinger.de>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/types.h>
> +#include <linux/units.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger.h>

So far you aren't providing a trigger so I'm not
expect to see this header used.  Looking at the datasheet
I see there is a dataready interrupt, but it doesn't look like
the device has an autonomous / sequencer type mode, so will always
need to use another trigger (hrtimer, sysfs or some other hardware
source).

> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/triggered_buffer.h>

> +
> +struct veml6046x00_scan_buf {
> +	__le16 chans[4];
> +
> +	s64 timestamp __aligned(8);
aligned_s64 now available as a type to use here.

> +};

> +static IIO_DEVICE_ATTR_RO(in_illuminance_period_available, 0);
> +
> +static struct attribute *veml6046x00_event_attributes[] = {
> +	&iio_dev_attr_in_illuminance_period_available.dev_attr.attr,
I thought we didn't have event support yet?  If not why do we
have event related attributes?
> +	NULL
> +};
> +
> +static const struct attribute_group veml6046x00_event_attr_group = {
> +	.attrs = veml6046x00_event_attributes,
> +};


> +
> +static int veml6046x00_set_it(struct iio_dev *iio, int val, int val2)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	int ret, new_it;
> +
> +	if (val)
> +		return -EINVAL;
> +
> +	switch (val2) {
> +	case 3125:
> +		new_it = 0x00;
> +		break;
> +	case 6250:
> +		new_it = 0x01;
> +		break;
> +	case 12500:
> +		new_it = 0x02;
> +		break;
> +	case 25000:
> +		new_it = 0x03;
> +		break;
> +	case 50000:
> +		new_it = 0x04;
> +		break;
> +	case 100000:
> +		new_it = 0x05;
> +		break;
> +	case 200000:
> +		new_it = 0x06;
> +		break;
> +	case 400000:
> +		new_it = 0x07;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = regmap_field_write(data->rf.it, new_it);
> +	if (ret)
> +		return ret;

return regmap_field_write()

> +
> +	return 0;
> +}
> +
> +static int veml6046x00_set_scale(struct iio_dev *iio, int val, int val2)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	int new_scale;
> +
> +	if (val == 0 && val2 == 250000) {

Maybe a lookup table and a loop?

> +		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_5) |
> +					VEML6046X00_CONF1_PD_D2;
> +	} else if (val == 0 && val2 == 330000) {
> +		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_66) |
> +					VEML6046X00_CONF1_PD_D2;
> +	} else if (val == 0 && val2 == 500000) {
> +		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_5);
> +	} else if (val == 0 && val2 == 660000) {
> +		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_66);
> +	} else if (val == 1 && val2 == 0) {
> +		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_1);
> +	} else if (val == 2 && val2 == 0) {
> +		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_2);
> +	} else {
> +		return -EINVAL;
> +	}
> +
> +	return regmap_update_bits(data->regmap, VEML6046X00_REG_CONF1,
> +				 VEML6046X00_CONF1_GAIN |
> +				 VEML6046X00_CONF1_PD_D2,
> +				 new_scale);
> +}
> +
> +static int veml6046x00_get_scale(struct veml6046x00_data *data,
> +				 int *val, int *val2)

How is this related to integration time?  I'd normally expect
to see that read in here somewhere as well as doubling integration
time tends to double scale.

> +{
> +	int ret, reg;
> +
> +	ret = regmap_read(data->regmap, VEML6046X00_REG_CONF1, &reg);
> +	if (ret)
> +		return ret;
> +
> +	switch (FIELD_GET(VEML6046X00_CONF1_GAIN, reg)) {
> +	case 0:
> +		*val = 1;
> +		*val2 = 0;
> +		break;
> +	case 1:
> +		*val = 2;
> +		*val2 = 0;
> +		break;
> +	case 2:
> +		*val = 0;
> +		*val2 = 660000;
> +		break;
> +	case 3:
> +		*val = 0;
> +		*val2 = 500000;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (reg & VEML6046X00_CONF1_PD_D2)
> +		*val2 /= 2;
> +
> +	return IIO_VAL_INT_PLUS_MICRO;
> +}
> +
> +static int veml6046x00_set_mode(struct veml6046x00_data *data, bool state)
> +{
> +	return regmap_field_write(data->rf.mode, state);
as below.

> +}
> +
> +static int veml6046x00_set_trig(struct veml6046x00_data *data, bool state)
> +{
> +	return regmap_field_write(data->rf.trig, state);

I'd argue these two aren't worth bothering with because the
field naming etc makes a direct call to regmap_field_write() obvious enough.

> +}
> +
> +static int veml6046x00_wait_data_available(struct iio_dev *iio, int usecs)

Document return value as non obvious.

> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	int ret, reg;
> +	int cnt = 2;
> +	int i;
> +
> +	for (i = 0; i < cnt; i++) {
> +		ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, &reg);
> +		if (ret) {
> +			dev_err(data->dev,
> +					"Failed to read interrupt register %d\n", ret);
> +			return -EIO;
> +		}
> +
> +		if (reg & VEML6046X00_INT_DRDY)
> +			return 1;
> +
> +		if (i < cnt)
> +			fsleep(usecs);
> +	}
> +
> +	return 0;
> +}
> +
> +static int veml6046x00_single_read(struct iio_dev *iio,
> +					enum iio_modifier modifier, int *val)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	int addr, it_usec, ret;
> +	uint8_t reg[2];
> +
> +	switch (modifier) {
> +	case IIO_MOD_LIGHT_RED:
> +		addr = VEML6046X00_REG_R_L;
> +	break;

break indent not matching kernel style. Needs to be one more tab in.

> +	case IIO_MOD_LIGHT_GREEN:
> +		addr = VEML6046X00_REG_G_L;
> +	break;
> +	case IIO_MOD_LIGHT_BLUE:
> +		addr = VEML6046X00_REG_B_L;
> +	break;
> +	case IIO_MOD_LIGHT_IR:
> +		addr = VEML6046X00_REG_IR_L;
> +	break;
> +	default:
> +		return -EINVAL;
> +	}
> +	ret = pm_runtime_resume_and_get(data->dev);
> +	if (ret)
> +		return ret;
> +
> +	ret = veml6046x00_get_it_usec(data, &it_usec);
> +	if (ret < 0)
> +		return ret;
> +
> +	veml6046x00_set_mode(data, 1);
Check for errors.
> +
> +	veml6046x00_set_trig(data, 1);
> +
> +	/* integration time + 10 % to ensure completion */
> +	fsleep(it_usec + (it_usec / 10));
> +
> +	ret = veml6046x00_wait_data_available(iio, it_usec * 10);
> +	if (ret == 1) {
> +		dev_dbg(data->dev, "data ready\n");
> +	} else {
> +		dev_warn(data->dev, "no data ready ret: %d\n", ret);
> +		goto no_data;
> +	}
> +
> +	ret = iio_device_claim_direct_mode(iio);
I'm killing these off slowly.  To save a follow up patch,
	if (!iio_device_claim_direct(iio))
		return -EBUSY;
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_bulk_read(data->regmap, addr, reg, sizeof(reg));
> +	iio_device_release_direct_mode(iio);
	
	iio_device_release_direct(iio);

If not I'll roll in the change with the many other driver updates
this entails.


> +	if (ret)
> +		return ret;
> +
> +	pm_runtime_mark_last_busy(data->dev);
> +	pm_runtime_put_autosuspend(data->dev);
> +
> +	*val = reg[1] << 8 | reg[0];

That's an endian conversion.  Use get_unaligned_le16() I think
Or read into an __le16 in the first place and you can use
le16_to_cpu() or similar.


> +
> +	return IIO_VAL_INT;
> +
> +no_data:
> +	pm_runtime_mark_last_busy(data->dev);
> +	pm_runtime_put_autosuspend(data->dev);
> +
> +	return -EINVAL;
> +}



> +static int veml6046x00_buffer_preenable(struct iio_dev *iio)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	struct device *dev = data->dev;
> +	int ret;
> +
> +	ret = veml6046x00_set_mode(data, 0);
> +	if (ret)
> +		dev_err(data->dev, "Failed to set mode %d\n", ret);

If these fail, error out.  We will fail to enter buffered mode, but
that is probably the correct thing to do if we are having comms
issues or similar.

> +
> +	ret = veml6046x00_set_trig(data, 0);
> +	if (ret)
> +		dev_err(data->dev, "Failed to set trigger %d\n", ret);
> +
> +	return pm_runtime_resume_and_get(dev);
> +}



> +static int veml6046x00_validate_part_id(struct veml6046x00_data *data)
> +{
> +	int part_id, ret;
> +	__le16 reg;
> +
> +	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_ID_L, &reg, sizeof(reg));
> +	if (ret) {
> +		dev_info(data->dev, "Failed to read ID\n");

return dev_err_probe() for this one.

> +		return -EIO;
> +	}
>
> +static int veml6046x00_setup_device(struct iio_dev *iio)
> +{
> +	struct veml6046x00_data *data = iio_priv(iio);
> +	struct device *dev = data->dev;
> +	int ret, val;
> +	__le16 reg16;
> +	uint8_t reg[2];
> +
> +	reg[0] = VEML6046X00_CONF0_AF;
> +	reg[1] = 0x00;
> +	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_CONF0, reg, sizeof(reg));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set configuration\n");
> +
> +	reg16 = cpu_to_le16(0);
> +	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_THDL_L, &reg16, sizeof(reg16));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set low threshold\n");
> +
> +	reg16 = cpu_to_le16(U16_MAX);
> +	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_THDH_L, &reg16, sizeof(reg16));
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to set high threshold\n");
> +
> +	ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, &val);
> +	if (ret < 0)

if (ret) here as well.  Good to be consistent for all regmap calls. None of them
return > 0 as far as I know.


> +		return dev_err_probe(dev, ret, "Failed to clear interrupts\n");
> +
> +	return 0;
> +}
> +
> +static int veml6046x00_probe(struct i2c_client *i2c)
> +{
> +	struct device *dev = &i2c->dev;
> +	struct veml6046x00_data *data;
> +	struct iio_dev *iio;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	regmap = devm_regmap_init_i2c(i2c, &veml6046x00_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "Failed to set regmap\n");
> +
> +	iio = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!iio)
> +		return -ENOMEM;
> +
> +	data = iio_priv(iio);
> +	i2c_set_clientdata(i2c, iio);
> +	data->dev = dev;
> +	data->regmap = regmap;
> +
> +	ret = veml6046x00_regfield_init(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to init regfield\n");
> +
> +	ret = devm_regulator_get_enable(dev, "vdd");
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable regulator\n");
> +
> +	ret = devm_add_action_or_reset(dev, veml6046x00_shutdown_action, data);

Mostly we want a devm action to match against a specific setup operation.  Here is
it that the device comes up in non shut down state?  Perhaps a comment to
make it clear.  Also, how do we know it's in a good state rather than part
configured by someone else?  I'm not seeing a reset sequence though perhaps
that effectively happens in setup_device()

> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to add shut down action\n");
> +
> +	ret = pm_runtime_set_active(dev);
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "Failed to activate PM runtime\n");
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable PM runtime\n");
> +
> +	pm_runtime_get_noresume(dev);
> +	pm_runtime_set_autosuspend_delay(dev, VEML6046X00_AUTOSUSPEND_MS);
> +	pm_runtime_use_autosuspend(dev);
> +
> +	ret = veml6046x00_validate_part_id(data);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to validate device ID\n");
> +
> +	iio->name = i2c->name;

Prefer this hard coded.  Depending on the firmware type, things like that have
an annoying habbit of not remaining predictable or stable.

> +	iio->channels = veml6046x00_channels;
> +	iio->num_channels = ARRAY_SIZE(veml6046x00_channels);
> +	iio->modes = INDIO_DIRECT_MODE;
> +
> +	iio->info = &veml6046x00_info_no_irq;
> +
> +	ret = veml6046x00_setup_device(iio);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
> +					      veml6046x00_trig_handler,
> +					      &veml6046x00_buffer_setup_ops);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to register triggered buffer");
> +
> +	pm_runtime_mark_last_busy(dev);
> +	pm_runtime_put_autosuspend(dev);
> +
> +	ret = devm_iio_device_register(dev, iio);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register iio device");
> +
> +	return 0;
> +}

Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Andreas Klinger]

[-- Attachment #1: Type: text/plain, Size: 15491 bytes --]

Hi Jonathan,

thanks for the review and comments. Answers are inline below.

Andreas


Jonathan Cameron <jic23@kernel.org> schrieb am Mo, 17. Mär 11:50:
> On Sun, 16 Mar 2025 12:31:30 +0100
> Andreas Klinger <ak@it-klinger.de> wrote:
> 
> > Add Vishay VEML6046X00 high accuracy RGBIR color sensor.
> > 
> > This sensor provides three colour (red, green and blue) as well as one
> > infrared (IR) channel through I2C.
> > 
> > Support direct and buffered mode.
> > 
> > An optional interrupt for signaling green colour threshold underflow or
> > overflow is not supported so far.
> > 
> > Signed-off-by: Andreas Klinger <ak@it-klinger.de>
> 
> Hi Andreas,
> 
> A nice clean driver.
> A few comments and questions inline.
> 
> Jonathan
> 
> > diff --git a/drivers/iio/light/veml6046x00.c b/drivers/iio/light/veml6046x00.c
> > new file mode 100644
> > index 000000000000..8e6232e1ab70
> > --- /dev/null
> > +++ b/drivers/iio/light/veml6046x00.c
> > @@ -0,0 +1,890 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * VEML6046X00 High Accuracy RGBIR Color Sensor
> > + *
> > + * Copyright (c) 2025 Andreas Klinger <ak@it-klinger.de>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/types.h>
> > +#include <linux/units.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger.h>
> 
> So far you aren't providing a trigger so I'm not
> expect to see this header used.  Looking at the datasheet
> I see there is a dataready interrupt, but it doesn't look like
> the device has an autonomous / sequencer type mode, so will always
> need to use another trigger (hrtimer, sysfs or some other hardware
> source).

I'll remove.

> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/triggered_buffer.h>
> 
> > +
> > +struct veml6046x00_scan_buf {
> > +	__le16 chans[4];
> > +
> > +	s64 timestamp __aligned(8);
> aligned_s64 now available as a type to use here.
> 
> > +};
> 
> > +static IIO_DEVICE_ATTR_RO(in_illuminance_period_available, 0);
> > +
> > +static struct attribute *veml6046x00_event_attributes[] = {
> > +	&iio_dev_attr_in_illuminance_period_available.dev_attr.attr,
> I thought we didn't have event support yet?  If not why do we
> have event related attributes?

I started implementing including event support by using the interrupt and the
threshold values which can be programmed on the sensor. It turned out that this
is not working as i expected on my side. Then i removed everything which is
about events, but forgot this here. I'm in the process of clarifying this with
the vendor. The plan is to submit a follow up patch later.

So i'll remove for this turn.

> > +	NULL
> > +};
> > +
> > +static const struct attribute_group veml6046x00_event_attr_group = {
> > +	.attrs = veml6046x00_event_attributes,
> > +};
> 
> 
> > +
> > +static int veml6046x00_set_it(struct iio_dev *iio, int val, int val2)
> > +{
> > +	struct veml6046x00_data *data = iio_priv(iio);
> > +	int ret, new_it;
> > +
> > +	if (val)
> > +		return -EINVAL;
> > +
> > +	switch (val2) {
> > +	case 3125:
> > +		new_it = 0x00;
> > +		break;
> > +	case 6250:
> > +		new_it = 0x01;
> > +		break;
> > +	case 12500:
> > +		new_it = 0x02;
> > +		break;
> > +	case 25000:
> > +		new_it = 0x03;
> > +		break;
> > +	case 50000:
> > +		new_it = 0x04;
> > +		break;
> > +	case 100000:
> > +		new_it = 0x05;
> > +		break;
> > +	case 200000:
> > +		new_it = 0x06;
> > +		break;
> > +	case 400000:
> > +		new_it = 0x07;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = regmap_field_write(data->rf.it, new_it);
> > +	if (ret)
> > +		return ret;
> 
> return regmap_field_write()
> 
> > +
> > +	return 0;
> > +}
> > +
> > +static int veml6046x00_set_scale(struct iio_dev *iio, int val, int val2)
> > +{
> > +	struct veml6046x00_data *data = iio_priv(iio);
> > +	int new_scale;
> > +
> > +	if (val == 0 && val2 == 250000) {
> 
> Maybe a lookup table and a loop?

Yes, would be nicer.

> > +		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_5) |
> > +					VEML6046X00_CONF1_PD_D2;
> > +	} else if (val == 0 && val2 == 330000) {
> > +		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_66) |
> > +					VEML6046X00_CONF1_PD_D2;
> > +	} else if (val == 0 && val2 == 500000) {
> > +		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_5);
> > +	} else if (val == 0 && val2 == 660000) {
> > +		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_0_66);
> > +	} else if (val == 1 && val2 == 0) {
> > +		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_1);
> > +	} else if (val == 2 && val2 == 0) {
> > +		new_scale = FIELD_PREP(VEML6046X00_CONF1_GAIN, VEML6046X00_GAIN_2);
> > +	} else {
> > +		return -EINVAL;
> > +	}
> > +
> > +	return regmap_update_bits(data->regmap, VEML6046X00_REG_CONF1,
> > +				 VEML6046X00_CONF1_GAIN |
> > +				 VEML6046X00_CONF1_PD_D2,
> > +				 new_scale);
> > +}
> > +
> > +static int veml6046x00_get_scale(struct veml6046x00_data *data,
> > +				 int *val, int *val2)
> 
> How is this related to integration time?  I'd normally expect
> to see that read in here somewhere as well as doubling integration
> time tends to double scale.
> 

That's true.

So i'll also need to present different gain values depending on the integration time.

> > +{
> > +	int ret, reg;
> > +
> > +	ret = regmap_read(data->regmap, VEML6046X00_REG_CONF1, &reg);
> > +	if (ret)
> > +		return ret;
> > +
> > +	switch (FIELD_GET(VEML6046X00_CONF1_GAIN, reg)) {
> > +	case 0:
> > +		*val = 1;
> > +		*val2 = 0;
> > +		break;
> > +	case 1:
> > +		*val = 2;
> > +		*val2 = 0;
> > +		break;
> > +	case 2:
> > +		*val = 0;
> > +		*val2 = 660000;
> > +		break;
> > +	case 3:
> > +		*val = 0;
> > +		*val2 = 500000;
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (reg & VEML6046X00_CONF1_PD_D2)
> > +		*val2 /= 2;
> > +
> > +	return IIO_VAL_INT_PLUS_MICRO;
> > +}
> > +
> > +static int veml6046x00_set_mode(struct veml6046x00_data *data, bool state)
> > +{
> > +	return regmap_field_write(data->rf.mode, state);
> as below.
> 
> > +}
> > +
> > +static int veml6046x00_set_trig(struct veml6046x00_data *data, bool state)
> > +{
> > +	return regmap_field_write(data->rf.trig, state);
> 
> I'd argue these two aren't worth bothering with because the
> field naming etc makes a direct call to regmap_field_write() obvious enough.
> 
> > +}
> > +
> > +static int veml6046x00_wait_data_available(struct iio_dev *iio, int usecs)
> 
> Document return value as non obvious.
> 
> > +{
> > +	struct veml6046x00_data *data = iio_priv(iio);
> > +	int ret, reg;
> > +	int cnt = 2;
> > +	int i;
> > +
> > +	for (i = 0; i < cnt; i++) {
> > +		ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, &reg);
> > +		if (ret) {
> > +			dev_err(data->dev,
> > +					"Failed to read interrupt register %d\n", ret);
> > +			return -EIO;
> > +		}
> > +
> > +		if (reg & VEML6046X00_INT_DRDY)
> > +			return 1;
> > +
> > +		if (i < cnt)
> > +			fsleep(usecs);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int veml6046x00_single_read(struct iio_dev *iio,
> > +					enum iio_modifier modifier, int *val)
> > +{
> > +	struct veml6046x00_data *data = iio_priv(iio);
> > +	int addr, it_usec, ret;
> > +	uint8_t reg[2];
> > +
> > +	switch (modifier) {
> > +	case IIO_MOD_LIGHT_RED:
> > +		addr = VEML6046X00_REG_R_L;
> > +	break;
> 
> break indent not matching kernel style. Needs to be one more tab in.
> 
> > +	case IIO_MOD_LIGHT_GREEN:
> > +		addr = VEML6046X00_REG_G_L;
> > +	break;
> > +	case IIO_MOD_LIGHT_BLUE:
> > +		addr = VEML6046X00_REG_B_L;
> > +	break;
> > +	case IIO_MOD_LIGHT_IR:
> > +		addr = VEML6046X00_REG_IR_L;
> > +	break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +	ret = pm_runtime_resume_and_get(data->dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = veml6046x00_get_it_usec(data, &it_usec);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	veml6046x00_set_mode(data, 1);
> Check for errors.
> > +
> > +	veml6046x00_set_trig(data, 1);
> > +
> > +	/* integration time + 10 % to ensure completion */
> > +	fsleep(it_usec + (it_usec / 10));
> > +
> > +	ret = veml6046x00_wait_data_available(iio, it_usec * 10);
> > +	if (ret == 1) {
> > +		dev_dbg(data->dev, "data ready\n");
> > +	} else {
> > +		dev_warn(data->dev, "no data ready ret: %d\n", ret);
> > +		goto no_data;
> > +	}
> > +
> > +	ret = iio_device_claim_direct_mode(iio);
> I'm killing these off slowly.  To save a follow up patch,
> 	if (!iio_device_claim_direct(iio))
> 		return -EBUSY;
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_bulk_read(data->regmap, addr, reg, sizeof(reg));
> > +	iio_device_release_direct_mode(iio);
> 	
> 	iio_device_release_direct(iio);
> 
> If not I'll roll in the change with the many other driver updates
> this entails.
> 
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	pm_runtime_mark_last_busy(data->dev);
> > +	pm_runtime_put_autosuspend(data->dev);
> > +
> > +	*val = reg[1] << 8 | reg[0];
> 
> That's an endian conversion.  Use get_unaligned_le16() I think
> Or read into an __le16 in the first place and you can use
> le16_to_cpu() or similar.
> 
> 
> > +
> > +	return IIO_VAL_INT;
> > +
> > +no_data:
> > +	pm_runtime_mark_last_busy(data->dev);
> > +	pm_runtime_put_autosuspend(data->dev);
> > +
> > +	return -EINVAL;
> > +}
> 
> 
> 
> > +static int veml6046x00_buffer_preenable(struct iio_dev *iio)
> > +{
> > +	struct veml6046x00_data *data = iio_priv(iio);
> > +	struct device *dev = data->dev;
> > +	int ret;
> > +
> > +	ret = veml6046x00_set_mode(data, 0);
> > +	if (ret)
> > +		dev_err(data->dev, "Failed to set mode %d\n", ret);
> 
> If these fail, error out.  We will fail to enter buffered mode, but
> that is probably the correct thing to do if we are having comms
> issues or similar.
> 
> > +
> > +	ret = veml6046x00_set_trig(data, 0);
> > +	if (ret)
> > +		dev_err(data->dev, "Failed to set trigger %d\n", ret);
> > +
> > +	return pm_runtime_resume_and_get(dev);
> > +}
> 
> 
> 
> > +static int veml6046x00_validate_part_id(struct veml6046x00_data *data)
> > +{
> > +	int part_id, ret;
> > +	__le16 reg;
> > +
> > +	ret = regmap_bulk_read(data->regmap, VEML6046X00_REG_ID_L, &reg, sizeof(reg));
> > +	if (ret) {
> > +		dev_info(data->dev, "Failed to read ID\n");
> 
> return dev_err_probe() for this one.
> 
> > +		return -EIO;
> > +	}
> >
> > +static int veml6046x00_setup_device(struct iio_dev *iio)
> > +{
> > +	struct veml6046x00_data *data = iio_priv(iio);
> > +	struct device *dev = data->dev;
> > +	int ret, val;
> > +	__le16 reg16;
> > +	uint8_t reg[2];
> > +
> > +	reg[0] = VEML6046X00_CONF0_AF;
> > +	reg[1] = 0x00;
> > +	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_CONF0, reg, sizeof(reg));
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to set configuration\n");
> > +
> > +	reg16 = cpu_to_le16(0);
> > +	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_THDL_L, &reg16, sizeof(reg16));
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to set low threshold\n");
> > +
> > +	reg16 = cpu_to_le16(U16_MAX);
> > +	ret = regmap_bulk_write(data->regmap, VEML6046X00_REG_THDH_L, &reg16, sizeof(reg16));
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to set high threshold\n");
> > +
> > +	ret = regmap_read(data->regmap, VEML6046X00_REG_INT_H, &val);
> > +	if (ret < 0)
> 
> if (ret) here as well.  Good to be consistent for all regmap calls. None of them
> return > 0 as far as I know.
> 
> 
> > +		return dev_err_probe(dev, ret, "Failed to clear interrupts\n");
> > +
> > +	return 0;
> > +}
> > +
> > +static int veml6046x00_probe(struct i2c_client *i2c)
> > +{
> > +	struct device *dev = &i2c->dev;
> > +	struct veml6046x00_data *data;
> > +	struct iio_dev *iio;
> > +	struct regmap *regmap;
> > +	int ret;
> > +
> > +	regmap = devm_regmap_init_i2c(i2c, &veml6046x00_regmap_config);
> > +	if (IS_ERR(regmap))
> > +		return dev_err_probe(dev, PTR_ERR(regmap),
> > +				     "Failed to set regmap\n");
> > +
> > +	iio = devm_iio_device_alloc(dev, sizeof(*data));
> > +	if (!iio)
> > +		return -ENOMEM;
> > +
> > +	data = iio_priv(iio);
> > +	i2c_set_clientdata(i2c, iio);
> > +	data->dev = dev;
> > +	data->regmap = regmap;
> > +
> > +	ret = veml6046x00_regfield_init(data);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to init regfield\n");
> > +
> > +	ret = devm_regulator_get_enable(dev, "vdd");
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to enable regulator\n");
> > +
> > +	ret = devm_add_action_or_reset(dev, veml6046x00_shutdown_action, data);
> 
> Mostly we want a devm action to match against a specific setup operation.  Here is
> it that the device comes up in non shut down state?  Perhaps a comment to
> make it clear.  Also, how do we know it's in a good state rather than part
> configured by someone else?  I'm not seeing a reset sequence though perhaps
> that effectively happens in setup_device()

In veml6046x00_setup_device() all registers are set up to bring the device in a
known state. This function also switches the device on. I could move the call to
setup_device() up to here and add a comment to make it clear.

> 
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Failed to add shut down action\n");
> > +
> > +	ret = pm_runtime_set_active(dev);
> > +	if (ret < 0)
> > +		return dev_err_probe(dev, ret, "Failed to activate PM runtime\n");
> > +
> > +	ret = devm_pm_runtime_enable(dev);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to enable PM runtime\n");
> > +
> > +	pm_runtime_get_noresume(dev);
> > +	pm_runtime_set_autosuspend_delay(dev, VEML6046X00_AUTOSUSPEND_MS);
> > +	pm_runtime_use_autosuspend(dev);
> > +
> > +	ret = veml6046x00_validate_part_id(data);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to validate device ID\n");
> > +
> > +	iio->name = i2c->name;
> 
> Prefer this hard coded.  Depending on the firmware type, things like that have
> an annoying habbit of not remaining predictable or stable.
> 
> > +	iio->channels = veml6046x00_channels;
> > +	iio->num_channels = ARRAY_SIZE(veml6046x00_channels);
> > +	iio->modes = INDIO_DIRECT_MODE;
> > +
> > +	iio->info = &veml6046x00_info_no_irq;
> > +
> > +	ret = veml6046x00_setup_device(iio);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	ret = devm_iio_triggered_buffer_setup(dev, iio, NULL,
> > +					      veml6046x00_trig_handler,
> > +					      &veml6046x00_buffer_setup_ops);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret,
> > +				     "Failed to register triggered buffer");
> > +
> > +	pm_runtime_mark_last_busy(dev);
> > +	pm_runtime_put_autosuspend(dev);
> > +
> > +	ret = devm_iio_device_register(dev, iio);
> > +	if (ret)
> > +		return dev_err_probe(dev, ret, "Failed to register iio device");
> > +
> > +	return 0;
> > +}
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Jonathan Cameron]

> > > +	data = iio_priv(iio);
> > > +	i2c_set_clientdata(i2c, iio);
> > > +	data->dev = dev;
> > > +	data->regmap = regmap;
> > > +
> > > +	ret = veml6046x00_regfield_init(data);
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "Failed to init regfield\n");
> > > +
> > > +	ret = devm_regulator_get_enable(dev, "vdd");
> > > +	if (ret)
> > > +		return dev_err_probe(dev, ret, "Failed to enable regulator\n");
> > > +
> > > +	ret = devm_add_action_or_reset(dev, veml6046x00_shutdown_action, data);  
> > 
> > Mostly we want a devm action to match against a specific setup operation.  Here is
> > it that the device comes up in non shut down state?  Perhaps a comment to
> > make it clear.  Also, how do we know it's in a good state rather than part
> > configured by someone else?  I'm not seeing a reset sequence though perhaps
> > that effectively happens in setup_device()  
> 
> In veml6046x00_setup_device() all registers are set up to bring the device in a
> known state. This function also switches the device on. I could move the call to
> setup_device() up to here and add a comment to make it clear.

Perfect.

Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Andreas Klinger]

[-- Attachment #1: Type: text/plain, Size: 2184 bytes --]

Hi Jonathan,

I need to pick up the meaning of scale once again for clarification.

Jonathan Cameron <jic23@kernel.org> schrieb am Mo, 17. Mär 11:50:
> On Sun, 16 Mar 2025 12:31:30 +0100
> Andreas Klinger <ak@it-klinger.de> wrote:
> 
> > +static int veml6046x00_get_scale(struct veml6046x00_data *data,
> > +				 int *val, int *val2)
> 
> How is this related to integration time?  I'd normally expect
> to see that read in here somewhere as well as doubling integration
> time tends to double scale.

In the documentation file "sysfs-bus-iio" it says:
"
What:           /sys/.../iio:deviceX/in_illuminanceY_raw
[...]
Description:
                Illuminance measurement, units after application of scale
                                and offset are lux.
"

This means that the scale should be the real factor and not the gain multiplied
by photodiode size (PDDIV) as i implemented it so far.

This means also that doubling integration time should halve the scale. The
higher raw value should lead to the same lux value.

The documentation of the sensor (veml6046x00.pdf) provides us the calculation
between raw green values and lux.
Wouldn't it be better to give the user the real factor to be able to get lux?

The fact that only the green channel can be used for calculation could be
documented in the driver.

Then i found the "in_illuminance_hardwaregain" property. It seems that this is
exactly what the combination of gain and PDDIV is used for.

So what is the scale at the moment could become the hardwaregain and the scale
the factor from raw value to lux.


To sum up the suggested interface under /sys/bus/iio/devices/iio\:deviceX would
be something like:

in_illuminance_hardwaregain --> set and get gain and PDDIV on the sensor

integration_time            --> set and get integration time on the sensor

integration_time_available  --> show available integration time values

scale                       --> (only) get real calculation value, taken from
                                sensor documenation, e.g. 1.3440

scale_available             --> not existing anymore


What do you think?

Andreas


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Jonathan Cameron]

On Sun, 6 Apr 2025 10:43:23 +0200
Andreas Klinger <ak@it-klinger.de> wrote:

> Hi Jonathan,
> 
> I need to pick up the meaning of scale once again for clarification.
> 
> Jonathan Cameron <jic23@kernel.org> schrieb am Mo, 17. Mär 11:50:
> > On Sun, 16 Mar 2025 12:31:30 +0100
> > Andreas Klinger <ak@it-klinger.de> wrote:
> >   
> > > +static int veml6046x00_get_scale(struct veml6046x00_data *data,
> > > +				 int *val, int *val2)  
> > 
> > How is this related to integration time?  I'd normally expect
> > to see that read in here somewhere as well as doubling integration
> > time tends to double scale.  
> 
> In the documentation file "sysfs-bus-iio" it says:
> "
> What:           /sys/.../iio:deviceX/in_illuminanceY_raw
> [...]
> Description:
>                 Illuminance measurement, units after application of scale
>                                 and offset are lux.
> "
> 
> This means that the scale should be the real factor and not the gain multiplied
> by photodiode size (PDDIV) as i implemented it so far.
> 
> This means also that doubling integration time should halve the scale. The
> higher raw value should lead to the same lux value.

Sounds correct.

> 
> The documentation of the sensor (veml6046x00.pdf) provides us the calculation
> between raw green values and lux.
> Wouldn't it be better to give the user the real factor to be able to get lux?

Absolutely.  That's the expectation if we are providing illuminance_raw and
illuminance_scale.

> 
> The fact that only the green channel can be used for calculation could be
> documented in the driver.

Ah. One of these devices.  Hmm. Why do people pretend they can get from
Green to illuminance.  That has to assume 'white light'.
I get grumpy about this, but if it is the best we can do I guess we have
to live with it (I might not be consistent on this).

> 
> Then i found the "in_illuminance_hardwaregain" property. It seems that this is
> exactly what the combination of gain and PDDIV is used for.
> 
> So what is the scale at the moment could become the hardwaregain and the scale
> the factor from raw value to lux.

If it is useful to export it separately that works, however it's not typically
the control attribute - those tend to be read only because, without access to
datasheets simple software has no idea how to control them.

The alternative is the GTS helpers that attempt to figure out the best
way to meet the user requirements in setting the integration time and amplifier
gain when a scale is requested.

+CC Matti who is the expert on those.


> 
> 
> To sum up the suggested interface under /sys/bus/iio/devices/iio\:deviceX would
> be something like:
> 
> in_illuminance_hardwaregain --> set and get gain and PDDIV on the sensor

This is usually the read only one as it reflects things that aren't
easy for a userspace program / user to tune.  They typically want to control
integration time because it reflects noise level and scale because they want
to avoid saturation etc and because we need it to get to the actual value
in lux.

> 
> integration_time            --> set and get integration time on the sensor
driving these directly is fine.
> 
> integration_time_available  --> show available integration time values
> 
> scale                       --> (only) get real calculation value, taken from
>                                 sensor documenation, e.g. 1.3440
This should remain a main control attribute.
> 
> scale_available             --> not existing anymore
This gets tricky but the GTS helpers will calculate it for you I think.

Jonathan

> 
> 
> What do you think?
> 
> Andreas
> 

Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Matti Vaittinen]

On 06/04/2025 14:08, Jonathan Cameron wrote:
> On Sun, 6 Apr 2025 10:43:23 +0200
> Andreas Klinger <ak@it-klinger.de> wrote:
> 
>> Hi Jonathan,
>>
>> I need to pick up the meaning of scale once again for clarification.
>>
>> Jonathan Cameron <jic23@kernel.org> schrieb am Mo, 17. Mär 11:50:
>>> On Sun, 16 Mar 2025 12:31:30 +0100
>>> Andreas Klinger <ak@it-klinger.de> wrote:
>>>    
>>>> +static int veml6046x00_get_scale(struct veml6046x00_data *data,
>>>> +				 int *val, int *val2)
>>>
>>> How is this related to integration time?  I'd normally expect
>>> to see that read in here somewhere as well as doubling integration
>>> time tends to double scale.
>>
>> In the documentation file "sysfs-bus-iio" it says:
>> "
>> What:           /sys/.../iio:deviceX/in_illuminanceY_raw
>> [...]
>> Description:
>>                  Illuminance measurement, units after application of scale
>>                                  and offset are lux.
>> "
>>
>> This means that the scale should be the real factor and not the gain multiplied
>> by photodiode size (PDDIV) as i implemented it so far.
>>
>> This means also that doubling integration time should halve the scale. The
>> higher raw value should lead to the same lux value.
> 
> Sounds correct.

I was CC'd due to the GTS (gain-time-scale)-helpers. The above is the 
beef of those helpers - which, attempt to aid drivers to convert the 
impact of the hardware gain + integration time into a single scale 
value. This approach has some caveats, but the goal is to fulfill the 
expectations of those user-space apps which expect only scale to change 
the gain, while also need to have the integration time controllable (for 
example to reduce the measurement time for one reason or another).

Problem is that, especially when there are multiple channels with 
separate gain control but common integration time, there will be some 
situations where the integration time change _will_ cause changes to 
"total gain (E.g. scale)" too. There may also be cases where some scale 
values can be met only with certain integration times, or where a scale 
for a channel can't be met maintaining the scale for other channels etc.

All in all, I am not sure if the 'unchangeable hardware gain' approach 
makes things as simple as possible - but as long as we want to have it, 
the GTS helpers may be of use :) There are couple of drivers using them 
- feel free to take a look. "git grep gts_ drivers/iio/light/" should 
point you the current users.

>> The documentation of the sensor (veml6046x00.pdf) provides us the calculation
>> between raw green values and lux.
>> Wouldn't it be better to give the user the real factor to be able to get lux?
> 
> Absolutely.  That's the expectation if we are providing illuminance_raw and
> illuminance_scale.
> 
>>
>> The fact that only the green channel can be used for calculation could be
>> documented in the driver.
> 
> Ah. One of these devices.  Hmm. Why do people pretend they can get from
> Green to illuminance.  That has to assume 'white light'.
> I get grumpy about this, but if it is the best we can do I guess we have
> to live with it (I might not be consistent on this).
> 
>>
>> Then i found the "in_illuminance_hardwaregain" property. It seems that this is
>> exactly what the combination of gain and PDDIV is used for.
>>
>> So what is the scale at the moment could become the hardwaregain and the scale
>> the factor from raw value to lux.
> 
> If it is useful to export it separately that works, however it's not typically
> the control attribute - those tend to be read only because, without access to
> datasheets simple software has no idea how to control them.
> 
> The alternative is the GTS helpers that attempt to figure out the best
> way to meet the user requirements in setting the integration time and amplifier
> gain when a scale is requested.
> 
> +CC Matti who is the expert on those.

Seems it doesn't take much to be an expert XD

I have understood that the simplest way to go is to always use the 
maximum integration time which provides the required scale. This should 
probably result the most accurate readings. If users for some reason 
want to explicitly set a shorter time, then the GTS helpers might be useful.

>> To sum up the suggested interface under /sys/bus/iio/devices/iio\:deviceX would
>> be something like:
>>
>> in_illuminance_hardwaregain --> set and get gain and PDDIV on the sensor
> 
> This is usually the read only one as it reflects things that aren't
> easy for a userspace program / user to tune.  They typically want to control
> integration time because it reflects noise level and scale because they want
> to avoid saturation etc and because we need it to get to the actual value
> in lux.
> 
>>
>> integration_time            --> set and get integration time on the sensor
> driving these directly is fine.
>>
>> integration_time_available  --> show available integration time values
>>
>> scale                       --> (only) get real calculation value, taken from
>>                                  sensor documenation, e.g. 1.3440
> This should remain a main control attribute.
>>
>> scale_available             --> not existing anymore
> This gets tricky but the GTS helpers will calculate it for you I think.

The GTS helpers can (or, at least should be able to) calculate the 
available_scales. AFAIR they offer two approaches for this. First one is 
listing _all_ scales which can be achieved by changing both the 
integration time and the hardware gain. The second one lists only the 
scales which can be set with the currently selected integration time.

I think that all of the current GTS users use this first approach of 
listing all the values, so the 'per time tables' - approach is not very 
thoroughly tested. Please, let me know if you hit to any hiccups.

Yours,
	-- Matti

Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Andreas Klinger]

[-- Attachment #1: Type: text/plain, Size: 3419 bytes --]

Hi Jonathan,
hi Matti,

Matti Vaittinen <mazziesaccount@gmail.com> schrieb am Mo, 07. Apr 08:52:
> On 06/04/2025 14:08, Jonathan Cameron wrote:
> > On Sun, 6 Apr 2025 10:43:23 +0200
> > Andreas Klinger <ak@it-klinger.de> wrote:
> > 
> > > Hi Jonathan,
> > > 
> > > I need to pick up the meaning of scale once again for clarification.
> > > 
> > > Jonathan Cameron <jic23@kernel.org> schrieb am Mo, 17. Mär 11:50:
> > > > On Sun, 16 Mar 2025 12:31:30 +0100
> > > > Andreas Klinger <ak@it-klinger.de> wrote:
> > > > > +static int veml6046x00_get_scale(struct veml6046x00_data *data,
> > > > > +				 int *val, int *val2)
> > > > 
> > > > How is this related to integration time?  I'd normally expect
> > > > to see that read in here somewhere as well as doubling integration
> > > > time tends to double scale.
> > > 
> > > In the documentation file "sysfs-bus-iio" it says:
> > > "
> > > What:           /sys/.../iio:deviceX/in_illuminanceY_raw
> > > [...]
> > > Description:
> > >                  Illuminance measurement, units after application of scale
> > >                                  and offset are lux.
> > > "
> > > 
> > > This means that the scale should be the real factor and not the gain multiplied
> > > by photodiode size (PDDIV) as i implemented it so far.
> > > 
> > > This means also that doubling integration time should halve the scale. The
> > > higher raw value should lead to the same lux value.
> > 
> > Sounds correct.
> 
> I was CC'd due to the GTS (gain-time-scale)-helpers. The above is the beef
> of those helpers - which, attempt to aid drivers to convert the impact of
> the hardware gain + integration time into a single scale value. This
> approach has some caveats, but the goal is to fulfill the expectations of
> those user-space apps which expect only scale to change the gain, while also
> need to have the integration time controllable (for example to reduce the
> measurement time for one reason or another).
> 
> Problem is that, especially when there are multiple channels with separate
> gain control but common integration time, there will be some situations
> where the integration time change _will_ cause changes to "total gain (E.g.
> scale)" too. There may also be cases where some scale values can be met only
> with certain integration times, or where a scale for a channel can't be met
> maintaining the scale for other channels etc.
> 
> All in all, I am not sure if the 'unchangeable hardware gain' approach makes
> things as simple as possible - but as long as we want to have it, the GTS
> helpers may be of use :) There are couple of drivers using them - feel free
> to take a look. "git grep gts_ drivers/iio/light/" should point you the
> current users.
> 

Thanks a lot for illustrating and explaining the GTS. I implemented the driver
with GTS and by this learned a lot about it. But at the end i found it in my
case to be simpler to implement it without GTS for some reasons:
- User wants to be able to set up the integration time as well as scale and the
  driver should not optimize it somehow.
- There is not only a relation from the scale to the gain of the sensor but also
  to the photodiode size. Because of this i need another helper table asize of
  GTS for translating the scale into sensor gain and photodiode size.

I'll come up with a version 3 shortly.

Andreas



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Matti Vaittinen]

On 05/05/2025 22:10, Andreas Klinger wrote:
>>
>> I was CC'd due to the GTS (gain-time-scale)-helpers. The above is the beef
>> of those helpers - which, attempt to aid drivers to convert the impact of
>> the hardware gain + integration time into a single scale value. This
>> approach has some caveats, but the goal is to fulfill the expectations of
>> those user-space apps which expect only scale to change the gain, while also
>> need to have the integration time controllable (for example to reduce the
>> measurement time for one reason or another).
>>
>> Problem is that, especially when there are multiple channels with separate
>> gain control but common integration time, there will be some situations
>> where the integration time change _will_ cause changes to "total gain (E.g.
>> scale)" too. There may also be cases where some scale values can be met only
>> with certain integration times, or where a scale for a channel can't be met
>> maintaining the scale for other channels etc.
>>
>> All in all, I am not sure if the 'unchangeable hardware gain' approach makes
>> things as simple as possible - but as long as we want to have it, the GTS
>> helpers may be of use :) There are couple of drivers using them - feel free
>> to take a look. "git grep gts_ drivers/iio/light/" should point you the
>> current users.
>>
> 
> Thanks a lot for illustrating and explaining the GTS. I implemented the driver
> with GTS and by this learned a lot about it. But at the end i found it in my
> case to be simpler to implement it without GTS for some reasons:

Firstly, it's perfectly Ok to choose to not to use the GTS helpers. Yet, 
I would like to understand couple of things.

> - User wants to be able to set up the integration time as well as scale and the
>    driver should not optimize it somehow.

How does using GTS to do the conversion from
HWGAIN + time => scale
cause optimization? Or, do you just mean that part of the functionality 
provided by these helpers wouldn't be applicable, while the conversion 
could still be done?

> - There is not only a relation from the scale to the gain of the sensor but also
>    to the photodiode size. Because of this i need another helper table asize of
>    GTS for translating the scale into sensor gain and photodiode size.

I suppose that using the GTS with gains and times without PD, and then 
computing the impact of the PD size after the GTS conversions would have 
caused the available scales to be wrong, right?

Couldn't you still have used two different set of gain tables (one for 
each PD size) if you chose to use the GTS?

> I'll come up with a version 3 shortly.

I took a very quick look at the v3 - not one worth a reviewed-by :) And, 
as I wrote, I have nothing against skipping the GTS. I'm not sure the 
values in the multi-dimensional array were clear to me at a glance - but 
I assume it's lean and efficient when one wraps his head around it. So, 
I'm not trying to argue you should've used GTS - I just want to 
understand what was the exact issue with it :)


Yours,
	-- Matti

Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Andreas Klinger]

[-- Attachment #1: Type: text/plain, Size: 4910 bytes --]

Hi Matti,

Matti Vaittinen <mazziesaccount@gmail.com> schrieb am Di, 06. Mai 09:03:
> On 05/05/2025 22:10, Andreas Klinger wrote:
> > > 
> > > I was CC'd due to the GTS (gain-time-scale)-helpers. The above is the beef
> > > of those helpers - which, attempt to aid drivers to convert the impact of
> > > the hardware gain + integration time into a single scale value. This
> > > approach has some caveats, but the goal is to fulfill the expectations of
> > > those user-space apps which expect only scale to change the gain, while also
> > > need to have the integration time controllable (for example to reduce the
> > > measurement time for one reason or another).
> > > 
> > > Problem is that, especially when there are multiple channels with separate
> > > gain control but common integration time, there will be some situations
> > > where the integration time change _will_ cause changes to "total gain (E.g.
> > > scale)" too. There may also be cases where some scale values can be met only
> > > with certain integration times, or where a scale for a channel can't be met
> > > maintaining the scale for other channels etc.
> > > 
> > > All in all, I am not sure if the 'unchangeable hardware gain' approach makes
> > > things as simple as possible - but as long as we want to have it, the GTS
> > > helpers may be of use :) There are couple of drivers using them - feel free
> > > to take a look. "git grep gts_ drivers/iio/light/" should point you the
> > > current users.
> > > 
> > 
> > Thanks a lot for illustrating and explaining the GTS. I implemented the driver
> > with GTS and by this learned a lot about it. But at the end i found it in my
> > case to be simpler to implement it without GTS for some reasons:
> 
> Firstly, it's perfectly Ok to choose to not to use the GTS helpers. Yet, I
> would like to understand couple of things.
> 
> > - User wants to be able to set up the integration time as well as scale and the
> >    driver should not optimize it somehow.
> 
> How does using GTS to do the conversion from
> HWGAIN + time => scale
> cause optimization? Or, do you just mean that part of the functionality
> provided by these helpers wouldn't be applicable, while the conversion could
> still be done?

This functionality is not applicable. The user wants to set up the time and gain
manually. There are applications in which the driver should be fast with a lower
integration time and others in which the best accurate result is required. As
i'm developing it for the sensor vendor they want to be able to demonstrate
all the different settings to their customers.

> > - There is not only a relation from the scale to the gain of the sensor but also
> >    to the photodiode size. Because of this i need another helper table asize of
> >    GTS for translating the scale into sensor gain and photodiode size.
> 
> I suppose that using the GTS with gains and times without PD, and then
> computing the impact of the PD size after the GTS conversions would have
> caused the available scales to be wrong, right?

Exactly this caused me most of the headache. With GTS i need to use a "virtual"
scale for the calculation. When user asks for available scales or set up the
desired scale i need to convert it to the real scale and then further translate
it to the register values of gain and PD.

> Couldn't you still have used two different set of gain tables (one for each
> PD size) if you chose to use the GTS?

Of course there are solutions for it and i was about to finish one with GTS as i
saw that the driver will get simpler without. As i cannot use the nice features
of GTS it turns out to use only the data structures and functions on it. But the
data structures don't fit exactly to this sensor as we have gain and PD in two
different regfields. So another table to translate the scale to the regfields is
needed anyway.

> > I'll come up with a version 3 shortly.
> 
> I took a very quick look at the v3 - not one worth a reviewed-by :) And, as
> I wrote, I have nothing against skipping the GTS. I'm not sure the values in
> the multi-dimensional array were clear to me at a glance - but I assume it's
> lean and efficient when one wraps his head around it. 

The large table contains exactly the values from the datasheet for the
conversion of raw measured counts to lux. I only combined the two tables for PD
1/2 and 2/2 as hwgain x0.5 and x1 are existing twice.

> So, I'm not trying to argue you should've used GTS - I just want to understand
> what was the exact issue with it :)

There is not an issue with GTS. In my opinion it's not the simplest solution for
this sensor in this case. I'm quite sure there are other cases in which it makes
a lot of sense to use it and as i'm now a little bit familiar with it i'll be
using it if appropriate.

Best regards,

Andreas


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH 2/3] iio: light: add support for veml6046x00 RGBIR color sensor

[Matti Vaittinen]

On 06/05/2025 11:28, Andreas Klinger wrote:
> Hi Matti,
> 
> Matti Vaittinen <mazziesaccount@gmail.com> schrieb am Di, 06. Mai 09:03:
>> On 05/05/2025 22:10, Andreas Klinger wrote:

>> Couldn't you still have used two different set of gain tables (one for each
>> PD size) if you chose to use the GTS?
> 
> Of course there are solutions for it and i was about to finish one with GTS as i
> saw that the driver will get simpler without. As i cannot use the nice features
> of GTS it turns out to use only the data structures and functions on it. But the
> data structures don't fit exactly to this sensor as we have gain and PD in two
> different regfields. So another table to translate the scale to the regfields is
> needed anyway.

Ah. Understood. Thanks for taking the time to explain :)

Yours,
	-- Matti

[PATCH 3/3] MAINTAINER: add maintainer for veml6046x00

[Andreas Klinger]

Add maintainer for Vishay veml6046x00 RGBIR color sensor driver and dt
binding.

Signed-off-by: Andreas Klinger <ak@it-klinger.de>
---
 MAINTAINERS | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index c9763412a508..5dc2ada88f2c 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -25281,6 +25281,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/vishay,veml6030.yaml
 F:	drivers/iio/light/veml6030.c
 
+VISHAY VEML6046X00 RGBIR COLOR SENSOR DRIVER
+M:	Andreas Klinger <ak@it-klinger.de>
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/light/vishay,veml6046x00.yaml
+F:	drivers/iio/light/veml6046x00.c
+
 VISHAY VEML6075 UVA AND UVB LIGHT SENSOR DRIVER
 M:	Javier Carrasco <javier.carrasco.cruz@gmail.com>
 S:	Maintained
-- 
2.39.5