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

[PATCH v2] 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. The 50ms timeout is deliberately conservative to allow
for kernel scheduling latency, which can be significant under load or
on slow machines.

Fix the comment to state the correct conversion time, use "lowest sample
rate" for clarity, and explain that the extra margin exists to absorb
scheduling latency so that no one is tempted to shrink the timeout to
match the conversion period.

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

diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index cf5f954206..6625d9f0d0 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -210,7 +210,9 @@ static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
 		return ret;
 	}

-	/* Cannot take longer than 40ms (250Hz) */
+	/* One conversion takes at most 4ms at the lowest sample rate (250Hz).
+	 * Use 50ms to allow for kernel scheduling latency.
+	 */
 	ret = wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50));
 	if (!ret)
 		return -ETIMEDOUT;
--
2.54.0.windows.1

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

[Andy Shevchenko]

On Sun, May 10, 2026 at 11:45:30AM +0300, Md Shofiqul Islam wrote:
> At the lowest supported data rate of 250Hz, one conversion period is
> 4ms, not 40ms. The 50ms timeout is deliberately conservative to allow
> for kernel scheduling latency, which can be significant under load or
> on slow machines.
> 
> Fix the comment to state the correct conversion time, use "lowest sample
> rate" for clarity, and explain that the extra margin exists to absorb
> scheduling latency so that no one is tempted to shrink the timeout to
> match the conversion period.

...

> -	/* Cannot take longer than 40ms (250Hz) */
> +	/* One conversion takes at most 4ms at the lowest sample rate (250Hz).
> +	 * Use 50ms to allow for kernel scheduling latency.
> +	 */

/*
 * This is wrong style of multi-line comments in the IIO
 * subsystem. Use this example on how to fix it.
 */

-- 
With Best Regards,
Andy Shevchenko

[PATCH v3] 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. The 50ms timeout is deliberately conservative to allow
for kernel scheduling latency, which can be significant under load or
on slow machines.

Fix the comment to state the correct conversion time, use "lowest sample
rate" for clarity, and explain that the extra margin exists to absorb
scheduling latency so that no one is tempted to shrink the timeout to
match the conversion period.

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

diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index cf5f954206..128b45a301 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -210,7 +210,11 @@ static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
 		return ret;
 	}

-	/* Cannot take longer than 40ms (250Hz) */
+	/*
+	 * One conversion takes at most 4ms at the lowest sample rate (250Hz).
+	 * Use 50ms to allow for kernel scheduling latency.
+	 */
 	ret = wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50));
 	if (!ret)
 		return -ETIMEDOUT;
--
2.54.0.windows.1

Re: [PATCH v3] iio: adc: ti-ads1298: Fix incorrect timeout comment

[Andy Shevchenko]

On Sun, May 10, 2026 at 03:38:01PM +0300, Md Shofiqul Islam wrote:
> At the lowest supported data rate of 250Hz, one conversion period is
> 4ms, not 40ms. The 50ms timeout is deliberately conservative to allow
> for kernel scheduling latency, which can be significant under load or
> on slow machines.
> 
> Fix the comment to state the correct conversion time, use "lowest sample
> rate" for clarity, and explain that the extra margin exists to absorb
> scheduling latency so that no one is tempted to shrink the timeout to
> match the conversion period.
> 
> Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
> ---

Here should be a changelog. What's different to v2 to v1?

> -	/* Cannot take longer than 40ms (250Hz) */
> +	/*
> +	 * One conversion takes at most 4ms at the lowest sample rate (250Hz).
> +	 * Use 50ms to allow for kernel scheduling latency.
> +	 */
>  	ret = wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50));
>  	if (!ret)
>  		return -ETIMEDOUT;

It's also better to drop a ret assignment here as it's counter intuitive.

	if (!wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50)))
		return -ETIMEDOUT;

(This might require more changes related to this ret drop.)

-- 
With Best Regards,
Andy Shevchenko

