linux-kernel - Re: [PATCH v6 3/4] iio: add MEMSensing MSA311 3-axis accelerometer driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 19 Aug 2022 12:57:54 +0000
From:   Dmitry Rokosov <DDRokosov@...rdevices.ru>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
CC:     "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "jic23@...nel.org" <jic23@...nel.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "christophe.jaillet@...adoo.fr" <christophe.jaillet@...adoo.fr>,
        "stano.jakubek@...il.com" <stano.jakubek@...il.com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "stephan@...hold.net" <stephan@...hold.net>,
        "daniel.lezcano@...aro.org" <daniel.lezcano@...aro.org>,
        "wsa@...nel.org" <wsa@...nel.org>,
        "lars@...afoo.de" <lars@...afoo.de>,
        "Michael.Hennerich@...log.com" <Michael.Hennerich@...log.com>,
        "jbhayana@...gle.com" <jbhayana@...gle.com>,
        "lucas.demarchi@...el.com" <lucas.demarchi@...el.com>,
        "jani.nikula@...el.com" <jani.nikula@...el.com>,
        "linus.walleij@...aro.org" <linus.walleij@...aro.org>,
        "linux-iio@...r.kernel.org" <linux-iio@...r.kernel.org>,
        kernel <kernel@...rdevices.ru>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 3/4] iio: add MEMSensing MSA311 3-axis accelerometer
 driver

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