[v2,1/2] iio: gyro: Add driver support for ADXRS290
diff mbox series

Message ID 20200721181926.27046-1-nish.malpani25@gmail.com
State New
Headers show
Series
  • [v2,1/2] iio: gyro: Add driver support for ADXRS290
Related show

Commit Message

Nishant Malpani July 21, 2020, 6:19 p.m. UTC
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

Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
---

Changes in v2:
  - append copyright tag with author's info
  - remove asm/unaligned.h header
  - remove unnecessary comments about the registers' description
  - rephrase comment on the usage of mutex_lock
  - discard the usage of local tx, rx buffers; use DMA-safe buffers
    provided by the SPI core instead
  - utilize spi_w8r16 provided by the SPI core instead of writing a
    wrapper over spi_sync_transfer which semantically does the same
  - equip spi_write_then_read instead of plain spi_write since the
    latter requires a DMA-safe buffer
  - implement exact matching of filter 3db frequencies instead of
    finding the "closest" match; rounding complexity is left to the
    userspace
  - include 'info_mask_shared_by_type_available' when initialising
    iio_chan_spec instead of explicitly exposing attributes
    signifying available filter 3db frequencies; with this we can
    utilize read_avail core callback
---
 MAINTAINERS                 |   6 +
 drivers/iio/gyro/Kconfig    |  10 +
 drivers/iio/gyro/Makefile   |   1 +
 drivers/iio/gyro/adxrs290.c | 444 ++++++++++++++++++++++++++++++++++++
 4 files changed, 461 insertions(+)
 create mode 100644 drivers/iio/gyro/adxrs290.c

Comments

Andy Shevchenko July 21, 2020, 7:46 p.m. UTC | #1
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!

> +};
Nishant Malpani July 21, 2020, 8:35 p.m. UTC | #2
Hello Andy,

Thanks for the review. Comments inline...

On 22/07/20 1:16 am, Andy Shevchenko wrote:
> 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: ...
> 
Okay. Will fix in v3.

>> 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 ?
> 
Sure, makes it very precise. Will address in v3.

>> +};
>> +
>> +/*
>> + * 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.
> 
Yes.

>> +};
> 
> ...
> 
>> +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?
>
'ret' will not be initialized if a successful spi_w8r16() takes place 
i.e if it doesn't go into the 'if' block. (Doesn't make sense to have it 
now since the below block of code is erroneous, but will need this in v3).

>> +       u16 temp;
>> +
>> +       mutex_lock(&st->lock);
>> +       temp = spi_w8r16(st->spi, cmd);
> 
>> +       if (temp < 0) {
> 
> How can this ever happen?
> 
Oops, you're right. Even though spi_w8r16() returns a negative error 
code on failure, 'temp' is declared unsigned. Thanks for pointing out. 
Will fix in v3.

>> +               ret = temp;
>> +               goto err_unlock;
>> +       }
>> +
>> +       *val = temp;
>> +
>> +err_unlock:
>> +       mutex_unlock(&st->lock);
>> +       return ret;
>> +}
> 
> Ditto for the rest of the similar cases.
>
Sure.

> ...
> 
>> +       case IIO_CHAN_INFO_SCALE:
>> +               switch (chan->type) {
>> +               case IIO_ANGL_VEL:
>> +                       *val = 0;
> 
> 
>> +                       *val2 = 87266;
> 
> Magic!
> 
Haha, will add comments in v3!

>> +                       return IIO_VAL_INT_PLUS_NANO;
>> +               case IIO_TEMP:
> 
>> +                       *val = 100;
> 
> Magic!
> 
Will add comments in v3.

>> +                       return IIO_VAL_INT;
>> +               default:
>> +                       return -EINVAL;
>> +               }
> 
> ...
> 
>> +               *vals = (const int *)adxrs290_lpf_3db_freq_tbl;
> 
> Why casting?
> 
adxrs290_lpf_3db_freq_tbl is of type (int *)[2], right? Without the 
casting, an incompatible-pointer-type error is thrown.

> ...
> 
>> +               *vals = (const int *)adxrs290_hpf_3db_freq_tbl;
> 
> Ditto.
> 
See above comment.

> ...
> 
> 
>> +       struct iio_dev *indio_dev;
>> +       struct adxrs290_state *st;
> 
>> +       int ret;
>> +       u8 val, val2;
> 
> Swap them to have in reversed spruce tree order.
> 
Okay, will do so in v3.

> ...
> 
>> +       indio_dev->dev.parent = &spi->dev;
> 
> Do you need this?
Oh, right (I'm aware of Alexandru's recent patch on this). Will address 
in v3.

> 
>> +       /* max transition time to measurement mode */
>> +       msleep_interruptible(ADXRS290_MAX_TRANSITION_TIME_MS);
> 
> I'm not sure what the point of interruptible variant here?
> 
I referred Documentation/timers/timers-howto.rst for this.
My reasoning was shaped to use the interruptible variant because the 
transition settles in a time *less than* 100ms and since 100ms is quite 
a huge time to sleep, it should be interrupted in case a signal arrives.

