| Message ID | 20210811133220.190264-2-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Fix ltc2983 probing | expand |
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 >
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
> -----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á
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á
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
> -----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á
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á >
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;
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(-)