Re: [PATCH] iio: accel: kx022a: Improve reset delay

[Matti Vaittinen <mazziesaccount@gmail.com>]

On 14/11/2024 14:26, Nuno Sá wrote:
> On Thu, 2024-11-14 at 13:30 +0200, Matti Vaittinen wrote:
>> On 14/11/2024 12:46, Nuno Sá wrote:
>>> On Thu, 2024-11-14 at 11:54 +0200, Matti Vaittinen wrote:
>>>> On 14/11/2024 11:43, Nuno Sá wrote:
>>>>> On Thu, 2024-11-14 at 08:57 +0200, Matti Vaittinen wrote:
>>>>>> All the sensors supported by kx022a driver seemed to require some
>>>>>> delay
>>>>>> after software reset to be operational again. More or less a random
>>>>>> msleep(1) was added to cause the driver to go to sleep so the sensor
>>>>>> has
>>>>>> time to become operational again.
>>>>>>
>>>>>> Now we have official docuumentation available:
>>>>>> https://fscdn.rohm.com/kionix/en/document/AN010_KX022ACR-Z_Power-on_Procedure_E.pdf
>>>>>> https://fscdn.rohm.com/kionix/en/document/TN027-Power-On-Procedure.pdf
>>>>>> https://fscdn.rohm.com/kionix/en/document/AN011_KX134ACR-LBZ_Power-on_Procedure_E.pdf
>>>>>>
>>>>>> stating the required time is 2 ms.
>>>>>>
>>>>>> Due to the nature of the current msleep implementation, the msleep(1)
>>>>>> is
>>>>>> likely to be sleeping more than 2ms already - but the value "1" is
>>>>>> misleading in case someone needs to optimize the start time and change
>>>>>> the msleep to a more accurate delay. Hence it is better for
>>>>>> "documentation" purposes to use value which actually reflects the
>>>>>> specified 2ms wait time.
>>>>>>
>>>>>> Change the value of delay after software reset to match the
>>>>>> specifications and add links to the power-on procedure specifications.
>>>>>>
>>>>>> Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
>>>>>> ---
>>>>>> Sorry for not including this to the KX134ACR-LBZ series I sent
>>>>>> yesterday. It was only half an hour after I had sent the KX134ACR-LBZ
>>>>>> support when I was notified about the existence of the KX022ACR-Z
>>>>>> start-up procedure specification... Hence this lone patch to code
>>>>>> which
>>>>>> I just sent a miscallaneous series for before.
>>>>>>
>>>>>>     drivers/iio/accel/kionix-kx022a.c | 11 ++++++++---
>>>>>>     1 file changed, 8 insertions(+), 3 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iio/accel/kionix-kx022a.c
>>>>>> b/drivers/iio/accel/kionix-
>>>>>> kx022a.c
>>>>>> index 32387819995d..ccabe2e3b130 100644
>>>>>> --- a/drivers/iio/accel/kionix-kx022a.c
>>>>>> +++ b/drivers/iio/accel/kionix-kx022a.c
>>>>>> @@ -1121,10 +1121,15 @@ static int kx022a_chip_init(struct kx022a_data
>>>>>> *data)
>>>>>>     		return ret;
>>>>>>     
>>>>>>     	/*
>>>>>> -	 * I've seen I2C read failures if we poll too fast after the
>>>>>> sensor
>>>>>> -	 * reset. Slight delay gives I2C block the time to recover.
>>>>>> +	 * According to the power-on procedure documents, there is
>>>>>> (at
>>>>>> least)
>>>>>> +	 * 2ms delay required after the software reset. This should
>>>>>> be
>>>>>> same
>>>>>> for
>>>>>> +	 * all, KX022ACR-Z, KX132-1211, KX132ACR-LBZ and KX134ACR-
>>>>>> LBZ.
>>>>>> +	 *
>>>>>> +	 *
>>>>>> https://fscdn.rohm.com/kionix/en/document/AN010_KX022ACR-Z_Power-on_Procedure_E.pdf
>>>>>> +	 *
>>>>>> https://fscdn.rohm.com/kionix/en/document/TN027-Power-On-Procedure.pdf
>>>>>> +	 *
>>>>>> https://fscdn.rohm.com/kionix/en/document/AN011_KX134ACR-LBZ_Power-on_Procedure_E.pdf
>>>>>>     	 */
>>>>>> -	msleep(1);
>>>>>> +	msleep(2);
>>>>>
>>>>> msleep() is not advisable for something lower than 20ms. Maybe take the
>>>>> opportunity and change it to fsleep()?
>>>>
>>>> Thank you for the suggestion Nuno. I did originally consider using the
>>>> usleep_range() since the checkpatch knows to warn about msleep with
>>>> small times.
>>>>
>>>> However, there should be no rush to power-on the sensor at startup. It
>>>> usually does not matter if the sleep is 2 or 20 milli seconds, as long
>>>> as it is long enough. I wonder if interrupting the system with hrtimers
>>>> for _all_ smallish delays (when the longer delay would not really hurt)
>>>
>>> That's why you have ranges of about 20% (I think) in usleep() so you
>>> minimize
>>> hrtimers interrupts.
>>>
>>> Other thing is boot time... Sleeping 20ms instead of 2ms is a huge
>>> difference.
>>> Imagine if everyone thought like this for small sleeps :)?
>>
>> I think this is interesting question. My thoughts were along the line
>> that, even if small sleeps were extended to longer (where small sleep is
>> not a priority), the CPUs would still (especially during the boot up)
>> have their hands full. I don't know if we might indeed end up a
>> situation where CPUs were idling, waiting for next timer slot.
> 
> My problem is not the CPU but delaying probing devices as you probe one device
> at time...
> 
>>
>> What comes to boot time, I doubt the CPUs run out of things to do,
>> especially when we use the probe_type = PROBE_PREFER_ASYNCHRONOUS.
> 
> Yeah, with this, the above does not apply. Still, spending more time in a worker
> than needed (and 18ms is huge) seems a waste to me.

