lkml.org 
[lkml]   [2020]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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

\
 
 \ /
  Last update: 2020-07-21 21:47    [W:0.766 / U:0.292 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site