Add DAC driver support for Vybrid SoC
Subsystems (2)
cover Add DAC driver support for Vybrid SoC
Hello,
This patchset adds support for the DAC peripheral on Vybrid SoC. Patchset is based on top of shawn's for-next branch and has been tested on a Toradex Colibri VF50 module.
I had one query as to whether my implementation under the IIO_CHAN_INFO_SCALE correct? I am not clear on exactly on how the voltage_scale parameter is suppose to be set and used.
Feedbacks and comments most welcome.
Thanks & Regards,
Sanchayan.
Sanchayan Maity (2):
ARM: dts: vfxxx: Add DAC node for Vybrid SoC
iio: dac: vf610_dac: Add IIO DAC driver for Vybrid SoC
.../devicetree/bindings/iio/dac/vf610-dac.txt | 20 ++ arch/arm/boot/dts/vfxxx.dtsi | 18 ++ drivers/iio/dac/Kconfig | 8 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/vf610_dac.c | 302 +++++++++++++++++++++ 5 files changed, 349 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/vf610-dac.txt create mode 100644 drivers/iio/dac/vf610_dac.c
Signature · 1 line
2.7.1
PATCH 1/2 ARM: dts: vfxxx: Add DAC node for Vybrid SoC
Add a device tree node entry for DAC peripheral on Vybrid SoC.
Signed-off-by Sanchayan Maity <[email protected]>
--- arch/arm/boot/dts/vfxxx.dtsi | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)
arch/arm/boot/dts/vfxxx.dtsi +18 -0
@@ -602,6 +602,24 @@ status = "disabled"; }; + dac0: dac at 400cc000 { + compatible = "fsl,vf610-dac"; + reg = <0x400cc000 1000>; + interrupts = <55 IRQ_TYPE_LEVEL_HIGH>; + clock-names = "dac"; + clocks = <&clks VF610_CLK_DAC0>; + status = "disabled"; + }; + + dac1: dac at 400cd000 { + compatible = "fsl,vf610-dac"; + reg = <0x400cd000 1000>; + interrupts = <56 IRQ_TYPE_LEVEL_HIGH>; + clock-names = "dac"; + clocks = <&clks VF610_CLK_DAC1>; + status = "disabled"; + }; + fec0: ethernet at 400d0000 { compatible = "fsl,mvf600-fec"; reg = <0x400d0000 0x1000>;
Signature · 1 line
2.7.1
PATCH 2/2 iio: dac: vf610_dac: Add IIO DAC driver for Vybrid SoC
In reply to Sanchayan MaityAdd driver support for DAC peripheral on Vybrid SoC.
Signed-off-by Sanchayan Maity <[email protected]>
--- .../devicetree/bindings/iio/dac/vf610-dac.txt | 20 ++ drivers/iio/dac/Kconfig | 8 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/vf610_dac.c | 302 +++++++++++++++++++++ 4 files changed, 331 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/vf610-dac.txt create mode 100644 drivers/iio/dac/vf610_dac.c
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +20 -0
@@ -0,0 +1,20 @@ +Freescale vf610 Digital to Analog Converter bindings + +The devicetree bindings are for the new DAC driver written for +vf610 SoCs from Freescale. + +Required properties: +- compatible: Should contain "fsl,vf610-dac" +- reg: Offset and length of the register set for the device +- interrupts: Should contain the interrupt for the device +- clocks: The clock is needed by the DAC controller +- clock-names: Must contain "dac", matching entry in the clocks property. + +Example: +dac0: dac at 400cc000 { + compatible = "fsl,vf610-dac"; + reg = <0x400cc000 0x1000>; + interrupts = <55 IRQ_TYPE_LEVEL_HIGH>; + clock-names = "dac"; + clocks = <&clks VF610_CLK_DAC0>; +};
drivers/iio/dac/Kconfig +8 -0
@@ -196,4 +196,12 @@ config MCP4922 To compile this driver as a module, choose M here: the module will be called mcp4922. +config VF610_DAC + tristate "Vybrid vf610 DAC driver" + help + Say yes here to support Vybrid board digital-to-analog converter. + + This driver can also be built as a module. If so, the module will + be called vf610_dac. + endmenu
drivers/iio/dac/Makefile +1 -0
@@ -21,3 +21,4 @@ obj-$(CONFIG_MAX517) += max517.o obj-$(CONFIG_MAX5821) += max5821.o obj-$(CONFIG_MCP4725) += mcp4725.o obj-$(CONFIG_MCP4922) += mcp4922.o +obj-$(CONFIG_VF610_DAC) += vf610_dac.o
drivers/iio/dac/vf610_dac.c new file +302 -0
302 lines not on this page · raw message
Signature · 1 line
2.7.1
PATCH 2/2 iio: dac: vf610_dac: Add IIO DAC driver for Vybrid SoC
Quoted 1 line · Sanchayan Maity
Add driver support for DAC peripheral on Vybrid SoC.
comments below
Quoted 17 lines
Signed-off-by Sanchayan Maity <[email protected]>--- .../devicetree/bindings/iio/dac/vf610-dac.txt | 20 ++ drivers/iio/dac/Kconfig | 8 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/vf610_dac.c | 302 +++++++++++++++++++++ 4 files changed, 331 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/vf610-dac.txt create mode 100644 drivers/iio/dac/vf610_dac.cDocumentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +1
-0@@ -0,0 +1,20 @@ +Freescale vf610 Digital to Analog Converter bindings
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt line 11
Quoted 10 lines
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +10
-0+ +The devicetree bindings are for the new DAC driver written for +vf610 SoCs from Freescale. + +Required properties: +- compatible: Should contain "fsl,vf610-dac" +- reg: Offset and length of the register set for the device +- interrupts: Should contain the interrupt for the device +- clocks: The clock is needed by the DAC controller +- clock-names: Must contain "dac", matching entry in the clocks property.
the last sentence is not quite clear; is the comma needed?
Quoted 61 lines
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +9
-0+ +Example: +dac0: dac at 400cc000 { + compatible = "fsl,vf610-dac"; + reg = <0x400cc000 0x1000>; + interrupts = <55 IRQ_TYPE_LEVEL_HIGH>; + clock-names = "dac"; + clocks = <&clks VF610_CLK_DAC0>; +};drivers/iio/dac/Kconfig +8
-0@@ -196,4 +196,12 @@ config MCP4922 To compile this driver as a module, choose M here: the module will be called mcp4922. +config VF610_DAC + tristate "Vybrid vf610 DAC driver" + help + Say yes here to support Vybrid board digital-to-analog converter. + + This driver can also be built as a module. If so, the module will + be called vf610_dac. + endmenudrivers/iio/dac/Makefile +1
-0@@ -21,3 +21,4 @@ obj-$(CONFIG_MAX517) += max517.o obj-$(CONFIG_MAX5821) += max5821.o obj-$(CONFIG_MCP4725) += mcp4725.o obj-$(CONFIG_MCP4922) += mcp4922.o +obj-$(CONFIG_VF610_DAC) += vf610_dac.odrivers/iio/dac/vf610_dac.c new file +20
-0@@ -0,0 +1,302 @@ +/* +* Freescale Vybrid vf610 DAC driver +* +* Copyright 2016 Toradex AG +* +* 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. +*/ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/interrupt.h> +#include <linux/io.h>
drivers/iio/dac/vf610_dac.c line 30
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> + +#define DRIVER_NAME "vf610-dac"
VF610_ prefix
drivers/iio/dac/vf610_dac.c line 40
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +#define VF610_DACx_STATCTRL 0x20 + +#define VF610_DAC_DACEN BIT(15) +#define VF610_DAC_DACRFS BIT(14) +#define VF610_DAC_LPEN BIT(11) + +#define VF610_DAC_DAT0 (x) ((x) & 0xFFF) + +enum conversion_mode_sel {
vf610_ prefix
drivers/iio/dac/vf610_dac.c line 53
Quoted 13 lines
drivers/iio/dac/vf610_dac.c new file +13
-0+VF610_DAC_CONV_HIGH_POWER , + VF610_DAC_CONV_LOW_POWER, +}; + +struct vf610_dac { +struct clk *clk; +struct device *dev; +enum conversion_mode_sel conv_mode; +void __iomem *regs; +int irq; +}; + +static int vf610_dac_init (struct vf610_dac *info)
should be void, or check the return value
Quoted 4 lines
drivers/iio/dac/vf610_dac.c new file +4
-0+{ +int val; + + info->conv_mode =VF610_DAC_CONV_LOW_POWER ;
drivers/iio/dac/vf610_dac.c line 67
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ val =VF610_DAC_DACEN |VF610_DAC_DACRFS | +VF610_DAC_LPEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); + +return 0 ; +} + +static int vf610_set_conversion_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +unsigned int mode)
mode should be enum conversion_mode_sel?
Quoted 8 lines
drivers/iio/dac/vf610_dac.c new file +8
-0+{ +struct vf610_dac *info =iio_priv (indio_dev); +int val; + +mutex_lock (&indio_dev->mlock); + info->conv_mode = mode; + val =readl (info->regs + VF610_DACx_STATCTRL); +if (mode)
drivers/iio/dac/vf610_dac.c line 85
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ val |=VF610_DAC_LPEN ; + else + val &= ~VF610_DAC_LPEN; +writel (val, info->regs + VF610_DACx_STATCTRL); +mutex_unlock (&indio_dev->mlock); + +return 0 ; +} + +static int vf610_get_conversion_mode(struct iio_dev *indio_dev,
return of type enum conversion_mode_sel?
drivers/iio/dac/vf610_dac.c line 93
Quoted 8 lines
drivers/iio/dac/vf610_dac.c new file +8
-0+ const struct iio_chan_spec *chan) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +return info->conv_mode; +} + +static const char *const vf610_conv_modes[] = {"high-power" ,"low-power" };
this should be documented in sysfs-bus-iio-vf610; maybe high-power should be high-speed
probably this interface should have been exposed as a settable conversion rate
Quoted 7 lines
drivers/iio/dac/vf610_dac.c new file +7
-0+ +static const struct iio_enum vf610_conversion_mode = { + .items = vf610_conv_modes, + .num_items =ARRAY_SIZE (vf610_conv_modes), + .get = vf610_get_conversion_mode, + .set = vf610_set_conversion_mode, +};
drivers/iio/dac/vf610_dac.c line 110
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +static const struct iio_chan_spec_ext_info vf610_ext_info[] = { +IIO_ENUM ("conversion_mode" ,IIO_SHARED_BY_DIR , + &vf610_conversion_mode), + {}, +}; + +#define VF610_DAC_CHAN (_chan_type) { \ + .type = (_chan_type), \ + .indexed = -1, \
indexed should be 0 (or not specified) if no indexing is to be used
Quoted 23 lines
drivers/iio/dac/vf610_dac.c new file +23
-0+ .output = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .ext_info = vf610_ext_info, \ +} + +static const struct iio_chan_spec vf610_dac_iio_channels[] = { +VF610_DAC_CHAN (IIO_VOLTAGE ), +}; + +static int vf610_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2, +long mask) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +switch (mask) { +case IIO_CHAN_INFO_RAW : + *val =VF610_DAC_DAT0 (readl (info->regs)); +return IIO_VAL_INT ; +case IIO_CHAN_INFO_SCALE : +/*
drivers/iio/dac/vf610_dac.c line 143
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+* DACRFS is always 1 for valid reference and typical +* reference voltage as per Vybrid datasheet is 3.3V +* from section 9.1.2.1 of Vybrid datasheet +*/ + *val =3300 /* mV */ ; + *val2 =12 ; +return IIO_VAL_FRACTIONAL_LOG2 ; + +default : +break ;
just return -EINVAL here?
and remove it below below
Quoted 11 lines
drivers/iio/dac/vf610_dac.c new file +11
-0+ } + +return -EINVAL ; +} + +static int vf610_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2, +long mask) +{ +struct vf610_dac *info =iio_priv (indio_dev);
drivers/iio/dac/vf610_dac.c line 164
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +switch (mask) { +case IIO_CHAN_INFO_RAW : +mutex_lock (&indio_dev->mlock); +writel (VF610_DAC_DAT0 (val), info->regs); +mutex_unlock (&indio_dev->mlock); +return 0 ; + +default : +break ;
return -EINVAL here should save two lines
Quoted 32 lines
drivers/iio/dac/vf610_dac.c new file +32
-0+ } + +return -EINVAL ; +} + +static const struct iio_info vf610_dac_iio_info = { + .driver_module =THIS_MODULE , + .read_raw = &vf610_read_raw, + .write_raw = &vf610_write_raw, +}; + +static const struct of_device_id vf610_dac_match[] = { + { .compatible ="fsl,vf610-dac" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, vf610_dac_match); + +static int vf610_dac_probe (struct platform_device *pdev) +{ +struct iio_dev *indio_dev; +struct vf610_dac *info; +struct resource *mem; +int ret; + + indio_dev =devm_iio_device_alloc (&pdev->dev, +sizeof (struct vf610_dac )); +if (!indio_dev) { +dev_err (&pdev->dev,"Failed allocating iio device\n" ); +return -ENOMEM ; + } + + info =iio_priv (indio_dev);
drivers/iio/dac/vf610_dac.c line 206
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ 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->clk =devm_clk_get (&pdev->dev,"dac" ); +if (IS_ERR (info->clk)) { + dev_err(&pdev->dev, "failed getting clock, err = %ld\n",
"Failed getting ..."
Quoted 18 lines
drivers/iio/dac/vf610_dac.c new file +18
-0+ PTR_ERR(info->clk)); +return PTR_ERR (info->clk); + } + +platform_set_drvdata (pdev, indio_dev); + + 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_dac_iio_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = vf610_dac_iio_channels; + indio_dev->num_channels =ARRAY_SIZE (vf610_dac_iio_channels); + + ret =clk_prepare_enable (info->clk); +if (ret) { +dev_err (&pdev->dev, +"Could not prepare or enable the clock\n" );
drivers/iio/dac/vf610_dac.c line 234
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+return ret; + } + + ret =iio_device_register (indio_dev); +if (ret) { +dev_err (&pdev->dev,"Couldn't register the device\n" ); + goto error_iio_device_register; + } + +vf610_dac_init (info);
there is a race here, move _dac_init() before _register() return value is not checked (or use void)
Quoted 69 lines
drivers/iio/dac/vf610_dac.c new file +68
-0+ +return 0 ; + +error_iio_device_register: +clk_disable_unprepare (info->clk); + +return ret; +} + +static int vf610_dac_remove (struct platform_device *pdev) +{ +struct iio_dev *indio_dev =platform_get_drvdata (pdev); +struct vf610_dac *info =iio_priv (indio_dev); + +iio_device_unregister (indio_dev); +clk_disable_unprepare (info->clk); + +return 0 ; +} + +#ifdef CONFIG_PM_SLEEP +static int vf610_dac_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct vf610_dac *info =iio_priv (indio_dev); +int val; + + val =readl (info->regs + VF610_DACx_STATCTRL); + val &= ~VF610_DAC_DACEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); + +clk_disable_unprepare (info->clk); + +return 0 ; +} + +static int vf610_dac_resume (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct vf610_dac *info =iio_priv (indio_dev); +int ret; + + ret =clk_prepare_enable (info->clk); +if (ret) +return ret; + +vf610_dac_init (info); + +return 0 ; +} +#endif + +static SIMPLE_DEV_PM_OPS(vf610_dac_pm_ops, vf610_dac_suspend, vf610_dac_resume); + +static struct platform_driver vf610_dac_driver = { + .probe = vf610_dac_probe, + .remove = vf610_dac_remove, + .driver = { + .name =DRIVER_NAME , + .of_match_table = vf610_dac_match, + .pm = &vf610_dac_pm_ops, + }, +}; +module_platform_driver (vf610_dac_driver); + +MODULE_AUTHOR ("Sanchayan Maity <[email protected]" ); +MODULE_DESCRIPTION ("Freescale VF610 DAC driver" ); +MODULE_LICENSE ("GPL v2" );
Signature · 2 lines
Peter Meerwald-Stadler +43-664-2444418 (mobile)
PATCH 2/2 iio: dac: vf610_dac: Add IIO DAC driver for Vybrid SoC
Hello Peter,
Quoted 168 lines · Peter Meerwald-Stadler
Quoted 1 line · Sanchayan Maity
Add driver support for DAC peripheral on Vybrid SoC.
comments below
Quoted 17 lines
Signed-off-by Sanchayan Maity <[email protected]>--- .../devicetree/bindings/iio/dac/vf610-dac.txt | 20 ++ drivers/iio/dac/Kconfig | 8 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/vf610_dac.c | 302 +++++++++++++++++++++ 4 files changed, 331 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/vf610-dac.txt create mode 100644 drivers/iio/dac/vf610_dac.cDocumentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +1
-0@@ -0,0 +1,20 @@ +Freescale vf610 Digital to Analog Converter bindingsDocumentation/devicetree/bindings/iio/dac/vf610-dac.txt line 11
Quoted 10 lines
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +10
-0+ +The devicetree bindings are for the new DAC driver written for +vf610 SoCs from Freescale. + +Required properties: +- compatible: Should contain "fsl,vf610-dac" +- reg: Offset and length of the register set for the device +- interrupts: Should contain the interrupt for the device +- clocks: The clock is needed by the DAC controller +- clock-names: Must contain "dac", matching entry in the clocks property.the last sentence is not quite clear; is the comma needed?
Quoted 61 lines
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +9
-0+ +Example: +dac0: dac at 400cc000 { + compatible = "fsl,vf610-dac"; + reg = <0x400cc000 0x1000>; + interrupts = <55 IRQ_TYPE_LEVEL_HIGH>; + clock-names = "dac"; + clocks = <&clks VF610_CLK_DAC0>; +};drivers/iio/dac/Kconfig +8
-0@@ -196,4 +196,12 @@ config MCP4922 To compile this driver as a module, choose M here: the module will be called mcp4922. +config VF610_DAC + tristate "Vybrid vf610 DAC driver" + help + Say yes here to support Vybrid board digital-to-analog converter. + + This driver can also be built as a module. If so, the module will + be called vf610_dac. + endmenudrivers/iio/dac/Makefile +1
-0@@ -21,3 +21,4 @@ obj-$(CONFIG_MAX517) += max517.o obj-$(CONFIG_MAX5821) += max5821.o obj-$(CONFIG_MCP4725) += mcp4725.o obj-$(CONFIG_MCP4922) += mcp4922.o +obj-$(CONFIG_VF610_DAC) += vf610_dac.odrivers/iio/dac/vf610_dac.c new file +20
-0@@ -0,0 +1,302 @@ +/* +* Freescale Vybrid vf610 DAC driver +* +* Copyright 2016 Toradex AG +* +* 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. +*/ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/interrupt.h> +#include <linux/io.h> drivers/iio/dac/vf610_dac.c line 30
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> + +#define DRIVER_NAME "vf610-dac"
VF610_prefixdrivers/iio/dac/vf610_dac.c line 40
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +#define VF610_DACx_STATCTRL 0x20 + +#define VF610_DAC_DACEN BIT(15) +#define VF610_DAC_DACRFS BIT(14) +#define VF610_DAC_LPEN BIT(11) + +#define VF610_DAC_DAT0 (x) ((x) & 0xFFF) + +enum conversion_mode_sel {
vf610_prefixdrivers/iio/dac/vf610_dac.c line 53
Quoted 13 lines
drivers/iio/dac/vf610_dac.c new file +13
-0+VF610_DAC_CONV_HIGH_POWER , + VF610_DAC_CONV_LOW_POWER, +}; + +struct vf610_dac { +struct clk *clk; +struct device *dev; +enum conversion_mode_sel conv_mode; +void __iomem *regs; +int irq; +}; + +static int vf610_dac_init (struct vf610_dac *info)should be void, or check the return value
Quoted 4 lines
drivers/iio/dac/vf610_dac.c new file +4
-0+{ +int val; + + info->conv_mode =VF610_DAC_CONV_LOW_POWER ;drivers/iio/dac/vf610_dac.c line 67
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ val =VF610_DAC_DACEN |VF610_DAC_DACRFS | +VF610_DAC_LPEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); + +return 0 ; +} + +static int vf610_set_conversion_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +unsigned int mode)mode should be enum
conversion_mode_sel?Quoted 12 lines
drivers/iio/dac/vf610_dac.c new file +12
-0+{ +struct vf610_dac *info =iio_priv (indio_dev); +int val; + +mutex_lock (&indio_dev->mlock); + info->conv_mode = mode; + val =readl (info->regs + VF610_DACx_STATCTRL); +if (mode) + val |=VF610_DAC_LPEN ; +else + val &= ~VF610_DAC_LPEN ; +writel (val, info->regs + VF610_DACx_STATCTRL);
Quoted 8 lines
drivers/iio/dac/vf610_dac.c line 85
Quoted 6 lines
drivers/iio/dac/vf610_dac.c new file +6
-0+mutex_unlock (&indio_dev->mlock); + +return 0 ; +} + +static int vf610_get_conversion_mode(struct iio_dev *indio_dev,return of type enum
conversion_mode_sel?
Regarding the enum. As per the function prototype in iio.h
struct iio_enum {
const char * const *items;
unsigned int num_items;
int (*set)(struct iio_dev *, const struct iio_chan_spec *, unsigned int);
int (*get)(struct iio_dev *, const struct iio_chan_spec *);
};the "mode" argument is unsigned int and return type of getter is int as well.
Quoted 13 lines
drivers/iio/dac/vf610_dac.c line 93
Quoted 9 lines
drivers/iio/dac/vf610_dac.c new file +8
-0+ const struct iio_chan_spec *chan) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +return info->conv_mode; +} + +static const char *const vf610_conv_modes[] = {"high-power" ,"low-power" };this should be documented in sysfs-bus-iio-vf610; maybe high-power should be high-speed
Quoted 2 lines
probably this interface should have been exposed as a settable conversion rate
Vybrid DAC block does not provide for setting a conversion rate. There is only one register for configration and only one LPEN bit concerning power mode. As per the operating behavior, this controls characteristics of supply current, settling time and slew rate.
While I agree this should be documented in ABI sysfs documentation, I am not clear as to how that should be. Currently that document specifies the modes observed for the vf610 ADC driver. As per the datasheet, "high-power" and "low-power" would be the most clear description in case of DAC but "normal", "high-speed" and "low-power" are applicable for ADC. This just stuck me just now while making the change.
How should I best include this? Peter, Jonathan, Stefan suggestions?
I have changed the code to include all other feedback. Thank you for feedback.
Best Regards,
Sanchayan.
Quoted 233 lines
Quoted 8 lines
drivers/iio/dac/vf610_dac.c new file +7
-0+ +static const struct iio_enum vf610_conversion_mode = { + .items = vf610_conv_modes, + .num_items =ARRAY_SIZE (vf610_conv_modes), + .get = vf610_get_conversion_mode, + .set = vf610_set_conversion_mode, +};drivers/iio/dac/vf610_dac.c line 110
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +static const struct iio_chan_spec_ext_info vf610_ext_info[] = { +IIO_ENUM ("conversion_mode" ,IIO_SHARED_BY_DIR , + &vf610_conversion_mode), + {}, +}; + +#define VF610_DAC_CHAN (_chan_type) { \ + .type = (_chan_type), \ + .indexed = -1, \indexed should be 0 (or not specified) if no indexing is to be used
Quoted 23 lines
drivers/iio/dac/vf610_dac.c new file +23
-0+ .output = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .ext_info = vf610_ext_info, \ +} + +static const struct iio_chan_spec vf610_dac_iio_channels[] = { +VF610_DAC_CHAN (IIO_VOLTAGE ), +}; + +static int vf610_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2, +long mask) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +switch (mask) { +case IIO_CHAN_INFO_RAW : + *val =VF610_DAC_DAT0 (readl (info->regs)); +return IIO_VAL_INT ; +case IIO_CHAN_INFO_SCALE : +/* drivers/iio/dac/vf610_dac.c line 143
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+* DACRFS is always 1 for valid reference and typical +* reference voltage as per Vybrid datasheet is 3.3V +* from section 9.1.2.1 of Vybrid datasheet +*/ + *val =3300 /* mV */ ; + *val2 =12 ; +return IIO_VAL_FRACTIONAL_LOG2 ; + +default : +break ;just return
-EINVALhere?
and remove it below belowQuoted 11 lines
drivers/iio/dac/vf610_dac.c new file +11
-0+ } + +return -EINVAL ; +} + +static int vf610_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2, +long mask) +{ +struct vf610_dac *info =iio_priv (indio_dev);drivers/iio/dac/vf610_dac.c line 164
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +switch (mask) { +case IIO_CHAN_INFO_RAW : +mutex_lock (&indio_dev->mlock); +writel (VF610_DAC_DAT0 (val), info->regs); +mutex_unlock (&indio_dev->mlock); +return 0 ; + +default : +break ;return
-EINVALhere should save two linesQuoted 32 lines
drivers/iio/dac/vf610_dac.c new file +32
-0+ } + +return -EINVAL ; +} + +static const struct iio_info vf610_dac_iio_info = { + .driver_module =THIS_MODULE , + .read_raw = &vf610_read_raw, + .write_raw = &vf610_write_raw, +}; + +static const struct of_device_id vf610_dac_match[] = { + { .compatible ="fsl,vf610-dac" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, vf610_dac_match); + +static int vf610_dac_probe (struct platform_device *pdev) +{ +struct iio_dev *indio_dev; +struct vf610_dac *info; +struct resource *mem; +int ret; + + indio_dev =devm_iio_device_alloc (&pdev->dev, +sizeof (struct vf610_dac )); +if (!indio_dev) { +dev_err (&pdev->dev,"Failed allocating iio device\n" ); +return -ENOMEM ; + } + + info =iio_priv (indio_dev);drivers/iio/dac/vf610_dac.c line 206
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ 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->clk =devm_clk_get (&pdev->dev,"dac" ); +if (IS_ERR (info->clk)) { + dev_err(&pdev->dev, "failed getting clock, err = %ld\n","Failed getting ..."
Quoted 18 lines
drivers/iio/dac/vf610_dac.c new file +18
-0+ PTR_ERR(info->clk)); +return PTR_ERR (info->clk); + } + +platform_set_drvdata (pdev, indio_dev); + + 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_dac_iio_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = vf610_dac_iio_channels; + indio_dev->num_channels =ARRAY_SIZE (vf610_dac_iio_channels); + + ret =clk_prepare_enable (info->clk); +if (ret) { +dev_err (&pdev->dev, +"Could not prepare or enable the clock\n" );drivers/iio/dac/vf610_dac.c line 234
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+return ret; + } + + ret =iio_device_register (indio_dev); +if (ret) { +dev_err (&pdev->dev,"Couldn't register the device\n" ); + goto error_iio_device_register; + } + +vf610_dac_init (info);there is a race here, move
_dac_init()before_register()return value is not checked (or use void)Quoted 69 lines
drivers/iio/dac/vf610_dac.c new file +68
-0+ +return 0 ; + +error_iio_device_register: +clk_disable_unprepare (info->clk); + +return ret; +} + +static int vf610_dac_remove (struct platform_device *pdev) +{ +struct iio_dev *indio_dev =platform_get_drvdata (pdev); +struct vf610_dac *info =iio_priv (indio_dev); + +iio_device_unregister (indio_dev); +clk_disable_unprepare (info->clk); + +return 0 ; +} + +#ifdef CONFIG_PM_SLEEP +static int vf610_dac_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct vf610_dac *info =iio_priv (indio_dev); +int val; + + val =readl (info->regs + VF610_DACx_STATCTRL); + val &= ~VF610_DAC_DACEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); + +clk_disable_unprepare (info->clk); + +return 0 ; +} + +static int vf610_dac_resume (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct vf610_dac *info =iio_priv (indio_dev); +int ret; + + ret =clk_prepare_enable (info->clk); +if (ret) +return ret; + +vf610_dac_init (info); + +return 0 ; +} +#endif + +static SIMPLE_DEV_PM_OPS(vf610_dac_pm_ops, vf610_dac_suspend, vf610_dac_resume); + +static struct platform_driver vf610_dac_driver = { + .probe = vf610_dac_probe, + .remove = vf610_dac_remove, + .driver = { + .name =DRIVER_NAME , + .of_match_table = vf610_dac_match, + .pm = &vf610_dac_pm_ops, + }, +}; +module_platform_driver (vf610_dac_driver); + +MODULE_AUTHOR ("Sanchayan Maity <[email protected]" ); +MODULE_DESCRIPTION ("Freescale VF610 DAC driver" ); +MODULE_LICENSE ("GPL v2" );Signature · 2 lines
Peter Meerwald-Stadler +43-664-2444418 (mobile)
Jonathan Cameron
maintainerPATCH 2/2 iio: dac: vf610_dac: Add IIO DAC driver for Vybrid SoC
Quoted 205 lines · maitysanchayan at gmail.com
Hello Peter,
Quoted 168 lines · Peter Meerwald-Stadler
Quoted 1 line · Sanchayan Maity
Add driver support for DAC peripheral on Vybrid SoC.
comments below
Quoted 17 lines
Signed-off-by Sanchayan Maity <[email protected]>--- .../devicetree/bindings/iio/dac/vf610-dac.txt | 20 ++ drivers/iio/dac/Kconfig | 8 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/vf610_dac.c | 302 +++++++++++++++++++++ 4 files changed, 331 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/vf610-dac.txt create mode 100644 drivers/iio/dac/vf610_dac.cDocumentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +1
-0@@ -0,0 +1,20 @@ +Freescale vf610 Digital to Analog Converter bindingsDocumentation/devicetree/bindings/iio/dac/vf610-dac.txt line 11
Quoted 10 lines
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +10
-0+ +The devicetree bindings are for the new DAC driver written for +vf610 SoCs from Freescale. + +Required properties: +- compatible: Should contain "fsl,vf610-dac" +- reg: Offset and length of the register set for the device +- interrupts: Should contain the interrupt for the device +- clocks: The clock is needed by the DAC controller +- clock-names: Must contain "dac", matching entry in the clocks property.the last sentence is not quite clear; is the comma needed?
Quoted 61 lines
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +9
-0+ +Example: +dac0: dac at 400cc000 { + compatible = "fsl,vf610-dac"; + reg = <0x400cc000 0x1000>; + interrupts = <55 IRQ_TYPE_LEVEL_HIGH>; + clock-names = "dac"; + clocks = <&clks VF610_CLK_DAC0>; +};drivers/iio/dac/Kconfig +8
-0@@ -196,4 +196,12 @@ config MCP4922 To compile this driver as a module, choose M here: the module will be called mcp4922. +config VF610_DAC + tristate "Vybrid vf610 DAC driver" + help + Say yes here to support Vybrid board digital-to-analog converter. + + This driver can also be built as a module. If so, the module will + be called vf610_dac. + endmenudrivers/iio/dac/Makefile +1
-0@@ -21,3 +21,4 @@ obj-$(CONFIG_MAX517) += max517.o obj-$(CONFIG_MAX5821) += max5821.o obj-$(CONFIG_MCP4725) += mcp4725.o obj-$(CONFIG_MCP4922) += mcp4922.o +obj-$(CONFIG_VF610_DAC) += vf610_dac.odrivers/iio/dac/vf610_dac.c new file +20
-0@@ -0,0 +1,302 @@ +/* +* Freescale Vybrid vf610 DAC driver +* +* Copyright 2016 Toradex AG +* +* 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. +*/ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/interrupt.h> +#include <linux/io.h> drivers/iio/dac/vf610_dac.c line 30
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> + +#define DRIVER_NAME "vf610-dac"
VF610_prefixdrivers/iio/dac/vf610_dac.c line 40
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +#define VF610_DACx_STATCTRL 0x20 + +#define VF610_DAC_DACEN BIT(15) +#define VF610_DAC_DACRFS BIT(14) +#define VF610_DAC_LPEN BIT(11) + +#define VF610_DAC_DAT0 (x) ((x) & 0xFFF) + +enum conversion_mode_sel {
vf610_prefixdrivers/iio/dac/vf610_dac.c line 53
Quoted 13 lines
drivers/iio/dac/vf610_dac.c new file +13
-0+VF610_DAC_CONV_HIGH_POWER , + VF610_DAC_CONV_LOW_POWER, +}; + +struct vf610_dac { +struct clk *clk; +struct device *dev; +enum conversion_mode_sel conv_mode; +void __iomem *regs; +int irq; +}; + +static int vf610_dac_init (struct vf610_dac *info)should be void, or check the return value
Quoted 4 lines
drivers/iio/dac/vf610_dac.c new file +4
-0+{ +int val; + + info->conv_mode =VF610_DAC_CONV_LOW_POWER ;drivers/iio/dac/vf610_dac.c line 67
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ val =VF610_DAC_DACEN |VF610_DAC_DACRFS | +VF610_DAC_LPEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); + +return 0 ; +} + +static int vf610_set_conversion_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +unsigned int mode)mode should be enum
conversion_mode_sel?Quoted 12 lines
drivers/iio/dac/vf610_dac.c new file +12
-0+{ +struct vf610_dac *info =iio_priv (indio_dev); +int val; + +mutex_lock (&indio_dev->mlock); + info->conv_mode = mode; + val =readl (info->regs + VF610_DACx_STATCTRL); +if (mode) + val |=VF610_DAC_LPEN ; +else + val &= ~VF610_DAC_LPEN ; +writel (val, info->regs + VF610_DACx_STATCTRL);Quoted 8 lines
drivers/iio/dac/vf610_dac.c line 85
Quoted 6 lines
drivers/iio/dac/vf610_dac.c new file +6
-0+mutex_unlock (&indio_dev->mlock); + +return 0 ; +} + +static int vf610_get_conversion_mode(struct iio_dev *indio_dev,return of type enum
conversion_mode_sel?Regarding the enum. As per the function prototype in
iio.hstruct iio_enum { const char * const *items; unsigned int num_items; int (*set)(struct iio_dev *, const struct iio_chan_spec *, unsigned int); int (*get)(struct iio_dev *, const struct iio_chan_spec *); };the "mode" argument is unsigned int and return type of getter is int as well.
Quoted 13 lines
drivers/iio/dac/vf610_dac.c line 93
Quoted 9 lines
drivers/iio/dac/vf610_dac.c new file +8
-0+ const struct iio_chan_spec *chan) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +return info->conv_mode; +} + +static const char *const vf610_conv_modes[] = {"high-power" ,"low-power" };this should be documented in sysfs-bus-iio-vf610; maybe high-power should be high-speed
Quoted 7 lines
Quoted 2 lines
probably this interface should have been exposed as a settable conversion rate
Vybrid DAC block does not provide for setting a conversion rate. There is only one register for configration and only one LPEN bit concerning power mode. As per the operating behavior, this controls characteristics of supply current, settling time and slew rate.
It strikes me that perhaps for DACs it would not be overkill to actually expose the settling time and slew rate via sysfs?
I'd be happy enough seeing these as new info_mask elements as they are fairly common. Are these the best ways to describe DAC performance characteristics? Sounds right to me but my electronics background is limited!
Quoted 6 lines
While I agree this should be documented in ABI sysfs documentation, I am not clear as to how that should be. Currently that document specifies the modes observed for the vf610 ADC driver. As per the datasheet, "high-power" and "low-power" would be the most clear description in case of DAC but "normal", "high-speed" and "low-power" are applicable for ADC. This just stuck me just now while making the change.
Certainly not obvious. Could use the slewrate or settling time as explicit controls on this (support both perhaps, with the usual IIO interface rule that you should never assume changing one sysfs attribute won't also change another!)
This would then correspond to some degree with the analogy of setting effectively power modes for ADCs via sampling frequency as the dominant attribute that effects their power usage.
Jonathan
Quoted 241 lines
How should I best include this? Peter, Jonathan, Stefan suggestions?
I have changed the code to include all other feedback. Thank you for feedback.
Best Regards,
Sanchayan.Quoted 233 lines
Quoted 8 lines
drivers/iio/dac/vf610_dac.c new file +7
-0+ +static const struct iio_enum vf610_conversion_mode = { + .items = vf610_conv_modes, + .num_items =ARRAY_SIZE (vf610_conv_modes), + .get = vf610_get_conversion_mode, + .set = vf610_set_conversion_mode, +};drivers/iio/dac/vf610_dac.c line 110
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +static const struct iio_chan_spec_ext_info vf610_ext_info[] = { +IIO_ENUM ("conversion_mode" ,IIO_SHARED_BY_DIR , + &vf610_conversion_mode), + {}, +}; + +#define VF610_DAC_CHAN (_chan_type) { \ + .type = (_chan_type), \ + .indexed = -1, \indexed should be 0 (or not specified) if no indexing is to be used
Quoted 23 lines
drivers/iio/dac/vf610_dac.c new file +23
-0+ .output = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .ext_info = vf610_ext_info, \ +} + +static const struct iio_chan_spec vf610_dac_iio_channels[] = { +VF610_DAC_CHAN (IIO_VOLTAGE ), +}; + +static int vf610_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2, +long mask) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +switch (mask) { +case IIO_CHAN_INFO_RAW : + *val =VF610_DAC_DAT0 (readl (info->regs)); +return IIO_VAL_INT ; +case IIO_CHAN_INFO_SCALE : +/* drivers/iio/dac/vf610_dac.c line 143
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+* DACRFS is always 1 for valid reference and typical +* reference voltage as per Vybrid datasheet is 3.3V +* from section 9.1.2.1 of Vybrid datasheet +*/ + *val =3300 /* mV */ ; + *val2 =12 ; +return IIO_VAL_FRACTIONAL_LOG2 ; + +default : +break ;just return
-EINVALhere?
and remove it below belowQuoted 11 lines
drivers/iio/dac/vf610_dac.c new file +11
-0+ } + +return -EINVAL ; +} + +static int vf610_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2, +long mask) +{ +struct vf610_dac *info =iio_priv (indio_dev);drivers/iio/dac/vf610_dac.c line 164
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +switch (mask) { +case IIO_CHAN_INFO_RAW : +mutex_lock (&indio_dev->mlock); +writel (VF610_DAC_DAT0 (val), info->regs); +mutex_unlock (&indio_dev->mlock); +return 0 ; + +default : +break ;return
-EINVALhere should save two linesQuoted 32 lines
drivers/iio/dac/vf610_dac.c new file +32
-0+ } + +return -EINVAL ; +} + +static const struct iio_info vf610_dac_iio_info = { + .driver_module =THIS_MODULE , + .read_raw = &vf610_read_raw, + .write_raw = &vf610_write_raw, +}; + +static const struct of_device_id vf610_dac_match[] = { + { .compatible ="fsl,vf610-dac" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, vf610_dac_match); + +static int vf610_dac_probe (struct platform_device *pdev) +{ +struct iio_dev *indio_dev; +struct vf610_dac *info; +struct resource *mem; +int ret; + + indio_dev =devm_iio_device_alloc (&pdev->dev, +sizeof (struct vf610_dac )); +if (!indio_dev) { +dev_err (&pdev->dev,"Failed allocating iio device\n" ); +return -ENOMEM ; + } + + info =iio_priv (indio_dev);drivers/iio/dac/vf610_dac.c line 206
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ 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->clk =devm_clk_get (&pdev->dev,"dac" ); +if (IS_ERR (info->clk)) { + dev_err(&pdev->dev, "failed getting clock, err = %ld\n","Failed getting ..."
Quoted 18 lines
drivers/iio/dac/vf610_dac.c new file +18
-0+ PTR_ERR(info->clk)); +return PTR_ERR (info->clk); + } + +platform_set_drvdata (pdev, indio_dev); + + 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_dac_iio_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = vf610_dac_iio_channels; + indio_dev->num_channels =ARRAY_SIZE (vf610_dac_iio_channels); + + ret =clk_prepare_enable (info->clk); +if (ret) { +dev_err (&pdev->dev, +"Could not prepare or enable the clock\n" );drivers/iio/dac/vf610_dac.c line 234
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+return ret; + } + + ret =iio_device_register (indio_dev); +if (ret) { +dev_err (&pdev->dev,"Couldn't register the device\n" ); + goto error_iio_device_register; + } + +vf610_dac_init (info);there is a race here, move
_dac_init()before_register()return value is not checked (or use void)Quoted 69 lines
drivers/iio/dac/vf610_dac.c new file +68
-0+ +return 0 ; + +error_iio_device_register: +clk_disable_unprepare (info->clk); + +return ret; +} + +static int vf610_dac_remove (struct platform_device *pdev) +{ +struct iio_dev *indio_dev =platform_get_drvdata (pdev); +struct vf610_dac *info =iio_priv (indio_dev); + +iio_device_unregister (indio_dev); +clk_disable_unprepare (info->clk); + +return 0 ; +} + +#ifdef CONFIG_PM_SLEEP +static int vf610_dac_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct vf610_dac *info =iio_priv (indio_dev); +int val; + + val =readl (info->regs + VF610_DACx_STATCTRL); + val &= ~VF610_DAC_DACEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); + +clk_disable_unprepare (info->clk); + +return 0 ; +} + +static int vf610_dac_resume (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct vf610_dac *info =iio_priv (indio_dev); +int ret; + + ret =clk_prepare_enable (info->clk); +if (ret) +return ret; + +vf610_dac_init (info); + +return 0 ; +} +#endif + +static SIMPLE_DEV_PM_OPS(vf610_dac_pm_ops, vf610_dac_suspend, vf610_dac_resume); + +static struct platform_driver vf610_dac_driver = { + .probe = vf610_dac_probe, + .remove = vf610_dac_remove, + .driver = { + .name =DRIVER_NAME , + .of_match_table = vf610_dac_match, + .pm = &vf610_dac_pm_ops, + }, +}; +module_platform_driver (vf610_dac_driver); + +MODULE_AUTHOR ("Sanchayan Maity <[email protected]" ); +MODULE_DESCRIPTION ("Freescale VF610 DAC driver" ); +MODULE_LICENSE ("GPL v2" );Signature · 2 lines
Peter Meerwald-Stadler +43-664-2444418 (mobile)
PATCH 2/2 iio: dac: vf610_dac: Add IIO DAC driver for Vybrid SoC
Hello Jonathan,
Quoted 226 lines · Jonathan Cameron
Quoted 205 lines · maitysanchayan at gmail.com
Hello Peter,
Quoted 168 lines · Peter Meerwald-Stadler
Quoted 1 line · Sanchayan Maity
Add driver support for DAC peripheral on Vybrid SoC.
comments below
Quoted 17 lines
Signed-off-by Sanchayan Maity <[email protected]>--- .../devicetree/bindings/iio/dac/vf610-dac.txt | 20 ++ drivers/iio/dac/Kconfig | 8 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/vf610_dac.c | 302 +++++++++++++++++++++ 4 files changed, 331 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/vf610-dac.txt create mode 100644 drivers/iio/dac/vf610_dac.cDocumentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +1
-0@@ -0,0 +1,20 @@ +Freescale vf610 Digital to Analog Converter bindingsDocumentation/devicetree/bindings/iio/dac/vf610-dac.txt line 11
Quoted 10 lines
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +10
-0+ +The devicetree bindings are for the new DAC driver written for +vf610 SoCs from Freescale. + +Required properties: +- compatible: Should contain "fsl,vf610-dac" +- reg: Offset and length of the register set for the device +- interrupts: Should contain the interrupt for the device +- clocks: The clock is needed by the DAC controller +- clock-names: Must contain "dac", matching entry in the clocks property.the last sentence is not quite clear; is the comma needed?
Quoted 61 lines
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +9
-0+ +Example: +dac0: dac at 400cc000 { + compatible = "fsl,vf610-dac"; + reg = <0x400cc000 0x1000>; + interrupts = <55 IRQ_TYPE_LEVEL_HIGH>; + clock-names = "dac"; + clocks = <&clks VF610_CLK_DAC0>; +};drivers/iio/dac/Kconfig +8
-0@@ -196,4 +196,12 @@ config MCP4922 To compile this driver as a module, choose M here: the module will be called mcp4922. +config VF610_DAC + tristate "Vybrid vf610 DAC driver" + help + Say yes here to support Vybrid board digital-to-analog converter. + + This driver can also be built as a module. If so, the module will + be called vf610_dac. + endmenudrivers/iio/dac/Makefile +1
-0@@ -21,3 +21,4 @@ obj-$(CONFIG_MAX517) += max517.o obj-$(CONFIG_MAX5821) += max5821.o obj-$(CONFIG_MCP4725) += mcp4725.o obj-$(CONFIG_MCP4922) += mcp4922.o +obj-$(CONFIG_VF610_DAC) += vf610_dac.odrivers/iio/dac/vf610_dac.c new file +20
-0@@ -0,0 +1,302 @@ +/* +* Freescale Vybrid vf610 DAC driver +* +* Copyright 2016 Toradex AG +* +* 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. +*/ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/interrupt.h> +#include <linux/io.h> drivers/iio/dac/vf610_dac.c line 30
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> + +#define DRIVER_NAME "vf610-dac"
VF610_prefixdrivers/iio/dac/vf610_dac.c line 40
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +#define VF610_DACx_STATCTRL 0x20 + +#define VF610_DAC_DACEN BIT(15) +#define VF610_DAC_DACRFS BIT(14) +#define VF610_DAC_LPEN BIT(11) + +#define VF610_DAC_DAT0 (x) ((x) & 0xFFF) + +enum conversion_mode_sel {
vf610_prefixdrivers/iio/dac/vf610_dac.c line 53
Quoted 13 lines
drivers/iio/dac/vf610_dac.c new file +13
-0+VF610_DAC_CONV_HIGH_POWER , + VF610_DAC_CONV_LOW_POWER, +}; + +struct vf610_dac { +struct clk *clk; +struct device *dev; +enum conversion_mode_sel conv_mode; +void __iomem *regs; +int irq; +}; + +static int vf610_dac_init (struct vf610_dac *info)should be void, or check the return value
Quoted 4 lines
drivers/iio/dac/vf610_dac.c new file +4
-0+{ +int val; + + info->conv_mode =VF610_DAC_CONV_LOW_POWER ;drivers/iio/dac/vf610_dac.c line 67
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ val =VF610_DAC_DACEN |VF610_DAC_DACRFS | +VF610_DAC_LPEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); + +return 0 ; +} + +static int vf610_set_conversion_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +unsigned int mode)mode should be enum
conversion_mode_sel?Quoted 12 lines
drivers/iio/dac/vf610_dac.c new file +12
-0+{ +struct vf610_dac *info =iio_priv (indio_dev); +int val; + +mutex_lock (&indio_dev->mlock); + info->conv_mode = mode; + val =readl (info->regs + VF610_DACx_STATCTRL); +if (mode) + val |=VF610_DAC_LPEN ; +else + val &= ~VF610_DAC_LPEN ; +writel (val, info->regs + VF610_DACx_STATCTRL);Quoted 8 lines
drivers/iio/dac/vf610_dac.c line 85
Quoted 6 lines
drivers/iio/dac/vf610_dac.c new file +6
-0+mutex_unlock (&indio_dev->mlock); + +return 0 ; +} + +static int vf610_get_conversion_mode(struct iio_dev *indio_dev,return of type enum
conversion_mode_sel?Regarding the enum. As per the function prototype in
iio.hstruct iio_enum { const char * const *items; unsigned int num_items; int (*set)(struct iio_dev *, const struct iio_chan_spec *, unsigned int); int (*get)(struct iio_dev *, const struct iio_chan_spec *); };the "mode" argument is unsigned int and return type of getter is int as well.
Quoted 13 lines
drivers/iio/dac/vf610_dac.c line 93
Quoted 9 lines
drivers/iio/dac/vf610_dac.c new file +8
-0+ const struct iio_chan_spec *chan) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +return info->conv_mode; +} + +static const char *const vf610_conv_modes[] = {"high-power" ,"low-power" };this should be documented in sysfs-bus-iio-vf610; maybe high-power should be high-speed
Quoted 7 lines
Quoted 2 lines
probably this interface should have been exposed as a settable conversion rate
Vybrid DAC block does not provide for setting a conversion rate. There is only one register for configration and only one LPEN bit concerning power mode. As per the operating behavior, this controls characteristics of supply current, settling time and slew rate.
It strikes me that perhaps for DACs it would not be overkill to actually expose the settling time and slew rate via sysfs?
I'd be happy enough seeing these as newinfo_maskelements as they are fairly common. Are these the best ways to describe DAC performance characteristics? Sounds right to me but my electronics background is limited!Quoted 6 lines
While I agree this should be documented in ABI sysfs documentation, I am not clear as to how that should be. Currently that document specifies the modes observed for the vf610 ADC driver. As per the datasheet, "high-power" and "low-power" would be the most clear description in case of DAC but "normal", "high-speed" and "low-power" are applicable for ADC. This just stuck me just now while making the change.
Quoted 8 lines
Certainly not obvious. Could use the slewrate or settling time as explicit controls on this (support both perhaps, with the usual IIO interface rule that you should never assume changing one sysfs attribute won't also change another!)
This would then correspond to some degree with the analogy of setting effectively power modes for ADCs via sampling frequency as the dominant attribute that effects their power usage.
There is no explicit setting or register to set for controlling slew rate or settling time. There is only one LPEN bit which specifies low or high power mode. This bit would then effect the settling time or slew rate which would be the typical values in the datasheet.
Regards,
Sanchayan.
Quoted 244 lines
Jonathan
Quoted 241 lines
How should I best include this? Peter, Jonathan, Stefan suggestions?
I have changed the code to include all other feedback. Thank you for feedback.
Best Regards,
Sanchayan.Quoted 233 lines
Quoted 8 lines
drivers/iio/dac/vf610_dac.c new file +7
-0+ +static const struct iio_enum vf610_conversion_mode = { + .items = vf610_conv_modes, + .num_items =ARRAY_SIZE (vf610_conv_modes), + .get = vf610_get_conversion_mode, + .set = vf610_set_conversion_mode, +};drivers/iio/dac/vf610_dac.c line 110
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +static const struct iio_chan_spec_ext_info vf610_ext_info[] = { +IIO_ENUM ("conversion_mode" ,IIO_SHARED_BY_DIR , + &vf610_conversion_mode), + {}, +}; + +#define VF610_DAC_CHAN (_chan_type) { \ + .type = (_chan_type), \ + .indexed = -1, \indexed should be 0 (or not specified) if no indexing is to be used
Quoted 23 lines
drivers/iio/dac/vf610_dac.c new file +23
-0+ .output = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .ext_info = vf610_ext_info, \ +} + +static const struct iio_chan_spec vf610_dac_iio_channels[] = { +VF610_DAC_CHAN (IIO_VOLTAGE ), +}; + +static int vf610_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2, +long mask) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +switch (mask) { +case IIO_CHAN_INFO_RAW : + *val =VF610_DAC_DAT0 (readl (info->regs)); +return IIO_VAL_INT ; +case IIO_CHAN_INFO_SCALE : +/* drivers/iio/dac/vf610_dac.c line 143
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+* DACRFS is always 1 for valid reference and typical +* reference voltage as per Vybrid datasheet is 3.3V +* from section 9.1.2.1 of Vybrid datasheet +*/ + *val =3300 /* mV */ ; + *val2 =12 ; +return IIO_VAL_FRACTIONAL_LOG2 ; + +default : +break ;just return
-EINVALhere?
and remove it below belowQuoted 11 lines
drivers/iio/dac/vf610_dac.c new file +11
-0+ } + +return -EINVAL ; +} + +static int vf610_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2, +long mask) +{ +struct vf610_dac *info =iio_priv (indio_dev);drivers/iio/dac/vf610_dac.c line 164
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +switch (mask) { +case IIO_CHAN_INFO_RAW : +mutex_lock (&indio_dev->mlock); +writel (VF610_DAC_DAT0 (val), info->regs); +mutex_unlock (&indio_dev->mlock); +return 0 ; + +default : +break ;return
-EINVALhere should save two linesQuoted 32 lines
drivers/iio/dac/vf610_dac.c new file +32
-0+ } + +return -EINVAL ; +} + +static const struct iio_info vf610_dac_iio_info = { + .driver_module =THIS_MODULE , + .read_raw = &vf610_read_raw, + .write_raw = &vf610_write_raw, +}; + +static const struct of_device_id vf610_dac_match[] = { + { .compatible ="fsl,vf610-dac" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, vf610_dac_match); + +static int vf610_dac_probe (struct platform_device *pdev) +{ +struct iio_dev *indio_dev; +struct vf610_dac *info; +struct resource *mem; +int ret; + + indio_dev =devm_iio_device_alloc (&pdev->dev, +sizeof (struct vf610_dac )); +if (!indio_dev) { +dev_err (&pdev->dev,"Failed allocating iio device\n" ); +return -ENOMEM ; + } + + info =iio_priv (indio_dev);drivers/iio/dac/vf610_dac.c line 206
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ 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->clk =devm_clk_get (&pdev->dev,"dac" ); +if (IS_ERR (info->clk)) { + dev_err(&pdev->dev, "failed getting clock, err = %ld\n","Failed getting ..."
Quoted 18 lines
drivers/iio/dac/vf610_dac.c new file +18
-0+ PTR_ERR(info->clk)); +return PTR_ERR (info->clk); + } + +platform_set_drvdata (pdev, indio_dev); + + 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_dac_iio_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = vf610_dac_iio_channels; + indio_dev->num_channels =ARRAY_SIZE (vf610_dac_iio_channels); + + ret =clk_prepare_enable (info->clk); +if (ret) { +dev_err (&pdev->dev, +"Could not prepare or enable the clock\n" );drivers/iio/dac/vf610_dac.c line 234
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+return ret; + } + + ret =iio_device_register (indio_dev); +if (ret) { +dev_err (&pdev->dev,"Couldn't register the device\n" ); + goto error_iio_device_register; + } + +vf610_dac_init (info);there is a race here, move
_dac_init()before_register()return value is not checked (or use void)Quoted 69 lines
drivers/iio/dac/vf610_dac.c new file +68
-0+ +return 0 ; + +error_iio_device_register: +clk_disable_unprepare (info->clk); + +return ret; +} + +static int vf610_dac_remove (struct platform_device *pdev) +{ +struct iio_dev *indio_dev =platform_get_drvdata (pdev); +struct vf610_dac *info =iio_priv (indio_dev); + +iio_device_unregister (indio_dev); +clk_disable_unprepare (info->clk); + +return 0 ; +} + +#ifdef CONFIG_PM_SLEEP +static int vf610_dac_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct vf610_dac *info =iio_priv (indio_dev); +int val; + + val =readl (info->regs + VF610_DACx_STATCTRL); + val &= ~VF610_DAC_DACEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); + +clk_disable_unprepare (info->clk); + +return 0 ; +} + +static int vf610_dac_resume (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct vf610_dac *info =iio_priv (indio_dev); +int ret; + + ret =clk_prepare_enable (info->clk); +if (ret) +return ret; + +vf610_dac_init (info); + +return 0 ; +} +#endif + +static SIMPLE_DEV_PM_OPS(vf610_dac_pm_ops, vf610_dac_suspend, vf610_dac_resume); + +static struct platform_driver vf610_dac_driver = { + .probe = vf610_dac_probe, + .remove = vf610_dac_remove, + .driver = { + .name =DRIVER_NAME , + .of_match_table = vf610_dac_match, + .pm = &vf610_dac_pm_ops, + }, +}; +module_platform_driver (vf610_dac_driver); + +MODULE_AUTHOR ("Sanchayan Maity <[email protected]" ); +MODULE_DESCRIPTION ("Freescale VF610 DAC driver" ); +MODULE_LICENSE ("GPL v2" );Signature · 2 lines
Peter Meerwald-Stadler +43-664-2444418 (mobile)
Jonathan Cameron
maintainerPATCH 2/2 iio: dac: vf610_dac: Add IIO DAC driver for Vybrid SoC
Quoted 269 lines · maitysanchayan at gmail.com
Hello Jonathan,
Quoted 8 lines · Jonathan Cameron
Quoted 7 lines · maitysanchayan at gmail.com
Hello Peter,
Quoted 4 lines · Peter Meerwald-Stadler
Quoted 1 line · Sanchayan Maity
Add driver support for DAC peripheral on Vybrid SoC.
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
comments below
Quoted 6 lines
Signed-off-by Sanchayan Maity <[email protected]>--- .../devicetree/bindings/iio/dac/vf610-dac.txt | 20 ++ drivers/iio/dac/Kconfig | 8 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/vf610_dac.c | 302+++++++++++++++++++++
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
4 files changed, 331 insertions(+) create mode 100644
Documentation/devicetree/bindings/iio/dac/vf610-dac.txtQuoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
create mode 100644 drivers/iio/dac/vf610_dac.cdiff --git
a/Documentation/devicetree/bindings/iio/dac/vf610-dac.txtb/Documentation/devicetree/bindings/iio/dac/vf610-dac.txtQuoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
new file mode 100644
index 0000000..97d7a40Documentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +3
-0@@ -0,0 +1,20 @@ +Freescale vf610 Digital to Analog Converter bindings + +The devicetree bindings are for the new DAC driver written forQuoted 8 lines
Quoted 8 lines
Quoted 8 lines
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
Quoted 8 lines
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +8
-0+vf610 SoCs from Freescale. + +Required properties: +- compatible: Should contain "fsl,vf610-dac" +- reg: Offset and length of the register set for the device +- interrupts: Should contain the interrupt for the device +- clocks: The clock is needed by the DAC controller +- clock-names: Must contain "dac", matching entry in the clocksproperty.
Quoted 20 lines
Quoted 20 lines
Quoted 20 lines
the last sentence is not quite clear; is the comma needed?
Quoted 17 lines
Documentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +9
-0+ +Example: +dac0: dac at 400cc000 { + compatible = "fsl,vf610-dac"; + reg = <0x400cc000 0x1000>; + interrupts = <55 IRQ_TYPE_LEVEL_HIGH>; + clock-names = "dac"; + clocks = <&clks VF610_CLK_DAC0>; +};drivers/iio/dac/Kconfig +0
-0@@ -196,4 +196,12 @@ config MCP4922 To compile this driver as a module, choose M here: the module will be called mcp4922.Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/iio/dac/Kconfig config MCP4922
Quoted 4 lines
drivers/iio/dac/Kconfig +4
-0+config VF610_DAC + tristate "Vybrid vf610 DAC driver" + help + Say yes here to support Vybrid board digital-to-analogconverter.
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/iio/dac/Kconfig config MCP4922 · line 204
Quoted 2 lines
drivers/iio/dac/Kconfig +2
-0+ + This driver can also be built as a module. If so, the modulewill
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/iio/dac/Kconfig +2
-0+ be called vf610_dac. + endmenudrivers/iio/dac/Makefile +0
-0Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Review
Quoted 8 lines
drivers/iio/dac/Makefile +1
-0@@ -21,3 +21,4 @@ obj-$(CONFIG_MAX517) += max517.o obj-$(CONFIG_MAX5821) += max5821.o obj-$(CONFIG_MCP4725) += mcp4725.o obj-$(CONFIG_MCP4922) += mcp4922.o +obj-$(CONFIG_VF610_DAC) += vf610_dac.o(quoted diff) +0
-0
b/drivers/iio/dac/vf610_dac.cQuoted 11 lines
Quoted 11 lines
Quoted 11 lines
drivers/iio/dac/vf610_dac.c line 6
Quoted 11 lines
new file mode 100644
index 0000000..775b349drivers/iio/dac/vf610_dac.c new file +6
-0@@ -0,0 +1,302 @@ +/* +* Freescale Vybrid vf610 DAC driver +* +* Copyright 2016 Toradex AG +* +* This program is free software; you can redistribute it and/or modify
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/iio/dac/vf610_dac.c line 7
Quoted 1 line
drivers/iio/dac/vf610_dac.c new file +1
-0+* it under the terms of the GNU General Public License as published by
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/iio/dac/vf610_dac.c line 8
Quoted 1 line
drivers/iio/dac/vf610_dac.c new file +1
-0+* the Free Software Foundation; either version 2 of the License, or
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/iio/dac/vf610_dac.c
Quoted 3 lines
drivers/iio/dac/vf610_dac.c new file +3
-0+* (at your option) any later version. +* +* This program is distributed in the hope that it will be useful,
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/iio/dac/vf610_dac.c line 12
Quoted 1 line
drivers/iio/dac/vf610_dac.c new file +1
-0+* but WITHOUT ANY WARRANTY; without even the implied warranty of
Quoted 89 lines
Quoted 89 lines
Quoted 87 lines
Quoted 8 lines
drivers/iio/dac/vf610_dac.c new file +8
-0+* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +* GNU General Public License for more details. +*/ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/interrupt.h> +#include <linux/io.h> drivers/iio/dac/vf610_dac.c line 30
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> + +#define DRIVER_NAME "vf610-dac"
VF610_prefixdrivers/iio/dac/vf610_dac.c line 40
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +#define VF610_DACx_STATCTRL 0x20 + +#define VF610_DAC_DACEN BIT(15) +#define VF610_DAC_DACRFS BIT(14) +#define VF610_DAC_LPEN BIT(11) + +#define VF610_DAC_DAT0 (x) ((x) & 0xFFF) + +enum conversion_mode_sel {
vf610_prefixdrivers/iio/dac/vf610_dac.c line 53
Quoted 13 lines
drivers/iio/dac/vf610_dac.c new file +13
-0+VF610_DAC_CONV_HIGH_POWER , + VF610_DAC_CONV_LOW_POWER, +}; + +struct vf610_dac { +struct clk *clk; +struct device *dev; +enum conversion_mode_sel conv_mode; +void __iomem *regs; +int irq; +}; + +static int vf610_dac_init (struct vf610_dac *info)should be void, or check the return value
Quoted 4 lines
drivers/iio/dac/vf610_dac.c new file +4
-0+{ +int val; + + info->conv_mode =VF610_DAC_CONV_LOW_POWER ;drivers/iio/dac/vf610_dac.c line 67
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ val =VF610_DAC_DACEN |VF610_DAC_DACRFS | +VF610_DAC_LPEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); + +return 0 ; +} + +static int vf610_set_conversion_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +unsigned int mode)mode should be enum
conversion_mode_sel?Quoted 8 lines
drivers/iio/dac/vf610_dac.c new file +8
-0+{ +struct vf610_dac *info =iio_priv (indio_dev); +int val; + +mutex_lock (&indio_dev->mlock); + info->conv_mode = mode; + val =readl (info->regs + VF610_DACx_STATCTRL); +if (mode)drivers/iio/dac/vf610_dac.c line 85
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ val |=VF610_DAC_LPEN ; + else + val &= ~VF610_DAC_LPEN; +writel (val, info->regs + VF610_DACx_STATCTRL); +mutex_unlock (&indio_dev->mlock); + +return 0 ; +} + +static int vf610_get_conversion_mode(struct iio_dev *indio_dev,return of type enum
conversion_mode_sel?Quoted 6 lines
Quoted 6 lines
Regarding the enum. As per the function prototype in
iio.hstruct iio_enum { const char * const *items; unsigned int num_items; int (*set)(struct iio_dev *, const struct iio_chan_spec *,unsigned int);
Quoted 4 lines
Quoted 4 lines
int (*get)(struct iio_dev *, const struct iio_chan_spec *); };the "mode" argument is unsigned int and return type of getter is
int as well.
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/iio/dac/vf610_dac.c
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +8
-0+ const struct iio_chan_spec *chan) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +return info->conv_mode; +} + +static const char *const vf610_conv_modes[] = {"high-power" ,"low-power"};
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
this should be documented in sysfs-bus-iio-vf610; maybe high-power
should
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
be high-speed
probably this interface should have been exposed as a settable conversion rate
Vybrid DAC block does not provide for setting a conversion rate.
There is only one
Quoted 1 line
Quoted 1 line
register for configration and only one LPEN bit concerning power
mode. As per the
Quoted 1 line
Quoted 1 line
operating behavior, this controls characteristics of supply
current, settling time
Quoted 2 lines
Quoted 1 line
and slew rate.
It strikes me that perhaps for DACs it would not be overkill to
actually
Quoted 3 lines
expose the settling time and slew rate via sysfs?
I'd be happy enough seeing these as newinfo_maskelements as they are fairly common. Are these the best ways to describe DACperformance
Quoted 5 lines
characteristics? Sounds right to me but my electronics background is limited!
Quoted 2 lines
While I agree this should be documented in ABI sysfs documentation,
I am not clear
Quoted 1 line
Quoted 1 line
as to how that should be. Currently that document specifies the
modes observed for
Quoted 1 line
Quoted 1 line
the vf610 ADC driver. As per the datasheet, "high-power" and
"low-power" would be
Quoted 1 line
Quoted 1 line
the most clear description in case of DAC but "normal",
"high-speed" and "low-power"
Quoted 1 line
Quoted 1 line
are applicable for ADC. This just stuck me just now while making
the change.
Quoted 1 line
Certainly not obvious. Could use the slewrate or settling time as
explicit
Quoted 1 line
controls on this (support both perhaps, with the usual IIO interface
rule
Quoted 1 line
that you should never assume changing one sysfs attribute won't also
change
Quoted 4 lines
another!)
This would then correspond to some degree with the analogy of setting effectively power modes for ADCs via sampling frequency as the
dominant
Quoted 1 line
attribute that effects their power usage.
Quoted 7 lines
There is no explicit setting or register to set for controlling slew rate or
settling time. There is only one LPEN bit which specifies low or high power
mode. This bit would then effect the settling time or slew rate which would
be the typical values in the datasheet.
That's fine. Just have available attributes with the two resulting values.
J
Quoted 256 lines
Regards,
Sanchayan.Quoted 4 lines
Jonathan
Quoted 2 lines
How should I best include this? Peter, Jonathan, Stefan
suggestions?
Quoted 2 lines
Quoted 2 lines
I have changed the code to include all other feedback. Thank you
for feedback.
Quoted 16 lines
Quoted 16 lines
Best Regards,
Sanchayan.Quoted 12 lines
Quoted 12 lines
drivers/iio/dac/vf610_dac.c new file +11
-0+ +static const struct iio_enum vf610_conversion_mode = { + .items = vf610_conv_modes, + .num_items =ARRAY_SIZE (vf610_conv_modes), + .get = vf610_get_conversion_mode, + .set = vf610_set_conversion_mode, +}; + +static const struct iio_chan_spec_ext_info vf610_ext_info[] = { +IIO_ENUM ("conversion_mode" ,IIO_SHARED_BY_DIR , + &vf610_conversion_mode),Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/iio/dac/vf610_dac.c line 110
Quoted 6 lines
drivers/iio/dac/vf610_dac.c new file +6
-0+ {}, +}; + +#define VF610_DAC_CHAN (_chan_type) { \ + .type = (_chan_type), \ + .indexed = -1, \indexed should be 0 (or not specified) if no indexing is to be
used
Quoted 184 lines
Quoted 184 lines
Quoted 184 lines
Quoted 24 lines
drivers/iio/dac/vf610_dac.c new file +23
-0+ .output = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .ext_info = vf610_ext_info, \ +} + +static const struct iio_chan_spec vf610_dac_iio_channels[] = { +VF610_DAC_CHAN (IIO_VOLTAGE ), +}; + +static int vf610_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2, +long mask) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +switch (mask) { +case IIO_CHAN_INFO_RAW : + *val =VF610_DAC_DAT0 (readl (info->regs)); +return IIO_VAL_INT ; +case IIO_CHAN_INFO_SCALE : +/* drivers/iio/dac/vf610_dac.c line 143
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+* DACRFS is always 1 for valid reference and typical +* reference voltage as per Vybrid datasheet is 3.3V +* from section 9.1.2.1 of Vybrid datasheet +*/ + *val =3300 /* mV */ ; + *val2 =12 ; +return IIO_VAL_FRACTIONAL_LOG2 ; + +default : +break ;just return
-EINVALhere?
and remove it below belowQuoted 11 lines
drivers/iio/dac/vf610_dac.c new file +11
-0+ } + +return -EINVAL ; +} + +static int vf610_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2, +long mask) +{ +struct vf610_dac *info =iio_priv (indio_dev);drivers/iio/dac/vf610_dac.c line 164
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +switch (mask) { +case IIO_CHAN_INFO_RAW : +mutex_lock (&indio_dev->mlock); +writel (VF610_DAC_DAT0 (val), info->regs); +mutex_unlock (&indio_dev->mlock); +return 0 ; + +default : +break ;return
-EINVALhere should save two linesQuoted 32 lines
drivers/iio/dac/vf610_dac.c new file +32
-0+ } + +return -EINVAL ; +} + +static const struct iio_info vf610_dac_iio_info = { + .driver_module =THIS_MODULE , + .read_raw = &vf610_read_raw, + .write_raw = &vf610_write_raw, +}; + +static const struct of_device_id vf610_dac_match[] = { + { .compatible ="fsl,vf610-dac" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, vf610_dac_match); + +static int vf610_dac_probe (struct platform_device *pdev) +{ +struct iio_dev *indio_dev; +struct vf610_dac *info; +struct resource *mem; +int ret; + + indio_dev =devm_iio_device_alloc (&pdev->dev, +sizeof (struct vf610_dac )); +if (!indio_dev) { +dev_err (&pdev->dev,"Failed allocating iio device\n" ); +return -ENOMEM ; + } + + info =iio_priv (indio_dev);drivers/iio/dac/vf610_dac.c line 206
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ 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->clk =devm_clk_get (&pdev->dev,"dac" ); +if (IS_ERR (info->clk)) { + dev_err(&pdev->dev, "failed getting clock, err = %ld\n","Failed getting ..."
Quoted 18 lines
drivers/iio/dac/vf610_dac.c new file +18
-0+ PTR_ERR(info->clk)); +return PTR_ERR (info->clk); + } + +platform_set_drvdata (pdev, indio_dev); + + 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_dac_iio_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = vf610_dac_iio_channels; + indio_dev->num_channels =ARRAY_SIZE (vf610_dac_iio_channels); + + ret =clk_prepare_enable (info->clk); +if (ret) { +dev_err (&pdev->dev, +"Could not prepare or enable the clock\n" );drivers/iio/dac/vf610_dac.c line 234
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+return ret; + } + + ret =iio_device_register (indio_dev); +if (ret) { +dev_err (&pdev->dev,"Couldn't register the device\n" ); + goto error_iio_device_register; + } + +vf610_dac_init (info);there is a race here, move
_dac_init()before_register()return value is not checked (or use void)Quoted 45 lines
drivers/iio/dac/vf610_dac.c new file +45
-0+ +return 0 ; + +error_iio_device_register: +clk_disable_unprepare (info->clk); + +return ret; +} + +static int vf610_dac_remove (struct platform_device *pdev) +{ +struct iio_dev *indio_dev =platform_get_drvdata (pdev); +struct vf610_dac *info =iio_priv (indio_dev); + +iio_device_unregister (indio_dev); +clk_disable_unprepare (info->clk); + +return 0 ; +} + +#ifdef CONFIG_PM_SLEEP +static int vf610_dac_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct vf610_dac *info =iio_priv (indio_dev); +int val; + + val =readl (info->regs + VF610_DACx_STATCTRL); + val &= ~VF610_DAC_DACEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); + +clk_disable_unprepare (info->clk); + +return 0 ; +} + +static int vf610_dac_resume (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct vf610_dac *info =iio_priv (indio_dev); +int ret; + + ret =clk_prepare_enable (info->clk); +if (ret) +return ret;Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/iio/dac/vf610_dac.c
Quoted 8 lines
drivers/iio/dac/vf610_dac.c new file +8
-0+ +vf610_dac_init (info); + +return 0 ; +} +#endif + +static SIMPLE_DEV_PM_OPS(vf610_dac_pm_ops, vf610_dac_suspend,
vf610_dac_resume);Quoted 19 lines
Quoted 19 lines
Quoted 19 lines
Quoted 16 lines
drivers/iio/dac/vf610_dac.c new file +15
-0+ +static struct platform_driver vf610_dac_driver = { + .probe = vf610_dac_probe, + .remove = vf610_dac_remove, + .driver = { + .name =DRIVER_NAME , + .of_match_table = vf610_dac_match, + .pm = &vf610_dac_pm_ops, + }, +}; +module_platform_driver (vf610_dac_driver); + +MODULE_AUTHOR ("Sanchayan Maity <[email protected]" ); +MODULE_DESCRIPTION ("Freescale VF610 DAC driver" ); +MODULE_LICENSE ("GPL v2" );Quoted 3 lines
Quoted 2 lines
Quoted 2 lines
Peter Meerwald-Stadler
+43-664-2444418 (mobile)--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Signature · 1 line
Sent from my Android device with K-9 Mail. Please excuse my brevity.
Jonathan Cameron
maintainerPATCH 2/2 iio: dac: vf610_dac: Add IIO DAC driver for Vybrid SoC
In reply to Sanchayan MaityQuoted 3 lines · Sanchayan Maity
Add driver support for DAC peripheral on Vybrid SoC.
Signed-off-by Sanchayan Maity <[email protected]>
Few minor comments inline.
Jonathan
Quoted 87 lines
--- .../devicetree/bindings/iio/dac/vf610-dac.txt | 20 ++ drivers/iio/dac/Kconfig | 8 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/vf610_dac.c | 302 +++++++++++++++++++++ 4 files changed, 331 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/vf610-dac.txt create mode 100644 drivers/iio/dac/vf610_dac.cDocumentation/devicetree/bindings/iio/dac/vf610-dac.txt new file +20
-0@@ -0,0 +1,20 @@ +Freescale vf610 Digital to Analog Converter bindings + +The devicetree bindings are for the new DAC driver written for +vf610 SoCs from Freescale. + +Required properties: +- compatible: Should contain "fsl,vf610-dac" +- reg: Offset and length of the register set for the device +- interrupts: Should contain the interrupt for the device +- clocks: The clock is needed by the DAC controller +- clock-names: Must contain "dac", matching entry in the clocks property. + +Example: +dac0: dac at 400cc000 { + compatible = "fsl,vf610-dac"; + reg = <0x400cc000 0x1000>; + interrupts = <55 IRQ_TYPE_LEVEL_HIGH>; + clock-names = "dac"; + clocks = <&clks VF610_CLK_DAC0>; +};drivers/iio/dac/Kconfig +8
-0@@ -196,4 +196,12 @@ config MCP4922 To compile this driver as a module, choose M here: the module will be called mcp4922. +config VF610_DAC + tristate "Vybrid vf610 DAC driver" + help + Say yes here to support Vybrid board digital-to-analog converter. + + This driver can also be built as a module. If so, the module will + be called vf610_dac. + endmenudrivers/iio/dac/Makefile +1
-0@@ -21,3 +21,4 @@ obj-$(CONFIG_MAX517) += max517.o obj-$(CONFIG_MAX5821) += max5821.o obj-$(CONFIG_MCP4725) += mcp4725.o obj-$(CONFIG_MCP4922) += mcp4922.o +obj-$(CONFIG_VF610_DAC) += vf610_dac.odrivers/iio/dac/vf610_dac.c new file +20
-0@@ -0,0 +1,302 @@ +/* +* Freescale Vybrid vf610 DAC driver +* +* Copyright 2016 Toradex AG +* +* 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. +*/ + +#include <linux/clk.h> +#include <linux/err.h> +#include <linux/interrupt.h> +#include <linux/io.h>
drivers/iio/dac/vf610_dac.c line 30
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/platform_device.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> + +#include <linux/iio/iio.h> +#include <linux/iio/sysfs.h> + +#define DRIVER_NAME "vf610-dac"
One of my pet hates is having defines like this that add nothing. Just put it in where it is used (as there is only one place).
Quoted 63 lines
drivers/iio/dac/vf610_dac.c new file +63
-0+ +#define VF610_DACx_STATCTRL 0x20 + +#define VF610_DAC_DACEN BIT(15) +#define VF610_DAC_DACRFS BIT(14) +#define VF610_DAC_LPEN BIT(11) + +#define VF610_DAC_DAT0 (x) ((x) & 0xFFF) + +enum conversion_mode_sel { +VF610_DAC_CONV_HIGH_POWER , +VF610_DAC_CONV_LOW_POWER , +}; + +struct vf610_dac { +struct clk *clk; +struct device *dev; +enum conversion_mode_sel conv_mode; +void __iomem *regs; +int irq; +}; + +static int vf610_dac_init (struct vf610_dac *info) +{ +int val; + + info->conv_mode =VF610_DAC_CONV_LOW_POWER ; + val =VF610_DAC_DACEN |VF610_DAC_DACRFS | +VF610_DAC_LPEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); + +return 0 ; +} + +static int vf610_set_conversion_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan, +unsigned int mode) +{ +struct vf610_dac *info =iio_priv (indio_dev); +int val; + +mutex_lock (&indio_dev->mlock); + info->conv_mode = mode; + val =readl (info->regs + VF610_DACx_STATCTRL); +if (mode) + val |=VF610_DAC_LPEN ; +else + val &= ~VF610_DAC_LPEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); +mutex_unlock (&indio_dev->mlock); + +return 0 ; +} + +static int vf610_get_conversion_mode (struct iio_dev *indio_dev, +const struct iio_chan_spec *chan) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +return info->conv_mode; +} + +static const char *const vf610_conv_modes[] = {"high-power" ,"low-power" };
drivers/iio/dac/vf610_dac.c line 103
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ +static const struct iio_enum vf610_conversion_mode = { + .items = vf610_conv_modes, + .num_items =ARRAY_SIZE (vf610_conv_modes), + .get = vf610_get_conversion_mode, + .set = vf610_set_conversion_mode, +}; + +static const struct iio_chan_spec_ext_info vf610_ext_info[] = { + IIO_ENUM("conversion_mode", IIO_SHARED_BY_DIR,
This will amke it called out_conversion_mode. Does it actually make sense, rather than IIO_SHARED_BY_ALL which would just be conversion_mode? Doesn't really matter either way.
Quoted 26 lines
drivers/iio/dac/vf610_dac.c new file +26
-0+ &vf610_conversion_mode), + {}, +}; + +#define VF610_DAC_CHAN (_chan_type) { \ + .type = (_chan_type), \ + .indexed = -1, \ + .output = 1, \ + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .ext_info = vf610_ext_info, \ +} + +static const struct iio_chan_spec vf610_dac_iio_channels[] = { +VF610_DAC_CHAN (IIO_VOLTAGE ), +}; + +static int vf610_read_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int *val,int *val2, +long mask) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +switch (mask) { +case IIO_CHAN_INFO_RAW :
drivers/iio/dac/vf610_dac.c line 139
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+ *val =VF610_DAC_DAT0 (readl (info->regs)); +return IIO_VAL_INT ; +case IIO_CHAN_INFO_SCALE : +/* +* DACRFS is always 1 for valid reference and typical +* reference voltage as per Vybrid datasheet is 3.3V +* from section 9.1.2.1 of Vybrid datasheet +*/ + *val =3300 /* mV */ ; + *val2 =12 ;
Looks at least superficially right to me - I'm being lazy and not reading the datasheet on this occasion!
Quoted 85 lines
drivers/iio/dac/vf610_dac.c new file +85
-0+return IIO_VAL_FRACTIONAL_LOG2 ; + +default : +break ; + } + +return -EINVAL ; +} + +static int vf610_write_raw (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +int val,int val2, +long mask) +{ +struct vf610_dac *info =iio_priv (indio_dev); + +switch (mask) { +case IIO_CHAN_INFO_RAW : +mutex_lock (&indio_dev->mlock); +writel (VF610_DAC_DAT0 (val), info->regs); +mutex_unlock (&indio_dev->mlock); +return 0 ; + +default : +break ; + } + +return -EINVAL ; +} + +static const struct iio_info vf610_dac_iio_info = { + .driver_module =THIS_MODULE , + .read_raw = &vf610_read_raw, + .write_raw = &vf610_write_raw, +}; + +static const struct of_device_id vf610_dac_match[] = { + { .compatible ="fsl,vf610-dac" , }, + {/* sentinel */ } +}; +MODULE_DEVICE_TABLE (of, vf610_dac_match); + +static int vf610_dac_probe (struct platform_device *pdev) +{ +struct iio_dev *indio_dev; +struct vf610_dac *info; +struct resource *mem; +int ret; + + indio_dev =devm_iio_device_alloc (&pdev->dev, +sizeof (struct vf610_dac )); +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->clk =devm_clk_get (&pdev->dev,"dac" ); +if (IS_ERR (info->clk)) { +dev_err (&pdev->dev,"failed getting clock, err = %ld\n" , +PTR_ERR (info->clk)); +return PTR_ERR (info->clk); + } + +platform_set_drvdata (pdev, indio_dev); + + 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_dac_iio_info; + indio_dev->modes =INDIO_DIRECT_MODE ; + indio_dev->channels = vf610_dac_iio_channels; + indio_dev->num_channels =ARRAY_SIZE (vf610_dac_iio_channels); + + ret =clk_prepare_enable (info->clk); +if (ret) { +dev_err (&pdev->dev, +"Could not prepare or enable the clock\n" );
drivers/iio/dac/vf610_dac.c line 234
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+return ret; + } + + ret =iio_device_register (indio_dev); +if (ret) { +dev_err (&pdev->dev,"Couldn't register the device\n" ); + goto error_iio_device_register; + } + +vf610_dac_init (info);
Peter already pointed this out, but iio_device_register is responsible for exposing an IIO device to the rest of the system (userspace and in kernel client driverS) so it should not be called until everything is ready. You might just get a read before we get to the init call here.
Quoted 17 lines
drivers/iio/dac/vf610_dac.c new file +17
-0+ +return 0 ; + +error_iio_device_register: +clk_disable_unprepare (info->clk); + +return ret; +} + +static int vf610_dac_remove (struct platform_device *pdev) +{ +struct iio_dev *indio_dev =platform_get_drvdata (pdev); +struct vf610_dac *info =iio_priv (indio_dev); + +iio_device_unregister (indio_dev); +clk_disable_unprepare (info->clk); +
drivers/iio/dac/vf610_dac.c line 261
Quoted 10 lines
drivers/iio/dac/vf610_dac.c new file +10
-0+return 0 ; +} + +#ifdef CONFIG_PM_SLEEP +static int vf610_dac_suspend (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct vf610_dac *info =iio_priv (indio_dev); +int val; +
Given this is presumably 'kind of' the oposite of the init below, for clear balance I'd break this out to a vf610_dac_exit function.
Also opens up the question of whether it makes sense to make this same write in the remove function above.
Quoted 42 lines
drivers/iio/dac/vf610_dac.c new file +41
-0+ val =readl (info->regs + VF610_DACx_STATCTRL); + val &= ~VF610_DAC_DACEN ; +writel (val, info->regs + VF610_DACx_STATCTRL); + +clk_disable_unprepare (info->clk); + +return 0 ; +} + +static int vf610_dac_resume (struct device *dev) +{ +struct iio_dev *indio_dev =dev_get_drvdata (dev); +struct vf610_dac *info =iio_priv (indio_dev); +int ret; + + ret =clk_prepare_enable (info->clk); +if (ret) +return ret; + +vf610_dac_init (info); + +return 0 ; +} +#endif + +static SIMPLE_DEV_PM_OPS(vf610_dac_pm_ops, vf610_dac_suspend, vf610_dac_resume); + +static struct platform_driver vf610_dac_driver = { + .probe = vf610_dac_probe, + .remove = vf610_dac_remove, + .driver = { + .name =DRIVER_NAME , + .of_match_table = vf610_dac_match, + .pm = &vf610_dac_pm_ops, + }, +}; +module_platform_driver (vf610_dac_driver); + +MODULE_AUTHOR ("Sanchayan Maity <[email protected]" ); +MODULE_DESCRIPTION ("Freescale VF610 DAC driver" ); +MODULE_LICENSE ("GPL v2" );
Rob Herring
maintainerPATCH 2/2 iio: dac: vf610_dac: Add IIO DAC driver for Vybrid SoC
In reply to Sanchayan MaityQuoted 5 lines · Sanchayan Maity
Add driver support for DAC peripheral on Vybrid SoC.
Signed-off-by Sanchayan Maity <[email protected]>--- .../devicetree/bindings/iio/dac/vf610-dac.txt | 20 ++
Acked-by Rob Herring <[email protected]>
Quoted 6 lines
drivers/iio/dac/Kconfig | 8 + drivers/iio/dac/Makefile | 1 + drivers/iio/dac/vf610_dac.c | 302 +++++++++++++++++++++ 4 files changed, 331 insertions(+) create mode 100644 Documentation/devicetree/bindings/iio/dac/vf610-dac.txt create mode 100644 drivers/iio/dac/vf610_dac.c