[PATCH 0/5] Add support for ADIS16550 and ADIS16550W

[PATCH 0/5] Add support for ADIS16550 and ADIS16550W

[Robert Budai]

The ADIS16550 is a complete inertial system that includes a triaxis gyroscope
and a triaxis accelerometer. Each inertial sensor in the ADIS16550 combines
industry leading MEMS only technology with signal conditioning that optimizes
dynamic performance. The factory calibration characterizes each sensor for
sensitivity, bias, and alignment. As a result, each sensor has its own dynamic
compensation formulas that provide accurate sensor measurements.

Nuno Sá (3):
  iio: imu: adis: Add custom ops struct
  iio: imu: adis: Add DIAG_STAT register size
  iio: imu: adis16550: add adis16550 support

Ramona Gradinariu (2):
  dt-bindings: iio: Add adis16550 bindings
  docs: iio: add documentation for adis16550 driver

 .../bindings/iio/imu/adi,adis16550.yaml       |   95 ++
 Documentation/iio/adis16550.rst               |  389 ++++++
 Documentation/iio/index.rst                   |    1 +
 MAINTAINERS                                   |   10 +
 drivers/iio/imu/Kconfig                       |   13 +
 drivers/iio/imu/Makefile                      |    1 +
 drivers/iio/imu/adis.c                        |   33 +-
 drivers/iio/imu/adis16550.c                   | 1228 +++++++++++++++++
 include/linux/iio/imu/adis.h                  |   33 +-
 9 files changed, 1788 insertions(+), 15 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/imu/adi,adis16550.yaml
 create mode 100644 Documentation/iio/adis16550.rst
 create mode 100644 drivers/iio/imu/adis16550.c

-- 
2.34.1

[PATCH 1/5] iio: imu: adis: Add custom ops struct

[Robert Budai]

From: Nuno Sá <nuno.sa@analog.com>

This patch introduces a custom ops struct letting users define
custom read and write functions. Some adis devices might define
a completely different spi protocol from the one used in the
default implementation.

Co-developed-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
Co-developed-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/imu/adis.c       | 21 ++++++++++++++++-----
 include/linux/iio/imu/adis.h | 30 +++++++++++++++++++++++-------
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index 99410733c1ca..504d18a36f90 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -223,13 +223,13 @@ int __adis_update_bits_base(struct adis *adis, unsigned int reg, const u32 mask,
 	int ret;
 	u32 __val;
 
-	ret = __adis_read_reg(adis, reg, &__val, size);
+	ret = adis->ops->read(adis, reg, &__val, size);
 	if (ret)
 		return ret;
 
 	__val = (__val & ~mask) | (val & mask);
 
-	return __adis_write_reg(adis, reg, __val, size);
+	return adis->ops->write(adis, reg, __val, size);
 }
 EXPORT_SYMBOL_NS_GPL(__adis_update_bits_base, IIO_ADISLIB);
 
@@ -339,8 +339,11 @@ int __adis_reset(struct adis *adis)
 	int ret;
 	const struct adis_timeout *timeouts = adis->data->timeouts;
 
