[PATCH 0/3] iio: adc: Extend ti-ads1100 driver

[PATCH 0/3] iio: adc: Extend ti-ads1100 driver

[Jakub Szczudlo]

Extend ADS 1100 driver to support ADS1110, which is a pin-to-pin 
compatible device with higher resolution. This patch also updates the 
device tree bindings and Kconfig description to reflect the new 
supported device.

Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
---
jszczudlo (3):
  dt-bindings: iio: adc: Update title and enum
  iio: adc: Update Kconfig description for TI_ADS1100
  iio: adc: Add ti-ads1110 support to ti-ads1100 driver

 .../bindings/iio/adc/ti,ads1100.yaml          |   3 +-
 drivers/iio/adc/Kconfig                       |   6 +-
 drivers/iio/adc/ti-ads1100.c                  | 165 +++++++++++++-----
 3 files changed, 131 insertions(+), 43 deletions(-)

-- 
2.47.3

[PATCH 1/3] dt-bindings: iio: adc: Update title and enum

[Jakub Szczudlo]

From: jszczudlo <jakubszczudlo40@gmail.com>

Add ads1110 into title and enum compatible for ads1100 driver.

Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
---
 Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
index 970ccab15e1e..4c43996b7ec6 100644
--- a/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
@@ -4,7 +4,7 @@
 $id: http://devicetree.org/schemas/iio/adc/ti,ads1100.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: TI ADS1100/ADS1000 single channel I2C analog to digital converter
+title: TI ADS1100/ADS1000/ADS1110 single channel I2C analog to digital converter
 
 maintainers:
   - Mike Looijmans <mike.looijmans@topic.nl>
@@ -17,6 +17,7 @@ properties:
     enum:
       - ti,ads1100
       - ti,ads1000
+      - ti,ads1110
 
   reg:
     maxItems: 1
-- 
2.47.3

Re: [PATCH 1/3] dt-bindings: iio: adc: Update title and enum

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1624 bytes --]

On Sun, Jun 07, 2026 at 08:35:40PM +0200, Jakub Szczudlo wrote:
> From: jszczudlo <jakubszczudlo40@gmail.com>
> 

$subject: dt-bindings: iio: adc: Update title and enum

This should read "dt-bindings: iio: adc: ti,ads1100: add support for $newdevice".

> Add ads1110 into title and enum compatible for ads1100 driver.

And so should this.
Additionally, please explain why this device is not compatible with
existing ones.
pw-bot: changes-requested

> 
> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>

Probably your signoff should be "Jakub Szczudlo <jakubszczudlo40@gmail.com>".


Thanks,
Conor.

> ---
>  Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
> index 970ccab15e1e..4c43996b7ec6 100644
> --- a/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
> @@ -4,7 +4,7 @@
>  $id: http://devicetree.org/schemas/iio/adc/ti,ads1100.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: TI ADS1100/ADS1000 single channel I2C analog to digital converter
> +title: TI ADS1100/ADS1000/ADS1110 single channel I2C analog to digital converter
>  
>  maintainers:
>    - Mike Looijmans <mike.looijmans@topic.nl>
> @@ -17,6 +17,7 @@ properties:
>      enum:
>        - ti,ads1100
>        - ti,ads1000
> +      - ti,ads1110
>  
>    reg:
>      maxItems: 1
> -- 
> 2.47.3
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/3] dt-bindings: iio: adc: Update title and enum

[Jonathan Cameron]

On Sun,  7 Jun 2026 20:35:40 +0200
Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:

> From: jszczudlo <jakubszczudlo40@gmail.com>
> 
> Add ads1110 into title and enum compatible for ads1100 driver.
> 
> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
Hi Jakub,

Obviously not related to your patch, but can we switch existing
pair of entries to be in alphabetical order whilst we do this?
Normally I'd ask for a separate patch for that, but given it's
so trivial and that would create churn vs your patch, do it
in one go (but make sure to mention it in the patch description!)



> ---
>  Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
> index 970ccab15e1e..4c43996b7ec6 100644
> --- a/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
> +++ b/Documentation/devicetree/bindings/iio/adc/ti,ads1100.yaml
> @@ -4,7 +4,7 @@
>  $id: http://devicetree.org/schemas/iio/adc/ti,ads1100.yaml#
>  $schema: http://devicetree.org/meta-schemas/core.yaml#
>  
> -title: TI ADS1100/ADS1000 single channel I2C analog to digital converter
> +title: TI ADS1100/ADS1000/ADS1110 single channel I2C analog to digital converter
This is on the edge of the normal problem of never ending title growth
that we cheat to solve by saying something like

title: TI ADS1100 and similar single channel I2C Analog to Digital Converters

When we do that though we tend to include a bulleted list of parts in the
description: section.  Whilst I was looking at that I noticed we only reference
one datasheet which I'd assume doesn't cover this new part?  Perhaps that needs
updating as well?

Thanks,

Jonathan
>  
>  maintainers:
>    - Mike Looijmans <mike.looijmans@topic.nl>
> @@ -17,6 +17,7 @@ properties:
>      enum:
>        - ti,ads1100
>        - ti,ads1000
> +      - ti,ads1110
>  
>    reg:
>      maxItems: 1

[PATCH 2/3] iio: adc: Update Kconfig description for TI_ADS1100

[Jakub Szczudlo]

From: jszczudlo <jakubszczudlo40@gmail.com>

Add mention of ads1110 in Kconfig for TI_ADS1100

Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
---
 drivers/iio/adc/Kconfig | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index a9dedbb8eb46..ad2a276ce0f8 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -1747,11 +1747,11 @@ config TI_ADS1018
          called ti-ads1018.
 
 config TI_ADS1100
-	tristate "Texas Instruments ADS1100 and ADS1000 ADC"
+	tristate "Texas Instruments ADS1100, ADS1000 and ADS1110 ADC"
 	depends on I2C
 	help
-	  If you say yes here you get support for Texas Instruments ADS1100 and
-	  ADS1000 ADC chips.
+	  If you say yes here you get support for Texas Instruments ADS1100,
+	  ADS1000 and ADS1110 ADC chips.
 
 	  This driver can also be built as a module. If so, the module will be
 	  called ti-ads1100.
-- 
2.47.3

Re: [PATCH 2/3] iio: adc: Update Kconfig description for TI_ADS1100

[Joshua Crofts]

On Sun, 7 Jun 2026 at 20:38, Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:
>
> From: jszczudlo <jakubszczudlo40@gmail.com>
>
> Add mention of ads1110 in Kconfig for TI_ADS1100
>
> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>

Better to fold this change into the patch that adds the actual driver code,
otherwise this is unnecessary churn.

-- 
Kind regards

CJD

Re: [PATCH 2/3] iio: adc: Update Kconfig description for TI_ADS1100

[Andy Shevchenko]

On Mon, Jun 08, 2026 at 08:45:46AM +0200, Joshua Crofts wrote:
> On Sun, 7 Jun 2026 at 20:38, Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:
> >
> > Add mention of ads1110 in Kconfig for TI_ADS1100
> >
> > Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
> 
> Better to fold this change into the patch that adds the actual driver code,
> otherwise this is unnecessary churn.

