[PATCH] IIO: Change msleep to usleep_range for small msecs

[PATCH] IIO: Change msleep to usleep_range for small msecs

[Aniroop Mathur]

msleep(1~20) may not do what the caller intends, and will often sleep longer.
(~20 ms actual sleep for any value given in the 1~20ms range)
This is not the desired behaviour for many cases like device resume time,
device suspend time, device enable time, data reading time, etc.
Thus, change msleep to usleep_range for precise wakeups.

Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
---
 drivers/iio/adc/exynos_adc.c             |  2 +-
 drivers/iio/pressure/bmp280-core.c       | 14 +++++++-------
 drivers/staging/iio/meter/ade7753.c      |  2 +-
 drivers/staging/iio/meter/ade7753.h      |  2 +-
 drivers/staging/iio/meter/ade7754.c      |  2 +-
 drivers/staging/iio/meter/ade7754.h      |  2 +-
 drivers/staging/iio/meter/ade7758.h      |  2 +-
 drivers/staging/iio/meter/ade7758_core.c |  2 +-
 drivers/staging/iio/meter/ade7759.c      |  2 +-
 drivers/staging/iio/meter/ade7759.h      |  2 +-
 drivers/staging/iio/meter/ade7854.c      |  2 +-
 drivers/staging/iio/meter/ade7854.h      |  2 +-
 12 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
index c15756d..ad1775b 100644
--- a/drivers/iio/adc/exynos_adc.c
+++ b/drivers/iio/adc/exynos_adc.c
@@ -632,7 +632,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
 		input_report_key(info->input, BTN_TOUCH, 1);
 		input_sync(info->input);
 
-		msleep(1);
+		usleep_range(1000, 1100);
 	};
 
 	writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