[PATCH v4] 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. The 50ms timeout is deliberately conservative to allow
for kernel scheduling latency, which can be significant under load or
on slow machines.

Fix the comment to state the correct conversion time, use "lowest sample
rate" for clarity, and explain that the extra margin exists to absorb
scheduling latency so that no one is tempted to shrink the timeout to
match the conversion period.

Also drop the redundant ret variable assignment by using the return value
of wait_for_completion_timeout() directly in the if() condition.

Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
---
Changes in v4:
 - Drop the ret assignment for wait_for_completion_timeout(); use the
   return value directly in the if() condition (Andy Shevchenko)

Changes in v3:
 - Fix multi-line comment style: move comment text to its own line
   after '/*' (Andy Shevchenko)

Changes in v2:
 - Correct conversion time to 4ms (was wrong 40ms), add scheduling
   latency explanation, clarify "lowest sample rate" (David Lechner,
   Andy Shevchenko)

 drivers/iio/adc/ti-ads1298.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index ae30b47e45..25261163d3 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -210,9 +210,11 @@ static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
 		return ret;
 	}
 
-	/* Cannot take longer than 40ms (250Hz) */
-	ret = wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50));
-	if (!ret)
+	/*
+	 * One conversion takes at most 4ms at the lowest sample rate (250Hz).
+	 * Use 50ms to allow for kernel scheduling latency.
+	 */
+	if (!wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50)))
 		return -ETIMEDOUT;
 
 	return 0;
-- 
2.54.0.windows.1

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

[Jonathan Cameron]

On Sun, 10 May 2026 22:34:35 +0300
Md Shofiqul Islam <shofiqtest@gmail.com> wrote:

> At the lowest supported data rate of 250Hz, one conversion period is
> 4ms, not 40ms. The 50ms timeout is deliberately conservative to allow
> for kernel scheduling latency, which can be significant under load or
> on slow machines.
> 
> Fix the comment to state the correct conversion time, use "lowest sample
> rate" for clarity, and explain that the extra margin exists to absorb
> scheduling latency so that no one is tempted to shrink the timeout to
> match the conversion period.
> 
> Also drop the redundant ret variable assignment by using the return value
> of wait_for_completion_timeout() directly in the if() condition.
> 
> Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
Hi. Please slow down a bit and perhaps more importantly be careful
with your various series numbers etc.  I see the same patch in a series
label v4 then as a patch on it's own labelled v2, one labelled v3 and
2 patches labeled v4 sent a few minute apart

I'm going to pick up the most recent one probably but that might
well be the wrong one!


> ---
> Changes in v4:
>  - Drop the ret assignment for wait_for_completion_timeout(); use the
>    return value directly in the if() condition (Andy Shevchenko)
> 
> Changes in v3:
>  - Fix multi-line comment style: move comment text to its own line
>    after '/*' (Andy Shevchenko)
> 
> Changes in v2:
>  - Correct conversion time to 4ms (was wrong 40ms), add scheduling
>    latency explanation, clarify "lowest sample rate" (David Lechner,
>    Andy Shevchenko)
> 
>  drivers/iio/adc/ti-ads1298.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
> index ae30b47e45..25261163d3 100644
> --- a/drivers/iio/adc/ti-ads1298.c
> +++ b/drivers/iio/adc/ti-ads1298.c
> @@ -210,9 +210,11 @@ static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
>  		return ret;
>  	}
>  
> -	/* Cannot take longer than 40ms (250Hz) */
> -	ret = wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50));
> -	if (!ret)
> +	/*
> +	 * One conversion takes at most 4ms at the lowest sample rate (250Hz).
> +	 * Use 50ms to allow for kernel scheduling latency.
> +	 */
> +	if (!wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50)))
>  		return -ETIMEDOUT;
>  
>  	return 0;

[PATCH v4] 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. The 50ms timeout is deliberately conservative to allow
for kernel scheduling latency, which can be significant under load or
on slow machines.

