[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@freescale.com>
---
 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@freescale.com>
---
 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@freescale.com>");
+MODULE_DESCRIPTION("Freescale VF610 ADC driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.2.rc3

[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@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"
+- 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 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@freescale.com>

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

[Peter Meerwald]

On Tue, 26 Nov 2013, Fugang Duan wrote:

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

nitpicking below

> Signed-off-by: Fugang Duan <B38611@freescale.com>
> ---
>  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

alphabetic order please

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

keep alphabetic order

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

VF610_ prefix

> +#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))

no parenthesis needed

> +
> +enum clk_sel {
> +	ADCIOC_BUSCLK_SET,
> +	ADCIOC_ALTCLK_SET,
> +	ADCIOC_ADACK_SET,

VF610 prefix

> +};
> +
> +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,					\

address not needed

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

what does "This config may upgrade to require from DT" mean?

> +	 */
> +	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");

"Missing" probably

> +	}
> +
> +	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);

no parenthesis needed

> +	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");

all those errors just logged and then ignored;
shouldn't this be warnings then or the handling improved?

> +			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*/

blank before */

> +	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");

error ignored

> +
> +	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)
> +{

why are some functions marked inline and others not?
this function does not seem time critical

> +	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) {

maybe a lookup table could be used; value of res_mode is trusted at this 
point, no error checking needed

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

should never happen

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

use IIO_VAL_FRACTIONAL_LOG2 probably

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

can this ever happen?

I'd rather check the value of reg
> +
> +	*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;

initialization of ret and info not needed

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

regulator stays enabled

> +		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 */

comment does not add to clarity of the code, remove?

> +	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@freescale.com>");
> +MODULE_DESCRIPTION("Freescale VF610 ADC driver");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

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

[Lars-Peter Clausen]

On 11/26/2013 01:24 PM, Peter Meerwald wrote:
>> +static inline void vf610_adc_cfg_set(struct vf610_adc *info)
>> +{
> 
> why are some functions marked inline and others not?
> this function does not seem time critical

And the compiler knows much better which functions to (partially) inline and
which not, I think inline is mostly ignored by the compiler anyway, so yes,
it should be removed here.

>> +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;
> 
> can this ever happen?
> 
> I'd rather check the value of reg

It is NULL when writing to a register.

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

[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@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?

> +- 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 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@freescale.com>
> ---
>  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 3/3] Documentation: add the binding file for Freescale vf610 ADC driver

[Fugang Duan]

From: Shawn Guo [mailto:shawn.guo@linaro.org]
Data: Tuesday, November 26, 2013 9:38 PM

>To: Duan Fugang-B38611
>Cc: jic23@kernel.org; sachin.kamat@linaro.org; devicetree@vger.kernel.org;=
 Li
>Frank-B20596; linux-iio@vger.kernel.org
>Subject: Re: [PATCH v3 3/3] Documentation: add the binding file for Freesc=
ale
>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 f=
or
>10-bit, 12 for 12-bit mode.
>> +- fsl,adc-sam-time: ADC sample time duration, number of ADC clocks, suc=
h as
>2, 4, 6, 8, 12, 16, 20, 24
>> +- fsl,adc-aver-sam-sel: Determines how many ADC conversions will be ave=
raged
>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 funct=
ion.
>> +- 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 mo=
de.
>> +- vref: The regulator to support ADC refrence voltage.
>
>Shouldn't it be vref-supply?
>
>Shawn

It is vref-supply.

Thanks.

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

[Fugang Duan]

From: Peter Meerwald <pmeerw@pmeerw.net>
Data: Tuesday, November 26, 2013 8:24 PM

>To: Duan Fugang-B38611
>Cc: jic23@kernel.org; shawn.guo@linaro.org; linux-iio@vger.kernel.org
>Subject: Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver
>
>On Tue, 26 Nov 2013, Fugang Duan wrote:
>
>> Add Freescale Vybrid vf610 adc driver. The driver only support ADC
>> software trigger.
>
>nitpicking below
>
>> Signed-off-by: Fugang Duan <B38611@freescale.com>
>> ---
>>  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
>
>alphabetic order please

Got it. Thanks.
>
>> +	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
>
>keep alphabetic order

Got it. Thanks.
>
>> 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 */
>
>VF610_ prefix

Hmm, I think it is not necessary to add prefix for the register address.


>
>> +#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))
>
>no parenthesis needed

Got it, thanks!
>
>> +
>> +enum clk_sel {
>> +	ADCIOC_BUSCLK_SET,
>> +	ADCIOC_ALTCLK_SET,
>> +	ADCIOC_ADACK_SET,
>
>VF610 prefix

Got it, thanks!
>
>> +};
>> +
>> +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,					\
>
>address not needed

Got it, thanks!
>
>> +	.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
>
>what does "This config may upgrade to require from DT" mean?

There many configurations for ADC, and I have import some setting from DT, below some setting is static defined.
In fact, all setting can get from DT.

I don't want to static define the ADC setting, expect for getting them from DT, how to config them in runtime for user ?
IIO subsystem don't have such interface for it. I think the only solution is to add ioctl interface for the driver. How do you think ?
 
>
>> +	 */
>> +	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");
>
>"Missing" probably
>

Got it, thanks!

>> +	}
>> +
>> +	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);
>
>no parenthesis needed

Got it , thanks!
>
>> +	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");
>
>all those errors just logged and then ignored; shouldn't this be warnings then
>or the handling improved?

If the setting is error, use the default setting, and print out err log for user.
In there, it use the default setting in fact.
>
>> +			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*/
>
>blank before */

You are very careful, thanks!

>
>> +	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");
>
>error ignored
>
Got it, thanks!
There cannot be interrupted for calibration.
So use wait_for_completion_timeout() instead of wait_for_completion_interruptible_timeout().

>> +
>> +	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) {
>
>why are some functions marked inline and others not?
>this function does not seem time critical

Agree, I will remove the inline.
>
>> +	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) {
>
>maybe a lookup table could be used; value of res_mode is trusted at this point,
>no error checking needed

Yes, there don't need to check error.
The entry number is only three, so lookup table is not valuable.

>
>> +	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;
>
>should never happen
>
Got it. Thanks!

>> +
>> +	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;
>
>use IIO_VAL_FRACTIONAL_LOG2 probably

Yes, it is better to use IIO_VAL_FRACTIONAL_LOG2, thanks!
>
>> +		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;
>
>can this ever happen?

No, it never happen. Thanks.

static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,
                              size_t count, loff_t *ppos)
{
        struct iio_dev *indio_dev = file->private_data;
        char buf[20];
        unsigned val = 0; 
        ssize_t len; 
        int ret; 

        ret = indio_dev->info->debugfs_reg_access(indio_dev,
                                                  indio_dev->cached_reg_addr,
                                                  0, &val);
	 ...
}
 
>
>I'd rather check the value of reg
>> +
>> +	*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;
>
>initialization of ret and info not needed

Got it.
>
>> +
>> +	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)
>
>regulator stays enabled
>
>> +		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 */
>
>comment does not add to clarity of the code, remove?

