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@xxxxxxxxx>
---
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.
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?
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).
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.
Thanks for the insight :)
Yours,
-- Matti
