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

From: jic23@kernel.org (Jonathan Cameron)
Date: 2016-02-13 15:26:19
Also in: linux-devicetree, linux-iio


On 13 February 2016 15:14:53 GMT+00:00, maitysanchayan at gmail.com wrote:

Hello Jonathan,

On 16-02-13 13:29:39, Jonathan Cameron wrote:

quoted

On 12/02/16 06:34, maitysanchayan at gmail.com wrote:

quoted

Hello Peter,

On 16-02-09 07:43:14, Peter Meerwald-Stadler wrote:

quoted

On Tue, 9 Feb 2016, Sanchayan Maity wrote:

quoted

Add driver support for DAC peripheral on Vybrid SoC.

comments below

quoted

Signed-off-by: Sanchayan Maity <redacted>
---
 .../devicetree/bindings/iio/dac/vf610-dac.txt      |  20 ++
 drivers/iio/dac/Kconfig                            |   8 +
 drivers/iio/dac/Makefile                           |   1 +
 drivers/iio/dac/vf610_dac.c                        | 302

+++++++++++++++++++++

quoted

 4 files changed, 331 insertions(+)
 create mode 100644

Documentation/devicetree/bindings/iio/dac/vf610-dac.txt

quoted

 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

quoted

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.

quoted

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

quoted

+
+Example:
+dac0: dac at 400cc000 {
+	compatible = "fsl,vf610-dac";
+	reg = <0x400cc000 0x1000>;
+	interrupts = <55 IRQ_TYPE_LEVEL_HIGH>;
+	clock-names = "dac";
+	clocks = <&clks VF610_CLK_DAC0>;
+};

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.

quoted

+
+	  This driver can also be built as a module. If so, the module

will

quoted

+	  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

quoted

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

quoted

+ * it under the terms of the GNU General Public License as

published by

quoted

+ * the Free Software Foundation; either version 2 of the

License, or

quoted

+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be

useful,

quoted

+ * but WITHOUT ANY WARRANTY; without even the implied warranty

of

quoted

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

quoted

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

quoted

+	VF610_DAC_CONV_HIGH_POWER,
+	VF610_DAC_CONV_LOW_POWER,
+};
+
+struct vf610_dac {
+	struct clk *clk;
+	struct device *dev;
+	enum conversion_mode_sel conv_mode;
+	void __iomem *regs;
+	int irq;
+};
+
+static int vf610_dac_init(struct vf610_dac *info)

should be void, or check the return value

quoted

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

quoted

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

quoted

        int (*get)(struct iio_dev *, const struct iio_chan_spec *);
};

the "mode" argument is unsigned int and return type of getter is

int as well.

quoted

+				const struct iio_chan_spec *chan)
+{
+	struct vf610_dac *info = iio_priv(indio_dev);
+
+	return info->conv_mode;
+}
+
+static const char * const vf610_conv_modes[] = { "high-power",

"low-power"};

quoted

this should be documented in sysfs-bus-iio-vf610; maybe high-power

should

quoted

be high-speed

probably this interface should have been exposed as a settable 
conversion rate

Vybrid DAC block does not provide for setting a conversion rate.

There is only one

quoted

register for configration and only one LPEN bit concerning power

mode. As per the

quoted

operating behavior, this controls characteristics of supply

current, settling time

quoted

and slew rate.

It strikes me that perhaps for DACs it would not be overkill to

actually

quoted

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

quoted

characteristics? Sounds right to me but my electronics background is
limited!

quoted

While I agree this should be documented in ABI sysfs documentation,

I am not clear

quoted

as to how that should be. Currently that document specifies the

modes observed for

quoted

the vf610 ADC driver. As per the datasheet, "high-power" and

"low-power" would be

quoted

the most clear description in case of DAC but "normal",

"high-speed" and "low-power"

quoted

are applicable for ADC. This just stuck me just now while making

the change.

quoted

Certainly not obvious.  Could use the slewrate or settling time as

explicit

quoted

controls on this (support both perhaps, with the usual IIO interface

rule

quoted

that you should never assume changing one sysfs attribute won't also

change

quoted

another!)

This would then correspond to some degree with the analogy of setting
effectively power modes for ADCs via sampling frequency as the

dominant

quoted

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.

quoted

Jonathan

quoted

How should I best include this? Peter, Jonathan, Stefan

suggestions?

quoted

I have changed the code to include all other feedback. Thank you

for feedback.

quoted

Best Regards,
Sanchayan.

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

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

quoted

+
+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 <sanchayan.maity@toradex.com");
+MODULE_DESCRIPTION("Freescale VF610 DAC driver");
+MODULE_LICENSE("GPL v2");

-- 

Peter Meerwald-Stadler
+43-664-2444418 (mobile)

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help