Re: [PATCH 3/3] iio: frequency: New driver for AD9523 SPI Low Jitter Clock Generator

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

On 05/12/2012 08:19 PM, Jonathan Cameron wrote:
> On 05/04/2012 02:31 PM, michael.hennerich@analog.com wrote:
>> From: Michael Hennerich<michael.hennerich@analog.com>
>>
> Various  bits inline, one thing I don't follow is why the number of
> channels is controlled by platform data?  Normally we just export
> them whether or not they are connected...
HI Jonathan,

Thanks for the review.

There are a few things in the output channel config, which are
clearly platform specific, such as the output driver mode.
A channel without proper output driver set, won't work.
Things like output driver shouldn't be runtime controllable -
Therefore we hide unused channels.

>> Signed-off-by: Michael Hennerich<michael.hennerich@analog.com>
>> ---
>>   .../Documentation/frequency/sysfs-bus-iio-ad9523   |   31 +
>>   drivers/staging/iio/frequency/Kconfig              |   22 +-
>>   drivers/staging/iio/frequency/Makefile             |    3 +-
>>   drivers/staging/iio/frequency/ad9523.c             | 1040 ++++++++++++++++++++
>>   drivers/staging/iio/frequency/ad9523.h             |  112 +++
>>   5 files changed, 1206 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/staging/iio/Documentation/frequency/sysfs-bus-iio-ad9523
>>   create mode 100644 drivers/staging/iio/frequency/ad9523.c
>>   create mode 100644 drivers/staging/iio/frequency/ad9523.h
>>
>> diff --git a/drivers/staging/iio/Documentation/frequency/sysfs-bus-iio-ad9523 b/drivers/staging/iio/Documentation/frequency/sysfs-bus-iio-ad9523
>> new file mode 100644
>> index 0000000..79328d1
>> --- /dev/null
>> +++ b/drivers/staging/iio/Documentation/frequency/sysfs-bus-iio-ad9523
>> @@ -0,0 +1,31 @@
>> +What:                /sys/bus/iio/devices/iio:deviceX/status_pllY_lock_detect
>> +What:                /sys/bus/iio/devices/iio:deviceX/status_pllY_feedback_clock
>> +What:                /sys/bus/iio/devices/iio:deviceX/status_pllY_reference_clock
>> +What:                /sys/bus/iio/devices/iio:deviceX/status_reference_a
>> +What:                /sys/bus/iio/devices/iio:deviceX/status_reference_b
>> +What:                /sys/bus/iio/devices/iio:deviceX/status_reference_test
>> +What:                /sys/bus/iio/devices/iio:deviceX/status_vcxo
>> +KernelVersion:       3.4.0
>> +Contact:     linux-iio@vger.kernel.org
>> +Description:
>> +             Reading returns either 'OK' or 'FAIL'. 'OK' means that the
>> +             clock in question is present or the pllY is locked.
> I'd break this into two descriptions.  One for pllY locked and one for
> the clocks.
> Also I have no idea what the reference tests are form this description!
REF_TEST is an reference clock bypass test input for PLL1. REF_TEST is
similar to REFA or REFB, but bypasses the REF selection and switchover 
logic.
REF_TEST is typically not used.
I'll add some comment.

>> 'FAIL'
>> +             means that the clock is missing or the pllY is unlocked.
> These look like the sort of thing that has previously led to discussion of
> an out of band means of reporting 'device failures' within the kernel.
> Still I'm not aware of anyone having actually worked on that, so I guess
> we need something like that.  I wonder if OK / FAIL is the best option.
> Would have suggested a simple 1/0 pair but obviously for somethings
> we'd need very specific naming to indicate whether 1 was a good thing
> or a bad thing.  Maybe an alternative naming might be (putting this
> out there mainly to open up debate - not because I necessarily think
> it improves on what you have!)
>
> pllY_locked
> pllY_feedback_clk_present
> pllY_reference_clk_present
>
> looking at the datasheet, aren't refa and refb two separate reference
> clk inputs?  Thats not clear in current naming if true.
Reference_a and reference_b are two separate inputs.
Do you mean we should better name it.

pll2_locked
pll2_feedback_clk_present
pll2_reference_clk_present

pll1_locked
pll1_reference_clk_a_present
pll1_reference_clk_b_present
pll1_reference_clk_test_present

With this naming a 0/1 pair would make sense.


