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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 3, 2022 at 3:11 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.
>
> 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.

...

> +#include <linux/i2c.h>

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

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

...

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

> + */

...

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

...

> +static const struct {
> +       int val;
> +       int val2;
> +} 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}
> +};

See above.

...

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

> +};

...

> +       .cache_type = REGCACHE_RBTREE,

Tree hash is good for sparse data, do you really have it sparse (a lot
of  big gaps in between)?

> +};

...

> +       .datasheet_name = "ACC_"#axis                                       \

Leave a comma here.

...

> +static const struct iio_chan_spec msa311_channels[] = {
> +       MSA311_ACCEL_CHANNEL(X),
> +       MSA311_ACCEL_CHANNEL(Y),
> +       MSA311_ACCEL_CHANNEL(Z),
> +       IIO_CHAN_SOFT_TIMESTAMP(MSA311_SI_TIMESTAMP)

Ditto.

> +};

...

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

> +                       msa311_pwr_modes[pwr_mode]);

...

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

...

> +       for (fs = 0; fs < ARRAY_SIZE(msa311_fs_table); ++fs)

fs++ will work as well.

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

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

...

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

frequency

...

> +               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@xxxxxxxxxxxxxxx/

...

> +       struct device *dev = msa311->dev;

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

...

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

How is the bit initial assignment used?

...

> +       /*
> +        * We do not check NEW_DATA int status, because of based on

because based on the

> +        * specification it's cleared automatically after a fixed time.
> +        * So just check that is enabled by driver logic.
> +        */

...

> +       /* TODO: check motion interrupts status */

I don't see the patches for this, so I assume they _will_ come in the
nearest future. Otherwise, drop these TODO lines.

...

> +               dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid);

Useless message.

...

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

interrupts

...

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

Ditto.

...

> +               return dev_err_probe(dev, err, "failed to unmap map0 intrs\n");

Ditto.

...

> +               return dev_err_probe(dev, err, "failed to unmap map1 intrs\n");

Ditto.

...

> +       /* Disable all axis by default */

axis...

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

...

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

frequency

...

> +               return dev_err_probe(dev, err, "failed to request irq\n");

IRQ

...

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

trigger

...

> +               return dev_err_probe(dev, err, "can't register newdata trig\n");

Ditto.

...

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

interrupt

...

> +               return dev_err_probe(dev, err, "cannot set active int level\n");

Ditto.

...

> +               return dev_err_probe(dev, err, "cannot latch int\n");

Ditto.

...

> +               return dev_err_probe(dev, err, "cannot reset int\n");

Ditto.

...

> +               return dev_err_probe(dev, err, "cannot map new data int\n");

interrupt

...

> +               return dev_err_probe(dev, -ENOMEM,
> +                                    "iio device allocation failed\n");

IIO

...

> +       indio_dev->modes = 0; /* setup buffered mode later */

Why explicit assignment to 0? Doesn't kzalloc() do it for you?

...

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

IIO trigger buffer

...

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

IIO

-- 
With Best Regards,
Andy Shevchenko



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux