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

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

The driver uses `extend_name` to differentiate channels, exposing
files like in_intensity_F1_raw for channel F1, etc. via sysfs.

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

Testing was performed on a Raspberry Pi 4.

Datasheet: [1]

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

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

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

-- 
2.55.0

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

Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
---
 .../bindings/iio/light/ams,as7343.yaml        | 46 +++++++++++++++++++
 1 file changed, 46 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..fb996160cf8d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
@@ -0,0 +1,46 @@
+# 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
+
+  vdd-supply: true
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        as73211@74 {
+            compatible = "ams,as7343";
+            reg = <0x39>;
+        };
+    };
-- 
2.55.0

[PATCH 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. More complex
features such as interrupt support and configurable gain/integration
time will be added in future patches.

Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
---
 MAINTAINERS                |   7 +
 drivers/iio/light/Kconfig  |  11 ++
 drivers/iio/light/Makefile |   1 +
 drivers/iio/light/as7343.c | 309 +++++++++++++++++++++++++++++++++++++
 4 files changed, 328 insertions(+)
 create mode 100644 drivers/iio/light/as7343.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 04fa5322d9f7..236102b3c28e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1430,6 +1430,13 @@ 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
+F:	drivers/iio/light/as7343.c
+
 AMT (Automatic Multicast Tunneling)
 M:	Taehee Yoo <ap420073@gmail.com>
 L:	netdev@vger.kernel.org
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..b620dd308380
--- /dev/null
+++ b/drivers/iio/light/as7343.c
@@ -0,0 +1,309 @@
+// 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:
+ *   - 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/regmap.h"
+#include <linux/bitfield.h>
+#include <linux/i2c.h>
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/stringify.h>
+
+#define AS7343_DRV_NAME "as7343"
+#define AS7343_DEVICE_ID 0x81
+
+/* AS7343 registers */
+#define AS7343_REG_ID 0x5a
+#define AS7343_REG_ENABLE 0x80
+#define AS7343_REG_ATIME 0x81
+#define AS7343_REG_CFG0 0xbf
+#define AS7343_REG_CFG1 0xc6
+#define AS7343_REG_CFG20 0xd6
+#define AS7343_REG_CONTROL 0xfa
+#define AS7343_REG_ASTATUS 0x94
+/* AS7343 data registers */
+#define AS7343_REG_DATA_FZ 0x95
+#define AS7343_REG_DATA_FY 0x97
+#define AS7343_REG_DATA_FXL 0x99
+#define AS7343_REG_DATA_NIR 0x9b
+#define AS7343_REG_DATA_F2 0xa1
+#define AS7343_REG_DATA_F3 0xa3
+#define AS7343_REG_DATA_F4 0xa5
+#define AS7343_REG_DATA_F6 0xa7
+#define AS7343_REG_DATA_F1 0xad
+#define AS7343_REG_DATA_F7 0xaf
+#define AS7343_REG_DATA_F8 0xb1
+#define AS7343_REG_DATA_F5 0xb3
+#define AS7343_REG_MAX 0xff
+
+/* AS7343 register bit masks */
+#define AS7343_ENABLE_PON BIT(0)
+#define AS7343_ENABLE_SP_EN BIT(1)
+#define AS7343_CFG0_REG_BANK BIT(4)
+#define AS7343_CFG20_AUTO_SMUX GENMASK(6, 5)
+#define AS7343_CONTROL_SW_RESET BIT(3)
+#define AS7343_CFG1_AGAIN GENMASK(4, 0)
+
+/* AS7343 settings */
+#define AS7343_INT_TIME 29 /* (29 + 1) * 2.87ms = 83.4ms */
+#define AS7343_GAIN 7 /* 64x gain */
+#define AS7343_AUTO_CHANNEL_READOUT 3 /* Automatic all-channel readout */
+
+/* AS7343 scan indices */
+#define AS7343_SCAN_INDEX_F1 0
+#define AS7343_SCAN_INDEX_F2 1
+#define AS7343_SCAN_INDEX_FZ 2
+#define AS7343_SCAN_INDEX_F3 3
+#define AS7343_SCAN_INDEX_F4 4
+#define AS7343_SCAN_INDEX_FY 5
+#define AS7343_SCAN_INDEX_F5 6
+#define AS7343_SCAN_INDEX_FXL 7
+#define AS7343_SCAN_INDEX_F6 8
+#define AS7343_SCAN_INDEX_F7 9
+#define AS7343_SCAN_INDEX_F8 10
+#define AS7343_SCAN_INDEX_NIR 11
+#define AS7343_SCAN_INDEX_TS 12
+
+#define AS7343_SCAN_MASK_ALL                                      \
+	(BIT(AS7343_SCAN_INDEX_F1) | BIT(AS7343_SCAN_INDEX_F2) |  \
+	 BIT(AS7343_SCAN_INDEX_FZ) | BIT(AS7343_SCAN_INDEX_F3) |  \
+	 BIT(AS7343_SCAN_INDEX_F4) | BIT(AS7343_SCAN_INDEX_FY) |  \
+	 BIT(AS7343_SCAN_INDEX_F5) | BIT(AS7343_SCAN_INDEX_FXL) | \
+	 BIT(AS7343_SCAN_INDEX_F6) | BIT(AS7343_SCAN_INDEX_F7) |  \
+	 BIT(AS7343_SCAN_INDEX_F8) | BIT(AS7343_SCAN_INDEX_NIR))
+
+static const unsigned long as7343_scan_masks[] = { AS7343_SCAN_MASK_ALL, 0 };
+
+#define AS7343_CHAN(_chan) \
+	{ \
+	.type = IIO_INTENSITY, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+	.address = AS7343_REG_DATA_##_chan, \
+	.extend_name = __stringify(_chan), \
+	.scan_index = AS7343_SCAN_INDEX_##_chan, \
+	.scan_type = { \
+		.sign = 'u', \
+		.realbits = 16, \
+		.storagebits = 16, \
+		.endianness = IIO_LE, \
+	}, \
+}
+
+static const struct iio_chan_spec as7343_channels[] = {
+	AS7343_CHAN(F1),
+	AS7343_CHAN(F2),
+	AS7343_CHAN(FZ),
+	AS7343_CHAN(F3),
+	AS7343_CHAN(F4),
+	AS7343_CHAN(FY),
+	AS7343_CHAN(F5),
+	AS7343_CHAN(FXL),
+	AS7343_CHAN(F6),
+	AS7343_CHAN(F7),
+	AS7343_CHAN(F8),
+	AS7343_CHAN(NIR),
+	IIO_CHAN_SOFT_TIMESTAMP(AS7343_SCAN_INDEX_TS),
+};
+
+/**
+ * struct as7343_data - Instance data for one AS7343
+ * @client: I2C client.
+ * @regmap: Register map.
+ */
+struct as7343_data {
+	struct i2c_client *client;
+	struct regmap *regmap;
+};
+
+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 low, high;
+	unsigned int unused;
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW: {
+		/* Reading ASTATUS latches all data registers to this read.
+		 * We don't care about the returned saturation/gain status for
+		 * now.
+		 */
+		ret = regmap_read(data->regmap, AS7343_REG_ASTATUS, &unused);
+		if (ret < 0)
+			return ret;
+
+		ret = regmap_read(data->regmap, chan->address, &low);
+		if (ret < 0)
+			return ret;
+		ret = regmap_read(data->regmap, chan->address + 1, &high);
+		if (ret < 0)
+			return ret;
+		*val = (high << 8) | low;
+		return IIO_VAL_INT;
+	}
+
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_info as7343_info = {
+	.read_raw = as7343_read_raw,
+};
+
+static const struct regmap_config as7343_regmap_config = {
+	.name = "as7343",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.max_register = AS7343_REG_MAX,
+	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.cache_type = REGCACHE_NONE,
+};
+
+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;
+	unsigned int val;
+	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->client = client;
+	data->regmap = regmap;
+
+	indio_dev->name = AS7343_DRV_NAME;
+	indio_dev->info = &as7343_info;
+	indio_dev->channels = as7343_channels;
+	indio_dev->num_channels = ARRAY_SIZE(as7343_channels);
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->available_scan_masks = as7343_scan_masks;
+
+	ret = devm_regulator_get_enable(&client->dev, "vdd");
+	if (ret < 0)
+		return ret;
+	/* Power on */
+	ret = regmap_set_bits(data->regmap, AS7343_REG_ENABLE,
+			      AS7343_ENABLE_PON);
+	if (ret < 0)
+		return ret;
+
+	/* Need to set REG_BANK to 1 before we can access ID */
+	ret = regmap_set_bits(data->regmap, AS7343_REG_CFG0,
+			      AS7343_CFG0_REG_BANK);
+	if (ret < 0)
+		return ret;
+	/* Check device ID */
+	ret = regmap_read(data->regmap, AS7343_REG_ID, &val);
+	if (val != AS7343_DEVICE_ID)
+		return -ENODEV;
+	/* Unset REG_BANK */
+	ret = regmap_clear_bits(data->regmap, AS7343_REG_CFG0,
+				AS7343_CFG0_REG_BANK);
+	if (ret < 0)
+		return ret;
+
+	/* Configure the SMUX to readout all channels */
+	ret = regmap_update_bits(data->regmap, AS7343_REG_CFG20,
+				 AS7343_CFG20_AUTO_SMUX,
+				 FIELD_PREP(AS7343_CFG20_AUTO_SMUX,
+					    AS7343_AUTO_CHANNEL_READOUT));
+	if (ret < 0)
+		return ret;
+
+	/* Set 83.4ms integration time and x64 gain for now */
+	ret = regmap_write(data->regmap, AS7343_REG_ATIME, AS7343_INT_TIME);
+	if (ret < 0)
+		return ret;
+	ret = regmap_update_bits(data->regmap, AS7343_REG_CFG1,
+				 AS7343_CFG1_AGAIN,
+				 FIELD_PREP(AS7343_CFG1_AGAIN, AS7343_GAIN));
+	if (ret < 0)
+		return ret;
+
+	/* Start measurements */
+	ret = regmap_set_bits(data->regmap, AS7343_REG_ENABLE,
+			      AS7343_ENABLE_SP_EN);
+	if (ret < 0)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static int as7343_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct as7343_data *data = iio_priv(indio_dev);
+
+	return regmap_clear_bits(data->regmap, AS7343_REG_ENABLE,
+				 AS7343_ENABLE_SP_EN);
+}
+
+static int as7343_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
+	struct as7343_data *data = iio_priv(indio_dev);
+
+	return regmap_set_bits(data->regmap, AS7343_REG_ENABLE,
+			       AS7343_ENABLE_SP_EN);
+}
+
+static DEFINE_SIMPLE_DEV_PM_OPS(as7343_pm_ops, as7343_suspend, as7343_resume);
+
+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_DRV_NAME,
+		.of_match_table = as7343_of_match,
+		.pm             = pm_sleep_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 2/2] iio: light: add AS7343 multi-spectral sensor driver

