diff mbox series

[v3,1/3] iio: frequency: adf4360: Add support for ADF4360 PLLs

Message ID 20200128111302.24359-1-alexandru.ardelean@analog.com (mailing list archive)
State New, archived
Headers show
Series [v3,1/3] iio: frequency: adf4360: Add support for ADF4360 PLLs | expand

Commit Message

Alexandru Ardelean Jan. 28, 2020, 11:13 a.m. UTC
From: Edward Kigwana <ekigwana@gmail.com>

The ADF4360-N (N is 0..9) are a family of integrated integer-N synthesizer
and voltage controlled oscillator (VCO).

Control of all the on-chip registers is through a simple 3-wire SPI
interface. The device operates with a power supply ranging from 3.0 V to
3.6 V and can be powered down when not in use.

The initial draft-work was done by Lars-Peter Clausen <lars@metafoo.de>
The current/latest/complete version was implemented by
Edward Kigwana <ekigwana@gmail.com>.

The ADF4360-9 is also present on the Analog Devices 'Advanced Active
Learning Module 2000' (ADALM-2000).

Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-0.pdf
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-1.pdf
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-2.pdf
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-3.pdf
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-4.pdf
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-5.pdf
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-6.pdf
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-7.pdf
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-8.pdf
Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-9.pdf
Link: https://www.analog.com/en/design-center/evaluation-hardware-and-software/evaluation-boards-kits/ADALM2000.html

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Edward Kigwana <ekigwana@gmail.com>
Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---

Changelog v2 -> v3:
* dropped patch about adf4371.yaml; added by accident since it was used to
  compare against
* addressed Rob's review comments for DT schema

Changelog v1 -> v2:
* validate dt-bindings file with
  make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/iio/frequency/adi,adf4360.yaml

 drivers/iio/frequency/Kconfig   |   11 +
 drivers/iio/frequency/Makefile  |    1 +
 drivers/iio/frequency/adf4360.c | 1263 +++++++++++++++++++++++++++++++
 3 files changed, 1275 insertions(+)
 create mode 100644 drivers/iio/frequency/adf4360.c

Comments

Jonathan Cameron Feb. 2, 2020, 2:45 p.m. UTC | #1
On Tue, 28 Jan 2020 13:13:00 +0200
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> From: Edward Kigwana <ekigwana@gmail.com>
> 
> The ADF4360-N (N is 0..9) are a family of integrated integer-N synthesizer
> and voltage controlled oscillator (VCO).
> 
> Control of all the on-chip registers is through a simple 3-wire SPI
> interface. The device operates with a power supply ranging from 3.0 V to
> 3.6 V and can be powered down when not in use.
> 
> The initial draft-work was done by Lars-Peter Clausen <lars@metafoo.de>
> The current/latest/complete version was implemented by
> Edward Kigwana <ekigwana@gmail.com>.
> 
> The ADF4360-9 is also present on the Analog Devices 'Advanced Active
> Learning Module 2000' (ADALM-2000).
> 
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-0.pdf
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-1.pdf
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-2.pdf
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-3.pdf
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-4.pdf
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-5.pdf
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-6.pdf
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-7.pdf
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-8.pdf
> Link: https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-9.pdf
> Link: https://www.analog.com/en/design-center/evaluation-hardware-and-software/evaluation-boards-kits/ADALM2000.html
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Edward Kigwana <ekigwana@gmail.com>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Superficially this looks like you really have a clock source driver.
You then wrap it in IIO in order to provide convenient userspace interfaces.

I'm not sure we want to do that and I definitely need agreement from those
responsible for clock source drivers before I take anything that does do
this sort of combination of the two types of driver.

I can see that, for these high speed devices, the intent is normally to provide
an input signal to some external circuitry rather than some internal clock
signal, but given this is registered as a clock source the argument that it is
somehow special doesn't seem to hold.

A few more specific comments inline.

Thanks,

Jonathan
 
Jonathan

