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
--- 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;
+}
+
+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"),
+};
+
+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;
+
+ 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");
--
2.49.0On 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
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
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! ~~
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
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.
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! ~~
On Fri, 2 Jan 2026 09:13:51 +0200 Matti Vaittinen <mazziesaccount@gmail.com> wrote: > 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 :) Low chance for any given patch, but high chance overall as so many patches touch it! Resolving it isn't really any easier if it is inside a bigger patch or not. Jonathan > > > Yours, > -- Matti > > --- > Matti Vaittinen > Linux kernel developer at ROHM Semiconductors > Oulu Finland > > ~~ When things go utterly wrong vim users can always type :help! ~~
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);
> +}
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
On Mon, 5 Jan 2026 11:16:39 +0100 Maxime Chevallier <maxime.chevallier@bootlin.com> wrote: > 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 > > Hi Maxime, Small process related request. If you agree with a bit of feedback in a review, just crop that bit out. Keep the polite response for the change log of the next version. Whilst this can feel a little rude, the gain in efficiency and the reduction in people missing the bits that actually warrant more discussion makes it worthwhile. 2nd time I'm posting this advice today! Lets see how many more times it is relevant ;) Jonathan
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");
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
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
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 >
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
© 2016 - 2026 Red Hat, Inc.