[Joshua Crofts]

Hi Chang,

Comments inline.

Josh

On Fri,  4 Sep 2026 22:53:26 -0700
Chang Yu <marcus.yu.56@gmail.com> 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. More complex
> features such as interrupt support and configurable gain/integration
> time will be added in future patches.
> 
> Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
> ---
>  MAINTAINERS                |   7 +
>  drivers/iio/light/Kconfig  |  11 ++
>  drivers/iio/light/Makefile |   1 +
>  drivers/iio/light/as7343.c | 309 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 328 insertions(+)
>  create mode 100644 drivers/iio/light/as7343.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 04fa5322d9f7..236102b3c28e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1430,6 +1430,13 @@ 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

This goes into the first patch in your series, i.e. the dt-binding
patch.

> +F:	drivers/iio/light/as7343.c

This line should be then added in this patch.

> +
>  AMT (Automatic Multicast Tunneling)
>  M:	Taehee Yoo <ap420073@gmail.com>
>  L:	netdev@vger.kernel.org
> 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..b620dd308380
> --- /dev/null
> +++ b/drivers/iio/light/as7343.c
> @@ -0,0 +1,309 @@
> +// 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:
> + *   - 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/regmap.h"

Why did you use quotation marks here? <linux/array_size.h>
and <linux/regmap.h> are valid.

> +#include <linux/bitfield.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/iio.h>

It's common to have the IIO headers separately below the
generic <linux/*> headers.

> +#include <linux/module.h>
> +#include <linux/pm.h>
> +#include <linux/stringify.h>

You're missing err.h, regulator/consumer.h,
> +
> +#define AS7343_DRV_NAME "as7343"
> +#define AS7343_DEVICE_ID 0x81
> +
> +/* AS7343 registers */
> +#define AS7343_REG_ID 0x5a
> +#define AS7343_REG_ENABLE 0x80
> +#define AS7343_REG_ATIME 0x81
> +#define AS7343_REG_CFG0 0xbf
> +#define AS7343_REG_CFG1 0xc6
> +#define AS7343_REG_CFG20 0xd6
> +#define AS7343_REG_CONTROL 0xfa
> +#define AS7343_REG_ASTATUS 0x94

Blank line here.

> +/* AS7343 data registers */
> +#define AS7343_REG_DATA_FZ 0x95
> +#define AS7343_REG_DATA_FY 0x97
> +#define AS7343_REG_DATA_FXL 0x99
> +#define AS7343_REG_DATA_NIR 0x9b
> +#define AS7343_REG_DATA_F2 0xa1
> +#define AS7343_REG_DATA_F3 0xa3
> +#define AS7343_REG_DATA_F4 0xa5
> +#define AS7343_REG_DATA_F6 0xa7
> +#define AS7343_REG_DATA_F1 0xad
> +#define AS7343_REG_DATA_F7 0xaf
> +#define AS7343_REG_DATA_F8 0xb1
> +#define AS7343_REG_DATA_F5 0xb3
> +#define AS7343_REG_MAX 0xff
> +
> +/* AS7343 register bit masks */
> +#define AS7343_ENABLE_PON BIT(0)
> +#define AS7343_ENABLE_SP_EN BIT(1)
> +#define AS7343_CFG0_REG_BANK BIT(4)
> +#define AS7343_CFG20_AUTO_SMUX GENMASK(6, 5)
> +#define AS7343_CONTROL_SW_RESET BIT(3)
> +#define AS7343_CFG1_AGAIN GENMASK(4, 0)
> +
> +/* AS7343 settings */
> +#define AS7343_INT_TIME 29 /* (29 + 1) * 2.87ms = 83.4ms */
> +#define AS7343_GAIN 7 /* 64x gain */
> +#define AS7343_AUTO_CHANNEL_READOUT 3 /* Automatic all-channel readout */
> +
> +/* AS7343 scan indices */
> +#define AS7343_SCAN_INDEX_F1 0
> +#define AS7343_SCAN_INDEX_F2 1
> +#define AS7343_SCAN_INDEX_FZ 2
> +#define AS7343_SCAN_INDEX_F3 3
> +#define AS7343_SCAN_INDEX_F4 4
> +#define AS7343_SCAN_INDEX_FY 5
> +#define AS7343_SCAN_INDEX_F5 6
> +#define AS7343_SCAN_INDEX_FXL 7
> +#define AS7343_SCAN_INDEX_F6 8
> +#define AS7343_SCAN_INDEX_F7 9
> +#define AS7343_SCAN_INDEX_F8 10
> +#define AS7343_SCAN_INDEX_NIR 11
> +#define AS7343_SCAN_INDEX_TS 12
> +
> +#define AS7343_SCAN_MASK_ALL                                      \
> +	(BIT(AS7343_SCAN_INDEX_F1) | BIT(AS7343_SCAN_INDEX_F2) |  \
> +	 BIT(AS7343_SCAN_INDEX_FZ) | BIT(AS7343_SCAN_INDEX_F3) |  \
> +	 BIT(AS7343_SCAN_INDEX_F4) | BIT(AS7343_SCAN_INDEX_FY) |  \
> +	 BIT(AS7343_SCAN_INDEX_F5) | BIT(AS7343_SCAN_INDEX_FXL) | \
> +	 BIT(AS7343_SCAN_INDEX_F6) | BIT(AS7343_SCAN_INDEX_F7) |  \
> +	 BIT(AS7343_SCAN_INDEX_F8) | BIT(AS7343_SCAN_INDEX_NIR))
> +
> +static const unsigned long as7343_scan_masks[] = { AS7343_SCAN_MASK_ALL, 0 };
> +
> +#define AS7343_CHAN(_chan) \
> +	{ \
> +	.type = IIO_INTENSITY, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.address = AS7343_REG_DATA_##_chan, \
> +	.extend_name = __stringify(_chan), \
> +	.scan_index = AS7343_SCAN_INDEX_##_chan, \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +		.endianness = IIO_LE, \
> +	}, \
> +}
> +
> +static const struct iio_chan_spec as7343_channels[] = {
> +	AS7343_CHAN(F1),
> +	AS7343_CHAN(F2),
> +	AS7343_CHAN(FZ),
> +	AS7343_CHAN(F3),
> +	AS7343_CHAN(F4),
> +	AS7343_CHAN(FY),
> +	AS7343_CHAN(F5),
> +	AS7343_CHAN(FXL),
> +	AS7343_CHAN(F6),
> +	AS7343_CHAN(F7),
> +	AS7343_CHAN(F8),
> +	AS7343_CHAN(NIR),

Don't forget to update any mentions of only 12 channels being
implemented once you add support for the Flicker and VIS channel.

> +	IIO_CHAN_SOFT_TIMESTAMP(AS7343_SCAN_INDEX_TS),
> +};
> +
> +/**
> + * struct as7343_data - Instance data for one AS7343
> + * @client: I2C client.
> + * @regmap: Register map.
> + */

