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

Jonathan Cameron <jic23@xxxxxxxxxxxxxxxxxxxxx> · Wed, 05 Jun 2013 16:25:52 +0100

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@xxxxxxxxxx wrote:
From: Michael Hennerich <michael.hennerich@xxxxxxxxxx>

Preferably get clkin (PLL reference clock) from clock framework

Signed-off-by: Michael Hennerich <michael.hennerich@xxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxx>");
+MODULE_AUTHOR("Michael Hennerich <michael.hennerich@xxxxxxxxxx>");
   MODULE_DESCRIPTION("Analog Devices ADF4350/ADF4351 PLL");
   MODULE_LICENSE("GPL v2");





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







