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
