[PATCH v3 0/3] *** iio:adc:imx: Add Freescale vf610 ADC driver support ***

[PATCH v3 0/3] *** iio:adc:imx: Add Freescale vf610 ADC driver support ***

[Fugang Duan]

The patch set is to enable Vybrid VF610 ADC driver.

Please see the Reference Manual of the ADC:
http://cache.freescale.com/files/32bit/doc/ref_manual/VYBRIDRM.pdf?fpsp=1&WT_TYPE=
Reference%20Manuals&WT_VENDOR=FREESCALE&WT_FILE_FORMAT=pdf&WT_ASSET=Documentation

*** Change ***
* V3: (review by Shawn Guo)
- DTS properties: use prefix "fsl," instead of "vf610".
- Drop DTS unnessary comments, and add "vref" optional propert comments.
- Modify driver format.
- Remove driver debug code.
- Remove the of_device_id .data init.
- Remove unnecessary device node check.
- Add clk_prepare_enable() return check.

* V2: (Simply review by Jonathan Cameron)
- Rename the driver after "vf610_adc".

* V1:
Feature:
- Enable Vybrid vf610 board ADC0 in dts file.
- ADC driver only support software trigger for the init version.
- ADC configuration support dts set.
Note: Since the ADC IP is used for Freescale Vybrid VF610, i.MX6SLX, i.MX7 serial sillicons.

Fugang Duan (3):
  ARM: dts: vf610-twr: Add ADC support
  iio:adc:imx: add Freescale Vybrid vf610 adc driver
  Documentation: add the binding file for Freescale vf610 ADC driver

 .../devicetree/bindings/iio/adc/vf610-adc.txt      |   57 ++
 arch/arm/boot/dts/vf610-twr.dts                    |   14 +
 arch/arm/boot/dts/vf610.dtsi                       |   26 +
 drivers/iio/adc/Kconfig                            |   11 +
 drivers/iio/adc/Makefile                           |    1 +
 drivers/iio/adc/vf610_adc.c                        |  744 ++++++++++++++++++++
 6 files changed, 853 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
 create mode 100644 drivers/iio/adc/vf610_adc.c

-- 
1.7.2.rc3

[PATCH v3 1/3] ARM: dts: vf610-twr: Add ADC support

[Fugang Duan]

vf610 have two ADC controllers, and vf610-twr board ADC0_SE5 pin connect
to sliding rheostat for ADC test, other ADC pins connect to connectors for
future use.

Add support for ADC0_SE5.

Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 arch/arm/boot/dts/vf610-twr.dts |   14 ++++++++++++++
 arch/arm/boot/dts/vf610.dtsi    |   26 ++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index c8047ca..0867004 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -36,6 +36,20 @@
 
 };
 
+&adc0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_adc0_ad5>;
+	fsl,adc-io-pinctl = <0x20>;
+	fsl,adc-vref = <3300000>;
+	fsl,adc-clk-div = <2>;
+	fsl,adc-res = <12>;
+	fsl,adc-sam-time = <4>;
+	fsl,adc-aver-sam-sel = <4>;
+	fsl,adc-hw-aver-en;
+	fsl,adc-low-power-mode;
+	status = "okay";
+};
+
 &dspi0 {
 	bus-num = <0>;
 	pinctrl-names = "default";
diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
index d31ce1b..b5b21ea 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -152,6 +152,15 @@
 				clock-names = "pit";
 			};
 
