Re: [PATCH v4 2/3] iio: add MEMSensing MSA311 3-axis accelerometer driver
From: Andy Shevchenko
Date: Wed Aug 03 2022 - 13:50:21 EST
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