On Fri, Aug 19, 2022 at 11:41:55AM +0300, Andy Shevchenko wrote: [...] > > 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. As I mentioned before, if I use Datasheet tag, line length limit will be exceeded. If it's okay, I don't mind. [...] > > + char chip_name[10]; > > Why limit it to 10? You may use devm_kasprintf() Oh, didn't know about that. Thank you for suggestion! > > > + struct iio_trigger *new_data_trig; > > + struct regulator *vdd; > > +}; > > ... > > > +static inline int msa311_set_odr(struct msa311_priv *msa311, unsigned int odr) > > Why inline? > Why not? :) It's short function which is good to be inline, I think. > > +{ > > + 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; > No problem. > > + 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. > Agreed. > > + 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: > Yes, usually I'm trying to fit the same rules. Will rework this place. > > + 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? > Oops > > + 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. > You are right. I see that i2c_register_driver initialize .owner to THIS_MODULE. Thank you for pointing. [...] -- Thank you, Dmitry
