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@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxxx>

Best,
Chang