index e5a533c..4d18826 100644
--- a/drivers/iio/pressure/bmp280-core.c
+++ b/drivers/iio/pressure/bmp280-core.c
@@ -65,7 +65,7 @@ struct bmp280_data {
 	struct bmp180_calib calib;
 	struct regulator *vddd;
 	struct regulator *vdda;
-	unsigned int start_up_time; /* in milliseconds */
+	unsigned int start_up_time; /* in microseconds */
 
 	/* log of base 2 of oversampling rate */
 	u8 oversampling_press;
@@ -935,14 +935,14 @@ int bmp280_common_probe(struct device *dev,
 		data->chip_info = &bmp180_chip_info;
 		data->oversampling_press = ilog2(8);
 		data->oversampling_temp = ilog2(1);
-		data->start_up_time = 10;
+		data->start_up_time = 10000;
 		break;
 	case BMP280_CHIP_ID:
 		indio_dev->num_channels = 2;
 		data->chip_info = &bmp280_chip_info;
 		data->oversampling_press = ilog2(16);
 		data->oversampling_temp = ilog2(2);
-		data->start_up_time = 2;
+		data->start_up_time = 2000;
 		break;
 	case BME280_CHIP_ID:
 		indio_dev->num_channels = 3;
@@ -950,7 +950,7 @@ int bmp280_common_probe(struct device *dev,
 		data->oversampling_press = ilog2(16);
 		data->oversampling_humid = ilog2(16);
 		data->oversampling_temp = ilog2(2);
-		data->start_up_time = 2;
+		data->start_up_time = 2000;
 		break;
 	default:
 		return -EINVAL;
@@ -979,7 +979,7 @@ int bmp280_common_probe(struct device *dev,
 		goto out_disable_vddd;
 	}
 	/* Wait to make sure we started up properly */
-	mdelay(data->start_up_time);
+	usleep_range(data->start_up_time, data->start_up_time + 100);
 
 	/* Bring chip out of reset if there is an assigned GPIO line */
 	gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
@@ -1038,7 +1038,7 @@ int bmp280_common_probe(struct device *dev,
 	 * Set autosuspend to two orders of magnitude larger than the
 	 * start-up time.
 	 */
-	pm_runtime_set_autosuspend_delay(dev, data->start_up_time *100);
+	pm_runtime_set_autosuspend_delay(dev, data->start_up_time / 10);
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_put(dev);
 
@@ -1101,7 +1101,7 @@ static int bmp280_runtime_resume(struct device *dev)
 	ret = regulator_enable(data->vdda);
 	if (ret)
 		return ret;
-	msleep(data->start_up_time);
+	usleep_range(data->start_up_time, data->start_up_time + 100);
 	return data->chip_info->chip_config(data);
 }
 #endif /* CONFIG_PM */
diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
index 4b5f05f..671dc99 100644
--- a/drivers/staging/iio/meter/ade7753.c
+++ b/drivers/staging/iio/meter/ade7753.c
@@ -377,7 +377,7 @@ static int ade7753_initial_setup(struct iio_dev *indio_dev)
 	}
 
 	ade7753_reset(dev);
-	msleep(ADE7753_STARTUP_DELAY);
+	usleep_range(ADE7753_STARTUP_DELAY, ADE7753_STARTUP_DELAY + 100);
 
 err_ret:
 	return ret;
diff --git a/drivers/staging/iio/meter/ade7753.h b/drivers/staging/iio/meter/ade7753.h
index a9d93cc..bfe7491 100644
--- a/drivers/staging/iio/meter/ade7753.h
+++ b/drivers/staging/iio/meter/ade7753.h
@@ -49,7 +49,7 @@
 
 #define ADE7753_MAX_TX    4
 #define ADE7753_MAX_RX    4
-#define ADE7753_STARTUP_DELAY 1
+#define ADE7753_STARTUP_DELAY 1000
 
 #define ADE7753_SPI_SLOW	(u32)(300 * 1000)
 #define ADE7753_SPI_BURST	(u32)(1000 * 1000)
diff --git a/drivers/staging/iio/meter/ade7754.c b/drivers/staging/iio/meter/ade7754.c
index c46bef6..49eb10b 100644
--- a/drivers/staging/iio/meter/ade7754.c
+++ b/drivers/staging/iio/meter/ade7754.c
@@ -396,7 +396,7 @@ static int ade7754_initial_setup(struct iio_dev *indio_dev)
 	}
 
 	ade7754_reset(dev);
-	msleep(ADE7754_STARTUP_DELAY);
+	usleep_range(ADE7754_STARTUP_DELAY, ADE7754_STARTUP_DELAY + 100);
 
 err_ret:
 	return ret;
diff --git a/drivers/staging/iio/meter/ade7754.h b/drivers/staging/iio/meter/ade7754.h
index e42ffc3..28f71c2 100644
--- a/drivers/staging/iio/meter/ade7754.h
+++ b/drivers/staging/iio/meter/ade7754.h
@@ -67,7 +67,7 @@
 
 #define ADE7754_MAX_TX    4
 #define ADE7754_MAX_RX    4
-#define ADE7754_STARTUP_DELAY 1
+#define ADE7754_STARTUP_DELAY 1000
 
 #define ADE7754_SPI_SLOW	(u32)(300 * 1000)
 #define ADE7754_SPI_BURST	(u32)(1000 * 1000)
diff --git a/drivers/staging/iio/meter/ade7758.h b/drivers/staging/iio/meter/ade7758.h
index 1d04ec9..6ae78d8 100644
--- a/drivers/staging/iio/meter/ade7758.h
+++ b/drivers/staging/iio/meter/ade7758.h
@@ -89,7 +89,7 @@
 
 #define ADE7758_MAX_TX    8
 #define ADE7758_MAX_RX    4
-#define ADE7758_STARTUP_DELAY 1
+#define ADE7758_STARTUP_DELAY 1000
 
 #define AD7758_NUM_WAVSEL	5
 #define AD7758_NUM_PHSEL	3
diff --git a/drivers/staging/iio/meter/ade7758_core.c b/drivers/staging/iio/meter/ade7758_core.c
index ebb8a19..3767246 100644
--- a/drivers/staging/iio/meter/ade7758_core.c
+++ b/drivers/staging/iio/meter/ade7758_core.c
@@ -459,7 +459,7 @@ static int ade7758_initial_setup(struct iio_dev *indio_dev)
 	}
 
 	ade7758_reset(dev);
-	msleep(ADE7758_STARTUP_DELAY);
+	usleep_range(ADE7758_STARTUP_DELAY, ADE7758_STARTUP_DELAY + 100);
 
 err_ret:
 	return ret;
diff --git a/drivers/staging/iio/meter/ade7759.c b/drivers/staging/iio/meter/ade7759.c
index 80144d4..944ee34 100644
--- a/drivers/staging/iio/meter/ade7759.c
+++ b/drivers/staging/iio/meter/ade7759.c
@@ -338,7 +338,7 @@ static int ade7759_initial_setup(struct iio_dev *indio_dev)
 	}
 
 	ade7759_reset(dev);
-	msleep(ADE7759_STARTUP_DELAY);
+	usleep_range(ADE7759_STARTUP_DELAY, ADE7759_STARTUP_DELAY + 100);
 
 err_ret:
 	return ret;
diff --git a/drivers/staging/iio/meter/ade7759.h b/drivers/staging/iio/meter/ade7759.h
index f9ff1f8..f0716d2 100644
--- a/drivers/staging/iio/meter/ade7759.h
+++ b/drivers/staging/iio/meter/ade7759.h
@@ -30,7 +30,7 @@
 
 #define ADE7759_MAX_TX    6
 #define ADE7759_MAX_RX    6