> ...
> 
>> +static const struct of_device_id adxrs290_of_match[] = {
>> +       { .compatible = "adi,adxrs290" },
> 
>> +       { },
> 
> No comma here!
> 
Okay. Will remove in v3.

With regards,
Nishant Malpani

>> +};
>
Nishant Malpani July 21, 2020, 9:14 p.m. UTC | #3
On 22/07/20 2:05 am, Nishant Malpani wrote:

[...]

>>> +                       return IIO_VAL_INT;
>>> +               default:
>>> +                       return -EINVAL;
>>> +               }
>>
>> ...
>>
>>> +               *vals = (const int *)adxrs290_lpf_3db_freq_tbl;
>>
>> Why casting?
>>
> adxrs290_lpf_3db_freq_tbl is of type (int *)[2], right? Without the 
> casting, an incompatible-pointer-type error is thrown.

Sorry, the above is incorrect. adxrs290_lpf_3db_freq_tbl is a pointer to 
an array of two ints.

[...]

Regards,
Nishant Malpani
Andy Shevchenko July 21, 2020, 9:38 p.m. UTC | #4
On Tue, Jul 21, 2020 at 11:35 PM Nishant Malpani
<nish.malpani25@gmail.com> wrote:
> On 22/07/20 1:16 am, Andy Shevchenko wrote:
> > On Tue, Jul 21, 2020 at 9:20 PM Nishant Malpani
> > <nish.malpani25@gmail.com> wrote:

...

> >> +               *vals = (const int *)adxrs290_lpf_3db_freq_tbl;
> >
> > Why casting?
> >
> adxrs290_lpf_3db_freq_tbl is of type (int *)[2], right? Without the
> casting, an incompatible-pointer-type error is thrown.
>
> > ...
> >
> >> +               *vals = (const int *)adxrs290_hpf_3db_freq_tbl;
> >
> > Ditto.
> >
> See above comment.

Can't you declare table as const int?

...

> >> +       /* max transition time to measurement mode */
> >> +       msleep_interruptible(ADXRS290_MAX_TRANSITION_TIME_MS);
> >
> > I'm not sure what the point of interruptible variant here?
> >
> I referred Documentation/timers/timers-howto.rst for this.
> My reasoning was shaped to use the interruptible variant because the
> transition settles in a time *less than* 100ms and since 100ms is quite
> a huge time to sleep, it should be interrupted in case a signal arrives.

This is probe of the device,
What are the expectations here?
Nishant Malpani July 22, 2020, 9:40 a.m. UTC | #5
On 22/07/20 3:08 am, Andy Shevchenko wrote:
> On Tue, Jul 21, 2020 at 11:35 PM Nishant Malpani
> <nish.malpani25@gmail.com> wrote:
>> On 22/07/20 1:16 am, Andy Shevchenko wrote:
>>> On Tue, Jul 21, 2020 at 9:20 PM Nishant Malpani
>>> <nish.malpani25@gmail.com> wrote:
> 
> ...
> 
>>>> +               *vals = (const int *)adxrs290_lpf_3db_freq_tbl;
>>>
>>> Why casting?
>>>
>> adxrs290_lpf_3db_freq_tbl is of type (int *)[2], right? Without the
>> casting, an incompatible-pointer-type error is thrown.
>>
>>> ...
>>>
>>>> +               *vals = (const int *)adxrs290_hpf_3db_freq_tbl;
>>>
>>> Ditto.
>>>
>> See above comment.
> 
> Can't you declare table as const int?
> 
I'm not sure I understand you completely here; do you mean const int *? 
So, an array of alternate integer and fractional parts? I suppose that's 
possible but we'd be introducing unwanted complexity I feel - for 
example, currently the index of the 3db frequency in the table is used 
to directly map & set bits in the filter register corresponding to that 
frequency but with the approach you share, we'd have to apply a 
transformation (div by 2) to set the same bits in the filter register. 
Do you think the added complexity justifies the removal of the casting?

