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

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 23 Dec 2025 16:55:33 +0100
Maxime Chevallier <maxime.chevallier@bootlin.com> wrote:

> This adds a new driver for the TI TLA2528 ADC chip. It ha 8 12-bit
> channels, that can also be configured as 16-bit averaging channels.
> 
> Add a very simple driver for it, allowing reading raw values for each
> channel.
> 
> Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Hi Maxime,

A few extra bits from me

Thanks,

Jonathan


> diff --git a/drivers/iio/adc/ti-tla2528.c b/drivers/iio/adc/ti-tla2528.c
> new file mode 100644
> index 000000000000..9c572e730ffb
> --- /dev/null
> +++ b/drivers/iio/adc/ti-tla2528.c
> @@ -0,0 +1,209 @@

> +
> +static s32 tla2528_write_reg(const struct i2c_client *client, u8 reg, u8 val)
> +{
> +	u8 data[3] = {TLA2528_OP_WRITE_REG, reg, val};

Style wise. Prefer { TLA25... val };

- couple of spaces next to the brackets.

> +	int ret;
> +
> +	ret = i2c_master_send(client, data, 3);
> +
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static int tla2528_read_sample(const struct i2c_client *client)
> +{
> +	__be16 data;
> +	int ret;
> +
> +	ret = i2c_master_recv(client, (char *)&data, 2);

sizeof(data)

> +	if (ret < 0)
> +		return ret;
> +
> +	return be16_to_cpu(data) >> 4;
> +}

> +
> +#define TLA2528_CHAN(_chan, _name) { \

The _ aren't adding anything here, so I'd drop them.

> +	.type = IIO_VOLTAGE,					\
> +	.channel = (_chan),					\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.datasheet_name = _name,				\
> +	.indexed = 1,						\
> +}

> +
> +static int tla2528_probe(struct i2c_client *client)
> +{
> +	struct iio_dev *indio_dev;
> +	struct tla2528 *tla2528;
> +	int ret;
> +
> +	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C |
> +				     I2C_FUNC_SMBUS_WRITE_WORD_DATA))
> +		return -EOPNOTSUPP;
> +
> +	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*tla2528));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	tla2528 = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);

Ever used? If not don't set it.

> +	tla2528->client = client;
> +
> +	indio_dev->name = client->name;

Prefer to see it hard coded as a string.  If we added extra firmware
types in future the content of client->name can become something other
than the part number.


> +	indio_dev->info = &tla2528_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = tla2528_channel;
> +	indio_dev->num_channels = ARRAY_SIZE(tla2528_channel);
> +
> +	mutex_init(&tla2528->lock);
> +
> +	tla2528->vref_uv = devm_regulator_get_enable_read_voltage(&client->dev,
> +								  "vref");
> +	if (tla2528->vref_uv < 0)
> +		return tla2528->vref_uv;
> +
> +	/* Set all inputs as analog */
> +	ret = tla2528_write_reg(tla2528->client, TLA2528_PIN_CFG_ADR, 0x00);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = tla2528_write_reg(tla2528->client, TLA2528_DATA_CFG_ADR,
> +				TLA2528_DATA_CFG_APPEND_STATUS);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set manual mode */
> +	ret = tla2528_write_reg(tla2528->client, TLA2528_SEQUENCE_CFG_ADR, 0x00);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Init private data */
> +	tla2528->last_read_channel = ~0;
> +
> +	return devm_iio_device_register(&client->dev, indio_dev);
> +}