Re: [PATCH v8 7/8] iio: dac: ad3552r: add high-speed platform driver

[Jonathan Cameron <jic23@kernel.org>]

On Mon, 28 Oct 2024 10:14:25 +0100
Angelo Dureghello <adureghello@baylibre.com> wrote:

> Hi Jonathan,
> 
> On 26.10.2024 18:57, Jonathan Cameron wrote:
> > On Fri, 25 Oct 2024 11:49:40 +0200
> > Angelo Dureghello <adureghello@baylibre.com> wrote:
> >   
> > > From: Angelo Dureghello <adureghello@baylibre.com>
> > > 
> > > Add High Speed ad3552r platform driver.
> > > 
> > > The ad3552r DAC is controlled by a custom (fpga-based) DAC IP
> > > through the current AXI backend, or similar alternative IIO backend.
> > > 
> > > Compared to the existing driver (ad3552r.c), that is a simple SPI
> > > driver, this driver is coupled with a DAC IIO backend that finally
> > > controls the ad3552r by a fpga-based "QSPI+DDR" interface, to reach
> > > maximum transfer rate of 33MUPS using dma stream capabilities.
> > > 
> > > All commands involving QSPI bus read/write are delegated to the backend
> > > through the provided APIs for bus read/write.
> > > 
> > > Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>
> > > ---  
> > Hi Angelo,
> > 
> > I'd missed a build issue in previous reviews. :(
> >   
> > >  drivers/iio/dac/Kconfig      |  14 ++
> > >  drivers/iio/dac/Makefile     |   1 +
> > >  drivers/iio/dac/ad3552r-hs.c | 530 +++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/iio/dac/ad3552r-hs.h |  19 ++
> > >  drivers/iio/dac/ad3552r.h    |   4 +
> > >  5 files changed, 568 insertions(+)
> > > 
> > > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> > > index 26f9de55b79f..f76eaba140d8 100644
> > > --- a/drivers/iio/dac/Kconfig
> > > +++ b/drivers/iio/dac/Kconfig
> > > @@ -6,6 +6,20 @@
> > >  
> > >  menu "Digital to analog converters"
> > >  
> > > +config AD3552R_HS
> > > +	tristate "Analog Devices AD3552R DAC High Speed driver"
> > > +	select ADI_AXI_DAC
> > > +	help
> > > +	  Say yes here to build support for Analog Devices AD3552R
> > > +	  Digital to Analog Converter High Speed driver.
> > > +
> > > +          The driver requires the assistance of an IP core to operate,
> > > +          since data is streamed into target device via DMA, sent over a
> > > +	  QSPI + DDR (Double Data Rate) bus.  
> > 
> > Tabs and space mix that needs fixing.
> >   
> > > +
> > > +	  To compile this driver as a module, choose M here: the
> > > +	  module will be called ad3552r-hs.
> > > +
> > >  config AD3552R
> > >  	tristate "Analog Devices AD3552R DAC driver"
> > >  	depends on SPI_MASTER
> > > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> > > index c92de0366238..d92e08ca93ca 100644
> > > --- a/drivers/iio/dac/Makefile
> > > +++ b/drivers/iio/dac/Makefile
> > > @@ -4,6 +4,7 @@
> > >  #
> > >  
> > >  # When adding new entries keep the list in alphabetical order
> > > +obj-$(CONFIG_AD3552R_HS) += ad3552r-hs.o ad3552r-common.o
> > >  obj-$(CONFIG_AD3552R) += ad3552r.o ad3552r-common.o  
> > 
> > This causes all sorts of issues. The same code should not be linked into two
> > separate drivers.  Try building one as a module and one built in.
> >   
> right now, seems i cannot catch any issue, nor building or in runtime:
> 
> ad3552r     [M]
> ad3552r-hs  [*]
> (ad3552r-common stays built in), ad3552r visible in lsmod, ad3552r-hs works
> 
> ad3552r     [*]
> ad3552r-hs  [M]
> (ad3552r-common stays built in), ad3552r-hs visible in lsmod, ad3552r-hs works
> 
> ad3552r     [M]
> ad3552r-hs  [M]
> (ad3552r-common.ko), ad3552r, ad3552r-hs and ad3552r-common are visible in lsmod,
> ad3552r-hs works, probe and removal, and also link/unlink tested).
> 
> Please let me know, i can proceed modifying as you require, if it's the case.
Hi Angelo,

I can't remember exactly what triggers this; maybe it's no longer a problem.
However, if nothing else it is a waste to end up with two copies in the
drivers. Hence switch to a common library module still a good idea.

Also this is missing includes for bitfield.h in ad3352r.c and ad3552r-common.c
so doesn't build for me at all.

Jonathan

> 
> 
> > The trick is a hidden symbol in Kconfig and an extra line in here
> > obj-$(CONFIG_AD3352R_LIB) += ad3552-common.o
> > 
> > and 
> > //note no text as we don't want this to be user selectable
> > 
> > config AD3352R_LIB
> > 	tristate
> > 
> > config AD3552R_HS
> > 	tristate "Analog Devices AD3552R DAC High Speed driver"
> > 	select ADI_AXI_DAC
> > 	select AD3352R_LIB
> > 	help
> > 	  Say yes here to build support for Analog Devices AD3552R
> > 	  Digital to Analog Converter High Speed driver.
> > 
> > 	  The driver requires the assistance of an IP core to operate,
> > 	  since data is streamed into target device via DMA, sent over a
> > 	  QSPI + DDR (Double Data Rate) bus.
> > 
> > 	  To compile this driver as a module, choose M here: the
> > 	  module will be called ad3552r-hs.
> > 
> > 
> > config AD3552R
> >  	tristate "Analog Devices AD3552R DAC driver"
> >  	depends on SPI_MASTER
> > 	select AD3352R_LIB
> > 	help
> > 	  ...
> > 
> > The pressure/mpl115 is done like this.
> > 
> >   
> > >  obj-$(CONFIG_AD5360) += ad5360.o
> > >  obj-$(CONFIG_AD5380) += ad5380.o  
> > 
> > Anyhow, to me the code looks ready to go subject to this.
> > 
> > If nothing else comes up I'm almost confident enough of the fix to just
> > do it (and the few trivial things in previous review), but probably quicker
> > and less prone to error if you have time to spin a v9, perhaps after letting others
> > have a day or two to review v8 next week.
> > 
> > rc5 is tomorrow, so we have a little time left this cycle.
> > 
> > Jonathan  
> 
> Regards,
>   angelo