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

On Wed, Aug 3, 2022 at 9:16 PM Dmitry Rokosov [off-list ref] wrote:

On Wed, Aug 03, 2022 at 07:49:33PM +0200, Andy Shevchenko wrote:

quoted

On Wed, Aug 3, 2022 at 3:11 PM Dmitry Rokosov [off-list ref] wrote:

...

quoted

quoted

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

...

quoted

quoted

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

...

quoted

quoted

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

quoted

quoted

+               /* 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);

quoted

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

quoted

quoted

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

...

quoted

quoted

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

...

quoted

quoted

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

...

quoted

quoted

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

...

quoted

quoted

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

...

quoted

quoted

+       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