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

From: "Matti Lehtimäki" <matti.lehtimaki@gmail.com>
To: Jonathan Cameron <jic23@kernel.org>
Cc: linux-iio@vger.kernel.org, ~postmarketos/upstreaming@lists.sr.ht,
	Lars-Peter Clausen <lars@metafoo.de>,
	Miquel Raynal <miquel.raynal@bootlin.com>,
	Linus Walleij <linus.walleij@linaro.org>,
	Alexandru Ardelean <aardelean@deviqon.com>,
	Cai Huoqing <cai.huoqing@linux.dev>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] iio: st_sensors: Retry ID verification on failure
Date: Sun, 31 Jul 2022 21:51:55 +0300	[thread overview]
Message-ID: <6c839ba3-b671-76fb-95e1-94bf2f2da303@gmail.com> (raw)
In-Reply-To: <20220731170057.2b8ac00e@jic23-huawei>

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