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

[Chang Yu <marcus.yu.56@gmail.com>]

On Mon, Sep 14, 2026 at 11:00:18AM +0300, Andy Shevchenko wrote:
> On Fri, Sep 11, 2026 at 06:39:12PM -0700, Chang Yu wrote:
> 
> ...
> 
> > +static int as7343_setup_device(struct device *dev, struct as7343_data *data)
> > +{
> > +	struct regmap *map = data->regmap;
> > +	unsigned int val;
> > +	__le16 step;
> > +	int ret;
> > +
> > +	/* Power on */
> > +	ret = regmap_set_bits(map, 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(map, AS7343_CFG0, AS7343_CFG0_REG_BANK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_read(map, AS7343_ID, &val);
> > +	if (ret)
> > +		return ret;
> 
> > +	if (val != 0x81)
> > +		dev_info(dev, "Unknown device ID: %x\n", val);
> 
> Wouldn't be better to define 0x81 with meaningful name?
> 

If I remeber correctly Jonathan prefers just putting 0x81 inline. I'm
OK with either style so I'll defer to the judgement to Jonathan here.

> > +	ret = regmap_clear_bits(map, AS7343_CFG0, AS7343_CFG0_REG_BANK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Configure the SMUX to readout all channels */
> > +	ret = regmap_update_bits(map, 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(map, AS7343_ASTEP, &step, sizeof(step));
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(map, AS7343_ATIME, AS7343_ATIME_VAL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return regmap_update_bits(map, AS7343_CFG1, AS7343_CFG1_AGAIN,
> > +				  FIELD_PREP(AS7343_CFG1_AGAIN,
> > +					     AS7343_CFG1_AGAIN_X256));
> > +}
> 
> ...
> 
> > +static int as7343_suspend(struct device *dev)
> > +{
> > +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> > +	struct as7343_data *data = iio_priv(indio_dev);
> > +	struct regmap *map = data->regmap;
> 
> > +	return regmap_clear_bits(map, AS7343_ENABLE, AS7343_ENABLE_SP_EN);
> 
> Can this mess up the raw read? If so, also needs a mutex to be held.
> 

People familiar with runtime PM correct me if I'm wrong, but I believe
PM_RUNTIME_ACQUIRE waits for any ongoing suspend callback to finish
before returning? If so I think there is no risk of a race here. In
the case of system suspend, worst case scenario SP_EN gets cleared
before we read ASTATUS or the data register, which for my use case
at least is OK since the entire system is suspending anyway.