[PATCH 1/2] iio: ltc2983: add support for optional reset gpio

[PATCH 1/2] iio: ltc2983: add support for optional reset gpio

[Nuno Sá]

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

[PATCH 2/2] iio: ltc2983: fail probe if no channels are given

[Nuno Sá]

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

Re: [PATCH 2/2] iio: ltc2983: fail probe if no channels are given

[Alexandru Ardelean]

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
>

Re: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio

[Alexandru Ardelean]

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
>

RE: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio

[Sa, Nuno]

> -----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
> >

Re: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio

[Alexandru Ardelean]

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
> > >

RE: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio

[Sa, Nuno]

> -----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á
 

RE: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio

[Sa, Nuno]

> 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á

Re: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio

[Andy Shevchenko]

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

Re: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio

[Nuno Sá]

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á

Re: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio

[Andy Shevchenko]

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

Re: [PATCH 1/2] iio: ltc2983: add support for optional reset gpio

[Nuno Sá]

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á