> ---
> 
> Changelog v2 -> v3:
> * dropped patch about adf4371.yaml; added by accident since it was used to
>   compare against
> * addressed Rob's review comments for DT schema
> 
> Changelog v1 -> v2:
> * validate dt-bindings file with
>   make dt_binding_check DT_SCHEMA_FILES=Documentation/devicetree/bindings/iio/frequency/adi,adf4360.yaml
> 
>  drivers/iio/frequency/Kconfig   |   11 +
>  drivers/iio/frequency/Makefile  |    1 +
>  drivers/iio/frequency/adf4360.c | 1263 +++++++++++++++++++++++++++++++
>  3 files changed, 1275 insertions(+)
>  create mode 100644 drivers/iio/frequency/adf4360.c
> 
> diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
> index 240b81502512..781236496661 100644
> --- a/drivers/iio/frequency/Kconfig
> +++ b/drivers/iio/frequency/Kconfig
> @@ -39,6 +39,17 @@ config ADF4350
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called adf4350.
>  
> +config ADF4360
> +	tristate "Analog Devices ADF4360 Wideband Synthesizers"
> +	depends on SPI
> +	depends on COMMON_CLK
> +	help
> +	  Say yes here to build support for Analog Devices ADF4360
> +	  Wideband Synthesizers. The driver provides direct access via sysfs.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called adf4360.
> +
>  config ADF4371
>  	tristate "Analog Devices ADF4371/ADF4372 Wideband Synthesizers"
>  	depends on SPI
> diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
> index 518b1e50caef..85f2f81da662 100644
> --- a/drivers/iio/frequency/Makefile
> +++ b/drivers/iio/frequency/Makefile
> @@ -6,4 +6,5 @@
>  # When adding new entries keep the list in alphabetical order
>  obj-$(CONFIG_AD9523) += ad9523.o
>  obj-$(CONFIG_ADF4350) += adf4350.o
> +obj-$(CONFIG_ADF4360) += adf4360.o
>  obj-$(CONFIG_ADF4371) += adf4371.o
> diff --git a/drivers/iio/frequency/adf4360.c b/drivers/iio/frequency/adf4360.c
> new file mode 100644
> index 000000000000..49ad58092171
> --- /dev/null
> +++ b/drivers/iio/frequency/adf4360.c
> @@ -0,0 +1,1263 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ADF4360 PLL with Integrated Synthesizer and VCO
> + *
> + * Copyright 2014-2019 Analog Devices Inc.
> + * Copyright 2019-2020 Edward Kigwana.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +#include <linux/util_macros.h>
> +
> +#include <linux/iio/iio.h>
> +
> +/* Register address macro */
> +#define ADF4360_REG(x)			(x)
> +
> +/* ADF4360_CTRL */
> +#define ADF4360_ADDR_CPL_MSK		GENMASK(3, 2)
> +#define ADF4360_CPL(x)			FIELD_PREP(ADF4360_ADDR_CPL_MSK, x)
> +#define ADF4360_ADDR_MUXOUT_MSK		GENMASK(7, 5)
> +#define ADF4360_MUXOUT(x)		FIELD_PREP(ADF4360_ADDR_MUXOUT_MSK, x)
> +#define ADF4360_ADDR_PDP_MSK		BIT(8)
> +#define ADF4360_PDP(x)			FIELD_PREP(ADF4360_ADDR_PDP_MSK, x)
> +#define ADF4360_ADDR_MTLD_MSK		BIT(11)
> +#define ADF4360_MTLD(x)			FIELD_PREP(ADF4360_ADDR_MTLD_MSK, x)
> +#define ADF4360_ADDR_OPL_MSK		GENMASK(13, 12)
> +#define ADF4360_OPL(x)			FIELD_PREP(ADF4360_ADDR_OPL_MSK, x)
> +#define ADF4360_ADDR_CPI1_MSK		GENMASK(16, 14)
> +#define ADF4360_CPI1(x)			FIELD_PREP(ADF4360_ADDR_CPI1_MSK, x)
> +#define ADF4360_ADDR_CPI2_MSK		GENMASK(19, 17)
> +#define ADF4360_CPI2(x)			FIELD_PREP(ADF4360_ADDR_CPI2_MSK, x)
> +#define ADF4360_ADDR_PWR_DWN_MSK	GENMASK(21, 20)
> +#define ADF4360_POWERDOWN(x)		FIELD_PREP(ADF4360_ADDR_PWR_DWN_MSK, x)
> +#define ADF4360_ADDR_PRESCALER_MSK	GENMASK(23, 22)
> +#define ADF4360_PRESCALER(x)		FIELD_PREP(ADF4360_ADDR_PRESCALER_MSK, x)
> +
> +/* ADF4360_NDIV */
> +#define ADF4360_ADDR_A_CNTR_MSK		GENMASK(6, 2)
> +#define ADF4360_A_COUNTER(x)		FIELD_PREP(ADF4360_ADDR_A_CNTR_MSK, x)
> +#define ADF4360_ADDR_B_CNTR_MSK		GENMASK(20, 8)
> +#define ADF4360_B_COUNTER(x)		FIELD_PREP(ADF4360_ADDR_B_CNTR_MSK, x)
> +#define ADF4360_ADDR_OUT_DIV2_MSK	BIT(22)
> +#define ADF4360_OUT_DIV2(x)		FIELD_PREP(ADF4360_ADDR_OUT_DIV2_MSK, x)
> +#define ADF4360_ADDR_DIV2_SEL_MSK	BIT(23)
> +#define ADF4360_PRESCALER_DIV2(x)	FIELD_PREP(ADF4360_ADDR_DIV2_SEL_MSK, x)
> +
> +/* ADF4360_RDIV */
> +#define ADF4360_ADDR_R_CNTR_MSK		GENMASK(15, 2)
> +#define ADF4360_R_COUNTER(x)		FIELD_PREP(ADF4360_ADDR_R_CNTR_MSK, x)
> +#define ADF4360_ADDR_ABP_MSK		GENMASK(17, 16)
> +#define ADF4360_ABP(x)			FIELD_PREP(ADF4360_ADDR_ABP_MSK, x)
> +#define ADF4360_ADDR_BSC_MSK		GENMASK(21, 20)
> +#define ADF4360_BSC(x)			FIELD_PREP(ADF4360_ADDR_BSC_MSK, x)
> +
> +/* Specifications */
> +#define ADF4360_MAX_PFD_RATE		8000000 /* 8 MHz */
> +#define ADF4360_MAX_COUNTER_RATE	300000000 /* 300 MHz */
> +#define ADF4360_MAX_REFIN_RATE		250000000 /* 250 MHz */
> +
> +enum {
> +	ADF4360_CTRL,
> +	ADF4360_RDIV,
> +	ADF4360_NDIV,
> +	ADF4360_REG_NUM,
> +};
> +
> +enum {
> +	ADF4360_GEN1_PC_5,
> +	ADF4360_GEN1_PC_10,
> +	ADF4360_GEN1_PC_15,
> +	ADF4360_GEN1_PC_20,
> +};
> +
> +enum {
> +	ADF4360_GEN2_PC_2_5,
> +	ADF4360_GEN2_PC_5,
> +	ADF4360_GEN2_PC_7_5,
> +	ADF4360_GEN2_PC_10,
> +};
> +
> +enum {
> +	ADF4360_MUXOUT_THREE_STATE,
> +	ADF4360_MUXOUT_LOCK_DETECT,
> +	ADF4360_MUXOUT_NDIV,
> +	ADF4360_MUXOUT_DVDD,
> +	ADF4360_MUXOUT_RDIV,
> +	ADF4360_MUXOUT_OD_LD,
> +	ADF4360_MUXOUT_SDO,
> +	ADF4360_MUXOUT_GND,
> +};
> +
> +enum {
> +	ADF4360_PL_3_5,
> +	ADF4360_PL_5,
> +	ADF4360_PL_7_5,
> +	ADF4360_PL_11,
> +};
> +
> +enum {
> +	ADF4360_CPI_0_31,
> +	ADF4360_CPI_0_62,
> +	ADF4360_CPI_0_93,
> +	ADF4360_CPI_1_25,
> +	ADF4360_CPI_1_56,
> +	ADF4360_CPI_1_87,
> +	ADF4360_CPI_2_18,
> +	ADF4360_CPI_2_50,
> +};
> +
> +enum {
> +	ADF4360_POWER_DOWN_NORMAL,
> +	ADF4360_POWER_DOWN_SOFT_ASYNC,
> +	ADF4360_POWER_DOWN_CE,
> +	ADF4360_POWER_DOWN_SOFT_SYNC,
> +	ADF4360_POWER_DOWN_REGULATOR,
> +};
> +
> +enum {
> +	ADF4360_PRESCALER_8,
> +	ADF4360_PRESCALER_16,
> +	ADF4360_PRESCALER_32,
> +};
> +
> +enum {
> +	ADF4360_ABP_3_0NS,
> +	ADF4360_ABP_1_3NS,
> +	ADF4360_ABP_6_0NS,
> +};
> +
> +enum {
> +	ADF4360_BSC_1,
> +	ADF4360_BSC_2,
> +	ADF4360_BSC_4,
> +	ADF4360_BSC_8,
> +};
> +
> +enum {
> +	ID_ADF4360_0,
> +	ID_ADF4360_1,
> +	ID_ADF4360_2,
> +	ID_ADF4360_3,
> +	ID_ADF4360_4,
> +	ID_ADF4360_5,
> +	ID_ADF4360_6,
> +	ID_ADF4360_7,
> +	ID_ADF4360_8,
> +	ID_ADF4360_9,
> +};
> +
> +enum {
> +	ADF4360_FREQ_REFIN,
> +	ADF4360_MTLD,
> +	ADF4360_FREQ_PFD,
> +};
> +
> +static const char * const adf4360_power_level_modes[] = {
This isn't an enum.  These are real values in sensible units not
magic strings.   Handle them as integers.

We may need to define additional ABI for this but it should be
possible to 'fit' it in the normal structure of ABI.

> +	[ADF4360_PL_3_5] = "3500-uA",
> +	[ADF4360_PL_5] = "5000-uA",
> +	[ADF4360_PL_7_5] = "7500-uA",
> +	[ADF4360_PL_11] = "11000-uA",
> +};
> +
> +static const unsigned int adf4360_power_lvl_microamp[] = {
> +	[ADF4360_PL_3_5] = 3500,
> +	[ADF4360_PL_5] = 5000,
> +	[ADF4360_PL_7_5] = 7500,
> +	[ADF4360_PL_11] = 11000,
> +};
> +
> +static const unsigned int adf4360_cpi_modes[] = {
> +	[ADF4360_CPI_0_31] = 310,
> +	[ADF4360_CPI_0_62] = 620,
> +	[ADF4360_CPI_0_93] = 930,
> +	[ADF4360_CPI_1_25] = 1250,
> +	[ADF4360_CPI_1_56] = 1560,
> +	[ADF4360_CPI_1_87] = 1870,
> +	[ADF4360_CPI_2_18] = 2180,
> +	[ADF4360_CPI_2_50] = 2500,
> +};
> +
> +static const char * const adf4360_muxout_modes[] = {
Superficially from glancing at the datasheet I thing this is debug
functionality?  Perhaps move it to debugfs so as to avoid creating
complex new ABI in sysfs.

> +	[ADF4360_MUXOUT_THREE_STATE] = "three-state",
> +	[ADF4360_MUXOUT_LOCK_DETECT] = "lock-detect",
> +	[ADF4360_MUXOUT_NDIV] = "ndiv",
> +	[ADF4360_MUXOUT_DVDD] = "dvdd",
> +	[ADF4360_MUXOUT_RDIV] = "rdiv",
> +	[ADF4360_MUXOUT_OD_LD] = "od-ld",
> +	[ADF4360_MUXOUT_SDO] = "sdo",
> +	[ADF4360_MUXOUT_GND] = "gnd",
> +};
> +
> +static const char * const adf4360_power_down_modes[] = {
> +	[ADF4360_POWER_DOWN_NORMAL] = "normal",
> +	[ADF4360_POWER_DOWN_SOFT_ASYNC] = "soft-async",
> +	[ADF4360_POWER_DOWN_CE] = "ce",
> +	[ADF4360_POWER_DOWN_SOFT_SYNC] = "soft-sync",
> +	[ADF4360_POWER_DOWN_REGULATOR] = "regulator",
This seems to map rather oddly to the datasheet.  Perhaps some
comments to explain what is going on here would help/
> +};
> +
> +struct adf4360_output {
> +	struct clk_hw hw;
> +	struct iio_dev *indio_dev;
> +};
> +
> +#define to_output(_hw) container_of(_hw, struct adf4360_output, hw)
> +
> +struct adf4360_chip_info {
> +	unsigned int vco_min;
> +	unsigned int vco_max;
> +	unsigned int default_cpl;
> +};
> +
> +struct adf4360_state {
> +	struct spi_device *spi;
> +	const struct adf4360_chip_info *info;
> +	struct adf4360_output output;
> +	struct clk *clkin;
> +	struct gpio_desc *muxout_gpio;
> +	struct gpio_desc *chip_en_gpio;
> +	struct regulator *vdd_reg;
> +	struct mutex lock; /* Protect PLL state. */
> +	unsigned int part_id;
> +	unsigned long clkin_freq;
> +	unsigned long freq_req;
> +	unsigned long r;
> +	unsigned long n;
> +	unsigned int vco_min;
> +	unsigned int vco_max;
> +	unsigned int pfd_freq;
> +	unsigned int cpi;
> +	bool pdp;
> +	bool mtld;
> +	unsigned int power_level;
> +	unsigned int muxout_mode;
> +	unsigned int power_down_mode;
> +	bool initial_reg_seq;
> +	const char *clk_out_name;
> +	unsigned int regs[ADF4360_REG_NUM];
> +	u8 spi_data[3] ____cacheline_aligned;
> +};
> +
> +static const struct adf4360_chip_info adf4360_chip_info_tbl[] = {
> +	{	/* ADF4360-0 */
> +		.vco_min = 2400000000U,
> +		.vco_max = 2725000000U,
> +		.default_cpl = ADF4360_GEN1_PC_10,
> +	}, {	/* ADF4360-1 */
> +		.vco_min = 2050000000U,
> +		.vco_max = 2450000000U,
> +		.default_cpl = ADF4360_GEN1_PC_15,
> +	}, {	/* ADF4360-2 */
> +		.vco_min = 1850000000U,
> +		.vco_max = 2170000000U,
> +		.default_cpl = ADF4360_GEN1_PC_15,
> +	}, {	/* ADF4360-3 */
> +		.vco_min = 1600000000U,
> +		.vco_max = 1950000000U,
> +		.default_cpl = ADF4360_GEN1_PC_15,
> +	}, {	/* ADF4360-4 */
> +		.vco_min = 1450000000U,
> +		.vco_max = 1750000000U,
> +		.default_cpl = ADF4360_GEN1_PC_15,
> +	}, {	/* ADF4360-5 */
> +		.vco_min = 1200000000U,
> +		.vco_max = 1400000000U,
> +		.default_cpl = ADF4360_GEN1_PC_10,
> +	}, {	/* ADF4360-6 */
> +		.vco_min = 1050000000U,
> +		.vco_max = 1250000000U,
> +		.default_cpl = ADF4360_GEN1_PC_10,
> +	}, {	/* ADF4360-7 */
> +		.vco_min = 350000000U,
> +		.vco_max = 1800000000U,
> +		.default_cpl = ADF4360_GEN1_PC_5,
> +	}, {	/* ADF4360-8 */
> +		.vco_min = 65000000U,
> +		.vco_max = 400000000U,
> +		.default_cpl = ADF4360_GEN2_PC_5,
> +	}, {	/* ADF4360-9 */
> +		.vco_min = 65000000U,
> +		.vco_max = 400000000U,
> +		.default_cpl = ADF4360_GEN2_PC_5,
> +	}
> +};
> +
> +static int adf4360_write_reg(struct adf4360_state *st, unsigned int reg,
> +			     unsigned int val)
> +{
> +	int ret;
> +
> +	val |= reg;
> +
> +	st->spi_data[0] = (val >> 16) & 0xff;
> +	st->spi_data[1] = (val >> 8) & 0xff;
> +	st->spi_data[2] = val & 0xff;
> +
> +	ret = spi_write(st->spi, st->spi_data, ARRAY_SIZE(st->spi_data));
> +	if (ret == 0)
> +		st->regs[reg] = val;
> +
> +	return ret;
> +}
> +
> +/* fVCO = B * fREFIN / R */
> +
> +static unsigned long adf4360_clk_recalc_rate(struct clk_hw *hw,
> +					     unsigned long parent_rate)
> +{
> +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +
> +	if (st->r == 0)
> +		return 0;
> +
> +	/*
> +	 * The result is guaranteed to fit in 32-bit, but the intermediate
> +	 * result might require 64-bit.
> +	 */
> +	return DIV_ROUND_CLOSEST_ULL((uint64_t)parent_rate * st->n, st->r);
> +}
> +
> +static unsigned int adf4360_calc_prescaler(unsigned int pfd_freq,
> +					   unsigned int n,
> +					   unsigned int *out_p,
> +					   unsigned int *out_a,
> +					   unsigned int *out_b)
> +{
> +	unsigned int rate = pfd_freq * n;
> +	unsigned int p, a, b;
> +
> +	/* Make sure divider counter input frequency is low enough */
> +	p = 8;
> +	while (p < 32 && rate / p > ADF4360_MAX_COUNTER_RATE)
> +		p *= 2;
> +
> +	/*
> +	 * The range of dividers that can be produced using the dual-modulus
> +	 * pre-scaler is not continuous for values of n < p*(p-1). If we end up
> +	 * with a non supported divider value, pick the next closest one.
> +	 */
> +	a = n % p;
> +	b = n / p;
> +
> +	if (b < 3) {
> +		b = 3;
> +		a = 0;
> +	} else if (a > b) {
> +		if (a - b < p - a) {
> +			a = b;
> +		} else {
> +			a = 0;
> +			b++;
> +		}
> +	}
> +
> +	if (out_p)
> +		*out_p = p;
> +	if (out_a)
> +		*out_a = a;
> +	if (out_b)
> +		*out_b = b;
> +
> +	return p * b + a;
> +}
> +
> +static long adf4360_clk_round_rate(struct clk_hw *hw,
> +				   unsigned long rate,
> +				   unsigned long *parent_rate)
> +{
> +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +	unsigned int r, n;
> +	unsigned int pfd_freq;
> +
> +	if (*parent_rate == 0)
> +		return 0;
> +
> +	if (st->part_id == ID_ADF4360_9)
> +		return *parent_rate * st->n / st->r;
> +
> +	if (rate > st->vco_max)
> +		return st->vco_max;
> +
> +	/* ADF4360-0 to AD4370-7 have an optional by two divider */
> +	if (st->part_id <= ID_ADF4360_7) {
> +		if (rate < st->vco_min / 2)
> +			return st->vco_min / 2;
> +		if (rate < st->vco_min && rate > st->vco_max / 2) {
> +			if (st->vco_min - rate < rate - st->vco_max / 2)
> +				return st->vco_min;
> +			else
> +				return st->vco_max / 2;
> +		}
> +	} else {
> +		if (rate < st->vco_min)
> +			return st->vco_min;
> +	}
> +
> +	r = DIV_ROUND_CLOSEST(*parent_rate, st->pfd_freq);
> +	pfd_freq = *parent_rate / r;
> +	n = DIV_ROUND_CLOSEST(rate, pfd_freq);
> +
> +	if (st->part_id <= ID_ADF4360_7)
> +		n = adf4360_calc_prescaler(pfd_freq, n, NULL, NULL, NULL);
> +
> +	return pfd_freq * n;
> +}
> +
> +static inline bool adf4360_is_powerdown(struct adf4360_state *st)
> +{
> +	return (st->power_down_mode != ADF4360_POWER_DOWN_NORMAL);
> +}
> +
> +static int adf4360_set_freq(struct adf4360_state *st, unsigned long rate)
> +{
> +	unsigned int val_r, val_n, val_ctrl;
> +	unsigned int pfd_freq;
> +	unsigned long r, n;
> +	int ret;
> +
> +	if (!st->clkin_freq || (st->clkin_freq > ADF4360_MAX_REFIN_RATE))
> +		return -EINVAL;
> +
> +	if ((rate < st->vco_min) || (rate > st->vco_max))
> +		return -EINVAL;
> +
> +	if (adf4360_is_powerdown(st))
> +		ret = -EBUSY;
> +
> +	r = DIV_ROUND_CLOSEST(st->clkin_freq, st->pfd_freq);
> +	pfd_freq = st->clkin_freq / r;
> +	n = DIV_ROUND_CLOSEST(rate, pfd_freq);
> +
> +	val_ctrl = ADF4360_CPL(st->info->default_cpl) |
> +		   ADF4360_MUXOUT(st->muxout_mode) |
> +		   ADF4360_PDP(!st->pdp) |
> +		   ADF4360_MTLD(st->mtld) |
> +		   ADF4360_OPL(st->power_level) |
> +		   ADF4360_CPI1(st->cpi) |
> +		   ADF4360_CPI2(st->cpi) |
> +		   ADF4360_POWERDOWN(st->power_down_mode);
> +
> +	/* ADF4360-0 to ADF4360-7 have a dual-modulous prescaler */
> +	if (st->part_id <= ID_ADF4360_7) {
> +		unsigned int p, a, b;
> +
> +		n = adf4360_calc_prescaler(pfd_freq, n, &p, &a, &b);
> +
> +		switch (p) {
> +		case 8:
> +			val_ctrl |= ADF4360_PRESCALER(ADF4360_PRESCALER_8);
> +			break;
> +		case 16:
> +			val_ctrl |= ADF4360_PRESCALER(ADF4360_PRESCALER_16);
> +			break;
> +		default:
> +			val_ctrl |= ADF4360_PRESCALER(ADF4360_PRESCALER_32);
> +			break;
> +		}
> +
> +		val_n = ADF4360_A_COUNTER(a) |
> +			ADF4360_B_COUNTER(b);
> +
> +		if (rate < st->vco_min)
> +			val_n |= ADF4360_OUT_DIV2(true) |
> +				 ADF4360_PRESCALER_DIV2(true);
> +	} else {
> +		val_n = ADF4360_B_COUNTER(n);
> +	}
> +
> +	/*
> +	 * Always use BSC divider of 8, see Analog Devices AN-1347.
> +	 * http://www.analog.com/media/en/technical-documentation/application-notes/AN-1347.pdf
> +	 */
> +	val_r = ADF4360_R_COUNTER(r) |
> +		ADF4360_ABP(ADF4360_ABP_3_0NS) |
> +		ADF4360_BSC(ADF4360_BSC_8);
> +
> +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_RDIV), val_r);
> +	if (ret)
> +		return ret;
> +
> +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val_ctrl);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Allow the transient behavior of the ADF4360-7 during initial
> +	 * power-up to settle.
> +	 */
> +	if (st->initial_reg_seq) {
> +		usleep_range(15000, 20000);
> +		st->initial_reg_seq = false;
> +	}
> +
> +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_NDIV), val_n);
> +	if (ret)
> +		return ret;
> +
> +	st->freq_req = rate;
> +	st->n = n;
> +	st->r = r;
> +
> +	return 0;
> +}
> +
> +static int adf4360_clk_set_rate(struct clk_hw *hw,
> +				unsigned long rate,
> +				unsigned long parent_rate)
> +{
> +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	if ((parent_rate == 0) || (parent_rate > ADF4360_MAX_REFIN_RATE))
> +		return -EINVAL;
> +
> +	mutex_lock(&st->lock);
> +	if (st->clkin_freq != parent_rate)
> +		st->clkin_freq = parent_rate;
> +
> +	ret = adf4360_set_freq(st, rate);
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int __adf4360_power_down(struct adf4360_state *st, unsigned int mode)
> +{
> +	struct device *dev = &st->spi->dev;
> +	unsigned int val;
> +	int ret = 0;
> +
> +	switch (mode) {
> +	case ADF4360_POWER_DOWN_NORMAL:
> +		if (st->vdd_reg) {
> +			ret = regulator_enable(st->vdd_reg);
> +			if (ret) {
> +				dev_err(dev, "Supply enable error: %d\n", ret);
> +				return ret;
> +			}
> +		}
> +
> +		st->initial_reg_seq = true;
> +		st->power_down_mode = mode;
> +		ret = adf4360_set_freq(st, st->freq_req);
> +		break;
> +	case ADF4360_POWER_DOWN_SOFT_ASYNC: /* fallthrough */
> +	case ADF4360_POWER_DOWN_SOFT_SYNC:
> +		val = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_MUXOUT_MSK;
> +		val |= ADF4360_POWERDOWN(mode);
> +		ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
> +		break;
> +	case ADF4360_POWER_DOWN_CE:
> +		if (st->chip_en_gpio)
> +			gpiod_set_value(st->chip_en_gpio, 0x0);
> +		else
> +			return -ENODEV;
> +		break;
> +	case ADF4360_POWER_DOWN_REGULATOR:
> +		if (!st->vdd_reg)
> +			return -ENODEV;
> +
> +		if (st->chip_en_gpio)
> +			ret = __adf4360_power_down(st, ADF4360_POWER_DOWN_CE);
> +		else
> +			ret = __adf4360_power_down(st,
> +						ADF4360_POWER_DOWN_SOFT_ASYNC);
> +
> +		ret = regulator_disable(st->vdd_reg);
> +		if (ret)
> +			dev_err(dev, "Supply disable error: %d\n", ret);
> +		break;
> +	}
> +	if (ret == 0)
> +		st->power_down_mode = mode;
> +
> +	return 0;
> +}
> +
> +static int adf4360_power_down(struct adf4360_state *st, unsigned int mode)
> +{
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	ret = __adf4360_power_down(st, mode);
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int adf4360_clk_prepare(struct clk_hw *hw)
> +{
> +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +
> +	return adf4360_power_down(st, ADF4360_POWER_DOWN_NORMAL);
> +}
> +
> +static void adf4360_clk_unprepare(struct clk_hw *hw)
> +{
> +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +
> +	adf4360_power_down(st, ADF4360_POWER_DOWN_SOFT_ASYNC);
> +}
> +
> +static int adf4360_clk_enable(struct clk_hw *hw)
> +{
> +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +
> +	if (st->chip_en_gpio)
> +		gpiod_set_value(st->chip_en_gpio, 0x1);
> +
> +	return 0;
> +}
> +
> +static void adf4360_clk_disable(struct clk_hw *hw)
> +{
> +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +
> +	if (st->chip_en_gpio)
> +		adf4360_power_down(st, ADF4360_POWER_DOWN_CE);
> +}
> +
> +static int adf4360_clk_is_enabled(struct clk_hw *hw)
> +{
> +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +
> +	return adf4360_is_powerdown(st);
> +}
> +
> +static const struct clk_ops adf4360_clk_ops = {
> +	.recalc_rate = adf4360_clk_recalc_rate,
> +	.round_rate = adf4360_clk_round_rate,
> +	.set_rate = adf4360_clk_set_rate,
> +	.prepare = adf4360_clk_prepare,
> +	.unprepare = adf4360_clk_unprepare,
> +	.enable = adf4360_clk_enable,
> +	.disable = adf4360_clk_disable,
> +	.is_enabled = adf4360_clk_is_enabled,
> +};
> +
> +static ssize_t adf4360_read(struct iio_dev *indio_dev,
> +			    uintptr_t private,
> +			    const struct iio_chan_spec *chan,
> +			    char *buf)
> +{
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +	unsigned long val;
> +	int ret = 0;
> +
> +	switch ((u32)private) {
> +	case ADF4360_FREQ_REFIN:
> +		val = st->clkin_freq;
> +		break;
> +	case ADF4360_MTLD:
> +		val = st->mtld;
> +		break;
> +	case ADF4360_FREQ_PFD:
> +		val = st->pfd_freq;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		val = 0;
> +	}
> +
> +	return ret < 0 ? ret : sprintf(buf, "%lu\n", val);
> +}
> +
> +static ssize_t adf4360_write(struct iio_dev *indio_dev,
> +			     uintptr_t private,
> +			     const struct iio_chan_spec *chan,
> +			     const char *buf, size_t len)
> +{
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +	unsigned long readin, tmp;
> +	bool mtld;
> +	int ret = 0;
> +
> +	mutex_lock(&st->lock);
> +	switch ((u32)private) {
> +	case ADF4360_FREQ_REFIN:
> +		ret = kstrtoul(buf, 10, &readin);
> +		if (ret)
> +			break;
> +
> +		if ((readin > ADF4360_MAX_REFIN_RATE) || (readin == 0)) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		if (st->clkin) {
> +			tmp = clk_round_rate(st->clkin, readin);
> +			if (tmp != readin) {
> +				ret = -EINVAL;
> +				break;
> +			}
> +
> +			ret = clk_set_rate(st->clkin, tmp);
> +			if (ret)
> +				break;
A bit odd to directly provide an interface to control and entirely different
bit of hardware.   If there are specific demands on the input clock as a result
of something to do with the outputs, then fair enough.  Direct tweaking like
this seems like a very odd interface.

> +		}
> +
> +		st->clkin_freq = readin;
> +		break;
> +	case ADF4360_MTLD:
> +		ret = kstrtobool(buf, &mtld);
> +		if (ret)
> +			break;
> +
> +		st->mtld = mtld;
> +		break;
> +	case ADF4360_FREQ_PFD:
> +		ret = kstrtoul(buf, 10, &readin);
> +		if (ret)
> +			break;
> +
> +		if ((readin > ADF4360_MAX_PFD_RATE) || (readin == 0)) {
> +			ret = -EINVAL;
> +			break;
> +		}
> +
> +		st->pfd_freq = readin;
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	if (ret == 0)
> +		ret = adf4360_set_freq(st, st->freq_req);
> +	mutex_unlock(&st->lock);
> +
> +	return ret ? ret : len;
> +}
> +
> +static int adf4360_get_muxout_mode(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan)
> +{
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +
> +	return st->muxout_mode;
> +}
> +
> +static int adf4360_set_muxout_mode(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   unsigned int mode)
> +{
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +	unsigned int writeval;
> +	int ret = 0;
> +
> +	mutex_lock(&st->lock);
> +	writeval = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_MUXOUT_MSK;
> +	writeval |= ADF4360_MUXOUT(mode & 0x7);
> +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), writeval);
> +	if (ret == 0)
> +		st->muxout_mode = mode & 0x7;
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static const struct iio_enum adf4360_muxout_modes_available = {
> +	.items = adf4360_muxout_modes,
> +	.num_items = ARRAY_SIZE(adf4360_muxout_modes),
> +	.get = adf4360_get_muxout_mode,
> +	.set = adf4360_set_muxout_mode,
> +};
> +
> +static int adf4360_get_power_down(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan)
> +{
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +
> +	return st->power_down_mode;
> +}
> +
> +static int adf4360_set_power_down(struct iio_dev *indio_dev,
> +				  const struct iio_chan_spec *chan,
> +				  unsigned int mode)
> +{
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +
> +	return adf4360_power_down(st, mode);
> +}
> +
> +static const struct iio_enum adf4360_pwr_dwn_modes_available = {
> +	.items = adf4360_power_down_modes,
> +	.num_items = ARRAY_SIZE(adf4360_power_down_modes),
> +	.get = adf4360_get_power_down,
> +	.set = adf4360_set_power_down,
> +};
> +
> +static int adf4360_get_power_level(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan)
> +{
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +
> +	return st->power_level;
> +}
> +
> +static int adf4360_set_power_level(struct iio_dev *indio_dev,
> +				   const struct iio_chan_spec *chan,
> +				   unsigned int level)
> +{
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +	unsigned int val;
> +	int ret;
> +
> +	if (adf4360_is_powerdown(st))
> +		return -EBUSY;
> +
> +	mutex_lock(&st->lock);
> +	val = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_OPL_MSK;
> +	val |= ADF4360_OPL(level);
> +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
> +	st->power_level = level;
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static const struct iio_enum adf4360_pwr_lvl_modes_available = {
> +	.items = adf4360_power_level_modes,
> +	.num_items = ARRAY_SIZE(adf4360_power_level_modes),
> +	.get = adf4360_get_power_level,
> +	.set = adf4360_set_power_level,
> +};
> +
> +#define _ADF4360_EXT_INFO(_name, _ident) { \
> +	.name = _name, \
> +	.read = adf4360_read, \
> +	.write = adf4360_write, \
> +	.private = _ident, \
> +	.shared = IIO_SEPARATE, \
> +}
> +
> +static const struct iio_chan_spec_ext_info adf4360_ext_info[] = {

This is a wide range of new ABI.  These all need documentation
in Documentation/ABI/testing/sysfs-bus-iio-*

Without docs, it's hard to discuss if these are appropriate but a few initial comments...
> +	_ADF4360_EXT_INFO("refin_frequency", ADF4360_FREQ_REFIN),
Looks like a control that should be matched to some clock source and
not change at runtime?

> +	_ADF4360_EXT_INFO("mute_till_lock_detect", ADF4360_MTLD),
> +	_ADF4360_EXT_INFO("pfd_frequency", ADF4360_FREQ_PFD),
> +	IIO_ENUM_AVAILABLE("muxout_mode", &adf4360_muxout_modes_available),
> +	IIO_ENUM("muxout_mode", false, &adf4360_muxout_modes_available),
> +	IIO_ENUM_AVAILABLE("power_down", &adf4360_pwr_dwn_modes_available),
> +	IIO_ENUM("power_down", false, &adf4360_pwr_dwn_modes_available),
> +	IIO_ENUM_AVAILABLE("power_level", &adf4360_pwr_lvl_modes_available),
> +	IIO_ENUM("power_level", false, &adf4360_pwr_lvl_modes_available),
> +	{ },
> +};
> +
> +static const struct iio_chan_spec adf4360_chan = {
> +	.type = IIO_ALTVOLTAGE,
> +	.indexed = 1,
> +	.output = 1,
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY),
> +	.ext_info = adf4360_ext_info,
> +};
> +
> +static int adf4360_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val,
> +			    int *val2,
> +			    long mask)
> +{
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +	bool lk_det;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_FREQUENCY:
> +		if (adf4360_is_powerdown(st))
> +			return -EBUSY;
> +
> +		lk_det = (ADF4360_MUXOUT_LOCK_DETECT | ADF4360_MUXOUT_OD_LD) &
> +			 st->muxout_mode;
> +		if (lk_det && st->muxout_gpio) {
> +			if (!gpiod_get_value(st->muxout_gpio)) {
> +				dev_dbg(&st->spi->dev, "PLL un-locked\n");
> +				return -EBUSY;
> +			}
> +		}
> +
> +		*val = st->freq_req;
> +		return IIO_VAL_INT;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +};
> +
> +static int adf4360_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val,
> +			     int val2,
> +			     long mask)
> +{
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +	int ret;
> +
> +	mutex_lock(&st->lock);
> +	switch (mask) {
> +	case IIO_CHAN_INFO_FREQUENCY:
> +		ret = adf4360_set_freq(st, val);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +	}
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static int adf4360_reg_access(struct iio_dev *indio_dev,
> +			      unsigned int reg,
> +			      unsigned int writeval,
> +			      unsigned int *readval)
> +{
> +	struct adf4360_state *st = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	if (reg >= ADF4360_REG_NUM)
> +		return -EFAULT;
> +
> +	mutex_lock(&st->lock);
> +	if (readval) {
> +		*readval = st->regs[reg];
> +	} else {
> +		writeval &= 0xFFFFFC;
> +		ret = adf4360_write_reg(st, reg, writeval);
> +	}
> +	mutex_unlock(&st->lock);
> +
> +	return ret;
> +}
> +
> +static const struct iio_info adf4360_iio_info = {
> +	.read_raw = &adf4360_read_raw,
> +	.write_raw = &adf4360_write_raw,
> +	.debugfs_reg_access = &adf4360_reg_access,
> +};
> +
> +static int adf4360_get_gpio(struct adf4360_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	unsigned int val;
> +	int ret, i;
> +
> +	st->chip_en_gpio = devm_gpiod_get_optional(dev, "enable",
> +						   GPIOD_OUT_HIGH);
> +	if (IS_ERR(st->chip_en_gpio)) {
> +		dev_err(dev, "Chip enable GPIO error\n");

Put handling in here to prevent an error message of DEFER.
Same for the other routes where this might happen.

> +		return PTR_ERR(st->chip_en_gpio);
> +	}
> +
> +	if (st->chip_en_gpio)
> +		st->power_down_mode = ADF4360_POWER_DOWN_CE;
> +
> +	st->muxout_gpio = devm_gpiod_get_optional(dev, "adi,muxout", GPIOD_IN);
> +	if (IS_ERR(st->muxout_gpio)) {
> +		dev_err(dev, "Muxout GPIO error\n");
> +		return PTR_ERR(st->muxout_gpio);
> +	}
> +
> +	if (!st->muxout_gpio)
> +		return 0;
> +
> +	/* ADF4360 PLLs are write only devices, try to probe using GPIO. */
> +	for (i = 0; i < 4; i++) {
> +		if (i & 1)
> +			val = ADF4360_MUXOUT(ADF4360_MUXOUT_DVDD);
> +		else
> +			val = ADF4360_MUXOUT(ADF4360_MUXOUT_GND);
> +
> +		ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
> +		if (ret)
> +			return ret;
> +
> +		ret = gpiod_get_value(st->muxout_gpio);
> +		if (ret ^ (i & 1)) {
> +			dev_err(dev, "Probe failed (muxout)");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static void adf4360_clkin_disable(void *data)
> +{
> +	struct adf4360_state *st = data;
> +
> +	clk_disable_unprepare(st->clkin);
> +}
> +
> +static int adf4360_get_clkin(struct adf4360_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	struct clk *clk;
> +	int ret;
> +
> +	clk = devm_clk_get(dev, "clkin");
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	ret = clk_prepare_enable(clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, adf4360_clkin_disable, st);
> +	if (ret)
> +		return ret;
> +
> +	st->clkin = clk;
> +	st->clkin_freq = clk_get_rate(clk);
> +
> +	return 0;
> +}
> +
> +static void adf4360_clk_del_provider(void *data)
> +{
> +	struct adf4360_state *st = data;
> +
> +	of_clk_del_provider(st->spi->dev.of_node);
> +}
> +
> +static int adf4360_clk_register(struct adf4360_state *st)
> +{

Hmm. This makes me wonder why this is an IIO driver rather than a clk
driver?  Definitely needs some more information in the patch description
or a cover letter.

> +	struct spi_device *spi = st->spi;
> +	struct clk_init_data init;
> +	struct clk *clk;
> +	const char *parent_name;
> +	int ret;
> +
> +	parent_name = of_clk_get_parent_name(spi->dev.of_node, 0);
> +	if (!parent_name)
> +		return -EINVAL;
> +
> +	init.name = st->clk_out_name;
> +	init.ops = &adf4360_clk_ops;
> +	init.flags = CLK_SET_RATE_GATE;
> +	init.parent_names = &parent_name;
> +	init.num_parents = 1;
> +
> +	st->output.hw.init = &init;
> +
> +	clk = devm_clk_register(&spi->dev, &st->output.hw);
> +	if (IS_ERR(clk))
> +		return PTR_ERR(clk);
> +
> +	ret = of_clk_add_provider(spi->dev.of_node, of_clk_src_simple_get, clk);
> +	if (ret)
> +		return ret;
> +
> +	return devm_add_action_or_reset(&spi->dev, adf4360_clk_del_provider, st);
> +}
> +
> +static int adf4360_parse_dt(struct adf4360_state *st)
> +{
> +	struct device *dev = &st->spi->dev;
> +	u32 tmp;
> +	int ret;
> +
> +	ret = device_property_read_string(dev, "clock-output-names",
> +					  &st->clk_out_name);
> +	if ((ret < 0) && dev->of_node)
> +		st->clk_out_name = dev->of_node->name;
> +
> +	if (st->part_id >= ID_ADF4360_7) {
> +		/*
> +		 * ADF4360-7 to ADF4360-9 have a VCO that is tuned to a specific
> +		 * range using an external inductor. These properties describe
> +		 * the range selected by the external inductor.
> +		 */
> +		ret = device_property_read_u32(dev,
> +					       "adi,vco-minimum-frequency-hz",
> +					       &tmp);
> +		if (ret == 0)
> +			st->vco_min = max(st->info->vco_min, tmp);
> +		else
> +			st->vco_min = st->info->vco_min;
> +
> +		ret = device_property_read_u32(dev,
> +					       "adi,vco-maximum-frequency-hz",
> +					       &tmp);
> +		if (ret == 0)
> +			st->vco_max = min(st->info->vco_max, tmp);
> +		else
> +			st->vco_max = st->info->vco_max;
> +	} else {
> +		st->vco_min = st->info->vco_min;
> +		st->vco_max = st->info->vco_max;
> +	}
> +
> +	st->pdp = device_property_read_bool(dev, "adi,loop-filter-inverting");
> +
> +	ret = device_property_read_u32(dev,
> +				       "adi,loop-filter-pfd-frequency-hz",
> +				       &tmp);
> +	if (ret == 0) {
> +		st->pfd_freq = tmp;
> +	} else {
> +		dev_err(dev, "PFD frequency property missing\n");
> +		return ret;
> +	}
> +
> +	ret = device_property_read_u32(dev,
> +				"adi,loop-filter-charge-pump-current-microamp",
> +				&tmp);
> +	if (ret == 0) {
> +		st->cpi = find_closest(tmp, adf4360_cpi_modes,
> +				       ARRAY_SIZE(adf4360_cpi_modes));
> +	} else {
> +		dev_err(dev, "CPI property missing\n");
> +		return ret;
> +	}
> +
> +	ret = device_property_read_u32(dev, "adi,power-up-frequency-hz", &tmp);
> +	if (ret == 0)
> +		st->freq_req = tmp;
> +	else
> +		st->freq_req = st->vco_min;
> +
> +	ret = device_property_read_u32(dev, "adi,power-out-level-microamp",
> +				       &tmp);
> +	if (ret == 0)
> +		st->power_level = find_closest(tmp, adf4360_power_lvl_microamp,
> +					ARRAY_SIZE(adf4360_power_lvl_microamp));
> +	else
> +		st->power_level = ADF4360_PL_5;
> +
> +	return 0;
> +}
> +
> +static int adf4360_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	const struct spi_device_id *id = spi_get_device_id(spi);

Given we require various dt parameters to be present, might as well
associate the id with the of_ structures instead and use the dt
calls throughout.  Even better if you use the firmware type independent
versions.

> +	struct adf4360_state *st;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	st = iio_priv(indio_dev);
> +
> +	mutex_init(&st->lock);
> +
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	st->spi = spi;
> +	st->info = &adf4360_chip_info_tbl[id->driver_data];
> +	st->part_id = id->driver_data;
> +	st->muxout_mode = ADF4360_MUXOUT_LOCK_DETECT;
> +	st->mtld = true;
> +
> +	ret = adf4360_parse_dt(st);
> +	if (ret) {
> +		dev_err(&spi->dev, "Parsing properties failed (%d)\n", ret);
> +		return -ENODEV;
> +	}
> +
> +	indio_dev->dev.parent = &spi->dev;
> +
> +	if (spi->dev.of_node)
> +		indio_dev->name = spi->dev.of_node->name;
> +	else
> +		indio_dev->name = spi_get_device_id(spi)->name;
> +
> +	indio_dev->info = &adf4360_iio_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = &adf4360_chan;
> +	indio_dev->num_channels = 1;
> +	st->output.indio_dev = indio_dev;
> +
> +	ret = adf4360_get_gpio(st);
> +	if (ret)
> +		return ret;
> +
> +	ret = adf4360_get_clkin(st);
> +	if (ret)
> +		return ret;
> +
> +	st->vdd_reg = devm_regulator_get_optional(&spi->dev, "vdd");
> +	if (IS_ERR(st->vdd_reg)) {
> +		if (PTR_ERR(st->vdd_reg) != -ENODEV) {
> +			dev_err(&spi->dev, "Regulator error\n");
> +			return PTR_ERR(st->vdd_reg);
> +		}
> +
> +		st->vdd_reg = NULL;
> +	}
> +
> +	ret = adf4360_power_down(st, ADF4360_POWER_DOWN_NORMAL);
> +	if (ret)
> +		return ret;
> +
> +	ret = adf4360_clk_register(st);
> +	if (ret)
> +		return ret;
> +
> +	return devm_iio_device_register(&spi->dev, indio_dev);
> +}
> +
> +static const struct of_device_id adf4360_of_match[] = {
> +	{ .compatible = "adi,adf4360-0", },
> +	{ .compatible = "adi,adf4360-1", },
> +	{ .compatible = "adi,adf4360-2", },
> +	{ .compatible = "adi,adf4360-3", },
> +	{ .compatible = "adi,adf4360-4", },
> +	{ .compatible = "adi,adf4360-5", },
> +	{ .compatible = "adi,adf4360-6", },
> +	{ .compatible = "adi,adf4360-7", },
> +	{ .compatible = "adi,adf4360-8", },
> +	{ .compatible = "adi,adf4360-9", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, adf4360_of_match);
> +
> +static const struct spi_device_id adf4360_id[] = {

As mentioned above, you can't actually probe this device
without a pile of dt stuff.  So this fallback doesn't
make much sense.  Use the data field in the of table
above and get rid of this table entirely.

> +	{"adf4360-0", ID_ADF4360_0},
> +	{"adf4360-1", ID_ADF4360_1},
> +	{"adf4360-2", ID_ADF4360_2},
> +	{"adf4360-3", ID_ADF4360_3},
> +	{"adf4360-4", ID_ADF4360_4},
> +	{"adf4360-5", ID_ADF4360_5},
> +	{"adf4360-6", ID_ADF4360_6},
> +	{"adf4360-7", ID_ADF4360_7},
> +	{"adf4360-8", ID_ADF4360_8},
> +	{"adf4360-9", ID_ADF4360_9},
> +	{}
> +};
> +
> +static struct spi_driver adf4360_driver = {
> +	.driver = {
> +		.name = "adf4360",
> +		.of_match_table = adf4360_of_match,
> +		.owner = THIS_MODULE,

It's a long time since we had to set the .owner field for each driver.

Follow through what happens in module_spi_driver, spi_register_driver
etc and you'll find it's set automatically during driver registration.

> +	},
> +	.probe = adf4360_probe,
> +	.id_table = adf4360_id,
> +};
> +module_spi_driver(adf4360_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
> +MODULE_AUTHOR("Edward Kigwana <ekigwana@gmail.com>");
> +MODULE_DESCRIPTION("Analog Devices ADF4360 PLL");
Alexandru Ardelean Feb. 3, 2020, 11:18 a.m. UTC | #2
On Sun, 2020-02-02 at 14:45 +0000, Jonathan Cameron wrote:
> [External]
> 
> On Tue, 28 Jan 2020 13:13:00 +0200
> Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> 
> > From: Edward Kigwana <ekigwana@gmail.com>
> > 
> > The ADF4360-N (N is 0..9) are a family of integrated integer-N synthesizer
> > and voltage controlled oscillator (VCO).
> > 
> > Control of all the on-chip registers is through a simple 3-wire SPI
> > interface. The device operates with a power supply ranging from 3.0 V to
> > 3.6 V and can be powered down when not in use.
> > 
> > The initial draft-work was done by Lars-Peter Clausen <lars@metafoo.de>
> > The current/latest/complete version was implemented by
> > Edward Kigwana <ekigwana@gmail.com>.
> > 
> > The ADF4360-9 is also present on the Analog Devices 'Advanced Active
> > Learning Module 2000' (ADALM-2000).
> > 
> > Link: 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-0.pdf
> > Link: 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-1.pdf
> > Link: 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-2.pdf
> > Link: 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-3.pdf
> > Link: 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-4.pdf
> > Link: 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-5.pdf
> > Link: 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-6.pdf
> > Link: 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-7.pdf
> > Link: 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-8.pdf
> > Link: 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-9.pdf
> > Link: 
> > https://www.analog.com/en/design-center/evaluation-hardware-and-software/evaluation-boards-kits/ADALM2000.html
> > 
> > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > Signed-off-by: Edward Kigwana <ekigwana@gmail.com>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> 
> Superficially this looks like you really have a clock source driver.
> You then wrap it in IIO in order to provide convenient userspace interfaces.
> 
> I'm not sure we want to do that and I definitely need agreement from those
> responsible for clock source drivers before I take anything that does do
> this sort of combination of the two types of driver.
> 

I'll admit that I am a bit fuzzy about these frequency-generators/clock-
sources/synthesizer chips.
In the sense, I don't know where to draw the line between when to consider it
[primarly] an IIO device [for the IIO subsystem] or when to consider it a clock-
device [primarily, for the clk subsystem].
Obviously there's an inertia [for me at least] to go IIO, as I know it a bit
better. 

We've had some quick/short discussions [internally] about this driver, and also
about the LTC6952. We didn't have a bigger one, where we try to discuss them
more in-detail; but we do have a plan to do it.

So, then maybe [until then] a question: how do we decide this? [generally
speaking, not just adf4360 & ltc6952]
i.e. when to consider it clk-first or iio-first;
I'm not sure if there is a clear-cut rule, but maybe some guidelines/thoughts?
Obviously, I'll have to read-up more on the clk framework code [as well] to get
a feel for it.

We've done some things internally [up until now] with some of these clock-chips
that's been mostly IIO-centric. Now, much of it may not be correct, but it is
what we use as template when writing new driver, which [of course] is not good.
And, I also don't want to push/force our mistakes upstream, because that is
[well...] bad. Hence this/these question(s).

Thanks
Alex

> I can see that, for these high speed devices, the intent is normally to
> provide
> an input signal to some external circuitry rather than some internal clock
> signal, but given this is registered as a clock source the argument that it is
> somehow special doesn't seem to hold.
> 
> A few more specific comments inline.
> 
> Thanks,
> 
> Jonathan
>  
> Jonathan
> 
> > ---
> > 
> > Changelog v2 -> v3:
> > * dropped patch about adf4371.yaml; added by accident since it was used to
> >   compare against
> > * addressed Rob's review comments for DT schema
> > 
> > Changelog v1 -> v2:
> > * validate dt-bindings file with
> >   make dt_binding_check
> > DT_SCHEMA_FILES=Documentation/devicetree/bindings/iio/frequency/adi,adf4360.
> > yaml
> > 
> >  drivers/iio/frequency/Kconfig   |   11 +
> >  drivers/iio/frequency/Makefile  |    1 +
> >  drivers/iio/frequency/adf4360.c | 1263 +++++++++++++++++++++++++++++++
> >  3 files changed, 1275 insertions(+)
> >  create mode 100644 drivers/iio/frequency/adf4360.c
> > 
> > diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
> > index 240b81502512..781236496661 100644
> > --- a/drivers/iio/frequency/Kconfig
> > +++ b/drivers/iio/frequency/Kconfig
> > @@ -39,6 +39,17 @@ config ADF4350
> >  	  To compile this driver as a module, choose M here: the
> >  	  module will be called adf4350.
> >  
> > +config ADF4360
> > +	tristate "Analog Devices ADF4360 Wideband Synthesizers"
> > +	depends on SPI
> > +	depends on COMMON_CLK
> > +	help
> > +	  Say yes here to build support for Analog Devices ADF4360
> > +	  Wideband Synthesizers. The driver provides direct access via sysfs.
> > +
> > +	  To compile this driver as a module, choose M here: the
> > +	  module will be called adf4360.
> > +
> >  config ADF4371
> >  	tristate "Analog Devices ADF4371/ADF4372 Wideband Synthesizers"
> >  	depends on SPI
> > diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
> > index 518b1e50caef..85f2f81da662 100644
> > --- a/drivers/iio/frequency/Makefile
> > +++ b/drivers/iio/frequency/Makefile
> > @@ -6,4 +6,5 @@
> >  # When adding new entries keep the list in alphabetical order
> >  obj-$(CONFIG_AD9523) += ad9523.o
> >  obj-$(CONFIG_ADF4350) += adf4350.o
> > +obj-$(CONFIG_ADF4360) += adf4360.o
> >  obj-$(CONFIG_ADF4371) += adf4371.o
> > diff --git a/drivers/iio/frequency/adf4360.c
> > b/drivers/iio/frequency/adf4360.c
> > new file mode 100644
> > index 000000000000..49ad58092171
> > --- /dev/null
> > +++ b/drivers/iio/frequency/adf4360.c
> > @@ -0,0 +1,1263 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * ADF4360 PLL with Integrated Synthesizer and VCO
> > + *
> > + * Copyright 2014-2019 Analog Devices Inc.
> > + * Copyright 2019-2020 Edward Kigwana.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/delay.h>
> > +#include <linux/device.h>
> > +#include <linux/err.h>
> > +#include <linux/gpio/consumer.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/regulator/consumer.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/util_macros.h>
> > +
> > +#include <linux/iio/iio.h>
> > +
> > +/* Register address macro */
> > +#define ADF4360_REG(x)			(x)
> > +
> > +/* ADF4360_CTRL */
> > +#define ADF4360_ADDR_CPL_MSK		GENMASK(3, 2)
> > +#define ADF4360_CPL(x)			FIELD_PREP(ADF4360_ADDR_CPL_MSK,
> > x)
> > +#define ADF4360_ADDR_MUXOUT_MSK		GENMASK(7, 5)
> > +#define ADF4360_MUXOUT(x)		FIELD_PREP(ADF4360_ADDR_MUXOUT_MSK, x)
> > +#define ADF4360_ADDR_PDP_MSK		BIT(8)
> > +#define ADF4360_PDP(x)			FIELD_PREP(ADF4360_ADDR_PDP_MSK,
> > x)
> > +#define ADF4360_ADDR_MTLD_MSK		BIT(11)
> > +#define ADF4360_MTLD(x)			FIELD_PREP(ADF4360_ADDR_MTLD_MSK
> > , x)
> > +#define ADF4360_ADDR_OPL_MSK		GENMASK(13, 12)
> > +#define ADF4360_OPL(x)			FIELD_PREP(ADF4360_ADDR_OPL_MSK,
> > x)
> > +#define ADF4360_ADDR_CPI1_MSK		GENMASK(16, 14)
> > +#define ADF4360_CPI1(x)			FIELD_PREP(ADF4360_ADDR_CPI1_MSK
> > , x)
> > +#define ADF4360_ADDR_CPI2_MSK		GENMASK(19, 17)
> > +#define ADF4360_CPI2(x)			FIELD_PREP(ADF4360_ADDR_CPI2_MSK
> > , x)
> > +#define ADF4360_ADDR_PWR_DWN_MSK	GENMASK(21, 20)
> > +#define ADF4360_POWERDOWN(x)		FIELD_PREP(ADF4360_ADDR_PWR_DWN_
> > MSK, x)
> > +#define ADF4360_ADDR_PRESCALER_MSK	GENMASK(23, 22)
> > +#define ADF4360_PRESCALER(x)		FIELD_PREP(ADF4360_ADDR_PRESCALE
> > R_MSK, x)
> > +
> > +/* ADF4360_NDIV */
> > +#define ADF4360_ADDR_A_CNTR_MSK		GENMASK(6, 2)
> > +#define ADF4360_A_COUNTER(x)		FIELD_PREP(ADF4360_ADDR_A_CNTR_M
> > SK, x)
> > +#define ADF4360_ADDR_B_CNTR_MSK		GENMASK(20, 8)
> > +#define ADF4360_B_COUNTER(x)		FIELD_PREP(ADF4360_ADDR_B_CNTR_M
> > SK, x)
> > +#define ADF4360_ADDR_OUT_DIV2_MSK	BIT(22)
> > +#define ADF4360_OUT_DIV2(x)		FIELD_PREP(ADF4360_ADDR_OUT_DIV2
> > _MSK, x)
> > +#define ADF4360_ADDR_DIV2_SEL_MSK	BIT(23)
> > +#define ADF4360_PRESCALER_DIV2(x)	FIELD_PREP(ADF4360_ADDR_DIV2_SEL_MSK, x)
> > +
> > +/* ADF4360_RDIV */
> > +#define ADF4360_ADDR_R_CNTR_MSK		GENMASK(15, 2)
> > +#define ADF4360_R_COUNTER(x)		FIELD_PREP(ADF4360_ADDR_R_CNTR_M
> > SK, x)
> > +#define ADF4360_ADDR_ABP_MSK		GENMASK(17, 16)
> > +#define ADF4360_ABP(x)			FIELD_PREP(ADF4360_ADDR_ABP_MSK,
> > x)
> > +#define ADF4360_ADDR_BSC_MSK		GENMASK(21, 20)
> > +#define ADF4360_BSC(x)			FIELD_PREP(ADF4360_ADDR_BSC_MSK,
> > x)
> > +
> > +/* Specifications */
> > +#define ADF4360_MAX_PFD_RATE		8000000 /* 8 MHz */
> > +#define ADF4360_MAX_COUNTER_RATE	300000000 /* 300 MHz */
> > +#define ADF4360_MAX_REFIN_RATE		250000000 /* 250 MHz */
> > +
> > +enum {
> > +	ADF4360_CTRL,
> > +	ADF4360_RDIV,
> > +	ADF4360_NDIV,
> > +	ADF4360_REG_NUM,
> > +};
> > +
> > +enum {
> > +	ADF4360_GEN1_PC_5,
> > +	ADF4360_GEN1_PC_10,
> > +	ADF4360_GEN1_PC_15,
> > +	ADF4360_GEN1_PC_20,
> > +};
> > +
> > +enum {
> > +	ADF4360_GEN2_PC_2_5,
> > +	ADF4360_GEN2_PC_5,
> > +	ADF4360_GEN2_PC_7_5,
> > +	ADF4360_GEN2_PC_10,
> > +};
> > +
> > +enum {
> > +	ADF4360_MUXOUT_THREE_STATE,
> > +	ADF4360_MUXOUT_LOCK_DETECT,
> > +	ADF4360_MUXOUT_NDIV,
> > +	ADF4360_MUXOUT_DVDD,
> > +	ADF4360_MUXOUT_RDIV,
> > +	ADF4360_MUXOUT_OD_LD,
> > +	ADF4360_MUXOUT_SDO,
> > +	ADF4360_MUXOUT_GND,
> > +};
> > +
> > +enum {
> > +	ADF4360_PL_3_5,
> > +	ADF4360_PL_5,
> > +	ADF4360_PL_7_5,
> > +	ADF4360_PL_11,
> > +};
> > +
> > +enum {
> > +	ADF4360_CPI_0_31,
> > +	ADF4360_CPI_0_62,
> > +	ADF4360_CPI_0_93,
> > +	ADF4360_CPI_1_25,
> > +	ADF4360_CPI_1_56,
> > +	ADF4360_CPI_1_87,
> > +	ADF4360_CPI_2_18,
> > +	ADF4360_CPI_2_50,
> > +};
> > +
> > +enum {
> > +	ADF4360_POWER_DOWN_NORMAL,
> > +	ADF4360_POWER_DOWN_SOFT_ASYNC,
> > +	ADF4360_POWER_DOWN_CE,
> > +	ADF4360_POWER_DOWN_SOFT_SYNC,
> > +	ADF4360_POWER_DOWN_REGULATOR,
> > +};
> > +
> > +enum {
> > +	ADF4360_PRESCALER_8,
> > +	ADF4360_PRESCALER_16,
> > +	ADF4360_PRESCALER_32,
> > +};
> > +
> > +enum {
> > +	ADF4360_ABP_3_0NS,
> > +	ADF4360_ABP_1_3NS,
> > +	ADF4360_ABP_6_0NS,
> > +};
> > +
> > +enum {
> > +	ADF4360_BSC_1,
> > +	ADF4360_BSC_2,
> > +	ADF4360_BSC_4,
> > +	ADF4360_BSC_8,
> > +};
> > +
> > +enum {
> > +	ID_ADF4360_0,
> > +	ID_ADF4360_1,
> > +	ID_ADF4360_2,
> > +	ID_ADF4360_3,
> > +	ID_ADF4360_4,
> > +	ID_ADF4360_5,
> > +	ID_ADF4360_6,
> > +	ID_ADF4360_7,
> > +	ID_ADF4360_8,
> > +	ID_ADF4360_9,
> > +};
> > +
> > +enum {
> > +	ADF4360_FREQ_REFIN,
> > +	ADF4360_MTLD,
> > +	ADF4360_FREQ_PFD,
> > +};
> > +
> > +static const char * const adf4360_power_level_modes[] = {
> This isn't an enum.  These are real values in sensible units not
> magic strings.   Handle them as integers.
> 
> We may need to define additional ABI for this but it should be
> possible to 'fit' it in the normal structure of ABI.
> 
> > +	[ADF4360_PL_3_5] = "3500-uA",
> > +	[ADF4360_PL_5] = "5000-uA",
> > +	[ADF4360_PL_7_5] = "7500-uA",
> > +	[ADF4360_PL_11] = "11000-uA",
> > +};
> > +
> > +static const unsigned int adf4360_power_lvl_microamp[] = {
> > +	[ADF4360_PL_3_5] = 3500,
> > +	[ADF4360_PL_5] = 5000,
> > +	[ADF4360_PL_7_5] = 7500,
> > +	[ADF4360_PL_11] = 11000,
> > +};
> > +
> > +static const unsigned int adf4360_cpi_modes[] = {
> > +	[ADF4360_CPI_0_31] = 310,
> > +	[ADF4360_CPI_0_62] = 620,
> > +	[ADF4360_CPI_0_93] = 930,
> > +	[ADF4360_CPI_1_25] = 1250,
> > +	[ADF4360_CPI_1_56] = 1560,
> > +	[ADF4360_CPI_1_87] = 1870,
> > +	[ADF4360_CPI_2_18] = 2180,
> > +	[ADF4360_CPI_2_50] = 2500,
> > +};
> > +
> > +static const char * const adf4360_muxout_modes[] = {
> Superficially from glancing at the datasheet I thing this is debug
> functionality?  Perhaps move it to debugfs so as to avoid creating
> complex new ABI in sysfs.
> 
> > +	[ADF4360_MUXOUT_THREE_STATE] = "three-state",
> > +	[ADF4360_MUXOUT_LOCK_DETECT] = "lock-detect",
> > +	[ADF4360_MUXOUT_NDIV] = "ndiv",
> > +	[ADF4360_MUXOUT_DVDD] = "dvdd",
> > +	[ADF4360_MUXOUT_RDIV] = "rdiv",
> > +	[ADF4360_MUXOUT_OD_LD] = "od-ld",
> > +	[ADF4360_MUXOUT_SDO] = "sdo",
> > +	[ADF4360_MUXOUT_GND] = "gnd",
> > +};
> > +
> > +static const char * const adf4360_power_down_modes[] = {
> > +	[ADF4360_POWER_DOWN_NORMAL] = "normal",
> > +	[ADF4360_POWER_DOWN_SOFT_ASYNC] = "soft-async",
> > +	[ADF4360_POWER_DOWN_CE] = "ce",
> > +	[ADF4360_POWER_DOWN_SOFT_SYNC] = "soft-sync",
> > +	[ADF4360_POWER_DOWN_REGULATOR] = "regulator",
> This seems to map rather oddly to the datasheet.  Perhaps some
> comments to explain what is going on here would help/
> > +};
> > +
> > +struct adf4360_output {
> > +	struct clk_hw hw;
> > +	struct iio_dev *indio_dev;
> > +};
> > +
> > +#define to_output(_hw) container_of(_hw, struct adf4360_output, hw)
> > +
> > +struct adf4360_chip_info {
> > +	unsigned int vco_min;
> > +	unsigned int vco_max;
> > +	unsigned int default_cpl;
> > +};
> > +
> > +struct adf4360_state {
> > +	struct spi_device *spi;
> > +	const struct adf4360_chip_info *info;
> > +	struct adf4360_output output;
> > +	struct clk *clkin;
> > +	struct gpio_desc *muxout_gpio;
> > +	struct gpio_desc *chip_en_gpio;
> > +	struct regulator *vdd_reg;
> > +	struct mutex lock; /* Protect PLL state. */
> > +	unsigned int part_id;
> > +	unsigned long clkin_freq;
> > +	unsigned long freq_req;
> > +	unsigned long r;
> > +	unsigned long n;
> > +	unsigned int vco_min;
> > +	unsigned int vco_max;
> > +	unsigned int pfd_freq;
> > +	unsigned int cpi;
> > +	bool pdp;
> > +	bool mtld;
> > +	unsigned int power_level;
> > +	unsigned int muxout_mode;
> > +	unsigned int power_down_mode;
> > +	bool initial_reg_seq;
> > +	const char *clk_out_name;
> > +	unsigned int regs[ADF4360_REG_NUM];
> > +	u8 spi_data[3] ____cacheline_aligned;
> > +};
> > +
> > +static const struct adf4360_chip_info adf4360_chip_info_tbl[] = {
> > +	{	/* ADF4360-0 */
> > +		.vco_min = 2400000000U,
> > +		.vco_max = 2725000000U,
> > +		.default_cpl = ADF4360_GEN1_PC_10,
> > +	}, {	/* ADF4360-1 */
> > +		.vco_min = 2050000000U,
> > +		.vco_max = 2450000000U,
> > +		.default_cpl = ADF4360_GEN1_PC_15,
> > +	}, {	/* ADF4360-2 */
> > +		.vco_min = 1850000000U,
> > +		.vco_max = 2170000000U,
> > +		.default_cpl = ADF4360_GEN1_PC_15,
> > +	}, {	/* ADF4360-3 */
> > +		.vco_min = 1600000000U,
> > +		.vco_max = 1950000000U,
> > +		.default_cpl = ADF4360_GEN1_PC_15,
> > +	}, {	/* ADF4360-4 */
> > +		.vco_min = 1450000000U,
> > +		.vco_max = 1750000000U,
> > +		.default_cpl = ADF4360_GEN1_PC_15,
> > +	}, {	/* ADF4360-5 */
> > +		.vco_min = 1200000000U,
> > +		.vco_max = 1400000000U,
> > +		.default_cpl = ADF4360_GEN1_PC_10,
> > +	}, {	/* ADF4360-6 */
> > +		.vco_min = 1050000000U,
> > +		.vco_max = 1250000000U,
> > +		.default_cpl = ADF4360_GEN1_PC_10,
> > +	}, {	/* ADF4360-7 */
> > +		.vco_min = 350000000U,
> > +		.vco_max = 1800000000U,
> > +		.default_cpl = ADF4360_GEN1_PC_5,
> > +	}, {	/* ADF4360-8 */
> > +		.vco_min = 65000000U,
> > +		.vco_max = 400000000U,
> > +		.default_cpl = ADF4360_GEN2_PC_5,
> > +	}, {	/* ADF4360-9 */
> > +		.vco_min = 65000000U,
> > +		.vco_max = 400000000U,
> > +		.default_cpl = ADF4360_GEN2_PC_5,
> > +	}
> > +};
> > +
> > +static int adf4360_write_reg(struct adf4360_state *st, unsigned int reg,
> > +			     unsigned int val)
> > +{
> > +	int ret;
> > +
> > +	val |= reg;
> > +
> > +	st->spi_data[0] = (val >> 16) & 0xff;
> > +	st->spi_data[1] = (val >> 8) & 0xff;
> > +	st->spi_data[2] = val & 0xff;
> > +
> > +	ret = spi_write(st->spi, st->spi_data, ARRAY_SIZE(st->spi_data));
> > +	if (ret == 0)
> > +		st->regs[reg] = val;
> > +
> > +	return ret;
> > +}
> > +
> > +/* fVCO = B * fREFIN / R */
> > +
> > +static unsigned long adf4360_clk_recalc_rate(struct clk_hw *hw,
> > +					     unsigned long parent_rate)
> > +{
> > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +
> > +	if (st->r == 0)
> > +		return 0;
> > +
> > +	/*
> > +	 * The result is guaranteed to fit in 32-bit, but the intermediate
> > +	 * result might require 64-bit.
> > +	 */
> > +	return DIV_ROUND_CLOSEST_ULL((uint64_t)parent_rate * st->n, st->r);
> > +}
> > +
> > +static unsigned int adf4360_calc_prescaler(unsigned int pfd_freq,
> > +					   unsigned int n,
> > +					   unsigned int *out_p,
> > +					   unsigned int *out_a,
> > +					   unsigned int *out_b)
> > +{
> > +	unsigned int rate = pfd_freq * n;
> > +	unsigned int p, a, b;
> > +
> > +	/* Make sure divider counter input frequency is low enough */
> > +	p = 8;
> > +	while (p < 32 && rate / p > ADF4360_MAX_COUNTER_RATE)
> > +		p *= 2;
> > +
> > +	/*
> > +	 * The range of dividers that can be produced using the dual-modulus
> > +	 * pre-scaler is not continuous for values of n < p*(p-1). If we end up
> > +	 * with a non supported divider value, pick the next closest one.
> > +	 */
> > +	a = n % p;
> > +	b = n / p;
> > +
> > +	if (b < 3) {
> > +		b = 3;
> > +		a = 0;
> > +	} else if (a > b) {
> > +		if (a - b < p - a) {
> > +			a = b;
> > +		} else {
> > +			a = 0;
> > +			b++;
> > +		}
> > +	}
> > +
> > +	if (out_p)
> > +		*out_p = p;
> > +	if (out_a)
> > +		*out_a = a;
> > +	if (out_b)
> > +		*out_b = b;
> > +
> > +	return p * b + a;
> > +}
> > +
> > +static long adf4360_clk_round_rate(struct clk_hw *hw,
> > +				   unsigned long rate,
> > +				   unsigned long *parent_rate)
> > +{
> > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +	unsigned int r, n;
> > +	unsigned int pfd_freq;
> > +
> > +	if (*parent_rate == 0)
> > +		return 0;
> > +
> > +	if (st->part_id == ID_ADF4360_9)
> > +		return *parent_rate * st->n / st->r;
> > +
> > +	if (rate > st->vco_max)
> > +		return st->vco_max;
> > +
> > +	/* ADF4360-0 to AD4370-7 have an optional by two divider */
> > +	if (st->part_id <= ID_ADF4360_7) {
> > +		if (rate < st->vco_min / 2)
> > +			return st->vco_min / 2;
> > +		if (rate < st->vco_min && rate > st->vco_max / 2) {
> > +			if (st->vco_min - rate < rate - st->vco_max / 2)
> > +				return st->vco_min;
> > +			else
> > +				return st->vco_max / 2;
> > +		}
> > +	} else {
> > +		if (rate < st->vco_min)
> > +			return st->vco_min;
> > +	}
> > +
> > +	r = DIV_ROUND_CLOSEST(*parent_rate, st->pfd_freq);
> > +	pfd_freq = *parent_rate / r;
> > +	n = DIV_ROUND_CLOSEST(rate, pfd_freq);
> > +
> > +	if (st->part_id <= ID_ADF4360_7)
> > +		n = adf4360_calc_prescaler(pfd_freq, n, NULL, NULL, NULL);
> > +
> > +	return pfd_freq * n;
> > +}
> > +
> > +static inline bool adf4360_is_powerdown(struct adf4360_state *st)
> > +{
> > +	return (st->power_down_mode != ADF4360_POWER_DOWN_NORMAL);
> > +}
> > +
> > +static int adf4360_set_freq(struct adf4360_state *st, unsigned long rate)
> > +{
> > +	unsigned int val_r, val_n, val_ctrl;
> > +	unsigned int pfd_freq;
> > +	unsigned long r, n;
> > +	int ret;
> > +
> > +	if (!st->clkin_freq || (st->clkin_freq > ADF4360_MAX_REFIN_RATE))
> > +		return -EINVAL;
> > +
> > +	if ((rate < st->vco_min) || (rate > st->vco_max))
> > +		return -EINVAL;
> > +
> > +	if (adf4360_is_powerdown(st))
> > +		ret = -EBUSY;
> > +
> > +	r = DIV_ROUND_CLOSEST(st->clkin_freq, st->pfd_freq);
> > +	pfd_freq = st->clkin_freq / r;
> > +	n = DIV_ROUND_CLOSEST(rate, pfd_freq);
> > +
> > +	val_ctrl = ADF4360_CPL(st->info->default_cpl) |
> > +		   ADF4360_MUXOUT(st->muxout_mode) |
> > +		   ADF4360_PDP(!st->pdp) |
> > +		   ADF4360_MTLD(st->mtld) |
> > +		   ADF4360_OPL(st->power_level) |
> > +		   ADF4360_CPI1(st->cpi) |
> > +		   ADF4360_CPI2(st->cpi) |
> > +		   ADF4360_POWERDOWN(st->power_down_mode);
> > +
> > +	/* ADF4360-0 to ADF4360-7 have a dual-modulous prescaler */
> > +	if (st->part_id <= ID_ADF4360_7) {
> > +		unsigned int p, a, b;
> > +
> > +		n = adf4360_calc_prescaler(pfd_freq, n, &p, &a, &b);
> > +
> > +		switch (p) {
> > +		case 8:
> > +			val_ctrl |= ADF4360_PRESCALER(ADF4360_PRESCALER_8);
> > +			break;
> > +		case 16:
> > +			val_ctrl |= ADF4360_PRESCALER(ADF4360_PRESCALER_16);
> > +			break;
> > +		default:
> > +			val_ctrl |= ADF4360_PRESCALER(ADF4360_PRESCALER_32);
> > +			break;
> > +		}
> > +
> > +		val_n = ADF4360_A_COUNTER(a) |
> > +			ADF4360_B_COUNTER(b);
> > +
> > +		if (rate < st->vco_min)
> > +			val_n |= ADF4360_OUT_DIV2(true) |
> > +				 ADF4360_PRESCALER_DIV2(true);
> > +	} else {
> > +		val_n = ADF4360_B_COUNTER(n);
> > +	}
> > +
> > +	/*
> > +	 * Always use BSC divider of 8, see Analog Devices AN-1347.
> > +	 * 
> > http://www.analog.com/media/en/technical-documentation/application-notes/AN-1347.pdf
> > +	 */
> > +	val_r = ADF4360_R_COUNTER(r) |
> > +		ADF4360_ABP(ADF4360_ABP_3_0NS) |
> > +		ADF4360_BSC(ADF4360_BSC_8);
> > +
> > +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_RDIV), val_r);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val_ctrl);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Allow the transient behavior of the ADF4360-7 during initial
> > +	 * power-up to settle.
> > +	 */
> > +	if (st->initial_reg_seq) {
> > +		usleep_range(15000, 20000);
> > +		st->initial_reg_seq = false;
> > +	}
> > +
> > +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_NDIV), val_n);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->freq_req = rate;
> > +	st->n = n;
> > +	st->r = r;
> > +
> > +	return 0;
> > +}
> > +
> > +static int adf4360_clk_set_rate(struct clk_hw *hw,
> > +				unsigned long rate,
> > +				unsigned long parent_rate)
> > +{
> > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	if ((parent_rate == 0) || (parent_rate > ADF4360_MAX_REFIN_RATE))
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&st->lock);
> > +	if (st->clkin_freq != parent_rate)
> > +		st->clkin_freq = parent_rate;
> > +
> > +	ret = adf4360_set_freq(st, rate);
> > +	mutex_unlock(&st->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int __adf4360_power_down(struct adf4360_state *st, unsigned int
> > mode)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	unsigned int val;
> > +	int ret = 0;
> > +
> > +	switch (mode) {
> > +	case ADF4360_POWER_DOWN_NORMAL:
> > +		if (st->vdd_reg) {
> > +			ret = regulator_enable(st->vdd_reg);
> > +			if (ret) {
> > +				dev_err(dev, "Supply enable error: %d\n", ret);
> > +				return ret;
> > +			}
> > +		}
> > +
> > +		st->initial_reg_seq = true;
> > +		st->power_down_mode = mode;
> > +		ret = adf4360_set_freq(st, st->freq_req);
> > +		break;
> > +	case ADF4360_POWER_DOWN_SOFT_ASYNC: /* fallthrough */
> > +	case ADF4360_POWER_DOWN_SOFT_SYNC:
> > +		val = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_MUXOUT_MSK;
> > +		val |= ADF4360_POWERDOWN(mode);
> > +		ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
> > +		break;
> > +	case ADF4360_POWER_DOWN_CE:
> > +		if (st->chip_en_gpio)
> > +			gpiod_set_value(st->chip_en_gpio, 0x0);
> > +		else
> > +			return -ENODEV;
> > +		break;
> > +	case ADF4360_POWER_DOWN_REGULATOR:
> > +		if (!st->vdd_reg)
> > +			return -ENODEV;
> > +
> > +		if (st->chip_en_gpio)
> > +			ret = __adf4360_power_down(st, ADF4360_POWER_DOWN_CE);
> > +		else
> > +			ret = __adf4360_power_down(st,
> > +						ADF4360_POWER_DOWN_SOFT_ASYNC);
> > +
> > +		ret = regulator_disable(st->vdd_reg);
> > +		if (ret)
> > +			dev_err(dev, "Supply disable error: %d\n", ret);
> > +		break;
> > +	}
> > +	if (ret == 0)
> > +		st->power_down_mode = mode;
> > +
> > +	return 0;
> > +}
> > +
> > +static int adf4360_power_down(struct adf4360_state *st, unsigned int mode)
> > +{
> > +	int ret;
> > +
> > +	mutex_lock(&st->lock);
> > +	ret = __adf4360_power_down(st, mode);
> > +	mutex_unlock(&st->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int adf4360_clk_prepare(struct clk_hw *hw)
> > +{
> > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +
> > +	return adf4360_power_down(st, ADF4360_POWER_DOWN_NORMAL);
> > +}
> > +
> > +static void adf4360_clk_unprepare(struct clk_hw *hw)
> > +{
> > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +
> > +	adf4360_power_down(st, ADF4360_POWER_DOWN_SOFT_ASYNC);
> > +}
> > +
> > +static int adf4360_clk_enable(struct clk_hw *hw)
> > +{
> > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +
> > +	if (st->chip_en_gpio)
> > +		gpiod_set_value(st->chip_en_gpio, 0x1);
> > +
> > +	return 0;
> > +}
> > +
> > +static void adf4360_clk_disable(struct clk_hw *hw)
> > +{
> > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +
> > +	if (st->chip_en_gpio)
> > +		adf4360_power_down(st, ADF4360_POWER_DOWN_CE);
> > +}
> > +
> > +static int adf4360_clk_is_enabled(struct clk_hw *hw)
> > +{
> > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +
> > +	return adf4360_is_powerdown(st);
> > +}
> > +
> > +static const struct clk_ops adf4360_clk_ops = {
> > +	.recalc_rate = adf4360_clk_recalc_rate,
> > +	.round_rate = adf4360_clk_round_rate,
> > +	.set_rate = adf4360_clk_set_rate,
> > +	.prepare = adf4360_clk_prepare,
> > +	.unprepare = adf4360_clk_unprepare,
> > +	.enable = adf4360_clk_enable,
> > +	.disable = adf4360_clk_disable,
> > +	.is_enabled = adf4360_clk_is_enabled,
> > +};
> > +
> > +static ssize_t adf4360_read(struct iio_dev *indio_dev,
> > +			    uintptr_t private,
> > +			    const struct iio_chan_spec *chan,
> > +			    char *buf)
> > +{
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +	unsigned long val;
> > +	int ret = 0;
> > +
> > +	switch ((u32)private) {
> > +	case ADF4360_FREQ_REFIN:
> > +		val = st->clkin_freq;
> > +		break;
> > +	case ADF4360_MTLD:
> > +		val = st->mtld;
> > +		break;
> > +	case ADF4360_FREQ_PFD:
> > +		val = st->pfd_freq;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		val = 0;
> > +	}
> > +
> > +	return ret < 0 ? ret : sprintf(buf, "%lu\n", val);
> > +}
> > +
> > +static ssize_t adf4360_write(struct iio_dev *indio_dev,
> > +			     uintptr_t private,
> > +			     const struct iio_chan_spec *chan,
> > +			     const char *buf, size_t len)
> > +{
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +	unsigned long readin, tmp;
> > +	bool mtld;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&st->lock);
> > +	switch ((u32)private) {
> > +	case ADF4360_FREQ_REFIN:
> > +		ret = kstrtoul(buf, 10, &readin);
> > +		if (ret)
> > +			break;
> > +
> > +		if ((readin > ADF4360_MAX_REFIN_RATE) || (readin == 0)) {
> > +			ret = -EINVAL;
> > +			break;
> > +		}
> > +
> > +		if (st->clkin) {
> > +			tmp = clk_round_rate(st->clkin, readin);
> > +			if (tmp != readin) {
> > +				ret = -EINVAL;
> > +				break;
> > +			}
> > +
> > +			ret = clk_set_rate(st->clkin, tmp);
> > +			if (ret)
> > +				break;
> A bit odd to directly provide an interface to control and entirely different
> bit of hardware.   If there are specific demands on the input clock as a
> result
> of something to do with the outputs, then fair enough.  Direct tweaking like
> this seems like a very odd interface.
> 
> > +		}
> > +
> > +		st->clkin_freq = readin;
> > +		break;
> > +	case ADF4360_MTLD:
> > +		ret = kstrtobool(buf, &mtld);
> > +		if (ret)
> > +			break;
> > +
> > +		st->mtld = mtld;
> > +		break;
> > +	case ADF4360_FREQ_PFD:
> > +		ret = kstrtoul(buf, 10, &readin);
> > +		if (ret)
> > +			break;
> > +
> > +		if ((readin > ADF4360_MAX_PFD_RATE) || (readin == 0)) {
> > +			ret = -EINVAL;
> > +			break;
> > +		}
> > +
> > +		st->pfd_freq = readin;
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +	}
> > +
> > +	if (ret == 0)
> > +		ret = adf4360_set_freq(st, st->freq_req);
> > +	mutex_unlock(&st->lock);
> > +
> > +	return ret ? ret : len;
> > +}
> > +
> > +static int adf4360_get_muxout_mode(struct iio_dev *indio_dev,
> > +				   const struct iio_chan_spec *chan)
> > +{
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +
> > +	return st->muxout_mode;
> > +}
> > +
> > +static int adf4360_set_muxout_mode(struct iio_dev *indio_dev,
> > +				   const struct iio_chan_spec *chan,
> > +				   unsigned int mode)
> > +{
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +	unsigned int writeval;
> > +	int ret = 0;
> > +
> > +	mutex_lock(&st->lock);
> > +	writeval = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_MUXOUT_MSK;
> > +	writeval |= ADF4360_MUXOUT(mode & 0x7);
> > +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), writeval);
> > +	if (ret == 0)
> > +		st->muxout_mode = mode & 0x7;
> > +	mutex_unlock(&st->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct iio_enum adf4360_muxout_modes_available = {
> > +	.items = adf4360_muxout_modes,
> > +	.num_items = ARRAY_SIZE(adf4360_muxout_modes),
> > +	.get = adf4360_get_muxout_mode,
> > +	.set = adf4360_set_muxout_mode,
> > +};
> > +
> > +static int adf4360_get_power_down(struct iio_dev *indio_dev,
> > +				  const struct iio_chan_spec *chan)
> > +{
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +
> > +	return st->power_down_mode;
> > +}
> > +
> > +static int adf4360_set_power_down(struct iio_dev *indio_dev,
> > +				  const struct iio_chan_spec *chan,
> > +				  unsigned int mode)
> > +{
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +
> > +	return adf4360_power_down(st, mode);
> > +}
> > +
> > +static const struct iio_enum adf4360_pwr_dwn_modes_available = {
> > +	.items = adf4360_power_down_modes,
> > +	.num_items = ARRAY_SIZE(adf4360_power_down_modes),
> > +	.get = adf4360_get_power_down,
> > +	.set = adf4360_set_power_down,
> > +};
> > +
> > +static int adf4360_get_power_level(struct iio_dev *indio_dev,
> > +				   const struct iio_chan_spec *chan)
> > +{
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +
> > +	return st->power_level;
> > +}
> > +
> > +static int adf4360_set_power_level(struct iio_dev *indio_dev,
> > +				   const struct iio_chan_spec *chan,
> > +				   unsigned int level)
> > +{
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +	unsigned int val;
> > +	int ret;
> > +
> > +	if (adf4360_is_powerdown(st))
> > +		return -EBUSY;
> > +
> > +	mutex_lock(&st->lock);
> > +	val = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_OPL_MSK;
> > +	val |= ADF4360_OPL(level);
> > +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
> > +	st->power_level = level;
> > +	mutex_unlock(&st->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct iio_enum adf4360_pwr_lvl_modes_available = {
> > +	.items = adf4360_power_level_modes,
> > +	.num_items = ARRAY_SIZE(adf4360_power_level_modes),
> > +	.get = adf4360_get_power_level,
> > +	.set = adf4360_set_power_level,
> > +};
> > +
> > +#define _ADF4360_EXT_INFO(_name, _ident) { \
> > +	.name = _name, \
> > +	.read = adf4360_read, \
> > +	.write = adf4360_write, \
> > +	.private = _ident, \
> > +	.shared = IIO_SEPARATE, \
> > +}
> > +
> > +static const struct iio_chan_spec_ext_info adf4360_ext_info[] = {
> 
> This is a wide range of new ABI.  These all need documentation
> in Documentation/ABI/testing/sysfs-bus-iio-*
> 
> Without docs, it's hard to discuss if these are appropriate but a few initial
> comments...
> > +	_ADF4360_EXT_INFO("refin_frequency", ADF4360_FREQ_REFIN),
> Looks like a control that should be matched to some clock source and
> not change at runtime?
> 
> > +	_ADF4360_EXT_INFO("mute_till_lock_detect", ADF4360_MTLD),
> > +	_ADF4360_EXT_INFO("pfd_frequency", ADF4360_FREQ_PFD),
> > +	IIO_ENUM_AVAILABLE("muxout_mode", &adf4360_muxout_modes_available),
> > +	IIO_ENUM("muxout_mode", false, &adf4360_muxout_modes_available),
> > +	IIO_ENUM_AVAILABLE("power_down", &adf4360_pwr_dwn_modes_available),
> > +	IIO_ENUM("power_down", false, &adf4360_pwr_dwn_modes_available),
> > +	IIO_ENUM_AVAILABLE("power_level", &adf4360_pwr_lvl_modes_available),
> > +	IIO_ENUM("power_level", false, &adf4360_pwr_lvl_modes_available),
> > +	{ },
> > +};
> > +
> > +static const struct iio_chan_spec adf4360_chan = {
> > +	.type = IIO_ALTVOLTAGE,
> > +	.indexed = 1,
> > +	.output = 1,
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY),
> > +	.ext_info = adf4360_ext_info,
> > +};
> > +
> > +static int adf4360_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int *val,
> > +			    int *val2,
> > +			    long mask)
> > +{
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +	bool lk_det;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_FREQUENCY:
> > +		if (adf4360_is_powerdown(st))
> > +			return -EBUSY;
> > +
> > +		lk_det = (ADF4360_MUXOUT_LOCK_DETECT | ADF4360_MUXOUT_OD_LD) &
> > +			 st->muxout_mode;
> > +		if (lk_det && st->muxout_gpio) {
> > +			if (!gpiod_get_value(st->muxout_gpio)) {
> > +				dev_dbg(&st->spi->dev, "PLL un-locked\n");
> > +				return -EBUSY;
> > +			}
> > +		}
> > +
> > +		*val = st->freq_req;
> > +		return IIO_VAL_INT;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +};
> > +
> > +static int adf4360_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val,
> > +			     int val2,
> > +			     long mask)
> > +{
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	mutex_lock(&st->lock);
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_FREQUENCY:
> > +		ret = adf4360_set_freq(st, val);
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +	}
> > +	mutex_unlock(&st->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static int adf4360_reg_access(struct iio_dev *indio_dev,
> > +			      unsigned int reg,
> > +			      unsigned int writeval,
> > +			      unsigned int *readval)
> > +{
> > +	struct adf4360_state *st = iio_priv(indio_dev);
> > +	int ret = 0;
> > +
> > +	if (reg >= ADF4360_REG_NUM)
> > +		return -EFAULT;
> > +
> > +	mutex_lock(&st->lock);
> > +	if (readval) {
> > +		*readval = st->regs[reg];
> > +	} else {
> > +		writeval &= 0xFFFFFC;
> > +		ret = adf4360_write_reg(st, reg, writeval);
> > +	}
> > +	mutex_unlock(&st->lock);
> > +
> > +	return ret;
> > +}
> > +
> > +static const struct iio_info adf4360_iio_info = {
> > +	.read_raw = &adf4360_read_raw,
> > +	.write_raw = &adf4360_write_raw,
> > +	.debugfs_reg_access = &adf4360_reg_access,
> > +};
> > +
> > +static int adf4360_get_gpio(struct adf4360_state *st)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	unsigned int val;
> > +	int ret, i;
> > +
> > +	st->chip_en_gpio = devm_gpiod_get_optional(dev, "enable",
> > +						   GPIOD_OUT_HIGH);
> > +	if (IS_ERR(st->chip_en_gpio)) {
> > +		dev_err(dev, "Chip enable GPIO error\n");
> 
> Put handling in here to prevent an error message of DEFER.
> Same for the other routes where this might happen.
> 
> > +		return PTR_ERR(st->chip_en_gpio);
> > +	}
> > +
> > +	if (st->chip_en_gpio)
> > +		st->power_down_mode = ADF4360_POWER_DOWN_CE;
> > +
> > +	st->muxout_gpio = devm_gpiod_get_optional(dev, "adi,muxout", GPIOD_IN);
> > +	if (IS_ERR(st->muxout_gpio)) {
> > +		dev_err(dev, "Muxout GPIO error\n");
> > +		return PTR_ERR(st->muxout_gpio);
> > +	}
> > +
> > +	if (!st->muxout_gpio)
> > +		return 0;
> > +
> > +	/* ADF4360 PLLs are write only devices, try to probe using GPIO. */
> > +	for (i = 0; i < 4; i++) {
> > +		if (i & 1)
> > +			val = ADF4360_MUXOUT(ADF4360_MUXOUT_DVDD);
> > +		else
> > +			val = ADF4360_MUXOUT(ADF4360_MUXOUT_GND);
> > +
> > +		ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
> > +		if (ret)
> > +			return ret;
> > +
> > +		ret = gpiod_get_value(st->muxout_gpio);
> > +		if (ret ^ (i & 1)) {
> > +			dev_err(dev, "Probe failed (muxout)");
> > +			return -ENODEV;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void adf4360_clkin_disable(void *data)
> > +{
> > +	struct adf4360_state *st = data;
> > +
> > +	clk_disable_unprepare(st->clkin);
> > +}
> > +
> > +static int adf4360_get_clkin(struct adf4360_state *st)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	struct clk *clk;
> > +	int ret;
> > +
> > +	clk = devm_clk_get(dev, "clkin");
> > +	if (IS_ERR(clk))
> > +		return PTR_ERR(clk);
> > +
> > +	ret = clk_prepare_enable(clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = devm_add_action_or_reset(dev, adf4360_clkin_disable, st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->clkin = clk;
> > +	st->clkin_freq = clk_get_rate(clk);
> > +
> > +	return 0;
> > +}
> > +
> > +static void adf4360_clk_del_provider(void *data)
> > +{
> > +	struct adf4360_state *st = data;
> > +
> > +	of_clk_del_provider(st->spi->dev.of_node);
> > +}
> > +
> > +static int adf4360_clk_register(struct adf4360_state *st)
> > +{
> 
> Hmm. This makes me wonder why this is an IIO driver rather than a clk
> driver?  Definitely needs some more information in the patch description
> or a cover letter.
> 
> > +	struct spi_device *spi = st->spi;
> > +	struct clk_init_data init;
> > +	struct clk *clk;
> > +	const char *parent_name;
> > +	int ret;
> > +
> > +	parent_name = of_clk_get_parent_name(spi->dev.of_node, 0);
> > +	if (!parent_name)
> > +		return -EINVAL;
> > +
> > +	init.name = st->clk_out_name;
> > +	init.ops = &adf4360_clk_ops;
> > +	init.flags = CLK_SET_RATE_GATE;
> > +	init.parent_names = &parent_name;
> > +	init.num_parents = 1;
> > +
> > +	st->output.hw.init = &init;
> > +
> > +	clk = devm_clk_register(&spi->dev, &st->output.hw);
> > +	if (IS_ERR(clk))
> > +		return PTR_ERR(clk);
> > +
> > +	ret = of_clk_add_provider(spi->dev.of_node, of_clk_src_simple_get, clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_add_action_or_reset(&spi->dev, adf4360_clk_del_provider,
> > st);
> > +}
> > +
> > +static int adf4360_parse_dt(struct adf4360_state *st)
> > +{
> > +	struct device *dev = &st->spi->dev;
> > +	u32 tmp;
> > +	int ret;
> > +
> > +	ret = device_property_read_string(dev, "clock-output-names",
> > +					  &st->clk_out_name);
> > +	if ((ret < 0) && dev->of_node)
> > +		st->clk_out_name = dev->of_node->name;
> > +
> > +	if (st->part_id >= ID_ADF4360_7) {
> > +		/*
> > +		 * ADF4360-7 to ADF4360-9 have a VCO that is tuned to a specific
> > +		 * range using an external inductor. These properties describe
> > +		 * the range selected by the external inductor.
> > +		 */
> > +		ret = device_property_read_u32(dev,
> > +					       "adi,vco-minimum-frequency-hz",
> > +					       &tmp);
> > +		if (ret == 0)
> > +			st->vco_min = max(st->info->vco_min, tmp);
> > +		else
> > +			st->vco_min = st->info->vco_min;
> > +
> > +		ret = device_property_read_u32(dev,
> > +					       "adi,vco-maximum-frequency-hz",
> > +					       &tmp);
> > +		if (ret == 0)
> > +			st->vco_max = min(st->info->vco_max, tmp);
> > +		else
> > +			st->vco_max = st->info->vco_max;
> > +	} else {
> > +		st->vco_min = st->info->vco_min;
> > +		st->vco_max = st->info->vco_max;
> > +	}
> > +
> > +	st->pdp = device_property_read_bool(dev, "adi,loop-filter-inverting");
> > +
> > +	ret = device_property_read_u32(dev,
> > +				       "adi,loop-filter-pfd-frequency-hz",
> > +				       &tmp);
> > +	if (ret == 0) {
> > +		st->pfd_freq = tmp;
> > +	} else {
> > +		dev_err(dev, "PFD frequency property missing\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = device_property_read_u32(dev,
> > +				"adi,loop-filter-charge-pump-current-microamp",
> > +				&tmp);
> > +	if (ret == 0) {
> > +		st->cpi = find_closest(tmp, adf4360_cpi_modes,
> > +				       ARRAY_SIZE(adf4360_cpi_modes));
> > +	} else {
> > +		dev_err(dev, "CPI property missing\n");
> > +		return ret;
> > +	}
> > +
> > +	ret = device_property_read_u32(dev, "adi,power-up-frequency-hz", &tmp);
> > +	if (ret == 0)
> > +		st->freq_req = tmp;
> > +	else
> > +		st->freq_req = st->vco_min;
> > +
> > +	ret = device_property_read_u32(dev, "adi,power-out-level-microamp",
> > +				       &tmp);
> > +	if (ret == 0)
> > +		st->power_level = find_closest(tmp, adf4360_power_lvl_microamp,
> > +					ARRAY_SIZE(adf4360_power_lvl_microamp));
> > +	else
> > +		st->power_level = ADF4360_PL_5;
> > +
> > +	return 0;
> > +}
> > +
> > +static int adf4360_probe(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	const struct spi_device_id *id = spi_get_device_id(spi);
> 
> Given we require various dt parameters to be present, might as well
> associate the id with the of_ structures instead and use the dt
> calls throughout.  Even better if you use the firmware type independent
> versions.
> 
> > +	struct adf4360_state *st;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
> > +
> > +	st = iio_priv(indio_dev);
> > +
> > +	mutex_init(&st->lock);
> > +
> > +	spi_set_drvdata(spi, indio_dev);
> > +
> > +	st->spi = spi;
> > +	st->info = &adf4360_chip_info_tbl[id->driver_data];
> > +	st->part_id = id->driver_data;
> > +	st->muxout_mode = ADF4360_MUXOUT_LOCK_DETECT;
> > +	st->mtld = true;
> > +
> > +	ret = adf4360_parse_dt(st);
> > +	if (ret) {
> > +		dev_err(&spi->dev, "Parsing properties failed (%d)\n", ret);
> > +		return -ENODEV;
> > +	}
> > +
> > +	indio_dev->dev.parent = &spi->dev;
> > +
> > +	if (spi->dev.of_node)
> > +		indio_dev->name = spi->dev.of_node->name;
> > +	else
> > +		indio_dev->name = spi_get_device_id(spi)->name;
> > +
> > +	indio_dev->info = &adf4360_iio_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->channels = &adf4360_chan;
> > +	indio_dev->num_channels = 1;
> > +	st->output.indio_dev = indio_dev;
> > +
> > +	ret = adf4360_get_gpio(st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = adf4360_get_clkin(st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	st->vdd_reg = devm_regulator_get_optional(&spi->dev, "vdd");
> > +	if (IS_ERR(st->vdd_reg)) {
> > +		if (PTR_ERR(st->vdd_reg) != -ENODEV) {
> > +			dev_err(&spi->dev, "Regulator error\n");
> > +			return PTR_ERR(st->vdd_reg);
> > +		}
> > +
> > +		st->vdd_reg = NULL;
> > +	}
> > +
> > +	ret = adf4360_power_down(st, ADF4360_POWER_DOWN_NORMAL);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = adf4360_clk_register(st);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return devm_iio_device_register(&spi->dev, indio_dev);
> > +}
> > +
> > +static const struct of_device_id adf4360_of_match[] = {
> > +	{ .compatible = "adi,adf4360-0", },
> > +	{ .compatible = "adi,adf4360-1", },
> > +	{ .compatible = "adi,adf4360-2", },
> > +	{ .compatible = "adi,adf4360-3", },
> > +	{ .compatible = "adi,adf4360-4", },
> > +	{ .compatible = "adi,adf4360-5", },
> > +	{ .compatible = "adi,adf4360-6", },
> > +	{ .compatible = "adi,adf4360-7", },
> > +	{ .compatible = "adi,adf4360-8", },
> > +	{ .compatible = "adi,adf4360-9", },
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, adf4360_of_match);
> > +
> > +static const struct spi_device_id adf4360_id[] = {
> 
> As mentioned above, you can't actually probe this device
> without a pile of dt stuff.  So this fallback doesn't
> make much sense.  Use the data field in the of table
> above and get rid of this table entirely.
> 
> > +	{"adf4360-0", ID_ADF4360_0},
> > +	{"adf4360-1", ID_ADF4360_1},
> > +	{"adf4360-2", ID_ADF4360_2},
> > +	{"adf4360-3", ID_ADF4360_3},
> > +	{"adf4360-4", ID_ADF4360_4},
> > +	{"adf4360-5", ID_ADF4360_5},
> > +	{"adf4360-6", ID_ADF4360_6},
> > +	{"adf4360-7", ID_ADF4360_7},
> > +	{"adf4360-8", ID_ADF4360_8},
> > +	{"adf4360-9", ID_ADF4360_9},
> > +	{}
> > +};
> > +
> > +static struct spi_driver adf4360_driver = {
> > +	.driver = {
> > +		.name = "adf4360",
> > +		.of_match_table = adf4360_of_match,
> > +		.owner = THIS_MODULE,
> 
> It's a long time since we had to set the .owner field for each driver.
> 
> Follow through what happens in module_spi_driver, spi_register_driver
> etc and you'll find it's set automatically during driver registration.
> 
> > +	},
> > +	.probe = adf4360_probe,
> > +	.id_table = adf4360_id,
> > +};
> > +module_spi_driver(adf4360_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
> > +MODULE_AUTHOR("Edward Kigwana <ekigwana@gmail.com>");
> > +MODULE_DESCRIPTION("Analog Devices ADF4360 PLL");
Jonathan Cameron Feb. 3, 2020, 11:27 a.m. UTC | #3
On Mon, 3 Feb 2020 11:18:51 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@analog.com> wrote:

> On Sun, 2020-02-02 at 14:45 +0000, Jonathan Cameron wrote:
> > [External]
> > 
> > On Tue, 28 Jan 2020 13:13:00 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:
> >   
> > > From: Edward Kigwana <ekigwana@gmail.com>
> > > 
> > > The ADF4360-N (N is 0..9) are a family of integrated integer-N synthesizer
> > > and voltage controlled oscillator (VCO).
> > > 
> > > Control of all the on-chip registers is through a simple 3-wire SPI
> > > interface. The device operates with a power supply ranging from 3.0 V to
> > > 3.6 V and can be powered down when not in use.
> > > 
> > > The initial draft-work was done by Lars-Peter Clausen <lars@metafoo.de>
> > > The current/latest/complete version was implemented by
> > > Edward Kigwana <ekigwana@gmail.com>.
> > > 
> > > The ADF4360-9 is also present on the Analog Devices 'Advanced Active
> > > Learning Module 2000' (ADALM-2000).
> > > 
> > > Link: 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-0.pdf
> > > Link: 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-1.pdf
> > > Link: 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-2.pdf
> > > Link: 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-3.pdf
> > > Link: 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-4.pdf
> > > Link: 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-5.pdf
> > > Link: 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-6.pdf
> > > Link: 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-7.pdf
> > > Link: 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-8.pdf
> > > Link: 
> > > https://www.analog.com/media/en/technical-documentation/data-sheets/ADF4360-9.pdf
> > > Link: 
> > > https://www.analog.com/en/design-center/evaluation-hardware-and-software/evaluation-boards-kits/ADALM2000.html
> > > 
> > > Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> > > Signed-off-by: Edward Kigwana <ekigwana@gmail.com>
> > > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>  
> > 
> > Superficially this looks like you really have a clock source driver.
> > You then wrap it in IIO in order to provide convenient userspace interfaces.
> > 
> > I'm not sure we want to do that and I definitely need agreement from those
> > responsible for clock source drivers before I take anything that does do
> > this sort of combination of the two types of driver.
> >   
> 
> I'll admit that I am a bit fuzzy about these frequency-generators/clock-
> sources/synthesizer chips.
> In the sense, I don't know where to draw the line between when to consider it
> [primarly] an IIO device [for the IIO subsystem] or when to consider it a clock-
> device [primarily, for the clk subsystem].
> Obviously there's an inertia [for me at least] to go IIO, as I know it a bit
> better. 
> 
> We've had some quick/short discussions [internally] about this driver, and also
> about the LTC6952. We didn't have a bigger one, where we try to discuss them
> more in-detail; but we do have a plan to do it.
> 
> So, then maybe [until then] a question: how do we decide this? [generally
> speaking, not just adf4360 & ltc6952]
> i.e. when to consider it clk-first or iio-first;
> I'm not sure if there is a clear-cut rule, but maybe some guidelines/thoughts?
> Obviously, I'll have to read-up more on the clk framework code [as well] to get
> a feel for it.
> 
> We've done some things internally [up until now] with some of these clock-chips
> that's been mostly IIO-centric. Now, much of it may not be correct, but it is
> what we use as template when writing new driver, which [of course] is not good.
> And, I also don't want to push/force our mistakes upstream, because that is
> [well...] bad. Hence this/these question(s).

Clocks aren't an area I know much about either.   I'd suggest an RFC with
a description of the types of devices and some examples.  Send that to
linux-iio and linux-clk and see what responses you get.

Make sure to highlight the fuzzy boundary when we get into waveform generators
etc rather than straight forward clocks. + include some of the devices that
definitely aren't suitable for use clocking normal SoCs etc.

Will be interesting to see where this one goes.

Jonathan


> 
> Thanks
> Alex
> 
> > I can see that, for these high speed devices, the intent is normally to
> > provide
> > an input signal to some external circuitry rather than some internal clock
> > signal, but given this is registered as a clock source the argument that it is
> > somehow special doesn't seem to hold.
> > 
> > A few more specific comments inline.
> > 
> > Thanks,
> > 
> > Jonathan
> >  
> > Jonathan
> >   
> > > ---
> > > 
> > > Changelog v2 -> v3:
> > > * dropped patch about adf4371.yaml; added by accident since it was used to
> > >   compare against
> > > * addressed Rob's review comments for DT schema
> > > 
> > > Changelog v1 -> v2:
> > > * validate dt-bindings file with
> > >   make dt_binding_check
> > > DT_SCHEMA_FILES=Documentation/devicetree/bindings/iio/frequency/adi,adf4360.
> > > yaml
> > > 
> > >  drivers/iio/frequency/Kconfig   |   11 +
> > >  drivers/iio/frequency/Makefile  |    1 +
> > >  drivers/iio/frequency/adf4360.c | 1263 +++++++++++++++++++++++++++++++
> > >  3 files changed, 1275 insertions(+)
> > >  create mode 100644 drivers/iio/frequency/adf4360.c
> > > 
> > > diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
> > > index 240b81502512..781236496661 100644
> > > --- a/drivers/iio/frequency/Kconfig
> > > +++ b/drivers/iio/frequency/Kconfig
> > > @@ -39,6 +39,17 @@ config ADF4350
> > >  	  To compile this driver as a module, choose M here: the
> > >  	  module will be called adf4350.
> > >  
> > > +config ADF4360
> > > +	tristate "Analog Devices ADF4360 Wideband Synthesizers"
> > > +	depends on SPI
> > > +	depends on COMMON_CLK
> > > +	help
> > > +	  Say yes here to build support for Analog Devices ADF4360
> > > +	  Wideband Synthesizers. The driver provides direct access via sysfs.
> > > +
> > > +	  To compile this driver as a module, choose M here: the
> > > +	  module will be called adf4360.
> > > +
> > >  config ADF4371
> > >  	tristate "Analog Devices ADF4371/ADF4372 Wideband Synthesizers"
> > >  	depends on SPI
> > > diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
> > > index 518b1e50caef..85f2f81da662 100644
> > > --- a/drivers/iio/frequency/Makefile
> > > +++ b/drivers/iio/frequency/Makefile
> > > @@ -6,4 +6,5 @@
> > >  # When adding new entries keep the list in alphabetical order
> > >  obj-$(CONFIG_AD9523) += ad9523.o
> > >  obj-$(CONFIG_ADF4350) += adf4350.o
> > > +obj-$(CONFIG_ADF4360) += adf4360.o
> > >  obj-$(CONFIG_ADF4371) += adf4371.o
> > > diff --git a/drivers/iio/frequency/adf4360.c
> > > b/drivers/iio/frequency/adf4360.c
> > > new file mode 100644
> > > index 000000000000..49ad58092171
> > > --- /dev/null
> > > +++ b/drivers/iio/frequency/adf4360.c
> > > @@ -0,0 +1,1263 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/*
> > > + * ADF4360 PLL with Integrated Synthesizer and VCO
> > > + *
> > > + * Copyright 2014-2019 Analog Devices Inc.
> > > + * Copyright 2019-2020 Edward Kigwana.
> > > + */
> > > +
> > > +#include <linux/bitfield.h>
> > > +#include <linux/clk.h>
> > > +#include <linux/clk-provider.h>
> > > +#include <linux/delay.h>
> > > +#include <linux/device.h>
> > > +#include <linux/err.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/util_macros.h>
> > > +
> > > +#include <linux/iio/iio.h>
> > > +
> > > +/* Register address macro */
> > > +#define ADF4360_REG(x)			(x)
> > > +
> > > +/* ADF4360_CTRL */
> > > +#define ADF4360_ADDR_CPL_MSK		GENMASK(3, 2)
> > > +#define ADF4360_CPL(x)			FIELD_PREP(ADF4360_ADDR_CPL_MSK,
> > > x)
> > > +#define ADF4360_ADDR_MUXOUT_MSK		GENMASK(7, 5)
> > > +#define ADF4360_MUXOUT(x)		FIELD_PREP(ADF4360_ADDR_MUXOUT_MSK, x)
> > > +#define ADF4360_ADDR_PDP_MSK		BIT(8)
> > > +#define ADF4360_PDP(x)			FIELD_PREP(ADF4360_ADDR_PDP_MSK,
> > > x)
> > > +#define ADF4360_ADDR_MTLD_MSK		BIT(11)
> > > +#define ADF4360_MTLD(x)			FIELD_PREP(ADF4360_ADDR_MTLD_MSK
> > > , x)
> > > +#define ADF4360_ADDR_OPL_MSK		GENMASK(13, 12)
> > > +#define ADF4360_OPL(x)			FIELD_PREP(ADF4360_ADDR_OPL_MSK,
> > > x)
> > > +#define ADF4360_ADDR_CPI1_MSK		GENMASK(16, 14)
> > > +#define ADF4360_CPI1(x)			FIELD_PREP(ADF4360_ADDR_CPI1_MSK
> > > , x)
> > > +#define ADF4360_ADDR_CPI2_MSK		GENMASK(19, 17)
> > > +/* ADF4360_RDIV */
> > > +#define ADF4360_CPI2(x)			FIELD_PREP(ADF4360_ADDR_CPI2_MSK
> > > , x)
> > > +#define ADF4360_ADDR_PWR_DWN_MSK	GENMASK(21, 20)
> > > +#define ADF4360_POWERDOWN(x)		FIELD_PREP(ADF4360_ADDR_PWR_DWN_
> > > MSK, x)
> > > +#define ADF4360_ADDR_PRESCALER_MSK	GENMASK(23, 22)
> > > +#define ADF4360_PRESCALER(x)		FIELD_PREP(ADF4360_ADDR_PRESCALE
> > > R_MSK, x)
> > > +
> > > +/* ADF4360_NDIV */
> > > +#define ADF4360_ADDR_A_CNTR_MSK		GENMASK(6, 2)
> > > +#define ADF4360_A_COUNTER(x)		FIELD_PREP(ADF4360_ADDR_A_CNTR_M
> > > SK, x)
> > > +#define ADF4360_ADDR_B_CNTR_MSK		GENMASK(20, 8)
> > > +#define ADF4360_B_COUNTER(x)		FIELD_PREP(ADF4360_ADDR_B_CNTR_M
> > > SK, x)
> > > +#define ADF4360_ADDR_OUT_DIV2_MSK	BIT(22)
> > > +#define ADF4360_OUT_DIV2(x)		FIELD_PREP(ADF4360_ADDR_OUT_DIV2
> > > _MSK, x)
> > > +#define ADF4360_ADDR_DIV2_SEL_MSK	BIT(23)
> > > +#define ADF4360_PRESCALER_DIV2(x)	FIELD_PREP(ADF4360_ADDR_DIV2_SEL_MSK, x)
> > > +
> > > +#define ADF4360_ADDR_R_CNTR_MSK		GENMASK(15, 2)
> > > +#define ADF4360_R_COUNTER(x)		FIELD_PREP(ADF4360_ADDR_R_CNTR_M
> > > SK, x)
> > > +#define ADF4360_ADDR_ABP_MSK		GENMASK(17, 16)
> > > +#define ADF4360_ABP(x)			FIELD_PREP(ADF4360_ADDR_ABP_MSK,
> > > x)
> > > +#define ADF4360_ADDR_BSC_MSK		GENMASK(21, 20)
> > > +#define ADF4360_BSC(x)			FIELD_PREP(ADF4360_ADDR_BSC_MSK,
> > > x)
> > > +
> > > +/* Specifications */
> > > +#define ADF4360_MAX_PFD_RATE		8000000 /* 8 MHz */
> > > +#define ADF4360_MAX_COUNTER_RATE	300000000 /* 300 MHz */
> > > +#define ADF4360_MAX_REFIN_RATE		250000000 /* 250 MHz */
> > > +
> > > +enum {
> > > +	ADF4360_CTRL,
> > > +	ADF4360_RDIV,
> > > +	ADF4360_NDIV,
> > > +	ADF4360_REG_NUM,
> > > +};
> > > +
> > > +enum {
> > > +	ADF4360_GEN1_PC_5,
> > > +	ADF4360_GEN1_PC_10,
> > > +	ADF4360_GEN1_PC_15,
> > > +	ADF4360_GEN1_PC_20,
> > > +};
> > > +
> > > +enum {
> > > +	ADF4360_GEN2_PC_2_5,
> > > +	ADF4360_GEN2_PC_5,
> > > +	ADF4360_GEN2_PC_7_5,
> > > +	ADF4360_GEN2_PC_10,
> > > +};
> > > +
> > > +enum {
> > > +	ADF4360_MUXOUT_THREE_STATE,
> > > +	ADF4360_MUXOUT_LOCK_DETECT,
> > > +	ADF4360_MUXOUT_NDIV,
> > > +	ADF4360_MUXOUT_DVDD,
> > > +	ADF4360_MUXOUT_RDIV,
> > > +	ADF4360_MUXOUT_OD_LD,
> > > +	ADF4360_MUXOUT_SDO,
> > > +	ADF4360_MUXOUT_GND,
> > > +};
> > > +
> > > +enum {
> > > +	ADF4360_PL_3_5,
> > > +	ADF4360_PL_5,
> > > +	ADF4360_PL_7_5,
> > > +	ADF4360_PL_11,
> > > +};
> > > +
> > > +enum {
> > > +	ADF4360_CPI_0_31,
> > > +	ADF4360_CPI_0_62,
> > > +	ADF4360_CPI_0_93,
> > > +	ADF4360_CPI_1_25,
> > > +	ADF4360_CPI_1_56,
> > > +	ADF4360_CPI_1_87,
> > > +	ADF4360_CPI_2_18,
> > > +	ADF4360_CPI_2_50,
> > > +};
> > > +
> > > +enum {
> > > +	ADF4360_POWER_DOWN_NORMAL,
> > > +	ADF4360_POWER_DOWN_SOFT_ASYNC,
> > > +	ADF4360_POWER_DOWN_CE,
> > > +	ADF4360_POWER_DOWN_SOFT_SYNC,
> > > +	ADF4360_POWER_DOWN_REGULATOR,
> > > +};
> > > +
> > > +enum {
> > > +	ADF4360_PRESCALER_8,
> > > +	ADF4360_PRESCALER_16,
> > > +	ADF4360_PRESCALER_32,
> > > +};
> > > +
> > > +enum {
> > > +	ADF4360_ABP_3_0NS,
> > > +	ADF4360_ABP_1_3NS,
> > > +	ADF4360_ABP_6_0NS,
> > > +};
> > > +
> > > +enum {
> > > +	ADF4360_BSC_1,
> > > +	ADF4360_BSC_2,
> > > +	ADF4360_BSC_4,
> > > +	ADF4360_BSC_8,
> > > +};
> > > +
> > > +enum {
> > > +	ID_ADF4360_0,
> > > +	ID_ADF4360_1,
> > > +	ID_ADF4360_2,
> > > +	ID_ADF4360_3,
> > > +	ID_ADF4360_4,
> > > +	ID_ADF4360_5,
> > > +	ID_ADF4360_6,
> > > +	ID_ADF4360_7,
> > > +	ID_ADF4360_8,
> > > +	ID_ADF4360_9,
> > > +};
> > > +
> > > +enum {
> > > +	ADF4360_FREQ_REFIN,
> > > +	ADF4360_MTLD,
> > > +	ADF4360_FREQ_PFD,
> > > +};
> > > +
> > > +static const char * const adf4360_power_level_modes[] = {  
> > This isn't an enum.  These are real values in sensible units not
> > magic strings.   Handle them as integers.
> > 
> > We may need to define additional ABI for this but it should be
> > possible to 'fit' it in the normal structure of ABI.
> >   
> > > +	[ADF4360_PL_3_5] = "3500-uA",
> > > +	[ADF4360_PL_5] = "5000-uA",
> > > +	[ADF4360_PL_7_5] = "7500-uA",
> > > +	[ADF4360_PL_11] = "11000-uA",
> > > +};
> > > +
> > > +static const unsigned int adf4360_power_lvl_microamp[] = {
> > > +	[ADF4360_PL_3_5] = 3500,
> > > +	[ADF4360_PL_5] = 5000,
> > > +	[ADF4360_PL_7_5] = 7500,
> > > +	[ADF4360_PL_11] = 11000,
> > > +};
> > > +
> > > +static const unsigned int adf4360_cpi_modes[] = {
> > > +	[ADF4360_CPI_0_31] = 310,
> > > +	[ADF4360_CPI_0_62] = 620,
> > > +	[ADF4360_CPI_0_93] = 930,
> > > +	[ADF4360_CPI_1_25] = 1250,
> > > +	[ADF4360_CPI_1_56] = 1560,
> > > +	[ADF4360_CPI_1_87] = 1870,
> > > +	[ADF4360_CPI_2_18] = 2180,
> > > +	[ADF4360_CPI_2_50] = 2500,
> > > +};
> > > +
> > > +static const char * const adf4360_muxout_modes[] = {  
> > Superficially from glancing at the datasheet I thing this is debug
> > functionality?  Perhaps move it to debugfs so as to avoid creating
> > complex new ABI in sysfs.
> >   
> > > +	[ADF4360_MUXOUT_THREE_STATE] = "three-state",
> > > +	[ADF4360_MUXOUT_LOCK_DETECT] = "lock-detect",
> > > +	[ADF4360_MUXOUT_NDIV] = "ndiv",
> > > +	[ADF4360_MUXOUT_DVDD] = "dvdd",
> > > +	[ADF4360_MUXOUT_RDIV] = "rdiv",
> > > +	[ADF4360_MUXOUT_OD_LD] = "od-ld",
> > > +	[ADF4360_MUXOUT_SDO] = "sdo",
> > > +	[ADF4360_MUXOUT_GND] = "gnd",
> > > +};
> > > +
> > > +static const char * const adf4360_power_down_modes[] = {
> > > +	[ADF4360_POWER_DOWN_NORMAL] = "normal",
> > > +	[ADF4360_POWER_DOWN_SOFT_ASYNC] = "soft-async",
> > > +	[ADF4360_POWER_DOWN_CE] = "ce",
> > > +	[ADF4360_POWER_DOWN_SOFT_SYNC] = "soft-sync",
> > > +	[ADF4360_POWER_DOWN_REGULATOR] = "regulator",  
> > This seems to map rather oddly to the datasheet.  Perhaps some
> > comments to explain what is going on here would help/  
> > > +};
> > > +
> > > +struct adf4360_output {
> > > +	struct clk_hw hw;
> > > +	struct iio_dev *indio_dev;
> > > +};
> > > +
> > > +#define to_output(_hw) container_of(_hw, struct adf4360_output, hw)
> > > +
> > > +struct adf4360_chip_info {
> > > +	unsigned int vco_min;
> > > +	unsigned int vco_max;
> > > +	unsigned int default_cpl;
> > > +};
> > > +
> > > +struct adf4360_state {
> > > +	struct spi_device *spi;
> > > +	const struct adf4360_chip_info *info;
> > > +	struct adf4360_output output;
> > > +	struct clk *clkin;
> > > +	struct gpio_desc *muxout_gpio;
> > > +	struct gpio_desc *chip_en_gpio;
> > > +	struct regulator *vdd_reg;
> > > +	struct mutex lock; /* Protect PLL state. */
> > > +	unsigned int part_id;
> > > +	unsigned long clkin_freq;
> > > +	unsigned long freq_req;
> > > +	unsigned long r;
> > > +	unsigned long n;
> > > +	unsigned int vco_min;
> > > +	unsigned int vco_max;
> > > +	unsigned int pfd_freq;
> > > +	unsigned int cpi;
> > > +	bool pdp;
> > > +	bool mtld;
> > > +	unsigned int power_level;
> > > +	unsigned int muxout_mode;
> > > +	unsigned int power_down_mode;
> > > +	bool initial_reg_seq;
> > > +	const char *clk_out_name;
> > > +	unsigned int regs[ADF4360_REG_NUM];
> > > +	u8 spi_data[3] ____cacheline_aligned;
> > > +};
> > > +
> > > +static const struct adf4360_chip_info adf4360_chip_info_tbl[] = {
> > > +	{	/* ADF4360-0 */
> > > +		.vco_min = 2400000000U,
> > > +		.vco_max = 2725000000U,
> > > +		.default_cpl = ADF4360_GEN1_PC_10,
> > > +	}, {	/* ADF4360-1 */
> > > +		.vco_min = 2050000000U,
> > > +		.vco_max = 2450000000U,
> > > +		.default_cpl = ADF4360_GEN1_PC_15,
> > > +	}, {	/* ADF4360-2 */
> > > +		.vco_min = 1850000000U,
> > > +		.vco_max = 2170000000U,
> > > +		.default_cpl = ADF4360_GEN1_PC_15,
> > > +	}, {	/* ADF4360-3 */
> > > +		.vco_min = 1600000000U,
> > > +		.vco_max = 1950000000U,
> > > +		.default_cpl = ADF4360_GEN1_PC_15,
> > > +	}, {	/* ADF4360-4 */
> > > +		.vco_min = 1450000000U,
> > > +		.vco_max = 1750000000U,
> > > +		.default_cpl = ADF4360_GEN1_PC_15,
> > > +	}, {	/* ADF4360-5 */
> > > +		.vco_min = 1200000000U,
> > > +		.vco_max = 1400000000U,
> > > +		.default_cpl = ADF4360_GEN1_PC_10,
> > > +	}, {	/* ADF4360-6 */
> > > +		.vco_min = 1050000000U,
> > > +		.vco_max = 1250000000U,
> > > +		.default_cpl = ADF4360_GEN1_PC_10,
> > > +	}, {	/* ADF4360-7 */
> > > +		.vco_min = 350000000U,
> > > +		.vco_max = 1800000000U,
> > > +		.default_cpl = ADF4360_GEN1_PC_5,
> > > +	}, {	/* ADF4360-8 */
> > > +		.vco_min = 65000000U,
> > > +		.vco_max = 400000000U,
> > > +		.default_cpl = ADF4360_GEN2_PC_5,
> > > +	}, {	/* ADF4360-9 */
> > > +		.vco_min = 65000000U,
> > > +		.vco_max = 400000000U,
> > > +		.default_cpl = ADF4360_GEN2_PC_5,
> > > +	}
> > > +};
> > > +
> > > +static int adf4360_write_reg(struct adf4360_state *st, unsigned int reg,
> > > +			     unsigned int val)
> > > +{
> > > +	int ret;
> > > +
> > > +	val |= reg;
> > > +
> > > +	st->spi_data[0] = (val >> 16) & 0xff;
> > > +	st->spi_data[1] = (val >> 8) & 0xff;
> > > +	st->spi_data[2] = val & 0xff;
> > > +
> > > +	ret = spi_write(st->spi, st->spi_data, ARRAY_SIZE(st->spi_data));
> > > +	if (ret == 0)
> > > +		st->regs[reg] = val;
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +/* fVCO = B * fREFIN / R */
> > > +
> > > +static unsigned long adf4360_clk_recalc_rate(struct clk_hw *hw,
> > > +					     unsigned long parent_rate)
> > > +{
> > > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +
> > > +	if (st->r == 0)
> > > +		return 0;
> > > +
> > > +	/*
> > > +	 * The result is guaranteed to fit in 32-bit, but the intermediate
> > > +	 * result might require 64-bit.
> > > +	 */
> > > +	return DIV_ROUND_CLOSEST_ULL((uint64_t)parent_rate * st->n, st->r);
> > > +}
> > > +
> > > +static unsigned int adf4360_calc_prescaler(unsigned int pfd_freq,
> > > +					   unsigned int n,
> > > +					   unsigned int *out_p,
> > > +					   unsigned int *out_a,
> > > +					   unsigned int *out_b)
> > > +{
> > > +	unsigned int rate = pfd_freq * n;
> > > +	unsigned int p, a, b;
> > > +
> > > +	/* Make sure divider counter input frequency is low enough */
> > > +	p = 8;
> > > +	while (p < 32 && rate / p > ADF4360_MAX_COUNTER_RATE)
> > > +		p *= 2;
> > > +
> > > +	/*
> > > +	 * The range of dividers that can be produced using the dual-modulus
> > > +	 * pre-scaler is not continuous for values of n < p*(p-1). If we end up
> > > +	 * with a non supported divider value, pick the next closest one.
> > > +	 */
> > > +	a = n % p;
> > > +	b = n / p;
> > > +
> > > +	if (b < 3) {
> > > +		b = 3;
> > > +		a = 0;
> > > +	} else if (a > b) {
> > > +		if (a - b < p - a) {
> > > +			a = b;
> > > +		} else {
> > > +			a = 0;
> > > +			b++;
> > > +		}
> > > +	}
> > > +
> > > +	if (out_p)
> > > +		*out_p = p;
> > > +	if (out_a)
> > > +		*out_a = a;
> > > +	if (out_b)
> > > +		*out_b = b;
> > > +
> > > +	return p * b + a;
> > > +}
> > > +
> > > +static long adf4360_clk_round_rate(struct clk_hw *hw,
> > > +				   unsigned long rate,
> > > +				   unsigned long *parent_rate)
> > > +{
> > > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +	unsigned int r, n;
> > > +	unsigned int pfd_freq;
> > > +
> > > +	if (*parent_rate == 0)
> > > +		return 0;
> > > +
> > > +	if (st->part_id == ID_ADF4360_9)
> > > +		return *parent_rate * st->n / st->r;
> > > +
> > > +	if (rate > st->vco_max)
> > > +		return st->vco_max;
> > > +
> > > +	/* ADF4360-0 to AD4370-7 have an optional by two divider */
> > > +	if (st->part_id <= ID_ADF4360_7) {
> > > +		if (rate < st->vco_min / 2)
> > > +			return st->vco_min / 2;
> > > +		if (rate < st->vco_min && rate > st->vco_max / 2) {
> > > +			if (st->vco_min - rate < rate - st->vco_max / 2)
> > > +				return st->vco_min;
> > > +			else
> > > +				return st->vco_max / 2;
> > > +		}
> > > +	} else {
> > > +		if (rate < st->vco_min)
> > > +			return st->vco_min;
> > > +	}
> > > +
> > > +	r = DIV_ROUND_CLOSEST(*parent_rate, st->pfd_freq);
> > > +	pfd_freq = *parent_rate / r;
> > > +	n = DIV_ROUND_CLOSEST(rate, pfd_freq);
> > > +
> > > +	if (st->part_id <= ID_ADF4360_7)
> > > +		n = adf4360_calc_prescaler(pfd_freq, n, NULL, NULL, NULL);
> > > +
> > > +	return pfd_freq * n;
> > > +}
> > > +
> > > +static inline bool adf4360_is_powerdown(struct adf4360_state *st)
> > > +{
> > > +	return (st->power_down_mode != ADF4360_POWER_DOWN_NORMAL);
> > > +}
> > > +
> > > +static int adf4360_set_freq(struct adf4360_state *st, unsigned long rate)
> > > +{
> > > +	unsigned int val_r, val_n, val_ctrl;
> > > +	unsigned int pfd_freq;
> > > +	unsigned long r, n;
> > > +	int ret;
> > > +
> > > +	if (!st->clkin_freq || (st->clkin_freq > ADF4360_MAX_REFIN_RATE))
> > > +		return -EINVAL;
> > > +
> > > +	if ((rate < st->vco_min) || (rate > st->vco_max))
> > > +		return -EINVAL;
> > > +
> > > +	if (adf4360_is_powerdown(st))
> > > +		ret = -EBUSY;
> > > +
> > > +	r = DIV_ROUND_CLOSEST(st->clkin_freq, st->pfd_freq);
> > > +	pfd_freq = st->clkin_freq / r;
> > > +	n = DIV_ROUND_CLOSEST(rate, pfd_freq);
> > > +
> > > +	val_ctrl = ADF4360_CPL(st->info->default_cpl) |
> > > +		   ADF4360_MUXOUT(st->muxout_mode) |
> > > +		   ADF4360_PDP(!st->pdp) |
> > > +		   ADF4360_MTLD(st->mtld) |
> > > +		   ADF4360_OPL(st->power_level) |
> > > +		   ADF4360_CPI1(st->cpi) |
> > > +		   ADF4360_CPI2(st->cpi) |
> > > +		   ADF4360_POWERDOWN(st->power_down_mode);
> > > +
> > > +	/* ADF4360-0 to ADF4360-7 have a dual-modulous prescaler */
> > > +	if (st->part_id <= ID_ADF4360_7) {
> > > +		unsigned int p, a, b;
> > > +
> > > +		n = adf4360_calc_prescaler(pfd_freq, n, &p, &a, &b);
> > > +
> > > +		switch (p) {
> > > +		case 8:
> > > +			val_ctrl |= ADF4360_PRESCALER(ADF4360_PRESCALER_8);
> > > +			break;
> > > +		case 16:
> > > +			val_ctrl |= ADF4360_PRESCALER(ADF4360_PRESCALER_16);
> > > +			break;
> > > +		default:
> > > +			val_ctrl |= ADF4360_PRESCALER(ADF4360_PRESCALER_32);
> > > +			break;
> > > +		}
> > > +
> > > +		val_n = ADF4360_A_COUNTER(a) |
> > > +			ADF4360_B_COUNTER(b);
> > > +
> > > +		if (rate < st->vco_min)
> > > +			val_n |= ADF4360_OUT_DIV2(true) |
> > > +				 ADF4360_PRESCALER_DIV2(true);
> > > +	} else {
> > > +		val_n = ADF4360_B_COUNTER(n);
> > > +	}
> > > +
> > > +	/*
> > > +	 * Always use BSC divider of 8, see Analog Devices AN-1347.
> > > +	 * 
> > > http://www.analog.com/media/en/technical-documentation/application-notes/AN-1347.pdf
> > > +	 */
> > > +	val_r = ADF4360_R_COUNTER(r) |
> > > +		ADF4360_ABP(ADF4360_ABP_3_0NS) |
> > > +		ADF4360_BSC(ADF4360_BSC_8);
> > > +
> > > +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_RDIV), val_r);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val_ctrl);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	/*
> > > +	 * Allow the transient behavior of the ADF4360-7 during initial
> > > +	 * power-up to settle.
> > > +	 */
> > > +	if (st->initial_reg_seq) {
> > > +		usleep_range(15000, 20000);
> > > +		st->initial_reg_seq = false;
> > > +	}
> > > +
> > > +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_NDIV), val_n);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	st->freq_req = rate;
> > > +	st->n = n;
> > > +	st->r = r;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int adf4360_clk_set_rate(struct clk_hw *hw,
> > > +				unsigned long rate,
> > > +				unsigned long parent_rate)
> > > +{
> > > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +	int ret;
> > > +
> > > +	if ((parent_rate == 0) || (parent_rate > ADF4360_MAX_REFIN_RATE))
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&st->lock);
> > > +	if (st->clkin_freq != parent_rate)
> > > +		st->clkin_freq = parent_rate;
> > > +
> > > +	ret = adf4360_set_freq(st, rate);
> > > +	mutex_unlock(&st->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int __adf4360_power_down(struct adf4360_state *st, unsigned int
> > > mode)
> > > +{
> > > +	struct device *dev = &st->spi->dev;
> > > +	unsigned int val;
> > > +	int ret = 0;
> > > +
> > > +	switch (mode) {
> > > +	case ADF4360_POWER_DOWN_NORMAL:
> > > +		if (st->vdd_reg) {
> > > +			ret = regulator_enable(st->vdd_reg);
> > > +			if (ret) {
> > > +				dev_err(dev, "Supply enable error: %d\n", ret);
> > > +				return ret;
> > > +			}
> > > +		}
> > > +
> > > +		st->initial_reg_seq = true;
> > > +		st->power_down_mode = mode;
> > > +		ret = adf4360_set_freq(st, st->freq_req);
> > > +		break;
> > > +	case ADF4360_POWER_DOWN_SOFT_ASYNC: /* fallthrough */
> > > +	case ADF4360_POWER_DOWN_SOFT_SYNC:
> > > +		val = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_MUXOUT_MSK;
> > > +		val |= ADF4360_POWERDOWN(mode);
> > > +		ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
> > > +		break;
> > > +	case ADF4360_POWER_DOWN_CE:
> > > +		if (st->chip_en_gpio)
> > > +			gpiod_set_value(st->chip_en_gpio, 0x0);
> > > +		else
> > > +			return -ENODEV;
> > > +		break;
> > > +	case ADF4360_POWER_DOWN_REGULATOR:
> > > +		if (!st->vdd_reg)
> > > +			return -ENODEV;
> > > +
> > > +		if (st->chip_en_gpio)
> > > +			ret = __adf4360_power_down(st, ADF4360_POWER_DOWN_CE);
> > > +		else
> > > +			ret = __adf4360_power_down(st,
> > > +						ADF4360_POWER_DOWN_SOFT_ASYNC);
> > > +
> > > +		ret = regulator_disable(st->vdd_reg);
> > > +		if (ret)
> > > +			dev_err(dev, "Supply disable error: %d\n", ret);
> > > +		break;
> > > +	}
> > > +	if (ret == 0)
> > > +		st->power_down_mode = mode;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int adf4360_power_down(struct adf4360_state *st, unsigned int mode)
> > > +{
> > > +	int ret;
> > > +
> > > +	mutex_lock(&st->lock);
> > > +	ret = __adf4360_power_down(st, mode);
> > > +	mutex_unlock(&st->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int adf4360_clk_prepare(struct clk_hw *hw)
> > > +{
> > > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +
> > > +	return adf4360_power_down(st, ADF4360_POWER_DOWN_NORMAL);
> > > +}
> > > +
> > > +static void adf4360_clk_unprepare(struct clk_hw *hw)
> > > +{
> > > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +
> > > +	adf4360_power_down(st, ADF4360_POWER_DOWN_SOFT_ASYNC);
> > > +}
> > > +
> > > +static int adf4360_clk_enable(struct clk_hw *hw)
> > > +{
> > > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +
> > > +	if (st->chip_en_gpio)
> > > +		gpiod_set_value(st->chip_en_gpio, 0x1);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void adf4360_clk_disable(struct clk_hw *hw)
> > > +{
> > > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +
> > > +	if (st->chip_en_gpio)
> > > +		adf4360_power_down(st, ADF4360_POWER_DOWN_CE);
> > > +}
> > > +
> > > +static int adf4360_clk_is_enabled(struct clk_hw *hw)
> > > +{
> > > +	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +
> > > +	return adf4360_is_powerdown(st);
> > > +}
> > > +
> > > +static const struct clk_ops adf4360_clk_ops = {
> > > +	.recalc_rate = adf4360_clk_recalc_rate,
> > > +	.round_rate = adf4360_clk_round_rate,
> > > +	.set_rate = adf4360_clk_set_rate,
> > > +	.prepare = adf4360_clk_prepare,
> > > +	.unprepare = adf4360_clk_unprepare,
> > > +	.enable = adf4360_clk_enable,
> > > +	.disable = adf4360_clk_disable,
> > > +	.is_enabled = adf4360_clk_is_enabled,
> > > +};
> > > +
> > > +static ssize_t adf4360_read(struct iio_dev *indio_dev,
> > > +			    uintptr_t private,
> > > +			    const struct iio_chan_spec *chan,
> > > +			    char *buf)
> > > +{
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +	unsigned long val;
> > > +	int ret = 0;
> > > +
> > > +	switch ((u32)private) {
> > > +	case ADF4360_FREQ_REFIN:
> > > +		val = st->clkin_freq;
> > > +		break;
> > > +	case ADF4360_MTLD:
> > > +		val = st->mtld;
> > > +		break;
> > > +	case ADF4360_FREQ_PFD:
> > > +		val = st->pfd_freq;
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +		val = 0;
> > > +	}
> > > +
> > > +	return ret < 0 ? ret : sprintf(buf, "%lu\n", val);
> > > +}
> > > +
> > > +static ssize_t adf4360_write(struct iio_dev *indio_dev,
> > > +			     uintptr_t private,
> > > +			     const struct iio_chan_spec *chan,
> > > +			     const char *buf, size_t len)
> > > +{
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +	unsigned long readin, tmp;
> > > +	bool mtld;
> > > +	int ret = 0;
> > > +
> > > +	mutex_lock(&st->lock);
> > > +	switch ((u32)private) {
> > > +	case ADF4360_FREQ_REFIN:
> > > +		ret = kstrtoul(buf, 10, &readin);
> > > +		if (ret)
> > > +			break;
> > > +
> > > +		if ((readin > ADF4360_MAX_REFIN_RATE) || (readin == 0)) {
> > > +			ret = -EINVAL;
> > > +			break;
> > > +		}
> > > +
> > > +		if (st->clkin) {
> > > +			tmp = clk_round_rate(st->clkin, readin);
> > > +			if (tmp != readin) {
> > > +				ret = -EINVAL;
> > > +				break;
> > > +			}
> > > +
> > > +			ret = clk_set_rate(st->clkin, tmp);
> > > +			if (ret)
> > > +				break;  
> > A bit odd to directly provide an interface to control and entirely different
> > bit of hardware.   If there are specific demands on the input clock as a
> > result
> > of something to do with the outputs, then fair enough.  Direct tweaking like
> > this seems like a very odd interface.
> >   
> > > +		}
> > > +
> > > +		st->clkin_freq = readin;
> > > +		break;
> > > +	case ADF4360_MTLD:
> > > +		ret = kstrtobool(buf, &mtld);
> > > +		if (ret)
> > > +			break;
> > > +
> > > +		st->mtld = mtld;
> > > +		break;
> > > +	case ADF4360_FREQ_PFD:
> > > +		ret = kstrtoul(buf, 10, &readin);
> > > +		if (ret)
> > > +			break;
> > > +
> > > +		if ((readin > ADF4360_MAX_PFD_RATE) || (readin == 0)) {
> > > +			ret = -EINVAL;
> > > +			break;
> > > +		}
> > > +
> > > +		st->pfd_freq = readin;
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +	}
> > > +
> > > +	if (ret == 0)
> > > +		ret = adf4360_set_freq(st, st->freq_req);
> > > +	mutex_unlock(&st->lock);
> > > +
> > > +	return ret ? ret : len;
> > > +}
> > > +
> > > +static int adf4360_get_muxout_mode(struct iio_dev *indio_dev,
> > > +				   const struct iio_chan_spec *chan)
> > > +{
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +
> > > +	return st->muxout_mode;
> > > +}
> > > +
> > > +static int adf4360_set_muxout_mode(struct iio_dev *indio_dev,
> > > +				   const struct iio_chan_spec *chan,
> > > +				   unsigned int mode)
> > > +{
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +	unsigned int writeval;
> > > +	int ret = 0;
> > > +
> > > +	mutex_lock(&st->lock);
> > > +	writeval = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_MUXOUT_MSK;
> > > +	writeval |= ADF4360_MUXOUT(mode & 0x7);
> > > +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), writeval);
> > > +	if (ret == 0)
> > > +		st->muxout_mode = mode & 0x7;
> > > +	mutex_unlock(&st->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct iio_enum adf4360_muxout_modes_available = {
> > > +	.items = adf4360_muxout_modes,
> > > +	.num_items = ARRAY_SIZE(adf4360_muxout_modes),
> > > +	.get = adf4360_get_muxout_mode,
> > > +	.set = adf4360_set_muxout_mode,
> > > +};
> > > +
> > > +static int adf4360_get_power_down(struct iio_dev *indio_dev,
> > > +				  const struct iio_chan_spec *chan)
> > > +{
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +
> > > +	return st->power_down_mode;
> > > +}
> > > +
> > > +static int adf4360_set_power_down(struct iio_dev *indio_dev,
> > > +				  const struct iio_chan_spec *chan,
> > > +				  unsigned int mode)
> > > +{
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +
> > > +	return adf4360_power_down(st, mode);
> > > +}
> > > +
> > > +static const struct iio_enum adf4360_pwr_dwn_modes_available = {
> > > +	.items = adf4360_power_down_modes,
> > > +	.num_items = ARRAY_SIZE(adf4360_power_down_modes),
> > > +	.get = adf4360_get_power_down,
> > > +	.set = adf4360_set_power_down,
> > > +};
> > > +
> > > +static int adf4360_get_power_level(struct iio_dev *indio_dev,
> > > +				   const struct iio_chan_spec *chan)
> > > +{
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +
> > > +	return st->power_level;
> > > +}
> > > +
> > > +static int adf4360_set_power_level(struct iio_dev *indio_dev,
> > > +				   const struct iio_chan_spec *chan,
> > > +				   unsigned int level)
> > > +{
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +	unsigned int val;
> > > +	int ret;
> > > +
> > > +	if (adf4360_is_powerdown(st))
> > > +		return -EBUSY;
> > > +
> > > +	mutex_lock(&st->lock);
> > > +	val = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_OPL_MSK;
> > > +	val |= ADF4360_OPL(level);
> > > +	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
> > > +	st->power_level = level;
> > > +	mutex_unlock(&st->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct iio_enum adf4360_pwr_lvl_modes_available = {
> > > +	.items = adf4360_power_level_modes,
> > > +	.num_items = ARRAY_SIZE(adf4360_power_level_modes),
> > > +	.get = adf4360_get_power_level,
> > > +	.set = adf4360_set_power_level,
> > > +};
> > > +
> > > +#define _ADF4360_EXT_INFO(_name, _ident) { \
> > > +	.name = _name, \
> > > +	.read = adf4360_read, \
> > > +	.write = adf4360_write, \
> > > +	.private = _ident, \
> > > +	.shared = IIO_SEPARATE, \
> > > +}
> > > +
> > > +static const struct iio_chan_spec_ext_info adf4360_ext_info[] = {  
> > 
> > This is a wide range of new ABI.  These all need documentation
> > in Documentation/ABI/testing/sysfs-bus-iio-*
> > 
> > Without docs, it's hard to discuss if these are appropriate but a few initial
> > comments...  
> > > +	_ADF4360_EXT_INFO("refin_frequency", ADF4360_FREQ_REFIN),  
> > Looks like a control that should be matched to some clock source and
> > not change at runtime?
> >   
> > > +	_ADF4360_EXT_INFO("mute_till_lock_detect", ADF4360_MTLD),
> > > +	_ADF4360_EXT_INFO("pfd_frequency", ADF4360_FREQ_PFD),
> > > +	IIO_ENUM_AVAILABLE("muxout_mode", &adf4360_muxout_modes_available),
> > > +	IIO_ENUM("muxout_mode", false, &adf4360_muxout_modes_available),
> > > +	IIO_ENUM_AVAILABLE("power_down", &adf4360_pwr_dwn_modes_available),
> > > +	IIO_ENUM("power_down", false, &adf4360_pwr_dwn_modes_available),
> > > +	IIO_ENUM_AVAILABLE("power_level", &adf4360_pwr_lvl_modes_available),
> > > +	IIO_ENUM("power_level", false, &adf4360_pwr_lvl_modes_available),
> > > +	{ },
> > > +};
> > > +
> > > +static const struct iio_chan_spec adf4360_chan = {
> > > +	.type = IIO_ALTVOLTAGE,
> > > +	.indexed = 1,
> > > +	.output = 1,
> > > +	.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY),
> > > +	.ext_info = adf4360_ext_info,
> > > +};
> > > +
> > > +static int adf4360_read_raw(struct iio_dev *indio_dev,
> > > +			    struct iio_chan_spec const *chan,
> > > +			    int *val,
> > > +			    int *val2,
> > > +			    long mask)
> > > +{
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +	bool lk_det;
> > > +
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_FREQUENCY:
> > > +		if (adf4360_is_powerdown(st))
> > > +			return -EBUSY;
> > > +
> > > +		lk_det = (ADF4360_MUXOUT_LOCK_DETECT | ADF4360_MUXOUT_OD_LD) &
> > > +			 st->muxout_mode;
> > > +		if (lk_det && st->muxout_gpio) {
> > > +			if (!gpiod_get_value(st->muxout_gpio)) {
> > > +				dev_dbg(&st->spi->dev, "PLL un-locked\n");
> > > +				return -EBUSY;
> > > +			}
> > > +		}
> > > +
> > > +		*val = st->freq_req;
> > > +		return IIO_VAL_INT;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +};
> > > +
> > > +static int adf4360_write_raw(struct iio_dev *indio_dev,
> > > +			     struct iio_chan_spec const *chan,
> > > +			     int val,
> > > +			     int val2,
> > > +			     long mask)
> > > +{
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +	int ret;
> > > +
> > > +	mutex_lock(&st->lock);
> > > +	switch (mask) {
> > > +	case IIO_CHAN_INFO_FREQUENCY:
> > > +		ret = adf4360_set_freq(st, val);
> > > +		break;
> > > +	default:
> > > +		ret = -EINVAL;
> > > +	}
> > > +	mutex_unlock(&st->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static int adf4360_reg_access(struct iio_dev *indio_dev,
> > > +			      unsigned int reg,
> > > +			      unsigned int writeval,
> > > +			      unsigned int *readval)
> > > +{
> > > +	struct adf4360_state *st = iio_priv(indio_dev);
> > > +	int ret = 0;
> > > +
> > > +	if (reg >= ADF4360_REG_NUM)
> > > +		return -EFAULT;
> > > +
> > > +	mutex_lock(&st->lock);
> > > +	if (readval) {
> > > +		*readval = st->regs[reg];
> > > +	} else {
> > > +		writeval &= 0xFFFFFC;
> > > +		ret = adf4360_write_reg(st, reg, writeval);
> > > +	}
> > > +	mutex_unlock(&st->lock);
> > > +
> > > +	return ret;
> > > +}
> > > +
> > > +static const struct iio_info adf4360_iio_info = {
> > > +	.read_raw = &adf4360_read_raw,
> > > +	.write_raw = &adf4360_write_raw,
> > > +	.debugfs_reg_access = &adf4360_reg_access,
> > > +};
> > > +
> > > +static int adf4360_get_gpio(struct adf4360_state *st)
> > > +{
> > > +	struct device *dev = &st->spi->dev;
> > > +	unsigned int val;
> > > +	int ret, i;
> > > +
> > > +	st->chip_en_gpio = devm_gpiod_get_optional(dev, "enable",
> > > +						   GPIOD_OUT_HIGH);
> > > +	if (IS_ERR(st->chip_en_gpio)) {
> > > +		dev_err(dev, "Chip enable GPIO error\n");  
> > 
> > Put handling in here to prevent an error message of DEFER.
> > Same for the other routes where this might happen.
> >   
> > > +		return PTR_ERR(st->chip_en_gpio);
> > > +	}
> > > +
> > > +	if (st->chip_en_gpio)
> > > +		st->power_down_mode = ADF4360_POWER_DOWN_CE;
> > > +
> > > +	st->muxout_gpio = devm_gpiod_get_optional(dev, "adi,muxout", GPIOD_IN);
> > > +	if (IS_ERR(st->muxout_gpio)) {
> > > +		dev_err(dev, "Muxout GPIO error\n");
> > > +		return PTR_ERR(st->muxout_gpio);
> > > +	}
> > > +
> > > +	if (!st->muxout_gpio)
> > > +		return 0;
> > > +
> > > +	/* ADF4360 PLLs are write only devices, try to probe using GPIO. */
> > > +	for (i = 0; i < 4; i++) {
> > > +		if (i & 1)
> > > +			val = ADF4360_MUXOUT(ADF4360_MUXOUT_DVDD);
> > > +		else
> > > +			val = ADF4360_MUXOUT(ADF4360_MUXOUT_GND);
> > > +
> > > +		ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
> > > +		if (ret)
> > > +			return ret;
> > > +
> > > +		ret = gpiod_get_value(st->muxout_gpio);
> > > +		if (ret ^ (i & 1)) {
> > > +			dev_err(dev, "Probe failed (muxout)");
> > > +			return -ENODEV;
> > > +		}
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void adf4360_clkin_disable(void *data)
> > > +{
> > > +	struct adf4360_state *st = data;
> > > +
> > > +	clk_disable_unprepare(st->clkin);
> > > +}
> > > +
> > > +static int adf4360_get_clkin(struct adf4360_state *st)
> > > +{
> > > +	struct device *dev = &st->spi->dev;
> > > +	struct clk *clk;
> > > +	int ret;
> > > +
> > > +	clk = devm_clk_get(dev, "clkin");
> > > +	if (IS_ERR(clk))
> > > +		return PTR_ERR(clk);
> > > +
> > > +	ret = clk_prepare_enable(clk);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = devm_add_action_or_reset(dev, adf4360_clkin_disable, st);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	st->clkin = clk;
> > > +	st->clkin_freq = clk_get_rate(clk);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void adf4360_clk_del_provider(void *data)
> > > +{
> > > +	struct adf4360_state *st = data;
> > > +
> > > +	of_clk_del_provider(st->spi->dev.of_node);
> > > +}
> > > +
> > > +static int adf4360_clk_register(struct adf4360_state *st)
> > > +{  
> > 
> > Hmm. This makes me wonder why this is an IIO driver rather than a clk
> > driver?  Definitely needs some more information in the patch description
> > or a cover letter.
> >   
> > > +	struct spi_device *spi = st->spi;
> > > +	struct clk_init_data init;
> > > +	struct clk *clk;
> > > +	const char *parent_name;
> > > +	int ret;
> > > +
> > > +	parent_name = of_clk_get_parent_name(spi->dev.of_node, 0);
> > > +	if (!parent_name)
> > > +		return -EINVAL;
> > > +
> > > +	init.name = st->clk_out_name;
> > > +	init.ops = &adf4360_clk_ops;
> > > +	init.flags = CLK_SET_RATE_GATE;
> > > +	init.parent_names = &parent_name;
> > > +	init.num_parents = 1;
> > > +
> > > +	st->output.hw.init = &init;
> > > +
> > > +	clk = devm_clk_register(&spi->dev, &st->output.hw);
> > > +	if (IS_ERR(clk))
> > > +		return PTR_ERR(clk);
> > > +
> > > +	ret = of_clk_add_provider(spi->dev.of_node, of_clk_src_simple_get, clk);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return devm_add_action_or_reset(&spi->dev, adf4360_clk_del_provider,
> > > st);
> > > +}
> > > +
> > > +static int adf4360_parse_dt(struct adf4360_state *st)
> > > +{
> > > +	struct device *dev = &st->spi->dev;
> > > +	u32 tmp;
> > > +	int ret;
> > > +
> > > +	ret = device_property_read_string(dev, "clock-output-names",
> > > +					  &st->clk_out_name);
> > > +	if ((ret < 0) && dev->of_node)
> > > +		st->clk_out_name = dev->of_node->name;
> > > +
> > > +	if (st->part_id >= ID_ADF4360_7) {
> > > +		/*
> > > +		 * ADF4360-7 to ADF4360-9 have a VCO that is tuned to a specific
> > > +		 * range using an external inductor. These properties describe
> > > +		 * the range selected by the external inductor.
> > > +		 */
> > > +		ret = device_property_read_u32(dev,
> > > +					       "adi,vco-minimum-frequency-hz",
> > > +					       &tmp);
> > > +		if (ret == 0)
> > > +			st->vco_min = max(st->info->vco_min, tmp);
> > > +		else
> > > +			st->vco_min = st->info->vco_min;
> > > +
> > > +		ret = device_property_read_u32(dev,
> > > +					       "adi,vco-maximum-frequency-hz",
> > > +					       &tmp);
> > > +		if (ret == 0)
> > > +			st->vco_max = min(st->info->vco_max, tmp);
> > > +		else
> > > +			st->vco_max = st->info->vco_max;
> > > +	} else {
> > > +		st->vco_min = st->info->vco_min;
> > > +		st->vco_max = st->info->vco_max;
> > > +	}
> > > +
> > > +	st->pdp = device_property_read_bool(dev, "adi,loop-filter-inverting");
> > > +
> > > +	ret = device_property_read_u32(dev,
> > > +				       "adi,loop-filter-pfd-frequency-hz",
> > > +				       &tmp);
> > > +	if (ret == 0) {
> > > +		st->pfd_freq = tmp;
> > > +	} else {
> > > +		dev_err(dev, "PFD frequency property missing\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = device_property_read_u32(dev,
> > > +				"adi,loop-filter-charge-pump-current-microamp",
> > > +				&tmp);
> > > +	if (ret == 0) {
> > > +		st->cpi = find_closest(tmp, adf4360_cpi_modes,
> > > +				       ARRAY_SIZE(adf4360_cpi_modes));
> > > +	} else {
> > > +		dev_err(dev, "CPI property missing\n");
> > > +		return ret;
> > > +	}
> > > +
> > > +	ret = device_property_read_u32(dev, "adi,power-up-frequency-hz", &tmp);
> > > +	if (ret == 0)
> > > +		st->freq_req = tmp;
> > > +	else
> > > +		st->freq_req = st->vco_min;
> > > +
> > > +	ret = device_property_read_u32(dev, "adi,power-out-level-microamp",
> > > +				       &tmp);
> > > +	if (ret == 0)
> > > +		st->power_level = find_closest(tmp, adf4360_power_lvl_microamp,
> > > +					ARRAY_SIZE(adf4360_power_lvl_microamp));
> > > +	else
> > > +		st->power_level = ADF4360_PL_5;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int adf4360_probe(struct spi_device *spi)
> > > +{
> > > +	struct iio_dev *indio_dev;
> > > +	const struct spi_device_id *id = spi_get_device_id(spi);  
> > 
> > Given we require various dt parameters to be present, might as well
> > associate the id with the of_ structures instead and use the dt
> > calls throughout.  Even better if you use the firmware type independent
> > versions.
> >   
> > > +	struct adf4360_state *st;
> > > +	int ret;
> > > +
> > > +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > > +	if (!indio_dev)
> > > +		return -ENOMEM;
> > > +
> > > +	st = iio_priv(indio_dev);
> > > +
> > > +	mutex_init(&st->lock);
> > > +
> > > +	spi_set_drvdata(spi, indio_dev);
> > > +
> > > +	st->spi = spi;
> > > +	st->info = &adf4360_chip_info_tbl[id->driver_data];
> > > +	st->part_id = id->driver_data;
> > > +	st->muxout_mode = ADF4360_MUXOUT_LOCK_DETECT;
> > > +	st->mtld = true;
> > > +
> > > +	ret = adf4360_parse_dt(st);
> > > +	if (ret) {
> > > +		dev_err(&spi->dev, "Parsing properties failed (%d)\n", ret);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	indio_dev->dev.parent = &spi->dev;
> > > +
> > > +	if (spi->dev.of_node)
> > > +		indio_dev->name = spi->dev.of_node->name;
> > > +	else
> > > +		indio_dev->name = spi_get_device_id(spi)->name;
> > > +
> > > +	indio_dev->info = &adf4360_iio_info;
> > > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > > +	indio_dev->channels = &adf4360_chan;
> > > +	indio_dev->num_channels = 1;
> > > +	st->output.indio_dev = indio_dev;
> > > +
> > > +	ret = adf4360_get_gpio(st);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = adf4360_get_clkin(st);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	st->vdd_reg = devm_regulator_get_optional(&spi->dev, "vdd");
> > > +	if (IS_ERR(st->vdd_reg)) {
> > > +		if (PTR_ERR(st->vdd_reg) != -ENODEV) {
> > > +			dev_err(&spi->dev, "Regulator error\n");
> > > +			return PTR_ERR(st->vdd_reg);
> > > +		}
> > > +
> > > +		st->vdd_reg = NULL;
> > > +	}
> > > +
> > > +	ret = adf4360_power_down(st, ADF4360_POWER_DOWN_NORMAL);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = adf4360_clk_register(st);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return devm_iio_device_register(&spi->dev, indio_dev);
> > > +}
> > > +
> > > +static const struct of_device_id adf4360_of_match[] = {
> > > +	{ .compatible = "adi,adf4360-0", },
> > > +	{ .compatible = "adi,adf4360-1", },
> > > +	{ .compatible = "adi,adf4360-2", },
> > > +	{ .compatible = "adi,adf4360-3", },
> > > +	{ .compatible = "adi,adf4360-4", },
> > > +	{ .compatible = "adi,adf4360-5", },
> > > +	{ .compatible = "adi,adf4360-6", },
> > > +	{ .compatible = "adi,adf4360-7", },
> > > +	{ .compatible = "adi,adf4360-8", },
> > > +	{ .compatible = "adi,adf4360-9", },
> > > +	{},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, adf4360_of_match);
> > > +
> > > +static const struct spi_device_id adf4360_id[] = {  
> > 
> > As mentioned above, you can't actually probe this device
> > without a pile of dt stuff.  So this fallback doesn't
> > make much sense.  Use the data field in the of table
> > above and get rid of this table entirely.
> >   
> > > +	{"adf4360-0", ID_ADF4360_0},
> > > +	{"adf4360-1", ID_ADF4360_1},
> > > +	{"adf4360-2", ID_ADF4360_2},
> > > +	{"adf4360-3", ID_ADF4360_3},
> > > +	{"adf4360-4", ID_ADF4360_4},
> > > +	{"adf4360-5", ID_ADF4360_5},
> > > +	{"adf4360-6", ID_ADF4360_6},
> > > +	{"adf4360-7", ID_ADF4360_7},
> > > +	{"adf4360-8", ID_ADF4360_8},
> > > +	{"adf4360-9", ID_ADF4360_9},
> > > +	{}
> > > +};
> > > +
> > > +static struct spi_driver adf4360_driver = {
> > > +	.driver = {
> > > +		.name = "adf4360",
> > > +		.of_match_table = adf4360_of_match,
> > > +		.owner = THIS_MODULE,  
> > 
> > It's a long time since we had to set the .owner field for each driver.
> > 
> > Follow through what happens in module_spi_driver, spi_register_driver
> > etc and you'll find it's set automatically during driver registration.
> >   
> > > +	},
> > > +	.probe = adf4360_probe,
> > > +	.id_table = adf4360_id,
> > > +};
> > > +module_spi_driver(adf4360_driver);
> > > +
> > > +MODULE_LICENSE("GPL v2");
> > > +MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
> > > +MODULE_AUTHOR("Edward Kigwana <ekigwana@gmail.com>");
> > > +MODULE_DESCRIPTION("Analog Devices ADF4360 PLL");
Andy Shevchenko Feb. 3, 2020, 2:10 p.m. UTC | #4
On Mon, Feb 3, 2020 at 1:47 PM Ardelean, Alexandru
<alexandru.Ardelean@analog.com> wrote:
> On Sun, 2020-02-02 at 14:45 +0000, Jonathan Cameron wrote:
> > On Tue, 28 Jan 2020 13:13:00 +0200
> > Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

Just few comments on the code in case either this will go, or to teach
an author about best practices in LK.

> > > +#include <linux/err.h>
> > > +#include <linux/gpio/consumer.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/regulator/consumer.h>
> > > +#include <linux/spi/spi.h>
> > > +#include <linux/util_macros.h>

...

> > > +enum {
> > > +   ADF4360_CTRL,
> > > +   ADF4360_RDIV,
> > > +   ADF4360_NDIV,

> > > +   ADF4360_REG_NUM,

Sounds line no need for comma (is it indeed a terminator line?).

> > > +};

...

> > > +static int adf4360_write_reg(struct adf4360_state *st, unsigned int reg,
> > > +                        unsigned int val)
> > > +{
> > > +   int ret;
> > > +
> > > +   val |= reg;

This is dangerous. Shouldn't be some mask applied?

> > > +   st->spi_data[0] = (val >> 16) & 0xff;
> > > +   st->spi_data[1] = (val >> 8) & 0xff;
> > > +   st->spi_data[2] = val & 0xff;

All ' & 0xff' are redundant.

> > > +   ret = spi_write(st->spi, st->spi_data, ARRAY_SIZE(st->spi_data));
> > > +   if (ret == 0)
> > > +           st->regs[reg] = val;
> > > +
> > > +   return ret;

Please, use pattern:
  if (ret)
    return ret;

  ...
  return 0;

> > > +}

...

> > > +static unsigned int adf4360_calc_prescaler(unsigned int pfd_freq,
> > > +                                      unsigned int n,
> > > +                                      unsigned int *out_p,
> > > +                                      unsigned int *out_a,
> > > +                                      unsigned int *out_b)
> > > +{
> > > +   unsigned int rate = pfd_freq * n;
> > > +   unsigned int p, a, b;
> > > +
> > > +   /* Make sure divider counter input frequency is low enough */
> > > +   p = 8;
> > > +   while (p < 32 && rate / p > ADF4360_MAX_COUNTER_RATE)
> > > +           p *= 2;
> > > +
> > > +   /*
> > > +    * The range of dividers that can be produced using the dual-modulus
> > > +    * pre-scaler is not continuous for values of n < p*(p-1). If we end up
> > > +    * with a non supported divider value, pick the next closest one.
> > > +    */
> > > +   a = n % p;
> > > +   b = n / p;

You may avoid divisions if you use shifts.
Currently it's a bit hard to compiler to prove that p is power of 2.

> > > +   if (b < 3) {
> > > +           b = 3;
> > > +           a = 0;
> > > +   } else if (a > b) {

Does this guarantee p >= a?

> > > +           if (a - b < p - a) {
> > > +                   a = b;
> > > +           } else {
> > > +                   a = 0;
> > > +                   b++;
> > > +           }
> > > +   }

I guess above conditional tree can be a bit improved, although I don't
see right now in what way.

> > > +   return p * b + a;
> > > +}

...

> > > +   /* ADF4360-0 to AD4370-7 have an optional by two divider */
> > > +   if (st->part_id <= ID_ADF4360_7) {
> > > +           if (rate < st->vco_min / 2)
> > > +                   return st->vco_min / 2;
> > > +           if (rate < st->vco_min && rate > st->vco_max / 2) {
> > > +                   if (st->vco_min - rate < rate - st->vco_max / 2)
> > > +                           return st->vco_min;

> > > +                   else

Redundant.

> > > +                           return st->vco_max / 2;
> > > +           }

> > > +   } else {
> > > +           if (rate < st->vco_min)

} else if (...) {

> > > +                   return st->vco_min;
> > > +   }

...

> > > +   case ADF4360_POWER_DOWN_REGULATOR:
> > > +           if (!st->vdd_reg)
> > > +                   return -ENODEV;
> > > +
> > > +           if (st->chip_en_gpio)
> > > +                   ret = __adf4360_power_down(st, ADF4360_POWER_DOWN_CE);
> > > +           else
> > > +                   ret = __adf4360_power_down(st,
> > > +                                           ADF4360_POWER_DOWN_SOFT_ASYNC);

Missed error check.

> > > +
> > > +           ret = regulator_disable(st->vdd_reg);
> > > +           if (ret)
> > > +                   dev_err(dev, "Supply disable error: %d\n", ret);
> > > +           break;
> > > +   }

...

> > > +   if (ret == 0)
> > > +           st->power_down_mode = mode;
> > > +
> > > +   return 0;

Looks like 'return ret;' but see one comment at the beginning of the letter.

...

> > > +static ssize_t adf4360_read(struct iio_dev *indio_dev,
> > > +                       uintptr_t private,
> > > +                       const struct iio_chan_spec *chan,
> > > +                       char *buf)
> > > +{
> > > +   struct adf4360_state *st = iio_priv(indio_dev);
> > > +   unsigned long val;

> > > +   int ret = 0;

Redundant variable.

> > > +
> > > +   switch ((u32)private) {
> > > +   case ADF4360_FREQ_REFIN:
> > > +           val = st->clkin_freq;
> > > +           break;
> > > +   case ADF4360_MTLD:
> > > +           val = st->mtld;
> > > +           break;
> > > +   case ADF4360_FREQ_PFD:
> > > +           val = st->pfd_freq;
> > > +           break;
> > > +   default:
> > > +           ret = -EINVAL;
> > > +           val = 0;
> > > +   }
> > > +
> > > +   return ret < 0 ? ret : sprintf(buf, "%lu\n", val);
> > > +}

...

> > > +static ssize_t adf4360_write(struct iio_dev *indio_dev,
> > > +                        uintptr_t private,
> > > +                        const struct iio_chan_spec *chan,
> > > +                        const char *buf, size_t len)
> > > +{
> > > +   struct adf4360_state *st = iio_priv(indio_dev);
> > > +   unsigned long readin, tmp;
> > > +   bool mtld;
> > > +   int ret = 0;
> > > +
> > > +   mutex_lock(&st->lock);
> > > +   switch ((u32)private) {

Strange casting. Why?

> > > +           if ((readin > ADF4360_MAX_REFIN_RATE) || (readin == 0)) {

Too many parentheses.

> > > +                   ret = -EINVAL;
> > > +                   break;
> > > +           }
> > > +
> > > +           if (st->clkin) {
> > > +                   tmp = clk_round_rate(st->clkin, readin);
> > > +                   if (tmp != readin) {
> > > +                           ret = -EINVAL;
> > > +                           break;
> > > +                   }
> > > +
> > > +                   ret = clk_set_rate(st->clkin, tmp);
> > > +                   if (ret)
> > > +                           break;
> > A bit odd to directly provide an interface to control and entirely different
> > bit of hardware.   If there are specific demands on the input clock as a
> > result
> > of something to do with the outputs, then fair enough.  Direct tweaking like
> > this seems like a very odd interface.
> >
> > > +           }
> > > +
> > > +           st->clkin_freq = readin;
> > > +           break;

> > > +   case ADF4360_FREQ_PFD:
> > > +           ret = kstrtoul(buf, 10, &readin);
> > > +           if (ret)
> > > +                   break;
> > > +


> > > +           if ((readin > ADF4360_MAX_PFD_RATE) || (readin == 0)) {

Ditto.

> > > +                   ret = -EINVAL;
> > > +                   break;
> > > +           }
> > > +
> > > +           st->pfd_freq = readin;
> > > +           break;
> > > +   default:
> > > +           ret = -EINVAL;

Nevertheless 'break;' is good to have even here.

> > > +   }

> > > +
> > > +   if (ret == 0)

Maybe this, or maybe

  if (ret)
    goto out_unlock;

> > > +           ret = adf4360_set_freq(st, st->freq_req);
> > > +   mutex_unlock(&st->lock);
> > > +
> > > +   return ret ? ret : len;
> > > +}

...

> > > +   int ret = 0;

Redundant assignment.

> > > +   mutex_lock(&st->lock);
> > > +   writeval = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_MUXOUT_MSK;
> > > +   writeval |= ADF4360_MUXOUT(mode & 0x7);
> > > +   ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), writeval);

> > > +   if (ret == 0)
> > > +           st->muxout_mode = mode & 0x7;
> > > +   mutex_unlock(&st->lock);

Similar comment to the return check style as above.

> > > +   return ret;
> > > +}

...

> > > +static const struct iio_chan_spec_ext_info adf4360_ext_info[] = {

> > > +   IIO_ENUM("power_level", false, &adf4360_pwr_lvl_modes_available),
> > > +   { },

No need to have comma for terminator line.

> > > +};

...

> > > +   struct adf4360_state *st = iio_priv(indio_dev);
> > > +   bool lk_det;
> > > +
> > > +   switch (mask) {
> > > +   case IIO_CHAN_INFO_FREQUENCY:
> > > +           if (adf4360_is_powerdown(st))
> > > +                   return -EBUSY;
> > > +
> > > +           lk_det = (ADF4360_MUXOUT_LOCK_DETECT | ADF4360_MUXOUT_OD_LD) &
> > > +                    st->muxout_mode;
> > > +           if (lk_det && st->muxout_gpio) {
> > > +                   if (!gpiod_get_value(st->muxout_gpio)) {
> > > +                           dev_dbg(&st->spi->dev, "PLL un-locked\n");
> > > +                           return -EBUSY;
> > > +                   }
> > > +           }
> > > +
> > > +           *val = st->freq_req;

> > > +           return IIO_VAL_INT;
> > > +   default:
> > > +           return -EINVAL;
> > > +   }

> > > +
> > > +   return 0;

How this is possible?

...

> > > +   default:
> > > +           ret = -EINVAL;

break;

...

> > > +   struct adf4360_state *st = iio_priv(indio_dev);

> > > +   int ret = 0;

Would be better to have this assignment...

> > > +
> > > +   if (reg >= ADF4360_REG_NUM)
> > > +           return -EFAULT;
> > > +
> > > +   mutex_lock(&st->lock);
> > > +   if (readval) {
> > > +           *readval = st->regs[reg];

...here.

> > > +   } else {
> > > +           writeval &= 0xFFFFFC;

What this magic does? Make a define using GENMASK().

> > > +           ret = adf4360_write_reg(st, reg, writeval);
> > > +   }
> > > +   mutex_unlock(&st->lock);
> > > +
> > > +   return ret;
> > > +}

...

> > > +   /* ADF4360 PLLs are write only devices, try to probe using GPIO. */
> > > +   for (i = 0; i < 4; i++) {
> > > +           if (i & 1)
> > > +                   val = ADF4360_MUXOUT(ADF4360_MUXOUT_DVDD);
> > > +           else
> > > +                   val = ADF4360_MUXOUT(ADF4360_MUXOUT_GND);
> > > +
> > > +           ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
> > > +           if (ret)
> > > +                   return ret;
> > > +
> > > +           ret = gpiod_get_value(st->muxout_gpio);

> > > +           if (ret ^ (i & 1)) {

I guess == or != is better than ^ here.

> > > +                   dev_err(dev, "Probe failed (muxout)");
> > > +                   return -ENODEV;
> > > +           }
> > > +   }
> > > +
> > > +   return 0;
> > > +}

...

> > Hmm. This makes me wonder why this is an IIO driver rather than a clk
> > driver?  Definitely needs some more information in the patch description
> > or a cover letter.

+1.

...

> > > +   ret = device_property_read_string(dev, "clock-output-names",
> > > +                                     &st->clk_out_name);

Your driver is OF dependent, why to bother with device property API?

> > > +   if ((ret < 0) && dev->of_node)

Oh, this is bad. Why do you need to check for OF node at all?!

> > > +           st->clk_out_name = dev->of_node->name;

...

> > > +           /*
> > > +            * ADF4360-7 to ADF4360-9 have a VCO that is tuned to a specific
> > > +            * range using an external inductor. These properties describe
> > > +            * the range selected by the external inductor.
> > > +            */
> > > +           ret = device_property_read_u32(dev,
> > > +                                          "adi,vco-minimum-frequency-hz",
> > > +                                          &tmp);
> > > +           if (ret == 0)
> > > +                   st->vco_min = max(st->info->vco_min, tmp);
> > > +           else
> > > +                   st->vco_min = st->info->vco_min;

Why to use tmp at all?

                                          &st->vco_min);
           if (ret)
                   st->vco_min = st->info->vco_min;

           st->vco_min = max(st->info->vco_min, st->vco_min);

> > > +           ret = device_property_read_u32(dev,
> > > +                                          "adi,vco-maximum-frequency-hz",
> > > +                                          &tmp);
> > > +           if (ret == 0)
> > > +                   st->vco_max = min(st->info->vco_max, tmp);
> > > +           else
> > > +                   st->vco_max = st->info->vco_max;

Ditto.

> > > +   ret = device_property_read_u32(dev,
> > > +                                  "adi,loop-filter-pfd-frequency-hz",
> > > +                                  &tmp);
> > > +   if (ret == 0) {
> > > +           st->pfd_freq = tmp;

Ditto!

> > > +   } else {
> > > +           dev_err(dev, "PFD frequency property missing\n");
> > > +           return ret;
> > > +   }

> > > +
> > > +   ret = device_property_read_u32(dev,
> > > +                           "adi,loop-filter-charge-pump-current-microamp",
> > > +                           &tmp);
> > > +   if (ret == 0) {
> > > +           st->cpi = find_closest(tmp, adf4360_cpi_modes,
> > > +                                  ARRAY_SIZE(adf4360_cpi_modes));

Ditto!

> > > +   } else {
> > > +           dev_err(dev, "CPI property missing\n");
> > > +           return ret;
> > > +   }
> > > +
> > > +   ret = device_property_read_u32(dev, "adi,power-up-frequency-hz", &tmp);
> > > +   if (ret == 0)
> > > +           st->freq_req = tmp;
> > > +   else
> > > +           st->freq_req = st->vco_min;

Ditto.

> > > +   ret = device_property_read_u32(dev, "adi,power-out-level-microamp",
> > > +                                  &tmp);
> > > +   if (ret == 0)
> > > +           st->power_level = find_closest(tmp, adf4360_power_lvl_microamp,
> > > +                                   ARRAY_SIZE(adf4360_power_lvl_microamp));
> > > +   else
> > > +           st->power_level = ADF4360_PL_5;

Ditto.

...

> > > +   if (spi->dev.of_node)
> > > +           indio_dev->name = spi->dev.of_node->name;

Use %pOFn instead

> > > +   else
> > > +           indio_dev->name = spi_get_device_id(spi)->name;

...

> > > +static const struct of_device_id adf4360_of_match[] = {
> > > +   { .compatible = "adi,adf4360-0", },
> > > +   { .compatible = "adi,adf4360-1", },
> > > +   { .compatible = "adi,adf4360-2", },
> > > +   { .compatible = "adi,adf4360-3", },
> > > +   { .compatible = "adi,adf4360-4", },
> > > +   { .compatible = "adi,adf4360-5", },
> > > +   { .compatible = "adi,adf4360-6", },
> > > +   { .compatible = "adi,adf4360-7", },
> > > +   { .compatible = "adi,adf4360-8", },
> > > +   { .compatible = "adi,adf4360-9", },

> > > +   {},

No comma here.

> > > +};
diff mbox series

Patch

diff --git a/drivers/iio/frequency/Kconfig b/drivers/iio/frequency/Kconfig
index 240b81502512..781236496661 100644
--- a/drivers/iio/frequency/Kconfig
+++ b/drivers/iio/frequency/Kconfig
@@ -39,6 +39,17 @@  config ADF4350
 	  To compile this driver as a module, choose M here: the
 	  module will be called adf4350.
 
+config ADF4360
+	tristate "Analog Devices ADF4360 Wideband Synthesizers"
+	depends on SPI
+	depends on COMMON_CLK
+	help
+	  Say yes here to build support for Analog Devices ADF4360
+	  Wideband Synthesizers. The driver provides direct access via sysfs.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called adf4360.
+
 config ADF4371
 	tristate "Analog Devices ADF4371/ADF4372 Wideband Synthesizers"
 	depends on SPI
diff --git a/drivers/iio/frequency/Makefile b/drivers/iio/frequency/Makefile
index 518b1e50caef..85f2f81da662 100644
--- a/drivers/iio/frequency/Makefile
+++ b/drivers/iio/frequency/Makefile
@@ -6,4 +6,5 @@ 
 # When adding new entries keep the list in alphabetical order
 obj-$(CONFIG_AD9523) += ad9523.o
 obj-$(CONFIG_ADF4350) += adf4350.o
+obj-$(CONFIG_ADF4360) += adf4360.o
 obj-$(CONFIG_ADF4371) += adf4371.o
diff --git a/drivers/iio/frequency/adf4360.c b/drivers/iio/frequency/adf4360.c
new file mode 100644
index 000000000000..49ad58092171
--- /dev/null
+++ b/drivers/iio/frequency/adf4360.c
@@ -0,0 +1,1263 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ADF4360 PLL with Integrated Synthesizer and VCO
+ *
+ * Copyright 2014-2019 Analog Devices Inc.
+ * Copyright 2019-2020 Edward Kigwana.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/consumer.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+#include <linux/util_macros.h>
+
+#include <linux/iio/iio.h>
+
+/* Register address macro */
+#define ADF4360_REG(x)			(x)
+
+/* ADF4360_CTRL */
+#define ADF4360_ADDR_CPL_MSK		GENMASK(3, 2)
+#define ADF4360_CPL(x)			FIELD_PREP(ADF4360_ADDR_CPL_MSK, x)
+#define ADF4360_ADDR_MUXOUT_MSK		GENMASK(7, 5)
+#define ADF4360_MUXOUT(x)		FIELD_PREP(ADF4360_ADDR_MUXOUT_MSK, x)
+#define ADF4360_ADDR_PDP_MSK		BIT(8)
+#define ADF4360_PDP(x)			FIELD_PREP(ADF4360_ADDR_PDP_MSK, x)
+#define ADF4360_ADDR_MTLD_MSK		BIT(11)
+#define ADF4360_MTLD(x)			FIELD_PREP(ADF4360_ADDR_MTLD_MSK, x)
+#define ADF4360_ADDR_OPL_MSK		GENMASK(13, 12)
+#define ADF4360_OPL(x)			FIELD_PREP(ADF4360_ADDR_OPL_MSK, x)
+#define ADF4360_ADDR_CPI1_MSK		GENMASK(16, 14)
+#define ADF4360_CPI1(x)			FIELD_PREP(ADF4360_ADDR_CPI1_MSK, x)
+#define ADF4360_ADDR_CPI2_MSK		GENMASK(19, 17)
+#define ADF4360_CPI2(x)			FIELD_PREP(ADF4360_ADDR_CPI2_MSK, x)
+#define ADF4360_ADDR_PWR_DWN_MSK	GENMASK(21, 20)
+#define ADF4360_POWERDOWN(x)		FIELD_PREP(ADF4360_ADDR_PWR_DWN_MSK, x)
+#define ADF4360_ADDR_PRESCALER_MSK	GENMASK(23, 22)
+#define ADF4360_PRESCALER(x)		FIELD_PREP(ADF4360_ADDR_PRESCALER_MSK, x)
+
+/* ADF4360_NDIV */
+#define ADF4360_ADDR_A_CNTR_MSK		GENMASK(6, 2)
+#define ADF4360_A_COUNTER(x)		FIELD_PREP(ADF4360_ADDR_A_CNTR_MSK, x)
+#define ADF4360_ADDR_B_CNTR_MSK		GENMASK(20, 8)
+#define ADF4360_B_COUNTER(x)		FIELD_PREP(ADF4360_ADDR_B_CNTR_MSK, x)
+#define ADF4360_ADDR_OUT_DIV2_MSK	BIT(22)
+#define ADF4360_OUT_DIV2(x)		FIELD_PREP(ADF4360_ADDR_OUT_DIV2_MSK, x)
+#define ADF4360_ADDR_DIV2_SEL_MSK	BIT(23)
+#define ADF4360_PRESCALER_DIV2(x)	FIELD_PREP(ADF4360_ADDR_DIV2_SEL_MSK, x)
+
+/* ADF4360_RDIV */
+#define ADF4360_ADDR_R_CNTR_MSK		GENMASK(15, 2)
+#define ADF4360_R_COUNTER(x)		FIELD_PREP(ADF4360_ADDR_R_CNTR_MSK, x)
+#define ADF4360_ADDR_ABP_MSK		GENMASK(17, 16)
+#define ADF4360_ABP(x)			FIELD_PREP(ADF4360_ADDR_ABP_MSK, x)
+#define ADF4360_ADDR_BSC_MSK		GENMASK(21, 20)
+#define ADF4360_BSC(x)			FIELD_PREP(ADF4360_ADDR_BSC_MSK, x)
+
+/* Specifications */
+#define ADF4360_MAX_PFD_RATE		8000000 /* 8 MHz */
+#define ADF4360_MAX_COUNTER_RATE	300000000 /* 300 MHz */
+#define ADF4360_MAX_REFIN_RATE		250000000 /* 250 MHz */
+
+enum {
+	ADF4360_CTRL,
+	ADF4360_RDIV,
+	ADF4360_NDIV,
+	ADF4360_REG_NUM,
+};
+
+enum {
+	ADF4360_GEN1_PC_5,
+	ADF4360_GEN1_PC_10,
+	ADF4360_GEN1_PC_15,
+	ADF4360_GEN1_PC_20,
+};
+
+enum {
+	ADF4360_GEN2_PC_2_5,
+	ADF4360_GEN2_PC_5,
+	ADF4360_GEN2_PC_7_5,
+	ADF4360_GEN2_PC_10,
+};
+
+enum {
+	ADF4360_MUXOUT_THREE_STATE,
+	ADF4360_MUXOUT_LOCK_DETECT,
+	ADF4360_MUXOUT_NDIV,
+	ADF4360_MUXOUT_DVDD,
+	ADF4360_MUXOUT_RDIV,
+	ADF4360_MUXOUT_OD_LD,
+	ADF4360_MUXOUT_SDO,
+	ADF4360_MUXOUT_GND,
+};
+
+enum {
+	ADF4360_PL_3_5,
+	ADF4360_PL_5,
+	ADF4360_PL_7_5,
+	ADF4360_PL_11,
+};
+
+enum {
+	ADF4360_CPI_0_31,
+	ADF4360_CPI_0_62,
+	ADF4360_CPI_0_93,
+	ADF4360_CPI_1_25,
+	ADF4360_CPI_1_56,
+	ADF4360_CPI_1_87,
+	ADF4360_CPI_2_18,
+	ADF4360_CPI_2_50,
+};
+
+enum {
+	ADF4360_POWER_DOWN_NORMAL,
+	ADF4360_POWER_DOWN_SOFT_ASYNC,
+	ADF4360_POWER_DOWN_CE,
+	ADF4360_POWER_DOWN_SOFT_SYNC,
+	ADF4360_POWER_DOWN_REGULATOR,
+};
+
+enum {
+	ADF4360_PRESCALER_8,
+	ADF4360_PRESCALER_16,
+	ADF4360_PRESCALER_32,
+};
+
+enum {
+	ADF4360_ABP_3_0NS,
+	ADF4360_ABP_1_3NS,
+	ADF4360_ABP_6_0NS,
+};
+
+enum {
+	ADF4360_BSC_1,
+	ADF4360_BSC_2,
+	ADF4360_BSC_4,
+	ADF4360_BSC_8,
+};
+
+enum {
+	ID_ADF4360_0,
+	ID_ADF4360_1,
+	ID_ADF4360_2,
+	ID_ADF4360_3,
+	ID_ADF4360_4,
+	ID_ADF4360_5,
+	ID_ADF4360_6,
+	ID_ADF4360_7,
+	ID_ADF4360_8,
+	ID_ADF4360_9,
+};
+
+enum {
+	ADF4360_FREQ_REFIN,
+	ADF4360_MTLD,
+	ADF4360_FREQ_PFD,
+};
+
+static const char * const adf4360_power_level_modes[] = {
+	[ADF4360_PL_3_5] = "3500-uA",
+	[ADF4360_PL_5] = "5000-uA",
+	[ADF4360_PL_7_5] = "7500-uA",
+	[ADF4360_PL_11] = "11000-uA",
+};
+
+static const unsigned int adf4360_power_lvl_microamp[] = {
+	[ADF4360_PL_3_5] = 3500,
+	[ADF4360_PL_5] = 5000,
+	[ADF4360_PL_7_5] = 7500,
+	[ADF4360_PL_11] = 11000,
+};
+
+static const unsigned int adf4360_cpi_modes[] = {
+	[ADF4360_CPI_0_31] = 310,
+	[ADF4360_CPI_0_62] = 620,
+	[ADF4360_CPI_0_93] = 930,
+	[ADF4360_CPI_1_25] = 1250,
+	[ADF4360_CPI_1_56] = 1560,
+	[ADF4360_CPI_1_87] = 1870,
+	[ADF4360_CPI_2_18] = 2180,
+	[ADF4360_CPI_2_50] = 2500,
+};
+
+static const char * const adf4360_muxout_modes[] = {
+	[ADF4360_MUXOUT_THREE_STATE] = "three-state",
+	[ADF4360_MUXOUT_LOCK_DETECT] = "lock-detect",
+	[ADF4360_MUXOUT_NDIV] = "ndiv",
+	[ADF4360_MUXOUT_DVDD] = "dvdd",
+	[ADF4360_MUXOUT_RDIV] = "rdiv",
+	[ADF4360_MUXOUT_OD_LD] = "od-ld",
+	[ADF4360_MUXOUT_SDO] = "sdo",
+	[ADF4360_MUXOUT_GND] = "gnd",
+};
+
+static const char * const adf4360_power_down_modes[] = {
+	[ADF4360_POWER_DOWN_NORMAL] = "normal",
+	[ADF4360_POWER_DOWN_SOFT_ASYNC] = "soft-async",
+	[ADF4360_POWER_DOWN_CE] = "ce",
+	[ADF4360_POWER_DOWN_SOFT_SYNC] = "soft-sync",
+	[ADF4360_POWER_DOWN_REGULATOR] = "regulator",
+};
+
+struct adf4360_output {
+	struct clk_hw hw;
+	struct iio_dev *indio_dev;
+};
+
+#define to_output(_hw) container_of(_hw, struct adf4360_output, hw)
+
+struct adf4360_chip_info {
+	unsigned int vco_min;
+	unsigned int vco_max;
+	unsigned int default_cpl;
+};
+
+struct adf4360_state {
+	struct spi_device *spi;
+	const struct adf4360_chip_info *info;
+	struct adf4360_output output;
+	struct clk *clkin;
+	struct gpio_desc *muxout_gpio;
+	struct gpio_desc *chip_en_gpio;
+	struct regulator *vdd_reg;
+	struct mutex lock; /* Protect PLL state. */
+	unsigned int part_id;
+	unsigned long clkin_freq;
+	unsigned long freq_req;
+	unsigned long r;
+	unsigned long n;
+	unsigned int vco_min;
+	unsigned int vco_max;
+	unsigned int pfd_freq;
+	unsigned int cpi;
+	bool pdp;
+	bool mtld;
+	unsigned int power_level;
+	unsigned int muxout_mode;
+	unsigned int power_down_mode;
+	bool initial_reg_seq;
+	const char *clk_out_name;
+	unsigned int regs[ADF4360_REG_NUM];
+	u8 spi_data[3] ____cacheline_aligned;
+};
+
+static const struct adf4360_chip_info adf4360_chip_info_tbl[] = {
+	{	/* ADF4360-0 */
+		.vco_min = 2400000000U,
+		.vco_max = 2725000000U,
+		.default_cpl = ADF4360_GEN1_PC_10,
+	}, {	/* ADF4360-1 */
+		.vco_min = 2050000000U,
+		.vco_max = 2450000000U,
+		.default_cpl = ADF4360_GEN1_PC_15,
+	}, {	/* ADF4360-2 */
+		.vco_min = 1850000000U,
+		.vco_max = 2170000000U,
+		.default_cpl = ADF4360_GEN1_PC_15,
+	}, {	/* ADF4360-3 */
+		.vco_min = 1600000000U,
+		.vco_max = 1950000000U,
+		.default_cpl = ADF4360_GEN1_PC_15,
+	}, {	/* ADF4360-4 */
+		.vco_min = 1450000000U,
+		.vco_max = 1750000000U,
+		.default_cpl = ADF4360_GEN1_PC_15,
+	}, {	/* ADF4360-5 */
+		.vco_min = 1200000000U,
+		.vco_max = 1400000000U,
+		.default_cpl = ADF4360_GEN1_PC_10,
+	}, {	/* ADF4360-6 */
+		.vco_min = 1050000000U,
+		.vco_max = 1250000000U,
+		.default_cpl = ADF4360_GEN1_PC_10,
+	}, {	/* ADF4360-7 */
+		.vco_min = 350000000U,
+		.vco_max = 1800000000U,
+		.default_cpl = ADF4360_GEN1_PC_5,
+	}, {	/* ADF4360-8 */
+		.vco_min = 65000000U,
+		.vco_max = 400000000U,
+		.default_cpl = ADF4360_GEN2_PC_5,
+	}, {	/* ADF4360-9 */
+		.vco_min = 65000000U,
+		.vco_max = 400000000U,
+		.default_cpl = ADF4360_GEN2_PC_5,
+	}
+};
+
+static int adf4360_write_reg(struct adf4360_state *st, unsigned int reg,
+			     unsigned int val)
+{
+	int ret;
+
+	val |= reg;
+
+	st->spi_data[0] = (val >> 16) & 0xff;
+	st->spi_data[1] = (val >> 8) & 0xff;
+	st->spi_data[2] = val & 0xff;
+
+	ret = spi_write(st->spi, st->spi_data, ARRAY_SIZE(st->spi_data));
+	if (ret == 0)
+		st->regs[reg] = val;
+
+	return ret;
+}
+
+/* fVCO = B * fREFIN / R */
+
+static unsigned long adf4360_clk_recalc_rate(struct clk_hw *hw,
+					     unsigned long parent_rate)
+{
+	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
+	struct adf4360_state *st = iio_priv(indio_dev);
+
+	if (st->r == 0)
+		return 0;
+
+	/*
+	 * The result is guaranteed to fit in 32-bit, but the intermediate
+	 * result might require 64-bit.
+	 */
+	return DIV_ROUND_CLOSEST_ULL((uint64_t)parent_rate * st->n, st->r);
+}
+
+static unsigned int adf4360_calc_prescaler(unsigned int pfd_freq,
+					   unsigned int n,
+					   unsigned int *out_p,
+					   unsigned int *out_a,
+					   unsigned int *out_b)
+{
+	unsigned int rate = pfd_freq * n;
+	unsigned int p, a, b;
+
+	/* Make sure divider counter input frequency is low enough */
+	p = 8;
+	while (p < 32 && rate / p > ADF4360_MAX_COUNTER_RATE)
+		p *= 2;
+
+	/*
+	 * The range of dividers that can be produced using the dual-modulus
+	 * pre-scaler is not continuous for values of n < p*(p-1). If we end up
+	 * with a non supported divider value, pick the next closest one.
+	 */
+	a = n % p;
+	b = n / p;
+
+	if (b < 3) {
+		b = 3;
+		a = 0;
+	} else if (a > b) {
+		if (a - b < p - a) {
+			a = b;
+		} else {
+			a = 0;
+			b++;
+		}
+	}
+
+	if (out_p)
+		*out_p = p;
+	if (out_a)
+		*out_a = a;
+	if (out_b)
+		*out_b = b;
+
+	return p * b + a;
+}
+
+static long adf4360_clk_round_rate(struct clk_hw *hw,
+				   unsigned long rate,
+				   unsigned long *parent_rate)
+{
+	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
+	struct adf4360_state *st = iio_priv(indio_dev);
+	unsigned int r, n;
+	unsigned int pfd_freq;
+
+	if (*parent_rate == 0)
+		return 0;
+
+	if (st->part_id == ID_ADF4360_9)
+		return *parent_rate * st->n / st->r;
+
+	if (rate > st->vco_max)
+		return st->vco_max;
+
+	/* ADF4360-0 to AD4370-7 have an optional by two divider */
+	if (st->part_id <= ID_ADF4360_7) {
+		if (rate < st->vco_min / 2)
+			return st->vco_min / 2;
+		if (rate < st->vco_min && rate > st->vco_max / 2) {
+			if (st->vco_min - rate < rate - st->vco_max / 2)
+				return st->vco_min;
+			else
+				return st->vco_max / 2;
+		}
+	} else {
+		if (rate < st->vco_min)
+			return st->vco_min;
+	}
+
+	r = DIV_ROUND_CLOSEST(*parent_rate, st->pfd_freq);
+	pfd_freq = *parent_rate / r;
+	n = DIV_ROUND_CLOSEST(rate, pfd_freq);
+
+	if (st->part_id <= ID_ADF4360_7)
+		n = adf4360_calc_prescaler(pfd_freq, n, NULL, NULL, NULL);
+
+	return pfd_freq * n;
+}
+
+static inline bool adf4360_is_powerdown(struct adf4360_state *st)
+{
+	return (st->power_down_mode != ADF4360_POWER_DOWN_NORMAL);
+}
+
+static int adf4360_set_freq(struct adf4360_state *st, unsigned long rate)
+{
+	unsigned int val_r, val_n, val_ctrl;
+	unsigned int pfd_freq;
+	unsigned long r, n;
+	int ret;
+
+	if (!st->clkin_freq || (st->clkin_freq > ADF4360_MAX_REFIN_RATE))
+		return -EINVAL;
+
+	if ((rate < st->vco_min) || (rate > st->vco_max))
+		return -EINVAL;
+
+	if (adf4360_is_powerdown(st))
+		ret = -EBUSY;
+
+	r = DIV_ROUND_CLOSEST(st->clkin_freq, st->pfd_freq);
+	pfd_freq = st->clkin_freq / r;
+	n = DIV_ROUND_CLOSEST(rate, pfd_freq);
+
+	val_ctrl = ADF4360_CPL(st->info->default_cpl) |
+		   ADF4360_MUXOUT(st->muxout_mode) |
+		   ADF4360_PDP(!st->pdp) |
+		   ADF4360_MTLD(st->mtld) |
+		   ADF4360_OPL(st->power_level) |
+		   ADF4360_CPI1(st->cpi) |
+		   ADF4360_CPI2(st->cpi) |
+		   ADF4360_POWERDOWN(st->power_down_mode);
+
+	/* ADF4360-0 to ADF4360-7 have a dual-modulous prescaler */
+	if (st->part_id <= ID_ADF4360_7) {
+		unsigned int p, a, b;
+
+		n = adf4360_calc_prescaler(pfd_freq, n, &p, &a, &b);
+
+		switch (p) {
+		case 8:
+			val_ctrl |= ADF4360_PRESCALER(ADF4360_PRESCALER_8);
+			break;
+		case 16:
+			val_ctrl |= ADF4360_PRESCALER(ADF4360_PRESCALER_16);
+			break;
+		default:
+			val_ctrl |= ADF4360_PRESCALER(ADF4360_PRESCALER_32);
+			break;
+		}
+
+		val_n = ADF4360_A_COUNTER(a) |
+			ADF4360_B_COUNTER(b);
+
+		if (rate < st->vco_min)
+			val_n |= ADF4360_OUT_DIV2(true) |
+				 ADF4360_PRESCALER_DIV2(true);
+	} else {
+		val_n = ADF4360_B_COUNTER(n);
+	}
+
+	/*
+	 * Always use BSC divider of 8, see Analog Devices AN-1347.
+	 * http://www.analog.com/media/en/technical-documentation/application-notes/AN-1347.pdf
+	 */
+	val_r = ADF4360_R_COUNTER(r) |
+		ADF4360_ABP(ADF4360_ABP_3_0NS) |
+		ADF4360_BSC(ADF4360_BSC_8);
+
+	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_RDIV), val_r);
+	if (ret)
+		return ret;
+
+	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val_ctrl);
+	if (ret)
+		return ret;
+
+	/*
+	 * Allow the transient behavior of the ADF4360-7 during initial
+	 * power-up to settle.
+	 */
+	if (st->initial_reg_seq) {
+		usleep_range(15000, 20000);
+		st->initial_reg_seq = false;
+	}
+
+	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_NDIV), val_n);
+	if (ret)
+		return ret;
+
+	st->freq_req = rate;
+	st->n = n;
+	st->r = r;
+
+	return 0;
+}
+
+static int adf4360_clk_set_rate(struct clk_hw *hw,
+				unsigned long rate,
+				unsigned long parent_rate)
+{
+	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
+	struct adf4360_state *st = iio_priv(indio_dev);
+	int ret;
+
+	if ((parent_rate == 0) || (parent_rate > ADF4360_MAX_REFIN_RATE))
+		return -EINVAL;
+
+	mutex_lock(&st->lock);
+	if (st->clkin_freq != parent_rate)
+		st->clkin_freq = parent_rate;
+
+	ret = adf4360_set_freq(st, rate);
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int __adf4360_power_down(struct adf4360_state *st, unsigned int mode)
+{
+	struct device *dev = &st->spi->dev;
+	unsigned int val;
+	int ret = 0;
+
+	switch (mode) {
+	case ADF4360_POWER_DOWN_NORMAL:
+		if (st->vdd_reg) {
+			ret = regulator_enable(st->vdd_reg);
+			if (ret) {
+				dev_err(dev, "Supply enable error: %d\n", ret);
+				return ret;
+			}
+		}
+
+		st->initial_reg_seq = true;
+		st->power_down_mode = mode;
+		ret = adf4360_set_freq(st, st->freq_req);
+		break;
+	case ADF4360_POWER_DOWN_SOFT_ASYNC: /* fallthrough */
+	case ADF4360_POWER_DOWN_SOFT_SYNC:
+		val = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_MUXOUT_MSK;
+		val |= ADF4360_POWERDOWN(mode);
+		ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
+		break;
+	case ADF4360_POWER_DOWN_CE:
+		if (st->chip_en_gpio)
+			gpiod_set_value(st->chip_en_gpio, 0x0);
+		else
+			return -ENODEV;
+		break;
+	case ADF4360_POWER_DOWN_REGULATOR:
+		if (!st->vdd_reg)
+			return -ENODEV;
+
+		if (st->chip_en_gpio)
+			ret = __adf4360_power_down(st, ADF4360_POWER_DOWN_CE);
+		else
+			ret = __adf4360_power_down(st,
+						ADF4360_POWER_DOWN_SOFT_ASYNC);
+
+		ret = regulator_disable(st->vdd_reg);
+		if (ret)
+			dev_err(dev, "Supply disable error: %d\n", ret);
+		break;
+	}
+	if (ret == 0)
+		st->power_down_mode = mode;
+
+	return 0;
+}
+
+static int adf4360_power_down(struct adf4360_state *st, unsigned int mode)
+{
+	int ret;
+
+	mutex_lock(&st->lock);
+	ret = __adf4360_power_down(st, mode);
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int adf4360_clk_prepare(struct clk_hw *hw)
+{
+	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
+	struct adf4360_state *st = iio_priv(indio_dev);
+
+	return adf4360_power_down(st, ADF4360_POWER_DOWN_NORMAL);
+}
+
+static void adf4360_clk_unprepare(struct clk_hw *hw)
+{
+	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
+	struct adf4360_state *st = iio_priv(indio_dev);
+
+	adf4360_power_down(st, ADF4360_POWER_DOWN_SOFT_ASYNC);
+}
+
+static int adf4360_clk_enable(struct clk_hw *hw)
+{
+	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
+	struct adf4360_state *st = iio_priv(indio_dev);
+
+	if (st->chip_en_gpio)
+		gpiod_set_value(st->chip_en_gpio, 0x1);
+
+	return 0;
+}
+
+static void adf4360_clk_disable(struct clk_hw *hw)
+{
+	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
+	struct adf4360_state *st = iio_priv(indio_dev);
+
+	if (st->chip_en_gpio)
+		adf4360_power_down(st, ADF4360_POWER_DOWN_CE);
+}
+
+static int adf4360_clk_is_enabled(struct clk_hw *hw)
+{
+	struct iio_dev *indio_dev = to_output(hw)->indio_dev;
+	struct adf4360_state *st = iio_priv(indio_dev);
+
+	return adf4360_is_powerdown(st);
+}
+
+static const struct clk_ops adf4360_clk_ops = {
+	.recalc_rate = adf4360_clk_recalc_rate,
+	.round_rate = adf4360_clk_round_rate,
+	.set_rate = adf4360_clk_set_rate,
+	.prepare = adf4360_clk_prepare,
+	.unprepare = adf4360_clk_unprepare,
+	.enable = adf4360_clk_enable,
+	.disable = adf4360_clk_disable,
+	.is_enabled = adf4360_clk_is_enabled,
+};
+
+static ssize_t adf4360_read(struct iio_dev *indio_dev,
+			    uintptr_t private,
+			    const struct iio_chan_spec *chan,
+			    char *buf)
+{
+	struct adf4360_state *st = iio_priv(indio_dev);
+	unsigned long val;
+	int ret = 0;
+
+	switch ((u32)private) {
+	case ADF4360_FREQ_REFIN:
+		val = st->clkin_freq;
+		break;
+	case ADF4360_MTLD:
+		val = st->mtld;
+		break;
+	case ADF4360_FREQ_PFD:
+		val = st->pfd_freq;
+		break;
+	default:
+		ret = -EINVAL;
+		val = 0;
+	}
+
+	return ret < 0 ? ret : sprintf(buf, "%lu\n", val);
+}
+
+static ssize_t adf4360_write(struct iio_dev *indio_dev,
+			     uintptr_t private,
+			     const struct iio_chan_spec *chan,
+			     const char *buf, size_t len)
+{
+	struct adf4360_state *st = iio_priv(indio_dev);
+	unsigned long readin, tmp;
+	bool mtld;
+	int ret = 0;
+
+	mutex_lock(&st->lock);
+	switch ((u32)private) {
+	case ADF4360_FREQ_REFIN:
+		ret = kstrtoul(buf, 10, &readin);
+		if (ret)
+			break;
+
+		if ((readin > ADF4360_MAX_REFIN_RATE) || (readin == 0)) {
+			ret = -EINVAL;
+			break;
+		}
+
+		if (st->clkin) {
+			tmp = clk_round_rate(st->clkin, readin);
+			if (tmp != readin) {
+				ret = -EINVAL;
+				break;
+			}
+
+			ret = clk_set_rate(st->clkin, tmp);
+			if (ret)
+				break;
+		}
+
+		st->clkin_freq = readin;
+		break;
+	case ADF4360_MTLD:
+		ret = kstrtobool(buf, &mtld);
+		if (ret)
+			break;
+
+		st->mtld = mtld;
+		break;
+	case ADF4360_FREQ_PFD:
+		ret = kstrtoul(buf, 10, &readin);
+		if (ret)
+			break;
+
+		if ((readin > ADF4360_MAX_PFD_RATE) || (readin == 0)) {
+			ret = -EINVAL;
+			break;
+		}
+
+		st->pfd_freq = readin;
+		break;
+	default:
+		ret = -EINVAL;
+	}
+
+	if (ret == 0)
+		ret = adf4360_set_freq(st, st->freq_req);
+	mutex_unlock(&st->lock);
+
+	return ret ? ret : len;
+}
+
+static int adf4360_get_muxout_mode(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan)
+{
+	struct adf4360_state *st = iio_priv(indio_dev);
+
+	return st->muxout_mode;
+}
+
+static int adf4360_set_muxout_mode(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan,
+				   unsigned int mode)
+{
+	struct adf4360_state *st = iio_priv(indio_dev);
+	unsigned int writeval;
+	int ret = 0;
+
+	mutex_lock(&st->lock);
+	writeval = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_MUXOUT_MSK;
+	writeval |= ADF4360_MUXOUT(mode & 0x7);
+	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), writeval);
+	if (ret == 0)
+		st->muxout_mode = mode & 0x7;
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static const struct iio_enum adf4360_muxout_modes_available = {
+	.items = adf4360_muxout_modes,
+	.num_items = ARRAY_SIZE(adf4360_muxout_modes),
+	.get = adf4360_get_muxout_mode,
+	.set = adf4360_set_muxout_mode,
+};
+
+static int adf4360_get_power_down(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan)
+{
+	struct adf4360_state *st = iio_priv(indio_dev);
+
+	return st->power_down_mode;
+}
+
+static int adf4360_set_power_down(struct iio_dev *indio_dev,
+				  const struct iio_chan_spec *chan,
+				  unsigned int mode)
+{
+	struct adf4360_state *st = iio_priv(indio_dev);
+
+	return adf4360_power_down(st, mode);
+}
+
+static const struct iio_enum adf4360_pwr_dwn_modes_available = {
+	.items = adf4360_power_down_modes,
+	.num_items = ARRAY_SIZE(adf4360_power_down_modes),
+	.get = adf4360_get_power_down,
+	.set = adf4360_set_power_down,
+};
+
+static int adf4360_get_power_level(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan)
+{
+	struct adf4360_state *st = iio_priv(indio_dev);
+
+	return st->power_level;
+}
+
+static int adf4360_set_power_level(struct iio_dev *indio_dev,
+				   const struct iio_chan_spec *chan,
+				   unsigned int level)
+{
+	struct adf4360_state *st = iio_priv(indio_dev);
+	unsigned int val;
+	int ret;
+
+	if (adf4360_is_powerdown(st))
+		return -EBUSY;
+
+	mutex_lock(&st->lock);
+	val = st->regs[ADF4360_CTRL] & ~ADF4360_ADDR_OPL_MSK;
+	val |= ADF4360_OPL(level);
+	ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
+	st->power_level = level;
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static const struct iio_enum adf4360_pwr_lvl_modes_available = {
+	.items = adf4360_power_level_modes,
+	.num_items = ARRAY_SIZE(adf4360_power_level_modes),
+	.get = adf4360_get_power_level,
+	.set = adf4360_set_power_level,
+};
+
+#define _ADF4360_EXT_INFO(_name, _ident) { \
+	.name = _name, \
+	.read = adf4360_read, \
+	.write = adf4360_write, \
+	.private = _ident, \
+	.shared = IIO_SEPARATE, \
+}
+
+static const struct iio_chan_spec_ext_info adf4360_ext_info[] = {
+	_ADF4360_EXT_INFO("refin_frequency", ADF4360_FREQ_REFIN),
+	_ADF4360_EXT_INFO("mute_till_lock_detect", ADF4360_MTLD),
+	_ADF4360_EXT_INFO("pfd_frequency", ADF4360_FREQ_PFD),
+	IIO_ENUM_AVAILABLE("muxout_mode", &adf4360_muxout_modes_available),
+	IIO_ENUM("muxout_mode", false, &adf4360_muxout_modes_available),
+	IIO_ENUM_AVAILABLE("power_down", &adf4360_pwr_dwn_modes_available),
+	IIO_ENUM("power_down", false, &adf4360_pwr_dwn_modes_available),
+	IIO_ENUM_AVAILABLE("power_level", &adf4360_pwr_lvl_modes_available),
+	IIO_ENUM("power_level", false, &adf4360_pwr_lvl_modes_available),
+	{ },
+};
+
+static const struct iio_chan_spec adf4360_chan = {
+	.type = IIO_ALTVOLTAGE,
+	.indexed = 1,
+	.output = 1,
+	.info_mask_separate = BIT(IIO_CHAN_INFO_FREQUENCY),
+	.ext_info = adf4360_ext_info,
+};
+
+static int adf4360_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val,
+			    int *val2,
+			    long mask)
+{
+	struct adf4360_state *st = iio_priv(indio_dev);
+	bool lk_det;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_FREQUENCY:
+		if (adf4360_is_powerdown(st))
+			return -EBUSY;
+
+		lk_det = (ADF4360_MUXOUT_LOCK_DETECT | ADF4360_MUXOUT_OD_LD) &
+			 st->muxout_mode;
+		if (lk_det && st->muxout_gpio) {
+			if (!gpiod_get_value(st->muxout_gpio)) {
+				dev_dbg(&st->spi->dev, "PLL un-locked\n");
+				return -EBUSY;
+			}
+		}
+
+		*val = st->freq_req;
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+};
+
+static int adf4360_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val,
+			     int val2,
+			     long mask)
+{
+	struct adf4360_state *st = iio_priv(indio_dev);
+	int ret;
+
+	mutex_lock(&st->lock);
+	switch (mask) {
+	case IIO_CHAN_INFO_FREQUENCY:
+		ret = adf4360_set_freq(st, val);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static int adf4360_reg_access(struct iio_dev *indio_dev,
+			      unsigned int reg,
+			      unsigned int writeval,
+			      unsigned int *readval)
+{
+	struct adf4360_state *st = iio_priv(indio_dev);
+	int ret = 0;
+
+	if (reg >= ADF4360_REG_NUM)
+		return -EFAULT;
+
+	mutex_lock(&st->lock);
+	if (readval) {
+		*readval = st->regs[reg];
+	} else {
+		writeval &= 0xFFFFFC;
+		ret = adf4360_write_reg(st, reg, writeval);
+	}
+	mutex_unlock(&st->lock);
+
+	return ret;
+}
+
+static const struct iio_info adf4360_iio_info = {
+	.read_raw = &adf4360_read_raw,
+	.write_raw = &adf4360_write_raw,
+	.debugfs_reg_access = &adf4360_reg_access,
+};
+
+static int adf4360_get_gpio(struct adf4360_state *st)
+{
+	struct device *dev = &st->spi->dev;
+	unsigned int val;
+	int ret, i;
+
+	st->chip_en_gpio = devm_gpiod_get_optional(dev, "enable",
+						   GPIOD_OUT_HIGH);
+	if (IS_ERR(st->chip_en_gpio)) {
+		dev_err(dev, "Chip enable GPIO error\n");
+		return PTR_ERR(st->chip_en_gpio);
+	}
+
+	if (st->chip_en_gpio)
+		st->power_down_mode = ADF4360_POWER_DOWN_CE;
+
+	st->muxout_gpio = devm_gpiod_get_optional(dev, "adi,muxout", GPIOD_IN);
+	if (IS_ERR(st->muxout_gpio)) {
+		dev_err(dev, "Muxout GPIO error\n");
+		return PTR_ERR(st->muxout_gpio);
+	}
+
+	if (!st->muxout_gpio)
+		return 0;
+
+	/* ADF4360 PLLs are write only devices, try to probe using GPIO. */
+	for (i = 0; i < 4; i++) {
+		if (i & 1)
+			val = ADF4360_MUXOUT(ADF4360_MUXOUT_DVDD);
+		else
+			val = ADF4360_MUXOUT(ADF4360_MUXOUT_GND);
+
+		ret = adf4360_write_reg(st, ADF4360_REG(ADF4360_CTRL), val);
+		if (ret)
+			return ret;
+
+		ret = gpiod_get_value(st->muxout_gpio);
+		if (ret ^ (i & 1)) {
+			dev_err(dev, "Probe failed (muxout)");
+			return -ENODEV;
+		}
+	}
+
+	return 0;
+}
+
+static void adf4360_clkin_disable(void *data)
+{
+	struct adf4360_state *st = data;
+
+	clk_disable_unprepare(st->clkin);
+}
+
+static int adf4360_get_clkin(struct adf4360_state *st)
+{
+	struct device *dev = &st->spi->dev;
+	struct clk *clk;
+	int ret;
+
+	clk = devm_clk_get(dev, "clkin");
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, adf4360_clkin_disable, st);
+	if (ret)
+		return ret;
+
+	st->clkin = clk;
+	st->clkin_freq = clk_get_rate(clk);
+
+	return 0;
+}
+
+static void adf4360_clk_del_provider(void *data)
+{
+	struct adf4360_state *st = data;
+
+	of_clk_del_provider(st->spi->dev.of_node);
+}
+
+static int adf4360_clk_register(struct adf4360_state *st)
+{
+	struct spi_device *spi = st->spi;
+	struct clk_init_data init;
+	struct clk *clk;
+	const char *parent_name;
+	int ret;
+
+	parent_name = of_clk_get_parent_name(spi->dev.of_node, 0);
+	if (!parent_name)
+		return -EINVAL;
+
+	init.name = st->clk_out_name;
+	init.ops = &adf4360_clk_ops;
+	init.flags = CLK_SET_RATE_GATE;
+	init.parent_names = &parent_name;
+	init.num_parents = 1;
+
+	st->output.hw.init = &init;
+
+	clk = devm_clk_register(&spi->dev, &st->output.hw);
+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	ret = of_clk_add_provider(spi->dev.of_node, of_clk_src_simple_get, clk);
+	if (ret)
+		return ret;
+
+	return devm_add_action_or_reset(&spi->dev, adf4360_clk_del_provider, st);
+}
+
+static int adf4360_parse_dt(struct adf4360_state *st)
+{
+	struct device *dev = &st->spi->dev;
+	u32 tmp;
+	int ret;
+
+	ret = device_property_read_string(dev, "clock-output-names",
+					  &st->clk_out_name);
+	if ((ret < 0) && dev->of_node)
+		st->clk_out_name = dev->of_node->name;
+
+	if (st->part_id >= ID_ADF4360_7) {
+		/*
+		 * ADF4360-7 to ADF4360-9 have a VCO that is tuned to a specific
+		 * range using an external inductor. These properties describe
+		 * the range selected by the external inductor.
+		 */
+		ret = device_property_read_u32(dev,
+					       "adi,vco-minimum-frequency-hz",
+					       &tmp);
+		if (ret == 0)
+			st->vco_min = max(st->info->vco_min, tmp);
+		else
+			st->vco_min = st->info->vco_min;
+
+		ret = device_property_read_u32(dev,
+					       "adi,vco-maximum-frequency-hz",
+					       &tmp);
+		if (ret == 0)
+			st->vco_max = min(st->info->vco_max, tmp);
+		else
+			st->vco_max = st->info->vco_max;
+	} else {
+		st->vco_min = st->info->vco_min;
+		st->vco_max = st->info->vco_max;
+	}
+
+	st->pdp = device_property_read_bool(dev, "adi,loop-filter-inverting");
+
+	ret = device_property_read_u32(dev,
+				       "adi,loop-filter-pfd-frequency-hz",
+				       &tmp);
+	if (ret == 0) {
+		st->pfd_freq = tmp;
+	} else {
+		dev_err(dev, "PFD frequency property missing\n");
+		return ret;
+	}
+
+	ret = device_property_read_u32(dev,
+				"adi,loop-filter-charge-pump-current-microamp",
+				&tmp);
+	if (ret == 0) {
+		st->cpi = find_closest(tmp, adf4360_cpi_modes,
+				       ARRAY_SIZE(adf4360_cpi_modes));
+	} else {
+		dev_err(dev, "CPI property missing\n");
+		return ret;
+	}
+
+	ret = device_property_read_u32(dev, "adi,power-up-frequency-hz", &tmp);
+	if (ret == 0)
+		st->freq_req = tmp;
+	else
+		st->freq_req = st->vco_min;
+
+	ret = device_property_read_u32(dev, "adi,power-out-level-microamp",
+				       &tmp);
+	if (ret == 0)
+		st->power_level = find_closest(tmp, adf4360_power_lvl_microamp,
+					ARRAY_SIZE(adf4360_power_lvl_microamp));
+	else
+		st->power_level = ADF4360_PL_5;
+
+	return 0;
+}
+
+static int adf4360_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	const struct spi_device_id *id = spi_get_device_id(spi);
+	struct adf4360_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	mutex_init(&st->lock);
+
+	spi_set_drvdata(spi, indio_dev);
+
+	st->spi = spi;
+	st->info = &adf4360_chip_info_tbl[id->driver_data];
+	st->part_id = id->driver_data;
+	st->muxout_mode = ADF4360_MUXOUT_LOCK_DETECT;
+	st->mtld = true;
+
+	ret = adf4360_parse_dt(st);
+	if (ret) {
+		dev_err(&spi->dev, "Parsing properties failed (%d)\n", ret);
+		return -ENODEV;
+	}
+
+	indio_dev->dev.parent = &spi->dev;
+
+	if (spi->dev.of_node)
+		indio_dev->name = spi->dev.of_node->name;
+	else
+		indio_dev->name = spi_get_device_id(spi)->name;
+
+	indio_dev->info = &adf4360_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = &adf4360_chan;
+	indio_dev->num_channels = 1;
+	st->output.indio_dev = indio_dev;
+
+	ret = adf4360_get_gpio(st);
+	if (ret)
+		return ret;
+
+	ret = adf4360_get_clkin(st);
+	if (ret)
+		return ret;
+
+	st->vdd_reg = devm_regulator_get_optional(&spi->dev, "vdd");
+	if (IS_ERR(st->vdd_reg)) {
+		if (PTR_ERR(st->vdd_reg) != -ENODEV) {
+			dev_err(&spi->dev, "Regulator error\n");
+			return PTR_ERR(st->vdd_reg);
+		}
+
+		st->vdd_reg = NULL;
+	}
+
+	ret = adf4360_power_down(st, ADF4360_POWER_DOWN_NORMAL);
+	if (ret)
+		return ret;
+
+	ret = adf4360_clk_register(st);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id adf4360_of_match[] = {
+	{ .compatible = "adi,adf4360-0", },
+	{ .compatible = "adi,adf4360-1", },
+	{ .compatible = "adi,adf4360-2", },
+	{ .compatible = "adi,adf4360-3", },
+	{ .compatible = "adi,adf4360-4", },
+	{ .compatible = "adi,adf4360-5", },
+	{ .compatible = "adi,adf4360-6", },
+	{ .compatible = "adi,adf4360-7", },
+	{ .compatible = "adi,adf4360-8", },
+	{ .compatible = "adi,adf4360-9", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, adf4360_of_match);
+
+static const struct spi_device_id adf4360_id[] = {
+	{"adf4360-0", ID_ADF4360_0},
+	{"adf4360-1", ID_ADF4360_1},
+	{"adf4360-2", ID_ADF4360_2},
+	{"adf4360-3", ID_ADF4360_3},
+	{"adf4360-4", ID_ADF4360_4},
+	{"adf4360-5", ID_ADF4360_5},
+	{"adf4360-6", ID_ADF4360_6},
+	{"adf4360-7", ID_ADF4360_7},
+	{"adf4360-8", ID_ADF4360_8},
+	{"adf4360-9", ID_ADF4360_9},
+	{}
+};
+
+static struct spi_driver adf4360_driver = {
+	.driver = {
+		.name = "adf4360",
+		.of_match_table = adf4360_of_match,
+		.owner = THIS_MODULE,
+	},
+	.probe = adf4360_probe,
+	.id_table = adf4360_id,
+};
+module_spi_driver(adf4360_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Lars-Peter Clausen <lars@metafoo.de>");
+MODULE_AUTHOR("Edward Kigwana <ekigwana@gmail.com>");
+MODULE_DESCRIPTION("Analog Devices ADF4360 PLL");