-#define ADE7759_STARTUP_DELAY 1
+#define ADE7759_STARTUP_DELAY 1000
 
 #define ADE7759_SPI_SLOW	(u32)(300 * 1000)
 #define ADE7759_SPI_BURST	(u32)(1000 * 1000)
diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
index 75e8685..58a05ec 100644
--- a/drivers/staging/iio/meter/ade7854.c
+++ b/drivers/staging/iio/meter/ade7854.c
@@ -444,7 +444,7 @@ static int ade7854_initial_setup(struct iio_dev *indio_dev)
 	}
 
 	ade7854_reset(dev);
-	msleep(ADE7854_STARTUP_DELAY);
+	usleep_range(ADE7854_STARTUP_DELAY, ADE7854_STARTUP_DELAY + 100);
 
 err_ret:
 	return ret;
diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h
index 52f4195..dbd97de 100644
--- a/drivers/staging/iio/meter/ade7854.h
+++ b/drivers/staging/iio/meter/ade7854.h
@@ -136,7 +136,7 @@
 
 #define ADE7854_MAX_TX    7
 #define ADE7854_MAX_RX    7
-#define ADE7854_STARTUP_DELAY 1
+#define ADE7854_STARTUP_DELAY 1000
 
 #define ADE7854_SPI_SLOW	(u32)(300 * 1000)
 #define ADE7854_SPI_BURST	(u32)(1000 * 1000)
-- 
2.6.2

Re: [PATCH] IIO: Change msleep to usleep_range for small msecs

[Jonathan Cameron]

On 26/11/16 03:47, Aniroop Mathur wrote:
> msleep(1~20) may not do what the caller intends, and will often sleep longer.
> (~20 ms actual sleep for any value given in the 1~20ms range)
> This is not the desired behaviour for many cases like device resume time,
> device suspend time, device enable time, data reading time, etc.
> Thus, change msleep to usleep_range for precise wakeups.
> 
> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
As these need individual review by the various original authors and driver maintainers to
establish the intent of the sleep, it would have been better to have done a series of
patches (one per driver) with the relevant maintainers cc'd on the ones that they care about.

Most of these are ADI parts looked after by Lars though so perhaps wait for his comments
before respining.
> ---
>  drivers/iio/adc/exynos_adc.c             |  2 +-
>  drivers/iio/pressure/bmp280-core.c       | 14 +++++++-------
>  drivers/staging/iio/meter/ade7753.c      |  2 +-
>  drivers/staging/iio/meter/ade7753.h      |  2 +-
>  drivers/staging/iio/meter/ade7754.c      |  2 +-
>  drivers/staging/iio/meter/ade7754.h      |  2 +-
>  drivers/staging/iio/meter/ade7758.h      |  2 +-
>  drivers/staging/iio/meter/ade7758_core.c |  2 +-
>  drivers/staging/iio/meter/ade7759.c      |  2 +-
>  drivers/staging/iio/meter/ade7759.h      |  2 +-
>  drivers/staging/iio/meter/ade7854.c      |  2 +-
>  drivers/staging/iio/meter/ade7854.h      |  2 +-
>  12 files changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/exynos_adc.c b/drivers/iio/adc/exynos_adc.c
> index c15756d..ad1775b 100644
> --- a/drivers/iio/adc/exynos_adc.c
> +++ b/drivers/iio/adc/exynos_adc.c
> @@ -632,7 +632,7 @@ static irqreturn_t exynos_ts_isr(int irq, void *dev_id)
>  		input_report_key(info->input, BTN_TOUCH, 1);
>  		input_sync(info->input);
>  
> -		msleep(1);
> +		usleep_range(1000, 1100);
This needs feedback on what an appropriate range actually is.  Also in this particular
case they will have a better idea of what msleep delivers as it's on a SoC where the clocks
are entirely under their control.  Worth adding a cc for Naveen as well as the original
driver author (may well bounce - I have no idea!)
>  	};
>  
>  	writel(0, ADC_V1_CLRINTPNDNUP(info->regs));
> diff --git a/drivers/iio/pressure/bmp280-core.c b/drivers/iio/pressure/bmp280-core.c
> index e5a533c..4d18826 100644
> --- a/drivers/iio/pressure/bmp280-core.c
> +++ b/drivers/iio/pressure/bmp280-core.c
> @@ -65,7 +65,7 @@ struct bmp280_data {
>  	struct bmp180_calib calib;
>  	struct regulator *vddd;
>  	struct regulator *vdda;
> -	unsigned int start_up_time; /* in milliseconds */
> +	unsigned int start_up_time; /* in microseconds */
Would have been less invasive to leave this alone and just multiply by 1000 where
relevant for the sleep.
>  
>  	/* log of base 2 of oversampling rate */
>  	u8 oversampling_press;
> @@ -935,14 +935,14 @@ int bmp280_common_probe(struct device *dev,
>  		data->chip_info = &bmp180_chip_info;
>  		data->oversampling_press = ilog2(8);
>  		data->oversampling_temp = ilog2(1);
> -		data->start_up_time = 10;
> +		data->start_up_time = 10000;
>  		break;
>  	case BMP280_CHIP_ID:
>  		indio_dev->num_channels = 2;
>  		data->chip_info = &bmp280_chip_info;
>  		data->oversampling_press = ilog2(16);
>  		data->oversampling_temp = ilog2(2);
> -		data->start_up_time = 2;
> +		data->start_up_time = 2000;
>  		break;
>  	case BME280_CHIP_ID:
>  		indio_dev->num_channels = 3;
> @@ -950,7 +950,7 @@ int bmp280_common_probe(struct device *dev,
>  		data->oversampling_press = ilog2(16);
>  		data->oversampling_humid = ilog2(16);
>  		data->oversampling_temp = ilog2(2);
> -		data->start_up_time = 2;
> +		data->start_up_time = 2000;
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -979,7 +979,7 @@ int bmp280_common_probe(struct device *dev,
>  		goto out_disable_vddd;
>  	}
>  	/* Wait to make sure we started up properly */
> -	mdelay(data->start_up_time);
> +	usleep_range(data->start_up_time, data->start_up_time + 100);
As this in probe I doubt we really care.  Could just set it longer to shut up the warnings.
Still would like some input from say Linus on this...
>  
>  	/* Bring chip out of reset if there is an assigned GPIO line */
>  	gpiod = devm_gpiod_get(dev, "reset", GPIOD_OUT_HIGH);
> @@ -1038,7 +1038,7 @@ int bmp280_common_probe(struct device *dev,
>  	 * Set autosuspend to two orders of magnitude larger than the
>  	 * start-up time.
>  	 */
> -	pm_runtime_set_autosuspend_delay(dev, data->start_up_time *100);
> +	pm_runtime_set_autosuspend_delay(dev, data->start_up_time / 10);
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_put(dev);
>  
> @@ -1101,7 +1101,7 @@ static int bmp280_runtime_resume(struct device *dev)
>  	ret = regulator_enable(data->vdda);
>  	if (ret)
>  		return ret;
> -	msleep(data->start_up_time);
> +	usleep_range(data->start_up_time, data->start_up_time + 100);
>  	return data->chip_info->chip_config(data);
>  }
>  #endif /* CONFIG_PM */
> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
> index 4b5f05f..671dc99 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -377,7 +377,7 @@ static int ade7753_initial_setup(struct iio_dev *indio_dev)
>  	}
>  
>  	ade7753_reset(dev);
> -	msleep(ADE7753_STARTUP_DELAY);
> +	usleep_range(ADE7753_STARTUP_DELAY, ADE7753_STARTUP_DELAY + 100);
Again these are in slow paths really so may be perfectly acceptable to wait much longer...
>  
>  err_ret:
>  	return ret;
> diff --git a/drivers/staging/iio/meter/ade7753.h b/drivers/staging/iio/meter/ade7753.h
> index a9d93cc..bfe7491 100644
> --- a/drivers/staging/iio/meter/ade7753.h
> +++ b/drivers/staging/iio/meter/ade7753.h
> @@ -49,7 +49,7 @@
>  
>  #define ADE7753_MAX_TX    4
>  #define ADE7753_MAX_RX    4
> -#define ADE7753_STARTUP_DELAY 1
> +#define ADE7753_STARTUP_DELAY 1000
>  
>  #define ADE7753_SPI_SLOW	(u32)(300 * 1000)
>  #define ADE7753_SPI_BURST	(u32)(1000 * 1000)
> diff --git a/drivers/staging/iio/meter/ade7754.c b/drivers/staging/iio/meter/ade7754.c
> index c46bef6..49eb10b 100644
> --- a/drivers/staging/iio/meter/ade7754.c
> +++ b/drivers/staging/iio/meter/ade7754.c
> @@ -396,7 +396,7 @@ static int ade7754_initial_setup(struct iio_dev *indio_dev)
>  	}
>  
>  	ade7754_reset(dev);
> -	msleep(ADE7754_STARTUP_DELAY);
> +	usleep_range(ADE7754_STARTUP_DELAY, ADE7754_STARTUP_DELAY + 100);
>  
>  err_ret:
>  	return ret;
> diff --git a/drivers/staging/iio/meter/ade7754.h b/drivers/staging/iio/meter/ade7754.h
> index e42ffc3..28f71c2 100644
> --- a/drivers/staging/iio/meter/ade7754.h
> +++ b/drivers/staging/iio/meter/ade7754.h
> @@ -67,7 +67,7 @@
>  
>  #define ADE7754_MAX_TX    4
>  #define ADE7754_MAX_RX    4
> -#define ADE7754_STARTUP_DELAY 1
> +#define ADE7754_STARTUP_DELAY 1000
>  
>  #define ADE7754_SPI_SLOW	(u32)(300 * 1000)
>  #define ADE7754_SPI_BURST	(u32)(1000 * 1000)
> diff --git a/drivers/staging/iio/meter/ade7758.h b/drivers/staging/iio/meter/ade7758.h
> index 1d04ec9..6ae78d8 100644
> --- a/drivers/staging/iio/meter/ade7758.h
> +++ b/drivers/staging/iio/meter/ade7758.h
> @@ -89,7 +89,7 @@
>  
>  #define ADE7758_MAX_TX    8
>  #define ADE7758_MAX_RX    4
> -#define ADE7758_STARTUP_DELAY 1
> +#define ADE7758_STARTUP_DELAY 1000
>  
>  #define AD7758_NUM_WAVSEL	5
>  #define AD7758_NUM_PHSEL	3
> diff --git a/drivers/staging/iio/meter/ade7758_core.c b/drivers/staging/iio/meter/ade7758_core.c
> index ebb8a19..3767246 100644
> --- a/drivers/staging/iio/meter/ade7758_core.c
> +++ b/drivers/staging/iio/meter/ade7758_core.c
> @@ -459,7 +459,7 @@ static int ade7758_initial_setup(struct iio_dev *indio_dev)
>  	}
>  
>  	ade7758_reset(dev);
> -	msleep(ADE7758_STARTUP_DELAY);
> +	usleep_range(ADE7758_STARTUP_DELAY, ADE7758_STARTUP_DELAY + 100);
>  
>  err_ret:
>  	return ret;
> diff --git a/drivers/staging/iio/meter/ade7759.c b/drivers/staging/iio/meter/ade7759.c
> index 80144d4..944ee34 100644
> --- a/drivers/staging/iio/meter/ade7759.c
> +++ b/drivers/staging/iio/meter/ade7759.c
> @@ -338,7 +338,7 @@ static int ade7759_initial_setup(struct iio_dev *indio_dev)
>  	}
>  
>  	ade7759_reset(dev);
> -	msleep(ADE7759_STARTUP_DELAY);
> +	usleep_range(ADE7759_STARTUP_DELAY, ADE7759_STARTUP_DELAY + 100);
>  
>  err_ret:
>  	return ret;
> diff --git a/drivers/staging/iio/meter/ade7759.h b/drivers/staging/iio/meter/ade7759.h
> index f9ff1f8..f0716d2 100644
> --- a/drivers/staging/iio/meter/ade7759.h
> +++ b/drivers/staging/iio/meter/ade7759.h
> @@ -30,7 +30,7 @@
>  
>  #define ADE7759_MAX_TX    6
>  #define ADE7759_MAX_RX    6
> -#define ADE7759_STARTUP_DELAY 1
> +#define ADE7759_STARTUP_DELAY 1000
>  
>  #define ADE7759_SPI_SLOW	(u32)(300 * 1000)
>  #define ADE7759_SPI_BURST	(u32)(1000 * 1000)
> diff --git a/drivers/staging/iio/meter/ade7854.c b/drivers/staging/iio/meter/ade7854.c
> index 75e8685..58a05ec 100644
> --- a/drivers/staging/iio/meter/ade7854.c
> +++ b/drivers/staging/iio/meter/ade7854.c
> @@ -444,7 +444,7 @@ static int ade7854_initial_setup(struct iio_dev *indio_dev)
>  	}
>  
>  	ade7854_reset(dev);
> -	msleep(ADE7854_STARTUP_DELAY);
> +	usleep_range(ADE7854_STARTUP_DELAY, ADE7854_STARTUP_DELAY + 100);
>  
>  err_ret:
>  	return ret;
> diff --git a/drivers/staging/iio/meter/ade7854.h b/drivers/staging/iio/meter/ade7854.h
> index 52f4195..dbd97de 100644
> --- a/drivers/staging/iio/meter/ade7854.h
> +++ b/drivers/staging/iio/meter/ade7854.h
> @@ -136,7 +136,7 @@
>  
>  #define ADE7854_MAX_TX    7
>  #define ADE7854_MAX_RX    7
> -#define ADE7854_STARTUP_DELAY 1
> +#define ADE7854_STARTUP_DELAY 1000
This particular define seems rather pointless. Would be more readable inline within the
code!  Same for the other cases.
>  
>  #define ADE7854_SPI_SLOW	(u32)(300 * 1000)
>  #define ADE7854_SPI_BURST	(u32)(1000 * 1000)
> 

