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

[Andy Shevchenko]

On Tue, Aug 16, 2022 at 10:18 PM Dmitry Rokosov
<DDRokosov@xxxxxxxxxxxxxx> wrote:
>
> 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.
>
> 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

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

The comments below are not a big deal, though.

...

> +static const struct iio_decimal_fract msa311_fs_table[] = {
> +       {0, 9580}, {0, 19160}, {0, 38320}, {0, 76641}

I would still keep the trailing comma for better consistency with a style.

> +};

...

> +static const struct iio_decimal_fract msa311_odr_table[] = {
> +       {1, 0}, {1, 950000}, {3, 900000}, {7, 810000}, {15, 630000},
> +       {31, 250000}, {62, 500000}, {125, 0}, {250, 0}, {500, 0}, {1000, 0}

Ditto.

> +};

...

> +struct msa311_priv {
> +       struct regmap *regs;
> +       struct regmap_field *fields[F_MAX_FIELDS];
> +
> +       struct device *dev;
> +       struct mutex lock;

> +       char chip_name[10];

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

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

...

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

Why inline?

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

> +       bool good_odr = false;

Can it be split to see the assignments together below?

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

> +       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);
> +}

...

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

> +       return 0;

...

> +err:

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

out_notify_done:

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

...

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

How 'used' is being used?

> +       return 0;
> +}

...

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

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

...

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

...

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

> +               .owner = THIS_MODULE,

Redundant.

> +               .of_match_table = msa311_of_match,
> +               .pm = pm_ptr(&msa311_pm_ops),
> +       },
> +       .probe_new      = msa311_probe,
> +       .id_table       = msa311_i2c_id,
> +};

-- 
With Best Regards,
Andy Shevchenko