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

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 29 Jan 2025 10:20:41 +0200
Robert Budai <robert.budai@analog.com> wrote:

> 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.
Wrap at 75 chars, not around 62

Otherwise this looks fine to me.  Could have been merged
with patch 2 as that just adds another op, but that's not important.

> 
> 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>
> Signed-off-by: Robert Budai <robert.budai@analog.com>
> ---
>  drivers/iio/imu/adis.c       | 16 +++++++++++++---
>  include/linux/iio/imu/adis.h | 28 +++++++++++++++++++++-------
>  2 files changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/imu/adis.c b/drivers/iio/imu/adis.c
> index 494171844812..54915c7a3e76 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");
>  
> @@ -468,7 +468,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 +488,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 +522,11 @@ 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;
> +	else if (!adis->ops->write || !adis->ops->read)
> +		return -EINVAL;
> +
>  	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..04140b36712a 100644
> --- a/include/linux/iio/imu/adis.h
> +++ b/include/linux/iio/imu/adis.h
> @@ -94,6 +94,18 @@ 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.
> + */
> +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);
> +};
> +
>  /**
>   * struct adis - ADIS device instance data
>   * @spi: Reference to SPI device which owns this ADIS IIO device
> @@ -102,6 +114,7 @@ struct adis_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.
> + * @ops: ops struct for custom read and write functions
>   * @state_lock: Lock used by the device to protect state
>   * @msg: SPI message object
>   * @xfer: SPI transfer objects to be used for a @msg
> @@ -117,6 +130,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 +183,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 +195,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 +207,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 +222,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 +241,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 +259,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 +273,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);
>  }
>  
>  /**