lkml.org 
[lkml]   [2022]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v6 3/4] iio: add MEMSensing MSA311 3-axis accelerometer driver
Date
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
\
 
 \ /
  Last update: 2022-08-19 15:00    [W:0.074 / U:0.112 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site