Eh, I'd remove the comment for now as the contents of the struct
are pretty self explanatory.

> +struct as7343_data {
> +	struct i2c_client *client;
> +	struct regmap *regmap;
> +};
> +
> +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 low, high;
> +	unsigned int unused;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		/* Reading ASTATUS latches all data registers to this read.
> +		 * We don't care about the returned saturation/gain status for
> +		 * now.
> +		 */
> +		ret = regmap_read(data->regmap, AS7343_REG_ASTATUS, &unused);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = regmap_read(data->regmap, chan->address, &low);
> +		if (ret < 0)
> +			return ret;

Blank line here.

> +		ret = regmap_read(data->regmap, chan->address + 1, &high);
> +		if (ret < 0)
> +			return ret;
> +		*val = (high << 8) | low;
> +		return IIO_VAL_INT;
> +	}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +

...

> +	/* Need to set REG_BANK to 1 before we can access ID */
> +	ret = regmap_set_bits(data->regmap, AS7343_REG_CFG0,
> +			      AS7343_CFG0_REG_BANK);
> +	if (ret < 0)
> +		return ret;
> +	/* Check device ID */

Unnecessary comment, add a blank line instead.

> +	ret = regmap_read(data->regmap, AS7343_REG_ID, &val);
> +	if (val != AS7343_DEVICE_ID)
> +		return -ENODEV;

It's better to just do a dev_warn() in case of any fallback
devices instead of a hard return.

> +	/* Unset REG_BANK */

Unnecessary comment.

> +	ret = regmap_clear_bits(data->regmap, AS7343_REG_CFG0,
> +				AS7343_CFG0_REG_BANK);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Configure the SMUX to readout all channels */
> +	ret = regmap_update_bits(data->regmap, AS7343_REG_CFG20,
> +				 AS7343_CFG20_AUTO_SMUX,
> +				 FIELD_PREP(AS7343_CFG20_AUTO_SMUX,
> +					    AS7343_AUTO_CHANNEL_READOUT));
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set 83.4ms integration time and x64 gain for now */

Good for an initial draft, however you'll definitely have to
implement the write function for this to get merged into mainline.
Skimming the datasheet shows that there are more integration times
possible, not to mention that you can also set the gain etc.

> +	ret = regmap_write(data->regmap, AS7343_REG_ATIME, AS7343_INT_TIME);
> +	if (ret < 0)
> +		return ret;

Blank line.

> +	ret = regmap_update_bits(data->regmap, AS7343_REG_CFG1,
> +				 AS7343_CFG1_AGAIN,
> +				 FIELD_PREP(AS7343_CFG1_AGAIN, AS7343_GAIN));
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Start measurements */
> +	ret = regmap_set_bits(data->regmap, AS7343_REG_ENABLE,
> +			      AS7343_ENABLE_SP_EN);
> +	if (ret < 0)
> +		return ret;
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static int as7343_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct as7343_data *data = iio_priv(indio_dev);
> +
> +	return regmap_clear_bits(data->regmap, AS7343_REG_ENABLE,
> +				 AS7343_ENABLE_SP_EN);
> +}
> +
> +static int as7343_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct as7343_data *data = iio_priv(indio_dev);
> +
> +	return regmap_set_bits(data->regmap, AS7343_REG_ENABLE,
> +			       AS7343_ENABLE_SP_EN);
> +}
> +

You have suspend/resume functions, yet you're missing a
devm_pm_runtime_enable() in probe. Additionally, you could enable
the autosuspend function as well (note, you'll have to wake the
device before reading, there are macros that simplify this though,
see PM_RUNTIME_ACQUIRE_AUTOSUSPEND)

Also, there isn't any devm_add_action_or_reset() function that
guarantees powering the device off on teardown (you can reuse your
suspend function as the callback).

