| 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
| ||
|
Message-ID: <CAHp75VewJ1taLhsCb4_yEQHpw4VDXRhkxpL0jzdu-JsajfF6oA@mail.gmail.com>
Date: Fri, 19 Aug 2022 11:41:55 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Dmitry Rokosov <DDRokosov@...rdevices.ru>
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 Tue, Aug 16, 2022 at 10:18 PM Dmitry Rokosov
<DDRokosov@...rdevices.ru> 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
Powered by blists - more mailing lists