Fix the comment to state the correct conversion time, use "lowest sample
rate" for clarity, and explain that the extra margin exists to absorb
scheduling latency so that no one is tempted to shrink the timeout to
match the conversion period.

Also drop the redundant ret variable assignment by using the return value
of wait_for_completion_timeout() directly in the if() condition.

Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
---
Changes in v4:
 - Drop the ret assignment for wait_for_completion_timeout(); use the
   return value directly in the if() condition (Andy Shevchenko)

Changes in v3:
 - Fix multi-line comment style: move comment text to its own line
   after '/*' (Andy Shevchenko)

Changes in v2:
 - Correct conversion time to 4ms (was wrong 40ms), add scheduling
   latency explanation, clarify "lowest sample rate" (David Lechner,
   Andy Shevchenko)

 drivers/iio/adc/ti-ads1298.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
index ae30b47e45..25261163d3 100644
--- a/drivers/iio/adc/ti-ads1298.c
+++ b/drivers/iio/adc/ti-ads1298.c
@@ -210,9 +210,11 @@ static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
 		return ret;
 	}
 
-	/* Cannot take longer than 40ms (250Hz) */
-	ret = wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50));
-	if (!ret)
+	/*
+	 * One conversion takes at most 4ms at the lowest sample rate (250Hz).
+	 * Use 50ms to allow for kernel scheduling latency.
+	 */
+	if (!wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50)))
 		return -ETIMEDOUT;
 
 	return 0;
-- 
2.54.0.windows.1

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

[Jonathan Cameron]

On Sun, 10 May 2026 22:36:45 +0300
Md Shofiqul Islam <shofiqtest@gmail.com> wrote:

> At the lowest supported data rate of 250Hz, one conversion period is
> 4ms, not 40ms. The 50ms timeout is deliberately conservative to allow
> for kernel scheduling latency, which can be significant under load or
> on slow machines.
> 
> Fix the comment to state the correct conversion time, use "lowest sample
> rate" for clarity, and explain that the extra margin exists to absorb
> scheduling latency so that no one is tempted to shrink the timeout to
> match the conversion period.
> 
> Also drop the redundant ret variable assignment by using the return value
> of wait_for_completion_timeout() directly in the if() condition.
> 
> Signed-off-by: Md Shofiqul Islam <shofiqtest@gmail.com>
Given the versioning mess - please check I got the right one on
the testing branch of iio.git.

Probably applied this one - it broke b4 btw as that picked up two
near identical patches as a single series.

Jonathan

> ---
> Changes in v4:
>  - Drop the ret assignment for wait_for_completion_timeout(); use the
>    return value directly in the if() condition (Andy Shevchenko)
> 
> Changes in v3:
>  - Fix multi-line comment style: move comment text to its own line
>    after '/*' (Andy Shevchenko)
> 
> Changes in v2:
>  - Correct conversion time to 4ms (was wrong 40ms), add scheduling
>    latency explanation, clarify "lowest sample rate" (David Lechner,
>    Andy Shevchenko)
> 
>  drivers/iio/adc/ti-ads1298.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti-ads1298.c b/drivers/iio/adc/ti-ads1298.c
> index ae30b47e45..25261163d3 100644
> --- a/drivers/iio/adc/ti-ads1298.c
> +++ b/drivers/iio/adc/ti-ads1298.c
> @@ -210,9 +210,11 @@ static int ads1298_read_one(struct ads1298_private *priv, int chan_index)
>  		return ret;
>  	}
>  
> -	/* Cannot take longer than 40ms (250Hz) */
> -	ret = wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50));
> -	if (!ret)
> +	/*
> +	 * One conversion takes at most 4ms at the lowest sample rate (250Hz).
> +	 * Use 50ms to allow for kernel scheduling latency.
> +	 */
> +	if (!wait_for_completion_timeout(&priv->completion, msecs_to_jiffies(50)))
>  		return -ETIMEDOUT;
>  
>  	return 0;