On Mon, 28 Oct 2024 10:14:25 +0100 Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote: > Hi Jonathan, > > On 26.10.2024 18:57, Jonathan Cameron wrote: > > On Fri, 25 Oct 2024 11:49:40 +0200 > > Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote: > > > > > From: Angelo Dureghello <adureghello@xxxxxxxxxxxx> > > > > > > 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@xxxxxxxxxxxx> > > > --- > > 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
