Hello Andy, Thank you for quick review, On Wed, Aug 03, 2022 at 07:49:33PM +0200, Andy Shevchenko wrote: > 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. Sure, no problem. > > ... > > > +/* > > + * 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. > Agree, will change in the v5. > > + */ > > ... > > > +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? > > ... > Good point, but looks like this struct is not so popular in the other kernel drivers: === grep "s32_fract" -r -l . | wc -l 3 === [...] > > +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. > Good catch, forgot it. > > +}; > > ... > > > + .cache_type = REGCACHE_RBTREE, > > Tree hash is good for sparse data, do you really have it sparse (a lot > of big gaps in between)? I suppose so. MSA311 regmap has ~6 gaps. [...] > > > + 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 ? MILLIHZ_PER_HZ :) > > > + 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? > > ... > I compare unintr_thresh_ms with wait_ms and use wait_ms with msleep_interruptible(), and I think 20ms is more readable than 20000us or 20 * USEC_PER_MSEC us. But I agree about sign. It should be unsigned anyway. > > + for (fs = 0; fs < ARRAY_SIZE(msa311_fs_table); ++fs) > > fs++ will work as well. > I would prefer ++fs if you don't mind :) > > + /* 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? > Are you talking about dev_err logging? Sure, it can be moved after pm_runtime* calls. > > + 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. I would prefer ++odr if you don't mind :) > > ... > > > + dev_err(dev, "cannot update freq (%d)\n", err); > > frequency It will be more ugly due 80 symbols restriction. > > ... > > > + 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/ Oh, really... Thank you for suggestion! > > ... > > > + struct device *dev = msa311->dev; > > Isn't it the same as indio_dev->dev.parent? > Do you really need that member? Should be the same... I will check. > > ... > > > + int bit = 0, err, i = 0; > > How is the bit initial assignment used? You are right, for for_each_set_bit() initializes bit inside. [...] > > + dev_dbg(dev, "found MSA311 compatible chip[%#x]\n", partid); > > Useless message. > Why? It's under dynamic debug, so I will see it if I really want to. > ... > > > + return dev_err_probe(dev, err, "cannot disable set0 intrs\n"); > > interrupts It will be more ugly due 80 symbols restriction. [...] > > + /* 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? Axes of course. > > ... > > > + return dev_err_probe(dev, err, "failed to set accel freq\n"); > > frequency > It will be more ugly due 80 symbols restriction. [...] > > + return dev_err_probe(dev, -ENOMEM, > > + "cannot allocate newdata trig\n"); > > trigger It will be more ugly due 80 symbols restriction. [...] > > + return dev_err_probe(dev, err, "cannot enable push-pull int\n"); > > interrupt It will be more ugly due 80 symbols restriction. > > + indio_dev->modes = 0; /* setup buffered mode later */ > > Why explicit assignment to 0? Doesn't kzalloc() do it for you? kzalloc() will do it for me, of course. Previously, I initialized modes to INDIO_DIRECT_MODE to just provide default value for that. Jonathan suggested to replace it with 0. I can remove this line at all, no problem. I just thought, it's more readable. > > ... > > > + return dev_err_probe(dev, err, "cannot setup iio trig buf\n"); > > IIO trigger buffer It will be more ugly due 80 symbols restriction. > > ... > > > + return dev_err_probe(dev, err, "iio device register failed\n"); > > IIO > > -- > With Best Regards, > Andy Shevchenko -- Thank you, Dmitry
