On Wed, Aug 11, 2021 at 4:32 PM Nuno Sá <nuno.sa@xxxxxxxxxx> wrote:
Thanks for an update, my comments below.
Depending on Jonathan's wishes it may or may not require a v3.
If you address the minor issues I commented on, take mine
Reviewed-by: Andy Shevchenko <andy.shevchenko@xxxxxxxxx>
> There is no reason to assume that the irq rising edge (indicating that
> the device start up phase is done) will happen after we request the irq.
> If the device is already up by the time we request it, the call to
> 'wait_for_completion_timeout()' will timeout and we will fail the device
> probe even though there's nothing wrong.
> This patch fixes it by just polling the status register until we get the
This patch fixes --> Fix
(see Submitting Patches for details of this requirement)
> indication that the device is up and running. As a side effect of this
> fix, requesting the irq is also moved to after the setup function.
In entire message irq --> IRQ
> Fixes: f110f3188e563 ("iio: temperature: Add support for LTC2983")
> Reported-by: Drew Fustini <drew@xxxxxxxx>
> Reviewed-by: Drew Fustini <drew@xxxxxxxx>
> Tested-by: Drew Fustini <drew@xxxxxxxx>
You may save a line by unifying Reported and Tested with Reported-and-tested.
"Reviewed" is usually a different story.
> Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> ---
> drivers/iio/temperature/ltc2983.c | 30 ++++++++++++++----------------
> 1 file changed, 14 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c
> index 3b5ba26d7d86..657eb8cb4be4 100644
> --- a/drivers/iio/temperature/ltc2983.c
> +++ b/drivers/iio/temperature/ltc2983.c
> @@ -89,6 +89,8 @@
>
> #define LTC2983_STATUS_START_MASK BIT(7)
> #define LTC2983_STATUS_START(x) FIELD_PREP(LTC2983_STATUS_START_MASK, x)
> +#define LTC2983_STATUS_UP_MASK GENMASK(7, 6)
> +#define LTC2983_STATUS_UP(reg) FIELD_GET(LTC2983_STATUS_UP_MASK, reg)
>
> #define LTC2983_STATUS_CHAN_SEL_MASK GENMASK(4, 0)
> #define LTC2983_STATUS_CHAN_SEL(x) \
> @@ -1362,17 +1364,16 @@ static int ltc2983_parse_dt(struct ltc2983_data *st)
>
> static int ltc2983_setup(struct ltc2983_data *st, bool assign_iio)
> {
> - u32 iio_chan_t = 0, iio_chan_v = 0, chan, iio_idx = 0;
> + u32 iio_chan_t = 0, iio_chan_v = 0, chan, iio_idx = 0, status = 0;
No need to assign 0 at least to the status.
> int ret;
> - unsigned long time;
> -
> - /* make sure the device is up */
> - time = wait_for_completion_timeout(&st->completion,
> - msecs_to_jiffies(250));
>
> - if (!time) {
> + /* make sure the device is up: start bit (7) is 0 and done bit (6) is 1 */
> + ret = regmap_read_poll_timeout(st->regmap, LTC2983_STATUS_REG, status,
> + LTC2983_STATUS_UP(status) == 1, 25000,
> + 25000 * 10);
> + if (ret) {
> dev_err(&st->spi->dev, "Device startup timed out\n");
> - return -ETIMEDOUT;
> + return ret;
> }
>
> st->iio_chan = devm_kzalloc(&st->spi->dev,
> @@ -1492,10 +1493,11 @@ static int ltc2983_probe(struct spi_device *spi)
> ret = ltc2983_parse_dt(st);
> if (ret)
> return ret;
> - /*
> - * let's request the irq now so it is used to sync the device
> - * startup in ltc2983_setup()
> - */
> +
> + ret = ltc2983_setup(st, true);
> + if (ret)
> + return ret;
> +
> ret = devm_request_irq(&spi->dev, spi->irq, ltc2983_irq_handler,
> IRQF_TRIGGER_RISING, name, st);
> if (ret) {
> @@ -1503,10 +1505,6 @@ static int ltc2983_probe(struct spi_device *spi)
> return ret;
> }
>
> - ret = ltc2983_setup(st, true);
> - if (ret)
> - return ret;
> -
> indio_dev->name = name;
> indio_dev->num_channels = st->iio_channels;
> indio_dev->channels = st->iio_chan;
> --
> 2.32.0
>
--
With Best Regards,
Andy Shevchenko