It's fine to do it separately, but after the code update. The combined version
will work for me as well.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/3] iio: adc: Update Kconfig description for TI_ADS1100

[Jonathan Cameron]

On Mon, 8 Jun 2026 14:43:16 +0300
Andy Shevchenko <andriy.shevchenko@intel.com> wrote:

> On Mon, Jun 08, 2026 at 08:45:46AM +0200, Joshua Crofts wrote:
> > On Sun, 7 Jun 2026 at 20:38, Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:  
> > >
> > > Add mention of ads1110 in Kconfig for TI_ADS1100
> > >
> > > Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>  
> > 
> > Better to fold this change into the patch that adds the actual driver code,
> > otherwise this is unnecessary churn.  
> 
> It's fine to do it separately, but after the code update. The combined version
> will work for me as well.
> 

FWIW I'd prefer it combined with the next patch.
I'm not keen on separate updates on build files in general as they
are easier to review in context. 

[PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver

[Jakub Szczudlo]

From: jszczudlo <jakubszczudlo40@gmail.com>

add ADS1100 support
make changing gain and datarate wait for new reading
fix unbalanced regulator disable when removing in singleshot mode

Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
---
 drivers/iio/adc/ti-ads1100.c | 165 ++++++++++++++++++++++++++---------
 1 file changed, 126 insertions(+), 39 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
index aa8946063c7d..11d6fe1e8abc 100644
--- a/drivers/iio/adc/ti-ads1100.c
+++ b/drivers/iio/adc/ti-ads1100.c
@@ -5,18 +5,15 @@
  * Copyright (c) 2023, Topic Embedded Products
  *
  * Datasheet: https://www.ti.com/lit/gpn/ads1100
- * IIO driver for ADS1100 and ADS1000 ADC 16-bit I2C
+ * IIO driver for ADS1100, ADS1000 and ADS1110 ADC 16-bit I2C
  */
 
 #include <linux/bitfield.h>
 #include <linux/bits.h>
-#include <linux/cleanup.h>
-#include <linux/delay.h>
-#include <linux/module.h>
-#include <linux/init.h>
 #include <linux/i2c.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
 #include <linux/mutex.h>
-#include <linux/property.h>
 #include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/units.h>
@@ -39,17 +36,39 @@
 #define	ADS1100_SINGLESHOT	ADS1100_CFG_SC
 
 #define ADS1100_SLEEP_DELAY_MS	2000
+#define ADS1110_REFERENCE_VOLTAGE_MICROVOLT 2048000
+
+/* Timeout based on the minimum sample rate of 8 SPS (7500000us) */
+#define ADS11x0_MAX_DRDY_TIMEOUT	7500000
 
 static const int ads1100_data_rate[] = { 128, 32, 16, 8 };
+static const int ads1110_data_rate[] = { 240, 60, 30, 15 };
 static const int ads1100_data_rate_bits[] = { 12, 14, 15, 16 };
 
+struct ads11x0_config {
+	const int *data_rate;
+	bool has_reference_voltage;
+};
+
+static const struct ads11x0_config ads1100_config = {
+	.data_rate = ads1100_data_rate,
+	.has_reference_voltage = false,
+};
+
+static const struct ads11x0_config ads1110_config = {
+	.data_rate = ads1110_data_rate,
+	.has_reference_voltage = true,
+};
+
 struct ads1100_data {
 	struct i2c_client *client;
 	struct regulator *reg_vdd;
 	struct mutex lock;
 	int scale_avail[2 * 4]; /* 4 gain settings */
 	u8 config;
-	bool supports_data_rate; /* Only the ADS1100 can select the rate */
+	bool supports_data_rate; /* Only the ADS1100/ADS1110 can select the rate */
+	bool has_reference_voltage; /* The ADS1110 has an internal reference, so fixed scale */
+	const int *data_rate;
 };
 
 static const struct iio_chan_spec ads1100_channel = {
@@ -59,12 +78,6 @@ static const struct iio_chan_spec ads1100_channel = {
 	    BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
 	.info_mask_shared_by_all_available =
 	    BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
-	.scan_type = {
-		      .sign = 's',
-		      .realbits = 16,
-		      .storagebits = 16,
-		      .endianness = IIO_CPU,
-		       },
 	.datasheet_name = "AIN",
 };
 
@@ -85,6 +98,50 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
 	return 0;
 };
 
+static int ads11x0_get_voltage_microvolts(struct ads1100_data *data)
+{
+	if (data->has_reference_voltage)
+		return ADS1110_REFERENCE_VOLTAGE_MICROVOLT;
+	else
+		return regulator_get_voltage(data->reg_vdd);
+}
+
+static int ads11x0_get_voltage_milivolts(struct ads1100_data *data)
+{
+	return ads11x0_get_voltage_microvolts(data) / (MICRO / MILLI);
+}
+
+static bool ads11x0_new_data_ready(struct ads1100_data *data)
+{
+	int ret;
+	u8 buffer[3];
+
+	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
+	if (ret < sizeof(buffer)) {
+		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
+		return 0;
+	}
+
+	int return_val = FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
+
+	return return_val;
+}
+
+static int ads11x0_poll_data_ready(struct ads1100_data *data)
+{
+	bool data_ready;
+	u8 buffer[3];
+	int datarate = data->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
+	unsigned long wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);
+
+	/* To be sure that polled value will have value after config change */
+	i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
+
+	return read_poll_timeout(ads11x0_new_data_ready, data_ready,
+				 !data_ready, wait_time,
+				 ADS11x0_MAX_DRDY_TIMEOUT, false, data);
+}
+
 static int ads1100_data_bits(struct ads1100_data *data)
 {
 	return ads1100_data_rate_bits[FIELD_GET(ADS1100_DR_MASK, data->config)];
@@ -105,9 +162,10 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
 
 	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
 
+	pm_runtime_mark_last_busy(&data->client->dev);
 	pm_runtime_put_autosuspend(&data->client->dev);
 
-	if (ret < 0) {
+	if (ret < sizeof(buffer)) {
 		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
 		return ret;
 	}
@@ -127,7 +185,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
 {
 	int microvolts;
 	int gain;
-
+	int ret;
 	/* With Vdd between 2.7 and 5V, the scale is always below 1 */
 	if (val)
 		return -EINVAL;
@@ -135,7 +193,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
 	if (!val2)
 		return -EINVAL;
 
-	microvolts = regulator_get_voltage(data->reg_vdd);
+	microvolts = ads11x0_get_voltage_microvolts(data);
 	/*
 	 * val2 is in 'micro' units, n = val2 / 1000000
 	 * result must be millivolts, d = microvolts / 1000
@@ -147,34 +205,49 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
 	if (gain < BIT(0) || gain > BIT(3))
 		return -EINVAL;
 
+	ret = pm_runtime_resume_and_get(&data->client->dev);
+	if (ret < 0)
+		return ret;
+
 	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
 
-	return 0;
+	ret = ads11x0_poll_data_ready(data);
+
+	pm_runtime_mark_last_busy(&data->client->dev);
+	pm_runtime_put_autosuspend(&data->client->dev);
+
+	return ret;
 }
 
 static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
 {
 	unsigned int i;
 	unsigned int size;
+	int ret;
 
 	size = data->supports_data_rate ? ARRAY_SIZE(ads1100_data_rate) : 1;
 	for (i = 0; i < size; i++) {
-		if (ads1100_data_rate[i] == rate)
-			return ads1100_set_config_bits(data, ADS1100_DR_MASK,
-						       FIELD_PREP(ADS1100_DR_MASK, i));
+		if (data->data_rate[i] == rate) {
+			ret = pm_runtime_resume_and_get(&data->client->dev);
+			if (ret < 0)
+				return ret;
+
+			ads1100_set_config_bits(data, ADS1100_DR_MASK,
+						FIELD_PREP(ADS1100_DR_MASK, i));
+			ret = ads11x0_poll_data_ready(data);
+
+			pm_runtime_mark_last_busy(&data->client->dev);
+			pm_runtime_put_autosuspend(&data->client->dev);
+			return ret;
+		}
 	}
 
 	return -EINVAL;
 }
 
-static int ads1100_get_vdd_millivolts(struct ads1100_data *data)
-{
-	return regulator_get_voltage(data->reg_vdd) / (MICRO / MILLI);
-}
-
 static void ads1100_calc_scale_avail(struct ads1100_data *data)
 {
-	int millivolts = ads1100_get_vdd_millivolts(data);
+	int millivolts = ads11x0_get_voltage_milivolts(data);
 	unsigned int i;
 
 	for (i = 0; i < ARRAY_SIZE(data->scale_avail) / 2; i++) {
@@ -196,7 +269,7 @@ static int ads1100_read_avail(struct iio_dev *indio_dev,
 	switch (mask) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		*type = IIO_VAL_INT;
-		*vals = ads1100_data_rate;
+		*vals = data->data_rate;
 		if (data->supports_data_rate)
 			*length = ARRAY_SIZE(ads1100_data_rate);
 		else
@@ -233,12 +306,11 @@ static int ads1100_read_raw(struct iio_dev *indio_dev,
 		return IIO_VAL_INT;
 	case IIO_CHAN_INFO_SCALE:
 		/* full-scale is the supply voltage in millivolts */
-		*val = ads1100_get_vdd_millivolts(data);
+		*val = ads11x0_get_voltage_milivolts(data);
 		*val2 = 15 + FIELD_GET(ADS1100_PGA_MASK, data->config);
 		return IIO_VAL_FRACTIONAL_LOG2;
 	case IIO_CHAN_INFO_SAMP_FREQ:
-		*val = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK,
-						   data->config)];
+		*val = data->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
 		return IIO_VAL_INT;
 	default:
 		return -EINVAL;
@@ -280,8 +352,8 @@ static int ads1100_setup(struct ads1100_data *data)
 		return ret;
 
 	ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
-	if (ret < 0)
-		return ret;
+	if (ret < sizeof(buffer))
+		return -1;
 
 	/* Config register returned in third byte, strip away the busy status */
 	data->config = buffer[2] & ~ADS1100_CFG_ST_BSY;
@@ -292,9 +364,13 @@ static int ads1100_setup(struct ads1100_data *data)
 	return 0;
 }
 
