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