Re: [PATCH] IIO: Change msleep to usleep_range for small msecs

[Linus Walleij]

On Sun, Nov 27, 2016 at 11:51 AM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 26/11/16 03:47, Aniroop Mathur wrote:

[bmp280.c]

>>       /* Wait to make sure we started up properly */
>> -     mdelay(data->start_up_time);
>> +     usleep_range(data->start_up_time, data->start_up_time + 100);
>
> As this in probe I doubt we really care.  Could just set it longer to shut up the warnings.
> Still would like some input from say Linus on this...

Hm, I don't think it's a big issue... this works too it just looks overworked.

On the runtime_resume() path we use msleep() instead which I guess is why
it is not changed in this patch, but they have the same purpose.

Yours,
Linus Walleij

Re: [PATCH] IIO: Change msleep to usleep_range for small msecs

[Lars-Peter Clausen]

On 11/27/2016 11:51 AM, Jonathan Cameron wrote:
> On 26/11/16 03:47, Aniroop Mathur wrote:
>> msleep(1~20) may not do what the caller intends, and will often sleep longer.
>> (~20 ms actual sleep for any value given in the 1~20ms range)
>> This is not the desired behaviour for many cases like device resume time,
>> device suspend time, device enable time, data reading time, etc.
>> Thus, change msleep to usleep_range for precise wakeups.
>>
>> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
> As these need individual review by the various original authors and driver maintainers to
> establish the intent of the sleep, it would have been better to have done a series of
> patches (one per driver) with the relevant maintainers cc'd on the ones that they care about.
> 
> Most of these are ADI parts looked after by Lars though so perhaps wait for his comments
> before respining.

I agree with what Jonathan said. For most of these extending the maximum
sleep time a bit further is ok.

RE: Re: [PATCH] IIO: Change msleep to usleep_range for small msecs

[Aniroop Mathur]

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

On 30 Nov 2016 19:05, "Lars-Peter Clausen" <lars@metafoo.de> wrote:
 >
 > On 11/27/2016 11:51 AM, Jonathan Cameron wrote:
 > > On 26/11/16 03:47, Aniroop Mathur wrote:
 > >> msleep(1~20) may not do what the caller intends, and will often sleep longer.
 > >> (~20 ms actual sleep for any value given in the 1~20ms range)
 > >> This is not the desired behaviour for many cases like device resume time,
 > >> device suspend time, device enable time, data reading time, etc.
 > >> Thus, change msleep to usleep_range for precise wakeups.
 > >>
 > >> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
 > > As these need individual review by the various original authors and driver maintainers to
 > > establish the intent of the sleep, it would have been better to have done a series of
 > > patches (one per driver) with the relevant maintainers cc'd on the ones that they care about.
 > >
 > > Most of these are ADI parts looked after by Lars though so perhaps wait for his comments
 > > before respining.
 >
 > I agree with what Jonathan said. For most of these extending the maximum
 > sleep time a bit further is ok.
 >

Well, its right that sleep a bit further is okay but this patch is not trying
to solve any major bug. This patch is only trying to make the wake up more
precise than before. So like with msleep(1), process could sleep for 20 ms
so this patch only making the making the process to sleep for 1 ms as
mentioned in the parameter. So I think, changing to usleep_range is only
advantageous and does not cause any harm here.
We have also applied the same change in enable/disable/suspend/resume
functions in accel, gyro, mag, etc drivers and found decent results like the
first sesor data is generated much faster than before so response time
increased. This is critical as  sensor can run at a rate of 200Hz / 5ms or
even more now a days in new devices. We even applied in probe as doing the
same in many drivers contribute to a little saving overall in kernel boot up.
Also, it is recommended and mentioned in kernel documentation to use
usleep_range for 1-10 ms.
Sources -
https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
https://lkml.org/lkml/2007/8/3/250

Thanks.

BR,
Aniroop Mathur

Re: [PATCH] IIO: Change msleep to usleep_range for small msecs

[Aniroop Mathur]

On Wed, Nov 30, 2016 at 5:49 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sun, Nov 27, 2016 at 11:51 AM, Jonathan Cameron <jic23@kernel.org> wrote:
>> On 26/11/16 03:47, Aniroop Mathur wrote:
>
> [bmp280.c]
>
>>>       /* Wait to make sure we started up properly */
>>> -     mdelay(data->start_up_time);
>>> +     usleep_range(data->start_up_time, data->start_up_time + 100);
>>
>> As this in probe I doubt we really care.  Could just set it longer to shut up the warnings.
>> Still would like some input from say Linus on this...
>
> Hm, I don't think it's a big issue... this works too it just looks overworked.
>
> On the runtime_resume() path we use msleep() instead which I guess is why
> it is not changed in this patch, but they have the same purpose.
>

