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

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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>

...

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

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

> +	if (ret)
> +		return ret;

-- 
With Best Regards,
Andy Shevchenko