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@xxxxxxxxxxxx> 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@xxxxxxxxxxxx> wrote:
> > > >
> > > > > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
> > > > >
> > > > > 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@xxxxxxxxxxxx>
> > > > > ---
> > > > > 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á
> >
