[PATCH v3 0/6] Support ADIS16475 and similar IMUs

[PATCH v3 0/6] Support ADIS16475 and similar IMUs

[Nuno Sá]

This series adds support for the adis16475 and similar IMUs. This driver
will be the first user of some changes on the adis library. Hence, the
first three patches are related to the library:
 * Add anaged device functions for registering triggers with the library;
 * Updates the way `irq_mask` is passed to `request_irq()`;
 * It adds an update_bits() like API.

A new patch was introduced (iio: adis: Add burst_max_len variable) in
order to make burst32 configuration at runtime.

Nuno Sá (6):
  iio: imu: adis: Add Managed device functions
  iio: imu: adis: Add irq mask variable
  iio: adis: Add adis_update_bits() APIs
  iio: adis: Support different burst sizes
  iio: imu: Add support for adis16475
  dt-bindings: iio: Add adis16475 documentation

 .../bindings/iio/imu/adi,adis16475.yaml       |  137 ++
 MAINTAINERS                                   |    9 +
 drivers/iio/imu/Kconfig                       |   13 +
 drivers/iio/imu/Makefile                      |    1 +
 drivers/iio/imu/adis.c                        |   26 +
 drivers/iio/imu/adis16400.c                   |    2 +-
 drivers/iio/imu/adis16475.c                   | 1344 +++++++++++++++++
 drivers/iio/imu/adis_buffer.c                 |   58 +-
 drivers/iio/imu/adis_trigger.c                |   65 +-
 include/linux/iio/imu/adis.h                  |   87 +-
 10 files changed, 1731 insertions(+), 11 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
 create mode 100644 drivers/iio/imu/adis16475.c

-- 
2.17.1

[PATCH v3 1/6] iio: imu: adis: Add Managed device functions

[Nuno Sá]

This patch adds support for a managed device version of
adis_setup_buffer_and_trigger. It works exactly as the original
one but it calls all the devm_iio_* functions to setup an iio
buffer and trigger. Hence we do not need to care about cleaning those
and we do not need to support a remove() callback for every driver using
the adis library.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
Changes in v2:
 * Added blank lines for readability.

Changes in V3:
 * Removed unnecessary inline;
 * Free buffer resources.

 drivers/iio/imu/adis_buffer.c  | 45 ++++++++++++++++++++++++++++++++++
 drivers/iio/imu/adis_trigger.c | 41 ++++++++++++++++++++++++++++---
 include/linux/iio/imu/adis.h   | 17 +++++++++++++
 3 files changed, 100 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index 04e5e2a0fd6b..c2211ab80d8c 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -156,6 +156,14 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
 	return IRQ_HANDLED;
 }
 
+static void adis_buffer_cleanup(void *arg)
+{
+	struct adis *adis = arg;
+
+	kfree(adis->buffer);
+	kfree(adis->xfer);
+}
+
 /**
  * adis_setup_buffer_and_trigger() - Sets up buffer and trigger for the adis device
  * @adis: The adis device.
@@ -198,6 +206,43 @@ int adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
 }
 EXPORT_SYMBOL_GPL(adis_setup_buffer_and_trigger);
 
+/**
+ * devm_adis_setup_buffer_and_trigger() - Sets up buffer and trigger for
+ *					  the managed adis device
+ * @adis: The adis device
+ * @indio_dev: The IIO device
+ * @trigger_handler: Optional trigger handler, may be NULL.
+ *
+ * Returns 0 on success, a negative error code otherwise.
+ *
+ * This function perfoms exactly the same as adis_setup_buffer_and_trigger()
+ */
+int
+devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
+				   irqreturn_t (*trigger_handler)(int, void *))
+{
+	int ret;
+
+	if (!trigger_handler)
+		trigger_handler = adis_trigger_handler;
+
+	ret = devm_iio_triggered_buffer_setup(&adis->spi->dev, indio_dev,
+					      &iio_pollfunc_store_time,
+					      trigger_handler, NULL);
+	if (ret)
+		return ret;
+
+	if (adis->spi->irq) {
+		ret = devm_adis_probe_trigger(adis, indio_dev);
+		if (ret)
+			return ret;
+	}
+
+	return devm_add_action_or_reset(&adis->spi->dev, adis_buffer_cleanup,
+					adis);
+}
+EXPORT_SYMBOL_GPL(devm_adis_setup_buffer_and_trigger);
+
 /**
  * adis_cleanup_buffer_and_trigger() - Free buffer and trigger resources
  * @adis: The adis device.
diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.c
index 8b9cd02c0f9f..a36810e0b1ab 100644
--- a/drivers/iio/imu/adis_trigger.c
+++ b/drivers/iio/imu/adis_trigger.c
@@ -27,6 +27,13 @@ static const struct iio_trigger_ops adis_trigger_ops = {
 	.set_trigger_state = &adis_data_rdy_trigger_set_state,
 };
 
+static void adis_trigger_setup(struct adis *adis)
+{
+	adis->trig->dev.parent = &adis->spi->dev;
+	adis->trig->ops = &adis_trigger_ops;
+	iio_trigger_set_drvdata(adis->trig, adis);
+}
+
 /**
  * adis_probe_trigger() - Sets up trigger for a adis device
  * @adis: The adis device
@@ -45,9 +52,7 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
 	if (adis->trig == NULL)
 		return -ENOMEM;
 
-	adis->trig->dev.parent = &adis->spi->dev;
-	adis->trig->ops = &adis_trigger_ops;
-	iio_trigger_set_drvdata(adis->trig, adis);
+	adis_trigger_setup(adis);
 
 	ret = request_irq(adis->spi->irq,
 			  &iio_trigger_generic_data_rdy_poll,
@@ -73,6 +78,36 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
 }
 EXPORT_SYMBOL_GPL(adis_probe_trigger);
 
+/**
+ * devm_adis_probe_trigger() - Sets up trigger for a managed adis device
+ * @adis: The adis device
+ * @indio_dev: The IIO device
+ *
+ * Returns 0 on success or a negative error code
+ */
+int devm_adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
+{
+	int ret;
+
+	adis->trig = devm_iio_trigger_alloc(&adis->spi->dev, "%s-dev%d",
+					    indio_dev->name, indio_dev->id);
+	if (!adis->trig)
+		return -ENOMEM;
+
+	adis_trigger_setup(adis);
+
+	ret = devm_request_irq(&adis->spi->dev, adis->spi->irq,
+			       &iio_trigger_generic_data_rdy_poll,
+			       IRQF_TRIGGER_RISING,
+			       indio_dev->name,
+			       adis->trig);
+	if (ret)
+		return ret;
+
+	return devm_iio_trigger_register(&adis->spi->dev, adis->trig);
+}
+EXPORT_SYMBOL_GPL(devm_adis_probe_trigger);
+
 /**
  * adis_remove_trigger() - Remove trigger for a adis devices
  * @adis: The adis device
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index dd8219138c2e..ac94c483bf2b 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -448,11 +448,15 @@ struct adis_burst {
 	unsigned int	extra_len;
 };
 
+int
+devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
+				   irqreturn_t (*trigger_handler)(int, void *));
 int adis_setup_buffer_and_trigger(struct adis *adis,
 	struct iio_dev *indio_dev, irqreturn_t (*trigger_handler)(int, void *));
 void adis_cleanup_buffer_and_trigger(struct adis *adis,
 	struct iio_dev *indio_dev);
 
+int devm_adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev);
 int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev);
 void adis_remove_trigger(struct adis *adis);
 
@@ -461,6 +465,13 @@ int adis_update_scan_mode(struct iio_dev *indio_dev,
 
 #else /* CONFIG_IIO_BUFFER */
 
+static inline int
+devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
+				   irqreturn_t (*trigger_handler)(int, void *))
+{
+	return 0;
+}
+
 static inline int adis_setup_buffer_and_trigger(struct adis *adis,
 	struct iio_dev *indio_dev, irqreturn_t (*trigger_handler)(int, void *))
 {
@@ -472,6 +483,12 @@ static inline void adis_cleanup_buffer_and_trigger(struct adis *adis,
 {
 }
 
+static inline int devm_adis_probe_trigger(struct adis *adis,
+					  struct iio_dev *indio_dev)
+{
+	return 0;
+}
+
 static inline int adis_probe_trigger(struct adis *adis,
 	struct iio_dev *indio_dev)
 {
-- 
2.17.1

[PATCH v3 2/6] iio: imu: adis: Add irq mask variable

[Nuno Sá]

There are some ADIS devices that can configure the data ready pin
polarity. Hence, we cannot hardcode our IRQ mask as IRQF_TRIGGER_RISING
since we might want to have it as IRQF_TRIGGER_FALLING.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
Changes in v2:
 * Add kernel doc-string for `irq-mask`.

Changes in v3:
 * Removed unnecessary inline;
 * Renamed `__adis_validate_irq_mask` to `adis_validate_irq_mask` to keep lib consistency;
 * Cosmetics in logs.

 drivers/iio/imu/adis_trigger.c | 26 ++++++++++++++++++++++++--
 include/linux/iio/imu/adis.h   |  2 ++
 2 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.c
index a36810e0b1ab..df88b74f4c2b 100644
--- a/drivers/iio/imu/adis_trigger.c
+++ b/drivers/iio/imu/adis_trigger.c
@@ -34,6 +34,20 @@ static void adis_trigger_setup(struct adis *adis)
 	iio_trigger_set_drvdata(adis->trig, adis);
 }
 
+static int adis_validate_irq_mask(struct adis *adis)
+{
+	if (!adis->irq_mask) {
+		adis->irq_mask = IRQF_TRIGGER_RISING;
+		return 0;
+	} else if (adis->irq_mask != IRQF_TRIGGER_RISING &&
+		   adis->irq_mask != IRQF_TRIGGER_FALLING) {
+		dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n",
+			adis->irq_mask);
+		return -EINVAL;
+	}
+
+	return 0;
+}
 /**
  * adis_probe_trigger() - Sets up trigger for a adis device
  * @adis: The adis device
@@ -54,9 +68,13 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
 
 	adis_trigger_setup(adis);
 
+	ret = adis_validate_irq_mask(adis);
+	if (ret)
+		return ret;
+
 	ret = request_irq(adis->spi->irq,
 			  &iio_trigger_generic_data_rdy_poll,
-			  IRQF_TRIGGER_RISING,
+			  adis->irq_mask,
 			  indio_dev->name,
 			  adis->trig);
 	if (ret)
@@ -96,9 +114,13 @@ int devm_adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
 
 	adis_trigger_setup(adis);
 
+	ret = adis_validate_irq_mask(adis);
+	if (ret)
+		return ret;
+
 	ret = devm_request_irq(&adis->spi->dev, adis->spi->irq,
 			       &iio_trigger_generic_data_rdy_poll,
-			       IRQF_TRIGGER_RISING,
+			       adis->irq_mask,
 			       indio_dev->name,
 			       adis->trig);
 	if (ret)
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index ac94c483bf2b..ed41c6b96d14 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -87,6 +87,7 @@ struct adis_data {
  * @msg: SPI message object
  * @xfer: SPI transfer objects to be used for a @msg
  * @current_page: Some ADIS devices have registers, this selects current page
+ * @irq_mask: IRQ handling flags as passed to request_irq()
  * @buffer: Data buffer for information read from the device
  * @tx: DMA safe TX buffer for SPI transfers
  * @rx: DMA safe RX buffer for SPI transfers
@@ -113,6 +114,7 @@ struct adis {
 	struct spi_message	msg;
 	struct spi_transfer	*xfer;
 	unsigned int		current_page;
+	unsigned long		irq_mask;
 	void			*buffer;
 
 	uint8_t			tx[10] ____cacheline_aligned;
-- 
2.17.1

[PATCH v3 3/6] iio: adis: Add adis_update_bits() APIs

[Nuno Sá]

This patch adds a `regmap_update_bits()` like API to the ADIS library.
It provides locked and unlocked variant.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
Changes in v2:
 * Add BUILD_BUG_ON() to avoid invalid types.

Changes in V3:
 * Nothing changed.

 drivers/iio/imu/adis.c       | 26 ++++++++++++++++
 include/linux/iio/imu/adis.h | 59 ++++++++++++++++++++++++++++++++++++
 2 files changed, 85 insertions(+)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index a8afd01de4f3..fa0ee35d96f0 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -223,6 +223,32 @@ int __adis_read_reg(struct adis *adis, unsigned int reg,
 	return ret;
 }
 EXPORT_SYMBOL_GPL(__adis_read_reg);
+/**
+ * __adis_update_bits_base() - ADIS Update bits function - Unlocked version
+ * @adis: The adis device
+ * @reg: The address of the lower of the two registers
+ * @mask: Bitmask to change
+ * @val: Value to be written
+ * @size: Size of the register to update
+ *
+ * Updates the desired bits of @reg in accordance with @mask and @val.
+ */
+int __adis_update_bits_base(struct adis *adis, unsigned int reg, const u32 mask,
+			    const u32 val, u8 size)
+{
+	int ret;
+	u32 __val;
+
+	ret = __adis_read_reg(adis, reg, &__val, size);
+	if (ret)
+		return ret;
+
+	__val &= ~mask;
+	__val |= val & mask;
+
+	return __adis_write_reg(adis, reg, __val, size);
+}
+EXPORT_SYMBOL_GPL(__adis_update_bits_base);
 
 #ifdef CONFIG_DEBUG_FS
 
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index ed41c6b96d14..94031b3fc9d5 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -333,6 +333,65 @@ static inline int adis_read_reg_32(struct adis *adis, unsigned int reg,
 	return ret;
 }
 
+int __adis_update_bits_base(struct adis *adis, unsigned int reg, const u32 mask,
+			    const u32 val, u8 size);
+/**
+ * adis_update_bits_base() - ADIS Update bits function - Locked version
+ * @adis: The adis device
+ * @reg: The address of the lower of the two registers
+ * @mask: Bitmask to change
+ * @val: Value to be written
+ * @size: Size of the register to update
+ *
+ * Updates the desired bits of @reg in accordance with @mask and @val.
+ */
+static inline int adis_update_bits_base(struct adis *adis, unsigned int reg,
+					const u32 mask, const u32 val, u8 size)
+{
+	int ret;
+
+	mutex_lock(&adis->state_lock);
+	ret = __adis_update_bits_base(adis, reg, mask, val, size);
+	mutex_unlock(&adis->state_lock);
+	return ret;
+}
+
+/**
+ * adis_update_bits() - Wrapper macro for adis_update_bits_base - Locked version
+ * @adis: The adis device
+ * @reg: The address of the lower of the two registers
+ * @mask: Bitmask to change
+ * @val: Value to be written
+ *
+ * This macro evaluates the sizeof of @val at compile time and calls
+ * adis_update_bits_base() accordingly. Be aware that using MACROS/DEFINES for
+ * @val can lead to undesired behavior if the register to update is 16bit.
+ */
+#define adis_update_bits(adis, reg, mask, val) ({			\
+	BUILD_BUG_ON(sizeof(val) == 1 || sizeof(val) == 8);		\
+	__builtin_choose_expr(sizeof(val) == 4,				\
+		adis_update_bits_base(adis, reg, mask, val, 4),         \
+		adis_update_bits_base(adis, reg, mask, val, 2));	\
+})
+
+/**
+ * adis_update_bits() - Wrapper macro for adis_update_bits_base
+ * @adis: The adis device
+ * @reg: The address of the lower of the two registers
+ * @mask: Bitmask to change
+ * @val: Value to be written
+ *
+ * This macro evaluates the sizeof of @val at compile time and calls
+ * adis_update_bits_base() accordingly. Be aware that using MACROS/DEFINES for
+ * @val can lead to undesired behavior if the register to update is 16bit.
+ */
+#define __adis_update_bits(adis, reg, mask, val) ({			\
+	BUILD_BUG_ON(sizeof(val) == 1 || sizeof(val) == 8);		\
+	__builtin_choose_expr(sizeof(val) == 4,				\
+		__adis_update_bits_base(adis, reg, mask, val, 4),	\
+		__adis_update_bits_base(adis, reg, mask, val, 2));	\
+})
+
 int adis_enable_irq(struct adis *adis, bool enable);
 int __adis_check_status(struct adis *adis);
 int __adis_initial_startup(struct adis *adis);
