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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, 3 Feb 2020 11:18:51 +0000
"Ardelean, Alexandru" <alexandru.Ardelean@xxxxxxxxxx> 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@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).

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@xxxxxxxxxx>");
> > > +MODULE_AUTHOR("Edward Kigwana <ekigwana@xxxxxxxxx>");
> > > +MODULE_DESCRIPTION("Analog Devices ADF4360 PLL");  





[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux