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

[Michael Hennerich <michael.hennerich@analog.com>]

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