On Fri, 25 Oct 2024 11:49:39 +0200
Angelo Dureghello <adureghello@xxxxxxxxxxxx> wrote:
> From: Angelo Dureghello <adureghello@xxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxx>
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@xxxxxxxxxx>");
> 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@xxxxxxxxxxxx>
> + */
> +
> +#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
