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

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

Commit Message

Nishant Malpani July 24, 2020, 11:01 a.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: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXRS290.pdf
Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
---

Changes in v3:
  - drop "Link" tag & extra line in commit message
  - rename cut-off frequencies tables to
    'adxrs290_{lpf, hpf}_3db_freq_hz_table' to be more descriptive
  - fix unsigned type errors
  - add comments on how to scale raw angular velocity and temperature
    values to appropriate units mentioned in the ABI
  - re-order declarations in reversed spruce tree order
  - remove 'indio_dev->dev.parent = &spi->dev' as the iio core handles it
    during iio_device_alloc()
  - use plain msleep() instead of the interruptible variant
  - remove extra terminal comma

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 | 446 ++++++++++++++++++++++++++++++++++++
 4 files changed, 463 insertions(+)
 create mode 100644 drivers/iio/gyro/adxrs290.c

Comments

Andy Shevchenko July 26, 2020, 8:03 a.m. UTC | #1
On Fri, Jul 24, 2020 at 2:02 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.

Thanks for an update!
My nits below, after addressing them
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXRS290.pdf
> Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
> ---
>
> Changes in v3:
>   - drop "Link" tag & extra line in commit message
>   - rename cut-off frequencies tables to
>     'adxrs290_{lpf, hpf}_3db_freq_hz_table' to be more descriptive
>   - fix unsigned type errors
>   - add comments on how to scale raw angular velocity and temperature
>     values to appropriate units mentioned in the ABI
>   - re-order declarations in reversed spruce tree order
>   - remove 'indio_dev->dev.parent = &spi->dev' as the iio core handles it
>     during iio_device_alloc()
>   - use plain msleep() instead of the interruptible variant
>   - remove extra terminal comma
>
> 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 | 446 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 463 insertions(+)
>  create mode 100644 drivers/iio/gyro/adxrs290.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9077411c9890..71ae9b184179 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..cff1af9211bc
> --- /dev/null
> +++ b/drivers/iio/gyro/adxrs290.c
> @@ -0,0 +1,446 @@
> +// 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>

Keep it ordered?

> +#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 /* Serial Number Registers, 4 bytes */
> +#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,
> +};

Perhaps a prefix
ADXRS290_STANDBY,
etc.

> +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_hz_table[][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_hz_table[][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;
> +       int 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;
> +       int 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;
> +       short 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,
> +                                   const unsigned int lpf_idx,
> +                                   const 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);
> +       unsigned int t;
> +       int ret;
> +
> +       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)

Please, revisit all ' < 0' checks and drop ones that are not necessary
(above seems fine, but here and below are subject to revisit).

