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

[Jonathan Cameron <jic23@kernel.org>]

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