> +static DEFINE_SIMPLE_DEV_PM_OPS(as7343_pm_ops, as7343_suspend, as7343_resume);
> +
> +static const struct of_device_id as7343_of_match[] = {
> +	{ .compatible = "ams,as7343" },
> +	{},

Remove the comma + add a space in between the {}

Same goes for the struct i2c_device_id definition.

-- 
Kind regards,
Joshua Crofts

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

[Krzysztof Kozlowski]

On 05/09/2026 07:53, Chang Yu wrote:
> Add binding for AMS AS7343 which is a 14-channel multi-spectral sensor
> with i2c address of 0x39.
> 
> Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>


Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline) or work on fork of kernel
(don't, instead use mainline). Just use b4 and everything should be
fine, although remember about `b4 prep --auto-to-cc` if you added new
patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling. Performing review on untested code might be
a waste of time.

Please kindly resend and include all necessary To/Cc entries.

Best regards,
Krzysztof

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

[Andy Shevchenko]

On Fri, Sep 04, 2026 at 10:53:24PM -0700, Chang Yu wrote:

...

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

This is a Datasheet link, no need to have three lines for it, just

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

-- 
With Best Regards,
Andy Shevchenko

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

[Jonathan Cameron]

On 2026-09-05 10:51:48+03:00, Andy Shevchenko wrote:
> On Fri, Sep 04, 2026 at 10:53:24PM -0700, Chang Yu wrote:
> 
> ...
> 
> > Datasheet: [1]
> > 
> > Link: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf [1]
> 
> This is a Datasheet link, no need to have three lines for it, just
> 
> Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf

In addition to this I'd like to see the tag in the driver commit tag
block. That is more useful than having it in the cover letter. 

Thanks,

Jonathan

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

[Jonathan Cameron]

On Sat, 5 Sep 2026 08:42:58 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> Hi Chang,
> 
> Comments inline.
> 
> Josh
> 
> On Fri,  4 Sep 2026 22:53:26 -0700
> Chang Yu <marcus.yu.56@gmail.com> 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. More complex
> > features such as interrupt support and configurable gain/integration
> > time will be added in future patches.
> > 
> > Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>

Nice review.  A few small comments on necessity of features etc
inline.

> > diff --git a/drivers/iio/light/as7343.c b/drivers/iio/light/as7343.c
> > new file mode 100644
> > index 000000000000..b620dd308380
> > --- /dev/null
> > +++ b/drivers/iio/light/as7343.c

> > +	/* Need to set REG_BANK to 1 before we can access ID */
> > +	ret = regmap_set_bits(data->regmap, AS7343_REG_CFG0,
> > +			      AS7343_CFG0_REG_BANK);
> > +	if (ret < 0)
> > +		return ret;
> > +	/* Check device ID */  

...

> 
> > +	ret = regmap_read(data->regmap, AS7343_REG_ID, &val);
> > +	if (val != AS7343_DEVICE_ID)
> > +		return -ENODEV;  
> 
> It's better to just do a dev_warn() in case of any fallback
> devices instead of a hard return.

dev_info(dev, "Unknown device ID: %x\n", val);

or something like this.  In many cases a mismatch is not an error, it is a
fallback device tree compatible being used.  This is common when a manufacturer
issues a new device that is a refresh of an older design and so interface
compatible but with a different device ID.
 

...

> > +
> > +	/* Set 83.4ms integration time and x64 gain for now */  
> 
> Good for an initial draft, however you'll definitely have to
> implement the write function for this to get merged into mainline.
> Skimming the datasheet shows that there are more integration times
> possible, not to mention that you can also set the gain etc.

If there is a sensible default / initial value that works most of the time
(short value probably to avoid saturation) then controlling this isn't
a requirement for merge.  It's a nice to have though!

...

> > +
> > +static int as7343_suspend(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > +	struct as7343_data *data = iio_priv(indio_dev);
> > +
> > +	return regmap_clear_bits(data->regmap, AS7343_REG_ENABLE,
> > +				 AS7343_ENABLE_SP_EN);
> > +}
> > +
> > +static int as7343_resume(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > +	struct as7343_data *data = iio_priv(indio_dev);
> > +
> > +	return regmap_set_bits(data->regmap, AS7343_REG_ENABLE,
> > +			       AS7343_ENABLE_SP_EN);
> > +}
> > +  
> 
> You have suspend/resume functions, yet you're missing a
> devm_pm_runtime_enable() in probe. 

There is not requirement to do any specific combination of power management
for an IIO driver because what is necessary is very dependent on the usecase
a particular developer has. So runtime pm is a nice to have only (as is the
suspend / resume stuff we have here).  May well make sense to use the same
for both types (there are macros to ensure that).

> Additionally, you could enable
> the autosuspend function as well (note, you'll have to wake the
> device before reading, there are macros that simplify this though,
> see PM_RUNTIME_ACQUIRE_AUTOSUSPEND)

All nice to haves indeed - but not strictly necessary. Many drivers
don't go that far initially and it is fairly easy to retrofit this stuff
if someone cares.

Jonathan

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

[Jonathan Cameron]

> Add binding for AMS AS7343 which is a 14-channel multi-spectral sensor
> with i2c address of 0x39.
> 
> Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>

Given you need to send again to include right folk for testing and
review here are a few initial things to fix up and make it a proper
v2

>
> 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..fb996160cf8d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
> @@ -0,0 +1,46 @@
> +# 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
> +
> +  vdd-supply: true

Power supplies that are needed in practice for a device to function
are always added to the required list (there are some historical
quirks in some bindings).  We do that even though the Linux regulator
framework will hand you a fake supply if you don't provide one via
the dt on basis the power is presumably always turned on.  The reason
for this is otherwise we can't see when we have a truely optional
power supply (common for references where there may be a number of
alternative sources such as internal voltage references).

I'd expect some binding info about whether LDR is connected to
anything or not. (LED Driver - who picked a common term like LDR
which is normally Light Dependent Resistor?!!)  We need the driver
to be able to decide to turn that on or off + set the drive
current.  Look at proximity sensors for examples of ways this might
be done.

There is also an interrupt pin that definitely wants to
be documented.  Bindings should be as full as possible.  The only
time we leave stuff out is when we have no idea how to write
the binding - that happens for sufficiently unusual corners that
aren't true for this.

The gpio is more interesting but at lest one option is to use
it as just that, so I'd expect to see binding for that.  The
sync stuff is perhaps an area where the binding design may be
less obvious and postponing that (mention it in the patch description)
might be the best path forwards.

> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>

Sashiko:
[Severity: Low]
Is it necessary to include irq.h here? The example does not appear to
define or use any interrupts.
-

Seems like a valid thing to clean up!
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        as73211@74 {

Should be a generic name not the part number.  If there isn't a
suitable one in the dt specification or in general use, make up
a new one.  Maybe multispectral is suitable here - I haven't really
thought about it much!

> +            compatible = "ams,as7343";
> +            reg = <0x39>;

Sashiko:
[Severity: Low]
Does the unit address (@74) mismatch the first address in the reg property
(<0x39>)? This might trigger a unit_address_vs_reg warning during
dt_binding_check and dtc compilation.

Also, should the node name use a generic functional name (such as
sensor@39) instead of referring to a different device (as73211)?
-


> +        };
> +    };

