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

[David Lechner <dlechner@baylibre.com>]

On 9/29/24 6:05 AM, Jonathan Cameron wrote:
> 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?

I'm not a fan of combining enable and disable functions into one function.

The implementation will pretty much always be:

if (enabled) {
        /* so stuff */
} else {
        /* do other stuff */
}

Which just adds indent and makes code harder to read.

> 
>> +	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);
>>  };