Re: [PATCH RFC 2/8] iio: backend: extend features

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 05 Sep 2024 12:28:51 +0200
Nuno Sá <noname.nuno@gmail.com> wrote:

> On Wed, 2024-09-04 at 14:01 +0200, Angelo Dureghello wrote:
> > Hi Jonathan,
> > 
> > On 03/09/24 9:11 PM, Jonathan Cameron wrote:  
> > > On Mon, 2 Sep 2024 16:03:22 +0200
> > > Angelo Dureghello <adureghello@baylibre.com> wrote:
> > >   
> > > > Hi Jonathan,
> > > > 
> > > > thanks for the feedbacks,
> > > > 
> > > > On 31/08/24 1:23 PM, Jonathan Cameron wrote:  
> > > > > On Thu, 29 Aug 2024 14:32:00 +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.
> > > > > > 
> > > > > > A bus type property has been added to the devicetree to
> > > > > > inform the backend about the type of bus (interface) in use
> > > > > > bu the IP.
> > > > > > 
> > > > > > 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.
> > > > > > iio_backend_bus_reg_read
> > > > > > 	generic bus read, bus-type dependent
> > > > > > iio_backend_bus_read_write
> > > > > > 	generic bus write, bus-type dependent
> > > > > > 
> > > > > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > > > > > ---
> > > > > >    drivers/iio/industrialio-backend.c | 151
> > > > > > +++++++++++++++++++++++++++++++++++++
> > > > > >    include/linux/iio/backend.h        |  24 ++++++
> > > > > >    2 files changed, 175 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/iio/industrialio-backend.c
> > > > > > b/drivers/iio/industrialio-backend.c
> > > > > > index a52a6b61c8b5..1f60c8626be7 100644
> > > > > > --- a/drivers/iio/industrialio-backend.c
> > > > > > +++ b/drivers/iio/industrialio-backend.c
> > > > > > @@ -718,6 +718,157 @@ static int __devm_iio_backend_get(struct device
> > > > > > *dev, struct iio_backend *back)
> > > > > >    	return 0;
> > > > > >    }  
> > > > >     
> > > > > > +
> > > > > > +/**
> > > > > > + * iio_backend_buffer_enable - Enable data buffering  
> > > > > Data buffering is a very vague term.  Perhaps some more detail on what
> > > > > this means?  
> > > > for this DAC IP, it is the dma buffer where i write the samples,
> > > > for other non-dac frontends may be something different, so i kept it
> > > > generic. Not sure what a proper name may be, maybe
> > > > 
> > > > "Enable optional data buffer" ?  
> > > How do you 'enable' a buffer?  Enable writing into it maybe?  
> > 
> > for the current case, this is done using the custom register
> > of the AXI IP, enabling a "stream".
> > 
> > return regmap_set_bits(st->regmap, AXI_DAC_REG_CUSTOM_CTRL,
> >                     AXI_DAC_STREAM_ENABLE);
> > 
> > Functionally, looks like dma data is processed (sent over qspi)
> > when the stream is enabled.
> > 
> > Maybe a name as "stream_enable" would me more appropriate ?
> > "Stream" seems less generic btw.
> >   

Ok. Maybe "enable buffer filling" or something like that?

> 
> Yes, stream enable is very specific for this usecase. This is basically
> connected to typical IIO buffering. So maybe we could either:
> 
> 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.
Structurally whatever makes sense - I was just quibbling over the
documentation ;)

>  
> > > >   
> > > > > > + * @back: Backend device
> > > > > > + *
> > > > > > + * 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_bus_reg_read - Read from the interface bus
> > > > > > + * @back: Backend device
> > > > > > + * @reg: Register valule
> > > > > > + * @val: Pointer to register value
> > > > > > + * @size: Size, in bytes
> > > > > > + *
> > > > > > + * A backend may operate on a specific interface with a related bus.
> > > > > > + * Read from the interface bus.  
> > > > > So this is effectively routing control plane data through the offloaded
> > > > > bus?  That sounds a lot more like a conventional bus than IIO backend.
> > > > > Perhaps it should be presented as that with the IIO device attached
> > > > > to that bus? I don't fully understand what is wired up here.
> > > > >     
> > > > Mainly, an IP may include a bus as 16bit parallel, or LVDS, or similar
> > > > to QSPI as in my case (ad3552r).  
> > > ok.
> > > 
> > > If this is a bus used for both control and dataplane, then we should really
> > > be presenting it as a bus (+ offload) similar to do for spi + offload.
> > >   
> 
> Yes, indeed. In this case we also use the axi-dac core for controlling the
> frontend device (accessing it's register) which is fairly weird. But not sure
> how we can do it differently. For the spi_engine that is really a spi controller
> with the extra offloading capability. For this one, it's now "acting" as a spi
> controller but in the future it may also "act" as a parallel controller (the
> axi-adc already is in works for that with the ad7606 series).
> 
> I was also very skeptical when I first saw these new functions but I'm not
> really sure how to do it differently. I mean, it also does not make much sense
> to have an additional bus driver as the register maps are the same. Not sure if
> turning it in a MFD device, helps...

Hmm. A given adi-axi-adc interface is going to be one of (or something else)
1) SPI(ish) controller + offloads like this one.
2) Parallel bus - data only
3) Parallel bus with control.

Maybe we argue these are tightly coupled enough that we don't care but it
feels like a direction that might bite us in the long run, particularly
if we end up dt bindings that are hard to work with if we change
how this fits together - imagine an SPI engine with a mode that does work
for this + an SPI offload engine that works with that.
Then the binding here will be hard to deal with.


> 
> FWIW, I still don't fully understand why can't we have this supported by the
> spi_engine core. My guess is that we need features from the axi-dac (for the
> dataplane) so we are incorporating the controlplane on it instead of going
> spi_engine + axi-dac.
> 
> Also want to leave a quick note about LVDS (that was mentioned). That interface
> is typically only used for data so I'm not seeing any special handling like this
> for that interface.
Makes sense.  I'm a bit surprised that the parallel bus being used for control
is on the list given that is also a bit messy to do (need some signalling that
direction is changing or a lot of wires).

Jonathan

> 
> - Nuno Sá
> > >