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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241026185740.4144f6c8@jic23-huawei>
Date: Sat, 26 Oct 2024 18:57:40 +0100
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 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.

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