> -----Original Message-----
> From: Drew Fustini <drew@xxxxxxxx>
> Sent: Wednesday, August 11, 2021 12:03 AM
> To: Sa, Nuno <Nuno.Sa@xxxxxxxxxx>
> Cc: linux-iio@xxxxxxxxxxxxxxx; =Jonathan Cameron <jic23@xxxxxxxxxx>;
> Lars-Peter Clausen <lars@xxxxxxxxxx>
> Subject: Re: [PATCH 1/1] iio: ltc2983: fix device probe
>
> On Tue, Aug 10, 2021 at 04:56:53PM +0200, Nuno Sá wrote:
> > 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
> > 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.
> >
> > Fixes: f110f3188e563 ("iio: temperature: Add support for LTC2983")
> > Reported-by: Drew Fustini <drew@xxxxxxxx>
> > Signed-off-by: Nuno Sá <nuno.sa@xxxxxxxxxx>
> > ---
> > drivers/iio/temperature/ltc2983.c | 31 +++++++++++++++++++------
> ------
> > 1 file changed, 19 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/iio/temperature/ltc2983.c
> b/drivers/iio/temperature/ltc2983.c
> > index 3b5ba26d7d86..c6c4877bdcff 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,13 +1364,21 @@ 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;
> > int ret;
> > - unsigned long time;
> > + unsigned long time = 10;
> >
> > /* make sure the device is up */
> > - time = wait_for_completion_timeout(&st->completion,
> > - msecs_to_jiffies(250));
> > + do {
> > + ret = regmap_read(st->regmap,
> LTC2983_STATUS_REG, &status);
> > + if (ret)
> > + return ret;
> > + /* start bit (7) is 0 and done bit (6) is 1 */
> > + if (LTC2983_STATUS_UP(status) == 1)
> > + break;
> > +
> > + msleep(25);
> > + } while (--time);
> >
> > if (!time) {
> > dev_err(&st->spi->dev, "Device startup timed out\n");
> > @@ -1492,10 +1502,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 +1514,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
> >
>
> I have tested this on my custom Zynq-7000 board and the probe
> completes
> okay. The read of LTC2983_STATUS_REG returns 0x40 on the first
> attempt
> which is good. I am also able to read channels through sysfs. I have not
> tried suspend and resume as that is not configured on this system.
Hi,
Thanks for testing. During my tests I kind of emulated this by adding
a devm_action which would put the device to sleep on the unbind path.
Hence, unbinding + binding the device will cause a sleep + wake up on it.
In this case I saw ~75 ms for it to come up. I read some 0's, 0x80
(as specified in the datasheet) and lastly 0x40.
Jonathan, I noticed some odd indentation in the new defines. If there's
no other reason for a v2, it would be nice if you could fix this when applying :)
As a side note, I'm also aware that you prefer to see FIELD_{PREP|GET} applied
inline in the code but in this case I made it like this to be consistent with the
rest of the driver.
- Nuno Sá
