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

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Wed, Aug 3, 2022 at 9:16 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:
> On Wed, Aug 03, 2022 at 07:49:33PM +0200, Andy Shevchenko wrote:
> > On Wed, Aug 3, 2022 at 3:11 PM Dmitry Rokosov <DDRokosov@sberdevices.ru> wrote:

...

> > > +static const struct {
> > > +       int val;
> > > +       int val2;
> > > +} msa311_fs_table[] = {
> > > +       {0, 9580}, {0, 19160}, {0, 38320}, {0, 76641}
> > > +};
> >
> > Besides that this struct is defined not only once in the file, this is
> > very well NIH struct s32_fract. Why not use the latter?

> Good point, but looks like this struct is not so popular in the other
> kernel drivers:

It's simply new, it is not about popularity. I would put it as it's
not _yet_ so popular.

...

> grep "s32_fract" -r -l . | wc -l
> 3

Hint: `git grep` much much faster on Git repositories (it goes via
index and not working copy) and see
`git grep -lw s32_fract`

...

> > > +       .cache_type = REGCACHE_RBTREE,
> >
> > Tree hash is good for sparse data, do you really have it sparse (a lot
> > of  big gaps in between)?
>
> I suppose so. MSA311 regmap has ~6 gaps.

Yes and how long is each gap in comparison to the overall space?

...

> > > +       for (fs = 0; fs < ARRAY_SIZE(msa311_fs_table); ++fs)
> >
> > fs++ will work as well.
>
> I would prefer ++fs if you don't mind :)

Why? It's a non-standard pattern, and needs an explanation.

> > > +               /* Do not check msa311_fs_table[fs].val, it's always 0 */
> > > +               if (val2 == msa311_fs_table[fs].val2) {
> > > +                       mutex_lock(&msa311->lock);
> > > +                       err = regmap_field_write(msa311->fields[F_FS], fs);
> > > +                       mutex_unlock(&msa311->lock);
> >
> > > +                       if (err)
> > > +                               dev_err(dev, "cannot update scale (%d)\n", err);
> >
> > This can be done after putting the device back into (auto)sleep, right?
> >
>
> Are you talking about dev_err logging? Sure, it can be moved after
> pm_runtime* calls.

Yes.

> > > +                       break;
> > > +               }
> > > +
> > > +       pm_runtime_mark_last_busy(dev);
> > > +       pm_runtime_put_autosuspend(dev);

...

> > > +       for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); ++odr)
> >
> > odr++ works well also.
>
> I would prefer ++odr if you don't mind :)

See above.

...

> > > +                               dev_err(dev, "cannot update freq (%d)\n", err);
> >
> > frequency
>
> It will be more ugly due 80 symbols restriction.

Nope, it will be understandable by the user. You do code for the user
and then for the developer, right?

...

> > > +               dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid);
> >
> > Useless message.
>
> Why? It's under dynamic debug, so I will see it if I really want to.

So what the point of the _successful_ detection? You already know from
the code, that partid, you know by other means that probe was
successful, why this message is needed? Especially for debugging when
we have initcall_debug option.

...

> > > +               return dev_err_probe(dev, err, "cannot disable set0 intrs\n");
> >
> > interrupts
>
> It will be more ugly due 80 symbols restriction.

As above to here and other cryptic messages. Please think about the users first.

...

> > > +       indio_dev->modes = 0; /* setup buffered mode later */
> >
> > Why explicit assignment to 0? Doesn't kzalloc() do it for you?
>
> kzalloc() will do it for me, of course. Previously, I initialized modes to
> INDIO_DIRECT_MODE to just provide default value for that. Jonathan
> suggested to replace it with 0. I can remove this line at all, no problem.
> I just thought, it's more readable.

You may leave comment without assignment explaining that IIO core will
set the buffered mode.

-- 
With Best Regards,
Andy Shevchenko