Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1530592 > unrolled thread
| Started by | Aniroop Mathur <a.mathur@samsung.com> |
|---|---|
| First post | 2016-11-26 04:40 +0100 |
| Last post | 2016-12-03 16:50 +0100 |
| Articles | 9 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH] IIO: Change msleep to usleep_range for small msecs Aniroop Mathur <a.mathur@samsung.com> - 2016-11-26 04:40 +0100
Re: [PATCH] IIO: Change msleep to usleep_range for small msecs Jonathan Cameron <jic23@kernel.org> - 2016-11-27 12:00 +0100
Re: [PATCH] IIO: Change msleep to usleep_range for small msecs Linus Walleij <linus.walleij@linaro.org> - 2016-11-30 13:20 +0100
Re: [PATCH] IIO: Change msleep to usleep_range for small msecs Aniroop Mathur <aniroop.mathur@gmail.com> - 2016-12-02 20:10 +0100
Re: [PATCH] IIO: Change msleep to usleep_range for small msecs Lars-Peter Clausen <lars@metafoo.de> - 2016-11-30 14:40 +0100
RE: Re: [PATCH] IIO: Change msleep to usleep_range for small msecs Aniroop Mathur <a.mathur@samsung.com> - 2016-11-30 15:50 +0100
Re: Re: [PATCH] IIO: Change msleep to usleep_range for small msecs Aniroop Mathur <aniroop.mathur@gmail.com> - 2016-12-02 20:10 +0100
Re: [PATCH] IIO: Change msleep to usleep_range for small msecs Jonathan Cameron <jic23@kernel.org> - 2016-12-03 15:40 +0100
Re: [PATCH] IIO: Change msleep to usleep_range for small msecs Aniroop Mathur <aniroop.mathur@gmail.com> - 2016-12-03 16:50 +0100
| From | Aniroop Mathur <a.mathur@samsung.com> |
|---|---|
| Date | 2016-11-26 04:40 +0100 |
| Subject | [PATCH] IIO: Change msleep to usleep_range for small msecs |
| Message-ID | <sHC0h-17I-3@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Jonathan Cameron <jic23@kernel.org> |
|---|---|
| Date | 2016-11-27 12:00 +0100 |
| Message-ID | <sI5lD-3eJ-5@gated-at.bofh.it> |
| In reply to | #1530592 |
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)
>
[toc] | [prev] | [next] | [standalone]
| From | Linus Walleij <linus.walleij@linaro.org> |
|---|---|
| Date | 2016-11-30 13:20 +0100 |
| Message-ID | <sJc1H-61D-13@gated-at.bofh.it> |
| In reply to | #1530856 |
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
[toc] | [prev] | [next] | [standalone]
| From | Aniroop Mathur <aniroop.mathur@gmail.com> |
|---|---|
| Date | 2016-12-02 20:10 +0100 |
| Message-ID | <sK1nz-7Px-17@gated-at.bofh.it> |
| In reply to | #1533262 |
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
[toc] | [prev] | [next] | [standalone]
| From | Lars-Peter Clausen <lars@metafoo.de> |
|---|---|
| Date | 2016-11-30 14:40 +0100 |
| Message-ID | <sJdh7-6FX-1@gated-at.bofh.it> |
| In reply to | #1530856 |
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.
[toc] | [prev] | [next] | [standalone]
| From | Aniroop Mathur <a.mathur@samsung.com> |
|---|---|
| Date | 2016-11-30 15:50 +0100 |
| Message-ID | <sJemS-7mo-13@gated-at.bofh.it> |
| In reply to | #1530856 |
[Multipart message — attachments visible in raw view] — view raw
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
[toc] | [prev] | [next] | [standalone]
| From | Aniroop Mathur <aniroop.mathur@gmail.com> |
|---|---|
| Date | 2016-12-02 20:10 +0100 |
| Message-ID | <sK1nA-7Px-29@gated-at.bofh.it> |
| In reply to | #1533358 |
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
[toc] | [prev] | [next] | [standalone]
| From | Jonathan Cameron <jic23@kernel.org> |
|---|---|
| Date | 2016-12-03 15:40 +0100 |
| Message-ID | <sKjDQ-2xI-17@gated-at.bofh.it> |
| In reply to | #1535135 |
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
[toc] | [prev] | [next] | [standalone]
| From | Aniroop Mathur <aniroop.mathur@gmail.com> |
|---|---|
| Date | 2016-12-03 16:50 +0100 |
| Message-ID | <sKkJz-39U-1@gated-at.bofh.it> |
| In reply to | #1535415 |
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 >
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web