| Message ID | ZzWfXbjaDkFnu_Jg@mva-rohm (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | iio: accel: kx022a: Improve reset delay | expand |
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()? - Nuno Sá
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) 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? Yours, -- Matti
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 :)? > 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). Not a big deal anyways... - Nuno Sá
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. 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. >> 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? > 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. Thanks for the discussion! Yours, -- Matti
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. > > > > > 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á
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). >>>> 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. 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. By the way, thanks for the reviewing work Nuno! :) I appreciate it. Yours, -- Matti
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. 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á
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. >>>>>> 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
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á
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); ret = regmap_read_poll_timeout(data->regmap, data->chip_info->cntl2, val, !(val & KX022A_MASK_SRST),
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(-) base-commit: 20fd1383cd616d61b2a79967da1221dc6cfb8430