* [PATCH 1/2] iio: ltc2983: add support for optional reset gpio @ 2021-08-20 6:55 Nuno Sá 2021-08-20 6:55 ` [PATCH 2/2] iio: ltc2983: fail probe if no channels are given Nuno Sá ` (2 more replies) 0 siblings, 3 replies; 12+ messages in thread From: Nuno Sá @ 2021-08-20 6:55 UTC (permalink / raw) To: linux-iio; +Cc: Jonathan Cameron, Lars-Peter Clausen, Drew Fustini 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(+) 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; -- 2.33.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/2] iio: ltc2983: fail probe if no channels are given 2021-08-20 6:55 [PATCH 1/2] iio: ltc2983: add support for optional reset gpio Nuno Sá @ 2021-08-20 6:55 ` Nuno Sá 2021-08-20 8:22 ` Alexandru Ardelean 2021-08-20 8:21 ` [PATCH 1/2] iio: ltc2983: add support for optional reset gpio Alexandru Ardelean 2021-08-23 11:14 ` Andy Shevchenko 2 siblings, 1 reply; 12+ messages in thread From: Nuno Sá @ 2021-08-20 6:55 UTC (permalink / raw) To: linux-iio; +Cc: Jonathan Cameron, Lars-Peter Clausen, Drew Fustini If there are no channels defined in the devicetree, there's no point in probing the device. We were actually requesting a zero sized 'kmalloc' array but since we were not touching the ZERO_SIZE_PTR afterwards, nothing bad was actually happening. Hence this is not really a fix but rather an improvement. Signed-off-by: Nuno Sá <nuno.sa@analog.com> --- drivers/iio/temperature/ltc2983.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c index 37903e9fb90f..7dd2f1dd3685 100644 --- a/drivers/iio/temperature/ltc2983.c +++ b/drivers/iio/temperature/ltc2983.c @@ -1275,6 +1275,11 @@ static int ltc2983_parse_dt(struct ltc2983_data *st) &st->filter_notch_freq); st->num_channels = of_get_available_child_count(dev->of_node); + if (!st->num_channels) { + dev_err(&st->spi->dev, "At least one channel must be given!"); + return -EINVAL; + } + st->sensors = devm_kcalloc(dev, st->num_channels, sizeof(*st->sensors), GFP_KERNEL); if (!st->sensors) -- 2.33.0 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 2/2] iio: ltc2983: fail probe if no channels are given 2021-08-20 6:55 ` [PATCH 2/2] iio: ltc2983: fail probe if no channels are given Nuno Sá @ 2021-08-20 8:22 ` Alexandru Ardelean 0 siblings, 0 replies; 12+ messages in thread From: Alexandru Ardelean @ 2021-08-20 8:22 UTC (permalink / raw) To: Nuno Sá Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Drew Fustini On Fri, Aug 20, 2021 at 9:53 AM Nuno Sá <nuno.sa@analog.com> wrote: > > If there are no channels defined in the devicetree, there's no point in > probing the device. We were actually requesting a zero sized 'kmalloc' > array but since we were not touching the ZERO_SIZE_PTR afterwards, > nothing bad was actually happening. Hence this is not really a fix but > rather an improvement. > Reviewed-by: Alexandru Ardelean <ardeleanalex@gmail.com> > Signed-off-by: Nuno Sá <nuno.sa@analog.com> > --- > drivers/iio/temperature/ltc2983.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/iio/temperature/ltc2983.c b/drivers/iio/temperature/ltc2983.c > index 37903e9fb90f..7dd2f1dd3685 100644 > --- a/drivers/iio/temperature/ltc2983.c > +++ b/drivers/iio/temperature/ltc2983.c > @@ -1275,6 +1275,11 @@ static int ltc2983_parse_dt(struct ltc2983_data *st) > &st->filter_notch_freq); > > st->num_channels = of_get_available_child_count(dev->of_node); > + if (!st->num_channels) { > + dev_err(&st->spi->dev, "At least one channel must be given!"); > + return -EINVAL; > + } > + > st->sensors = devm_kcalloc(dev, st->num_channels, sizeof(*st->sensors), > GFP_KERNEL); > if (!st->sensors) > -- > 2.33.0 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio 2021-08-20 6:55 [PATCH 1/2] iio: ltc2983: add support for optional reset gpio Nuno Sá 2021-08-20 6:55 ` [PATCH 2/2] iio: ltc2983: fail probe if no channels are given Nuno Sá @ 2021-08-20 8:21 ` Alexandru Ardelean 2021-08-20 9:29 ` Sa, Nuno 2021-08-23 11:14 ` Andy Shevchenko 2 siblings, 1 reply; 12+ messages in thread From: Alexandru Ardelean @ 2021-08-20 8:21 UTC (permalink / raw) To: Nuno Sá Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Drew Fustini 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio 2021-08-20 8:21 ` [PATCH 1/2] iio: ltc2983: add support for optional reset gpio Alexandru Ardelean @ 2021-08-20 9:29 ` Sa, Nuno 2021-08-20 18:58 ` Alexandru Ardelean 0 siblings, 1 reply; 12+ messages in thread From: Sa, Nuno @ 2021-08-20 9:29 UTC (permalink / raw) To: Alexandru Ardelean Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Drew Fustini > -----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 > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio 2021-08-20 9:29 ` Sa, Nuno @ 2021-08-20 18:58 ` Alexandru Ardelean 2021-08-23 7:04 ` Sa, Nuno 0 siblings, 1 reply; 12+ messages in thread From: Alexandru Ardelean @ 2021-08-20 18:58 UTC (permalink / raw) To: Sa, Nuno; +Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Drew Fustini 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 > > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio 2021-08-20 18:58 ` Alexandru Ardelean @ 2021-08-23 7:04 ` Sa, Nuno 2021-08-25 8:24 ` Sa, Nuno 0 siblings, 1 reply; 12+ messages in thread From: Sa, Nuno @ 2021-08-23 7:04 UTC (permalink / raw) To: Alexandru Ardelean Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Drew Fustini > -----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á ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio 2021-08-23 7:04 ` Sa, Nuno @ 2021-08-25 8:24 ` Sa, Nuno 0 siblings, 0 replies; 12+ messages in thread From: Sa, Nuno @ 2021-08-25 8:24 UTC (permalink / raw) To: Sa, Nuno, Alexandru Ardelean Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Drew Fustini > 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á ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio 2021-08-20 6:55 [PATCH 1/2] iio: ltc2983: add support for optional reset gpio Nuno Sá 2021-08-20 6:55 ` [PATCH 2/2] iio: ltc2983: fail probe if no channels are given Nuno Sá 2021-08-20 8:21 ` [PATCH 1/2] iio: ltc2983: add support for optional reset gpio Alexandru Ardelean @ 2021-08-23 11:14 ` Andy Shevchenko 2021-08-23 12:51 ` Nuno Sá 2 siblings, 1 reply; 12+ messages in thread From: Andy Shevchenko @ 2021-08-23 11:14 UTC (permalink / raw) To: Nuno Sá Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Drew Fustini 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? -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio 2021-08-23 11:14 ` Andy Shevchenko @ 2021-08-23 12:51 ` Nuno Sá 2021-08-23 14:27 ` Andy Shevchenko 0 siblings, 1 reply; 12+ messages in thread From: Nuno Sá @ 2021-08-23 12:51 UTC (permalink / raw) To: Andy Shevchenko, Nuno Sá Cc: linux-iio, Jonathan Cameron, Lars-Peter Clausen, Drew Fustini 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á ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio 2021-08-23 12:51 ` Nuno Sá @ 2021-08-23 14:27 ` Andy Shevchenko 2021-08-23 14:51 ` Nuno Sá 0 siblings, 1 reply; 12+ messages in thread From: Andy Shevchenko @ 2021-08-23 14:27 UTC (permalink / raw) To: Nuno Sá Cc: Nuno Sá, linux-iio, Jonathan Cameron, Lars-Peter Clausen, Drew Fustini 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()'). -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio 2021-08-23 14:27 ` Andy Shevchenko @ 2021-08-23 14:51 ` Nuno Sá 0 siblings, 0 replies; 12+ messages in thread From: Nuno Sá @ 2021-08-23 14:51 UTC (permalink / raw) To: Andy Shevchenko Cc: Nuno Sá, linux-iio, Jonathan Cameron, Lars-Peter Clausen, Drew Fustini 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á ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-08-25 8:25 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-20 6:55 [PATCH 1/2] iio: ltc2983: add support for optional reset gpio Nuno Sá 2021-08-20 6:55 ` [PATCH 2/2] iio: ltc2983: fail probe if no channels are given Nuno Sá 2021-08-20 8:22 ` Alexandru Ardelean 2021-08-20 8:21 ` [PATCH 1/2] iio: ltc2983: add support for optional reset gpio Alexandru Ardelean 2021-08-20 9:29 ` Sa, Nuno 2021-08-20 18:58 ` Alexandru Ardelean 2021-08-23 7:04 ` Sa, Nuno 2021-08-25 8:24 ` Sa, Nuno 2021-08-23 11:14 ` Andy Shevchenko 2021-08-23 12:51 ` Nuno Sá 2021-08-23 14:27 ` Andy Shevchenko 2021-08-23 14:51 ` Nuno Sá
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.