-static void ads1100_reg_disable(void *reg)
+static void ads1100_reg_disable(void *data)
 {
-	regulator_disable(reg);
+	struct ads1100_data *ads1100_data = data;
+
+	/* Disable when not already disabled by the driver */
+	if (!(ads1100_data->config & ADS1100_CFG_SC))
+		regulator_disable(ads1100_data->reg_vdd);
 }
 
 static void ads1100_disable_continuous(void *data)
@@ -307,6 +383,7 @@ static int ads1100_probe(struct i2c_client *client)
 	struct iio_dev *indio_dev;
 	struct ads1100_data *data;
 	struct device *dev = &client->dev;
+	const struct ads11x0_config *model;
 	int ret;
 
 	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
@@ -334,10 +411,18 @@ static int ads1100_probe(struct i2c_client *client)
 		return dev_err_probe(dev, ret,
 				     "Failed to enable vdd regulator\n");
 
-	ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
+	ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data);
 	if (ret)
 		return ret;
 
+	model = device_get_match_data(dev);
+	if (!model)
+		return dev_err_probe(dev, ret,
+				"Can't set device data\n");
+
+	data->data_rate = model->data_rate;
+	data->has_reference_voltage = model->has_reference_voltage;
+
 	ret = ads1100_setup(data);
 	if (ret)
 		return dev_err_probe(dev, ret,
@@ -400,16 +485,18 @@ static DEFINE_RUNTIME_DEV_PM_OPS(ads1100_pm_ops,
 				 NULL);
 
 static const struct i2c_device_id ads1100_id[] = {
-	{ "ads1100" },
-	{ "ads1000" },
+	{ .name = "ads1100", .driver_data = (kernel_ulong_t)&ads1100_config },
+	{ .name = "ads1000", .driver_data = (kernel_ulong_t)&ads1100_config },
+	{ .name = "ads1110", .driver_data = (kernel_ulong_t)&ads1110_config },
 	{ }
 };
 
 MODULE_DEVICE_TABLE(i2c, ads1100_id);
 
 static const struct of_device_id ads1100_of_match[] = {
-	{.compatible = "ti,ads1100" },
-	{.compatible = "ti,ads1000" },
+	{ .compatible = "ti,ads1100", .data = &ads1100_config },
+	{ .compatible = "ti,ads1000", .data = &ads1100_config },
+	{ .compatible = "ti,ads1110", .data = &ads1110_config },
 	{ }
 };
 
-- 
2.47.3

Re: [PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver

[Joshua Crofts]

Hi Jakub,

various comments inline, some nitpicks and some more serious.

Josh

On Sun, 7 Jun 2026 at 20:38, Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:
>
> From: jszczudlo <jakubszczudlo40@gmail.com>
>
> add ADS1100 support

Wrap the commit message to 72 characters per line, this is too
short.

> make changing gain and datarate wait for new reading
> fix unbalanced regulator disable when removing in singleshot mode

Additionally, write the commit messages as regular sentences, not
a list of changes.

>
> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>

Ensure that your full name is in the Signed-off-by tag (this goes for all
patches in this series).

> ---
>  drivers/iio/adc/ti-ads1100.c | 165 ++++++++++++++++++++++++++---------
>  1 file changed, 126 insertions(+), 39 deletions(-)
>
> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index aa8946063c7d..11d6fe1e8abc 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -5,18 +5,15 @@
>   * Copyright (c) 2023, Topic Embedded Products
>   *
>   * Datasheet: https://www.ti.com/lit/gpn/ads1100
> - * IIO driver for ADS1100 and ADS1000 ADC 16-bit I2C
> + * IIO driver for ADS1100, ADS1000 and ADS1110 ADC 16-bit I2C
>   */
>
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
> -#include <linux/cleanup.h>
> -#include <linux/delay.h>
> -#include <linux/module.h>
> -#include <linux/init.h>
>  #include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
>  #include <linux/mutex.h>
> -#include <linux/property.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/units.h>

This should go as a separate patch (or part of a cleanup series). Not only
it's undocumented in the commit message, it's also unrelated to adding
support for a new device.

> @@ -39,17 +36,39 @@
>  #define        ADS1100_SINGLESHOT      ADS1100_CFG_SC
>
>  #define ADS1100_SLEEP_DELAY_MS 2000
> +#define ADS1110_REFERENCE_VOLTAGE_MICROVOLT 2048000
> +
> +/* Timeout based on the minimum sample rate of 8 SPS (7500000us) */
> +#define ADS11x0_MAX_DRDY_TIMEOUT       7500000
>
>  static const int ads1100_data_rate[] = { 128, 32, 16, 8 };
> +static const int ads1110_data_rate[] = { 240, 60, 30, 15 };
>  static const int ads1100_data_rate_bits[] = { 12, 14, 15, 16 };
>
> +struct ads11x0_config {
> +       const int *data_rate;
> +       bool has_reference_voltage;
> +};
> +
> +static const struct ads11x0_config ads1100_config = {
> +       .data_rate = ads1100_data_rate,
> +       .has_reference_voltage = false,
> +};
> +
> +static const struct ads11x0_config ads1110_config = {
> +       .data_rate = ads1110_data_rate,
> +       .has_reference_voltage = true,
> +};
> +
>  struct ads1100_data {
>         struct i2c_client *client;
>         struct regulator *reg_vdd;
>         struct mutex lock;
>         int scale_avail[2 * 4]; /* 4 gain settings */
>         u8 config;
> -       bool supports_data_rate; /* Only the ADS1100 can select the rate */
> +       bool supports_data_rate; /* Only the ADS1100/ADS1110 can select the rate */
> +       bool has_reference_voltage; /* The ADS1110 has an internal reference, so fixed scale */
> +       const int *data_rate;
>  };

Have you used pahole to ensure this struct layout is ideal? I'd
move the data_rate pointer up to the larger types.

>  static const struct iio_chan_spec ads1100_channel = {
> @@ -59,12 +78,6 @@ static const struct iio_chan_spec ads1100_channel = {
>             BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
>         .info_mask_shared_by_all_available =
>             BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
> -       .scan_type = {
> -                     .sign = 's',
> -                     .realbits = 16,
> -                     .storagebits = 16,
> -                     .endianness = IIO_CPU,
> -                      },
>         .datasheet_name = "AIN",
>  };
>
> @@ -85,6 +98,50 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
>         return 0;
>  };
>
> +static int ads11x0_get_voltage_microvolts(struct ads1100_data *data)
> +{
> +       if (data->has_reference_voltage)
> +               return ADS1110_REFERENCE_VOLTAGE_MICROVOLT;
> +       else
> +               return regulator_get_voltage(data->reg_vdd);
> +}
> +
> +static int ads11x0_get_voltage_milivolts(struct ads1100_data *data)
> +{
> +       return ads11x0_get_voltage_microvolts(data) / (MICRO / MILLI);
> +}
> +
> +static bool ads11x0_new_data_ready(struct ads1100_data *data)
> +{
> +       int ret;
> +       u8 buffer[3];
> +
> +       ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> +       if (ret < sizeof(buffer)) {

Sashiko raises an issue here. sizeof returns a size_t, therefore the compiler
will promote ret to a size_t, wrapping any potential negative error value to
a large positive value, throwing away the error.

> +               dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> +               return 0;
> +       }
> +
> +       int return_val = FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> +
> +       return return_val;
> +}
> +
> +static int ads11x0_poll_data_ready(struct ads1100_data *data)
> +{
> +       bool data_ready;
> +       u8 buffer[3];
> +       int datarate = data->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> +       unsigned long wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);

Reverse Christmas tree order please. This goes for all new functions
and variable
declarations.

> +
> +       /* To be sure that polled value will have value after config change */
> +       i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));

