[PATCH v2 0/3] iio: adc: ti-ads1298: Three driver cleanups

[PATCH v2 0/3] iio: adc: ti-ads1298: Three driver cleanups

[Md Shofiqul Islam]

Three small cleanups to the TI ADS1298 medical ECG ADC driver.

Changes since v1:
- Patch 3/3: Per Mike Looijmans' suggestion, instead of writing only
  the RESERVED bit to CONFIG2 (which equals the reset default), remove
  the write entirely. Added Suggested-by tag.

Patches 1/3 and 2/3 are unchanged from v1.

Md Shofiqul Islam (3):
  iio: adc: ti-ads1298: Add parentheses around macro parameter
  iio: adc: ti-ads1298: Fix incorrect timeout comment
  iio: adc: ti-ads1298: Remove unnecessary CONFIG2 write during init

 drivers/iio/adc/ti-ads1298.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

-- 
2.54.0.windows.1

[PATCH v2 1/3] iio: adc: ti-ads1298: Add parentheses around macro parameter

[Md Shofiqul Islam]

ADS1298_REG_CHnSET() is missing parentheses around the parameter 'n'.
Add them to follow kernel macro coding style and prevent potential
operator precedence issues if the argument is an expression.

Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
---
 drivers/iio/adc/ti-ads1298.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index ae30b47e45..cf5f954206 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -66,7 +66,7 @@
 #define ADS1298_MASK_CONFIG3_VREF_4V		BIT(5)
 
 #define ADS1298_REG_LOFF	0x04
-#define ADS1298_REG_CHnSET(n)	(0x05 + n)
+#define ADS1298_REG_CHnSET(n)	(0x05 + (n))
 #define ADS1298_MASK_CH_PD		BIT(7)
 #define ADS1298_MASK_CH_PGA		GENMASK(6, 4)
 #define ADS1298_MASK_CH_MUX		GENMASK(2, 0)
-- 
2.54.0.windows.1

Re: [PATCH v2 1/3] iio: adc: ti-ads1298: Add parentheses around macro parameter

[Jonathan Cameron]

On Thu,  7 May 2026 13:51:26 +0300
Md Shofiqul Islam <shofiqtest@gmail.com> wrote:

> ADS1298_REG_CHnSET() is missing parentheses around the parameter 'n'.
> Add them to follow kernel macro coding style and prevent potential
> operator precedence issues if the argument is an expression.
> 
> Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
Hi.

You should have picked up Mike's tag for v2.
Maybe he'll give it again - if not if everything else is fine I'll sort
this out whilst applying.

Thanks,

Jonathan


> ---
>  drivers/iio/adc/ti-ads1298.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
> index ae30b47e45..cf5f954206 100644
> --- a/drivers/iio/adc/ti-ads1298.c
> +++ b/drivers/iio/adc/ti-ads1298.c
> @@ -66,7 +66,7 @@
>  #define ADS1298_MASK_CONFIG3_VREF_4V		BIT(5)
>  
>  #define ADS1298_REG_LOFF	0x04
> -#define ADS1298_REG_CHnSET(n)	(0x05 + n)
> +#define ADS1298_REG_CHnSET(n)	(0x05 + (n))
>  #define ADS1298_MASK_CH_PD		BIT(7)
>  #define ADS1298_MASK_CH_PGA		GENMASK(6, 4)
>  #define ADS1298_MASK_CH_MUX		GENMASK(2, 0)

[PATCH v2 2/3] iio: adc: ti-ads1298: Fix incorrect timeout comment

[Md Shofiqul Islam]

At the lowest supported data rate of 250Hz, one conversion period is
4ms, not 40ms. Fix the comment to correctly reflect the timing.
The 50ms timeout value itself is correct as a conservative margin.

Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
---
 drivers/iio/adc/ti-ads1298.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index cf5f954206..186bda3087 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -210,7 +210,7 @@ static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
 		return ret;
 	}
 
-	/* Cannot take longer than 40ms (250Hz) */
+	/* Cannot take longer than 4ms at the lowest rate (250Hz) */
 	ret = wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50));
 	if (!ret)
 		return -ETIMEDOUT;
-- 
2.54.0.windows.1

Re: [PATCH v2 2/3] iio: adc: ti-ads1298: Fix incorrect timeout comment

[Jonathan Cameron]

On Thu,  7 May 2026 13:51:27 +0300
Md Shofiqul Islam <shofiqtest@gmail.com> wrote:

