Re: [PATCH v6 3/4] iio: add MEMSensing MSA311 3-axis accelerometer driver

On Fri, Aug 19, 2022 at 11:41:55AM +0300, Andy Shevchenko wrote:

[...]

quoted

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

Have you ever seen such a tag?
We have the Datasheet that is more or less established for this kind of links.

As I mentioned before, if I use Datasheet tag, line length limit will be
exceeded. If it's okay, I don't mind.

[...]

quoted

+       char chip_name[10];

Why limit it to 10? You may use devm_kasprintf()

Oh, didn't know about that. Thank you for suggestion!

quoted

+       struct iio_trigger *new_data_trig;
+       struct regulator *vdd;
+};

...

quoted

+static inline int msa311_set_odr(struct msa311_priv *msa311, unsigned int odr)

Why inline?

Why not? :) It's short function which is good to be inline, I think.

quoted

+{
+       struct device *dev = msa311->dev;
+       unsigned int pwr_mode;

quoted

+       bool good_odr = false;

Can it be split to see the assignments together below?

quoted

+       int err;
+
+       err = regmap_field_read(msa311->fields[F_PWR_MODE], &pwr_mode);
+       if (err)
+               return err;
+
+       /* Filter bad ODR values */
+       if (pwr_mode == MSA311_PWR_MODE_NORMAL)
+               good_odr = (odr > MSA311_ODR_1_95_HZ);

else
  good_odr = false;

No problem.

quoted

+       if (!good_odr) {
+               dev_err(dev,
+                       "can't set odr %u.%06uHz, not available in %s mode\n",
+                       msa311_odr_table[odr].integral,
+                       msa311_odr_table[odr].microfract,
+                       msa311_pwr_modes[pwr_mode]);
+               return -EINVAL;
+       }
+
+       return regmap_field_write(msa311->fields[F_ODR], odr);
+}

...

quoted

+       if (wait_ms < unintr_thresh_ms)
+               usleep_range(wait_ms * USEC_PER_MSEC,
+                            unintr_thresh_ms * USEC_PER_MSEC);
+       else
+               return msleep_interruptible(wait_ms) ? -EINTR : 0;

Can be refactored to simple

else if (msleep...)
  return -EINTR;

Same amount of LoCs, but more readable.

Agreed.

quoted

+       return 0;

...

quoted

+err:

We usually name labels after what they are doing, I don't see any
error here, but notify done. Hence,

out_notify_done:

Yes, usually I'm trying to fit the same rules. Will rework this place.

quoted

+       iio_trigger_notify_done(indio_dev->trig);
+
+       return IRQ_HANDLED;

...

quoted

+       used = scnprintf(msa311->chip_name, sizeof(msa311->chip_name),
+                        "msa311-%hhx", partid);

How 'used' is being used?

Oops

quoted

+       return 0;
+}

...

quoted

+       const char zero_bulk[2] = {0};

0 is not needed, '{ }' will work.

...

quoted

+       /*
+        * Register powerdown deferred callback which suspends the chip
+        * after module unloaded.
+        *
+        * MSA311 should be in SUSPEND mode in the two cases:
+        * 1) When driver is loaded, but we do not have any data or
+        *    configuration requests to it (we are solving it using
+        *    autosuspend feature).
+        * 2) When driver is unloaded and device is not used (devm action is
+        *    used in this case).
+        */

...

quoted

+static struct i2c_driver msa311_driver = {
+       .driver = {
+               .name = "msa311",

quoted

+               .owner = THIS_MODULE,

Redundant.

You are right. I see that i2c_register_driver initialize .owner to
THIS_MODULE. Thank you for pointing.

[...]

-- 
Thank you,
Dmitry