| Message ID | 20200813075125.4949-4-cmo@melexis.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iio: temperature: mlx90632: Add extended calibration calculations | expand |
On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <cmo@melexis.com> wrote: > > Reduce number of lines and improve readability to convert polling while > loops to do-while. The iopoll.h interface was not used, because we > require more than 20ms timeout, because time for sensor to perform a > measurement is around 10ms and it needs to perform measurements for each > channel (which currently is 3). I don't see how it prevents using iopoll.h. It uses usleep_range() under the hood in the same way you did here, but open coded. ... > - while (tries-- > 0) { > + do { > ret = regmap_read(data->regmap, MLX90632_REG_STATUS, > ®_status); > if (ret < 0) > return ret; > - if (reg_status & MLX90632_STAT_DATA_RDY) > - break; > usleep_range(10000, 11000); > - } > + } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--); > > if (tries < 0) { > dev_err(&data->client->dev, "data not ready");
On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <cmo@melexis.com> wrote: > > > > Reduce number of lines and improve readability to convert polling while > > loops to do-while. The iopoll.h interface was not used, because we > > require more than 20ms timeout, because time for sensor to perform a > > measurement is around 10ms and it needs to perform measurements for each > > channel (which currently is 3). > > I don't see how it prevents using iopoll.h. It uses usleep_range() > under the hood in the same way you did here, but open coded. > One loop is indeed 10ms and that is not the problem, the problem is that timeout is at least 3 calls of this data ready (3 channels), so that is at minimum 30ms of timeout, or it could even be 4 in worse case scenario and that is outside of the range for usleep to measure. So in case of the other loop, where we wait 200ms for channel refresh it is also out of scope. Timeout should be in number of tries or in msleep range if you ask me. > ... > > > - while (tries-- > 0) { > > + do { > > ret = regmap_read(data->regmap, MLX90632_REG_STATUS, > > ®_status); > > if (ret < 0) > > return ret; > > - if (reg_status & MLX90632_STAT_DATA_RDY) > > - break; > > usleep_range(10000, 11000); > > - } > > + } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--); > > > > if (tries < 0) { > > dev_err(&data->client->dev, "data not ready"); > > -- > With Best Regards, > Andy Shevchenko
On Thu, Aug 13, 2020 at 2:14 PM Crt Mori <cmo@melexis.com> wrote: > > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <cmo@melexis.com> wrote: > > > > > > Reduce number of lines and improve readability to convert polling while > > > loops to do-while. The iopoll.h interface was not used, because we > > > require more than 20ms timeout, because time for sensor to perform a > > > measurement is around 10ms and it needs to perform measurements for each > > > channel (which currently is 3). > > > > I don't see how it prevents using iopoll.h. It uses usleep_range() > > under the hood in the same way you did here, but open coded. > > > > One loop is indeed 10ms and that is not the problem, the problem is > that timeout is at least 3 calls of this data ready (3 channels), so > that is at minimum 30ms of timeout, or it could even be 4 in worse > case scenario and that is outside of the range for usleep to measure. > So in case of the other loop, where we wait 200ms for channel refresh > it is also out of scope. Timeout should be in number of tries or in > msleep range if you ask me. I still didn't buy it. You have in both cases usleep_range(). Why in your case it's okay and in regmap_read_poll_timeout() is not? > > ... > > > > > - while (tries-- > 0) { > > > + do { > > > ret = regmap_read(data->regmap, MLX90632_REG_STATUS, > > > ®_status); > > > if (ret < 0) > > > return ret; > > > - if (reg_status & MLX90632_STAT_DATA_RDY) > > > - break; > > > usleep_range(10000, 11000); > > > - } > > > + } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--); > > > > > > if (tries < 0) { > > > dev_err(&data->client->dev, "data not ready");
On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Aug 13, 2020 at 2:14 PM Crt Mori <cmo@melexis.com> wrote: > > > > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <cmo@melexis.com> wrote: > > > > > > > > Reduce number of lines and improve readability to convert polling while > > > > loops to do-while. The iopoll.h interface was not used, because we > > > > require more than 20ms timeout, because time for sensor to perform a > > > > measurement is around 10ms and it needs to perform measurements for each > > > > channel (which currently is 3). > > > > > > I don't see how it prevents using iopoll.h. It uses usleep_range() > > > under the hood in the same way you did here, but open coded. > > > > > > > One loop is indeed 10ms and that is not the problem, the problem is > > that timeout is at least 3 calls of this data ready (3 channels), so > > that is at minimum 30ms of timeout, or it could even be 4 in worse > > case scenario and that is outside of the range for usleep to measure. > > So in case of the other loop, where we wait 200ms for channel refresh > > it is also out of scope. Timeout should be in number of tries or in > > msleep range if you ask me. > > I still didn't buy it. You have in both cases usleep_range(). Why in > your case it's okay and in regmap_read_poll_timeout() is not? > I tried and it did not work, so then I read the manual. Looking into * regmap_read_poll_timeout_atomic - Poll until a condition is met or a timeout occurs ... * @delay_us: Time to udelay between reads in us (0 tight-loops). * Should be less than ~10us since udelay is used * (see Documentation/timers/timers-howto.rst). * @timeout_us: Timeout in us, 0 means never timeout So I went to read Documentation/timers/timers-howto.rst SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms): * Use usleep_range - Why not msleep for (1ms - 20ms)? Explained originally here: http://lkml.org/lkml/2007/8/3/250 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). In many cases this is not the desired behavior. Since I am above the 20ms range, it is too much for usleep_range and that proved to be a case as I got -ETIMEOUT and the desired channels were not read. > > > ... > > > > > > > - while (tries-- > 0) { > > > > + do { > > > > ret = regmap_read(data->regmap, MLX90632_REG_STATUS, > > > > ®_status); > > > > if (ret < 0) > > > > return ret; > > > > - if (reg_status & MLX90632_STAT_DATA_RDY) > > > > - break; > > > > usleep_range(10000, 11000); > > > > - } > > > > + } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--); > > > > > > > > if (tries < 0) { > > > > dev_err(&data->client->dev, "data not ready"); > > -- > With Best Regards, > Andy Shevchenko
On Thu, Aug 13, 2020 at 4:04 PM Crt Mori <cmo@melexis.com> wrote: > On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Aug 13, 2020 at 2:14 PM Crt Mori <cmo@melexis.com> wrote: > > > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <cmo@melexis.com> wrote: ... > > > > I don't see how it prevents using iopoll.h. It uses usleep_range() > > > > under the hood in the same way you did here, but open coded. > > > > > > > > > > One loop is indeed 10ms and that is not the problem, the problem is > > > that timeout is at least 3 calls of this data ready (3 channels), so > > > that is at minimum 30ms of timeout, or it could even be 4 in worse > > > case scenario and that is outside of the range for usleep to measure. > > > So in case of the other loop, where we wait 200ms for channel refresh > > > it is also out of scope. Timeout should be in number of tries or in > > > msleep range if you ask me. > > > > I still didn't buy it. You have in both cases usleep_range(). Why in > > your case it's okay and in regmap_read_poll_timeout() is not? > > > > I tried and it did not work, so then I read the manual. Looking into > > * regmap_read_poll_timeout_atomic - Poll until a condition is met or a > timeout occurs Why _atomic?! > ... > * @delay_us: Time to udelay between reads in us (0 tight-loops). > * Should be less than ~10us since udelay is used > * (see Documentation/timers/timers-howto.rst). > * @timeout_us: Timeout in us, 0 means never timeout > > > So I went to read Documentation/timers/timers-howto.rst > > SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms): > * Use usleep_range > > - Why not msleep for (1ms - 20ms)? > Explained originally here: > http://lkml.org/lkml/2007/8/3/250 > > 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). In many cases this > is not the desired behavior. > > Since I am above the 20ms range, it is too much for usleep_range and > that proved to be a case as I got -ETIMEOUT and the desired channels > were not read. > > > > ... > > > > > > > > > - while (tries-- > 0) { > > > > > + do { > > > > > ret = regmap_read(data->regmap, MLX90632_REG_STATUS, > > > > > ®_status); > > > > > if (ret < 0) > > > > > return ret; > > > > > - if (reg_status & MLX90632_STAT_DATA_RDY) > > > > > - break; > > > > > usleep_range(10000, 11000); You use here usleep_range(). The same is used for regmap_read_poll_timeout(). What's the difference? Since it uses 1/4 of the range you probably need to update tries and timeout_us to make it work. > > > > > - } > > > > > + } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--); > > > > > > > > > > if (tries < 0) { > > > > > dev_err(&data->client->dev, "data not ready");
On Thu, 13 Aug 2020 at 21:41, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Aug 13, 2020 at 4:04 PM Crt Mori <cmo@melexis.com> wrote: > > On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Thu, Aug 13, 2020 at 2:14 PM Crt Mori <cmo@melexis.com> wrote: > > > > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <cmo@melexis.com> wrote: > > ... > > > > > > I don't see how it prevents using iopoll.h. It uses usleep_range() > > > > > under the hood in the same way you did here, but open coded. > > > > > > > > > > > > > One loop is indeed 10ms and that is not the problem, the problem is > > > > that timeout is at least 3 calls of this data ready (3 channels), so > > > > that is at minimum 30ms of timeout, or it could even be 4 in worse > > > > case scenario and that is outside of the range for usleep to measure. > > > > So in case of the other loop, where we wait 200ms for channel refresh > > > > it is also out of scope. Timeout should be in number of tries or in > > > > msleep range if you ask me. > > > > > > I still didn't buy it. You have in both cases usleep_range(). Why in > > > your case it's okay and in regmap_read_poll_timeout() is not? > > > > > > > I tried and it did not work, so then I read the manual. Looking into > > > > * regmap_read_poll_timeout_atomic - Poll until a condition is met or a > > timeout occurs > > Why _atomic?! I just pasted something, it is the same as for non _atomic > > > ... > > * @delay_us: Time to udelay between reads in us (0 tight-loops). > > * Should be less than ~10us since udelay is used > > * (see Documentation/timers/timers-howto.rst). > > * @timeout_us: Timeout in us, 0 means never timeout > > > > > > So I went to read Documentation/timers/timers-howto.rst > > > > SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms): > > * Use usleep_range > > > > - Why not msleep for (1ms - 20ms)? > > Explained originally here: > > http://lkml.org/lkml/2007/8/3/250 > > > > 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). In many cases this > > is not the desired behavior. > > > > Since I am above the 20ms range, it is too much for usleep_range and > > that proved to be a case as I got -ETIMEOUT and the desired channels > > were not read. > > > > > ... > > > > > > > > > > > - while (tries-- > 0) { > > > > > > + do { > > > > > > ret = regmap_read(data->regmap, MLX90632_REG_STATUS, > > > > > > ®_status); > > > > > > if (ret < 0) > > > > > > return ret; > > > > > > - if (reg_status & MLX90632_STAT_DATA_RDY) > > > > > > - break; > > > > > > usleep_range(10000, 11000); > > You use here usleep_range(). The same is used for > regmap_read_poll_timeout(). What's the difference? > > Since it uses 1/4 of the range you probably need to update tries and > timeout_us to make it work. > Timeout_us here needs to be in one case 100 * 10ms (maybe not realistic as we could live with number of around 40 * 10ms), but this is a lot more than proposed range of usleep which Is up to 20ms. Even in best case this timeout should be 40 ms to give enough time to measure 2 channels for sure. So with the current timeout_us requirement we are outside of the range of the udelay timer and that is why I would need a macro with number of tries, not with the timeout value (or timeout value of ms). > > > > > > - } > > > > > > + } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--); > > > > > > > > > > > > if (tries < 0) { > > > > > > dev_err(&data->client->dev, "data not ready"); > > -- > With Best Regards, > Andy Shevchenko
On Fri, Aug 14, 2020 at 10:33 AM Crt Mori <cmo@melexis.com> wrote: > On Thu, 13 Aug 2020 at 21:41, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Thu, Aug 13, 2020 at 4:04 PM Crt Mori <cmo@melexis.com> wrote: > > > On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Thu, Aug 13, 2020 at 2:14 PM Crt Mori <cmo@melexis.com> wrote: > > > > > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <cmo@melexis.com> wrote: > > > > ... > > > > > > > > I don't see how it prevents using iopoll.h. It uses usleep_range() > > > > > > under the hood in the same way you did here, but open coded. > > > > > > > > > > > > > > > > One loop is indeed 10ms and that is not the problem, the problem is > > > > > that timeout is at least 3 calls of this data ready (3 channels), so > > > > > that is at minimum 30ms of timeout, or it could even be 4 in worse > > > > > case scenario and that is outside of the range for usleep to measure. > > > > > So in case of the other loop, where we wait 200ms for channel refresh > > > > > it is also out of scope. Timeout should be in number of tries or in > > > > > msleep range if you ask me. > > > > > > > > I still didn't buy it. You have in both cases usleep_range(). Why in > > > > your case it's okay and in regmap_read_poll_timeout() is not? > > > > > > > > > > I tried and it did not work, so then I read the manual. Looking into > > > > > > * regmap_read_poll_timeout_atomic - Poll until a condition is met or a > > > timeout occurs > > > > Why _atomic?! > > I just pasted something, it is the same as for non _atomic OK. ... > > > * @delay_us: Time to udelay between reads in us (0 tight-loops). > > > * Should be less than ~10us since udelay is used > > > * (see Documentation/timers/timers-howto.rst). > > > * @timeout_us: Timeout in us, 0 means never timeout ... > > > > > > > usleep_range(10000, 11000); > > > > You use here usleep_range(). The same is used for > > regmap_read_poll_timeout(). What's the difference? > > > > Since it uses 1/4 of the range you probably need to update tries and > > timeout_us to make it work. > > > > Timeout_us here needs to be in one case 100 * 10ms (maybe not > realistic as we could live with number of around 40 * 10ms), but this > is a lot more than proposed range of usleep which Is up to 20ms. Even > in best case this timeout should be 40 ms to give enough time to > measure 2 channels for sure. So with the current timeout_us > requirement we are outside of the range of the udelay timer and that > is why I would need a macro with number of tries, not with the timeout > value (or timeout value of ms). I do not understand. The regmap_read_poll_timeout() is a macro which unrolls in the very similar loop you have now in the code. What prevents it from using it? I think there is a big misunderstanding about the parameters of that macro. delay_us (must be small enough), timeout_us can be any long.
On Fri, 14 Aug 2020 at 11:32, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Aug 14, 2020 at 10:33 AM Crt Mori <cmo@melexis.com> wrote: > > On Thu, 13 Aug 2020 at 21:41, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > On Thu, Aug 13, 2020 at 4:04 PM Crt Mori <cmo@melexis.com> wrote: > > > > On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > On Thu, Aug 13, 2020 at 2:14 PM Crt Mori <cmo@melexis.com> wrote: > > > > > > On Thu, 13 Aug 2020 at 13:03, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > > > > On Thu, Aug 13, 2020 at 10:53 AM Crt Mori <cmo@melexis.com> wrote: > > > > > > ... > > > > > > > > > > I don't see how it prevents using iopoll.h. It uses usleep_range() > > > > > > > under the hood in the same way you did here, but open coded. > > > > > > > > > > > > > > > > > > > One loop is indeed 10ms and that is not the problem, the problem is > > > > > > that timeout is at least 3 calls of this data ready (3 channels), so > > > > > > that is at minimum 30ms of timeout, or it could even be 4 in worse > > > > > > case scenario and that is outside of the range for usleep to measure. > > > > > > So in case of the other loop, where we wait 200ms for channel refresh > > > > > > it is also out of scope. Timeout should be in number of tries or in > > > > > > msleep range if you ask me. > > > > > > > > > > I still didn't buy it. You have in both cases usleep_range(). Why in > > > > > your case it's okay and in regmap_read_poll_timeout() is not? > > > > > > > > > > > > > I tried and it did not work, so then I read the manual. Looking into > > > > > > > > * regmap_read_poll_timeout_atomic - Poll until a condition is met or a > > > > timeout occurs > > > > > > Why _atomic?! > > > > I just pasted something, it is the same as for non _atomic > > OK. > > ... > > > > > * @delay_us: Time to udelay between reads in us (0 tight-loops). > > > > * Should be less than ~10us since udelay is used > > > > * (see Documentation/timers/timers-howto.rst). > > > > * @timeout_us: Timeout in us, 0 means never timeout > > ... > > > > > > > > > usleep_range(10000, 11000); > > > > > > You use here usleep_range(). The same is used for > > > regmap_read_poll_timeout(). What's the difference? > > > > > > Since it uses 1/4 of the range you probably need to update tries and > > > timeout_us to make it work. > > > > > > > Timeout_us here needs to be in one case 100 * 10ms (maybe not > > realistic as we could live with number of around 40 * 10ms), but this > > is a lot more than proposed range of usleep which Is up to 20ms. Even > > in best case this timeout should be 40 ms to give enough time to > > measure 2 channels for sure. So with the current timeout_us > > requirement we are outside of the range of the udelay timer and that > > is why I would need a macro with number of tries, not with the timeout > > value (or timeout value of ms). > > I do not understand. The regmap_read_poll_timeout() is a macro which > unrolls in the very similar loop you have now in the code. > What prevents it from using it? > > I think there is a big misunderstanding about the parameters of that macro. > delay_us (must be small enough), timeout_us can be any long. > I tested on Beaglebone with the 100 * 10000 as timeout_us and I always got the -ETIMEDOUT error. I also tested in the other case where delay_us is 250000 and then timeout_us would be 4*250000 and I have also received -ETIMEDOUT as a response. I can prepare a patch with the iopoll.h API and maybe you will spot the mistake, as after rechecking timeout_us is indeed 64bit and is only used in the time comparison operations and not with timers. > -- > With Best Regards, > Andy Shevchenko
On Fri, Aug 14, 2020 at 12:42 PM Crt Mori <cmo@melexis.com> wrote: > On Fri, 14 Aug 2020 at 11:32, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > On Fri, Aug 14, 2020 at 10:33 AM Crt Mori <cmo@melexis.com> wrote: > > > On Thu, 13 Aug 2020 at 21:41, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > > > > On Thu, Aug 13, 2020 at 4:04 PM Crt Mori <cmo@melexis.com> wrote: > > > > > On Thu, 13 Aug 2020 at 13:24, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: ... > > > > > usleep_range(10000, 11000); Above for 4 attempts is up to 44000us. With iopoll.h case we may set delay_us = 18000 (the result range will be 4500, 18000) timeout_us = 72000 (this will give up to 16 attempts to read) > I tested on Beaglebone with the 100 * 10000 as timeout_us and I always > got the -ETIMEDOUT error. I also tested in the other case where > delay_us is 250000 and then timeout_us would be 4*250000 and I have > also received -ETIMEDOUT as a response. I don't see how delay_us should be bigger than 20ms (20000). > I can prepare a patch with the iopoll.h API and maybe you will spot > the mistake, as after rechecking timeout_us is indeed 64bit and is > only used in the time comparison operations and not with timers. Perhaps above will clarify.
diff --git a/drivers/iio/temperature/mlx90632.c b/drivers/iio/temperature/mlx90632.c index ce75f5a3486b..d87c792b7e72 100644 --- a/drivers/iio/temperature/mlx90632.c +++ b/drivers/iio/temperature/mlx90632.c @@ -188,15 +188,13 @@ static int mlx90632_perform_measurement(struct mlx90632_data *data) if (ret < 0) return ret; - while (tries-- > 0) { + do { ret = regmap_read(data->regmap, MLX90632_REG_STATUS, ®_status); if (ret < 0) return ret; - if (reg_status & MLX90632_STAT_DATA_RDY) - break; usleep_range(10000, 11000); - } + } while (!(reg_status & MLX90632_STAT_DATA_RDY) && tries--); if (tries < 0) { dev_err(&data->client->dev, "data not ready");
Reduce number of lines and improve readability to convert polling while loops to do-while. The iopoll.h interface was not used, because we require more than 20ms timeout, because time for sensor to perform a measurement is around 10ms and it needs to perform measurements for each channel (which currently is 3). Signed-off-by: Crt Mori <cmo@melexis.com> --- drivers/iio/temperature/mlx90632.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-)