[PATCH 1/2] iio: st_sensors: Retry ID verification on failure

[PATCH 1/2] iio: st_sensors: Retry ID verification on failure

[Matti Lehtimäki]

Some sensors do not always start fast enough to read a valid ID from
registers at first attempt. Let's retry at most 3 times with short sleep
in between to fix random timing issues.

Signed-off-by: Matti Lehtimäki <matti.lehtimaki@gmail.com>
---
 drivers/iio/common/st_sensors/st_sensors_core.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 9910ba1da085..106f7953683e 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -21,6 +21,8 @@
 
 #include "st_sensors_core.h"
 
+#define VERIFY_ID_RETRY_COUNT 3
+
 int st_sensors_write_data_with_mask(struct iio_dev *indio_dev,
 				    u8 reg_addr, u8 mask, u8 data)
 {
@@ -619,11 +621,18 @@ EXPORT_SYMBOL_NS(st_sensors_get_settings_index, IIO_ST_SENSORS);
 int st_sensors_verify_id(struct iio_dev *indio_dev)
 {
 	struct st_sensor_data *sdata = iio_priv(indio_dev);
-	int wai, err;
+	int wai, err, i;
 
 	if (sdata->sensor_settings->wai_addr) {
-		err = regmap_read(sdata->regmap,
-				  sdata->sensor_settings->wai_addr, &wai);
+		for (i = 0; i < VERIFY_ID_RETRY_COUNT; i++) {
+			err = regmap_read(sdata->regmap,
+					  sdata->sensor_settings->wai_addr, &wai);
+
+			if (!err && sdata->sensor_settings->wai == wai)
+				return 0;
+
+			msleep(20);
+		}
 		if (err < 0) {
 			dev_err(&indio_dev->dev,
 				"failed to read Who-Am-I register.\n");
-- 
2.34.1

[PATCH 2/2] iio: st_sensors: Fix null pointer on defer_probe error

[Matti Lehtimäki]

dev_err_probe() calls __device_set_deferred_probe_reason()
on -EPROBE_DEFER error. The device pointer to driver core private
structure is not yet initialized at this stage for the iio device causing
a null pointer error. Use parent device instead.

Fixes: 4dff75487695 ("iio: st_sensors: Make use of the helper function dev_err_probe()")
Signed-off-by: Matti Lehtimäki <matti.lehtimaki@gmail.com>
---
 drivers/iio/common/st_sensors/st_sensors_core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
index 106f7953683e..575607058291 100644
--- a/drivers/iio/common/st_sensors/st_sensors_core.c
+++ b/drivers/iio/common/st_sensors/st_sensors_core.c
@@ -235,7 +235,7 @@ int st_sensors_power_enable(struct iio_dev *indio_dev)
 	/* Regulators not mandatory, but if requested we should enable them. */
 	pdata->vdd = devm_regulator_get(parent, "vdd");
 	if (IS_ERR(pdata->vdd))
-		return dev_err_probe(&indio_dev->dev, PTR_ERR(pdata->vdd),
+		return dev_err_probe(indio_dev->dev.parent, PTR_ERR(pdata->vdd),
 				     "unable to get Vdd supply\n");
 
 	err = regulator_enable(pdata->vdd);
@@ -251,7 +251,7 @@ int st_sensors_power_enable(struct iio_dev *indio_dev)
 
 	pdata->vdd_io = devm_regulator_get(parent, "vddio");
 	if (IS_ERR(pdata->vdd_io))
-		return dev_err_probe(&indio_dev->dev, PTR_ERR(pdata->vdd_io),
+		return dev_err_probe(indio_dev->dev.parent, PTR_ERR(pdata->vdd_io),
 				     "unable to get Vdd_IO supply\n");
 
 	err = regulator_enable(pdata->vdd_io);
-- 
2.34.1

Re: [PATCH 2/2] iio: st_sensors: Fix null pointer on defer_probe error

[Andy Shevchenko]

On Sun, Jul 24, 2022 at 7:01 PM Matti Lehtimäki
<matti.lehtimaki@gmail.com> wrote:
>
> dev_err_probe() calls __device_set_deferred_probe_reason()
> on -EPROBE_DEFER error. The device pointer to driver core private
> structure is not yet initialized at this stage for the iio device causing
> a null pointer error. Use parent device instead.

the parent device pointer

...

>         pdata->vdd = devm_regulator_get(parent, "vdd");
>         if (IS_ERR(pdata->vdd))
> -               return dev_err_probe(&indio_dev->dev, PTR_ERR(pdata->vdd),
> +               return dev_err_probe(indio_dev->dev.parent, PTR_ERR(pdata->vdd),
>                                      "unable to get Vdd supply\n");

Why not use the 'parent' variable?

...

>         pdata->vdd_io = devm_regulator_get(parent, "vddio");
>         if (IS_ERR(pdata->vdd_io))
> -               return dev_err_probe(&indio_dev->dev, PTR_ERR(pdata->vdd_io),
> +               return dev_err_probe(indio_dev->dev.parent, PTR_ERR(pdata->vdd_io),

Ditto.

>                                      "unable to get Vdd_IO supply\n");


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] iio: st_sensors: Retry ID verification on failure

[Andy Shevchenko]

On Sun, Jul 24, 2022 at 6:54 PM Matti Lehtimäki
<matti.lehtimaki@gmail.com> wrote:
>
> Some sensors do not always start fast enough to read a valid ID from
> registers at first attempt. Let's retry at most 3 times with short sleep
> in between to fix random timing issues.

...

> +               for (i = 0; i < VERIFY_ID_RETRY_COUNT; i++) {
> +                       err = regmap_read(sdata->regmap,
> +                                         sdata->sensor_settings->wai_addr, &wai);
> +
> +                       if (!err && sdata->sensor_settings->wai == wai)
> +                               return 0;
> +
> +                       msleep(20);

NIH regmap_read_poll_timeout()


> +               }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] iio: st_sensors: Retry ID verification on failure

[Jonathan Cameron]

On Sun, 24 Jul 2022 19:43:15 +0300
Matti Lehtimäki <matti.lehtimaki@gmail.com> wrote:

> Some sensors do not always start fast enough to read a valid ID from
> registers at first attempt. Let's retry at most 3 times with short sleep
> in between to fix random timing issues.
> 
> Signed-off-by: Matti Lehtimäki <matti.lehtimaki@gmail.com>
Hi Matti,

My gut feeling is this isn't in a fast path, so why not just wait
for whatever the documented power up time of the sensor is?

I'd expect to see a sleep in st_sensors_power_enable() if one is
required.

Jonathan

> ---
>  drivers/iio/common/st_sensors/st_sensors_core.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/common/st_sensors/st_sensors_core.c b/drivers/iio/common/st_sensors/st_sensors_core.c
> index 9910ba1da085..106f7953683e 100644
> --- a/drivers/iio/common/st_sensors/st_sensors_core.c
> +++ b/drivers/iio/common/st_sensors/st_sensors_core.c
> @@ -21,6 +21,8 @@
>  
>  #include "st_sensors_core.h"
>  
> +#define VERIFY_ID_RETRY_COUNT 3
> +
>  int st_sensors_write_data_with_mask(struct iio_dev *indio_dev,
>  				    u8 reg_addr, u8 mask, u8 data)
>  {
> @@ -619,11 +621,18 @@ EXPORT_SYMBOL_NS(st_sensors_get_settings_index, IIO_ST_SENSORS);
>  int st_sensors_verify_id(struct iio_dev *indio_dev)
>  {
>  	struct st_sensor_data *sdata = iio_priv(indio_dev);
> -	int wai, err;
> +	int wai, err, i;
>  
>  	if (sdata->sensor_settings->wai_addr) {
> -		err = regmap_read(sdata->regmap,
> -				  sdata->sensor_settings->wai_addr, &wai);
> +		for (i = 0; i < VERIFY_ID_RETRY_COUNT; i++) {
> +			err = regmap_read(sdata->regmap,
> +					  sdata->sensor_settings->wai_addr, &wai);
> +
> +			if (!err && sdata->sensor_settings->wai == wai)
> +				return 0;
> +
> +			msleep(20);
How do we know 60msecs is long enough for all sensors?

> +		}
>  		if (err < 0) {
>  			dev_err(&indio_dev->dev,
>  				"failed to read Who-Am-I register.\n");

Re: [PATCH 1/2] iio: st_sensors: Retry ID verification on failure

[Matti Lehtimäki]

On 31.7.2022 19.00, Jonathan Cameron wrote:
> On Sun, 24 Jul 2022 19:43:15 +0300
> Matti Lehtimäki <matti.lehtimaki@gmail.com> wrote:
> 
>> Some sensors do not always start fast enough to read a valid ID from
>> registers at first attempt. Let's retry at most 3 times with short sleep
>> in between to fix random timing issues.
>>
>> Signed-off-by: Matti Lehtimäki <matti.lehtimaki@gmail.com>
> Hi Matti,
> 
> My gut feeling is this isn't in a fast path, so why not just wait
> for whatever the documented power up time of the sensor is?
> 
> I'd expect to see a sleep in st_sensors_power_enable() if one is
> required.

In the specification for the sensor (lis2hh12) I have on my device I
found that the maximum boot time of the sensor (starting from Vdd power
on) is defined as 20 ms. Not sure if the other sensors supported by the
driver have different values but based on checking a couple of
specifications I didn't find any bigger values so far.

>> +			msleep(20);
> How do we know 60msecs is long enough for all sensors?

Based on the specification for the sensor I have and also driver used in
Android kernel for my device (it uses a 3 x 20 ms loop) I think 20 ms is
a good value but to be sure a slightly longer might make sense. As
suggested in the other review comment by changing the regmap_read to
regmap_read_poll_timeout the function doesn't always need to wait at
least 20 ms in case first read doesn't provide the correct value, if a
suitable shorter poll interval is used (something like 1-10 ms).

However testing on my device has shown that I still need to have a loop
or at least a retry possibility because I have noticed a rare random
read error (-6, happens after some time not at first read) when reading
the id from the hardware. This could be due to for example internal
init failure of the sensor chip causing an internal reset. Because of
this read error regmap_read_poll_timeout returns with an error and
without retrying to read the id the sensor probe fails.

-Matti

Re: [PATCH 1/2] iio: st_sensors: Retry ID verification on failure

[Jonathan Cameron]

On Sun, 31 Jul 2022 21:51:55 +0300
Matti Lehtimäki <matti.lehtimaki@gmail.com> wrote:

> On 31.7.2022 19.00, Jonathan Cameron wrote:
> > On Sun, 24 Jul 2022 19:43:15 +0300
> > Matti Lehtimäki <matti.lehtimaki@gmail.com> wrote:
> >   
> >> Some sensors do not always start fast enough to read a valid ID from
> >> registers at first attempt. Let's retry at most 3 times with short sleep
> >> in between to fix random timing issues.
> >>
> >> Signed-off-by: Matti Lehtimäki <matti.lehtimaki@gmail.com>  
> > Hi Matti,
> > 
> > My gut feeling is this isn't in a fast path, so why not just wait
> > for whatever the documented power up time of the sensor is?
> > 
> > I'd expect to see a sleep in st_sensors_power_enable() if one is
> > required.  
> 
> In the specification for the sensor (lis2hh12) I have on my device I
> found that the maximum boot time of the sensor (starting from Vdd power
> on) is defined as 20 ms. Not sure if the other sensors supported by the
> driver have different values but based on checking a couple of
> specifications I didn't find any bigger values so far.
> 
> >> +			msleep(20);  
> > How do we know 60msecs is long enough for all sensors?  
> 
> Based on the specification for the sensor I have and also driver used in
> Android kernel for my device (it uses a 3 x 20 ms loop) I think 20 ms is
> a good value but to be sure a slightly longer might make sense. As
> suggested in the other review comment by changing the regmap_read to
> regmap_read_poll_timeout the function doesn't always need to wait at
> least 20 ms in case first read doesn't provide the correct value, if a
> suitable shorter poll interval is used (something like 1-10 ms).
> 
> However testing on my device has shown that I still need to have a loop
> or at least a retry possibility because I have noticed a rare random
> read error (-6, happens after some time not at first read) when reading
> the id from the hardware. This could be due to for example internal
> init failure of the sensor chip causing an internal reset. Because of
> this read error regmap_read_poll_timeout returns with an error and
> without retrying to read the id the sensor probe fails.

Nasty. If you can get a confirmation that it's a possible failure on startup
from the manufacturer then I'd be happier with that justification to retry
rather than just sleep for say 30msec after power on.

Jonathan

> 
> -Matti

Re: [PATCH 1/2] iio: st_sensors: Retry ID verification on failure

[Linus Walleij]

On Sun, Jul 31, 2022 at 10:07 PM Jonathan Cameron <jic23@kernel.org> wrote:
> Matti Lehtimäki <matti.lehtimaki@gmail.com> wrote:

> > Based on the specification for the sensor I have and also driver used in
> > Android kernel for my device (it uses a 3 x 20 ms loop) I think 20 ms is
> > a good value but to be sure a slightly longer might make sense. As
> > suggested in the other review comment by changing the regmap_read to
> > regmap_read_poll_timeout the function doesn't always need to wait at
> > least 20 ms in case first read doesn't provide the correct value, if a
> > suitable shorter poll interval is used (something like 1-10 ms).
> >
> > However testing on my device has shown that I still need to have a loop
> > or at least a retry possibility because I have noticed a rare random
> > read error (-6, happens after some time not at first read) when reading
> > the id from the hardware. This could be due to for example internal
> > init failure of the sensor chip causing an internal reset. Because of
> > this read error regmap_read_poll_timeout returns with an error and
> > without retrying to read the id the sensor probe fails.
>
> Nasty. If you can get a confirmation that it's a possible failure on startup
> from the manufacturer then I'd be happier with that justification to retry
> rather than just sleep for say 30msec after power on.

If the power comes from an external regulator (such as a fixed-regulator
on a GPIO) it could be that the startup time for that regulator is incorrectly
specified or unspecified (startup-delay-us = ... for regulator-fixed)?

Else I think if the a vendor version of a driver for this HW does this quirk,
that's as good indication as you will ever get from a vendor. Do you
have the android driver source code? Or is it a userspace blob?

Yours,
Linus Walleij