Re: [PATCH v3 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver

[Andy Shevchenko]

On Thu, Jun 16, 2022 at 7:02 PM Dmitry Rokosov <DDRokosov@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> wrote:

...

> > Not sure why you put those blank lines herey, it makes code not compact.
>
> Here I use blank lines to split fields from different registers.
> In other words, in the msa311_fields enum one line contains fields from one
> register. But for some heavy registers (like TAP_ACTIVE_STS) we have so many
> fields and their declaration doesn't fit to 80 symbols.
> So I've made a decision to split registers using blank lines.

Better is to add a comment explaining what register is described
below, and not just a blank line.

...

> > Not sure you need this. Dropping i2c dependency from this structure
> > allows much easier to add, e.g. SPI support of the same hardware.
>
> Mainly I use i2c pointer in the probe() path, and you are right, we can
> change i2c pointer to dev and generalize msa311_priv struct from bus
> perspective.

Yep, note that you may easily retrieve i2c_client from struct device
pointer if you need to do that in some (I believe rare to none) cases.

...

> > > +       struct regmap *regs;
> >
> > I believe this is used most, so making this a first member in the
> > structure saves  some instructions (check with bloat-o-meter).
> >
>
> Are you talking about archs where offset calculation adds more bytes to
> instruction? And when offset equals to 0, we can save some space.

It doesn't have anything to do with arches, simply compiler
optimization, otherwise yes, that's what I meant.

...

> > > +       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:
>
>         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.

...

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

...

> > > +       mutex_lock(&msa311->lock);
> > > +       err = regmap_field_write(msa311->fields[F_NEW_DATA_INT_EN], state);
> > > +       mutex_unlock(&msa311->lock);
> >
> > > +
> >
> > No need.
>
> Sorry, I don't understand. We do not need to call it under lock, right?
> I think we have to wrap it by msa311 lock, because other msa311
> operations should not be executed when we enable or disable new data
> interrupt (for example ODR value changing or something else).

The blank line is not needed, I specifically commented on the
emphasized paragraph (by delimiting it with blank lines and leaving
the rest for the better context for you to understand, it seems it did
the opposite...).

...

> > > +       mutex_lock(&msa311->lock);
> > > +       err = msa311_set_pwr_mode(msa311, MSA311_PWR_MODE_NORMAL);
> > > +       mutex_unlock(&msa311->lock);
> >
> > > +
> >
> > No need.
>
> Again I don't understand why, sorry. We do not want to get sporadic
> MSA311 attributes changing during power mode transition from another
> userspace process.

As per above.

-- 
With Best Regards,
Andy Shevchenko