Hello Andy,

Thank you for quick review,

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

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

quoted

MSA311 is a tri-axial, low-g accelerometer with I2C digital output for
sensitivity consumer applications. It has dynamic user-selectable full
scales range of +-2g/+-4g/+-8g/+-16g and allows acceleration measurements
with output data rates from 1Hz to 1000Hz.

Spec: https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf

This driver supports following MSA311 features:
    - IIO interface
    - Different power modes: NORMAL and SUSPEND (using pm_runtime)
    - ODR (Output Data Rate) selection
    - Scale and samp_freq selection
    - IIO triggered buffer, IIO reg access
    - NEW_DATA interrupt + trigger

Below features to be done:
    - Motion Events: ACTIVE, TAP, ORIENT, FREEFALL
    - Low Power mode

Thanks for an update, my comments below.

...

quoted

+#include <linux/i2c.h>

quoted

+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>

I would split this group out of generic headers...

quoted

+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/pm.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/string_helpers.h>
+#include <linux/units.h>
+

...and move it here to clearly show that the driver belongs to the IIO
subsystem.

Sure, no problem.

...

quoted

+/*
+ * Possible Full Scale ranges
+ *
+ * Axis data is 12-bit signed value, so
+ *
+ * fs0 = (2 + 2) * 9.81 / (2<<11) = 0.009580
+ * fs1 = (4 + 4) * 9.81 / (2<<11) = 0.019160
+ * fs2 = (8 + 8) * 9.81 / (2<<11) = 0.038320
+ * fs3 = (16 + 16) * 9.81 / (2<<11) = 0.076641

I'm not sure if you are using programming language here or math language?
In math we refer to powers like 2^11, the 2<<11 puzzles me.

Agree, will change in the v5.

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:

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

[...]

quoted

+static const char * const msa311_pwr_modes[] = {
+       [MSA311_PWR_MODE_NORMAL] = "normal",
+       [MSA311_PWR_MODE_LOW] = "low",
+       [MSA311_PWR_MODE_UNKNOWN] = "unknown",
+       [MSA311_PWR_MODE_SUSPEND] = "suspend"

Leave a comma here even if we know that in the nearest future it won't
be changed.

Good catch, forgot it.

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.

[...]

quoted

+               dev_err(dev,
+                       "failed to set odr %u.%uHz, not available in %s mode\n",
+                       msa311_odr_table[odr].val,
+                       msa311_odr_table[odr].val2 / 1000,

KILO ?

MILLIHZ_PER_HZ :)

quoted

+                       msa311_pwr_modes[pwr_mode]);

...

quoted

+       static const int unintr_thresh_ms = 20;

You seem not using it as _ms, but always as _us, why not define accordingly?
Also the other one is defined as unsigned long and this is as int. Why
not unsigned?

...

I compare unintr_thresh_ms with wait_ms and use wait_ms with
msleep_interruptible(), and I think 20ms is more readable than 20000us
or 20 * USEC_PER_MSEC us.

But I agree about sign. It should be unsigned anyway.

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 :)

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.

quoted

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

...

quoted

+       for (odr = 0; odr < ARRAY_SIZE(msa311_odr_table); ++odr)

odr++ works well also.

I would prefer ++odr if you don't mind :)

...

quoted

+                               dev_err(dev, "cannot update freq (%d)\n", err);

frequency

It will be more ugly due 80 symbols restriction.

...

quoted

+               dev_err(dev, "cannot %s register %u from debugfs (%d)\n",
+                       readval ? "read" : "write", reg, err);

You may consider taking [1] as a precursor here and use str_read_write().

[1]: https://lore.kernel.org/linux-i2c/20220703154232.55549-1-andriy.shevchenko@linux.intel.com/ (local)

Oh, really... Thank you for suggestion!

...

quoted

+       struct device *dev = msa311->dev;

Isn't it the same as indio_dev->dev.parent?
Do you really need that member?

Should be the same... I will check.

...

quoted

+       int bit = 0, err, i = 0;

How is the bit initial assignment used?

You are right, for for_each_set_bit() initializes bit inside.

[...]

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.

...

quoted

+               return dev_err_probe(dev, err, "cannot disable set0 intrs\n");

interrupts

It will be more ugly due 80 symbols restriction.

[...]

quoted

+       /* Disable all axis by default */

axis...

quoted

+       err = regmap_update_bits(msa311->regs, MSA311_ODR_REG,
+                                MSA311_GENMASK(F_X_AXIS_DIS) |
+                                MSA311_GENMASK(F_Y_AXIS_DIS) |
+                                MSA311_GENMASK(F_Z_AXIS_DIS), 0);
+       if (err)
+               return dev_err_probe(dev, err, "cannot enable all axes\n");

...or axes?

Axes of course.

...

quoted

+               return dev_err_probe(dev, err, "failed to set accel freq\n");

frequency

It will be more ugly due 80 symbols restriction.

[...]

quoted

+               return dev_err_probe(dev, -ENOMEM,
+                                    "cannot allocate newdata trig\n");

trigger

It will be more ugly due 80 symbols restriction.

[...]

quoted

+               return dev_err_probe(dev, err, "cannot enable push-pull int\n");

interrupt

It will be more ugly due 80 symbols restriction.

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.

...

quoted

+               return dev_err_probe(dev, err, "cannot setup iio trig buf\n");

IIO trigger buffer

It will be more ugly due 80 symbols restriction.

...

quoted

+               return dev_err_probe(dev, err, "iio device register failed\n");

IIO

-- 
With Best Regards,
Andy Shevchenko

-- 
Thank you,
Dmitry