Below code let ADC enter to stop mode when suspend.
>
>> +	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@freescale.com>");
>> +MODULE_DESCRIPTION("Freescale VF610 ADC driver"); MODULE_LICENSE("GPL
>> +v2");
>>

Peter,
thanks for your review.

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

[Fugang Duan]

From: Lars-Peter Clausen <lars@metafoo.de>
Data: Tuesday, November 26, 2013 7:52 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 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@freescale.com>
>
>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

[Fugang Duan]

From: Mark Rutland <mark.rutland@arm.com>
Data: Tuesday, November 26, 2013 10:26 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 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc dr=
iver
>
>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@freescale.com>
>> ---
>>  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 =3D info->dev->of_node;
>> +
>> +       /*
>> +        * set default Configuration for ADC controller
>> +        * This config may upgrade to require from DT
>> +        */
>> +       info->adc_feature.clk_sel =3D ADCIOC_BUSCLK_SET;
>> +       info->adc_feature.vol_ref =3D VF610_ADCIOC_VR_VREF_SET;
>> +       info->adc_feature.calibration =3D true;
>> +       info->adc_feature.tri_hw =3D false;
>> +       info->adc_feature.dataov_en =3D true;
>> +       info->adc_feature.ac_clk_en =3D false;
>> +       info->adc_feature.dma_en =3D false;
>> +       info->adc_feature.cc_en =3D false;
>> +       info->adc_feature.cmp_func_en =3D false;
>> +       info->adc_feature.cmp_range_en =3D false;
>> +       info->adc_feature.cmp_greater_en =3D false;
>> +
>> +       if (of_property_read_u32(np, "fsl,adc-io-pinctl",
>> +                       &info->adc_feature.pctl)) {
>> +               info->adc_feature.pctl =3D 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 condit=
ions 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 a=
ny pin with its
input buffer disabled.
* The pullup is disabled.=20

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


>> +
>> +       if (info->vref)
>> +               info->vref_uv =3D 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 fixe=
d voltage from MCU digital power,
We get the reference voltage from DT.
 =20
>
>> +
>> +       if (of_property_read_u32(np, "fsl,adc-clk-div",
>> +                       &info->adc_feature.clk_div_num)) {
>> +               info->adc_feature.clk_div_num =3D 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 th=
e
>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 =3D 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 mod=
e, all pass.
>
>> +
>> +       if (of_property_read_u32(np, "fsl,adc-sam-time",
>> +                       &info->adc_feature.sam_time)) {
>> +               info->adc_feature.sam_time =3D 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 ?=20
Use ioctl ?


For the ADC, there have many setting, some of setting are defined staticall=
y,  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 =3D 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 c=
onversion
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 =3D 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 introduc=
e ioctl interface for user.
How do you think ?


>> +
>> +       info->adc_feature.lpm =3D of_property_read_bool(np, "fsl,adc-low=
-power-
>mode");
>> +       info->adc_feature.hs_oper =3D 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 introd=
uce ioctl interface for user.=20


Mark,=20

Thanks for your review.

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

[Fugang Duan]

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.

>
>> +- 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 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver

[Peter Meerwald]

some clarification below

> >To: Duan Fugang-B38611
> >Cc: jic23@kernel.org; shawn.guo@linaro.org; linux-iio@vger.kernel.org
> >Subject: Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver
> >
> >On Tue, 26 Nov 2013, Fugang Duan wrote:
> >
> >> Add Freescale Vybrid vf610 adc driver. The driver only support ADC
> >> software trigger.
> >
> >nitpicking below
> >
> >> Signed-off-by: Fugang Duan <B38611@freescale.com>
> >> ---
> >>  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
> >
> >alphabetic order please
> 
> Got it. Thanks.
> >
> >> +	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
> >
> >keep alphabetic order
> 
> Got it. Thanks.
> >
> >> 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 */
> >
> >VF610_ prefix
> 
> Hmm, I think it is not necessary to add prefix for the register address.

the argument for the prefix is that something in the headers you include 
may #define ADC_SOMETHING in the future and cause breakage; prefixing 
reduces this risk

plus other define have the prefix, so for the sake of consistency...
 
> >
> >> +#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))
> >
> >no parenthesis needed
> 
> Got it, thanks!
> >
> >> +
> >> +enum clk_sel {
> >> +	ADCIOC_BUSCLK_SET,
> >> +	ADCIOC_ALTCLK_SET,
> >> +	ADCIOC_ADACK_SET,
> >
> >VF610 prefix
> 
> Got it, thanks!
> >
> >> +};
> >> +
> >> +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,					\
> >
> >address not needed
> 
> Got it, thanks!
> >
> >> +	.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
> >
> >what does "This config may upgrade to require from DT" mean?
> 
> There many configurations for ADC, and I have import some setting from DT, below some setting is static defined.
> In fact, all setting can get from DT.
> 
> I don't want to static define the ADC setting, expect for getting them from DT, how to config them in runtime for user ?
> IIO subsystem don't have such interface for it. I think the only solution is to add ioctl interface for the driver. How do you think ?

I was just picking on the wording; I fail to understand what is meant by 
the comment
  
> >> +	 */
> >> +	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");
> >
> >"Missing" probably
> >
> 
> Got it, thanks!
> 
> >> +	}
> >> +
> >> +	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);
> >
> >no parenthesis needed
> 
> Got it , thanks!
> >
> >> +	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");
> >
> >all those errors just logged and then ignored; shouldn't this be warnings then
> >or the handling improved?
> 
> If the setting is error, use the default setting, and print out err log for user.
> In there, it use the default setting in fact.
> >
> >> +			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*/
> >
> >blank before */
> 
> You are very careful, thanks!
> 
> >
> >> +	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");
> >
> >error ignored
> >
> Got it, thanks!
> There cannot be interrupted for calibration.
> So use wait_for_completion_timeout() instead of wait_for_completion_interruptible_timeout().
> 
> >> +
> >> +	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) {
> >
> >why are some functions marked inline and others not?
> >this function does not seem time critical
> 
> Agree, I will remove the inline.
> >
> >> +	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) {
> >
> >maybe a lookup table could be used; value of res_mode is trusted at this point,
> >no error checking needed
> 
> Yes, there don't need to check error.
> The entry number is only three, so lookup table is not valuable.
> 
> >
> >> +	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;
> >
> >should never happen
> >
> Got it. Thanks!
> 
> >> +
> >> +	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;
> >
> >use IIO_VAL_FRACTIONAL_LOG2 probably
> 
> Yes, it is better to use IIO_VAL_FRACTIONAL_LOG2, thanks!
> >
> >> +		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;
> >
> >can this ever happen?
> 
> No, it never happen. Thanks.

see Lars comment; I was wrong

I think reg should be range checked in addition
 
> static ssize_t iio_debugfs_read_reg(struct file *file, char __user *userbuf,
>                               size_t count, loff_t *ppos)
> {
>         struct iio_dev *indio_dev = file->private_data;
>         char buf[20];
>         unsigned val = 0; 
>         ssize_t len; 
>         int ret; 
> 
>         ret = indio_dev->info->debugfs_reg_access(indio_dev,
>                                                   indio_dev->cached_reg_addr,
>                                                   0, &val);
> 	 ...
> }
>  
> >
> >I'd rather check the value of reg
> >> +
> >> +	*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;
> >
> >initialization of ret and info not needed
> 
> Got it.
> >
> >> +
> >> +	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)
> >
> >regulator stays enabled
> >
> >> +		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 */
> >
> >comment does not add to clarity of the code, remove?
> 
> Below code let ADC enter to stop mode when suspend.

yes; VF610_ADC_CONV_DISABLE is pretty descriptive on its own :)

> >
> >> +	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@freescale.com>");
> >> +MODULE_DESCRIPTION("Freescale VF610 ADC driver"); MODULE_LICENSE("GPL
> >> +v2");
> >>
> 
> Peter,
> thanks for your review.
> 
> --
> 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
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

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

[Fugang Duan]

From: Peter Meerwald <pmeerw@pmeerw.net>
Data: Wednesday, November 27, 2013 3:17 PM

>To: Duan Fugang-B38611
>Cc: jic23@kernel.org; shawn.guo@linaro.org; linux-iio@vger.kernel.org
>Subject: RE: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc dr=
iver
>
>
>some clarification below
>
>> >To: Duan Fugang-B38611
>> >Cc: jic23@kernel.org; shawn.guo@linaro.org; linux-iio@vger.kernel.org
>> >Subject: Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610
>> >adc driver
>> >
>> >On Tue, 26 Nov 2013, Fugang Duan wrote:
>> >
>> >> Add Freescale Vybrid vf610 adc driver. The driver only support ADC
>> >> software trigger.
>> >
>> >nitpicking below
>> >
>> >> Signed-off-by: Fugang Duan <B38611@freescale.com>
>> >> ---
>> >>  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
>> >
>> >alphabetic order please
>>
>> Got it. Thanks.
>> >
>> >> +	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) +=3D ti-adc081c.o
>> >>  obj-$(CONFIG_TI_AM335X_ADC) +=3D ti_am335x_adc.o
>> >>  obj-$(CONFIG_TWL6030_GPADC) +=3D twl6030-gpadc.o
>> >>  obj-$(CONFIG_VIPERBOARD_ADC) +=3D viperboard_adc.o
>> >> +obj-$(CONFIG_VF610_ADC) +=3D vf610_adc.o
>> >
>> >keep alphabetic order
>>
>> Got it. Thanks.
>> >
>> >> 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 */
>> >
>> >VF610_ prefix
>>
>> Hmm, I think it is not necessary to add prefix for the register address.
>
>the argument for the prefix is that something in the headers you include m=
ay
>#define ADC_SOMETHING in the future and cause breakage; prefixing reduces =
this
>risk
>
>plus other define have the prefix, so for the sake of consistency...

Ok, I will add the prefix for them. Thanks.

>
>> >
>> >> +#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))
>> >
>> >no parenthesis needed
>>
>> Got it, thanks!
>> >
>> >> +
>> >> +enum clk_sel {
>> >> +	ADCIOC_BUSCLK_SET,
>> >> +	ADCIOC_ALTCLK_SET,
>> >> +	ADCIOC_ADACK_SET,
>> >
>> >VF610 prefix
>>
>> Got it, thanks!
>> >
>> >> +};
>> >> +
>> >> +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 =3D (_chan_type),					\
>> >> +	.indexed =3D 1,						\
>> >> +	.channel =3D _idx,					\
>> >> +	.address =3D _idx,					\
>> >
>> >address not needed
>>
>> Got it, thanks!
>> >
>> >> +	.info_mask_separate =3D BIT(IIO_CHAN_INFO_RAW),		\
>> >> +	.info_mask_shared_by_type =3D BIT(IIO_CHAN_INFO_SCALE),	\
>> >> +}
>> >> +
>> >> +static const struct iio_chan_spec vf610_adc_iio_channels[] =3D {
>> >> +	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 =3D info->dev->of_node;
>> >> +
>> >> +	/*
>> >> +	 * set default Configuration for ADC controller
>> >> +	 * This config may upgrade to require from DT
>> >
>> >what does "This config may upgrade to require from DT" mean?
>>
>> There many configurations for ADC, and I have import some setting from D=
T,
>below some setting is static defined.
>> In fact, all setting can get from DT.
>>
>> I don't want to static define the ADC setting, expect for getting them f=
rom
>DT, how to config them in runtime for user ?
>> IIO subsystem don't have such interface for it. I think the only solutio=
n is
>to add ioctl interface for the driver. How do you think ?
>
>I was just picking on the wording; I fail to understand what is meant by t=
he
>comment
>

For the ADC controller, there have many setting for it. Some of them are de=
fined statically, some of them are got from DT.
As the suggestion to set the configuration at runtime, how to do it ?  Add =
ioctl interface for user ?=20

>> >> +	 */
>> >> +	info->adc_feature.clk_sel =3D ADCIOC_BUSCLK_SET;
>> >> +	info->adc_feature.vol_ref =3D VF610_ADCIOC_VR_VREF_SET;
>> >> +	info->adc_feature.calibration =3D true;
>> >> +	info->adc_feature.tri_hw =3D false;
>> >> +	info->adc_feature.dataov_en =3D true;
>> >> +	info->adc_feature.ac_clk_en =3D false;
>> >> +	info->adc_feature.dma_en =3D false;
>> >> +	info->adc_feature.cc_en =3D false;
>> >> +	info->adc_feature.cmp_func_en =3D false;
>> >> +	info->adc_feature.cmp_range_en =3D false;
>> >> +	info->adc_feature.cmp_greater_en =3D false;
>> >> +
>> >> +	if (of_property_read_u32(np, "fsl,adc-io-pinctl",
>> >> +			&info->adc_feature.pctl)) {
>> >> +		info->adc_feature.pctl =3D VF610_ADC_IOPCTL5;
>> >> +		dev_info(info->dev,
>> >> +			"Miss adc-io-pinctl in dt, enable pin SE5.\n");
>> >
>> >"Missing" probably
>> >
>>
>> Got it, thanks!
>>
>> >> +	}
>> >> +
>> >> +	if (info->vref)
>> >> +		info->vref_uv =3D 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 =3D 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 =3D 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 =3D 4;
>> >> +		dev_info(info->dev,
>> >> +			"Miss adc-sam-time property in dt, set to 4.\n");
>> >> +	}
>> >> +
>> >> +	info->adc_feature.hw_average =3D 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 =3D 4;
>> >> +		dev_info(info->dev,
>> >> +			"Miss adc-aver-sam-sel property in dt, set to 4.\n");
>> >> +	}
>> >> +
>> >> +	info->adc_feature.lpm =3D of_property_read_bool(np,
>> >> +"fsl,adc-low-power-
>> >mode");
>> >> +	info->adc_feature.hs_oper =3D 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 =3D &(info->adc_feature);
>> >
>> >no parenthesis needed
>>
>> Got it , thanks!
>> >
>> >> +	int cfg_data =3D 0;
>> >> +	int gc_data =3D 0;
>> >> +
>> >> +	/* clock select and clock devider */
>> >> +	switch (adc_feature->clk_div_num) {
>> >> +	case 1:
>> >> +		break;
>> >> +	case 2:
>> >> +		cfg_data |=3D VF610_ADC_CLK_DIV2;
>> >> +		break;
>> >> +	case 4:
>> >> +		cfg_data |=3D VF610_ADC_CLK_DIV4;
>> >> +		break;
>> >> +	case 8:
>> >> +		cfg_data |=3D VF610_ADC_CLK_DIV8;
>> >> +		break;
>> >> +	case 16:
>> >> +		switch (adc_feature->clk_sel) {
>> >> +		case ADCIOC_BUSCLK_SET:
>> >> +			cfg_data |=3D VF610_ADC_BUSCLK2_SEL | VF610_ADC_CLK_DIV8;
>> >> +			break;
>> >> +		default:
>> >> +			dev_err(info->dev, "error clk divider\n");
>> >
>> >all those errors just logged and then ignored; shouldn't this be
>> >warnings then or the handling improved?
>>
>> If the setting is error, use the default setting, and print out err log =
for
>user.
>> In there, it use the default setting in fact.
>> >
>> >> +			break;
>> >> +		}
>> >> +		break;
>> >> +	}
>> >> +
>> >> +	switch (adc_feature->clk_sel) {
>> >> +	case ADCIOC_ALTCLK_SET:
>> >> +		cfg_data |=3D VF610_ADC_ALTCLK_SEL;
>> >> +		break;
>> >> +	case ADCIOC_ADACK_SET:
>> >> +		cfg_data |=3D VF610_ADC_ADACK_SEL;
>> >> +		break;
>> >> +	default:
>> >> +		break;
>> >> +	}
>> >> +
>> >> +	/* resolution mode */
>> >> +	switch (adc_feature->res_mode) {
>> >> +	case 8:
>> >> +		cfg_data |=3D VF610_ADC_MODE_BIT8;
>> >> +		break;
>> >> +	case 10:
>> >> +		cfg_data |=3D VF610_ADC_MODE_BIT10;
>> >> +		break;
>> >> +	case 12:
>> >> +		cfg_data |=3D 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 |=3D VF610_ADC_ADSTS_SHORT;
>> >> +		break;
>> >> +	case 6:
>> >> +		cfg_data |=3D VF610_ADC_ADSTS_NORMAL;
>> >> +		break;
>> >> +	case 8:
>> >> +		cfg_data |=3D VF610_ADC_ADSTS_LONG;
>> >> +		break;
>> >> +	case 12:
>> >> +		cfg_data |=3D VF610_ADC_ADLSMP_LONG;
>> >> +		break;
>> >> +	case 16:
>> >> +		cfg_data |=3D VF610_ADC_ADLSMP_LONG | VF610_ADC_ADSTS_SHORT;
>> >> +		break;
>> >> +	case 20:
>> >> +		cfg_data |=3D VF610_ADC_ADLSMP_LONG | VF610_ADC_ADSTS_NORMAL;
>> >> +		break;
>> >> +	case 24:
>> >> +		cfg_data |=3D VF610_ADC_ADLSMP_LONG | VF610_ADC_ADSTS_LONG;
>> >> +		break;
>> >> +	default:
>> >> +		dev_err(info->dev, "error sample duration\n");
>> >> +		break;
>> >> +	}
>> >> +
>> >> +	/* low power configuration */
>> >> +	cfg_data |=3D VF610_ADC_ADLPC_EN;
>> >> +
>> >> +	/* high speed operation */
>> >> +	cfg_data |=3D VF610_ADC_ADHSC_EN;
>> >> +
>> >> +	/* voltage reference*/
>> >
>> >blank before */
>>
>> You are very careful, thanks!
>>
>> >
>> >> +	switch (adc_feature->vol_ref) {
>> >> +	case VF610_ADCIOC_VR_VREF_SET:
>> >> +		break;
>> >> +	case VF610_ADCIOC_VR_VALT_SET:
>> >> +		cfg_data |=3D VF610_ADC_REFSEL_VALT;
>> >> +		break;
>> >> +	case VF610_ADCIOC_VR_VBG_SET:
>> >> +		cfg_data |=3D VF610_ADC_REFSEL_VBG;
>> >> +		break;
>> >> +	default:
>> >> +		dev_err(info->dev, "error voltage reference\n");
>> >> +	}
>> >> +
>> >> +	/* trigger select */
>> >> +	if (adc_feature->tri_hw)
>> >> +		cfg_data |=3D VF610_ADC_ADTRG_HARD;
>> >> +
>> >> +	/* hardware average select */
>> >> +	if (adc_feature->hw_average) {
>> >> +		switch (adc_feature->hw_sam) {
>> >> +		case 4:
>> >> +			break;
>> >> +		case 8:
>> >> +			cfg_data |=3D VF610_ADC_AVGS_8;
>> >> +			break;
>> >> +		case 16:
>> >> +			cfg_data |=3D VF610_ADC_AVGS_16;
>> >> +			break;
>> >> +		case 32:
>> >> +			cfg_data |=3D VF610_ADC_AVGS_32;
>> >> +			break;
>> >> +		default:
>> >> +			dev_err(info->dev,
>> >> +			"error hardware sample average select\n");
>> >> +		}
>> >> +
>> >> +		gc_data |=3D VF610_ADC_AVGEN;
>> >> +	}
>> >> +
>> >> +	/* data overwrite enable */
>> >> +	if (adc_feature->dataov_en)
>> >> +		cfg_data |=3D VF610_ADC_OVWREN;
>> >> +
>> >> +	/* Asynchronous clock output enable */
>> >> +	if (adc_feature->ac_clk_en)
>> >> +		gc_data |=3D VF610_ADC_ADACKEN;
>> >> +
>> >> +	/* dma enable */
>> >> +	if (adc_feature->dma_en)
>> >> +		gc_data |=3D VF610_ADC_DMAEN;
>> >> +
>> >> +	/* continue function enable */
>> >> +	if (adc_feature->cc_en)
>> >> +		gc_data |=3D VF610_ADC_ADCON;
>> >> +
>> >> +	/* compare function enable */
>> >> +	if (adc_feature->cmp_func_en)
>> >> +		gc_data |=3D VF610_ADC_ACFE;
>> >> +
>> >> +	/* greater than enable */
>> >> +	if (adc_feature->cmp_greater_en)
>> >> +		gc_data |=3D VF610_ADC_ACFGT;
>> >> +
>> >> +	/* range enable */
>> >> +	if (adc_feature->cmp_range_en)
>> >> +		gc_data |=3D 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 =3D VF610_ADC_AIEN | VF610_ADC_CONV_DISABLE;
>> >> +	writel(hc_cfg, info->regs + ADC_HC0);
>> >> +
>> >> +	adc_gc =3D readl(info->regs + ADC_GC);
>> >> +	writel(adc_gc | VF610_ADC_CAL, info->regs + ADC_GC);
>> >> +
>> >> +	timeout =3D wait_for_completion_interruptible_timeout
>> >> +			(&info->completion, VF610_ADC_TIMEOUT);
>> >> +	if (timeout =3D=3D 0)
>> >> +		dev_err(info->dev, "Timeout for adc calibration\n");
>> >
>> >error ignored
>> >
>> Got it, thanks!
>> There cannot be interrupted for calibration.
>> So use wait_for_completion_timeout() instead of
>wait_for_completion_interruptible_timeout().
>>
>> >> +
>> >> +	adc_gc =3D readl(info->regs + ADC_GS);
>> >> +	if (adc_gc & VF610_ADC_CALF)
>> >> +		dev_err(info->dev, "ADC calibration failed\n");
>> >> +
>> >> +	info->adc_feature.calibration =3D false; }
>> >> +
>> >> +static inline void vf610_adc_cfg_set(struct vf610_adc *info) {
>> >
>> >why are some functions marked inline and others not?
>> >this function does not seem time critical
>>
>> Agree, I will remove the inline.
>> >
>> >> +	struct vf610_adc_feature *adc_feature =3D &(info->adc_feature);
>> >> +	int cfg_data =3D 0;
>> >> +
>> >> +	cfg_data =3D readl(info->regs + ADC_CFG);
>> >> +
>> >> +	/* low power configuration */
>> >> +	if (!adc_feature->lpm)
>> >> +		cfg_data &=3D ~VF610_ADC_ADLPC_EN;
>> >> +
>> >> +	/* high speed operation */
>> >> +	if (!adc_feature->hs_oper)
>> >> +		cfg_data &=3D ~VF610_ADC_ADHSC_EN;
>> >> +
>> >> +	/* trigger select */
>> >> +	if (adc_feature->tri_hw)
>> >> +		cfg_data |=3D 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 =3D readl(info->regs + ADC_R0);
>> >> +
>> >> +	switch (info->adc_feature.res_mode) {
>> >
>> >maybe a lookup table could be used; value of res_mode is trusted at
>> >this point, no error checking needed
>>
>> Yes, there don't need to check error.
>> The entry number is only three, so lookup table is not valuable.
>>
>> >
>> >> +	case 8:
>> >> +		result &=3D VF610_ADC_DATA_BIT8_MASK;
>> >> +		break;
>> >> +	case 10:
>> >> +		result &=3D VF610_ADC_DATA_BIT10_MASK;
>> >> +		break;
>> >> +	case 12:
>> >> +		result &=3D 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 =3D (struct vf610_adc *)dev_id;
>> >> +	int coco;
>> >> +
>> >> +	coco =3D readl(info->regs + ADC_HS);
>> >> +	if (coco & VF610_ADC_HS_COCO0) {
>> >> +		info->value =3D 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 =3D 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;
>> >
>> >should never happen
>> >
>> Got it. Thanks!
>>
>> >> +
>> >> +	switch (mask) {
>> >> +	case IIO_CHAN_INFO_RAW:
>> >> +		mutex_lock(&indio_dev->mlock);
>> >> +
>> >> +		hc_cfg =3D VF610_ADC_ADCHC(chan->channel);
>> >> +		hc_cfg |=3D VF610_ADC_AIEN;
>> >> +		writel(hc_cfg, info->regs + ADC_HC0);
>> >> +		timeout =3D wait_for_completion_interruptible_timeout
>> >> +				(&info->completion, VF610_ADC_TIMEOUT);
>> >> +		*val =3D info->value;
>> >> +
>> >> +		mutex_unlock(&indio_dev->mlock);
>> >> +
>> >> +		if (timeout =3D=3D 0)
>> >> +			return -ETIMEDOUT;
>> >> +
>> >> +		return IIO_VAL_INT;
>> >> +
>> >> +	case IIO_CHAN_INFO_SCALE:
>> >> +		*val =3D info->vref_uv >> info->adc_feature.res_mode;
>> >> +		*val2 =3D 0;
>> >
>> >use IIO_VAL_FRACTIONAL_LOG2 probably
>>
>> Yes, it is better to use IIO_VAL_FRACTIONAL_LOG2, thanks!
>> >
>> >> +		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 =3D iio_priv(indio_dev);
>> >> +
>> >> +	if (readval =3D=3D NULL)
>> >> +		return -EINVAL;
>> >
>> >can this ever happen?
>>
>> No, it never happen. Thanks.
>
>see Lars comment; I was wrong

I have seen Lars comment.


>
>I think reg should be range checked in addition
>
>> static ssize_t iio_debugfs_read_reg(struct file *file, char __user *user=
buf,
>>                               size_t count, loff_t *ppos) {
>>         struct iio_dev *indio_dev =3D file->private_data;
>>         char buf[20];
>>         unsigned val =3D 0;
>>         ssize_t len;
>>         int ret;
>>
>>         ret =3D indio_dev->info->debugfs_reg_access(indio_dev,
>>                                                   indio_dev->cached_reg_=
addr,
>>                                                   0, &val);
>> 	 ...
>> }
>>
>> >
>> >I'd rather check the value of reg
>> >> +
>> >> +	*readval =3D readl(info->regs + reg);
>> >> +
>> >> +	return 0;
>> >> +}
>> >> +
>> >> +static const struct iio_info vf610_adc_iio_info =3D {
>> >> +	.read_raw =3D &vf610_read_raw,
>> >> +	.debugfs_reg_access =3D &vf610_adc_reg_access,
>> >> +	.driver_module =3D THIS_MODULE,
>> >> +};
>> >> +
>> >> +static const struct of_device_id vf610_adc_match[] =3D {
>> >> +	{ .compatible =3D "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 =3D NULL;
>> >> +	struct iio_dev *indio_dev;
>> >> +	struct resource *mem;
>> >> +	int ret =3D -ENODEV;
>> >
>> >initialization of ret and info not needed
>>
>> Got it.
>> >
>> >> +
>> >> +	indio_dev =3D 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 =3D iio_priv(indio_dev);
>> >> +	info->dev =3D &pdev->dev;
>> >> +
>> >> +	mem =3D platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> >> +	info->regs =3D devm_ioremap_resource(&pdev->dev, mem);
>> >> +	if (IS_ERR(info->regs))
>> >> +		return PTR_ERR(info->regs);
>> >> +
>> >> +	info->irq =3D platform_get_irq(pdev, 0);
>> >> +	if (info->irq < 0) {
>> >> +		dev_err(&pdev->dev, "no irq resource?\n");
>> >> +		return -EINVAL;
>> >> +	}
>> >> +
>> >> +	ret =3D 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 =3D %d\n",
>> >> +							info->irq);
>> >> +		return ret;
>> >> +	}
>> >> +
>> >> +	info->clk =3D devm_clk_get(&pdev->dev, "adc");
>> >> +	if (IS_ERR(info->clk)) {
>> >> +		dev_err(&pdev->dev, "failed getting clock, err =3D %ld\n",
>> >> +						PTR_ERR(info->clk));
>> >> +		ret =3D PTR_ERR(info->clk);
>> >> +		return ret;
>> >> +	}
>> >> +
>> >> +	info->vref =3D devm_regulator_get(&pdev->dev, "vref");
>> >> +	if (!IS_ERR(info->vref)) {
>> >> +		ret =3D regulator_enable(info->vref);
>> >> +		if (ret) {
>> >> +			dev_err(&pdev->dev,
>> >> +				"Failed to enable vref regulator: %d\n", ret);
>> >> +			return ret;
>> >> +		}
>> >> +	} else {
>> >> +		info->vref =3D NULL;
>> >> +	}
>> >> +
>> >> +	platform_set_drvdata(pdev, indio_dev);
>> >> +
>> >> +	init_completion(&info->completion);
>> >> +
>> >> +	indio_dev->name =3D dev_name(&pdev->dev);
>> >> +	indio_dev->dev.parent =3D &pdev->dev;
>> >> +	indio_dev->dev.of_node =3D pdev->dev.of_node;
>> >> +	indio_dev->info =3D &vf610_adc_iio_info;
>> >> +	indio_dev->modes =3D INDIO_DIRECT_MODE;
>> >> +	indio_dev->channels =3D vf610_adc_iio_channels;
>> >> +	indio_dev->num_channels =3D ARRAY_SIZE(vf610_adc_iio_channels);
>> >> +
>> >> +	ret =3D devm_iio_device_register(&pdev->dev, indio_dev);
>> >> +	if (ret)
>> >
>> >regulator stays enabled
>> >
>> >> +		return ret;
>> >> +
>> >> +	ret =3D 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 =3D platform_get_drvdata(pdev);
>> >> +	struct vf610_adc *info =3D 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 =3D dev_get_drvdata(dev);
>> >> +	struct vf610_adc *info =3D iio_priv(indio_dev);
>> >> +	int hc_cfg;
>> >> +
>> >> +	/* enter to stop mode */
>> >
>> >comment does not add to clarity of the code, remove?
>>
>> Below code let ADC enter to stop mode when suspend.
>
>yes; VF610_ADC_CONV_DISABLE is pretty descriptive on its own :)
>
>> >
>> >> +	hc_cfg =3D readl(info->regs + ADC_HC0);
>> >> +	hc_cfg |=3D 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 =3D dev_get_drvdata(dev);
>> >> +	struct vf610_adc *info =3D iio_priv(indio_dev);
>> >> +	int ret;
>> >> +
>> >> +	if (info->vref) {
>> >> +		ret =3D regulator_enable(info->vref);
>> >> +		if (ret)
>> >> +			return ret;
>> >> +	}
>> >> +
>> >> +	ret =3D 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 =3D {
>> >> +	.probe          =3D vf610_adc_probe,
>> >> +	.remove         =3D vf610_adc_remove,
>> >> +	.driver         =3D {
>> >> +		.name   =3D DRIVER_NAME,
>> >> +		.owner  =3D THIS_MODULE,
>> >> +		.of_match_table =3D vf610_adc_match,
>> >> +		.pm     =3D &vf610_adc_pm_ops,
>> >> +	},
>> >> +};
>> >> +
>> >> +module_platform_driver(vf610_adc_driver);
>> >> +
>> >> +MODULE_AUTHOR("Fugang Duan <B38611@freescale.com>");
>> >> +MODULE_DESCRIPTION("Freescale VF610 ADC driver");
>> >> +MODULE_LICENSE("GPL v2");
>> >>
>>
>> Peter,
>> thanks for your review.
>>
>> --
>> 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
>>
>
>--
>
>Peter Meerwald
>+43-664-2444418 (mobile)

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

[Jonathan Cameron]

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 
>
>>
>>> +- 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@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.

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@metafoo.de]
Data: Wednesday, November 27, 2013 4:21 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 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc dr=
iver
>
>On 11/27/2013 05:44 AM, Fugang Duan wrote:
>[...]
>>>> +	if (info->vref)
>>>> +		info->vref_uv =3D 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 desc=
ribed
>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 =3D 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 =3D 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 =3D 4;
>>>> +		dev_info(info->dev,
>>>> +			"Miss adc-sam-time property in dt, set to 4.\n");
>>>> +	}
>>>> +
>>>> +	info->adc_feature.hw_average =3D 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 =3D 4;
>>>> +		dev_info(info->dev,
>>>> +			"Miss adc-aver-sam-sel property in dt, set to 4.\n");
>>>> +	}
>>>> +
>>>> +	info->adc_feature.lpm =3D of_property_read_bool(np,
>>>> +"fsl,adc-low-power-
>>> mode");
>>>> +	info->adc_feature.hs_oper =3D 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 stand=
ard
>attributes here instead of inventing your own with cryptic names.
>
>- Lars

Good suggestion, I will use the standard IIO interface for the ADC configur=
ation.

Thanks,
Andy

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

[Fugang Duan]

RnJvbTogSm9uYXRoYW4gQ2FtZXJvbiA8amljMjNAa2VybmVsLm9yZz4NCkRhdGE6IFdlZG5lc2Rh
eSwgTm92ZW1iZXIgMjcsIDIwMTMgNDoxMCBQTQ0KDQo+VG86IER1YW4gRnVnYW5nLUIzODYxMTsg
TWFyayBSdXRsYW5kDQo+Q2M6IHNhY2hpbi5rYW1hdEBsaW5hcm8ub3JnOyBkZXZpY2V0cmVlQHZn
ZXIua2VybmVsLm9yZzsgc2hhd24uZ3VvQGxpbmFyby5vcmc7DQo+TGkgRnJhbmstQjIwNTk2OyBs
aW51eC1paW9Admdlci5rZXJuZWwub3JnDQo+U3ViamVjdDogUkU6IFtQQVRDSCB2MyAzLzNdIERv
Y3VtZW50YXRpb246IGFkZCB0aGUgYmluZGluZyBmaWxlIGZvciBGcmVlc2NhbGUNCj52ZjYxMCBB
REMgZHJpdmVyDQo+DQo+DQo+DQo+RnVnYW5nIER1YW4gPGZ1Z2FuZy5kdWFuQGZyZWVzY2FsZS5j
b20+IHdyb3RlOg0KPj5Gcm9tOiBNYXJrIFJ1dGxhbmQgW21haWx0bzptYXJrLnJ1dGxhbmRAYXJt
LmNvbV0NCj4+RGF0YTogVHVlc2RheSwgTm92ZW1iZXIgMjYsIDIwMTMgMTA6MDkgUE0NCj4+DQo+
Pj5UbzogRHVhbiBGdWdhbmctQjM4NjExDQo+Pj5DYzogamljMjNAa2VybmVsLm9yZzsgc2FjaGlu
LmthbWF0QGxpbmFyby5vcmc7DQo+PmRldmljZXRyZWVAdmdlci5rZXJuZWwub3JnOw0KPj4+c2hh
d24uZ3VvQGxpbmFyby5vcmc7IExpIEZyYW5rLUIyMDU5NjsgbGludXgtaWlvQHZnZXIua2VybmVs
Lm9yZw0KPj4+U3ViamVjdDogUmU6IFtQQVRDSCB2MyAzLzNdIERvY3VtZW50YXRpb246IGFkZCB0
aGUgYmluZGluZyBmaWxlIGZvcg0KPj5GcmVlc2NhbGUNCj4+PnZmNjEwIEFEQyBkcml2ZXINCj4+
Pg0KPj4+T24gVHVlLCBOb3YgMjYsIDIwMTMgYXQgMTA6NTY6MzRBTSArMDAwMCwgRnVnYW5nIER1
YW4gd3JvdGU6DQo+Pj4+IFRoZSBwYXRjaCBhZGRzIHRoZSBiaW5kaW5nIGZpbGUgZm9yIEZyZWVz
Y2FsZSB2ZjYxMCBBREMgZHJpdmVyLg0KPj4+Pg0KPj4+PiBTaWduZWQtb2ZmLWJ5OiBGdWdhbmcg
RHVhbiA8QjM4NjExQGZyZWVzY2FsZS5jb20+DQo+Pj4+IC0tLQ0KPj4+PiAgLi4uL2RldmljZXRy
ZWUvYmluZGluZ3MvaWlvL2FkYy92ZjYxMC1hZGMudHh0ICAgICAgfCAgIDU3DQo+Pj4rKysrKysr
KysrKysrKysrKysrKw0KPj4+PiAgMSBmaWxlcyBjaGFuZ2VkLCA1NyBpbnNlcnRpb25zKCspLCAw
IGRlbGV0aW9ucygtKQ0KPj4+Pg0KPj4+PiBkaWZmIC0tZ2l0IGEvRG9jdW1lbnRhdGlvbi9kZXZp
Y2V0cmVlL2JpbmRpbmdzL2lpby9hZGMvdmY2MTAtYWRjLnR4dA0KPj4+PiBiL0RvY3VtZW50YXRp
b24vZGV2aWNldHJlZS9iaW5kaW5ncy9paW8vYWRjL3ZmNjEwLWFkYy50eHQNCj4+Pj4gbmV3IGZp
bGUgbW9kZSAxMDA2NDQNCj4+Pj4gaW5kZXggMDAwMDAwMC4uNDEwMTUxNg0KPj4+PiAtLS0gL2Rl
di9udWxsDQo+Pj4+ICsrKyBiL0RvY3VtZW50YXRpb24vZGV2aWNldHJlZS9iaW5kaW5ncy9paW8v
YWRjL3ZmNjEwLWFkYy50eHQNCj4+Pj4gQEAgLTAsMCArMSw1NyBAQA0KPj4+PiArRnJlZXNjYWxl
IHZmNjEwIEFuYWxvZyB0byBEaWdpdGFsIENvbnZlcnRlciBiaW5kaW5ncw0KPj4+PiArDQo+Pj4+
ICtUaGUgZGV2aWNldHJlZSBiaW5kaW5ncyBhcmUgZm9yIHRoZSBuZXcgQURDIGRyaXZlciB3cml0
dGVuIGZvcg0KPj4+PiArdmY2MTAvaS5NWDZzbHggYW5kIHVwd2FyZCBTb0NzIGZyb20gRnJlZXNj
YWxlLg0KPj4+PiArDQo+Pj4+ICtSZXF1aXJlZCBwcm9wZXJ0aWVzOg0KPj4+PiArLSBjb21wYXRp
YmxlOiBTaG91bGQgYmUgImZzbCx2ZjYxMC1hZGMiDQo+Pj4NCj4+PnMvYmUvY29udGFpbi8NCj4+
Pg0KPj4+PiArLSByZWc6IE9mZnNldCBhbmQgbGVuZ3RoIG9mIHRoZSByZWdpc3RlciBzZXQgZm9y
IHRoZSBkZXZpY2UNCj4+Pj4gKy0gaW50ZXJydXB0czogU2hvdWxkIGNvbnRhaW4gdGhlIGludGVy
cnVwdCBmb3IgdGhlIGRldmljZQ0KPj4+PiArLSBjbG9ja3M6IFRoZSBjbG9ja3MgbmVlZGVkIGJ5
IHRoZSBBREMgY29udHJvbGxlcg0KPj4+DQo+Pj5Ib3cgbWFueT8gV2hpY2ggb25lcz8NCj4+Pg0K
Pj4+PiArLSBjbG9jay1uYW1lczogdGhlIG5hbWUgb2YgdGhlIGNsb2Nrcw0KPj4+DQo+Pj5FaXRo
ZXIgZGVmaW5lIHRoZSBzZXQgb2YgbmFtZXMsIG9yIGRvbid0IHVzZSBjbG9jay1uYW1lcy4gSXQn
cyB1c2VsZXNzDQo+PmlmIGl0DQo+Pj5kb2Vzbid0IHRlbGwgeW91IGFueXRoaW5nLg0KPj4+DQo+
Pj4+ICsNCj4+Pj4gK09wdGlvbmFsIHByb3BlcnRpZXM6DQo+Pj4+ICstIGZzbCxhZGMtaW8tcGlu
Y3RsOiBFbmFibGUgZmllbGQgZm9yIHRoZSBJL08gcG9ydCBjb250cm9sIG9mIE1DVQ0KPj5waW5z
IHVzZWQNCj4+PmFzIGFuYWxvZyBpbnB1dHMuDQo+Pj4+ICstIGZzbCxhZGMtdnJlZjogQURDIHJl
ZnJlbmNlIHZvbHRhZ2UgdmFsdWUsIHVuaXQgaXMgdVYuDQo+Pj4NCj4+PkNhbiB5b3Ugbm90IHF1
ZXJ5IHRoZSByZWd1bGF0b3IgdG8gZmlndXJlIHRoaXMgb3V0Pw0KPj4NCj4+VGhpcyBpcyBvcHRp
b25hbCwgc29tZSBib2FyZCBkZXNpZ24gaGF2ZSBubyBmaXhlZCByZWd1bGF0b3IsIGp1c3QNCj4+
c3VwcGx5IGZpeGVkIHZvbHRhZ2UgZnJvbSBNQ1UgZGlnaXRhbCBwb3dlciB3aGljaCBDYW5ub3Qg
aGFuZGxlZCBieSB0aGUNCj4+bW9kdWxlLg0KPkZpeGVkIHJlZ3VsYXRvcnMgYXJlIHVzZWQgd2hl
biB0aGluZ3MgZWl0aGVyIHN1cHBsaWVkIGJ5IGFuIGFjdHVhbCBmaXhlZCByZWcgb3INCj53aGVu
ICBkaXJlY3RseSBjb25uZWN0ZWQgdG8gYW5vdGhlciBzdXBwbHkuIEl0IGRvZXMgbm90IGhhdmUg
dG8gY29ycmVzcG9uZCB0byBhDQo+Yml0IG9mIHJlYWwgIGhhcmR3YXJlIHRob3VnaCB0aGF0IE1D
VSBkaWdpdGFsIHBvd2VyIGlzIGNvbWluZyBmcm9tIHNvbWV3aGVyZSENCj4NCj5IZW5jZSB0aGlz
IGRlZmluaXRlbHkgd2FudHMgdG8gYmUgYSByZWd1bGF0b3INCg0KR29vZCBzdWdnZXN0aW9uLCB0
aGFua3MgIQ0KDQo+Pg0KPj4+DQo+Pj4+ICstIGZzbCxhZGMtY2xrLWRpdjogQ3VycmVudCBjbG9j
ayBkaXZpZGVyIHZhbHVlLCBzdWNoIGFzIDEsMiw0LDgsMTYNCj4+YW5kIHNvIG9uLg0KPj4+DQo+
Pj5Db3VsZCB5b3UgZWxhYm9yYXRlIG9uIHRoaXM/IFdoYXQncyBpdCB1c2VkIGZvciBhbmQgd2h5
IGlzIGl0IG5lZWRlZD8NCj4+DQo+PkV4cDogQURDIGNsb2NrIHNvdXJjZSBpcyBpcGcgYnVzIGNs
b2NrLCB0aGUgY2xrIGRpdmlkZXIgY2FuIGNvbnRyb2wgdGhlDQo+PkFEQyBzYW1wbGUgc3BlZWQg
d2hpbGUgb3RoZXIgc2V0dGluZyBkb24ndCBjaGFuZ2U6DQo+PglUaGUgc2FtcGxlIHNwZWVkIHRo
YXQgY2xvY2sgZGl2aWRlciBpcyBlcXVhbCB0byAxIGlzIHR3aWNlICB0aGFuIHRoZQ0KPj5kaXZp
ZGVyIGlzIGVxdWFsIHRvIDIuDQo+Pg0KPj5Bbnl3YXksIGl0IGNhbiBjb250cm9sIHNhbXBsZSBz
cGVlZCBieSB1c2VyLg0KPklmIGxlZnQgaW4gZGV2aWNlIHRyZWUgdGhlbiB0aGlzIG1pZ2h0IGJl
IGJlc3QgaW1wbGVtZW50ZWQgYXMgYSB2ZXJ5IHNpbXBsZQ0KPmNsb2NrLg0KPkkgd291bGQgcHJl
ZmVyIGl0IHRvIGJlIGNvbnRyb2xsZWQgZnJvbSB1c2VyIHNwYWNlIGFzIHNhbXBsaW5nX2ZyZXF1
ZW5jeS4NCj4+DQo+Pj4NCj4+Pj4gKy0gZnNsLGFkYy1yZXM6IEFEQyBjb252ZXJzaW9uIG1vZGUg
c2VsZWN0aW9uLCBzdWNoIGFzIDggZm9yIDgtYml0LA0KPj4xMCBmb3INCj4+PjEwLWJpdCwgMTIg
Zm9yIDEyLWJpdCBtb2RlLg0KPj4+DQo+Pj5UaGlzIHNvdW5kcyBsaWtlIHNvbWV0aGluZyB0aGF0
IGNvdWxkIGJlIGNoYW5nZWQgYXQgcnVudGltZS4gV2h5IGRvZXMNCj4+dGhpcyBuZWVkDQo+Pj50
byBiZSBjb25maWd1cmVkIGluIHRoZSBEVD8NCj4+Pg0KPj4+PiArLSBmc2wsYWRjLXNhbS10aW1l
OiBBREMgc2FtcGxlIHRpbWUgZHVyYXRpb24sIG51bWJlciBvZiBBREMgY2xvY2tzLA0KPj4+PiAr
c3VjaCBhcyAyLCA0LCA2LCA4LCAxMiwgMTYsIDIwLCAyNA0KPj4+DQo+Pj5MaWtld2lzZS4NCj4+
Pg0KPj4+UGxlYXNlIGRvbid0IHBvaW5sdGVzc2x5IGFiYnJldmlhdGUsICJzYW1wbGUiIGlzIG11
Y2ggYmV0dGVyIHRoYW4NCj4+InNhbSINCj4+PmhlcmUuLi4NCj4+Pg0KPj4+PiArLSBmc2wsYWRj
LWF2ZXItc2FtLXNlbDogRGV0ZXJtaW5lcyBob3cgbWFueSBBREMgY29udmVyc2lvbnMgd2lsbCBi
ZQ0KPj5hdmVyYWdlZA0KPj4+dG8gY3JlYXRlIHRoZSBBREMgYXZlcmFnZSByZXN1bHQuDQo+Pj4+
ICsJCQlUaGUgT3B0aW9uYWwgdmFsdWUgaXMgNCwgOCwgMTYsIDMyLg0KPj4+DQo+Pj5MaWtld2lz
ZS4NCj4+DQo+Pg0KPj5Gb3IgYWJvdmUgQURDIGNvbmZpZ3VyYXRpb24sIEFEQyBjb252ZXJzaW9u
IG1vZGUsIHNhbXBsZSB0aW1lIGR1cmF0aW9uLA0KPj5oYXJkd2FyZSBhdmVyYWdlIHNhbXBsZSBz
ZWxlY3Rpb24sIGFuZCBzbyBvbi4NCj4+QWxsIG9mIHRoZW0gY2FuIGNoYW5nZWQgYXQgcnVudGlt
ZS4gVGhlIG9ubHkgbWV0aG9kIGlzIHRvIGludHJvZHVjZSB0aGUNCj4+aW9jdGwgaW50ZXJmYWNl
IGZvciB1c2VyIHRvIGNoYW5nZSB0aGVtIGF0IHJ1bnRpbWUuDQo+PkhvdyBhYm91dCB5b3UgdGhp
bmsgPw0KPk5vIGl0IGlzIG5vdC4gTGlrZSBhbGwgc2ltaWxhciBjb250cm9scyB0aGVzZSBTaG91
bGQgYmUgdmlhIHN5c2ZzIGF0dHJpYnV0ZXMNCj5jcmVhdGVkIHZpYSBUaGUgc3RhbmRhcmQgY2hh
bm5lbCBzdHJ1Y3R1cmVzIGluIElJTy4NCj5TYW1wbGUgZHVyYXRpb24gbWlnaHQgYmUgaW50ZWdy
YXRpb25fdGltZSAgb3Igc2FtcGxpbmdfZnJlcXVlbmN5IGRlcGVuZGluZyBvbg0KPml0cyBleGFj
dCBzZW1hbnRpY3MuIEF2ZXJhZ2luZyBpcyBhIGZvcm0gb2YgbG93IHBhc3MgZmlsdGVyIGFuZCBz
aG91bGQgYmUNCj5zcGVjaWZpZWQgYXMgc3VjaC4NCj4NCj5JZiBpdCBpcyBub3QgYWxyZWFkeSBp
biB0aGUgYWJpIHRoZW4gcHJvcG9zZSBzb21ldGhpbmcgbmV3Lg0KPg0KPlRoZSBvbmx5IG9uZSBo
ZXJlIHRoYXQgSSBjYW4gc2VlIGFuIGlzc3VlIHdpdGggZG9pbmcgdGhhdCB3YXkgaXMgdGhlIHJl
c29sdXRpb24uDQo+U28gZmFyIGl0IGhhc24ndCBhY3R1YWxseSBtYWRlIHNlbnNlIHRvIG1ha2Ug
dGhpcyBjb250cm9sbGFibGUuLg0KPkNvdXBsZSBvZiBxdWVzdGlvbnMgb24gdGhhdC4NCj4xLiBX
aGF0IGVmZmVjdCBvbiBzcGVlZCBvZiBzYW1wbGluZyBkb2VzIGNoYW5naW5nIHRoaXMgaGF2ZT8g
Tm90ZSB0aGF0IHdlDQo+cHJvYmFibHkgZG8gbm90IGNhcmUgYWJvdXQgdGhpcyBpZiB1c2luZyBz
eXNmcyByZWFkaW5nLg0KPjIuIFdoYXQgb3RoZXIgZGlzYWR2YW50YWdlcyBhcmUgdGhlcmUgdG8g
YWx3YXlzIHJ1bm5pbmcgaW4gMTIgYml0IG1vZGU/DQo+DQo+SSBoYXZlIG5vIHByb2JsZW0gd2l0
aCBhZGRpbmcgd3JpdGUgc3VwcG9ydCBmb3IgdGhlIHR5cGUgb2YgYnVmZmVyIGVsZW1lbnRzDQo+
dGhvdWdoIGl0IHdvdWxkIHJlcXVpcmUgc29tZSBJSU8gY29yZSBpbmZyYXN0cnVjdHVyZS4NCj5I
b3dldmVyIEkgZG8gd2FudCBhIHdlbGwgZGVzY3JpYmVkIHJlYXNvbiB0byBib3RoZXIgd2l0aCB0
aGUgYWRkZWQgY29tcGxleGl0eS4NCj4NClRoYW5rcyB2ZXJ5IG11Y2ghIFlvdXIgc3VnZ2VzdGlv
biBpcyB2ZXJ5IHVzZWZ1bCBmb3IgbWUuIEkgd2lsbCB1c2UgdGhlIHN0YW5kYXJkIHN5cyBpbnRl
cmZhY2UgZm9yIEFEQyBjb25maWd1cmF0aW9uLg0KDQpBbmQgZm9yIHlvdXIgcXVlc3Rpb246DQox
LiBlZmZlY3Qgb24gc3BlZWQgb2Ygc2FtcGxpbmc6IA0KCS0gQURDIHNhbXBsZSB0aW1lIGR1cmF0
aW9uLCBudW1iZXIgb2YgQURDIGNsb2Nrcywgc3VjaCBhcyAyLCA0LCA2LCA4LCAxMiwgMTYsIDIw
LCAyNA0KCS0gY2xvY2ssIHRoZSBjbG9jayBzb3VyY2UgaXMgaXBnIGJ1cyBjbG9jaywgY3VycmVu
dCBjbG9jayBkaXZpZGVyIHZhbHVlIGZvciBBREMsIHN1Y2ggYXMgMSwyLDQsOCwxNiBhbmQgc28g
b24NCgktIGlmIGhhcmR3YXJlIGF2ZXJhZ2UgZnVuY3Rpb24gZW5hYmxlLA0KCQlUaGUgQURDIGF2
ZXJhZ2Ugc2FtcGxlIG51bWJlciBhbHNvIGFmZmVjdCBzcGVlZCwgZGV0ZXJtaW5lcyBob3cgbWFu
eSBBREMgY29udmVyc2lvbnMgd2lsbCBiZSBhdmVyYWdlZCB0byBjcmVhdGUgdGhlIEFEQyBhdmVy
YWdlIHJlc3VsdC4NCglBbGwgb2YgYWJvdmUgc2V0dGluZyBhZmZlY3QgdGhlIHNwZWVkIG9mIHNh
bXBsaW5nLg0KDQoJSWYgdXNlIHRoZSBzeXNmcyByZWFkaW5nLCB0aGUgc3BlZWQgZG9uJ3QgbmVl
ZCBjYXJlLiANCglTbyBJIGhhdmUgc29tZSBxdWVzdGlvbnM6DQoJYS4gRm9yIGhpZ2ggc3BlZWQg
c2FtcGxpbmcsIElJTyBzdWJzeXN0ZW0gaG93IHRvIGhhbmRsZSBpdCBpZiB0aGVyZSBubyBpb2N0
bCBpbnRlcmZhY2UgZm9yIHVzZXIgPyANCgkgICBpdCBpcyBpaW8gYnVmZmVyL2ZpZm8gPw0KCWIu
IEZvciBzb2Z0d2FyZSB0cmlnZ2VyLCB0aGUgc3BlZWQgb2Ygc2FtcGxpbmcgYWxzbyBkb24ndCBu
ZWVkIHRvIGNhcmUsIGlzIGl0IHJpZ2h0ID8gDQoJYy4gSWYgdXNlIGNhc2UgbmVlZCBoaWdoIHNw
ZWVkIHNhbXBsaW5nLCB0aGVyZSBtdXN0IGhhdmUgaGFyZXdhcmUgdHJpZ2dlciBhbmQgaWlvIGJ1
ZmZlciBmb3IgaXQsIHJpZ2h0ID8gDQogDQoyLiBubyBkaXNhZHZhbnRhZ2UsIEkgaGF2ZSB2YWxp
ZGF0ZWQgQURDIGNhbiB3b3JrIGZpbmUgYXQgOC1iaXQsIDEwLWJpdCwgMTItYml0IG1vZGUuDQoN
Cj4+DQo+Pj4NCj4+Pj4gKy0gZnNsLGFkYy1ody1hdmVyLWVuOiBCb29sIHR5cGUgdG8gZGVjaWRl
IGVuYWJsZSBoYXJkd2FyZSBhdmVyYWdlDQo+PmZ1bmN0aW9uLg0KPj4+DQo+Pj5XaGVuIHdvdWxk
IHlvdSB3bmF0IHRoaXMgYW5kIHdoZW4gd291bGRuJ3QgeW91Pw0KPj4+DQo+Pj5TaW1pbGFybHks
ICJhdmVyYWdlcyIgaXMgZmFyIGNsZWFyZXIgdGhhbiAiYXZlciIuDQo+Pg0KPj5PaywgSSBzZWUu
IFRoYW5rcy4NCj4+Pg0KPj4+PiArLSBmc2wsYWRjLWxvdy1wb3dlci1tb2RlOiBCb29sIHR5cGUg
dG8gZGVjaWRlIGVuYWJsZSBBREMgbG93IHBvd2VyDQo+Pm1vZGUuDQo+Pj4NCj4+PlNpbWlsYXJs
eT8NCj5UaGlzIG9uZSBpcyBpbnRlcmVzdGluZy4gV2hhdCBlZmZlY3RzIGRvZXMgaXQgaGF2ZSBp
biB0aGlzIGRldmljZT8gU29tZXRpbWVzIGl0DQo+aXMgYSAgc2FtcGxpbmcgZnJlcXVlbmN5IHJl
c3RyaWN0aW9uLi4uDQo+Pj4NCj4+Pj4gKy0gZnNsLGFkYy1oaWdoLXNwZWVkLWNvbnY6IEJvb2wg
dHlwZSB0byBkZWNpZGUgZW5hYmxlIEFEQyBoaWdoDQo+PnNwZWVkIG1vZGUuDQo+Pj4NCj4+PlNp
bWlsYXJseT8NCj4+Pg0KPj4+PiArLSB2cmVmOiBUaGUgcmVndWxhdG9yIHRvIHN1cHBvcnQgQURD
IHJlZnJlbmNlIHZvbHRhZ2UuDQo+Pj4NCj4+PnMvdnJlZi92cmVmLXN1cHBseS8NCj4+Pg0KPj4+
VGhhbmtzLA0KPj4+TWFyay4NCj4+DQo+PlRoYW5rcyBmb3IgeW91ciByZXZpZXcuDQo+PlN1bW1h
cnksIHRoZXJlIGhhdmUgbWFueSBjb21tZW50cyB3aHkgZG9uJ3QgbGV0IHNvbWUgQURDIGNvbmZp
Z3VyYXRpb24NCj4+Y2hhbmdlIGF0IHJ1bnRpbWUuDQo+PlRoZSBkcml2ZXIgc3RpbGwgZG9uJ3Qg
YWRkIGlvY3RsIGludGVyZmFjZSBmb3IgdXNlciB0byBjb25maWd1cmF0ZSB0aGUNCj4+QURDIHNl
dHRpbmcuIFNvIHRoZSBzZXR0aW5nIGFyZSBnb3QgZnJvbSBEVC4NCj4+DQo+PkRvIHlvdSBoYXZl
IG9uZSBnb29kIG1ldGhvZCB0byBjaGFuZ2UgdGhlIGNvbmZpZ3VyYXRpb24gYXQgcnVudGltZSA/
DQo+U2VlIGFib3ZlIDopDQo+DQo+SW4gc2hvcnQuLi5BZGQgcmVsZXZhbnQgYml0cyB0byBpbmZv
X21hc2tfKiBpbiBhbGwgdGhlIGlpb19DaGFuX3NwZWMgc3RydWN0dXJlcw0KPnRoZW4gcHV0IHRo
ZSAgcmVsZXZhbnQgaGFuZGxpbmcgaW4gdGhlIHJlYWRfcmF3IGFuZCB3cml0ZV9yYXcgaWlvX2lu
Zm8NCj5jYWxsYmFja3MuDQo+Pg0KPj5UaGFua3MsDQo+PkFuZHkNCj4+DQo+Pi0tDQo+PlRvIHVu
c3Vic2NyaWJlIGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZSBsaW51
eC1paW8iIGluDQo+PnRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJu
ZWwub3JnIE1vcmUgbWFqb3Jkb21vIGluZm8NCj4+YXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcv
bWFqb3Jkb21vLWluZm8uaHRtbA0KPg0KPi0tDQo+U2VudCBmcm9tIG15IEFuZHJvaWQgcGhvbmUg
d2l0aCBLLTkgTWFpbC4gUGxlYXNlIGV4Y3VzZSBteSBicmV2aXR5Lg0KDQpUaGFua3MgZm9yIHlv
dXIgcmV2aWV3IGFuZCBhbnN3ZXJzLg0KDQpBbmR5DQo=

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@arm.com>
> Data: Tuesday, November 26, 2013 10:26 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 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@freescale.com>
> >> ---
> >>  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@arm.com]
Data: Wednesday, November 27, 2013 10:18 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 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@arm.com>
>> Data: Tuesday, November 26, 2013 10:26 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 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@freescale.com>
>> >> ---
>> >>  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