Add a check here as well maybe.

> +       return read_poll_timeout(ads11x0_new_data_ready, data_ready,
> +                                !data_ready, wait_time,
> +                                ADS11x0_MAX_DRDY_TIMEOUT, false, data);
> +}
> +
>  static int ads1100_data_bits(struct ads1100_data *data)
>  {
>         return ads1100_data_rate_bits[FIELD_GET(ADS1100_DR_MASK, data->config)];
> @@ -105,9 +162,10 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
>
>         ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
>
> +       pm_runtime_mark_last_busy(&data->client->dev);

AFAIK pm_runtime_put_autosuspend automatically calls mark_last_busy
when executed, so this is probably unnecessary. Additionally, this has nothing
to do with adding new device support.

>         pm_runtime_put_autosuspend(&data->client->dev);
>
> -       if (ret < 0) {
> +       if (ret < sizeof(buffer)) {

Same issue with type promotion here.

>                 dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
>                 return ret;
>         }
> @@ -127,7 +185,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>  {
>         int microvolts;
>         int gain;
> -
> +       int ret;

Newline here.

>         /* With Vdd between 2.7 and 5V, the scale is always below 1 */
>         if (val)
>                 return -EINVAL;
> @@ -135,7 +193,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>         if (!val2)
>                 return -EINVAL;
>
> -       microvolts = regulator_get_voltage(data->reg_vdd);
> +       microvolts = ads11x0_get_voltage_microvolts(data);
>         /*
>          * val2 is in 'micro' units, n = val2 / 1000000
>          * result must be millivolts, d = microvolts / 1000
> @@ -147,34 +205,49 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>         if (gain < BIT(0) || gain > BIT(3))
>                 return -EINVAL;
>
> +       ret = pm_runtime_resume_and_get(&data->client->dev);
> +       if (ret < 0)
> +               return ret;
> +
>         ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
>
> -       return 0;
> +       ret = ads11x0_poll_data_ready(data);
> +
> +       pm_runtime_mark_last_busy(&data->client->dev);

Same issue with mark_last_busy being called automatically.

> +       pm_runtime_put_autosuspend(&data->client->dev);
> +
> +       return ret;
>  }
>
>  static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
>  {
>         unsigned int i;
>         unsigned int size;
> +       int ret;
>
>         size = data->supports_data_rate ? ARRAY_SIZE(ads1100_data_rate) : 1;
>         for (i = 0; i < size; i++) {
> -               if (ads1100_data_rate[i] == rate)
> -                       return ads1100_set_config_bits(data, ADS1100_DR_MASK,
> -                                                      FIELD_PREP(ADS1100_DR_MASK, i));
> +               if (data->data_rate[i] == rate) {
> +                       ret = pm_runtime_resume_and_get(&data->client->dev);
> +                       if (ret < 0)
> +                               return ret;
> +
> +                       ads1100_set_config_bits(data, ADS1100_DR_MASK,
> +                                               FIELD_PREP(ADS1100_DR_MASK, i));
> +                       ret = ads11x0_poll_data_ready(data);
> +
> +                       pm_runtime_mark_last_busy(&data->client->dev);
> +                       pm_runtime_put_autosuspend(&data->client->dev);
> +                       return ret;
> +               }
>         }
>
>         return -EINVAL;
>  }
>
> -static int ads1100_get_vdd_millivolts(struct ads1100_data *data)
> -{
> -       return regulator_get_voltage(data->reg_vdd) / (MICRO / MILLI);
> -}
> -
>  static void ads1100_calc_scale_avail(struct ads1100_data *data)
>  {
> -       int millivolts = ads1100_get_vdd_millivolts(data);
> +       int millivolts = ads11x0_get_voltage_milivolts(data);
>         unsigned int i;
>
>         for (i = 0; i < ARRAY_SIZE(data->scale_avail) / 2; i++) {
> @@ -196,7 +269,7 @@ static int ads1100_read_avail(struct iio_dev *indio_dev,
>         switch (mask) {
>         case IIO_CHAN_INFO_SAMP_FREQ:
>                 *type = IIO_VAL_INT;
> -               *vals = ads1100_data_rate;
> +               *vals = data->data_rate;
>                 if (data->supports_data_rate)
>                         *length = ARRAY_SIZE(ads1100_data_rate);
>                 else
> @@ -233,12 +306,11 @@ static int ads1100_read_raw(struct iio_dev *indio_dev,
>                 return IIO_VAL_INT;
>         case IIO_CHAN_INFO_SCALE:
>                 /* full-scale is the supply voltage in millivolts */
> -               *val = ads1100_get_vdd_millivolts(data);
> +               *val = ads11x0_get_voltage_milivolts(data);
>                 *val2 = 15 + FIELD_GET(ADS1100_PGA_MASK, data->config);
>                 return IIO_VAL_FRACTIONAL_LOG2;
>         case IIO_CHAN_INFO_SAMP_FREQ:
> -               *val = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK,
> -                                                  data->config)];
> +               *val = data->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
>                 return IIO_VAL_INT;
>         default:
>                 return -EINVAL;
> @@ -280,8 +352,8 @@ static int ads1100_setup(struct ads1100_data *data)
>                 return ret;
>
>         ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
> -       if (ret < 0)
> -               return ret;
> +       if (ret < sizeof(buffer))
> +               return -1;

Same type promotion issue here. Additionally, I'd rather return -EIO than -1.

>
>         /* Config register returned in third byte, strip away the busy status */
>         data->config = buffer[2] & ~ADS1100_CFG_ST_BSY;
> @@ -292,9 +364,13 @@ static int ads1100_setup(struct ads1100_data *data)
>         return 0;
>  }
>
> -static void ads1100_reg_disable(void *reg)
> +static void ads1100_reg_disable(void *data)
>  {
> -       regulator_disable(reg);
> +       struct ads1100_data *ads1100_data = data;
> +
> +       /* Disable when not already disabled by the driver */
> +       if (!(ads1100_data->config & ADS1100_CFG_SC))
> +               regulator_disable(ads1100_data->reg_vdd);

Sashiko has something to say:

Could this leak the `vdd` regulator during device unbind due to the devres
execution order?
Devres executes registered actions in reverse order (LIFO). During unbind,
`ads1100_disable_continuous()` executes first and unconditionally sets
`ADS1100_CFG_SC`.
When `ads1100_reg_disable()` runs immediately after, the `ADS1100_CFG_SC` bit
is already set, causing this condition to fail and permanently skipping the
`regulator_disable()` call.

>  }
>
>  static void ads1100_disable_continuous(void *data)
> @@ -307,6 +383,7 @@ static int ads1100_probe(struct i2c_client *client)
>         struct iio_dev *indio_dev;
>         struct ads1100_data *data;
>         struct device *dev = &client->dev;
> +       const struct ads11x0_config *model;
>         int ret;
>
>         indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> @@ -334,10 +411,18 @@ static int ads1100_probe(struct i2c_client *client)
>                 return dev_err_probe(dev, ret,
>                                      "Failed to enable vdd regulator\n");
>
> -       ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
> +       ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data);
>         if (ret)
>                 return ret;
>
> +       model = device_get_match_data(dev);
> +       if (!model)
> +               return dev_err_probe(dev, ret,
> +                               "Can't set device data\n");