-	ret = __adis_write_reg_8(adis, adis->data->glob_cmd_reg,
-				 ADIS_GLOB_CMD_SW_RESET);
+	if (adis->ops->reset)
+		ret = adis->ops->reset(adis);
+	else
+		ret = __adis_write_reg_8(adis, adis->data->glob_cmd_reg,
+					 ADIS_GLOB_CMD_SW_RESET);
 	if (ret) {
 		dev_err(&adis->spi->dev, "Failed to reset device: %d\n", ret);
 		return ret;
@@ -468,7 +471,7 @@ int adis_single_conversion(struct iio_dev *indio_dev,
 
 	guard(mutex)(&adis->state_lock);
 
-	ret = __adis_read_reg(adis, chan->address, &uval,
+	ret = adis->ops->read(adis, chan->address, &uval,
 			      chan->scan_type.storagebits / 8);
 	if (ret)
 		return ret;
@@ -488,6 +491,11 @@ int adis_single_conversion(struct iio_dev *indio_dev,
 }
 EXPORT_SYMBOL_NS_GPL(adis_single_conversion, IIO_ADISLIB);
 
+static const struct adis_ops adis_default_ops = {
+	.read = __adis_read_reg,
+	.write = __adis_write_reg,
+};
+
 /**
  * adis_init() - Initialize adis device structure
  * @adis:	The adis device
@@ -517,6 +525,9 @@ int adis_init(struct adis *adis, struct iio_dev *indio_dev,
 
 	adis->spi = spi;
 	adis->data = data;
+	if (!adis->ops->write || !adis->ops->read)
+		adis->ops = &adis_default_ops;
+
 	iio_device_set_drvdata(indio_dev, adis);
 
 	if (data->has_paging) {
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index e6a75356567a..7b589cc83380 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -94,6 +94,21 @@ struct adis_data {
 	unsigned int burst_max_speed_hz;
 };
 
+/**
+ * struct adis_ops: Custom ops for adis devices.
+ * @write: Custom spi write implementation.
+ * @read: Custom spi read implementation.
+ * @reset: Custom sw reset implementation. The custom implementation does not
+ *	   need to sleep after the reset. It's done by the library already.
+ */
+struct adis_ops {
+	int (*write)(struct adis *adis, unsigned int reg, unsigned int value,
+		     unsigned int size);
+	int (*read)(struct adis *adis, unsigned int reg, unsigned int *value,
+		    unsigned int size);
+	int (*reset)(struct adis *adis);
+};
+
 /**
  * struct adis - ADIS device instance data
  * @spi: Reference to SPI device which owns this ADIS IIO device
@@ -117,6 +132,7 @@ struct adis {
 
 	const struct adis_data	*data;
 	unsigned int		burst_extra_len;
+	const struct adis_ops	*ops;
 	/**
 	 * 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
@@ -169,7 +185,7 @@ int __adis_read_reg(struct adis *adis, unsigned int reg,
 static inline int __adis_write_reg_8(struct adis *adis, unsigned int reg,
 				     u8 val)
 {
-	return __adis_write_reg(adis, reg, val, 1);
+	return adis->ops->write(adis, reg, val, 1);
 }
 
 /**
@@ -181,7 +197,7 @@ static inline int __adis_write_reg_8(struct adis *adis, unsigned int reg,
 static inline int __adis_write_reg_16(struct adis *adis, unsigned int reg,
 				      u16 val)
 {
-	return __adis_write_reg(adis, reg, val, 2);
+	return adis->ops->write(adis, reg, val, 2);
 }
 
 /**
@@ -193,7 +209,7 @@ static inline int __adis_write_reg_16(struct adis *adis, unsigned int reg,
 static inline int __adis_write_reg_32(struct adis *adis, unsigned int reg,
 				      u32 val)
 {
-	return __adis_write_reg(adis, reg, val, 4);
+	return adis->ops->write(adis, reg, val, 4);
 }
 
 /**
@@ -208,7 +224,7 @@ static inline int __adis_read_reg_16(struct adis *adis, unsigned int reg,
 	unsigned int tmp;
 	int ret;
 
-	ret = __adis_read_reg(adis, reg, &tmp, 2);
+	ret = adis->ops->read(adis, reg, &tmp, 2);
 	if (ret == 0)
 		*val = tmp;
 
@@ -227,7 +243,7 @@ static inline int __adis_read_reg_32(struct adis *adis, unsigned int reg,
 	unsigned int tmp;
 	int ret;
 
-	ret = __adis_read_reg(adis, reg, &tmp, 4);
+	ret = adis->ops->read(adis, reg, &tmp, 4);
 	if (ret == 0)
 		*val = tmp;
 
@@ -245,7 +261,7 @@ static inline int adis_write_reg(struct adis *adis, unsigned int reg,
 				 unsigned int val, unsigned int size)
 {
 	guard(mutex)(&adis->state_lock);
-	return __adis_write_reg(adis, reg, val, size);
+	return adis->ops->write(adis, reg, val, size);
 }
 
 /**
@@ -259,7 +275,7 @@ static int adis_read_reg(struct adis *adis, unsigned int reg,
 			 unsigned int *val, unsigned int size)
 {
 	guard(mutex)(&adis->state_lock);
-	return __adis_read_reg(adis, reg, val, size);
+	return adis->ops->read(adis, reg, val, size);
 }
 
 /**
-- 
2.34.1

[PATCH 2/5] iio: imu: adis: Add DIAG_STAT register size

[Robert Budai]

From: Nuno Sá <nuno.sa@analog.com>

Some devices may have more than 16 bits of status. This patch allows the
user to specify the size of the DIAG_STAT register. It defaults to 2 if
not specified. This is mainly for backward compatibility.

Co-developed-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
Co-developed-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 drivers/iio/imu/adis.c       | 12 +++++++++---
 include/linux/iio/imu/adis.h |  3 +++
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
index 504d18a36f90..f03f35c94f76 100644
--- a/drivers/iio/imu/adis.c
+++ b/drivers/iio/imu/adis.c
@@ -304,11 +304,17 @@ EXPORT_SYMBOL_NS(__adis_enable_irq, IIO_ADISLIB);
  */
 int __adis_check_status(struct adis *adis)
 {
-	u16 status;
+	unsigned int status = 0;
 	int ret;
 	int i;
+	/* default to 2 bytes */
+	unsigned int reg_size = 2;
 
-	ret = __adis_read_reg_16(adis, adis->data->diag_stat_reg, &status);
+	if (adis->data->diag_stat_size)
+		reg_size = adis->data->diag_stat_size;
+
+	ret = adis->ops->read(adis, adis->data->diag_stat_reg, &status,
+			      reg_size);
 	if (ret)
 		return ret;
 
@@ -317,7 +323,7 @@ int __adis_check_status(struct adis *adis)
 	if (status == 0)
 		return 0;
 
-	for (i = 0; i < 16; ++i) {
+	for (i = 0; i < (reg_size * 8); ++i) {
 		if (status & BIT(i)) {
 			dev_err(&adis->spi->dev, "%s.\n",
 				adis->data->status_error_msgs[i]);
diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
index 7b589cc83380..fae31042a622 100644
--- a/include/linux/iio/imu/adis.h
+++ b/include/linux/iio/imu/adis.h
@@ -44,6 +44,8 @@ struct adis_timeout {
  * @glob_cmd_reg: Register address of the GLOB_CMD register
  * @msc_ctrl_reg: Register address of the MSC_CTRL register
  * @diag_stat_reg: Register address of the DIAG_STAT register
+ * @diag_stat_size: Length (in bytes) of the DIAG_STAT register.
+ *		    Defaults to 2 if not set.
  * @prod_id_reg: Register address of the PROD_ID register
  * @prod_id: Product ID code that should be expected when reading @prod_id_reg
  * @self_test_mask: Bitmask of supported self-test operations
@@ -70,6 +72,7 @@ struct adis_data {
 	unsigned int glob_cmd_reg;
 	unsigned int msc_ctrl_reg;
 	unsigned int diag_stat_reg;
+	unsigned int diag_stat_size;
 	unsigned int prod_id_reg;
 
 	unsigned int prod_id;
-- 
2.34.1

Re: [PATCH 1/5] iio: imu: adis: Add custom ops struct

[Jonathan Cameron]

On Mon, 28 Oct 2024 14:25:33 +0200
Robert Budai <robert.budai@analog.com> wrote:

> From: Nuno Sá <nuno.sa@analog.com>
> 
> This patch introduces a custom ops struct letting users define
> custom read and write functions. Some adis devices might define
> a completely different spi protocol from the one used in the
> default implementation.

Also needs to mention the reset as that is nothing to do with
bus access.

> 
> Co-developed-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> Co-developed-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Minor comments inline

Jonathan

>  
> @@ -339,8 +339,11 @@ int __adis_reset(struct adis *adis)
>  	int ret;
>  	const struct adis_timeout *timeouts = adis->data->timeouts;
>  
> -	ret = __adis_write_reg_8(adis, adis->data->glob_cmd_reg,
> -				 ADIS_GLOB_CMD_SW_RESET);
> +	if (adis->ops->reset)

This one looks to be unrelated to the read / write path and
isn't mentioned in the patch description. Perhaps better to add
it in a separate patch where you can talk about why is it is needed. 

> +		ret = adis->ops->reset(adis);
> +	else
> +		ret = __adis_write_reg_8(adis, adis->data->glob_cmd_reg,
> +					 ADIS_GLOB_CMD_SW_RESET);
>  	if (ret) {
>  		dev_err(&adis->spi->dev, "Failed to reset device: %d\n", ret);
>  		return ret;

>  /**
>   * adis_init() - Initialize adis device structure
>   * @adis:	The adis device
> @@ -517,6 +525,9 @@ int adis_init(struct adis *adis, struct iio_dev *indio_dev,
>  
>  	adis->spi = spi;
>  	adis->data = data;
> +	if (!adis->ops->write || !adis->ops->read)
> +		adis->ops = &adis_default_ops;

If only write or read is specified, error out, don't replace with
the default ops as that clearly indicates a bug.


> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index e6a75356567a..7b589cc83380 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -94,6 +94,21 @@ struct adis_data {
>  	unsigned int burst_max_speed_hz;
>  };
>  
> +/**
> + * struct adis_ops: Custom ops for adis devices.
> + * @write: Custom spi write implementation.
> + * @read: Custom spi read implementation.
> + * @reset: Custom sw reset implementation. The custom implementation does not
> + *	   need to sleep after the reset. It's done by the library already.
> + */
> +struct adis_ops {
> +	int (*write)(struct adis *adis, unsigned int reg, unsigned int value,
> +		     unsigned int size);
> +	int (*read)(struct adis *adis, unsigned int reg, unsigned int *value,
> +		    unsigned int size);
> +	int (*reset)(struct adis *adis);
> +};
> +
>  /**
>   * struct adis - ADIS device instance data
>   * @spi: Reference to SPI device which owns this ADIS IIO device
> @@ -117,6 +132,7 @@ struct adis {
>  
>  	const struct adis_data	*data;
>  	unsigned int		burst_extra_len;
> +	const struct adis_ops	*ops;

Docs?  This structure has kernel-doc that needs updating to cover this new element.
Also, whilst you are here, please can you fix that doc in general (as 
precursor patch preferably).

At least one element in the docs doesn't seem to exist in the structure.

>  	/**
>  	 * 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

Re: [PATCH 1/5] iio: imu: adis: Add custom ops struct

[Jonathan Cameron]

On Mon, 28 Oct 2024 14:25:33 +0200
Robert Budai <robert.budai@analog.com> wrote:

> From: Nuno Sá <nuno.sa@analog.com>
> 
> This patch introduces a custom ops struct letting users define
> custom read and write functions. Some adis devices might define
> a completely different spi protocol from the one used in the
> default implementation.
> 
> Co-developed-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> Co-developed-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>

Robert, you need to sign off on these if you are the person sending them
to the list - this says you 'handled' them and can verify the
rest of the tags are what you received etc.
(welcome to IIO btw!)

Jonathan

Re: [PATCH 2/5] iio: imu: adis: Add DIAG_STAT register size

[Jonathan Cameron]

On Mon, 28 Oct 2024 14:25:34 +0200
Robert Budai <robert.budai@analog.com> wrote:

> From: Nuno Sá <nuno.sa@analog.com>
> 
> Some devices may have more than 16 bits of status. This patch allows the
> user to specify the size of the DIAG_STAT register. It defaults to 2 if
> not specified. This is mainly for backward compatibility.
> 
> Co-developed-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> Signed-off-by: Ramona Gradinariu <ramona.gradinariu@analog.com>
> Co-developed-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@analog.com>
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
I'd rather we didn't use a default for this one.
The value 2 isn't obvious.  So just update all existing cases
in this patch and drop the check on whether it is set.

Again, remember to add your SoB on this.

One other minor comment inline.

Jonathan

> ---
>  drivers/iio/imu/adis.c       | 12 +++++++++---
>  include/linux/iio/imu/adis.h |  3 +++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 504d18a36f90..f03f35c94f76 100644
> --- a/drivers/iio/imu/adis.c
> +++ b/drivers/iio/imu/adis.c
> @@ -304,11 +304,17 @@ EXPORT_SYMBOL_NS(__adis_enable_irq, IIO_ADISLIB);
>   */
>  int __adis_check_status(struct adis *adis)
>  {
> -	u16 status;
> +	unsigned int status = 0;
>  	int ret;
>  	int i;
> +	/* default to 2 bytes */
> +	unsigned int reg_size = 2;
>  
> -	ret = __adis_read_reg_16(adis, adis->data->diag_stat_reg, &status);
> +	if (adis->data->diag_stat_size)
> +		reg_size = adis->data->diag_stat_size;
> +
> +	ret = adis->ops->read(adis, adis->data->diag_stat_reg, &status,
> +			      reg_size);
>  	if (ret)
>  		return ret;
>  
> @@ -317,7 +323,7 @@ int __adis_check_status(struct adis *adis)
>  	if (status == 0)
>  		return 0;
>  
> -	for (i = 0; i < 16; ++i) {
> +	for (i = 0; i < (reg_size * 8); ++i) {
BITS_PER_BYTE instead of 8 and no need for the brackets around (reg_size * BITS_PER_BYTE)

>  		if (status & BIT(i)) {
>  			dev_err(&adis->spi->dev, "%s.\n",
>  				adis->data->status_error_msgs[i]);
> diff --git a/include/linux/iio/imu/adis.h b/include/linux/iio/imu/adis.h
> index 7b589cc83380..fae31042a622 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -44,6 +44,8 @@ struct adis_timeout {
>   * @glob_cmd_reg: Register address of the GLOB_CMD register
>   * @msc_ctrl_reg: Register address of the MSC_CTRL register
>   * @diag_stat_reg: Register address of the DIAG_STAT register
> + * @diag_stat_size: Length (in bytes) of the DIAG_STAT register.
> + *		    Defaults to 2 if not set.
>   * @prod_id_reg: Register address of the PROD_ID register
>   * @prod_id: Product ID code that should be expected when reading @prod_id_reg
>   * @self_test_mask: Bitmask of supported self-test operations
> @@ -70,6 +72,7 @@ struct adis_data {
>  	unsigned int glob_cmd_reg;
>  	unsigned int msc_ctrl_reg;
>  	unsigned int diag_stat_reg;
> +	unsigned int diag_stat_size;
>  	unsigned int prod_id_reg;
>  
>  	unsigned int prod_id;