[v1,2/2] iio: dac: vf610_dac: Add IIO DAC driver for Vybrid SoC

Commit Message

Sanchayan Feb. 9, 2016, 5:26 a.m. UTC

Add driver support for DAC peripheral on Vybrid SoC.

Signed-off-by: Sanchayan Maity <[e-mail]>
---
 .../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

Comments

Peter Meerwald-Stadler Feb. 9, 2016, 6:43 a.m. UTC | #1

On Tue, 9 Feb 2016, Sanchayan Maity wrote:

> Add driver support for DAC peripheral on Vybrid SoC.

comments below
 
> Signed-off-by: Sanchayan Maity <[e-mail]>
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
> new file mode 100644
> index 0000000..97d7a40
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
> @@ -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.

the last sentence is not quite clear; is the comma needed?

> +
> +Example:
> +dac0: dac@400cc000 {
> +	compatible = "fsl,vf610-dac";
> +	reg = <0x400cc000 0x1000>;
> +	interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
> +	clock-names = "dac";
> +	clocks = <&clks VF610_CLK_DAC0>;
> +};
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index e701e28..8eb0502 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -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
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 63ae056..93feb27 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -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
> diff --git a/drivers/iio/dac/vf610_dac.c b/drivers/iio/dac/vf610_dac.c
> new file mode 100644
> index 0000000..775b349
> --- /dev/null
> +++ b/drivers/iio/dac/vf610_dac.c
> @@ -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>
> +#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

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

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

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

mode should be enum conversion_mode_sel?

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

return of type enum conversion_mode_sel?

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

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

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

> +	}
> +
> +	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 here should save two lines

> +	}
> +
> +	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",

"Failed getting ..."

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

> +
> +	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 <[e-mail]");
> +MODULE_DESCRIPTION("Freescale VF610 DAC driver");
> +MODULE_LICENSE("GPL v2");
>

Jonathan Cameron Feb. 9, 2016, 10:44 p.m. UTC | #2

On 09/02/16 05:26, Sanchayan Maity wrote:
> Add driver support for DAC peripheral on Vybrid SoC.
> 
> Signed-off-by: Sanchayan Maity <[e-mail]>
Few minor comments inline.

Jonathan
> ---
>  .../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
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
> new file mode 100644
> index 0000000..97d7a40
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
> @@ -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@400cc000 {
> +	compatible = "fsl,vf610-dac";
> +	reg = <0x400cc000 0x1000>;
> +	interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
> +	clock-names = "dac";
> +	clocks = <&clks VF610_CLK_DAC0>;
> +};
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index e701e28..8eb0502 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -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
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 63ae056..93feb27 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -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
> diff --git a/drivers/iio/dac/vf610_dac.c b/drivers/iio/dac/vf610_dac.c
> new file mode 100644
> index 0000000..775b349
> --- /dev/null
> +++ b/drivers/iio/dac/vf610_dac.c
> @@ -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>
> +#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).

> +
> +#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"};
> +
> +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.
> +		&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:
> +		*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!
> +		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");
> +		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.
> +
> +	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;
> +
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.

> +	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 <[e-mail]");
> +MODULE_DESCRIPTION("Freescale VF610 DAC driver");
> +MODULE_LICENSE("GPL v2");
>

Sanchayan Feb. 12, 2016, 6:34 a.m. UTC | #3

Hello Peter,

