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

[PATCH v3 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 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                    | 397 ++++++++++++++++++
 5 files changed, 488 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
 create mode 100644 drivers/iio/light/as7343.c

-- 
2.55.0

[PATCH v3 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 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..6b8a05be0043
--- /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
+
+  ams,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>;
+            ams,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 v3 2/2] iio: light: add AS7343 multi-spectral sensor driver

[Chang Yu]

This patch adds a driver for the AMS AS7343 14-channel multi-spectral
sensor with I2C interface.

The driver exposes 12 spectral channels (11 visible + 1 near-infrared)
via the IIO sysfs interface. Each channel's raw data is provided as a
16-bit little-endian unsigned integer.

Basic power management (suspend/resume) is supported. Note that power
management is designed in such a way that we only stop spectrual
measurements when susepended and do not power off.

More complex features such as auto-suspend, interrupts, and
configurable gain/integration time will be added in future patches.

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 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 | 397 +++++++++++++++++++++++++++++++++++++
 4 files changed, 410 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..8982cd6ec5d8
--- /dev/null
+++ b/drivers/iio/light/as7343.c
@@ -0,0 +1,397 @@
+// 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/dev_printk.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/pm.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>
+
+/* AS7343 registers */
+#define AS7343_ID	    0x5a
+
+#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 test 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_ASTATUS			   0x94
+
+/* AS7343 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_MAX	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;
+	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);
+	struct device *dev = regmap_get_device(data->regmap);
+	int ret;
+	unsigned int unused;
+	__le16 result;
+
+	ret = pm_runtime_resume_and_get(dev);
+	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(data->regmap, AS7343_ASTATUS, &unused);
+		if (ret)
+			break;
+
+		ret = regmap_bulk_read(data->regmap, chan->address, &result, 2);
+		if (ret)
+			break;
+
+		*val = le16_to_cpu(result);
+		ret = IIO_VAL_INT;
+		break;
+	}
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	pm_runtime_put(dev);
+	return ret;
+}
+
+static const char *as7343_channel_label(struct iio_chan_spec const *chan)
+{
+	switch (chan->channel) {
+	case AS7343_CHAN_IDX_FZ:
+		return "FZ";
+	case AS7343_CHAN_IDX_FY:
+		return "FY";
+	case AS7343_CHAN_IDX_FXL:
+		return "FXL";
+	case AS7343_CHAN_IDX_NIR:
+		return "NIR";
+	case AS7343_CHAN_IDX_F2:
+		return "F2";
+	case AS7343_CHAN_IDX_F3:
+		return "F3";
+	case AS7343_CHAN_IDX_F4:
+		return "F4";
+	case AS7343_CHAN_IDX_F6:
+		return "F6";
+	case AS7343_CHAN_IDX_F1:
+		return "F1";
+	case AS7343_CHAN_IDX_F7:
+		return "F7";
+	case AS7343_CHAN_IDX_F8:
+		return "F8";
+	case AS7343_CHAN_IDX_F5:
+		return "F5";
+	default:
+		return NULL;
+	}
+}
+
+static int as7343_read_label(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan, char *label)
+{
+	const char *name;
+
+	name = as7343_channel_label(chan);
+	if (!name)
+		return -EINVAL;
+
+	return sysfs_emit(label, "%s\n", name);
+}
+
+static const struct iio_info as7343_info = {
+	.read_raw = as7343_read_raw,
+	.read_label = as7343_read_label,
+};
+
+static const struct regmap_config as7343_regmap_config = {
+	.name = "as7343",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AS7343_MAX,
+	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.cache_type = REGCACHE_NONE,
+};
+
+static int as7343_setup_device(struct device *dev, struct as7343_data *data)
+{
+	unsigned int val;
+	__le16 step;
+	int ret;
+
+	/* Power on */
+	ret = regmap_set_bits(data->regmap, 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(data->regmap, AS7343_CFG0, AS7343_CFG0_REG_BANK);
+	if (ret)
+		return ret;
+
+	ret = regmap_read(data->regmap, AS7343_ID, &val);
+	if (ret)
+		return ret;
+
+	if (val != 0x81)
+		dev_info(dev, "Unknown device ID: %x\n", val);
+
+	ret = regmap_clear_bits(data->regmap, AS7343_CFG0,
+				AS7343_CFG0_REG_BANK);
+	if (ret)
+		return ret;
+
+	/* Configure the SMUX to readout all channels */
+	ret = regmap_update_bits(
+		data->regmap, 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(data->regmap, AS7343_ASTEP, &step, 2);
+	if (ret)
+		return ret;
+
+	ret = regmap_write(data->regmap, AS7343_ATIME, AS7343_ATIME_VAL);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(data->regmap, 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);
+
+	return regmap_clear_bits(data->regmap, 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);
+
+	return regmap_set_bits(data->regmap, AS7343_ENABLE,
+			       AS7343_ENABLE_SP_EN);
+}
+
+static void as7343_suspend_action(void *data)
+{
+	as7343_suspend(data);
+}
+
+static int as7343_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct as7343_data *data;
+	struct iio_dev *indio_dev;
+	struct regmap *regmap;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	regmap = devm_regmap_init_i2c(client, &as7343_regmap_config);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
+
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->regmap = regmap;
+	mutex_init(&data->mutex);
+
+	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(data->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 v3 1/2] dt-bindings: iio: light: add as7343

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The generic LED property `led-max-microamp` was incorrectly added with a vendor prefix (`ams,led-max-microamp`), contradicting the commit message and standard property conventions.
--

commit bc94ab43a595b8c728d510d6725b14dbeb5f151f
Author: Chang Yu <marcus.yu.56@gmail.com>

dt-bindings: iio: light: add as7343

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

> 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 0000000000000..6b8a05be00430
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
[ ... ]
> +  vdd-supply: true
> +
> +  ams,led-max-microamp:

[Severity: Medium]
Should this property be defined without the vendor prefix?

The generic LED property led-max-microamp is added here with a vendor
prefix (ams,led-max-microamp), which contradicts standard property
conventions and the stated intent in the patch version changelog.

Device trees written for this device will use this vendor-prefixed
property, creating permanent ABI baggage. If a driver later implements
this feature and correctly parses the standard led-max-microamp, it will
break compatibility with these early device trees or force the driver to
parse both properties.

> +    description:
> +      The driver current for the external LED connected to the LDR pin.
> +    minimum: 4000
> +    maximum: 258000
> +    multipleOf: 2000
> +    default: 12000
> +
[ ... ]
> +        multispectral-sensor@39 {
> +            compatible = "ams,as7343";
> +            reg = <0x39>;
> +            interrupts = <7 IRQ_TYPE_LEVEL_LOW>;
> +            vdd-supply = <&vdd_regulator>;
> +            ams,led-max-microamp = <14000>;

[Severity: Medium]
Would it be better to also remove the vendor prefix from this property in the
device tree example?

> +            sync-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
> +        };
> +    };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910063813.56419-1-marcus.yu.56@gmail.com?part=1

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Device remains continuously active and measuring until the first sysfs read due to missing idle trigger in probe.
- [Low] Missing <linux/delay.h> include for msleep().
--

commit 11270e0cc23d66ee1d725cbe09a1342ed010ace3
Author: Chang Yu <marcus.yu.56@gmail.com>

iio: light: add AS7343 multi-spectral sensor driver

This commit adds a new driver for the AMS AS7343 14-channel multi-spectral
sensor. It exposes 12 spectral channels via the IIO sysfs interface and
supports basic suspend/resume power management functionality.

> diff --git a/drivers/iio/light/as7343.c b/drivers/iio/light/as7343.c
> new file mode 100644
> index 0000000000000..8982cd6ec5d88
> --- /dev/null
> +++ b/drivers/iio/light/as7343.c
[ ... ]
> +static int as7343_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan, int *val,
> +			   int *val2, long mask)
> +{
[ ... ]
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		/* Wait until integration time passes for all 3 cycles. */
> +		msleep(160);

[Severity: Low]
Does this msleep() call require including <linux/delay.h>?

Building the driver on architectures where the header is not implicitly
included by other headers might result in a build failure.

[ ... ]
> +static int as7343_probe(struct i2c_client *client)
> +{
[ ... ]
> +	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(data->regmap, AS7343_ENABLE, AS7343_ENABLE_SP_EN);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}

[Severity: Medium]
Does this probe function leave the device continuously active and measuring
until the first sysfs read?

The code marks the device as active and starts measurements, but does not
appear to drop its usage count or trigger an idle state. The power management
core may consider it active with a zero usage count, but never automatically
suspend it.

This seems to leave the sensor taking measurements and drawing power
indefinitely, only resolving when the first sysfs read invokes
pm_runtime_resume_and_get() followed by pm_runtime_put().

Could pm_runtime_idle() be called before probe finishes, or should the device
be transitioned to suspend instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910063813.56419-1-marcus.yu.56@gmail.com?part=2

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

[Andy Shevchenko]

On Wed, Sep 09, 2026 at 11:38:13PM -0700, Chang Yu wrote:
> This patch adds a driver for the AMS AS7343 14-channel multi-spectral
> sensor with I2C interface.
> 
> The driver exposes 12 spectral channels (11 visible + 1 near-infrared)
> via the IIO sysfs interface. Each channel's raw data is provided as a
> 16-bit little-endian unsigned integer.
> 
> Basic power management (suspend/resume) is supported. Note that power
> management is designed in such a way that we only stop spectrual
> measurements when susepended and do not power off.
> 
> More complex features such as auto-suspend, interrupts, and
> configurable gain/integration time will be added in future patches.

It's v3 already. Can you browse the linux-iio@ mailing list archive and see
what are the common comments on the new contributions? I think you may ask
AI to help with the summary. This patch has tons of what has been repeated
over and over...

...

> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/cleanup.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.h>

This is implied by pm_runtime.h IIRC.

> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/sysfs.h>
> +#include <linux/types.h>

...

> +/* AS7343 registers */
> +#define AS7343_ID	    0x5a
> +
> +#define AS7343_ENABLE	    0x80

Make sure the indentation of the definition of the same kind are the same.

...

> +/*
> + * Integration time is calculated as (ATIME + 1) * ((ASTEP + 1) * 2.78us).
> + * Setting a 30 * 1.67ms = 50.1ms integration test as the default for now.
> + */
> +#define AS7343_ATIME			   0x81

I believe this one is related to the register offsets? (see above why)

...

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

Please, split logically.

			   struct iio_chan_spec const *chan,
			   int *val, int *val2, long mask)

