Re: [PATCH v3 05/10] iio: backend: extend features

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 19 Sep 2024 11:20:01 +0200
Angelo Dureghello <adureghello@baylibre.com> wrote:

> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Extend backend features with new calls needed later on this
> patchset from axi version of ad3552r.
> 
> The follwoing calls are added:
> 
> iio_backend_ext_sync_enable
> 	enable synchronize channels on external trigger
> iio_backend_ext_sync_disable
> 	disable synchronize channels on external trigger
> iio_backend_ddr_enable
> 	enable ddr bus transfer
> iio_backend_ddr_disable
> 	disable ddr bus transfer
> iio_backend_set_bus_mode
> 	select the type of bus, so that specific read / write
> 	operations are performed accordingly
> iio_backend_buffer_enable
> 	enable buffer
> iio_backend_buffer_disable
> 	disable buffer
> iio_backend_data_transfer_addr
> 	define the target register address where the DAC sample
> 	will be written.
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
Hi Angelo,
A few trivial comments inline.

> ---
>  drivers/iio/industrialio-backend.c | 111 +++++++++++++++++++++++++++++++++++++
>  include/linux/iio/backend.h        |  23 ++++++++
>  2 files changed, 134 insertions(+)
> 
> diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-backend.c
> index 20b3b5212da7..f4802c422dbf 100644
> --- a/drivers/iio/industrialio-backend.c
> +++ b/drivers/iio/industrialio-backend.c
> @@ -718,6 +718,117 @@ static int __devm_iio_backend_get(struct device *dev, struct iio_backend *back)
...

> +/**
> + * iio_backend_ddr_disable - Disable interface DDR (Double Data Rate) mode
> + * @back: Backend device
> + *
> + * Disabling DDR data is generated byt the IP at rising or falling front

Spell check your comments.

> + * of the interface clock signal (SDR, Single Data Rate).
> + *
> + * RETURNS:
> + * 0 on success, negative error number on failure.
> + */
> +int iio_backend_ddr_disable(struct iio_backend *back)
> +{
> +	return iio_backend_op_call(back, ddr_disable);
> +}
> +EXPORT_SYMBOL_NS_GPL(iio_backend_ddr_disable, IIO_BACKEND);
				 struct fwnode_handle *fwnode)
>  {
> diff --git a/include/linux/iio/backend.h b/include/linux/iio/backend.h
> index 37d56914d485..41619b803cd6 100644
> --- a/include/linux/iio/backend.h
> +++ b/include/linux/iio/backend.h
> @@ -14,12 +14,14 @@ struct iio_dev;
>  enum iio_backend_data_type {
>  	IIO_BACKEND_TWOS_COMPLEMENT,
>  	IIO_BACKEND_OFFSET_BINARY,
> +	IIO_BACKEND_DATA_UNSIGNED,
>  	IIO_BACKEND_DATA_TYPE_MAX
>  };
>  
>  enum iio_backend_data_source {
>  	IIO_BACKEND_INTERNAL_CONTINUOUS_WAVE,
>  	IIO_BACKEND_EXTERNAL,
> +	IIO_BACKEND_INTERNAL_RAMP_16BIT,
>  	IIO_BACKEND_DATA_SOURCE_MAX
>  };
>  
> @@ -89,6 +91,13 @@ enum iio_backend_sample_trigger {
>   * @read_raw: Read a channel attribute from a backend device
>   * @debugfs_print_chan_status: Print channel status into a buffer.
>   * @debugfs_reg_access: Read or write register value of backend.
> + * @ext_sync_enable: Enable external synchronization.
> + * @ext_sync_disable: Disable external synchronization.
> + * @ddr_enable: Enable interface DDR (Double Data Rate) mode.
> + * @ddr_disable: Disable interface DDR (Double Data Rate) mode.
> + * @buffer_enable: Enable data buffer.
> + * @buffer_disable: Disable data buffer.

This needs more specific text. What buffer?  I think this came
up earlier but it needs to say something about the fact it's enabling
or disabling the actual capture of data into the DMA buffers that
userspace will read.

> + * @data_transfer_addr: Set data address.
>   **/
>  struct iio_backend_ops {
>  	int (*enable)(struct iio_backend *back);
> @@ -129,6 +138,13 @@ struct iio_backend_ops {
>  					 size_t len);
>  	int (*debugfs_reg_access)(struct iio_backend *back, unsigned int reg,
>  				  unsigned int writeval, unsigned int *readval);
> +	int (*ext_sync_enable)(struct iio_backend *back);
I know we've done it this way for existing items, but I wonder if we should
squish down the ops slightly and have new enable/disable pairs as
single functions.
	int (*ext_sync_set_state)(struct iio_backend *back, bool enable);
etc.  If nothing else reduces how many things need documentation ;)

Nuno, what do you think? Worth squashing these pairs into single
callbacks?

> +	int (*ext_sync_disable)(struct iio_backend *back);
> +	int (*ddr_enable)(struct iio_backend *back);
> +	int (*ddr_disable)(struct iio_backend *back);
> +	int (*buffer_enable)(struct iio_backend *back);
> +	int (*buffer_disable)(struct iio_backend *back);
> +	int (*data_transfer_addr)(struct iio_backend *back, u32 address);
>  };