Messages in this thread | | | From | Andy Shevchenko <> | Date | Fri, 17 Jun 2022 18:54:14 +0200 | Subject | Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver |
| |
On Fri, Jun 17, 2022 at 4:22 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote: > > On Thu, Jun 16, 2022 at 08:38:46PM +0200, Andy Shevchenko wrote: > > On Thu, Jun 16, 2022 at 7:02 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote: > > > On Thu, Jun 16, 2022 at 02:18:52PM +0200, Andy Shevchenko wrote: > > > > On Thu, Jun 16, 2022 at 12:42 PM Dmitry Rokosov > > > > <DDRokosov@sberdevices.ru> wrote:
...
> > > > > + wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz; > > > > > > > > This looks very odd from a physics perspective: sec * sec * sec == sec ?! > > > > > > > > Perhaps you meant some HZ* macros from units.h? > > > > > > I suppose because of UHZ calculation I have to use NANO instead of > > > USEC_PER_SEC in the following line: > > > > > > freq_uhz = msa311_odr_table[odr].val * USEC_PER_SEC + > > > msa311_odr_table[odr].val2; > > > > > > But below line is right from physics perspective. 1sec = 1/Hz, so > > > msec = (USEC_PER_SEC / freq_uhz) * MSEC_PER_SEC:
I believe the first one should be HZ_PER_MHZ, then it will be fine.
> > > wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz; > > > > > > Or do you mean that I should change MSEC_PER_SEC to just MILLI? > > > > 1 / Hz = 1 sec. That's how physics defines it. Try to figure out what > > you meant by above multiplications / divisions and come up with the > > best that fits your purposes. > > From my point of view, I've already implemented the best way to calculate > how much time I need to wait for the next data chunk based on ODR Hz > value :-) > > ODR value from the table has val integer part and val2 in microHz. > By this line we calculate microHz conversion to take into account val2 > part: > > freq_uhz = msa311_odr_table[odr].val * USEC_PER_SEC + > msa311_odr_table[odr].val2; > > By the next line we try to calculate miliseconds for msleep() from ODR > microHz value: > > wait_ms = (USEC_PER_SEC * MSEC_PER_SEC) / freq_uhz; > > (USEC_PER_SEC / freq_uhz) => seconds
> seconds * MSEC_PER_SEC => milliseconds>
> USEC_PER_SEC and MSEC_PER_SEC are just coefficients, they are not > measured in "seconds" units.
Nope, it's a mistake. Those multipliers imply the unit. The rest are the numbers. See above how to fix this (as far as I can tell).
...
> > > > > + if (err) { > > > > > + dev_err(dev, "cannot update freq (%d)\n", err); > > > > > + goto failed; > > > > > + } > > > > > > > > Why is this inside the loop and more important under lock? Also you > > > > may cover the initial error code by this message when moving it out of > > > > the loop and lock. > > > > > > > > Ditto for other code snippets in other function(s) where applicable. > > > > > > Yes, I can move dev_err() outside of loop. But all ODR search loop > > > should be under lock fully, because other msa311 operations should not > > > be executed when we search proper ODR place. > > > > I didn't suggest getting rid of the lock.
> Sorry, I didn't get you... But I fully agree with you about dev_err() > movement.
Yes, that's what I'm talking about. The dev_err() should be outside of critical section, for example:
mutex_unlock(); if (ret) { dev_err(...); return ret; } ... return 0;
-- With Best Regards, Andy Shevchenko
| |