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

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 14 Sep 2026 19:49:36 -0700
Chang Yu <marcus.yu.56@gmail.com> wrote:

> 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.

I briefly wondered why I said that and went to look.
I've been reading too many multipart drivers that do

#define THING1_ID 42
#define THING2_ID 43

struct something_chip_info thing1_chip_info = {
	.id = THING1_ID,
};

struct something_chip_info thing2_chip_info = {
	.id = THING2_ID,
};

And in those cases it doesn't make any sense to have defines.
So I was thinking don't do it here either so we'd be in a nicer
position when another device (maybe) shows up.

Jonathan