RE: [PATCH v3 2/2] iio: frequency: adf4377: add support for ADF4377

[Sa, Nuno]

> From: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> Sent: Tuesday, November 15, 2022 12:01 PM
> To: jic23@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; linux-iio@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: Miclaus, Antoniu <Antoniu.Miclaus@xxxxxxxxxx>
> Subject: [PATCH v3 2/2] iio: frequency: adf4377: add support for ADF4377
> 
> [External]
> 
> The ADF4377 is a high performance, ultralow jitter, dual output integer-N
> phased locked loop (PLL) with integrated voltage controlled oscillator
> (VCO) ideally suited for data converter and mixed signal front end (MxFE)
> clock applications.
> 
> Datasheet: https://www.analog.com/media/en/technical-
> documentation/data-sheets/adf4377.pdf
> Signed-off-by: Antoniu Miclaus <antoniu.miclaus@xxxxxxxxxx>
> ---
> changes in v3:
>  - add register name/value for the macro definitions
>  - use `__aligned(IIO_DMA_MINALIGN)`
>  - add extra spacing for `reg_sequence` structure
>  - check frequency value before reg writes in `adf4377_set_freq` function
>  - remove ternary operators inside `adf4377_write` and `adf4377_read`
>  - remove comma after null terminator
>  drivers/iio/frequency/Kconfig   |  10 +
>  drivers/iio/frequency/Makefile  |   1 +
>  drivers/iio/frequency/adf4377.c | 994
> ++++++++++++++++++++++++++++++++
>  3 files changed, 1005 insertions(+)
>  create mode 100644 drivers/iio/frequency/adf4377.c
> 
> diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
> index f3702f36436c..9e85dfa58508 100644
> --- a/drivers/iio/frequency/Kconfig
> +++ b/drivers/iio/frequency/Kconfig
> @@ -50,6 +50,16 @@ config ADF4371
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called adf4371.
> 
> +config ADF4377
> +	tristate "Analog Devices ADF4377 Microwave Wideband Synthesizer"
> +	depends on SPI && COMMON_CLK
> +	help
> +	  Say yes here to build support for Analog Devices ADF4377
> Microwave
> +	  Wideband Synthesizer.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called adf4377.
> +
>  config ADMV1013
>  	tristate "Analog Devices ADMV1013 Microwave Upconverter"
>  	depends on SPI && COMMON_CLK
> diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
> index 48add732f1d3..b616c29b4a08 100644
> --- a/drivers/iio/frequency/Makefile
> +++ b/drivers/iio/frequency/Makefile
> @@ -7,6 +7,7 @@
>  obj-$(CONFIG_AD9523) += ad9523.o
>  obj-$(CONFIG_ADF4350) += adf4350.o
>  obj-$(CONFIG_ADF4371) += adf4371.o
> +obj-$(CONFIG_ADF4377) += adf4377.o
>  obj-$(CONFIG_ADMV1013) += admv1013.o
>  obj-$(CONFIG_ADMV1014) += admv1014.o
>  obj-$(CONFIG_ADMV4420) += admv4420.o
> diff --git a/drivers/iio/frequency/adf4377.c
> b/drivers/iio/frequency/adf4377.c
> new file mode 100644
> index 000000000000..26abecbd51e0
> --- /dev/null
> +++ b/drivers/iio/frequency/adf4377.c
> @@ -0,0 +1,994 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ADF4377 driver
> + *
> + * Copyright 2022 Analog Devices Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/clk.h>
> +#include <linux/clkdev.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/notifier.h>
> +#include <linux/property.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/regmap.h>
> +#include <linux/units.h>
> +
> +#include <asm/unaligned.h>
> +

...

> +
> +/* Specifications */
> +#define ADF4377_SPI_READ_CMD			BIT(7)
> +#define ADF4377_MAX_VCO_FREQ			(12800ULL *
> HZ_PER_MHZ)
> +#define ADF4377_MIN_VCO_FREQ			(6400ULL *
> HZ_PER_MHZ)
> +#define ADF4377_MAX_REFIN_FREQ			(1000 *
> HZ_PER_MHZ)
> +#define ADF4377_MIN_REFIN_FREQ			(10 * HZ_PER_MHZ)
> +#define ADF4377_MAX_FREQ_PFD			(500 * HZ_PER_MHZ)
> +#define ADF4377_MIN_FREQ_PFD			(3 * HZ_PER_MHZ)
> +#define ADF4377_MAX_CLKPN_FREQ
> 	ADF4377_MAX_VCO_FREQ

Well, I think we are more than fine in using s64 :). Anyways, as the write
side is still a bit weird, let's stay with extended attr. 

...

> +static int adf4377_get_freq(struct adf4377_state *st, u64 *freq)
> +{
> +	unsigned int ref_div_factor, n_int;
> +	u64 clkin_freq;
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	ret = regmap_read(st->regmap, 0x12, &ref_div_factor);
> +	if (ret)
> +		goto exit;
> +
> +	ret = regmap_bulk_read(st->regmap, 0x10, st->buf, sizeof(st->buf));
> +	if (ret)
> +		goto exit;
> +
> +	clkin_freq = clk_get_rate(st->clkin);
> +	ref_div_factor = FIELD_GET(ADF4377_0012_R_DIV_MSK,
> ref_div_factor);
> +	n_int = FIELD_GET(ADF4377_0010_N_INT_LSB_MSK |
> ADF4377_0011_N_INT_MSB_MSK,
> +			  get_unaligned_le16(&st->buf));
> +
> +	*freq = div_u64(clkin_freq, ref_div_factor) * n_int;
> +exit:
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int adf4377_set_freq(struct adf4377_state *st, u64 freq)
> +{
> +	unsigned int read_val;
> +	u64 f_vco;
> +	int ret;

Could initialize ret to -EINVAL and save one line of code :)...

Anyways...

Reviewed-by: Nuno Sá <nuno.sa@xxxxxxxxxx>