On 13 February 2016 15:14:53 GMT+00:00, maitysanchayan@xxxxxxxxx wrote:
>Hello Jonathan,
>
>On 16-02-13 13:29:39, Jonathan Cameron wrote:
>> On 12/02/16 06:34, maitysanchayan@xxxxxxxxx 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 <maitysanchayan@xxxxxxxxx>
>> >>> ---
>> >>> .../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 <sanchayan.maity@xxxxxxxxxxx");
>> >>> +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@xxxxxxxxxxxxxxx
>More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