I did change msleep to usleep_range in runtime_resume() in bmp280.c
as you know resume time is critical indeed.


> Yours,
> Linus Walleij

Re: Re: [PATCH] IIO: Change msleep to usleep_range for small msecs

[Aniroop Mathur]

On Wed, Nov 30, 2016 at 8:13 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
> On 30 Nov 2016 19:05, "Lars-Peter Clausen" <lars@metafoo.de> wrote:
>  >
>  > On 11/27/2016 11:51 AM, Jonathan Cameron wrote:
>  > > On 26/11/16 03:47, Aniroop Mathur wrote:
>  > >> msleep(1~20) may not do what the caller intends, and will often sleep longer.
>  > >> (~20 ms actual sleep for any value given in the 1~20ms range)
>  > >> This is not the desired behaviour for many cases like device resume time,
>  > >> device suspend time, device enable time, data reading time, etc.
>  > >> Thus, change msleep to usleep_range for precise wakeups.
>  > >>
>  > >> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
>  > > As these need individual review by the various original authors and driver maintainers to
>  > > establish the intent of the sleep, it would have been better to have done a series of
>  > > patches (one per driver) with the relevant maintainers cc'd on the ones that they care about.
>  > >
>  > > Most of these are ADI parts looked after by Lars though so perhaps wait for his comments
>  > > before respining.
>  >
>  > I agree with what Jonathan said. For most of these extending the maximum
>  > sleep time a bit further is ok.
>  >
>
> Well, its right that sleep a bit further is okay but this patch is not trying
> to solve any major bug. This patch is only trying to make the wake up more
> precise than before. So like with msleep(1), process could sleep for 20 ms
> so this patch only making the making the process to sleep for 1 ms as
> mentioned in the parameter. So I think, changing to usleep_range is only
> advantageous and does not cause any harm here.
> We have also applied the same change in enable/disable/suspend/resume
> functions in accel, gyro, mag, etc drivers and found decent results like the
> first sesor data is generated much faster than before so response time
> increased. This is critical as  sensor can run at a rate of 200Hz / 5ms or
> even more now a days in new devices. We even applied in probe as doing the
> same in many drivers contribute to a little saving overall in kernel boot up.
> Also, it is recommended and mentioned in kernel documentation to use
> usleep_range for 1-10 ms.
> Sources -
> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
> https://lkml.org/lkml/2007/8/3/250
>


Hello Mr. Jonathan / Mr. Lars / All,


Would you kindly update further about this change?


> Thanks.
>
> BR,
> Aniroop Mathur

Re: [PATCH] IIO: Change msleep to usleep_range for small msecs

[Jonathan Cameron]

On 02/12/16 19:07, Aniroop Mathur wrote:
> On Wed, Nov 30, 2016 at 8:13 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
>> On 30 Nov 2016 19:05, "Lars-Peter Clausen" <lars@metafoo.de> wrote:
>>  >
>>  > On 11/27/2016 11:51 AM, Jonathan Cameron wrote:
>>  > > On 26/11/16 03:47, Aniroop Mathur wrote:
>>  > >> msleep(1~20) may not do what the caller intends, and will often sleep longer.
>>  > >> (~20 ms actual sleep for any value given in the 1~20ms range)
>>  > >> This is not the desired behaviour for many cases like device resume time,
>>  > >> device suspend time, device enable time, data reading time, etc.
>>  > >> Thus, change msleep to usleep_range for precise wakeups.
>>  > >>
>>  > >> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
>>  > > As these need individual review by the various original authors and driver maintainers to
>>  > > establish the intent of the sleep, it would have been better to have done a series of
>>  > > patches (one per driver) with the relevant maintainers cc'd on the ones that they care about.
>>  > >
>>  > > Most of these are ADI parts looked after by Lars though so perhaps wait for his comments
>>  > > before respining.
>>  >
>>  > I agree with what Jonathan said. For most of these extending the maximum
>>  > sleep time a bit further is ok.
>>  >
>>
>> Well, its right that sleep a bit further is okay but this patch is not trying
>> to solve any major bug. This patch is only trying to make the wake up more
>> precise than before. So like with msleep(1), process could sleep for 20 ms
>> so this patch only making the making the process to sleep for 1 ms as
>> mentioned in the parameter. So I think, changing to usleep_range is only
>> advantageous and does not cause any harm here.
>> We have also applied the same change in enable/disable/suspend/resume
>> functions in accel, gyro, mag, etc drivers and found decent results like the
>> first sesor data is generated much faster than before so response time
>> increased. This is critical as  sensor can run at a rate of 200Hz / 5ms or
>> even more now a days in new devices. We even applied in probe as doing the
>> same in many drivers contribute to a little saving overall in kernel boot up.
>> Also, it is recommended and mentioned in kernel documentation to use
>> usleep_range for 1-10 ms.
>> Sources -
>> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
>> https://lkml.org/lkml/2007/8/3/250
>>
> 
> 
> Hello Mr. Jonathan / Mr. Lars / All,
> 
> 
> Would you kindly update further about this change?
Hi Aniroop,

