Re: [PATCH v3 1/3] iio: frequency: adf4360: Add support for ADF4360 PLLs
From: Ardelean, Alexandru
Date: Mon Feb 03 2020 - 06:19:12 EST
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@xxxxxxxxxx> wrote:
>
> > From: Edward Kigwana <ekigwana@xxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > The current/latest/complete version was implemented by
> > Edward Kigwana <ekigwana@xxxxxxxxx>.
> >
> > 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@xxxxxxxxxx>
> > Signed-off-by: Edward Kigwana <ekigwana@xxxxxxxxx>
> > Signed-off-by: Alexandru Ardelean <alexandru.ardelean@xxxxxxxxxx>
>
> 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@xxxxxxxxxx>");
> > +MODULE_AUTHOR("Edward Kigwana <ekigwana@xxxxxxxxx>");
> > +MODULE_DESCRIPTION("Analog Devices ADF4360 PLL");