+			adc0: adc@4003b000 {
+				compatible = "fsl,vf610-adc";
+				reg = <0x4003b000 0x1000>;
+				interrupts = <0 53 0x04>;
+				clocks = <&clks VF610_CLK_ADC0>;
+				clock-names = "adc";
+				status = "disabled";
+			};
+
 			wdog@4003e000 {
 				compatible = "fsl,vf610-wdt", "fsl,imx21-wdt";
 				reg = <0x4003e000 0x1000>;
@@ -178,6 +187,14 @@
 
 				/* functions and groups pins */
 
+				adc0 {
+					pinctrl_adc0_ad5: adc0_ad5 {
+						fsl,pins = <
+						VF610_PAD_PTC30__ADC0_SE5	0xa1
+						>;
+					};
+				};
+
 				dcu0 {
 					pinctrl_dcu0_1: dcu0grp_1 {
 						fsl,pins = <
@@ -450,6 +467,15 @@
 				status = "disabled";
 			};
 
+			adc1: adc@400bb000 {
+				compatible = "fsl,vf610-adc";
+				reg = <0x400bb000 0x1000>;
+				interrupts = <0 54 0x04>;
+				clocks = <&clks VF610_CLK_ADC1>;
+				clock-names = "adc";
+				status = "disabled";
+			};
+
 			fec0: ethernet@400d0000 {
 				compatible = "fsl,mvf600-fec";
 				reg = <0x400d0000 0x1000>;
-- 
1.7.2.rc3

[PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver

[Fugang Duan]

Add Freescale Vybrid vf610 adc driver. The driver only support
ADC software trigger.

Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 drivers/iio/adc/Kconfig     |   11 +
 drivers/iio/adc/Makefile    |    1 +
 drivers/iio/adc/vf610_adc.c |  744 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 756 insertions(+), 0 deletions(-)

diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 2209f28..d0476ec 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -204,4 +204,15 @@ config VIPERBOARD_ADC
 	  Say yes here to access the ADC part of the Nano River
 	  Technologies Viperboard.
 
+config VF610_ADC
+	tristate "Freescale vf610 ADC driver"
+	help
+	  Say yes here if you want support for the Vybrid board
+	  analog-to-digital converter.
+	  Since the IP is used for i.MX6SLX and i.MX7 serial sillicons, the driver
+	  also support the subsequent chips.
+
+	  This driver can also be built as a module. If so, the module will be
+	  called vf610_adc.
+
 endmenu
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index ba9a10a..c39ae38 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -22,3 +22,4 @@ obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
 obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
 obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
 obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
+obj-$(CONFIG_VF610_ADC) += vf610_adc.o
diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
new file mode 100644
index 0000000..0c200d91
--- /dev/null
+++ b/drivers/iio/adc/vf610_adc.c
@@ -0,0 +1,744 @@
+/*
+ * Freescale Vybrid vf610 ADC driver
+ *
+ * Copyright 2013 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/interrupt.h>
+#include <linux/delay.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/regulator/consumer.h>
+#include <linux/of_platform.h>
+#include <linux/err.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/machine.h>
+#include <linux/iio/driver.h>
+
+/* This will be the driver name the kernel reports */
+#define DRIVER_NAME "vf610-adc"
+
+/* Vybrid/IMX ADC registers */
+#define ADC_HC0		0x00
+#define ADC_HC1		0x04
+#define ADC_HS		0x08
+#define ADC_R0		0x0c
+#define ADC_R1		0x10
+#define ADC_CFG		0x14
+#define ADC_GC		0x18
+#define ADC_GS		0x1c
+#define ADC_CV		0x20
+#define ADC_OFS		0x24
+#define ADC_CAL		0x28
+#define ADC_PCTL	0x30
+
+/* Configuration register field define */
+#define VF610_ADC_MODE_BIT8		0x00
+#define VF610_ADC_MODE_BIT10		0x04
+#define VF610_ADC_MODE_BIT12		0x08
+#define VF610_ADC_DATA_BIT8_MASK	0xFF
+#define VF610_ADC_DATA_BIT10_MASK	0x3FF
+#define VF610_ADC_DATA_BIT12_MASK	0xFFF
+#define VF610_ADC_BUSCLK2_SEL		0x01
+#define VF610_ADC_ALTCLK_SEL		0x02
+#define VF610_ADC_ADACK_SEL		0x03
+#define VF610_ADC_CLK_DIV2		0x20
+#define VF610_ADC_CLK_DIV4		0x40
+#define VF610_ADC_CLK_DIV8		0x60
+#define VF610_ADC_ADLSMP_LONG		0x10
+#define VF610_ADC_ADSTS_SHORT		0x100
+#define VF610_ADC_ADSTS_NORMAL		0x200
+#define VF610_ADC_ADSTS_LONG		0x300
+#define VF610_ADC_ADLPC_EN		0x80
+#define VF610_ADC_ADHSC_EN		0x400
+#define VF610_ADC_REFSEL_VALT		0x100
+#define VF610_ADC_REFSEL_VBG		0x1000
+#define VF610_ADC_ADTRG_HARD		0x2000
+#define VF610_ADC_AVGS_8		0x4000
+#define VF610_ADC_AVGS_16		0x8000
+#define VF610_ADC_AVGS_32		0xC000
+#define VF610_ADC_OVWREN		0x10000
+
+/* General control register field define */
+#define VF610_ADC_ADACKEN	0x1
+#define VF610_ADC_DMAEN		0x2
+#define VF610_ADC_ACREN		0x4
+#define VF610_ADC_ACFGT		0x8
+#define VF610_ADC_ACFE		0x10
+#define VF610_ADC_AVGEN		0x20
+#define VF610_ADC_ADCON		0x40
+#define VF610_ADC_CAL		0x80
+
+/* Other field define */
+#define VF610_ADC_IOPCTL5	0x20
+#define VF610_ADC_ADCHC(x)	((x) & 0xF)
+#define VF610_ADC_AIEN		(1 << 7)
+#define VF610_ADC_CONV_DISABLE	0x1F
+#define VF610_ADC_HS_COCO0	0x1
+#define VF610_ADC_CALF		0x2
+#define VF610_ADC_MAX_CHANS_NUM	16
+#define VF610_ADC_TIMEOUT	(msecs_to_jiffies(100))
+
+enum clk_sel {
+	ADCIOC_BUSCLK_SET,
+	ADCIOC_ALTCLK_SET,
+	ADCIOC_ADACK_SET,
+};
+
+enum vol_ref {
+	VF610_ADCIOC_VR_VREF_SET,
+	VF610_ADCIOC_VR_VALT_SET,
+	VF610_ADCIOC_VR_VBG_SET,
+};
+
+struct vf610_adc_feature {
+	enum clk_sel clk_sel;
+	enum vol_ref vol_ref;
+
+	int pctl;
+	int clk_div_num;
+	int res_mode;
+	int sam_time;
+	int hw_sam;
+
+	bool calibration;
+	bool cc_en;
+	bool dataov_en;
+	bool hw_average;
+	bool tri_hw;
+	bool hs_oper;
+	bool lpm;
+	bool dma_en;
+	bool ac_clk_en;
+	bool cmp_func_en;
+	bool cmp_range_en;
+	bool cmp_greater_en;
+};
+
+struct vf610_adc {
+	struct device           *dev;
+	void __iomem            *regs;
+	struct clk              *clk;
+	unsigned int            irq;
+
+	u32			vref_uv;
+	u32			value;
+	struct regulator        *vref;
+	struct vf610_adc_feature	adc_feature;
+
+	struct completion       completion;
+};
+
+#define VF610_ADC_CHAN(_idx, _chan_type) {			\
+	.type = (_chan_type),					\
+	.indexed = 1,						\
+	.channel = _idx,					\
+	.address = _idx,					\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+}
+
+static const struct iio_chan_spec vf610_adc_iio_channels[] = {
+	VF610_ADC_CHAN(0, IIO_VOLTAGE),
+	VF610_ADC_CHAN(1, IIO_VOLTAGE),
+	VF610_ADC_CHAN(2, IIO_VOLTAGE),
+	VF610_ADC_CHAN(3, IIO_VOLTAGE),
+	VF610_ADC_CHAN(4, IIO_VOLTAGE),
+	VF610_ADC_CHAN(5, IIO_VOLTAGE),
+	VF610_ADC_CHAN(6, IIO_VOLTAGE),
+	VF610_ADC_CHAN(7, IIO_VOLTAGE),
+	VF610_ADC_CHAN(8, IIO_VOLTAGE),
+	VF610_ADC_CHAN(9, IIO_VOLTAGE),
+	VF610_ADC_CHAN(10, IIO_VOLTAGE),
+	VF610_ADC_CHAN(11, IIO_VOLTAGE),
+	VF610_ADC_CHAN(12, IIO_VOLTAGE),
+	VF610_ADC_CHAN(13, IIO_VOLTAGE),
+	VF610_ADC_CHAN(14, IIO_VOLTAGE),
+	VF610_ADC_CHAN(15, IIO_VOLTAGE),
+};
+
+static void vf610_adc_cfg_of_init(struct vf610_adc *info)
+{
+	struct device_node *np = info->dev->of_node;
+
+	/*
+	 * set default Configuration for ADC controller
+	 * This config may upgrade to require from DT
+	 */
+	info->adc_feature.clk_sel = ADCIOC_BUSCLK_SET;
+	info->adc_feature.vol_ref = VF610_ADCIOC_VR_VREF_SET;
+	info->adc_feature.calibration = true;
+	info->adc_feature.tri_hw = false;
+	info->adc_feature.dataov_en = true;
+	info->adc_feature.ac_clk_en = false;
+	info->adc_feature.dma_en = false;
+	info->adc_feature.cc_en = false;
+	info->adc_feature.cmp_func_en = false;
+	info->adc_feature.cmp_range_en = false;
+	info->adc_feature.cmp_greater_en = false;
+
+	if (of_property_read_u32(np, "fsl,adc-io-pinctl",
+			&info->adc_feature.pctl)) {
+		info->adc_feature.pctl = VF610_ADC_IOPCTL5;
+		dev_info(info->dev,
+			"Miss adc-io-pinctl in dt, enable pin SE5.\n");
+	}
+
+	if (info->vref)
+		info->vref_uv = regulator_get_voltage(info->vref);
+	else if (of_property_read_u32(np, "fsl,adc-vref", &info->vref_uv))
+		dev_err(info->dev,
+			"Miss adc-vref property or vref regulator in the DT.\n");
+
+	if (of_property_read_u32(np, "fsl,adc-clk-div",
+			&info->adc_feature.clk_div_num)) {
+		info->adc_feature.clk_div_num = 2;
+		dev_info(info->dev,
+			"Miss adc-clk-div in dt, set divider to 2.\n");
+	}
+
+	if (of_property_read_u32(np, "fsl,adc-res",
+			&info->adc_feature.res_mode)) {
+		info->adc_feature.res_mode = 12;
+		dev_info(info->dev,
+			"Miss adc-res property in dt, use 8bit mode.\n");
+	}
+
+	if (of_property_read_u32(np, "fsl,adc-sam-time",
+			&info->adc_feature.sam_time)) {
+		info->adc_feature.sam_time = 4;
+		dev_info(info->dev,
+			"Miss adc-sam-time property in dt, set to 4.\n");
+	}
+
+	info->adc_feature.hw_average = of_property_read_bool(np,
+					"fsl,adc-hw-aver-en");
+	if (info->adc_feature.hw_average &&
+		 of_property_read_u32(np, "fsl,adc-aver-sam-sel",
+				&info->adc_feature.hw_sam)) {
+		info->adc_feature.hw_sam = 4;
+		dev_info(info->dev,
+			"Miss adc-aver-sam-sel property in dt, set to 4.\n");
+	}
+
+	info->adc_feature.lpm = of_property_read_bool(np, "fsl,adc-low-power-mode");
+	info->adc_feature.hs_oper = of_property_read_bool(np, "fsl,adc-high-speed-conv");
+}
+
+static inline void vf610_adc_cfg_post_set(struct vf610_adc *info)
+{
+	struct vf610_adc_feature *adc_feature = &(info->adc_feature);
+	int cfg_data = 0;
+	int gc_data = 0;
+
+	/* clock select and clock devider */
+	switch (adc_feature->clk_div_num) {
+	case 1:
+		break;
+	case 2:
+		cfg_data |= VF610_ADC_CLK_DIV2;
+		break;
+	case 4:
+		cfg_data |= VF610_ADC_CLK_DIV4;
+		break;
+	case 8:
+		cfg_data |= VF610_ADC_CLK_DIV8;
+		break;
+	case 16:
+		switch (adc_feature->clk_sel) {
+		case ADCIOC_BUSCLK_SET:
+			cfg_data |= VF610_ADC_BUSCLK2_SEL | VF610_ADC_CLK_DIV8;
+			break;
+		default:
+			dev_err(info->dev, "error clk divider\n");
+			break;
+		}
+		break;
+	}
+
+	switch (adc_feature->clk_sel) {
+	case ADCIOC_ALTCLK_SET:
+		cfg_data |= VF610_ADC_ALTCLK_SEL;
+		break;
+	case ADCIOC_ADACK_SET:
+		cfg_data |= VF610_ADC_ADACK_SEL;
+		break;
+	default:
+		break;
+	}
+
+	/* resolution mode */
+	switch (adc_feature->res_mode) {
+	case 8:
+		cfg_data |= VF610_ADC_MODE_BIT8;
+		break;
+	case 10:
+		cfg_data |= VF610_ADC_MODE_BIT10;
+		break;
+	case 12:
+		cfg_data |= VF610_ADC_MODE_BIT12;
+		break;
+	default:
+		dev_err(info->dev, "error resolution mode\n");
+		break;
+	}
+
+	/* Defines the sample time duration */
+	switch (adc_feature->sam_time) {
+	case 2:
+		break;
+	case 4:
+		cfg_data |= VF610_ADC_ADSTS_SHORT;
+		break;
+	case 6:
+		cfg_data |= VF610_ADC_ADSTS_NORMAL;
+		break;
+	case 8:
+		cfg_data |= VF610_ADC_ADSTS_LONG;
+		break;
+	case 12:
+		cfg_data |= VF610_ADC_ADLSMP_LONG;
+		break;
+	case 16:
+		cfg_data |= VF610_ADC_ADLSMP_LONG | VF610_ADC_ADSTS_SHORT;
+		break;
+	case 20:
+		cfg_data |= VF610_ADC_ADLSMP_LONG | VF610_ADC_ADSTS_NORMAL;
+		break;
+	case 24:
+		cfg_data |= VF610_ADC_ADLSMP_LONG | VF610_ADC_ADSTS_LONG;
+		break;
+	default:
+		dev_err(info->dev, "error sample duration\n");
+		break;
+	}
+
+	/* low power configuration */
+	cfg_data |= VF610_ADC_ADLPC_EN;
+
+	/* high speed operation */
+	cfg_data |= VF610_ADC_ADHSC_EN;
+
+	/* voltage reference*/
+	switch (adc_feature->vol_ref) {
+	case VF610_ADCIOC_VR_VREF_SET:
+		break;
+	case VF610_ADCIOC_VR_VALT_SET:
+		cfg_data |= VF610_ADC_REFSEL_VALT;
+		break;
+	case VF610_ADCIOC_VR_VBG_SET:
+		cfg_data |= VF610_ADC_REFSEL_VBG;
+		break;
+	default:
+		dev_err(info->dev, "error voltage reference\n");
+	}
+
+	/* trigger select */
+	if (adc_feature->tri_hw)
+		cfg_data |= VF610_ADC_ADTRG_HARD;
+
+	/* hardware average select */
+	if (adc_feature->hw_average) {
+		switch (adc_feature->hw_sam) {
+		case 4:
+			break;
+		case 8:
+			cfg_data |= VF610_ADC_AVGS_8;
+			break;
+		case 16:
+			cfg_data |= VF610_ADC_AVGS_16;
+			break;
+		case 32:
+			cfg_data |= VF610_ADC_AVGS_32;
+			break;
+		default:
+			dev_err(info->dev,
+			"error hardware sample average select\n");
+		}
+
+		gc_data |= VF610_ADC_AVGEN;
+	}
+
+	/* data overwrite enable */
+	if (adc_feature->dataov_en)
+		cfg_data |= VF610_ADC_OVWREN;
+
+	/* Asynchronous clock output enable */
+	if (adc_feature->ac_clk_en)
+		gc_data |= VF610_ADC_ADACKEN;
+
+	/* dma enable */
+	if (adc_feature->dma_en)
+		gc_data |= VF610_ADC_DMAEN;
+
+	/* continue function enable */
+	if (adc_feature->cc_en)
+		gc_data |= VF610_ADC_ADCON;
+
+	/* compare function enable */
+	if (adc_feature->cmp_func_en)
+		gc_data |= VF610_ADC_ACFE;
+
+	/* greater than enable */
+	if (adc_feature->cmp_greater_en)
+		gc_data |= VF610_ADC_ACFGT;
+
+	/* range enable */
+	if (adc_feature->cmp_range_en)
+		gc_data |= VF610_ADC_ACREN;
+
+	writel(cfg_data, info->regs + ADC_CFG);
+	writel(gc_data, info->regs + ADC_GC);
+}
+
+static void vf610_adc_calibration(struct vf610_adc *info)
+{
+	int adc_gc, hc_cfg;
+	int timeout;
+
+	if (!info->adc_feature.calibration)
+		return;
+
+	/* enable calibration interrupt */
+	hc_cfg = VF610_ADC_AIEN | VF610_ADC_CONV_DISABLE;
+	writel(hc_cfg, info->regs + ADC_HC0);
+
+	adc_gc = readl(info->regs + ADC_GC);
+	writel(adc_gc | VF610_ADC_CAL, info->regs + ADC_GC);
+
+	timeout = wait_for_completion_interruptible_timeout
+			(&info->completion, VF610_ADC_TIMEOUT);
+	if (timeout == 0)
+		dev_err(info->dev, "Timeout for adc calibration\n");
+
+	adc_gc = readl(info->regs + ADC_GS);
+	if (adc_gc & VF610_ADC_CALF)
+		dev_err(info->dev, "ADC calibration failed\n");
+
+	info->adc_feature.calibration = false;
+}
+
+static inline void vf610_adc_cfg_set(struct vf610_adc *info)
+{
+	struct vf610_adc_feature *adc_feature = &(info->adc_feature);
+	int cfg_data = 0;
+
+	cfg_data = readl(info->regs + ADC_CFG);
+
+	/* low power configuration */
+	if (!adc_feature->lpm)
+		cfg_data &= ~VF610_ADC_ADLPC_EN;
+
+	/* high speed operation */
+	if (!adc_feature->hs_oper)
+		cfg_data &= ~VF610_ADC_ADHSC_EN;
+
+	/* trigger select */
+	if (adc_feature->tri_hw)
+		cfg_data |= VF610_ADC_ADTRG_HARD;
+
+	writel(cfg_data, info->regs + ADC_CFG);
+}
+
+static void vf610_adc_hw_init(struct vf610_adc *info)
+{
+	/* pin control for Sliding rheostat */
+	writel(info->adc_feature.pctl, info->regs + ADC_PCTL);
+
+	/* CFG: Feature set */
+	vf610_adc_cfg_post_set(info);
+
+	/* adc calibration */
+	vf610_adc_calibration(info);
+
+	/* CFG: power and speed set */
+	vf610_adc_cfg_set(info);
+}
+
+static inline int vf610_adc_read_data(struct vf610_adc *info)
+{
+	int result;
+
+	result = readl(info->regs + ADC_R0);
+
+	switch (info->adc_feature.res_mode) {
+	case 8:
+		result &= VF610_ADC_DATA_BIT8_MASK;
+		break;
+	case 10:
+		result &= VF610_ADC_DATA_BIT10_MASK;
+		break;
+	case 12:
+		result &= VF610_ADC_DATA_BIT12_MASK;
+		break;
+	default:
+		dev_err(info->dev, "error resolution mode\n");
+		break;
+	}
+
+	return result;
+}
+
+static irqreturn_t vf610_adc_isr(int irq, void *dev_id)
+{
+	struct vf610_adc *info = (struct vf610_adc *)dev_id;
+	int coco;
+
+	coco = readl(info->regs + ADC_HS);
+	if (coco & VF610_ADC_HS_COCO0) {
+		info->value = vf610_adc_read_data(info);
+		complete(&info->completion);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int vf610_read_raw(struct iio_dev *indio_dev,
+			struct iio_chan_spec const *chan,
+			int *val,
+			int *val2,
+			long mask)
+{
+	struct vf610_adc *info = iio_priv(indio_dev);
+	unsigned int hc_cfg;
+	unsigned long timeout;
+
+	/* Check for invalid channel */
+	if (chan->channel > VF610_ADC_MAX_CHANS_NUM)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		mutex_lock(&indio_dev->mlock);
+
+		hc_cfg = VF610_ADC_ADCHC(chan->channel);
+		hc_cfg |= VF610_ADC_AIEN;
+		writel(hc_cfg, info->regs + ADC_HC0);
+		timeout = wait_for_completion_interruptible_timeout
+				(&info->completion, VF610_ADC_TIMEOUT);
+		*val = info->value;
+
+		mutex_unlock(&indio_dev->mlock);
+
+		if (timeout == 0)
+			return -ETIMEDOUT;
+
+		return IIO_VAL_INT;
+
+	case IIO_CHAN_INFO_SCALE:
+		*val = info->vref_uv >> info->adc_feature.res_mode;
+		*val2 = 0;
+		return IIO_VAL_INT_PLUS_MICRO;
+	default:
+		break;
+	}
+
+	return -EINVAL;
+}
+
+static int vf610_adc_reg_access(struct iio_dev *indio_dev,
+			unsigned reg, unsigned writeval,
+			unsigned *readval)
+{
+	struct vf610_adc *info = iio_priv(indio_dev);
+
+	if (readval == NULL)
+		return -EINVAL;
+
+	*readval = readl(info->regs + reg);
+
+	return 0;
+}
+
+static const struct iio_info vf610_adc_iio_info = {
+	.read_raw = &vf610_read_raw,
+	.debugfs_reg_access = &vf610_adc_reg_access,
+	.driver_module = THIS_MODULE,
+};
+
+static const struct of_device_id vf610_adc_match[] = {
+	{ .compatible = "fsl,vf610-adc", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, vf610_adc_dt_ids);
+
+static int vf610_adc_probe(struct platform_device *pdev)
+{
+	struct vf610_adc *info = NULL;
+	struct iio_dev *indio_dev;
+	struct resource *mem;
+	int ret = -ENODEV;
+
+	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct vf610_adc));
+	if (!indio_dev) {
+		dev_err(&pdev->dev, "Failed allocating iio device\n");
+		return -ENOMEM;
+	}
+
+	info = iio_priv(indio_dev);
+	info->dev = &pdev->dev;
+
+	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	info->regs = devm_ioremap_resource(&pdev->dev, mem);
+	if (IS_ERR(info->regs))
+		return PTR_ERR(info->regs);
+
+	info->irq = platform_get_irq(pdev, 0);
+	if (info->irq < 0) {
+		dev_err(&pdev->dev, "no irq resource?\n");
+		return -EINVAL;
+	}
+
+	ret = devm_request_irq(info->dev, info->irq,
+				vf610_adc_isr, 0,
+				dev_name(&pdev->dev), info);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "failed requesting irq, irq = %d\n",
+							info->irq);
+		return ret;
+	}
+
+	info->clk = devm_clk_get(&pdev->dev, "adc");
+	if (IS_ERR(info->clk)) {
+		dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
+						PTR_ERR(info->clk));
+		ret = PTR_ERR(info->clk);
+		return ret;
+	}
+
+	info->vref = devm_regulator_get(&pdev->dev, "vref");
+	if (!IS_ERR(info->vref)) {
+		ret = regulator_enable(info->vref);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"Failed to enable vref regulator: %d\n", ret);
+			return ret;
+		}
+	} else {
+		info->vref = NULL;
+	}
+
+	platform_set_drvdata(pdev, indio_dev);
+
+	init_completion(&info->completion);
+
+	indio_dev->name = dev_name(&pdev->dev);
+	indio_dev->dev.parent = &pdev->dev;
+	indio_dev->dev.of_node = pdev->dev.of_node;
+	indio_dev->info = &vf610_adc_iio_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = vf610_adc_iio_channels;
+	indio_dev->num_channels = ARRAY_SIZE(vf610_adc_iio_channels);
+
+	ret = devm_iio_device_register(&pdev->dev, indio_dev);
+	if (ret)
+		return ret;
+
+	ret = clk_prepare_enable(info->clk);
+	if (ret) {
+		dev_err(&pdev->dev,
+			"Could not prepare or enable the clock.\n");
+		return ret;
+	}
+
+	vf610_adc_cfg_of_init(info);
+	vf610_adc_hw_init(info);
+
+	return 0;
+}
+
+static int vf610_adc_remove(struct platform_device *pdev)
+{
+	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
+	struct vf610_adc *info = iio_priv(indio_dev);
+
+	if (info->vref)
+		regulator_disable(info->vref);
+	clk_disable_unprepare(info->clk);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int vf610_adc_suspend(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct vf610_adc *info = iio_priv(indio_dev);
+	int hc_cfg;
+
+	/* enter to stop mode */
+	hc_cfg = readl(info->regs + ADC_HC0);
+	hc_cfg |= VF610_ADC_CONV_DISABLE;
+	writel(hc_cfg, info->regs + ADC_HC0);
+
+	clk_disable_unprepare(info->clk);
+	if (info->vref)
+		regulator_disable(info->vref);
+
+	return 0;
+}
+
+static int vf610_adc_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct vf610_adc *info = iio_priv(indio_dev);
+	int ret;
+
+	if (info->vref) {
+		ret = regulator_enable(info->vref);
+		if (ret)
+			return ret;
+	}
+
+	ret = clk_prepare_enable(info->clk);
+	if (ret)
+		return ret;
+
+	vf610_adc_hw_init(info);
+
+	return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(vf610_adc_pm_ops,
+			vf610_adc_suspend,
+			vf610_adc_resume);
+
+static struct platform_driver vf610_adc_driver = {
+	.probe          = vf610_adc_probe,
+	.remove         = vf610_adc_remove,
+	.driver         = {
+		.name   = DRIVER_NAME,
+		.owner  = THIS_MODULE,
+		.of_match_table = vf610_adc_match,
+		.pm     = &vf610_adc_pm_ops,
+	},
+};
+
+module_platform_driver(vf610_adc_driver);
+
+MODULE_AUTHOR("Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>");
+MODULE_DESCRIPTION("Freescale VF610 ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.2.rc3

Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver

[Lars-Peter Clausen]

On 11/26/2013 11:56 AM, Fugang Duan wrote:
> Add Freescale Vybrid vf610 adc driver. The driver only support
> ADC software trigger.
> 
> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>

The driver itself looks mostly fine. I'm not so sure about the dt bindings
though.

> ---
>  drivers/iio/adc/Kconfig     |   11 +
>  drivers/iio/adc/Makefile    |    1 +
>  drivers/iio/adc/vf610_adc.c |  744 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 756 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 2209f28..d0476ec 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -204,4 +204,15 @@ config VIPERBOARD_ADC
>  	  Say yes here to access the ADC part of the Nano River
>  	  Technologies Viperboard.
>  
> +config VF610_ADC

Keep things in alphabetical order...

> +	tristate "Freescale vf610 ADC driver"
> +	help
> +	  Say yes here if you want support for the Vybrid board
> +	  analog-to-digital converter.
> +	  Since the IP is used for i.MX6SLX and i.MX7 serial sillicons, the driver
> +	  also support the subsequent chips.
> +
> +	  This driver can also be built as a module. If so, the module will be
> +	  called vf610_adc.
> +
>  endmenu
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index ba9a10a..c39ae38 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -22,3 +22,4 @@ obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>  obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
>  obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
> +obj-$(CONFIG_VF610_ADC) += vf610_adc.o

Same here

> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
> new file mode 100644
> index 0000000..0c200d91
> --- /dev/null
> +++ b/drivers/iio/adc/vf610_adc.c
> @@ -0,0 +1,744 @@
[...]
> +struct vf610_adc {
> +	struct device           *dev;
> +	void __iomem            *regs;
> +	struct clk              *clk;
> +	unsigned int            irq;

irq doesn't seem to be used outside of the probe function.

> +
> +	u32			vref_uv;
> +	u32			value;
> +	struct regulator        *vref;
> +	struct vf610_adc_feature	adc_feature;
> +
> +	struct completion       completion;
> +};
[...]
> +static void vf610_adc_cfg_of_init(struct vf610_adc *info)
> +{
> +	struct device_node *np = info->dev->of_node;
> +
> +	/*
> +	 * set default Configuration for ADC controller
> +	 * This config may upgrade to require from DT
> +	 */
> +	info->adc_feature.clk_sel = ADCIOC_BUSCLK_SET;
> +	info->adc_feature.vol_ref = VF610_ADCIOC_VR_VREF_SET;
> +	info->adc_feature.calibration = true;
> +	info->adc_feature.tri_hw = false;
> +	info->adc_feature.dataov_en = true;
> +	info->adc_feature.ac_clk_en = false;
> +	info->adc_feature.dma_en = false;
> +	info->adc_feature.cc_en = false;
> +	info->adc_feature.cmp_func_en = false;
> +	info->adc_feature.cmp_range_en = false;
> +	info->adc_feature.cmp_greater_en = false;
> +
> +	if (of_property_read_u32(np, "fsl,adc-io-pinctl",
> +			&info->adc_feature.pctl)) {
> +		info->adc_feature.pctl = VF610_ADC_IOPCTL5;
> +		dev_info(info->dev,
> +			"Miss adc-io-pinctl in dt, enable pin SE5.\n");
> +	}
> +
> +	if (info->vref)
> +		info->vref_uv = regulator_get_voltage(info->vref);
> +	else if (of_property_read_u32(np, "fsl,adc-vref", &info->vref_uv))
> +		dev_err(info->dev,
> +			"Miss adc-vref property or vref regulator in the DT.\n");

If you have a fixed reference voltage use the fixed voltage regulator. Don't
invent custom ways of specifying this.

> +
> +	if (of_property_read_u32(np, "fsl,adc-clk-div",
> +			&info->adc_feature.clk_div_num)) {
> +		info->adc_feature.clk_div_num = 2;
> +		dev_info(info->dev,
> +			"Miss adc-clk-div in dt, set divider to 2.\n");
> +	}
> +
> +	if (of_property_read_u32(np, "fsl,adc-res",
> +			&info->adc_feature.res_mode)) {
> +		info->adc_feature.res_mode = 12;
> +		dev_info(info->dev,
> +			"Miss adc-res property in dt, use 8bit mode.\n");
> +	}
> +
> +	if (of_property_read_u32(np, "fsl,adc-sam-time",
> +			&info->adc_feature.sam_time)) {
> +		info->adc_feature.sam_time = 4;
> +		dev_info(info->dev,
> +			"Miss adc-sam-time property in dt, set to 4.\n");
> +	}
> +
> +	info->adc_feature.hw_average = of_property_read_bool(np,
> +					"fsl,adc-hw-aver-en");
> +	if (info->adc_feature.hw_average &&
> +		 of_property_read_u32(np, "fsl,adc-aver-sam-sel",
> +				&info->adc_feature.hw_sam)) {
> +		info->adc_feature.hw_sam = 4;
> +		dev_info(info->dev,
> +			"Miss adc-aver-sam-sel property in dt, set to 4.\n");
> +	}
> +
> +	info->adc_feature.lpm = of_property_read_bool(np, "fsl,adc-low-power-mode");
> +	info->adc_feature.hs_oper = of_property_read_bool(np, "fsl,adc-high-speed-conv");

Some of the properties look like they should be runtime configurable rather
then board specific settings. E.g. the resolution or the sampling frequency,
or whether averaging is enabled.


> +}
[...]
> +static void vf610_adc_calibration(struct vf610_adc *info)
> +{
> +	int adc_gc, hc_cfg;
> +	int timeout;
> +
> +	if (!info->adc_feature.calibration)
> +		return;
> +
> +	/* enable calibration interrupt */
> +	hc_cfg = VF610_ADC_AIEN | VF610_ADC_CONV_DISABLE;
> +	writel(hc_cfg, info->regs + ADC_HC0);
> +
> +	adc_gc = readl(info->regs + ADC_GC);
> +	writel(adc_gc | VF610_ADC_CAL, info->regs + ADC_GC);
> +
> +	timeout = wait_for_completion_interruptible_timeout
> +			(&info->completion, VF610_ADC_TIMEOUT);

This should probably not be interruptible.

> +	if (timeout == 0)
> +		dev_err(info->dev, "Timeout for adc calibration\n");
> +
> +	adc_gc = readl(info->regs + ADC_GS);
> +	if (adc_gc & VF610_ADC_CALF)
> +		dev_err(info->dev, "ADC calibration failed\n");
> +
> +	info->adc_feature.calibration = false;
> +}
> +
> +static inline void vf610_adc_cfg_set(struct vf610_adc *info)

I'd drop the inline

> +{
[...]
> +}

> +static inline int vf610_adc_read_data(struct vf610_adc *info)
same here
> +{
[...]
> +}
> +
[...]
> +static int vf610_read_raw(struct iio_dev *indio_dev,
> +			struct iio_chan_spec const *chan,
> +			int *val,
> +			int *val2,
> +			long mask)
> +{
> +	struct vf610_adc *info = iio_priv(indio_dev);
> +	unsigned int hc_cfg;
> +	unsigned long timeout;
> +
> +	/* Check for invalid channel */
> +	if (chan->channel > VF610_ADC_MAX_CHANS_NUM)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		mutex_lock(&indio_dev->mlock);
> +

putting a reinit_completion() here would make sense

> +		hc_cfg = VF610_ADC_ADCHC(chan->channel);
> +		hc_cfg |= VF610_ADC_AIEN;
> +		writel(hc_cfg, info->regs + ADC_HC0);
> +		timeout = wait_for_completion_interruptible_timeout
> +				(&info->completion, VF610_ADC_TIMEOUT);
> +		*val = info->value;
> +
> +		mutex_unlock(&indio_dev->mlock);
> +
> +		if (timeout == 0)
> +			return -ETIMEDOUT;

timeout can be negative if the thread was interrupted. In that case return
the error code in timeout.

> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = info->vref_uv >> info->adc_feature.res_mode;
> +		*val2 = 0;
> +		return IIO_VAL_INT_PLUS_MICRO;

It's better to use:
		*val = info->vref_uv / 1000;
		*val2 = info->adc_feature.res_mode;
		return IIO_VAL_FACTIONAL_LOG2;

> +	default:
> +		break;
> +	}
> +
> +	return -EINVAL;
> +}
[...]
> +static int vf610_adc_probe(struct platform_device *pdev)
> +{
> +	struct vf610_adc *info = NULL;
> +	struct iio_dev *indio_dev;
> +	struct resource *mem;
> +	int ret = -ENODEV;
> +
> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct vf610_adc));
> +	if (!indio_dev) {
> +		dev_err(&pdev->dev, "Failed allocating iio device\n");
> +		return -ENOMEM;
> +	}
> +
> +	info = iio_priv(indio_dev);
> +	info->dev = &pdev->dev;
> +
> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	info->regs = devm_ioremap_resource(&pdev->dev, mem);
> +	if (IS_ERR(info->regs))
> +		return PTR_ERR(info->regs);
> +
> +	info->irq = platform_get_irq(pdev, 0);
> +	if (info->irq < 0) {

0 isn't a valid irq number so this should be "irq <= 0"

> +		dev_err(&pdev->dev, "no irq resource?\n");
> +		return -EINVAL;
> +	}
> +
[...]
> +	vf610_adc_cfg_of_init(info);
> +	vf610_adc_hw_init(info);

This should probably be done before registering the device. Same goes for
the clk_enable() call.

> +
> +	return 0;
> +}
[...]

RE: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver

[Fugang Duan]

From: Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org>
Data: Tuesday, November 26, 2013 7:52 PM

>To: Duan Fugang-B38611
>Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
>shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Li Frank-B20596; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>Subject: Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver
>
>On 11/26/2013 11:56 AM, Fugang Duan wrote:
>> Add Freescale Vybrid vf610 adc driver. The driver only support ADC
>> software trigger.
>>
>> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>
>The driver itself looks mostly fine. I'm not so sure about the dt bindings
>though.
>
>> ---
>>  drivers/iio/adc/Kconfig     |   11 +
>>  drivers/iio/adc/Makefile    |    1 +
>>  drivers/iio/adc/vf610_adc.c |  744
>> +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 756 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig index
>> 2209f28..d0476ec 100644
>> --- a/drivers/iio/adc/Kconfig
>> +++ b/drivers/iio/adc/Kconfig
>> @@ -204,4 +204,15 @@ config VIPERBOARD_ADC
>>  	  Say yes here to access the ADC part of the Nano River
>>  	  Technologies Viperboard.
>>
>> +config VF610_ADC
>
>Keep things in alphabetical order...
>
>> +	tristate "Freescale vf610 ADC driver"
>> +	help
>> +	  Say yes here if you want support for the Vybrid board
>> +	  analog-to-digital converter.
>> +	  Since the IP is used for i.MX6SLX and i.MX7 serial sillicons, the
>driver
>> +	  also support the subsequent chips.
>> +
>> +	  This driver can also be built as a module. If so, the module will be
>> +	  called vf610_adc.
>> +
>>  endmenu
>> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile index
>> ba9a10a..c39ae38 100644
>> --- a/drivers/iio/adc/Makefile
>> +++ b/drivers/iio/adc/Makefile
>> @@ -22,3 +22,4 @@ obj-$(CONFIG_TI_ADC081C) += ti-adc081c.o
>>  obj-$(CONFIG_TI_AM335X_ADC) += ti_am335x_adc.o
>>  obj-$(CONFIG_TWL6030_GPADC) += twl6030-gpadc.o
>>  obj-$(CONFIG_VIPERBOARD_ADC) += viperboard_adc.o
>> +obj-$(CONFIG_VF610_ADC) += vf610_adc.o
>
>Same here
>
>> diff --git a/drivers/iio/adc/vf610_adc.c b/drivers/iio/adc/vf610_adc.c
>> new file mode 100644 index 0000000..0c200d91
>> --- /dev/null
>> +++ b/drivers/iio/adc/vf610_adc.c
>> @@ -0,0 +1,744 @@
>[...]
>> +struct vf610_adc {
>> +	struct device           *dev;
>> +	void __iomem            *regs;
>> +	struct clk              *clk;
>> +	unsigned int            irq;
>
>irq doesn't seem to be used outside of the probe function.

Yes, I will remove the private variable.
>
>> +
>> +	u32			vref_uv;
>> +	u32			value;
>> +	struct regulator        *vref;
>> +	struct vf610_adc_feature	adc_feature;
>> +
>> +	struct completion       completion;
>> +};
>[...]
>> +static void vf610_adc_cfg_of_init(struct vf610_adc *info) {
>> +	struct device_node *np = info->dev->of_node;
>> +
>> +	/*
>> +	 * set default Configuration for ADC controller
>> +	 * This config may upgrade to require from DT
>> +	 */
>> +	info->adc_feature.clk_sel = ADCIOC_BUSCLK_SET;
>> +	info->adc_feature.vol_ref = VF610_ADCIOC_VR_VREF_SET;
>> +	info->adc_feature.calibration = true;
>> +	info->adc_feature.tri_hw = false;
>> +	info->adc_feature.dataov_en = true;
>> +	info->adc_feature.ac_clk_en = false;
>> +	info->adc_feature.dma_en = false;
>> +	info->adc_feature.cc_en = false;
>> +	info->adc_feature.cmp_func_en = false;
>> +	info->adc_feature.cmp_range_en = false;
>> +	info->adc_feature.cmp_greater_en = false;
>> +
>> +	if (of_property_read_u32(np, "fsl,adc-io-pinctl",
>> +			&info->adc_feature.pctl)) {
>> +		info->adc_feature.pctl = VF610_ADC_IOPCTL5;
>> +		dev_info(info->dev,
>> +			"Miss adc-io-pinctl in dt, enable pin SE5.\n");
>> +	}
>> +
>> +	if (info->vref)
>> +		info->vref_uv = regulator_get_voltage(info->vref);
>> +	else if (of_property_read_u32(np, "fsl,adc-vref", &info->vref_uv))
>> +		dev_err(info->dev,
>> +			"Miss adc-vref property or vref regulator in the DT.\n");
>
>If you have a fixed reference voltage use the fixed voltage regulator. Don't
>invent custom ways of specifying this.
>
Different boards design may use different method to supply the reference voltage.
There maybe fixed voltage regulator, maybe no regulator just use fixed voltage.

>> +
>> +	if (of_property_read_u32(np, "fsl,adc-clk-div",
>> +			&info->adc_feature.clk_div_num)) {
>> +		info->adc_feature.clk_div_num = 2;
>> +		dev_info(info->dev,
>> +			"Miss adc-clk-div in dt, set divider to 2.\n");
>> +	}
>> +
>> +	if (of_property_read_u32(np, "fsl,adc-res",
>> +			&info->adc_feature.res_mode)) {
>> +		info->adc_feature.res_mode = 12;
>> +		dev_info(info->dev,
>> +			"Miss adc-res property in dt, use 8bit mode.\n");
>> +	}
>> +
>> +	if (of_property_read_u32(np, "fsl,adc-sam-time",
>> +			&info->adc_feature.sam_time)) {
>> +		info->adc_feature.sam_time = 4;
>> +		dev_info(info->dev,
>> +			"Miss adc-sam-time property in dt, set to 4.\n");
>> +	}
>> +
>> +	info->adc_feature.hw_average = of_property_read_bool(np,
>> +					"fsl,adc-hw-aver-en");
>> +	if (info->adc_feature.hw_average &&
>> +		 of_property_read_u32(np, "fsl,adc-aver-sam-sel",
>> +				&info->adc_feature.hw_sam)) {
>> +		info->adc_feature.hw_sam = 4;
>> +		dev_info(info->dev,
>> +			"Miss adc-aver-sam-sel property in dt, set to 4.\n");
>> +	}
>> +
>> +	info->adc_feature.lpm = of_property_read_bool(np, "fsl,adc-low-power-
>mode");
>> +	info->adc_feature.hs_oper = of_property_read_bool(np,
>> +"fsl,adc-high-speed-conv");
>
>Some of the properties look like they should be runtime configurable rather
>then board specific settings. E.g. the resolution or the sampling frequency, or
>whether averaging is enabled.
>
>
So, I have question:
Since the ADC have many user configurable setting, I have import some of them from DT, and some of them use static define.
How to set/change them in runtime for user configuration ? 
Introduce ioctl ?

>> +}
>[...]

The format is caused by outlook.

>> +static void vf610_adc_calibration(struct vf610_adc *info) {
>> +	int adc_gc, hc_cfg;
>> +	int timeout;
>> +
>> +	if (!info->adc_feature.calibration)
>> +		return;
>> +
>> +	/* enable calibration interrupt */
>> +	hc_cfg = VF610_ADC_AIEN | VF610_ADC_CONV_DISABLE;
>> +	writel(hc_cfg, info->regs + ADC_HC0);
>> +
>> +	adc_gc = readl(info->regs + ADC_GC);
>> +	writel(adc_gc | VF610_ADC_CAL, info->regs + ADC_GC);
>> +
>> +	timeout = wait_for_completion_interruptible_timeout
>> +			(&info->completion, VF610_ADC_TIMEOUT);
>
>This should probably not be interruptible.
>
Agree.

>> +	if (timeout == 0)
>> +		dev_err(info->dev, "Timeout for adc calibration\n");
>> +
>> +	adc_gc = readl(info->regs + ADC_GS);
>> +	if (adc_gc & VF610_ADC_CALF)
>> +		dev_err(info->dev, "ADC calibration failed\n");
>> +
>> +	info->adc_feature.calibration = false; }
>> +
>> +static inline void vf610_adc_cfg_set(struct vf610_adc *info)
>
>I'd drop the inline

Agree, thanks!
>
>> +{
>[...]
>> +}
>
>> +static inline int vf610_adc_read_data(struct vf610_adc *info)
>same here

Ok, thanks.

>> +{
>[...]
>> +}
>> +
>[...]
>> +static int vf610_read_raw(struct iio_dev *indio_dev,
>> +			struct iio_chan_spec const *chan,
>> +			int *val,
>> +			int *val2,
>> +			long mask)
>> +{
>> +	struct vf610_adc *info = iio_priv(indio_dev);
>> +	unsigned int hc_cfg;
>> +	unsigned long timeout;
>> +
>> +	/* Check for invalid channel */
>> +	if (chan->channel > VF610_ADC_MAX_CHANS_NUM)
>> +		return -EINVAL;
>> +
>> +	switch (mask) {
>> +	case IIO_CHAN_INFO_RAW:
>> +		mutex_lock(&indio_dev->mlock);
>> +
>
>putting a reinit_completion() here would make sense

Agree it , thanks!
>
>> +		hc_cfg = VF610_ADC_ADCHC(chan->channel);
>> +		hc_cfg |= VF610_ADC_AIEN;
>> +		writel(hc_cfg, info->regs + ADC_HC0);
>> +		timeout = wait_for_completion_interruptible_timeout
>> +				(&info->completion, VF610_ADC_TIMEOUT);
>> +		*val = info->value;
>> +
>> +		mutex_unlock(&indio_dev->mlock);
>> +
>> +		if (timeout == 0)
>> +			return -ETIMEDOUT;
>
>timeout can be negative if the thread was interrupted. In that case return the
>error code in timeout.

Agree, thanks!

>
>> +
>> +		return IIO_VAL_INT;
>> +
>> +	case IIO_CHAN_INFO_SCALE:
>> +		*val = info->vref_uv >> info->adc_feature.res_mode;
>> +		*val2 = 0;
>> +		return IIO_VAL_INT_PLUS_MICRO;
>
>It's better to use:
>		*val = info->vref_uv / 1000;
>		*val2 = info->adc_feature.res_mode;
>		return IIO_VAL_FACTIONAL_LOG2;
>
Thanks for your suggestion.


>> +	default:
>> +		break;
>> +	}
>> +
>> +	return -EINVAL;
>> +}
>[...]
>> +static int vf610_adc_probe(struct platform_device *pdev) {
>> +	struct vf610_adc *info = NULL;
>> +	struct iio_dev *indio_dev;
>> +	struct resource *mem;
>> +	int ret = -ENODEV;
>> +
>> +	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(struct vf610_adc));
>> +	if (!indio_dev) {
>> +		dev_err(&pdev->dev, "Failed allocating iio device\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	info = iio_priv(indio_dev);
>> +	info->dev = &pdev->dev;
>> +
>> +	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	info->regs = devm_ioremap_resource(&pdev->dev, mem);
>> +	if (IS_ERR(info->regs))
>> +		return PTR_ERR(info->regs);
>> +
>> +	info->irq = platform_get_irq(pdev, 0);
>> +	if (info->irq < 0) {
>
>0 isn't a valid irq number so this should be "irq <= 0"

Greet. Thanks.
>
>> +		dev_err(&pdev->dev, "no irq resource?\n");
>> +		return -EINVAL;
>> +	}
>> +
>[...]
>> +	vf610_adc_cfg_of_init(info);
>> +	vf610_adc_hw_init(info);
>
>This should probably be done before registering the device. Same goes for the
>clk_enable() call.
>
Got it, thanks!

>> +
>> +	return 0;
>> +}

Lars-Peter,  thanks for your review.

Andy

Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver

[Lars-Peter Clausen]

On 11/27/2013 05:44 AM, Fugang Duan wrote:
[...]
>>> +	if (info->vref)
>>> +		info->vref_uv = regulator_get_voltage(info->vref);
>>> +	else if (of_property_read_u32(np, "fsl,adc-vref", &info->vref_uv))
>>> +		dev_err(info->dev,
>>> +			"Miss adc-vref property or vref regulator in the DT.\n");
>>
>> If you have a fixed reference voltage use the fixed voltage regulator. Don't
>> invent custom ways of specifying this.
>>
> Different boards design may use different method to supply the reference voltage.
> There maybe fixed voltage regulator, maybe no regulator just use fixed voltage.

Well the voltage has to come from somewhere and that source should be
described in the DT with a fixed regulator.

> 
>>> +
>>> +	if (of_property_read_u32(np, "fsl,adc-clk-div",
>>> +			&info->adc_feature.clk_div_num)) {
>>> +		info->adc_feature.clk_div_num = 2;
>>> +		dev_info(info->dev,
>>> +			"Miss adc-clk-div in dt, set divider to 2.\n");
>>> +	}
>>> +
>>> +	if (of_property_read_u32(np, "fsl,adc-res",
>>> +			&info->adc_feature.res_mode)) {
>>> +		info->adc_feature.res_mode = 12;
>>> +		dev_info(info->dev,
>>> +			"Miss adc-res property in dt, use 8bit mode.\n");
>>> +	}
>>> +
>>> +	if (of_property_read_u32(np, "fsl,adc-sam-time",
>>> +			&info->adc_feature.sam_time)) {
>>> +		info->adc_feature.sam_time = 4;
>>> +		dev_info(info->dev,
>>> +			"Miss adc-sam-time property in dt, set to 4.\n");
>>> +	}
>>> +
>>> +	info->adc_feature.hw_average = of_property_read_bool(np,
>>> +					"fsl,adc-hw-aver-en");
>>> +	if (info->adc_feature.hw_average &&
>>> +		 of_property_read_u32(np, "fsl,adc-aver-sam-sel",
>>> +				&info->adc_feature.hw_sam)) {
>>> +		info->adc_feature.hw_sam = 4;
>>> +		dev_info(info->dev,
>>> +			"Miss adc-aver-sam-sel property in dt, set to 4.\n");
>>> +	}
>>> +
>>> +	info->adc_feature.lpm = of_property_read_bool(np, "fsl,adc-low-power-
>> mode");
>>> +	info->adc_feature.hs_oper = of_property_read_bool(np,
>>> +"fsl,adc-high-speed-conv");
>>
>> Some of the properties look like they should be runtime configurable rather
>> then board specific settings. E.g. the resolution or the sampling frequency, or
>> whether averaging is enabled.
>>
>>
> So, I have question:
> Since the ADC have many user configurable setting, I have import some of them from DT, and some of them use static define.
> How to set/change them in runtime for user configuration ? 
> Introduce ioctl ?

I think a lot of them already map to standard iio properties. E.g. the
samplerate. So you'd expose them as sysfs attributes. But please use
standard attributes here instead of inventing your own with cryptic names.

- Lars

RE: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver

[Fugang Duan]

From: Lars-Peter Clausen [mailto:lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org]
Data: Wednesday, November 27, 2013 4:21 PM

>To: Duan Fugang-B38611
>Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
>shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Li Frank-B20596; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>Subject: Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver
>
>On 11/27/2013 05:44 AM, Fugang Duan wrote:
>[...]
>>>> +	if (info->vref)
>>>> +		info->vref_uv = regulator_get_voltage(info->vref);
>>>> +	else if (of_property_read_u32(np, "fsl,adc-vref", &info->vref_uv))
>>>> +		dev_err(info->dev,
>>>> +			"Miss adc-vref property or vref regulator in the DT.\n");
>>>
>>> If you have a fixed reference voltage use the fixed voltage
>>> regulator. Don't invent custom ways of specifying this.
>>>
>> Different boards design may use different method to supply the reference
>voltage.
>> There maybe fixed voltage regulator, maybe no regulator just use fixed
>voltage.
>
>Well the voltage has to come from somewhere and that source should be described
>in the DT with a fixed regulator.
>

Good suggestion. Thanks.

>>
>>>> +
>>>> +	if (of_property_read_u32(np, "fsl,adc-clk-div",
>>>> +			&info->adc_feature.clk_div_num)) {
>>>> +		info->adc_feature.clk_div_num = 2;
>>>> +		dev_info(info->dev,
>>>> +			"Miss adc-clk-div in dt, set divider to 2.\n");
>>>> +	}
>>>> +
>>>> +	if (of_property_read_u32(np, "fsl,adc-res",
>>>> +			&info->adc_feature.res_mode)) {
>>>> +		info->adc_feature.res_mode = 12;
>>>> +		dev_info(info->dev,
>>>> +			"Miss adc-res property in dt, use 8bit mode.\n");
>>>> +	}
>>>> +
>>>> +	if (of_property_read_u32(np, "fsl,adc-sam-time",
>>>> +			&info->adc_feature.sam_time)) {
>>>> +		info->adc_feature.sam_time = 4;
>>>> +		dev_info(info->dev,
>>>> +			"Miss adc-sam-time property in dt, set to 4.\n");
>>>> +	}
>>>> +
>>>> +	info->adc_feature.hw_average = of_property_read_bool(np,
>>>> +					"fsl,adc-hw-aver-en");
>>>> +	if (info->adc_feature.hw_average &&
>>>> +		 of_property_read_u32(np, "fsl,adc-aver-sam-sel",
>>>> +				&info->adc_feature.hw_sam)) {
>>>> +		info->adc_feature.hw_sam = 4;
>>>> +		dev_info(info->dev,
>>>> +			"Miss adc-aver-sam-sel property in dt, set to 4.\n");
>>>> +	}
>>>> +
>>>> +	info->adc_feature.lpm = of_property_read_bool(np,
>>>> +"fsl,adc-low-power-
>>> mode");
>>>> +	info->adc_feature.hs_oper = of_property_read_bool(np,
>>>> +"fsl,adc-high-speed-conv");
>>>
>>> Some of the properties look like they should be runtime configurable
>>> rather then board specific settings. E.g. the resolution or the
>>> sampling frequency, or whether averaging is enabled.
>>>
>>>
>> So, I have question:
>> Since the ADC have many user configurable setting, I have import some of them
>from DT, and some of them use static define.
>> How to set/change them in runtime for user configuration ?
>> Introduce ioctl ?
>
>I think a lot of them already map to standard iio properties. E.g. the
>samplerate. So you'd expose them as sysfs attributes. But please use standard
>attributes here instead of inventing your own with cryptic names.
>
>- Lars

Good suggestion, I will use the standard IIO interface for the ADC configuration.

Thanks,
Andy

Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver

[Mark Rutland]

On Tue, Nov 26, 2013 at 10:56:33AM +0000, Fugang Duan wrote:
> Add Freescale Vybrid vf610 adc driver. The driver only support
> ADC software trigger.
>
> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  drivers/iio/adc/Kconfig     |   11 +
>  drivers/iio/adc/Makefile    |    1 +
>  drivers/iio/adc/vf610_adc.c |  744 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 756 insertions(+), 0 deletions(-)

[...]

> +static void vf610_adc_cfg_of_init(struct vf610_adc *info)
> +{
> +       struct device_node *np = info->dev->of_node;
> +
> +       /*
> +        * set default Configuration for ADC controller
> +        * This config may upgrade to require from DT
> +        */
> +       info->adc_feature.clk_sel = ADCIOC_BUSCLK_SET;
> +       info->adc_feature.vol_ref = VF610_ADCIOC_VR_VREF_SET;
> +       info->adc_feature.calibration = true;
> +       info->adc_feature.tri_hw = false;
> +       info->adc_feature.dataov_en = true;
> +       info->adc_feature.ac_clk_en = false;
> +       info->adc_feature.dma_en = false;
> +       info->adc_feature.cc_en = false;
> +       info->adc_feature.cmp_func_en = false;
> +       info->adc_feature.cmp_range_en = false;
> +       info->adc_feature.cmp_greater_en = false;
> +
> +       if (of_property_read_u32(np, "fsl,adc-io-pinctl",
> +                       &info->adc_feature.pctl)) {
> +               info->adc_feature.pctl = VF610_ADC_IOPCTL5;
> +               dev_info(info->dev,
> +                       "Miss adc-io-pinctl in dt, enable pin SE5.\n");
> +       }

Could you elaborate on what this means? What is this doing when pinctrl
is not specified in the DT?

> +
> +       if (info->vref)
> +               info->vref_uv = regulator_get_voltage(info->vref);
> +       else if (of_property_read_u32(np, "fsl,adc-vref", &info->vref_uv))
> +               dev_err(info->dev,
> +                       "Miss adc-vref property or vref regulator in the DT.\n");

Query the regulator instead.

> +
> +       if (of_property_read_u32(np, "fsl,adc-clk-div",
> +                       &info->adc_feature.clk_div_num)) {
> +               info->adc_feature.clk_div_num = 2;
> +               dev_info(info->dev,
> +                       "Miss adc-clk-div in dt, set divider to 2.\n");
> +       }

Why do you need this?

I'd expect the use to request a frequency they wanted to sample at, and
the driver would try its best by fiddling with this and the clock input.

> +
> +       if (of_property_read_u32(np, "fsl,adc-res",
> +                       &info->adc_feature.res_mode)) {
> +               info->adc_feature.res_mode = 12;
> +               dev_info(info->dev,
> +                       "Miss adc-res property in dt, use 8bit mode.\n");

This message does not seem to match the code above it.

Is this a hardware limitation in some instances? Or is it always
possible to select any of the resolutions?

> +
> +       if (of_property_read_u32(np, "fsl,adc-sam-time",
> +                       &info->adc_feature.sam_time)) {
> +               info->adc_feature.sam_time = 4;
> +               dev_info(info->dev,
> +                       "Miss adc-sam-time property in dt, set to 4.\n");
> +       }

This looks like it should be handled at runtime.

> +
> +       info->adc_feature.hw_average = of_property_read_bool(np,
> +                                       "fsl,adc-hw-aver-en");

Is hw average support not always present?

> +       if (info->adc_feature.hw_average &&
> +                of_property_read_u32(np, "fsl,adc-aver-sam-sel",
> +                               &info->adc_feature.hw_sam)) {
> +               info->adc_feature.hw_sam = 4;
> +               dev_info(info->dev,
> +                       "Miss adc-aver-sam-sel property in dt, set to 4.\n");
> +       }

Why not configure this at runtime instead?

> +
> +       info->adc_feature.lpm = of_property_read_bool(np, "fsl,adc-low-power-mode");
> +       info->adc_feature.hs_oper = of_property_read_bool(np, "fsl,adc-high-speed-conv");

Please elaborate on what these mean, and why they need to be in the DT.

Thanks,
Mark.

RE: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver

[Fugang Duan]

From: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
Data: Tuesday, November 26, 2013 10:26 PM

>To: Duan Fugang-B38611
>Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
>shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Li Frank-B20596; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>Subject: Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver
>
>On Tue, Nov 26, 2013 at 10:56:33AM +0000, Fugang Duan wrote:
>> Add Freescale Vybrid vf610 adc driver. The driver only support ADC
>> software trigger.
>>
>> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> ---
>>  drivers/iio/adc/Kconfig     |   11 +
>>  drivers/iio/adc/Makefile    |    1 +
>>  drivers/iio/adc/vf610_adc.c |  744
>> +++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 756 insertions(+), 0 deletions(-)
>
>[...]
>
>> +static void vf610_adc_cfg_of_init(struct vf610_adc *info) {
>> +       struct device_node *np = info->dev->of_node;
>> +
>> +       /*
>> +        * set default Configuration for ADC controller
>> +        * This config may upgrade to require from DT
>> +        */
>> +       info->adc_feature.clk_sel = ADCIOC_BUSCLK_SET;
>> +       info->adc_feature.vol_ref = VF610_ADCIOC_VR_VREF_SET;
>> +       info->adc_feature.calibration = true;
>> +       info->adc_feature.tri_hw = false;
>> +       info->adc_feature.dataov_en = true;
>> +       info->adc_feature.ac_clk_en = false;
>> +       info->adc_feature.dma_en = false;
>> +       info->adc_feature.cc_en = false;
>> +       info->adc_feature.cmp_func_en = false;
>> +       info->adc_feature.cmp_range_en = false;
>> +       info->adc_feature.cmp_greater_en = false;
>> +
>> +       if (of_property_read_u32(np, "fsl,adc-io-pinctl",
>> +                       &info->adc_feature.pctl)) {
>> +               info->adc_feature.pctl = VF610_ADC_IOPCTL5;
>> +               dev_info(info->dev,
>> +                       "Miss adc-io-pinctl in dt, enable pin SE5.\n");
>> +       }
>
>Could you elaborate on what this means? What is this doing when pinctrl is not
>specified in the DT?
>

As vf610 reference manually describe ADC pin control:

37.8.2 Input Select and Pin Control
The pin control registers (ADC_PC) disable the I/O port control of the pins used as
analog inputs. When a pin control register bit is set, the following conditions are forced
for the associated MCU pin:
* The output buffer is forced to its high impedance state.
* The input buffer is disabled. A read of the I/O port returns a zero for any pin with its
input buffer disabled.
* The pullup is disabled. 

So, when the relative ADC pin is used as Sliding rheostat test, disable the pin control.


>> +
>> +       if (info->vref)
>> +               info->vref_uv = regulator_get_voltage(info->vref);
>> +       else if (of_property_read_u32(np, "fsl,adc-vref", &info->vref_uv))
>> +               dev_err(info->dev,
>> +                       "Miss adc-vref property or vref regulator in
>> + the DT.\n");
>
>Query the regulator instead.

When the ADC reference voltage is not controlled by regulator, just by fixed voltage from MCU digital power,
We get the reference voltage from DT.
  
>
>> +
>> +       if (of_property_read_u32(np, "fsl,adc-clk-div",
>> +                       &info->adc_feature.clk_div_num)) {
>> +               info->adc_feature.clk_div_num = 2;
>> +               dev_info(info->dev,
>> +                       "Miss adc-clk-div in dt, set divider to 2.\n");
>> +       }
>
>Why do you need this?
>
>I'd expect the use to request a frequency they wanted to sample at, and the
>driver would try its best by fiddling with this and the clock input.
>
>> +
>> +       if (of_property_read_u32(np, "fsl,adc-res",
>> +                       &info->adc_feature.res_mode)) {
>> +               info->adc_feature.res_mode = 12;
>> +               dev_info(info->dev,
>> +                       "Miss adc-res property in dt, use 8bit
>> + mode.\n");
>
>This message does not seem to match the code above it.
>
>Is this a hardware limitation in some instances? Or is it always possible to
>select any of the resolutions?

No limitation, just mistake for it. I have tested 8-bit, 10-bit, 12-bit mode, all pass.
>
>> +
>> +       if (of_property_read_u32(np, "fsl,adc-sam-time",
>> +                       &info->adc_feature.sam_time)) {
>> +               info->adc_feature.sam_time = 4;
>> +               dev_info(info->dev,
>> +                       "Miss adc-sam-time property in dt, set to 4.\n");
>> +       }
>
>This looks like it should be handled at runtime.

Driver can get the ADC sample time duration fro DT.
fsl,adc-sam-time: ADC sample time duration, number of ADC clocks, such as 2, 4, 6, 8, 12, 16, 20, 24

How to handled at runtime ? 
Use ioctl ?


For the ADC, there have many setting, some of setting are defined statically,  some of them get from DT.
If let the setting handled at runtime, it need to introduce ioctl interface for the driver.

>
>> +
>> +       info->adc_feature.hw_average = of_property_read_bool(np,
>> +                                       "fsl,adc-hw-aver-en");
>
>Is hw average support not always present?

Vf610 RM describe:
37.8.5.7 Hardware Average Function
After the selected input is sampled and converted, the result is placed in an accumulator
from which an average is calculated once the selected number of conversions has been
completed. When hardware averaging is selected the completion of a single conversion
will not set the COCOn bit.

In general, hw average function is useful to reduce cpu loading.
The function is optional for user.

>
>> +       if (info->adc_feature.hw_average &&
>> +                of_property_read_u32(np, "fsl,adc-aver-sam-sel",
>> +                               &info->adc_feature.hw_sam)) {
>> +               info->adc_feature.hw_sam = 4;
>> +               dev_info(info->dev,
>> +                       "Miss adc-aver-sam-sel property in dt, set to 4.\n");
>> +       }
>
>Why not configure this at runtime instead?
>
As above said, if let the configuration set at runtime, we need to introduce ioctl interface for user.
How do you think ?


>> +
>> +       info->adc_feature.lpm = of_property_read_bool(np, "fsl,adc-low-power-
>mode");
>> +       info->adc_feature.hs_oper = of_property_read_bool(np,
>> + "fsl,adc-high-speed-conv");
>
>Please elaborate on what these mean, and why they need to be in the DT.
>
Yes, there have some configuration have been set in default,  some of them from DT.
I think the hw function/feature enable/disable are judged from DT.
Some general configurations are set in default.

Of course, let all of them are set at runtime is better. But need to introduce ioctl interface for user. 


Mark, 

Thanks for your review.

Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver

[Mark Rutland]

On Wed, Nov 27, 2013 at 05:37:24AM +0000, Fugang Duan wrote:
> From: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
> Data: Tuesday, November 26, 2013 10:26 PM
> 
> >To: Duan Fugang-B38611
> >Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> >shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Li Frank-B20596; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >Subject: Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver
> >
> >On Tue, Nov 26, 2013 at 10:56:33AM +0000, Fugang Duan wrote:
> >> Add Freescale Vybrid vf610 adc driver. The driver only support ADC
> >> software trigger.
> >>
> >> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >> ---
> >>  drivers/iio/adc/Kconfig     |   11 +
> >>  drivers/iio/adc/Makefile    |    1 +
> >>  drivers/iio/adc/vf610_adc.c |  744
> >> +++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 756 insertions(+), 0 deletions(-)
> >
> >[...]
> >
> >> +static void vf610_adc_cfg_of_init(struct vf610_adc *info) {
> >> +       struct device_node *np = info->dev->of_node;
> >> +
> >> +       /*
> >> +        * set default Configuration for ADC controller
> >> +        * This config may upgrade to require from DT
> >> +        */
> >> +       info->adc_feature.clk_sel = ADCIOC_BUSCLK_SET;
> >> +       info->adc_feature.vol_ref = VF610_ADCIOC_VR_VREF_SET;
> >> +       info->adc_feature.calibration = true;
> >> +       info->adc_feature.tri_hw = false;
> >> +       info->adc_feature.dataov_en = true;
> >> +       info->adc_feature.ac_clk_en = false;
> >> +       info->adc_feature.dma_en = false;
> >> +       info->adc_feature.cc_en = false;
> >> +       info->adc_feature.cmp_func_en = false;
> >> +       info->adc_feature.cmp_range_en = false;
> >> +       info->adc_feature.cmp_greater_en = false;
> >> +
> >> +       if (of_property_read_u32(np, "fsl,adc-io-pinctl",
> >> +                       &info->adc_feature.pctl)) {
> >> +               info->adc_feature.pctl = VF610_ADC_IOPCTL5;
> >> +               dev_info(info->dev,
> >> +                       "Miss adc-io-pinctl in dt, enable pin SE5.\n");
> >> +       }
> >
> >Could you elaborate on what this means? What is this doing when pinctrl is not
> >specified in the DT?
> >
> 
> As vf610 reference manually describe ADC pin control:
> 
> 37.8.2 Input Select and Pin Control
> The pin control registers (ADC_PC) disable the I/O port control of the pins used as
> analog inputs. When a pin control register bit is set, the following conditions are forced
> for the associated MCU pin:
> * The output buffer is forced to its high impedance state.
> * The input buffer is disabled. A read of the I/O port returns a zero for any pin with its
> input buffer disabled.
> * The pullup is disabled. 
> 
> So, when the relative ADC pin is used as Sliding rheostat test, disable the pin control.
> 
> 
> >> +
> >> +       if (info->vref)
> >> +               info->vref_uv = regulator_get_voltage(info->vref);
> >> +       else if (of_property_read_u32(np, "fsl,adc-vref", &info->vref_uv))
> >> +               dev_err(info->dev,
> >> +                       "Miss adc-vref property or vref regulator in
> >> + the DT.\n");
> >
> >Query the regulator instead.
> 
> When the ADC reference voltage is not controlled by regulator, just by
> fixed voltage from MCU digital power, We get the reference voltage
> from DT.

You could use a fixed-regulator in that case in the DT, and query the
voltage through the regulator framework. There is no need for this
property.

>   
> >
> >> +
> >> +       if (of_property_read_u32(np, "fsl,adc-clk-div",
> >> +                       &info->adc_feature.clk_div_num)) {
> >> +               info->adc_feature.clk_div_num = 2;
> >> +               dev_info(info->dev,
> >> +                       "Miss adc-clk-div in dt, set divider to 2.\n");
> >> +       }
> >
> >Why do you need this?
> >
> >I'd expect the use to request a frequency they wanted to sample at, and the
> >driver would try its best by fiddling with this and the clock input.
> >
> >> +
> >> +       if (of_property_read_u32(np, "fsl,adc-res",
> >> +                       &info->adc_feature.res_mode)) {
> >> +               info->adc_feature.res_mode = 12;
> >> +               dev_info(info->dev,
> >> +                       "Miss adc-res property in dt, use 8bit
> >> + mode.\n");
> >
> >This message does not seem to match the code above it.
> >
> >Is this a hardware limitation in some instances? Or is it always possible to
> >select any of the resolutions?
> 
> No limitation, just mistake for it. I have tested 8-bit, 10-bit, 12-bit mode, all pass.

Ok. So this is not necessary?

> >
> >> +
> >> +       if (of_property_read_u32(np, "fsl,adc-sam-time",
> >> +                       &info->adc_feature.sam_time)) {
> >> +               info->adc_feature.sam_time = 4;
> >> +               dev_info(info->dev,
> >> +                       "Miss adc-sam-time property in dt, set to 4.\n");
> >> +       }
> >
> >This looks like it should be handled at runtime.
> 
> Driver can get the ADC sample time duration fro DT.
> fsl,adc-sam-time: ADC sample time duration, number of ADC clocks, such
> as 2, 4, 6, 8, 12, 16, 20, 24

If this is not a fixed property of the hardware, and there isn't some
limitation such that one of these values _must_ be used, I do not
believe it should be in the device tree.

Just because it is easier to put it there doesn't make putting it there
correct.

> 
> How to handled at runtime ? 
> Use ioctl ?

Something of that sort. Hopefully someone more familiar with the
subsystem can describe what's preferred.

> For the ADC, there have many setting, some of setting are defined
> statically,  some of them get from DT.  If let the setting handled at
> runtime, it need to introduce ioctl interface for the driver.

If it's a static property of the HW that cannot be probed, it should be
in the DT. If it's a limitation of the HW, that should be in the DT.

If it's an arbitrary value that does not have a significant effect on
performance or correctness, I do not see why it should be in the DT.

As far as I can see, this does not belong in the DT. The fact that you'd
need an ioctl to configure it dynamically is an orthogonal issue.

> 
> >
> >> +
> >> +       info->adc_feature.hw_average = of_property_read_bool(np,
> >> +                                       "fsl,adc-hw-aver-en");
> >
> >Is hw average support not always present?
> 
> Vf610 RM describe:
> 37.8.5.7 Hardware Average Function
> After the selected input is sampled and converted, the result is placed in an accumulator
> from which an average is calculated once the selected number of conversions has been
> completed. When hardware averaging is selected the completion of a single conversion
> will not set the COCOn bit.
> 
> In general, hw average function is useful to reduce cpu loading.
> The function is optional for user.

Ok. If this is always possible to use, as your description implies, then
I do not see why this needs to be in the DT. Whether you choose to
enable or disable it in the driver by default, or whether you expose an
interface to userspace to configure this is an orthogonal issue.

> 
> >
> >> +       if (info->adc_feature.hw_average &&
> >> +                of_property_read_u32(np, "fsl,adc-aver-sam-sel",
> >> +                               &info->adc_feature.hw_sam)) {
> >> +               info->adc_feature.hw_sam = 4;
> >> +               dev_info(info->dev,
> >> +                       "Miss adc-aver-sam-sel property in dt, set to 4.\n");
> >> +       }
> >
> >Why not configure this at runtime instead?
> >
> As above said, if let the configuration set at runtime, we need to
> introduce ioctl interface for user.
> How do you think ?

An ioctl or sysfs interface sounds like the correct solution. I don't
have any strong opinion on the particular interface used, but DT is not
the right place to expose this configuration option.

> 
> 
> >> +
> >> +       info->adc_feature.lpm = of_property_read_bool(np, "fsl,adc-low-power-
> >mode");
> >> +       info->adc_feature.hs_oper = of_property_read_bool(np,
> >> + "fsl,adc-high-speed-conv");
> >
> >Please elaborate on what these mean, and why they need to be in the DT.
> >
> Yes, there have some configuration have been set in default,  some of them from DT.
> I think the hw function/feature enable/disable are judged from DT.
> Some general configurations are set in default.

Sorry, but that does not answer my questions:

* What do these two properties mean?
* Why must they be in the DT?

As far as I can tell, they do not need to be in the DT.

> 
> Of course, let all of them are set at runtime is better. But need to
> introduce ioctl interface for user. 

I don't see this as an argument for placing these in the DT. Describe
the hardware in the DT. Anything that is expected to be changed by the
user is clearly not a property of the hardware and should not be in the
DT.

There are grey areas with some devices where a specific configuration is
far more optimal for a given platform (e.g. reserving specific portions
of memory for frame buffers). As far as I can see, none of the
properties proposed in this patch fall into that area, and sane defaults
can be selected for all of them which could later be overridden with a
userspace interface of some sort.

Thanks,
Mark.

RE: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver

[Fugang Duan]

From: Mark Rutland [mailto:mark.rutland-5wv7dgnIgG8@public.gmane.org]
Data: Wednesday, November 27, 2013 10:18 PM

>To: Duan Fugang-B38611
>Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
>shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Li Frank-B20596; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>Subject: Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver
>
>On Wed, Nov 27, 2013 at 05:37:24AM +0000, Fugang Duan wrote:
>> From: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>
>> Data: Tuesday, November 26, 2013 10:26 PM
>>
>> >To: Duan Fugang-B38611
>> >Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org;
>> >devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Li Frank-B20596;
>> >linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> >Subject: Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610
>> >adc driver
>> >
>> >On Tue, Nov 26, 2013 at 10:56:33AM +0000, Fugang Duan wrote:
>> >> Add Freescale Vybrid vf610 adc driver. The driver only support ADC
>> >> software trigger.
>> >>
>> >> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> >> ---
>> >>  drivers/iio/adc/Kconfig     |   11 +
>> >>  drivers/iio/adc/Makefile    |    1 +
>> >>  drivers/iio/adc/vf610_adc.c |  744
>> >> +++++++++++++++++++++++++++++++++++++++++++
>> >>  3 files changed, 756 insertions(+), 0 deletions(-)
>> >
>> >[...]
>> >
>> >> +static void vf610_adc_cfg_of_init(struct vf610_adc *info) {
>> >> +       struct device_node *np = info->dev->of_node;
>> >> +
>> >> +       /*
>> >> +        * set default Configuration for ADC controller
>> >> +        * This config may upgrade to require from DT
>> >> +        */
>> >> +       info->adc_feature.clk_sel = ADCIOC_BUSCLK_SET;
>> >> +       info->adc_feature.vol_ref = VF610_ADCIOC_VR_VREF_SET;
>> >> +       info->adc_feature.calibration = true;
>> >> +       info->adc_feature.tri_hw = false;
>> >> +       info->adc_feature.dataov_en = true;
>> >> +       info->adc_feature.ac_clk_en = false;
>> >> +       info->adc_feature.dma_en = false;
>> >> +       info->adc_feature.cc_en = false;
>> >> +       info->adc_feature.cmp_func_en = false;
>> >> +       info->adc_feature.cmp_range_en = false;
>> >> +       info->adc_feature.cmp_greater_en = false;
>> >> +
>> >> +       if (of_property_read_u32(np, "fsl,adc-io-pinctl",
>> >> +                       &info->adc_feature.pctl)) {
>> >> +               info->adc_feature.pctl = VF610_ADC_IOPCTL5;
>> >> +               dev_info(info->dev,
>> >> +                       "Miss adc-io-pinctl in dt, enable pin SE5.\n");
>> >> +       }
>> >
>> >Could you elaborate on what this means? What is this doing when
>> >pinctrl is not specified in the DT?
>> >
>>
>> As vf610 reference manually describe ADC pin control:
>>
>> 37.8.2 Input Select and Pin Control
>> The pin control registers (ADC_PC) disable the I/O port control of the
>> pins used as analog inputs. When a pin control register bit is set,
>> the following conditions are forced for the associated MCU pin:
>> * The output buffer is forced to its high impedance state.
>> * The input buffer is disabled. A read of the I/O port returns a zero
>> for any pin with its input buffer disabled.
>> * The pullup is disabled.
>>
>> So, when the relative ADC pin is used as Sliding rheostat test, disable the
>pin control.
>>
>>
>> >> +
>> >> +       if (info->vref)
>> >> +               info->vref_uv = regulator_get_voltage(info->vref);
>> >> +       else if (of_property_read_u32(np, "fsl,adc-vref", &info->vref_uv))
>> >> +               dev_err(info->dev,
>> >> +                       "Miss adc-vref property or vref regulator
>> >> + in the DT.\n");
>> >
>> >Query the regulator instead.
>>
>> When the ADC reference voltage is not controlled by regulator, just by
>> fixed voltage from MCU digital power, We get the reference voltage
>> from DT.
>
>You could use a fixed-regulator in that case in the DT, and query the voltage
>through the regulator framework. There is no need for this property.
>
>>
>> >
>> >> +
>> >> +       if (of_property_read_u32(np, "fsl,adc-clk-div",
>> >> +                       &info->adc_feature.clk_div_num)) {
>> >> +               info->adc_feature.clk_div_num = 2;
>> >> +               dev_info(info->dev,
>> >> +                       "Miss adc-clk-div in dt, set divider to 2.\n");
>> >> +       }
>> >
>> >Why do you need this?
>> >
>> >I'd expect the use to request a frequency they wanted to sample at,
>> >and the driver would try its best by fiddling with this and the clock input.
>> >
>> >> +
>> >> +       if (of_property_read_u32(np, "fsl,adc-res",
>> >> +                       &info->adc_feature.res_mode)) {
>> >> +               info->adc_feature.res_mode = 12;
>> >> +               dev_info(info->dev,
>> >> +                       "Miss adc-res property in dt, use 8bit
>> >> + mode.\n");
>> >
>> >This message does not seem to match the code above it.
>> >
>> >Is this a hardware limitation in some instances? Or is it always
>> >possible to select any of the resolutions?
>>
>> No limitation, just mistake for it. I have tested 8-bit, 10-bit, 12-bit mode,
>all pass.
>
>Ok. So this is not necessary?
>
>> >
>> >> +
>> >> +       if (of_property_read_u32(np, "fsl,adc-sam-time",
>> >> +                       &info->adc_feature.sam_time)) {
>> >> +               info->adc_feature.sam_time = 4;
>> >> +               dev_info(info->dev,
>> >> +                       "Miss adc-sam-time property in dt, set to 4.\n");
>> >> +       }
>> >
>> >This looks like it should be handled at runtime.
>>
>> Driver can get the ADC sample time duration fro DT.
>> fsl,adc-sam-time: ADC sample time duration, number of ADC clocks, such
>> as 2, 4, 6, 8, 12, 16, 20, 24
>
>If this is not a fixed property of the hardware, and there isn't some
>limitation such that one of these values _must_ be used, I do not believe it
>should be in the device tree.
>
>Just because it is easier to put it there doesn't make putting it there correct.
>
>>
>> How to handled at runtime ?
>> Use ioctl ?
>
>Something of that sort. Hopefully someone more familiar with the subsystem can
>describe what's preferred.
>
>> For the ADC, there have many setting, some of setting are defined
>> statically,  some of them get from DT.  If let the setting handled at
>> runtime, it need to introduce ioctl interface for the driver.
>
>If it's a static property of the HW that cannot be probed, it should be in the
>DT. If it's a limitation of the HW, that should be in the DT.
>
>If it's an arbitrary value that does not have a significant effect on
>performance or correctness, I do not see why it should be in the DT.
>
>As far as I can see, this does not belong in the DT. The fact that you'd need
>an ioctl to configure it dynamically is an orthogonal issue.
>
>>
>> >
>> >> +
>> >> +       info->adc_feature.hw_average = of_property_read_bool(np,
>> >> +                                       "fsl,adc-hw-aver-en");
>> >
>> >Is hw average support not always present?
>>
>> Vf610 RM describe:
>> 37.8.5.7 Hardware Average Function
>> After the selected input is sampled and converted, the result is
>> placed in an accumulator from which an average is calculated once the
>> selected number of conversions has been completed. When hardware
>> averaging is selected the completion of a single conversion will not set the
>COCOn bit.
>>
>> In general, hw average function is useful to reduce cpu loading.
>> The function is optional for user.
>
>Ok. If this is always possible to use, as your description implies, then I do
>not see why this needs to be in the DT. Whether you choose to enable or disable
>it in the driver by default, or whether you expose an interface to userspace to
>configure this is an orthogonal issue.
>
>>
>> >
>> >> +       if (info->adc_feature.hw_average &&
>> >> +                of_property_read_u32(np, "fsl,adc-aver-sam-sel",
>> >> +                               &info->adc_feature.hw_sam)) {
>> >> +               info->adc_feature.hw_sam = 4;
>> >> +               dev_info(info->dev,
>> >> +                       "Miss adc-aver-sam-sel property in dt, set to
>4.\n");
>> >> +       }
>> >
>> >Why not configure this at runtime instead?
>> >
>> As above said, if let the configuration set at runtime, we need to
>> introduce ioctl interface for user.
>> How do you think ?
>
>An ioctl or sysfs interface sounds like the correct solution. I don't have any
>strong opinion on the particular interface used, but DT is not the right place
>to expose this configuration option.
>
>>
>>
>> >> +
>> >> +       info->adc_feature.lpm = of_property_read_bool(np,
>> >> + "fsl,adc-low-power-
>> >mode");
>> >> +       info->adc_feature.hs_oper = of_property_read_bool(np,
>> >> + "fsl,adc-high-speed-conv");
>> >
>> >Please elaborate on what these mean, and why they need to be in the DT.
>> >
>> Yes, there have some configuration have been set in default,  some of them
>from DT.
>> I think the hw function/feature enable/disable are judged from DT.
>> Some general configurations are set in default.
>
>Sorry, but that does not answer my questions:
>
>* What do these two properties mean?
>* Why must they be in the DT?
>
>As far as I can tell, they do not need to be in the DT.
>
>>
>> Of course, let all of them are set at runtime is better. But need to
>> introduce ioctl interface for user.
>
>I don't see this as an argument for placing these in the DT. Describe the
>hardware in the DT. Anything that is expected to be changed by the user is
>clearly not a property of the hardware and should not be in the DT.
>
>There are grey areas with some devices where a specific configuration is far
>more optimal for a given platform (e.g. reserving specific portions of memory
>for frame buffers). As far as I can see, none of the properties proposed in
>this patch fall into that area, and sane defaults can be selected for all of
>them which could later be overridden with a userspace interface of some sort.
>
>Thanks,
>Mark.

Thanks, Mark.

Anyway, I need to re-architect the ADC configuration, as you suggestion, move some setting from DT, add it for runtime.
For iio subsystem, there have some sysfs interface for related ADC setting, but other runtime setting need ioctl interface.

Thanks very much!
Andy

[PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver

[Fugang Duan]

The patch adds the binding file for Freescale vf610 ADC driver.

Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 .../devicetree/bindings/iio/adc/vf610-adc.txt      |   57 ++++++++++++++++++++
 1 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
new file mode 100644
index 0000000..4101516
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
@@ -0,0 +1,57 @@
+Freescale vf610 Analog to Digital Converter bindings
+
+The devicetree bindings are for the new ADC driver written for
+vf610/i.MX6slx and upward SoCs from Freescale.
+
+Required properties:
+- compatible: Should be "fsl,vf610-adc"
+- reg: Offset and length of the register set for the device
+- interrupts: Should contain the interrupt for the device
+- clocks: The clocks needed by the ADC controller
+- clock-names: the name of the clocks
+
+Optional properties:
+- fsl,adc-io-pinctl: Enable field for the I/O port control of MCU pins used as analog inputs.
+- fsl,adc-vref: ADC refrence voltage value, unit is uV.
+- fsl,adc-clk-div: Current clock divider value, such as 1,2,4,8,16 and so on.
+- fsl,adc-res: ADC conversion mode selection, such as 8 for 8-bit, 10 for 10-bit, 12 for 12-bit mode.
+- fsl,adc-sam-time: ADC sample time duration, number of ADC clocks, such as 2, 4, 6, 8, 12, 16, 20, 24
+- fsl,adc-aver-sam-sel: Determines how many ADC conversions will be averaged to create the ADC average result.
+			The Optional value is 4, 8, 16, 32.
+- fsl,adc-hw-aver-en: Bool type to decide enable hardware average function.
+- fsl,adc-low-power-mode: Bool type to decide enable ADC low power mode.
+- fsl,adc-high-speed-conv: Bool type to decide enable ADC high speed mode.
+- vref: The regulator to support ADC refrence voltage.
+
+Example:
+adc0: adc@4003b000 {
+	compatible = "fsl,vf610-adc";
+	reg = <0x4003b000 0x1000>;
+	interrupts = <0 53 0x04>;
+	clocks = <&clks VF610_CLK_ADC0>;
+	clock-names = "adc";
+	status = "disabled";
+};
+
+adc1: adc@400bb000 {
+	compatible = "fsl,vf610-adc";
+	reg = <0x400bb000 0x1000>;
+	interrupts = <0 54 0x04>;
+	clocks = <&clks VF610_CLK_ADC1>;
+	clock-names = "adc";
+	status = "disabled";
+};
+
+&adc0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_adc0_ad5>;
+	fsl,adc-io-pinctl = <0x20>;
+	fsl,adc-vref = <3300000>;
+	fsl,adc-clk-div = <2>;
+	fsl,adc-res = <12>;
+	fsl,adc-sam-time = <4>;
+	fsl,adc-aver-sam-sel = <4>;
+	fsl,adc-hw-aver-en;
+	fsl,adc-low-power-mode;
+	status = "okay";
+};
-- 
1.7.2.rc3

Re: [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver

[Shawn Guo]

On Tue, Nov 26, 2013 at 06:56:34PM +0800, Fugang Duan wrote:
> +Optional properties:
> +- fsl,adc-io-pinctl: Enable field for the I/O port control of MCU pins used as analog inputs.
> +- fsl,adc-vref: ADC refrence voltage value, unit is uV.
> +- fsl,adc-clk-div: Current clock divider value, such as 1,2,4,8,16 and so on.
> +- fsl,adc-res: ADC conversion mode selection, such as 8 for 8-bit, 10 for 10-bit, 12 for 12-bit mode.
> +- fsl,adc-sam-time: ADC sample time duration, number of ADC clocks, such as 2, 4, 6, 8, 12, 16, 20, 24
> +- fsl,adc-aver-sam-sel: Determines how many ADC conversions will be averaged to create the ADC average result.
> +			The Optional value is 4, 8, 16, 32.
> +- fsl,adc-hw-aver-en: Bool type to decide enable hardware average function.
> +- fsl,adc-low-power-mode: Bool type to decide enable ADC low power mode.
> +- fsl,adc-high-speed-conv: Bool type to decide enable ADC high speed mode.
> +- vref: The regulator to support ADC refrence voltage.

Shouldn't it be vref-supply?

Shawn

RE: [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver

[Fugang Duan]

From: Shawn Guo [mailto:shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org]
Data: Tuesday, November 26, 2013 9:38 PM

>To: Duan Fugang-B38611
>Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Li
>Frank-B20596; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>Subject: Re: [PATCH v3 3/3] Documentation: add the binding file for Freescale
>vf610 ADC driver
>
>On Tue, Nov 26, 2013 at 06:56:34PM +0800, Fugang Duan wrote:
>> +Optional properties:
>> +- fsl,adc-io-pinctl: Enable field for the I/O port control of MCU pins used
>as analog inputs.
>> +- fsl,adc-vref: ADC refrence voltage value, unit is uV.
>> +- fsl,adc-clk-div: Current clock divider value, such as 1,2,4,8,16 and so on.
>> +- fsl,adc-res: ADC conversion mode selection, such as 8 for 8-bit, 10 for
>10-bit, 12 for 12-bit mode.
>> +- fsl,adc-sam-time: ADC sample time duration, number of ADC clocks, such as
>2, 4, 6, 8, 12, 16, 20, 24
>> +- fsl,adc-aver-sam-sel: Determines how many ADC conversions will be averaged
>to create the ADC average result.
>> +			The Optional value is 4, 8, 16, 32.
>> +- fsl,adc-hw-aver-en: Bool type to decide enable hardware average function.
>> +- fsl,adc-low-power-mode: Bool type to decide enable ADC low power mode.
>> +- fsl,adc-high-speed-conv: Bool type to decide enable ADC high speed mode.
>> +- vref: The regulator to support ADC refrence voltage.
>
>Shouldn't it be vref-supply?
>
>Shawn

It is vref-supply.

Thanks.

Re: [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver

[Mark Rutland]

On Tue, Nov 26, 2013 at 10:56:34AM +0000, Fugang Duan wrote:
> The patch adds the binding file for Freescale vf610 ADC driver.
> 
> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  .../devicetree/bindings/iio/adc/vf610-adc.txt      |   57 ++++++++++++++++++++
>  1 files changed, 57 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
> new file mode 100644
> index 0000000..4101516
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
> @@ -0,0 +1,57 @@
> +Freescale vf610 Analog to Digital Converter bindings
> +
> +The devicetree bindings are for the new ADC driver written for
> +vf610/i.MX6slx and upward SoCs from Freescale.
> +
> +Required properties:
> +- compatible: Should be "fsl,vf610-adc"

s/be/contain/

> +- reg: Offset and length of the register set for the device
> +- interrupts: Should contain the interrupt for the device
> +- clocks: The clocks needed by the ADC controller

How many? Which ones?

> +- clock-names: the name of the clocks

Either define the set of names, or don't use clock-names. It's useless
if it doesn't tell you anything.

> +
> +Optional properties:
> +- fsl,adc-io-pinctl: Enable field for the I/O port control of MCU pins used as analog inputs.
> +- fsl,adc-vref: ADC refrence voltage value, unit is uV.

Can you not query the regulator to figure this out?

> +- fsl,adc-clk-div: Current clock divider value, such as 1,2,4,8,16 and so on.

Could you elaborate on this? What's it used for and why is it needed?

> +- fsl,adc-res: ADC conversion mode selection, such as 8 for 8-bit, 10 for 10-bit, 12 for 12-bit mode.

This sounds like something that could be changed at runtime. Why does
this need to be configured in the DT?

> +- fsl,adc-sam-time: ADC sample time duration, number of ADC clocks, such as 2, 4, 6, 8, 12, 16, 20, 24

Likewise.

Please don't poinltessly abbreviate, "sample" is much better than "sam"
here...

> +- fsl,adc-aver-sam-sel: Determines how many ADC conversions will be averaged to create the ADC average result.
> +			The Optional value is 4, 8, 16, 32.

Likewise.

> +- fsl,adc-hw-aver-en: Bool type to decide enable hardware average function.

When would you wnat this and when wouldn't you?

Similarly, "averages" is far clearer than "aver".

> +- fsl,adc-low-power-mode: Bool type to decide enable ADC low power mode.

Similarly?

> +- fsl,adc-high-speed-conv: Bool type to decide enable ADC high speed mode.

Similarly?

> +- vref: The regulator to support ADC refrence voltage.

s/vref/vref-supply/

Thanks,
Mark.

RE: [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver

[Fugang Duan]

From: Mark Rutland [mailto:mark.rutland-5wv7dgnIgG8@public.gmane.org]
Data: Tuesday, November 26, 2013 10:09 PM

>To: Duan Fugang-B38611
>Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
>shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Li Frank-B20596; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>Subject: Re: [PATCH v3 3/3] Documentation: add the binding file for Freescale
>vf610 ADC driver
>
>On Tue, Nov 26, 2013 at 10:56:34AM +0000, Fugang Duan wrote:
>> The patch adds the binding file for Freescale vf610 ADC driver.
>>
>> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>> ---
>>  .../devicetree/bindings/iio/adc/vf610-adc.txt      |   57
>++++++++++++++++++++
>>  1 files changed, 57 insertions(+), 0 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
>> b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
>> new file mode 100644
>> index 0000000..4101516
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
>> @@ -0,0 +1,57 @@
>> +Freescale vf610 Analog to Digital Converter bindings
>> +
>> +The devicetree bindings are for the new ADC driver written for
>> +vf610/i.MX6slx and upward SoCs from Freescale.
>> +
>> +Required properties:
>> +- compatible: Should be "fsl,vf610-adc"
>
>s/be/contain/
>
>> +- reg: Offset and length of the register set for the device
>> +- interrupts: Should contain the interrupt for the device
>> +- clocks: The clocks needed by the ADC controller
>
>How many? Which ones?
>
>> +- clock-names: the name of the clocks
>
>Either define the set of names, or don't use clock-names. It's useless if it
>doesn't tell you anything.
>
>> +
>> +Optional properties:
>> +- fsl,adc-io-pinctl: Enable field for the I/O port control of MCU pins used
>as analog inputs.
>> +- fsl,adc-vref: ADC refrence voltage value, unit is uV.
>
>Can you not query the regulator to figure this out?

This is optional, some board design have no fixed regulator, just supply fixed voltage from MCU digital power which
Cannot handled by the module.

>
>> +- fsl,adc-clk-div: Current clock divider value, such as 1,2,4,8,16 and so on.
>
>Could you elaborate on this? What's it used for and why is it needed?

Exp: ADC clock source is ipg bus clock, the clk divider can control the ADC sample speed while other setting don't change:
	The sample speed that clock divider is equal to 1 is twice  than the divider is equal to 2.

Anyway, it can control sample speed by user. 

>
>> +- fsl,adc-res: ADC conversion mode selection, such as 8 for 8-bit, 10 for
>10-bit, 12 for 12-bit mode.
>
>This sounds like something that could be changed at runtime. Why does this need
>to be configured in the DT?
>
>> +- fsl,adc-sam-time: ADC sample time duration, number of ADC clocks,
>> +such as 2, 4, 6, 8, 12, 16, 20, 24
>
>Likewise.
>
>Please don't poinltessly abbreviate, "sample" is much better than "sam"
>here...
>
>> +- fsl,adc-aver-sam-sel: Determines how many ADC conversions will be averaged
>to create the ADC average result.
>> +			The Optional value is 4, 8, 16, 32.
>
>Likewise.


For above ADC configuration, ADC conversion mode, sample time duration, hardware average sample selection, and so on.
All of them can changed at runtime. The only method is to introduce the ioctl interface for user to change them at runtime.
How about you think ?  

>
>> +- fsl,adc-hw-aver-en: Bool type to decide enable hardware average function.
>
>When would you wnat this and when wouldn't you?
>
>Similarly, "averages" is far clearer than "aver".

Ok, I see. Thanks.
>
>> +- fsl,adc-low-power-mode: Bool type to decide enable ADC low power mode.
>
>Similarly?
>
>> +- fsl,adc-high-speed-conv: Bool type to decide enable ADC high speed mode.
>
>Similarly?
>
>> +- vref: The regulator to support ADC refrence voltage.
>
>s/vref/vref-supply/
>
>Thanks,
>Mark.

Thanks for your review.
Summary, there have many comments why don't let some ADC configuration change at runtime.
The driver still don't add ioctl interface for user to configurate the ADC setting. So the setting are got from DT.

Do you have one good method to change the configuration at runtime ?

Thanks,
Andy

RE: [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver

[Jonathan Cameron]

Fugang Duan <fugang.duan-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>From: Mark Rutland [mailto:mark.rutland-5wv7dgnIgG8@public.gmane.org]
>Data: Tuesday, November 26, 2013 10:09 PM
>
>>To: Duan Fugang-B38611
>>Cc: jic23-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; sachin.kamat-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org;
>devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
>>shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org; Li Frank-B20596; linux-iio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>Subject: Re: [PATCH v3 3/3] Documentation: add the binding file for
>Freescale
>>vf610 ADC driver
>>
>>On Tue, Nov 26, 2013 at 10:56:34AM +0000, Fugang Duan wrote:
>>> The patch adds the binding file for Freescale vf610 ADC driver.
>>>
>>> Signed-off-by: Fugang Duan <B38611-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
>>> ---
>>>  .../devicetree/bindings/iio/adc/vf610-adc.txt      |   57
>>++++++++++++++++++++
>>>  1 files changed, 57 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
>>> b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
>>> new file mode 100644
>>> index 0000000..4101516
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
>>> @@ -0,0 +1,57 @@
>>> +Freescale vf610 Analog to Digital Converter bindings
>>> +
>>> +The devicetree bindings are for the new ADC driver written for
>>> +vf610/i.MX6slx and upward SoCs from Freescale.
>>> +
>>> +Required properties:
>>> +- compatible: Should be "fsl,vf610-adc"
>>
>>s/be/contain/
>>
>>> +- reg: Offset and length of the register set for the device
>>> +- interrupts: Should contain the interrupt for the device
>>> +- clocks: The clocks needed by the ADC controller
>>
>>How many? Which ones?
>>
>>> +- clock-names: the name of the clocks
>>
>>Either define the set of names, or don't use clock-names. It's useless
>if it
>>doesn't tell you anything.
>>
>>> +
>>> +Optional properties:
>>> +- fsl,adc-io-pinctl: Enable field for the I/O port control of MCU
>pins used
>>as analog inputs.
>>> +- fsl,adc-vref: ADC refrence voltage value, unit is uV.
>>
>>Can you not query the regulator to figure this out?
>
>This is optional, some board design have no fixed regulator, just
>supply fixed voltage from MCU digital power which
>Cannot handled by the module.
Fixed regulators are used when things either supplied by an actual fixed reg or when
 directly connected to another supply. It does not have to correspond to a bit of real
 hardware though that MCU digital power is coming from somewhere!

Hence this definitely wants to be a regulator 
>
>>
>>> +- fsl,adc-clk-div: Current clock divider value, such as 1,2,4,8,16
>and so on.
>>
>>Could you elaborate on this? What's it used for and why is it needed?
>
>Exp: ADC clock source is ipg bus clock, the clk divider can control the
>ADC sample speed while other setting don't change:
>	The sample speed that clock divider is equal to 1 is twice  than the
>divider is equal to 2.
>
>Anyway, it can control sample speed by user. 
If left in device tree then this might be best implemented as a very simple clock.
I would prefer it to be controlled from user space as sampling_frequency.
>
>>
>>> +- fsl,adc-res: ADC conversion mode selection, such as 8 for 8-bit,
>10 for
>>10-bit, 12 for 12-bit mode.
>>
>>This sounds like something that could be changed at runtime. Why does
>this need
>>to be configured in the DT?
>>
>>> +- fsl,adc-sam-time: ADC sample time duration, number of ADC clocks,
>>> +such as 2, 4, 6, 8, 12, 16, 20, 24
>>
>>Likewise.
>>
>>Please don't poinltessly abbreviate, "sample" is much better than
>"sam"
>>here...
>>
>>> +- fsl,adc-aver-sam-sel: Determines how many ADC conversions will be
>averaged
>>to create the ADC average result.
>>> +			The Optional value is 4, 8, 16, 32.
>>
>>Likewise.
>
>
>For above ADC configuration, ADC conversion mode, sample time duration,
>hardware average sample selection, and so on.
>All of them can changed at runtime. The only method is to introduce the
>ioctl interface for user to change them at runtime.
>How about you think ?  
No it is not. Like all similar controls these
Should be via sysfs attributes created via
The standard channel structures in IIO.
Sample duration might be integration_time
 or sampling_frequency depending on its exact semantics. Averaging is a form of low pass filter and should be specified as such.

If it is not already in the abi then propose something new.

The only one here that I can see an issue with doing that way is the resolution. So far it hasn't actually made sense to make this controllable..
Couple of questions on that.
1. What effect on speed of sampling does changing this have? Note that we probably do not care about this if using sysfs reading.
2. What other disadvantages are there to always running in 12 bit mode?

I have no problem with adding write support for the type of buffer elements though it would require some IIO core infrastructure.
However I do want a well described reason to bother with the added complexity.

>
>>
>>> +- fsl,adc-hw-aver-en: Bool type to decide enable hardware average
>function.
>>
>>When would you wnat this and when wouldn't you?
>>
>>Similarly, "averages" is far clearer than "aver".
>
>Ok, I see. Thanks.
>>
>>> +- fsl,adc-low-power-mode: Bool type to decide enable ADC low power
>mode.
>>
>>Similarly?
This one is interesting. What effects does it have in this device? Sometimes it is a
 sampling frequency restriction... 
>>
>>> +- fsl,adc-high-speed-conv: Bool type to decide enable ADC high
>speed mode.
>>
>>Similarly?
>>
>>> +- vref: The regulator to support ADC refrence voltage.
>>
>>s/vref/vref-supply/
>>
>>Thanks,
>>Mark.
>
>Thanks for your review.
>Summary, there have many comments why don't let some ADC configuration
>change at runtime.
>The driver still don't add ioctl interface for user to configurate the
>ADC setting. So the setting are got from DT.
>
>Do you have one good method to change the configuration at runtime ?
See above :)

In short...Add relevant bits to info_mask_* in all the iio_Chan_spec structures then put the
 relevant handling in the read_raw and write_raw iio_info callbacks.
>
>Thanks,
>Andy
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

RE: [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver

[Fugang Duan]

From: Jonathan Cameron <jic23@kernel.org>
Data: Wednesday, November 27, 2013 4:10 PM

>To: Duan Fugang-B38611; Mark Rutland
>Cc: sachin.kamat@linaro.org; devicetree@vger.kernel.org; shawn.guo@linaro.org;
>Li Frank-B20596; linux-iio@vger.kernel.org
>Subject: RE: [PATCH v3 3/3] Documentation: add the binding file for Freescale
>vf610 ADC driver
>
>
>
>Fugang Duan <fugang.duan@freescale.com> wrote:
>>From: Mark Rutland [mailto:mark.rutland@arm.com]
>>Data: Tuesday, November 26, 2013 10:09 PM
>>
>>>To: Duan Fugang-B38611
>>>Cc: jic23@kernel.org; sachin.kamat@linaro.org;
>>devicetree@vger.kernel.org;
>>>shawn.guo@linaro.org; Li Frank-B20596; linux-iio@vger.kernel.org
>>>Subject: Re: [PATCH v3 3/3] Documentation: add the binding file for
>>Freescale
>>>vf610 ADC driver
>>>
>>>On Tue, Nov 26, 2013 at 10:56:34AM +0000, Fugang Duan wrote:
>>>> The patch adds the binding file for Freescale vf610 ADC driver.
>>>>
>>>> Signed-off-by: Fugang Duan <B38611@freescale.com>
>>>> ---
>>>>  .../devicetree/bindings/iio/adc/vf610-adc.txt      |   57
>>>++++++++++++++++++++
>>>>  1 files changed, 57 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
>>>> b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
>>>> new file mode 100644
>>>> index 0000000..4101516
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iio/adc/vf610-adc.txt
>>>> @@ -0,0 +1,57 @@
>>>> +Freescale vf610 Analog to Digital Converter bindings
>>>> +
>>>> +The devicetree bindings are for the new ADC driver written for
>>>> +vf610/i.MX6slx and upward SoCs from Freescale.
>>>> +
>>>> +Required properties:
>>>> +- compatible: Should be "fsl,vf610-adc"
>>>
>>>s/be/contain/
>>>
>>>> +- reg: Offset and length of the register set for the device
>>>> +- interrupts: Should contain the interrupt for the device
>>>> +- clocks: The clocks needed by the ADC controller
>>>
>>>How many? Which ones?
>>>
>>>> +- clock-names: the name of the clocks
>>>
>>>Either define the set of names, or don't use clock-names. It's useless
>>if it
>>>doesn't tell you anything.
>>>
>>>> +
>>>> +Optional properties:
>>>> +- fsl,adc-io-pinctl: Enable field for the I/O port control of MCU
>>pins used
>>>as analog inputs.
>>>> +- fsl,adc-vref: ADC refrence voltage value, unit is uV.
>>>
>>>Can you not query the regulator to figure this out?
>>
>>This is optional, some board design have no fixed regulator, just
>>supply fixed voltage from MCU digital power which Cannot handled by the
>>module.
>Fixed regulators are used when things either supplied by an actual fixed reg or
>when  directly connected to another supply. It does not have to correspond to a
>bit of real  hardware though that MCU digital power is coming from somewhere!
>
>Hence this definitely wants to be a regulator

Good suggestion, thanks !

>>
>>>
>>>> +- fsl,adc-clk-div: Current clock divider value, such as 1,2,4,8,16
>>and so on.
>>>
>>>Could you elaborate on this? What's it used for and why is it needed?
>>
>>Exp: ADC clock source is ipg bus clock, the clk divider can control the
>>ADC sample speed while other setting don't change:
>>	The sample speed that clock divider is equal to 1 is twice  than the
>>divider is equal to 2.
>>
>>Anyway, it can control sample speed by user.
>If left in device tree then this might be best implemented as a very simple
>clock.
>I would prefer it to be controlled from user space as sampling_frequency.
>>
>>>
>>>> +- fsl,adc-res: ADC conversion mode selection, such as 8 for 8-bit,
>>10 for
>>>10-bit, 12 for 12-bit mode.
>>>
>>>This sounds like something that could be changed at runtime. Why does
>>this need
>>>to be configured in the DT?
>>>
>>>> +- fsl,adc-sam-time: ADC sample time duration, number of ADC clocks,
>>>> +such as 2, 4, 6, 8, 12, 16, 20, 24
>>>
>>>Likewise.
>>>
>>>Please don't poinltessly abbreviate, "sample" is much better than
>>"sam"
>>>here...
>>>
>>>> +- fsl,adc-aver-sam-sel: Determines how many ADC conversions will be
>>averaged
>>>to create the ADC average result.
>>>> +			The Optional value is 4, 8, 16, 32.
>>>
>>>Likewise.
>>
>>
>>For above ADC configuration, ADC conversion mode, sample time duration,
>>hardware average sample selection, and so on.
>>All of them can changed at runtime. The only method is to introduce the
>>ioctl interface for user to change them at runtime.
>>How about you think ?
>No it is not. Like all similar controls these Should be via sysfs attributes
>created via The standard channel structures in IIO.
>Sample duration might be integration_time  or sampling_frequency depending on
>its exact semantics. Averaging is a form of low pass filter and should be
>specified as such.
>
>If it is not already in the abi then propose something new.
>
>The only one here that I can see an issue with doing that way is the resolution.
>So far it hasn't actually made sense to make this controllable..
>Couple of questions on that.
>1. What effect on speed of sampling does changing this have? Note that we
>probably do not care about this if using sysfs reading.
>2. What other disadvantages are there to always running in 12 bit mode?
>
>I have no problem with adding write support for the type of buffer elements
>though it would require some IIO core infrastructure.
>However I do want a well described reason to bother with the added complexity.
>
Thanks very much! Your suggestion is very useful for me. I will use the standard sys interface for ADC configuration.

And for your question:
1. effect on speed of sampling: 
	- ADC sample time duration, number of ADC clocks, such as 2, 4, 6, 8, 12, 16, 20, 24
	- clock, the clock source is ipg bus clock, current clock divider value for ADC, such as 1,2,4,8,16 and so on
	- if hardware average function enable,
		The ADC average sample number also affect speed, determines how many ADC conversions will be averaged to create the ADC average result.
	All of above setting affect the speed of sampling.

	If use the sysfs reading, the speed don't need care. 
	So I have some questions:
	a. For high speed sampling, IIO subsystem how to handle it if there no ioctl interface for user ? 
	   it is iio buffer/fifo ?
	b. For software trigger, the speed of sampling also don't need to care, is it right ? 
	c. If use case need high speed sampling, there must have hareware trigger and iio buffer for it, right ? 
 
2. no disadvantage, I have validated ADC can work fine at 8-bit, 10-bit, 12-bit mode.

>>
>>>
>>>> +- fsl,adc-hw-aver-en: Bool type to decide enable hardware average
>>function.
>>>
>>>When would you wnat this and when wouldn't you?
>>>
>>>Similarly, "averages" is far clearer than "aver".
>>
>>Ok, I see. Thanks.
>>>
>>>> +- fsl,adc-low-power-mode: Bool type to decide enable ADC low power
>>mode.
>>>
>>>Similarly?
>This one is interesting. What effects does it have in this device? Sometimes it
>is a  sampling frequency restriction...
>>>
>>>> +- fsl,adc-high-speed-conv: Bool type to decide enable ADC high
>>speed mode.
>>>
>>>Similarly?
>>>
>>>> +- vref: The regulator to support ADC refrence voltage.
>>>
>>>s/vref/vref-supply/
>>>
>>>Thanks,
>>>Mark.
>>
>>Thanks for your review.
>>Summary, there have many comments why don't let some ADC configuration
>>change at runtime.
>>The driver still don't add ioctl interface for user to configurate the
>>ADC setting. So the setting are got from DT.
>>
>>Do you have one good method to change the configuration at runtime ?
>See above :)
>
>In short...Add relevant bits to info_mask_* in all the iio_Chan_spec structures
>then put the  relevant handling in the read_raw and write_raw iio_info
>callbacks.
>>
>>Thanks,
>>Andy
>>
>>--
>>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
>
>--
>Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Thanks for your review and answers.

Andy