-- 
2.17.1

[PATCH v3 4/6] iio: adis: Support different burst sizes

[Nuno Sá]

Add burst_max_len to `adis_burst`. This is useful for devices which
support different burst modes with different sizes. The buffer to be
used in the spi transfer is allocated with this variable making sure
that has space for all burst modes. The spi transfer length should hold
the "real" burst length depending on the current burst mode configured
in the device.

Moreover, `extra_len` in `adis_burst` is made const and it should
contain the smallest extra length necessary for a burst transfer. In
`struct adis` was added a new `burst_extra_len` that should hold the
extra bytes needed depending on the device instance being used.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
Changes in v2:
 * Nothing changed.

Changes in v3:
 * Reworked commit message;
 * Made `extra_len` const;
 * Added `burst_extra_len` to `struct adis`;
 * Changed adis16400 accordingly.

 drivers/iio/imu/adis16400.c   |  2 +-
 drivers/iio/imu/adis_buffer.c | 13 +++++++++----
 include/linux/iio/imu/adis.h  |  9 +++++++--
 3 files changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/iio/imu/adis16400.c b/drivers/iio/imu/adis16400.c
index 05e70c1c4835..ef9d26151448 100644
--- a/drivers/iio/imu/adis16400.c
+++ b/drivers/iio/imu/adis16400.c
@@ -1194,7 +1194,7 @@ static int adis16400_probe(struct spi_device *spi)
 		indio_dev->available_scan_masks = st->avail_scan_mask;
 		st->adis.burst = &adis16400_burst;
 		if (st->variant->flags & ADIS16400_BURST_DIAG_STAT)
-			st->adis.burst->extra_len = sizeof(u16);
+			st->adis.burst_extra_len = sizeof(u16);
 	}
 
 	adis16400_data = &st->variant->adis_data;
diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
index c2211ab80d8c..22d95cb3ded8 100644
--- a/drivers/iio/imu/adis_buffer.c
+++ b/drivers/iio/imu/adis_buffer.c
@@ -23,25 +23,30 @@ static int adis_update_scan_mode_burst(struct iio_dev *indio_dev,
 	const unsigned long *scan_mask)
 {
 	struct adis *adis = iio_device_get_drvdata(indio_dev);
-	unsigned int burst_length;
+	unsigned int burst_length, burst_max_length;
 	u8 *tx;
 
 	/* All but the timestamp channel */
 	burst_length = (indio_dev->num_channels - 1) * sizeof(u16);
-	burst_length += adis->burst->extra_len;
+	burst_length += adis->burst->extra_len + adis->burst_extra_len;
+
+	if (adis->burst->burst_max_len)
+		burst_max_length = adis->burst->burst_max_len;
+	else
+		burst_max_length = burst_length;
 
 	adis->xfer = kcalloc(2, sizeof(*adis->xfer), GFP_KERNEL);
 	if (!adis->xfer)
 		return -ENOMEM;
 
-	adis->buffer = kzalloc(burst_length + sizeof(u16), GFP_KERNEL);
+	adis->buffer = kzalloc(burst_max_length + sizeof(u16), GFP_KERNEL);
 	if (!adis->buffer) {
 		kfree(adis->xfer);
 		adis->xfer = NULL;
 		return -ENOMEM;
 	}
 
-	tx = adis->buffer + burst_length;
+	tx = adis->buffer + burst_max_length;
 	tx[0] = ADIS_READ_REG(adis->burst->reg_cmd);
 	tx[1] = 0;
 
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 94031b3fc9d5..221715099429 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -83,6 +83,8 @@ struct adis_data {
  * @trig: IIO trigger object data
  * @data: ADIS chip variant specific data
  * @burst: ADIS burst transfer information
+ * @burst_extra_len: Burst extra length. Should only be used by devices that can
+ *		     dynamically change their burst mode length.
  * @state_lock: Lock used by the device to protect state
  * @msg: SPI message object
  * @xfer: SPI transfer objects to be used for a @msg
@@ -98,7 +100,7 @@ struct adis {
 
 	const struct adis_data	*data;
 	struct adis_burst	*burst;
-
+	unsigned int		burst_extra_len;
 	/**
 	 * The state_lock is meant to be used during operations that require
 	 * a sequence of SPI R/W in order to protect the SPI transfer
@@ -502,11 +504,14 @@ int adis_single_conversion(struct iio_dev *indio_dev,
  * @en			burst mode enabled
  * @reg_cmd		register command that triggers burst
  * @extra_len		extra length to account in the SPI RX buffer
+ * @burst_max_len	holds the maximum burst size when the device supports
+ *			more than one burst mode with different sizes
  */
 struct adis_burst {
 	bool		en;
 	unsigned int	reg_cmd;
-	unsigned int	extra_len;
+	const u32	extra_len;
+	const u32	burst_max_len;
 };
 
 int
-- 
2.17.1

[PATCH v3 5/6] iio: imu: Add support for adis16475

[Nuno Sá]

Support ADIS16475 and similar IMU devices. These devices are
a precision, miniature MEMS inertial measurement unit (IMU) that
includes a triaxial gyroscope and a triaxial accelerometer. Each
inertial sensor combines with signal conditioning that optimizes
dynamic performance.

The driver adds support for the following devices:
 * adis16470, adis16475, adis16477, adis16465, adis16467, adis16500,
   adis16505, adis16507.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
Changes in v2:
 * Remove burst mode config (enabled by default);
 * Fix timestamp channel index;
 * Do return code checks inline in `adis16475_read_raw`;
 * Increase buffer size to fit aligned timestamp;
 * Remove spi_device_id table;
 * Rename `adis16475_clks` to `adis16475_sync`;
 * Rename `adis16475_config_ext_clk` `adis16475_config_sync_mode`;
 * Fix output-sync mode. There's no need to call clk_get in this mode as
it acts as an output pin;
 * Fix scaled-mode configuration. Up scale was not being written on the right register;
 * Handle burst32 configuration at runtime. When the decimation or the FIR filter is enabled
the LSB bits are meangninfull. Hence, use burst32 in that case (if the part supports it);
 * When there's no burst32 mode and the LSB data is meangninfull, read it manually...
 * Do not push data to buffers if crc is invalid.

Changes in V3:
 * Fix multiple instance devices for different burst sizes;
 * Only return error if `freq` is 0 on `adis16475_set_freq``;
 * Fixed typo in comment;
 * Do not allow 7 to be set in `adis16475_set_filter`;
 * Change `adis16475_validate_crc` return type to bool;
 * Return 1 if crc is valid instead of 0.

 MAINTAINERS                 |    8 +
 drivers/iio/imu/Kconfig     |   13 +
 drivers/iio/imu/Makefile    |    1 +
 drivers/iio/imu/adis16475.c | 1344 +++++++++++++++++++++++++++++++++++
 4 files changed, 1366 insertions(+)
 create mode 100644 drivers/iio/imu/adis16475.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 8687cc5397c8..ae9cce22fa40 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1009,6 +1009,14 @@ W:	http://ez.analog.com/community/linux-device-drivers
 F:	drivers/iio/imu/adis16460.c
 F:	Documentation/devicetree/bindings/iio/imu/adi,adis16460.yaml
 
+ANALOG DEVICES INC ADIS16475 DRIVER
+M:	Nuno Sa <nuno.sa@analog.com>
+L:	linux-iio@vger.kernel.org
+W:	http://ez.analog.com/community/linux-device-drivers
+S:	Supported
+F:	drivers/iio/imu/adis16475.c
+F:	Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475
+
 ANALOG DEVICES INC ADM1177 DRIVER
 M:	Beniamin Bia <beniamin.bia@analog.com>
 M:	Michael Hennerich <Michael.Hennerich@analog.com>
diff --git a/drivers/iio/imu/Kconfig b/drivers/iio/imu/Kconfig
index 60bb1029e759..fc4123d518bc 100644
--- a/drivers/iio/imu/Kconfig
+++ b/drivers/iio/imu/Kconfig
@@ -29,6 +29,19 @@ config ADIS16460
 	  To compile this driver as a module, choose M here: the module will be
 	  called adis16460.
 
+config ADIS16475
+	tristate "Analog Devices ADIS16475 and similar IMU driver"
+	depends on SPI
+	select IIO_ADIS_LIB
+	select IIO_ADIS_LIB_BUFFER if IIO_BUFFER
+	help
+	  Say yes here to build support for Analog Devices ADIS16470, ADIS16475,
+	  ADIS16477, ADIS16465, ADIS16467, ADIS16500, ADIS16505, ADIS16507 inertial
+	  sensors.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called adis16475.
+
 config ADIS16480
 	tristate "Analog Devices ADIS16480 and similar IMU driver"
 	depends on SPI
diff --git a/drivers/iio/imu/Makefile b/drivers/iio/imu/Makefile
index 5237fd4bc384..88b2c4555230 100644
--- a/drivers/iio/imu/Makefile
+++ b/drivers/iio/imu/Makefile
@@ -6,6 +6,7 @@
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_ADIS16400) += adis16400.o
 obj-$(CONFIG_ADIS16460) += adis16460.o
+obj-$(CONFIG_ADIS16475) += adis16475.o
 obj-$(CONFIG_ADIS16480) += adis16480.o
 
 adis_lib-y += adis.o
diff --git a/drivers/iio/imu/adis16475.c b/drivers/iio/imu/adis16475.c
new file mode 100644
index 000000000000..4ed03d4a28d0
--- /dev/null
+++ b/drivers/iio/imu/adis16475.c
@@ -0,0 +1,1344 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ADIS16475 IMU driver
+ *
+ * Copyright 2019 Analog Devices Inc.
+ */
+#include <asm/unaligned.h>
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/debugfs.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/imu/adis.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/irq.h>
+#include <linux/module.h>
+#include <linux/of_device.h>
+#include <linux/spi/spi.h>
+
+#define ADIS16475_REG_DIAG_STAT		0x02
+#define ADIS16475_REG_X_GYRO_L		0x04
+#define ADIS16475_REG_Y_GYRO_L		0x08
+#define ADIS16475_REG_Z_GYRO_L		0x0C
+#define ADIS16475_REG_X_ACCEL_L		0x10
+#define ADIS16475_REG_Y_ACCEL_L		0x14
+#define ADIS16475_REG_Z_ACCEL_L		0x18
+#define ADIS16475_REG_TEMP_OUT		0x1c
+#define ADIS16475_REG_X_GYRO_BIAS_L	0x40
+#define ADIS16475_REG_Y_GYRO_BIAS_L	0x44
+#define ADIS16475_REG_Z_GYRO_BIAS_L	0x48
+#define ADIS16475_REG_X_ACCEL_BIAS_L	0x4c
+#define ADIS16475_REG_Y_ACCEL_BIAS_L	0x50
+#define ADIS16475_REG_Z_ACCEL_BIAS_L	0x54
+#define ADIS16475_REG_FILT_CTRL		0x5c
+#define ADIS16475_FILT_CTRL_MASK	GENMASK(2, 0)
+#define ADIS16475_FILT_CTRL(x)		FIELD_PREP(ADIS16475_FILT_CTRL_MASK, x)
+#define ADIS16475_REG_MSG_CTRL		0x60
+#define ADIS16475_MSG_CTRL_DR_POL_MASK	BIT(0)
+#define ADIS16475_MSG_CTRL_DR_POL(x) \
+				FIELD_PREP(ADIS16475_MSG_CTRL_DR_POL_MASK, x)
+#define ADIS16475_SYNC_MODE_MASK	GENMASK(4, 2)
+#define ADIS16475_SYNC_MODE(x)		FIELD_PREP(ADIS16475_SYNC_MODE_MASK, x)
+#define ADIS16475_REG_UP_SCALE		0x62
+#define ADIS16475_REG_DEC_RATE		0x64
+#define ADIS16475_REG_GLOB_CMD		0x68
+#define ADIS16475_REG_FIRM_REV		0x6c
+#define ADIS16475_REG_FIRM_DM		0x6e
+#define ADIS16475_REG_FIRM_Y		0x70
+#define ADIS16475_REG_PROD_ID		0x72
+#define ADIS16475_REG_SERIAL_NUM	0x74
+#define ADIS16475_REG_FLASH_CNT		0x7c
+#define ADIS16500_BURST32_MASK		BIT(9)
+#define ADIS16500_BURST32(x)		FIELD_PREP(ADIS16500_BURST32_MASK, x)
+/* number of data elements in burst mode */
+#define ADIS16475_BURST32_MAX_DATA	32
+#define ADIS16475_BURST_MAX_DATA	20
+#define ADIS16475_MAX_SCAN_DATA		20
+/* spi max speed in brust mode */
+#define ADIS16475_BURST_MAX_SPEED	1000000
+#define ADIS16475_LSB_DEC_MASK		BIT(0)
+#define ADIS16475_LSB_FIR_MASK		BIT(1)
+
+enum {
+	ADIS16475_SYNC_DIRECT = 1,
+	ADIS16475_SYNC_SCALED,
+	ADIS16475_SYNC_OUTPUT,
+	ADIS16475_SYNC_PULSE = 5,
+};
+
+struct adis16475_sync {
+	u16 sync_mode;
+	u16 min_rate;
+	u16 max_rate;
+};
+
+struct adis16475_chip_info {
+	const struct iio_chan_spec *channels;
+	const struct adis16475_sync *sync;
+	const struct adis_data adis_data;
+	const char *name;
+	u32 num_channels;
+	u32 gyro_max_val;
+	u32 gyro_max_scale;
+	u32 accel_max_val;
+	u32 accel_max_scale;
+	u32 temp_scale;
+	u32 int_clk;
+	u16 max_dec;
+	u8 num_sync;
+	bool has_burst32;
+};
+
+struct adis16475 {
+	const struct adis16475_chip_info *info;
+	struct adis adis;
+	u32 clk_freq;
+	bool burst32;
+	unsigned long lsb_flag;
+};
+
+enum {
+	ADIS16475_SCAN_GYRO_X,
+	ADIS16475_SCAN_GYRO_Y,
+	ADIS16475_SCAN_GYRO_Z,
+	ADIS16475_SCAN_ACCEL_X,
+	ADIS16475_SCAN_ACCEL_Y,
+	ADIS16475_SCAN_ACCEL_Z,
+	ADIS16475_SCAN_TEMP,
+	ADIS16475_SCAN_DIAG_S_FLAGS,
+	ADIS16475_SCAN_CRC_FAILURE,
+};
+
+#ifdef CONFIG_DEBUG_FS
+static ssize_t adis16475_show_firmware_revision(struct file *file,
+						char __user *userbuf,
+						size_t count, loff_t *ppos)
+{
+	struct adis16475 *st = file->private_data;
+	char buf[7];
+	size_t len;
+	u16 rev;
+	int ret;
+
+	ret = adis_read_reg_16(&st->adis, ADIS16475_REG_FIRM_REV, &rev);
+	if (ret)
+		return ret;
+
+	len = scnprintf(buf, sizeof(buf), "%x.%x\n", rev >> 8, rev & 0xff);
+
+	return simple_read_from_buffer(userbuf, count, ppos, buf, len);
+}
+
+static const struct file_operations adis16475_firmware_revision_fops = {
+	.open = simple_open,
+	.read = adis16475_show_firmware_revision,
+	.llseek = default_llseek,
+	.owner = THIS_MODULE,
+};
+
+static ssize_t adis16475_show_firmware_date(struct file *file,
+					    char __user *userbuf,
+					    size_t count, loff_t *ppos)
+{
+	struct adis16475 *st = file->private_data;
+	u16 md, year;
+	char buf[12];
+	size_t len;
+	int ret;
+
+	ret = adis_read_reg_16(&st->adis, ADIS16475_REG_FIRM_Y, &year);
+	if (ret)
+		return ret;
+
+	ret = adis_read_reg_16(&st->adis, ADIS16475_REG_FIRM_DM, &md);
+	if (ret)
+		return ret;
+
+	len = snprintf(buf, sizeof(buf), "%.2x-%.2x-%.4x\n", md >> 8, md & 0xff,
+		       year);
+
+	return simple_read_from_buffer(userbuf, count, ppos, buf, len);
+}
+
+static const struct file_operations adis16475_firmware_date_fops = {
+	.open = simple_open,
+	.read = adis16475_show_firmware_date,
+	.llseek = default_llseek,
+	.owner = THIS_MODULE,
+};
+
+static int adis16475_show_serial_number(void *arg, u64 *val)
+{
+	struct adis16475 *st = arg;
+	u16 serial;
+	int ret;
+
+	ret = adis_read_reg_16(&st->adis, ADIS16475_REG_SERIAL_NUM, &serial);
+	if (ret)
+		return ret;
+
+	*val = serial;
+
+	return 0;
+}
+DEFINE_SIMPLE_ATTRIBUTE(adis16475_serial_number_fops,
+			adis16475_show_serial_number, NULL, "0x%.4llx\n");
+
+static int adis16475_show_product_id(void *arg, u64 *val)
+{
+	struct adis16475 *st = arg;
+	u16 prod_id;
+	int ret;
+
+	ret = adis_read_reg_16(&st->adis, ADIS16475_REG_PROD_ID, &prod_id);
+	if (ret)
+		return ret;
+
+	*val = prod_id;
+
+	return 0;
+}
+DEFINE_SIMPLE_ATTRIBUTE(adis16475_product_id_fops,
+			adis16475_show_product_id, NULL, "%llu\n");
+
+static int adis16475_show_flash_count(void *arg, u64 *val)
+{
+	struct adis16475 *st = arg;
+	u32 flash_count;
+	int ret;
+
+	ret = adis_read_reg_32(&st->adis, ADIS16475_REG_FLASH_CNT,
+			       &flash_count);
+	if (ret)
+		return ret;
+
+	*val = flash_count;
+
+	return 0;
+}
+DEFINE_SIMPLE_ATTRIBUTE(adis16475_flash_count_fops,
+			adis16475_show_flash_count, NULL, "%lld\n");
+
+static int adis16475_debugfs_init(struct iio_dev *indio_dev)
+{
+	struct adis16475 *st = iio_priv(indio_dev);
+
+	debugfs_create_file("serial_number", 0400, indio_dev->debugfs_dentry,
+			    st, &adis16475_serial_number_fops);
+	debugfs_create_file("product_id", 0400, indio_dev->debugfs_dentry, st,
+			    &adis16475_product_id_fops);
+	debugfs_create_file("flash_count", 0400, indio_dev->debugfs_dentry, st,
+			    &adis16475_flash_count_fops);
+	debugfs_create_file("firmware_revision", 0400,
+			    indio_dev->debugfs_dentry, st,
+			    &adis16475_firmware_revision_fops);
+	debugfs_create_file("firmware_date", 0400, indio_dev->debugfs_dentry,
+			    st, &adis16475_firmware_date_fops);
+	return 0;
+}
+#else
+static int adis16475_debugfs_init(struct iio_dev *indio_dev)
+{
+	return 0;
+}
+#endif
+
+static int adis16475_get_freq(struct adis16475 *st, u32 *freq)
+{
+	int ret;
+	u16 dec;
+
+	ret = adis_read_reg_16(&st->adis, ADIS16475_REG_DEC_RATE, &dec);
+	if (ret)
+		return -EINVAL;
+
+	*freq = DIV_ROUND_CLOSEST(st->clk_freq, dec + 1);
+
+	return 0;
+}
+
+static int adis16475_set_freq(struct adis16475 *st, const u32 freq)
+{
+	u16 dec;
+	int ret;
+
+	if (!freq)
+		return -EINVAL;
+
+	dec = DIV_ROUND_CLOSEST(st->clk_freq, freq);
+
+	if (dec)
+		dec--;
+
+	if (dec > st->info->max_dec)
+		dec = st->info->max_dec;
+
+	ret = adis_write_reg_16(&st->adis, ADIS16475_REG_DEC_RATE, dec);
+	if (ret)
+		return ret;
+
+	/*
+	 * If decimation is used, then gyro and accel data will have meaningful
+	 * bits on the LSB registers. This info is used on the trigger handler.
+	 */
+	if (!dec)
+		clear_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag);
+	else
+		set_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag);
+
+	return 0;
+}
+
+/* The values are approximated. */
+static const u32 adis16475_3db_freqs[] = {
+	[0] = 720, /* Filter disabled, full BW (~720Hz) */
+	[1] = 360,
+	[2] = 164,
+	[3] = 80,
+	[4] = 40,
+	[5] = 20,
+	[6] = 10,
+	[7] = 10, /* not a valid setting */
+};
+
+static int adis16475_get_filter(struct adis16475 *st, u32 *filter)
+{
+	u16 filter_sz;
+	int ret;
+	const int mask = ADIS16475_FILT_CTRL_MASK;
+
+	ret = adis_read_reg_16(&st->adis, ADIS16475_REG_FILT_CTRL, &filter_sz);
+	if (ret)
+		return ret;
+
+	*filter = adis16475_3db_freqs[filter_sz & mask];
+
+	return 0;
+}
+
+static int adis16475_set_filter(struct adis16475 *st, const u32 filter)
+{
+	int i;
+	int ret;
+
+	for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) {
+		if (adis16475_3db_freqs[i] >= filter)
+			break;
+	}
+
+	ret = adis_write_reg_16(&st->adis, ADIS16475_REG_FILT_CTRL,
+				ADIS16475_FILT_CTRL(i));
+	if (ret)
+		return ret;
+
+	/*
+	 * If FIR is used, then gyro and accel data will have meaningful
+	 * bits on the LSB registers. This info is used on the trigger handler.
+	 */
+	if (!i)
+		clear_bit(ADIS16475_LSB_FIR_MASK, &st->lsb_flag);
+	else
+		set_bit(ADIS16475_LSB_FIR_MASK, &st->lsb_flag);
+
+	return 0;
+}
+
+static const u32 adis16475_calib_regs[] = {
+	[ADIS16475_SCAN_GYRO_X] = ADIS16475_REG_X_GYRO_BIAS_L,
+	[ADIS16475_SCAN_GYRO_Y] = ADIS16475_REG_Y_GYRO_BIAS_L,
+	[ADIS16475_SCAN_GYRO_Z] = ADIS16475_REG_Z_GYRO_BIAS_L,
+	[ADIS16475_SCAN_ACCEL_X] = ADIS16475_REG_X_ACCEL_BIAS_L,
+	[ADIS16475_SCAN_ACCEL_Y] = ADIS16475_REG_Y_ACCEL_BIAS_L,
+	[ADIS16475_SCAN_ACCEL_Z] = ADIS16475_REG_Z_ACCEL_BIAS_L,
+};
+
+static int adis16475_read_raw(struct iio_dev *indio_dev,
+			      const struct iio_chan_spec *chan,
+			      int *val, int *val2, long info)
+{
+	struct adis16475 *st = iio_priv(indio_dev);
+	int ret;
+	u32 tmp;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		return adis_single_conversion(indio_dev, chan, 0, val);
+	case IIO_CHAN_INFO_SCALE:
+		switch (chan->type) {
+		case IIO_ANGL_VEL:
+			*val = st->info->gyro_max_val;
+			*val2 = st->info->gyro_max_scale;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_ACCEL:
+			*val = st->info->accel_max_val;
+			*val2 = st->info->accel_max_scale;
+			return IIO_VAL_FRACTIONAL;
+		case IIO_TEMP:
+			*val = st->info->temp_scale;
+			return IIO_VAL_INT;
+		default:
+			return -EINVAL;
+		}
+	case IIO_CHAN_INFO_CALIBBIAS:
+		ret = adis_read_reg_32(&st->adis,
+				       adis16475_calib_regs[chan->scan_index],
+				       val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		ret = adis16475_get_filter(st, val);
+		if (ret)
+			return ret;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		ret = adis16475_get_freq(st, &tmp);
+		if (ret)
+			return ret;
+
+		*val = tmp / 1000;
+		*val2 = (tmp % 1000) * 1000;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int adis16475_write_raw(struct iio_dev *indio_dev,
+			       const struct iio_chan_spec *chan,
+			       int val, int val2, long info)
+{
+	struct adis16475 *st = iio_priv(indio_dev);
+	u32 tmp;
+
+	switch (info) {
+	case IIO_CHAN_INFO_SAMP_FREQ:
+		tmp = val * 1000 + val2 / 1000;
+		return adis16475_set_freq(st, tmp);
+	case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
+		return adis16475_set_filter(st, val);
+	case IIO_CHAN_INFO_CALIBBIAS:
+		return adis_write_reg_32(&st->adis,
+					 adis16475_calib_regs[chan->scan_index],
+					 val);
+	default:
+		return -EINVAL;
+	}
+}
+
+#define ADIS16475_MOD_CHAN(_type, _mod, _address, _si, _r_bits, _s_bits) \
+	{ \
+		.type = (_type), \
+		.modified = 1, \
+		.channel2 = (_mod), \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+			BIT(IIO_CHAN_INFO_CALIBBIAS), \
+		.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+			BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \
+		.address = (_address), \
+		.scan_index = (_si), \
+		.scan_type = { \
+			.sign = 's', \
+			.realbits = (_r_bits), \
+			.storagebits = (_s_bits), \
+			.endianness = IIO_BE, \
+		}, \
+	}
+
+#define ADIS16475_GYRO_CHANNEL(_mod) \
+	ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \
+	ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_ ## _mod, 32, \
+	32)
+
+#define ADIS16475_ACCEL_CHANNEL(_mod) \
+	ADIS16475_MOD_CHAN(IIO_ACCEL, IIO_MOD_ ## _mod, \
+	ADIS16475_REG_ ## _mod ## _ACCEL_L, ADIS16475_SCAN_ACCEL_ ## _mod, 32, \
+	32)
+
+#define ADIS16475_TEMP_CHANNEL() { \
+		.type = IIO_TEMP, \
+		.indexed = 1, \
+		.channel = 0, \
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+			BIT(IIO_CHAN_INFO_SCALE), \
+		.info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
+			BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY), \
+		.address = ADIS16475_REG_TEMP_OUT, \
+		.scan_index = ADIS16475_SCAN_TEMP, \
+		.scan_type = { \
+			.sign = 's', \
+			.realbits = 16, \
+			.storagebits = 16, \
+			.endianness = IIO_BE, \
+		}, \
+	}
+
+static const struct iio_chan_spec adis16475_channels[] = {
+	ADIS16475_GYRO_CHANNEL(X),
+	ADIS16475_GYRO_CHANNEL(Y),
+	ADIS16475_GYRO_CHANNEL(Z),
+	ADIS16475_ACCEL_CHANNEL(X),
+	ADIS16475_ACCEL_CHANNEL(Y),
+	ADIS16475_ACCEL_CHANNEL(Z),
+	ADIS16475_TEMP_CHANNEL(),
+	IIO_CHAN_SOFT_TIMESTAMP(7)
+};
+
+enum adis16475_variant {
+	ADIS16470,
+	ADIS16475_1,
+	ADIS16475_2,
+	ADIS16475_3,
+	ADIS16477_1,
+	ADIS16477_2,
+	ADIS16477_3,
+	ADIS16465_1,
+	ADIS16465_2,
+	ADIS16465_3,
+	ADIS16467_1,
+	ADIS16467_2,
+	ADIS16467_3,
+	ADIS16500,
+	ADIS16505_1,
+	ADIS16505_2,
+	ADIS16505_3,
+	ADIS16507_1,
+	ADIS16507_2,
+	ADIS16507_3,
+
+};
+
+enum {
+	ADIS16475_DIAG_STAT_DATA_PATH = 1,
+	ADIS16475_DIAG_STAT_FLASH_MEM,
+	ADIS16475_DIAG_STAT_SPI,
+	ADIS16475_DIAG_STAT_STANDBY,
+	ADIS16475_DIAG_STAT_SENSOR,
+	ADIS16475_DIAG_STAT_MEMORY,
+	ADIS16475_DIAG_STAT_CLK,
+};
+
+static const char * const adis16475_status_error_msgs[] = {
+	[ADIS16475_DIAG_STAT_DATA_PATH] = "Data Path Overrun",
+	[ADIS16475_DIAG_STAT_FLASH_MEM] = "Flash memory update failure",
+	[ADIS16475_DIAG_STAT_SPI] = "SPI communication error",
+	[ADIS16475_DIAG_STAT_STANDBY] = "Standby mode",
+	[ADIS16475_DIAG_STAT_SENSOR] = "Sensor failure",
+	[ADIS16475_DIAG_STAT_MEMORY] = "Memory failure",
+	[ADIS16475_DIAG_STAT_CLK] = "Clock error",
+};
+
+static int adis16475_enable_irq(struct adis *adis, bool enable)
+{
+	/*
+	 * There is no way to gate the data-ready signal internally inside the
+	 * ADIS16475. We can only control it's polarity...
+	 */
+	if (enable)
+		enable_irq(adis->spi->irq);
+	else
+		disable_irq(adis->spi->irq);
+
+	return 0;
+}
+
+#define ADIS16475_DATA(_prod_id, _timeouts)				\
+{									\
+	.msc_ctrl_reg = ADIS16475_REG_MSG_CTRL,				\
+	.glob_cmd_reg = ADIS16475_REG_GLOB_CMD,				\
+	.diag_stat_reg = ADIS16475_REG_DIAG_STAT,			\
+	.prod_id_reg = ADIS16475_REG_PROD_ID,				\
+	.prod_id = (_prod_id),						\
+	.self_test_mask = BIT(2),					\
+	.self_test_reg = ADIS16475_REG_GLOB_CMD,			\
+	.cs_change_delay = 16,						\
+	.read_delay = 5,						\
+	.write_delay = 5,						\
+	.status_error_msgs = adis16475_status_error_msgs,		\
+	.status_error_mask = BIT(ADIS16475_DIAG_STAT_DATA_PATH) |	\
+		BIT(ADIS16475_DIAG_STAT_FLASH_MEM) |			\
+		BIT(ADIS16475_DIAG_STAT_SPI) |				\
+		BIT(ADIS16475_DIAG_STAT_STANDBY) |			\
+		BIT(ADIS16475_DIAG_STAT_SENSOR) |			\
+		BIT(ADIS16475_DIAG_STAT_MEMORY) |			\
+		BIT(ADIS16475_DIAG_STAT_CLK),				\
+	.enable_irq = adis16475_enable_irq,				\
+	.timeouts = (_timeouts),					\
+}
+
+static const struct adis16475_sync adis16475_sync_mode[] = {
+	{ ADIS16475_SYNC_OUTPUT },
+	{ ADIS16475_SYNC_DIRECT, 1900, 2100 },
+	{ ADIS16475_SYNC_SCALED, 1, 128 },
+	{ ADIS16475_SYNC_PULSE, 1000, 2100 },
+};
+
+static const struct adis_timeout adis16475_timeouts = {
+	.reset_ms = 200,
+	.sw_reset_ms = 200,
+	.self_test_ms = 20,
+};
+
+static const struct adis_timeout adis1650x_timeouts = {
+	.reset_ms = 260,
+	.sw_reset_ms = 260,
+	.self_test_ms = 30,
+};
+
+static const struct adis16475_chip_info adis16475_chip_info[] = {
+	[ADIS16470] = {
+		.name = "adis16470",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(10 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16470, &adis16475_timeouts),
+	},
+	[ADIS16475_1] = {
+		.name = "adis16475-1",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(160 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(4000 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16475, &adis16475_timeouts),
+	},
+	[ADIS16475_2] = {
+		.name = "adis16475-2",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(40 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(4000 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16475, &adis16475_timeouts),
+	},
+	[ADIS16475_3] = {
+		.name = "adis16475-3",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(10 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(4000 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16475, &adis16475_timeouts),
+	},
+	[ADIS16477_1] = {
+		.name = "adis16477-1",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(160 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16477, &adis16475_timeouts),
+	},
+	[ADIS16477_2] = {
+		.name = "adis16477-2",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(40 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16477, &adis16475_timeouts),
+	},
+	[ADIS16477_3] = {
+		.name = "adis16477-3",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(10 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16477, &adis16475_timeouts),
+	},
+	[ADIS16465_1] = {
+		.name = "adis16465-1",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(160 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(4000 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16465, &adis16475_timeouts),
+	},
+	[ADIS16465_2] = {
+		.name = "adis16465-2",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(40 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(4000 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16465, &adis16475_timeouts),
+	},
+	[ADIS16465_3] = {
+		.name = "adis16465-3",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(10 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(4000 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16465, &adis16475_timeouts),
+	},
+	[ADIS16467_1] = {
+		.name = "adis16467-1",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(160 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16467, &adis16475_timeouts),
+	},
+	[ADIS16467_2] = {
+		.name = "adis16467-2",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(40 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16467, &adis16475_timeouts),
+	},
+	[ADIS16467_3] = {
+		.name = "adis16467-3",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(10 << 16),
+		.accel_max_val = 1,
+		.accel_max_scale = IIO_M_S_2_TO_G(800 << 16),
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode),
+		.adis_data = ADIS16475_DATA(16467, &adis16475_timeouts),
+	},
+	[ADIS16500] = {
+		.name = "adis16500",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(10 << 16),
+		.accel_max_val = 392,
+		.accel_max_scale = 32000 << 16,
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		/* pulse sync not supported */
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode) - 1,
+		.has_burst32 = true,
+		.adis_data = ADIS16475_DATA(16500, &adis1650x_timeouts),
+	},
+	[ADIS16505_1] = {
+		.name = "adis16505-1",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(160 << 16),
+		.accel_max_val = 78,
+		.accel_max_scale = 32000 << 16,
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		/* pulse sync not supported */
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode) - 1,
+		.has_burst32 = true,
+		.adis_data = ADIS16475_DATA(16505, &adis1650x_timeouts),
+	},
+	[ADIS16505_2] = {
+		.name = "adis16505-2",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(40 << 16),
+		.accel_max_val = 78,
+		.accel_max_scale = 32000 << 16,
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		/* pulse sync not supported */
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode) - 1,
+		.has_burst32 = true,
+		.adis_data = ADIS16475_DATA(16505, &adis1650x_timeouts),
+	},
+	[ADIS16505_3] = {
+		.name = "adis16505-3",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(10 << 16),
+		.accel_max_val = 78,
+		.accel_max_scale = 32000 << 16,
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		/* pulse sync not supported */
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode) - 1,
+		.has_burst32 = true,
+		.adis_data = ADIS16475_DATA(16505, &adis1650x_timeouts),
+	},
+	[ADIS16507_1] = {
+		.name = "adis16507-1",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(160 << 16),
+		.accel_max_val = 392,
+		.accel_max_scale = 32000 << 16,
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		/* pulse sync not supported */
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode) - 1,
+		.has_burst32 = true,
+		.adis_data = ADIS16475_DATA(16507, &adis1650x_timeouts),
+	},
+	[ADIS16507_2] = {
+		.name = "adis16507-2",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(40 << 16),
+		.accel_max_val = 392,
+		.accel_max_scale = 32000 << 16,
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		/* pulse sync not supported */
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode) - 1,
+		.has_burst32 = true,
+		.adis_data = ADIS16475_DATA(16507, &adis1650x_timeouts),
+	},
+	[ADIS16507_3] = {
+		.name = "adis16507-3",
+		.num_channels = ARRAY_SIZE(adis16475_channels),
+		.channels = adis16475_channels,
+		.gyro_max_val = 1,
+		.gyro_max_scale = IIO_RAD_TO_DEGREE(10 << 16),
+		.accel_max_val = 392,
+		.accel_max_scale = 32000 << 16,
+		.temp_scale = 100,
+		.int_clk = 2000,
+		.max_dec = 1999,
+		.sync = adis16475_sync_mode,
+		/* pulse sync not supported */
+		.num_sync = ARRAY_SIZE(adis16475_sync_mode) - 1,
+		.has_burst32 = true,
+		.adis_data = ADIS16475_DATA(16507, &adis1650x_timeouts),
+	},
+};
+
+static const struct iio_info adis16475_info = {
+	.read_raw = &adis16475_read_raw,
+	.write_raw = &adis16475_write_raw,
+	.update_scan_mode = adis_update_scan_mode,
+	.debugfs_reg_access = adis_debugfs_reg_access,
+};
+
+static struct adis_burst adis16475_burst = {
+	.en = true,
+	.reg_cmd = ADIS16475_REG_GLOB_CMD,
+	/*
+	 * adis_update_scan_mode_burst() sets the burst length in respect with
+	 * the number of channels and allocates 16 bits for each. However,
+	 * adis1647x devices also need space for DIAG_STAT, DATA_CNTR or
+	 * TIME_STAMP (depending on the clock mode but for us these bytes are
+	 * don't care...) and CRC.
+	 */
+	.extra_len = 3 * sizeof(u16),
+	.burst_max_len = ADIS16475_BURST32_MAX_DATA,
+};
+
+static bool adis16475_validate_crc(const u8 *buffer, u16 crc,
+				   const bool burst32)
+{
+	int i;
+	/* extra 6 elements for low gyro and accel */
+	const u16 sz = burst32 ? ADIS16475_BURST32_MAX_DATA :
+		ADIS16475_BURST_MAX_DATA;
+
+	for (i = 0; i < sz - 2; i++)
+		crc -= buffer[i];
+
+	return (crc == 0);
+}
+
+static void adis16475_burst32_check(struct adis16475 *st)
+{
+	int ret;
+	struct adis *adis = &st->adis;
+
+	if (!st->info->has_burst32)
+		return;
+
+	if (st->lsb_flag && !st->burst32) {
+		const u16 en = ADIS16500_BURST32(1);
+
+		ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
+					 ADIS16500_BURST32_MASK, en);
+		if (ret < 0)
+			return;
+
+		st->burst32 = true;
+		/*
+		 * In 32bit mode we need extra 2 bytes for all gyro
+		 * and accel channels.
+		 */
+		adis->burst_extra_len = 6 * sizeof(u16);
+		adis->xfer[1].len += 6 * sizeof(u16);
+		dev_dbg(&adis->spi->dev, "Enable burst32 mode, xfer:%d",
+			adis->xfer[1].len);
+
+	} else if (!st->lsb_flag && st->burst32) {
+		const u16 en = ADIS16500_BURST32(0);
+
+		ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
+					 ADIS16500_BURST32_MASK, en);
+		if (ret < 0)
+			return;
+
+		st->burst32 = false;
+		/* Remove the extra bits */
+		adis->burst_extra_len = 0;
+		adis->xfer[1].len -= 6 * sizeof(u16);
+		dev_dbg(&adis->spi->dev, "Disable burst32 mode, xfer:%d\n",
+			adis->xfer[1].len);
+	}
+}
+
+static irqreturn_t adis16475_trigger_handler(int irq, void *p)
+{
+	struct iio_poll_func *pf = p;
+	struct iio_dev *indio_dev = pf->indio_dev;
+	struct adis16475 *st = iio_priv(indio_dev);
+	struct adis *adis = &st->adis;
+	int ret, bit, i = 0;
+	u16 crc, data[ADIS16475_MAX_SCAN_DATA], *buffer;
+	bool valid;
+	/* offset until the first element after gyro and accel */
+	const u8 offset = st->burst32 ? 13 : 7;
+	const u32 cached_spi_speed_hz = adis->spi->max_speed_hz;
+
+	adis->spi->max_speed_hz = ADIS16475_BURST_MAX_SPEED;
+
+	ret = spi_sync(adis->spi, &adis->msg);
+	if (ret)
+		return ret;
+
+	adis->spi->max_speed_hz = cached_spi_speed_hz;
+	buffer = (u16 *)adis->buffer;
+
+	crc = get_unaligned_be16(&buffer[offset + 2]);
+	valid = adis16475_validate_crc((u8 *)adis->buffer, crc, st->burst32);
+	if (!valid) {
+		dev_err(&adis->spi->dev, "Invalid crc\n");
+		goto check_burst32;
+	}
+
+	for_each_set_bit(bit, indio_dev->active_scan_mask,
+			 indio_dev->masklength) {
+		/*
+		 * When burst mode is used, system flags is the first data
+		 * channel in the sequence, but the scan index is 7.
+		 */
+		switch (bit) {
+		case ADIS16475_SCAN_TEMP:
+			data[i++] = get_unaligned(&buffer[offset]);
+			break;
+		case ADIS16475_SCAN_GYRO_X ... ADIS16475_SCAN_ACCEL_Z:
+			/*
+			 * The first 2 bytes on the received data are the
+			 * DIAG_STAT reg, hence the +1 offset here...
+			 */
+			if (st->burst32) {
+				/* upper 16 */
+				data[i++] = get_unaligned(&buffer[bit * 2 + 2]);
+				/* lower 16 */
+				data[i++] = get_unaligned(&buffer[bit * 2 + 1]);
+			} else {
+				data[i++] = get_unaligned(&buffer[bit + 1]);
+				/*
+				 * Don't bother in doing the manual read if the
+				 * device supports burst32. burst32 will be
+				 * enabled in the next call to
+				 * adis16475_burst32_check()...
+				 */
+				if (st->lsb_flag && !st->info->has_burst32) {
+					u16 val = 0;
+					const u32 reg = ADIS16475_REG_X_GYRO_L +
+						(bit * 4);
+					__be16 __val;
+
+					adis_read_reg_16(adis, reg, &val);
+					__val = cpu_to_be16(val);
+					/* keep sparse happy */
+					data[i++] = (__force u16)__val;
+				} else {
+					/* lower not used */
+					data[i++] = 0;
+				}
+			}
+			break;
+		}
+	}
+
+	buffer = data;
+
+	iio_push_to_buffers_with_timestamp(indio_dev, buffer, pf->timestamp);
+check_burst32:
+	/*
+	 * We only check the burst mode at the end of the current capture since
+	 * it takes a full data ready cycle for the device to update the burst
+	 * array.
+	 */
+	adis16475_burst32_check(st);
+	iio_trigger_notify_done(indio_dev->trig);
+
+	return IRQ_HANDLED;
+}
+
+static void adis16475_disable_clk(void *data)
+{
+	clk_disable_unprepare((struct clk *)data);
+}
+
+static int adis16475_config_sync_mode(struct adis16475 *st)
+{
+	int ret;
+	struct device *dev = &st->adis.spi->dev;
+	const struct adis16475_sync *sync;
+	u32 sync_mode;
+
+	/* default to internal clk */
+	st->clk_freq = st->info->int_clk * 1000;
+
+	ret = device_property_read_u32(dev, "adi,sync-mode", &sync_mode);
+	if (ret)
+		return 0;
+
+	if (sync_mode >= st->info->num_sync) {
+		dev_err(dev, "Invalid sync mode: %u for %s\n", sync_mode,
+			st->info->name);
+		return -EINVAL;
+	}
+
+	sync = &st->info->sync[sync_mode];
+
+	/* All the other modes require external input signal */
+	if (sync->sync_mode != ADIS16475_SYNC_OUTPUT) {
+		struct clk *clk = devm_clk_get(dev, NULL);
+
+		if (IS_ERR(clk))
+			return PTR_ERR(clk);
+
+		ret = clk_prepare_enable(clk);
+		if (ret)
+			return ret;
+
+		ret = devm_add_action_or_reset(dev, adis16475_disable_clk, clk);
+		if (ret)
+			return ret;
+
+		st->clk_freq = clk_get_rate(clk);
+		if (st->clk_freq < sync->min_rate ||
+		    st->clk_freq > sync->max_rate) {
+			dev_err(dev,
+				"Clk rate:%u not in a valid range:[%u %u]\n",
+				st->clk_freq, sync->min_rate, sync->max_rate);
+			return -EINVAL;
+		}
+
+		if (sync->sync_mode == ADIS16475_SYNC_SCALED) {
+			u16 up_scale;
+			u32 scaled_out_freq = 0;
+			/*
+			 * If we are in scaled mode, we must have an up_scale.
+			 * In scaled mode the allowable input clock range is
+			 * 1 Hz to 128 Hz, and the allowable output range is
+			 * 1900 to 2100 Hz. Hence, a scale must be given to
+			 * get the allowable output.
+			 */
+			device_property_read_u32(dev, "adi,scaled-output-hz",
+						 &scaled_out_freq);
+
+			if (scaled_out_freq < 1900 || scaled_out_freq > 2100) {
+				dev_err(dev,
+					"Invalid value:%u for adi,scaled-output-hz",
+					scaled_out_freq);
+				return -EINVAL;
+			}
+
+			up_scale = DIV_ROUND_CLOSEST(scaled_out_freq,
+						     st->clk_freq);
+
+			ret = __adis_write_reg_16(&st->adis,
+						  ADIS16475_REG_UP_SCALE,
+						  up_scale);
+			if (ret)
+				return ret;
+
+			st->clk_freq = scaled_out_freq;
+		}
+
+		st->clk_freq *= 1000;
+	}
+	/*
+	 * Keep in mind that the mask for the clk modes in adis1650*
+	 * chips is different (1100 instead of 11100). However, we
+	 * are not configuring BIT(4) in these chips and the default
+	 * value is 0, so we are fine in doing the below operations.
+	 * I'm keeping this for simplicity and avoiding extra variables
+	 * in chip_info.
+	 */
+	ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
+				 ADIS16475_SYNC_MODE_MASK, sync->sync_mode);
+	if (ret)
+		return ret;
+
+	usleep_range(250, 260);
+
+	return 0;
+}
+
+static int adis16475_config_irq_pin(struct adis16475 *st)
+{
+	int ret;
+	struct irq_data *desc;
+	u32 irq_type;
+	u16 val = 0;
+	u8 polarity;
+	struct spi_device *spi = st->adis.spi;
+
+	desc = irq_get_irq_data(spi->irq);
+	if (!desc) {
+		dev_err(&spi->dev, "Could not find IRQ %d\n", spi->irq);
+		return -EINVAL;
+	}
+	/*
+	 * It is possible to configure the data ready polarity. Furthermore, we
+	 * need to update the adis struct if we want data ready as active low.
+	 */
+	irq_type = irqd_get_trigger_type(desc);
+	if (irq_type == IRQF_TRIGGER_RISING) {
+		polarity = 1;
+	} else if (irq_type == IRQF_TRIGGER_FALLING) {
+		polarity = 0;
+		st->adis.irq_mask = IRQF_TRIGGER_FALLING;
+	} else {
+		dev_err(&spi->dev, "Invalid interrupt type 0x%x specified\n",
+			irq_type);
+		return -EINVAL;
+	}
+
+	val = ADIS16475_MSG_CTRL_DR_POL(polarity);
+	ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
+				 ADIS16475_MSG_CTRL_DR_POL_MASK, val);
+	if (ret)
+		return ret;
+	/*
+	 * There is a delay writing to any bits written to the MSC_CTRL
+	 * register. It should not be bigger than 200us, so 250 should be more
+	 * than enough!
+	 */
+	usleep_range(250, 260);
+
+	return 0;
+}
+
+static const struct of_device_id adis16475_of_match[] = {
+	{ .compatible = "adi,adis16470",
+		.data = &adis16475_chip_info[ADIS16470] },
+	{ .compatible = "adi,adis16475-1",
+		.data = &adis16475_chip_info[ADIS16475_1] },
+	{ .compatible = "adi,adis16475-2",
+		.data = &adis16475_chip_info[ADIS16475_2] },
+	{ .compatible = "adi,adis16475-3",
+		.data = &adis16475_chip_info[ADIS16475_3] },
+	{ .compatible = "adi,adis16477-1",
+		.data = &adis16475_chip_info[ADIS16477_1] },
+	{ .compatible = "adi,adis16477-2",
+		.data = &adis16475_chip_info[ADIS16477_2] },
+	{ .compatible = "adi,adis16477-3",
+		.data = &adis16475_chip_info[ADIS16477_3] },
+	{ .compatible = "adi,adis16465-1",
+		.data = &adis16475_chip_info[ADIS16465_1] },
+	{ .compatible = "adi,adis16465-2",
+		.data = &adis16475_chip_info[ADIS16465_2] },
+	{ .compatible = "adi,adis16465-3",
+		.data = &adis16475_chip_info[ADIS16465_3] },
+	{ .compatible = "adi,adis16467-1",
+		.data = &adis16475_chip_info[ADIS16467_1] },
+	{ .compatible = "adi,adis16467-2",
+		.data = &adis16475_chip_info[ADIS16467_2] },
+	{ .compatible = "adi,adis16467-3",
+		.data = &adis16475_chip_info[ADIS16467_3] },
+	{ .compatible = "adi,adis16500",
+		.data = &adis16475_chip_info[ADIS16500] },
+	{ .compatible = "adi,adis16505-1",
+		.data = &adis16475_chip_info[ADIS16505_1] },
+	{ .compatible = "adi,adis16505-2",
+		.data = &adis16475_chip_info[ADIS16505_2] },
+	{ .compatible = "adi,adis16505-3",
+		.data = &adis16475_chip_info[ADIS16505_3] },
+	{ .compatible = "adi,adis16507-1",
+		.data = &adis16475_chip_info[ADIS16507_1] },
+	{ .compatible = "adi,adis16507-2",
+		.data = &adis16475_chip_info[ADIS16507_2] },
+	{ .compatible = "adi,adis16507-3",
+		.data = &adis16475_chip_info[ADIS16507_3] },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, adis16475_of_match);
+
+static int adis16475_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct adis16475 *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+	spi_set_drvdata(spi, indio_dev);
+	st->adis.burst = &adis16475_burst;
+
+	st->info = of_device_get_match_data(&spi->dev);
+	if (!st->info)
+		return -EINVAL;
+
+	ret = adis_init(&st->adis, indio_dev, spi, &st->info->adis_data);
+	if (ret)
+		return ret;
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->name = st->info->name;
+	indio_dev->channels = st->info->channels;
+	indio_dev->num_channels = st->info->num_channels;
+	indio_dev->info = &adis16475_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	ret = __adis_initial_startup(&st->adis);
+	if (ret)
+		return ret;
+
+	ret = adis16475_config_irq_pin(st);
+	if (ret)
+		return ret;
+
+	ret = adis16475_config_sync_mode(st);
+	if (ret)
+		return ret;
+
+	ret = devm_adis_setup_buffer_and_trigger(&st->adis, indio_dev,
+						 adis16475_trigger_handler);
+	if (ret)
+		return ret;
+
+	adis16475_enable_irq(&st->adis, false);
+
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret)
+		return ret;
+
+	adis16475_debugfs_init(indio_dev);
+
+	return 0;
+}
+
+static struct spi_driver adis16475_driver = {
+	.driver = {
+		.name = "adis16475",
+		.of_match_table = adis16475_of_match,
+	},
+	.probe = adis16475_probe,
+};
+module_spi_driver(adis16475_driver);
+
+MODULE_AUTHOR("Nuno Sa <nuno.sa@analog.com>");
+MODULE_DESCRIPTION("Analog Devices ADIS16475 IMU driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

[PATCH v3 6/6] dt-bindings: iio: Add adis16475 documentation

[Nuno Sá]

Document the ADIS16475 device devicetree bindings.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
Changes in v2:
 * Remove burst32 property;
 * Rename clk-mode to adi,sync-mode;
 * Remove clock-names;
 * Add conditionals to state that clocks is only needed depending on adi,sync-mode property.

Changes in v3:
 * Make use of 'allOf' in conditionals.

 .../bindings/iio/imu/adi,adis16475.yaml       | 137 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 138 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml

diff --git a/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml b/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
new file mode 100644
index 000000000000..98baecb4b98a
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
@@ -0,0 +1,137 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/imu/adi,adis16475.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices ADIS16475 and similar IMUs
+
+maintainers:
+  - Nuno Sá <nuno.sa@analog.com>
+
+description: |
+  Analog Devices ADIS16475 and similar IMUs
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ADIS16475.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,adis16475-1
+      - adi,adis16475-2
+      - adi,adis16475-3
+      - adi,adis16477-1
+      - adi,adis16477-2
+      - adi,adis16477-3
+      - adi,adis16470
+      - adi,adis16465-1
+      - adi,adis16465-2
+      - adi,adis16465-3
+      - adi,adis16467-1
+      - adi,adis16467-2
+      - adi,adis16467-3
+      - adi,adis16500
+      - adi,adis16505-1
+      - adi,adis16505-2
+      - adi,adis16505-3
+      - adi,adis16507-1
+      - adi,adis16507-2
+      - adi,adis16507-3
+
+  reg:
+    maxItems: 1
+
+  spi-cpha: true
+
+  spi-cpol: true
+
+  spi-max-frequency:
+    maximum: 2000000
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  reset-gpios:
+    description:
+      Must be the device tree identifier of the RESET pin. If specified,
+      it will be asserted during driver probe. As the line is active low,
+      it should be marked GPIO_ACTIVE_LOW.
+    maxItems: 1
+
+  adi,sync-mode:
+    description:
+      Configures the device SYNC pin. The following modes are supported
+      0 - output_sync
+      1 - direct_sync
+      2 - scaled_sync
+      3 - pulse_sync
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 3
+
+  adi,scaled-output-hz:
+    description:
+      This property must be present if the clock mode is scaled-sync through
+      clock-names property. In this mode, the input clock can have a range
+      of 1Hz to 128HZ which must be scaled to originate an allowable sample
+      rate. This property specifies that rate.
+    minimum: 1900
+    maximum: 2100
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - spi-cpha
+  - spi-cpol
+
+allOf:
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,adis16500
+              - adi,adis16505-1
+              - adi,adis16505-2
+              - adi,adis16505-3
+              - adi,adis16507-1
+              - adi,adis16507-2
+              - adi,adis16507-3
+
+    then:
+      properties:
+        adi,sync-mode:
+          minimum: 0
+          maximum: 2
+
+  - if:
+      properties:
+        adi,sync-mode:
+          enum: [1, 2, 3]
+
+    then:
+      dependencies:
+        adi,sync-mode: [ clocks ]
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    spi {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            adis16475: adis16475-3@0 {
+                    compatible = "adi,adis16475-3";
+                    reg = <0>;
+                    spi-cpha;
+                    spi-cpol;
+                    spi-max-frequency = <2000000>;
+                    interrupts = <4 IRQ_TYPE_EDGE_RISING>;
+                    interrupt-parent = <&gpio>;
+            };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index ae9cce22fa40..0fd9dbf2f793 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1016,6 +1016,7 @@ W:	http://ez.analog.com/community/linux-device-drivers
 S:	Supported
 F:	drivers/iio/imu/adis16475.c
 F:	Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475
+F:	Documentation/devicetree/bindings/iio/imu/adi,adis16475.yaml
 
 ANALOG DEVICES INC ADM1177 DRIVER
 M:	Beniamin Bia <beniamin.bia@analog.com>
-- 
2.17.1

Re: [PATCH v3 2/6] iio: imu: adis: Add irq mask variable

[Andy Shevchenko]

On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> There are some ADIS devices that can configure the data ready pin
> polarity. Hence, we cannot hardcode our IRQ mask as IRQF_TRIGGER_RISING
> since we might want to have it as IRQF_TRIGGER_FALLING.

...

> +static int adis_validate_irq_mask(struct adis *adis)
> +{
> +       if (!adis->irq_mask) {
> +               adis->irq_mask = IRQF_TRIGGER_RISING;
> +               return 0;

> +       } else if (adis->irq_mask != IRQF_TRIGGER_RISING &&

'else' is redundant.

> +                  adis->irq_mask != IRQF_TRIGGER_FALLING) {

But this condition rises questions. Why i can't configure both?
Why I can't configure other flags there?

> +               dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n",
> +                       adis->irq_mask);
> +               return -EINVAL;
> +       }

> +       return 0;
> +}

And actually name of the function is not exactly what it does. It
validates *or* initializes.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 3/6] iio: adis: Add adis_update_bits() APIs

[Andy Shevchenko]

On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> This patch adds a `regmap_update_bits()` like API to the ADIS library.
> It provides locked and unlocked variant.

> +       __val &= ~mask;
> +       __val |= val & mask;

You can use standard one liner, i.e.

       __val = (__val & ~mask) | (val & mask);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 5/6] iio: imu: Add support for adis16475

[Andy Shevchenko]

On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
>
> Support ADIS16475 and similar IMU devices. These devices are
> a precision, miniature MEMS inertial measurement unit (IMU) that
> includes a triaxial gyroscope and a triaxial accelerometer. Each
> inertial sensor combines with signal conditioning that optimizes
> dynamic performance.
>
> The driver adds support for the following devices:
>  * adis16470, adis16475, adis16477, adis16465, adis16467, adis16500,
>    adis16505, adis16507.

...

> +ANALOG DEVICES INC ADIS16475 DRIVER
> +M:     Nuno Sa <nuno.sa@analog.com>
> +L:     linux-iio@vger.kernel.org
> +W:     http://ez.analog.com/community/linux-device-drivers
> +S:     Supported
> +F:     drivers/iio/imu/adis16475.c
> +F:     Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475

Run parse-maintainers.pl to fix this.

...

> +#include <asm/unaligned.h>

Usually it goes after linux/*

> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/debugfs.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>

> +#include <linux/kernel.h>

What this is for?

> +#include <linux/iio/buffer.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/imu/adis.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/irq.h>
> +#include <linux/module.h>

> +#include <linux/of_device.h>

Do you really need this? Perhaps mod_devicetable.h is what you are looking for.

> +#include <linux/spi/spi.h>

...

> +#ifdef CONFIG_DEBUG_FS

> +DEFINE_SIMPLE_ATTRIBUTE(adis16475_serial_number_fops,
> +                       adis16475_show_serial_number, NULL, "0x%.4llx\n");

DEBUGFS ATTRIBUTE?

> +DEFINE_SIMPLE_ATTRIBUTE(adis16475_product_id_fops,
> +                       adis16475_show_product_id, NULL, "%llu\n");

> +DEFINE_SIMPLE_ATTRIBUTE(adis16475_flash_count_fops,
> +                       adis16475_show_flash_count, NULL, "%lld\n");

Ditto.

> +#else
> +static int adis16475_debugfs_init(struct iio_dev *indio_dev)
> +{
> +       return 0;
> +}
> +#endif

...

> +       /*
> +        * If decimation is used, then gyro and accel data will have meaningful
> +        * bits on the LSB registers. This info is used on the trigger handler.
> +        */
> +       if (!dec)
> +               clear_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag);
> +       else
> +               set_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag);

assign_bit()

Also to the rest of same

...

> +       for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) {

Why those margins? size-2 and 1 ?

> +               if (adis16475_3db_freqs[i] >= filter)
> +                       break;
> +       }

...

> +#define ADIS16475_GYRO_CHANNEL(_mod) \
> +       ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \
> +       ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_ ## _mod, 32, \
> +       32)

It's not obvious that this is macro inside macro. Can you indent better?
Ditto for the rest similar ones.

...

> +static int adis16475_enable_irq(struct adis *adis, bool enable)
> +{
> +       /*
> +        * There is no way to gate the data-ready signal internally inside the
> +        * ADIS16475. We can only control it's polarity...
> +        */
> +       if (enable)
> +               enable_irq(adis->spi->irq);
> +       else
> +               disable_irq(adis->spi->irq);
> +
> +       return 0;
> +}

It's seems this function is bigger than in-place calls for enable or
disable IRQ.

...

> +       return (crc == 0);

Too many parentheses.

...

> +               ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> +                                        ADIS16500_BURST32_MASK, en);
> +               if (ret < 0)

ret > 0 has any meaning? Maybe drop all these ' < 0' parts from the
code (where it's appropriate)?

> +                       return;

...

> +       buffer = (u16 *)adis->buffer;

Why the casting is needed?

> +       crc = get_unaligned_be16(&buffer[offset + 2]);

If your buffer is aligned in the structure, you may simple use be16_to_cpu().
Same for the rest of get_unaligned*() calls.
Or do you have unaligned data there?

> +       valid = adis16475_validate_crc((u8 *)adis->buffer, crc, st->burst32);

Why casting?

> +       if (!valid) {
> +               dev_err(&adis->spi->dev, "Invalid crc\n");
> +               goto check_burst32;
> +       }

...

> +                                       /* keep sparse happy */

Perhaps buffer should be declared as __be16.

> +                                       data[i++] = (__force u16)__val;

...


> +       desc = irq_get_irq_data(spi->irq);

> +       if (!desc) {
> +               dev_err(&spi->dev, "Could not find IRQ %d\n", spi->irq);
> +               return -EINVAL;
> +       }

Is this even possible?

...

> +       { .compatible = "adi,adis16507-3",
> +               .data = &adis16475_chip_info[ADIS16507_3] },
> +       { },

Comma is not needed.

...

> +       st->info = of_device_get_match_data(&spi->dev);

device_get_match_data()

> +       if (!st->info)
> +               return -EINVAL;

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 5/6] iio: imu: Add support for adis16475

[Sa, Nuno]

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Dienstag, 31. März 2020 20:16
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree
> <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>;
> Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ardelean,
> Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH v3 5/6] iio: imu: Add support for adis16475
> 
> [External]
> 
> On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > Support ADIS16475 and similar IMU devices. These devices are
> > a precision, miniature MEMS inertial measurement unit (IMU) that
> > includes a triaxial gyroscope and a triaxial accelerometer. Each
> > inertial sensor combines with signal conditioning that optimizes
> > dynamic performance.
> >
> > The driver adds support for the following devices:
> >  * adis16470, adis16475, adis16477, adis16465, adis16467, adis16500,
> >    adis16505, adis16507.
> 
> ...
> 
> > +ANALOG DEVICES INC ADIS16475 DRIVER
> > +M:     Nuno Sa <nuno.sa@analog.com>
> > +L:     linux-iio@vger.kernel.org
> > +W:     http://ez.analog.com/community/linux-device-drivers
> > +S:     Supported
> > +F:     drivers/iio/imu/adis16475.c
> > +F:     Documentation/ABI/testing/sysfs-bus-iio-imu-adis16475
> 
> Run parse-maintainers.pl to fix this.
> 

Ups. Leftover from v1. Thanks!

> ...
> 
> > +#include <asm/unaligned.h>
> 

I thought we wanted alphabetic order...

> Usually it goes after linux/*
> 
> > +#include <linux/bitfield.h>
> > +#include <linux/bitops.h>
> > +#include <linux/clk.h>
> > +#include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> 
> > +#include <linux/kernel.h>
> 
> What this is for?
> 
Yeps. Not really needed...

> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/imu/adis.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/irq.h>
> > +#include <linux/module.h>
> 
> > +#include <linux/of_device.h>
> 
> Do you really need this? Perhaps mod_devicetable.h is what you are looking
> for.
> 

Yes. For ` of_device_get_match_data ``. If changed by `device_get_match_data`, then I guess
I can drop it..
> > +#include <linux/spi/spi.h>
> 
> ...
> 
> > +#ifdef CONFIG_DEBUG_FS
> 
> > +DEFINE_SIMPLE_ATTRIBUTE(adis16475_serial_number_fops,
> > +                       adis16475_show_serial_number, NULL, "0x%.4llx\n");
> 
> DEBUGFS ATTRIBUTE?
> 
> > +DEFINE_SIMPLE_ATTRIBUTE(adis16475_product_id_fops,
> > +                       adis16475_show_product_id, NULL, "%llu\n");
> 
> > +DEFINE_SIMPLE_ATTRIBUTE(adis16475_flash_count_fops,
> > +                       adis16475_show_flash_count, NULL, "%lld\n");
> 
> Ditto.
> 
> > +#else
> > +static int adis16475_debugfs_init(struct iio_dev *indio_dev)
> > +{
> > +       return 0;
> > +}
> > +#endif
> 
> ...
> 
> > +       /*
> > +        * If decimation is used, then gyro and accel data will have meaningful
> > +        * bits on the LSB registers. This info is used on the trigger handler.
> > +        */
> > +       if (!dec)
> > +               clear_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag);
> > +       else
> > +               set_bit(ADIS16475_LSB_DEC_MASK, &st->lsb_flag);
> 
> assign_bit()
> 

Will do that...

> Also to the rest of same
> 
> ...
> 
> > +       for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) {
> 
> Why those margins? size-2 and 1 ?
> 

The -2 is needed since index 7 is not valid. The 1 I honestly don't remember why I did it
like this. Using > 0 is the same and more "common"...

> > +               if (adis16475_3db_freqs[i] >= filter)
> > +                       break;
> > +       }
> 
> ...
> 
> > +#define ADIS16475_GYRO_CHANNEL(_mod) \
> > +       ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \
> > +       ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_ ##
> _mod, 32, \
> > +       32)
> 
> It's not obvious that this is macro inside macro. Can you indent better?
> Ditto for the rest similar ones.
> 

