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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6b7bf02e-d016-4d2b-a409-6cd378d409fb@bootlin.com>
Date: Mon, 5 Jan 2026 11:16:39 +0100
From: Maxime Chevallier <maxime.chevallier@...tlin.com>
To: Jonathan Cameron <jic23@...nel.org>
Cc: David Lechner <dlechner@...libre.com>, nuno.sa@...log.com,
 Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Marcelo Schmitt <marcelo.schmitt@...log.com>,
 Matti Vaittinen <mazziesaccount@...il.com>,
 Antoniu Miclaus <antoniu.miclaus@...log.com>,
 Angelo Dureghello <adureghello@...libre.com>,
 Tobias Sperling <tobias.sperling@...ting.com>,
 Eason Yang <j2anfernee@...il.com>,
 Marilene Andrade Garcia <marilene.agarcia@...il.com>,
 Cosmin Tanislav <cosmin-gabriel.tanislav.xa@...esas.com>,
 duje@...emihanovic.xyz, herve.codina@...tlin.com,
 Rodolfo Giometti <giometti@...eenne.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 thomas.petazzoni@...tlin.com
Subject: Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528
 adc

Hi Jonathan,

On 27/12/2025 19:42, Jonathan Cameron wrote:
> On Tue, 23 Dec 2025 16:55:33 +0100
> Maxime Chevallier <maxime.chevallier@...tlin.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@...eenne.com>
>> Signed-off-by: Maxime Chevallier <maxime.chevallier@...tlin.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 };

Sure thing, I'll address that.

> 
> - 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,						\
>> +}

Absolutely :)

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

David mentionned that as well, I'll drop this as this is no longer
required. It used to be required before switching to devm_ helpers.

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

True, I'll change that

> 
> 
>> +	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);
>> +}

Thanks for the reviews :)

Maxime