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

[Jonathan Cameron]

On Fri, 30 Oct 2026 17:47:04 +0100
Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx> 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.
> 
> 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.
> 
> Co-developed-by: Rodolfo Giometti <giometti@xxxxxxxxxxxx>
> Signed-off-by: Rodolfo Giometti <giometti@xxxxxxxxxxxx>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@xxxxxxxxxxx>
Hi Maxime

A couple of minor things inline.  At least one due to cleanup
that happened tree wide in the last year.

I'd also like to avoid propagating the fallback to all channels
if none are specified. In this particular case it is problematic
because if you add the GPIO mode, for chips like this, we tend
to expose all pins that aren't in use for anything else as GPIOs.
With the fallback scheme you have no way to tell if you want all
pins as GPIOs.

That hack went in a few drivers were we added per channel properties
after the initial driver was added added and had to paper over
the backwards compatibility issue.  Hopefully it didn't make it
into fresh drivers whilst I wasn't paying attention!

Thanks,

Jonathan

> diff --git a/drivers/iio/adc/ti-tla2528.c b/drivers/iio/adc/ti-tla2528.c
> new file mode 100644
> index 000000000000..287e52dc9d6d
> --- /dev/null
> +++ b/drivers/iio/adc/ti-tla2528.c

> +
> +static int tla2528_read_sample(const struct i2c_client *client)

Benefit of this helper seems marginal.  Maybe squash into where
it is currently called?  I don't care either way on this one.


> +{
> +	__be16 data;
> +	int ret;
> +
> +	ret = i2c_master_recv(client, (char *)&data, sizeof(data));
> +	if (ret < 0)
> +		return ret;
> +
> +	return be16_to_cpu(data) >> 4;
> +}
> +
> +static int tla2528_read(struct tla2528 *tla2528, u8 channel, int *val)
> +{
> +	int ret;

As below. Use guard() in here. That makes it really obvious why
the mutex is needed.

> +
> +	ret = regmap_update_bits(tla2528->regmap, TLA2528_CHANNEL_SEL_ADR,
> +				 TLA2528_CHANNEL_SEL_MANUAL_CHID, channel);
> +	if (ret)
> +		return ret;
> +
> +	ret = tla2528_read_sample(tla2528->client);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret;
> +
> +	return 0;
> +}
> +
> +static int tla2528_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct tla2528 *tla2528 = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&tla2528->lock);
> +		ret = tla2528_read(tla2528, chan->channel, val);
> +		mutex_unlock(&tla2528->lock);
> +		if (ret < 0)
> +			return ret;
> +
> +		return IIO_VAL_INT;
	case IIO_CHAN_INFO_RAW: {
		guard(mutex)(&tla2528->lock);

		ret = tla2528_read(tla2528, chan->channel, val);
		if (ret < 0)
			return ret;

		return IIO_VAL_INT;
	}
Small gain here in keeping the error check right next to the thing
that set it, but might as well have that given it is simple to
use a guard.

Or better still: Move the guard into tla2528_read() 
There it will be more obvious that it is the two step nature
of the read that requires the lock.

> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = tla2528->vref_mv;
> +		*val2 = 12;
> +
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +
> +	default:
> +		return -EINVAL;
> +	}
> +}

> +
> +static int tla2528_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	struct iio_chan_spec *channels;
> +	struct iio_dev *indio_dev;
> +	struct tla2528 *tla2528;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))

Add a comment that the data reads directly use i2c with
no register address.  FWIW I briefly wondered why the checks in
the regmap init weren't enough.  They aren't because for this particular
mix of reg and val sizes can match others I2C protocol options in
regmap_get_i2c_bus().  With this present though we know they will match
the first and simplest one.

> +		return -EOPNOTSUPP;
> +
> +	if (i2c_check_quirks(client->adapter, I2C_AQ_NO_CLK_STRETCH))
> +		return dev_err_probe(dev, -EOPNOTSUPP,
> +				     "adapter does not support clock stretching\n");
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*tla2528));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	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);
> +
> +	ret = devm_mutex_init(dev, &tla2528->lock);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_regulator_get_enable(dev, "dvdd");
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_regulator_get_enable_read_voltage(dev, "avdd");
> +	if (ret < 0)
> +		return ret;
> +
> +	tla2528->vref_mv = ret / (MICRO / MILLI);
> +
> +	ret = devm_iio_adc_device_alloc_chaninfo_se(dev, &tla2528_chan_template,
> +						    TLA2528_NUM_CHANNELS - 1,
> +						    &channels);
> +	if (ret < 0 && ret != -ENOENT)
> +		return ret;
> +
> +	if (ret == -ENOENT) {

We do this trick for old drivers where we only added per channel properties
later.  For a new driver, why not always require the binding to be explicit
and list the channels (even if that is all of them)?

> +		indio_dev->channels = tla2528_channel;
> +		indio_dev->num_channels = ARRAY_SIZE(tla2528_channel);
> +	} else {
> +		indio_dev->channels = channels;
> +		indio_dev->num_channels = ret;
> +	}
> +
> +	indio_dev->name = "tla2528";
> +	indio_dev->info = &tla2528_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	ret = regmap_write(tla2528->regmap, TLA2528_GENERAL_CFG_ADR,
> +			   TLA2528_GENERAL_CFG_RST);
> +	if (ret)
> +		return ret;
> +
> +	fsleep(5 * USEC_PER_MSEC);
> +
> +	return devm_iio_device_register(dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id tla2528_id[] = {
> +	{ "tla2528" },

This is something that happened in last year, but for these
we now always use named initializers.  There is work ongoing
for a change in the structure definition and that only works
if they are named.  + It is consistent with what we always
did for the of_device_id tables.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, tla2528_id);