linux-kernel - Re: [PATCH v5 6/7] 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:   Sun, 14 Aug 2022 17:41:22 +0100
From:   Jonathan Cameron <jic23@...nel.org>
To:     Dmitry Rokosov <DDRokosov@...rdevices.ru>
Cc:     "akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
        "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "andriy.shevchenko@...ux.intel.com" 
        <andriy.shevchenko@...ux.intel.com>,
        "andy.shevchenko@...il.com" <andy.shevchenko@...il.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 v5 6/7] iio: add MEMSensing MSA311 3-axis accelerometer
 driver

On Fri, 12 Aug 2022 16:52:29 +0000
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.
> 
> Spec: https://cdn-shop.adafruit.com/product-files/5309/MSA311-V1.1-ENG.pdf
> 
> 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
> 
> Signed-off-by: Dmitry Rokosov <ddrokosov@...rdevices.ru>
Hi Dmitry, a few minor comments from another read through.

> + */
> +static inline int msa311_set_odr(struct msa311_priv *msa311, unsigned int odr)
> +{
> +	struct device *dev = msa311->dev;
> +	unsigned int pwr_mode;
> +	bool good_odr = false;
> +	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);
> +
> +	if (!good_odr) {
> +		dev_err(dev,
> +			"cannot set odr %u.%luHz, not available in %s mode\n",

Don't you need to zero pad the fractional part?
or use iio_format_values.

> +			msa311_odr_table[odr].val,
> +			msa311_odr_table[odr].val2 / MILLIHZ_PER_HZ,
> +			msa311_pwr_modes[pwr_mode]);
> +		return -EINVAL;
> +	}
> +
> +	return regmap_field_write(msa311->fields[F_ODR], odr);
> +}


> +
> +static int msa311_setup_interrupts(struct msa311_priv *msa311)
> +{
> +	struct device *dev = msa311->dev;
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +	struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
> +	struct iio_trigger *trig;
> +	int err;
> +
> +	/* Keep going without interrupts if no initialized I2C irq */
> +	if (i2c->irq <= 0)
> +		return 0;
> +
> +	err = devm_request_threaded_irq(&i2c->dev, i2c->irq,
> +					NULL, msa311_irq_thread,
> +					IRQF_ONESHOT,
> +					msa311->chip_name,
> +					indio_dev);

Could wrap this a little less given you are respinning anyway. It's way under
80 chars currently.

...
	     "cannot map new data interrupt\n");
> +
> +	return 0;
> +}
> +

> +	msa311->vdd = devm_regulator_get_optional(dev, "vdd");
> +	if (IS_ERR(msa311->vdd)) {
> +		err = PTR_ERR(msa311->vdd);
> +		if (err == -ENODEV)
> +			msa311->vdd = NULL;
> +		else
> +			return dev_err_probe(dev, PTR_ERR(msa311->vdd),

use err instead of PTR_ERR(msa311->vdd) ?

> +					     "cannot get vdd supply\n");
> +	}
> +
> +	if (msa311->vdd) {
> +		err = regulator_enable(msa311->vdd);
> +		if (err)
> +			return dev_err_probe(dev, err,
> +					     "cannot enable vdd supply\n");
> +
> +		err = devm_add_action_or_reset(dev, msa311_vdd_disable,
> +					       msa311->vdd);
> +		if (err) {
> +			return dev_err_probe(dev, err,
> +					     "cannot add vdd disable action\n");
> +		}
> +	}
> +
> +	err = msa311_check_partid(msa311);
> +	if (err)
> +		return err;
> +
> +	err = msa311_soft_reset(msa311);
> +	if (err)
> +		return err;
> +