linux-kernel - Re: [PATCH v5 09/10] 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 for Android: free password hash cracker in your pocket

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

Message-ID: <20241012150709.1278066c@jic23-huawei>
Date: Sat, 12 Oct 2024 15:07:09 +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 Sa <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>,
 Jonathan Cameron <Jonathan.Cameron@...wei.com>, linux-iio@...r.kernel.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 dletchner@...libre.com, Mark Brown <broonie@...nel.org>
Subject: Re: [PATCH v5 09/10] iio: dac: ad3552r: add high-speed platform
 driver

On Tue, 08 Oct 2024 17:43:41 +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>
> ---
>  drivers/iio/dac/Kconfig                  |  14 +
>  drivers/iio/dac/Makefile                 |   1 +
>  drivers/iio/dac/ad3552r-hs.c             | 526 +++++++++++++++++++++++++++++++
>  drivers/iio/dac/ad3552r.h                |   7 +
>  include/linux/platform_data/ad3552r-hs.h |  18 ++
I'd missed this before.  No obvious reason to put the include in a 'global' location
when for now at least only includers re in drivers/iio/dac/

So please move it there.

Some stuff include/linux/platform_data/ is ancient things dating back to board file
days.

Other than that and things bit Nuno raised, this series looks good to me.
Please drop the first 2 patches from v6 as I've applied those now.

Jonathan


> diff --git a/include/linux/platform_data/ad3552r-hs.h b/include/linux/platform_data/ad3552r-hs.h
> new file mode 100644
> index 000000000000..4e3213a0c73b
> --- /dev/null
> +++ b/include/linux/platform_data/ad3552r-hs.h

This isn't what we'd think of as 'platform_data' normally and it is only used
in drivers/iio/dac/ so I would simply move the header to drivers/iio/dac/ad3552r-hs.h

> @@ -0,0 +1,18 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * Copyright (c) 2010-2024 Analog Devices Inc.

Generous on the years.  This one should probably just be new copyright given
the code is effectively new.

> + * Copyright (c) 2024 Baylibre, SAS
> + */
> +#ifndef __LINUX_PLATFORM_DATA_AD3552R_HS_H__
> +#define __LINUX_PLATFORM_DATA_AD3552R_HS_H__
> +
> +#include <linux/iio/backend.h>
For this use a forwards reference

struct iio_backend;

Nice to avoid nests of includes where we can easily do so.

> +
> +struct ad3552r_hs_platform_data {
> +	int (*bus_reg_read)(struct iio_backend *back, u32 reg, u32 *val,
> +			    size_t data_size);
> +	int (*bus_reg_write)(struct iio_backend *back, u32 reg, u32 val,
> +			     size_t data_size);
> +};
> +
> +#endif /* __LINUX_PLATFORM_DATA_AD3552R_HS_H__ */
>