Message-ID: <b0a9eecb7b83c29aa545ed7717e3a6c2275b5e27.camel@gmail.com>
Date: Thu, 14 Nov 2024 13:26:01 +0100
From: Nuno Sá <noname.nuno@...il.com>
To: Matti Vaittinen <mazziesaccount@...il.com>, Matti Vaittinen
	 <matti.vaittinen@...rohmeurope.com>
Cc: Jonathan Cameron <jic23@...nel.org>, Lars-Peter Clausen
 <lars@...afoo.de>, 	linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] iio: accel: kx022a: Improve reset delay

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@...il.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.

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

- Nuno Sá

Powered by blists - more mailing lists