Hmm, if device_get_match_data fails, ret will still be 0 per previous
devm_add_action_or_reset() call, therefore you're returning a "successful
error". Additionally, the error message isn't aligned with the parenthesis.

Re: [PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver

[Jonathan Cameron]

On Mon, 8 Jun 2026 09:39:07 +0200
Joshua Crofts <joshua.crofts1@gmail.com> wrote:

> Hi Jakub,
> 
> various comments inline, some nitpicks and some more serious.
> 
> Josh
> 
> On Sun, 7 Jun 2026 at 20:38, Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:
> >
> > From: jszczudlo <jakubszczudlo40@gmail.com>
> >
> > add ADS1100 support  
> 
> Wrap the commit message to 72 characters per line, this is too
> short.
> 
> > make changing gain and datarate wait for new reading
> > fix unbalanced regulator disable when removing in singleshot mode  
> 
> Additionally, write the commit messages as regular sentences, not
> a list of changes.
> 
> >
> > Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>  
> 
> Ensure that your full name is in the Signed-off-by tag (this goes for all
> patches in this series).

A few follow ups to the good review you already have from Joshua.

> > @@ -85,6 +98,50 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
> >         return 0;
> >  };
> >
> > +static int ads11x0_get_voltage_microvolts(struct ads1100_data *data)
> > +{
> > +       if (data->has_reference_voltage)
> > +               return ADS1110_REFERENCE_VOLTAGE_MICROVOLT;
> > +       else
> > +               return regulator_get_voltage(data->reg_vdd);
> > +}
> > +
> > +static int ads11x0_get_voltage_milivolts(struct ads1100_data *data)
> > +{
> > +       return ads11x0_get_voltage_microvolts(data) / (MICRO / MILLI);
> > +}
> > +
> > +static bool ads11x0_new_data_ready(struct ads1100_data *data)
> > +{
> > +       int ret;
> > +       u8 buffer[3];
> > +
> > +       ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> > +       if (ret < sizeof(buffer)) {  
> 
> Sashiko raises an issue here. sizeof returns a size_t, therefore the compiler
> will promote ret to a size_t, wrapping any potential negative error value to
> a large positive value, throwing away the error.
> 
> > +               dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> > +               return 0;
It gets thrown away here anyway which is also very wrong!  If an error occurs
it should be propagated. If it makes sense to ignore it, do that at the
caller and add a comment on why.
> > +       }
> > +
> > +       int return_val = FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);

When there isn't a good reason to do otherwise we still flow oldstyle c
where local variable declarations come at the top of scope.
However

	return FIELD_GET();
should be fine.

> > +
> > +       return return_val;
> > +}


> >  static void ads1100_disable_continuous(void *data)
> > @@ -307,6 +383,7 @@ static int ads1100_probe(struct i2c_client *client)
> >         struct iio_dev *indio_dev;
> >         struct ads1100_data *data;
> >         struct device *dev = &client->dev;
> > +       const struct ads11x0_config *model;
> >         int ret;
> >
> >         indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > @@ -334,10 +411,18 @@ static int ads1100_probe(struct i2c_client *client)
> >                 return dev_err_probe(dev, ret,
> >                                      "Failed to enable vdd regulator\n");
> >
> > -       ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
> > +       ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data);
> >         if (ret)
> >                 return ret;
> >
> > +       model = device_get_match_data(dev);
> > +       if (!model)
> > +               return dev_err_probe(dev, ret,
> > +                               "Can't set device data\n");  
> 
> Hmm, if device_get_match_data fails, ret will still be 0 per previous
> devm_add_action_or_reset() call, therefore you're returning a "successful
> error". Additionally, the error message isn't aligned with the parenthesis.

