Re: [PATCH v4 06/11] iio: backend: extend features

[Jonathan Cameron]

On Fri, 4 Oct 2024 15:45:21 +0200
Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote:

> Hi Nuno,
> 
> On 04.10.2024 14:54, Nuno Sá wrote:
> > On Thu, 2024-10-03 at 19:29 +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_ddr_enable
> > > 	enable ddr bus transfer
> > > iio_backend_ddr_disable
> > > 	disable ddr bus transfer
> > > 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 | 79 ++++++++++++++++++++++++++++++++++++++
> > >  include/linux/iio/backend.h        | 17 ++++++++
> > >  2 files changed, 96 insertions(+)
> > > 
> > > diff --git a/drivers/iio/industrialio-backend.c b/drivers/iio/industrialio-
> > > backend.c
> > > index 20b3b5212da7..d5e0a4da761e 100644
> > > --- a/drivers/iio/industrialio-backend.c
> > > +++ b/drivers/iio/industrialio-backend.c
> > > @@ -718,6 +718,85 @@ static int __devm_iio_backend_get(struct device *dev, struct
> > > iio_backend *back)
> > >  	return 0;
> > >  }
> > >  
> > > +/**
> > > + * iio_backend_ddr_enable - Enable interface DDR (Double Data Rate) mode
> > > + * @back: Backend device
> > > + *
> > > + * Enable 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
> > > + *
> > > + * Disable DDR, setting into SDR mode (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_dma_stream_enable - Enable iio buffering
> > > + * @back: Backend device
> > > + *
> > > + * Enabling sending the dma data stream over the bus.
> > > + * bus interface.
> > > + *
> > > + * RETURNS:
> > > + * 0 on success, negative error number on failure.
> > > + */
> > > +int iio_backend_dma_stream_enable(struct iio_backend *back)
> > > +{
> > > +	return iio_backend_op_call(back, dma_stream_enable);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(iio_backend_dma_stream_enable, IIO_BACKEND);
> > > +
> > > +/**
> > > + * iio_backend_dma_stream_disable - Disable iio buffering
> > > + * @back: Backend device
> > > + *
> > > + * Disable sending the dma data stream over the bus.
> > > + *
> > > + * RETURNS:
> > > + * 0 on success, negative error number on failure.
> > > + */
> > > +int iio_backend_dma_stream_disable(struct iio_backend *back)
> > > +{
> > > +	return iio_backend_op_call(back, dma_stream_disable);
> > > +}
> > > +EXPORT_SYMBOL_NS_GPL(iio_backend_dma_stream_disable, IIO_BACKEND);
> > > +  
> > 
> > I'm not sure if this is what Jonathan was suggesting... Ate least I don't really
> > agree with it. I mean, yes, this is about buffering and to start receiving (or
> > sending) a stream of data. But AFAICT, it might have nothing to do with DMA. Same as
> > .request_buffer() - It's pretty much always a DMA one but we should not take that for
> > granted.

Agreed. The stream bit works, the DMA is more tenuous.  Maybe *data_stream_enable()
is generic enough.

> > 
> > So going back to the RFC [1], you can see I was suggesting something like struct
> > iio_buffer_setup_ops. Maybe just add the ones we use for now? So that would
> > be.buffer_postenable() and .buffer_predisable(). Like this, it should be obvious the
> > intent of the ops.
> >   
> ok, thanks,
> 
> so something as :
> 
> struct iio_backend_setup_ops {
> 	int (*buffer_postenable)(struct iio_backend *back);
> 	int (*buffer_predisable)(struct iio_backend *back);

Hmm. Maybe.  My issue with the original naming was the lack of clarify of
what it actually meant.  I'm not sure this helps though in some cases we
do put similar calls in the postenable callback (ones that start the
data flow) so at least it's consistent with that.

Jonathan
> };
> 
> struct iio_backend_ops {
> 	struct iio_backend_setup_ops setup_ops;
> 
> ?
> 
> > - Nuno Sá
> > 
> >   
>