[PATCH v2 0/1] Fix ltc2983 probing

[PATCH v2 0/1] Fix ltc2983 probing

[Nuno Sá]

The issue was reported by Drew [1]. I tested this fix on a rpi and it
works. It would be nice to have a tested-by tag for this on your
platform Drew.

On top of the reported issue, the patch also fixes device binding and
unbinding since that would also fail (that also obviously failed on
rpi). The same goes for module unloading and loading.

[1]: https://marc.info/?l=linux-iio&m=162829198205250&w=2

Changes in v2:
 * make use of 'regmap_read_poll_timeout()'

Nuno Sá (1):
  iio: ltc2983: fix device probe

 drivers/iio/temperature/ltc2983.c | 30 ++++++++++++++----------------
 1 file changed, 14 insertions(+), 16 deletions(-)

-- 
2.32.0

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

[Nuno Sá]

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>
Reviewed-by: Drew Fustini <drew@pdp7.com>
Tested-by: Drew Fustini <drew@pdp7.com>
Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 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;
 	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

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

[Andy Shevchenko]

On Wed, Aug 11, 2021 at 4:32 PM Nuno Sá <nuno.sa@analog.com> 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@gmail.com>

> 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@pdp7.com>
> Reviewed-by: Drew Fustini <drew@pdp7.com>
> Tested-by: Drew Fustini <drew@pdp7.com>

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@analog.com>
> ---
>  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

RE: [PATCH v2 1/1] iio: ltc2983: fix device probe

[Sa, Nuno]

> -----Original Message-----
> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Wednesday, August 11, 2021 6:15 PM
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; Drew
> Fustini <drew@pdp7.com>
> Subject: Re: [PATCH v2 1/1] iio: ltc2983: fix device probe
> 
> [External]
> 
> On Wed, Aug 11, 2021 at 4:32 PM Nuno Sá <nuno.sa@analog.com>
> 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@gmail.com>
> 

Thanks for reviewing... Yeah, I will wait for Jonathan's feedback before
sending a v3. It might be that he can apply your inputs when applying
the patch.

Thanks!
- Nuno Sá

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

[Jonathan Cameron]

On Thu, 12 Aug 2021 06:54:13 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > -----Original Message-----
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Wednesday, August 11, 2021 6:15 PM
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; Drew
> > Fustini <drew@pdp7.com>
> > Subject: Re: [PATCH v2 1/1] iio: ltc2983: fix device probe
> > 
> > [External]
> > 
> > On Wed, Aug 11, 2021 at 4:32 PM Nuno Sá <nuno.sa@analog.com>
> > 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@gmail.com>
> >   
> 
> Thanks for reviewing... Yeah, I will wait for Jonathan's feedback before
> sending a v3. It might be that he can apply your inputs when applying
> the patch.

Patch looks sensible. I can make the tweaks whilst applying when I
happen to be on the right computer.

Having glanced at the datasheet, I wonder if you ever had the
reset pin wired up (and perhaps decided to drop that
complexity later)?  The driver rather oddly
include of_gpio.h and has no gpio accesses which makes me
wonder ;)

Jonathan

> 
> Thanks!
> - Nuno Sá

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

[Drew Fustini]

On Thu, Aug 12, 2021 at 07:19:19PM +0100, Jonathan Cameron wrote:
> On Thu, 12 Aug 2021 06:54:13 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Wednesday, August 11, 2021 6:15 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; Drew
> > > Fustini <drew@pdp7.com>
> > > Subject: Re: [PATCH v2 1/1] iio: ltc2983: fix device probe
> > > 
> > > [External]
> > > 
> > > On Wed, Aug 11, 2021 at 4:32 PM Nuno Sá <nuno.sa@analog.com>
> > > 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@gmail.com>
> > >   
> > 
> > Thanks for reviewing... Yeah, I will wait for Jonathan's feedback before
> > sending a v3. It might be that he can apply your inputs when applying
> > the patch.
> 
> Patch looks sensible. I can make the tweaks whilst applying when I
> happen to be on the right computer.
> 
> Having glanced at the datasheet, I wonder if you ever had the
> reset pin wired up (and perhaps decided to drop that
> complexity later)?  The driver rather oddly
> include of_gpio.h and has no gpio accesses which makes me
> wonder ;)

I'm sure Nuno will have something to say but I wanted to mention that I
was initially thinking about this.  The reset pin is connected to a GPIO
on the Zynq-7000 on the custom board I am using.  I added that gpio line
as an active low gpio hog under the gpio controller node in the DTS and
that worked ok.  Though I had wondered whether there would ever be a
case in which the driver would want to reset the LTC2983.

Thanks,
Drew

RE: [PATCH v2 1/1] iio: ltc2983: fix device probe

[Sa, Nuno]

> -----Original Message-----
> From: Drew Fustini <drew@pdp7.com>
> Sent: Thursday, August 12, 2021 8:32 PM
> To: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> Cc: Sa, Nuno <Nuno.Sa@analog.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; linux-iio <linux-iio@vger.kernel.org>;
> Jonathan Cameron <jic23@kernel.org>; Lars-Peter Clausen
> <lars@metafoo.de>
> Subject: Re: [PATCH v2 1/1] iio: ltc2983: fix device probe
> 
> [External]
> 
> On Thu, Aug 12, 2021 at 07:19:19PM +0100, Jonathan Cameron wrote:
> > On Thu, 12 Aug 2021 06:54:13 +0000
> > "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> >
> > > > -----Original Message-----
> > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Sent: Wednesday, August 11, 2021 6:15 PM
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > > <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>;
> Drew
> > > > Fustini <drew@pdp7.com>
> > > > Subject: Re: [PATCH v2 1/1] iio: ltc2983: fix device probe
> > > >
> > > > [External]
> > > >
> > > > On Wed, Aug 11, 2021 at 4:32 PM Nuno Sá
> <nuno.sa@analog.com>
> > > > 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@gmail.com>
> > > >
> > >
> > > Thanks for reviewing... Yeah, I will wait for Jonathan's feedback
> before
> > > sending a v3. It might be that he can apply your inputs when
> applying
> > > the patch.
> >
> > Patch looks sensible. I can make the tweaks whilst applying when I
> > happen to be on the right computer.
> >
> > Having glanced at the datasheet, I wonder if you ever had the
> > reset pin wired up (and perhaps decided to drop that
> > complexity later)?  The driver rather oddly
> > include of_gpio.h and has no gpio accesses which makes me
> > wonder ;)
> 
> I'm sure Nuno will have something to say but I wanted to mention that

If my memory is not playing tricks on me, I'm almost positive that I never
used the reset pin when working on this driver so my best bet is that the
header is there because of some "stupid" copy paste that I did.... Though
I also wondered why I did not added support for the reset pin and
possibly vdd regulator...
   
> I
> was initially thinking about this.  The reset pin is connected to a GPIO
> on the Zynq-7000 on the custom board I am using.  I added that gpio
> line
> as an active low gpio hog under the gpio controller node in the DTS and
> that worked ok.  Though I had wondered whether there would ever
> be a
> case in which the driver would want to reset the LTC2983.
> 

Yeah, since I have the setup ready I might just add support for the
optional reset pin. If given, we force a reset during probe. Then,
you can remove the gpio hog...

- Nuno Sá

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

[Jonathan Cameron]

On Thu, 12 Aug 2021 19:19:19 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> wrote:

> On Thu, 12 Aug 2021 06:54:13 +0000
> "Sa, Nuno" <Nuno.Sa@analog.com> wrote:
> 
> > > -----Original Message-----
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Wednesday, August 11, 2021 6:15 PM
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-iio <linux-iio@vger.kernel.org>; Jonathan Cameron
> > > <jic23@kernel.org>; Lars-Peter Clausen <lars@metafoo.de>; Drew
> > > Fustini <drew@pdp7.com>
> > > Subject: Re: [PATCH v2 1/1] iio: ltc2983: fix device probe
> > > 
> > > [External]
> > > 
> > > On Wed, Aug 11, 2021 at 4:32 PM Nuno Sá <nuno.sa@analog.com>
> > > 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@gmail.com>
> > >     
> > 
> > Thanks for reviewing... Yeah, I will wait for Jonathan's feedback before
> > sending a v3. It might be that he can apply your inputs when applying
> > the patch.  
> 
> Patch looks sensible. I can make the tweaks whilst applying when I
> happen to be on the right computer.
Given we are too late for fixes (other than urgent regressions introduced
this cycle) I've tweaked as Andy suggested and applied to the togreg branch of
iio.git (pushed out as testing for 0-day to take first go at breaking it).

I've also marked it for stable.

Thanks,

Jonathan

> 
> Having glanced at the datasheet, I wonder if you ever had the
> reset pin wired up (and perhaps decided to drop that
> complexity later)?  The driver rather oddly
> include of_gpio.h and has no gpio accesses which makes me
> wonder ;)
> 
> Jonathan
> 
> > 
> > Thanks!
> > - Nuno Sá  
> 

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

[Drew Fustini]

On Wed, Aug 11, 2021 at 03:32:20PM +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>
> Reviewed-by: Drew Fustini <drew@pdp7.com>
> Tested-by: Drew Fustini <drew@pdp7.com>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  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;
>  	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
> 

I tested this version on my Zynq-7000 board and it works well too.

Thanks,
Drew