Re: [PATCH 1/1] iio: ltc2983: fix device probe

[Drew Fustini <drew@pdp7.com>]

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@pdp7.com>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  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.

Reviewed-by: Drew Fustini <drew@pdp7.com>
Tested-by: Drew Fustini <drew@pdp7.com>

Thanks for fixing,
Drew