* [PATCH v3 0/3] *** iio:adc:imx: Add Freescale vf610 ADC driver support ***
@ 2013-11-26 10:56 Fugang Duan
2013-11-26 10:56 ` [PATCH v3 1/3] ARM: dts: vf610-twr: Add ADC support Fugang Duan
` (2 more replies)
0 siblings, 3 replies; 23+ messages in thread
From: Fugang Duan @ 2013-11-26 10:56 UTC (permalink / raw)
To: jic23, sachin.kamat; +Cc: devicetree, shawn.guo, b20596, linux-iio
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
^ permalink raw reply [flat|nested] 23+ messages in thread* [PATCH v3 1/3] ARM: dts: vf610-twr: Add ADC support 2013-11-26 10:56 [PATCH v3 0/3] *** iio:adc:imx: Add Freescale vf610 ADC driver support *** Fugang Duan @ 2013-11-26 10:56 ` Fugang Duan 2013-11-26 10:56 ` [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver Fugang Duan 2013-11-26 10:56 ` [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver Fugang Duan 2 siblings, 0 replies; 23+ messages in thread From: Fugang Duan @ 2013-11-26 10:56 UTC (permalink / raw) To: jic23, sachin.kamat; +Cc: devicetree, shawn.guo, b20596, linux-iio 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-26 10:56 [PATCH v3 0/3] *** iio:adc:imx: Add Freescale vf610 ADC driver support *** Fugang Duan 2013-11-26 10:56 ` [PATCH v3 1/3] ARM: dts: vf610-twr: Add ADC support Fugang Duan @ 2013-11-26 10:56 ` Fugang Duan 2013-11-26 11:51 ` Lars-Peter Clausen ` (2 more replies) 2013-11-26 10:56 ` [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver Fugang Duan 2 siblings, 3 replies; 23+ messages in thread From: Fugang Duan @ 2013-11-26 10:56 UTC (permalink / raw) To: jic23, sachin.kamat; +Cc: devicetree, shawn.guo, b20596, linux-iio 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-26 10:56 ` [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver Fugang Duan @ 2013-11-26 11:51 ` Lars-Peter Clausen 2013-11-27 4:44 ` Fugang Duan 2013-11-26 12:24 ` Peter Meerwald 2013-11-26 14:25 ` Mark Rutland 2 siblings, 1 reply; 23+ messages in thread From: Lars-Peter Clausen @ 2013-11-26 11:51 UTC (permalink / raw) To: Fugang Duan; +Cc: jic23, sachin.kamat, devicetree, shawn.guo, b20596, linux-iio 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; > +} [...] ^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-26 11:51 ` Lars-Peter Clausen @ 2013-11-27 4:44 ` Fugang Duan 2013-11-27 8:20 ` Lars-Peter Clausen 0 siblings, 1 reply; 23+ messages in thread From: Fugang Duan @ 2013-11-27 4:44 UTC (permalink / raw) To: Lars-Peter Clausen Cc: jic23@kernel.org, sachin.kamat@linaro.org, devicetree@vger.kernel.org, shawn.guo@linaro.org, Frank Li, linux-iio@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-27 4:44 ` Fugang Duan @ 2013-11-27 8:20 ` Lars-Peter Clausen 2013-11-27 8:31 ` Fugang Duan 0 siblings, 1 reply; 23+ messages in thread From: Lars-Peter Clausen @ 2013-11-27 8:20 UTC (permalink / raw) To: Fugang Duan Cc: jic23@kernel.org, sachin.kamat@linaro.org, devicetree@vger.kernel.org, shawn.guo@linaro.org, Frank Li, linux-iio@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-27 8:20 ` Lars-Peter Clausen @ 2013-11-27 8:31 ` Fugang Duan 0 siblings, 0 replies; 23+ messages in thread From: Fugang Duan @ 2013-11-27 8:31 UTC (permalink / raw) To: Lars-Peter Clausen Cc: jic23@kernel.org, sachin.kamat@linaro.org, devicetree@vger.kernel.org, shawn.guo@linaro.org, Frank Li, linux-iio@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-26 10:56 ` [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver Fugang Duan 2013-11-26 11:51 ` Lars-Peter Clausen @ 2013-11-26 12:24 ` Peter Meerwald 2013-11-26 12:49 ` Lars-Peter Clausen 2013-11-27 2:57 ` Fugang Duan 2013-11-26 14:25 ` Mark Rutland 2 siblings, 2 replies; 23+ messages in thread From: Peter Meerwald @ 2013-11-26 12:24 UTC (permalink / raw) To: Fugang Duan; +Cc: jic23, shawn.guo, linux-iio 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) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-26 12:24 ` Peter Meerwald @ 2013-11-26 12:49 ` Lars-Peter Clausen 2013-11-27 2:57 ` Fugang Duan 1 sibling, 0 replies; 23+ messages in thread From: Lars-Peter Clausen @ 2013-11-26 12:49 UTC (permalink / raw) To: Peter Meerwald; +Cc: Fugang Duan, jic23, shawn.guo, linux-iio 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-26 12:24 ` Peter Meerwald 2013-11-26 12:49 ` Lars-Peter Clausen @ 2013-11-27 2:57 ` Fugang Duan 2013-11-27 7:17 ` Peter Meerwald 1 sibling, 1 reply; 23+ messages in thread From: Fugang Duan @ 2013-11-27 2:57 UTC (permalink / raw) To: Peter Meerwald Cc: jic23@kernel.org, shawn.guo@linaro.org, linux-iio@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-27 2:57 ` Fugang Duan @ 2013-11-27 7:17 ` Peter Meerwald 2013-11-27 7:30 ` Fugang Duan 0 siblings, 1 reply; 23+ messages in thread From: Peter Meerwald @ 2013-11-27 7:17 UTC (permalink / raw) To: Fugang Duan Cc: jic23@kernel.org, shawn.guo@linaro.org, linux-iio@vger.kernel.org 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) ^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-27 7:17 ` Peter Meerwald @ 2013-11-27 7:30 ` Fugang Duan 0 siblings, 0 replies; 23+ messages in thread From: Fugang Duan @ 2013-11-27 7:30 UTC (permalink / raw) To: Peter Meerwald Cc: jic23@kernel.org, shawn.guo@linaro.org, linux-iio@vger.kernel.org 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) ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-26 10:56 ` [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver Fugang Duan 2013-11-26 11:51 ` Lars-Peter Clausen 2013-11-26 12:24 ` Peter Meerwald @ 2013-11-26 14:25 ` Mark Rutland 2013-11-27 5:37 ` Fugang Duan 2 siblings, 1 reply; 23+ messages in thread From: Mark Rutland @ 2013-11-26 14:25 UTC (permalink / raw) To: Fugang Duan Cc: jic23@kernel.org, sachin.kamat@linaro.org, devicetree@vger.kernel.org, shawn.guo@linaro.org, b20596@freescale.com, linux-iio@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-26 14:25 ` Mark Rutland @ 2013-11-27 5:37 ` Fugang Duan 2013-11-27 14:18 ` Mark Rutland 0 siblings, 1 reply; 23+ messages in thread From: Fugang Duan @ 2013-11-27 5:37 UTC (permalink / raw) To: Mark Rutland Cc: jic23@kernel.org, sachin.kamat@linaro.org, devicetree@vger.kernel.org, shawn.guo@linaro.org, Frank Li, linux-iio@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-27 5:37 ` Fugang Duan @ 2013-11-27 14:18 ` Mark Rutland 2013-11-28 1:25 ` Fugang Duan 0 siblings, 1 reply; 23+ messages in thread From: Mark Rutland @ 2013-11-27 14:18 UTC (permalink / raw) To: Fugang Duan Cc: jic23@kernel.org, sachin.kamat@linaro.org, devicetree@vger.kernel.org, shawn.guo@linaro.org, Frank Li, linux-iio@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver 2013-11-27 14:18 ` Mark Rutland @ 2013-11-28 1:25 ` Fugang Duan 0 siblings, 0 replies; 23+ messages in thread From: Fugang Duan @ 2013-11-28 1:25 UTC (permalink / raw) To: Mark Rutland Cc: jic23@kernel.org, sachin.kamat@linaro.org, devicetree@vger.kernel.org, shawn.guo@linaro.org, Frank Li, linux-iio@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver 2013-11-26 10:56 [PATCH v3 0/3] *** iio:adc:imx: Add Freescale vf610 ADC driver support *** Fugang Duan 2013-11-26 10:56 ` [PATCH v3 1/3] ARM: dts: vf610-twr: Add ADC support Fugang Duan 2013-11-26 10:56 ` [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver Fugang Duan @ 2013-11-26 10:56 ` Fugang Duan 2013-11-26 13:37 ` Shawn Guo 2013-11-26 14:09 ` Mark Rutland 2 siblings, 2 replies; 23+ messages in thread From: Fugang Duan @ 2013-11-26 10:56 UTC (permalink / raw) To: jic23, sachin.kamat; +Cc: devicetree, shawn.guo, b20596, linux-iio 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 ^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver 2013-11-26 10:56 ` [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver Fugang Duan @ 2013-11-26 13:37 ` Shawn Guo 2013-11-27 1:38 ` Fugang Duan 2013-11-26 14:09 ` Mark Rutland 1 sibling, 1 reply; 23+ messages in thread From: Shawn Guo @ 2013-11-26 13:37 UTC (permalink / raw) To: Fugang Duan; +Cc: jic23, sachin.kamat, devicetree, b20596, linux-iio 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver 2013-11-26 13:37 ` Shawn Guo @ 2013-11-27 1:38 ` Fugang Duan 0 siblings, 0 replies; 23+ messages in thread From: Fugang Duan @ 2013-11-27 1:38 UTC (permalink / raw) To: Shawn Guo Cc: jic23@kernel.org, sachin.kamat@linaro.org, devicetree@vger.kernel.org, Frank Li, linux-iio@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver 2013-11-26 10:56 ` [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver Fugang Duan 2013-11-26 13:37 ` Shawn Guo @ 2013-11-26 14:09 ` Mark Rutland 2013-11-27 6:05 ` Fugang Duan 1 sibling, 1 reply; 23+ messages in thread From: Mark Rutland @ 2013-11-26 14:09 UTC (permalink / raw) To: Fugang Duan Cc: jic23@kernel.org, sachin.kamat@linaro.org, devicetree@vger.kernel.org, shawn.guo@linaro.org, b20596@freescale.com, linux-iio@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver 2013-11-26 14:09 ` Mark Rutland @ 2013-11-27 6:05 ` Fugang Duan 2013-11-27 8:09 ` Jonathan Cameron 0 siblings, 1 reply; 23+ messages in thread From: Fugang Duan @ 2013-11-27 6:05 UTC (permalink / raw) To: Mark Rutland Cc: jic23@kernel.org, sachin.kamat@linaro.org, devicetree@vger.kernel.org, shawn.guo@linaro.org, Frank Li, linux-iio@vger.kernel.org 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 ^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver 2013-11-27 6:05 ` Fugang Duan @ 2013-11-27 8:09 ` Jonathan Cameron 2013-11-27 8:55 ` Fugang Duan 0 siblings, 1 reply; 23+ messages in thread From: Jonathan Cameron @ 2013-11-27 8:09 UTC (permalink / raw) To: Fugang Duan, Mark Rutland Cc: sachin.kamat@linaro.org, devicetree@vger.kernel.org, shawn.guo@linaro.org, Frank Li, linux-iio@vger.kernel.org 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. ^ permalink raw reply [flat|nested] 23+ messages in thread
* RE: [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver 2013-11-27 8:09 ` Jonathan Cameron @ 2013-11-27 8:55 ` Fugang Duan 0 siblings, 0 replies; 23+ messages in thread From: Fugang Duan @ 2013-11-27 8:55 UTC (permalink / raw) To: Jonathan Cameron, Mark Rutland Cc: sachin.kamat@linaro.org, devicetree@vger.kernel.org, shawn.guo@linaro.org, Frank Li, linux-iio@vger.kernel.org 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= ^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2013-11-28 1:26 UTC | newest] Thread overview: 23+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-11-26 10:56 [PATCH v3 0/3] *** iio:adc:imx: Add Freescale vf610 ADC driver support *** Fugang Duan 2013-11-26 10:56 ` [PATCH v3 1/3] ARM: dts: vf610-twr: Add ADC support Fugang Duan 2013-11-26 10:56 ` [PATCH v3 2/3] iio:adc:imx: add Freescale Vybrid vf610 adc driver Fugang Duan 2013-11-26 11:51 ` Lars-Peter Clausen 2013-11-27 4:44 ` Fugang Duan 2013-11-27 8:20 ` Lars-Peter Clausen 2013-11-27 8:31 ` Fugang Duan 2013-11-26 12:24 ` Peter Meerwald 2013-11-26 12:49 ` Lars-Peter Clausen 2013-11-27 2:57 ` Fugang Duan 2013-11-27 7:17 ` Peter Meerwald 2013-11-27 7:30 ` Fugang Duan 2013-11-26 14:25 ` Mark Rutland 2013-11-27 5:37 ` Fugang Duan 2013-11-27 14:18 ` Mark Rutland 2013-11-28 1:25 ` Fugang Duan 2013-11-26 10:56 ` [PATCH v3 3/3] Documentation: add the binding file for Freescale vf610 ADC driver Fugang Duan 2013-11-26 13:37 ` Shawn Guo 2013-11-27 1:38 ` Fugang Duan 2013-11-26 14:09 ` Mark Rutland 2013-11-27 6:05 ` Fugang Duan 2013-11-27 8:09 ` Jonathan Cameron 2013-11-27 8:55 ` Fugang Duan
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).