Re: [PATCH 3/5] iio: Add support for LMP92001 DAC

From: Peter Meerwald-Stadler
Date: Tue Aug 01 2017 - 07:17:00 EST



> From: Abhisit Sangjan <s.abhisit@xxxxxxxxx>

comments below

documentation is needed for the driver-specific API
some magic constants need a #define

> ---
> drivers/iio/dac/Kconfig | 9 ++
> drivers/iio/dac/Makefile | 1 +
> drivers/iio/dac/lmp92001-dac.c | 350 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 360 insertions(+)
> create mode 100644 drivers/iio/dac/lmp92001-dac.c
>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 25bed2d..69d4ad3 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -310,4 +310,13 @@ config VF610_DAC
> This driver can also be built as a module. If so, the module will
> be called vf610_dac.
>
> +config LMP92001_DAC

alphabetic order please

> + tristate "TI LMP92001 DAC Driver"
> + depends on MFD_LMP92001
> + help
> + If you say yes here you get support for TI LMP92001 DACs.
> +
> + This driver can also be built as a module. If so, the module will
> + be called lmp92001_dac.
> +
> endmenu
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 603587c..82395b6 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -33,3 +33,4 @@ obj-$(CONFIG_MCP4922) += mcp4922.o
> obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
> obj-$(CONFIG_STM32_DAC) += stm32-dac.o
> obj-$(CONFIG_VF610_DAC) += vf610_dac.o
> +obj-$(CONFIG_LMP92001_DAC) += lmp92001-dac.o

alphabetic order please