Honestly here I don't see any problems with indentation and it goes in conformity with
other IMU drivers already in tree. So here, as long as anyone else has a problem with this, I prefer
to keep it this way...

> ...
> 
> > +static int adis16475_enable_irq(struct adis *adis, bool enable)
> > +{
> > +       /*
> > +        * There is no way to gate the data-ready signal internally inside the
> > +        * ADIS16475. We can only control it's polarity...
> > +        */
> > +       if (enable)
> > +               enable_irq(adis->spi->irq);
> > +       else
> > +               disable_irq(adis->spi->irq);
> > +
> > +       return 0;
> > +}
> 
> It's seems this function is bigger than in-place calls for enable or
> disable IRQ.
> 

This api is callbacked from the ADIS library...

> ...
> 
> > +       return (crc == 0);
> 
> Too many parentheses.
> 
> ...
> 
> > +               ret = __adis_update_bits(&st->adis, ADIS16475_REG_MSG_CTRL,
> > +                                        ADIS16500_BURST32_MASK, en);
> > +               if (ret < 0)
> 
> ret > 0 has any meaning? Maybe drop all these ' < 0' parts from the
> code (where it's appropriate)?
> 
> > +                       return;
> 
> ...
> 
> > +       buffer = (u16 *)adis->buffer;
> 
> Why the casting is needed?
> 
> > +       crc = get_unaligned_be16(&buffer[offset + 2]);
> 
> If your buffer is aligned in the structure, you may simple use be16_to_cpu().
> Same for the rest of get_unaligned*() calls.
> Or do you have unaligned data there?

This is a nice point. So, honestly I made it like this to keep conformity with other drivers we have
in our internal tree (in queue for upstream) and I also wondered about this. The only justification I can
find to use unligned calls is to keep this independent from the ADIS lib (not sure if it makes sense) since
we get the pointer from the library (allocated there).

Now, if Im not missing nothing obvious we can access the buffer normally since it's being allocated
with kmalloc which means we have  ARCH_KMALLOC_MINALIGN (which is at least 8 if Im not mistaken).
On top of this, the device sends the data as n 16 bits segments. So in theory, I guess we can ditch the
overhead of the *unaligned calls if any objections? 

> > +       valid = adis16475_validate_crc((u8 *)adis->buffer, crc, st->burst32);
> 
> Why casting?
> 

Not really needed...

> > +       if (!valid) {
> > +               dev_err(&adis->spi->dev, "Invalid crc\n");
> > +               goto check_burst32;
> > +       }
> 
> ...
> 
> > +                                       /* keep sparse happy */
> 
> Perhaps buffer should be declared as __be16.
> 
Will do it if removing the *unaligned calls...
> > +                                       data[i++] = (__force u16)__val;
> 
> ...
> 
> 
> > +       desc = irq_get_irq_data(spi->irq);
> 
> > +       if (!desc) {
> > +               dev_err(&spi->dev, "Could not find IRQ %d\n", spi->irq);
> > +               return -EINVAL;
> > +       }
> 
> Is this even possible?
> 

I guess. If someone does not include it in device tree...

> ...
> 
> > +       { .compatible = "adi,adis16507-3",
> > +               .data = &adis16475_chip_info[ADIS16507_3] },
> > +       { },
> 
> Comma is not needed.
> 
Will remove it
> ...
> 
> > +       st->info = of_device_get_match_data(&spi->dev);
> 
> device_get_match_data()
Will use it...
> 
> > +       if (!st->info)
> > +               return -EINVAL;
> 

Thanks for the review
- Nuno Sá
> --
> With Best Regards,
> Andy Shevchenko

RE: [PATCH v3 3/6] iio: adis: Add adis_update_bits() APIs

[Sa, Nuno]

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Dienstag, 31. März 2020 19:42
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree
> <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>;
> Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ardelean,
> Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH v3 3/6] iio: adis: Add adis_update_bits() APIs
> 
> [External]
> 
> On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > This patch adds a `regmap_update_bits()` like API to the ADIS library.
> > It provides locked and unlocked variant.
> 
> > +       __val &= ~mask;
> > +       __val |= val & mask;
> 
> You can use standard one liner, i.e.
> 
>        __val = (__val & ~mask) | (val & mask);

