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