> At the lowest supported data rate of 250Hz, one conversion period is
> 4ms, not 40ms. Fix the comment to correctly reflect the timing.
> The 50ms timeout value itself is correct as a conservative margin.
> 
> Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
I'm confused - why didn't you make the change Mike requested on v1?

Fine to argue against it but you need to reply to his review.
Perhaps it went missing.

J
> ---
>  drivers/iio/adc/ti-ads1298.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
> index cf5f954206..186bda3087 100644
> --- a/drivers/iio/adc/ti-ads1298.c
> +++ b/drivers/iio/adc/ti-ads1298.c
> @@ -210,7 +210,7 @@ static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
>  		return ret;
>  	}
>  
> -	/* Cannot take longer than 40ms (250Hz) */
> +	/* Cannot take longer than 4ms at the lowest rate (250Hz) */
>  	ret = wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50));
>  	if (!ret)
>  		return -ETIMEDOUT;

[PATCH v2 3/3] iio: adc: ti-ads1298: Remove unnecessary CONFIG2 write during init

[Md Shofiqul Islam]

The driver was enabling the internal test signal (INT_TEST), double
amplitude (TEST_AMP), and fast frequency (TEST_FREQ_FAST) bits in
CONFIG2 during initialization. These bits activate an internal square
wave generator intended for device testing and calibration, not normal
ECG operation.

CONFIG2 defaults to having only the RESERVED bit set after reset, which
is the correct value for normal operation. Remove the write entirely
since it would just be writing the reset default value.

Suggested-by: Mike Looijmans <mike.looijmans@topic.nl>
Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
---
 drivers/iio/adc/ti-ads1298.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index 186bda3087..8957e873e1 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -615,15 +615,6 @@ static int ads1298_init(struct iio_dev *indio_dev)
 	if (!indio_dev->name)
 		return -ENOMEM;
 
-	/* Enable internal test signal, double amplitude, double frequency */
-	ret = regmap_write(priv->regmap, ADS1298_REG_CONFIG2,
-			   ADS1298_MASK_CONFIG2_RESERVED |
-			   ADS1298_MASK_CONFIG2_INT_TEST |
-			   ADS1298_MASK_CONFIG2_TEST_AMP |
-			   ADS1298_MASK_CONFIG2_TEST_FREQ_FAST);
-	if (ret)
-		return ret;
-
 	val = ADS1298_MASK_CONFIG3_RESERVED; /* Must write 1 always */
 	if (!priv->reg_vref) {
 		/* Enable internal reference */
-- 
2.54.0.windows.1

Re: [PATCH v2 3/3] iio: adc: ti-ads1298: Remove unnecessary CONFIG2 write during init

[Jonathan Cameron]

On Thu,  7 May 2026 13:51:28 +0300
Md Shofiqul Islam <shofiqtest@gmail.com> wrote:

> The driver was enabling the internal test signal (INT_TEST), double
> amplitude (TEST_AMP), and fast frequency (TEST_FREQ_FAST) bits in
> CONFIG2 during initialization. These bits activate an internal square
> wave generator intended for device testing and calibration, not normal
> ECG operation.
> 
> CONFIG2 defaults to having only the RESERVED bit set after reset, which
> is the correct value for normal operation. Remove the write entirely
> since it would just be writing the reset default value.
> 
> Suggested-by: Mike Looijmans <mike.looijmans@topic.nl>
> Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
Looks fine to me.

I'll pick it up once you've addressed comments on patch 2 that
are outstanding.

> ---
>  drivers/iio/adc/ti-ads1298.c | 9 ---------
>  1 file changed, 9 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
> index 186bda3087..8957e873e1 100644
> --- a/drivers/iio/adc/ti-ads1298.c
> +++ b/drivers/iio/adc/ti-ads1298.c
> @@ -615,15 +615,6 @@ static int ads1298_init(struct iio_dev *indio_dev)
>  	if (!indio_dev->name)
>  		return -ENOMEM;
>  
> -	/* Enable internal test signal, double amplitude, double frequency */
> -	ret = regmap_write(priv->regmap, ADS1298_REG_CONFIG2,
> -			   ADS1298_MASK_CONFIG2_RESERVED |
> -			   ADS1298_MASK_CONFIG2_INT_TEST |
> -			   ADS1298_MASK_CONFIG2_TEST_AMP |
> -			   ADS1298_MASK_CONFIG2_TEST_FREQ_FAST);
> -	if (ret)
> -		return ret;
> -
>  	val = ADS1298_MASK_CONFIG3_RESERVED; /* Must write 1 always */
>  	if (!priv->reg_vref) {
>  		/* Enable internal reference */