Quite a few of us only manage to get to kernel patches once or twice a week
(in my case only on weekends most weeks).

Anyhow, I've applied this patch as is.  I don't necessarily think the change
is that important in the probe paths, but as you said it does little harm.

So what the heck ;)

Applied to the togreg branch of iio.git which I'll push out as testing
once I have a net connection (currently on a train).

Thanks,

Jonathan
> 
> 
>> Thanks.
>>
>> BR,
>> Aniroop Mathur

Re: [PATCH] IIO: Change msleep to usleep_range for small msecs

[Aniroop Mathur]

On Sat, Dec 3, 2016 at 2:30 PM, Jonathan Cameron <jic23@kernel.org> wrote:
> On 02/12/16 19:07, Aniroop Mathur wrote:
>> On Wed, Nov 30, 2016 at 8:13 PM, Aniroop Mathur <a.mathur@samsung.com> wrote:
>>> On 30 Nov 2016 19:05, "Lars-Peter Clausen" <lars@metafoo.de> wrote:
>>>  >
>>>  > On 11/27/2016 11:51 AM, Jonathan Cameron wrote:
>>>  > > On 26/11/16 03:47, Aniroop Mathur wrote:
>>>  > >> msleep(1~20) may not do what the caller intends, and will often sleep longer.
>>>  > >> (~20 ms actual sleep for any value given in the 1~20ms range)
>>>  > >> This is not the desired behaviour for many cases like device resume time,
>>>  > >> device suspend time, device enable time, data reading time, etc.
>>>  > >> Thus, change msleep to usleep_range for precise wakeups.
>>>  > >>
>>>  > >> Signed-off-by: Aniroop Mathur <a.mathur@samsung.com>
>>>  > > As these need individual review by the various original authors and driver maintainers to
>>>  > > establish the intent of the sleep, it would have been better to have done a series of
>>>  > > patches (one per driver) with the relevant maintainers cc'd on the ones that they care about.
>>>  > >
>>>  > > Most of these are ADI parts looked after by Lars though so perhaps wait for his comments
>>>  > > before respining.
>>>  >
>>>  > I agree with what Jonathan said. For most of these extending the maximum
>>>  > sleep time a bit further is ok.
>>>  >
>>>
>>> Well, its right that sleep a bit further is okay but this patch is not trying
>>> to solve any major bug. This patch is only trying to make the wake up more
>>> precise than before. So like with msleep(1), process could sleep for 20 ms
>>> so this patch only making the making the process to sleep for 1 ms as
>>> mentioned in the parameter. So I think, changing to usleep_range is only
>>> advantageous and does not cause any harm here.
>>> We have also applied the same change in enable/disable/suspend/resume
>>> functions in accel, gyro, mag, etc drivers and found decent results like the
>>> first sesor data is generated much faster than before so response time
>>> increased. This is critical as  sensor can run at a rate of 200Hz / 5ms or
>>> even more now a days in new devices. We even applied in probe as doing the
>>> same in many drivers contribute to a little saving overall in kernel boot up.
>>> Also, it is recommended and mentioned in kernel documentation to use
>>> usleep_range for 1-10 ms.
>>> Sources -
>>> https://www.kernel.org/doc/Documentation/timers/timers-howto.txt
>>> https://lkml.org/lkml/2007/8/3/250
>>>
>>
>>
>> Hello Mr. Jonathan / Mr. Lars / All,
>>
>>
>> Would you kindly update further about this change?
> Hi Aniroop,
>
> Quite a few of us only manage to get to kernel patches once or twice a week
> (in my case only on weekends most weeks).
>
> Anyhow, I've applied this patch as is.  I don't necessarily think the change
> is that important in the probe paths, but as you said it does little harm.
>
> So what the heck ;)
>
> Applied to the togreg branch of iio.git which I'll push out as testing
> once I have a net connection (currently on a train).
>

This sounds great !! Thanks a lot :)

BR,
Aniroop Mathur

> Thanks,
>
> Jonathan
>>
>>
>>> Thanks.
>>>
>>> BR,
>>> Aniroop Mathur
>