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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20240929120535.6b41c37e@jic23-huawei>
Date: Sun, 29 Sep 2024 12:05:35 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Angelo Dureghello <adureghello@...libre.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Nuno Sa <nuno.sa@...log.com>, Rob Herring
 <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
 <conor+dt@...nel.org>, Olivier Moysan <olivier.moysan@...s.st.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org,
 devicetree@...r.kernel.org, dlechner@...libre.com
Subject: Re: [PATCH v3 05/10] iio: backend: extend features

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

> From: Angelo Dureghello <adureghello@...libre.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@...libre.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);
>  };