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

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

[Stepan Ionichev]

The current call sites all pass plain identifiers (chan_index,
channel, i), so this is a preventive hardening rather than a bug
fix, but wrapping macro arguments matches the kernel macro style
recommended by checkpatch and avoids surprises for future callers
that pass an expression.

Reviewed-by: Stepan Ionichev <sozdayvek@gmail.com>

Stepan

[PATCH v4 0/3] iio: adc: ti-ads1298: Minor driver cleanups

[Md Shofiqul Islam]

This series fixes three minor issues in the TI ADS1298 8-channel
medical ECG ADC driver.

Patch 1 adds missing parentheses around the macro parameter in
ADS1298_REG_CHnSET() to follow kernel macro coding style and prevent
potential operator-precedence issues when the argument is an expression.

Patch 2 corrects an incorrect comment: at the lowest supported data
rate of 250 Hz, one conversion takes 4 ms, not 40 ms. The 50 ms
timeout value itself is correct and is not changed.

Patch 3 removes an unnecessary CONFIG2 write from the init path.
The driver was enabling the internal test signal generator (INT_TEST,
TEST_AMP, TEST_FREQ_FAST), which is inappropriate for normal ECG
operation. CONFIG2 defaults to the correct value after reset (RESERVED
bit only), so the write is removed entirely.

Changes in v4:
  - Add this cover letter (pointed out by Andy Shevchenko)

Changes in v3:
  - No code changes; resent to correct the recipient list

Changes in v2:
  - Patch 3: Rework following Mike Looijmans' feedback: remove the
    CONFIG2 write entirely instead of just stripping the test signal
    bits, since CONFIG2 defaults to the correct reset value
  - Patch 3: Add Suggested-by: Mike Looijmans <mike.looijmans@topic.nl>
  - Patch 3: Rename subject to "Remove unnecessary CONFIG2 write during init"

Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>

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 v4 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

[PATCH v4 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

[PATCH v4 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 v4 2/3] iio: adc: ti-ads1298: Fix incorrect timeout comment

[David Lechner]

On 5/9/26 10:19 AM, Md Shofiqul Islam 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>
> ---
>  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));

I would say "lowest sample rate" so we know which rate it is talking about.

However, there could be latency in the kernel delaying the interrupt from
firing. The kernel latency can be much larger (I've seen 100s of ms on old
single core ARM CPUs). So I think we should mention that in the comment as
well so that no one is tempted to set it to msecs_to_jiffies(5) (or 4). Even
if that works most of the time on a fast machine, we may need the longer
timeout on slower machines.


>  	if (!ret)
>  		return -ETIMEDOUT;

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

[David Lechner]

On 5/9/26 10:19 AM, Md Shofiqul Islam 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>
> ---
>  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 */

This is why comments should say why and not what. If the original author
had a plan for this, we don't know what it was.

I look at the datasheet and the driver. Seems OK since the test signals
aren't used anywhere in the driver right now. 

> -	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 */

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

[Andy Shevchenko]

On Sat, May 09, 2026 at 03:27:35PM -0500, David Lechner wrote:
> On 5/9/26 10:19 AM, Md Shofiqul Islam 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.

...

> > -	/* 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));
> 
> I would say "lowest sample rate" so we know which rate it is talking about.
> 
> However, there could be latency in the kernel delaying the interrupt from
> firing. The kernel latency can be much larger (I've seen 100s of ms on old
> single core ARM CPUs). So I think we should mention that in the comment as
> well so that no one is tempted to set it to msecs_to_jiffies(5) (or 4). Even
> if that works most of the time on a fast machine, we may need the longer
> timeout on slower machines.

Actually it's not about fast/slow machine, it's about scheduler and load.
Even on the fast machine under heavy load the completion (if it's thread
based) may take quite a significant time to be delivered. For the hard IRQ
based completions it might be much better case, but nowadays it's more of
a niche.

-- 
With Best Regards,
Andy Shevchenko

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

[mike.looijmans]

On 10-05-2026 08:59, Andy Shevchenko wrote:
> On Sat, May 09, 2026 at 03:27:35PM -0500, David Lechner wrote:
>> On 5/9/26 10:19 AM, Md Shofiqul Islam 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.
> ...
>
>>> -	/* 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));
>> I would say "lowest sample rate" so we know which rate it is talking about.
>>
>> However, there could be latency in the kernel delaying the interrupt from
>> firing. The kernel latency can be much larger (I've seen 100s of ms on old
>> single core ARM CPUs). So I think we should mention that in the comment as
>> well so that no one is tempted to set it to msecs_to_jiffies(5) (or 4). Even
>> if that works most of the time on a fast machine, we may need the longer
>> timeout on slower machines.
> Actually it's not about fast/slow machine, it's about scheduler and load.
> Even on the fast machine under heavy load the completion (if it's thread
> based) may take quite a significant time to be delivered. For the hard IRQ
> based completions it might be much better case, but nowadays it's more of
> a niche.

This particular driver uses hard IRQ for delivering the data. At the 
common sampling rate of 500 Hz, it generates interrupts at 1 kHz (each 
cycle needs one for the chip's data ready signal and one for the SPI 
controller).

Although this particular case is the "single read", so there's indeed 
more scheduling involved.

-- 
Mike Looijmans
System Expert

TOPIC Embedded Products B.V.
Materiaalweg 4, 5681 RJ Best
The Netherlands

T: +31 (0) 499 33 69 69
E: mike.looijmans@topic.nl
W: www.topic.nl