>
> When you get down to it I'm not entirely sure on what you attributes
> all mean, so on that basis the naming / description need some work!
> I had to look up what a vcxo actually was..  x = crystal?
Voltage Controlled Crystal Oscillator.
X for Crystal is a pretty common.
>
>> +
>> +What:                /sys/bus/iio/devices/iio:deviceX/store_eeprom
>> +KernelVersion:       3.4.0
>> +Contact:     linux-iio@vger.kernel.org
>> +Description:
>> +             Writing '1' stores the current device configuration into
>> +             on-chip EEPROM. After power-up or chip reset the device will
>> +             automatically load the saved configuration.
> This is a fairly common thing to see on devices.  The complexity lies in
> that
> sometimes only a subset of stuff is saved...  I guess there is no harm
> in what you have here, but we may well want to think about it a bit more
> at a later date!
>
>> +
>> +What:                /sys/bus/iio/devices/iio:deviceX/sync_dividers
>> +KernelVersion:       3.4.0
>> +Contact:     linux-iio@vger.kernel.org
>> +Description:
>> +             Writing '1' triggers the clock distribution synchronization
>> +             functionality. All dividers are reset and the channels start
>> +             with their predefined phase offsets (out_altvoltageY_phase).
>> +             Writing this file has the effect as driving the external
>> +             /SYNC pin low.
>> diff --git a/drivers/staging/iio/frequency/Kconfig b/drivers/staging/iio/frequency/Kconfig
>> index 93b7141..7b2c0c3 100644
>> --- a/drivers/staging/iio/frequency/Kconfig
>> +++ b/drivers/staging/iio/frequency/Kconfig
>> @@ -1,6 +1,25 @@
>>   #
>> -# Direct Digital Synthesis drivers
>> +# Frequency
>> +#    Direct Digital Synthesis drivers (DDS)
>> +#    Clock Distribution device drivers
>> +#    Phase-Locked Loop (PLL) frequency synthesizers
>>   #
>> +
>> +menu "Frequency Synthesizers DDS/PLL"
> a newline here?
>> +menu "Clock Generator/Distribution"
>> +
>> +config AD9523
>> +     tristate "Analog Devices AD9523 Low Jitter Clock Generator"
>> +     depends on SPI
>> +     help
>> +       Say yes here to build support for Analog Devices AD9523 Low Jitter
>> +       Clock Generator. The driver provides direct access via sysfs.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called ad9523.
>> +
>> +endmenu
>> +
>>   menu "Direct Digital Synthesis"
>>
>>   config AD5930
>> @@ -59,3 +78,4 @@ config AD9951
>>          ad9951, provides direct access via sysfs.
>>
>>   endmenu
>> +endmenu
>> diff --git a/drivers/staging/iio/frequency/Makefile b/drivers/staging/iio/frequency/Makefile
>> index 1477461..e0157b6 100644
>> --- a/drivers/staging/iio/frequency/Makefile
>> +++ b/drivers/staging/iio/frequency/Makefile
>> @@ -1,5 +1,5 @@
>>   #
>> -# Makefile for Direct Digital Synthesis drivers
>> +# Makefile iio/frequency
>>   #
>>
>>   obj-$(CONFIG_AD5930) += ad5930.o
>> @@ -9,3 +9,4 @@ obj-$(CONFIG_AD9850) += ad9850.o
>>   obj-$(CONFIG_AD9852) += ad9852.o
>>   obj-$(CONFIG_AD9910) += ad9910.o
>>   obj-$(CONFIG_AD9951) += ad9951.o
>> +obj-$(CONFIG_AD9523) += ad9523.o
>> diff --git a/drivers/staging/iio/frequency/ad9523.c b/drivers/staging/iio/frequency/ad9523.c
>> new file mode 100644
>> index 0000000..bb43386
>> --- /dev/null
>> +++ b/drivers/staging/iio/frequency/ad9523.c
>> @@ -0,0 +1,1040 @@
>> +/*
>> + * AD9523 SPI Low Jitter Clock Generator
>> + *
>> + * Copyright 2012 Analog Devices Inc.
>> + *
>> + * Licensed under the GPL-2.
>> + */
>> +
>> +#include<linux/device.h>
>> +#include<linux/kernel.h>
>> +#include<linux/slab.h>
>> +#include<linux/sysfs.h>
>> +#include<linux/spi/spi.h>
>> +#include<linux/regulator/consumer.h>
>> +#include<linux/err.h>
>> +#include<linux/module.h>
>> +#include<linux/delay.h>
>> +
>> +#include<linux/iio/iio.h>
>> +#include<linux/iio/sysfs.h>
>> +
>> +#include "ad9523.h"
>> +
>> +#define AD9523_READ  (1<<  15)
>> +#define AD9523_WRITE (0<<  15)
>> +#define AD9523_CNT(x)        (((x) - 1)<<  13)
>> +#define AD9523_ADDR(x)       ((x)&  0xFFF)
>> +
>> +#define AD9523_R1B   (1<<  16)
>> +#define AD9523_R2B   (2<<  16)
>> +#define AD9523_R3B   (3<<  16)
>> +#define AD9523_TRANSF_LEN(x)                 ((x)>>  16)
>> +
>> +#define AD9523_SERIAL_PORT_CONFIG            (AD9523_R1B | 0x0)
>> +#define AD9523_VERSION_REGISTER                      (AD9523_R1B | 0x2)
>> +#define AD9523_PART_REGISTER                 (AD9523_R1B | 0x3)
>> +#define AD9523_READBACK_CTRL                 (AD9523_R1B | 0x4)
>> +
>> +#define AD9523_EEPROM_CUSTOMER_VERSION_ID    (AD9523_R2B | 0x6)
>> +
>> +#define AD9523_PLL1_REF_A_DIVIDER            (AD9523_R2B | 0x11)
>> +#define AD9523_PLL1_REF_B_DIVIDER            (AD9523_R2B | 0x13)
>> +#define AD9523_PLL1_REF_TEST_DIVIDER         (AD9523_R1B | 0x14)
>> +#define AD9523_PLL1_FEEDBACK_DIVIDER         (AD9523_R2B | 0x17)
>> +#define AD9523_PLL1_CHARGE_PUMP_CTRL         (AD9523_R2B | 0x19)
>> +#define AD9523_PLL1_INPUT_RECEIVERS_CTRL     (AD9523_R1B | 0x1A)
>> +#define AD9523_PLL1_REF_CTRL                 (AD9523_R1B | 0x1B)
>> +#define AD9523_PLL1_MISC_CTRL                        (AD9523_R1B | 0x1C)
>> +#define AD9523_PLL1_LOOP_FILTER_CTRL         (AD9523_R1B | 0x1D)
>> +
>> +#define AD9523_PLL2_CHARGE_PUMP                      (AD9523_R1B | 0xF0)
>> +#define AD9523_PLL2_FEEDBACK_DIVIDER_AB              (AD9523_R1B | 0xF1)
>> +#define AD9523_PLL2_CTRL                     (AD9523_R1B | 0xF2)
>> +#define AD9523_PLL2_VCO_CTRL                 (AD9523_R1B | 0xF3)
>> +#define AD9523_PLL2_VCO_DIVIDER                      (AD9523_R1B | 0xF4)
>> +#define AD9523_PLL2_LOOP_FILTER_CTRL         (AD9523_R2B | 0xF6)
>> +#define AD9523_PLL2_R2_DIVIDER                       (AD9523_R1B | 0xF7)
>> +
>> +#define AD9523_CHANNEL_CLOCK_DIST(ch)                (AD9523_R3B | (0x192 + 3 * ch))
>> +
>> +#define AD9523_PLL1_OUTPUT_CTRL                      (AD9523_R1B | 0x1BA)
>> +#define AD9523_PLL1_OUTPUT_CHANNEL_CTRL              (AD9523_R1B | 0x1BB)
>> +
>> +#define AD9523_READBACK_0                    (AD9523_R1B | 0x22C)
>> +#define AD9523_READBACK_1                    (AD9523_R1B | 0x22D)
>> +
>> +#define AD9523_STATUS_SIGNALS                        (AD9523_R3B | 0x232)
>> +#define AD9523_POWER_DOWN_CTRL                       (AD9523_R1B | 0x233)
>> +#define AD9523_IO_UPDATE                     (AD9523_R1B | 0x234)
>> +
>> +#define AD9523_EEPROM_DATA_XFER_STATUS               (AD9523_R1B | 0xB00)
>> +#define AD9523_EEPROM_ERROR_READBACK         (AD9523_R1B | 0xB01)
>> +#define AD9523_EEPROM_CTRL1                  (AD9523_R1B | 0xB02)
>> +#define AD9523_EEPROM_CTRL2                  (AD9523_R1B | 0xB03)
>> +
>> +/* AD9523_SERIAL_PORT_CONFIG */
>> +
>> +#define AD9523_SER_CONF_SDO_ACTIVE           (1<<  7)
>> +#define AD9523_SER_CONF_SOFT_RESET           (1<<  5)
>> +
>> +/* AD9523_READBACK_CTRL */
>> +#define AD9523_READBACK_CTRL_READ_BUFFERED   (1<<  0)
>> +
>> +/* AD9523_PLL1_CHARGE_PUMP_CTRL */
>> +#define AD9523_PLL1_CHARGE_PUMP_CURRENT_nA(x)        (((x) / 500)&  0x7F)
>> +#define AD9523_PLL1_CHARGE_PUMP_TRISTATE     (1<<  7)
>> +#define AD9523_PLL1_CHARGE_PUMP_MODE_NORMAL  (3<<  8)
>> +#define AD9523_PLL1_CHARGE_PUMP_MODE_PUMP_DOWN       (2<<  8)
>> +#define AD9523_PLL1_CHARGE_PUMP_MODE_PUMP_UP (1<<  8)
>> +#define AD9523_PLL1_CHARGE_PUMP_MODE_TRISTATE        (0<<  8)
>> +#define AD9523_PLL1_BACKLASH_PW_MIN          (0<<  10)
>> +#define AD9523_PLL1_BACKLASH_PW_LOW          (1<<  10)
>> +#define AD9523_PLL1_BACKLASH_PW_HIGH         (2<<  10)
>> +#define AD9523_PLL1_BACKLASH_PW_MAX          (3<<  10)
>> +
>> +/* AD9523_PLL1_INPUT_RECEIVERS_CTRL */
>> +#define AD9523_PLL1_REF_TEST_RCV_EN          (1<<  7)
>> +#define AD9523_PLL1_REFB_DIFF_RCV_EN         (1<<  6)
>> +#define AD9523_PLL1_REFA_DIFF_RCV_EN         (1<<  5)
>> +#define AD9523_PLL1_REFB_RCV_EN                      (1<<  4)
>> +#define AD9523_PLL1_REFA_RCV_EN                      (1<<  3)
>> +#define AD9523_PLL1_REFA_REFB_PWR_CTRL_EN    (1<<  2)
>> +#define AD9523_PLL1_OSC_IN_CMOS_NEG_INP_EN   (1<<  1)
>> +#define AD9523_PLL1_OSC_IN_DIFF_EN           (1<<  0)
>> +
>> +/* AD9523_PLL1_REF_CTRL */
>> +#define AD9523_PLL1_BYPASS_REF_TEST_DIV_EN   (1<<  7)
>> +#define AD9523_PLL1_BYPASS_FEEDBACK_DIV_EN   (1<<  6)
>> +#define AD9523_PLL1_ZERO_DELAY_MODE_INT              (1<<  5)
>> +#define AD9523_PLL1_ZERO_DELAY_MODE_EXT              (0<<  5)
>> +#define AD9523_PLL1_OSC_IN_PLL_FEEDBACK_EN   (1<<  4)
>> +#define AD9523_PLL1_ZD_IN_CMOS_NEG_INP_EN    (1<<  3)
>> +#define AD9523_PLL1_ZD_IN_DIFF_EN            (1<<  2)
>> +#define AD9523_PLL1_REFB_CMOS_NEG_INP_EN     (1<<  1)
>> +#define AD9523_PLL1_REFA_CMOS_NEG_INP_EN     (1<<  0)
>> +
>> +/* AD9523_PLL1_MISC_CTRL */
>> +#define AD9523_PLL1_REFB_INDEP_DIV_CTRL_EN   (1<<  7)
>> +#define AD9523_PLL1_OSC_CTRL_FAIL_VCC_BY2_EN (1<<  6)
>> +#define AD9523_PLL1_REF_MODE(x)                      ((x)<<  2)
>> +#define AD9523_PLL1_BYPASS_REFB_DIV          (1<<  1)
>> +#define AD9523_PLL1_BYPASS_REFA_DIV          (1<<  0)
>> +
>> +/* AD9523_PLL1_LOOP_FILTER_CTRL */
>> +#define AD9523_PLL1_LOOP_FILTER_RZERO(x)     ((x)&  0xF)
>> +
>> +/* AD9523_PLL2_CHARGE_PUMP */
>> +#define AD9523_PLL2_CHARGE_PUMP_CURRENT_nA(x)        ((x) / 3500)
>> +
>> +/* AD9523_PLL2_FEEDBACK_DIVIDER_AB */
>> +#define AD9523_PLL2_FB_NDIV_A_CNT(x)         (((x)&  0x3)<<  6)
>> +#define AD9523_PLL2_FB_NDIV_B_CNT(x)         (((x)&  0x3F)<<  0)
>> +#define AD9523_PLL2_FB_NDIV(a, b)            (4 * (b) + (a))
>> +
>> +/* AD9523_PLL2_CTRL */
>> +#define AD9523_PLL2_CHARGE_PUMP_MODE_NORMAL  (3<<  0)
>> +#define AD9523_PLL2_CHARGE_PUMP_MODE_PUMP_DOWN       (2<<  0)
>> +#define AD9523_PLL2_CHARGE_PUMP_MODE_PUMP_UP (1<<  0)
>> +#define AD9523_PLL2_CHARGE_PUMP_MODE_TRISTATE        (0<<  0)
>> +#define AD9523_PLL2_BACKLASH_PW_MIN          (0<<  2)
>> +#define AD9523_PLL2_BACKLASH_PW_LOW          (1<<  2)
>> +#define AD9523_PLL2_BACKLASH_PW_HIGH         (2<<  2)
>> +#define AD9523_PLL2_BACKLASH_PW_MAX          (3<<  1)
>> +#define AD9523_PLL2_BACKLASH_CTRL_EN         (1<<  4)
>> +#define AD9523_PLL2_FREQ_DOUBLER_EN          (1<<  5)
>> +#define AD9523_PLL2_LOCK_DETECT_PWR_DOWN_EN  (1<<  7)
>> +
>> +/* AD9523_PLL2_VCO_CTRL */
>> +#define AD9523_PLL2_VCO_CALIBRATE            (1<<  1)
>> +#define AD9523_PLL2_FORCE_VCO_MIDSCALE               (1<<  2)
>> +#define AD9523_PLL2_FORCE_REFERENCE_VALID    (1<<  3)
>> +#define AD9523_PLL2_FORCE_RELEASE_SYNC               (1<<  4)
>> +
>> +/* AD9523_PLL2_VCO_DIVIDER */
>> +#define AD9523_PLL2_VCO_DIV_M1(x)            ((((x) - 3)&  0x3)<<  0)
>> +#define AD9523_PLL2_VCO_DIV_M2(x)            ((((x) - 3)&  0x3)<<  4)
>> +#define AD9523_PLL2_VCO_DIV_M1_PWR_DOWN_EN   (1<<  2)
>> +#define AD9523_PLL2_VCO_DIV_M2_PWR_DOWN_EN   (1<<  6)
>> +
>> +/* AD9523_PLL2_LOOP_FILTER_CTRL */
>> +#define AD9523_PLL2_LOOP_FILTER_CPOLE1(x)    (((x)&  0x7)<<  0)
>> +#define AD9523_PLL2_LOOP_FILTER_RZERO(x)     (((x)&  0x7)<<  3)
>> +#define AD9523_PLL2_LOOP_FILTER_RPOLE2(x)    (((x)&  0x7)<<  6)
>> +#define AD9523_PLL2_LOOP_FILTER_RZERO_BYPASS_EN      (1<<  8)
>> +
>> +/* AD9523_PLL2_R2_DIVIDER */
>> +#define AD9523_PLL2_R2_DIVIDER_VAL(x)                (((x)&  0x1F)<<  0)
>> +
>> +/* AD9523_CHANNEL_CLOCK_DIST */
>> +#define AD9523_CLK_DIST_DIV_PHASE(x)         (((x)&  0x3F)<<  18)
>> +#define AD9523_CLK_DIST_DIV_PHASE_REV(x)     ((ret>>  18)&  0x3F)
>> +#define AD9523_CLK_DIST_DIV(x)                       ((((x) - 1)&  0x3FF)<<  8)
>> +#define AD9523_CLK_DIST_DIV_REV(x)           (((ret>>  8)&  0x3FF) + 1)
>> +#define AD9523_CLK_DIST_INV_DIV_OUTPUT_EN    (1<<  7)
>> +#define AD9523_CLK_DIST_IGNORE_SYNC_EN               (1<<  6)
>> +#define AD9523_CLK_DIST_PWR_DOWN_EN          (1<<  5)
>> +#define AD9523_CLK_DIST_LOW_PWR_MODE_EN              (1<<  4)
>> +#define AD9523_CLK_DIST_DRIVER_MODE(x)               (((x)&  0xF)<<  0)
>> +
>> +/* AD9523_PLL1_OUTPUT_CTRL */
>> +#define AD9523_PLL1_OUTP_CTRL_VCO_DIV_SEL_CH6_M2     (1<<  7)
>> +#define AD9523_PLL1_OUTP_CTRL_VCO_DIV_SEL_CH5_M2     (1<<  6)
>> +#define AD9523_PLL1_OUTP_CTRL_VCO_DIV_SEL_CH4_M2     (1<<  5)
>> +#define AD9523_PLL1_OUTP_CTRL_CMOS_DRV_WEAK          (1<<  4)
>> +#define AD9523_PLL1_OUTP_CTRL_OUTPUT_DIV_1           (0<<  0)
>> +#define AD9523_PLL1_OUTP_CTRL_OUTPUT_DIV_2           (1<<  0)
>> +#define AD9523_PLL1_OUTP_CTRL_OUTPUT_DIV_4           (2<<  0)
>> +#define AD9523_PLL1_OUTP_CTRL_OUTPUT_DIV_8           (4<<  0)
>> +#define AD9523_PLL1_OUTP_CTRL_OUTPUT_DIV_16          (8<<  0)
>> +
>> +/* AD9523_PLL1_OUTPUT_CHANNEL_CTRL */
>> +#define AD9523_PLL1_OUTP_CH_CTRL_OUTPUT_PWR_DOWN_EN  (1<<  7)
>> +#define AD9523_PLL1_OUTP_CH_CTRL_VCO_DIV_SEL_CH9_M2  (1<<  6)
>> +#define AD9523_PLL1_OUTP_CH_CTRL_VCO_DIV_SEL_CH8_M2  (1<<  5)
>> +#define AD9523_PLL1_OUTP_CH_CTRL_VCO_DIV_SEL_CH7_M2  (1<<  4)
>> +#define AD9523_PLL1_OUTP_CH_CTRL_VCXO_SRC_SEL_CH3    (1<<  3)
>> +#define AD9523_PLL1_OUTP_CH_CTRL_VCXO_SRC_SEL_CH2    (1<<  2)
>> +#define AD9523_PLL1_OUTP_CH_CTRL_VCXO_SRC_SEL_CH1    (1<<  1)
>> +#define AD9523_PLL1_OUTP_CH_CTRL_VCXO_SRC_SEL_CH0    (1<<  0)
>> +
>> +/* AD9523_READBACK_0 */
>> +#define AD9523_READBACK_0_STAT_PLL2_REF_CLK          (1<<  7)
>> +#define AD9523_READBACK_0_STAT_PLL2_FB_CLK           (1<<  6)
>> +#define AD9523_READBACK_0_STAT_VCXO                  (1<<  5)
>> +#define AD9523_READBACK_0_STAT_REF_TEST                      (1<<  4)
>> +#define AD9523_READBACK_0_STAT_REFB                  (1<<  3)
>> +#define AD9523_READBACK_0_STAT_REFA                  (1<<  2)
>> +#define AD9523_READBACK_0_STAT_PLL2_LD                       (1<<  1)
>> +#define AD9523_READBACK_0_STAT_PLL1_LD                       (1<<  0)
>> +
>> +/* AD9523_READBACK_1 */
>> +#define AD9523_READBACK_1_HOLDOVER_ACTIVE            (1<<  3)
>> +#define AD9523_READBACK_1_AUTOMODE_SEL_REFB          (1<<  2)
>> +#define AD9523_READBACK_1_VCO_CALIB_IN_PROGRESS              (1<<  0)
>> +
>> +/* AD9523_STATUS_SIGNALS */
>> +#define AD9523_STATUS_SIGNALS_SYNC_MAN_CTRL          (1<<  16)
>> +#define AD9523_STATUS_MONITOR_01_PLL12_LOCKED                (0x302)
>> +/* AD9523_POWER_DOWN_CTRL */
>> +#define AD9523_POWER_DOWN_CTRL_PLL1_PWR_DOWN         (1<<  2)
>> +#define AD9523_POWER_DOWN_CTRL_PLL2_PWR_DOWN         (1<<  1)
>> +#define AD9523_POWER_DOWN_CTRL_DIST_PWR_DOWN         (1<<  0)
>> +
>> +/* AD9523_IO_UPDATE */
>> +#define AD9523_IO_UPDATE_EN                          (1<<  0)
>> +
>> +/* AD9523_EEPROM_DATA_XFER_STATUS */
>> +#define AD9523_EEPROM_DATA_XFER_IN_PROGRESS          (1<<  0)
>> +
>> +/* AD9523_EEPROM_ERROR_READBACK */
>> +#define AD9523_EEPROM_ERROR_READBACK_FAIL            (1<<  0)
>> +
>> +/* AD9523_EEPROM_CTRL1 */
>> +#define AD9523_EEPROM_CTRL1_SOFT_EEPROM                      (1<<  1)
>> +#define AD9523_EEPROM_CTRL1_EEPROM_WRITE_PROT_DIS    (1<<  0)
>> +
>> +/* AD9523_EEPROM_CTRL2 */
>> +#define AD9523_EEPROM_CTRL2_REG2EEPROM                       (1<<  0)
>> +
>> +#define AD9523_NUM_CHAN                                      14
>> +#define AD9523_NUM_CHAN_ALT_CLK_SRC                  10
>> +
> Hmm. these are convenient but a little nasty.  Maybe a comment
> and defining the first in terms of the second would help?
Will do - I mainly introduced these to avoid excess line breaks.
>> +#define AD_IF(_pde, _a) ((pdata->_pde) ? _a : 0)
>> +#define AD_IFE(_pde, _a, _b) ((pdata->_pde) ? _a : _b)
>> +
>> +enum {
>> +     AD9523_STAT_PLL1_LD,
>> +     AD9523_STAT_PLL2_LD,
>> +     AD9523_STAT_REFA,
>> +     AD9523_STAT_REFB,
>> +     AD9523_STAT_REF_TEST,
>> +     AD9523_STAT_VCXO,
>> +     AD9523_STAT_PLL2_FB_CLK,
>> +     AD9523_STAT_PLL2_REF_CLK,
>> +     AD9523_SYNC,
>> +     AD9523_EEPROM,
>> +};
>> +
>> +enum {
>> +     AD9523_VCO1,
>> +     AD9523_VCO2,
>> +     AD9523_VCXO,
>> +     AD9523_NUM_CLK_SRC,
>> +};
>> +
>> +struct ad9523_state {
>> +     struct spi_device               *spi;
>> +     struct regulator                *reg;
>> +     struct ad9523_platform_data     *pdata;
>> +     struct iio_chan_spec            ad9523_channels[AD9523_NUM_CHAN];
>> +
>> +     unsigned long           vcxo_freq;
>> +     unsigned long           vco_freq;
>> +     unsigned long           vco_out_freq[AD9523_NUM_CLK_SRC];
>> +     unsigned char           vco_out_map[AD9523_NUM_CHAN_ALT_CLK_SRC];
>> +
>> +     /*
>> +      * DMA (thus cache coherency maintenance) requires the
>> +      * transfer buffers to live in their own cache lines.
>> +      */
>> +     union {
>> +             __be32 d32;
>> +             u8 d8[4];
>> +     } data[2] ____cacheline_aligned;
>> +};
>> +
>> +static int ad9523_read(struct iio_dev *indio_dev, unsigned addr)
>> +{
>> +     struct ad9523_state *st = iio_priv(indio_dev);
>> +     struct spi_message m;
>> +     int ret;
>> +     struct spi_transfer t[] = {
>> +             {
>> +                     .tx_buf =&st->data[0].d8[2],
>> +                     .len = 2,
> any reason not to drop this setting of cs_change to it's default?
leftover by some debug code.
I'll remove.
>> +                     .cs_change = 0,
>> +             }, {
> This address setting bit is a bit cunning. Perhaps a comment will make
> it more obvious what you are up to for a casual reader?
I'll add some comments.
>> +                     .rx_buf =&st->data[1].d8[4 - AD9523_TRANSF_LEN(addr)],
>> +                     .len = AD9523_TRANSF_LEN(addr),
>> +             },
>> +     };
>> +
>> +     spi_message_init(&m);
>> +     spi_message_add_tail(&t[0],&m);
>> +     spi_message_add_tail(&t[1],&m);
>> +
>> +     st->data[0].d32 = cpu_to_be32(AD9523_READ |
>> +                                   AD9523_CNT(AD9523_TRANSF_LEN(addr)) |
>> +                                   AD9523_ADDR(addr));
>> +
>> +     ret = spi_sync(st->spi,&m);
>> +     if (ret>= 0)
> spi_sync is defined to return zero on success. Is the>  necessary
> to avoid a false warning?
No. I fix it up.
>> +             ret = be32_to_cpu(st->data[1].d32)&  (0xFFFFFF>>
>> +                               (8 * (3 - AD9523_TRANSF_LEN(addr))));
>> +     else
>> +             dev_err(&indio_dev->dev, "read failed (%d)", ret);
>> +
>> +     return ret;
>> +};
>> +
>> +static int ad9523_write(struct iio_dev *indio_dev, unsigned addr, unsigned val)
>> +{
>> +     struct ad9523_state *st = iio_priv(indio_dev);
>> +     struct spi_message m;
>> +     int ret;
>> +     struct spi_transfer t[] = {
>> +             {
>> +                     .tx_buf =&st->data[0].d8[2],
>> +                     .len = 2,
> again, why set to default.
>> +                     .cs_change = 0,
>> +             }, {
>> +                     .tx_buf =&st->data[1].d8[4 - AD9523_TRANSF_LEN(addr)],
>> +                     .len = AD9523_TRANSF_LEN(addr),
>> +             },
>> +     };
>> +
>> +     spi_message_init(&m);
>> +     spi_message_add_tail(&t[0],&m);
>> +     spi_message_add_tail(&t[1],&m);
>> +
>> +     st->data[0].d32 = cpu_to_be32(AD9523_WRITE |
>> +                                   AD9523_CNT(AD9523_TRANSF_LEN(addr)) |
>> +                                   AD9523_ADDR(addr));
>> +     st->data[1].d32 = cpu_to_be32(val);
>> +
>> +     ret = spi_sync(st->spi,&m);
>> +
>> +     if (ret<  0)
>> +             dev_err(&indio_dev->dev, "write failed (%d)", ret);
>> +
>> +     return ret;
>> +}
>> +
>> +static int ad9523_io_update(struct iio_dev *indio_dev)
>> +{
>> +     return ad9523_write(indio_dev, AD9523_IO_UPDATE, AD9523_IO_UPDATE_EN);
>> +}
>> +
>> +static int ad9523_vco_out_map(struct iio_dev *indio_dev,
>> +                           unsigned ch, bool out)
>> +{
>> +     struct ad9523_state *st = iio_priv(indio_dev);
>> +     int ret;
>> +     unsigned mask;
>> +
>> +     switch (ch) {
>> +     case 0 ... 3:
>> +             ret = ad9523_read(indio_dev, AD9523_PLL1_OUTPUT_CHANNEL_CTRL);
>> +             if (ret<  0)
>> +                     break;
>> +             mask = AD9523_PLL1_OUTP_CH_CTRL_VCXO_SRC_SEL_CH0<<  ch;
>> +             if (out) {
>> +                     ret |= mask;
>> +                     out = 2;
>> +             } else {
>> +                     ret&= ~mask;
>> +             }
>> +             ret = ad9523_write(indio_dev,
>> +                                AD9523_PLL1_OUTPUT_CHANNEL_CTRL, ret);
>> +             break;
>> +     case 4 ... 6:
>> +             ret = ad9523_read(indio_dev, AD9523_PLL1_OUTPUT_CTRL);
>> +             if (ret<  0)
>> +                     break;
>> +             mask = AD9523_PLL1_OUTP_CTRL_VCO_DIV_SEL_CH4_M2<<  (ch - 4);
>> +             if (out)
>> +                     ret |= mask;
>> +             else
>> +                     ret&= ~mask;
>> +             ret = ad9523_write(indio_dev, AD9523_PLL1_OUTPUT_CTRL, ret);
>> +             break;
>> +     case 7 ... 9:
>> +             ret = ad9523_read(indio_dev, AD9523_PLL1_OUTPUT_CHANNEL_CTRL);
>> +             if (ret<  0)
>> +                     break;
>> +             mask = AD9523_PLL1_OUTP_CH_CTRL_VCO_DIV_SEL_CH7_M2<<  (ch - 7);
>> +             if (out)
>> +                     ret |= mask;
>> +             else
>> +                     ret&= ~mask;
>> +             ret = ad9523_write(indio_dev,
>> +                                AD9523_PLL1_OUTPUT_CHANNEL_CTRL, ret);
>> +             break;
>> +     default:
>> +             return 0;
>> +     }
>> +
>> +     st->vco_out_map[ch] = out;
> blank line please
>> +     return ret;
>> +}
>> +
>> +static int ad9523_set_clock_provider(struct iio_dev *indio_dev,
>> +                           unsigned ch, unsigned long freq)
>> +{
>> +     struct ad9523_state *st = iio_priv(indio_dev);
>> +     long tmp1, tmp2;
>> +     bool use_alt_clk_src;
>> +
>> +     switch (ch) {
>> +     case 0 ... 3:
>> +             if (freq == st->vco_out_freq[AD9523_VCXO])
>> +                     use_alt_clk_src = true;
>> +             else
>> +                     use_alt_clk_src = false;
>> +
>> +             break;
>> +     case 4 ... 9:
>> +             tmp1 = st->vco_out_freq[AD9523_VCO1] / freq;
>> +             tmp2 = st->vco_out_freq[AD9523_VCO2] / freq;
>> +             tmp1 *= freq;
>> +             tmp2 *= freq;
>> +             if (abs(tmp1 - freq)>  abs(tmp2 - freq))
> do this directly?
>
> use_alt_clk_src = (abs(tmp1 - freq)>  abs(tmp2 - freq));
Right...
>> +                     use_alt_clk_src = true;
>> +             else
>> +                     use_alt_clk_src = false;
>> +             break;
>> +     default:
> Is this a valid route?  If not I'd just use einval to make it
> clear to readers that this shouldn't happen.
10..14 is nothing to do, thus it's a valid route and we return 0,
instead of taking actions.

>> +             return 0;
>> +     }
>> +
>> +     return ad9523_vco_out_map(indio_dev, ch, use_alt_clk_src);
>> +}
>> +
>> +static ssize_t ad9523_store(struct device *dev,
>> +                             struct device_attribute *attr,
>> +                             const char *buf, size_t len)
>> +{
>> +     struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +     struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +     unsigned long readin, tmp;
>> +     int ret;
>> +
>> +     ret = kstrtoul(buf, 10,&readin);
>> +     if (ret)
>> +             return ret;
>> +
>> +     mutex_lock(&indio_dev->mlock);
>> +     switch ((u32)this_attr->address) {
>> +     case AD9523_SYNC:
> I'd break these two cases out into separate functions.  This one
> ends up looking ugglier than it really is!
Ok
>> +             ret = ad9523_read(indio_dev, AD9523_STATUS_SIGNALS);
>> +             if (ret<  0)
>> +                     goto out;
>> +             tmp = ret;
>> +             tmp |= AD9523_STATUS_SIGNALS_SYNC_MAN_CTRL;
>> +             ret = ad9523_write(indio_dev, AD9523_STATUS_SIGNALS, tmp);
>> +             if (ret<  0)
>> +                     goto out;
>> +             ad9523_io_update(indio_dev);
>> +             tmp&= ~AD9523_STATUS_SIGNALS_SYNC_MAN_CTRL;
>> +             ret = ad9523_write(indio_dev, AD9523_STATUS_SIGNALS, tmp);
>> +             if (ret<  0)
>> +                     goto out;
>> +             break;
>> +     case AD9523_EEPROM:
>> +             if (readin != 1) {
>> +                     ret = -EINVAL;
>> +                     goto out;
>> +             }
>> +             ret = ad9523_write(indio_dev, AD9523_EEPROM_CTRL1,
>> +                                AD9523_EEPROM_CTRL1_EEPROM_WRITE_PROT_DIS);
>> +             if (ret<  0)
>> +                     goto out;
>> +             ret = ad9523_write(indio_dev, AD9523_EEPROM_CTRL2,
>> +                                AD9523_EEPROM_CTRL2_REG2EEPROM);
>> +             if (ret<  0)
>> +                     goto out;
>> +
>> +             tmp = 4;
>> +             do {
>> +                     msleep(16);
>> +                     ret = ad9523_read(indio_dev,
>> +                                       AD9523_EEPROM_DATA_XFER_STATUS);
>> +                     if (ret<  0)
>> +                             goto out;
>> +             } while ((ret&  AD9523_EEPROM_DATA_XFER_IN_PROGRESS)&&  tmp--);
>> +
>> +             ret = ad9523_write(indio_dev, AD9523_EEPROM_CTRL1, 0);
>> +             if (ret<  0)
>> +                     goto out;
>> +
>> +             ret = ad9523_read(indio_dev, AD9523_EEPROM_ERROR_READBACK);
>> +             if (ret<  0)
>> +                     goto out;
>> +
>> +             if (ret&  AD9523_EEPROM_ERROR_READBACK_FAIL) {
>> +                     dev_err(&indio_dev->dev, "Verify EEPROM failed");
>> +                     ret = -EIO;
>> +             }
>> +             break;
>> +     default:
>> +             ret = -ENODEV;
>> +             goto out;
>> +     }
>> +
>> +     ret = ad9523_io_update(indio_dev);
>> +out:
>> +     mutex_unlock(&indio_dev->mlock);
>> +
>> +     return ret ? ret : len;
>> +}
>> +
>> +static ssize_t ad9523_show(struct device *dev,
>> +                     struct device_attribute *attr,
>> +                     char *buf)
>> +{
>> +     struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +     struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +     int ret = 0;
>> +
>> +     mutex_lock(&indio_dev->mlock);
>> +     ret = ad9523_read(indio_dev, AD9523_READBACK_0);
>> +     if (ret>= 0) {
>> +             ret = sprintf(buf, "%s\n", (ret&  (1<<
>> +                     (u32)this_attr->address)) ? "OK" : "FAIL");
>> +     }
>> +     mutex_unlock(&indio_dev->mlock);
>> +
>> +     return ret;
>> +}
>> +
>> +static IIO_DEVICE_ATTR(status_pll1_lock_detect, S_IRUGO,
>> +                     ad9523_show,
>> +                     NULL,
>> +                     AD9523_STAT_PLL1_LD);
>> +
>> +static IIO_DEVICE_ATTR(status_pll2_lock_detect, S_IRUGO,
>> +                     ad9523_show,
>> +                     NULL,
>> +                     AD9523_STAT_PLL2_LD);
>> +
>> +static IIO_DEVICE_ATTR(status_reference_a, S_IRUGO,
>> +                     ad9523_show,
>> +                     NULL,
>> +                     AD9523_STAT_REFA);
>> +
>> +static IIO_DEVICE_ATTR(status_reference_b, S_IRUGO,
>> +                     ad9523_show,
>> +                     NULL,
>> +                     AD9523_STAT_REFB);
>> +
>> +static IIO_DEVICE_ATTR(status_reference_test, S_IRUGO,
>> +                     ad9523_show,
>> +                     NULL,
>> +                     AD9523_STAT_REF_TEST);
>> +
>> +static IIO_DEVICE_ATTR(status_vcxo, S_IRUGO,
>> +                     ad9523_show,
>> +                     NULL,
>> +                     AD9523_STAT_VCXO);
>> +
>> +static IIO_DEVICE_ATTR(status_pll2_feedback_clock, S_IRUGO,
>> +                     ad9523_show,
>> +                     NULL,
>> +                     AD9523_STAT_PLL2_FB_CLK);
>> +
>> +static IIO_DEVICE_ATTR(status_pll2_reference_clock, S_IRUGO,
>> +                     ad9523_show,
>> +                     NULL,
>> +                     AD9523_STAT_PLL2_REF_CLK);
>> +
>> +static IIO_DEVICE_ATTR(sync_dividers, S_IWUSR,
>> +                     NULL,
>> +                     ad9523_store,
>> +                     AD9523_SYNC);
>> +
>> +static IIO_DEVICE_ATTR(store_eeprom, S_IWUSR,
>> +                     NULL,
>> +                     ad9523_store,
>> +                     AD9523_EEPROM);
>> +
>> +static struct attribute *ad9523_attributes[] = {
>> +&iio_dev_attr_sync_dividers.dev_attr.attr,
>> +&iio_dev_attr_store_eeprom.dev_attr.attr,
>> +&iio_dev_attr_status_pll1_lock_detect.dev_attr.attr,
>> +&iio_dev_attr_status_pll2_lock_detect.dev_attr.attr,
>> +&iio_dev_attr_status_reference_a.dev_attr.attr,
>> +&iio_dev_attr_status_reference_b.dev_attr.attr,
>> +&iio_dev_attr_status_reference_test.dev_attr.attr,
>> +&iio_dev_attr_status_vcxo.dev_attr.attr,
>> +&iio_dev_attr_status_pll2_feedback_clock.dev_attr.attr,
>> +&iio_dev_attr_status_pll2_reference_clock.dev_attr.attr,
>> +     NULL,
>> +};
>> +
>> +static const struct attribute_group ad9523_attribute_group = {
>> +     .attrs = ad9523_attributes,
>> +};
>> +
>> +static int ad9523_read_raw(struct iio_dev *indio_dev,
>> +                        struct iio_chan_spec const *chan,
>> +                        int *val,
>> +                        int *val2,
>> +                        long m)
>> +{
>> +     struct ad9523_state *st = iio_priv(indio_dev);
>> +     unsigned code;
>> +     int ret;
>> +
>> +     mutex_lock(&indio_dev->mlock);
>> +     ret = ad9523_read(indio_dev, AD9523_CHANNEL_CLOCK_DIST(chan->channel));
>> +     mutex_unlock(&indio_dev->mlock);
>> +
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     switch (m) {
>> +     case IIO_CHAN_INFO_RAW:
>> +             *val = !(ret&  AD9523_CLK_DIST_PWR_DOWN_EN);
>> +             return IIO_VAL_INT;
>> +     case IIO_CHAN_INFO_FREQUENCY:
>> +             *val = st->vco_out_freq[st->vco_out_map[chan->channel]] /
>> +                     AD9523_CLK_DIST_DIV_REV(ret);
>> +             return IIO_VAL_INT;
>> +     case IIO_CHAN_INFO_PHASE:
>> +             code = (AD9523_CLK_DIST_DIV_PHASE_REV(ret) * 3141592) /
>> +                     AD9523_CLK_DIST_DIV_REV(ret);
>> +             *val = code / 1000000;
>> +             *val2 = (code % 1000000) * 10;
>> +             return IIO_VAL_INT_PLUS_MICRO;
>> +     default:
>> +             return -EINVAL;
>> +     }
>> +};
>> +
>> +static int ad9523_write_raw(struct iio_dev *indio_dev,
>> +                         struct iio_chan_spec const *chan,
>> +                         int val,
>> +                         int val2,
>> +                         long mask)
>> +{
>> +     struct ad9523_state *st = iio_priv(indio_dev);
>> +     unsigned reg;
>> +     int ret, tmp, code;
>> +
>> +     mutex_lock(&indio_dev->mlock);
>> +     ret = ad9523_read(indio_dev, AD9523_CHANNEL_CLOCK_DIST(chan->channel));
>> +     if (ret<  0)
>> +             goto out;
>> +
>> +     reg = ret;
>> +
>> +     switch (mask) {
>> +     case IIO_CHAN_INFO_RAW:
>> +             if (val)
>> +                     reg&= ~AD9523_CLK_DIST_PWR_DOWN_EN;
>> +             else
>> +                     reg |= AD9523_CLK_DIST_PWR_DOWN_EN;
>> +             break;
>> +     case IIO_CHAN_INFO_FREQUENCY:
>> +             ret = ad9523_set_clock_provider(indio_dev, chan->channel, val);
>> +             if (ret<  0)
>> +                     goto out;
>> +             tmp = st->vco_out_freq[st->vco_out_map[chan->channel]] / val;
>> +             tmp = clamp(tmp, 1, 1024);
>> +             reg&= ~(0x3FF<<  8);
>> +             reg |= AD9523_CLK_DIST_DIV(tmp);
>> +             break;
>> +     case IIO_CHAN_INFO_PHASE:
>> +             code = val * 1000000 + val2 % 1000000;
>> +             tmp = (code * AD9523_CLK_DIST_DIV_REV(ret)) / 3141592;
>> +             tmp = clamp(tmp, 0, 63);
>> +             reg&= ~AD9523_CLK_DIST_DIV_PHASE(~0);
>> +             reg |= AD9523_CLK_DIST_DIV_PHASE(tmp);
>> +             break;
>> +     default:
>> +             ret = -EINVAL;
>> +             goto out;
>> +     }
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_CHANNEL_CLOCK_DIST(chan->channel),
>> +                        reg);
>> +     if (ret<  0)
>> +             goto out;
>> +
>> +     ad9523_io_update(indio_dev);
>> +out:
>> +     mutex_unlock(&indio_dev->mlock);
>> +     return ret;
>> +}
>> +
>> +static int ad9523_reg_access(struct iio_dev *indio_dev,
>> +                           unsigned reg, unsigned writeval,
>> +                           unsigned *readval)
>> +{
>> +     int ret;
>> +
>> +     mutex_lock(&indio_dev->mlock);
>> +     if (readval == NULL) {
>> +             ret = ad9523_write(indio_dev, reg | AD9523_R1B, writeval);
>> +             ad9523_io_update(indio_dev);
>> +     } else {
>> +             ret = ad9523_read(indio_dev, reg | AD9523_R1B);
>> +             if (ret<  0)
>> +                     return ret;
>> +             *readval = ret;
>> +             ret = 0;
>> +     }
>> +     mutex_unlock(&indio_dev->mlock);
>> +
>> +     return ret;
>> +}
>> +
>> +static const struct iio_info ad9523_info = {
>> +     .read_raw =&ad9523_read_raw,
>> +     .write_raw =&ad9523_write_raw,
>> +     .debugfs_reg_access =&ad9523_reg_access,
>> +     .attrs =&ad9523_attribute_group,
>> +     .driver_module = THIS_MODULE,
>> +};
>> +
>> +static int ad9523_setup(struct iio_dev *indio_dev)
>> +{
>> +     struct ad9523_state *st = iio_priv(indio_dev);
>> +     struct ad9523_platform_data *pdata = st->pdata;
>> +     struct ad9523_channel_spec      *chan;
> odd spacing above.
>> +     unsigned active_mask = 0;
>> +     int ret, i;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_SERIAL_PORT_CONFIG,
>> +                        AD9523_SER_CONF_SOFT_RESET |
>> +                       (st->spi->mode&  SPI_3WIRE ? 0 :
>> +                       AD9523_SER_CONF_SDO_ACTIVE));
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_READBACK_CTRL,
>> +                       AD9523_READBACK_CTRL_READ_BUFFERED);
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_io_update(indio_dev);
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     /*
>> +      * PLL1 Setup
>> +      */
>> +     ret = ad9523_write(indio_dev, AD9523_PLL1_REF_A_DIVIDER,
>> +             pdata->refa_r_div);
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL1_REF_B_DIVIDER,
>> +             pdata->refb_r_div);
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL1_FEEDBACK_DIVIDER,
>> +             pdata->pll1_feedback_div);
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL1_CHARGE_PUMP_CTRL,
>> +             AD9523_PLL1_CHARGE_PUMP_CURRENT_nA(pdata->
>> +                     pll1_charge_pump_current_nA) |
>> +             AD9523_PLL1_CHARGE_PUMP_MODE_NORMAL |
>> +             AD9523_PLL1_BACKLASH_PW_MIN);
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL1_INPUT_RECEIVERS_CTRL,
>> +             AD_IF(refa_diff_rcv_en, AD9523_PLL1_REFA_RCV_EN) |
>> +             AD_IF(refb_diff_rcv_en, AD9523_PLL1_REFB_RCV_EN) |
>> +             AD_IF(osc_in_diff_en, AD9523_PLL1_OSC_IN_DIFF_EN) |
>> +             AD_IF(osc_in_cmos_neg_inp_en,
>> +                   AD9523_PLL1_OSC_IN_CMOS_NEG_INP_EN) |
>> +             AD_IF(refa_diff_rcv_en, AD9523_PLL1_REFA_DIFF_RCV_EN) |
>> +             AD_IF(refb_diff_rcv_en, AD9523_PLL1_REFB_DIFF_RCV_EN));
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +
> excess blank lines.
>> +     ret = ad9523_write(indio_dev, AD9523_PLL1_REF_CTRL,
>> +             AD_IF(zd_in_diff_en, AD9523_PLL1_ZD_IN_DIFF_EN) |
>> +             AD_IF(zd_in_cmos_neg_inp_en,
>> +                   AD9523_PLL1_ZD_IN_CMOS_NEG_INP_EN) |
>> +             AD_IF(zero_delay_mode_internal_en,
>> +                   AD9523_PLL1_ZERO_DELAY_MODE_INT) |
>> +             AD_IF(osc_in_feedback_en, AD9523_PLL1_OSC_IN_PLL_FEEDBACK_EN) |
>> +             AD_IF(refa_cmos_neg_inp_en, AD9523_PLL1_REFA_CMOS_NEG_INP_EN) |
>> +             AD_IF(refb_cmos_neg_inp_en, AD9523_PLL1_REFB_CMOS_NEG_INP_EN));
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL1_MISC_CTRL,
>> +             AD9523_PLL1_REFB_INDEP_DIV_CTRL_EN |
>> +             AD9523_PLL1_REF_MODE(pdata->ref_mode));
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL1_LOOP_FILTER_CTRL,
>> +             AD9523_PLL1_LOOP_FILTER_RZERO(pdata->pll1_loop_filter_rzero));
>> +     if (ret<  0)
>> +             return ret;
>> +     /*
>> +      * PLL2 Setup
>> +      */
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL2_CHARGE_PUMP,
>> +             AD9523_PLL2_CHARGE_PUMP_CURRENT_nA(pdata->
>> +                     pll2_charge_pump_current_nA));
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL2_FEEDBACK_DIVIDER_AB,
>> +             AD9523_PLL2_FB_NDIV_A_CNT(pdata->pll2_ndiv_a_cnt) |
>> +             AD9523_PLL2_FB_NDIV_B_CNT(pdata->pll2_ndiv_b_cnt));
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL2_CTRL,
>> +             AD9523_PLL2_CHARGE_PUMP_MODE_NORMAL |
>> +             AD9523_PLL2_BACKLASH_CTRL_EN |
>> +             AD_IF(pll2_freq_doubler_en, AD9523_PLL2_FREQ_DOUBLER_EN));
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     st->vco_freq = (pdata->vcxo_freq * (pdata->pll2_freq_doubler_en ? 2 : 1)
>> +                     / pdata->pll2_r2_div) * AD9523_PLL2_FB_NDIV(pdata->
>> +                     pll2_ndiv_a_cnt, pdata->pll2_ndiv_b_cnt);
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL2_VCO_CTRL,
>> +             AD9523_PLL2_VCO_CALIBRATE);
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL2_VCO_DIVIDER,
>> +             AD9523_PLL2_VCO_DIV_M1(pdata->pll2_vco_diff_m1) |
>> +             AD9523_PLL2_VCO_DIV_M2(pdata->pll2_vco_diff_m2) |
>> +             AD_IFE(pll2_vco_diff_m1, 0,
>> +                    AD9523_PLL2_VCO_DIV_M1_PWR_DOWN_EN) |
>> +             AD_IFE(pll2_vco_diff_m2, 0,
>> +                    AD9523_PLL2_VCO_DIV_M2_PWR_DOWN_EN));
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     if (pdata->pll2_vco_diff_m1)
>> +             st->vco_out_freq[AD9523_VCO1] =
>> +                     st->vco_freq / pdata->pll2_vco_diff_m1;
>> +
>> +     if (pdata->pll2_vco_diff_m2)
>> +             st->vco_out_freq[AD9523_VCO2] =
>> +                     st->vco_freq / pdata->pll2_vco_diff_m2;
>> +
>> +     st->vco_out_freq[AD9523_VCXO] = pdata->vcxo_freq;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL2_R2_DIVIDER,
>> +             AD9523_PLL2_R2_DIVIDER_VAL(pdata->pll2_r2_div));
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_PLL2_LOOP_FILTER_CTRL,
>> +             AD9523_PLL2_LOOP_FILTER_CPOLE1(pdata->cpole1) |
>> +             AD9523_PLL2_LOOP_FILTER_RZERO(pdata->rzero) |
>> +             AD9523_PLL2_LOOP_FILTER_RPOLE2(pdata->rpole2) |
>> +             AD_IF(rzero_bypass_en,
>> +                   AD9523_PLL2_LOOP_FILTER_RZERO_BYPASS_EN));
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     for (i = 0; i<  pdata->num_channels; i++) {
>> +             chan =&pdata->channels[i];
>> +             if (chan->channel_num<  AD9523_NUM_CHAN) {
>> +                     active_mask |= (1<<  chan->channel_num);
>> +                     ret = ad9523_write(indio_dev,
>> +                             AD9523_CHANNEL_CLOCK_DIST(chan->channel_num),
>> +                             AD9523_CLK_DIST_DRIVER_MODE(chan->driver_mode) |
>> +                             AD9523_CLK_DIST_DIV(chan->channel_divider) |
>> +                             AD9523_CLK_DIST_DIV_PHASE(chan->divider_phase) |
>> +                             (chan->sync_ignore_en ?
>> +                                     AD9523_CLK_DIST_IGNORE_SYNC_EN : 0) |
>> +                             (chan->divider_output_invert_en ?
>> +                                     AD9523_CLK_DIST_INV_DIV_OUTPUT_EN : 0) |
>> +                             (chan->low_power_mode_en ?
>> +                                     AD9523_CLK_DIST_LOW_PWR_MODE_EN : 0) |
>> +                             (chan->output_dis ?
>> +                                     AD9523_CLK_DIST_PWR_DOWN_EN : 0));
>> +                     if (ret<  0)
>> +                             return ret;
>> +
>> +                     ret = ad9523_vco_out_map(indio_dev, chan->channel_num,
>> +                                        chan->use_alt_clock_src);
>> +                     if (ret<  0)
>> +                             return ret;
>> +
> Hmm. by making this all dynamic we can't have a nice clean static array
> to define things.  Please explain why we need to be able to mask
> channels in the board config for this device?
> I'm not saying it isn't valid, merely that you haven't explained why!
See my comment above. Parts of the channel configuration
belong to platform data. I don't have problems exposing unused ADC inputs,
however exposing controls for something that could cause unwanted radiations
is a different story. On purpose we explicitly disable all channels that 
are not
in the pdata channel array.
If a channel is on purpose configured, it is assumed that it has proper
termination, and that traces are routed in a fashion that unwanted radiation
is not going to be a problem. (FCC, CE, etc. qualification)

>
>> +                     st->ad9523_channels[i].type = IIO_ALTVOLTAGE;
>> +                     st->ad9523_channels[i].output = 1;
>> +                     st->ad9523_channels[i].indexed = 1;
>> +                     st->ad9523_channels[i].channel = chan->channel_num;
>> +                     st->ad9523_channels[i].extend_name =
>> +                             chan->extended_name;
>> +                     st->ad9523_channels[i].info_mask =
>> +                             IIO_CHAN_INFO_RAW |
>> +                             IIO_CHAN_INFO_PHASE_SEPARATE_BIT |
>> +                             IIO_CHAN_INFO_FREQUENCY_SEPARATE_BIT;
>> +             }
>> +     }
>> +
>> +     for (i = 0; i<  AD9523_NUM_CHAN; i++)
>> +             if (!(active_mask&  (1<<  i)))
>> +                     ad9523_write(indio_dev,
>> +                                  AD9523_CHANNEL_CLOCK_DIST(i),
>> +                                  AD9523_CLK_DIST_DRIVER_MODE(TRISTATE) |
>> +                                  AD9523_CLK_DIST_PWR_DOWN_EN);
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_POWER_DOWN_CTRL, 0);
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_write(indio_dev, AD9523_STATUS_SIGNALS,
>> +                        AD9523_STATUS_MONITOR_01_PLL12_LOCKED);
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     ret = ad9523_io_update(indio_dev);
>> +     if (ret<  0)
>> +             return ret;
>> +
>> +     return 0;
>> +}
>> +
>> +static int __devinit ad9523_probe(struct spi_device *spi)
>> +{
>> +     struct ad9523_platform_data *pdata = spi->dev.platform_data;
>> +     struct iio_dev *indio_dev;
>> +     struct ad9523_state *st;
>> +     int ret;
>> +
>> +     if (!pdata) {
>> +             dev_err(&spi->dev, "no platform data?\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     indio_dev = iio_device_alloc(sizeof(*st));
>> +     if (indio_dev == NULL)
>> +             return -ENOMEM;
>> +
>> +     st = iio_priv(indio_dev);
>> +
>> +     st->reg = regulator_get(&spi->dev, "vcc");
>> +     if (!IS_ERR(st->reg)) {
>> +             ret = regulator_enable(st->reg);
>> +             if (ret)
>> +                     goto error_put_reg;
>> +     }
>> +
>> +     spi_set_drvdata(spi, indio_dev);
>> +     st->spi = spi;
>> +     st->pdata = pdata;
>> +
>> +     indio_dev->dev.parent =&spi->dev;
>> +     indio_dev->name = (pdata->name[0] != 0) ? pdata->name :
>> +                       spi_get_device_id(spi)->name;
>> +     indio_dev->info =&ad9523_info;
>> +     indio_dev->modes = INDIO_DIRECT_MODE;
>> +     indio_dev->channels = st->ad9523_channels;
>> +     indio_dev->num_channels = pdata->num_channels;
>> +
>> +     ret = ad9523_setup(indio_dev);
>> +     if (ret<  0)
>> +             goto error_disable_reg;
>> +
>> +     ret = iio_device_register(indio_dev);
>> +     if (ret)
>> +             goto error_disable_reg;
>> +
>> +     dev_info(&spi->dev, "probed %s\n", indio_dev->name);
>> +
>> +     return 0;
>> +
>> +error_disable_reg:
>> +     if (!IS_ERR(st->reg))
>> +             regulator_disable(st->reg);
>> +error_put_reg:
>> +     if (!IS_ERR(st->reg))
>> +             regulator_put(st->reg);
>> +
>> +     iio_device_free(indio_dev);
>> +
>> +     return ret;
>> +}
>> +
>> +static int __devexit ad9523_remove(struct spi_device *spi)
>> +{
>> +     struct iio_dev *indio_dev = spi_get_drvdata(spi);
>> +     struct ad9523_state *st = iio_priv(indio_dev);
>> +     struct regulator *reg = st->reg;
>> +
>> +     iio_device_unregister(indio_dev);
>> +
>> +     if (!IS_ERR(reg)) {
>> +             regulator_disable(reg);
>> +             regulator_put(reg);
>> +     }
>> +
>> +     iio_device_free(indio_dev);
>> +
>> +     return 0;
>> +}
>> +
>> +static const struct spi_device_id ad9523_id[] = {
>> +     {"ad9523", 9523},
>> +     {}
>> +};
>> +MODULE_DEVICE_TABLE(spi, ad9523_id);
>> +
>> +static struct spi_driver ad9523_driver = {
>> +     .driver = {
>> +             .name   = "ad9523",
>> +             .owner  = THIS_MODULE,
>> +     },
>> +     .probe          = ad9523_probe,
>> +     .remove         = __devexit_p(ad9523_remove),
>> +     .id_table       = ad9523_id,
>> +};
>> +module_spi_driver(ad9523_driver);
>> +
>> +MODULE_AUTHOR("Michael Hennerich<hennerich@blackfin.uclinux.org>");
>> +MODULE_DESCRIPTION("Analog Devices AD9523 CLOCKDIST/PLL");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/staging/iio/frequency/ad9523.h b/drivers/staging/iio/frequency/ad9523.h
>> new file mode 100644
>> index 0000000..17bba03
>> --- /dev/null
>> +++ b/drivers/staging/iio/frequency/ad9523.h
>> @@ -0,0 +1,112 @@
>> +/*
>> + * AD9523 SPI Low Jitter Clock Generator
>> + *
>> + * Copyright 2012 Analog Devices Inc.
>> + *
>> + * Licensed under the GPL-2.
>> + */
>> +
>> +#ifndef IIO_PLL_AD9523_H_
>> +#define IIO_PLL_AD9523_H_
>> +
>> +/*
>> + * TODO: move to include/linux/iio
>> + */
>> +
>> +enum outp_drv_mode {
>> +     TRISTATE,
>> +     LVPECL_8mA,
>> +     LVDS_4mA,
>> +     LVDS_7mA,
>> +     HSTL0_16mA,
>> +     HSTL1_8mA,
>> +     CMOS_CONF1,
>> +     CMOS_CONF2,
>> +     CMOS_CONF3,
>> +     CMOS_CONF4,
>> +     CMOS_CONF5,
>> +     CMOS_CONF6,
>> +     CMOS_CONF7,
>> +     CMOS_CONF8,
>> +     CMOS_CONF9
>> +};
>> +
>> +enum ref_sel_mode {
>> +     NONEREVERTIVE_STAY_ON_REFB,
>> +     REVERT_TO_REFA,
>> +     SELECT_REFA,
>> +     SELECT_REFB,
>> +     EXT_REF_SEL
>> +};
>> +
> I'd like to see this documented.
ok
>> +struct ad9523_channel_spec {
>> +     unsigned                channel_num;
>> +     bool                    divider_output_invert_en;
>> +     bool                    sync_ignore_en;
>> +     bool                    low_power_mode_en;
>> +                              /* CH0..CH3 VCXO, CH4..CH9 VCO2 */
>> +     bool                    use_alt_clock_src;
>> +     bool                    output_dis;
>> +     enum outp_drv_mode      driver_mode;
>> +     unsigned char           divider_phase;
>> +     unsigned short          channel_divider;
>> +     char                    extended_name[16];
>> +};
>> +
>> +/*
>> + * struct ad9523_platform_data - platform specific information
>> + */
>> +
>> +struct ad9523_platform_data {
>> +     unsigned long vcxo_freq;
>> +
>> +     /* Differential/ Single-Ended Input Configuration */
>> +     bool            refa_diff_rcv_en;
>> +     bool            refb_diff_rcv_en;
>> +     bool            zd_in_diff_en;
>> +     bool            osc_in_diff_en;
>> +
>> +     /*
>> +      * Valid if differential input disabled
>> +      * if not true defaults to pos input
>> +      */
>> +     bool            refa_cmos_neg_inp_en;
>> +     bool            refb_cmos_neg_inp_en;
>> +     bool            zd_in_cmos_neg_inp_en;
>> +     bool            osc_in_cmos_neg_inp_en;
>> +
>> +     /* PLL1 Setting */
>> +     unsigned short  refa_r_div;
>> +     unsigned short  refb_r_div;
>> +     unsigned short  pll1_feedback_div;
>> +     unsigned short  pll1_charge_pump_current_nA;
>> +     bool            zero_delay_mode_internal_en;
>> +     bool            osc_in_feedback_en;
>> +     unsigned char   pll1_loop_filter_rzero;
>> +
>> +     /* Reference */
>> +     enum ref_sel_mode       ref_mode;
>> +
>> +     /* PLL2 Setting */
>> +     unsigned int    pll2_charge_pump_current_nA;
>> +     unsigned char   pll2_ndiv_a_cnt;
>> +     unsigned char   pll2_ndiv_b_cnt;
>> +     bool            pll2_freq_doubler_en;
>> +     unsigned char   pll2_r2_div;
>> +     unsigned char   pll2_vco_diff_m1; /* 3..5 */
>> +     unsigned char   pll2_vco_diff_m2; /* 3..5 */
>> +
>> +     /* Loop Filter PLL2 */
>> +     unsigned char   rpole2;
>> +     unsigned char   rzero;
>> +     unsigned char   cpole1;
>> +     bool            rzero_bypass_en;
>> +
>> +     /* Output Channel Configuration */
>> +     int             num_channels;
>> +     struct ad9523_channel_spec      *channels;
>> +
>> +     char            name[SPI_NAME_SIZE];
>> +};
>> +
>> +#endif /* IIO_PLL_AD9523_H_ */
> --
> 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
>


-- 
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