On Thu, 31 Aug 2017 01:17:15 +0700
s.abhi...@gmail.com wrote:
> From: Abhisit Sangjan <s.abhi...@gmail.com>
>
> TI LMP92001 Analog System Monitor and Controller
>
> 8-bit GPIOs.
> 12 DACs with 12-bit resolution.
>
> The GPIOs and DACs are shared port function with Cy function pin to
> take control the pin suddenly from external hardware.
> DAC's referance voltage selectable for Internal/External.
>
> 16 + 1 ADCs with 12-bit resolution.
>
> Built-in internal Temperature Sensor on channel 17.
> Window Comparator Function is supported on channel 1-3 and 9-11 for
> monitoring with interrupt signal (pending to implement for interrupt).
> ADC's referance voltage selectable for Internal/External.
>
> Signed-off-by: Abhisit Sangjan <s.abhi...@gmail.com>
Various comments inline.
Thanks,
Jonathan
> ---
> drivers/iio/dac/Kconfig | 9 ++
> drivers/iio/dac/Makefile | 1 +
> drivers/iio/dac/lmp92001-dac.c | 337
> +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 347 insertions(+)
> create mode 100644 drivers/iio/dac/lmp92001-dac.c
>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 25bed2d7d2b9..3e0d81606927 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -221,6 +221,15 @@ config DPOT_DAC
> To compile this driver as a module, choose M here: the module will be
> called dpot-dac.
>
> +config LMP92001_DAC
> + 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.
> +
> config LPC18XX_DAC
> tristate "NXP LPC18xx DAC driver"
> depends on ARCH_LPC18XX || COMPILE_TEST
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 603587cc2f07..516d2beced7b 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_AD7303) += ad7303.o
> obj-$(CONFIG_AD8801) += ad8801.o
> obj-$(CONFIG_CIO_DAC) += cio-dac.o
> obj-$(CONFIG_DPOT_DAC) += dpot-dac.o
> +obj-$(CONFIG_LMP92001_DAC) += lmp92001-dac.o
> obj-$(CONFIG_LPC18XX_DAC) += lpc18xx_dac.o
> obj-$(CONFIG_LTC2632) += ltc2632.o
> obj-$(CONFIG_M62332) += m62332.o
> diff --git a/drivers/iio/dac/lmp92001-dac.c b/drivers/iio/dac/lmp92001-dac.c
> new file mode 100644
> index 000000000000..072026e9895c
> --- /dev/null
> +++ b/drivers/iio/dac/lmp92001-dac.c
> @@ -0,0 +1,337 @@
> +/*
> + * lmp92001-dac.c - Support for TI LMP92001 DACs
> + *
> + * Copyright 2016-2017 Celestica Ltd.
> + *
> + * Author: Abhisit Sangjan <s.abhi...@gmail.com>
> + *
> + * 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/iio/iio.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.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:
> + mutex_lock(&lmp92001->dac_lock);
Why does it need to be locked? The regmap locks will protect
against concurrent writes of this (I think!) so I'm not
sure what dac_lock is for.
> +
> + ret = regmap_read(lmp92001->regmap,
> + 0x7F + channel->channel, val);
> +
> + mutex_unlock(&lmp92001->dac_lock);
> +
> + if (ret < 0)
> + return ret;
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static 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 < 0 || val > 4095)
> + return -EINVAL;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + switch (channel->type) {
> + case IIO_VOLTAGE:
> + mutex_lock(&lmp92001->dac_lock);
> +
> + ret = regmap_write(lmp92001->regmap,
> + 0x7F + channel->channel, val);
> +
> + mutex_unlock(&lmp92001->dac_lock);
> +
> + if (ret < 0)
> + return ret;
> +
> + return 0;
> + default:
> + return -EINVAL;
> + }
> + break;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static const struct iio_info lmp92001_info = {
> + .read_raw = lmp92001_read_raw,
> + .write_raw = lmp92001_write_raw,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static 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 & CREF_DEXT ? "external" : "internal");
> +}
> +
> +static 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 (strncmp("external", buf, 8) == 0)
> + cref = 1;
> + else if (strncmp("internal", buf, 8) == 0)
> + cref = 0;
> + else
> + return -EINVAL;
> +
> + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CREF, CREF_DEXT,
> + cref);
> + if (ret < 0)
> + return ret;
> +
> + return len;
> +}
> +
> +static 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 & CDAC_OFF)
> + outx = "hiz";
> + else {
> + if (cdac & CDAC_OLVL)
> + outx = "1 or dac";
> + else
> + outx = "0 or dac";
> + }
> +
> + return sprintf(buf, "%s\n", outx);
> +}
> +
> +static 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 (strncmp("hiz", buf, 3) == 0) {
> + cdac = CDAC_OFF;
> + mask = CDAC_OFF;
> + } else if (strncmp("dac", buf, 3) == 0) {
> + cdac = ~CDAC_OFF;
> + mask = CDAC_OFF;
> + } else if (strncmp("0", buf, 1) == 0) {
> + cdac = ~(CDAC_OLVL | CDAC_OFF);
> + mask = CDAC_OLVL | CDAC_OFF;
> + } else if (strncmp("1", buf, 1) == 0) {
> + cdac = CDAC_OLVL;
> + mask = CDAC_OLVL | CDAC_OFF;
> + } else
> + return -EINVAL;
> +
> + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CDAC, mask, cdac);
> + if (ret < 0)
> + return ret;
> +
> + return len;
> +}
> +
> +static 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 & CDAC_GANG ? "1" : "0");
> +}
> +
> +static 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;
Doesn't need an init. All paths fill it or exit before it is used.
> + int ret;
> +
> + if (strncmp("0", buf, 1) == 0)
> + cdac = ~CDAC_GANG;
> + else if (strncmp("1", buf, 1) == 0)
> + cdac = CDAC_GANG;
> + else
> + return -EINVAL;
> +
> + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CDAC, CDAC_GANG,
> + 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,
Similar comments to before.
> + }, {
> + .name = "outx",
> + .read = lmp92001_outx_read,
> + .write = lmp92001_outx_write,
> + .shared = IIO_SHARED_BY_ALL,
There are standard interfaces for this around the power_off modes.
Please use those.
> + }, {
> + .name = "gang",
Do we have a usecase where having this expose to userspace
makes sense? Seems likely to be a more hardware related thing to me.
> + .read = lmp92001_gang_read,
> + .write = lmp92001_gang_write,
> + .shared = IIO_SHARED_BY_ALL,
> + }, { },
> +};
> +
> +#define LMP92001_CHAN_SPEC(_ch) \
> +{ \
> + .channel = _ch, \
> + .type = IIO_VOLTAGE, \
> + .indexed = 1, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .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;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&pdev->dev, 0);
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + iio_device_set_drvdata(indio_dev, lmp92001);
> +
> + mutex_init(&lmp92001->dac_lock);
> +
> + 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;
Previously we have always had this a sysfs controlled rather
than coming from the device tree. Explain why it should be in
the device tree...
> +
> + of_property_read_u8(np, "ti,lmp92001-dac-outx", &outx);
> + cdac |= outx << 1;
This one is just odd. Not sure what it is supposed to be for.
Is it about making the pin a gpio? If so should be supported
explicitly through a gpio driver.
> +
> + of_property_read_u8(np, "ti,lmp92001-dac-gang", &gang);
> + cdac |= gang << 2;
I'll admit I am having trouble thinking of how else you could describe
this quite nasty bit of hardware logic.. Guess go with this until
someone comes up with something better!
> +
> + ret = regmap_update_bits(lmp92001->regmap, LMP92001_CDAC,
> + CDAC_GANG | CDAC_OLVL | CDAC_OFF, cdac);
> + if (ret < 0)
> + return ret;
> +
> + return devm_iio_device_register(&pdev->dev, indio_dev);
> +}
> +
> +static int lmp92001_dac_remove(struct platform_device *pdev)
> +{
> + return 0;
No need to provide an empty remove.
> +}
> +
> +static struct platform_driver lmp92001_dac_driver = {
> + .driver.name = "lmp92001-dac",
> + .probe = lmp92001_dac_probe,
> + .remove = lmp92001_dac_remove,
> +};
> +
> +static int __init lmp92001_dac_init(void)
> +{
> + return platform_driver_register(&lmp92001_dac_driver);
> +}
> +subsys_initcall(lmp92001_dac_init);
Again, justify this not being module_init. I'm unconvinced at
the moment.
> +
> +static void __exit lmp92001_dac_exit(void)
> +{
> + platform_driver_unregister(&lmp92001_dac_driver);
> +}
> +module_exit(lmp92001_dac_exit);
> +
> +MODULE_AUTHOR("Abhisit Sangjan <s.abhi...@gmail.com>");
> +MODULE_DESCRIPTION("IIO DAC interface for TI LMP92001");
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:lmp92001-dac");