Sure. Can do that...

- Nuno Sá
> --
> With Best Regards,
> Andy Shevchenko

RE: [PATCH v3 2/6] iio: imu: adis: Add irq mask variable

[Sa, Nuno]

> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On
> Behalf Of Andy Shevchenko
> Sent: Dienstag, 31. März 2020 19:40
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree
> <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>;
> Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ardelean,
> Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH v3 2/6] iio: imu: adis: Add irq mask variable
> 
> [External]
> 
> On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
> >
> > There are some ADIS devices that can configure the data ready pin
> > polarity. Hence, we cannot hardcode our IRQ mask as
> IRQF_TRIGGER_RISING
> > since we might want to have it as IRQF_TRIGGER_FALLING.
> 
> ...
> 
> > +static int adis_validate_irq_mask(struct adis *adis)
> > +{
> > +       if (!adis->irq_mask) {
> > +               adis->irq_mask = IRQF_TRIGGER_RISING;
> > +               return 0;
> 
> > +       } else if (adis->irq_mask != IRQF_TRIGGER_RISING &&
> 
> 'else' is redundant.
> 
> > +                  adis->irq_mask != IRQF_TRIGGER_FALLING) {
> 
> But this condition rises questions. Why i can't configure both?
> Why I can't configure other flags there?

Both you can't because it is just how these type of devices work. Data is ready either
on the rising edge or on the falling edge (if supported by the device)...
I agree this could check if only one of the flags are set instead of directly comparing the
values (invalidating other flags) but I would prefer to keep this simple for now...

> 
> > +               dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n",
> > +                       adis->irq_mask);
> > +               return -EINVAL;
> > +       }
> 
> > +       return 0;
> > +}
> 
> And actually name of the function is not exactly what it does. It
> validates *or* initializes.

