[PATCH v4 0/2] Add support for AS7343 multi-spectral sensor

[PATCH v4 0/2] Add support for AS7343 multi-spectral sensor

[Chang Yu]

This series adds initial support for the AMS AS7343 14-channel
multi-spectral sensor.

The sensor has 14 optical channels (11 visible + NIR + clear +
flicker) in total. This initial driver supports reading raw counts
from all 12 spectral channels (11 visible + NIR) via sysfs and basic
power management.

Not yet supported:
- Configurable integration time and gain
- Interrupt support
- Clear (VIS) channel and flicker detection

Patch 1 adds device tree bindings.
Patch 2 adds the driver.

Testing was performed on a Raspberry Pi 4.

Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf

Changes in v4:
  - Fix led-max-microamp property in dt bindings.
  - Use REGCACHE_MAPLE and add volatile table.
  - Use PM_RUNTIME_ACQUIRE* when resuming.

Changes in v3:
  - Fix gpio description and example in dt.
  - Only start measurements after suspend action has been registered.
  - Add a mutex and pm_runtime_resume_and_get before reading measurements.

Changes in v2:
  - Add the LDR, the interrupt pin, and the GPIO pin to the dt bindings.
  - Adjust the default gain to x256 and integration time to 50.1ms as
  recommended by the datasheet.
  - Use read_label instead of .extend
  - Remove all unused scan_index related constants.

Chang Yu (2):
  dt-bindings: iio: light: add as7343
  iio: light: add AS7343 multi-spectral sensor driver

 .../bindings/iio/light/ams,as7343.yaml        |  72 +++
 MAINTAINERS                                   |   7 +
 drivers/iio/light/Kconfig                     |  11 +
 drivers/iio/light/Makefile                    |   1 +
 drivers/iio/light/as7343.c                    | 434 ++++++++++++++++++
 5 files changed, 525 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
 create mode 100644 drivers/iio/light/as7343.c

-- 
2.55.0

[PATCH v4 1/2] dt-bindings: iio: light: add as7343

[Chang Yu]

Add binding for AMS AS7343 which is a 14-channel multi-spectral sensor
with i2c address of 0x39.

Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
---
Changes in v4:
 - Fix led-max-microamp property name.

Changes in v3:
 - Add missing include for gpio.
 - Expand GPIO descrption to include hardware synchronization.
 - Use led-max-microamp instead of led-current-microamp for the LDR pin.

Changes in v2:
 - Add the LDR, the interrupt pin, and the GPIO pin to the bindings.
 - Fix node name and unit address mismatch.
 - Include MAINTAINERS changes.

 .../bindings/iio/light/ams,as7343.yaml        | 72 +++++++++++++++++++
 MAINTAINERS                                   |  6 ++
 2 files changed, 78 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/ams,as7343.yaml

diff --git a/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml b/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
new file mode 100644
index 000000000000..d963832fb413
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
@@ -0,0 +1,72 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/light/ams,as7343.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: AMS AS7343 14-Channel Multi-Spectral Sensor
+
+maintainers:
+  - Chang Yu <marcus.yu.56@gmail.com>
+
+description: |
+  The AMS AS7343 is a 14-channel multi-spectral sensor with i2c address of 0x39.
+  https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
+
+properties:
+  compatible:
+    enum:
+      - ams,as7343
+
+  reg:
+    description:
+      I2C address of the device (0x39).
+    maxItems: 1
+
+  interrupts:
+    description:
+      Open drain output active low interrupt pin.
+    maxItems: 1
+
+  vdd-supply: true
+
+  led-max-microamp:
+    description:
+      The driver current for the external LED connected to the LDR pin.
+    minimum: 4000
+    maximum: 258000
+    multipleOf: 2000
+    default: 12000
+
+  sync-gpios:
+    description: |
+      Optional GPIO pin that can be used either as a synchronization input to
+      start/stop measurement or as an external start/stop signal for
+      synchronizing with the connected LED.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - vdd-supply
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        multispectral-sensor@39 {
+            compatible = "ams,as7343";
+            reg = <0x39>;
+            interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
+            vdd-supply = <&vdd_regulator>;
+            led-max-microamp = <14000>;
+            sync-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
+        };
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index 04fa5322d9f7..8cf4e1635053 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1430,6 +1430,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/ams,as73211.yaml
 F:	drivers/iio/light/as73211.c
 
+AMS AS7343 DRIVER
+M:	Chang Yu <marcus.yu.56@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
+
 AMT (Automatic Multicast Tunneling)
 M:	Taehee Yoo <ap420073@gmail.com>
 L:	netdev@vger.kernel.org
-- 
2.55.0

[PATCH v4 2/2] iio: light: add AS7343 multi-spectral sensor driver

[Chang Yu]

Add a driver for the AMS AS7343 14-channel multi-spectral sensor.

The AS7343 is a 14-channel spectral sensor featuring 11 visible
channels, 1 near-infrared channel, 1 clear channel (VIS), and 1
flicker detection channel.

The driver exposes 12 spectral channels (11 visible light and 1
near-infrared) via sysfs. Runtime PM is implemented to stop
measurements when the device is suspended or torn down. Power is
never cut (PON=1 always) to preserve register values.

Future patches will add configurable gain and integration time,
interrupt support, buffered reads, VIS channel, and flicker
detection.

Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
---
Changes in v4:
 - Use REGCACHE_MAPLE and add volatile register table.
 - Use PM_RUNTIME_ACQUIRE* when resuming.
 - Use a static array for labels.
 - Various code style fixes.

Changes in v3:
 - Hold a mutex before reading measurements.
 - Add pm_runtime_resume_and_get before reading measurements.
 - Only start measurements after suspend action has been successfully
   registered
 - Check the return value of regmap_read when verifying device ID.

Changes in v2:
 - Adjust the default gain to x256 and integration time to 50.1ms as
 recommended by the datasheet.
 - Use read_label instead of .extend
 - Remove all unused scan_index related constants.
 - Add missing calls to pm_runtime_* methods in probe.
 - Fix various code style issues

 MAINTAINERS                |   1 +
 drivers/iio/light/Kconfig  |  11 +
 drivers/iio/light/Makefile |   1 +
 drivers/iio/light/as7343.c | 434 +++++++++++++++++++++++++++++++++++++
 4 files changed, 447 insertions(+)
 create mode 100644 drivers/iio/light/as7343.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8cf4e1635053..236102b3c28e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1435,6 +1435,7 @@ M:	Chang Yu <marcus.yu.56@gmail.com>
 L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
+F:	drivers/iio/light/as7343.c
 
 AMT (Automatic Multicast Tunneling)
 M:	Taehee Yoo <ap420073@gmail.com>
diff --git a/drivers/iio/light/Kconfig b/drivers/iio/light/Kconfig
index ef36824f312f..edbeeba9d873 100644
--- a/drivers/iio/light/Kconfig
+++ b/drivers/iio/light/Kconfig
@@ -149,6 +149,17 @@ config AS73211
 	 This driver can also be built as a module.  If so, the module
 	 will be called as73211.
 
+config AS7343
+	tristate "AMS AS7343 14-Channel Multi-Spectral Sensor"
+	depends on I2C
+	select REGMAP_I2C
+	help
+	 Say Y here to build support for the AMS AS7343 14-channel
+	 multi-spectral sensor.
+
+	 To compile this driver as a module, choose M here: the module will
+	 be called as7343.
+
 config BH1745
 	tristate "ROHM BH1745 colour sensor"
 	depends on I2C
diff --git a/drivers/iio/light/Makefile b/drivers/iio/light/Makefile
index 64e354c49ed8..5d0d33802519 100644
--- a/drivers/iio/light/Makefile
+++ b/drivers/iio/light/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_APDS9306)		+= apds9306.o
 obj-$(CONFIG_APDS9960)		+= apds9960.o
 obj-$(CONFIG_APDS9999)		+= apds9999.o
 obj-$(CONFIG_AS73211)		+= as73211.o
