Add support for AS7343 multi-spectral sensor
Subsystems (2)
cover Add support for AS7343 multi-spectral sensor
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]
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
Signature · 1 line
2.55.0
PATCH 1/2 dt-bindings: iio: light: add as7343
Add binding for AMS AS7343 which is a 14-channel multi-spectral sensor with i2c address of 0x39.
--- .../bindings/iio/light/ams,as7343.yaml | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +46 -0
@@ -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 <[email protected]> + +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>; + }; + };
Signature · 1 line
2.55.0
PATCH 2/2 iio: light: add AS7343 multi-spectral sensor driver
In reply to Chang YuThis 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.
--- 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
MAINTAINERS +7 -0
@@ -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 <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/iio/light/ams,as7343.yaml +F: drivers/iio/light/as7343.c + AMT (Automatic Multicast Tunneling) M: Taehee Yoo <[email protected]> L: [email protected]
drivers/iio/light/Kconfig +11 -0
@@ -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
drivers/iio/light/Makefile +1 -0
@@ -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
drivers/iio/light/as7343.c new file +309 -0
309 lines not on this page · raw message
Signature · 1 line
2.55.0
Re: [PATCH 2/2] iio: light: add AS7343 multi-spectral sensor driver
Hi Chang,
Comments inline.
Josh
Quoted 25 lines · On Fri, 4 Sep 2026 22:53:26 -0700 Chang Yu <[email protected]> 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 <[email protected]>--- 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.cMAINTAINERS +0
-0
MAINTAINERS S: Maintained · line 1437
Quoted 8 lines
MAINTAINERS +5
-0@@ -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 <[email protected]> +L: [email protected] +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.
MAINTAINERS S: Maintained · line 1438
Quoted 1 line
MAINTAINERS +1
-0+F: drivers/iio/light/as7343.c
This line should be then added in this patch.
Quoted 56 lines
MAINTAINERS +1
-0+ AMT (Automatic Multicast Tunneling) M: Taehee Yoo <[email protected]> L: [email protected]drivers/iio/light/Kconfig +11
-0@@ -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 I2Cdrivers/iio/light/Makefile +1
-0@@ -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.odrivers/iio/light/as7343.c new file +12
-0@@ -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 <[email protected]> (AS73211 driver) +* +* Copyright (c) 2026 Chang Yu <[email protected]> +* +* Datasheets: +* https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
drivers/iio/light/as7343.c line 22
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+* +* 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.
drivers/iio/light/as7343.c line 25
Quoted 3 lines
drivers/iio/light/as7343.c new file +3
-0+#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.
drivers/iio/light/as7343.c line 28
Quoted 3 lines
drivers/iio/light/as7343.c new file +3
-0+#include <linux/module.h> +#include <linux/pm.h> +#include <linux/stringify.h>
You're missing err.h, regulator/consumer.h,
drivers/iio/light/as7343.c line 41
Quoted 13 lines
drivers/iio/light/as7343.c new file +13
-0+ +#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.
Quoted 71 lines
drivers/iio/light/as7343.c new file +71
-0+/* 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 ),
drivers/iio/light/as7343.c line 122
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+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.
drivers/iio/light/as7343.c line 130
Quoted 8 lines
drivers/iio/light/as7343.c new file +8
-0+ 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.
Quoted 17 lines
drivers/iio/light/as7343.c new file +17
-0+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.
drivers/iio/light/as7343.c line 157
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+* 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.
Quoted 12 lines
drivers/iio/light/as7343.c new file +12
-0+ 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 ; + } +} +
...
drivers/iio/light/as7343.c line 227
Quoted 6 lines
drivers/iio/light/as7343.c new file +6
-0+/* 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.
drivers/iio/light/as7343.c line 230
Quoted 3 lines
drivers/iio/light/as7343.c new file +3
-0+ 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.
drivers/iio/light/as7343.c line 231
Quoted 1 line
drivers/iio/light/as7343.c new file +1
-0+/* Unset REG_BANK */
Unnecessary comment.
Quoted 4 lines
drivers/iio/light/as7343.c new file +4
-0+ ret =regmap_clear_bits (data->regmap,AS7343_REG_CFG0 , +AS7343_CFG0_REG_BANK ); +if (ret <0 ) +return ret;
drivers/iio/light/as7343.c line 245
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+ +/* 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.
drivers/iio/light/as7343.c line 248
Quoted 3 lines
drivers/iio/light/as7343.c new file +3
-0+ ret =regmap_write (data->regmap,AS7343_REG_ATIME ,AS7343_INT_TIME ); +if (ret <0 ) +return ret;
Blank line.
Quoted 23 lines
drivers/iio/light/as7343.c new file +23
-0+ 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 ); +}
drivers/iio/light/as7343.c line 281
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+ +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).
drivers/iio/light/as7343.c line 286
Quoted 5 lines
drivers/iio/light/as7343.c new file +5
-0+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.
Signature · 2 lines
Kind regards, Joshua Crofts
Re: [PATCH 1/2] dt-bindings: iio: light: add as7343
In reply to Chang YuQuoted 4 lines · 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 <[email protected]>
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
On Fri, Sep 04, 2026 at 10:53:24PM -0700, Chang Yu wrote:
...
Quoted 3 lines
Datasheet: [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
Signature · 2 lines
With Best Regards, Andy Shevchenko
Jonathan Cameron
maintainerQuoted 11 lines · Andy Shevchenko
On Fri, Sep 04, 2026 at 10:53:24PM -0700, Chang Yu wrote:
...
Quoted 3 lines
Datasheet: [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
Jonathan Cameron
maintainerRe: [PATCH 2/2] iio: light: add AS7343 multi-spectral sensor driver
In reply to Joshua CroftsQuoted 21 lines · On Sat, 5 Sep 2026 08:42:58 +0200 Joshua Crofts <[email protected]>…
Hi Chang,
Comments inline.
Josh
Quoted 12 lines · On Fri, 4 Sep 2026 22:53:26 -0700 Chang Yu <[email protected]> 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 <[email protected]>
Nice review. A few small comments on necessity of features etc inline.
Quoted 12 lines
Quoted 12 lines
drivers/iio/light/as7343.c new file +6
-0+/* 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 */
...
Quoted 7 lines
drivers/iio/light/as7343.c line 230
Quoted 4 lines
drivers/iio/light/as7343.c new file +3
-0+ 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.
...
Quoted 7 lines
drivers/iio/light/as7343.c
Quoted 2 lines
drivers/iio/light/as7343.c new file +2
-0+ +/* 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!
...
Quoted 14 lines
Quoted 14 lines
drivers/iio/light/as7343.c new file +14
-0+ +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);
Quoted 8 lines
drivers/iio/light/as7343.c line 281
Quoted 5 lines
drivers/iio/light/as7343.c new file +5
-0+ +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).
Quoted 4 lines
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, seePM_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
Jonathan Cameron
maintainerRe: [PATCH 1/2] dt-bindings: iio: light: add as7343
In reply to Chang YuQuoted 4 lines
Add binding for AMS AS7343 which is a 14-channel multi-spectral sensor with i2c address of
0x39.Signed-off-by Chang Yu <[email protected]>
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
Quoted 23 lines
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +16
-0@@ -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 <[email protected]> + +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:
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml line 26
Quoted 10 lines
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +10
-0+ 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.
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml line 36
Quoted 10 lines
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +10
-0+ +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!
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml line 42
Quoted 6 lines
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +6
-0+ + 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!
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml line 44
Quoted 2 lines
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +2
-0+ 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)? -
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml line 46
Quoted 2 lines
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +2
-0+ }; + };
Thanks,
Jonathan
Signature · 1 line
Jonathan Cameron <[email protected]>
Jonathan Cameron
maintainerRe: [PATCH 2/2] iio: light: add AS7343 multi-spectral sensor driver
In reply to Chang YuQuoted 7 lines
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.
Quoted 5 lines
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 <[email protected]>
Hi Chang Yu,
I've avoided too much duplication with Joshua's already pretty thorough review so just a few additional comments inline.
Quoted 5 lines
drivers/iio/light/as7343.c new file +0
-0
...
drivers/iio/light/as7343.c line 31
Quoted 3 lines
drivers/iio/light/as7343.c new file +3
-0+ +#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.
drivers/iio/light/as7343.c line 39
Quoted 8 lines
drivers/iio/light/as7343.c new file +8
-0+ +/* 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.
Quoted 19 lines
drivers/iio/light/as7343.c new file +17
-0+#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 +
drivers/iio/light/as7343.c line 66
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+/* 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?
Quoted 1 line
+#define
AS7343_GAIN7 /* 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.
Quoted 1 line
+#define
AS7343_AUTO_CHANNEL_READOUT3 /* Automatic all-channel readout */
This is a field value in AS7343_CFG20_AUTO_SMUX so naming should reflect that.
or something like that.
Quoted 5 lines
drivers/iio/light/as7343.c new file +5
-0+ +/* AS7343 scan indices */ +#define AS7343_SCAN_INDEX_F1 0 +#define AS7343_SCAN_INDEX_F2 1 +#define AS7343_SCAN_INDEX_FZ 2
drivers/iio/light/as7343.c line 83
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+#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.
drivers/iio/light/as7343.c line 93
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+ +#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.
drivers/iio/light/as7343.c line 100
Quoted 7 lines
drivers/iio/light/as7343.c new file +7
-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), \
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.
drivers/iio/light/as7343.c line 107
Quoted 7 lines
drivers/iio/light/as7343.c new file +7
-0+ .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.
Quoted 19 lines
drivers/iio/light/as7343.c new file +17
-0+} + +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 ), +};
...
drivers/iio/light/as7343.c line 132
Quoted 2 lines
drivers/iio/light/as7343.c new file +2
-0+struct as7343_data { +struct i2c_client *client;
Not used that I can see - so drop client from this.
Quoted 5 lines
drivers/iio/light/as7343.c new file +5
-0+struct regmap *regmap; +}; + +static int as7343_read_raw(struct iio_dev *indio_dev, + struct iio_chan_spec const *chan, int *val,
drivers/iio/light/as7343.c line 147
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+ 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.
drivers/iio/light/as7343.c line 152
Quoted 5 lines
drivers/iio/light/as7343.c new file +5
-0+* 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;
drivers/iio/light/as7343.c line 160
Quoted 10 lines
drivers/iio/light/as7343.c new file +8
-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;
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.
Quoted 8 lines
drivers/iio/light/as7343.c new file +8
-0+ *val = (high <<8 ) | low; +return IIO_VAL_INT ; + } + +default : +return -EINVAL ; + } +}
...
drivers/iio/light/as7343.c line 181
Quoted 8 lines
drivers/iio/light/as7343.c new file +8
-0+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.
Quoted 12 lines
drivers/iio/light/as7343.c new file +12
-0+}; + +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));
drivers/iio/light/as7343.c line 203
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+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.
drivers/iio/light/as7343.c line 206
Quoted 3 lines
drivers/iio/light/as7343.c new file +3
-0+ 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.
drivers/iio/light/as7343.c line 211
Quoted 5 lines
drivers/iio/light/as7343.c new file +5
-0+ 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.
drivers/iio/light/as7343.c line 219
Quoted 8 lines
drivers/iio/light/as7343.c new file +8
-0+ + 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.
Quoted 2 lines
drivers/iio/light/as7343.c new file +2
-0+return ret; +
...
drivers/iio/light/as7343.c line 267
Quoted 4 lines
drivers/iio/light/as7343.c new file +4
-0+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.
drivers/iio/light/as7343.c line 275
Quoted 8 lines
drivers/iio/light/as7343.c new file +8
-0+ +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.
Quoted 5 lines
drivers/iio/light/as7343.c new file +5
-0+struct as7343_data *data =iio_priv (indio_dev); + +return regmap_set_bits (data->regmap,AS7343_REG_ENABLE , +AS7343_ENABLE_SP_EN ); +}
drivers/iio/light/as7343.c line 298
Quoted 10 lines
drivers/iio/light/as7343.c new file +8
-0+ +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.
Signature · 1 line
Jonathan Cameron <[email protected]>
Re: [PATCH 2/2] iio: light: add AS7343 multi-spectral sensor driver
In reply to Jonathan CameronHi 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.
Quoted 25 lines · Jonathan Cameron
Quoted 11 lines · On Sat, 5 Sep 2026 08:42:58 +0200 Joshua Crofts <[email protected]>…
Hi Chang,
Comments inline.
Josh
Quoted 2 lines · On Fri, 4 Sep 2026 22:53:26 -0700 Chang Yu <[email protected]> wrote:
This patch adds a driver for the AMS AS7343 14-channel multi-spectral sensor with I2C interface.
...
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+ + /* 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.
Quoted 3 lines
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.
Quoted 32 lines
...
Quoted 14 lines
Quoted 14 lines
drivers/iio/light/as7343.c new file +14
-0+ +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);Quoted 8 lines
drivers/iio/light/as7343.c line 281
Quoted 5 lines
drivers/iio/light/as7343.c new file +5
-0+ +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).
Quoted 9 lines
Quoted 4 lines
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, seePM_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
In reply to Jonathan CameronHi 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.
Quoted 26 lines · Jonathan Cameron
Quoted 7 lines
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.
Quoted 5 lines
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 <[email protected]>Hi Chang Yu,
I've avoided too much duplication with Joshua's already pretty thorough review so just a few additional comments inline.
Quoted 5 lines
drivers/iio/light/as7343.c new file +0
-0...
Quoted 15 lines
drivers/iio/light/as7343.c line 66
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+/* 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)
Quoted 15 lines
...
drivers/iio/light/as7343.c line 181
Quoted 8 lines
drivers/iio/light/as7343.c new file +8
-0+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.
Quoted 2 lines
Signature · 1 line
Jonathan Cameron <[email protected]>
Best,
Chang