Well, yes. It just sets the mask to the default value to keep backward compatibility
with all the other devices that don't support/use this variable...

- Nuno Sá
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v3 5/6] iio: imu: Add support for adis16475

[Andy Shevchenko]

On Wed, Apr 1, 2020 at 10:13 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:
>
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Dienstag, 31. März 2020 20:16
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree
> > <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> > Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>;
> > Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring
> > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ardelean,
> > Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>
> > Subject: Re: [PATCH v3 5/6] iio: imu: Add support for adis16475
> > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > > +#include <asm/unaligned.h>

> I thought we wanted alphabetic order...

Yes, but from more generic header groups to less generic. Inside each
group is alphabetical.
asm/ is less generic than linux/.

> > Usually it goes after linux/*

> > > +#include <linux/bitfield.h>
> > > +#include <linux/bitops.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/debugfs.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> >
> > > +#include <linux/kernel.h>
> >
> > What this is for?
> >
> Yeps. Not really needed...

I think you needed it for DIV_ROUND_UP or alike macros. It also has
container_of...

> > > +#include <linux/iio/buffer.h>
> > > +#include <linux/iio/iio.h>
> > > +#include <linux/iio/imu/adis.h>
> > > +#include <linux/iio/sysfs.h>
> > > +#include <linux/iio/trigger_consumer.h>
> > > +#include <linux/irq.h>
> > > +#include <linux/module.h>
> >
> > > +#include <linux/of_device.h>
> >
> > Do you really need this? Perhaps mod_devicetable.h is what you are looking
> > for.
> >
>
> Yes. For ` of_device_get_match_data ``. If changed by `device_get_match_data`, then I guess
> I can drop it..

Probably change to mod_devicetable.h with property.h.

> > > +#include <linux/spi/spi.h>

...

> > > +       for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) {
> >
> > Why those margins? size-2 and 1 ?
> >
>
> The -2 is needed since index 7 is not valid. The 1 I honestly don't remember why I did it
> like this. Using > 0 is the same and more "common"...

More common is >= 0. That's my question basically.
And if 7 is not valid why to keep it in the array at all?

> > > +               if (adis16475_3db_freqs[i] >= filter)
> > > +                       break;
> > > +       }

...

> > > +#define ADIS16475_GYRO_CHANNEL(_mod) \
> > > +       ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \
> > > +       ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_ ##
> > _mod, 32, \
> > > +       32)
> >
> > It's not obvious that this is macro inside macro. Can you indent better?
> > Ditto for the rest similar ones.
> >
>
> Honestly here I don't see any problems with indentation and it goes in conformity with
> other IMU drivers already in tree. So here, as long as anyone else has a problem with this, I prefer
> to keep it this way...

I'm not a maintainer, not my call :-)

...

> > > +       buffer = (u16 *)adis->buffer;
> >
> > Why the casting is needed?
> >
> > > +       crc = get_unaligned_be16(&buffer[offset + 2]);
> >
> > If your buffer is aligned in the structure, you may simple use be16_to_cpu().
> > Same for the rest of get_unaligned*() calls.
> > Or do you have unaligned data there?
>
> This is a nice point. So, honestly I made it like this to keep conformity with other drivers we have
> in our internal tree (in queue for upstream) and I also wondered about this. The only justification I can
> find to use unligned calls is to keep this independent from the ADIS lib (not sure if it makes sense) since
> we get the pointer from the library (allocated there).
>
> Now, if Im not missing nothing obvious we can access the buffer normally since it's being allocated
> with kmalloc which means we have  ARCH_KMALLOC_MINALIGN (which is at least 8 if Im not mistaken).
> On top of this, the device sends the data as n 16 bits segments. So in theory, I guess we can ditch the
> overhead of the *unaligned calls if any objections?

No objections from my side at least.

...

> > > +       desc = irq_get_irq_data(spi->irq);
> >
> > > +       if (!desc) {
> > > +               dev_err(&spi->dev, "Could not find IRQ %d\n", spi->irq);
> > > +               return -EINVAL;
> > > +       }
> >
> > Is this even possible?

> I guess. If someone does not include it in device tree...

Hmm... and this function will be called anyway?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/6] iio: imu: adis: Add irq mask variable

[Andy Shevchenko]

On Wed, Apr 1, 2020 at 10:22 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On
> > Behalf Of Andy Shevchenko
> > Sent: Dienstag, 31. März 2020 19:40
> > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
> > >
> > > There are some ADIS devices that can configure the data ready pin
> > > polarity. Hence, we cannot hardcode our IRQ mask as
> > IRQF_TRIGGER_RISING
> > > since we might want to have it as IRQF_TRIGGER_FALLING.

...

> > > +static int adis_validate_irq_mask(struct adis *adis)
> > > +{
> > > +       if (!adis->irq_mask) {
> > > +               adis->irq_mask = IRQF_TRIGGER_RISING;
> > > +               return 0;
> >
> > > +       } else if (adis->irq_mask != IRQF_TRIGGER_RISING &&
> >
> > 'else' is redundant.
> >
> > > +                  adis->irq_mask != IRQF_TRIGGER_FALLING) {
> >
> > But this condition rises questions. Why i can't configure both?
> > Why I can't configure other flags there?
>
> Both you can't because it is just how these type of devices work. Data is ready either
> on the rising edge or on the falling edge (if supported by the device)...
> I agree this could check if only one of the flags are set instead of directly comparing the
> values (invalidating other flags) but I would prefer to keep this simple for now...
>
> >
> > > +               dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n",
> > > +                       adis->irq_mask);
> > > +               return -EINVAL;
> > > +       }
> >
> > > +       return 0;
> > > +}
> >
> > And actually name of the function is not exactly what it does. It
> > validates *or* initializes.
>
> Well, yes. It just sets the mask to the default value to keep backward compatibility
> with all the other devices that don't support/use this variable...

Perhaps documentation in a comment form should be added.
Moreover, I realized that you added to variable and function mask
suffix while it's actually flag.

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 2/6] iio: imu: adis: Add irq mask variable

[Sa, Nuno]

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Mittwoch, 1. April 2020 12:27
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree
> <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>;
> Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ardelean,
> Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH v3 2/6] iio: imu: adis: Add irq mask variable
> 
> On Wed, Apr 1, 2020 at 10:22 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org>
> On
> > > Behalf Of Andy Shevchenko
> > > Sent: Dienstag, 31. März 2020 19:40
> > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
> > > >
> > > > There are some ADIS devices that can configure the data ready pin
> > > > polarity. Hence, we cannot hardcode our IRQ mask as
> > > IRQF_TRIGGER_RISING
> > > > since we might want to have it as IRQF_TRIGGER_FALLING.
> 
> ...
> 
> > > > +static int adis_validate_irq_mask(struct adis *adis)
> > > > +{
> > > > +       if (!adis->irq_mask) {
> > > > +               adis->irq_mask = IRQF_TRIGGER_RISING;
> > > > +               return 0;
> > >
> > > > +       } else if (adis->irq_mask != IRQF_TRIGGER_RISING &&
> > >
> > > 'else' is redundant.
> > >
> > > > +                  adis->irq_mask != IRQF_TRIGGER_FALLING) {
> > >
> > > But this condition rises questions. Why i can't configure both?
> > > Why I can't configure other flags there?
> >
> > Both you can't because it is just how these type of devices work. Data is
> ready either
> > on the rising edge or on the falling edge (if supported by the device)...
> > I agree this could check if only one of the flags are set instead of directly
> comparing the
> > values (invalidating other flags) but I would prefer to keep this simple for
> now...
> >
> > >
> > > > +               dev_err(&adis->spi->dev, "Invalid IRQ mask: %08lx\n",
> > > > +                       adis->irq_mask);
> > > > +               return -EINVAL;
> > > > +       }
> > >
> > > > +       return 0;
> > > > +}
> > >
> > > And actually name of the function is not exactly what it does. It
> > > validates *or* initializes.
> >
> > Well, yes. It just sets the mask to the default value to keep backward
> compatibility
> > with all the other devices that don't support/use this variable...
> 
> Perhaps documentation in a comment form should be added.
> Moreover, I realized that you added to variable and function mask
> suffix while it's actually flag.

Will change the suffix to flag...

- Nuno Sá
> --
> With Best Regards,
> Andy Shevchenko

RE: [PATCH v3 5/6] iio: imu: Add support for adis16475

[Sa, Nuno]

> From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On
> Behalf Of Andy Shevchenko
> Sent: Mittwoch, 1. April 2020 12:23
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree
> <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>;
> Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ardelean,
> Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH v3 5/6] iio: imu: Add support for adis16475
> 
> On Wed, Apr 1, 2020 at 10:13 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> >
> > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > Sent: Dienstag, 31. März 2020 20:16
> > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree
> > > <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> > > Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen
> <lars@metafoo.de>;
> > > Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring
> > > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>;
> Ardelean,
> > > Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael
> > > <Michael.Hennerich@analog.com>
> > > Subject: Re: [PATCH v3 5/6] iio: imu: Add support for adis16475
> > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote:
> 
> ...
> 
> > > > +#include <asm/unaligned.h>
> 
> > I thought we wanted alphabetic order...
> 
> Yes, but from more generic header groups to less generic. Inside each
> group is alphabetical.
> asm/ is less generic than linux/.
>

Got it...

> > > Usually it goes after linux/*
> 
> > > > +#include <linux/bitfield.h>
> > > > +#include <linux/bitops.h>
> > > > +#include <linux/clk.h>
> > > > +#include <linux/debugfs.h>
> > > > +#include <linux/delay.h>
> > > > +#include <linux/device.h>
> > >
> > > > +#include <linux/kernel.h>
> > >
> > > What this is for?
> > >
> > Yeps. Not really needed...
> 
> I think you needed it for DIV_ROUND_UP or alike macros. It also has
> container_of...
> 

Yes, DIV_ROUND_CLOSEST is defined there...

> > > > +#include <linux/iio/buffer.h>
> > > > +#include <linux/iio/iio.h>
> > > > +#include <linux/iio/imu/adis.h>
> > > > +#include <linux/iio/sysfs.h>
> > > > +#include <linux/iio/trigger_consumer.h>
> > > > +#include <linux/irq.h>
> > > > +#include <linux/module.h>
> > >
> > > > +#include <linux/of_device.h>
> > >
> > > Do you really need this? Perhaps mod_devicetable.h is what you are
> looking
> > > for.
> > >
> >
> > Yes. For ` of_device_get_match_data ``. If changed by
> `device_get_match_data`, then I guess
> > I can drop it..
> 
> Probably change to mod_devicetable.h with property.h.
> 
> > > > +#include <linux/spi/spi.h>
> 
> ...
> 
> > > > +       for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) {
> > >
> > > Why those margins? size-2 and 1 ?
> > >
> >
> > The -2 is needed since index 7 is not valid. The 1 I honestly don't remember
> why I did it
> > like this. Using > 0 is the same and more "common"...
> 
> More common is >= 0. That's my question basically.
> And if 7 is not valid why to keep it in the array at all?

Well, I can remove the 7. I honestly took it from another driver and I guess the idea
is to make explicit that 7 is not supported. Since this is a 3 bit field and the datasheet
does not state this directly.

As for the >=0, I prefer to have either as is or >0 since we don't really need to check the
index 0. If 1 fails, then we will use 0 either way...

> > > > +               if (adis16475_3db_freqs[i] >= filter)
> > > > +                       break;
> > > > +       }
> 
> ...
> 
> > > > +#define ADIS16475_GYRO_CHANNEL(_mod) \
> > > > +       ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \
> > > > +       ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_
> ##
> > > _mod, 32, \
> > > > +       32)
> > >
> > > It's not obvious that this is macro inside macro. Can you indent better?
> > > Ditto for the rest similar ones.
> > >
> >
> > Honestly here I don't see any problems with indentation and it goes in
> conformity with
> > other IMU drivers already in tree. So here, as long as anyone else has a
> problem with this, I prefer
> > to keep it this way...
> 
> I'm not a maintainer, not my call :-)
> 
> ...
> 
> > > > +       buffer = (u16 *)adis->buffer;
> > >
> > > Why the casting is needed?
> > >
> > > > +       crc = get_unaligned_be16(&buffer[offset + 2]);
> > >
> > > If your buffer is aligned in the structure, you may simple use
> be16_to_cpu().
> > > Same for the rest of get_unaligned*() calls.
> > > Or do you have unaligned data there?
> >
> > This is a nice point. So, honestly I made it like this to keep conformity with
> other drivers we have
> > in our internal tree (in queue for upstream) and I also wondered about this.
> The only justification I can
> > find to use unligned calls is to keep this independent from the ADIS lib (not
> sure if it makes sense) since
> > we get the pointer from the library (allocated there).
> >
> > Now, if Im not missing nothing obvious we can access the buffer normally
> since it's being allocated
> > with kmalloc which means we have  ARCH_KMALLOC_MINALIGN (which is
> at least 8 if Im not mistaken).
> > On top of this, the device sends the data as n 16 bits segments. So in theory,
> I guess we can ditch the
> > overhead of the *unaligned calls if any objections?
> 
> No objections from my side at least.
> 

I will wait to see if someone else has anything to add and if not, I will change it
to normal buffer accesses (probably using restricted types).

> ...
> 
> > > > +       desc = irq_get_irq_data(spi->irq);
> > >
> > > > +       if (!desc) {
> > > > +               dev_err(&spi->dev, "Could not find IRQ %d\n", spi->irq);
> > > > +               return -EINVAL;
> > > > +       }
> > >
> > > Is this even possible?
> 
> > I guess. If someone does not include it in device tree...
> 
> Hmm... and this function will be called anyway?
> 

Yes, it is called on probe... And we should fail if no interrupt is given
in device tree. It’s a required property.

- Nuno Sá
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v3 5/6] iio: imu: Add support for adis16475

[Andy Shevchenko]

On Wed, Apr 1, 2020 at 4:27 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > On Wed, Apr 1, 2020 at 10:13 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote:

...

> > > > > +       for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) {
> > > >
> > > > Why those margins? size-2 and 1 ?
> > > >
> > >
> > > The -2 is needed since index 7 is not valid. The 1 I honestly don't remember
> > why I did it
> > > like this. Using > 0 is the same and more "common"...
> >
> > More common is >= 0. That's my question basically.
> > And if 7 is not valid why to keep it in the array at all?
>
> Well, I can remove the 7. I honestly took it from another driver and I guess the idea
> is to make explicit that 7 is not supported. Since this is a 3 bit field and the datasheet
> does not state this directly.
>
> As for the >=0, I prefer to have either as is or >0 since we don't really need to check the
> index 0. If 1 fails, then we will use 0 either way...

It makes sense to check to get better optimization (and increased readability).
Look for this

i = ARRAY_SIZE(...);

while (i--) {
 ...
}

It's much better to read and understand.

> > > > > +               if (adis16475_3db_freqs[i] >= filter)
> > > > > +                       break;
> > > > > +       }

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3 5/6] iio: imu: Add support for adis16475

[Sa, Nuno]

> From: Andy Shevchenko <andy.shevchenko@gmail.com>
> Sent: Mittwoch, 1. April 2020 16:06
> To: Sa, Nuno <Nuno.Sa@analog.com>
> Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree
> <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>;
> Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring
> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ardelean,
> Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael
> <Michael.Hennerich@analog.com>
> Subject: Re: [PATCH v3 5/6] iio: imu: Add support for adis16475
> 
> On Wed, Apr 1, 2020 at 4:27 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > On Wed, Apr 1, 2020 at 10:13 AM Sa, Nuno <Nuno.Sa@analog.com>
> wrote:
> > > > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com>
> wrote:
> 
> ...
> 
> > > > > > +       for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) {
> > > > >
> > > > > Why those margins? size-2 and 1 ?
> > > > >
> > > >
> > > > The -2 is needed since index 7 is not valid. The 1 I honestly don't
> remember
> > > why I did it
> > > > like this. Using > 0 is the same and more "common"...
> > >
> > > More common is >= 0. That's my question basically.
> > > And if 7 is not valid why to keep it in the array at all?
> >
> > Well, I can remove the 7. I honestly took it from another driver and I guess
> the idea
> > is to make explicit that 7 is not supported. Since this is a 3 bit field and the
> datasheet
> > does not state this directly.
> >
> > As for the >=0, I prefer to have either as is or >0 since we don't really need to
> check the
> > index 0. If 1 fails, then we will use 0 either way...
> 
> It makes sense to check to get better optimization (and increased readability).
> Look for this
> 
> i = ARRAY_SIZE(...);
> 
> while (i--) {
>  ...
> }
> 
> It's much better to read and understand.

Well, about the readability it's a bit subjective 😊. It depends who is
reading the code. Just curious, how would you get better optimization
by doing >=0 instead of > 0?

Either way, I don’t have any strong feeling about this so I can do as
you suggest.

- Nuno Sá
> > > > > > +               if (adis16475_3db_freqs[i] >= filter)
> > > > > > +                       break;
> > > > > > +       }
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH v3 5/6] iio: imu: Add support for adis16475

[Andy Shevchenko]

On Wed, Apr 1, 2020 at 5:18 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
>
> > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Sent: Mittwoch, 1. April 2020 16:06
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree
> > <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> > Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>;
> > Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring
> > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ardelean,
> > Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>
> > Subject: Re: [PATCH v3 5/6] iio: imu: Add support for adis16475
> >
> > On Wed, Apr 1, 2020 at 4:27 PM Sa, Nuno <Nuno.Sa@analog.com> wrote:
> > > > On Wed, Apr 1, 2020 at 10:13 AM Sa, Nuno <Nuno.Sa@analog.com>
> > wrote:
> > > > > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com>
> > wrote:

...

> > > Well, I can remove the 7. I honestly took it from another driver and I guess
> > the idea
> > > is to make explicit that 7 is not supported. Since this is a 3 bit field and the
> > datasheet
> > > does not state this directly.
> > >
> > > As for the >=0, I prefer to have either as is or >0 since we don't really need to
> > check the
> > > index 0. If 1 fails, then we will use 0 either way...
> >
> > It makes sense to check to get better optimization (and increased readability).
> > Look for this
> >
> > i = ARRAY_SIZE(...);
> >
> > while (i--) {
> >  ...
> > }
> >
> > It's much better to read and understand.
>
> Well, about the readability it's a bit subjective . It depends who is
> reading the code. Just curious, how would you get better optimization
> by doing >=0 instead of > 0?

Feel the difference while (i--) vs. while (--i > 0).
The latter one is a bit unusual.

> Either way, I don’t have any strong feeling about this so I can do as
> you suggest.



-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 5/6] iio: imu: Add support for adis16475

[Jonathan Cameron]

...
> 
> > > > +#define ADIS16475_GYRO_CHANNEL(_mod) \
> > > > +       ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \
> > > > +       ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_ ##  
> > > _mod, 32, \  
> > > > +       32)  
> > >
> > > It's not obvious that this is macro inside macro. Can you indent better?
> > > Ditto for the rest similar ones.
> > >  
> >
> > Honestly here I don't see any problems with indentation and it goes in conformity with
> > other IMU drivers already in tree. So here, as long as anyone else has a problem with this, I prefer
> > to keep it this way...  
> 
> I'm not a maintainer, not my call :-)

I'm lazy when it comes to things like this.  Yes it could be better, but
it's still fairly readable so I don't really mind.

That's not to say I don't like beautiful things if you don't mind
tidying it up? :)

Jonathan

Re: [PATCH v3 5/6] iio: imu: Add support for adis16475

[Jonathan Cameron]

On Wed, 1 Apr 2020 13:27:31 +0000
"Sa, Nuno" <Nuno.Sa@analog.com> wrote:

> > From: linux-iio-owner@vger.kernel.org <linux-iio-owner@vger.kernel.org> On
> > Behalf Of Andy Shevchenko
> > Sent: Mittwoch, 1. April 2020 12:23
> > To: Sa, Nuno <Nuno.Sa@analog.com>
> > Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree
> > <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> > Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen <lars@metafoo.de>;
> > Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring
> > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; Ardelean,
> > Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael
> > <Michael.Hennerich@analog.com>
> > Subject: Re: [PATCH v3 5/6] iio: imu: Add support for adis16475
> > 
> > On Wed, Apr 1, 2020 at 10:13 AM Sa, Nuno <Nuno.Sa@analog.com> wrote:  
> > >  
> > > > From: Andy Shevchenko <andy.shevchenko@gmail.com>
> > > > Sent: Dienstag, 31. März 2020 20:16
> > > > To: Sa, Nuno <Nuno.Sa@analog.com>
> > > > Cc: linux-iio <linux-iio@vger.kernel.org>; devicetree
> > > > <devicetree@vger.kernel.org>; Jonathan Cameron <jic23@kernel.org>;
> > > > Hartmut Knaack <knaack.h@gmx.de>; Lars-Peter Clausen  
> > <lars@metafoo.de>;  
> > > > Peter Meerwald-Stadler <pmeerw@pmeerw.net>; Rob Herring
> > > > <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>;  
> > Ardelean,  
> > > > Alexandru <alexandru.Ardelean@analog.com>; Hennerich, Michael
> > > > <Michael.Hennerich@analog.com>
> > > > Subject: Re: [PATCH v3 5/6] iio: imu: Add support for adis16475
> > > > On Tue, Mar 31, 2020 at 2:49 PM Nuno Sá <nuno.sa@analog.com> wrote:  
> > 
> > ...
> >   
> > > > > +#include <asm/unaligned.h>  
> >   
> > > I thought we wanted alphabetic order...  
> > 
> > Yes, but from more generic header groups to less generic. Inside each
> > group is alphabetical.
> > asm/ is less generic than linux/.
> >  
> 
> Got it...
> 
> > > > Usually it goes after linux/*  
> >   
> > > > > +#include <linux/bitfield.h>
> > > > > +#include <linux/bitops.h>
> > > > > +#include <linux/clk.h>
> > > > > +#include <linux/debugfs.h>
> > > > > +#include <linux/delay.h>
> > > > > +#include <linux/device.h>  
> > > >  
> > > > > +#include <linux/kernel.h>  
> > > >
> > > > What this is for?
> > > >  
> > > Yeps. Not really needed...  
> > 
> > I think you needed it for DIV_ROUND_UP or alike macros. It also has
> > container_of...
> >   
> 
> Yes, DIV_ROUND_CLOSEST is defined there...
> 
> > > > > +#include <linux/iio/buffer.h>
> > > > > +#include <linux/iio/iio.h>
> > > > > +#include <linux/iio/imu/adis.h>
> > > > > +#include <linux/iio/sysfs.h>
> > > > > +#include <linux/iio/trigger_consumer.h>
> > > > > +#include <linux/irq.h>
> > > > > +#include <linux/module.h>  
> > > >  
> > > > > +#include <linux/of_device.h>  
> > > >
> > > > Do you really need this? Perhaps mod_devicetable.h is what you are  
> > looking  
> > > > for.
> > > >  
> > >
> > > Yes. For ` of_device_get_match_data ``. If changed by  
> > `device_get_match_data`, then I guess  
> > > I can drop it..  
> > 
> > Probably change to mod_devicetable.h with property.h.
> >   
> > > > > +#include <linux/spi/spi.h>  
> > 
> > ...
> >   
> > > > > +       for (i = ARRAY_SIZE(adis16475_3db_freqs) - 2; i >= 1; i--) {  
> > > >
> > > > Why those margins? size-2 and 1 ?
> > > >  
> > >
> > > The -2 is needed since index 7 is not valid. The 1 I honestly don't remember  
> > why I did it  
> > > like this. Using > 0 is the same and more "common"...  
> > 
> > More common is >= 0. That's my question basically.
> > And if 7 is not valid why to keep it in the array at all?  
> 
> Well, I can remove the 7. I honestly took it from another driver and I guess the idea
> is to make explicit that 7 is not supported. Since this is a 3 bit field and the datasheet
> does not state this directly.
> 
> As for the >=0, I prefer to have either as is or >0 since we don't really need to check the
> index 0. If 1 fails, then we will use 0 either way...
> 
> > > > > +               if (adis16475_3db_freqs[i] >= filter)
> > > > > +                       break;
> > > > > +       }  
> > 
> > ...
> >   
> > > > > +#define ADIS16475_GYRO_CHANNEL(_mod) \
> > > > > +       ADIS16475_MOD_CHAN(IIO_ANGL_VEL, IIO_MOD_ ## _mod, \
> > > > > +       ADIS16475_REG_ ## _mod ## _GYRO_L, ADIS16475_SCAN_GYRO_  
> > ##  
> > > > _mod, 32, \  
> > > > > +       32)  
> > > >
> > > > It's not obvious that this is macro inside macro. Can you indent better?
> > > > Ditto for the rest similar ones.
> > > >  
> > >
> > > Honestly here I don't see any problems with indentation and it goes in  
> > conformity with  
> > > other IMU drivers already in tree. So here, as long as anyone else has a  
> > problem with this, I prefer  
> > > to keep it this way...  
> > 
> > I'm not a maintainer, not my call :-)
> > 
> > ...
> >   
> > > > > +       buffer = (u16 *)adis->buffer;  
> > > >
> > > > Why the casting is needed?
> > > >  
> > > > > +       crc = get_unaligned_be16(&buffer[offset + 2]);  
> > > >
> > > > If your buffer is aligned in the structure, you may simple use  
> > be16_to_cpu().  
> > > > Same for the rest of get_unaligned*() calls.
> > > > Or do you have unaligned data there?  
> > >
> > > This is a nice point. So, honestly I made it like this to keep conformity with  
> > other drivers we have  
> > > in our internal tree (in queue for upstream) and I also wondered about this.  
> > The only justification I can  
> > > find to use unligned calls is to keep this independent from the ADIS lib (not  
> > sure if it makes sense) since  
> > > we get the pointer from the library (allocated there).

It would be very odd to get a buffer from a library dealing with this sort of
device that wanted at least 8 byte aligned.  I guess we could add a paranoid
check in the driver, but I think we can safely assume this is fine without one.

> > >
> > > Now, if Im not missing nothing obvious we can access the buffer normally  
> > since it's being allocated  
> > > with kmalloc which means we have  ARCH_KMALLOC_MINALIGN (which is  
> > at least 8 if Im not mistaken).  
> > > On top of this, the device sends the data as n 16 bits segments. So in theory,  
> > I guess we can ditch the  
> > > overhead of the *unaligned calls if any objections?  
> > 
> > No objections from my side at least.
> >   
> 
> I will wait to see if someone else has anything to add and if not, I will change it
> to normal buffer accesses (probably using restricted types).
> 

If it's aligned, definitely prefer that to be explicit in the driver and use
the relevant endian types.

We have had a few cases where things are oddly padded so this may be cut and
paste from one of those.

Jonathan

Re: [PATCH v3 1/6] iio: imu: adis: Add Managed device functions

[Jonathan Cameron]

On Tue, 31 Mar 2020 13:48:06 +0200
Nuno Sá <nuno.sa@analog.com> wrote:

> This patch adds support for a managed device version of
> adis_setup_buffer_and_trigger. It works exactly as the original
> one but it calls all the devm_iio_* functions to setup an iio
> buffer and trigger. Hence we do not need to care about cleaning those
> and we do not need to support a remove() callback for every driver using
> the adis library.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Random thought inline.  Something we could use more in IIO :)

> ---
> Changes in v2:
>  * Added blank lines for readability.
> 
> Changes in V3:
>  * Removed unnecessary inline;
>  * Free buffer resources.
> 
>  drivers/iio/imu/adis_buffer.c  | 45 ++++++++++++++++++++++++++++++++++
>  drivers/iio/imu/adis_trigger.c | 41 ++++++++++++++++++++++++++++---
>  include/linux/iio/imu/adis.h   | 17 +++++++++++++
>  3 files changed, 100 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis_buffer.c b/drivers/iio/imu/adis_buffer.c
> index 04e5e2a0fd6b..c2211ab80d8c 100644
> --- a/drivers/iio/imu/adis_buffer.c
> +++ b/drivers/iio/imu/adis_buffer.c
> @@ -156,6 +156,14 @@ static irqreturn_t adis_trigger_handler(int irq, void *p)
>  	return IRQ_HANDLED;
>  }
>  
> +static void adis_buffer_cleanup(void *arg)
> +{
> +	struct adis *adis = arg;
> +
> +	kfree(adis->buffer);
> +	kfree(adis->xfer);
> +}
> +
>  /**
>   * adis_setup_buffer_and_trigger() - Sets up buffer and trigger for the adis device
>   * @adis: The adis device.
> @@ -198,6 +206,43 @@ int adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
>  }
>  EXPORT_SYMBOL_GPL(adis_setup_buffer_and_trigger);
>  
> +/**
> + * devm_adis_setup_buffer_and_trigger() - Sets up buffer and trigger for
> + *					  the managed adis device
> + * @adis: The adis device
> + * @indio_dev: The IIO device
> + * @trigger_handler: Optional trigger handler, may be NULL.
> + *
> + * Returns 0 on success, a negative error code otherwise.
> + *
> + * This function perfoms exactly the same as adis_setup_buffer_and_trigger()
> + */
> +int
> +devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
> +				   irqreturn_t (*trigger_handler)(int, void *))