On 16-02-09 07:43:14, Peter Meerwald-Stadler wrote:
> On Tue, 9 Feb 2016, Sanchayan Maity wrote:
> 
> > Add driver support for DAC peripheral on Vybrid SoC.
> 
> comments below
>  
> > Signed-off-by: Sanchayan Maity <[e-mail]>
> > ---
> >  .../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
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
> > new file mode 100644
> > index 0000000..97d7a40
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
> > @@ -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.
> 
> the last sentence is not quite clear; is the comma needed?
> 
> > +
> > +Example:
> > +dac0: dac@400cc000 {
> > +	compatible = "fsl,vf610-dac";
> > +	reg = <0x400cc000 0x1000>;
> > +	interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
> > +	clock-names = "dac";
> > +	clocks = <&clks VF610_CLK_DAC0>;
> > +};
> > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> > index e701e28..8eb0502 100644
> > --- a/drivers/iio/dac/Kconfig
> > +++ b/drivers/iio/dac/Kconfig
> > @@ -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
> > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> > index 63ae056..93feb27 100644
> > --- a/drivers/iio/dac/Makefile
> > +++ b/drivers/iio/dac/Makefile
> > @@ -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
> > diff --git a/drivers/iio/dac/vf610_dac.c b/drivers/iio/dac/vf610_dac.c
> > new file mode 100644
> > index 0000000..775b349
> > --- /dev/null
> > +++ b/drivers/iio/dac/vf610_dac.c
> > @@ -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>
> > +#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
> 
> > +
> > +#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
> 
> > +	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
> 
> > +{
> > +	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)
> 
> mode should be enum conversion_mode_sel?
> 
> > +{
> > +	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,
> 
> 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.

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

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.

> 
> > +
> > +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),
> > +	{},
> > +};
> > +
> > +#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
> 
> > +	.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:
> > +		/*
> > +		 * 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
> 
> > +	}
> > +
> > +	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 here should save two lines
> 
> > +	}
> > +
> > +	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",
> 
> "Failed getting ..."
> 
> > +			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");
> > +		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)
> 
> > +
> > +	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 <[e-mail]");
> > +MODULE_DESCRIPTION("Freescale VF610 DAC driver");
> > +MODULE_LICENSE("GPL v2");
> > 
> 
> -- 
> 
> Peter Meerwald-Stadler
> [phone] (mobile)

Rob Herring Feb. 12, 2016, 2:51 p.m. UTC | #4

On Tue, Feb 09, 2016 at 10:56:06AM +0530, Sanchayan Maity wrote:
> Add driver support for DAC peripheral on Vybrid SoC.
> 
> Signed-off-by: Sanchayan Maity <[e-mail]>
> ---
>  .../devicetree/bindings/iio/dac/vf610-dac.txt      |  20 ++

Acked-by: Rob Herring <[e-mail]>

>  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

Jonathan Cameron Feb. 13, 2016, 1:29 p.m. UTC | #5

On 12/02/16 06:34, [e-mail] wrote:
> Hello Peter,
> 
> On 16-02-09 07:43:14, Peter Meerwald-Stadler wrote:
>> On Tue, 9 Feb 2016, Sanchayan Maity wrote:
>>
>>> Add driver support for DAC peripheral on Vybrid SoC.
>>
>> comments below
>>  
>>> Signed-off-by: Sanchayan Maity <[e-mail]>
>>> ---
>>>  .../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
>>>
>>> diff --git a/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
>>> new file mode 100644
>>> index 0000000..97d7a40
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
>>> @@ -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.
>>
>> the last sentence is not quite clear; is the comma needed?
>>
>>> +
>>> +Example:
>>> +dac0: dac@400cc000 {
>>> +	compatible = "fsl,vf610-dac";
>>> +	reg = <0x400cc000 0x1000>;
>>> +	interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
>>> +	clock-names = "dac";
>>> +	clocks = <&clks VF610_CLK_DAC0>;
>>> +};
>>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
>>> index e701e28..8eb0502 100644
>>> --- a/drivers/iio/dac/Kconfig
>>> +++ b/drivers/iio/dac/Kconfig
>>> @@ -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
>>> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
>>> index 63ae056..93feb27 100644
>>> --- a/drivers/iio/dac/Makefile
>>> +++ b/drivers/iio/dac/Makefile
>>> @@ -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
>>> diff --git a/drivers/iio/dac/vf610_dac.c b/drivers/iio/dac/vf610_dac.c
>>> new file mode 100644
>>> index 0000000..775b349
>>> --- /dev/null
>>> +++ b/drivers/iio/dac/vf610_dac.c
>>> @@ -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>
>>> +#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
>>
>>> +
>>> +#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
>>
>>> +	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
>>
>>> +{
>>> +	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)
>>
>> mode should be enum conversion_mode_sel?
>>
>>> +{
>>> +	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,
>>
>> 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.
> 
>>
>>> +				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
> 
> 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!

> 
> 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
> 
> 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.
> 
>>
>>> +
>>> +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),
>>> +	{},
>>> +};
>>> +
>>> +#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
>>
>>> +	.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:
>>> +		/*
>>> +		 * 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
>>
>>> +	}
>>> +
>>> +	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 here should save two lines
>>
>>> +	}
>>> +
>>> +	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",
>>
>> "Failed getting ..."
>>
>>> +			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");
>>> +		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)
>>
>>> +
>>> +	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 <[e-mail]");
>>> +MODULE_DESCRIPTION("Freescale VF610 DAC driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
>>
>> -- 
>>
>> Peter Meerwald-Stadler
>> [phone] (mobile)

Sanchayan Feb. 13, 2016, 3:14 p.m. UTC | #6

Hello Jonathan,

On 16-02-13 13:29:39, Jonathan Cameron wrote:
> On 12/02/16 06:34, [e-mail] wrote:
> > Hello Peter,
> > 
> > On 16-02-09 07:43:14, Peter Meerwald-Stadler wrote:
> >> On Tue, 9 Feb 2016, Sanchayan Maity wrote:
> >>
> >>> Add driver support for DAC peripheral on Vybrid SoC.
> >>
> >> comments below
> >>  
> >>> Signed-off-by: Sanchayan Maity <[e-mail]>
> >>> ---
> >>>  .../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
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
> >>> new file mode 100644
> >>> index 0000000..97d7a40
> >>> --- /dev/null
> >>> +++ b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
> >>> @@ -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.
> >>
> >> the last sentence is not quite clear; is the comma needed?
> >>
> >>> +
> >>> +Example:
> >>> +dac0: dac@400cc000 {
> >>> +	compatible = "fsl,vf610-dac";
> >>> +	reg = <0x400cc000 0x1000>;
> >>> +	interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
> >>> +	clock-names = "dac";
> >>> +	clocks = <&clks VF610_CLK_DAC0>;
> >>> +};
> >>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> >>> index e701e28..8eb0502 100644
> >>> --- a/drivers/iio/dac/Kconfig
> >>> +++ b/drivers/iio/dac/Kconfig
> >>> @@ -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
> >>> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> >>> index 63ae056..93feb27 100644
> >>> --- a/drivers/iio/dac/Makefile
> >>> +++ b/drivers/iio/dac/Makefile
> >>> @@ -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
> >>> diff --git a/drivers/iio/dac/vf610_dac.c b/drivers/iio/dac/vf610_dac.c
> >>> new file mode 100644
> >>> index 0000000..775b349
> >>> --- /dev/null
> >>> +++ b/drivers/iio/dac/vf610_dac.c
> >>> @@ -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>
> >>> +#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
> >>
> >>> +
> >>> +#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
> >>
> >>> +	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
> >>
> >>> +{
> >>> +	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)
> >>
> >> mode should be enum conversion_mode_sel?
> >>
> >>> +{
> >>> +	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,
> >>
> >> 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.
> > 
> >>
> >>> +				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
> > 
> > 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!
> 
> > 
> > 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.

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.

> 
> Jonathan
> > 
> > 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.
> > 
> >>
> >>> +
> >>> +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),
> >>> +	{},
> >>> +};
> >>> +
> >>> +#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
> >>
> >>> +	.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:
> >>> +		/*
> >>> +		 * 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
> >>
> >>> +	}
> >>> +
> >>> +	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 here should save two lines
> >>
> >>> +	}
> >>> +
> >>> +	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",
> >>
> >> "Failed getting ..."
> >>
> >>> +			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");
> >>> +		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)
> >>
> >>> +
> >>> +	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 <[e-mail]");
> >>> +MODULE_DESCRIPTION("Freescale VF610 DAC driver");
> >>> +MODULE_LICENSE("GPL v2");
> >>>
> >>
> >> -- 
> >>
> >> Peter Meerwald-Stadler
> >> [phone] (mobile)
>

Jonathan Cameron Feb. 13, 2016, 3:26 p.m. UTC | #7

On 13 February 2016 15:14:53 GMT+00:00, [e-mail] wrote:
>Hello Jonathan,
>
>On 16-02-13 13:29:39, Jonathan Cameron wrote:
>> On 12/02/16 06:34, [e-mail] wrote:
>> > Hello Peter,
>> > 
>> > On 16-02-09 07:43:14, Peter Meerwald-Stadler wrote:
>> >> On Tue, 9 Feb 2016, Sanchayan Maity wrote:
>> >>
>> >>> Add driver support for DAC peripheral on Vybrid SoC.
>> >>
>> >> comments below
>> >>  
>> >>> Signed-off-by: Sanchayan Maity <[e-mail]>
>> >>> ---
>> >>>  .../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
>> >>>
>> >>> diff --git
>a/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
>b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
>> >>> new file mode 100644
>> >>> index 0000000..97d7a40
>> >>> --- /dev/null
>> >>> +++ b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
>> >>> @@ -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.
>> >>
>> >> the last sentence is not quite clear; is the comma needed?
>> >>
>> >>> +
>> >>> +Example:
>> >>> +dac0: dac@400cc000 {
>> >>> +	compatible = "fsl,vf610-dac";
>> >>> +	reg = <0x400cc000 0x1000>;
>> >>> +	interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
>> >>> +	clock-names = "dac";
>> >>> +	clocks = <&clks VF610_CLK_DAC0>;
>> >>> +};
>> >>> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
>> >>> index e701e28..8eb0502 100644
>> >>> --- a/drivers/iio/dac/Kconfig
>> >>> +++ b/drivers/iio/dac/Kconfig
>> >>> @@ -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
>> >>> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
>> >>> index 63ae056..93feb27 100644
>> >>> --- a/drivers/iio/dac/Makefile
>> >>> +++ b/drivers/iio/dac/Makefile
>> >>> @@ -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
>> >>> diff --git a/drivers/iio/dac/vf610_dac.c
>b/drivers/iio/dac/vf610_dac.c
>> >>> new file mode 100644
>> >>> index 0000000..775b349
>> >>> --- /dev/null
>> >>> +++ b/drivers/iio/dac/vf610_dac.c
>> >>> @@ -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>
>> >>> +#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
>> >>
>> >>> +
>> >>> +#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
>> >>
>> >>> +	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
>> >>
>> >>> +{
>> >>> +	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)
>> >>
>> >> mode should be enum conversion_mode_sel?
>> >>
>> >>> +{
>> >>> +	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,
>> >>
>> >> 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.
>> > 
>> >>
>> >>> +				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
>> > 
>> > 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!
>> 
>> > 
>> > 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.
>
>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
>
>Regards,
>Sanchayan.
>
>> 
>> Jonathan
>> > 
>> > 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.
>> > 
>> >>
>> >>> +
>> >>> +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),
>> >>> +	{},
>> >>> +};
>> >>> +
>> >>> +#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
>> >>
>> >>> +	.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:
>> >>> +		/*
>> >>> +		 * 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
>> >>
>> >>> +	}
>> >>> +
>> >>> +	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 here should save two lines
>> >>
>> >>> +	}
>> >>> +
>> >>> +	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",
>> >>
>> >> "Failed getting ..."
>> >>
>> >>> +			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");
>> >>> +		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)
>> >>
>> >>> +
>> >>> +	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 <[e-mail]");
>> >>> +MODULE_DESCRIPTION("Freescale VF610 DAC driver");
>> >>> +MODULE_LICENSE("GPL v2");
>> >>>
>> >>
>> >> -- 
>> >>
>> >> Peter Meerwald-Stadler
>> >> [phone] (mobile)
>> 
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to [e-mail]
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
new file mode 100644
index 0000000..97d7a40
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/vf610-dac.txt
@@ -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@400cc000 {
+	compatible = "fsl,vf610-dac";
+	reg = <0x400cc000 0x1000>;
+	interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
+	clock-names = "dac";
+	clocks = <&clks VF610_CLK_DAC0>;
+};
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index e701e28..8eb0502 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -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
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 63ae056..93feb27 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -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
diff --git a/drivers/iio/dac/vf610_dac.c b/drivers/iio/dac/vf610_dac.c
new file mode 100644
index 0000000..775b349
--- /dev/null
+++ b/drivers/iio/dac/vf610_dac.c
@@ -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>
+#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"
+
+#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"};
+
+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),
+	{},
+};
+
+#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:
+		*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;
+		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");
+		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);
+
+	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 <[e-mail]");
+MODULE_DESCRIPTION("Freescale VF610 DAC driver");
+MODULE_LICENSE("GPL v2");