> +                               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:
> +                       /* 1 LSB = 0.005 degrees/sec */
> +                       *val = 0;
> +                       *val2 = 87266;
> +                       return IIO_VAL_INT_PLUS_NANO;
> +               case IIO_TEMP:
> +                       /* 1 LSB = 0.1 degrees Celsius */
> +                       *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_hz_table[t][0];
> +                       *val2 = adxrs290_lpf_3db_freq_hz_table[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_hz_table[t][0];
> +                       *val2 = adxrs290_hpf_3db_freq_hz_table[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_hz_table,
> +                                             ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table),
> +                                             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_hz_table,
> +                                             ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table),
> +                                             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_hz_table;
> +               *type = IIO_VAL_INT_PLUS_MICRO;
> +               /* Values are stored in a 2D matrix */
> +               *length = ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table) * 2;
> +
> +               return IIO_AVAIL_LIST;
> +       case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
> +               *vals = (const int *)adxrs290_hpf_3db_freq_hz_table;
> +               *type = IIO_VAL_INT_PLUS_MICRO;
> +               /* Values are stored in a 2D matrix */
> +               *length = ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table) * 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;
> +       u8 val, val2;
> +       int ret;
> +
> +       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->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(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");
> --
> 2.20.1
>
Jonathan Cameron July 26, 2020, 12:15 p.m. UTC | #2
On Fri, 24 Jul 2020 16:31:59 +0530
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: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXRS290.pdf
> Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>

Looks pretty good to me.  A few minor comments seeing as you'll be doing
a v4 anyway to tidy up the bits Andy pointed out.

I've pointed out the locking is probably in excess of what is needed, but
I have no problem with you leaving it as it stands, in the interests of
having less fragile code as you extend the driver futher.

Thanks,

Jonathan

> ---
> 
> Changes in v3:
>   - drop "Link" tag & extra line in commit message
>   - rename cut-off frequencies tables to
>     'adxrs290_{lpf, hpf}_3db_freq_hz_table' to be more descriptive
>   - fix unsigned type errors
>   - add comments on how to scale raw angular velocity and temperature
>     values to appropriate units mentioned in the ABI
>   - re-order declarations in reversed spruce tree order
>   - remove 'indio_dev->dev.parent = &spi->dev' as the iio core handles it
>     during iio_device_alloc()
>   - use plain msleep() instead of the interruptible variant
>   - remove extra terminal comma
> 
> 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 | 446 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 463 insertions(+)
>  create mode 100644 drivers/iio/gyro/adxrs290.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9077411c9890..71ae9b184179 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..cff1af9211bc
> --- /dev/null
> +++ b/drivers/iio/gyro/adxrs290.c
> @@ -0,0 +1,446 @@
> +// 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 /* Serial Number Registers, 4 bytes */
> +#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_hz_table[][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_hz_table[][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)

Why go through the effort of signed to unsigned to signed for val?

> +{
> +	struct adxrs290_state *st = iio_priv(indio_dev);
> +	int ret = 0;
> +	int 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;
> +	int temp;
> +
> +	mutex_lock(&st->lock);

Just a quick note here.  I don't think you actually need the lock
(yet) as you aren't doing any read modify write cycles and in this
particular case there is no local cached state to protect.
However, I don't mind if you want to leave them as it may make things
less fragile as you move forwards.

> +	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;
> +	short 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,
> +				    const unsigned int lpf_idx,
> +				    const 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);
> +	unsigned int t;
> +	int ret;
> +
> +	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;

As mentioned above, you should be fine with using a signed value where you have &t.
As we don't use *val in the error case, it's fine to write directly into it and
avoid the need for the local variable t at all.

> +			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:
> +			/* 1 LSB = 0.005 degrees/sec */
> +			*val = 0;
> +			*val2 = 87266;
> +			return IIO_VAL_INT_PLUS_NANO;
> +		case IIO_TEMP:
> +			/* 1 LSB = 0.1 degrees Celsius */
> +			*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_hz_table[t][0];
> +			*val2 = adxrs290_lpf_3db_freq_hz_table[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_hz_table[t][0];
> +			*val2 = adxrs290_hpf_3db_freq_hz_table[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_hz_table,
> +					      ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table),
> +					      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_hz_table,
> +					      ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table),
> +					      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_hz_table;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		/* Values are stored in a 2D matrix */
> +		*length = ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table) * 2;
> +
> +		return IIO_AVAIL_LIST;
> +	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
> +		*vals = (const int *)adxrs290_hpf_3db_freq_hz_table;
> +		*type = IIO_VAL_INT_PLUS_MICRO;
> +		/* Values are stored in a 2D matrix */
> +		*length = ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table) * 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;
> +	u8 val, val2;
> +	int ret;
> +
> +	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);

As things currently stand I don't think you ever use this.
So drop it for now and reintroduce it when you need it (probably when
you add power management support).

> +
> +	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(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");
Nishant Malpani July 26, 2020, 1:51 p.m. UTC | #3
On 26/07/20 1:33 pm, Andy Shevchenko wrote:
> On Fri, Jul 24, 2020 at 2:02 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.
> 
> Thanks for an update!
> My nits below, after addressing them

> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>
> 
Thanks!

>> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXRS290.pdf
>> Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
>> ---
>>
>> Changes in v3:
>>    - drop "Link" tag & extra line in commit message
>>    - rename cut-off frequencies tables to
>>      'adxrs290_{lpf, hpf}_3db_freq_hz_table' to be more descriptive
>>    - fix unsigned type errors
>>    - add comments on how to scale raw angular velocity and temperature
>>      values to appropriate units mentioned in the ABI
>>    - re-order declarations in reversed spruce tree order
>>    - remove 'indio_dev->dev.parent = &spi->dev' as the iio core handles it
>>      during iio_device_alloc()
>>    - use plain msleep() instead of the interruptible variant
>>    - remove extra terminal comma
>>
>> 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 | 446 ++++++++++++++++++++++++++++++++++++
>>   4 files changed, 463 insertions(+)
>>   create mode 100644 drivers/iio/gyro/adxrs290.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9077411c9890..71ae9b184179 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..cff1af9211bc
>> --- /dev/null
>> +++ b/drivers/iio/gyro/adxrs290.c
>> @@ -0,0 +1,446 @@
>> +// 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>
> 
> Keep it ordered?
> 
Ah, missed it somehow. Will reorder in v4.

>> +#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 /* Serial Number Registers, 4 bytes */
>> +#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,
>> +};
> 
> Perhaps a prefix
> ADXRS290_STANDBY,
> etc.
> 
Makes sense. I'll prefix with 'ADXRS290_MODE_' (in v4) as 
'ADXRS290_MEASUREMENT' is already a macro defintion.

