Re: [PATCH v4 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 13 Nov 2026 17:40:45 +0200
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Fri, Nov 13, 2026 at 04:15:42PM +0100, Maxime Chevallier wrote:
> > Add a new driver for the TI TLA2528 ADC chip. It has 8 12-bit
> > channels, that can also be configured as 16-bit averaging channels.  
> 
> I forgot to check if there is a justification of the brand new driver,
> I assume there is.
> 
> > This is a very simple driver, allowing reading raw values for each
> > channel. GPIO mode isn't supported for now, as well as buffered mode and
> > the 16-bit averaging mode.  
> 
> Some nit-picks below, but they do not prevent from this patch being accepted,
> so
> 
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@intel.com>
> 
Applied, I did the second of these suggestions as a tweak.

Thanks,

Jonathan

> ...
> 
> > +static int tla2528_read(struct tla2528 *tla2528, u8 channel, int *val)
> > +{
> > +	__be16 data;
> > +	int ret;
> > +
> > +	guard(mutex)(&tla2528->lock);
> > +
> > +	ret = regmap_update_bits(tla2528->regmap, TLA2528_CHANNEL_SEL_ADR,
> > +				 TLA2528_CHANNEL_SEL_MANUAL_CHID, channel);
> > +	if (ret)
> > +		return ret;  
> 
> Ideally this needs a comment explaining in short why regmap IO accessors may
> not be used.
I left this alone because there is a comment in probe. I'm not sure repeating
a comment that basically says 'because weird' is worth it.

> 
> > +	ret = i2c_master_recv(tla2528->client, (char *)&data, sizeof(data));
> > +	if (ret < 0)
> > +		return ret;
> > +	if (ret != sizeof(data))
> > +		return -EIO;
> > +
> > +	*val = be16_to_cpu(data) >> 4;
> > +
> > +	return 0;
> > +}  
> 
> ...
> 
> > +	ret = regmap_write(tla2528->regmap, TLA2528_GENERAL_CFG_ADR,
> > +			   TLA2528_GENERAL_CFG_RST);  
> 
> With locally defined
> 
> 	struct regmap *map;
> 	...
> 	map = devm_regmap_*init*(...);
> 	...
> 	tla2528->regmap = map;
> 	...
> 	ret = regmap_write(map, TLA2528_GENERAL_CFG_ADR, TLA2528_GENERAL_CFG_RST);
> 
> this can kill two birds with one stone:
> - make this one line (yes, 82 characters is okay)
> - keeping data structure free from error pointers
> 
diff --git a/drivers/iio/adc/ti-tla2528.c b/drivers/iio/adc/ti-tla2528.c
index c02b04e19785..754b31d6725f 100644
--- a/drivers/iio/adc/ti-tla2528.c
+++ b/drivers/iio/adc/ti-tla2528.c
@@ -128,6 +128,7 @@ static int tla2528_probe(struct i2c_client *client)
 	struct iio_chan_spec *channels;
 	struct iio_dev *indio_dev;
 	struct tla2528 *tla2528;
+	struct regmap *map;
 	int ret;
 
 	/*
@@ -148,9 +149,11 @@ static int tla2528_probe(struct i2c_client *client)
 	tla2528 = iio_priv(indio_dev);
 	tla2528->client = client;
 
-	tla2528->regmap = devm_regmap_init_i2c(client, &tla2528_regmap_config);
-	if (IS_ERR(tla2528->regmap))
-		return PTR_ERR(tla2528->regmap);
+	map = devm_regmap_init_i2c(client, &tla2528_regmap_config);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+
+	tla2528->regmap = map;
 
 	ret = devm_mutex_init(dev, &tla2528->lock);
 	if (ret)
@@ -179,8 +182,7 @@ static int tla2528_probe(struct i2c_client *client)
 	indio_dev->info = &tla2528_info;
 	indio_dev->modes = INDIO_DIRECT_MODE;
 
-	ret = regmap_write(tla2528->regmap, TLA2528_GENERAL_CFG_ADR,
-			   TLA2528_GENERAL_CFG_RST);
+	ret = regmap_write(map, TLA2528_GENERAL_CFG_ADR, TLA2528_GENERAL_CFG_RST);
 	if (ret)
 		return ret;
 
                                                   
                                 
> > +	if (ret)
> > +		return ret;  
>