1. Patchew
  2. linux
  3. [PATCH 0/5] Add support for ADIS16550 and ADIS16550W
  4. View patch

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

Robert Budai posted 5 patches 3 weeks, 6 days ago
Only 2 patches received!
There is a newer version of this series
[PATCH 1/5] iio: imu: adis: Add custom ops struct
Posted by Robert Budai 3 weeks, 6 days ago 
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
Re: [PATCH 1/5] iio: imu: adis: Add custom ops struct
Posted by Jonathan Cameron 3 weeks, 6 days ago 
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 1/5] iio: imu: adis: Add custom ops struct
Posted by Jonathan Cameron 3 weeks, 6 days ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2024 Red Hat, Inc.