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

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

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