Re: [PATCH v5 09/10] iio: dac: ad3552r: add high-speed platform driver

[Jonathan Cameron]

On Tue, 08 Oct 2024 17:43:41 +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>
> ---
>  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__ */
>