Thanks,

Jonathan

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

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

[Jonathan Cameron]

> 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. More complex
> features such as interrupt support and configurable gain/integration
> time will be added in future patches.
> 
> Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
Hi Chang Yu,

I've avoided too much duplication with Joshua's already pretty
thorough review so just a few additional comments inline.


> diff --git a/drivers/iio/light/as7343.c b/drivers/iio/light/as7343.c
> new file mode 100644
> index 000000000000..b620dd308380
> --- /dev/null
> +++ b/drivers/iio/light/as7343.c

...

> +
> +#define AS7343_DRV_NAME "as7343"
> +#define AS7343_DEVICE_ID 0x81

For now I'd just put that inline.  If you end up with additional
supported parts it will end up in a chip_info structure of some kind.

> +
> +/* AS7343 registers */
> +#define AS7343_REG_ID 0x5a
> +#define AS7343_REG_ENABLE 0x80
> +#define AS7343_REG_ATIME 0x81
> +#define AS7343_REG_CFG0 0xbf
> +#define AS7343_REG_CFG1 0xc6
> +#define AS7343_REG_CFG20 0xd6

It is useful practice to document a register address its fields
and their values all in one place. If you look at other drivers
we often do that using some indentiation. E.g.


This keeps the register map easy to read and the names all make
it very obvious if writes are to the wrong register or similar.

> +#define AS7343_REG_CONTROL 0xfa
> +#define AS7343_REG_ASTATUS 0x94


> +/* AS7343 data registers */
> +#define AS7343_REG_DATA_FZ 0x95
> +#define AS7343_REG_DATA_FY 0x97
> +#define AS7343_REG_DATA_FXL 0x99
> +#define AS7343_REG_DATA_NIR 0x9b
> +#define AS7343_REG_DATA_F2 0xa1
> +#define AS7343_REG_DATA_F3 0xa3
> +#define AS7343_REG_DATA_F4 0xa5
> +#define AS7343_REG_DATA_F6 0xa7
> +#define AS7343_REG_DATA_F1 0xad
> +#define AS7343_REG_DATA_F7 0xaf
> +#define AS7343_REG_DATA_F8 0xb1
> +#define AS7343_REG_DATA_F5 0xb3
> +#define AS7343_REG_MAX 0xff
> +
> +/* AS7343 register bit masks */
> +#define AS7343_ENABLE_PON BIT(0)
> +#define AS7343_ENABLE_SP_EN BIT(1)
> +#define AS7343_CFG0_REG_BANK BIT(4)
> +#define AS7343_CFG20_AUTO_SMUX GENMASK(6, 5)
> +#define AS7343_CONTROL_SW_RESET BIT(3)
> +#define AS7343_CFG1_AGAIN GENMASK(4, 0)
> +
> +/* AS7343 settings */
> +#define AS7343_INT_TIME 29 /* (29 + 1) * 2.87ms = 83.4ms */

Implement this as a function to do the maths and take the input in
usecs.  Then you can call that with 83400 as the parameter to set the
default value.  Why this default?

> +#define AS7343_GAIN 7 /* 64x gain */

This is a field value, so naming should refect that.
AS7343_CFG1_AGAIN_X64 or something like that. May well make sense
to just specify all the possible field values by define - or replace
them with function given they are powers of 2 I think (be it -1 as
the first one.


> +#define AS7343_AUTO_CHANNEL_READOUT 3 /* Automatic all-channel readout */
This is a field value in AS7343_CFG20_AUTO_SMUX so naming
should reflect that.
or something like that.

> +
> +/* AS7343 scan indices */
> +#define AS7343_SCAN_INDEX_F1 0
> +#define AS7343_SCAN_INDEX_F2 1
> +#define AS7343_SCAN_INDEX_FZ 2
> +#define AS7343_SCAN_INDEX_F3 3
> +#define AS7343_SCAN_INDEX_F4 4
> +#define AS7343_SCAN_INDEX_FY 5
> +#define AS7343_SCAN_INDEX_F5 6
> +#define AS7343_SCAN_INDEX_FXL 7
> +#define AS7343_SCAN_INDEX_F6 8
> +#define AS7343_SCAN_INDEX_F7 9
> +#define AS7343_SCAN_INDEX_F8 10
> +#define AS7343_SCAN_INDEX_NIR 11
> +#define AS7343_SCAN_INDEX_TS 12
Probably an enum is appropriate.  Not used for now though so drop.
> +
> +#define AS7343_SCAN_MASK_ALL                                      \
> +	(BIT(AS7343_SCAN_INDEX_F1) | BIT(AS7343_SCAN_INDEX_F2) |  \
> +	 BIT(AS7343_SCAN_INDEX_FZ) | BIT(AS7343_SCAN_INDEX_F3) |  \
> +	 BIT(AS7343_SCAN_INDEX_F4) | BIT(AS7343_SCAN_INDEX_FY) |  \
> +	 BIT(AS7343_SCAN_INDEX_F5) | BIT(AS7343_SCAN_INDEX_FXL) | \
> +	 BIT(AS7343_SCAN_INDEX_F6) | BIT(AS7343_SCAN_INDEX_F7) |  \
> +	 BIT(AS7343_SCAN_INDEX_F8) | BIT(AS7343_SCAN_INDEX_NIR))
> +
> +static const unsigned long as7343_scan_masks[] = { AS7343_SCAN_MASK_ALL, 0 };

As below. This should only come in when or if you add buffered
data capture. Today it is meaningless so tremove it and the MASK_ALL
definition.

> +
> +#define AS7343_CHAN(_chan) \
> +	{ \
> +	.type = IIO_INTENSITY, \
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> +	.address = AS7343_REG_DATA_##_chan, \
> +	.extend_name = __stringify(_chan), \

No for this - we no longer use extend_name.  It was a terrible bit
of ABI design a long time back.  Instead use channel labels to provide
in_intensityX_label with a suitable string.

> +	.scan_index = AS7343_SCAN_INDEX_##_chan, \
> +	.scan_type = { \
> +		.sign = 'u', \
> +		.realbits = 16, \
> +		.storagebits = 16, \
> +		.endianness = IIO_LE, \
> +	}, \
You aren't implementing any buffered stuff so you definitely
aren't using all of this scan stuff. 
Only specify values of things that you actually use.
> +}
> +
> +static const struct iio_chan_spec as7343_channels[] = {
> +	AS7343_CHAN(F1),
> +	AS7343_CHAN(F2),
> +	AS7343_CHAN(FZ),
> +	AS7343_CHAN(F3),
> +	AS7343_CHAN(F4),
> +	AS7343_CHAN(FY),
> +	AS7343_CHAN(F5),
> +	AS7343_CHAN(FXL),
> +	AS7343_CHAN(F6),
> +	AS7343_CHAN(F7),
> +	AS7343_CHAN(F8),
> +	AS7343_CHAN(NIR),


> +	IIO_CHAN_SOFT_TIMESTAMP(AS7343_SCAN_INDEX_TS),
> +};

...

> +struct as7343_data {
> +	struct i2c_client *client;

Not used that I can see - so drop client from this.

> +	struct regmap *regmap;
> +};
> +
> +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 low, high;
> +	unsigned int unused;
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW: {
> +		/* Reading ASTATUS latches all data registers to this read.

Wrong comment style - see all the other IIO drivers.
Note only net and a few other places use this style in the kernel.

> +		 * We don't care about the returned saturation/gain status for
> +		 * now.
> +		 */
> +		ret = regmap_read(data->regmap, AS7343_REG_ASTATUS, &unused);
> +		if (ret < 0)
As below, for regmap calls prefer
		if (ret)
			return ret;
			
> +			return ret;
> +
> +		ret = regmap_read(data->regmap, chan->address, &low);
> +		if (ret < 0)
> +			return ret;


> +		ret = regmap_read(data->regmap, chan->address + 1, &high);
> +		if (ret < 0)
> +			return ret;

No option to do a bulk read?  I'd rather see one of those combined
with an appropriate endian conversion. The datasheet mentions auto
increment so that regmap_bulk_read() should just work I think.

> +		*val = (high << 8) | low;
> +		return IIO_VAL_INT;
> +	}
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}