It occurred to me that there must be a lot of irq handling function pointers
in the kernel and it would be odd if there wasn't a type for this...

There is :) irq_handler_t 

https://elixir.bootlin.com/linux/latest/source/include/linux/interrupt.h#L92

Not sure why I never noticed that before.  Hohum.

Jonathan


> +{
> +	int ret;
> +
> +	if (!trigger_handler)
> +		trigger_handler = adis_trigger_handler;
> +
> +	ret = devm_iio_triggered_buffer_setup(&adis->spi->dev, indio_dev,
> +					      &iio_pollfunc_store_time,
> +					      trigger_handler, NULL);
> +	if (ret)
> +		return ret;
> +
> +	if (adis->spi->irq) {
> +		ret = devm_adis_probe_trigger(adis, indio_dev);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return devm_add_action_or_reset(&adis->spi->dev, adis_buffer_cleanup,
> +					adis);
> +}
> +EXPORT_SYMBOL_GPL(devm_adis_setup_buffer_and_trigger);
> +
>  /**
>   * adis_cleanup_buffer_and_trigger() - Free buffer and trigger resources
>   * @adis: The adis device.
> diff --git a/drivers/iio/imu/adis_trigger.c b/drivers/iio/imu/adis_trigger.c
> index 8b9cd02c0f9f..a36810e0b1ab 100644
> --- a/drivers/iio/imu/adis_trigger.c
> +++ b/drivers/iio/imu/adis_trigger.c
> @@ -27,6 +27,13 @@ static const struct iio_trigger_ops adis_trigger_ops = {
>  	.set_trigger_state = &adis_data_rdy_trigger_set_state,
>  };
>  
> +static void adis_trigger_setup(struct adis *adis)
> +{
> +	adis->trig->dev.parent = &adis->spi->dev;
> +	adis->trig->ops = &adis_trigger_ops;
> +	iio_trigger_set_drvdata(adis->trig, adis);
> +}
> +
>  /**
>   * adis_probe_trigger() - Sets up trigger for a adis device
>   * @adis: The adis device
> @@ -45,9 +52,7 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
>  	if (adis->trig == NULL)
>  		return -ENOMEM;
>  
> -	adis->trig->dev.parent = &adis->spi->dev;
> -	adis->trig->ops = &adis_trigger_ops;
> -	iio_trigger_set_drvdata(adis->trig, adis);
> +	adis_trigger_setup(adis);
>  
>  	ret = request_irq(adis->spi->irq,
>  			  &iio_trigger_generic_data_rdy_poll,
> @@ -73,6 +78,36 @@ int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
>  }
>  EXPORT_SYMBOL_GPL(adis_probe_trigger);
>  
> +/**
> + * devm_adis_probe_trigger() - Sets up trigger for a managed adis device
> + * @adis: The adis device
> + * @indio_dev: The IIO device
> + *
> + * Returns 0 on success or a negative error code
> + */
> +int devm_adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev)
> +{
> +	int ret;
> +
> +	adis->trig = devm_iio_trigger_alloc(&adis->spi->dev, "%s-dev%d",
> +					    indio_dev->name, indio_dev->id);
> +	if (!adis->trig)
> +		return -ENOMEM;
> +
> +	adis_trigger_setup(adis);
> +
> +	ret = devm_request_irq(&adis->spi->dev, adis->spi->irq,
> +			       &iio_trigger_generic_data_rdy_poll,
> +			       IRQF_TRIGGER_RISING,
> +			       indio_dev->name,
> +			       adis->trig);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_trigger_register(&adis->spi->dev, adis->trig);
> +}
> +EXPORT_SYMBOL_GPL(devm_adis_probe_trigger);
> +
>  /**
>   * adis_remove_trigger() - Remove trigger for a adis devices
>   * @adis: The adis device
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index dd8219138c2e..ac94c483bf2b 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -448,11 +448,15 @@ struct adis_burst {
>  	unsigned int	extra_len;
>  };
>  
> +int
> +devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
> +				   irqreturn_t (*trigger_handler)(int, void *));
>  int adis_setup_buffer_and_trigger(struct adis *adis,
>  	struct iio_dev *indio_dev, irqreturn_t (*trigger_handler)(int, void *));
>  void adis_cleanup_buffer_and_trigger(struct adis *adis,
>  	struct iio_dev *indio_dev);
>  
> +int devm_adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev);
>  int adis_probe_trigger(struct adis *adis, struct iio_dev *indio_dev);
>  void adis_remove_trigger(struct adis *adis);
>  
> @@ -461,6 +465,13 @@ int adis_update_scan_mode(struct iio_dev *indio_dev,
>  
>  #else /* CONFIG_IIO_BUFFER */
>  
> +static inline int
> +devm_adis_setup_buffer_and_trigger(struct adis *adis, struct iio_dev *indio_dev,
> +				   irqreturn_t (*trigger_handler)(int, void *))
> +{
> +	return 0;
> +}
> +
>  static inline int adis_setup_buffer_and_trigger(struct adis *adis,
>  	struct iio_dev *indio_dev, irqreturn_t (*trigger_handler)(int, void *))
>  {
> @@ -472,6 +483,12 @@ static inline void adis_cleanup_buffer_and_trigger(struct adis *adis,
>  {
>  }
>  
> +static inline int devm_adis_probe_trigger(struct adis *adis,
> +					  struct iio_dev *indio_dev)
> +{
> +	return 0;
> +}
> +
>  static inline int adis_probe_trigger(struct adis *adis,
>  	struct iio_dev *indio_dev)
>  {