* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-23 15:55 ` [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc Maxime Chevallier
@ 2025-12-23 17:12 ` Maxime Chevallier
2025-12-23 17:33 ` David Lechner
2025-12-23 18:26 ` David Lechner
` (3 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: Maxime Chevallier @ 2025-12-23 17:12 UTC (permalink / raw)
To: Jonathan Cameron, David Lechner, nuno.sa, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Matti Vaittinen, Antoniu Miclaus, Angelo Dureghello,
Tobias Sperling, Eason Yang, Marilene Andrade Garcia,
Cosmin Tanislav, duje, herve.codina, Rodolfo Giometti
Cc: linux-iio, devicetree, linux-kernel, thomas.petazzoni
Hi again,
On 23/12/2025 16:55, Maxime Chevallier 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>
Looking closer at this, Rodolfo hasn't seen this patch prior to me
sending it, so it should rather be :
Orginally-by: Rodolfo Giometti <giometti@enneenne.com>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
I can resend if need be :)
Maxime
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-23 17:12 ` Maxime Chevallier
@ 2025-12-23 17:33 ` David Lechner
2025-12-27 14:43 ` Andy Shevchenko
0 siblings, 1 reply; 22+ messages in thread
From: David Lechner @ 2025-12-23 17:33 UTC (permalink / raw)
To: Maxime Chevallier, Jonathan Cameron, nuno.sa, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Matti Vaittinen, Antoniu Miclaus, Angelo Dureghello,
Tobias Sperling, Eason Yang, Marilene Andrade Garcia,
Cosmin Tanislav, duje, herve.codina, Rodolfo Giometti
Cc: linux-iio, devicetree, linux-kernel, thomas.petazzoni
On 12/23/25 11:12 AM, Maxime Chevallier wrote:
> Hi again,
>
> On 23/12/2025 16:55, Maxime Chevallier 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>
>
> Looking closer at this, Rodolfo hasn't seen this patch prior to me
> sending it, so it should rather be :
>
> Orginally-by: Rodolfo Giometti <giometti@enneenne.com>
I think the usual way would be to keep the Signed-off-by: and add
Co-developed-by:.
See https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
>
>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>
> I can resend if need be :)
Wait a week for other reviews. :-)
>
> Maxime
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-23 17:33 ` David Lechner
@ 2025-12-27 14:43 ` Andy Shevchenko
0 siblings, 0 replies; 22+ messages in thread
From: Andy Shevchenko @ 2025-12-27 14:43 UTC (permalink / raw)
To: David Lechner
Cc: Maxime Chevallier, Jonathan Cameron, nuno.sa, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Matti Vaittinen, Antoniu Miclaus, Angelo Dureghello,
Tobias Sperling, Eason Yang, Marilene Andrade Garcia,
Cosmin Tanislav, duje, herve.codina, Rodolfo Giometti, linux-iio,
devicetree, linux-kernel, thomas.petazzoni
On Tue, Dec 23, 2025 at 11:33:02AM -0600, David Lechner wrote:
> On 12/23/25 11:12 AM, Maxime Chevallier wrote:
> > On 23/12/2025 16:55, Maxime Chevallier 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>
> >
> > Looking closer at this, Rodolfo hasn't seen this patch prior to me
> > sending it, so it should rather be :
> >
> > Orginally-by: Rodolfo Giometti <giometti@enneenne.com>
>
> I think the usual way would be to keep the Signed-off-by: and add
> Co-developed-by:.
>
> See https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
Hmm... I think the Originally-by is used when the contributor gave
a PoC / basic idea in a form of code that was (heavily?) rewritten.
The only documentation mentions this tag is
Documentation/process/maintainer-tip.rst.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-23 15:55 ` [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc Maxime Chevallier
2025-12-23 17:12 ` Maxime Chevallier
@ 2025-12-23 18:26 ` David Lechner
2026-01-05 10:11 ` Maxime Chevallier
2025-12-27 18:42 ` Jonathan Cameron
` (2 subsequent siblings)
4 siblings, 1 reply; 22+ messages in thread
From: David Lechner @ 2025-12-23 18:26 UTC (permalink / raw)
To: Maxime Chevallier, Jonathan Cameron, nuno.sa, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Matti Vaittinen, Antoniu Miclaus, Angelo Dureghello,
Tobias Sperling, Eason Yang, Marilene Andrade Garcia,
Cosmin Tanislav, duje, herve.codina, Rodolfo Giometti
Cc: linux-iio, devicetree, linux-kernel, thomas.petazzoni
On 12/23/25 9:55 AM, Maxime Chevallier wrote:
> This adds a new driver for the TI TLA2528 ADC chip. It ha 8 12-bit
s/This adds/Add/
s/ha/has/
> channels, that can also be configured as 16-bit averaging channels.
>
> Add a very simple driver for it, allowing reading raw values for each
Don't need to say we are adding a driver twice.
> channel.
>
> Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
> MAINTAINERS | 7 ++
> drivers/iio/adc/Kconfig | 10 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-tla2528.c | 209 +++++++++++++++++++++++++++++++++++
> 4 files changed, 227 insertions(+)
> create mode 100644 drivers/iio/adc/ti-tla2528.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dc731d37c8fe..5c382ae216c7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25866,6 +25866,13 @@ F: include/dt-bindings/soc/ti,sci_pm_domain.h
> F: include/linux/soc/ti/ti_sci_inta_msi.h
> F: include/linux/soc/ti/ti_sci_protocol.h
>
> +TEXAS INSTRUMENTS' TLA2528 ADC DRIVER
> +M: Maxime Chevallier <maxime.chevallier@bootlin.com>
> +L: linux-iio@vger.kernel.org
> +S: Supported
> +F: Documentation/devicetree/bindings/iio/adc/ti,tla2528.yaml
This part can be included in the dt-bindings patch since that is where
the file is introduced.
> +F: drivers/iio/adc/ti-tla2528.c
And just keep this line in this patch.
> +
> TEXAS INSTRUMENTS' TMP117 TEMPERATURE SENSOR DRIVER
> M: Puranjay Mohan <puranjay@kernel.org>
> L: linux-iio@vger.kernel.org
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 58da8255525e..67376de410bf 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1803,6 +1803,16 @@ config TI_LMP92064
> This driver can also be built as a module. If so, the module will be called
> ti-lmp92064.
>
> +config TI_TLA2528
> + tristate "Texas Instruments TLA2528 ADC driver"
> + depends on I2C
> + help
> + Say yes here to build support for Texas Instruments TLA2528
> + 12-Bit 8-Channel ADC.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called ti-tla2528.
> +
> config TI_TLC4541
> tristate "Texas Instruments TLC4541 ADC driver"
> depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 7cc8f9a12f76..941606defbf7 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -157,6 +157,7 @@ obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o
> obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
> obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> obj-$(CONFIG_TI_LMP92064) += ti-lmp92064.o
> +obj-$(CONFIG_TI_TLA2528) += ti-tla2528.o
> obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
> obj-$(CONFIG_TI_TSC2046) += ti-tsc2046.o
> obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
> 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 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Texas Instruments TLA2528 ADC
> + *
> + * Copyright (C) 2020-2021 Rodolfo Giometti <giometti@enneenne.com>
> + * Copyright (C) 2025 Maxime Chevallier <maxime.chevallier@bootlin.com>
> + */
> +
> +#include <linux/delay.h>
Check the headers to see what is actually used. For example,
I don't seen any delays/sleeps. And we have a mutex, but no
mutex header.
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define TLA2528_OP_WRITE_REG 0x08
> +
> +#define TLA2528_DATA_CFG_ADR 0x02
> +
> +/* Datasheet says [5:4] sets the append status, but only bit 4 is used */
> +#define TLA2528_DATA_CFG_APPEND_STATUS BIT(4)
> +#define TLA2528_PIN_CFG_ADR 0x05
> +#define TLA2528_SEQUENCE_CFG_ADR 0x10
> +#define TLA2528_CHANNEL_SEL_ADR 0x11
> +
> +struct tla2528 {
> + struct i2c_client *client;
> + int vref_uv;
> +
> + /* Protects manual channel selection, i.e. last_read_channel */
> + struct mutex lock;
> + u8 last_read_channel;
> +};
> +
> +static s32 tla2528_write_reg(const struct i2c_client *client, u8 reg, u8 val)
Usually type is just "int" for error code returns.
> +{
> + u8 data[3] = {TLA2528_OP_WRITE_REG, reg, val};
> + int ret;
> +
> + ret = i2c_master_send(client, data, 3);
> +
> + return ret < 0 ? ret : 0;
> +}
> +
Would it make sense to use regmap instead?
> +static int tla2528_read_sample(const struct i2c_client *client)
> +{
> + __be16 data;
> + int ret;
> +
Don't we need to set the CNVST bit in GENERAL_CFG register to trigger
a conversion in manual mode?
> + ret = i2c_master_recv(client, (char *)&data, 2);
> + if (ret < 0)
> + return ret;
> +
> + return be16_to_cpu(data) >> 4;
> +}
> +
> +static int tla2528_read(struct tla2528 *tla2528, u8 channel, int *val)
> +{
> + struct i2c_client *client = tla2528->client;
> + int ret;
> +
> + if (channel != tla2528->last_read_channel) {
> + ret = tla2528_write_reg(client, TLA2528_CHANNEL_SEL_ADR, channel);
> + if (ret < 0)
> + return ret;
> +
> + tla2528->last_read_channel = channel;
> + }
If we implemented regmap with cache, then we could avoid having to
track last_read_channel. We could just call regmap_write() unconditionally
and the regmap framework would decide if it needs to actually do the write
or not.
> +
> + ret = tla2528_read_sample(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_SCALE:
> + *val = tla2528->vref_uv / 1000;
Why not just store vref_mV?
> + *val2 = 12;
> +
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +#define TLA2528_CHAN(_chan, _name) { \
> + .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 const struct iio_chan_spec tla2528_channel[] = {
> + TLA2528_CHAN(0, "AIN0"),
> + TLA2528_CHAN(1, "AIN1"),
> + TLA2528_CHAN(2, "AIN2"),
> + TLA2528_CHAN(3, "AIN3"),
> + TLA2528_CHAN(4, "AIN4"),
> + TLA2528_CHAN(5, "AIN5"),
> + TLA2528_CHAN(6, "AIN6"),
> + TLA2528_CHAN(7, "AIN7"),
> +};
> +
> +static const struct iio_info tla2528_info = {
> + .read_raw = tla2528_read_raw,
> +};
> +
> +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;
Perhaps we should also fail if the adapter has I2C_AQ_NO_CLK_STRETCH?
It looks like clock stretching is required for the conversion time.
> +
> + 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);
Not sure this is needed if there is no i2c_get_clientdata() anywhere.
> + tla2528->client = client;
> +
> + indio_dev->name = client->name;
This should be the chip name ("tla2528"), not the I2C device name.
> + 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);
Use dem_mutex_init().
> +
> + 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);
Why? It doesn't appear to be used.
> + if (ret < 0)
> + return ret;
> +
> + /* Set manual mode */
> + ret = tla2528_write_reg(tla2528->client, TLA2528_SEQUENCE_CFG_ADR, 0x00);
> + if (ret < 0)
> + return ret;
> +
Perhaps simpler to just write the RST bit GENERAL_CFG to reset everything
to a known state?
> + /* Init private data */
> + tla2528->last_read_channel = ~0;
> +
> + return devm_iio_device_register(&client->dev, indio_dev);
> +}
> +
> +static const struct i2c_device_id tla2528_id[] = {
> + { "tla2528", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, tla2528_id);
> +
> +static const struct of_device_id tla2528_of_match[] = {
> + { .compatible = "ti,tla2528", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, tla2528_of_match);
> +
> +static struct i2c_driver tla2528_driver = {
> + .driver = {
> + .name = "tla2528",
> + .of_match_table = tla2528_of_match,
> + },
> + .probe = tla2528_probe,
> + .id_table = tla2528_id,
> +};
> +module_i2c_driver(tla2528_driver);
> +
> +MODULE_AUTHOR("Maxime Chevallier <maxime.chevallier@bootlin.com>");
> +MODULE_AUTHOR("Rodolfo Giometti <giometti@enneenne.com>");
> +MODULE_DESCRIPTION("Texas Instruments TLA2528 ADC driver");
> +MODULE_LICENSE("GPL");
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-23 18:26 ` David Lechner
@ 2026-01-05 10:11 ` Maxime Chevallier
0 siblings, 0 replies; 22+ messages in thread
From: Maxime Chevallier @ 2026-01-05 10:11 UTC (permalink / raw)
To: David Lechner, Jonathan Cameron, nuno.sa, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Matti Vaittinen, Antoniu Miclaus, Angelo Dureghello,
Tobias Sperling, Eason Yang, Marilene Andrade Garcia,
Cosmin Tanislav, duje, herve.codina, Rodolfo Giometti
Cc: linux-iio, devicetree, linux-kernel, thomas.petazzoni
Hi David,
On 23/12/2025 19:26, David Lechner wrote:
> On 12/23/25 9:55 AM, Maxime Chevallier wrote:
>> This adds a new driver for the TI TLA2528 ADC chip. It ha 8 12-bit
>
> s/This adds/Add/
>
> s/ha/has/
>
>> channels, that can also be configured as 16-bit averaging channels.
>>
>> Add a very simple driver for it, allowing reading raw values for each
>
> Don't need to say we are adding a driver twice.
I'll reword accordingly, thanks :)
>
>> channel.
>>
>> Signed-off-by: Rodolfo Giometti <giometti@enneenne.com>
>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
>> ---
>> MAINTAINERS | 7 ++
>> drivers/iio/adc/Kconfig | 10 ++
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/ti-tla2528.c | 209 +++++++++++++++++++++++++++++++++++
>> 4 files changed, 227 insertions(+)
>> create mode 100644 drivers/iio/adc/ti-tla2528.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index dc731d37c8fe..5c382ae216c7 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -25866,6 +25866,13 @@ F: include/dt-bindings/soc/ti,sci_pm_domain.h
>> F: include/linux/soc/ti/ti_sci_inta_msi.h
>> F: include/linux/soc/ti/ti_sci_protocol.h
>>
>> +TEXAS INSTRUMENTS' TLA2528 ADC DRIVER
>> +M: Maxime Chevallier <maxime.chevallier@bootlin.com>
>> +L: linux-iio@vger.kernel.org
>> +S: Supported
>> +F: Documentation/devicetree/bindings/iio/adc/ti,tla2528.yaml
>
> This part can be included in the dt-bindings patch since that is where
> the file is introduced.
>
>> +F: drivers/iio/adc/ti-tla2528.c
>
> And just keep this line in this patch.
ACK, no problem
>
>> +
>> TEXAS INSTRUMENTS' TMP117 TEMPERATURE SENSOR DRIVER
>> M: Puranjay Mohan <puranjay@kernel.org>
>> L: linux-iio@vger.kernel.org
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 58da8255525e..67376de410bf 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -1803,6 +1803,16 @@ config TI_LMP92064
>> This driver can also be built as a module. If so, the module will be called
>> ti-lmp92064.
>>
>> +config TI_TLA2528
>> + tristate "Texas Instruments TLA2528 ADC driver"
>> + depends on I2C
>> + help
>> + Say yes here to build support for Texas Instruments TLA2528
>> + 12-Bit 8-Channel ADC.
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called ti-tla2528.
>> +
>> config TI_TLC4541
>> tristate "Texas Instruments TLC4541 ADC driver"
>> depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 7cc8f9a12f76..941606defbf7 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -157,6 +157,7 @@ obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o
>> obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
>> obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>> obj-$(CONFIG_TI_LMP92064) += ti-lmp92064.o
>> +obj-$(CONFIG_TI_TLA2528) += ti-tla2528.o
>> obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
>> obj-$(CONFIG_TI_TSC2046) += ti-tsc2046.o
>> obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
>> 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 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for Texas Instruments TLA2528 ADC
>> + *
>> + * Copyright (C) 2020-2021 Rodolfo Giometti <giometti@enneenne.com>
>> + * Copyright (C) 2025 Maxime Chevallier <maxime.chevallier@bootlin.com>
>> + */
>> +
>> +#include <linux/delay.h>
>
> Check the headers to see what is actually used. For example,
> I don't seen any delays/sleeps. And we have a mutex, but no
> mutex header.
I'll give another pass at the includes, thanks :)
>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <linux/iio/iio.h>
>> +
>> +#define TLA2528_OP_WRITE_REG 0x08
>> +
>> +#define TLA2528_DATA_CFG_ADR 0x02
>> +
>> +/* Datasheet says [5:4] sets the append status, but only bit 4 is used */
>> +#define TLA2528_DATA_CFG_APPEND_STATUS BIT(4)
>> +#define TLA2528_PIN_CFG_ADR 0x05
>> +#define TLA2528_SEQUENCE_CFG_ADR 0x10
>> +#define TLA2528_CHANNEL_SEL_ADR 0x11
>> +
>> +struct tla2528 {
>> + struct i2c_client *client;
>> + int vref_uv;
>> +
>> + /* Protects manual channel selection, i.e. last_read_channel */
>> + struct mutex lock;
>> + u8 last_read_channel;
>> +};
>> +
>> +static s32 tla2528_write_reg(const struct i2c_client *client, u8 reg, u8 val)
>
> Usually type is just "int" for error code returns.
yeah int makes sense, that's a leftover form the rewriting i've made...
>
>> +{
>> + u8 data[3] = {TLA2528_OP_WRITE_REG, reg, val};
>> + int ret;
>> +
>> + ret = i2c_master_send(client, data, 3);
>> +
>> + return ret < 0 ? ret : 0;
>> +}
>> +
>
> Would it make sense to use regmap instead?
I think so yeah, let me add that to the next iteration. Based on your
comments further down, it should really be worth it.
>
>
>> +static int tla2528_read_sample(const struct i2c_client *client)
>> +{
>> + __be16 data;
>> + int ret;
>> +
>
> Don't we need to set the CNVST bit in GENERAL_CFG register to trigger
> a conversion in manual mode?
Let me double-check. From what I've tested, it seems to work without
this, but this may be out of luck.
>
>> + ret = i2c_master_recv(client, (char *)&data, 2);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return be16_to_cpu(data) >> 4;
>> +}
>> +
>> +static int tla2528_read(struct tla2528 *tla2528, u8 channel, int *val)
>> +{
>> + struct i2c_client *client = tla2528->client;
>> + int ret;
>> +
>> + if (channel != tla2528->last_read_channel) {
>> + ret = tla2528_write_reg(client, TLA2528_CHANNEL_SEL_ADR, channel);
>> + if (ret < 0)
>> + return ret;
>> +
>> + tla2528->last_read_channel = channel;
>> + }
>
> If we implemented regmap with cache, then we could avoid having to
> track last_read_channel. We could just call regmap_write() unconditionally
> and the regmap framework would decide if it needs to actually do the write
> or not.
True, that would simplify code so much ! I'll add that
>> +
>> + ret = tla2528_read_sample(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_SCALE:
>> + *val = tla2528->vref_uv / 1000;
>
> Why not just store vref_mV?
heh very good point :)
>
>> + *val2 = 12;
>> +
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +#define TLA2528_CHAN(_chan, _name) { \
>> + .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 const struct iio_chan_spec tla2528_channel[] = {
>> + TLA2528_CHAN(0, "AIN0"),
>> + TLA2528_CHAN(1, "AIN1"),
>> + TLA2528_CHAN(2, "AIN2"),
>> + TLA2528_CHAN(3, "AIN3"),
>> + TLA2528_CHAN(4, "AIN4"),
>> + TLA2528_CHAN(5, "AIN5"),
>> + TLA2528_CHAN(6, "AIN6"),
>> + TLA2528_CHAN(7, "AIN7"),
>> +};
>> +
>> +static const struct iio_info tla2528_info = {
>> + .read_raw = tla2528_read_raw,
>> +};
>> +
>> +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;
>
> Perhaps we should also fail if the adapter has I2C_AQ_NO_CLK_STRETCH?
> It looks like clock stretching is required for the conversion time.
Good point indeed, I'll add that
>
>> +
>> + 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);
>
> Not sure this is needed if there is no i2c_get_clientdata() anywhere.
Ah yeah this is a leftover from the devm_* conversion, thanks !
>
>> + tla2528->client = client;
>> +
>
>> + indio_dev->name = client->name;
>
> This should be the chip name ("tla2528"), not the I2C device name.
Fair, 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);
>
> Use dem_mutex_init().
right :)
>
>> +
>> + 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);
>
> Why? It doesn't appear to be used.
>
>> + if (ret < 0)
>> + return ret;
>> +
>> + /* Set manual mode */
>> + ret = tla2528_write_reg(tla2528->client, TLA2528_SEQUENCE_CFG_ADR, 0x00);
>> + if (ret < 0)
>> + return ret;
>> +
>
>
> Perhaps simpler to just write the RST bit GENERAL_CFG to reset everything
> to a known state?
Alright, I'll clean-up that init sequence
>
>
>> + /* Init private data */
>> + tla2528->last_read_channel = ~0;
>> +
>> + return devm_iio_device_register(&client->dev, indio_dev);
>> +}
>> +
>> +static const struct i2c_device_id tla2528_id[] = {
>> + { "tla2528", 0 },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, tla2528_id);
>> +
>> +static const struct of_device_id tla2528_of_match[] = {
>> + { .compatible = "ti,tla2528", },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, tla2528_of_match);
>> +
>> +static struct i2c_driver tla2528_driver = {
>> + .driver = {
>> + .name = "tla2528",
>> + .of_match_table = tla2528_of_match,
>> + },
>> + .probe = tla2528_probe,
>> + .id_table = tla2528_id,
>> +};
>> +module_i2c_driver(tla2528_driver);
>> +
>> +MODULE_AUTHOR("Maxime Chevallier <maxime.chevallier@bootlin.com>");
>> +MODULE_AUTHOR("Rodolfo Giometti <giometti@enneenne.com>");
>> +MODULE_DESCRIPTION("Texas Instruments TLA2528 ADC driver");
>> +MODULE_LICENSE("GPL");
>
Thank you very much for the thourough review !
Maxime
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-23 15:55 ` [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc Maxime Chevallier
2025-12-23 17:12 ` Maxime Chevallier
2025-12-23 18:26 ` David Lechner
@ 2025-12-27 18:42 ` Jonathan Cameron
2026-01-05 10:16 ` Maxime Chevallier
2025-12-29 8:20 ` Matti Vaittinen
2025-12-29 9:39 ` Andy Shevchenko
4 siblings, 1 reply; 22+ messages in thread
From: Jonathan Cameron @ 2025-12-27 18:42 UTC (permalink / raw)
To: Maxime Chevallier
Cc: David Lechner, nuno.sa, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Matti Vaittinen, Antoniu Miclaus, Angelo Dureghello,
Tobias Sperling, Eason Yang, Marilene Andrade Garcia,
Cosmin Tanislav, duje, herve.codina, Rodolfo Giometti, linux-iio,
devicetree, linux-kernel, thomas.petazzoni
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);
> +}
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-27 18:42 ` Jonathan Cameron
@ 2026-01-05 10:16 ` Maxime Chevallier
0 siblings, 0 replies; 22+ messages in thread
From: Maxime Chevallier @ 2026-01-05 10:16 UTC (permalink / raw)
To: Jonathan Cameron
Cc: David Lechner, nuno.sa, Andy Shevchenko, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Matti Vaittinen, Antoniu Miclaus, Angelo Dureghello,
Tobias Sperling, Eason Yang, Marilene Andrade Garcia,
Cosmin Tanislav, duje, herve.codina, Rodolfo Giometti, linux-iio,
devicetree, linux-kernel, thomas.petazzoni
Hi Jonathan,
On 27/12/2025 19:42, Jonathan Cameron wrote:
> 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 };
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
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-23 15:55 ` [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc Maxime Chevallier
` (2 preceding siblings ...)
2025-12-27 18:42 ` Jonathan Cameron
@ 2025-12-29 8:20 ` Matti Vaittinen
2025-12-31 17:12 ` Jonathan Cameron
2026-01-05 10:25 ` Maxime Chevallier
2025-12-29 9:39 ` Andy Shevchenko
4 siblings, 2 replies; 22+ messages in thread
From: Matti Vaittinen @ 2025-12-29 8:20 UTC (permalink / raw)
To: Maxime Chevallier, Jonathan Cameron, David Lechner, nuno.sa,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Marcelo Schmitt, Antoniu Miclaus, Angelo Dureghello,
Tobias Sperling, Eason Yang, Marilene Andrade Garcia,
Cosmin Tanislav, duje, herve.codina, Rodolfo Giometti
Cc: linux-iio, devicetree, linux-kernel, thomas.petazzoni
On 23/12/2025 17:55, Maxime Chevallier 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>
> ---
> MAINTAINERS | 7 ++
> drivers/iio/adc/Kconfig | 10 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ti-tla2528.c | 209 +++++++++++++++++++++++++++++++++++
> 4 files changed, 227 insertions(+)
> create mode 100644 drivers/iio/adc/ti-tla2528.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index dc731d37c8fe..5c382ae216c7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -25866,6 +25866,13 @@ F: include/dt-bindings/soc/ti,sci_pm_domain.h
> F: include/linux/soc/ti/ti_sci_inta_msi.h
> F: include/linux/soc/ti/ti_sci_protocol.h
>
> +TEXAS INSTRUMENTS' TLA2528 ADC DRIVER
> +M: Maxime Chevallier <maxime.chevallier@bootlin.com>
> +L: linux-iio@vger.kernel.org
> +S: Supported
> +F: Documentation/devicetree/bindings/iio/adc/ti,tla2528.yaml
> +F: drivers/iio/adc/ti-tla2528.c
> +
> TEXAS INSTRUMENTS' TMP117 TEMPERATURE SENSOR DRIVER
> M: Puranjay Mohan <puranjay@kernel.org>
> L: linux-iio@vger.kernel.org
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 58da8255525e..67376de410bf 100644
Hmm. Would it ease merging if MAINTAINERS changes were in their own patch?
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -1803,6 +1803,16 @@ config TI_LMP92064
> This driver can also be built as a module. If so, the module will be called
> ti-lmp92064.
>
> +config TI_TLA2528
> + tristate "Texas Instruments TLA2528 ADC driver"
> + depends on I2C
> + help
> + Say yes here to build support for Texas Instruments TLA2528
> + 12-Bit 8-Channel ADC.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called ti-tla2528.
> +
> config TI_TLC4541
> tristate "Texas Instruments TLC4541 ADC driver"
> depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index 7cc8f9a12f76..941606defbf7 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -157,6 +157,7 @@ obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o
> obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
> obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
> obj-$(CONFIG_TI_LMP92064) += ti-lmp92064.o
> +obj-$(CONFIG_TI_TLA2528) += ti-tla2528.o
> obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
> obj-$(CONFIG_TI_TSC2046) += ti-tsc2046.o
> obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
> 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 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for Texas Instruments TLA2528 ADC
> + *
> + * Copyright (C) 2020-2021 Rodolfo Giometti <giometti@enneenne.com>
> + * Copyright (C) 2025 Maxime Chevallier <maxime.chevallier@bootlin.com>
> + */
> +
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <linux/iio/iio.h>
> +
> +#define TLA2528_OP_WRITE_REG 0x08
> +
> +#define TLA2528_DATA_CFG_ADR 0x02
> +
> +/* Datasheet says [5:4] sets the append status, but only bit 4 is used */
> +#define TLA2528_DATA_CFG_APPEND_STATUS BIT(4)
> +#define TLA2528_PIN_CFG_ADR 0x05
> +#define TLA2528_SEQUENCE_CFG_ADR 0x10
> +#define TLA2528_CHANNEL_SEL_ADR 0x11
> +
> +struct tla2528 {
> + struct i2c_client *client;
> + int vref_uv;
> +
> + /* Protects manual channel selection, i.e. last_read_channel */
> + struct mutex lock;
> + u8 last_read_channel;
> +};
> +
> +static s32 tla2528_write_reg(const struct i2c_client *client, u8 reg, u8 val)
> +{
> + u8 data[3] = {TLA2528_OP_WRITE_REG, reg, val};
> + 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);
> + if (ret < 0)
> + return ret;
> +
> + return be16_to_cpu(data) >> 4;
> +}
Can regmap be used for all the usual benefits?
> +
> +static int tla2528_read(struct tla2528 *tla2528, u8 channel, int *val)
> +{
> + struct i2c_client *client = tla2528->client;
> + int ret;
> +
> + if (channel != tla2528->last_read_channel) {
> + ret = tla2528_write_reg(client, TLA2528_CHANNEL_SEL_ADR, channel);
> + if (ret < 0)
> + return ret;
> +
> + tla2528->last_read_channel = channel;
> + }
> +
> + ret = tla2528_read_sample(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_SCALE:
> + *val = tla2528->vref_uv / 1000;
> + *val2 = 12;
> +
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +#define TLA2528_CHAN(_chan, _name) { \
> + .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 const struct iio_chan_spec tla2528_channel[] = {
> + TLA2528_CHAN(0, "AIN0"),
> + TLA2528_CHAN(1, "AIN1"),
> + TLA2528_CHAN(2, "AIN2"),
> + TLA2528_CHAN(3, "AIN3"),
> + TLA2528_CHAN(4, "AIN4"),
> + TLA2528_CHAN(5, "AIN5"),
> + TLA2528_CHAN(6, "AIN6"),
> + TLA2528_CHAN(7, "AIN7"),
> +};
Not really insisting a change here, merely giving a nudge :)
I wonder if the channels should be obtained from fwnode? I think the
dt-review mentioned channel inputs may be used for GPIOs? One might be
able to use the devm_iio_adc_device_alloc_chaninfo_se() to build the
chan_spec based on the dt - depending on the non dt use-cases.
All in all, I think this is clean and nice driver.
> +static const struct iio_info tla2528_info = {
> + .read_raw = tla2528_read_raw,
> +};
> +
> +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);
> + tla2528->client = client;
> +
> + indio_dev->name = client->name;
> + 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;
As mentioned above, perhaps get the channels from the DT, and only mux
the given channels?
> +
> + 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);
> +}
> +
> +static const struct i2c_device_id tla2528_id[] = {
> + { "tla2528", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, tla2528_id);
> +
> +static const struct of_device_id tla2528_of_match[] = {
> + { .compatible = "ti,tla2528", },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, tla2528_of_match);
> +
> +static struct i2c_driver tla2528_driver = {
> + .driver = {
> + .name = "tla2528",
> + .of_match_table = tla2528_of_match,
> + },
> + .probe = tla2528_probe,
> + .id_table = tla2528_id,
> +};
> +module_i2c_driver(tla2528_driver);
> +
> +MODULE_AUTHOR("Maxime Chevallier <maxime.chevallier@bootlin.com>");
> +MODULE_AUTHOR("Rodolfo Giometti <giometti@enneenne.com>");
> +MODULE_DESCRIPTION("Texas Instruments TLA2528 ADC driver");
> +MODULE_LICENSE("GPL");
Yours,
-- Matti
--
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-29 8:20 ` Matti Vaittinen
@ 2025-12-31 17:12 ` Jonathan Cameron
2026-01-02 7:13 ` Matti Vaittinen
2026-01-05 10:25 ` Maxime Chevallier
1 sibling, 1 reply; 22+ messages in thread
From: Jonathan Cameron @ 2025-12-31 17:12 UTC (permalink / raw)
To: Matti Vaittinen
Cc: Maxime Chevallier, David Lechner, nuno.sa, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Antoniu Miclaus, Angelo Dureghello, Tobias Sperling, Eason Yang,
Marilene Andrade Garcia, Cosmin Tanislav, duje, herve.codina,
Rodolfo Giometti, linux-iio, devicetree, linux-kernel,
thomas.petazzoni
On Mon, 29 Dec 2025 10:20:23 +0200
Matti Vaittinen <mazziesaccount@gmail.com> wrote:
> On 23/12/2025 17:55, Maxime Chevallier 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>
> > ---
> > MAINTAINERS | 7 ++
> > drivers/iio/adc/Kconfig | 10 ++
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ti-tla2528.c | 209 +++++++++++++++++++++++++++++++++++
> > 4 files changed, 227 insertions(+)
> > create mode 100644 drivers/iio/adc/ti-tla2528.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index dc731d37c8fe..5c382ae216c7 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -25866,6 +25866,13 @@ F: include/dt-bindings/soc/ti,sci_pm_domain.h
> > F: include/linux/soc/ti/ti_sci_inta_msi.h
> > F: include/linux/soc/ti/ti_sci_protocol.h
> >
> > +TEXAS INSTRUMENTS' TLA2528 ADC DRIVER
> > +M: Maxime Chevallier <maxime.chevallier@bootlin.com>
> > +L: linux-iio@vger.kernel.org
> > +S: Supported
> > +F: Documentation/devicetree/bindings/iio/adc/ti,tla2528.yaml
> > +F: drivers/iio/adc/ti-tla2528.c
> > +
> > TEXAS INSTRUMENTS' TMP117 TEMPERATURE SENSOR DRIVER
> > M: Puranjay Mohan <puranjay@kernel.org>
> > L: linux-iio@vger.kernel.org
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 58da8255525e..67376de410bf 100644
>
> Hmm. Would it ease merging if MAINTAINERS changes were in their own patch?
Not particularly. Though I personally slightly prefer the logic
of bringing the entry in with the first file, then adding additional files
in later patches.
Given it is huge and in alphabetical order, conflicts in MAINTAINERS are
fairly rare and trivial to resolve.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-31 17:12 ` Jonathan Cameron
@ 2026-01-02 7:13 ` Matti Vaittinen
0 siblings, 0 replies; 22+ messages in thread
From: Matti Vaittinen @ 2026-01-02 7:13 UTC (permalink / raw)
To: Jonathan Cameron
Cc: Maxime Chevallier, David Lechner, nuno.sa, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Antoniu Miclaus, Angelo Dureghello, Tobias Sperling, Eason Yang,
Marilene Andrade Garcia, Cosmin Tanislav, duje, herve.codina,
Rodolfo Giometti, linux-iio, devicetree, linux-kernel,
thomas.petazzoni
On 31/12/2025 19:12, Jonathan Cameron wrote:
> On Mon, 29 Dec 2025 10:20:23 +0200
> Matti Vaittinen <mazziesaccount@gmail.com> wrote:
>
>> On 23/12/2025 17:55, Maxime Chevallier 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>
>>> ---
>>> MAINTAINERS | 7 ++
>>> drivers/iio/adc/Kconfig | 10 ++
>>> drivers/iio/adc/Makefile | 1 +
>>> drivers/iio/adc/ti-tla2528.c | 209 +++++++++++++++++++++++++++++++++++
>>> 4 files changed, 227 insertions(+)
>>> create mode 100644 drivers/iio/adc/ti-tla2528.c
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index dc731d37c8fe..5c382ae216c7 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -25866,6 +25866,13 @@ F: include/dt-bindings/soc/ti,sci_pm_domain.h
>>> F: include/linux/soc/ti/ti_sci_inta_msi.h
>>> F: include/linux/soc/ti/ti_sci_protocol.h
>>>
>>> +TEXAS INSTRUMENTS' TLA2528 ADC DRIVER
>>> +M: Maxime Chevallier <maxime.chevallier@bootlin.com>
>>> +L: linux-iio@vger.kernel.org
>>> +S: Supported
>>> +F: Documentation/devicetree/bindings/iio/adc/ti,tla2528.yaml
>>> +F: drivers/iio/adc/ti-tla2528.c
>>> +
>>> TEXAS INSTRUMENTS' TMP117 TEMPERATURE SENSOR DRIVER
>>> M: Puranjay Mohan <puranjay@kernel.org>
>>> L: linux-iio@vger.kernel.org
>>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>>> index 58da8255525e..67376de410bf 100644
>>
>> Hmm. Would it ease merging if MAINTAINERS changes were in their own patch?
>
> Not particularly. Though I personally slightly prefer the logic
> of bringing the entry in with the first file, then adding additional files
> in later patches.
>
> Given it is huge and in alphabetical order, conflicts in MAINTAINERS are
> fairly rare and trivial to resolve.
>
Thanks for this clarification :)
I don't know where I had picked up this idea, but I thought that the
volume of changes in MAINTAINERs was somewhat annoying source of
conflicts. I sit and type corrected :)
Yours,
-- Matti
---
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-29 8:20 ` Matti Vaittinen
2025-12-31 17:12 ` Jonathan Cameron
@ 2026-01-05 10:25 ` Maxime Chevallier
1 sibling, 0 replies; 22+ messages in thread
From: Maxime Chevallier @ 2026-01-05 10:25 UTC (permalink / raw)
To: Matti Vaittinen, Jonathan Cameron, David Lechner, nuno.sa,
Andy Shevchenko, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Marcelo Schmitt, Antoniu Miclaus, Angelo Dureghello,
Tobias Sperling, Eason Yang, Marilene Andrade Garcia,
Cosmin Tanislav, duje, herve.codina, Rodolfo Giometti
Cc: linux-iio, devicetree, linux-kernel, thomas.petazzoni
Hi Matti,
On 29/12/2025 09:20, Matti Vaittinen wrote:
> On 23/12/2025 17:55, Maxime Chevallier 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>
>> ---
>> MAINTAINERS | 7 ++
>> drivers/iio/adc/Kconfig | 10 ++
>> drivers/iio/adc/Makefile | 1 +
>> drivers/iio/adc/ti-tla2528.c | 209 +++++++++++++++++++++++++++++++++++
>> 4 files changed, 227 insertions(+)
>> create mode 100644 drivers/iio/adc/ti-tla2528.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index dc731d37c8fe..5c382ae216c7 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -25866,6 +25866,13 @@ F: include/dt-bindings/soc/ti,sci_pm_domain.h
>> F: include/linux/soc/ti/ti_sci_inta_msi.h
>> F: include/linux/soc/ti/ti_sci_protocol.h
>>
>> +TEXAS INSTRUMENTS' TLA2528 ADC DRIVER
>> +M: Maxime Chevallier <maxime.chevallier@bootlin.com>
>> +L: linux-iio@vger.kernel.org
>> +S: Supported
>> +F: Documentation/devicetree/bindings/iio/adc/ti,tla2528.yaml
>> +F: drivers/iio/adc/ti-tla2528.c
>> +
>> TEXAS INSTRUMENTS' TMP117 TEMPERATURE SENSOR DRIVER
>> M: Puranjay Mohan <puranjay@kernel.org>
>> L: linux-iio@vger.kernel.org
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
>> index 58da8255525e..67376de410bf 100644
>
> Hmm. Would it ease merging if MAINTAINERS changes were in their own patch?
>
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -1803,6 +1803,16 @@ config TI_LMP92064
>> This driver can also be built as a module. If so, the module will be called
>> ti-lmp92064.
>>
>> +config TI_TLA2528
>> + tristate "Texas Instruments TLA2528 ADC driver"
>> + depends on I2C
>> + help
>> + Say yes here to build support for Texas Instruments TLA2528
>> + 12-Bit 8-Channel ADC.
>> +
>> + To compile this driver as a module, choose M here: the module will be
>> + called ti-tla2528.
>> +
>> config TI_TLC4541
>> tristate "Texas Instruments TLC4541 ADC driver"
>> depends on SPI
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
>> index 7cc8f9a12f76..941606defbf7 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -157,6 +157,7 @@ obj-$(CONFIG_TI_ADS8344) += ti-ads8344.o
>> obj-$(CONFIG_TI_ADS8688) += ti-ads8688.o
>> obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>> obj-$(CONFIG_TI_LMP92064) += ti-lmp92064.o
>> +obj-$(CONFIG_TI_TLA2528) += ti-tla2528.o
>> obj-$(CONFIG_TI_TLC4541) += ti-tlc4541.o
>> obj-$(CONFIG_TI_TSC2046) += ti-tsc2046.o
>> obj-$(CONFIG_TWL4030_MADC) += twl4030-madc.o
>> 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 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for Texas Instruments TLA2528 ADC
>> + *
>> + * Copyright (C) 2020-2021 Rodolfo Giometti <giometti@enneenne.com>
>> + * Copyright (C) 2025 Maxime Chevallier <maxime.chevallier@bootlin.com>
>> + */
>> +
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <linux/iio/iio.h>
>> +
>> +#define TLA2528_OP_WRITE_REG 0x08
>> +
>> +#define TLA2528_DATA_CFG_ADR 0x02
>> +
>> +/* Datasheet says [5:4] sets the append status, but only bit 4 is used */
>> +#define TLA2528_DATA_CFG_APPEND_STATUS BIT(4)
>> +#define TLA2528_PIN_CFG_ADR 0x05
>> +#define TLA2528_SEQUENCE_CFG_ADR 0x10
>> +#define TLA2528_CHANNEL_SEL_ADR 0x11
>> +
>> +struct tla2528 {
>> + struct i2c_client *client;
>> + int vref_uv;
>> +
>> + /* Protects manual channel selection, i.e. last_read_channel */
>> + struct mutex lock;
>> + u8 last_read_channel;
>> +};
>> +
>> +static s32 tla2528_write_reg(const struct i2c_client *client, u8 reg, u8 val)
>> +{
>> + u8 data[3] = {TLA2528_OP_WRITE_REG, reg, val};
>> + 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);
>> + if (ret < 0)
>> + return ret;
>> +
>> + return be16_to_cpu(data) >> 4;
>> +}
>
> Can regmap be used for all the usual benefits?
I will use that indeed :)
>
>> +
>> +static int tla2528_read(struct tla2528 *tla2528, u8 channel, int *val)
>> +{
>> + struct i2c_client *client = tla2528->client;
>> + int ret;
>> +
>> + if (channel != tla2528->last_read_channel) {
>> + ret = tla2528_write_reg(client, TLA2528_CHANNEL_SEL_ADR, channel);
>> + if (ret < 0)
>> + return ret;
>> +
>> + tla2528->last_read_channel = channel;
>> + }
>> +
>> + ret = tla2528_read_sample(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_SCALE:
>> + *val = tla2528->vref_uv / 1000;
>> + *val2 = 12;
>> +
>> + return IIO_VAL_FRACTIONAL_LOG2;
>> +
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +#define TLA2528_CHAN(_chan, _name) { \
>> + .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 const struct iio_chan_spec tla2528_channel[] = {
>> + TLA2528_CHAN(0, "AIN0"),
>> + TLA2528_CHAN(1, "AIN1"),
>> + TLA2528_CHAN(2, "AIN2"),
>> + TLA2528_CHAN(3, "AIN3"),
>> + TLA2528_CHAN(4, "AIN4"),
>> + TLA2528_CHAN(5, "AIN5"),
>> + TLA2528_CHAN(6, "AIN6"),
>> + TLA2528_CHAN(7, "AIN7"),
>> +};
>
> Not really insisting a change here, merely giving a nudge :)
>
> I wonder if the channels should be obtained from fwnode? I think the
> dt-review mentioned channel inputs may be used for GPIOs? One might be
> able to use the devm_iio_adc_device_alloc_chaninfo_se() to build the
> chan_spec based on the dt - depending on the non dt use-cases.
>
> All in all, I think this is clean and nice driver.
>
>> +static const struct iio_info tla2528_info = {
>> + .read_raw = tla2528_read_raw,
>> +};
>> +
>> +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);
>> + tla2528->client = client;
>> +
>> + indio_dev->name = client->name;
>> + 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;
>
> As mentioned above, perhaps get the channels from the DT, and only mux
> the given channels?
I will give this a try, I didn't know about these helpers you mention
above :)
>
>> +
>> + 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);
>> +}
>> +
>> +static const struct i2c_device_id tla2528_id[] = {
>> + { "tla2528", 0 },
>> + { }
>> +};
>> +MODULE_DEVICE_TABLE(i2c, tla2528_id);
>> +
>> +static const struct of_device_id tla2528_of_match[] = {
>> + { .compatible = "ti,tla2528", },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, tla2528_of_match);
>> +
>> +static struct i2c_driver tla2528_driver = {
>> + .driver = {
>> + .name = "tla2528",
>> + .of_match_table = tla2528_of_match,
>> + },
>> + .probe = tla2528_probe,
>> + .id_table = tla2528_id,
>> +};
>> +module_i2c_driver(tla2528_driver);
>> +
>> +MODULE_AUTHOR("Maxime Chevallier <maxime.chevallier@bootlin.com>");
>> +MODULE_AUTHOR("Rodolfo Giometti <giometti@enneenne.com>");
>> +MODULE_DESCRIPTION("Texas Instruments TLA2528 ADC driver");
>> +MODULE_LICENSE("GPL");
>
> Yours,
> -- Matti
Thank you for reviewing,
Maxime
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-23 15:55 ` [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc Maxime Chevallier
` (3 preceding siblings ...)
2025-12-29 8:20 ` Matti Vaittinen
@ 2025-12-29 9:39 ` Andy Shevchenko
2026-01-05 10:27 ` Maxime Chevallier
4 siblings, 1 reply; 22+ messages in thread
From: Andy Shevchenko @ 2025-12-29 9:39 UTC (permalink / raw)
To: Maxime Chevallier
Cc: Jonathan Cameron, David Lechner, nuno.sa, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Matti Vaittinen, Antoniu Miclaus, Angelo Dureghello,
Tobias Sperling, Eason Yang, Marilene Andrade Garcia,
Cosmin Tanislav, duje, herve.codina, Rodolfo Giometti, linux-iio,
devicetree, linux-kernel, thomas.petazzoni
On Tue, Dec 23, 2025 at 5:55 PM 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.
...
> +#include <linux/delay.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/regulator/consumer.h>
Follow IWYU. Here are missing headers, such as bits.h, mutex.h, types.h.
...
> + case IIO_CHAN_INFO_SCALE:
> + *val = tla2528->vref_uv / 1000;
(MICRO/MILLI) ?
> + *val2 = 12;
> +
> + return IIO_VAL_FRACTIONAL_LOG2;
> + tla2528->vref_uv = devm_regulator_get_enable_read_voltage(&client->dev,
> + "vref");
With
struct device *dev = &client->dev;
at the top of the function this will be one line and others also can
be shortened.
> + 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);
Why not simply use the "client"?
> + 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;
Ditto.
...
> +static const struct i2c_device_id tla2528_id[] = {
> + { "tla2528", 0 },
No ', 0' part.
> + { }
> +};
...
> +static const struct of_device_id tla2528_of_match[] = {
> + { .compatible = "ti,tla2528", },
No inner comma.
> + { },
No trailing comma.
> +};
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH 2/2] iio: adc: add driver for Texas Instruments TLA2528 adc
2025-12-29 9:39 ` Andy Shevchenko
@ 2026-01-05 10:27 ` Maxime Chevallier
0 siblings, 0 replies; 22+ messages in thread
From: Maxime Chevallier @ 2026-01-05 10:27 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jonathan Cameron, David Lechner, nuno.sa, Andy Shevchenko,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Marcelo Schmitt,
Matti Vaittinen, Antoniu Miclaus, Angelo Dureghello,
Tobias Sperling, Eason Yang, Marilene Andrade Garcia,
Cosmin Tanislav, duje, herve.codina, Rodolfo Giometti, linux-iio,
devicetree, linux-kernel, thomas.petazzoni
Hi Andy,
On 29/12/2025 10:39, Andy Shevchenko wrote:
> On Tue, Dec 23, 2025 at 5:55 PM 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.
>
> ...
>
>> +#include <linux/delay.h>
>> +#include <linux/i2c.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/regulator/consumer.h>
>
> Follow IWYU. Here are missing headers, such as bits.h, mutex.h, types.h.
>
> ...
>
>> + case IIO_CHAN_INFO_SCALE:
>> + *val = tla2528->vref_uv / 1000;
>
> (MICRO/MILLI) ?
Absolutely
>
>> + *val2 = 12;
>> +
>> + return IIO_VAL_FRACTIONAL_LOG2;
>
>
>
>> + tla2528->vref_uv = devm_regulator_get_enable_read_voltage(&client->dev,
>> + "vref");
>
> With
>
> struct device *dev = &client->dev;
>
> at the top of the function this will be one line and others also can
> be shortened.
Ah yes indeed !
>
>> + 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);
>
> Why not simply use the "client"?
Good point
>
>> + 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;
>
> Ditto.
>
> ...
>
>> +static const struct i2c_device_id tla2528_id[] = {
>> + { "tla2528", 0 },
>
> No ', 0' part.
>
>> + { }
>> +};
>
> ...
>
>> +static const struct of_device_id tla2528_of_match[] = {
>> + { .compatible = "ti,tla2528", },
>
> No inner comma.
>
>> + { },
>
> No trailing comma.
>
>> +};
>
I'll address these formatting comments as well, thank you for taking a
look :)
Maxime
^ permalink raw reply [flat|nested] 22+ messages in thread