Add support for AS7343 multi-spectral sensor
Subsystems (2)
v3 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
Patch 1 adds device tree bindings.
Patch 2 adds the driver.
Testing was performed on a Raspberry Pi 4.
Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
Changes in v3:
- Fix gpio description and example in dt.
- Only start measurements after suspend action has been registered.
- Add a mutex and pm_runtime_resume_and_get before reading measurements.
Changes in v2:
- Add the LDR, the interrupt pin, and the GPIO pin to the dt bindings.
- Adjust the default gain to x256 and integration time to 50.1ms as recommended by the datasheet.
- Use read_label instead of .extend
- Remove all unused scan_index related constants.
Chang Yu (2):
dt-bindings: iio: light: add as7343
iio: light: add AS7343 multi-spectral sensor driver
.../bindings/iio/light/ams,as7343.yaml | 72 ++++ MAINTAINERS | 7 + drivers/iio/light/Kconfig | 11 + drivers/iio/light/Makefile | 1 + drivers/iio/light/as7343.c | 397 ++++++++++++++++++ 5 files changed, 488 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/light/ams,as7343.yaml create mode 100644 drivers/iio/light/as7343.c
Signature · 1 line
2.55.0
PATCH v3 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.
Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
---
Changes in v3:
- Add missing include for gpio.
- Expand GPIO descrption to include hardware synchronization.
- Use led-max-microamp instead of led-current-microamp for the LDR pin.
Changes in v2:
- Add the LDR, the interrupt pin, and the GPIO pin to the bindings.
- Fix node name and unit address mismatch.
- Include MAINTAINERS changes.
.../bindings/iio/light/ams,as7343.yaml | 72 +++++++++++++++++++ MAINTAINERS | 6 ++ 2 files changed, 78 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/light/ams,as7343.yaml
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +72 -0
@@ -0,0 +1,72 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/iio/light/ams,as7343.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: AMS AS7343 14-Channel Multi-Spectral Sensor + +maintainers: + - Chang Yu <[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 + + interrupts: + description: + Open drain output active low interrupt pin. + maxItems: 1 + + vdd-supply: true + + ams,led-max-microamp: + description: + The driver current for the external LED connected to the LDR pin. + minimum: 4000 + maximum: 258000 + multipleOf: 2000 + default: 12000 + + sync-gpios: + description: | + Optional GPIO pin that can be used either as a synchronization input to + start/stop measurement or as an external start/stop signal for + synchronizing with the connected LED. + maxItems: 1 + +required: + - compatible + - reg + - vdd-supply + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/irq.h> + + i2c { + #address-cells = <1>; + #size-cells = <0>; + + multispectral-sensor@39 { + compatible = "ams,as7343"; + reg = <0x39>; + interrupts = <7 IRQ_TYPE_LEVEL_LOW>; + vdd-supply = <&vdd_regulator>; + ams,led-max-microamp = <14000>; + sync-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>; + }; + };
MAINTAINERS +6 -0
@@ -1430,6 +1430,12 @@ S: Maintained F: Documentation/devicetree/bindings/iio/light/ams,as73211.yaml F: drivers/iio/light/as73211.c +AMS AS7343 DRIVER +M: Chang Yu <[email protected]> +L: [email protected] +S: Maintained +F: Documentation/devicetree/bindings/iio/light/ams,as7343.yaml + AMT (Automatic Multicast Tunneling) M: Taehee Yoo <[email protected]> L: [email protected]
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v3 1/2] dt-bindings: iio: light: add as7343
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The generic LED property `led-max-microamp` was incorrectly added with a vendor prefix…
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The generic LED property led-max-microamp was incorrectly added with a vendor prefix (ams,led-max-microamp), contradicting the commit message and standard property conventions. --
commit bc94ab43a595b8c728d510d6725b14dbeb5f151f
Author: Chang Yu <[email protected]>
dt-bindings: iio: light: add as7343
Add binding for AMS AS7343 which is a 14-channel multi-spectral sensor with i2c address of 0x39.
Quoted 5 lines
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +0
-0
[ ... ]
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml line 33
Quoted 3 lines
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +3
-0+ vdd-supply: true + + ams,led-max-microamp:
[Severity: Medium]
Should this property be defined without the vendor prefix?
The generic LED property led-max-microamp is added here with a vendor prefix (ams,led-max-microamp), which contradicts standard property conventions and the stated intent in the patch version changelog.
Device trees written for this device will use this vendor-prefixed property, creating permanent ABI baggage. If a driver later implements this feature and correctly parses the standard led-max-microamp, it will break compatibility with these early device trees or force the driver to parse both properties.
Quoted 7 lines
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +7
-0+ description: + The driver current for the external LED connected to the LDR pin. + minimum: 4000 + maximum: 258000 + multipleOf: 2000 + default: 12000 +
[ ... ]
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml line 69
Quoted 6 lines
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +6
-0+ multispectral-sensor@39 { + compatible = "ams,as7343"; + reg = <0x39>; + interrupts = <7 IRQ_TYPE_LEVEL_LOW>; + vdd-supply = <&vdd_regulator>; + ams,led-max-microamp = <14000>;
[Severity: Medium]
Would it be better to also remove the vendor prefix from this property in the device tree example?
Quoted 3 lines
Documentation/devicetree/bindings/iio/light/ams,as7343.yaml new file +3
-0+ sync-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>; + }; + };
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH v3 1/2] dt-bindings: iio: light: add as7343
In reply to Chang YuQuoted 11 lines · Chang Yu
Add binding for AMS AS7343 which is a 14-channel multi-spectral sensor with i2c address of
0x39.Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
Signed-off-by Chang Yu <[email protected]>---
Changes in v3:
- Add missing include for gpio.
- Expand GPIO descrption to include hardware synchronization.
- Use led-max-microamp instead of led-current-microamp for the LDR pin.
Quoted 5 lines
Changes in v2:
- Add the LDR, the interrupt pin, and the GPIO pin to the bindings.
- Fix node name and unit address mismatch.
- IncludeMAINTAINERSchanges.
You need to address Sashiko review.
Best regards,
Krzysztof
PATCH v3 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. Note that power management is designed in such a way that we only stop spectrual measurements when susepended and do not power off.
More complex features such as auto-suspend, interrupts, and configurable gain/integration time will be added in future patches.
Datasheet: https://look.ams-osram.com/m/5f2d27fff9a874d2/original/AS7343-14-Channel-Multi-Spectral-Sensor.pdf
---
Changes in v3:
- Hold a mutex before reading measurements.
- Add pm_runtime_resume_and_get before reading measurements.
- Only start measurements after suspend action has been successfully registered
- Check the return value of regmap_read when verifying device ID.
Changes in v2:
- Adjust the default gain to x256 and integration time to 50.1ms as recommended by the datasheet.
- Use read_label instead of .extend
- Remove all unused scan_index related constants.
- Add missing calls to pm_runtime_* methods in probe.
- Fix various code style issues
MAINTAINERS | 1 + drivers/iio/light/Kconfig | 11 + drivers/iio/light/Makefile | 1 + drivers/iio/light/as7343.c | 397 +++++++++++++++++++++++++++++++++++++ 4 files changed, 410 insertions(+) create mode 100644 drivers/iio/light/as7343.c
MAINTAINERS +1 -0
@@ -1435,6 +1435,7 @@ 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]>
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 +397 -0
397 lines not on this page · raw message
Signature · 1 line
2.55.0
[email protected]
AI reviewRe: [PATCH v3 2/2] iio: light: add AS7343 multi-spectral sensor driver
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] Device remains continuously active and measuring until the first sysfs read due to missing idle…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Device remains continuously active and measuring until the first sysfs read due to missing idle trigger in probe.
- [Low] Missing <linux/delay.h> include for msleep().
--
commit 11270e0cc23d66ee1d725cbe09a1342ed010ace3
Author: Chang Yu <[email protected]>
iio: light: add AS7343 multi-spectral sensor driver
This commit adds a new driver for the AMS AS7343 14-channel multi-spectral sensor. It exposes 12 spectral channels via the IIO sysfs interface and supports basic suspend/resume power management functionality.
Quoted 5 lines
drivers/iio/light/as7343.c new file +0
-0
[ ... ]
Quoted 4 lines
drivers/iio/light/as7343.c new file +4
-0+static int as7343_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan,int *val, +int *val2,long mask) +{
[ ... ]
drivers/iio/light/as7343.c line 149
Quoted 4 lines
drivers/iio/light/as7343.c new file +4
-0+switch (mask) { +case IIO_CHAN_INFO_RAW : { +/* Wait until integration time passes for all 3 cycles. */ +msleep (160 );
[Severity: Low]
Does this msleep() call require including <linux/delay.h>?
Building the driver on architectures where the header is not implicitly included by other headers might result in a build failure.
[ ... ]
Quoted 2 lines
drivers/iio/light/as7343.c new file +2
-0+static int as7343_probe (struct i2c_client *client) +{
[ ... ]
drivers/iio/light/as7343.c line 367
Quoted 11 lines
drivers/iio/light/as7343.c new file +11
-0+ ret =devm_pm_runtime_enable (dev); +if (ret) +return dev_err_probe (dev, ret,"Failed to enable PM runtime\n" ); + +/* Start measurements */ + ret =regmap_set_bits (data->regmap,AS7343_ENABLE ,AS7343_ENABLE_SP_EN ); +if (ret) +return ret; + +return devm_iio_device_register (dev, indio_dev); +}
[Severity: Medium]
Does this probe function leave the device continuously active and measuring until the first sysfs read?
The code marks the device as active and starts measurements, but does not appear to drop its usage count or trigger an idle state. The power management core may consider it active with a zero usage count, but never automatically suspend it.
This seems to leave the sensor taking measurements and drawing power indefinitely, only resolving when the first sysfs read invokes pm_runtime_resume_and_get() followed by pm_runtime_put().
Could pm_runtime_idle() be called before probe finishes, or should the device be transitioned to suspend instead?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH v3 2/2] iio: light: add AS7343 multi-spectral sensor driver
In reply to Chang YuQuoted 7 lines · 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.
Quoted 6 lines
Basic power management (suspend/resume) is supported. Note that power management is designed in such a way that we only stop spectrual measurements when susepended and do not power off.
More complex features such as auto-suspend, interrupts, and configurable gain/integration time will be added in future patches.
It's v3 already. Can you browse the linux-iio@ mailing list archive and see what are the common comments on the new contributions? I think you may ask AI to help with the summary. This patch has tons of what has been repeated over and over...
...
drivers/iio/light/as7343.c line 31
Quoted 11 lines
drivers/iio/light/as7343.c new file +10
-0+#include <linux/array_size.h> +#include <linux/bitfield.h> +#include <linux/bits.h> +#include <linux/cleanup.h> +#include <linux/dev_printk.h> +#include <linux/err.h> +#include <linux/i2c.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/pm.h>
This is implied by pm_runtime.h IIRC.
Quoted 5 lines
drivers/iio/light/as7343.c new file +5
-0+#include <linux/pm_runtime.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/sysfs.h> +#include <linux/types.h>
...
drivers/iio/light/as7343.c line 45
Quoted 4 lines
drivers/iio/light/as7343.c new file +4
-0+/* AS7343 registers */ +#define AS7343_ID 0x5a + +#define AS7343_ENABLE 0x80
Make sure the indentation of the definition of the same kind are the same.
...
drivers/iio/light/as7343.c line 53
Quoted 5 lines
drivers/iio/light/as7343.c new file +5
-0+/* +* Integration time is calculated as (ATIME + 1) * ((ASTEP + 1) * 2.78us). +* Setting a 30 * 1.67ms = 50.1ms integration test as the default for now. +*/ +#define AS7343_ATIME 0x81
I believe this one is related to the register offsets? (see above why)
...
drivers/iio/light/as7343.c line 134
Quoted 3 lines
drivers/iio/light/as7343.c new file +3
-0+static int as7343_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan,int *val, +int *val2,long mask)
Please, split logically.
struct iio_chan_spec const *chan, int *val, int *val2, long mask)
drivers/iio/light/as7343.c line 140
Quoted 6 lines
drivers/iio/light/as7343.c new file +6
-0+{ +struct as7343_data *data =iio_priv (indio_dev); +struct device *dev =regmap_get_device (data->regmap); +int ret; +unsigned int unused; +__le16 result;
Preserve reversed xmas tree order.
drivers/iio/light/as7343.c line 144
Quoted 3 lines
drivers/iio/light/as7343.c new file +3
-0+ ret =pm_runtime_resume_and_get (dev); +if (ret) +return ret;
Use PM_RUNTIME_ACQUIRE*().
Quoted 6 lines
drivers/iio/light/as7343.c new file +6
-0+switch (mask) { +case IIO_CHAN_INFO_RAW : { +/* Wait until integration time passes for all 3 cycles. */ +msleep (160 ); + +/*
drivers/iio/light/as7343.c line 161
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+* Reading ASTATUS latches all data registers to this read. +* We don't care about the returned saturation/gain status for +* now. +*/ +guard (mutex)(&data->mutex); + ret =regmap_read (data->regmap,AS7343_ASTATUS , &unused); +if (ret) +break ; + + ret =regmap_bulk_read (data->regmap, chan->address, &result,2 );
sizeof()
Quoted 6 lines
drivers/iio/light/as7343.c new file +6
-0+if (ret) +break ; + + *val =le16_to_cpu (result); + ret =IIO_VAL_INT ; +break ;
drivers/iio/light/as7343.c line 177
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+ } + +default : + ret = -EINVAL ; +break ; + } + +pm_runtime_put (dev); +return ret; +}
(All comments for the above function is what has been repeated in many
contributions for sure.)
...
Quoted 20 lines
drivers/iio/light/as7343.c new file +20
-0+static const char *as7343_channel_label (struct iio_chan_spec const *chan) +{ +switch (chan->channel) { +case AS7343_CHAN_IDX_FZ : +return "FZ" ; +case AS7343_CHAN_IDX_FY : +return "FY" ; +case AS7343_CHAN_IDX_FXL : +return "FXL" ; +case AS7343_CHAN_IDX_NIR : +return "NIR" ; +case AS7343_CHAN_IDX_F2 : +return "F2" ; +case AS7343_CHAN_IDX_F3 : +return "F3" ; +case AS7343_CHAN_IDX_F4 : +return "F4" ; +case AS7343_CHAN_IDX_F6 : +return "F6" ; +case AS7343_CHAN_IDX_F1 :
drivers/iio/light/as7343.c line 208
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+return "F1" ; +case AS7343_CHAN_IDX_F7 : +return "F7" ; +case AS7343_CHAN_IDX_F8 : +return "F8" ; +case AS7343_CHAN_IDX_F5 : +return "F5" ; +default : +return NULL ; + }
Why not keeping this in a static array?
drivers/iio/light/as7343.c line 218
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+} + +static int as7343_read_label (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan,char *label) +{ +const char *name; + + name =as7343_channel_label (chan); +if (!name) +return -EINVAL ;
Why? Can't it be taken from DT?
Quoted 2 lines
drivers/iio/light/as7343.c new file +2
-0+return sysfs_emit (label,"%s\n" , name); +}
...
drivers/iio/light/as7343.c line 235
Quoted 9 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_MAX , + .reg_format_endian =REGMAP_ENDIAN_LITTLE , + .val_format_endian =REGMAP_ENDIAN_LITTLE , + .cache_type =REGCACHE_NONE ,
Why?! This needs a very good justification.
Quoted 1 line
+};
...
Review
Quoted 2 lines
(quoted diff) +2
-0+static int as7343_setup_device(struct device *dev, struct as7343_data *data) +{
struct regmap *map = data->regmap;
will help to reduce verbosity of the below, and might even save some LoC...
Quoted 13 lines
drivers/iio/light/as7343.c new file +13
-0+unsigned int val; +__le16 step; +int ret; + +/* Power on */ + ret =regmap_set_bits (data->regmap,AS7343_ENABLE ,AS7343_ENABLE_PON ); +if (ret) +return ret; + +/* Need to set REG_BANK to 1 before we can access ID */ + ret =regmap_set_bits (data->regmap,AS7343_CFG0 ,AS7343_CFG0_REG_BANK ); +if (ret) +return ret;
drivers/iio/light/as7343.c line 262
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+ + ret =regmap_read (data->regmap,AS7343_ID , &val); +if (ret) +return ret; + +if (val !=0x81 ) +dev_info (dev,"Unknown device ID: %x\n" , val); + + ret =regmap_clear_bits (data->regmap,AS7343_CFG0 , +AS7343_CFG0_REG_BANK );
...for example, here:
ret = regmap_clear_bits(map, AS7343_CFG0, AS7343_CFG0_REG_BANK);
drivers/iio/light/as7343.c line 267
Quoted 6 lines
drivers/iio/light/as7343.c new file +5
-0+if (ret) +return ret; + +/* Configure the SMUX to readout all channels */ + ret = regmap_update_bits(
Huh?! Please, check the formatting and indentation style.
Quoted 20 lines
drivers/iio/light/as7343.c new file +20
-0+ data->regmap,AS7343_CFG20 ,AS7343_CFG20_AUTO_SMUX , +FIELD_PREP (AS7343_CFG20_AUTO_SMUX , + AS7343_CFG20_AUTO_SMUX_READOUT_ALL)); +if (ret) +return ret; + +/* Set 50.1ms integration time and x256 gain for now */ + step =cpu_to_le16 (AS7343_ASTEP_VAL ); + ret =regmap_bulk_write (data->regmap,AS7343_ASTEP , &step,2 ); +if (ret) +return ret; + + ret =regmap_write (data->regmap,AS7343_ATIME ,AS7343_ATIME_VAL ); +if (ret) +return ret; + +return regmap_update_bits (data->regmap,AS7343_CFG1 ,AS7343_CFG1_AGAIN , +FIELD_PREP (AS7343_CFG1_AGAIN , +AS7343_CFG1_AGAIN_X256 )); +}
...
Quoted 7 lines
drivers/iio/light/as7343.c new file +7
-0+static int as7343_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct as7343_data *data =iio_priv (indio_dev); + +return regmap_clear_bits (data->regmap,AS7343_ENABLE , +AS7343_ENABLE_SP_EN );
drivers/iio/light/as7343.c line 305
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+} + +static int as7343_resume (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct as7343_data *data =iio_priv (indio_dev); + +return regmap_set_bits (data->regmap,AS7343_ENABLE , +AS7343_ENABLE_SP_EN ); +}
Same, use temporary for struct regmap.
...
Quoted 11 lines
drivers/iio/light/as7343.c new file +11
-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; +int ret; + + indio_dev =devm_iio_device_alloc (dev,sizeof (*data)); +if (!indio_dev) +return -ENOMEM ;
drivers/iio/light/as7343.c line 331
Quoted 10 lines
drivers/iio/light/as7343.c new file +9
-0+ + regmap =devm_regmap_init_i2c (client, &as7343_regmap_config); +if (IS_ERR (regmap)) +return PTR_ERR (regmap); + + data =iio_priv (indio_dev); +i2c_set_clientdata (client, indio_dev); + data->regmap = regmap; +mutex_init (&data->mutex);
devm_mutex_init().
Quoted 36 lines
drivers/iio/light/as7343.c new file +36
-0+ + indio_dev->name ="as7343" ; + indio_dev->info = &as7343_info; + indio_dev->channels = as7343_channels; + indio_dev->num_channels =ARRAY_SIZE (as7343_channels); + indio_dev->modes =INDIO_DIRECT_MODE ; + + ret =devm_regulator_get_enable (dev,"vdd" ); +if (ret) +return ret; + + ret =as7343_setup_device (dev, data); +if (ret) +return ret; + + ret =devm_add_action_or_reset (dev, as7343_suspend_action, dev); +if (ret) +return dev_err_probe (dev, ret, +"Failed to add suspend action\n" ); + + ret =pm_runtime_set_active (dev); +if (ret) +return dev_err_probe (dev, ret, +"Failed to activate PM runtime\n" ); + + ret =devm_pm_runtime_enable (dev); +if (ret) +return dev_err_probe (dev, ret,"Failed to enable PM runtime\n" ); + +/* Start measurements */ + ret =regmap_set_bits (data->regmap,AS7343_ENABLE ,AS7343_ENABLE_SP_EN ); +if (ret) +return ret; + +return devm_iio_device_register (dev, indio_dev); +}
...
drivers/iio/light/as7343.c line 370
Quoted 2 lines
drivers/iio/light/as7343.c new file +2
-0+static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops, as7343_suspend, as7343_resume, + NULL);
Again, split logically. Options are:
static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops, as7343_suspend, as7343_resume, NULL);
static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops,
as7343_suspend, as7343_resume, NULL);
static DEFINE_RUNTIME_DEV_PM_OPS(as7343_pm_ops,
as7343_suspend, as7343_resume, NULL);
(I personally prefer compromise as depicted in the second example).
...
drivers/iio/light/as7343.c line 374
Quoted 3 lines
drivers/iio/light/as7343.c new file +3
-0+static const struct of_device_id as7343_of_match[] = { + { .compatible ="ams,as7343" }, + { },
No comma in the terminator entry.
drivers/iio/light/as7343.c line 380
Quoted 6 lines
drivers/iio/light/as7343.c new file +6
-0+}; +MODULE_DEVICE_TABLE (of, as7343_of_match); + +static const struct i2c_device_id as7343_id[] = { + { .name ="as7343" }, + { },
Ditto.
Quoted 2 lines
(quoted diff) +2
-0+}; +MODULE_DEVICE_TABLE(i2c, as7343_id);
...
drivers/iio/light/as7343.c line 391
Quoted 8 lines
drivers/iio/light/as7343.c new file +8
-0+static struct i2c_driver as7343_driver = { + .driver = { + .name ="as7343" , + .of_match_table = as7343_of_match, + .pm =pm_ptr (&as7343_pm_ops), + }, + .probe = as7343_probe, + .id_table = as7343_id,
Indentation of the assignees with tabs makes it harder to maintain (in case more lines got added it might require to reindent all of them).
Quoted 2 lines
(quoted diff) +2
-0+}; +module_i2c_driver(as7343_driver);
Signature · 2 lines
With Best Regards, Andy Shevchenko
Re: [PATCH v3 2/2] iio: light: add AS7343 multi-spectral sensor driver
Hi Andy
Thanks for the thorough review. Just one quick clarification inline.
Quoted 4 lines · Andy Shevchenko
On Wed, Sep 09, 2026 at 11:38:13PM -0700, Chang Yu wrote:
...
Quoted 13 lines
drivers/iio/light/as7343.c line 218
Quoted 10 lines
drivers/iio/light/as7343.c new file +10
-0+} + +static int as7343_read_label (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan,char *label) +{ +const char *name; + + name =as7343_channel_label (chan); +if (!name) +return -EINVAL ;Why? Can't it be taken from DT?
These channel labels correspond to the physical optical channel names defined by the datasheet. Each name corresponds to a pre-defined range of wavelengths. They are fixed internal properties of the chip, so IMO hardcoding them in the driver is more approriate.
Quoted 5 lines
Quoted 2 lines
drivers/iio/light/as7343.c new file +2
-0+return sysfs_emit (label,"%s\n" , name); +}...
Best,
Chang
Re: [PATCH v3 2/2] iio: light: add AS7343 multi-spectral sensor driver
Quoted 2 lines · Chang Yu
Quoted 1 line · Andy Shevchenko
On Wed, Sep 09, 2026 at 11:38:13PM -0700, Chang Yu wrote:
...
Quoted 11 lines
Quoted 5 lines
drivers/iio/light/as7343.c line 218
Quoted 3 lines
drivers/iio/light/as7343.c new file +3
-0+ name =as7343_channel_label (chan); +if (!name) +return -EINVAL ;Why? Can't it be taken from DT?
These channel labels correspond to the physical optical channel names defined by the datasheet. Each name corresponds to a pre-defined range of wavelengths. They are fixed internal properties of the chip, so IMO hardcoding them in the driver is more approriate.
Please, make a comment on top of the string array, with probably citing the datasheet table with the wavelength ranges.
/*
* Channel names as defined in the datasheet:
* // hypothetical example // ... - 200-300 nm
* ...
*/
static const char * const ..._names[] = {
...names...
};Signature · 2 lines
With Best Regards, Andy Shevchenko