Re: [PATCH v8 6/8] iio: dac: ad3552r: extract common code (no changes in behavior intended)

[Jonathan Cameron <jic23@kernel.org>]

On Fri, 25 Oct 2024 11:49:39 +0200
Angelo Dureghello <adureghello@baylibre.com> wrote:

> From: Angelo Dureghello <adureghello@baylibre.com>
> 
> Extracting common code, to share common code to be used later
> by the AXI driver version (ad3552r-axi.c).
> 
> Signed-off-by: Angelo Dureghello <adureghello@baylibre.com>

Hi Angelo,

A few trivial things but one bigger one that actually only becomes a problem
in the next patch so I'll comment on that.

> +
> +MODULE_DESCRIPTION("ad3552r common functions");
Ah. This rang alarm bells.  I'll comment in next patch but you can't link
the same file twice.


> +MODULE_LICENSE("GPL");


> @@ -1072,3 +727,4 @@ module_spi_driver(ad3552r_driver);
>  MODULE_AUTHOR("Mihail Chindris <mihail.chindris@analog.com>");
>  MODULE_DESCRIPTION("Analog Device AD3552R DAC");
>  MODULE_LICENSE("GPL v2");
> +MODULE_IMPORT_NS(IIO_AD3552R);
> diff --git a/drivers/iio/dac/ad3552r.h b/drivers/iio/dac/ad3552r.h
> new file mode 100644
> index 000000000000..22bd9ad27c65
> --- /dev/null
> +++ b/drivers/iio/dac/ad3552r.h
> @@ -0,0 +1,226 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * AD3552R Digital <-> Analog converters common header
> + *
> + * Copyright 2021-2024 Analog Devices Inc.
> + * Author: Angelo Dureghello <adureghello@baylibre.com>
> + */
> +
> +#ifndef __DRIVERS_IIO_DAC_AD3552R_H__
> +#define __DRIVERS_IIO_DAC_AD3552R_H__
> +
> +/* Register addresses */
> +/* Primary address space */

> +#define   AD3552R_MASK_MULTI_IO_MODE			GENMASK(7, 6)
> +#define   AD3552R_MASK_STREAM_LENGTH_KEEP_VALUE		BIT(2)
> +#define AD3552R_REG_ADDR_INTERFACE_CONFIG_C		0x10
> +#define   AD3552R_MASK_CRC_ENABLE			(GENMASK(7, 6) |\
> +							 GENMASK(1, 0))
If for whatever reason we go around again, (otherwise I might tweak anyway)
#define   AD3552R_MASK_CRC_ENABLE	\
		(GENMASK(7, 6) | GENMASK(1, 0))


> +#define   AD3552R_MASK_CH_OUTPUT_RANGE			GENMASK(7, 0)
> +#define   AD3552R_MASK_CH_OUTPUT_RANGE_SEL(ch)		((ch) ? \
> +							 GENMASK(7, 4) : \
> +							 GENMASK(3, 0))
I may tweak this whilst applying to be something like

#define   AD3552R_MASK_CH_OUTPUT_RANGE_SEL(ch)	\
		((ch) ? GENMASK(7, 4) : GENMASK(3, 0))


> +/* Useful defines */
Made me laugh.  I hope we don't ever have a comment that says "Useless defines" :)

> +#define AD3552R_MAX_CH					2
> +#define AD3552R_MASK_CH(ch)				BIT(ch)
> +#define AD3552R_MASK_ALL_CH				GENMASK(1, 0)
> +#define AD3552R_MAX_REG_SIZE				3
> +#define AD3552R_READ_BIT				BIT(7)
> +#define AD3552R_ADDR_MASK				GENMASK(6, 0)
> +#define AD3552R_MASK_DAC_12B				GENMASK(15, 4)
> +#define AD3552R_DEFAULT_CONFIG_B_VALUE			0x8
> +#define AD3552R_SCRATCH_PAD_TEST_VAL1			0x34
> +#define AD3552R_SCRATCH_PAD_TEST_VAL2			0xB2
> +#define AD3552R_GAIN_SCALE				1000
> +#define AD3552R_LDAC_PULSE_US				100