Message-ID: <9073554f353273d0aa99a7aebfc5f367cfaa7c1a.camel@gmail.com>
Date: Fri, 15 Nov 2024 08:44:04 +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 Fri, 2024-11-15 at 08:20 +0200, Matti Vaittinen wrote:
> 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@...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.
> 
> 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).

Well, AFAIK, async probing is using the async.c API which is based on workers.
If you spend (worst case scenario) 18ms more than you need in the handler (and
18ms is __huge__), it means that worker can't go on and do some other useful
stuff, right?

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

Not that it's even every likely that you're not adding a new interrupt
necessarily. That's the point of the range in usleep(). So that multiple
handlers can be done in one interrupt.

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

Put it this way... if that was true, I would assume it would be somewhere
described in the sleeping docs. More, I don't think the rule of thumb would be
to use hrtirmers for things < 20ms.

- Nuno Sá

Powered by blists - more mailing lists