| 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
| ||
|
Date: Tue, 21 Jul 2020 22:46:34 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Nishant Malpani <nish.malpani25@...il.com>
Cc: Jonathan Cameron <jic23@...nel.org>,
"Bogdan, Dragos" <dragos.bogdan@...log.com>,
darius.berghe@...log.com,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
linux-iio <linux-iio@...r.kernel.org>
Subject: Re: [PATCH v2 1/2] iio: gyro: Add driver support for ADXRS290
On Tue, Jul 21, 2020 at 9:20 PM Nishant Malpani
<nish.malpani25@...il.com> wrote:
>
> ADXRS290 is a high performance MEMS pitch and roll (dual-axis in-plane)
> angular rate sensor (gyroscope) designed for use in stabilization
> applications. It also features an internal temperature sensor and
> programmable high-pass and low-pass filters.
>
> Add support for ADXRS290 in direct-access mode for now.
> Datasheet:
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXRS290.pdf
Drop that 'Link:' part and followed blank line, so get something like
Datasheet: https://...
Signed-off-by: ...
> Signed-off-by: Nishant Malpani <nish.malpani25@...il.com>
...
> +config ADXRS290
> + tristate "Analog Devices ADXRS290 Dual-Axis MEMS Gyroscope SPI driver"
> + depends on SPI
> + help
> + Say yes here to build support for Analog Devices ADXRS290 programmable
> + digital output gyroscope.
> +
> + This driver can also be built as a module. If so, the module will be
> + called adxrs290.
> +enum adxrs290_mode {
> + STANDBY,
> + MEASUREMENT,
> +};
> +struct adxrs290_state {
> + struct spi_device *spi;
> + /* Serialize reads and their subsequent processing */
> + struct mutex lock;
> + enum adxrs290_mode mode;
> + unsigned int lpf_3db_freq_idx;
> + unsigned int hpf_3db_freq_idx;
> +};
...
> +/*
> + * Available cut-off frequencies of the low pass filter in Hz.
> + * The integer part and fractional part are represented separately.
> + */
> +static const int adxrs290_lpf_3db_freq_tbl[][2] = {
What about adxrs290_lpf_3db_freq_hz_table ?
> +};
> +
> +/*
> + * Available cut-off frequencies of the high pass filter in Hz.
> + * The integer part and fractional part are represented separately.
> + */
> +static const int adxrs290_hpf_3db_freq_tbl[][2] = {
Ditto.
> +};
...
> +static int adxrs290_get_rate_data(struct iio_dev *indio_dev, const u8 cmd,
> + unsigned int *val)
> +{
> + struct adxrs290_state *st = iio_priv(indio_dev);
> + int ret = 0;
Purpose of this?
> + u16 temp;
> +
> + mutex_lock(&st->lock);
> + temp = spi_w8r16(st->spi, cmd);
> + if (temp < 0) {
How can this ever happen?
> + ret = temp;
> + goto err_unlock;
> + }
> +
> + *val = temp;
> +
> +err_unlock:
> + mutex_unlock(&st->lock);
> + return ret;
> +}
Ditto for the rest of the similar cases.
...
> + case IIO_CHAN_INFO_SCALE:
> + switch (chan->type) {
> + case IIO_ANGL_VEL:
> + *val = 0;
> + *val2 = 87266;
Magic!
> + return IIO_VAL_INT_PLUS_NANO;
> + case IIO_TEMP:
> + *val = 100;
Magic!
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
...
> + *vals = (const int *)adxrs290_lpf_3db_freq_tbl;
Why casting?
...
> + *vals = (const int *)adxrs290_hpf_3db_freq_tbl;
Ditto.
...
> + struct iio_dev *indio_dev;
> + struct adxrs290_state *st;
> + int ret;
> + u8 val, val2;
Swap them to have in reversed spruce tree order.
...
> + indio_dev->dev.parent = &spi->dev;
Do you need this?
> + /* max transition time to measurement mode */
> + msleep_interruptible(ADXRS290_MAX_TRANSITION_TIME_MS);
I'm not sure what the point of interruptible variant here?
...
> +static const struct of_device_id adxrs290_of_match[] = {
> + { .compatible = "adi,adxrs290" },
> + { },
No comma here!
> +};
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists