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

[Angelo Dureghello]

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@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>
> > > > ---
> > > >   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)

-- 

Regards,
  Angelo