[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241028200057.369dcd58@jic23-huawei>
Date: Mon, 28 Oct 2024 20:00:57 +0000
From: Jonathan Cameron <jic23@...nel.org>
To: Angelo Dureghello <adureghello@...libre.com>
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, Mark Brown
<broonie@...nel.org>, dlechner@...libre.com
Subject: Re: [PATCH v8 7/8] iio: dac: ad3552r: add high-speed platform
driver
On Mon, 28 Oct 2024 10:14:25 +0100
Angelo Dureghello <adureghello@...libre.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@...libre.com> wrote:
> >
> > > From: Angelo Dureghello <adureghello@...libre.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@...libre.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
Powered by blists - more mailing lists