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

[Jonathan Cameron <jic23@kernel.org>]

On Sat, 12 Sep 2026 21:11:07 -0700
Chang Yu <marcus.yu.56@gmail.com> wrote:

> On Sun, Sep 13, 2026 at 03:47:14AM +0100, Jonathan Cameron wrote:
> > On Fri, 11 Sep 2026 18:39:12 -0700
> > Chang Yu <marcus.yu.56@gmail.com> wrote:  
> > > +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 unused;
> > > +	struct regmap *map;  
> > 	struct regmap *map = data->regmap;
> > 	struct device *dev = regmap_get_device(map);
> > 
> > Neither is checked so no point in waiting until a few lines
> > later to initialize them.
> >   
> 
> I was trying to preserve the reverse christmas tree order and
> was unsure how the rules apply here. Is the following OK:
> 
>   struct as7343_data *data = iio_priv(indio_dev);
>   struct regmap *map = data->regmap;
>   struct device *dev = regmap_get_device(map);
> 
No line break
>   struct device *dev;
>   __le16 result;
>   int ret;

Yes this is fine.  Reverse xmas tree is just for when there are no
dependencies.

> 
> > ...  
> > > +	ret = PM_RUNTIME_ACQUIRE_ERR(&pm);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_RAW: {
> > > +		/* Wait until integration time passes for all 3 cycles. */
> > > +		msleep(160);
> > > +
> > > +		/*
> > > +		 * Reading ASTATUS latches all data registers to this read.
> > > +		 * We don't care about the returned saturation/gain status for
> > > +		 * now.  
> > 
> > Why do we care given only reading one channel and...
> >   
> > > +		 */
> > > +		guard(mutex)(&data->mutex);
> > > +		ret = regmap_read(map, AS7343_ASTATUS, &unused);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		ret = regmap_bulk_read(map, chan->address,  
> > .. it seems that if you read the low byte first (as this does) it is latched anyway.
> > There is a statement about this in the i2c intro part section 9.
> > 
> > so we get a consistent register pair.  Thus not needing the latch astatus
> > gives us.
> > With that in place reads can't stomp on each other as only
> > one regmap read is required, so the mutex isn't needed either.
> > It may be necessary once you add more features - hard to tell yet.
> >   
> When testing on hardware I discovered, for whatever reason, register
> values for other channels won't refresh unless I read ASTATUS or the
> FZ channel first. The only thing I can find in the datasheet pertaining
> to this is 10.2.7 page 41.
> 
> "Reading the ASTATUS register (0x94) latches all 36 spectral data bytes
> to that status read. Reading these bytes consecutively (0x94 to 0xB8)
> ensures that the data is concurrent."
> 
> My most charitable interpretation is that by "latches" they also mean
> "updates". I have no idea why reading FZ also works though. Could be
> a hardware bug since ASTATUS (0x94) and FZ DATA_0_L (0x95) are next
> to each other.
It is curious as if they are latched anyway what is the text about latching
if read from lower byte about.  Who knows!  This is definitely a less
than ideal datasheet but I don't think we have anyone active currently
upstream who works for AMS.

Jonathan