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

["Nuno Sá" <noname.nuno@gmail.com>]

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á