> +{
> +	struct as7343_data *data = iio_priv(indio_dev);
> +	struct device *dev = regmap_get_device(data->regmap);
> +	int ret;
> +	unsigned int unused;
> +	__le16 result;

Preserve reversed xmas tree order.

> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret)
> +		return ret;

Use PM_RUNTIME_ACQUIRE*().

> +	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(data->regmap, AS7343_ASTATUS, &unused);
> +		if (ret)
> +			break;
> +
> +		ret = regmap_bulk_read(data->regmap, chan->address, &result, 2);

sizeof()

> +		if (ret)
> +			break;
> +
> +		*val = le16_to_cpu(result);
> +		ret = IIO_VAL_INT;
> +		break;
> +	}
> +
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	pm_runtime_put(dev);
> +	return ret;
> +}

(All comments for the above function is what has been repeated in many
 contributions for sure.)

 ...

> +static const char *as7343_channel_label(struct iio_chan_spec const *chan)
> +{
> +	switch (chan->channel) {
> +	case AS7343_CHAN_IDX_FZ:
> +		return "FZ";
> +	case AS7343_CHAN_IDX_FY:
> +		return "FY";
> +	case AS7343_CHAN_IDX_FXL:
> +		return "FXL";
> +	case AS7343_CHAN_IDX_NIR:
> +		return "NIR";
> +	case AS7343_CHAN_IDX_F2:
> +		return "F2";
> +	case AS7343_CHAN_IDX_F3:
> +		return "F3";
> +	case AS7343_CHAN_IDX_F4:
> +		return "F4";
> +	case AS7343_CHAN_IDX_F6:
> +		return "F6";
> +	case AS7343_CHAN_IDX_F1:
> +		return "F1";
> +	case AS7343_CHAN_IDX_F7:
> +		return "F7";
> +	case AS7343_CHAN_IDX_F8:
> +		return "F8";
> +	case AS7343_CHAN_IDX_F5:
> +		return "F5";
> +	default:
> +		return NULL;
> +	}

Why not keeping this in a static array?

> +}
> +
> +static int as7343_read_label(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, char *label)
> +{
> +	const char *name;
> +
> +	name = as7343_channel_label(chan);
> +	if (!name)
> +		return -EINVAL;

Why? Can't it be taken from DT?

> +	return sysfs_emit(label, "%s\n", name);
> +}