>> +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_hz_table[][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_hz_table[][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;
>> +       int 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;
>> +       int 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;
>> +       short 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,
>> +                                   const unsigned int lpf_idx,
>> +                                   const 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);
>> +       unsigned int t;
>> +       int ret;
>> +
>> +       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)
> 
> Please, revisit all ' < 0' checks and drop ones that are not necessary
> (above seems fine, but here and below are subject to revisit).
>
Revisited all '< 0' checks and I consider them all necessary as they 
(mostly) reflect failed spi transactions.

With regards,
Nishant Malpani

>> +                               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:
>> +                       /* 1 LSB = 0.005 degrees/sec */
>> +                       *val = 0;
>> +                       *val2 = 87266;
>> +                       return IIO_VAL_INT_PLUS_NANO;
>> +               case IIO_TEMP:
>> +                       /* 1 LSB = 0.1 degrees Celsius */
>> +                       *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_hz_table[t][0];
>> +                       *val2 = adxrs290_lpf_3db_freq_hz_table[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_hz_table[t][0];
>> +                       *val2 = adxrs290_hpf_3db_freq_hz_table[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_hz_table,
>> +                                             ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table),
>> +                                             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_hz_table,
>> +                                             ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table),
>> +                                             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_hz_table;
>> +               *type = IIO_VAL_INT_PLUS_MICRO;
>> +               /* Values are stored in a 2D matrix */
>> +               *length = ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table) * 2;
>> +
>> +               return IIO_AVAIL_LIST;
>> +       case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
>> +               *vals = (const int *)adxrs290_hpf_3db_freq_hz_table;
>> +               *type = IIO_VAL_INT_PLUS_MICRO;
>> +               /* Values are stored in a 2D matrix */
>> +               *length = ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table) * 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;
>> +       u8 val, val2;
>> +       int ret;
>> +
>> +       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->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(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");
>> --
>> 2.20.1
>>
> 
>
Nishant Malpani July 26, 2020, 2:05 p.m. UTC | #4
On 26/07/20 5:45 pm, Jonathan Cameron wrote:
> On Fri, 24 Jul 2020 16:31:59 +0530
> 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: https://www.analog.com/media/en/technical-documentation/data-sheets/ADXRS290.pdf
>> Signed-off-by: Nishant Malpani <nish.malpani25@gmail.com>
> 
> Looks pretty good to me.  A few minor comments seeing as you'll be doing
> a v4 anyway to tidy up the bits Andy pointed out.
> 
> I've pointed out the locking is probably in excess of what is needed, but
> I have no problem with you leaving it as it stands, in the interests of
> having less fragile code as you extend the driver futher.
> 
> Thanks,
> 
> Jonathan
> 
>> ---
>>
>> Changes in v3:
>>    - drop "Link" tag & extra line in commit message
>>    - rename cut-off frequencies tables to
>>      'adxrs290_{lpf, hpf}_3db_freq_hz_table' to be more descriptive
>>    - fix unsigned type errors
>>    - add comments on how to scale raw angular velocity and temperature
>>      values to appropriate units mentioned in the ABI
>>    - re-order declarations in reversed spruce tree order
>>    - remove 'indio_dev->dev.parent = &spi->dev' as the iio core handles it
>>      during iio_device_alloc()
>>    - use plain msleep() instead of the interruptible variant
>>    - remove extra terminal comma
>>
>> 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 | 446 ++++++++++++++++++++++++++++++++++++
>>   4 files changed, 463 insertions(+)
>>   create mode 100644 drivers/iio/gyro/adxrs290.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9077411c9890..71ae9b184179 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..cff1af9211bc
>> --- /dev/null
>> +++ b/drivers/iio/gyro/adxrs290.c
>> @@ -0,0 +1,446 @@
>> +// 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 /* Serial Number Registers, 4 bytes */
>> +#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_hz_table[][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_hz_table[][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)
> 
> Why go through the effort of signed to unsigned to signed for val?
>
Yeah, those are some unnecessary type promotions and demotions. Will 
change the function signature to directly accept 'int *val' in v4.

>> +{
>> +	struct adxrs290_state *st = iio_priv(indio_dev);
>> +	int ret = 0;
>> +	int 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;
>> +	int temp;
>> +
>> +	mutex_lock(&st->lock);
> 
> Just a quick note here.  I don't think you actually need the lock
> (yet) as you aren't doing any read modify write cycles and in this
> particular case there is no local cached state to protect.
> However, I don't mind if you want to leave them as it may make things
> less fragile as you move forwards.
> 
I was also doubtful about the protections previously but, as you've 
summarized perfectly, I left those as a 'good-to-have' for now, in the 
hope that they might be necessary as I incrementally add more support in 
the driver.

>> +	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;
>> +	short 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,
>> +				    const unsigned int lpf_idx,
>> +				    const 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);
>> +	unsigned int t;
>> +	int ret;
>> +
>> +	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;
> 
> As mentioned above, you should be fine with using a signed value where you have &t.
> As we don't use *val in the error case, it's fine to write directly into it and
> avoid the need for the local variable t at all.

Okay, will take care of this in v4.

> 
>> +			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:
>> +			/* 1 LSB = 0.005 degrees/sec */
>> +			*val = 0;
>> +			*val2 = 87266;
>> +			return IIO_VAL_INT_PLUS_NANO;
>> +		case IIO_TEMP:
>> +			/* 1 LSB = 0.1 degrees Celsius */
>> +			*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_hz_table[t][0];
>> +			*val2 = adxrs290_lpf_3db_freq_hz_table[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_hz_table[t][0];
>> +			*val2 = adxrs290_hpf_3db_freq_hz_table[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_hz_table,
>> +					      ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table),
>> +					      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_hz_table,
>> +					      ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table),
>> +					      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_hz_table;
>> +		*type = IIO_VAL_INT_PLUS_MICRO;
>> +		/* Values are stored in a 2D matrix */
>> +		*length = ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table) * 2;
>> +
>> +		return IIO_AVAIL_LIST;
>> +	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
>> +		*vals = (const int *)adxrs290_hpf_3db_freq_hz_table;
>> +		*type = IIO_VAL_INT_PLUS_MICRO;
>> +		/* Values are stored in a 2D matrix */
>> +		*length = ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table) * 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;
>> +	u8 val, val2;
>> +	int ret;
>> +
>> +	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);
> 
> As things currently stand I don't think you ever use this.
> So drop it for now and reintroduce it when you need it (probably when
> you add power management support).