> ...
> 
>>>> +       /* max transition time to measurement mode */
>>>> +       msleep_interruptible(ADXRS290_MAX_TRANSITION_TIME_MS);
>>>
>>> I'm not sure what the point of interruptible variant here?
>>>
>> I referred Documentation/timers/timers-howto.rst for this.
>> My reasoning was shaped to use the interruptible variant because the
>> transition settles in a time *less than* 100ms and since 100ms is quite
>> a huge time to sleep, it should be interrupted in case a signal arrives.
> 
> This is probe of the device,
> What are the expectations here?
> 
I fail to understand why this can't be used in the probe() but perhaps 
in a routine to standby/resume. Could you please elaborate?

With regards,
Nishant Malpani
Andy Shevchenko July 22, 2020, 10:23 a.m. UTC | #6
On Wed, Jul 22, 2020 at 12:40 PM Nishant Malpani
<nish.malpani25@gmail.com> wrote:
> On 22/07/20 3:08 am, Andy Shevchenko wrote:
> > On Tue, Jul 21, 2020 at 11:35 PM Nishant Malpani
> > <nish.malpani25@gmail.com> wrote:
> >> On 22/07/20 1:16 am, Andy Shevchenko wrote:

...

> > Can't you declare table as const int?
> >
> I'm not sure I understand you completely here; do you mean const int *?
> So, an array of alternate integer and fractional parts? I suppose that's
> possible but we'd be introducing unwanted complexity I feel - for
> example, currently the index of the 3db frequency in the table is used
> to directly map & set bits in the filter register corresponding to that
> frequency but with the approach you share, we'd have to apply a
> transformation (div by 2) to set the same bits in the filter register.
> Do you think the added complexity justifies the removal of the casting?

It was a question. If you think it is too much, don't change :-)

...

> >>>> +       /* max transition time to measurement mode */
> >>>> +       msleep_interruptible(ADXRS290_MAX_TRANSITION_TIME_MS);
> >>>
> >>> I'm not sure what the point of interruptible variant here?
> >>>
> >> I referred Documentation/timers/timers-howto.rst for this.
> >> My reasoning was shaped to use the interruptible variant because the
> >> transition settles in a time *less than* 100ms and since 100ms is quite
> >> a huge time to sleep, it should be interrupted in case a signal arrives.
> >
> > This is probe of the device,
> > What are the expectations here?
> >
> I fail to understand why this can't be used in the probe() but perhaps
> in a routine to standby/resume. Could you please elaborate?

I didn't say it can not be used, what I'm asking is what are the
expectations of the interruptible part here.
In other words what is the benefit that makes you choose this over
plain msleep().
Nishant Malpani July 22, 2020, 2:29 p.m. UTC | #7
On 22/07/20 3:53 pm, Andy Shevchenko wrote:
> On Wed, Jul 22, 2020 at 12:40 PM Nishant Malpani
> <nish.malpani25@gmail.com> wrote:
>> On 22/07/20 3:08 am, Andy Shevchenko wrote:
>>> On Tue, Jul 21, 2020 at 11:35 PM Nishant Malpani
>>> <nish.malpani25@gmail.com> wrote:
>>>> On 22/07/20 1:16 am, Andy Shevchenko wrote:
> 
> ...
> 
>>> Can't you declare table as const int?
>>>
>> I'm not sure I understand you completely here; do you mean const int *?
>> So, an array of alternate integer and fractional parts? I suppose that's
>> possible but we'd be introducing unwanted complexity I feel - for
>> example, currently the index of the 3db frequency in the table is used
>> to directly map & set bits in the filter register corresponding to that
>> frequency but with the approach you share, we'd have to apply a
>> transformation (div by 2) to set the same bits in the filter register.
>> Do you think the added complexity justifies the removal of the casting?
> 
> It was a question. If you think it is too much, don't change :-)
>  > ...
> 
>>>>>> +       /* max transition time to measurement mode */
>>>>>> +       msleep_interruptible(ADXRS290_MAX_TRANSITION_TIME_MS);
>>>>>
>>>>> I'm not sure what the point of interruptible variant here?
>>>>>
>>>> I referred Documentation/timers/timers-howto.rst for this.
>>>> My reasoning was shaped to use the interruptible variant because the
>>>> transition settles in a time *less than* 100ms and since 100ms is quite
>>>> a huge time to sleep, it should be interrupted in case a signal arrives.
>>>
>>> This is probe of the device,
>>> What are the expectations here?
>>>
>> I fail to understand why this can't be used in the probe() but perhaps
>> in a routine to standby/resume. Could you please elaborate?
> 
> I didn't say it can not be used, what I'm asking is what are the
> expectations of the interruptible part here.
> In other words what is the benefit that makes you choose this over
> plain msleep().
> 
Oh, sorry for I misunderstood.

