Re: [PATCH RFC 7/8] iio: dac: ad3552r: add axi platform driver

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 3 Sep 2024 10:17:35 +0200
Angelo Dureghello <adureghello@baylibre.com> wrote:

> Hi Jonathan,
> 
> On 31/08/24 2:13 PM, Jonathan Cameron wrote:
> > On Thu, 29 Aug 2024 14:32:05 +0200
> > Angelo Dureghello <adureghello@baylibre.com> wrote:
> >  
> >> From: Angelo Dureghello <adureghello@baylibre.com>
> >>
> >> Add support for ad3552r AXI DAC IP version.
> >>
> >> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>  
> > Hi Angelo
> >
> > To me this feels like the interface is much closer to SPI + SPI offload
> > than to a conventional IIO backend on the basis it carries the configuration
> > path as well.
> >
> > Can we see if it can be fitted into that model?  You will need to define
> > a new bus type etc for it but should be fairly simple given constrained
> > setup (at least today!)
> >
> > That will resolve a bunch of questions around the binding as well.  
> 
> thanks for all the feedbacks.
> 
> I see, spi offload may have more sense but as of now looks like moving to
> AXI SPI engine instead of AXI DAC would require quite a lot of work from the
> ADI HDL guys and also then, for me some work reworking all this stuff.
>  From an initial discussion with Nuno and David, we was oriented to use the
> iio backend for the current HDL, so at least for this chip at this stage 
> would
> be good, if possible, to stay this way.

Superficially, even with the existing IP it feels to me like it's just
a qspi controller + an offload that happens not to need much programming.

You'd pass that offload the spi message structure etc and it would 'notice'
that it corresponds to what it has in hardware and then use that.

For register reads it looks like a simple (Q)SPI bus controller anyway.

So I'm not sure any real changes are needed in the IP to map it
in a more standard way as as a device on a bus.

Note though that key here may be how we do the dt-binding, rather than
what the code does (we can change the internals of the driver later
if we like).

If you built a binding that looked like an spi bus + offload and
could we bind a backend etc as you do currently? 

Might require a bit of juggling to make it work.


> 
> 
> > Jonathan
> >  
> >> diff --git a/drivers/iio/dac/ad3552r-axi.c b/drivers/iio/dac/ad3552r-axi.c
> >> new file mode 100644
> >> index 000000000000..98e5da08c973
> >> --- /dev/null
> >> +++ b/drivers/iio/dac/ad3552r-axi.c
> >> @@ -0,0 +1,572 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Analog Devices AD3552R
> >> + * Digital to Analog converter driver, AXI DAC backend version
> >> + *
> >> + * Copyright 2024 Analog Devices Inc.
> >> + */
> >> +
> >> +#include <linux/bitfield.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/gpio/consumer.h>
> >> +#include <linux/iio/buffer.h>
> >> +#include <linux/iio/backend.h>
> >> +#include <linux/of.h>  
> > Why?  Probably want mod_devicetable.h  
> 
> 
> with mod_devicetable.h in place of of.h i get
> 
> drivers/iio/dac/ad3552r-axi.c:272:9: error: cleanup argument not a function
>    struct fwnode_handle *child __free(fwnode_handle) = NULL;
>           ^~~~~~~~~~~~~
That's not in of.h either
add linux/property.h as well.

> 

...

> >> +static int ad3552r_axi_setup(struct ad3552r_axi_state *st)
> >> +{

> >  
> >> +	u32 val, range;
> >> +	int err;
> >> +
> >> +	err = ad3552r_axi_reset(st);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	err = iio_backend_ddr_disable(st->back);
> >> +	if (err)
> >> +		return err;
> >> +
> >> +	val = AD3552R_SCRATCH_PAD_TEST_VAL1;
> >> +	err = iio_backend_bus_reg_write(st->back, AD3552R_REG_ADDR_SCRATCH_PAD,
> >> +					&val, 1);  
> > as per earlier review, I'd pass an unsigned int instead of a void *
> > Then you can avoid the dance with a local variable.  
> void * was chosen thinking to future busses, please let me know if
> it can stay this way.
I'd not bother future proofing that much.  If you think > 32 bit is
likely use a u64.

> >> +	if (err)
> >> +		return err;