From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from tx2ehsobe005.messaging.microsoft.com ([65.55.88.15]:27521 "EHLO tx2outboundpool.messaging.microsoft.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756722Ab2ENOJv (ORCPT ); Mon, 14 May 2012 10:09:51 -0400 Message-ID: <4FB11229.3010105@analog.com> Date: Mon, 14 May 2012 16:09:45 +0200 From: Michael Hennerich Reply-To: MIME-Version: 1.0 To: Jonathan Cameron CC: "linux-iio@vger.kernel.org" , "device-drivers-devel@blackfin.uclinux.org" Subject: Re: [PATCH 3/3] iio: frequency: New driver for AD9523 SPI Low Jitter Clock Generator References: <1336138266-25480-1-git-send-email-michael.hennerich@analog.com> <1336138266-25480-3-git-send-email-michael.hennerich@analog.com> <4FAEA998.5070200@kernel.org> In-Reply-To: <4FAEA998.5070200@kernel.org> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Sender: linux-iio-owner@vger.kernel.org List-Id: linux-iio@vger.kernel.org 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 >> > 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 >> --- >> .../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 >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> +#include >> + >> +#include >> +#include >> + >> +#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"); >> +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