> diff --git a/drivers/iio/dac/lmp92001-dac.c b/drivers/iio/dac/lmp92001-dac.c
> new file mode 100644
> index 0000000..9ed4183
> --- /dev/null
> +++ b/drivers/iio/dac/lmp92001-dac.c
> @@ -0,0 +1,350 @@
> +/*
> + * lmp92001-dac.c - Support for TI LMP92001 DACs
> + *
> + * Copyright 2016-2017 Celestica Ltd.
> + *
> + * Author: Abhisit Sangjan <s.abhisit@xxxxxxxxx>
> + *
> + * Inspired by wm831x driver.
> + *
> + * 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/iio/iio.h>
> +#include <linux/mfd/core.h>
> +#include <linux/platform_device.h>
> +
> +#include <linux/mfd/lmp92001/core.h>
> +
> +static int lmp92001_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel, int *val,
> + int *val2, long mask)
> +{
> + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev);
> + int ret;
> +
> + switch (mask)
> + {
> + case IIO_CHAN_INFO_RAW:
> + switch (channel->type) {
> + case IIO_VOLTAGE:
> + ret = regmap_read(lmp92001->regmap,
> + 0x7F + channel->channel, val);
> + if (ret < 0)
> + return ret;
> +
> + return IIO_VAL_INT;
> + default:
> + break;

return -EINVAL;
directly here

> + }
> + break;
> + default:

return -EINVAL;
directly here

> + break;
> + }
> +
> + return -EINVAL;
> +}
> +
> +int lmp92001_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel,
> + int val, int val2, long mask)
> +{
> + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev);
> + int ret;
> +
> + if (val > 4095)

what about val < 0?

> + return -EINVAL;
> +
> + switch (mask)
> + {
> + case IIO_CHAN_INFO_RAW:
> + switch (channel->type) {
> + case IIO_VOLTAGE:
> + ret = regmap_write(lmp92001->regmap,
> + 0x7F + channel->channel, val);
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> + default:

return -EINVAL;
directly

> + break;
> + }
> + break;
> + default:

return -EINVAL;
directly

> + break;
> + }
> +
> + return -EINVAL;
> +}
> +
> +static const struct iio_info lmp92001_info = {
> + .read_raw = lmp92001_read_raw,
> + .write_raw = lmp92001_write_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +ssize_t lmp92001_dvref_read(struct iio_dev *indio_dev, uintptr_t private,
> + struct iio_chan_spec const *channel, char *buf)
> +{
> + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev);
> + unsigned int cref;
> + int ret;
> +
> + ret = regmap_read(lmp92001->regmap, LMP92001_CREF, &cref);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%s\n", cref & 1 ? "external" : "internal");
> +}
> +
> +ssize_t lmp92001_dvref_write(struct iio_dev *indio_dev, uintptr_t private,
> + struct iio_chan_spec const *channel, const char *buf,
> + size_t len)
> +{
> + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev);
> + unsigned int cref;
> + int ret;
> +
> + if (strcmp("external\n", buf) == 0)
> + cref = 1;
> + else if (strcmp("internal\n", buf) == 0)
> + cref = 0;
> + else
> + return -EINVAL;
> +
> + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CREF, 1, cref);
> + if (ret < 0)
> + return ret;
> +
> + return len;
> +}
> +
> +ssize_t lmp92001_outx_read(struct iio_dev *indio_dev, uintptr_t private,
> + struct iio_chan_spec const *channel, char *buf)
> +{
> + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev);
> + unsigned int cdac;
> + const char *outx;
> + int ret;
> +
> + ret = regmap_read(lmp92001->regmap, LMP92001_CDAC, &cdac);
> + if (ret < 0)
> + return ret;
> +
> + if (cdac & 1)
> + outx = "hiz";
> + else
> + {
> + if (cdac & 2)
> + outx = "1 or dac";
> + else
> + outx = "0 or dac";
> + }
> +
> + return sprintf(buf, "%s\n", outx);
> +}
> +
> +ssize_t lmp92001_outx_write(struct iio_dev *indio_dev, uintptr_t private,
> + struct iio_chan_spec const *channel, const char *buf,
> + size_t len)
> +{
> + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev);
> + unsigned int cdac, mask;
> + int ret;
> +
> + if (strcmp("hiz\n", buf) == 0)
> + {
> + cdac = 1;
> + mask = 1;
> + }
> + else if (strcmp("dac\n", buf) == 0)
> + {
> + cdac = 0;
> + mask = 1;
> + }
> + else if (strcmp("0\n", buf) == 0)
> + {
> + cdac = 0;
> + mask = 3;
> + }
> + else if (strcmp("1\n", buf) == 0)
> + {
> + cdac = 2;
> + mask = 3;
> + }
> + else
> + return -EINVAL;
> +
> + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CDAC, mask, cdac);
> + if (ret < 0)
> + return ret;
> +
> + return len;
> +}
> +
> +ssize_t lmp92001_gang_read(struct iio_dev *indio_dev, uintptr_t private,
> + struct iio_chan_spec const *channel, char *buf)
> +{
> + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev);
> + unsigned int cdac;
> + int ret;
> +
> + ret = regmap_read(lmp92001->regmap, LMP92001_CDAC, &cdac);
> + if (ret < 0)
> + return ret;
> +
> + return sprintf(buf, "%s\n", cdac & 4 ? "1" : "0");
> +}
> +
> +ssize_t lmp92001_gang_write(struct iio_dev *indio_dev, uintptr_t private,
> + struct iio_chan_spec const *channel, const char *buf,
> + size_t len)
> +{
> + struct lmp92001 *lmp92001 = iio_device_get_drvdata(indio_dev);
> + unsigned int cdac = 0;
> + int ret;
> +
> + if (strcmp("0\n", buf) == 0)
> + cdac = 0;
> + else if (strcmp("1\n", buf) == 0)
> + cdac = 4;
> + else
> + return -EINVAL;
> +
> + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CDAC, 4, cdac);
> + if (ret < 0)
> + return ret;
> +
> + return len;
> +}
> +
> +static const struct iio_chan_spec_ext_info lmp92001_ext_info[] = {
> + {
> + .name = "vref",
> + .read = lmp92001_dvref_read,
> + .write = lmp92001_dvref_write,
> + .shared = IIO_SHARED_BY_ALL,
> + },
> + {
> + .name = "outx",
> + .read = lmp92001_outx_read,
> + .write = lmp92001_outx_write,
> + .shared = IIO_SHARED_BY_ALL,
> + },
> + {
> + .name = "gang",
> + .read = lmp92001_gang_read,
> + .write = lmp92001_gang_write,
> + .shared = IIO_SHARED_BY_ALL,
> + },
> + { },
> +};
> +
> +#define LMP92001_CHAN_SPEC(_ch) \
> +{ \
> + .channel = _ch, \
> + .scan_index = _ch, \

.scan_index not needed

> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .ext_info = lmp92001_ext_info, \
> + .output = 1, \
> +}
> +
> +static const struct iio_chan_spec lmp92001_dac_channels[] = {
> + LMP92001_CHAN_SPEC(1),
> + LMP92001_CHAN_SPEC(2),
> + LMP92001_CHAN_SPEC(3),
> + LMP92001_CHAN_SPEC(4),
> + LMP92001_CHAN_SPEC(5),
> + LMP92001_CHAN_SPEC(6),
> + LMP92001_CHAN_SPEC(7),
> + LMP92001_CHAN_SPEC(8),
> + LMP92001_CHAN_SPEC(9),
> + LMP92001_CHAN_SPEC(10),
> + LMP92001_CHAN_SPEC(11),
> + LMP92001_CHAN_SPEC(12),
> +};
> +
> +static int lmp92001_dac_probe(struct platform_device *pdev)
> +{
> + struct lmp92001 *lmp92001 = dev_get_drvdata(pdev->dev.parent);
> + struct iio_dev *indio_dev;
> + struct device_node *np = pdev->dev.of_node;
> + u8 gang = 0, outx = 0, hiz = 0;
> + unsigned int cdac = 0;

initialization needed?

> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*lmp92001));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + iio_device_set_drvdata(indio_dev, lmp92001);
> +
> + indio_dev->name = pdev->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &lmp92001_info;
> + indio_dev->channels = lmp92001_dac_channels;
> + indio_dev->num_channels = ARRAY_SIZE(lmp92001_dac_channels);
> +
> + of_property_read_u8(np, "ti,lmp92001-dac-hiz", &hiz);
> + cdac |= hiz;
> +
> + of_property_read_u8(np, "ti,lmp92001-dac-outx", &outx);
> + cdac |= outx << 1;
> +
> + of_property_read_u8(np, "ti,lmp92001-dac-gang", &gang);
> + cdac |= gang << 2;
> +
> + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CDAC, 7, cdac);
> + if (ret < 0)
> + return ret;
> +
> + platform_set_drvdata(pdev, indio_dev);
> +
> + return iio_device_register(indio_dev);
> +}
> +
> +static int lmp92001_dac_remove(struct platform_device *pdev)

can use devm_iio_device_register() if _remove() is essentially empty

> +{
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> +
> + iio_device_unregister(indio_dev);
> +
> + return 0;
> +}
> +
> +static struct platform_driver lmp92001_dac_driver = {
> + .driver.name = "lmp92001-dac",
> + .driver.owner = THIS_MODULE,
> + .probe = lmp92001_dac_probe,

THIS_MODULE not needed anymore

> + .remove = lmp92001_dac_remove,
> +};
> +
> +static int __init lmp92001_dac_init(void)
> +{
> + return platform_driver_register(&lmp92001_dac_driver);
> +}
> +subsys_initcall(lmp92001_dac_init);
> +
> +static void __exit lmp92001_dac_exit(void)
> +{
> + platform_driver_unregister(&lmp92001_dac_driver);
> +}
> +module_exit(lmp92001_dac_exit);
> +
> +MODULE_AUTHOR("Abhisit Sangjan <s.abhisit@xxxxxxxxx>");
> +MODULE_DESCRIPTION("IIO DAC interface for TI LMP92001");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:lmp92001-dac");
>

--

Peter Meerwald-Stadler
Mobile: +43 664 24 44 418