| Message ID | 20240919-wip-bl-ad3552r-axi-v0-iio-testing-v3-5-a17b9b3d05d9@baylibre.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | iio: add support for the ad3552r AXI DAC IP | expand |
On Thu, 2024-09-19 at 11:20 +0200, Angelo Dureghello 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> > --- > 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) > return 0; > } > > +/** > + * iio_backend_ext_sync_enable - Enable external synchronization > + * @back: Backend device > + * > + * Enable synchronization by external signal. > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int iio_backend_ext_sync_enable(struct iio_backend *back) > +{ > + return iio_backend_op_call(back, ext_sync_enable); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_ext_sync_enable, IIO_BACKEND); > + > +/** > + * iio_backend_ext_sync_disable - Disable external synchronization > + * @back: Backend device > + * > + * Disable synchronization by external signal. > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int iio_backend_ext_sync_disable(struct iio_backend *back) > +{ > + return iio_backend_op_call(back, ext_sync_disable); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_ext_sync_disable, IIO_BACKEND); > + > +/** > + * iio_backend_ddr_enable - Enable interface DDR (Double Data Rate) mode > + * @back: Backend device > + * > + * Enabling DDR, data is generated by the IP at each front > + * (raising and falling) of the bus clock signal. > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int iio_backend_ddr_enable(struct iio_backend *back) > +{ > + return iio_backend_op_call(back, ddr_enable); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_ddr_enable, IIO_BACKEND); > + > +/** > + * 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 > + * 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); > + > +/** > + * iio_backend_buffer_enable - Enable iio buffering > + * @back: Backend device > + * > + * Enabling the buffer, buffer data is processed and sent out from the > + * bus interface. > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int iio_backend_buffer_enable(struct iio_backend *back) > +{ > + return iio_backend_op_call(back, buffer_enable); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_enable, IIO_BACKEND); > + > +/** > + * iio_backend_buffer_disable - Disable iio buffering > + * @back: Backend device > + * > + * Disabling the buffer, buffer data transfer on the bus interface > + * is stopped. > + * > + * RETURNS: > + * 0 on success, negative error number on failure. > + */ > +int iio_backend_buffer_disable(struct iio_backend *back) > +{ > + return iio_backend_op_call(back, buffer_disable); > +} > +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_disable, IIO_BACKEND); > + IIRC, both me and Jonathan had some comments about the above 2 calls? Aren't they about buffering? I think I mentioned something about using the same buffer ops as typical IIO devices use. - Nuno Sá
On 20/09/24 14:50, Nuno Sá wrote: > On Thu, 2024-09-19 at 11:20 +0200, Angelo Dureghello 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> >> --- >> 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) >> return 0; >> } >> >> +/** >> + * iio_backend_ext_sync_enable - Enable external synchronization >> + * @back: Backend device >> + * >> + * Enable synchronization by external signal. >> + * >> + * RETURNS: >> + * 0 on success, negative error number on failure. >> + */ >> +int iio_backend_ext_sync_enable(struct iio_backend *back) >> +{ >> + return iio_backend_op_call(back, ext_sync_enable); >> +} >> +EXPORT_SYMBOL_NS_GPL(iio_backend_ext_sync_enable, IIO_BACKEND); >> + >> +/** >> + * iio_backend_ext_sync_disable - Disable external synchronization >> + * @back: Backend device >> + * >> + * Disable synchronization by external signal. >> + * >> + * RETURNS: >> + * 0 on success, negative error number on failure. >> + */ >> +int iio_backend_ext_sync_disable(struct iio_backend *back) >> +{ >> + return iio_backend_op_call(back, ext_sync_disable); >> +} >> +EXPORT_SYMBOL_NS_GPL(iio_backend_ext_sync_disable, IIO_BACKEND); >> + >> +/** >> + * iio_backend_ddr_enable - Enable interface DDR (Double Data Rate) mode >> + * @back: Backend device >> + * >> + * Enabling DDR, data is generated by the IP at each front >> + * (raising and falling) of the bus clock signal. >> + * >> + * RETURNS: >> + * 0 on success, negative error number on failure. >> + */ >> +int iio_backend_ddr_enable(struct iio_backend *back) >> +{ >> + return iio_backend_op_call(back, ddr_enable); >> +} >> +EXPORT_SYMBOL_NS_GPL(iio_backend_ddr_enable, IIO_BACKEND); >> + >> +/** >> + * 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 >> + * 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); >> + >> +/** >> + * iio_backend_buffer_enable - Enable iio buffering >> + * @back: Backend device >> + * >> + * Enabling the buffer, buffer data is processed and sent out from the >> + * bus interface. >> + * >> + * RETURNS: >> + * 0 on success, negative error number on failure. >> + */ >> +int iio_backend_buffer_enable(struct iio_backend *back) >> +{ >> + return iio_backend_op_call(back, buffer_enable); >> +} >> +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_enable, IIO_BACKEND); >> + >> +/** >> + * iio_backend_buffer_disable - Disable iio buffering >> + * @back: Backend device >> + * >> + * Disabling the buffer, buffer data transfer on the bus interface >> + * is stopped. >> + * >> + * RETURNS: >> + * 0 on success, negative error number on failure. >> + */ >> +int iio_backend_buffer_disable(struct iio_backend *back) >> +{ >> + return iio_backend_op_call(back, buffer_disable); >> +} >> +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_disable, IIO_BACKEND); >> + > IIRC, both me and Jonathan had some comments about the above 2 calls? Aren't they > about buffering? I think I mentioned something about using the same buffer ops as > typical IIO devices use. i have now separated iio_backend_ops, keeping buffer enable/disable for axi-ad3352r case only, static const struct iio_backend_ops axi_dac_generic_ops = { .enable = axi_dac_enable, .disable = axi_dac_disable, .request_buffer = axi_dac_request_buffer, .free_buffer = axi_dac_free_buffer, .extend_chan_spec = axi_dac_extend_chan, .ext_info_set = axi_dac_ext_info_set, .ext_info_get = axi_dac_ext_info_get, .data_source_set = axi_dac_data_source_set, .set_sample_rate = axi_dac_set_sample_rate, .debugfs_reg_access = iio_backend_debugfs_ptr(axi_dac_reg_access), }; static const struct iio_backend_ops axi_ad3552r_ops = { .enable = axi_dac_enable, .read_raw = axi_dac_read_raw, .request_buffer = axi_dac_request_buffer, .data_source_set = axi_dac_data_source_set, .ext_sync_enable = axi_dac_ext_sync_enable, .ext_sync_disable = axi_dac_ext_sync_disable, .ddr_enable = axi_dac_ddr_enable, .ddr_disable = axi_dac_ddr_disable, .buffer_enable = axi_dac_buffer_enable, .buffer_disable = axi_dac_buffer_disable, .data_format_set = axi_dac_data_format_set, .data_transfer_addr = axi_dac_data_transfer_addr, }; could this be good ? > - Nuno Sá Regards, Angelo
On Tue, 2024-09-24 at 16:11 +0200, Angelo Dureghello wrote: > > On 20/09/24 14:50, Nuno Sá wrote: > > On Thu, 2024-09-19 at 11:20 +0200, Angelo Dureghello 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> > > > --- > > > 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) > > > return 0; > > > } > > > > > > +/** > > > + * iio_backend_ext_sync_enable - Enable external synchronization > > > + * @back: Backend device > > > + * > > > + * Enable synchronization by external signal. > > > + * > > > + * RETURNS: > > > + * 0 on success, negative error number on failure. > > > + */ > > > +int iio_backend_ext_sync_enable(struct iio_backend *back) > > > +{ > > > + return iio_backend_op_call(back, ext_sync_enable); > > > +} > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_ext_sync_enable, IIO_BACKEND); > > > + > > > +/** > > > + * iio_backend_ext_sync_disable - Disable external synchronization > > > + * @back: Backend device > > > + * > > > + * Disable synchronization by external signal. > > > + * > > > + * RETURNS: > > > + * 0 on success, negative error number on failure. > > > + */ > > > +int iio_backend_ext_sync_disable(struct iio_backend *back) > > > +{ > > > + return iio_backend_op_call(back, ext_sync_disable); > > > +} > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_ext_sync_disable, IIO_BACKEND); > > > + > > > +/** > > > + * iio_backend_ddr_enable - Enable interface DDR (Double Data Rate) mode > > > + * @back: Backend device > > > + * > > > + * Enabling DDR, data is generated by the IP at each front > > > + * (raising and falling) of the bus clock signal. > > > + * > > > + * RETURNS: > > > + * 0 on success, negative error number on failure. > > > + */ > > > +int iio_backend_ddr_enable(struct iio_backend *back) > > > +{ > > > + return iio_backend_op_call(back, ddr_enable); > > > +} > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_ddr_enable, IIO_BACKEND); > > > + > > > +/** > > > + * 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 > > > + * 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); > > > + > > > +/** > > > + * iio_backend_buffer_enable - Enable iio buffering > > > + * @back: Backend device > > > + * > > > + * Enabling the buffer, buffer data is processed and sent out from the > > > + * bus interface. > > > + * > > > + * RETURNS: > > > + * 0 on success, negative error number on failure. > > > + */ > > > +int iio_backend_buffer_enable(struct iio_backend *back) > > > +{ > > > + return iio_backend_op_call(back, buffer_enable); > > > +} > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_enable, IIO_BACKEND); > > > + > > > +/** > > > + * iio_backend_buffer_disable - Disable iio buffering > > > + * @back: Backend device > > > + * > > > + * Disabling the buffer, buffer data transfer on the bus interface > > > + * is stopped. > > > + * > > > + * RETURNS: > > > + * 0 on success, negative error number on failure. > > > + */ > > > +int iio_backend_buffer_disable(struct iio_backend *back) > > > +{ > > > + return iio_backend_op_call(back, buffer_disable); > > > +} > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_disable, IIO_BACKEND); > > > + > > IIRC, both me and Jonathan had some comments about the above 2 calls? Aren't > > they > > about buffering? I think I mentioned something about using the same buffer > > ops as > > typical IIO devices use. > > i have now separated iio_backend_ops, keeping buffer enable/disable > for axi-ad3352r case only, > > static const struct iio_backend_ops axi_dac_generic_ops = { > .enable = axi_dac_enable, > .disable = axi_dac_disable, > .request_buffer = axi_dac_request_buffer, > .free_buffer = axi_dac_free_buffer, > .extend_chan_spec = axi_dac_extend_chan, > .ext_info_set = axi_dac_ext_info_set, > .ext_info_get = axi_dac_ext_info_get, > .data_source_set = axi_dac_data_source_set, > .set_sample_rate = axi_dac_set_sample_rate, > .debugfs_reg_access = iio_backend_debugfs_ptr(axi_dac_reg_access), > }; > > static const struct iio_backend_ops axi_ad3552r_ops = { > .enable = axi_dac_enable, > .read_raw = axi_dac_read_raw, > .request_buffer = axi_dac_request_buffer, > .data_source_set = axi_dac_data_source_set, > .ext_sync_enable = axi_dac_ext_sync_enable, > .ext_sync_disable = axi_dac_ext_sync_disable, > .ddr_enable = axi_dac_ddr_enable, > .ddr_disable = axi_dac_ddr_disable, > .buffer_enable = axi_dac_buffer_enable, > .buffer_disable = axi_dac_buffer_disable, > .data_format_set = axi_dac_data_format_set, > .data_transfer_addr = axi_dac_data_transfer_addr, > }; > > > could this be good ? > I think you're replying to the wrong email :). But yeah, I made a comment about the above and that is something I'm also expecting. Regarding the buffer_enable/disable() stuff, please go check past discussions (I think on the RFC). I'm fairly sure I had (and Jonathan as well) some comments about directly using IIO buffer options or replicating them in the backend_ops. Likely the second option is the best one so we can take a reference to backend object directly. - Nuno Sá
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); > };
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); >> };
On Mon, 2024-09-30 at 14:25 -0500, David Lechner wrote: > 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. > Hi Jonathan and David, Yeah, I have this on my todo list and to be fair with Angelo, he already had something like you're suggesting. I kind of asked him to postpone that so we don't have mixed styles in the file for now. Then I would convert them all. My plan would be to squash the .ops into one and then have inline enable()/disable() helpers (at least for the current users in order to keep things easier to convert). As for David's comment, I see your point but one can always improve things a bit if (enable) { /* do stuff */ return; } /* do disable stuff */ return 0 I'm aware the above is always not that straight... but I do think there's always ways to rearrange things a bit to make it better. Because even with the enable()/disable() approach, if you start to have a lot of common code, likely you'll add an helper function. In some cases, one can even add the helper right away with an 'enable' argument effectively doing what is being suggested in here. It always depends on the person implementing the ops :) Anyways, I really don't have a strong feeling about this. I had in my mind to do something like this. It feels that Jonathan would already be ok with it. If it's not that awful for David, I'll eventually send the patches (unless Angelo wants to take care if it in this series). - Nuno Sá >
On 01.10.2024 10:14, Nuno Sá wrote: > On Mon, 2024-09-30 at 14:25 -0500, David Lechner wrote: > > 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. > > > > Hi Jonathan and David, > > Yeah, I have this on my todo list and to be fair with Angelo, he already had > something like you're suggesting. I kind of asked him to postpone that so we > don't have mixed styles in the file for now. Then I would convert them all. My > plan would be to squash the .ops into one and then have inline > enable()/disable() helpers (at least for the current users in order to keep > things easier to convert). > > As for David's comment, I see your point but one can always improve things a bit > > if (enable) { > /* do stuff */ > return; > } > > /* do disable stuff */ > return 0 > > I'm aware the above is always not that straight... but I do think there's always > ways to rearrange things a bit to make it better. Because even with the > enable()/disable() approach, if you start to have a lot of common code, likely > you'll add an helper function. In some cases, one can even add the helper right > away with an 'enable' argument effectively doing what is being suggested in > here. It always depends on the person implementing the ops :) > > Anyways, I really don't have a strong feeling about this. I had in my mind to do > something like this. It feels that Jonathan would already be ok with it. If it's > not that awful for David, I'll eventually send the patches (unless Angelo wants > to take care if it in this series). > I agree a single function for enable/disable may be good, reducing the calls and also the code size should benefit of some few bytes. Honestly, i would not do this in this patchset since i am a bit in difficulties to have this job accepted as is, and also cannot retest all of them properly right now. > - Nuno Sá > >
On Tue, 1 Oct 2024 10:35:17 +0200 Angelo Dureghello <adureghello@baylibre.com> wrote: > On 01.10.2024 10:14, Nuno Sá wrote: > > On Mon, 2024-09-30 at 14:25 -0500, David Lechner wrote: > > > 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. > > > > > > > Hi Jonathan and David, > > > > Yeah, I have this on my todo list and to be fair with Angelo, he already had > > something like you're suggesting. I kind of asked him to postpone that so we > > don't have mixed styles in the file for now. Then I would convert them all. My > > plan would be to squash the .ops into one and then have inline > > enable()/disable() helpers (at least for the current users in order to keep > > things easier to convert). > > > > As for David's comment, I see your point but one can always improve things a bit > > > > if (enable) { > > /* do stuff */ > > return; > > } > > > > /* do disable stuff */ > > return 0 > > > > I'm aware the above is always not that straight... but I do think there's always > > ways to rearrange things a bit to make it better. Because even with the > > enable()/disable() approach, if you start to have a lot of common code, likely > > you'll add an helper function. In some cases, one can even add the helper right > > away with an 'enable' argument effectively doing what is being suggested in > > here. It always depends on the person implementing the ops :) > > > > Anyways, I really don't have a strong feeling about this. I had in my mind to do > > something like this. It feels that Jonathan would already be ok with it. If it's > > not that awful for David, I'll eventually send the patches (unless Angelo wants > > to take care if it in this series). > > > > I agree a single function for enable/disable may be good, reducing the calls and > also the code size should benefit of some few bytes. Normally I'd be on David's side on this but I don't really want to end up with hundreds of callbacks (vs a single hundred). Thinking more about it, maybe I don't care about keeping them split. > > Honestly, i would not do this in this patchset since i am a bit in difficulties > to have this job accepted as is, and also cannot retest all of them properly > right now. That's fine given it's an open question on whether it is even a good idea and not really related to your work here. Jonathan > > > - Nuno Sá > > > >
On 25.09.2024 13:59, Nuno Sá wrote: > On Tue, 2024-09-24 at 16:11 +0200, Angelo Dureghello wrote: > > > > On 20/09/24 14:50, Nuno Sá wrote: > > > On Thu, 2024-09-19 at 11:20 +0200, Angelo Dureghello 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> > > > > --- > > > > 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) > > > > return 0; > > > > } > > > > > > > > +/** > > > > + * iio_backend_ext_sync_enable - Enable external synchronization > > > > + * @back: Backend device > > > > + * > > > > + * Enable synchronization by external signal. > > > > + * > > > > + * RETURNS: > > > > + * 0 on success, negative error number on failure. > > > > + */ > > > > +int iio_backend_ext_sync_enable(struct iio_backend *back) > > > > +{ > > > > + return iio_backend_op_call(back, ext_sync_enable); > > > > +} > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_ext_sync_enable, IIO_BACKEND); > > > > + > > > > +/** > > > > + * iio_backend_ext_sync_disable - Disable external synchronization > > > > + * @back: Backend device > > > > + * > > > > + * Disable synchronization by external signal. > > > > + * > > > > + * RETURNS: > > > > + * 0 on success, negative error number on failure. > > > > + */ > > > > +int iio_backend_ext_sync_disable(struct iio_backend *back) > > > > +{ > > > > + return iio_backend_op_call(back, ext_sync_disable); > > > > +} > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_ext_sync_disable, IIO_BACKEND); > > > > + > > > > +/** > > > > + * iio_backend_ddr_enable - Enable interface DDR (Double Data Rate) mode > > > > + * @back: Backend device > > > > + * > > > > + * Enabling DDR, data is generated by the IP at each front > > > > + * (raising and falling) of the bus clock signal. > > > > + * > > > > + * RETURNS: > > > > + * 0 on success, negative error number on failure. > > > > + */ > > > > +int iio_backend_ddr_enable(struct iio_backend *back) > > > > +{ > > > > + return iio_backend_op_call(back, ddr_enable); > > > > +} > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_ddr_enable, IIO_BACKEND); > > > > + > > > > +/** > > > > + * 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 > > > > + * 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); > > > > + > > > > +/** > > > > + * iio_backend_buffer_enable - Enable iio buffering > > > > + * @back: Backend device > > > > + * > > > > + * Enabling the buffer, buffer data is processed and sent out from the > > > > + * bus interface. > > > > + * > > > > + * RETURNS: > > > > + * 0 on success, negative error number on failure. > > > > + */ > > > > +int iio_backend_buffer_enable(struct iio_backend *back) > > > > +{ > > > > + return iio_backend_op_call(back, buffer_enable); > > > > +} > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_enable, IIO_BACKEND); > > > > + > > > > +/** > > > > + * iio_backend_buffer_disable - Disable iio buffering > > > > + * @back: Backend device > > > > + * > > > > + * Disabling the buffer, buffer data transfer on the bus interface > > > > + * is stopped. > > > > + * > > > > + * RETURNS: > > > > + * 0 on success, negative error number on failure. > > > > + */ > > > > +int iio_backend_buffer_disable(struct iio_backend *back) > > > > +{ > > > > + return iio_backend_op_call(back, buffer_disable); > > > > +} > > > > +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_disable, IIO_BACKEND); > > > > + > > > IIRC, both me and Jonathan had some comments about the above 2 calls? Aren't > > > they > > > about buffering? I think I mentioned something about using the same buffer > > > ops as > > > typical IIO devices use. > > > > i have now separated iio_backend_ops, keeping buffer enable/disable > > for axi-ad3352r case only, > > > > static const struct iio_backend_ops axi_dac_generic_ops = { > > .enable = axi_dac_enable, > > .disable = axi_dac_disable, > > .request_buffer = axi_dac_request_buffer, > > .free_buffer = axi_dac_free_buffer, > > .extend_chan_spec = axi_dac_extend_chan, > > .ext_info_set = axi_dac_ext_info_set, > > .ext_info_get = axi_dac_ext_info_get, > > .data_source_set = axi_dac_data_source_set, > > .set_sample_rate = axi_dac_set_sample_rate, > > .debugfs_reg_access = iio_backend_debugfs_ptr(axi_dac_reg_access), > > }; > > > > static const struct iio_backend_ops axi_ad3552r_ops = { > > .enable = axi_dac_enable, > > .read_raw = axi_dac_read_raw, > > .request_buffer = axi_dac_request_buffer, > > .data_source_set = axi_dac_data_source_set, > > .ext_sync_enable = axi_dac_ext_sync_enable, > > .ext_sync_disable = axi_dac_ext_sync_disable, > > .ddr_enable = axi_dac_ddr_enable, > > .ddr_disable = axi_dac_ddr_disable, > > .buffer_enable = axi_dac_buffer_enable, > > .buffer_disable = axi_dac_buffer_disable, > > .data_format_set = axi_dac_data_format_set, > > .data_transfer_addr = axi_dac_data_transfer_addr, > > }; > > > > > > could this be good ? > > > > I think you're replying to the wrong email :). But yeah, I made a comment about > the above and that is something I'm also expecting. > > Regarding the buffer_enable/disable() stuff, please go check past discussions (I > think on the RFC). I'm fairly sure I had (and Jonathan as well) some comments > about directly using IIO buffer options or replicating them in the backend_ops. > Likely the second option is the best one so we can take a reference to backend > object directly. > > - Nuno Sá > Hi Nuno, trying to fix this now as (hopefully :) ) last point for v4, looking the RFC i find: 1) Embed struct iio_buffer_setup_ops in the backend ops struct; 2) Or just define directly the ones we need now in backend ops About 1, looks like that then the frontend cannot customize the buffer ops. About 2, i already setup a specific backend structure only for axi-ad3552r. Removed the sync stuff, we have now only 5 new APIs. What i can do here is a better naming, and better description as asked from Jonathan. Mainly, this functions are starting and stopping the stream from dma to the chip. Proposals may be static int axi_dac_dma_stream_ebable(struct iio_backend *back) static int axi_dac_dma_stream_disaable(struct iio_backend *back) or static int axi_dac_data_stream_ebable(struct iio_backend *back) static int axi_dac_data_stream_disaable(struct iio_backend *back) or still more generic static int axi_dac_bus_ebable(struct iio_backend *back) static int axi_dac_bus_disaable(struct iio_backend *back) (or bus_transfer_enable)
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) return 0; } +/** + * iio_backend_ext_sync_enable - Enable external synchronization + * @back: Backend device + * + * Enable synchronization by external signal. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_ext_sync_enable(struct iio_backend *back) +{ + return iio_backend_op_call(back, ext_sync_enable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_ext_sync_enable, IIO_BACKEND); + +/** + * iio_backend_ext_sync_disable - Disable external synchronization + * @back: Backend device + * + * Disable synchronization by external signal. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_ext_sync_disable(struct iio_backend *back) +{ + return iio_backend_op_call(back, ext_sync_disable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_ext_sync_disable, IIO_BACKEND); + +/** + * iio_backend_ddr_enable - Enable interface DDR (Double Data Rate) mode + * @back: Backend device + * + * Enabling DDR, data is generated by the IP at each front + * (raising and falling) of the bus clock signal. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_ddr_enable(struct iio_backend *back) +{ + return iio_backend_op_call(back, ddr_enable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_ddr_enable, IIO_BACKEND); + +/** + * 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 + * 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); + +/** + * iio_backend_buffer_enable - Enable iio buffering + * @back: Backend device + * + * Enabling the buffer, buffer data is processed and sent out from the + * bus interface. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_buffer_enable(struct iio_backend *back) +{ + return iio_backend_op_call(back, buffer_enable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_enable, IIO_BACKEND); + +/** + * iio_backend_buffer_disable - Disable iio buffering + * @back: Backend device + * + * Disabling the buffer, buffer data transfer on the bus interface + * is stopped. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_buffer_disable(struct iio_backend *back) +{ + return iio_backend_op_call(back, buffer_disable); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_buffer_disable, IIO_BACKEND); + +/** + * iio_backend_data_transfer_addr - Set data address. + * @back: Backend device + * @address: Data register address + * + * Some devices may need to inform the backend about an address + * where to read or write the data. + * + * RETURNS: + * 0 on success, negative error number on failure. + */ +int iio_backend_data_transfer_addr(struct iio_backend *back, u32 address) +{ + return iio_backend_op_call(back, data_transfer_addr, address); +} +EXPORT_SYMBOL_NS_GPL(iio_backend_data_transfer_addr, IIO_BACKEND); + static struct iio_backend *__devm_iio_backend_fwnode_get(struct device *dev, const char *name, 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. + * @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); + 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); }; /** @@ -164,6 +180,13 @@ int iio_backend_data_sample_trigger(struct iio_backend *back, int devm_iio_backend_request_buffer(struct device *dev, struct iio_backend *back, struct iio_dev *indio_dev); +int iio_backend_ext_sync_enable(struct iio_backend *back); +int iio_backend_ext_sync_disable(struct iio_backend *back); +int iio_backend_ddr_enable(struct iio_backend *back); +int iio_backend_ddr_disable(struct iio_backend *back); +int iio_backend_buffer_enable(struct iio_backend *back); +int iio_backend_buffer_disable(struct iio_backend *back); +int iio_backend_data_transfer_addr(struct iio_backend *back, u32 address); ssize_t iio_backend_ext_info_set(struct iio_dev *indio_dev, uintptr_t private, const struct iio_chan_spec *chan, const char *buf, size_t len);