Okay, makes sense. Will remove it in v4.

With regards,
Nishant Malpani

> 
>> +
>> +	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(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");
>

Patch
diff mbox series

diff --git a/MAINTAINERS b/MAINTAINERS
index 9077411c9890..71ae9b184179 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..cff1af9211bc
--- /dev/null
+++ b/drivers/iio/gyro/adxrs290.c
@@ -0,0 +1,446 @@ 
+// 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 /* Serial Number Registers, 4 bytes */
+#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_hz_table[][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_hz_table[][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;
+	int 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;
+	int 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;
+	short 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,
+				    const unsigned int lpf_idx,
+				    const 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);
+	unsigned int t;
+	int ret;
+
+	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:
+			/* 1 LSB = 0.005 degrees/sec */
+			*val = 0;
+			*val2 = 87266;
+			return IIO_VAL_INT_PLUS_NANO;
+		case IIO_TEMP:
+			/* 1 LSB = 0.1 degrees Celsius */
+			*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_hz_table[t][0];
+			*val2 = adxrs290_lpf_3db_freq_hz_table[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_hz_table[t][0];
+			*val2 = adxrs290_hpf_3db_freq_hz_table[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_hz_table,
+					      ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table),
+					      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_hz_table,
+					      ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table),
+					      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_hz_table;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		/* Values are stored in a 2D matrix */
+		*length = ARRAY_SIZE(adxrs290_lpf_3db_freq_hz_table) * 2;
+
+		return IIO_AVAIL_LIST;
+	case IIO_CHAN_INFO_HIGH_PASS_FILTER_3DB_FREQUENCY:
+		*vals = (const int *)adxrs290_hpf_3db_freq_hz_table;
+		*type = IIO_VAL_INT_PLUS_MICRO;
+		/* Values are stored in a 2D matrix */
+		*length = ARRAY_SIZE(adxrs290_hpf_3db_freq_hz_table) * 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;
+	u8 val, val2;
+	int ret;
+
+	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->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(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");