[PATCH RESEND V2 1/4] iio: frequency: ad4350: Fix bug / typo in mask

[PATCH RESEND V2 1/4] iio: frequency: ad4350: Fix bug / typo in mask

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/iio/frequency/adf4350.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
index a884252..e76d4ac 100644
--- a/drivers/iio/frequency/adf4350.c
+++ b/drivers/iio/frequency/adf4350.c
@@ -212,7 +212,7 @@ static int adf4350_set_freq(struct adf4350_state *st, unsigned long long freq)
 		(pdata->r2_user_settings & (ADF4350_REG2_PD_POLARITY_POS |
 		ADF4350_REG2_LDP_6ns | ADF4350_REG2_LDF_INT_N |
 		ADF4350_REG2_CHARGE_PUMP_CURR_uA(5000) |
-		ADF4350_REG2_MUXOUT(0x7) | ADF4350_REG2_NOISE_MODE(0x9)));
+		ADF4350_REG2_MUXOUT(0x7) | ADF4350_REG2_NOISE_MODE(0x3)));
 
 	st->regs[ADF4350_REG3] = pdata->r3_user_settings &
 				 (ADF4350_REG3_12BIT_CLKDIV(0xFFF) |
-- 
1.7.9.5

[PATCH RESEND V2 2/4] iio: frequency: adf4350: cast value to unsigned to make code checkers happy

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 include/linux/iio/frequency/adf4350.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/iio/frequency/adf4350.h b/include/linux/iio/frequency/adf4350.h
index be91f34..ffd8c8f 100644
--- a/include/linux/iio/frequency/adf4350.h
+++ b/include/linux/iio/frequency/adf4350.h
@@ -1,7 +1,7 @@
 /*
  * ADF4350/ADF4351 SPI PLL driver
  *
- * Copyright 2012 Analog Devices Inc.
+ * Copyright 2012-2013 Analog Devices Inc.
  *
  * Licensed under the GPL-2.
  */
@@ -41,7 +41,7 @@
 #define ADF4350_REG2_RDIV2_EN			(1 << 24)
 #define ADF4350_REG2_RMULT2_EN			(1 << 25)
 #define ADF4350_REG2_MUXOUT(x)			((x) << 26)
-#define ADF4350_REG2_NOISE_MODE(x)		((x) << 29)
+#define ADF4350_REG2_NOISE_MODE(x)		(((unsigned)(x)) << 29)
 #define ADF4350_MUXOUT_THREESTATE		0
 #define ADF4350_MUXOUT_DVDD			1
 #define ADF4350_MUXOUT_GND			2
-- 
1.7.9.5

[PATCH RESEND V2 3/4] iio: frequency: adf4350: Add support for clock consumer framework

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

Preferably get clkin (PLL reference clock) from clock framework

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/iio/frequency/adf4350.c |   58 +++++++++++++++++++++++++++++++++------
 1 file changed, 49 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
index e76d4ac..f6849c8 100644
--- a/drivers/iio/frequency/adf4350.c
+++ b/drivers/iio/frequency/adf4350.c
@@ -1,7 +1,7 @@
 /*
  * ADF4350/ADF4351 SPI Wideband Synthesizer driver
  *
- * Copyright 2012 Analog Devices Inc.
+ * Copyright 2012-2013 Analog Devices Inc.
  *
  * Licensed under the GPL-2.
  */
@@ -17,6 +17,7 @@
 #include <linux/gcd.h>
 #include <linux/gpio.h>
 #include <asm/div64.h>
+#include <linux/clk.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -33,6 +34,7 @@ struct adf4350_state {
 	struct spi_device		*spi;
 	struct regulator		*reg;
 	struct adf4350_platform_data	*pdata;
+	struct clk			*clk;
 	unsigned long			clkin;
 	unsigned long			chspc; /* Channel Spacing */
 	unsigned long			fpfd; /* Phase Frequency Detector */
@@ -43,7 +45,7 @@ struct adf4350_state {
 	unsigned			r4_rf_div_sel;
 	unsigned long			regs[6];
 	unsigned long			regs_hw[6];
-
+	unsigned long long		freq_req;
 	/*
 	 * DMA (thus cache coherency maintenance) requires the
 	 * transfer buffers to live in their own cache lines.
@@ -52,7 +54,6 @@ struct adf4350_state {
 };
 
 static struct adf4350_platform_data default_pdata = {
-	.clkin = 122880000,
 	.channel_spacing = 10000,
 	.r2_user_settings = ADF4350_REG2_PD_POLARITY_POS |
 			    ADF4350_REG2_CHARGE_PUMP_CURR_uA(2500),
@@ -235,6 +236,7 @@ static int adf4350_set_freq(struct adf4350_state *st, unsigned long long freq)
 		ADF4350_REG4_MUTE_TILL_LOCK_EN));
 
 	st->regs[ADF4350_REG5] = ADF4350_REG5_LD_PIN_MODE_DIGITAL;
+	st->freq_req = freq;
 
 	return adf4350_sync_config(st);
 }
@@ -246,6 +248,7 @@ static ssize_t adf4350_write(struct iio_dev *indio_dev,
 {
 	struct adf4350_state *st = iio_priv(indio_dev);
 	unsigned long long readin;
+	unsigned long tmp;
 	int ret;
 
 	ret = kstrtoull(buf, 10, &readin);
@@ -258,10 +261,23 @@ static ssize_t adf4350_write(struct iio_dev *indio_dev,
 		ret = adf4350_set_freq(st, readin);
 		break;
 	case ADF4350_FREQ_REFIN:
-		if (readin > ADF4350_MAX_FREQ_REFIN)
+		if (readin > ADF4350_MAX_FREQ_REFIN) {
 			ret = -EINVAL;
-		else
-			st->clkin = readin;
+			break;
+		}
+
+		if (st->clk) {
+			tmp = clk_round_rate(st->clk, readin);
+			if (tmp != readin) {
+				ret = -EINVAL;
+				break;
+			}
+			ret = clk_set_rate(st->clk, tmp);
+			if (ret < 0)
+				break;
+		}
+		st->clkin = readin;
+		ret = adf4350_set_freq(st, st->freq_req);
 		break;
 	case ADF4350_FREQ_RESOLUTION:
 		if (readin == 0)
@@ -308,6 +324,9 @@ static ssize_t adf4350_read(struct iio_dev *indio_dev,
 			}
 		break;
 	case ADF4350_FREQ_REFIN:
+		if (st->clk)
+			st->clkin = clk_get_rate(st->clk);
+
 		val = st->clkin;
 		break;
 	case ADF4350_FREQ_RESOLUTION:
@@ -318,6 +337,7 @@ static ssize_t adf4350_read(struct iio_dev *indio_dev,
 		break;
 	default:
 		ret = -EINVAL;
+		val = 0;
 	}
 	mutex_unlock(&indio_dev->mlock);
 
@@ -360,14 +380,24 @@ static int adf4350_probe(struct spi_device *spi)
 	struct adf4350_platform_data *pdata = spi->dev.platform_data;
 	struct iio_dev *indio_dev;
 	struct adf4350_state *st;
+	struct clk *clk = NULL;
 	int ret;
 
 	if (!pdata) {
 		dev_warn(&spi->dev, "no platform data? using default\n");
-
 		pdata = &default_pdata;
 	}
 
+	if (!pdata->clkin) {
+		clk = clk_get(&spi->dev, "clkin");
+		if (IS_ERR(clk))
+			return -EPROBE_DEFER;
+
+		ret = clk_prepare_enable(clk);
+		if (ret < 0)
+			return ret;
+	}
+
 	indio_dev = iio_device_alloc(sizeof(*st));
 	if (indio_dev == NULL)
 		return -ENOMEM;
@@ -395,7 +425,12 @@ static int adf4350_probe(struct spi_device *spi)
 	indio_dev->num_channels = 1;
 
 	st->chspc = pdata->channel_spacing;
-	st->clkin = pdata->clkin;
+	if (clk) {
+		st->clk = clk;
+		st->clkin = clk_get_rate(clk);
+	} else {
+		st->clkin = pdata->clkin;
+	}
 
 	st->min_out_freq = spi_get_device_id(spi)->driver_data == 4351 ?
 		ADF4351_MIN_OUT_FREQ : ADF4350_MIN_OUT_FREQ;
@@ -435,6 +470,8 @@ error_put_reg:
 	if (!IS_ERR(st->reg))
 		regulator_put(st->reg);
 
+	if (clk)
+		clk_disable_unprepare(clk);
 	iio_device_free(indio_dev);
 
 	return ret;
@@ -451,6 +488,9 @@ static int adf4350_remove(struct spi_device *spi)
 
 	iio_device_unregister(indio_dev);
 
+	if (st->clk)
+		clk_disable_unprepare(st->clk);
+
 	if (!IS_ERR(reg)) {
 		regulator_disable(reg);
 		regulator_put(reg);
@@ -481,6 +521,6 @@ static struct spi_driver adf4350_driver = {
 };
 module_spi_driver(adf4350_driver);
 
-MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
 MODULE_DESCRIPTION("Analog Devices ADF4350/ADF4351 PLL");
 MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

[PATCH RESEND V2 4/4] iio: frequency: adf4350: Add support for dt bindings

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>

Per review feedback from Lars-Peter Clausen <lars@metafoo.de>
Changes since V1:
	Fix return value handling of adf4350_parse_dt()
	Use of_get_gpio
	Avoid abbreviations in devicetree properties
	Fix typo in docs

Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 .../devicetree/bindings/iio/frequency/adf4350.txt  |   86 +++++++++++++
 drivers/iio/frequency/adf4350.c                    |  128 +++++++++++++++++++-
 2 files changed, 213 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/iio/frequency/adf4350.txt

diff --git a/Documentation/devicetree/bindings/iio/frequency/adf4350.txt b/Documentation/devicetree/bindings/iio/frequency/adf4350.txt
new file mode 100644
index 0000000..f8c181d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/frequency/adf4350.txt
@@ -0,0 +1,86 @@
+Analog Devices ADF4350/ADF4351 device driver
+
+Required properties:
+	- compatible: Should be one of
+		* "adi,adf4350": When using the ADF4350 device
+		* "adi,adf4351": When using the ADF4351 device
+	- reg: SPI chip select numbert for the device
+	- spi-max-frequency: Max SPI frequency to use (< 20000000)
+	- clocks: From common clock binding. Clock is phandle to clock for
+		ADF435x Reference Clock (CLKIN).
+
+Optional properties:
+	- gpios:	 GPIO Lock detect - If set with a valid phandle and GPIO number,
+			pll lock state is tested upon read.
+	- adi,channel-spacing: Channel spacing in Hz (influences MODULUS).
+	- adi,power-up-frequency:	If set in Hz the PLL tunes to
+			the desired frequency on probe.
+	- adi,reference-div-factor: If set the driver skips dynamic calculation
+			and uses this default value instead.
+	- adi,reference-doubler-enable: Enables reference doubler.
+	- adi,reference-div2-enable: Enables reference divider.
+	- adi,phase-detector-polarity-positive-enable: Enables positive phase
+			detector polarity. Default = negative.
+	- adi,lock-detect-precision-6ns-enable: Enables 6ns lock detect precision.
+			Default = 10ns.
+	- adi,lock-detect-function-integer-n-enable: Enables lock detect
+			for integer-N mode. Default = factional-N mode.
+	- adi,charge-pump-current: Charge pump current in mA.
+			Default = 2500mA.
+	- adi,muxout-select: On chip multiplexer output selection.
+			Valid values for the multiplexer output are:
+			0: Three-State Output (default)
+			1: DVDD
+			2: DGND
+			3: R-Counter output
+			4: N-Divider output
+			5: Analog lock detect
+			6: Digital lock detect
+	- adi,low-spur-mode-enable: Enables low spur mode.
+			Default = Low noise mode.
+	- adi,cycle-slip-reduction-enable: Enables cycle slip reduction.
+	- adi,charge-cancellation-enable: Enabled charge pump
+			charge cancellation for integer-N modes.
+	- adi,anti-backlash-3ns-enable: Enables 3ns antibacklash pulse width
+			 for integer-N modes.
+	- adi,band-select-clock-mode-high-enable: Enables faster band
+			selection logic.
+	- adi,12bit-clk-divider: Clock divider value used when
+			adi,12bit-clkdiv-mode != 0
+	- adi,clk-divider-mode:
+			Valid values for the clkdiv mode are:
+			0: Clock divider off (default)
+			1: Fast lock enable
+			2: Phase resync enable
+	- adi,aux-output-enable: Enables auxiliary RF output.
+	- adi,aux-output-fundamental-enable: Selects fundamental VCO output on
+			the auxiliary RF output. Default = Output of RF dividers.
+	- adi,mute-till-lock-enable: Enables Mute-Till-Lock-Detect function.
+	- adi,output-power: Output power selection.
+			Valid values for the power mode are:
+			0: -4dBm (default)
+			1: -1dBm
+			2: +2dBm
+			3: +5dBm
+	- adi,aux-output-power: Auxiliary output power selection.
+			Valid values for the power mode are:
+			0: -4dBm (default)
+			1: -1dBm
+			2: +2dBm
+			3: +5dBm
+
+
+Example:
+		lo_pll0_rx_adf4351: adf4351-rx-lpc@4 {
+			compatible = "adi,adf4351";
+			reg = <4>;
+			spi-max-frequency = <10000000>;
+			clocks = <&clk0_ad9523 9>;
+			clock-names = "clkin";
+			adi,channel-spacing = <10000>;
+			adi,power-up-frequency = <2400000000>;
+			adi,phase-detector-polarity-positive-enable;
+			adi,charge-pump-current = <2500>;
+			adi,output-power = <3>;
+			adi,mute-till-lock-enable;
+		};
diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
index f6849c8..a4157cd 100644
--- a/drivers/iio/frequency/adf4350.c
+++ b/drivers/iio/frequency/adf4350.c
@@ -18,6 +18,8 @@
 #include <linux/gpio.h>
 #include <asm/div64.h>
 #include <linux/clk.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -375,14 +377,138 @@ static const struct iio_info adf4350_info = {
 	.driver_module = THIS_MODULE,
 };
 
+#ifdef CONFIG_OF
+static struct adf4350_platform_data *adf4350_parse_dt(struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct adf4350_platform_data *pdata;
+	unsigned int tmp;
+	int ret;
+
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata) {
+		dev_err(dev, "could not allocate memory for platform data\n");
+		return NULL;
+	}
+
+	strncpy(&pdata->name[0], np->name, SPI_NAME_SIZE - 1);
+
+	tmp = 10000;
+	of_property_read_u32(np, "adi,channel-spacing", &tmp);
+	pdata->channel_spacing = tmp;
+
+	tmp = 0;
+	of_property_read_u32(np, "adi,power-up-frequency", &tmp);
+	pdata->power_up_frequency = tmp;
+
+	tmp = 0;
+	of_property_read_u32(np, "adi,reference-div-factor", &tmp);
+	pdata->ref_div_factor = tmp;
+
+	ret = of_get_gpio(np, 0);
+	if (ret < 0)
+		pdata->gpio_lock_detect = -1;
+	else
+		pdata->gpio_lock_detect = ret;
+
+	pdata->ref_doubler_en = of_property_read_bool(np,
+			"adi,reference-doubler-enable");
+	pdata->ref_div2_en = of_property_read_bool(np,
+			"adi,reference-div2-enable");
+
+	/* r2_user_settings */
+	pdata->r2_user_settings = of_property_read_bool(np,
+			"adi,phase-detector-polarity-positive-enable") ?
+			ADF4350_REG2_PD_POLARITY_POS : 0;
+	pdata->r2_user_settings |= of_property_read_bool(np,
+			"adi,lock-detect-precision-6ns-enable") ?
+			ADF4350_REG2_LDP_6ns : 0;
+	pdata->r2_user_settings |= of_property_read_bool(np,
+			"adi,lock-detect-function-integer-n-enable") ?
+			ADF4350_REG2_LDF_INT_N : 0;
+
+	tmp = 2500;
+	of_property_read_u32(np, "adi,charge-pump-current", &tmp);
+	pdata->r2_user_settings |= ADF4350_REG2_CHARGE_PUMP_CURR_uA(tmp);
+
+	tmp = 0;
+	of_property_read_u32(np, "adi,muxout-select", &tmp);
+	pdata->r2_user_settings |= ADF4350_REG2_MUXOUT(tmp);
+
+	pdata->r2_user_settings |= of_property_read_bool(np,
+			"adi,low-spur-mode-enable") ?
+			ADF4350_REG2_NOISE_MODE(0x3) : 0;
+
+	/* r3_user_settings */
+
+	pdata->r3_user_settings = of_property_read_bool(np,
+			"adi,cycle-slip-reduction-enable") ?
+			ADF4350_REG3_12BIT_CSR_EN : 0;
+	pdata->r3_user_settings |= of_property_read_bool(np,
+			"adi,charge-cancellation-enable") ?
+			ADF4351_REG3_CHARGE_CANCELLATION_EN : 0;
+
+	pdata->r3_user_settings |= of_property_read_bool(np,
+			"adi,anti-backlash-3ns-enable") ?
+			ADF4351_REG3_ANTI_BACKLASH_3ns_EN : 0;
+	pdata->r3_user_settings |= of_property_read_bool(np,
+			"adi,band-select-clock-mode-high-enable") ?
+			ADF4351_REG3_BAND_SEL_CLOCK_MODE_HIGH : 0;
+
+	tmp = 0;
+	of_property_read_u32(np, "adi,12bit-clk-divider", &tmp);
+	pdata->r3_user_settings |= ADF4350_REG3_12BIT_CLKDIV(tmp);
+
+	tmp = 0;
+	of_property_read_u32(np, "adi,clk-divider-mode", &tmp);
+	pdata->r3_user_settings |= ADF4350_REG3_12BIT_CLKDIV_MODE(tmp);
+
+	/* r4_user_settings */
+
+	pdata->r4_user_settings = of_property_read_bool(np,
+			"adi,aux-output-enable") ?
+			ADF4350_REG4_AUX_OUTPUT_EN : 0;
+	pdata->r4_user_settings |= of_property_read_bool(np,
+			"adi,aux-output-fundamental-enable") ?
+			ADF4350_REG4_AUX_OUTPUT_FUND : 0;
+	pdata->r4_user_settings |= of_property_read_bool(np,
+			"adi,mute-till-lock-enable") ?
+			ADF4350_REG4_MUTE_TILL_LOCK_EN : 0;
+
+	tmp = 0;
+	of_property_read_u32(np, "adi,output-power", &tmp);
+	pdata->r4_user_settings |= ADF4350_REG4_OUTPUT_PWR(tmp);
+
+	tmp = 0;
+	of_property_read_u32(np, "adi,aux-output-power", &tmp);
+	pdata->r4_user_settings |= ADF4350_REG4_AUX_OUTPUT_PWR(tmp);
+
+	return pdata;
+}
+#else
+static
+struct adf4350_platform_data *adf4350_parse_dt(struct device *dev)
+{
+	return NULL;
+}
+#endif
+
 static int adf4350_probe(struct spi_device *spi)
 {
-	struct adf4350_platform_data *pdata = spi->dev.platform_data;
+	struct adf4350_platform_data *pdata;
 	struct iio_dev *indio_dev;
 	struct adf4350_state *st;
 	struct clk *clk = NULL;
 	int ret;
 
+	if (spi->dev.of_node) {
+		pdata = adf4350_parse_dt(&spi->dev);
+		if (pdata == NULL)
+			return -EINVAL;
+	} else {
+		pdata = spi->dev.platform_data;
+	}
+
 	if (!pdata) {
 		dev_warn(&spi->dev, "no platform data? using default\n");
 		pdata = &default_pdata;
-- 
1.7.9.5

Re: [PATCH RESEND V2 1/4] iio: frequency: ad4350: Fix bug / typo in mask

[Lars-Peter Clausen]

On 06/03/2013 03:30 PM, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> t
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>me.

FWIW, complete series looks good to. All four patches:

Reviewed-by: Lars-Peter Clausen <lars@metafoo.de>

> ---
>  drivers/iio/frequency/adf4350.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
> index a884252..e76d4ac 100644
> --- a/drivers/iio/frequency/adf4350.c
> +++ b/drivers/iio/frequency/adf4350.c
> @@ -212,7 +212,7 @@ static int adf4350_set_freq(struct adf4350_state *st, unsigned long long freq)
>  		(pdata->r2_user_settings & (ADF4350_REG2_PD_POLARITY_POS |
>  		ADF4350_REG2_LDP_6ns | ADF4350_REG2_LDF_INT_N |
>  		ADF4350_REG2_CHARGE_PUMP_CURR_uA(5000) |
> -		ADF4350_REG2_MUXOUT(0x7) | ADF4350_REG2_NOISE_MODE(0x9)));
> +		ADF4350_REG2_MUXOUT(0x7) | ADF4350_REG2_NOISE_MODE(0x3)));
>  
>  	st->regs[ADF4350_REG3] = pdata->r3_user_settings &
>  				 (ADF4350_REG3_12BIT_CLKDIV(0xFFF) |

Re: [PATCH RESEND V2 1/4] iio: frequency: ad4350: Fix bug / typo in mask

[Jonathan Cameron]

On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Applied to fixes-togreg branch of iio.git
> ---
>  drivers/iio/frequency/adf4350.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
> index a884252..e76d4ac 100644
> --- a/drivers/iio/frequency/adf4350.c
> +++ b/drivers/iio/frequency/adf4350.c
> @@ -212,7 +212,7 @@ static int adf4350_set_freq(struct adf4350_state *st, unsigned long long freq)
>  		(pdata->r2_user_settings & (ADF4350_REG2_PD_POLARITY_POS |
>  		ADF4350_REG2_LDP_6ns | ADF4350_REG2_LDF_INT_N |
>  		ADF4350_REG2_CHARGE_PUMP_CURR_uA(5000) |
> -		ADF4350_REG2_MUXOUT(0x7) | ADF4350_REG2_NOISE_MODE(0x9)));
> +		ADF4350_REG2_MUXOUT(0x7) | ADF4350_REG2_NOISE_MODE(0x3)));
>  
>  	st->regs[ADF4350_REG3] = pdata->r3_user_settings &
>  				 (ADF4350_REG3_12BIT_CLKDIV(0xFFF) |
> 

Re: [PATCH RESEND V2 3/4] iio: frequency: adf4350: Add support for clock consumer framework

[Jonathan Cameron]

On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> Preferably get clkin (PLL reference clock) from clock framework
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>

ERROR: "clk_round_rate" [drivers/iio/frequency/adf4350.ko] undefined!
make[1]: *** [__modpost] Error 1

on my arm test build. Sorry, I was being lazy before and hadn't done
any test builds till I tried merging it.  Backed out the merge of this
patch for now.

clk.h does say that api is optional for machine classes.  No idea what you want to
do about this.

Incidentally, if you want to play squash the false warnings I also get...

  CC [M]  drivers/iio/frequency/adf4350.o
drivers/iio/frequency/adf4350.c: In function 'adf4350_read':
drivers/iio/frequency/adf4350.c:294:21: warning: 'val' may be used uninitialized in this function

which I don't think is addressed in this series.  IIRC it is an obvious false positive
so I've never bothered mentioning it before :)

> ---
>  drivers/iio/frequency/adf4350.c |   58 +++++++++++++++++++++++++++++++++------
>  1 file changed, 49 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
> index e76d4ac..f6849c8 100644
> --- a/drivers/iio/frequency/adf4350.c
> +++ b/drivers/iio/frequency/adf4350.c
> @@ -1,7 +1,7 @@
>  /*
>   * ADF4350/ADF4351 SPI Wideband Synthesizer driver
>   *
> - * Copyright 2012 Analog Devices Inc.
> + * Copyright 2012-2013 Analog Devices Inc.
>   *
>   * Licensed under the GPL-2.
>   */
> @@ -17,6 +17,7 @@
>  #include <linux/gcd.h>
>  #include <linux/gpio.h>
>  #include <asm/div64.h>
> +#include <linux/clk.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -33,6 +34,7 @@ struct adf4350_state {
>  	struct spi_device		*spi;
>  	struct regulator		*reg;
>  	struct adf4350_platform_data	*pdata;
> +	struct clk			*clk;
>  	unsigned long			clkin;
>  	unsigned long			chspc; /* Channel Spacing */
>  	unsigned long			fpfd; /* Phase Frequency Detector */
> @@ -43,7 +45,7 @@ struct adf4350_state {
>  	unsigned			r4_rf_div_sel;
>  	unsigned long			regs[6];
>  	unsigned long			regs_hw[6];
> -
> +	unsigned long long		freq_req;
>  	/*
>  	 * DMA (thus cache coherency maintenance) requires the
>  	 * transfer buffers to live in their own cache lines.
> @@ -52,7 +54,6 @@ struct adf4350_state {
>  };
>  
>  static struct adf4350_platform_data default_pdata = {
> -	.clkin = 122880000,
>  	.channel_spacing = 10000,
>  	.r2_user_settings = ADF4350_REG2_PD_POLARITY_POS |
>  			    ADF4350_REG2_CHARGE_PUMP_CURR_uA(2500),
> @@ -235,6 +236,7 @@ static int adf4350_set_freq(struct adf4350_state *st, unsigned long long freq)
>  		ADF4350_REG4_MUTE_TILL_LOCK_EN));
>  
>  	st->regs[ADF4350_REG5] = ADF4350_REG5_LD_PIN_MODE_DIGITAL;
> +	st->freq_req = freq;
>  
>  	return adf4350_sync_config(st);
>  }
> @@ -246,6 +248,7 @@ static ssize_t adf4350_write(struct iio_dev *indio_dev,
>  {
>  	struct adf4350_state *st = iio_priv(indio_dev);
>  	unsigned long long readin;
> +	unsigned long tmp;
>  	int ret;
>  
>  	ret = kstrtoull(buf, 10, &readin);
> @@ -258,10 +261,23 @@ static ssize_t adf4350_write(struct iio_dev *indio_dev,
>  		ret = adf4350_set_freq(st, readin);
>  		break;
>  	case ADF4350_FREQ_REFIN:
> -		if (readin > ADF4350_MAX_FREQ_REFIN)
> +		if (readin > ADF4350_MAX_FREQ_REFIN) {
>  			ret = -EINVAL;
> -		else
> -			st->clkin = readin;
> +			break;
> +		}
> +
> +		if (st->clk) {
> +			tmp = clk_round_rate(st->clk, readin);
> +			if (tmp != readin) {
> +				ret = -EINVAL;
> +				break;
> +			}
> +			ret = clk_set_rate(st->clk, tmp);
> +			if (ret < 0)
> +				break;
> +		}
> +		st->clkin = readin;
> +		ret = adf4350_set_freq(st, st->freq_req);
>  		break;
>  	case ADF4350_FREQ_RESOLUTION:
>  		if (readin == 0)
> @@ -308,6 +324,9 @@ static ssize_t adf4350_read(struct iio_dev *indio_dev,
>  			}
>  		break;
>  	case ADF4350_FREQ_REFIN:
> +		if (st->clk)
> +			st->clkin = clk_get_rate(st->clk);
> +
>  		val = st->clkin;
>  		break;
>  	case ADF4350_FREQ_RESOLUTION:
> @@ -318,6 +337,7 @@ static ssize_t adf4350_read(struct iio_dev *indio_dev,
>  		break;
>  	default:
>  		ret = -EINVAL;
> +		val = 0;
>  	}
>  	mutex_unlock(&indio_dev->mlock);
>  
> @@ -360,14 +380,24 @@ static int adf4350_probe(struct spi_device *spi)
>  	struct adf4350_platform_data *pdata = spi->dev.platform_data;
>  	struct iio_dev *indio_dev;
>  	struct adf4350_state *st;
> +	struct clk *clk = NULL;
>  	int ret;
>  
>  	if (!pdata) {
>  		dev_warn(&spi->dev, "no platform data? using default\n");
> -
>  		pdata = &default_pdata;
>  	}
>  
> +	if (!pdata->clkin) {
> +		clk = clk_get(&spi->dev, "clkin");
> +		if (IS_ERR(clk))
> +			return -EPROBE_DEFER;
> +
> +		ret = clk_prepare_enable(clk);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	indio_dev = iio_device_alloc(sizeof(*st));
>  	if (indio_dev == NULL)
>  		return -ENOMEM;
> @@ -395,7 +425,12 @@ static int adf4350_probe(struct spi_device *spi)
>  	indio_dev->num_channels = 1;
>  
>  	st->chspc = pdata->channel_spacing;
> -	st->clkin = pdata->clkin;
> +	if (clk) {
> +		st->clk = clk;
> +		st->clkin = clk_get_rate(clk);
> +	} else {
> +		st->clkin = pdata->clkin;
> +	}
>  
>  	st->min_out_freq = spi_get_device_id(spi)->driver_data == 4351 ?
>  		ADF4351_MIN_OUT_FREQ : ADF4350_MIN_OUT_FREQ;
> @@ -435,6 +470,8 @@ error_put_reg:
>  	if (!IS_ERR(st->reg))
>  		regulator_put(st->reg);
>  
> +	if (clk)
> +		clk_disable_unprepare(clk);
>  	iio_device_free(indio_dev);
>  
>  	return ret;
> @@ -451,6 +488,9 @@ static int adf4350_remove(struct spi_device *spi)
>  
>  	iio_device_unregister(indio_dev);
>  
> +	if (st->clk)
> +		clk_disable_unprepare(st->clk);
> +
>  	if (!IS_ERR(reg)) {
>  		regulator_disable(reg);
>  		regulator_put(reg);
> @@ -481,6 +521,6 @@ static struct spi_driver adf4350_driver = {
>  };
>  module_spi_driver(adf4350_driver);
>  
> -MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
>  MODULE_DESCRIPTION("Analog Devices ADF4350/ADF4351 PLL");
>  MODULE_LICENSE("GPL v2");
> 

Re: [PATCH RESEND V2 2/4] iio: frequency: adf4350: cast value to unsigned to make code checkers happy

[Jonathan Cameron]

On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Applied to the togreg branch of iio.git.
> ---
>  include/linux/iio/frequency/adf4350.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/iio/frequency/adf4350.h b/include/linux/iio/frequency/adf4350.h
> index be91f34..ffd8c8f 100644
> --- a/include/linux/iio/frequency/adf4350.h
> +++ b/include/linux/iio/frequency/adf4350.h
> @@ -1,7 +1,7 @@
>  /*
>   * ADF4350/ADF4351 SPI PLL driver
>   *
> - * Copyright 2012 Analog Devices Inc.
> + * Copyright 2012-2013 Analog Devices Inc.
>   *
>   * Licensed under the GPL-2.
>   */
> @@ -41,7 +41,7 @@
>  #define ADF4350_REG2_RDIV2_EN			(1 << 24)
>  #define ADF4350_REG2_RMULT2_EN			(1 << 25)
>  #define ADF4350_REG2_MUXOUT(x)			((x) << 26)
> -#define ADF4350_REG2_NOISE_MODE(x)		((x) << 29)
> +#define ADF4350_REG2_NOISE_MODE(x)		(((unsigned)(x)) << 29)
>  #define ADF4350_MUXOUT_THREESTATE		0
>  #define ADF4350_MUXOUT_DVDD			1
>  #define ADF4350_MUXOUT_GND			2
> 

Re: [PATCH RESEND V2 3/4] iio: frequency: adf4350: Add support for clock consumer framework

[Michael Hennerich]

On 06/04/2013 07:44 PM, Jonathan Cameron wrote:
> On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>> Preferably get clkin (PLL reference clock) from clock framework
>>
>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> ERROR: "clk_round_rate" [drivers/iio/frequency/adf4350.ko] undefined!
> make[1]: *** [__modpost] Error 1
>
> on my arm test build. Sorry, I was being lazy before and hadn't done
> any test builds till I tried merging it.  Backed out the merge of this
> patch for now.
>
> clk.h does say that api is optional for machine classes.  No idea what you want to
> do about this.

The CLK API is optional in the driver - so I prefer to keep it like that 
and don't make
the driver depend on COMMON_CLK.

The simplest and probably the best workaround is to guard the CLK 
Round/Set code with
#if defined(CONFIG_COMMON_CLK).

Thoughts?


> Incidentally, if you want to play squash the false warnings I also get...
>
>    CC [M]  drivers/iio/frequency/adf4350.o
> drivers/iio/frequency/adf4350.c: In function 'adf4350_read':
> drivers/iio/frequency/adf4350.c:294:21: warning: 'val' may be used uninitialized in this function
>
> which I don't think is addressed in this series.  IIRC it is an obvious false positive
> so I've never bothered mentioning it before :)
Yes it is a false positive - and my compilers doesn't warn on it.
Anyways I can initialize val...

-Michael

>
>> ---
>>   drivers/iio/frequency/adf4350.c |   58 +++++++++++++++++++++++++++++++++------
>>   1 file changed, 49 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
>> index e76d4ac..f6849c8 100644
>> --- a/drivers/iio/frequency/adf4350.c
>> +++ b/drivers/iio/frequency/adf4350.c
>> @@ -1,7 +1,7 @@
>>   /*
>>    * ADF4350/ADF4351 SPI Wideband Synthesizer driver
>>    *
>> - * Copyright 2012 Analog Devices Inc.
>> + * Copyright 2012-2013 Analog Devices Inc.
>>    *
>>    * Licensed under the GPL-2.
>>    */
>> @@ -17,6 +17,7 @@
>>   #include <linux/gcd.h>
>>   #include <linux/gpio.h>
>>   #include <asm/div64.h>
>> +#include <linux/clk.h>
>>   
>>   #include <linux/iio/iio.h>
>>   #include <linux/iio/sysfs.h>
>> @@ -33,6 +34,7 @@ struct adf4350_state {
>>   	struct spi_device		*spi;
>>   	struct regulator		*reg;
>>   	struct adf4350_platform_data	*pdata;
>> +	struct clk			*clk;
>>   	unsigned long			clkin;
>>   	unsigned long			chspc; /* Channel Spacing */
>>   	unsigned long			fpfd; /* Phase Frequency Detector */
>> @@ -43,7 +45,7 @@ struct adf4350_state {
>>   	unsigned			r4_rf_div_sel;
>>   	unsigned long			regs[6];
>>   	unsigned long			regs_hw[6];
>> -
>> +	unsigned long long		freq_req;
>>   	/*
>>   	 * DMA (thus cache coherency maintenance) requires the
>>   	 * transfer buffers to live in their own cache lines.
>> @@ -52,7 +54,6 @@ struct adf4350_state {
>>   };
>>   
>>   static struct adf4350_platform_data default_pdata = {
>> -	.clkin = 122880000,
>>   	.channel_spacing = 10000,
>>   	.r2_user_settings = ADF4350_REG2_PD_POLARITY_POS |
>>   			    ADF4350_REG2_CHARGE_PUMP_CURR_uA(2500),
>> @@ -235,6 +236,7 @@ static int adf4350_set_freq(struct adf4350_state *st, unsigned long long freq)
>>   		ADF4350_REG4_MUTE_TILL_LOCK_EN));
>>   
>>   	st->regs[ADF4350_REG5] = ADF4350_REG5_LD_PIN_MODE_DIGITAL;
>> +	st->freq_req = freq;
>>   
>>   	return adf4350_sync_config(st);
>>   }
>> @@ -246,6 +248,7 @@ static ssize_t adf4350_write(struct iio_dev *indio_dev,
>>   {
>>   	struct adf4350_state *st = iio_priv(indio_dev);
>>   	unsigned long long readin;
>> +	unsigned long tmp;
>>   	int ret;
>>   
>>   	ret = kstrtoull(buf, 10, &readin);
>> @@ -258,10 +261,23 @@ static ssize_t adf4350_write(struct iio_dev *indio_dev,
>>   		ret = adf4350_set_freq(st, readin);
>>   		break;
>>   	case ADF4350_FREQ_REFIN:
>> -		if (readin > ADF4350_MAX_FREQ_REFIN)
>> +		if (readin > ADF4350_MAX_FREQ_REFIN) {
>>   			ret = -EINVAL;
>> -		else
>> -			st->clkin = readin;
>> +			break;
>> +		}
>> +
>> +		if (st->clk) {
>> +			tmp = clk_round_rate(st->clk, readin);
>> +			if (tmp != readin) {
>> +				ret = -EINVAL;
>> +				break;
>> +			}
>> +			ret = clk_set_rate(st->clk, tmp);
>> +			if (ret < 0)
>> +				break;
>> +		}
>> +		st->clkin = readin;
>> +		ret = adf4350_set_freq(st, st->freq_req);
>>   		break;
>>   	case ADF4350_FREQ_RESOLUTION:
>>   		if (readin == 0)
>> @@ -308,6 +324,9 @@ static ssize_t adf4350_read(struct iio_dev *indio_dev,
>>   			}
>>   		break;
>>   	case ADF4350_FREQ_REFIN:
>> +		if (st->clk)
>> +			st->clkin = clk_get_rate(st->clk);
>> +
>>   		val = st->clkin;
>>   		break;
>>   	case ADF4350_FREQ_RESOLUTION:
>> @@ -318,6 +337,7 @@ static ssize_t adf4350_read(struct iio_dev *indio_dev,
>>   		break;
>>   	default:
>>   		ret = -EINVAL;
>> +		val = 0;
>>   	}
>>   	mutex_unlock(&indio_dev->mlock);
>>   
>> @@ -360,14 +380,24 @@ static int adf4350_probe(struct spi_device *spi)
>>   	struct adf4350_platform_data *pdata = spi->dev.platform_data;
>>   	struct iio_dev *indio_dev;
>>   	struct adf4350_state *st;
>> +	struct clk *clk = NULL;
>>   	int ret;
>>   
>>   	if (!pdata) {
>>   		dev_warn(&spi->dev, "no platform data? using default\n");
>> -
>>   		pdata = &default_pdata;
>>   	}
>>   
>> +	if (!pdata->clkin) {
>> +		clk = clk_get(&spi->dev, "clkin");
>> +		if (IS_ERR(clk))
>> +			return -EPROBE_DEFER;
>> +
>> +		ret = clk_prepare_enable(clk);
>> +		if (ret < 0)
>> +			return ret;
>> +	}
>> +
>>   	indio_dev = iio_device_alloc(sizeof(*st));
>>   	if (indio_dev == NULL)
>>   		return -ENOMEM;
>> @@ -395,7 +425,12 @@ static int adf4350_probe(struct spi_device *spi)
>>   	indio_dev->num_channels = 1;
>>   
>>   	st->chspc = pdata->channel_spacing;
>> -	st->clkin = pdata->clkin;
>> +	if (clk) {
>> +		st->clk = clk;
>> +		st->clkin = clk_get_rate(clk);
>> +	} else {
>> +		st->clkin = pdata->clkin;
>> +	}
>>   
>>   	st->min_out_freq = spi_get_device_id(spi)->driver_data == 4351 ?
>>   		ADF4351_MIN_OUT_FREQ : ADF4350_MIN_OUT_FREQ;
>> @@ -435,6 +470,8 @@ error_put_reg:
>>   	if (!IS_ERR(st->reg))
>>   		regulator_put(st->reg);
>>   
>> +	if (clk)
>> +		clk_disable_unprepare(clk);
>>   	iio_device_free(indio_dev);
>>   
>>   	return ret;
>> @@ -451,6 +488,9 @@ static int adf4350_remove(struct spi_device *spi)
>>   
>>   	iio_device_unregister(indio_dev);
>>   
>> +	if (st->clk)
>> +		clk_disable_unprepare(st->clk);
>> +
>>   	if (!IS_ERR(reg)) {
>>   		regulator_disable(reg);
>>   		regulator_put(reg);
>> @@ -481,6 +521,6 @@ static struct spi_driver adf4350_driver = {
>>   };
>>   module_spi_driver(adf4350_driver);
>>   
>> -MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
>> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
>>   MODULE_DESCRIPTION("Analog Devices ADF4350/ADF4351 PLL");
>>   MODULE_LICENSE("GPL v2");
>>


-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif

Re: [PATCH RESEND V2 3/4] iio: frequency: adf4350: Add support for clock consumer framework

[Lars-Peter Clausen]

On 06/05/2013 09:32 AM, Michael Hennerich wrote:
> On 06/04/2013 07:44 PM, Jonathan Cameron wrote:
>> On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>
>>> Preferably get clkin (PLL reference clock) from clock framework
>>>
>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>> ERROR: "clk_round_rate" [drivers/iio/frequency/adf4350.ko] undefined!
>> make[1]: *** [__modpost] Error 1
>>
>> on my arm test build. Sorry, I was being lazy before and hadn't done
>> any test builds till I tried merging it.  Backed out the merge of this
>> patch for now.
>>
>> clk.h does say that api is optional for machine classes.  No idea what you
>> want to
>> do about this.
> 
> The CLK API is optional in the driver - so I prefer to keep it like that and
> don't make
> the driver depend on COMMON_CLK.
> 
> The simplest and probably the best workaround is to guard the CLK Round/Set
> code with
> #if defined(CONFIG_COMMON_CLK).
> 
> Thoughts?
> 

This is not a bug in the driver. If the clk API is not implemented it is
stubbed out. It looks as if Jonathan is testing on a platform which
implements the clk API but not clk_round_rate() (None of the other clk_*
symbols are undefined).

Jonathan on which platform are you testing this?

- Lars

> 
>> Incidentally, if you want to play squash the false warnings I also get...
>>
>>    CC [M]  drivers/iio/frequency/adf4350.o
>> drivers/iio/frequency/adf4350.c: In function 'adf4350_read':
>> drivers/iio/frequency/adf4350.c:294:21: warning: 'val' may be used
>> uninitialized in this function
>>
>> which I don't think is addressed in this series.  IIRC it is an obvious
>> false positive
>> so I've never bothered mentioning it before :)
> Yes it is a false positive - and my compilers doesn't warn on it.
> Anyways I can initialize val...
> 
> -Michael
> 
>>
>>> ---
>>>   drivers/iio/frequency/adf4350.c |   58
>>> +++++++++++++++++++++++++++++++++------
>>>   1 file changed, 49 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/iio/frequency/adf4350.c
>>> b/drivers/iio/frequency/adf4350.c
>>> index e76d4ac..f6849c8 100644
>>> --- a/drivers/iio/frequency/adf4350.c
>>> +++ b/drivers/iio/frequency/adf4350.c
>>> @@ -1,7 +1,7 @@
>>>   /*
>>>    * ADF4350/ADF4351 SPI Wideband Synthesizer driver
>>>    *
>>> - * Copyright 2012 Analog Devices Inc.
>>> + * Copyright 2012-2013 Analog Devices Inc.
>>>    *
>>>    * Licensed under the GPL-2.
>>>    */
>>> @@ -17,6 +17,7 @@
>>>   #include <linux/gcd.h>
>>>   #include <linux/gpio.h>
>>>   #include <asm/div64.h>
>>> +#include <linux/clk.h>
>>>     #include <linux/iio/iio.h>
>>>   #include <linux/iio/sysfs.h>
>>> @@ -33,6 +34,7 @@ struct adf4350_state {
>>>       struct spi_device        *spi;
>>>       struct regulator        *reg;
>>>       struct adf4350_platform_data    *pdata;
>>> +    struct clk            *clk;
>>>       unsigned long            clkin;
>>>       unsigned long            chspc; /* Channel Spacing */
>>>       unsigned long            fpfd; /* Phase Frequency Detector */
>>> @@ -43,7 +45,7 @@ struct adf4350_state {
>>>       unsigned            r4_rf_div_sel;
>>>       unsigned long            regs[6];
>>>       unsigned long            regs_hw[6];
>>> -
>>> +    unsigned long long        freq_req;
>>>       /*
>>>        * DMA (thus cache coherency maintenance) requires the
>>>        * transfer buffers to live in their own cache lines.
>>> @@ -52,7 +54,6 @@ struct adf4350_state {
>>>   };
>>>     static struct adf4350_platform_data default_pdata = {
>>> -    .clkin = 122880000,
>>>       .channel_spacing = 10000,
>>>       .r2_user_settings = ADF4350_REG2_PD_POLARITY_POS |
>>>                   ADF4350_REG2_CHARGE_PUMP_CURR_uA(2500),
>>> @@ -235,6 +236,7 @@ static int adf4350_set_freq(struct adf4350_state *st,
>>> unsigned long long freq)
>>>           ADF4350_REG4_MUTE_TILL_LOCK_EN));
>>>         st->regs[ADF4350_REG5] = ADF4350_REG5_LD_PIN_MODE_DIGITAL;
>>> +    st->freq_req = freq;
>>>         return adf4350_sync_config(st);
>>>   }
>>> @@ -246,6 +248,7 @@ static ssize_t adf4350_write(struct iio_dev *indio_dev,
>>>   {
>>>       struct adf4350_state *st = iio_priv(indio_dev);
>>>       unsigned long long readin;
>>> +    unsigned long tmp;
>>>       int ret;
>>>         ret = kstrtoull(buf, 10, &readin);
>>> @@ -258,10 +261,23 @@ static ssize_t adf4350_write(struct iio_dev
>>> *indio_dev,
>>>           ret = adf4350_set_freq(st, readin);
>>>           break;
>>>       case ADF4350_FREQ_REFIN:
>>> -        if (readin > ADF4350_MAX_FREQ_REFIN)
>>> +        if (readin > ADF4350_MAX_FREQ_REFIN) {
>>>               ret = -EINVAL;
>>> -        else
>>> -            st->clkin = readin;
>>> +            break;
>>> +        }
>>> +
>>> +        if (st->clk) {
>>> +            tmp = clk_round_rate(st->clk, readin);
>>> +            if (tmp != readin) {
>>> +                ret = -EINVAL;
>>> +                break;
>>> +            }
>>> +            ret = clk_set_rate(st->clk, tmp);
>>> +            if (ret < 0)
>>> +                break;
>>> +        }
>>> +        st->clkin = readin;
>>> +        ret = adf4350_set_freq(st, st->freq_req);
>>>           break;
>>>       case ADF4350_FREQ_RESOLUTION:
>>>           if (readin == 0)
>>> @@ -308,6 +324,9 @@ static ssize_t adf4350_read(struct iio_dev *indio_dev,
>>>               }
>>>           break;
>>>       case ADF4350_FREQ_REFIN:
>>> +        if (st->clk)
>>> +            st->clkin = clk_get_rate(st->clk);
>>> +
>>>           val = st->clkin;
>>>           break;
>>>       case ADF4350_FREQ_RESOLUTION:
>>> @@ -318,6 +337,7 @@ static ssize_t adf4350_read(struct iio_dev *indio_dev,
>>>           break;
>>>       default:
>>>           ret = -EINVAL;
>>> +        val = 0;
>>>       }
>>>       mutex_unlock(&indio_dev->mlock);
>>>   @@ -360,14 +380,24 @@ static int adf4350_probe(struct spi_device *spi)
>>>       struct adf4350_platform_data *pdata = spi->dev.platform_data;
>>>       struct iio_dev *indio_dev;
>>>       struct adf4350_state *st;
>>> +    struct clk *clk = NULL;
>>>       int ret;
>>>         if (!pdata) {
>>>           dev_warn(&spi->dev, "no platform data? using default\n");
>>> -
>>>           pdata = &default_pdata;
>>>       }
>>>   +    if (!pdata->clkin) {
>>> +        clk = clk_get(&spi->dev, "clkin");
>>> +        if (IS_ERR(clk))
>>> +            return -EPROBE_DEFER;
>>> +
>>> +        ret = clk_prepare_enable(clk);
>>> +        if (ret < 0)
>>> +            return ret;
>>> +    }
>>> +
>>>       indio_dev = iio_device_alloc(sizeof(*st));
>>>       if (indio_dev == NULL)
>>>           return -ENOMEM;
>>> @@ -395,7 +425,12 @@ static int adf4350_probe(struct spi_device *spi)
>>>       indio_dev->num_channels = 1;
>>>         st->chspc = pdata->channel_spacing;
>>> -    st->clkin = pdata->clkin;
>>> +    if (clk) {
>>> +        st->clk = clk;
>>> +        st->clkin = clk_get_rate(clk);
>>> +    } else {
>>> +        st->clkin = pdata->clkin;
>>> +    }
>>>         st->min_out_freq = spi_get_device_id(spi)->driver_data == 4351 ?
>>>           ADF4351_MIN_OUT_FREQ : ADF4350_MIN_OUT_FREQ;
>>> @@ -435,6 +470,8 @@ error_put_reg:
>>>       if (!IS_ERR(st->reg))
>>>           regulator_put(st->reg);
>>>   +    if (clk)
>>> +        clk_disable_unprepare(clk);
>>>       iio_device_free(indio_dev);
>>>         return ret;
>>> @@ -451,6 +488,9 @@ static int adf4350_remove(struct spi_device *spi)
>>>         iio_device_unregister(indio_dev);
>>>   +    if (st->clk)
>>> +        clk_disable_unprepare(st->clk);
>>> +
>>>       if (!IS_ERR(reg)) {
>>>           regulator_disable(reg);
>>>           regulator_put(reg);
>>> @@ -481,6 +521,6 @@ static struct spi_driver adf4350_driver = {
>>>   };
>>>   module_spi_driver(adf4350_driver);
>>>   -MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
>>> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
>>>   MODULE_DESCRIPTION("Analog Devices ADF4350/ADF4351 PLL");
>>>   MODULE_LICENSE("GPL v2");
>>>
> 
> 

Re: [PATCH RESEND V2 3/4] iio: frequency: adf4350: Add support for clock consumer framework

[Jonathan Cameron]

On 05/06/13 15:25, Lars-Peter Clausen wrote:
> On 06/05/2013 09:32 AM, Michael Hennerich wrote:
>> On 06/04/2013 07:44 PM, Jonathan Cameron wrote:
>>> On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>
>>>> Preferably get clkin (PLL reference clock) from clock framework
>>>>
>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>> ERROR: "clk_round_rate" [drivers/iio/frequency/adf4350.ko] undefined!
>>> make[1]: *** [__modpost] Error 1
>>>
>>> on my arm test build. Sorry, I was being lazy before and hadn't done
>>> any test builds till I tried merging it.  Backed out the merge of this
>>> patch for now.
>>>
>>> clk.h does say that api is optional for machine classes.  No idea what you
>>> want to
>>> do about this.
>>
>> The CLK API is optional in the driver - so I prefer to keep it like that and
>> don't make
>> the driver depend on COMMON_CLK.
>>
>> The simplest and probably the best workaround is to guard the CLK Round/Set
>> code with
>> #if defined(CONFIG_COMMON_CLK).
>>
>> Thoughts?
>>
>
> This is not a bug in the driver. If the clk API is not implemented it is
> stubbed out. It looks as if Jonathan is testing on a platform which
> implements the clk API but not clk_round_rate() (None of the other clk_*
> symbols are undefined).
>
> Jonathan on which platform are you testing this?
arm specifically pxa27x eabi
>
> - Lars
>
>>
>>> Incidentally, if you want to play squash the false warnings I also get...
>>>
>>>     CC [M]  drivers/iio/frequency/adf4350.o
>>> drivers/iio/frequency/adf4350.c: In function 'adf4350_read':
>>> drivers/iio/frequency/adf4350.c:294:21: warning: 'val' may be used
>>> uninitialized in this function
>>>
>>> which I don't think is addressed in this series.  IIRC it is an obvious
>>> false positive
>>> so I've never bothered mentioning it before :)
>> Yes it is a false positive - and my compilers doesn't warn on it.
>> Anyways I can initialize val...
>>
>> -Michael
>>
>>>
>>>> ---
>>>>    drivers/iio/frequency/adf4350.c |   58
>>>> +++++++++++++++++++++++++++++++++------
>>>>    1 file changed, 49 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/drivers/iio/frequency/adf4350.c
>>>> b/drivers/iio/frequency/adf4350.c
>>>> index e76d4ac..f6849c8 100644
>>>> --- a/drivers/iio/frequency/adf4350.c
>>>> +++ b/drivers/iio/frequency/adf4350.c
>>>> @@ -1,7 +1,7 @@
>>>>    /*
>>>>     * ADF4350/ADF4351 SPI Wideband Synthesizer driver
>>>>     *
>>>> - * Copyright 2012 Analog Devices Inc.
>>>> + * Copyright 2012-2013 Analog Devices Inc.
>>>>     *
>>>>     * Licensed under the GPL-2.
>>>>     */
>>>> @@ -17,6 +17,7 @@
>>>>    #include <linux/gcd.h>
>>>>    #include <linux/gpio.h>
>>>>    #include <asm/div64.h>
>>>> +#include <linux/clk.h>
>>>>      #include <linux/iio/iio.h>
>>>>    #include <linux/iio/sysfs.h>
>>>> @@ -33,6 +34,7 @@ struct adf4350_state {
>>>>        struct spi_device        *spi;
>>>>        struct regulator        *reg;
>>>>        struct adf4350_platform_data    *pdata;
>>>> +    struct clk            *clk;
>>>>        unsigned long            clkin;
>>>>        unsigned long            chspc; /* Channel Spacing */
>>>>        unsigned long            fpfd; /* Phase Frequency Detector */
>>>> @@ -43,7 +45,7 @@ struct adf4350_state {
>>>>        unsigned            r4_rf_div_sel;
>>>>        unsigned long            regs[6];
>>>>        unsigned long            regs_hw[6];
>>>> -
>>>> +    unsigned long long        freq_req;
>>>>        /*
>>>>         * DMA (thus cache coherency maintenance) requires the
>>>>         * transfer buffers to live in their own cache lines.
>>>> @@ -52,7 +54,6 @@ struct adf4350_state {
>>>>    };
>>>>      static struct adf4350_platform_data default_pdata = {
>>>> -    .clkin = 122880000,
>>>>        .channel_spacing = 10000,
>>>>        .r2_user_settings = ADF4350_REG2_PD_POLARITY_POS |
>>>>                    ADF4350_REG2_CHARGE_PUMP_CURR_uA(2500),
>>>> @@ -235,6 +236,7 @@ static int adf4350_set_freq(struct adf4350_state *st,
>>>> unsigned long long freq)
>>>>            ADF4350_REG4_MUTE_TILL_LOCK_EN));
>>>>          st->regs[ADF4350_REG5] = ADF4350_REG5_LD_PIN_MODE_DIGITAL;
>>>> +    st->freq_req = freq;
>>>>          return adf4350_sync_config(st);
>>>>    }
>>>> @@ -246,6 +248,7 @@ static ssize_t adf4350_write(struct iio_dev *indio_dev,
>>>>    {
>>>>        struct adf4350_state *st = iio_priv(indio_dev);
>>>>        unsigned long long readin;
>>>> +    unsigned long tmp;
>>>>        int ret;
>>>>          ret = kstrtoull(buf, 10, &readin);
>>>> @@ -258,10 +261,23 @@ static ssize_t adf4350_write(struct iio_dev
>>>> *indio_dev,
>>>>            ret = adf4350_set_freq(st, readin);
>>>>            break;
>>>>        case ADF4350_FREQ_REFIN:
>>>> -        if (readin > ADF4350_MAX_FREQ_REFIN)
>>>> +        if (readin > ADF4350_MAX_FREQ_REFIN) {
>>>>                ret = -EINVAL;
>>>> -        else
>>>> -            st->clkin = readin;
>>>> +            break;
>>>> +        }
>>>> +
>>>> +        if (st->clk) {
>>>> +            tmp = clk_round_rate(st->clk, readin);
>>>> +            if (tmp != readin) {
>>>> +                ret = -EINVAL;
>>>> +                break;
>>>> +            }
>>>> +            ret = clk_set_rate(st->clk, tmp);
>>>> +            if (ret < 0)
>>>> +                break;
>>>> +        }
>>>> +        st->clkin = readin;
>>>> +        ret = adf4350_set_freq(st, st->freq_req);
>>>>            break;
>>>>        case ADF4350_FREQ_RESOLUTION:
>>>>            if (readin == 0)
>>>> @@ -308,6 +324,9 @@ static ssize_t adf4350_read(struct iio_dev *indio_dev,
>>>>                }
>>>>            break;
>>>>        case ADF4350_FREQ_REFIN:
>>>> +        if (st->clk)
>>>> +            st->clkin = clk_get_rate(st->clk);
>>>> +
>>>>            val = st->clkin;
>>>>            break;
>>>>        case ADF4350_FREQ_RESOLUTION:
>>>> @@ -318,6 +337,7 @@ static ssize_t adf4350_read(struct iio_dev *indio_dev,
>>>>            break;
>>>>        default:
>>>>            ret = -EINVAL;
>>>> +        val = 0;
>>>>        }
>>>>        mutex_unlock(&indio_dev->mlock);
>>>>    @@ -360,14 +380,24 @@ static int adf4350_probe(struct spi_device *spi)
>>>>        struct adf4350_platform_data *pdata = spi->dev.platform_data;
>>>>        struct iio_dev *indio_dev;
>>>>        struct adf4350_state *st;
>>>> +    struct clk *clk = NULL;
>>>>        int ret;
>>>>          if (!pdata) {
>>>>            dev_warn(&spi->dev, "no platform data? using default\n");
>>>> -
>>>>            pdata = &default_pdata;
>>>>        }
>>>>    +    if (!pdata->clkin) {
>>>> +        clk = clk_get(&spi->dev, "clkin");
>>>> +        if (IS_ERR(clk))
>>>> +            return -EPROBE_DEFER;
>>>> +
>>>> +        ret = clk_prepare_enable(clk);
>>>> +        if (ret < 0)
>>>> +            return ret;
>>>> +    }
>>>> +
>>>>        indio_dev = iio_device_alloc(sizeof(*st));
>>>>        if (indio_dev == NULL)
>>>>            return -ENOMEM;
>>>> @@ -395,7 +425,12 @@ static int adf4350_probe(struct spi_device *spi)
>>>>        indio_dev->num_channels = 1;
>>>>          st->chspc = pdata->channel_spacing;
>>>> -    st->clkin = pdata->clkin;
>>>> +    if (clk) {
>>>> +        st->clk = clk;
>>>> +        st->clkin = clk_get_rate(clk);
>>>> +    } else {
>>>> +        st->clkin = pdata->clkin;
>>>> +    }
>>>>          st->min_out_freq = spi_get_device_id(spi)->driver_data == 4351 ?
>>>>            ADF4351_MIN_OUT_FREQ : ADF4350_MIN_OUT_FREQ;
>>>> @@ -435,6 +470,8 @@ error_put_reg:
>>>>        if (!IS_ERR(st->reg))
>>>>            regulator_put(st->reg);
>>>>    +    if (clk)
>>>> +        clk_disable_unprepare(clk);
>>>>        iio_device_free(indio_dev);
>>>>          return ret;
>>>> @@ -451,6 +488,9 @@ static int adf4350_remove(struct spi_device *spi)
>>>>          iio_device_unregister(indio_dev);
>>>>    +    if (st->clk)
>>>> +        clk_disable_unprepare(st->clk);
>>>> +
>>>>        if (!IS_ERR(reg)) {
>>>>            regulator_disable(reg);
>>>>            regulator_put(reg);
>>>> @@ -481,6 +521,6 @@ static struct spi_driver adf4350_driver = {
>>>>    };
>>>>    module_spi_driver(adf4350_driver);
>>>>    -MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
>>>> +MODULE_AUTHOR("Michael Hennerich <michael.hennerich@analog.com>");
>>>>    MODULE_DESCRIPTION("Analog Devices ADF4350/ADF4351 PLL");
>>>>    MODULE_LICENSE("GPL v2");
>>>>
>>
>>
>

Re: [PATCH RESEND V2 3/4] iio: frequency: adf4350: Add support for clock consumer framework

[Lars-Peter Clausen]

On 06/05/2013 05:25 PM, Jonathan Cameron wrote:
> On 05/06/13 15:25, Lars-Peter Clausen wrote:
>> On 06/05/2013 09:32 AM, Michael Hennerich wrote:
>>> On 06/04/2013 07:44 PM, Jonathan Cameron wrote:
>>>> On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>
>>>>> Preferably get clkin (PLL reference clock) from clock framework
>>>>>
>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>> ERROR: "clk_round_rate" [drivers/iio/frequency/adf4350.ko] undefined!
>>>> make[1]: *** [__modpost] Error 1
>>>>
>>>> on my arm test build. Sorry, I was being lazy before and hadn't done
>>>> any test builds till I tried merging it.  Backed out the merge of this
>>>> patch for now.
>>>>
>>>> clk.h does say that api is optional for machine classes.  No idea what you
>>>> want to
>>>> do about this.
>>>
>>> The CLK API is optional in the driver - so I prefer to keep it like that and
>>> don't make
>>> the driver depend on COMMON_CLK.
>>>
>>> The simplest and probably the best workaround is to guard the CLK Round/Set
>>> code with
>>> #if defined(CONFIG_COMMON_CLK).
>>>
>>> Thoughts?
>>>
>>
>> This is not a bug in the driver. If the clk API is not implemented it is
>> stubbed out. It looks as if Jonathan is testing on a platform which
>> implements the clk API but not clk_round_rate() (None of the other clk_*
>> symbols are undefined).
>>
>> Jonathan on which platform are you testing this?
> arm specifically pxa27x eabi
>>

No clk_round_rate() in arch/arm/mach-pxa/clock.c

- Lars

Re: [PATCH RESEND V2 3/4] iio: frequency: adf4350: Add support for clock consumer framework

[Jonathan Cameron]

On 06/05/2013 06:34 PM, Lars-Peter Clausen wrote:
> On 06/05/2013 05:25 PM, Jonathan Cameron wrote:
>> On 05/06/13 15:25, Lars-Peter Clausen wrote:
>>> On 06/05/2013 09:32 AM, Michael Hennerich wrote:
>>>> On 06/04/2013 07:44 PM, Jonathan Cameron wrote:
>>>>> On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
>>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>
>>>>>> Preferably get clkin (PLL reference clock) from clock framework
>>>>>>
>>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>> ERROR: "clk_round_rate" [drivers/iio/frequency/adf4350.ko] undefined!
>>>>> make[1]: *** [__modpost] Error 1
>>>>>
>>>>> on my arm test build. Sorry, I was being lazy before and hadn't done
>>>>> any test builds till I tried merging it.  Backed out the merge of this
>>>>> patch for now.
>>>>>
>>>>> clk.h does say that api is optional for machine classes.  No idea what you
>>>>> want to
>>>>> do about this.
>>>>
>>>> The CLK API is optional in the driver - so I prefer to keep it like that and
>>>> don't make
>>>> the driver depend on COMMON_CLK.
>>>>
>>>> The simplest and probably the best workaround is to guard the CLK Round/Set
>>>> code with
>>>> #if defined(CONFIG_COMMON_CLK).
>>>>
>>>> Thoughts?
>>>>
>>>
>>> This is not a bug in the driver. If the clk API is not implemented it is
>>> stubbed out. It looks as if Jonathan is testing on a platform which
>>> implements the clk API but not clk_round_rate() (None of the other clk_*
>>> symbols are undefined).
>>>
>>> Jonathan on which platform are you testing this?
>> arm specifically pxa27x eabi
>>>
> 
> No clk_round_rate() in arch/arm/mach-pxa/clock.c
Hmm.. Indeed.  So what does one do if one wants to use those functions?


> 
> - Lars
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH RESEND V2 3/4] iio: frequency: adf4350: Add support for clock consumer framework

[Lars-Peter Clausen]

On 06/05/2013 07:43 PM, Jonathan Cameron wrote:
> On 06/05/2013 06:34 PM, Lars-Peter Clausen wrote:
>> On 06/05/2013 05:25 PM, Jonathan Cameron wrote:
>>> On 05/06/13 15:25, Lars-Peter Clausen wrote:
>>>> On 06/05/2013 09:32 AM, Michael Hennerich wrote:
>>>>> On 06/04/2013 07:44 PM, Jonathan Cameron wrote:
>>>>>> On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
>>>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>
>>>>>>> Preferably get clkin (PLL reference clock) from clock framework
>>>>>>>
>>>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>>> ERROR: "clk_round_rate" [drivers/iio/frequency/adf4350.ko] undefined!
>>>>>> make[1]: *** [__modpost] Error 1
>>>>>>
>>>>>> on my arm test build. Sorry, I was being lazy before and hadn't done
>>>>>> any test builds till I tried merging it.  Backed out the merge of this
>>>>>> patch for now.
>>>>>>
>>>>>> clk.h does say that api is optional for machine classes.  No idea what you
>>>>>> want to
>>>>>> do about this.
>>>>>
>>>>> The CLK API is optional in the driver - so I prefer to keep it like that and
>>>>> don't make
>>>>> the driver depend on COMMON_CLK.
>>>>>
>>>>> The simplest and probably the best workaround is to guard the CLK Round/Set
>>>>> code with
>>>>> #if defined(CONFIG_COMMON_CLK).
>>>>>
>>>>> Thoughts?
>>>>>
>>>>
>>>> This is not a bug in the driver. If the clk API is not implemented it is
>>>> stubbed out. It looks as if Jonathan is testing on a platform which
>>>> implements the clk API but not clk_round_rate() (None of the other clk_*
>>>> symbols are undefined).
>>>>
>>>> Jonathan on which platform are you testing this?
>>> arm specifically pxa27x eabi
>>>>
>>
>> No clk_round_rate() in arch/arm/mach-pxa/clock.c
> Hmm.. Indeed.  So what does one do if one wants to use those functions?

I think the pxa implementation of the clk API is just broken in this regard.
The clk API requires you to implement all the functions declared in
include/linux/clk.h

- Lars

Re: [PATCH RESEND V2 3/4] iio: frequency: adf4350: Add support for clock consumer framework

[Jonathan Cameron]

On 06/05/2013 07:07 PM, Lars-Peter Clausen wrote:
> On 06/05/2013 07:43 PM, Jonathan Cameron wrote:
>> On 06/05/2013 06:34 PM, Lars-Peter Clausen wrote:
>>> On 06/05/2013 05:25 PM, Jonathan Cameron wrote:
>>>> On 05/06/13 15:25, Lars-Peter Clausen wrote:
>>>>> On 06/05/2013 09:32 AM, Michael Hennerich wrote:
>>>>>> On 06/04/2013 07:44 PM, Jonathan Cameron wrote:
>>>>>>> On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
>>>>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>>
>>>>>>>> Preferably get clkin (PLL reference clock) from clock framework
>>>>>>>>
>>>>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>> ERROR: "clk_round_rate" [drivers/iio/frequency/adf4350.ko] undefined!
>>>>>>> make[1]: *** [__modpost] Error 1
>>>>>>>
>>>>>>> on my arm test build. Sorry, I was being lazy before and hadn't done
>>>>>>> any test builds till I tried merging it.  Backed out the merge of this
>>>>>>> patch for now.
>>>>>>>
>>>>>>> clk.h does say that api is optional for machine classes.  No idea what you
>>>>>>> want to
>>>>>>> do about this.
>>>>>>
>>>>>> The CLK API is optional in the driver - so I prefer to keep it like that and
>>>>>> don't make
>>>>>> the driver depend on COMMON_CLK.
>>>>>>
>>>>>> The simplest and probably the best workaround is to guard the CLK Round/Set
>>>>>> code with
>>>>>> #if defined(CONFIG_COMMON_CLK).
>>>>>>
>>>>>> Thoughts?
>>>>>>
>>>>>
>>>>> This is not a bug in the driver. If the clk API is not implemented it is
>>>>> stubbed out. It looks as if Jonathan is testing on a platform which
>>>>> implements the clk API but not clk_round_rate() (None of the other clk_*
>>>>> symbols are undefined).
>>>>>
>>>>> Jonathan on which platform are you testing this?
>>>> arm specifically pxa27x eabi
>>>>>
>>>
>>> No clk_round_rate() in arch/arm/mach-pxa/clock.c
>> Hmm.. Indeed.  So what does one do if one wants to use those functions?
> 
> I think the pxa implementation of the clk API is just broken in this regard.
> The clk API requires you to implement all the functions declared in
> include/linux/clk.h
Not according to the header it doesn't.

/*
 * The remaining APIs are optional for machine class support.
 */

Just above clk_round_rate

Odd situation admittedly but the header certainly implies that these do
not 'need' to be implemented.
> 
> - Lars
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH RESEND V2 3/4] iio: frequency: adf4350: Add support for clock consumer framework

[Lars-Peter Clausen]

On 06/05/2013 10:18 PM, Jonathan Cameron wrote:
> On 06/05/2013 07:07 PM, Lars-Peter Clausen wrote:
>> On 06/05/2013 07:43 PM, Jonathan Cameron wrote:
>>> On 06/05/2013 06:34 PM, Lars-Peter Clausen wrote:
>>>> On 06/05/2013 05:25 PM, Jonathan Cameron wrote:
>>>>> On 05/06/13 15:25, Lars-Peter Clausen wrote:
>>>>>> On 06/05/2013 09:32 AM, Michael Hennerich wrote:
>>>>>>> On 06/04/2013 07:44 PM, Jonathan Cameron wrote:
>>>>>>>> On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
>>>>>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>>>
>>>>>>>>> Preferably get clkin (PLL reference clock) from clock framework
>>>>>>>>>
>>>>>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>> ERROR: "clk_round_rate" [drivers/iio/frequency/adf4350.ko] undefined!
>>>>>>>> make[1]: *** [__modpost] Error 1
>>>>>>>>
>>>>>>>> on my arm test build. Sorry, I was being lazy before and hadn't done
>>>>>>>> any test builds till I tried merging it.  Backed out the merge of this
>>>>>>>> patch for now.
>>>>>>>>
>>>>>>>> clk.h does say that api is optional for machine classes.  No idea what you
>>>>>>>> want to
>>>>>>>> do about this.
>>>>>>>
>>>>>>> The CLK API is optional in the driver - so I prefer to keep it like that and
>>>>>>> don't make
>>>>>>> the driver depend on COMMON_CLK.
>>>>>>>
>>>>>>> The simplest and probably the best workaround is to guard the CLK Round/Set
>>>>>>> code with
>>>>>>> #if defined(CONFIG_COMMON_CLK).
>>>>>>>
>>>>>>> Thoughts?
>>>>>>>
>>>>>>
>>>>>> This is not a bug in the driver. If the clk API is not implemented it is
>>>>>> stubbed out. It looks as if Jonathan is testing on a platform which
>>>>>> implements the clk API but not clk_round_rate() (None of the other clk_*
>>>>>> symbols are undefined).
>>>>>>
>>>>>> Jonathan on which platform are you testing this?
>>>>> arm specifically pxa27x eabi
>>>>>>
>>>>
>>>> No clk_round_rate() in arch/arm/mach-pxa/clock.c
>>> Hmm.. Indeed.  So what does one do if one wants to use those functions?
>>
>> I think the pxa implementation of the clk API is just broken in this regard.
>> The clk API requires you to implement all the functions declared in
>> include/linux/clk.h
> Not according to the header it doesn't.
> 
> /*
>  * The remaining APIs are optional for machine class support.
>  */
> 
> Just above clk_round_rate
> 
> Odd situation admittedly but the header certainly implies that these do
> not 'need' to be implemented.

It should at least be stubbed out, otherwise this doesn't make too much sense.

- Lars

Re: [PATCH RESEND V2 3/4] iio: frequency: adf4350: Add support for clock consumer framework

[Jonathan Cameron]

On 06/06/2013 03:41 PM, Lars-Peter Clausen wrote:
> On 06/05/2013 10:18 PM, Jonathan Cameron wrote:
>> On 06/05/2013 07:07 PM, Lars-Peter Clausen wrote:
>>> On 06/05/2013 07:43 PM, Jonathan Cameron wrote:
>>>> On 06/05/2013 06:34 PM, Lars-Peter Clausen wrote:
>>>>> On 06/05/2013 05:25 PM, Jonathan Cameron wrote:
>>>>>> On 05/06/13 15:25, Lars-Peter Clausen wrote:
>>>>>>> On 06/05/2013 09:32 AM, Michael Hennerich wrote:
>>>>>>>> On 06/04/2013 07:44 PM, Jonathan Cameron wrote:
>>>>>>>>> On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
>>>>>>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>>>>
>>>>>>>>>> Preferably get clkin (PLL reference clock) from clock framework
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>>> ERROR: "clk_round_rate" [drivers/iio/frequency/adf4350.ko] undefined!
>>>>>>>>> make[1]: *** [__modpost] Error 1
>>>>>>>>>
>>>>>>>>> on my arm test build. Sorry, I was being lazy before and hadn't done
>>>>>>>>> any test builds till I tried merging it.  Backed out the merge of this
>>>>>>>>> patch for now.
>>>>>>>>>
>>>>>>>>> clk.h does say that api is optional for machine classes.  No idea what you
>>>>>>>>> want to
>>>>>>>>> do about this.
>>>>>>>>
>>>>>>>> The CLK API is optional in the driver - so I prefer to keep it like that and
>>>>>>>> don't make
>>>>>>>> the driver depend on COMMON_CLK.
>>>>>>>>
>>>>>>>> The simplest and probably the best workaround is to guard the CLK Round/Set
>>>>>>>> code with
>>>>>>>> #if defined(CONFIG_COMMON_CLK).
>>>>>>>>
>>>>>>>> Thoughts?
>>>>>>>>
>>>>>>>
>>>>>>> This is not a bug in the driver. If the clk API is not implemented it is
>>>>>>> stubbed out. It looks as if Jonathan is testing on a platform which
>>>>>>> implements the clk API but not clk_round_rate() (None of the other clk_*
>>>>>>> symbols are undefined).
>>>>>>>
>>>>>>> Jonathan on which platform are you testing this?
>>>>>> arm specifically pxa27x eabi
>>>>>>>
>>>>>
>>>>> No clk_round_rate() in arch/arm/mach-pxa/clock.c
>>>> Hmm.. Indeed.  So what does one do if one wants to use those functions?
>>>
>>> I think the pxa implementation of the clk API is just broken in this regard.
>>> The clk API requires you to implement all the functions declared in
>>> include/linux/clk.h
>> Not according to the header it doesn't.
>>
>> /*
>>  * The remaining APIs are optional for machine class support.
>>  */
>>
>> Just above clk_round_rate
>>
>> Odd situation admittedly but the header certainly implies that these do
>> not 'need' to be implemented.
> 
> It should at least be stubbed out, otherwise this doesn't make too much sense.
> 
> - Lars
> 
So as not to step on any toes I've raised the issue on lkml and the arm kernel
list.

My personal feeling is that architectures should just implement a stub returning
an error code to indicate it isn't available but we will see what
others think.

Jonathan

Re: [PATCH RESEND V2 4/4] iio: frequency: adf4350: Add support for dt bindings

[Jonathan Cameron]

On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> Per review feedback from Lars-Peter Clausen <lars@metafoo.de>
> Changes since V1:
> 	Fix return value handling of adf4350_parse_dt()
> 	Use of_get_gpio
> 	Avoid abbreviations in devicetree properties
> 	Fix typo in docs
> 
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
Applied to togreg branch of iio.git
> ---
>  .../devicetree/bindings/iio/frequency/adf4350.txt  |   86 +++++++++++++
>  drivers/iio/frequency/adf4350.c                    |  128 +++++++++++++++++++-
>  2 files changed, 213 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/iio/frequency/adf4350.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/frequency/adf4350.txt b/Documentation/devicetree/bindings/iio/frequency/adf4350.txt
> new file mode 100644
> index 0000000..f8c181d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/frequency/adf4350.txt
> @@ -0,0 +1,86 @@
> +Analog Devices ADF4350/ADF4351 device driver
> +
> +Required properties:
> +	- compatible: Should be one of
> +		* "adi,adf4350": When using the ADF4350 device
> +		* "adi,adf4351": When using the ADF4351 device
> +	- reg: SPI chip select numbert for the device
> +	- spi-max-frequency: Max SPI frequency to use (< 20000000)
> +	- clocks: From common clock binding. Clock is phandle to clock for
> +		ADF435x Reference Clock (CLKIN).
> +
> +Optional properties:
> +	- gpios:	 GPIO Lock detect - If set with a valid phandle and GPIO number,
> +			pll lock state is tested upon read.
> +	- adi,channel-spacing: Channel spacing in Hz (influences MODULUS).
> +	- adi,power-up-frequency:	If set in Hz the PLL tunes to
> +			the desired frequency on probe.
> +	- adi,reference-div-factor: If set the driver skips dynamic calculation
> +			and uses this default value instead.
> +	- adi,reference-doubler-enable: Enables reference doubler.
> +	- adi,reference-div2-enable: Enables reference divider.
> +	- adi,phase-detector-polarity-positive-enable: Enables positive phase
> +			detector polarity. Default = negative.
> +	- adi,lock-detect-precision-6ns-enable: Enables 6ns lock detect precision.
> +			Default = 10ns.
> +	- adi,lock-detect-function-integer-n-enable: Enables lock detect
> +			for integer-N mode. Default = factional-N mode.
> +	- adi,charge-pump-current: Charge pump current in mA.
> +			Default = 2500mA.
> +	- adi,muxout-select: On chip multiplexer output selection.
> +			Valid values for the multiplexer output are:
> +			0: Three-State Output (default)
> +			1: DVDD
> +			2: DGND
> +			3: R-Counter output
> +			4: N-Divider output
> +			5: Analog lock detect
> +			6: Digital lock detect
> +	- adi,low-spur-mode-enable: Enables low spur mode.
> +			Default = Low noise mode.
> +	- adi,cycle-slip-reduction-enable: Enables cycle slip reduction.
> +	- adi,charge-cancellation-enable: Enabled charge pump
> +			charge cancellation for integer-N modes.
> +	- adi,anti-backlash-3ns-enable: Enables 3ns antibacklash pulse width
> +			 for integer-N modes.
> +	- adi,band-select-clock-mode-high-enable: Enables faster band
> +			selection logic.
> +	- adi,12bit-clk-divider: Clock divider value used when
> +			adi,12bit-clkdiv-mode != 0
> +	- adi,clk-divider-mode:
> +			Valid values for the clkdiv mode are:
> +			0: Clock divider off (default)
> +			1: Fast lock enable
> +			2: Phase resync enable
> +	- adi,aux-output-enable: Enables auxiliary RF output.
> +	- adi,aux-output-fundamental-enable: Selects fundamental VCO output on
> +			the auxiliary RF output. Default = Output of RF dividers.
> +	- adi,mute-till-lock-enable: Enables Mute-Till-Lock-Detect function.
> +	- adi,output-power: Output power selection.
> +			Valid values for the power mode are:
> +			0: -4dBm (default)
> +			1: -1dBm
> +			2: +2dBm
> +			3: +5dBm
> +	- adi,aux-output-power: Auxiliary output power selection.
> +			Valid values for the power mode are:
> +			0: -4dBm (default)
> +			1: -1dBm
> +			2: +2dBm
> +			3: +5dBm
> +
> +
> +Example:
> +		lo_pll0_rx_adf4351: adf4351-rx-lpc@4 {
> +			compatible = "adi,adf4351";
> +			reg = <4>;
> +			spi-max-frequency = <10000000>;
> +			clocks = <&clk0_ad9523 9>;
> +			clock-names = "clkin";
> +			adi,channel-spacing = <10000>;
> +			adi,power-up-frequency = <2400000000>;
> +			adi,phase-detector-polarity-positive-enable;
> +			adi,charge-pump-current = <2500>;
> +			adi,output-power = <3>;
> +			adi,mute-till-lock-enable;
> +		};
> diff --git a/drivers/iio/frequency/adf4350.c b/drivers/iio/frequency/adf4350.c
> index f6849c8..a4157cd 100644
> --- a/drivers/iio/frequency/adf4350.c
> +++ b/drivers/iio/frequency/adf4350.c
> @@ -18,6 +18,8 @@
>  #include <linux/gpio.h>
>  #include <asm/div64.h>
>  #include <linux/clk.h>
> +#include <linux/of.h>
> +#include <linux/of_gpio.h>
>  
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -375,14 +377,138 @@ static const struct iio_info adf4350_info = {
>  	.driver_module = THIS_MODULE,
>  };
>  
> +#ifdef CONFIG_OF
> +static struct adf4350_platform_data *adf4350_parse_dt(struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	struct adf4350_platform_data *pdata;
> +	unsigned int tmp;
> +	int ret;
> +
> +	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata) {
> +		dev_err(dev, "could not allocate memory for platform data\n");
> +		return NULL;
> +	}
> +
> +	strncpy(&pdata->name[0], np->name, SPI_NAME_SIZE - 1);
> +
> +	tmp = 10000;
> +	of_property_read_u32(np, "adi,channel-spacing", &tmp);
> +	pdata->channel_spacing = tmp;
> +
> +	tmp = 0;
> +	of_property_read_u32(np, "adi,power-up-frequency", &tmp);
> +	pdata->power_up_frequency = tmp;
> +
> +	tmp = 0;
> +	of_property_read_u32(np, "adi,reference-div-factor", &tmp);
> +	pdata->ref_div_factor = tmp;
> +
> +	ret = of_get_gpio(np, 0);
> +	if (ret < 0)
> +		pdata->gpio_lock_detect = -1;
> +	else
> +		pdata->gpio_lock_detect = ret;
> +
> +	pdata->ref_doubler_en = of_property_read_bool(np,
> +			"adi,reference-doubler-enable");
> +	pdata->ref_div2_en = of_property_read_bool(np,
> +			"adi,reference-div2-enable");
> +
> +	/* r2_user_settings */
> +	pdata->r2_user_settings = of_property_read_bool(np,
> +			"adi,phase-detector-polarity-positive-enable") ?
> +			ADF4350_REG2_PD_POLARITY_POS : 0;
> +	pdata->r2_user_settings |= of_property_read_bool(np,
> +			"adi,lock-detect-precision-6ns-enable") ?
> +			ADF4350_REG2_LDP_6ns : 0;
> +	pdata->r2_user_settings |= of_property_read_bool(np,
> +			"adi,lock-detect-function-integer-n-enable") ?
> +			ADF4350_REG2_LDF_INT_N : 0;
> +
> +	tmp = 2500;
> +	of_property_read_u32(np, "adi,charge-pump-current", &tmp);
> +	pdata->r2_user_settings |= ADF4350_REG2_CHARGE_PUMP_CURR_uA(tmp);
> +
> +	tmp = 0;
> +	of_property_read_u32(np, "adi,muxout-select", &tmp);
> +	pdata->r2_user_settings |= ADF4350_REG2_MUXOUT(tmp);
> +
> +	pdata->r2_user_settings |= of_property_read_bool(np,
> +			"adi,low-spur-mode-enable") ?
> +			ADF4350_REG2_NOISE_MODE(0x3) : 0;
> +
> +	/* r3_user_settings */
> +
> +	pdata->r3_user_settings = of_property_read_bool(np,
> +			"adi,cycle-slip-reduction-enable") ?
> +			ADF4350_REG3_12BIT_CSR_EN : 0;
> +	pdata->r3_user_settings |= of_property_read_bool(np,
> +			"adi,charge-cancellation-enable") ?
> +			ADF4351_REG3_CHARGE_CANCELLATION_EN : 0;
> +
> +	pdata->r3_user_settings |= of_property_read_bool(np,
> +			"adi,anti-backlash-3ns-enable") ?
> +			ADF4351_REG3_ANTI_BACKLASH_3ns_EN : 0;
> +	pdata->r3_user_settings |= of_property_read_bool(np,
> +			"adi,band-select-clock-mode-high-enable") ?
> +			ADF4351_REG3_BAND_SEL_CLOCK_MODE_HIGH : 0;
> +
> +	tmp = 0;
> +	of_property_read_u32(np, "adi,12bit-clk-divider", &tmp);
> +	pdata->r3_user_settings |= ADF4350_REG3_12BIT_CLKDIV(tmp);
> +
> +	tmp = 0;
> +	of_property_read_u32(np, "adi,clk-divider-mode", &tmp);
> +	pdata->r3_user_settings |= ADF4350_REG3_12BIT_CLKDIV_MODE(tmp);
> +
> +	/* r4_user_settings */
> +
> +	pdata->r4_user_settings = of_property_read_bool(np,
> +			"adi,aux-output-enable") ?
> +			ADF4350_REG4_AUX_OUTPUT_EN : 0;
> +	pdata->r4_user_settings |= of_property_read_bool(np,
> +			"adi,aux-output-fundamental-enable") ?
> +			ADF4350_REG4_AUX_OUTPUT_FUND : 0;
> +	pdata->r4_user_settings |= of_property_read_bool(np,
> +			"adi,mute-till-lock-enable") ?
> +			ADF4350_REG4_MUTE_TILL_LOCK_EN : 0;
> +
> +	tmp = 0;
> +	of_property_read_u32(np, "adi,output-power", &tmp);
> +	pdata->r4_user_settings |= ADF4350_REG4_OUTPUT_PWR(tmp);
> +
> +	tmp = 0;
> +	of_property_read_u32(np, "adi,aux-output-power", &tmp);
> +	pdata->r4_user_settings |= ADF4350_REG4_AUX_OUTPUT_PWR(tmp);
> +
> +	return pdata;
> +}
> +#else
> +static
> +struct adf4350_platform_data *adf4350_parse_dt(struct device *dev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  static int adf4350_probe(struct spi_device *spi)
>  {
> -	struct adf4350_platform_data *pdata = spi->dev.platform_data;
> +	struct adf4350_platform_data *pdata;
>  	struct iio_dev *indio_dev;
>  	struct adf4350_state *st;
>  	struct clk *clk = NULL;
>  	int ret;
>  
> +	if (spi->dev.of_node) {
> +		pdata = adf4350_parse_dt(&spi->dev);
> +		if (pdata == NULL)
> +			return -EINVAL;
> +	} else {
> +		pdata = spi->dev.platform_data;
> +	}
> +
>  	if (!pdata) {
>  		dev_warn(&spi->dev, "no platform data? using default\n");
>  		pdata = &default_pdata;
> 

Re: [PATCH RESEND V2 3/4] iio: frequency: adf4350: Add support for clock consumer framework

[Jonathan Cameron]

On 06/08/2013 09:58 AM, Jonathan Cameron wrote:
> On 06/06/2013 03:41 PM, Lars-Peter Clausen wrote:
>> On 06/05/2013 10:18 PM, Jonathan Cameron wrote:
>>> On 06/05/2013 07:07 PM, Lars-Peter Clausen wrote:
>>>> On 06/05/2013 07:43 PM, Jonathan Cameron wrote:
>>>>> On 06/05/2013 06:34 PM, Lars-Peter Clausen wrote:
>>>>>> On 06/05/2013 05:25 PM, Jonathan Cameron wrote:
>>>>>>> On 05/06/13 15:25, Lars-Peter Clausen wrote:
>>>>>>>> On 06/05/2013 09:32 AM, Michael Hennerich wrote:
>>>>>>>>> On 06/04/2013 07:44 PM, Jonathan Cameron wrote:
>>>>>>>>>> On 06/03/2013 02:30 PM, michael.hennerich@analog.com wrote:
>>>>>>>>>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>>>>>
>>>>>>>>>>> Preferably get clkin (PLL reference clock) from clock framework
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>>>>>>>>> ERROR: "clk_round_rate" [drivers/iio/frequency/adf4350.ko] undefined!
>>>>>>>>>> make[1]: *** [__modpost] Error 1
>>>>>>>>>>
>>>>>>>>>> on my arm test build. Sorry, I was being lazy before and hadn't done
>>>>>>>>>> any test builds till I tried merging it.  Backed out the merge of this
>>>>>>>>>> patch for now.
>>>>>>>>>>
>>>>>>>>>> clk.h does say that api is optional for machine classes.  No idea what you
>>>>>>>>>> want to
>>>>>>>>>> do about this.
>>>>>>>>>
>>>>>>>>> The CLK API is optional in the driver - so I prefer to keep it like that and
>>>>>>>>> don't make
>>>>>>>>> the driver depend on COMMON_CLK.
>>>>>>>>>
>>>>>>>>> The simplest and probably the best workaround is to guard the CLK Round/Set
>>>>>>>>> code with
>>>>>>>>> #if defined(CONFIG_COMMON_CLK).
>>>>>>>>>
>>>>>>>>> Thoughts?
>>>>>>>>>
>>>>>>>>
>>>>>>>> This is not a bug in the driver. If the clk API is not implemented it is
>>>>>>>> stubbed out. It looks as if Jonathan is testing on a platform which
>>>>>>>> implements the clk API but not clk_round_rate() (None of the other clk_*
>>>>>>>> symbols are undefined).
>>>>>>>>
>>>>>>>> Jonathan on which platform are you testing this?
>>>>>>> arm specifically pxa27x eabi
>>>>>>>>
>>>>>>
>>>>>> No clk_round_rate() in arch/arm/mach-pxa/clock.c
>>>>> Hmm.. Indeed.  So what does one do if one wants to use those functions?
>>>>
>>>> I think the pxa implementation of the clk API is just broken in this regard.
>>>> The clk API requires you to implement all the functions declared in
>>>> include/linux/clk.h
>>> Not according to the header it doesn't.
>>>
>>> /*
>>>  * The remaining APIs are optional for machine class support.
>>>  */
>>>
>>> Just above clk_round_rate
>>>
>>> Odd situation admittedly but the header certainly implies that these do
>>> not 'need' to be implemented.
>>
>> It should at least be stubbed out, otherwise this doesn't make too much sense.
>>
>> - Lars
>>
> So as not to step on any toes I've raised the issue on lkml and the arm kernel
> list.
> 
> My personal feeling is that architectures should just implement a stub returning
> an error code to indicate it isn't available but we will see what
> others think.
> 
A fix is on its way for my test platform so given that everyone acknowledges that
these functions are no longer optional, I've applied this patch to the togreg
branch of iio.git.  Sorry for the delay.  Will be interesting to see if there
any other subarchs that are missing this.

Jonathan
> Jonathan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>