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

["Nuno Sá" <noname.nuno@gmail.com>]

On Tue, 2024-11-19 at 07:55 +0200, Matti Vaittinen wrote:
> On 15/11/2024 09:44, Nuno Sá wrote:
> > 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@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).
> > 
> > Well, AFAIK, async probing is using the async.c API which is based on
> > workers.
> 
> Yes.
> 
> > 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?
> 
> I thought there can be more than one concurrent active work items? It 
> would be surprizing to me if aynchronous probe would block other probes. 
> Please, let me know if this is the case.

Not my point... Naturally it won't as every async schedule has it's own worker.
Still a waste of time having a thread sleeping just because.

> 
> > > > > > > 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.
> 
> This could be true. Especially if every other "thing" which needs some 
> delay (but has no strict lower limit) defaults to hrtimers.
> 
> > 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.
> 
> This is exactly why I questioned the rule-of-thumb. I deeply dislike 
> "rules of thumb" - when I don't understand the rationale. If we assume 
> hrtimers came without a cost, then we should have no need for msleep() 
> at all, right?
> 

Well that can very well be just because hrtimers came after and no one really
cares about changing all of the existing msleep() and friends.

> Everything I read suggests the msleep() is actually lighter (but with 
> the downside it can't guarantee short timeouts). Hence I have preferred 
> it when short timeout does _not_ need to be guaranteed. (I still very 
> much understand the checkpatch warn because one might very well assume 
> msleep() could be used to sleep 1 ms).

Depends on the perspective. If you end up sleeping 18ms more than needed and
adding the fact that your handler might not even need any extra IRQ...
 
> 
> After all this discussing, I don't really see point of switching to 
> fsleep() - unless delaying of the (asynchronous) probe proves to be a 
> real problem. If it does, then my assumption that the short timeout does 
> not need to be guaranteed is false and this should be changed.
> 

Sure and I was doing a suggestion anyways. I feel we're bikeshedding now so
let's agree on disagree :)

- Nuno Sá