This is likely to be my ignorance, but I don't know what is wasted here. 
(genuine question, not trying to be a smart-ass).

>>>> is a the best design choice. Hence I'd rather keep the msleep when we
>>>> don't need to guarantee delay to be short instead of defaulting to
>>>> hrtimers or even busy-loop when it is not required.
>>>>
>>>> Do you think I am mistaken?
>>>>
>>>
>>> To me this is more about correctness and do what the docs tell us to do :).
>>> Sure, here you know what you're doing and you don't care if you end up
>>> sleeping
>>> more than 2ms but that's not always the case and code like this allows for
>>> legit
>>> mistakes (if someone just copy paste this for example).
>>
>> Right. I just wonder if always requiring stricter wake-up instead of
>> allowing things to run uninterrupted is the best role model either?
> 
> Why not :)? If we just need to wait 2ms, why waiting more? I would be very
> surprised if hrtimers are a deal breaker in here. Otherwise, we should remove it
> from the docs...

Again I may be wrong, but I think each of the interrupts we add, require 
tiny bit of handling - which I thought is more of a waste than sleeping.

I admit this is all hand-waving as I have no test data to back up my 
pondering. And, I believe you are right that this surely is not a deal 
breaker - but neither do I see adding more interrupts (when not really 
needed) as a good design.

>>> Not a big deal anyways...
>>
>> Agree :) But I think this is a spot where I could learn a bit. I will
>> gladly switch to the fsleep() if someone explains me relying on hrtimers
>> should be preferred also when there is no real need to wake up quicker
>> than msleep() allows.
>>
> 
> Personally, I think that sleeping more than needed is always a wast and then it
> comes back to my correctness comment. In here you know what you're doing but I
> dunno that switching to hrtimers will do any arm to the device :) and allows
> proper patterns to be copied.

I have been thinking that handling the (hrtimer) interrupts generates 
more overhead (waste) than sleeping.

By the way, thanks for the reviewing work Nuno! :) I appreciate it.

Yours,
     -- Matti