Message-ID: <f2568dd151efc2da76659fea4300fa7b3610d1e1.camel@gmail.com>
Date: Thu, 05 Sep 2024 12:28:51 +0200
From: Nuno Sá <noname.nuno@...il.com>
To: Angelo Dureghello <adureghello@...libre.com>, Jonathan Cameron
	 <jic23@...nel.org>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich
 <Michael.Hennerich@...log.com>, Nuno Sá
 <nuno.sa@...log.com>,  Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,  Olivier Moysan
 <olivier.moysan@...s.st.com>, linux-iio@...r.kernel.org,
 devicetree@...r.kernel.org,  linux-kernel@...r.kernel.org,
 dlechner@...libre.com
Subject: Re: [PATCH RFC 2/8] iio: backend: extend features

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@...libre.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@...libre.com> wrote:
> > > >   
> > > > > From: Angelo Dureghello <adureghello@...libre.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@...libre.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.
> 

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.
 
> > > 
> > > > > + * @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...

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.

- Nuno Sá
> > 

Powered by blists - more mailing lists