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@xxxxxxxxx> wrote:
> 
> > Hi Chang,
> > 
> > Comments inline.
> > 
> > Josh
> > 
> > On Fri,  4 Sep 2026 22:53:26 -0700
> > Chang Yu <marcus.yu.56@xxxxxxxxx> 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