I cannot think of anything more to add to the reasoning that I explained 
earlier; in that case, I'll fall back to msleep() (in v3) unless someone 
else comes with a strong point in msleep_interruptible()'s favor.

With regards,
Nishant Malpani

Patch
diff mbox series

diff --git a/MAINTAINERS b/MAINTAINERS
index e016ee39c74a..dd02cfc410e8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1098,6 +1098,12 @@  L:	linux-media@vger.kernel.org
 S:	Maintained
 F:	drivers/media/i2c/adv7842*
 
+ANALOG DEVICES INC ADXRS290 DRIVER
+M:	Nishant Malpani <nish.malpani25@gmail.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+F:	drivers/iio/gyro/adxrs290.c
+
 ANALOG DEVICES INC ASOC CODEC DRIVERS
 M:	Lars-Peter Clausen <lars@metafoo.de>
 M:	Nuno Sá <nuno.sa@analog.com>
diff --git a/drivers/iio/gyro/Kconfig b/drivers/iio/gyro/Kconfig
index 6daeddf37f60..024a34139875 100644
--- a/drivers/iio/gyro/Kconfig
+++ b/drivers/iio/gyro/Kconfig
@@ -41,6 +41,16 @@  config ADIS16260
 	  This driver can also be built as a module.  If so, the module
 	  will be called adis16260.
 
+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.
+
 config ADXRS450
 	tristate "Analog Devices ADXRS450/3 Digital Output Gyroscope SPI driver"
 	depends on SPI
diff --git a/drivers/iio/gyro/Makefile b/drivers/iio/gyro/Makefile
index 45cbd5dc644e..0319b397dc3f 100644
--- a/drivers/iio/gyro/Makefile
+++ b/drivers/iio/gyro/Makefile
@@ -8,6 +8,7 @@  obj-$(CONFIG_ADIS16080) += adis16080.o
 obj-$(CONFIG_ADIS16130) += adis16130.o
 obj-$(CONFIG_ADIS16136) += adis16136.o
 obj-$(CONFIG_ADIS16260) += adis16260.o
+obj-$(CONFIG_ADXRS290) += adxrs290.o
 obj-$(CONFIG_ADXRS450) += adxrs450.o
 obj-$(CONFIG_BMG160) += bmg160_core.o
 obj-$(CONFIG_BMG160_I2C) += bmg160_i2c.o