Message also talks about 'setting' when it is 'getting' data from firmware.
So needs a rewrite.

Jonathan

Re: [PATCH 3/3] iio: adc: Add ti-ads1110 support to ti-ads1100 driver

[Jonathan Cameron]

On Sun,  7 Jun 2026 20:35:42 +0200
Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:

> From: jszczudlo <jakubszczudlo40@gmail.com>
> 
> add ADS1100 support
> make changing gain and datarate wait for new reading
> fix unbalanced regulator disable when removing in singleshot mode
> 
> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
Hi Jakub,

A few additional things for me.
Note that the IIO tree is closed for this cycle and even if it wasn't
don't rush on sending a new version (at least a few days for a patch like
this between versions). That will give more time for other reviewers
to spot things Joshua and I missed!

Thanks,

Jonathan

> ---
>  drivers/iio/adc/ti-ads1100.c | 165 ++++++++++++++++++++++++++---------
>  1 file changed, 126 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1100.c b/drivers/iio/adc/ti-ads1100.c
> index aa8946063c7d..11d6fe1e8abc 100644
> --- a/drivers/iio/adc/ti-ads1100.c
> +++ b/drivers/iio/adc/ti-ads1100.c
> @@ -5,18 +5,15 @@
>   * Copyright (c) 2023, Topic Embedded Products
>   *
>   * Datasheet: https://www.ti.com/lit/gpn/ads1100
> - * IIO driver for ADS1100 and ADS1000 ADC 16-bit I2C
> + * IIO driver for ADS1100, ADS1000 and ADS1110 ADC 16-bit I2C
>   */
>  
>  #include <linux/bitfield.h>
>  #include <linux/bits.h>
> -#include <linux/cleanup.h>
> -#include <linux/delay.h>
> -#include <linux/module.h>
> -#include <linux/init.h>
>  #include <linux/i2c.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
>  #include <linux/mutex.h>
> -#include <linux/property.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/units.h>
> @@ -39,17 +36,39 @@
>  #define	ADS1100_SINGLESHOT	ADS1100_CFG_SC
>  
>  #define ADS1100_SLEEP_DELAY_MS	2000
> +#define ADS1110_REFERENCE_VOLTAGE_MICROVOLT 2048000
Why not express this in milivolts and move the divisor for the
regulator into the place it is read?

> +
> +/* Timeout based on the minimum sample rate of 8 SPS (7500000us) */
> +#define ADS11x0_MAX_DRDY_TIMEOUT	7500000
>  
>  static const int ads1100_data_rate[] = { 128, 32, 16, 8 };
> +static const int ads1110_data_rate[] = { 240, 60, 30, 15 };
>  static const int ads1100_data_rate_bits[] = { 12, 14, 15, 16 };
>  
> +struct ads11x0_config {
> +	const int *data_rate;
> +	bool has_reference_voltage;

const char *name;
(see below)

> +};
> +
> +static const struct ads11x0_config ads1100_config = {
> +	.data_rate = ads1100_data_rate,
> +	.has_reference_voltage = false,
> +};
> +
> +static const struct ads11x0_config ads1110_config = {
> +	.data_rate = ads1110_data_rate,
> +	.has_reference_voltage = true,
> +};
> +
>  struct ads1100_data {
>  	struct i2c_client *client;
>  	struct regulator *reg_vdd;
>  	struct mutex lock;
>  	int scale_avail[2 * 4]; /* 4 gain settings */
>  	u8 config;
> -	bool supports_data_rate; /* Only the ADS1100 can select the rate */
> +	bool supports_data_rate; /* Only the ADS1100/ADS1110 can select the rate */
> +	bool has_reference_voltage; /* The ADS1110 has an internal reference, so fixed scale */

As suggested below
	struct ads1100_config *config;
rather than fields to copy.

> +	const int *data_rate;
>  };
>  
>  static const struct iio_chan_spec ads1100_channel = {
> @@ -59,12 +78,6 @@ static const struct iio_chan_spec ads1100_channel = {
>  	    BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
>  	.info_mask_shared_by_all_available =
>  	    BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_SAMP_FREQ),
> -	.scan_type = {
> -		      .sign = 's',
> -		      .realbits = 16,
> -		      .storagebits = 16,
> -		      .endianness = IIO_CPU,
> -		       },

Unrelated change so separate patch.

>  	.datasheet_name = "AIN",
>  };
>  
> @@ -85,6 +98,50 @@ static int ads1100_set_config_bits(struct ads1100_data *data, u8 mask, u8 value)
>  	return 0;
>  };
>  
> +static int ads11x0_get_voltage_microvolts(struct ads1100_data *data)
> +{
> +	if (data->has_reference_voltage)
> +		return ADS1110_REFERENCE_VOLTAGE_MICROVOLT;
> +	else
> +		return regulator_get_voltage(data->reg_vdd);
> +}
> +
> +static int ads11x0_get_voltage_milivolts(struct ads1100_data *data)
> +{
> +	return ads11x0_get_voltage_microvolts(data) / (MICRO / MILLI);
> +}
> +
> +static bool ads11x0_new_data_ready(struct ads1100_data *data)
> +{
> +	int ret;
> +	u8 buffer[3];
> +
> +	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
> +	if (ret < sizeof(buffer)) {
> +		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
> +		return 0;
> +	}
> +
> +	int return_val = FIELD_GET(ADS1100_CFG_ST_BSY, buffer[2]);
> +
> +	return return_val;
Commented on this in Joshua's thread.
> +}
> +
> +static int ads11x0_poll_data_ready(struct ads1100_data *data)
> +{
> +	bool data_ready;
> +	u8 buffer[3];
> +	int datarate = data->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
> +	unsigned long wait_time = DIV_ROUND_CLOSEST(MICRO, 5 * datarate);

Why that number?

> +
> +	/* To be sure that polled value will have value after config change */
> +	i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));

Why no return value check?  Even if it is just to ensure new data, nice
to know if the bus is falling over.


> +
> +	return read_poll_timeout(ads11x0_new_data_ready, data_ready,
> +				 !data_ready, wait_time,
> +				 ADS11x0_MAX_DRDY_TIMEOUT, false, data);
> +}
> +
>  static int ads1100_data_bits(struct ads1100_data *data)
>  {
>  	return ads1100_data_rate_bits[FIELD_GET(ADS1100_DR_MASK, data->config)];
> @@ -105,9 +162,10 @@ static int ads1100_get_adc_result(struct ads1100_data *data, int chan, int *val)
>  
>  	ret = i2c_master_recv(data->client, (char *)&buffer, sizeof(buffer));
>  
> +	pm_runtime_mark_last_busy(&data->client->dev);

See below. This is probably an issue with forward porting from an old kernel.
Make sure to check for things like this in upstream as you should have
wondered why something so obvious was missing!  If it were missing this
would have been a fix (it's not missing).

>  	pm_runtime_put_autosuspend(&data->client->dev);
>  
> -	if (ret < 0) {
> +	if (ret < sizeof(buffer)) {
>  		dev_err(&data->client->dev, "I2C read fail: %d\n", ret);
>  		return ret;
>  	}
> @@ -127,7 +185,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>  {
>  	int microvolts;
>  	int gain;
> -
> +	int ret;

Blank line here.

>  	/* With Vdd between 2.7 and 5V, the scale is always below 1 */
>  	if (val)
>  		return -EINVAL;
> @@ -135,7 +193,7 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>  	if (!val2)
>  		return -EINVAL;
>  
> -	microvolts = regulator_get_voltage(data->reg_vdd);
> +	microvolts = ads11x0_get_voltage_microvolts(data);
>  	/*
>  	 * val2 is in 'micro' units, n = val2 / 1000000
>  	 * result must be millivolts, d = microvolts / 1000
> @@ -147,34 +205,49 @@ static int ads1100_set_scale(struct ads1100_data *data, int val, int val2)
>  	if (gain < BIT(0) || gain > BIT(3))
>  		return -EINVAL;
>  
> +	ret = pm_runtime_resume_and_get(&data->client->dev);
> +	if (ret < 0)
> +		return ret;
> +
>  	ads1100_set_config_bits(data, ADS1100_PGA_MASK, ffs(gain) - 1);
>  
> -	return 0;
> +	ret = ads11x0_poll_data_ready(data);

As below - this applies to existing parts so if it makes sense, separate
patch before you add the new device support. That can have a description
that tells us why this is needed.

> +
> +	pm_runtime_mark_last_busy(&data->client->dev);
> +	pm_runtime_put_autosuspend(&data->client->dev);
> +
> +	return ret;
>  }
>  
>  static int ads1100_set_data_rate(struct ads1100_data *data, int chan, int rate)
>  {
>  	unsigned int i;
>  	unsigned int size;
> +	int ret;
>  
>  	size = data->supports_data_rate ? ARRAY_SIZE(ads1100_data_rate) : 1;
>  	for (i = 0; i < size; i++) {
> -		if (ads1100_data_rate[i] == rate)
> -			return ads1100_set_config_bits(data, ADS1100_DR_MASK,
> -						       FIELD_PREP(ADS1100_DR_MASK, i));
> +		if (data->data_rate[i] == rate) {
Flip logic.

		if (data->data_rate[i] != rate)
			continue;

		ret = ...

reduces indent and generally simplifies things.

> +			ret = pm_runtime_resume_and_get(&data->client->dev);
> +			if (ret < 0)
> +				return ret;
> +
> +			ads1100_set_config_bits(data, ADS1100_DR_MASK,
> +						FIELD_PREP(ADS1100_DR_MASK, i));
> +			ret = ads11x0_poll_data_ready(data);

Why is the more complex handling needed for existing devices?
If it is then smells like it should be a precursor patch with that
well explained.

> +
> +			pm_runtime_mark_last_busy(&data->client->dev);
> +			pm_runtime_put_autosuspend(&data->client->dev);

Joshua called this out already.  I guess you are forward porting form an old
kernel as for a while the mark_last_busy() has been called by
put_autosuspend().

> +			return ret;
> +		}
>  	}
>  
>  	return -EINVAL;
>  }
>  
> -static int ads1100_get_vdd_millivolts(struct ads1100_data *data)
> -{
> -	return regulator_get_voltage(data->reg_vdd) / (MICRO / MILLI);
> -}
> -
>  static void ads1100_calc_scale_avail(struct ads1100_data *data)
>  {
> -	int millivolts = ads1100_get_vdd_millivolts(data);
> +	int millivolts = ads11x0_get_voltage_milivolts(data);

As below. No to the wild card. If you want to rename that's fine
but almost certainly wants to be a separate patch with a description
of why the rename makes sense.

>  	unsigned int i;
>  
>  	for (i = 0; i < ARRAY_SIZE(data->scale_avail) / 2; i++) {
> @@ -196,7 +269,7 @@ static int ads1100_read_avail(struct iio_dev *indio_dev,
>  	switch (mask) {
>  	case IIO_CHAN_INFO_SAMP_FREQ:
>  		*type = IIO_VAL_INT;
> -		*vals = ads1100_data_rate;
> +		*vals = data->data_rate;
>  		if (data->supports_data_rate)
>  			*length = ARRAY_SIZE(ads1100_data_rate);
>  		else
> @@ -233,12 +306,11 @@ static int ads1100_read_raw(struct iio_dev *indio_dev,
>  		return IIO_VAL_INT;
>  	case IIO_CHAN_INFO_SCALE:
>  		/* full-scale is the supply voltage in millivolts */
> -		*val = ads1100_get_vdd_millivolts(data);
> +		*val = ads11x0_get_voltage_milivolts(data);
Nope. We don't use wild card naming in IIO drivers.  It goes wrong
too often as manufacturers have very inconsistent naming schemes.
+ The wild card is already wrong for the ads1000.

Sticking to existing prefix that matches the driver name is the
way to go.

>  		*val2 = 15 + FIELD_GET(ADS1100_PGA_MASK, data->config);
>  		return IIO_VAL_FRACTIONAL_LOG2;
>  	case IIO_CHAN_INFO_SAMP_FREQ:
> -		*val = ads1100_data_rate[FIELD_GET(ADS1100_DR_MASK,
> -						   data->config)];
> +		*val = data->data_rate[FIELD_GET(ADS1100_DR_MASK, data->config)];
Unrelated.  Check your patches for stuff like this. It is noise
that makes for lower quality reviews.

>  		return IIO_VAL_INT;
>  	default:
>  		return -EINVAL;
> @@ -280,8 +352,8 @@ static int ads1100_setup(struct ads1100_data *data)
>  		return ret;
>  
>  	ret = i2c_master_recv(data->client, buffer, sizeof(buffer));
> -	if (ret < 0)
> -		return ret;
> +	if (ret < sizeof(buffer))
> +		return -1;

Proper error returns and first check for ret < 0 as if it is you should return
that rather than eating the more informative error. This check is then only
on postive but too small values - so the rare case of a short recieve.

>  
>  	/* Config register returned in third byte, strip away the busy status */
>  	data->config = buffer[2] & ~ADS1100_CFG_ST_BSY;
> @@ -292,9 +364,13 @@ static int ads1100_setup(struct ads1100_data *data)
>  	return 0;
>  }
>  
> -static void ads1100_reg_disable(void *reg)
> +static void ads1100_reg_disable(void *data)
>  {
> -	regulator_disable(reg);
> +	struct ads1100_data *ads1100_data = data;
> +
> +	/* Disable when not already disabled by the driver */
> +	if (!(ads1100_data->config & ADS1100_CFG_SC))

Not obvious why regulator power is correlated with that register(?)
bit?  I'd imagine this is where issue Joshua called out that sashiko
reported is coming from.

> +		regulator_disable(ads1100_data->reg_vdd);
>  }
>  
>  static void ads1100_disable_continuous(void *data)
> @@ -307,6 +383,7 @@ static int ads1100_probe(struct i2c_client *client)
>  	struct iio_dev *indio_dev;
>  	struct ads1100_data *data;
>  	struct device *dev = &client->dev;
> +	const struct ads11x0_config *model;
>  	int ret;
>  
>  	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> @@ -334,10 +411,18 @@ static int ads1100_probe(struct i2c_client *client)
>  		return dev_err_probe(dev, ret,
>  				     "Failed to enable vdd regulator\n");
>  
> -	ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data->reg_vdd);
> +	ret = devm_add_action_or_reset(dev, ads1100_reg_disable, data);
Unrelated change. 
>  	if (ret)
>  		return ret;
>  
> +	model = device_get_match_data(dev);
> +	if (!model)
> +		return dev_err_probe(dev, ret,
> +				"Can't set device data\n");
Discussion of this in Joshua's thread.
> +

The name should where possible reflect the actual device. Easy way to do 
this is normally to put a string in the model data and use that for
iio_dev->name.  There is a slightly ABI quirk that the driver doesn't do
this for the existing pair of support parts. We should probably leave
that alone, but we can at least do better for this one.

> +	data->data_rate = model->data_rate;
> +	data->has_reference_voltage = model->has_reference_voltage;

Probably just stash a pointer to model rather that copying elements.
The element copying route tends not to scale as a driver gets more complex.

> +
>  	ret = ads1100_setup(data);
>  	if (ret)
>  		return dev_err_probe(dev, ret,
> @@ -400,16 +485,18 @@ static DEFINE_RUNTIME_DEV_PM_OPS(ads1100_pm_ops,
>  				 NULL);
>  
>  static const struct i2c_device_id ads1100_id[] = {
> -	{ "ads1100" },
> -	{ "ads1000" },
> +	{ .name = "ads1100", .driver_data = (kernel_ulong_t)&ads1100_config },
> +	{ .name = "ads1000", .driver_data = (kernel_ulong_t)&ads1100_config },
> +	{ .name = "ads1110", .driver_data = (kernel_ulong_t)&ads1110_config },
>  	{ }
>  };
>  
>  MODULE_DEVICE_TABLE(i2c, ads1100_id);
>  
>  static const struct of_device_id ads1100_of_match[] = {
> -	{.compatible = "ti,ads1100" },
> -	{.compatible = "ti,ads1000" },
> +	{ .compatible = "ti,ads1100", .data = &ads1100_config },
> +	{ .compatible = "ti,ads1000", .data = &ads1100_config },
> +	{ .compatible = "ti,ads1110", .data = &ads1110_config },

As with binding, fix the ordering to be alphanumeric whilst we are here

>  	{ }
>  };
>  

Re: [PATCH 0/3] iio: adc: Extend ti-ads1100 driver

[Joshua Crofts]

On Sun, 7 Jun 2026 at 20:38, Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:
>
> Extend ADS 1100 driver to support ADS1110, which is a pin-to-pin
> compatible device with higher resolution. This patch also updates the
> device tree bindings and Kconfig description to reflect the new
> supported device.
>
> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>

Update your git config to have your full name, you can't have usernames
etc. in your Signed-off-by tag as seen on all patches in this series.

-- 
Kind regards

CJD

Re: [PATCH 0/3] iio: adc: Extend ti-ads1100 driver

[Andy Shevchenko]

On Sun, Jun 07, 2026 at 08:35:39PM +0200, Jakub Szczudlo wrote:
> Extend ADS 1100 driver to support ADS1110, which is a pin-to-pin 
> compatible device with higher resolution. This patch also updates the 
> device tree bindings and Kconfig description to reflect the new 
> supported device.

> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>

You should use your official name and not an alias.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 0/3] iio: adc: Extend ti-ads1100 driver

[Jonathan Cameron]

On Sun,  7 Jun 2026 20:35:39 +0200
Jakub Szczudlo <jakubszczudlo40@gmail.com> wrote:

> Extend ADS 1100 driver to support ADS1110, which is a pin-to-pin 
> compatible device with higher resolution. This patch also updates the 
> device tree bindings and Kconfig description to reflect the new 
> supported device.
> 
> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
Look at your series title vs others that add support of new
devices to an existing driver.  It needs to be a lot more
specific than 'extend'.

J
> ---
> jszczudlo (3):
>   dt-bindings: iio: adc: Update title and enum
>   iio: adc: Update Kconfig description for TI_ADS1100
>   iio: adc: Add ti-ads1110 support to ti-ads1100 driver
> 
>  .../bindings/iio/adc/ti,ads1100.yaml          |   3 +-
>  drivers/iio/adc/Kconfig                       |   6 +-
>  drivers/iio/adc/ti-ads1100.c                  | 165 +++++++++++++-----
>  3 files changed, 131 insertions(+), 43 deletions(-)
> 

Re: [PATCH 0/3] iio: adc: Extend ti-ads1100 driver

[David Lechner]

On 6/7/26 1:35 PM, Jakub Szczudlo wrote:
> Extend ADS 1100 driver to support ADS1110, which is a pin-to-pin 
> compatible device with higher resolution. This patch also updates the 
> device tree bindings and Kconfig description to reflect the new 
> supported device.
> 
> Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
> ---
> jszczudlo (3):
>   dt-bindings: iio: adc: Update title and enum
>   iio: adc: Update Kconfig description for TI_ADS1100
>   iio: adc: Add ti-ads1110 support to ti-ads1100 driver
> 
>  .../bindings/iio/adc/ti,ads1100.yaml          |   3 +-
>  drivers/iio/adc/Kconfig                       |   6 +-
>  drivers/iio/adc/ti-ads1100.c                  | 165 +++++++++++++-----
>  3 files changed, 131 insertions(+), 43 deletions(-)
> 

I assume this is v2 of [1]? the subject should include [PATCH v2] and the
cover letter should include a changelog. See [2].

[1]: https://lore.kernel.org/linux-iio/20260527164312.355729-1-jakubszczudlo40@gmail.com/
[2]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html

Re: [PATCH 0/3] iio: adc: Extend ti-ads1100 driver

[Jakub Szczudło]

wt., 9 cze 2026 o 18:10 David Lechner <dlechner@baylibre.com> napisał(a):
>
> On 6/7/26 1:35 PM, Jakub Szczudlo wrote:
> > Extend ADS 1100 driver to support ADS1110, which is a pin-to-pin
> > compatible device with higher resolution. This patch also updates the
> > device tree bindings and Kconfig description to reflect the new
> > supported device.
> >
> > Signed-off-by: jszczudlo <jakubszczudlo40@gmail.com>
> > ---
> > jszczudlo (3):
> >   dt-bindings: iio: adc: Update title and enum
> >   iio: adc: Update Kconfig description for TI_ADS1100
> >   iio: adc: Add ti-ads1110 support to ti-ads1100 driver
> >
> >  .../bindings/iio/adc/ti,ads1100.yaml          |   3 +-
> >  drivers/iio/adc/Kconfig                       |   6 +-
> >  drivers/iio/adc/ti-ads1100.c                  | 165 +++++++++++++-----
> >  3 files changed, 131 insertions(+), 43 deletions(-)
> >
>
> I assume this is v2 of [1]? the subject should include [PATCH v2] and the
> cover letter should include a changelog. See [2].
>
> [1]: https://lore.kernel.org/linux-iio/20260527164312.355729-1-jakubszczudlo40@gmail.com/
> [2]: https://www.kernel.org/doc/html/latest/process/submitting-patches.html


Thanks for all reviews, I will work on implementing those