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