diff --git a/drivers/iio/gyro/adxrs290.c b/drivers/iio/gyro/adxrs290.c
new file mode 100644
index 000000000000..6c7d4cbfa785
--- /dev/null
+++ b/drivers/iio/gyro/adxrs290.c
@@ -0,0 +1,444 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * ADXRS290 SPI Gyroscope Driver
+ *
+ * Copyright (C) 2020 Nishant Malpani <nish.malpani25@gmail.com>
+ * Copyright (C) 2020 Analog Devices, Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#define ADXRS290_ADI_ID		0xAD
+#define ADXRS290_MEMS_ID	0x1D
+#define ADXRS290_DEV_ID		0x92
+
+#define ADXRS290_REG_ADI_ID	0x00
+#define ADXRS290_REG_MEMS_ID	0x01
+#define ADXRS290_REG_DEV_ID	0x02
+#define ADXRS290_REG_REV_ID	0x03
+#define ADXRS290_REG_SN0	0x04
+#define ADXRS290_REG_DATAX0	0x08 /* Roll Rate o/p Data Regs, 2 bytes */
+#define ADXRS290_REG_DATAY0	0x0A /* Pitch Rate o/p Data Regs, 2 bytes */
+#define ADXRS290_REG_TEMP0	0x0C
+#define ADXRS290_REG_POWER_CTL	0x10
+#define ADXRS290_REG_FILTER	0x11
+#define ADXRS290_REG_DATA_RDY	0x12
+
+#define ADXRS290_READ		BIT(7)
+#define ADXRS290_TSM		BIT(0)
+#define ADXRS290_MEASUREMENT	BIT(1)
+#define ADXRS290_SYNC		GENMASK(1, 0)
+#define ADXRS290_LPF_MASK	GENMASK(2, 0)
+#define ADXRS290_LPF(x)		FIELD_PREP(ADXRS290_LPF_MASK, x)
+#define ADXRS290_HPF_MASK	GENMASK(7, 4)
+#define ADXRS290_HPF(x)		FIELD_PREP(ADXRS290_HPF_MASK, x)
+
+#define ADXRS290_READ_REG(reg)	(ADXRS290_READ | (reg))
+
+#define ADXRS290_MAX_TRANSITION_TIME_MS 100
+
+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] = {
+	[0] = {480, 0},
+	[1] = {320, 0},
+	[2] = {160, 0},
+	[3] = {80, 0},
+	[4] = {56, 600000},
+	[5] = {40, 0},
+	[6] = {28, 300000},
+	[7] = {20, 0},
+};
+
+/*
+ * 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] = {
+	[0] = {0, 0},
+	[1] = {0, 11000},
+	[2] = {0, 22000},
+	[3] = {0, 44000},
+	[4] = {0, 87000},
+	[5] = {0, 175000},
+	[6] = {0, 350000},
+	[7] = {0, 700000},
+	[8] = {1, 400000},
+	[9] = {2, 800000},
+	[10] = {11, 300000},
+};
+
+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;
+	u16 temp;
+
+	mutex_lock(&st->lock);
+	temp = spi_w8r16(st->spi, cmd);
+	if (temp < 0) {
+		ret = temp;
+		goto err_unlock;
+	}
+
+	*val = temp;
+
+err_unlock:
+	mutex_unlock(&st->lock);
+	return ret;
+}
+
+static int adxrs290_get_temp_data(struct iio_dev *indio_dev, unsigned int *val)
+{
+	const u8 cmd = ADXRS290_READ_REG(ADXRS290_REG_TEMP0);
+	struct adxrs290_state *st = iio_priv(indio_dev);
+	int ret = 0;
+	u16 temp;
+
+	mutex_lock(&st->lock);
+	temp = spi_w8r16(st->spi, cmd);
+	if (temp < 0) {
+		ret = temp;
+		goto err_unlock;
+	}
+
+	/* extract lower 12 bits temperature reading */
+	*val = temp & 0x0FFF;
+
+err_unlock:
+	mutex_unlock(&st->lock);
+	return ret;
+}
+
+static int adxrs290_get_3db_freq(struct iio_dev *indio_dev, u8 *val, u8 *val2)
+{
+	const u8 cmd = ADXRS290_READ_REG(ADXRS290_REG_FILTER);
+	struct adxrs290_state *st = iio_priv(indio_dev);
+	int ret = 0;
+	int temp;
+
+	mutex_lock(&st->lock);
+	temp = spi_w8r8(st->spi, cmd);
+	if (temp < 0) {
+		ret = temp;
+		goto err_unlock;
+	}
+
+	*val = FIELD_GET(ADXRS290_LPF_MASK, temp);
+	*val2 = FIELD_GET(ADXRS290_HPF_MASK, temp);
+
+err_unlock:
+	mutex_unlock(&st->lock);
+	return ret;
+}
+
+static int adxrs290_spi_write_reg(struct spi_device *spi, const u8 reg,
+				  const u8 val)
+{
+	u8 buf[2];
+
+	buf[0] = reg;
+	buf[1] = val;
+
+	return spi_write_then_read(spi, buf, ARRAY_SIZE(buf), NULL, 0);
+}
+
+static int adxrs290_find_match(const int (*freq_tbl)[2], const int n,
+			       const int val, const int val2)
+{
+	int i;
+
+	for (i = 0; i < n; i++) {
+		if (freq_tbl[i][0] == val && freq_tbl[i][1] == val2)
+			return i;
+	}
+
+	return -EINVAL;
+}
+
+static int adxrs290_set_filter_freq(struct iio_dev *indio_dev,
+				    unsigned int lpf_idx, unsigned int hpf_idx)
+{
+	struct adxrs290_state *st = iio_priv(indio_dev);
+	u8 val;
+
+	val = ADXRS290_HPF(hpf_idx) | ADXRS290_LPF(lpf_idx);
+
+	return adxrs290_spi_write_reg(st->spi, ADXRS290_REG_FILTER, val);
+}
+
+static int adxrs290_initial_setup(struct iio_dev *indio_dev)
+{
+	struct adxrs290_state *st = iio_priv(indio_dev);
+
+	st->mode = MEASUREMENT;
+
+	return adxrs290_spi_write_reg(st->spi,
+				      ADXRS290_REG_POWER_CTL,
+				      ADXRS290_MEASUREMENT | ADXRS290_TSM);
+}
+
+static int adxrs290_read_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int *val,
+			     int *val2,
+			     long mask)
+{
+	struct adxrs290_state *st = iio_priv(indio_dev);
+	int ret;
+	unsigned int t;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		switch (chan->type) {
+		case IIO_ANGL_VEL:
+			ret = adxrs290_get_rate_data(indio_dev,
+						     ADXRS290_READ_REG(chan->address),
+						     &t);
+			if (ret < 0)
+				return ret;
+			*val = t;
+			return IIO_VAL_INT;
+		case IIO_TEMP:
+			ret = adxrs290_get_temp_data(indio_dev, &t);
+			if (ret < 0)
+				return ret;
+			*val = t;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ANGL_VEL:
+			*val = 0;
+			*val2 = 87266;
+			return IIO_VAL_INT_PLUS_NANO;
+		case IIO_TEMP:
+			*val = 100;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		switch (chan->type) {
+		case IIO_ANGL_VEL:
+			t = st->lpf_3db_freq_idx;
+			*val = adxrs290_lpf_3db_freq_tbl[t][0];
+			*val2 = adxrs290_lpf_3db_freq_tbl[t][1];
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+		switch (chan->type) {
+		case IIO_ANGL_VEL:
+			t = st->hpf_3db_freq_idx;
+			*val = adxrs290_hpf_3db_freq_tbl[t][0];
+			*val2 = adxrs290_hpf_3db_freq_tbl[t][1];
+			return IIO_VAL_INT_PLUS_MICRO;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return -EINVAL;
+}
+
+static int adxrs290_write_raw(struct iio_dev *indio_dev,
+			      struct iio_chan_spec const *chan,
+			      int val,
+			      int val2,
+			      long mask)
+{
+	struct adxrs290_state *st = iio_priv(indio_dev);
+	int lpf_idx, hpf_idx;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		lpf_idx = adxrs290_find_match(adxrs290_lpf_3db_freq_tbl,
+					      ARRAY_SIZE(adxrs290_lpf_3db_freq_tbl),
+					      val, val2);
+		if (lpf_idx < 0)
+			return -EINVAL;
+		/* caching the updated state of the low-pass filter */
+		st->lpf_3db_freq_idx = lpf_idx;
+		/* retrieving the current state of the high-pass filter */
+		hpf_idx = st->hpf_3db_freq_idx;
+		return adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
+	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+		hpf_idx = adxrs290_find_match(adxrs290_hpf_3db_freq_tbl,
+					      ARRAY_SIZE(adxrs290_hpf_3db_freq_tbl),
+					      val, val2);
+		if (hpf_idx < 0)
+			return -EINVAL;
+		/* caching the updated state of the high-pass filter */
+		st->hpf_3db_freq_idx = hpf_idx;
+		/* retrieving the current state of the low-pass filter */
+		lpf_idx = st->lpf_3db_freq_idx;
+		return adxrs290_set_filter_freq(indio_dev, lpf_idx, hpf_idx);
+	}
+
+	return -EINVAL;
+}
+
+static int adxrs290_read_avail(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       const int **vals, int *type, int *length,
+			       long mask)
+{
+	switch (mask) {
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		*vals = (const int *)adxrs290_lpf_3db_freq_tbl;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		/* Values are stored in a 2D matrix */
+		*length = ARRAY_SIZE(adxrs290_lpf_3db_freq_tbl) * 2;
+
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+		*vals = (const int *)adxrs290_hpf_3db_freq_tbl;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		/* Values are stored in a 2D matrix */
+		*length = ARRAY_SIZE(adxrs290_hpf_3db_freq_tbl) * 2;
+
+		return IIO_AVAIL_LIST;
+	default:
+		return -EINVAL;
+	}
+}
+
+#define ADXRS290_ANGL_VEL_CHANNEL(reg, axis) {				\
+	.type = IIO_ANGL_VEL,						\
+	.address = reg,							\
+	.modified = 1,							\
+	.channel2 = IIO_MOD_##axis,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),			\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) |		\
+	BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |		\
+	BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY),		\
+	.info_mask_shared_by_type_available =				\
+	BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) |		\
+	BIT(IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY),		\
+}
+
+static const struct iio_chan_spec adxrs290_channels[] = {
+	ADXRS290_ANGL_VEL_CHANNEL(ADXRS290_REG_DATAX0, X),
+	ADXRS290_ANGL_VEL_CHANNEL(ADXRS290_REG_DATAY0, Y),
+	{
+		.type = IIO_TEMP,
+		.address = ADXRS290_REG_TEMP0,
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+		BIT(IIO_CHAN_INFO_SCALE),
+	},
+};
+
+static const struct iio_info adxrs290_info = {
+	.read_raw = &adxrs290_read_raw,
+	.write_raw = &adxrs290_write_raw,
+	.read_avail = &adxrs290_read_avail,
+};
+
+static int adxrs290_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct adxrs290_state *st;
+	int ret;
+	u8 val, val2;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	st->spi = spi;
+	spi_set_drvdata(spi, indio_dev);
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->name = "adxrs290";
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = adxrs290_channels;
+	indio_dev->num_channels = ARRAY_SIZE(adxrs290_channels);
+	indio_dev->info = &adxrs290_info;
+
+	val = spi_w8r8(spi, ADXRS290_READ_REG(ADXRS290_REG_ADI_ID));
+	if (val != ADXRS290_ADI_ID) {
+		dev_err(&spi->dev, "Wrong ADI ID 0x%02x\n", val);
+		return -ENODEV;
+	}
+
+	val = spi_w8r8(spi, ADXRS290_READ_REG(ADXRS290_REG_MEMS_ID));
+	if (val != ADXRS290_MEMS_ID) {
+		dev_err(&spi->dev, "Wrong MEMS ID 0x%02x\n", val);
+		return -ENODEV;
+	}
+
+	val = spi_w8r8(spi, ADXRS290_READ_REG(ADXRS290_REG_DEV_ID));
+	if (val != ADXRS290_DEV_ID) {
+		dev_err(&spi->dev, "Wrong DEV ID 0x%02x\n", val);
+		return -ENODEV;
+	}
+
+	/* default mode the gyroscope starts in */
+	st->mode = STANDBY;
+
+	/* switch to measurement mode and switch on the temperature sensor */
+	ret = adxrs290_initial_setup(indio_dev);
+	if (ret < 0)
+		return ret;
+
+	/* max transition time to measurement mode */
+	msleep_interruptible(ADXRS290_MAX_TRANSITION_TIME_MS);
+
+	ret = adxrs290_get_3db_freq(indio_dev, &val, &val2);
+	if (ret < 0)
+		return ret;
+
+	st->lpf_3db_freq_idx = val;
+	st->hpf_3db_freq_idx = val2;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id adxrs290_of_match[] = {
+	{ .compatible = "adi,adxrs290" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, adxrs290_of_match);
+
+static struct spi_driver adxrs290_driver = {
+	.driver = {
+		.name = "adxrs290",
+		.of_match_table = adxrs290_of_match,
+	},
+	.probe = adxrs290_probe,
+};
+module_spi_driver(adxrs290_driver);
+
+MODULE_AUTHOR("Nishant Malpani <nish.malpani25@gmail.com>");
+MODULE_DESCRIPTION("Analog Devices ADXRS290 Gyroscope SPI driver");
+MODULE_LICENSE("GPL");