...

> +static const struct regmap_config as7343_regmap_config = {
> +	.name = "as7343",
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	.max_register = AS7343_MAX,
> +	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
> +	.val_format_endian = REGMAP_ENDIAN_LITTLE,

> +	.cache_type = REGCACHE_NONE,

Why?! This needs a very good justification.

> +};

...

> +static int as7343_setup_device(struct device *dev, struct as7343_data *data)
> +{

	struct regmap *map = data->regmap;

will help to reduce verbosity of the below, and might even save some LoC...

> +	unsigned int val;
> +	__le16 step;
> +	int ret;
> +
> +	/* Power on */
> +	ret = regmap_set_bits(data->regmap, 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(data->regmap, AS7343_CFG0, AS7343_CFG0_REG_BANK);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_read(data->regmap, AS7343_ID, &val);
> +	if (ret)
> +		return ret;
> +
> +	if (val != 0x81)
> +		dev_info(dev, "Unknown device ID: %x\n", val);
> +
> +	ret = regmap_clear_bits(data->regmap, AS7343_CFG0,
> +				AS7343_CFG0_REG_BANK);

...for example, here:

	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(

Huh?! Please, check the formatting and indentation style.

> +		data->regmap, 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(data->regmap, AS7343_ASTEP, &step, 2);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(data->regmap, AS7343_ATIME, AS7343_ATIME_VAL);
> +	if (ret)
> +		return ret;
> +
> +	return regmap_update_bits(data->regmap, 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);
> +
> +	return regmap_clear_bits(data->regmap, 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);
> +
> +	return regmap_set_bits(data->regmap, AS7343_ENABLE,
> +			       AS7343_ENABLE_SP_EN);
> +}

Same, use temporary for struct regmap.

...

> +static int as7343_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct as7343_data *data;
> +	struct iio_dev *indio_dev;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	regmap = devm_regmap_init_i2c(client, &as7343_regmap_config);
> +	if (IS_ERR(regmap))
> +		return PTR_ERR(regmap);
> +
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->regmap = regmap;

> +	mutex_init(&data->mutex);

devm_mutex_init().

> +
> +	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(data->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);

Again, split logically. Options are:

static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops, as7343_suspend, as7343_resume, NULL);

static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops,
				 as7343_suspend, as7343_resume, NULL);

