On 9/29/24 6:05 AM, Jonathan Cameron wrote:
> On Thu, 19 Sep 2024 11:20:01 +0200
> Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote:
>
>> From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
>>
>> 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@xxxxxxxxxxxx>
> 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);
>> };
