| Message ID | 20210820065535.6994-1-nuno.sa@analog.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [1/2] iio: ltc2983: add support for optional reset gpio | expand |
On Fri, Aug 20, 2021 at 9:53 AM Nuno Sá <nuno.sa@analog.com> wrote: > > Check if an optional reset gpio is present and if so, make sure to reset > the device. > Just one note/question inline. > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > --- > drivers/iio/temperature/ltc2983.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c > index 3b4a0e60e605..37903e9fb90f 100644 > --- a/drivers/iio/temperature/ltc2983.c > +++ b/drivers/iio/temperature/ltc2983.c > @@ -1470,6 +1470,7 @@ static int ltc2983_probe(struct spi_device *spi) > { > struct ltc2983_data *st; > struct iio_dev *indio_dev; > + struct gpio_desc *gpio; > const char *name = spi_get_device_id(spi)->name; > int ret; > > @@ -1494,6 +1495,16 @@ static int ltc2983_probe(struct spi_device *spi) > if (ret) > return ret; > > + gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", GPIOD_OUT_HIGH); > + if (IS_ERR(gpio)) > + return PTR_ERR(gpio); > + > + if (gpio) { > + /* bring device out of reset */ > + usleep_range(1000, 1005); > + gpiod_set_value_cansleep(gpio, 0); Datasheet mentions that it takes up to 100 ms for the device to fully start-up. It also mentions that the (command) status register will be unavailable to the user before this point. Page 16, Conversion State Details section, second paragraph. I think there should probably be a sleep here of 100 ms. Other than that change looks good. > + } > + > ret = ltc2983_setup(st, true); > if (ret) > return ret; > -- > 2.33.0 >
> -----Original Message----- > From: Alexandru Ardelean <ardeleanalex@gmail.com> > Sent: Friday, August 20, 2021 10:21 AM > 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 1/2] iio: ltc2983: add support for optional reset > gpio > > [External] > > On Fri, Aug 20, 2021 at 9:53 AM Nuno Sá <nuno.sa@analog.com> > wrote: > > > > Check if an optional reset gpio is present and if so, make sure to > reset > > the device. > > > > Just one note/question inline. > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > --- > > drivers/iio/temperature/ltc2983.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/iio/temperature/ltc2983.c > b/drivers/iio/temperature/ltc2983.c > > index 3b4a0e60e605..37903e9fb90f 100644 > > --- a/drivers/iio/temperature/ltc2983.c > > +++ b/drivers/iio/temperature/ltc2983.c > > @@ -1470,6 +1470,7 @@ static int ltc2983_probe(struct spi_device > *spi) > > { > > struct ltc2983_data *st; > > struct iio_dev *indio_dev; > > + struct gpio_desc *gpio; > > const char *name = spi_get_device_id(spi)->name; > > int ret; > > > > @@ -1494,6 +1495,16 @@ static int ltc2983_probe(struct spi_device > *spi) > > if (ret) > > return ret; > > > > + gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", > GPIOD_OUT_HIGH); > > + if (IS_ERR(gpio)) > > + return PTR_ERR(gpio); > > + > > + if (gpio) { > > + /* bring device out of reset */ > > + usleep_range(1000, 1005); > > + gpiod_set_value_cansleep(gpio, 0); > > Datasheet mentions that it takes up to 100 ms for the device to fully > start-up. > It also mentions that the (command) status register will be > unavailable to the user before this point. > Page 16, Conversion State Details section, second paragraph. > > I think there should probably be a sleep here of 100 ms. > > Other than that change looks good. > In the setup function we do a polled read on the status register until we get the indication we are up. This was actually a fix sent recently [1]. [1]: https://patchwork.kernel.org/project/linux-iio/patch/20210811133220.190264-2-nuno.sa@analog.com/ - Nuno Sá > > + } > > + > > ret = ltc2983_setup(st, true); > > if (ret) > > return ret; > > -- > > 2.33.0 > >
On Fri, Aug 20, 2021 at 12:29 PM Sa, Nuno <Nuno.Sa@analog.com> wrote: > > > > > -----Original Message----- > > From: Alexandru Ardelean <ardeleanalex@gmail.com> > > Sent: Friday, August 20, 2021 10:21 AM > > 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 1/2] iio: ltc2983: add support for optional reset > > gpio > > > > [External] > > > > On Fri, Aug 20, 2021 at 9:53 AM Nuno Sá <nuno.sa@analog.com> > > wrote: > > > > > > Check if an optional reset gpio is present and if so, make sure to > > reset > > > the device. > > > > > > > Just one note/question inline. > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > --- > > > drivers/iio/temperature/ltc2983.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/drivers/iio/temperature/ltc2983.c > > b/drivers/iio/temperature/ltc2983.c > > > index 3b4a0e60e605..37903e9fb90f 100644 > > > --- a/drivers/iio/temperature/ltc2983.c > > > +++ b/drivers/iio/temperature/ltc2983.c > > > @@ -1470,6 +1470,7 @@ static int ltc2983_probe(struct spi_device > > *spi) > > > { > > > struct ltc2983_data *st; > > > struct iio_dev *indio_dev; > > > + struct gpio_desc *gpio; > > > const char *name = spi_get_device_id(spi)->name; > > > int ret; > > > > > > @@ -1494,6 +1495,16 @@ static int ltc2983_probe(struct spi_device > > *spi) > > > if (ret) > > > return ret; > > > > > > + gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", > > GPIOD_OUT_HIGH); > > > + if (IS_ERR(gpio)) > > > + return PTR_ERR(gpio); > > > + > > > + if (gpio) { > > > + /* bring device out of reset */ > > > + usleep_range(1000, 1005); > > > + gpiod_set_value_cansleep(gpio, 0); > > > > Datasheet mentions that it takes up to 100 ms for the device to fully > > start-up. > > It also mentions that the (command) status register will be > > unavailable to the user before this point. > > Page 16, Conversion State Details section, second paragraph. > > > > I think there should probably be a sleep here of 100 ms. > > > > Other than that change looks good. > > > > In the setup function we do a polled read on the status register until > we get the indication we are up. This was actually a fix sent recently > [1]. > Yes, I saw that. But I did not have energy to look at it too in-depth [at that moment in time]. Apologies. But the question is: is the statement on page 16 valid? i.e. do we need to wait 100ms after the reset pin goes low? because it states: In the first phase of the start-up state all critical analog circuits are powered up. This includes the LDO, reference, charge pump and ADCs. ***During this first phase, the com-mand status register will be inaccessible to the user.*** ***This phase takes a maximum of 100mS to complete. *** Once this phase completes, the command status register will be accessible and return a value of 0x80 until the LTC2983 is completely initialized. > [1]: https://patchwork.kernel.org/project/linux-iio/patch/20210811133220.190264-2-nuno.sa@analog.com/ > - Nuno Sá > > > > + } > > > + > > > ret = ltc2983_setup(st, true); > > > if (ret) > > > return ret; > > > -- > > > 2.33.0 > > >
> -----Original Message----- > From: Alexandru Ardelean <ardeleanalex@gmail.com> > Sent: Friday, August 20, 2021 8:59 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 1/2] iio: ltc2983: add support for optional reset > gpio > > On Fri, Aug 20, 2021 at 12:29 PM Sa, Nuno <Nuno.Sa@analog.com> > wrote: > > > > > > > > > -----Original Message----- > > > From: Alexandru Ardelean <ardeleanalex@gmail.com> > > > Sent: Friday, August 20, 2021 10:21 AM > > > 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 1/2] iio: ltc2983: add support for optional reset > > > gpio > > > > > > [External] > > > > > > On Fri, Aug 20, 2021 at 9:53 AM Nuno Sá <nuno.sa@analog.com> > > > wrote: > > > > > > > > Check if an optional reset gpio is present and if so, make sure to > > > reset > > > > the device. > > > > > > > > > > Just one note/question inline. > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > --- > > > > drivers/iio/temperature/ltc2983.c | 11 +++++++++++ > > > > 1 file changed, 11 insertions(+) > > > > > > > > diff --git a/drivers/iio/temperature/ltc2983.c > > > b/drivers/iio/temperature/ltc2983.c > > > > index 3b4a0e60e605..37903e9fb90f 100644 > > > > --- a/drivers/iio/temperature/ltc2983.c > > > > +++ b/drivers/iio/temperature/ltc2983.c > > > > @@ -1470,6 +1470,7 @@ static int ltc2983_probe(struct > spi_device > > > *spi) > > > > { > > > > struct ltc2983_data *st; > > > > struct iio_dev *indio_dev; > > > > + struct gpio_desc *gpio; > > > > const char *name = spi_get_device_id(spi)->name; > > > > int ret; > > > > > > > > @@ -1494,6 +1495,16 @@ static int ltc2983_probe(struct > spi_device > > > *spi) > > > > if (ret) > > > > return ret; > > > > > > > > + gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", > > > GPIOD_OUT_HIGH); > > > > + if (IS_ERR(gpio)) > > > > + return PTR_ERR(gpio); > > > > + > > > > + if (gpio) { > > > > + /* bring device out of reset */ > > > > + usleep_range(1000, 1005); > > > > + gpiod_set_value_cansleep(gpio, 0); > > > > > > Datasheet mentions that it takes up to 100 ms for the device to > fully > > > start-up. > > > It also mentions that the (command) status register will be > > > unavailable to the user before this point. > > > Page 16, Conversion State Details section, second paragraph. > > > > > > I think there should probably be a sleep here of 100 ms. > > > > > > Other than that change looks good. > > > > > > > In the setup function we do a polled read on the status register until > > we get the indication we are up. This was actually a fix sent recently > > [1]. > > > > Yes, I saw that. > But I did not have energy to look at it too in-depth [at that moment in > time]. > Apologies. > > But the question is: is the statement on page 16 valid? > i.e. do we need to wait 100ms after the reset pin goes low? because it > states: > > In the first phase of the start-up state all critical analog circuits > are powered up. > This includes the LDO, reference, charge pump and ADCs. > ***During this first phase, the com-mand status register will be > inaccessible to the user.*** > ***This phase takes a maximum of 100mS to complete. *** > Once this phase completes, the command status register will be > accessible and return > a value of 0x80 until the LTC2983 is completely initialized. Yes, I saw that on the datasheet. My interpretation is that the status register is just not available so we just get 0x00 or 0xff (whatever the default state of the SDO line). In my tests, it took 3 iterations to acknowledge the device as up. Reading 0x00, 0x80 and 0x40... Being this said, let's see if someone else has something to say about this. I'm more than fine in adding the 100ms as I also wondered about this to be honest. - Nuno Sá
On Fri, Aug 20, 2021 at 9:53 AM Nuno Sá <nuno.sa@analog.com> wrote: > > Check if an optional reset gpio is present and if so, make sure to reset > the device. ... > + usleep_range(1000, 1005); The delta should be at least 20%, otherwise I'm not sure why such a strict range?
On Mon, 2021-08-23 at 14:14 +0300, Andy Shevchenko wrote: > On Fri, Aug 20, 2021 at 9:53 AM Nuno Sá <nuno.sa@analog.com> wrote: > > Check if an optional reset gpio is present and if so, make sure to > > reset > > the device. > > ... > > > + usleep_range(1000, 1005); > > The delta should be at least 20%, otherwise I'm not sure why such a > strict range? > No special reason... I just had no hard requirement for delta so I just gave something small. Is 20% documented anywhere? I did a quick look on the API's and I could not find nothing related. Anyways, if that is a best practise for being more power friendly I'm happy to change it... (well, we might end up just having 100ms here which means 'msleep()'). - Nuno Sá
On Mon, Aug 23, 2021 at 3:51 PM Nuno Sá <noname.nuno@gmail.com> wrote: > On Mon, 2021-08-23 at 14:14 +0300, Andy Shevchenko wrote: > > On Fri, Aug 20, 2021 at 9:53 AM Nuno Sá <nuno.sa@analog.com> wrote: > > > Check if an optional reset gpio is present and if so, make sure to > > > reset > > > the device. > > > > ... > > > > > + usleep_range(1000, 1005); > > > > The delta should be at least 20%, otherwise I'm not sure why such a > > strict range? > > > > No special reason... I just had no hard requirement for delta so I just > gave something small. Is 20% documented anywhere? Quick search shows nothing, but I remember I saw it somewhere. So, the explanation is empirical, because the idea behind is to allow less HRT interrupts. When you do a tough margin, you may generate too many interrupts from the timer. So, 20% seems like a good balance for most of the values. The parameters to take into account are: - minimum (or maybe rather median?) CPU frequency the code will be run on - minimal sleep (for small sleeps even better to have udelay() as I believe documented in timers.rst, for bigger sleeps, like 10ms the margin can be 10% or so) - NO_HZ kernel configuration - etc (if anything I forgot) > I did a quick look on > the API's and I could not find nothing related. Anyways, if that is a > best practise for being more power friendly I'm happy to change it... > (well, we might end up just having 100ms here which means 'msleep()').
On Mon, 2021-08-23 at 17:27 +0300, Andy Shevchenko wrote: > On Mon, Aug 23, 2021 at 3:51 PM Nuno Sá <noname.nuno@gmail.com> > wrote: > > On Mon, 2021-08-23 at 14:14 +0300, Andy Shevchenko wrote: > > > On Fri, Aug 20, 2021 at 9:53 AM Nuno Sá <nuno.sa@analog.com> > > > wrote: > > > > Check if an optional reset gpio is present and if so, make sure > > > > to > > > > reset > > > > the device. > > > > > > ... > > > > > > > + usleep_range(1000, 1005); > > > > > > The delta should be at least 20%, otherwise I'm not sure why such > > > a > > > strict range? > > > > > > > No special reason... I just had no hard requirement for delta so I > > just > > gave something small. Is 20% documented anywhere? > > Quick search shows nothing, but I remember I saw it somewhere. > So, the explanation is empirical, because the idea behind is to allow > less HRT interrupts. When you do a tough margin, you may generate too > many interrupts from the timer. So, 20% seems like a good balance for > most of the values. > I see, that makes sense to me. > The parameters to take into account are: > - minimum (or maybe rather median?) CPU frequency the code will be > run on > - minimal sleep (for small sleeps even better to have udelay() as I > believe documented in timers.rst, for bigger sleeps, like 10ms the > margin can be 10% or so) udelay() would be for sleeps < 10us - Nuno Sá
> From: Sa, Nuno <Nuno.Sa@analog.com> > Sent: Monday, August 23, 2021 9:05 AM > To: Alexandru Ardelean <ardeleanalex@gmail.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 1/2] iio: ltc2983: add support for optional reset > gpio > > [External] > > > > > -----Original Message----- > > From: Alexandru Ardelean <ardeleanalex@gmail.com> > > Sent: Friday, August 20, 2021 8:59 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 1/2] iio: ltc2983: add support for optional reset > > gpio > > > > On Fri, Aug 20, 2021 at 12:29 PM Sa, Nuno <Nuno.Sa@analog.com> > > wrote: > > > > > > > > > > > > > -----Original Message----- > > > > From: Alexandru Ardelean <ardeleanalex@gmail.com> > > > > Sent: Friday, August 20, 2021 10:21 AM > > > > 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 1/2] iio: ltc2983: add support for optional > reset > > > > gpio > > > > > > > > [External] > > > > > > > > On Fri, Aug 20, 2021 at 9:53 AM Nuno Sá <nuno.sa@analog.com> > > > > wrote: > > > > > > > > > > Check if an optional reset gpio is present and if so, make sure to > > > > reset > > > > > the device. > > > > > > > > > > > > > Just one note/question inline. > > > > > > > > > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > > > > > --- > > > > > drivers/iio/temperature/ltc2983.c | 11 +++++++++++ > > > > > 1 file changed, 11 insertions(+) > > > > > > > > > > diff --git a/drivers/iio/temperature/ltc2983.c > > > > b/drivers/iio/temperature/ltc2983.c > > > > > index 3b4a0e60e605..37903e9fb90f 100644 > > > > > --- a/drivers/iio/temperature/ltc2983.c > > > > > +++ b/drivers/iio/temperature/ltc2983.c > > > > > @@ -1470,6 +1470,7 @@ static int ltc2983_probe(struct > > spi_device > > > > *spi) > > > > > { > > > > > struct ltc2983_data *st; > > > > > struct iio_dev *indio_dev; > > > > > + struct gpio_desc *gpio; > > > > > const char *name = spi_get_device_id(spi)->name; > > > > > int ret; > > > > > > > > > > @@ -1494,6 +1495,16 @@ static int ltc2983_probe(struct > > spi_device > > > > *spi) > > > > > if (ret) > > > > > return ret; > > > > > > > > > > + gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", > > > > GPIOD_OUT_HIGH); > > > > > + if (IS_ERR(gpio)) > > > > > + return PTR_ERR(gpio); > > > > > + > > > > > + if (gpio) { > > > > > + /* bring device out of reset */ > > > > > + usleep_range(1000, 1005); > > > > > + gpiod_set_value_cansleep(gpio, 0); > > > > > > > > Datasheet mentions that it takes up to 100 ms for the device to > > fully > > > > start-up. > > > > It also mentions that the (command) status register will be > > > > unavailable to the user before this point. > > > > Page 16, Conversion State Details section, second paragraph. > > > > > > > > I think there should probably be a sleep here of 100 ms. > > > > > > > > Other than that change looks good. > > > > > > > > > > In the setup function we do a polled read on the status register > until > > > we get the indication we are up. This was actually a fix sent recently > > > [1]. > > > > > > > Yes, I saw that. > > But I did not have energy to look at it too in-depth [at that moment > in > > time]. > > Apologies. > > > > But the question is: is the statement on page 16 valid? > > i.e. do we need to wait 100ms after the reset pin goes low? because > it > > states: > > > > In the first phase of the start-up state all critical analog circuits > > are powered up. > > This includes the LDO, reference, charge pump and ADCs. > > ***During this first phase, the com-mand status register will be > > inaccessible to the user.*** > > ***This phase takes a maximum of 100mS to complete. *** > > Once this phase completes, the command status register will be > > accessible and return > > a value of 0x80 until the LTC2983 is completely initialized. > > Yes, I saw that on the datasheet. My interpretation is that the status > register is just not available so we just get 0x00 or 0xff (whatever the > default state of the SDO line). In my tests, it took 3 iterations to > acknowledge > the device as up. Reading 0x00, 0x80 and 0x40... > > Being this said, let's see if someone else has something to say about > this. > I'm more than fine in adding the 100ms as I also wondered about this > to > be honest. So, I did consulted internally about this and the status register is 0 initialized during the initial powert-up state which means we don't really have to wait 100ms here.... Anyways, I will still send a v2 with a improved range for 'usleep_range()'. - Nuno Sá
diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c index 3b4a0e60e605..37903e9fb90f 100644 --- a/drivers/iio/temperature/ltc2983.c +++ b/drivers/iio/temperature/ltc2983.c @@ -1470,6 +1470,7 @@ static int ltc2983_probe(struct spi_device *spi) { struct ltc2983_data *st; struct iio_dev *indio_dev; + struct gpio_desc *gpio; const char *name = spi_get_device_id(spi)->name; int ret; @@ -1494,6 +1495,16 @@ static int ltc2983_probe(struct spi_device *spi) if (ret) return ret; + gpio = devm_gpiod_get_optional(&st->spi->dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(gpio)) + return PTR_ERR(gpio); + + if (gpio) { + /* bring device out of reset */ + usleep_range(1000, 1005); + gpiod_set_value_cansleep(gpio, 0); + } + ret = ltc2983_setup(st, true); if (ret) return ret;
Check if an optional reset gpio is present and if so, make sure to reset the device. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/temperature/ltc2983.c | 11 +++++++++++ 1 file changed, 11 insertions(+)