+obj-$(CONFIG_AS7343)		+= as7343.o
 obj-$(CONFIG_BH1745)		+= bh1745.o
 obj-$(CONFIG_BH1750)		+= bh1750.o
 obj-$(CONFIG_BH1780)		+= bh1780.o
diff --git a/drivers/iio/light/as7343.c b/drivers/iio/light/as7343.c
new file mode 100644
index 000000000000..9ea6a44c35b9
--- /dev/null
+++ b/drivers/iio/light/as7343.c
@@ -0,0 +1,434 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Support for AMS AS7343 14-channel multi-spectral sensor.
+ * (7-bit I2C slave address 0x39)
+ *
+ * Based on the work of:
+ *   Christian Eggers <ceggers@arri.de> (AS73211 driver)
+ *
+ * Copyright (c) 2026 Chang Yu <marcus.yu.56@gmail.com>
+ *
+ * Datasheets:
+ *   https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
+ *
+ * TODO:
+ *   - Autosuspend
+ *   - Support for configurable gain and integration time
+ *   - Interrupt support
+ *   - Add support for reading the VIS channel
+ *   - Flicker detection
+ */
+
+#include <linux/array_size.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/cleanup.h>
+#include <linux/delay.h>
+#include <linux/dev_printk.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/sysfs.h>
+#include <linux/types.h>
+
+#include <asm/byteorder.h>
+
+#include <linux/iio/iio.h>
+
+#define AS7343_ID				0x5a
+
+/* AS7343 config registers */
+#define AS7343_ENABLE				0x80
+#define AS7343_ENABLE_PON			BIT(0)
+#define AS7343_ENABLE_SP_EN			BIT(1)
+
+/*
+ * Integration time is calculated as (ATIME + 1) * ((ASTEP + 1) * 2.78us).
+ * Setting a 30 * 1.67ms = 50.1ms integration time as the default for now.
+ */
+#define AS7343_ATIME				0x81
+#define AS7343_ATIME_VAL			29 /* (29 + 1) = 30 steps */
+#define AS7343_ASTEP				0xd4
+#define AS7343_ASTEP_VAL			599 /* 1.67ms step size */
+
+#define AS7343_CFG0				0xbf
+#define AS7343_CFG0_REG_BANK			BIT(4)
+
+#define AS7343_CFG1				0xc6
+#define AS7343_CFG1_AGAIN			GENMASK(4, 0)
+#define AS7343_CFG1_AGAIN_X0_5			0
+#define AS7343_CFG1_AGAIN_X1			1
+#define AS7343_CFG1_AGAIN_X2			2
+#define AS7343_CFG1_AGAIN_X4			3
+#define AS7343_CFG1_AGAIN_X8			4
+#define AS7343_CFG1_AGAIN_X16			5
+#define AS7343_CFG1_AGAIN_X32			6
+#define AS7343_CFG1_AGAIN_X64			7
+#define AS7343_CFG1_AGAIN_X128			8
+#define AS7343_CFG1_AGAIN_X256			9
+#define AS7343_CFG1_AGAIN_X512			10
+#define AS7343_CFG1_AGAIN_X1024			11
+#define AS7343_CFG1_AGAIN_X2048			12
+
+#define AS7343_CFG20				0xd6
+#define AS7343_CFG20_AUTO_SMUX			GENMASK(6, 5)
+#define AS7343_CFG20_AUTO_SMUX_READOUT_ALL	3 /* all-channel readout */
+
+#define AS7343_CONTROL				0xfa
+
+/* AS7343 status registers */
+#define AS7343_STATUS2				0x90
+#define AS7343_STATUS3				0x91
+#define AS7343_STATUS				0x93
+#define AS7343_ASTATUS				0x94
+#define AS7343_STATUS5				0xbb
+#define AS7343_STATUS4				0xbc
+#define AS7343_FD_STATUS			0xe3
+
+/* AS7343 spectral data registers */
+#define AS7343_DATA_FZ		0x95
+#define AS7343_DATA_FY		0x97
+#define AS7343_DATA_FXL		0x99
+#define AS7343_DATA_NIR		0x9b
+#define AS7343_DATA_F2		0xa1
+#define AS7343_DATA_F3		0xa3
+#define AS7343_DATA_F4		0xa5
+#define AS7343_DATA_F6		0xa7
+#define AS7343_DATA_F1		0xad
+#define AS7343_DATA_F7		0xaf
+#define AS7343_DATA_F8		0xb1
+#define AS7343_DATA_F5		0xb3
+#define AS7343_DATA_FD_L	0xb7
+#define AS7343_DATA_FD_H	0xb8
+
+/* AS7343 FIFO buffer data registers */
+#define AS7343_FIFO_LVL		0xfd
+#define AS7343_FDATA_L		0xfe
+#define AS7343_FDATA_H		0xff
+
+/* AS7343 channel indices. MUST match data register order above. */
+#define AS7343_CHAN_IDX_FZ	0
+#define AS7343_CHAN_IDX_FY	1
+#define AS7343_CHAN_IDX_FXL	2
+#define AS7343_CHAN_IDX_NIR	3
+#define AS7343_CHAN_IDX_F2	4
+#define AS7343_CHAN_IDX_F3	5
+#define AS7343_CHAN_IDX_F4	6
+#define AS7343_CHAN_IDX_F6	7
+#define AS7343_CHAN_IDX_F1	8
+#define AS7343_CHAN_IDX_F7	9
+#define AS7343_CHAN_IDX_F8	10
+#define AS7343_CHAN_IDX_F5	11
+
+#define AS7343_CHAN(_chan)					\
+	{							\
+		.type = IIO_INTENSITY,				\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+		.address = AS7343_DATA_##_chan,			\
+		.indexed = 1,					\
+		.channel = AS7343_CHAN_IDX_##_chan,		\
+	}
+
+static const struct iio_chan_spec as7343_channels[] = {
+	AS7343_CHAN(FZ), AS7343_CHAN(FY), AS7343_CHAN(FXL), AS7343_CHAN(NIR),
+	AS7343_CHAN(F2), AS7343_CHAN(F3), AS7343_CHAN(F4),  AS7343_CHAN(F6),
+	AS7343_CHAN(F1), AS7343_CHAN(F7), AS7343_CHAN(F8),  AS7343_CHAN(F5),
+};
+
+struct as7343_data {
+	struct regmap *regmap;
+	/* Ensures reads don't stomp on each other */
+	struct mutex mutex;
+};
+
+static int as7343_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct as7343_data *data = iio_priv(indio_dev);
+	unsigned int unused;
+	struct regmap *map;
+	struct device *dev;
+	__le16 result;
+	int ret;
+
+	map = data->regmap;
+	dev = regmap_get_device(map);
+
+	PM_RUNTIME_ACQUIRE_IF_ENABLED(dev, pm);
+	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
+	if (ret)
+		return ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW: {
+		/* Wait until integration time passes for all 3 cycles. */
+		msleep(160);
+
+		/*
+		 * Reading ASTATUS latches all data registers to this read.
+		 * We don't care about the returned saturation/gain status for
+		 * now.
+		 */
+		guard(mutex)(&data->mutex);
+		ret = regmap_read(map, AS7343_ASTATUS, &unused);
+		if (ret)
+			return ret;
+
+		ret = regmap_bulk_read(map, chan->address,
+				       &result, sizeof(result));
+		if (ret)
+			return ret;
+
+		*val = le16_to_cpu(result);
+		return IIO_VAL_INT;
+	}
+
+	default:
+		return -EINVAL;
+	}
+}
+
+/*
+ * Channel names and wavelength ranges as defined in the datasheet
+ * (Figure 7, "AS7343 Optical Channel Summary"). Values are the
+ * minimum and maximum peak wavelength in nanometers.
+ *
+ * F1:  395-415 nm
+ * F2:  415-435 nm
+ * FZ:  440-460 nm
+ * F3:  465-485 nm
+ * F4:  505-525 nm
+ * FY:  545-565 nm
+ * F5:  540-560 nm
+ * FXL: 590-610 nm
+ * F6:  630-650 nm
+ * F7:  680-700 nm
+ * F8:  735-755 nm
+ * NIR: 845-865 nm
+ */
+static const char * const as7343_channel_labels[] = {
+	[AS7343_CHAN_IDX_F1] = "F1",
+	[AS7343_CHAN_IDX_F2] = "F2",
+	[AS7343_CHAN_IDX_FZ] = "FZ",
+	[AS7343_CHAN_IDX_F3] = "F3",
+	[AS7343_CHAN_IDX_F4] = "F4",
+	[AS7343_CHAN_IDX_FY] = "FY",
+	[AS7343_CHAN_IDX_F5] = "F5",
+	[AS7343_CHAN_IDX_FXL] = "FXL",
+	[AS7343_CHAN_IDX_F6] = "F6",
+	[AS7343_CHAN_IDX_F7] = "F7",
+	[AS7343_CHAN_IDX_F8] = "F8",
+	[AS7343_CHAN_IDX_NIR] = "NIR",
+};
+
+static int as7343_read_label(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, char *label)
+{
+	int channel = chan->channel;
+
+	if (channel >= ARRAY_SIZE(as7343_channel_labels))
+		return -EINVAL;
+
+	return sysfs_emit(label, "%s\n", as7343_channel_labels[channel]);
+}
+
+static const struct iio_info as7343_info = {
+	.read_raw = as7343_read_raw,
+	.read_label = as7343_read_label,
+};
+
+static const struct regmap_range as7343_volatile_ranges[] = {
+	regmap_reg_range(AS7343_ENABLE, AS7343_ENABLE),
+	regmap_reg_range(AS7343_STATUS2, AS7343_DATA_FD_H),
+	regmap_reg_range(AS7343_STATUS5, AS7343_STATUS4),
+	regmap_reg_range(AS7343_FD_STATUS, AS7343_FD_STATUS),
+	regmap_reg_range(AS7343_CONTROL, AS7343_CONTROL),
+	regmap_reg_range(AS7343_FIFO_LVL, AS7343_FDATA_H),
+};
+
+static const struct regmap_access_table as7343_volatile_table = {
+	.yes_ranges = as7343_volatile_ranges,
+	.n_yes_ranges = ARRAY_SIZE(as7343_volatile_ranges),
+};
+
+static const struct regmap_config as7343_regmap_config = {
+	.name = "as7343",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AS7343_FDATA_H,
+	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.cache_type = REGCACHE_MAPLE,
+	.volatile_table = &as7343_volatile_table,
+};
+
+static int as7343_setup_device(struct device *dev, struct as7343_data *data)
+{
+	struct regmap *map = data->regmap;
+	unsigned int val;
+	__le16 step;
+	int ret;
+
+	/* Power on */
+	ret = regmap_set_bits(map, AS7343_ENABLE, AS7343_ENABLE_PON);
+	if (ret)
+		return ret;
+
+	/* Need to set REG_BANK to 1 before we can access ID */
+	ret = regmap_set_bits(map, AS7343_CFG0, AS7343_CFG0_REG_BANK);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(map, AS7343_ID, &val);
+	if (ret)
+		return ret;
+
+	if (val != 0x81)
+		dev_info(dev, "Unknown device ID: %x\n", val);
+
+	ret = regmap_clear_bits(map, AS7343_CFG0, AS7343_CFG0_REG_BANK);
+	if (ret)
+		return ret;
+
+	/* Configure the SMUX to readout all channels */
+	ret = regmap_update_bits(map, AS7343_CFG20, AS7343_CFG20_AUTO_SMUX,
+				 FIELD_PREP(AS7343_CFG20_AUTO_SMUX,
+					    AS7343_CFG20_AUTO_SMUX_READOUT_ALL));
+	if (ret)
+		return ret;
+
+	/* Set 50.1ms integration time and x256 gain for now */
+	step = cpu_to_le16(AS7343_ASTEP_VAL);
+	ret = regmap_bulk_write(map, AS7343_ASTEP, &step, sizeof(step));
+	if (ret)
+		return ret;
+
+	ret = regmap_write(map, AS7343_ATIME, AS7343_ATIME_VAL);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(map, AS7343_CFG1, AS7343_CFG1_AGAIN,
+				  FIELD_PREP(AS7343_CFG1_AGAIN,
+					     AS7343_CFG1_AGAIN_X256));
+}
+
+static int as7343_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct as7343_data *data = iio_priv(indio_dev);
+	struct regmap *map = data->regmap;
+
+	return regmap_clear_bits(map, AS7343_ENABLE, AS7343_ENABLE_SP_EN);
+}
+
+static int as7343_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct as7343_data *data = iio_priv(indio_dev);
+	struct regmap *map = data->regmap;
+
+	return regmap_set_bits(map, AS7343_ENABLE, AS7343_ENABLE_SP_EN);
+}
+
+static void as7343_suspend_action(void *data)
+{
+	struct device *dev = data;
+
+	as7343_suspend(dev);
+}
+
+static int as7343_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct iio_dev *indio_dev;
+	struct as7343_data *data;
+	struct regmap *regmap;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, indio_dev);
+
+	regmap = devm_regmap_init_i2c(client, &as7343_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	data = iio_priv(indio_dev);
+	data->regmap = regmap;
+
+	ret = devm_mutex_init(dev, &data->mutex);
+	if (ret)
+		return ret;
+
+	indio_dev->name = "as7343";
+	indio_dev->info = &as7343_info;
+	indio_dev->channels = as7343_channels;
+	indio_dev->num_channels = ARRAY_SIZE(as7343_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = devm_regulator_get_enable(dev, "vdd");
+	if (ret)
+		return ret;
+
+	ret = as7343_setup_device(dev, data);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, as7343_suspend_action, dev);
+	if (ret)
+		return dev_err_probe(dev, ret,
+				     "Failed to add suspend action\n");
+
+	ret = pm_runtime_set_active(dev);
+	if (ret)
+		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");
+
+	/* Start measurements */
+	ret = regmap_set_bits(regmap, AS7343_ENABLE, AS7343_ENABLE_SP_EN);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops,
+				 as7343_suspend, as7343_resume, NULL);
+
+static const struct of_device_id as7343_of_match[] = {
+	{ .compatible = "ams,as7343" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, as7343_of_match);
+
+static const struct i2c_device_id as7343_id[] = {
+	{ .name = "as7343" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, as7343_id);
+
+static struct i2c_driver as7343_driver = {
+	.driver = {
+		.name = "as7343",
+		.of_match_table = as7343_of_match,
+		.pm = pm_ptr(&as7343_pm_ops),
+	},
+	.probe = as7343_probe,
+	.id_table = as7343_id,
+};
+module_i2c_driver(as7343_driver);
+
+MODULE_AUTHOR("Chang Yu <marcus.yu.56@gmail.com>");
+MODULE_DESCRIPTION("AS7343 14 Channel Multi-Spectral Sensor driver");
+MODULE_LICENSE("GPL");
-- 
2.55.0

Re: [PATCH v4 0/2] Add support for AS7343 multi-spectral sensor

[Jonathan Cameron]

On Fri, 11 Sep 2026 18:39:10 -0700
Chang Yu <marcus.yu.56@gmail.com> wrote:

> This series adds initial support for the AMS AS7343 14-channel
> multi-spectral sensor.
> 
> The sensor has 14 optical channels (11 visible + NIR + clear +
> flicker) in total. This initial driver supports reading raw counts
> from all 12 spectral channels (11 visible + NIR) via sysfs and basic
> power management.

Hi Chang Yu,

We all appreciate enthusiasm but...

No idea if anyone called it out already but please slow down.
Typically reviewers only get to looking at IIO patches once every week
or so (in totality - they may review a few random ones more often whilst
waiting for something to happen).

So for a driver in early stages of review, once a week at most for
new versions.  That way we consolidate a bunch of feedback.  Key thing
is many folk looking at your code doe enough review that they don't
have the previous version in their head so each very takes roughly the
same amount of time to do - it doesn't get shorter until very near merging
when we know tiny stuff only is changing.

Jonathan
> 
> Not yet supported:
> - Configurable integration time and gain
> - Interrupt support
> - Clear (VIS) channel and flicker detection
> 
> Patch 1 adds device tree bindings.
> Patch 2 adds the driver.
> 
> Testing was performed on a Raspberry Pi 4.
> 
> Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
> 
> Changes in v4:
>   - Fix led-max-microamp property in dt bindings.
>   - Use REGCACHE_MAPLE and add volatile table.
>   - Use PM_RUNTIME_ACQUIRE* when resuming.
> 
> Changes in v3:
>   - Fix gpio description and example in dt.
>   - Only start measurements after suspend action has been registered.
>   - Add a mutex and pm_runtime_resume_and_get before reading measurements.
> 
> Changes in v2:
>   - Add the LDR, the interrupt pin, and the GPIO pin to the dt bindings.
>   - Adjust the default gain to x256 and integration time to 50.1ms as
>   recommended by the datasheet.
>   - Use read_label instead of .extend
>   - Remove all unused scan_index related constants.
> 
> Chang Yu (2):
>   dt-bindings: iio: light: add as7343
>   iio: light: add AS7343 multi-spectral sensor driver
> 
>  .../bindings/iio/light/ams,as7343.yaml        |  72 +++
>  MAINTAINERS                                   |   7 +
>  drivers/iio/light/Kconfig                     |  11 +
>  drivers/iio/light/Makefile                    |   1 +
>  drivers/iio/light/as7343.c                    | 434 ++++++++++++++++++
>  5 files changed, 525 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
>  create mode 100644 drivers/iio/light/as7343.c
> 

Re: [PATCH v4 0/2] Add support for AS7343 multi-spectral sensor

[Chang Yu]

Hi Jonathan,

My sincere apologies. This is my first proper driver patch so
I didn't quite know how things work around here (I've only done
cleanup patches before). I'll slow down the submission
frequency from now on.

Best,
Chang

On Sun, Sep 13, 2026 at 01:30:56AM +0100, Jonathan Cameron wrote:
> On Fri, 11 Sep 2026 18:39:10 -0700
> Chang Yu <marcus.yu.56@gmail.com> wrote:
> 
> > This series adds initial support for the AMS AS7343 14-channel
> > multi-spectral sensor.
> > 
> > The sensor has 14 optical channels (11 visible + NIR + clear +
> > flicker) in total. This initial driver supports reading raw counts
> > from all 12 spectral channels (11 visible + NIR) via sysfs and basic
> > power management.
> 
> Hi Chang Yu,
> 
> We all appreciate enthusiasm but...
> 
> No idea if anyone called it out already but please slow down.
> Typically reviewers only get to looking at IIO patches once every week
> or so (in totality - they may review a few random ones more often whilst
> waiting for something to happen).
> 
> So for a driver in early stages of review, once a week at most for
> new versions.  That way we consolidate a bunch of feedback.  Key thing
> is many folk looking at your code doe enough review that they don't
> have the previous version in their head so each very takes roughly the
> same amount of time to do - it doesn't get shorter until very near merging
> when we know tiny stuff only is changing.
> 
> Jonathan
> > 
> > Not yet supported:
> > - Configurable integration time and gain
> > - Interrupt support
> > - Clear (VIS) channel and flicker detection
> > 
> > Patch 1 adds device tree bindings.
> > Patch 2 adds the driver.
> > 
> > Testing was performed on a Raspberry Pi 4.
> > 
> > Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
> > 
> > Changes in v4:
> >   - Fix led-max-microamp property in dt bindings.
> >   - Use REGCACHE_MAPLE and add volatile table.
> >   - Use PM_RUNTIME_ACQUIRE* when resuming.
> > 
> > Changes in v3:
> >   - Fix gpio description and example in dt.
> >   - Only start measurements after suspend action has been registered.
> >   - Add a mutex and pm_runtime_resume_and_get before reading measurements.
> > 
> > Changes in v2:
> >   - Add the LDR, the interrupt pin, and the GPIO pin to the dt bindings.
> >   - Adjust the default gain to x256 and integration time to 50.1ms as
> >   recommended by the datasheet.
> >   - Use read_label instead of .extend
> >   - Remove all unused scan_index related constants.
> > 
> > Chang Yu (2):
> >   dt-bindings: iio: light: add as7343
> >   iio: light: add AS7343 multi-spectral sensor driver
> > 
> >  .../bindings/iio/light/ams,as7343.yaml        |  72 +++
> >  MAINTAINERS                                   |   7 +
> >  drivers/iio/light/Kconfig                     |  11 +
> >  drivers/iio/light/Makefile                    |   1 +
> >  drivers/iio/light/as7343.c                    | 434 ++++++++++++++++++
> >  5 files changed, 525 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
> >  create mode 100644 drivers/iio/light/as7343.c
> > 
> 

Re: [PATCH v4 1/2] dt-bindings: iio: light: add as7343

[Jonathan Cameron]

On Fri, 11 Sep 2026 18:39:11 -0700
Chang Yu <marcus.yu.56@gmail.com> wrote:

> Add binding for AMS AS7343 which is a 14-channel multi-spectral sensor
> with i2c address of 0x39.
> 
> Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
> Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>

Some stuff on v2 still needs discussion.  A few comments inline.
Wait for that v2 discussion to finish or come to some conclusions
before sending a v5.

>  .../bindings/iio/light/ams,as7343.yaml        | 72 +++++++++++++++++++
>  MAINTAINERS                                   |  6 ++
>  2 files changed, 78 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml b/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
> new file mode 100644
> index 000000000000..d963832fb413
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
> @@ -0,0 +1,72 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/light/ams,as7343.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: AMS AS7343 14-Channel Multi-Spectral Sensor
> +
> +maintainers:
> +  - Chang Yu <marcus.yu.56@gmail.com>
> +
> +description: |
> +  The AMS AS7343 is a 14-channel multi-spectral sensor with i2c address of 0x39.
> +  https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - ams,as7343
> +
> +  reg:
> +    description:
> +      I2C address of the device (0x39).

Might as well specify it as const given it can only take that value.
They you can drop the description as it won't add anything.

> +    maxItems: 1
> +
> +  interrupts:
> +    description:
> +      Open drain output active low interrupt pin.
> +    maxItems: 1
> +
> +  vdd-supply: true
> +
> +  led-max-microamp:

Discussion on whether an led node is needed back in v2 is not resolved.

> +    description:
> +      The driver current for the external LED connected to the LDR pin.
> +    minimum: 4000
> +    maximum: 258000
> +    multipleOf: 2000
> +    default: 12000
> +
> +  sync-gpios:
> +    description: |
> +      Optional GPIO pin that can be used either as a synchronization input to
> +      start/stop measurement or as an external start/stop signal for
> +      synchronizing with the connected LED.

So the datasheet starts referring to using this for some level of sync
but then only actually treats it as as a gpio (maybe I'm missing something).
Any idea what that stuff about start/stop is talking about?

The only thing I can find about this is the device diagram in figure 13 that has the GPIO
wired to the led driver and the automatic measurement engine.

> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - vdd-supply
> +
> +additionalProperties: false

Re: [PATCH v4 1/2] dt-bindings: iio: light: add as7343

[Chang Yu]

Hi Jonathan,

A quick comment inline.

On Sun, Sep 13, 2026 at 01:56:52AM +0100, Jonathan Cameron wrote:
> On Fri, 11 Sep 2026 18:39:11 -0700
> Chang Yu <marcus.yu.56@gmail.com> wrote:
> 
> > +    description:
> > +      The driver current for the external LED connected to the LDR pin.
> > +    minimum: 4000
> > +    maximum: 258000
> > +    multipleOf: 2000
> > +    default: 12000
> > +
> > +  sync-gpios:
> > +    description: |
> > +      Optional GPIO pin that can be used either as a synchronization input to
> > +      start/stop measurement or as an external start/stop signal for
> > +      synchronizing with the connected LED.
> 
> So the datasheet starts referring to using this for some level of sync
> but then only actually treats it as as a gpio (maybe I'm missing something).
> Any idea what that stuff about start/stop is talking about?
> 
> The only thing I can find about this is the device diagram in figure 13 that has the GPIO
> wired to the led driver and the automatic measurement engine.
> 
That's what I got from the datasheet as well. They don't describe how to
use the GPIO pin to perform synchronizations at all.

The adafruit arduino driver
(https://github.com/adafruit/Adafruit_AS7343/blob/main/Adafruit_AS7343.cpp)
is slightly more helpful - the GPIO in output mode signals when the
spectral measurement starts. Unfortunately no information about input
mode.

> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - vdd-supply
> > +
> > +additionalProperties: false
> 
> 

Re: [PATCH v4 2/2] iio: light: add AS7343 multi-spectral sensor driver

[Jonathan Cameron]

On Fri, 11 Sep 2026 18:39:12 -0700
Chang Yu <marcus.yu.56@gmail.com> wrote:

> Add a driver for the AMS AS7343 14-channel multi-spectral sensor.
> 
> The AS7343 is a 14-channel spectral sensor featuring 11 visible
> channels, 1 near-infrared channel, 1 clear channel (VIS), and 1
> flicker detection channel.
> 
> The driver exposes 12 spectral channels (11 visible light and 1
> near-infrared) via sysfs. Runtime PM is implemented to stop
> measurements when the device is suspended or torn down. Power is
> never cut (PON=1 always) to preserve register values.
> 
> Future patches will add configurable gain and integration time,
> interrupt support, buffered reads, VIS channel, and flicker
> detection.
> 
> Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
> Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>

Various things inline,

Thanks,

Jonathan

> diff --git a/drivers/iio/light/as7343.c b/drivers/iio/light/as7343.c
> new file mode 100644
> index 000000000000..9ea6a44c35b9
> --- /dev/null
> +++ b/drivers/iio/light/as7343.c
> @@ -0,0 +1,434 @@

> + * TODO:
> + *   - Autosuspend

This should be trivial to do, so I'd prefer you do it from the start
as requires about 3 lines to be changed.

> + *   - Support for configurable gain and integration time
> + *   - Interrupt support
> + *   - Add support for reading the VIS channel
> + *   - Flicker detection
> + */

> + * Integration time is calculated as (ATIME + 1) * ((ASTEP + 1) * 2.78us).
> + * Setting a 30 * 1.67ms = 50.1ms integration time as the default for now.
> + */
> +#define AS7343_ATIME				0x81
> +#define AS7343_ATIME_VAL			29 /* (29 + 1) = 30 steps */
> +#define AS7343_ASTEP				0xd4
> +#define AS7343_ASTEP_VAL			599 /* 1.67ms step size */

See below. Define these values in terms of what they actually are.
That probably means a macro to indicate the maths applied.

> +
> +#define AS7343_CFG0				0xbf
> +#define AS7343_CFG0_REG_BANK			BIT(4)
> +
> +#define AS7343_CFG1				0xc6
> +#define AS7343_CFG1_AGAIN			GENMASK(4, 0)
> +#define AS7343_CFG1_AGAIN_X0_5			0
> +#define AS7343_CFG1_AGAIN_X1			1
> +#define AS7343_CFG1_AGAIN_X2			2
> +#define AS7343_CFG1_AGAIN_X4			3
> +#define AS7343_CFG1_AGAIN_X8			4
> +#define AS7343_CFG1_AGAIN_X16			5
> +#define AS7343_CFG1_AGAIN_X32			6
> +#define AS7343_CFG1_AGAIN_X64			7
> +#define AS7343_CFG1_AGAIN_X128			8
> +#define AS7343_CFG1_AGAIN_X256			9
> +#define AS7343_CFG1_AGAIN_X512			10
> +#define AS7343_CFG1_AGAIN_X1024			11
> +#define AS7343_CFG1_AGAIN_X2048			12
> +
> +#define AS7343_CFG20				0xd6
> +#define AS7343_CFG20_AUTO_SMUX			GENMASK(6, 5)
> +#define AS7343_CFG20_AUTO_SMUX_READOUT_ALL	3 /* all-channel readout */
> +
> +#define AS7343_CONTROL				0xfa
> +
> +/* AS7343 status registers */
> +#define AS7343_STATUS2				0x90
> +#define AS7343_STATUS3				0x91
> +#define AS7343_STATUS				0x93
> +#define AS7343_ASTATUS				0x94
> +#define AS7343_STATUS5				0xbb
> +#define AS7343_STATUS4				0xbc
> +#define AS7343_FD_STATUS			0xe3
> +
> +/* AS7343 spectral data registers */
> +#define AS7343_DATA_FZ		0x95

Given some of your register have a _ in the register name part
I'd add something else to make it clear what is a register
and what is a field in these defines.  Postfix of _REG often
does this job in other drivers.  Apply that consistently
to all register address definitions.

> +#define AS7343_DATA_FY		0x97
> +#define AS7343_DATA_FXL		0x99
> +#define AS7343_DATA_NIR		0x9b
> +#define AS7343_DATA_F2		0xa1
> +#define AS7343_DATA_F3		0xa3
> +#define AS7343_DATA_F4		0xa5
> +#define AS7343_DATA_F6		0xa7
> +#define AS7343_DATA_F1		0xad
> +#define AS7343_DATA_F7		0xaf
> +#define AS7343_DATA_F8		0xb1
> +#define AS7343_DATA_F5		0xb3
> +#define AS7343_DATA_FD_L	0xb7
> +#define AS7343_DATA_FD_H	0xb8
> +
> +/* AS7343 FIFO buffer data registers */
> +#define AS7343_FIFO_LVL		0xfd
> +#define AS7343_FDATA_L		0xfe
> +#define AS7343_FDATA_H		0xff
> +
> +/* AS7343 channel indices. MUST match data register order above. */
> +#define AS7343_CHAN_IDX_FZ	0
> +#define AS7343_CHAN_IDX_FY	1
> +#define AS7343_CHAN_IDX_FXL	2
> +#define AS7343_CHAN_IDX_NIR	3
> +#define AS7343_CHAN_IDX_F2	4
> +#define AS7343_CHAN_IDX_F3	5
> +#define AS7343_CHAN_IDX_F4	6
> +#define AS7343_CHAN_IDX_F6	7
> +#define AS7343_CHAN_IDX_F1	8
> +#define AS7343_CHAN_IDX_F7	9
> +#define AS7343_CHAN_IDX_F8	10
> +#define AS7343_CHAN_IDX_F5	11
> +
> +#define AS7343_CHAN(_chan)					\
> +	{							\
> +		.type = IIO_INTENSITY,				\
> +		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> +		.address = AS7343_DATA_##_chan,			\
> +		.indexed = 1,					\
> +		.channel = AS7343_CHAN_IDX_##_chan,		\
> +	}
> +
> +static const struct iio_chan_spec as7343_channels[] = {
> +	AS7343_CHAN(FZ), AS7343_CHAN(FY), AS7343_CHAN(FXL), AS7343_CHAN(NIR),
> +	AS7343_CHAN(F2), AS7343_CHAN(F3), AS7343_CHAN(F4),  AS7343_CHAN(F6),
> +	AS7343_CHAN(F1), AS7343_CHAN(F7), AS7343_CHAN(F8),  AS7343_CHAN(F5),
> +};
> +
> +struct as7343_data {
> +	struct regmap *regmap;
> +	/* Ensures reads don't stomp on each other */
> +	struct mutex mutex;
> +};
> +
> +static int as7343_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct as7343_data *data = iio_priv(indio_dev);
> +	unsigned int unused;
> +	struct regmap *map;
	struct regmap *map = data->regmap;
	struct device *dev = regmap_get_device(map);

Neither is checked so no point in waiting until a few lines
later to initialize them.

> +	struct device *dev;
> +	__le16 result;
> +	int ret;
> +
> +	map = data->regmap;
> +	dev = regmap_get_device(map);
> +
> +	PM_RUNTIME_ACQUIRE_IF_ENABLED(dev, pm);

Why _IF_ENABLED() variant?  This has come up in several drivers
recently and it makes little sense when used like this. You do this
when you care whether or not runtime pm is enabled.  We don't. Either
device is on already, or runtime pm is enabled and we can turn it on.

> +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> +	if (ret)
> +		return ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		/* Wait until integration time passes for all 3 cycles. */
> +		msleep(160);
> +
> +		/*
> +		 * Reading ASTATUS latches all data registers to this read.
> +		 * We don't care about the returned saturation/gain status for
> +		 * now.

Why do we care given only reading one channel and...

> +		 */
> +		guard(mutex)(&data->mutex);
> +		ret = regmap_read(map, AS7343_ASTATUS, &unused);
> +		if (ret)
> +			return ret;
> +
> +		ret = regmap_bulk_read(map, chan->address,
.. it seems that if you read the low byte first (as this does) it is latched anyway.
There is a statement about this in the i2c intro part section 9.

so we get a consistent register pair.  Thus not needing the latch astatus
gives us.
With that in place reads can't stomp on each other as only
one regmap read is required, so the mutex isn't needed either.
It may be necessary once you add more features - hard to tell yet.

> +				       &result, sizeof(result));
> +		if (ret)
> +			return ret;
> +
> +		*val = le16_to_cpu(result);
> +		return IIO_VAL_INT;
> +	}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +/*
> + * Channel names and wavelength ranges as defined in the datasheet
> + * (Figure 7, "AS7343 Optical Channel Summary"). Values are the
> + * minimum and maximum peak wavelength in nanometers.
> + *
> + * F1:  395-415 nm
> + * F2:  415-435 nm
> + * FZ:  440-460 nm
> + * F3:  465-485 nm
> + * F4:  505-525 nm
> + * FY:  545-565 nm
> + * F5:  540-560 nm
> + * FXL: 590-610 nm
> + * F6:  630-650 nm
> + * F7:  680-700 nm
> + * F8:  735-755 nm
> + * NIR: 845-865 nm
> + */
> +static const char * const as7343_channel_labels[] = {
> +	[AS7343_CHAN_IDX_F1] = "F1",
> +	[AS7343_CHAN_IDX_F2] = "F2",
> +	[AS7343_CHAN_IDX_FZ] = "FZ",
> +	[AS7343_CHAN_IDX_F3] = "F3",
> +	[AS7343_CHAN_IDX_F4] = "F4",
> +	[AS7343_CHAN_IDX_FY] = "FY",
> +	[AS7343_CHAN_IDX_F5] = "F5",
> +	[AS7343_CHAN_IDX_FXL] = "FXL",
> +	[AS7343_CHAN_IDX_F6] = "F6",
> +	[AS7343_CHAN_IDX_F7] = "F7",
> +	[AS7343_CHAN_IDX_F8] = "F8",
> +	[AS7343_CHAN_IDX_NIR] = "NIR",

I wonder if these could be more useful to a user?  Or are these things standard
in some sense?  Maybe we could put the frequency range in the label given they
are free form 'hints' to userspace.
"F1: 395-415 nm" might be fine?  I'm curious what others think of this suggestion.

> +};
> +
> +static int as7343_read_label(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, char *label)
> +{
> +	int channel = chan->channel;

What stops this being negative?  I'm wondering now why channel in iio_chan_spec
is signed (maybe a decision lost to the mists of time), but it is so you
need to check for < 0 as well.

> +
> +	if (channel >= ARRAY_SIZE(as7343_channel_labels))
> +		return -EINVAL;
> +
> +	return sysfs_emit(label, "%s\n", as7343_channel_labels[channel]);
> +}

> +
> +static int as7343_setup_device(struct device *dev, struct as7343_data *data)
> +{
> +	struct regmap *map = data->regmap;
> +	unsigned int val;
> +	__le16 step;
> +	int ret;
> +
> +	/* Power on */
> +	ret = regmap_set_bits(map, AS7343_ENABLE, AS7343_ENABLE_PON);
> +	if (ret)
> +		return ret;
> +
> +	/* Need to set REG_BANK to 1 before we can access ID */
> +	ret = regmap_set_bits(map, AS7343_CFG0, AS7343_CFG0_REG_BANK);

For the bank I'd not use set_bits / clear_bits because the 0 / 1 nature
of the bank being selected is lost.  Sure it ends up as the same
resule but I would like that FIELD_SET(AS7343_CFG0_REG_BANK, 1) visible.

> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(map, AS7343_ID, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != 0x81)
> +		dev_info(dev, "Unknown device ID: %x\n", val);
> +
> +	ret = regmap_clear_bits(map, AS7343_CFG0, AS7343_CFG0_REG_BANK);

As above.  We are setting a field to 0 or 1 not clearing or setting
a flag.  It's a bit odd to call them register banks though given there
are no overlapping addresses.

> +	if (ret)
> +		return ret;
> +
> +	/* Configure the SMUX to readout all channels */
> +	ret = regmap_update_bits(map, AS7343_CFG20, AS7343_CFG20_AUTO_SMUX,
> +				 FIELD_PREP(AS7343_CFG20_AUTO_SMUX,
> +					    AS7343_CFG20_AUTO_SMUX_READOUT_ALL));
> +	if (ret)
> +		return ret;
> +
> +	/* Set 50.1ms integration time and x256 gain for now */
> +	step = cpu_to_le16(AS7343_ASTEP_VAL);

As below, what does this mean?  Some sort of macro or function
to associate that with the step size  AS7343_ASTEP_X270ns(0xd5) or
something like that. Given datasheet uses decimal for the actual definition
if not the demo value, use decimal here.

> +	ret = regmap_bulk_write(map, AS7343_ASTEP, &step, sizeof(step));
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(map, AS7343_ATIME, AS7343_ATIME_VAL);
What does ATIME_VAL indicate?  Name it after what it represents
or better yet use a macro AS7343_ATIME_STEPS(30)

> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(map, AS7343_CFG1, AS7343_CFG1_AGAIN,
> +				  FIELD_PREP(AS7343_CFG1_AGAIN,
> +					     AS7343_CFG1_AGAIN_X256));
> +}

> +
> +static void as7343_suspend_action(void *data)
> +{
> +	struct device *dev = data;
> +
> +	as7343_suspend(dev);

static void as7343_suspend_action(void *dev)
{
	as7343_suspend(dev);
}

is fine for this sort of thing. dev is standard naming for a struct
device so the local variable isn't adding anyting.

> +}
> +
> +static int as7343_probe(struct i2c_client *client)
> +{

..

> +	ret = as7343_setup_device(dev, data);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, as7343_suspend_action, dev);

Why here? You only start sensing sometime later.

> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to add suspend action\n");
> +
> +	ret = pm_runtime_set_active(dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret,
> +				     "Failed to activate PM runtime\n");
> +
> +	ret = devm_pm_runtime_enable(dev);

Why is devm_pm_runtime_enable_set_active() not appropriate here?
(Note I'm not saying it is, but it seems worth considering).

> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to enable PM runtime\n");
> +
> +	/* Start measurements */
> +	ret = regmap_set_bits(regmap, AS7343_ENABLE, AS7343_ENABLE_SP_EN);

Logically I would do this before 'set_active' is called as this is
what puts the device in active state.

> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

Re: [PATCH v4 1/2] dt-bindings: iio: light: add as7343

[Jonathan Cameron]

On Sat, 12 Sep 2026 18:35:14 -0700
Chang Yu <marcus.yu.56@gmail.com> wrote:

> Hi Jonathan,
> 
> A quick comment inline.
> 
> On Sun, Sep 13, 2026 at 01:56:52AM +0100, Jonathan Cameron wrote:
> > On Fri, 11 Sep 2026 18:39:11 -0700
> > Chang Yu <marcus.yu.56@gmail.com> wrote:
> >   
> > > +    description:
> > > +      The driver current for the external LED connected to the LDR pin.
> > > +    minimum: 4000
> > > +    maximum: 258000
> > > +    multipleOf: 2000
> > > +    default: 12000
> > > +
> > > +  sync-gpios:
> > > +    description: |
> > > +      Optional GPIO pin that can be used either as a synchronization input to
> > > +      start/stop measurement or as an external start/stop signal for
> > > +      synchronizing with the connected LED.  
> > 
> > So the datasheet starts referring to using this for some level of sync
> > but then only actually treats it as as a gpio (maybe I'm missing something).
> > Any idea what that stuff about start/stop is talking about?
> > 
> > The only thing I can find about this is the device diagram in figure 13 that has the GPIO
> > wired to the led driver and the automatic measurement engine.
> >   
> That's what I got from the datasheet as well. They don't describe how to
> use the GPIO pin to perform synchronizations at all.
> 
> The adafruit arduino driver
> (https://github.com/adafruit/Adafruit_AS7343/blob/main/Adafruit_AS7343.cpp)
> is slightly more helpful - the GPIO in output mode signals when the
> spectral measurement starts. Unfortunately no information about input
> mode.

There is no separate control?  If so we can't use it as a GPIO as whilst
there is a control to set the state it will randomly get overwritten if
we do such a read. So to me sounds like that is there for testing the
connection and similar usecases only and we should not expose this as
a GPIO. Maybe input is possible but that may also have side effects.
So I'd be tempted to drop it from the binding - with some description
of why it is not there in the patch description.

> 
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - vdd-supply
> > > +
> > > +additionalProperties: false  
> > 
> >   
> 

Re: [PATCH v4 1/2] dt-bindings: iio: light: add as7343

[Chang Yu]

On Sun, Sep 13, 2026 at 03:51:32AM +0100, Jonathan Cameron wrote:
> On Sat, 12 Sep 2026 18:35:14 -0700
> Chang Yu <marcus.yu.56@gmail.com> wrote:
> 
> > Hi Jonathan,
> > 
> > A quick comment inline.
> > 
> > On Sun, Sep 13, 2026 at 01:56:52AM +0100, Jonathan Cameron wrote:
> > > On Fri, 11 Sep 2026 18:39:11 -0700
> > > Chang Yu <marcus.yu.56@gmail.com> wrote:
> > >   
> > > > +    description:
> > > > +      The driver current for the external LED connected to the LDR pin.
> > > > +    minimum: 4000
> > > > +    maximum: 258000
> > > > +    multipleOf: 2000
> > > > +    default: 12000
> > > > +
> > > > +  sync-gpios:
> > > > +    description: |
> > > > +      Optional GPIO pin that can be used either as a synchronization input to
> > > > +      start/stop measurement or as an external start/stop signal for
> > > > +      synchronizing with the connected LED.  
> > > 
> > > So the datasheet starts referring to using this for some level of sync
> > > but then only actually treats it as as a gpio (maybe I'm missing something).
> > > Any idea what that stuff about start/stop is talking about?
> > > 
> > > The only thing I can find about this is the device diagram in figure 13 that has the GPIO
> > > wired to the led driver and the automatic measurement engine.
> > >   
> > That's what I got from the datasheet as well. They don't describe how to
> > use the GPIO pin to perform synchronizations at all.
> > 
> > The adafruit arduino driver
> > (https://github.com/adafruit/Adafruit_AS7343/blob/main/Adafruit_AS7343.cpp)
> > is slightly more helpful - the GPIO in output mode signals when the
> > spectral measurement starts. Unfortunately no information about input
> > mode.
> 
> There is no separate control?  If so we can't use it as a GPIO as whilst
> there is a control to set the state it will randomly get overwritten if
> we do such a read. So to me sounds like that is there for testing the
> connection and similar usecases only and we should not expose this as
> a GPIO. Maybe input is possible but that may also have side effects.
> So I'd be tempted to drop it from the binding - with some description
> of why it is not there in the patch description.
> 

Agreed. What does this mean for the LED then? For all intents and purposes
it's controlled by only 1 register. It may have some interactions with the
GPIO but since they are completely undocumented so we might as well consider
them non-existent. IMO it probably doesn't warrant a LED subnode and
led-max-microamp is enough. Maybe DT folks can also comment on that.

> > 
> > > > +    maxItems: 1
> > > > +
> > > > +required:
> > > > +  - compatible
> > > > +  - reg
> > > > +  - vdd-supply
> > > > +
> > > > +additionalProperties: false  
> > > 
> > >   
> > 
> 

Re: [PATCH v4 2/2] iio: light: add AS7343 multi-spectral sensor driver

[Chang Yu]

On Sun, Sep 13, 2026 at 03:47:14AM +0100, Jonathan Cameron wrote:
> On Fri, 11 Sep 2026 18:39:12 -0700
> Chang Yu <marcus.yu.56@gmail.com> wrote:
> > +static int as7343_read_raw(struct iio_dev *indio_dev,
> > +			   struct iio_chan_spec const *chan,
> > +			   int *val, int *val2, long mask)
> > +{
> > +	struct as7343_data *data = iio_priv(indio_dev);
> > +	unsigned int unused;
> > +	struct regmap *map;
> 	struct regmap *map = data->regmap;
> 	struct device *dev = regmap_get_device(map);
> 
> Neither is checked so no point in waiting until a few lines
> later to initialize them.
> 

I was trying to preserve the reverse christmas tree order and
was unsure how the rules apply here. Is the following OK:

  struct as7343_data *data = iio_priv(indio_dev);
  struct regmap *map = data->regmap;
  struct device *dev = regmap_get_device(map);

  struct device *dev;
  __le16 result;
  int ret;

> ...
> > +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> > +	if (ret)
> > +		return ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW: {
> > +		/* Wait until integration time passes for all 3 cycles. */
> > +		msleep(160);
> > +
> > +		/*
> > +		 * Reading ASTATUS latches all data registers to this read.
> > +		 * We don't care about the returned saturation/gain status for
> > +		 * now.
> 
> Why do we care given only reading one channel and...
> 
> > +		 */
> > +		guard(mutex)(&data->mutex);
> > +		ret = regmap_read(map, AS7343_ASTATUS, &unused);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = regmap_bulk_read(map, chan->address,
> .. it seems that if you read the low byte first (as this does) it is latched anyway.
> There is a statement about this in the i2c intro part section 9.
> 
> so we get a consistent register pair.  Thus not needing the latch astatus
> gives us.
> With that in place reads can't stomp on each other as only
> one regmap read is required, so the mutex isn't needed either.
> It may be necessary once you add more features - hard to tell yet.
> 
When testing on hardware I discovered, for whatever reason, register
values for other channels won't refresh unless I read ASTATUS or the
FZ channel first. The only thing I can find in the datasheet pertaining
to this is 10.2.7 page 41.

"Reading the ASTATUS register (0x94) latches all 36 spectral data bytes
to that status read. Reading these bytes consecutively (0x94 to 0xB8)
ensures that the data is concurrent."

My most charitable interpretation is that by "latches" they also mean
"updates". I have no idea why reading FZ also works though. Could be
a hardware bug since ASTATUS (0x94) and FZ DATA_0_L (0x95) are next
to each other.

Re: [PATCH v4 1/2] dt-bindings: iio: light: add as7343

[Jonathan Cameron]

On Sat, 12 Sep 2026 20:20:13 -0700
Chang Yu <marcus.yu.56@gmail.com> wrote:

> On Sun, Sep 13, 2026 at 03:51:32AM +0100, Jonathan Cameron wrote:
> > On Sat, 12 Sep 2026 18:35:14 -0700
> > Chang Yu <marcus.yu.56@gmail.com> wrote:
> >   
> > > Hi Jonathan,
> > > 
> > > A quick comment inline.
> > > 
> > > On Sun, Sep 13, 2026 at 01:56:52AM +0100, Jonathan Cameron wrote:  
> > > > On Fri, 11 Sep 2026 18:39:11 -0700
> > > > Chang Yu <marcus.yu.56@gmail.com> wrote:
> > > >     
> > > > > +    description:
> > > > > +      The driver current for the external LED connected to the LDR pin.
> > > > > +    minimum: 4000
> > > > > +    maximum: 258000
> > > > > +    multipleOf: 2000
> > > > > +    default: 12000
> > > > > +
> > > > > +  sync-gpios:
> > > > > +    description: |
> > > > > +      Optional GPIO pin that can be used either as a synchronization input to
> > > > > +      start/stop measurement or as an external start/stop signal for
> > > > > +      synchronizing with the connected LED.    
> > > > 
> > > > So the datasheet starts referring to using this for some level of sync
> > > > but then only actually treats it as as a gpio (maybe I'm missing something).
> > > > Any idea what that stuff about start/stop is talking about?
> > > > 
> > > > The only thing I can find about this is the device diagram in figure 13 that has the GPIO
> > > > wired to the led driver and the automatic measurement engine.
> > > >     
> > > That's what I got from the datasheet as well. They don't describe how to
> > > use the GPIO pin to perform synchronizations at all.
> > > 
> > > The adafruit arduino driver
> > > (https://github.com/adafruit/Adafruit_AS7343/blob/main/Adafruit_AS7343.cpp)
> > > is slightly more helpful - the GPIO in output mode signals when the
> > > spectral measurement starts. Unfortunately no information about input
> > > mode.  
> > 
> > There is no separate control?  If so we can't use it as a GPIO as whilst
> > there is a control to set the state it will randomly get overwritten if
> > we do such a read. So to me sounds like that is there for testing the
> > connection and similar usecases only and we should not expose this as
> > a GPIO. Maybe input is possible but that may also have side effects.
> > So I'd be tempted to drop it from the binding - with some description
> > of why it is not there in the patch description.
> >   
> 
> Agreed. What does this mean for the LED then? For all intents and purposes
> it's controlled by only 1 register. It may have some interactions with the
> GPIO but since they are completely undocumented so we might as well consider
> them non-existent. IMO it probably doesn't warrant a LED subnode and
> led-max-microamp is enough. Maybe DT folks can also comment on that.

Agreed, given software is controlling the current we need that and there
isn't a lot more we can do.  I guess you have the Adafruit board. Are the
pins wired out so you can perhaps see what happens in practice?

> 
> > >   
> > > > > +    maxItems: 1
> > > > > +
> > > > > +required:
> > > > > +  - compatible
> > > > > +  - reg
> > > > > +  - vdd-supply
> > > > > +
> > > > > +additionalProperties: false    
> > > > 
> > > >     
> > >   
> >   

Re: [PATCH v4 2/2] iio: light: add AS7343 multi-spectral sensor driver

[Jonathan Cameron]

On Sat, 12 Sep 2026 21:11:07 -0700
Chang Yu <marcus.yu.56@gmail.com> wrote:

> On Sun, Sep 13, 2026 at 03:47:14AM +0100, Jonathan Cameron wrote:
> > On Fri, 11 Sep 2026 18:39:12 -0700
> > Chang Yu <marcus.yu.56@gmail.com> wrote:  
> > > +static int as7343_read_raw(struct iio_dev *indio_dev,
> > > +			   struct iio_chan_spec const *chan,
> > > +			   int *val, int *val2, long mask)
> > > +{
> > > +	struct as7343_data *data = iio_priv(indio_dev);
> > > +	unsigned int unused;
> > > +	struct regmap *map;  
> > 	struct regmap *map = data->regmap;
> > 	struct device *dev = regmap_get_device(map);
> > 
> > Neither is checked so no point in waiting until a few lines
> > later to initialize them.
> >   
> 
> I was trying to preserve the reverse christmas tree order and
> was unsure how the rules apply here. Is the following OK:
> 
>   struct as7343_data *data = iio_priv(indio_dev);
>   struct regmap *map = data->regmap;
>   struct device *dev = regmap_get_device(map);
> 
No line break
>   struct device *dev;
>   __le16 result;
>   int ret;

Yes this is fine.  Reverse xmas tree is just for when there are no
dependencies.

> 
> > ...  
> > > +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_RAW: {
> > > +		/* Wait until integration time passes for all 3 cycles. */
> > > +		msleep(160);
> > > +
> > > +		/*
> > > +		 * Reading ASTATUS latches all data registers to this read.
> > > +		 * We don't care about the returned saturation/gain status for
> > > +		 * now.  
> > 
> > Why do we care given only reading one channel and...
> >   
> > > +		 */
> > > +		guard(mutex)(&data->mutex);
> > > +		ret = regmap_read(map, AS7343_ASTATUS, &unused);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		ret = regmap_bulk_read(map, chan->address,  
> > .. it seems that if you read the low byte first (as this does) it is latched anyway.
> > There is a statement about this in the i2c intro part section 9.
> > 
> > so we get a consistent register pair.  Thus not needing the latch astatus
> > gives us.
> > With that in place reads can't stomp on each other as only
> > one regmap read is required, so the mutex isn't needed either.
> > It may be necessary once you add more features - hard to tell yet.
> >   
> When testing on hardware I discovered, for whatever reason, register
> values for other channels won't refresh unless I read ASTATUS or the
> FZ channel first. The only thing I can find in the datasheet pertaining
> to this is 10.2.7 page 41.
> 
> "Reading the ASTATUS register (0x94) latches all 36 spectral data bytes
> to that status read. Reading these bytes consecutively (0x94 to 0xB8)
> ensures that the data is concurrent."
> 
> My most charitable interpretation is that by "latches" they also mean
> "updates". I have no idea why reading FZ also works though. Could be
> a hardware bug since ASTATUS (0x94) and FZ DATA_0_L (0x95) are next
> to each other.
It is curious as if they are latched anyway what is the text about latching
if read from lower byte about.  Who knows!  This is definitely a less
than ideal datasheet but I don't think we have anyone active currently
upstream who works for AMS.

Jonathan