Messages in this thread | | | From | Andy Shevchenko <> | Date | Tue, 21 Jul 2020 22:46:34 +0300 | 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@gmail.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@gmail.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
| |