...

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

It is a big enough register map that it may make sense to use
regcache and provide all the info on what is volatile etc.

> +};
> +
> +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;
> +	unsigned int val;
> +	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->client = client;

This rings alarm bells given you also have a regmap.  Turns
out you don't use it - so drop this from data.

> +	data->regmap = regmap;
> +
> +	indio_dev->name = AS7343_DRV_NAME;

Use the string directly here. There is no particular reason it should
be the same as the driver naming, so I'd much rather see what the
value is here.

> +	indio_dev->info = &as7343_info;
> +	indio_dev->channels = as7343_channels;
> +	indio_dev->num_channels = ARRAY_SIZE(as7343_channels);
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->available_scan_masks = as7343_scan_masks;

This is only relevant if you are doing buffered outputs, which
you aren't. So it's unused. Remove it.
> +
> +	ret = devm_regulator_get_enable(&client->dev, "vdd");
> +	if (ret < 0)
> +		return ret;
> +	/* Power on */
> +	ret = regmap_set_bits(data->regmap, AS7343_REG_ENABLE,
> +			      AS7343_ENABLE_PON);
> +	if (ret < 0)

regmap only uses 0 for sucess and negative for failure, so
where they are regmap calls I'd prefer
	if (ret)
		return ret;

That makes some places where you can do
	return regmap*();
at the end of a function the same as all the places you have
to check it before carrying on.

> +		return ret;
> +

...

> +static int as7343_suspend(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> +	struct as7343_data *data = iio_priv(indio_dev);

As below.

> +
> +	return regmap_clear_bits(data->regmap, AS7343_REG_ENABLE,
> +				 AS7343_ENABLE_SP_EN);
> +}
> +
> +static int as7343_resume(struct device *dev)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));

	struct iio_dev *indio_dev = dev_get_drvdata(dev);

Whilst a little odd given you use i2c_set_clientdata() to set it up
this pattern is well enough known by reviewers that we do it
this way anyway to sae on complexity of going backwards and forwards
via the i2c client.

> +	struct as7343_data *data = iio_priv(indio_dev);
> +
> +	return regmap_set_bits(data->regmap, AS7343_REG_ENABLE,
> +			       AS7343_ENABLE_SP_EN);
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(as7343_pm_ops, as7343_suspend, as7343_resume);


> +};
> +MODULE_DEVICE_TABLE(i2c, as7343_id);
> +
> +static struct i2c_driver as7343_driver = {
> +	.driver = {
> +		.name           = AS7343_DRV_NAME,

As above, just put the string here.  Only use macros for things
like this when they help readability or enforce necessary matching
between multiple locations.

-- 
Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

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

[Chang Yu]

Hi Jonathan and Joshua,

Thanks for the thorough review. Just adding some clarifying comments inline
for what I plan to do in v2. I should be able to send over v2 within a few
business days.

On Sun, Sep 06, 2026 at 01:11:45AM +0100, Jonathan Cameron wrote:
> On Sat, 5 Sep 2026 08:42:58 +0200
> Joshua Crofts <joshua.crofts1@gmail.com> wrote:
> 
> > Hi Chang,
> > 
> > Comments inline.
> > 
> > Josh
> > 
> > On Fri,  4 Sep 2026 22:53:26 -0700
> > Chang Yu <marcus.yu.56@gmail.com> wrote:
> > 
> > > This patch adds a driver for the AMS AS7343 14-channel multi-spectral
> > > sensor with I2C interface.
> 
> ...
> 
> > > +
> > > +	/* Set 83.4ms integration time and x64 gain for now */  
> > 
> > Good for an initial draft, however you'll definitely have to
> > implement the write function for this to get merged into mainline.
> > Skimming the datasheet shows that there are more integration times
> > possible, not to mention that you can also set the gain etc.
> 
> If there is a sensible default / initial value that works most of the time
> (short value probably to avoid saturation) then controlling this isn't
> a requirement for merge.  It's a nice to have though!

I'll defer controlling integration/gain to future patches then. x256
gain and 50.1ms integration test are the defaults recommended by the
datasheet. It is also what adafruit uses in their arduino driver
(https://github.com/adafruit/Adafruit_AS7343/blob/main/Adafruit_AS7343.cpp).
They also seem to work well enough when I was testing on hardware.
So I'll use those values in v2 for now.

> 
> ...
> 
> > > +
> > > +static int as7343_suspend(struct device *dev)
> > > +{
> > > +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > > +	struct as7343_data *data = iio_priv(indio_dev);
> > > +
> > > +	return regmap_clear_bits(data->regmap, AS7343_REG_ENABLE,
> > > +				 AS7343_ENABLE_SP_EN);
> > > +}
> > > +
> > > +static int as7343_resume(struct device *dev)
> > > +{
> > > +	struct iio_dev *indio_dev = i2c_get_clientdata(to_i2c_client(dev));
> > > +	struct as7343_data *data = iio_priv(indio_dev);
> > > +
> > > +	return regmap_set_bits(data->regmap, AS7343_REG_ENABLE,
> > > +			       AS7343_ENABLE_SP_EN);
> > > +}
> > > +  
> > 
> > You have suspend/resume functions, yet you're missing a
> > devm_pm_runtime_enable() in probe. 
> 
> There is not requirement to do any specific combination of power management
> for an IIO driver because what is necessary is very dependent on the usecase
> a particular developer has. So runtime pm is a nice to have only (as is the
> suspend / resume stuff we have here).  May well make sense to use the same
> for both types (there are macros to ensure that).
> 
> > Additionally, you could enable
> > the autosuspend function as well (note, you'll have to wake the
> > device before reading, there are macros that simplify this though,
> > see PM_RUNTIME_ACQUIRE_AUTOSUSPEND)
> 
> All nice to haves indeed - but not strictly necessary. Many drivers
> don't go that far initially and it is fairly easy to retrofit this stuff
> if someone cares.
> 
I'll fix up the suspend/resume stuff per Joshua's comments. But I'll
defer autosuspend to future patches. I'll mention this in the v2 patch as well.

Best,
Chang

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

[Chang Yu]

Hi Jonathan,

Thanks for the thorough review. Just some clarifying comments in line. I
should be able to send v2 over within a few business days.

On Sun, Sep 06, 2026 at 01:30:39AM +0100, Jonathan Cameron 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. More complex
> > features such as interrupt support and configurable gain/integration
> > time will be added in future patches.
> > 
> > Signed-off-by: Chang Yu <marcus.yu.56@gmail.com>
> Hi Chang Yu,
> 
> I've avoided too much duplication with Joshua's already pretty
> thorough review so just a few additional comments inline.
> 
> 
> > diff --git a/drivers/iio/light/as7343.c b/drivers/iio/light/as7343.c
> > new file mode 100644
> > index 000000000000..b620dd308380
> > --- /dev/null
> > +++ b/drivers/iio/light/as7343.c
> 
> ...
> 
> > +/* AS7343 register bit masks */
> > +#define AS7343_ENABLE_PON BIT(0)
> > +#define AS7343_ENABLE_SP_EN BIT(1)
> > +#define AS7343_CFG0_REG_BANK BIT(4)
> > +#define AS7343_CFG20_AUTO_SMUX GENMASK(6, 5)
> > +#define AS7343_CONTROL_SW_RESET BIT(3)
> > +#define AS7343_CFG1_AGAIN GENMASK(4, 0)
> > +
> > +/* AS7343 settings */
> > +#define AS7343_INT_TIME 29 /* (29 + 1) * 2.87ms = 83.4ms */
> 
> Implement this as a function to do the maths and take the input in
> usecs.  Then you can call that with 83400 as the parameter to set the
> default value.  Why this default?
> 
Annoyingly for this sensor the integration time is calculated from two
registers: (ATIME + 1) * (ASTEP + 1) * 2.78us. For simplicity, I'll use
define for these two values in v2 instead of a math function.

I'll adjust the default values. The defaults in v1 are just some random
values chosen by me. In v2 I'll change them to the official recommended
values in the datasheet (x256 gain and 50.1ms integration time). These
are also the values used by adafruit in their arduino driver.
(https://github.com/adafruit/Adafruit_AS7343/blob/main/Adafruit_AS7343.cpp)

> 
> ...
> 
> > +static const struct regmap_config as7343_regmap_config = {
> > +	.name = "as7343",
> > +	.reg_bits = 8,
> > +	.val_bits = 8,
> > +	.max_register = AS7343_REG_MAX,
> > +	.reg_format_endian = REGMAP_ENDIAN_LITTLE,
> > +	.val_format_endian = REGMAP_ENDIAN_LITTLE,
> > +	.cache_type = REGCACHE_NONE,
> 
> It is a big enough register map that it may make sense to use
> regcache and provide all the info on what is volatile etc.
> 

I'm debating whether this is worth it or not. All data registers
are volatile. Plus ENABLE because we need power management.
Potentially ASTEP, ATIME, CFG1 as well if we want configurable
gain/integration test in the future. That leaves us with may be
1 or 2 registers in the mapping that are not volatile. I'm leaning
towards leaving this as REGCACHE_NONE for now. Let me know what you
think.

> -- 
> Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

Best,
Chang