static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops,
				 as7343_suspend,
				 as7343_resume,
				 NULL);

(I personally prefer compromise as depicted in the second example).

...

> +static const struct of_device_id as7343_of_match[] = {
> +	{ .compatible = "ams,as7343" },
> +	{ },

No comma in the terminator entry.

> +};
> +MODULE_DEVICE_TABLE(of, as7343_of_match);
> +
> +static const struct i2c_device_id as7343_id[] = {
> +	{ .name = "as7343" },
> +	{ },

Ditto.

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

Indentation of the assignees with tabs makes it harder to maintain (in case
more lines got added it might require to reindent all of them).

> +};
> +module_i2c_driver(as7343_driver);

-- 
With Best Regards,
Andy Shevchenko

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

[Chang Yu]

Hi Andy

Thanks for the thorough review. Just one quick clarification inline.

On Thu, Sep 10, 2026 at 12:07:42PM +0300, Andy Shevchenko wrote:
> On Wed, Sep 09, 2026 at 11:38:13PM -0700, Chang Yu wrote:
> 
> ...
> 
> > +}
> > +
> > +static int as7343_read_label(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan, char *label)
> > +{
> > +	const char *name;
> > +
> > +	name = as7343_channel_label(chan);
> > +	if (!name)
> > +		return -EINVAL;
> 
> Why? Can't it be taken from DT?
> 

These channel labels correspond to the physical optical channel
names defined by the datasheet. Each name corresponds to a
pre-defined range of wavelengths. They are fixed internal properties
of the chip, so IMO hardcoding them in the driver is more
approriate.

> > +	return sysfs_emit(label, "%s\n", name);
> > +}
> 
> ...
> 

Best,
Chang

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

[Andy Shevchenko]

On Thu, Sep 10, 2026 at 12:22:01PM -0700, Chang Yu wrote:
> On Thu, Sep 10, 2026 at 12:07:42PM +0300, Andy Shevchenko wrote:
> > On Wed, Sep 09, 2026 at 11:38:13PM -0700, Chang Yu wrote:

...

> > > +	name = as7343_channel_label(chan);
> > > +	if (!name)
> > > +		return -EINVAL;
> > 
> > Why? Can't it be taken from DT?
> 
> These channel labels correspond to the physical optical channel
> names defined by the datasheet. Each name corresponds to a
> pre-defined range of wavelengths. They are fixed internal properties
> of the chip, so IMO hardcoding them in the driver is more
> approriate.

Please, make a comment on top of the string array, with probably citing the
datasheet table with the wavelength ranges.

/*
 * Channel names as defined in the datasheet:
 * // hypothetical example // ... - 200-300 nm
 * ...
 */
static const char * const ..._names[] = {
	...names...
};

-- 
With Best Regards,
Andy Shevchenko

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

[Krzysztof Kozlowski]

On Wed, Sep 09, 2026 at 11:38:12PM -0700, Chang Yu 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>
> ---
> 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.
> 

You need to address Sashiko review.

Best regards,
Krzysztof