[PATCH] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[PATCH] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[Michael Welling]

This patch provides an iio device driver for the Microchip
MCP49x2 series DACs.

Signed-off-by: Michael Welling <mwelling@ieee.org>
---
 drivers/iio/dac/Kconfig   |   10 +++
 drivers/iio/dac/Makefile  |    1 +
 drivers/iio/dac/mcp49x2.c |  215 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 226 insertions(+)
 create mode 100644 drivers/iio/dac/mcp49x2.c

diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index f378ca8..032d678 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -163,4 +163,14 @@ config MCP4725
 	  To compile this driver as a module, choose M here: the module
 	  will be called mcp4725.
 
+config MCP49X2
+	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
+	depends on SPI
+	---help---
+	  Say yes here to build the driver for the Microchip MCP49x2
+	  series DAC devices.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mcp49x2.
+
 endmenu
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index bb84ad6..2fdfc2e 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_AD5686) += ad5686.o
 obj-$(CONFIG_AD7303) += ad7303.o
 obj-$(CONFIG_MAX517) += max517.o
 obj-$(CONFIG_MCP4725) += mcp4725.o
+obj-$(CONFIG_MCP49X2) += mcp49x2.o
diff --git a/drivers/iio/dac/mcp49x2.c b/drivers/iio/dac/mcp49x2.c
new file mode 100644
index 0000000..0375046
--- /dev/null
+++ b/drivers/iio/dac/mcp49x2.c
@@ -0,0 +1,215 @@
+/*
+ * mcp49x2.c
+ *
+ * Driver for Microchip Digital to Analog Converters.
+ * Supports MCP4902, MCP4912, and MCP4922.
+ *
+ * Copyright (c) 2014 EMAC Inc.
+ *
+ * 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/module.h>
+#include <linux/init.h>
+#include <linux/spi/spi.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/regulator/consumer.h>
+
+#define	MCP49X2_RES_MASK(x)		((1 << (x)) - 1)
+
+enum mcp49x2_supported_device_ids {
+	ID_MCP4902,
+	ID_MCP4912,
+	ID_MCP4922,
+};
+
+struct mcp49x2_state {
+	struct spi_device *spi;
+	unsigned int value[2];
+	unsigned int power_mode[2];
+	unsigned int vref_mv;
+	struct regulator *vref_reg;
+};
+
+#define MCP49X2_CHAN(chan, bits) {			\
+	.type = IIO_VOLTAGE,				\
+	.output = 1,					\
+	.indexed = 1,					\
+	.channel = chan,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.address = chan,				\
+	.scan_type = IIO_ST('u', bits, 16, 12-(bits)),	\
+	.ext_info = NULL,				\
+}
+
+static int mcp49x2_spi_write(struct spi_device *spi, u8 addr, u32 val)
+{
+	u8 mosi[2];
+
+	mosi[1] = val & 0xff;
+	mosi[0] = (addr == 0) ? 0x00 : 0x80;
+	mosi[0] |= 0x30 | ((val >> 8) & 0x0f);
+
+	return spi_write(spi, mosi, 2);
+}
+
+static int mcp49x2_read_raw(struct iio_dev *indio_dev,
+		struct iio_chan_spec const *chan,
+		int *val,
+		int *val2,
+		long mask)
+{
+	struct mcp49x2_state *state = iio_priv(indio_dev);
+
+	if (chan->address >= 2)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*val = state->value[chan->address];
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = state->vref_mv;
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp49x2_write_raw(struct iio_dev *indio_dev,
+		struct iio_chan_spec const *chan,
+		int val,
+		int val2,
+		long mask)
+{
+	struct mcp49x2_state *state = iio_priv(indio_dev);
+
+	if (chan->address >= 2)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		val &= MCP49X2_RES_MASK(chan->scan_type.realbits);
+		val <<= chan->scan_type.shift;
+		state->value[chan->address] = val;
+		return mcp49x2_spi_write(state->spi, chan->address, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_chan_spec mcp49x2_channels[3][2] = {
+	[ID_MCP4902] = { MCP49X2_CHAN(0, 8),	MCP49X2_CHAN(1, 8) },
+	[ID_MCP4912] = { MCP49X2_CHAN(0, 10),	MCP49X2_CHAN(1, 10) },
+	[ID_MCP4922] = { MCP49X2_CHAN(0, 12),	MCP49X2_CHAN(1, 12) },
+};
+
+static const struct iio_info mcp49x2_info = {
+	.read_raw = &mcp49x2_read_raw,
+	.write_raw = &mcp49x2_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int mcp49x2_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct mcp49x2_state *state;
+	const struct spi_device_id *id;
+	int ret;
+
+	indio_dev = iio_device_alloc(sizeof(*state));
+	if (indio_dev == NULL)
+		return -ENOMEM;
+
+	state = iio_priv(indio_dev);
+	state->spi = spi;
+
+	state->vref_reg = devm_regulator_get(spi->dev, "vref");
+	if (!IS_ERR(state->vref_reg)) {
+		ret = regulator_enable(state->vref_reg);
+		if (ret) {
+			dev_err(dev, "Failed to enable vref regulator: %d\n",
+				ret);
+			goto error_free;
+		}
+
+		ret = regulator_get_voltage(state->vref_reg);
+		if (ret < 0)
+			goto error_disable_reg;
+
+		state->vref_mv = ret / 1000;
+	} else {
+		dev_info(dev, "Vref regulator not specified assuming 2.5V\n");
+		state->vref_mv = 2500;
+	}
+
+	spi_set_drvdata(spi, indio_dev);
+	id = spi_get_device_id(spi);
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &mcp49x2_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = mcp49x2_channels[id->driver_data];
+	indio_dev->num_channels = 2;
+	indio_dev->name = id->name;
+
+	ret = iio_device_register(indio_dev);
+
+	if (ret) {
+		dev_err(dev, "Failed to register iio device: %d\n",
+			ret);
+		goto error_disable_reg;
+	}
+
+	return 0;
+
+error_disable_reg:
+	if (!IS_ERR(state->vref_reg))
+		regulator_disable(state->vref_reg);
+error_free:
+	iio_device_free(indio_dev);
+
+	return ret;
+}
+
+static int mcp49x2_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+
+	iio_device_free(indio_dev);
+	return 0;
+}
+
+static const struct spi_device_id mcp49x2_id[] = {
+	{"mcp4902", ID_MCP4902},
+	{"mcp4912", ID_MCP4912},
+	{"mcp4922", ID_MCP4922},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, mcp49x2_id);
+
+static struct spi_driver mcp49x2_driver = {
+	.driver = {
+		   .name = "mcp49x2",
+		   .owner = THIS_MODULE,
+		   },
+	.probe = mcp49x2_probe,
+	.remove = mcp49x2_remove,
+	.id_table = mcp49x2_id,
+};
+module_spi_driver(mcp49x2_driver);
+
+MODULE_AUTHOR("Michael Welling <mwelling@ieee.org>");
+MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[Peter Meerwald]

> This patch provides an iio device driver for the Microchip
> MCP49x2 series DACs.

comments inline
 
> Signed-off-by: Michael Welling <mwelling@ieee.org>
> ---
>  drivers/iio/dac/Kconfig   |   10 +++
>  drivers/iio/dac/Makefile  |    1 +
>  drivers/iio/dac/mcp49x2.c |  215 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 226 insertions(+)
>  create mode 100644 drivers/iio/dac/mcp49x2.c
> 
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index f378ca8..032d678 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -163,4 +163,14 @@ config MCP4725
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called mcp4725.
>  
> +config MCP49X2
> +	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
> +	depends on SPI
> +	---help---
> +	  Say yes here to build the driver for the Microchip MCP49x2
> +	  series DAC devices.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called mcp49x2.
> +
>  endmenu
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index bb84ad6..2fdfc2e 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -18,3 +18,4 @@ obj-$(CONFIG_AD5686) += ad5686.o
>  obj-$(CONFIG_AD7303) += ad7303.o
>  obj-$(CONFIG_MAX517) += max517.o
>  obj-$(CONFIG_MCP4725) += mcp4725.o
> +obj-$(CONFIG_MCP49X2) += mcp49x2.o
> diff --git a/drivers/iio/dac/mcp49x2.c b/drivers/iio/dac/mcp49x2.c
> new file mode 100644
> index 0000000..0375046
> --- /dev/null
> +++ b/drivers/iio/dac/mcp49x2.c
> @@ -0,0 +1,215 @@
> +/*
> + * mcp49x2.c
> + *
> + * Driver for Microchip Digital to Analog Converters.
> + * Supports MCP4902, MCP4912, and MCP4922.
> + *
> + * Copyright (c) 2014 EMAC Inc.
> + *
> + * 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/module.h>
> +#include <linux/init.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define	MCP49X2_RES_MASK(x)		((1 << (x)) - 1)

use GENMASK() in linux/bitops.h

> +
> +enum mcp49x2_supported_device_ids {
> +	ID_MCP4902,
> +	ID_MCP4912,
> +	ID_MCP4922,
> +};
> +
> +struct mcp49x2_state {
> +	struct spi_device *spi;
> +	unsigned int value[2];
> +	unsigned int power_mode[2];
> +	unsigned int vref_mv;
> +	struct regulator *vref_reg;
> +};
> +
> +#define MCP49X2_CHAN(chan, bits) {			\
> +	.type = IIO_VOLTAGE,				\
> +	.output = 1,					\
> +	.indexed = 1,					\
> +	.channel = chan,				\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.address = chan,				\

could use .channel instead of .address

> +	.scan_type = IIO_ST('u', bits, 16, 12-(bits)),	\

consistently put macro arguments in () or don't

> +	.ext_info = NULL,				\

no need to set ext_info

> +}
> +
> +static int mcp49x2_spi_write(struct spi_device *spi, u8 addr, u32 val)
> +{
> +	u8 mosi[2];
> +
> +	mosi[1] = val & 0xff;
> +	mosi[0] = (addr == 0) ? 0x00 : 0x80;
> +	mosi[0] |= 0x30 | ((val >> 8) & 0x0f);
> +
> +	return spi_write(spi, mosi, 2);

SPI buffers need to be cacheline aligned?

> +}
> +
> +static int mcp49x2_read_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int *val,
> +		int *val2,
> +		long mask)
> +{
> +	struct mcp49x2_state *state = iio_priv(indio_dev);
> +
> +	if (chan->address >= 2)
> +		return -EINVAL;

no need to check the address; you are relying on chan being valid anyway

> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = state->value[chan->address];
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = state->vref_mv;
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int mcp49x2_write_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int val,
> +		int val2,
> +		long mask)
> +{
> +	struct mcp49x2_state *state = iio_priv(indio_dev);
> +
> +	if (chan->address >= 2)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:

maybe check the range of val and report EINVAL if out of range?
this silently makes val fit, no matter what

> +		val &= MCP49X2_RES_MASK(chan->scan_type.realbits);
> +		val <<= chan->scan_type.shift;
> +		state->value[chan->address] = val;
> +		return mcp49x2_spi_write(state->spi, chan->address, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_chan_spec mcp49x2_channels[3][2] = {
> +	[ID_MCP4902] = { MCP49X2_CHAN(0, 8),	MCP49X2_CHAN(1, 8) },
> +	[ID_MCP4912] = { MCP49X2_CHAN(0, 10),	MCP49X2_CHAN(1, 10) },
> +	[ID_MCP4922] = { MCP49X2_CHAN(0, 12),	MCP49X2_CHAN(1, 12) },
> +};
> +
> +static const struct iio_info mcp49x2_info = {
> +	.read_raw = &mcp49x2_read_raw,
> +	.write_raw = &mcp49x2_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int mcp49x2_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct mcp49x2_state *state;
> +	const struct spi_device_id *id;
> +	int ret;
> +
> +	indio_dev = iio_device_alloc(sizeof(*state));

devm_iio_device_alloc()

> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	state = iio_priv(indio_dev);
> +	state->spi = spi;
> +
> +	state->vref_reg = devm_regulator_get(spi->dev, "vref");
> +	if (!IS_ERR(state->vref_reg)) {
> +		ret = regulator_enable(state->vref_reg);
> +		if (ret) {
> +			dev_err(dev, "Failed to enable vref regulator: %d\n",
> +				ret);
> +			goto error_free;
> +		}
> +
> +		ret = regulator_get_voltage(state->vref_reg);
> +		if (ret < 0)
> +			goto error_disable_reg;
> +
> +		state->vref_mv = ret / 1000;
> +	} else {
> +		dev_info(dev, "Vref regulator not specified assuming 2.5V\n");
> +		state->vref_mv = 2500;
> +	}
> +
> +	spi_set_drvdata(spi, indio_dev);
> +	id = spi_get_device_id(spi);
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->info = &mcp49x2_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = mcp49x2_channels[id->driver_data];
> +	indio_dev->num_channels = 2;

maybe use a constant for the number of channels here and also
when declaring mcp49x2_channels[3][2]

> +	indio_dev->name = id->name;
> +
> +	ret = iio_device_register(indio_dev);

use devm_iio_device_register()
drop newline
> +
> +	if (ret) {
> +		dev_err(dev, "Failed to register iio device: %d\n",
> +			ret);
> +		goto error_disable_reg;
> +	}
> +
> +	return 0;
> +
> +error_disable_reg:
> +	if (!IS_ERR(state->vref_reg))
> +		regulator_disable(state->vref_reg);
> +error_free:
> +	iio_device_free(indio_dev);
> +
> +	return ret;
> +}
> +
> +static int mcp49x2_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +

iio_device_unregister()?
but should go away with devm_ stuff

> +	iio_device_free(indio_dev);
> +	return 0;
> +}
> +
> +static const struct spi_device_id mcp49x2_id[] = {
> +	{"mcp4902", ID_MCP4902},
> +	{"mcp4912", ID_MCP4912},
> +	{"mcp4922", ID_MCP4922},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, mcp49x2_id);
> +
> +static struct spi_driver mcp49x2_driver = {
> +	.driver = {
> +		   .name = "mcp49x2",
> +		   .owner = THIS_MODULE,
> +		   },
> +	.probe = mcp49x2_probe,
> +	.remove = mcp49x2_remove,
> +	.id_table = mcp49x2_id,
> +};
> +module_spi_driver(mcp49x2_driver);
> +
> +MODULE_AUTHOR("Michael Welling <mwelling@ieee.org>");
> +MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

Re: [PATCH] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[Michael Welling]

On Wed, Jun 04, 2014 at 11:45:40AM +0200, Peter Meerwald wrote:
> 
> > This patch provides an iio device driver for the Microchip
> > MCP49x2 series DACs.
> 
> comments inline
I appologize for submitting this patch in such a poor state.

Looking at it today I noticed compilation errors and fixed them.
The changes recommended were implemented as well.

I will craft a new version of the driver and submit the patch for
review as soon as it is ready.

>  
> > Signed-off-by: Michael Welling <mwelling@ieee.org>
> > ---
> >  drivers/iio/dac/Kconfig   |   10 +++
> >  drivers/iio/dac/Makefile  |    1 +
> >  drivers/iio/dac/mcp49x2.c |  215 +++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 226 insertions(+)
> >  create mode 100644 drivers/iio/dac/mcp49x2.c
> > 
> > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> > index f378ca8..032d678 100644
> > --- a/drivers/iio/dac/Kconfig
> > +++ b/drivers/iio/dac/Kconfig
> > @@ -163,4 +163,14 @@ config MCP4725
> >  	  To compile this driver as a module, choose M here: the module
> >  	  will be called mcp4725.
> >  
> > +config MCP49X2
> > +	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
> > +	depends on SPI
> > +	---help---
> > +	  Say yes here to build the driver for the Microchip MCP49x2
> > +	  series DAC devices.
> > +
> > +	  To compile this driver as a module, choose M here: the module
> > +	  will be called mcp49x2.
> > +
> >  endmenu
> > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> > index bb84ad6..2fdfc2e 100644
> > --- a/drivers/iio/dac/Makefile
> > +++ b/drivers/iio/dac/Makefile
> > @@ -18,3 +18,4 @@ obj-$(CONFIG_AD5686) += ad5686.o
> >  obj-$(CONFIG_AD7303) += ad7303.o
> >  obj-$(CONFIG_MAX517) += max517.o
> >  obj-$(CONFIG_MCP4725) += mcp4725.o
> > +obj-$(CONFIG_MCP49X2) += mcp49x2.o
> > diff --git a/drivers/iio/dac/mcp49x2.c b/drivers/iio/dac/mcp49x2.c
> > new file mode 100644
> > index 0000000..0375046
> > --- /dev/null
> > +++ b/drivers/iio/dac/mcp49x2.c
> > @@ -0,0 +1,215 @@
> > +/*
> > + * mcp49x2.c
> > + *
> > + * Driver for Microchip Digital to Analog Converters.
> > + * Supports MCP4902, MCP4912, and MCP4922.
> > + *
> > + * Copyright (c) 2014 EMAC Inc.
> > + *
> > + * 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/module.h>
> > +#include <linux/init.h>
> > +#include <linux/spi/spi.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +#define	MCP49X2_RES_MASK(x)		((1 << (x)) - 1)
> 
> use GENMASK() in linux/bitops.h
> 
> > +
> > +enum mcp49x2_supported_device_ids {
> > +	ID_MCP4902,
> > +	ID_MCP4912,
> > +	ID_MCP4922,
> > +};
> > +
> > +struct mcp49x2_state {
> > +	struct spi_device *spi;
> > +	unsigned int value[2];
> > +	unsigned int power_mode[2];
> > +	unsigned int vref_mv;
> > +	struct regulator *vref_reg;
> > +};
> > +
> > +#define MCP49X2_CHAN(chan, bits) {			\
> > +	.type = IIO_VOLTAGE,				\
> > +	.output = 1,					\
> > +	.indexed = 1,					\
> > +	.channel = chan,				\
> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> > +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > +	.address = chan,				\
> 
> could use .channel instead of .address
> 
> > +	.scan_type = IIO_ST('u', bits, 16, 12-(bits)),	\
> 
> consistently put macro arguments in () or don't
> 
> > +	.ext_info = NULL,				\
> 
> no need to set ext_info
> 
> > +}
> > +
> > +static int mcp49x2_spi_write(struct spi_device *spi, u8 addr, u32 val)
> > +{
> > +	u8 mosi[2];
> > +
> > +	mosi[1] = val & 0xff;
> > +	mosi[0] = (addr == 0) ? 0x00 : 0x80;
> > +	mosi[0] |= 0x30 | ((val >> 8) & 0x0f);
> > +
> > +	return spi_write(spi, mosi, 2);
> 
> SPI buffers need to be cacheline aligned?
> 
> > +}
> > +
> > +static int mcp49x2_read_raw(struct iio_dev *indio_dev,
> > +		struct iio_chan_spec const *chan,
> > +		int *val,
> > +		int *val2,
> > +		long mask)
> > +{
> > +	struct mcp49x2_state *state = iio_priv(indio_dev);
> > +
> > +	if (chan->address >= 2)
> > +		return -EINVAL;
> 
> no need to check the address; you are relying on chan being valid anyway
> 
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		*val = state->value[chan->address];
> > +		return IIO_VAL_INT;
> > +	case IIO_CHAN_INFO_SCALE:
> > +		*val = state->vref_mv;
> > +		*val2 = chan->scan_type.realbits;
> > +		return IIO_VAL_FRACTIONAL_LOG2;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static int mcp49x2_write_raw(struct iio_dev *indio_dev,
> > +		struct iio_chan_spec const *chan,
> > +		int val,
> > +		int val2,
> > +		long mask)
> > +{
> > +	struct mcp49x2_state *state = iio_priv(indio_dev);
> > +
> > +	if (chan->address >= 2)
> > +		return -EINVAL;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> 
> maybe check the range of val and report EINVAL if out of range?
> this silently makes val fit, no matter what
> 
> > +		val &= MCP49X2_RES_MASK(chan->scan_type.realbits);
> > +		val <<= chan->scan_type.shift;
> > +		state->value[chan->address] = val;
> > +		return mcp49x2_spi_write(state->spi, chan->address, val);
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +}
> > +
> > +static const struct iio_chan_spec mcp49x2_channels[3][2] = {
> > +	[ID_MCP4902] = { MCP49X2_CHAN(0, 8),	MCP49X2_CHAN(1, 8) },
> > +	[ID_MCP4912] = { MCP49X2_CHAN(0, 10),	MCP49X2_CHAN(1, 10) },
> > +	[ID_MCP4922] = { MCP49X2_CHAN(0, 12),	MCP49X2_CHAN(1, 12) },
> > +};
> > +
> > +static const struct iio_info mcp49x2_info = {
> > +	.read_raw = &mcp49x2_read_raw,
> > +	.write_raw = &mcp49x2_write_raw,
> > +	.driver_module = THIS_MODULE,
> > +};
> > +
> > +static int mcp49x2_probe(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev;
> > +	struct mcp49x2_state *state;
> > +	const struct spi_device_id *id;
> > +	int ret;
> > +
> > +	indio_dev = iio_device_alloc(sizeof(*state));
> 
> devm_iio_device_alloc()
> 
> > +	if (indio_dev == NULL)
> > +		return -ENOMEM;
> > +
> > +	state = iio_priv(indio_dev);
> > +	state->spi = spi;
> > +
> > +	state->vref_reg = devm_regulator_get(spi->dev, "vref");
> > +	if (!IS_ERR(state->vref_reg)) {
> > +		ret = regulator_enable(state->vref_reg);
> > +		if (ret) {
> > +			dev_err(dev, "Failed to enable vref regulator: %d\n",
> > +				ret);
> > +			goto error_free;
> > +		}
> > +
> > +		ret = regulator_get_voltage(state->vref_reg);
> > +		if (ret < 0)
> > +			goto error_disable_reg;
> > +
> > +		state->vref_mv = ret / 1000;
> > +	} else {
> > +		dev_info(dev, "Vref regulator not specified assuming 2.5V\n");
> > +		state->vref_mv = 2500;
> > +	}
> > +
> > +	spi_set_drvdata(spi, indio_dev);
> > +	id = spi_get_device_id(spi);
> > +	indio_dev->dev.parent = &spi->dev;
> > +	indio_dev->info = &mcp49x2_info;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->channels = mcp49x2_channels[id->driver_data];
> > +	indio_dev->num_channels = 2;
> 
> maybe use a constant for the number of channels here and also
> when declaring mcp49x2_channels[3][2]
> 
> > +	indio_dev->name = id->name;
> > +
> > +	ret = iio_device_register(indio_dev);
> 
> use devm_iio_device_register()
> drop newline
> > +
> > +	if (ret) {
> > +		dev_err(dev, "Failed to register iio device: %d\n",
> > +			ret);
> > +		goto error_disable_reg;
> > +	}
> > +
> > +	return 0;
> > +
> > +error_disable_reg:
> > +	if (!IS_ERR(state->vref_reg))
> > +		regulator_disable(state->vref_reg);
> > +error_free:
> > +	iio_device_free(indio_dev);
> > +
> > +	return ret;
> > +}
> > +
> > +static int mcp49x2_remove(struct spi_device *spi)
> > +{
> > +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> > +
> 
> iio_device_unregister()?
> but should go away with devm_ stuff
> 
> > +	iio_device_free(indio_dev);
> > +	return 0;
> > +}
> > +
> > +static const struct spi_device_id mcp49x2_id[] = {
> > +	{"mcp4902", ID_MCP4902},
> > +	{"mcp4912", ID_MCP4912},
> > +	{"mcp4922", ID_MCP4922},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(spi, mcp49x2_id);
> > +
> > +static struct spi_driver mcp49x2_driver = {
> > +	.driver = {
> > +		   .name = "mcp49x2",
> > +		   .owner = THIS_MODULE,
> > +		   },
> > +	.probe = mcp49x2_probe,
> > +	.remove = mcp49x2_remove,
> > +	.id_table = mcp49x2_id,
> > +};
> > +module_spi_driver(mcp49x2_driver);
> > +
> > +MODULE_AUTHOR("Michael Welling <mwelling@ieee.org>");
> > +MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
> > +MODULE_LICENSE("GPL v2");
> > 
> 
> -- 
> 
> Peter Meerwald
> +43-664-2444418 (mobile)

[PATCH v2] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[Michael Welling]

This patch provides an iio device driver for the Microchip
MCP49x2 series DACs.

Signed-off-by: Michael Welling <mwelling@ieee.org>
---
 drivers/iio/dac/Kconfig   |   10 +++
 drivers/iio/dac/Makefile  |    1 +
 drivers/iio/dac/mcp49x2.c |  213 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/iio/dac/mcp49x2.c

diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index f378ca8..032d678 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -163,4 +163,14 @@ config MCP4725
 	  To compile this driver as a module, choose M here: the module
 	  will be called mcp4725.
 
+config MCP49X2
+	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
+	depends on SPI
+	---help---
+	  Say yes here to build the driver for the Microchip MCP49x2
+	  series DAC devices.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mcp49x2.
+
 endmenu
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index bb84ad6..2fdfc2e 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_AD5686) += ad5686.o
 obj-$(CONFIG_AD7303) += ad7303.o
 obj-$(CONFIG_MAX517) += max517.o
 obj-$(CONFIG_MCP4725) += mcp4725.o
+obj-$(CONFIG_MCP49X2) += mcp49x2.o
diff --git a/drivers/iio/dac/mcp49x2.c b/drivers/iio/dac/mcp49x2.c
new file mode 100644
index 0000000..f90a83a
--- /dev/null
+++ b/drivers/iio/dac/mcp49x2.c
@@ -0,0 +1,213 @@
+/*
+ * mcp49x2.c
+ *
+ * Driver for Microchip Digital to Analog Converters.
+ * Supports MCP4902, MCP4912, and MCP4922.
+ *
+ * Copyright (c) 2014 EMAC Inc.
+ *
+ * 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/module.h>
+#include <linux/init.h>
+#include <linux/spi/spi.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/regulator/consumer.h>
+#include <linux/bitops.h>
+
+#define	MCP49X2_RES_MASK(x)	GENMASK(x, 0)
+#define MCP49X2_NUM_CHANNELS	2
+
+enum mcp49x2_supported_device_ids {
+	ID_MCP4902,
+	ID_MCP4912,
+	ID_MCP4922,
+};
+
+struct mcp49x2_state {
+	struct spi_device *spi;
+	unsigned int value[MCP49X2_NUM_CHANNELS];
+	unsigned int power_mode[MCP49X2_NUM_CHANNELS];
+	unsigned int vref_mv;
+	struct regulator *vref_reg;
+};
+
+#define MCP49X2_CHAN(chan, bits) {			\
+	.type = IIO_VOLTAGE,				\
+	.output = 1,					\
+	.indexed = 1,					\
+	.channel = chan,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.scan_type = IIO_ST('u', bits, 16, 12-bits),	\
+}
+
+static int mcp49x2_spi_write(struct spi_device *spi, u8 addr, u32 val)
+{
+	u8 mosi[2] ____cacheline_aligned;
+
+	mosi[1] = val & 0xff;
+	mosi[0] = (addr == 0) ? 0x00: 0x80;
+	mosi[0] |= 0x30 | ((val >> 8) & 0x0f);
+
+	return spi_write(spi, mosi, 2);
+}
+
+static int mcp49x2_read_raw(struct iio_dev *indio_dev,
+		struct iio_chan_spec const *chan,
+		int *val,
+		int *val2,
+		long mask)
+{
+	struct mcp49x2_state *state = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*val = state->value[chan->address];
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = state->vref_mv;
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp49x2_write_raw(struct iio_dev *indio_dev,
+		struct iio_chan_spec const *chan,
+		int val,
+		int val2,
+		long mask)
+{
+	struct mcp49x2_state *state = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (val > MCP49X2_RES_MASK(chan->scan_type.realbits))
+			return -EINVAL;
+		val <<= chan->scan_type.shift;
+		state->value[chan->address] = val;
+		return mcp49x2_spi_write(state->spi, chan->address, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_chan_spec mcp49x2_channels[3][MCP49X2_NUM_CHANNELS] = {
+	[ID_MCP4902] = { MCP49X2_CHAN(0, 8),	MCP49X2_CHAN(1, 8) },
+	[ID_MCP4912] = { MCP49X2_CHAN(0, 10),	MCP49X2_CHAN(1, 10) },
+	[ID_MCP4922] = { MCP49X2_CHAN(0, 12),	MCP49X2_CHAN(1, 12) },
+};
+
+static const struct iio_info mcp49x2_info = {
+	.read_raw = &mcp49x2_read_raw,
+	.write_raw = &mcp49x2_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int mcp49x2_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct mcp49x2_state *state;
+	const struct spi_device_id *id;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
+	if (indio_dev == NULL)
+		return -ENOMEM;
+
+	state = iio_priv(indio_dev);
+	state->spi = spi;
+	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
+	if (!IS_ERR(state->vref_reg)) {
+		ret = regulator_enable(state->vref_reg);
+		if (ret) {
+			dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
+				ret);
+			return ret;
+		}
+
+		ret = regulator_get_voltage(state->vref_reg);
+		if (ret < 0) {
+			dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
+				ret);
+			goto error_disable_reg;
+		}
+		state->vref_mv = ret / 1000;
+	} else {
+		dev_info(&spi->dev, "Vref regulator not specified assuming 2.5V\n");
+		state->vref_mv = 2500;
+	}
+
+	spi_set_drvdata(spi, indio_dev);
+	id = spi_get_device_id(spi);
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &mcp49x2_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = mcp49x2_channels[id->driver_data];
+	indio_dev->num_channels = MCP49X2_NUM_CHANNELS;
+	indio_dev->name = id->name;
+
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to register iio device: %d\n",
+				ret);
+		goto error_disable_reg;
+	}
+
+	return 0;
+
+error_disable_reg:
+	if (!IS_ERR(state->vref_reg))
+		regulator_disable(state->vref_reg);
+
+	return ret;
+}
+
+static int mcp49x2_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct mcp49x2_state *state;
+
+	state = iio_priv(indio_dev);
+
+	if (!IS_ERR(state->vref_reg))
+		regulator_disable(state->vref_reg);
+
+	return 0;
+}
+
+static const struct spi_device_id mcp49x2_id[] = {
+	{"mcp4902", ID_MCP4902},
+	{"mcp4912", ID_MCP4912},
+	{"mcp4922", ID_MCP4922},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, mcp49x2_id);
+
+static struct spi_driver mcp49x2_driver = {
+	.driver = {
+		   .name = "mcp49x2",
+		   .owner = THIS_MODULE,
+		   },
+	.probe = mcp49x2_probe,
+	.remove = mcp49x2_remove,
+	.id_table = mcp49x2_id,
+};
+module_spi_driver(mcp49x2_driver);
+
+MODULE_AUTHOR("Michael Welling <mwelling@ieee.org>");
+MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v2] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[Peter Meerwald]

> This patch provides an iio device driver for the Microchip
> MCP49x2 series DACs.

comments inline
 
> Signed-off-by: Michael Welling <mwelling@ieee.org>
> ---
>  drivers/iio/dac/Kconfig   |   10 +++
>  drivers/iio/dac/Makefile  |    1 +
>  drivers/iio/dac/mcp49x2.c |  213 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 224 insertions(+)
>  create mode 100644 drivers/iio/dac/mcp49x2.c
> 
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index f378ca8..032d678 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -163,4 +163,14 @@ config MCP4725
>  	  To compile this driver as a module, choose M here: the module
>  	  will be called mcp4725.
>  
> +config MCP49X2
> +	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
> +	depends on SPI
> +	---help---
> +	  Say yes here to build the driver for the Microchip MCP49x2
> +	  series DAC devices.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called mcp49x2.
> +
>  endmenu
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index bb84ad6..2fdfc2e 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -18,3 +18,4 @@ obj-$(CONFIG_AD5686) += ad5686.o
>  obj-$(CONFIG_AD7303) += ad7303.o
>  obj-$(CONFIG_MAX517) += max517.o
>  obj-$(CONFIG_MCP4725) += mcp4725.o
> +obj-$(CONFIG_MCP49X2) += mcp49x2.o
> diff --git a/drivers/iio/dac/mcp49x2.c b/drivers/iio/dac/mcp49x2.c
> new file mode 100644
> index 0000000..f90a83a
> --- /dev/null
> +++ b/drivers/iio/dac/mcp49x2.c
> @@ -0,0 +1,213 @@
> +/*
> + * mcp49x2.c
> + *
> + * Driver for Microchip Digital to Analog Converters.
> + * Supports MCP4902, MCP4912, and MCP4922.
> + *
> + * Copyright (c) 2014 EMAC Inc.
> + *
> + * 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/module.h>
> +#include <linux/init.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/bitops.h>
> +
> +#define	MCP49X2_RES_MASK(x)	GENMASK(x, 0)

MCP49X2_RES_MASK() could go away
it should be GENMASK(x-1, 0)

> +#define MCP49X2_NUM_CHANNELS	2
> +
> +enum mcp49x2_supported_device_ids {
> +	ID_MCP4902,
> +	ID_MCP4912,
> +	ID_MCP4922,
> +};
> +
> +struct mcp49x2_state {
> +	struct spi_device *spi;
> +	unsigned int value[MCP49X2_NUM_CHANNELS];
> +	unsigned int power_mode[MCP49X2_NUM_CHANNELS];
> +	unsigned int vref_mv;
> +	struct regulator *vref_reg;
> +};
> +
> +#define MCP49X2_CHAN(chan, bits) {			\
> +	.type = IIO_VOLTAGE,				\
> +	.output = 1,					\
> +	.indexed = 1,					\
> +	.channel = chan,				\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.scan_type = IIO_ST('u', bits, 16, 12-bits),	\
> +}
> +
> +static int mcp49x2_spi_write(struct spi_device *spi, u8 addr, u32 val)
> +{
> +	u8 mosi[2] ____cacheline_aligned;
> +
> +	mosi[1] = val & 0xff;
> +	mosi[0] = (addr == 0) ? 0x00: 0x80;
> +	mosi[0] |= 0x30 | ((val >> 8) & 0x0f);
> +
> +	return spi_write(spi, mosi, 2);
> +}
> +
> +static int mcp49x2_read_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int *val,
> +		int *val2,
> +		long mask)
> +{
> +	struct mcp49x2_state *state = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = state->value[chan->address];

address is gone, use chan->channel

> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = state->vref_mv;
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int mcp49x2_write_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int val,
> +		int val2,
> +		long mask)
> +{
> +	struct mcp49x2_state *state = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val > MCP49X2_RES_MASK(chan->scan_type.realbits))
> +			return -EINVAL;
> +		val <<= chan->scan_type.shift;
> +		state->value[chan->address] = val;

use chan->channel

> +		return mcp49x2_spi_write(state->spi, chan->address, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_chan_spec mcp49x2_channels[3][MCP49X2_NUM_CHANNELS] = {
> +	[ID_MCP4902] = { MCP49X2_CHAN(0, 8),	MCP49X2_CHAN(1, 8) },
> +	[ID_MCP4912] = { MCP49X2_CHAN(0, 10),	MCP49X2_CHAN(1, 10) },
> +	[ID_MCP4922] = { MCP49X2_CHAN(0, 12),	MCP49X2_CHAN(1, 12) },
> +};
> +
> +static const struct iio_info mcp49x2_info = {
> +	.read_raw = &mcp49x2_read_raw,
> +	.write_raw = &mcp49x2_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int mcp49x2_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct mcp49x2_state *state;
> +	const struct spi_device_id *id;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	state = iio_priv(indio_dev);
> +	state->spi = spi;
> +	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
> +	if (!IS_ERR(state->vref_reg)) {
> +		ret = regulator_enable(state->vref_reg);
> +		if (ret) {
> +			dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		ret = regulator_get_voltage(state->vref_reg);
> +		if (ret < 0) {
> +			dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
> +				ret);
> +			goto error_disable_reg;
> +		}
> +		state->vref_mv = ret / 1000;
> +	} else {
> +		dev_info(&spi->dev, "Vref regulator not specified assuming 2.5V\n");
> +		state->vref_mv = 2500;
> +	}
> +
> +	spi_set_drvdata(spi, indio_dev);
> +	id = spi_get_device_id(spi);
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->info = &mcp49x2_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = mcp49x2_channels[id->driver_data];
> +	indio_dev->num_channels = MCP49X2_NUM_CHANNELS;
> +	indio_dev->name = id->name;
> +
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to register iio device: %d\n",
> +				ret);
> +		goto error_disable_reg;
> +	}
> +
> +	return 0;
> +
> +error_disable_reg:
> +	if (!IS_ERR(state->vref_reg))
> +		regulator_disable(state->vref_reg);
> +
> +	return ret;
> +}
> +
> +static int mcp49x2_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct mcp49x2_state *state;
> +
> +	state = iio_priv(indio_dev);
> +
> +	if (!IS_ERR(state->vref_reg))
> +		regulator_disable(state->vref_reg);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id mcp49x2_id[] = {
> +	{"mcp4902", ID_MCP4902},
> +	{"mcp4912", ID_MCP4912},
> +	{"mcp4922", ID_MCP4922},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, mcp49x2_id);
> +
> +static struct spi_driver mcp49x2_driver = {
> +	.driver = {
> +		   .name = "mcp49x2",
> +		   .owner = THIS_MODULE,
> +		   },
> +	.probe = mcp49x2_probe,
> +	.remove = mcp49x2_remove,
> +	.id_table = mcp49x2_id,
> +};
> +module_spi_driver(mcp49x2_driver);
> +
> +MODULE_AUTHOR("Michael Welling <mwelling@ieee.org>");
> +MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
> +MODULE_LICENSE("GPL v2");
> 

-- 

Peter Meerwald
+43-664-2444418 (mobile)

[PATCH v3] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[Michael Welling]

This patch provides an iio device driver for the Microchip
MCP49x2 series DACs.

Signed-off-by: Michael Welling <mwelling@ieee.org>
---
 drivers/iio/dac/Kconfig   |   10 +++
 drivers/iio/dac/Makefile  |    1 +
 drivers/iio/dac/mcp49x2.c |  212 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 223 insertions(+)
 create mode 100644 drivers/iio/dac/mcp49x2.c

diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index f378ca8..032d678 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -163,4 +163,14 @@ config MCP4725
 	  To compile this driver as a module, choose M here: the module
 	  will be called mcp4725.
 
+config MCP49X2
+	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
+	depends on SPI
+	---help---
+	  Say yes here to build the driver for the Microchip MCP49x2
+	  series DAC devices.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mcp49x2.
+
 endmenu
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index bb84ad6..2fdfc2e 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_AD5686) += ad5686.o
 obj-$(CONFIG_AD7303) += ad7303.o
 obj-$(CONFIG_MAX517) += max517.o
 obj-$(CONFIG_MCP4725) += mcp4725.o
+obj-$(CONFIG_MCP49X2) += mcp49x2.o
diff --git a/drivers/iio/dac/mcp49x2.c b/drivers/iio/dac/mcp49x2.c
new file mode 100644
index 0000000..398282c
--- /dev/null
+++ b/drivers/iio/dac/mcp49x2.c
@@ -0,0 +1,212 @@
+/*
+ * mcp49x2.c
+ *
+ * Driver for Microchip Digital to Analog Converters.
+ * Supports MCP4902, MCP4912, and MCP4922.
+ *
+ * Copyright (c) 2014 EMAC Inc.
+ *
+ * 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/module.h>
+#include <linux/init.h>
+#include <linux/spi/spi.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/regulator/consumer.h>
+#include <linux/bitops.h>
+
+#define MCP49X2_NUM_CHANNELS	2
+
+enum mcp49x2_supported_device_ids {
+	ID_MCP4902,
+	ID_MCP4912,
+	ID_MCP4922,
+};
+
+struct mcp49x2_state {
+	struct spi_device *spi;
+	unsigned int value[MCP49X2_NUM_CHANNELS];
+	unsigned int power_mode[MCP49X2_NUM_CHANNELS];
+	unsigned int vref_mv;
+	struct regulator *vref_reg;
+};
+
+#define MCP49X2_CHAN(chan, bits) {			\
+	.type = IIO_VOLTAGE,				\
+	.output = 1,					\
+	.indexed = 1,					\
+	.channel = chan,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.scan_type = IIO_ST('u', bits, 16, 12-bits),	\
+}
+
+static int mcp49x2_spi_write(struct spi_device *spi, u8 addr, u32 val)
+{
+	u8 mosi[2] ____cacheline_aligned;
+
+	mosi[1] = val & 0xff;
+	mosi[0] = (addr == 0) ? 0x00 : 0x80;
+	mosi[0] |= 0x30 | ((val >> 8) & 0x0f);
+
+	return spi_write(spi, mosi, 2);
+}
+
+static int mcp49x2_read_raw(struct iio_dev *indio_dev,
+		struct iio_chan_spec const *chan,
+		int *val,
+		int *val2,
+		long mask)
+{
+	struct mcp49x2_state *state = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*val = state->value[chan->channel];
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = state->vref_mv;
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp49x2_write_raw(struct iio_dev *indio_dev,
+		struct iio_chan_spec const *chan,
+		int val,
+		int val2,
+		long mask)
+{
+	struct mcp49x2_state *state = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (val > GENMASK(chan->scan_type.realbits-1, 0))
+			return -EINVAL;
+		val <<= chan->scan_type.shift;
+		state->value[chan->channel] = val;
+		return mcp49x2_spi_write(state->spi, chan->channel, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_chan_spec mcp49x2_channels[3][MCP49X2_NUM_CHANNELS] = {
+	[ID_MCP4902] = { MCP49X2_CHAN(0, 8),	MCP49X2_CHAN(1, 8) },
+	[ID_MCP4912] = { MCP49X2_CHAN(0, 10),	MCP49X2_CHAN(1, 10) },
+	[ID_MCP4922] = { MCP49X2_CHAN(0, 12),	MCP49X2_CHAN(1, 12) },
+};
+
+static const struct iio_info mcp49x2_info = {
+	.read_raw = &mcp49x2_read_raw,
+	.write_raw = &mcp49x2_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int mcp49x2_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct mcp49x2_state *state;
+	const struct spi_device_id *id;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
+	if (indio_dev == NULL)
+		return -ENOMEM;
+
+	state = iio_priv(indio_dev);
+	state->spi = spi;
+	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
+	if (!IS_ERR(state->vref_reg)) {
+		ret = regulator_enable(state->vref_reg);
+		if (ret) {
+			dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
+				ret);
+			return ret;
+		}
+
+		ret = regulator_get_voltage(state->vref_reg);
+		if (ret < 0) {
+			dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
+				ret);
+			goto error_disable_reg;
+		}
+		state->vref_mv = ret / 1000;
+	} else {
+		dev_info(&spi->dev, "Vref regulator not specified assuming 2.5V\n");
+		state->vref_mv = 2500;
+	}
+
+	spi_set_drvdata(spi, indio_dev);
+	id = spi_get_device_id(spi);
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &mcp49x2_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = mcp49x2_channels[id->driver_data];
+	indio_dev->num_channels = MCP49X2_NUM_CHANNELS;
+	indio_dev->name = id->name;
+
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to register iio device: %d\n",
+				ret);
+		goto error_disable_reg;
+	}
+
+	return 0;
+
+error_disable_reg:
+	if (!IS_ERR(state->vref_reg))
+		regulator_disable(state->vref_reg);
+
+	return ret;
+}
+
+static int mcp49x2_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct mcp49x2_state *state;
+
+	state = iio_priv(indio_dev);
+
+	if (!IS_ERR(state->vref_reg))
+		regulator_disable(state->vref_reg);
+
+	return 0;
+}
+
+static const struct spi_device_id mcp49x2_id[] = {
+	{"mcp4902", ID_MCP4902},
+	{"mcp4912", ID_MCP4912},
+	{"mcp4922", ID_MCP4922},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, mcp49x2_id);
+
+static struct spi_driver mcp49x2_driver = {
+	.driver = {
+		   .name = "mcp49x2",
+		   .owner = THIS_MODULE,
+		   },
+	.probe = mcp49x2_probe,
+	.remove = mcp49x2_remove,
+	.id_table = mcp49x2_id,
+};
+module_spi_driver(mcp49x2_driver);
+
+MODULE_AUTHOR("Michael Welling <mwelling@ieee.org>");
+MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v3] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[Lars-Peter Clausen]

On 06/06/2014 03:12 AM, Michael Welling wrote:
[...]
> +enum mcp49x2_supported_device_ids {
> +	ID_MCP4902,
> +	ID_MCP4912,
> +	ID_MCP4922,
> +};
> +
> +struct mcp49x2_state {
> +	struct spi_device *spi;
> +	unsigned int value[MCP49X2_NUM_CHANNELS];
> +	unsigned int power_mode[MCP49X2_NUM_CHANNELS];
> +	unsigned int vref_mv;
> +	struct regulator *vref_reg;
> +};
> +
> +#define MCP49X2_CHAN(chan, bits) {			\
> +	.type = IIO_VOLTAGE,				\
> +	.output = 1,					\
> +	.indexed = 1,					\
> +	.channel = chan,				\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.scan_type = IIO_ST('u', bits, 16, 12-bits),	\

The IIO_ST macro was removed a while ago, this will not compile with the 
latest IIO sources.

> +}
> +
> +static int mcp49x2_spi_write(struct spi_device *spi, u8 addr, u32 val)
> +{
> +	u8 mosi[2] ____cacheline_aligned;

It is also important that the transfer buffer is not on the stack and 
strictly speaking the buffer should also be allocated with GFP_DMA.

> +
> +	mosi[1] = val & 0xff;
> +	mosi[0] = (addr == 0) ? 0x00 : 0x80;
> +	mosi[0] |= 0x30 | ((val >> 8) & 0x0f);
> +
> +	return spi_write(spi, mosi, 2);
> +}
[...]
> +
> +static int mcp49x2_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct mcp49x2_state *state;
> +	const struct spi_device_id *id;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	state = iio_priv(indio_dev);
> +	state->spi = spi;
> +	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
> +	if (!IS_ERR(state->vref_reg)) {
> +		ret = regulator_enable(state->vref_reg);
> +		if (ret) {
> +			dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		ret = regulator_get_voltage(state->vref_reg);
> +		if (ret < 0) {
> +			dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
> +				ret);
> +			goto error_disable_reg;
> +		}
> +		state->vref_mv = ret / 1000;
> +	} else {
> +		dev_info(&spi->dev, "Vref regulator not specified assuming 2.5V\n");
> +		state->vref_mv = 2500;

This is not a good idea, unless there is a built-in vref with 2.5V. If that 
is the case use devm_regulator_get_optional() as devm_regulator_get() will 
return a dummy regulator if no regulator was specified for the device.

> +	}
> +
> +	spi_set_drvdata(spi, indio_dev);
> +	id = spi_get_device_id(spi);
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->info = &mcp49x2_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = mcp49x2_channels[id->driver_data];
> +	indio_dev->num_channels = MCP49X2_NUM_CHANNELS;
> +	indio_dev->name = id->name;
> +
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to register iio device: %d\n",
> +				ret);
> +		goto error_disable_reg;
> +	}
> +
> +	return 0;
> +
> +error_disable_reg:
> +	if (!IS_ERR(state->vref_reg))
> +		regulator_disable(state->vref_reg);
> +
> +	return ret;
> +}

[PATCH v4] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[Michael Welling]

This patch provides an iio device driver for the Microchip
MCP49x2 series DACs.

Signed-off-by: Michael Welling <mwelling@ieee.org>
---
 drivers/iio/dac/Kconfig   |   10 +++
 drivers/iio/dac/Makefile  |    1 +
 drivers/iio/dac/mcp49x2.c |  213 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/iio/dac/mcp49x2.c

diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index f378ca8..032d678 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -163,4 +163,14 @@ config MCP4725
 	  To compile this driver as a module, choose M here: the module
 	  will be called mcp4725.
 
+config MCP49X2
+	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
+	depends on SPI
+	---help---
+	  Say yes here to build the driver for the Microchip MCP49x2
+	  series DAC devices.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mcp49x2.
+
 endmenu
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index bb84ad6..2fdfc2e 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_AD5686) += ad5686.o
 obj-$(CONFIG_AD7303) += ad7303.o
 obj-$(CONFIG_MAX517) += max517.o
 obj-$(CONFIG_MCP4725) += mcp4725.o
+obj-$(CONFIG_MCP49X2) += mcp49x2.o
diff --git a/drivers/iio/dac/mcp49x2.c b/drivers/iio/dac/mcp49x2.c
new file mode 100644
index 0000000..073af6e
--- /dev/null
+++ b/drivers/iio/dac/mcp49x2.c
@@ -0,0 +1,213 @@
+/*
+ * mcp49x2.c
+ *
+ * Driver for Microchip Digital to Analog Converters.
+ * Supports MCP4902, MCP4912, and MCP4922.
+ *
+ * Copyright (c) 2014 EMAC Inc.
+ *
+ * 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/module.h>
+#include <linux/init.h>
+#include <linux/spi/spi.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/regulator/consumer.h>
+#include <linux/bitops.h>
+
+#define MCP49X2_NUM_CHANNELS	2
+
+enum mcp49x2_supported_device_ids {
+	ID_MCP4902,
+	ID_MCP4912,
+	ID_MCP4922,
+};
+
+struct mcp49x2_state {
+	struct spi_device *spi;
+	unsigned int value[MCP49X2_NUM_CHANNELS];
+	unsigned int power_mode[MCP49X2_NUM_CHANNELS];
+	unsigned int vref_mv;
+	struct regulator *vref_reg;
+	u8 mosi[2] ___cacheline_aligned;
+};
+
+#define MCP49X2_CHAN(chan, bits) {			\
+	.type = IIO_VOLTAGE,				\
+	.output = 1,					\
+	.indexed = 1,					\
+	.channel = chan,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.scan_type = {					\
+		.sign = 'u',				\
+		.realbits = (bits),			\
+		.storagebits = 16,			\
+		.shift = 12 - (bits),			\
+	},						\
+}
+
+static int mcp49x2_spi_write(struct mcp49x2_state *state, u8 addr, u32 val)
+{
+	state->mosi[1] = val & 0xff;
+	state->mosi[0] = (addr == 0) ? 0x00 : 0x80;
+	state->mosi[0] |= 0x30 | ((val >> 8) & 0x0f);
+
+	return spi_write(state->spi, state->mosi, 2);
+}
+
+static int mcp49x2_read_raw(struct iio_dev *indio_dev,
+		struct iio_chan_spec const *chan,
+		int *val,
+		int *val2,
+		long mask)
+{
+	struct mcp49x2_state *state = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*val = state->value[chan->channel];
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = state->vref_mv;
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp49x2_write_raw(struct iio_dev *indio_dev,
+		struct iio_chan_spec const *chan,
+		int val,
+		int val2,
+		long mask)
+{
+	struct mcp49x2_state *state = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (val > GENMASK(chan->scan_type.realbits-1, 0))
+			return -EINVAL;
+		val <<= chan->scan_type.shift;
+		state->value[chan->channel] = val;
+		return mcp49x2_spi_write(state, chan->channel, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_chan_spec mcp49x2_channels[3][MCP49X2_NUM_CHANNELS] = {
+	[ID_MCP4902] = { MCP49X2_CHAN(0, 8),	MCP49X2_CHAN(1, 8) },
+	[ID_MCP4912] = { MCP49X2_CHAN(0, 10),	MCP49X2_CHAN(1, 10) },
+	[ID_MCP4922] = { MCP49X2_CHAN(0, 12),	MCP49X2_CHAN(1, 12) },
+};
+
+static const struct iio_info mcp49x2_info = {
+	.read_raw = &mcp49x2_read_raw,
+	.write_raw = &mcp49x2_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int mcp49x2_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct mcp49x2_state *state;
+	const struct spi_device_id *id;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
+	if (indio_dev == NULL)
+		return -ENOMEM;
+
+	state = iio_priv(indio_dev);
+	state->spi = spi;
+	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
+	if (IS_ERR(state->vref_reg)) {
+		dev_err(&spi->dev, "Vref regulator not specified\n");
+		return PTR_ERR(state->vref_reg);
+	}
+
+	ret = regulator_enable(state->vref_reg);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
+				ret);
+		return ret;
+	}
+
+	ret = regulator_get_voltage(state->vref_reg);
+	if (ret < 0) {
+		dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
+				ret);
+		goto error_disable_reg;
+	}
+	state->vref_mv = ret / 1000;
+
+	spi_set_drvdata(spi, indio_dev);
+	id = spi_get_device_id(spi);
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &mcp49x2_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = mcp49x2_channels[id->driver_data];
+	indio_dev->num_channels = MCP49X2_NUM_CHANNELS;
+	indio_dev->name = id->name;
+
+	ret = devm_iio_device_register(&spi->dev, indio_dev);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to register iio device: %d\n",
+				ret);
+		goto error_disable_reg;
+	}
+
+	return 0;
+
+error_disable_reg:
+	regulator_disable(state->vref_reg);
+
+	return ret;
+}
+
+static int mcp49x2_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct mcp49x2_state *state;
+
+	state = iio_priv(indio_dev);
+	regulator_disable(state->vref_reg);
+
+	return 0;
+}
+
+static const struct spi_device_id mcp49x2_id[] = {
+	{"mcp4902", ID_MCP4902},
+	{"mcp4912", ID_MCP4912},
+	{"mcp4922", ID_MCP4922},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, mcp49x2_id);
+
+static struct spi_driver mcp49x2_driver = {
+	.driver = {
+		   .name = "mcp49x2",
+		   .owner = THIS_MODULE,
+		   },
+	.probe = mcp49x2_probe,
+	.remove = mcp49x2_remove,
+	.id_table = mcp49x2_id,
+};
+module_spi_driver(mcp49x2_driver);
+
+MODULE_AUTHOR("Michael Welling <mwelling@ieee.org>");
+MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v4] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[Jonathan Cameron]

On 06/06/14 13:48, Michael Welling wrote:
> This patch provides an iio device driver for the Microchip
> MCP49x2 series DACs.
>
> Signed-off-by: Michael Welling <mwelling@ieee.org>
> ---
Hi Michael,

One small point on patch formatting.  The convention is to always
include a description of the change that have occurred since the
previous version, here below the ---.  That just makes life
easy for reviewers as they can see everything they have asked
about has been addressed.

Anyhow, looking pretty good after the review cycles it's been through
this week. A couple of bits inline.  The big ones are:

1) Don't use wildcards in driver or prefix naming. So call this mcp4902
throughout (or one of the others if you prefer).  The arguements for using
or not using wildcards have gone back and forth for years. In IIO we almost
always say don't.
2) Don't use devm_iio_device_register - it causes a race condition.

>   drivers/iio/dac/Kconfig   |   10 +++
>   drivers/iio/dac/Makefile  |    1 +
>   drivers/iio/dac/mcp49x2.c |  213 +++++++++++++++++++++++++++++++++++++++++++++

Please don't use wild cards in naming.  It frequently goes wrong when
some clever person in marketing picks a new part that clashes with
the previous naming scheme.  The convention is to pick a part. Name
everything after that (including all prefixes in driver) then make
it clear in the kconfig text which parts are supported.

>   3 files changed, 224 insertions(+)
>   create mode 100644 drivers/iio/dac/mcp49x2.c
>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index f378ca8..032d678 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -163,4 +163,14 @@ config MCP4725
>   	  To compile this driver as a module, choose M here: the module
>   	  will be called mcp4725.
>
> +config MCP49X2
> +	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
> +	depends on SPI
> +	---help---
I was about to say that the standard format in this file is no --- round
the help, but then noticed the other MCP driver snuck in with this.
Lets go with the majority view and not have them. Anyone bored can fix
up the existing case of this.
> +	  Say yes here to build the driver for the Microchip MCP49x2
> +	  series DAC devices.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called mcp49x2.
> +
>   endmenu
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index bb84ad6..2fdfc2e 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -18,3 +18,4 @@ obj-$(CONFIG_AD5686) += ad5686.o
>   obj-$(CONFIG_AD7303) += ad7303.o
>   obj-$(CONFIG_MAX517) += max517.o
>   obj-$(CONFIG_MCP4725) += mcp4725.o
> +obj-$(CONFIG_MCP49X2) += mcp49x2.o
> diff --git a/drivers/iio/dac/mcp49x2.c b/drivers/iio/dac/mcp49x2.c
> new file mode 100644
> index 0000000..073af6e
> --- /dev/null
> +++ b/drivers/iio/dac/mcp49x2.c
> @@ -0,0 +1,213 @@
> +/*
> + * mcp49x2.c
> + *
> + * Driver for Microchip Digital to Analog Converters.
> + * Supports MCP4902, MCP4912, and MCP4922.
> + *
> + * Copyright (c) 2014 EMAC Inc.
> + *
> + * 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/module.h>
> +#include <linux/init.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/bitops.h>
> +
> +#define MCP49X2_NUM_CHANNELS	2
> +
> +enum mcp49x2_supported_device_ids {
> +	ID_MCP4902,
> +	ID_MCP4912,
> +	ID_MCP4922,
> +};
> +
> +struct mcp49x2_state {
> +	struct spi_device *spi;
> +	unsigned int value[MCP49X2_NUM_CHANNELS];
> +	unsigned int power_mode[MCP49X2_NUM_CHANNELS];
I can't see power_mode being used anywhere...

> +	unsigned int vref_mv;
> +	struct regulator *vref_reg;
> +	u8 mosi[2] ___cacheline_aligned;
> +};
> +
> +#define MCP49X2_CHAN(chan, bits) {			\
> +	.type = IIO_VOLTAGE,				\
> +	.output = 1,					\
> +	.indexed = 1,					\
> +	.channel = chan,				\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.scan_type = {					\
> +		.sign = 'u',				\
> +		.realbits = (bits),			\
> +		.storagebits = 16,			\
> +		.shift = 12 - (bits),			\
> +	},						\
> +}
> +
> +static int mcp49x2_spi_write(struct mcp49x2_state *state, u8 addr, u32 val)
> +{
> +	state->mosi[1] = val & 0xff;
> +	state->mosi[0] = (addr == 0) ? 0x00 : 0x80;
> +	state->mosi[0] |= 0x30 | ((val >> 8) & 0x0f);
> +
> +	return spi_write(state->spi, state->mosi, 2);
> +}
> +
> +static int mcp49x2_read_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int *val,
> +		int *val2,
> +		long mask)
> +{
> +	struct mcp49x2_state *state = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = state->value[chan->channel];
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = state->vref_mv;
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int mcp49x2_write_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int val,
> +		int val2,
> +		long mask)
> +{
> +	struct mcp49x2_state *state = iio_priv(indio_dev);
> +
If being really tight on checking for valid inputs, one should
verify that val2 == 0 and return -EINVAL otherwise.
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val > GENMASK(chan->scan_type.realbits-1, 0))
> +			return -EINVAL;
> +		val <<= chan->scan_type.shift;
> +		state->value[chan->channel] = val;
> +		return mcp49x2_spi_write(state, chan->channel, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_chan_spec mcp49x2_channels[3][MCP49X2_NUM_CHANNELS] = {
> +	[ID_MCP4902] = { MCP49X2_CHAN(0, 8),	MCP49X2_CHAN(1, 8) },
> +	[ID_MCP4912] = { MCP49X2_CHAN(0, 10),	MCP49X2_CHAN(1, 10) },
> +	[ID_MCP4922] = { MCP49X2_CHAN(0, 12),	MCP49X2_CHAN(1, 12) },
> +};
> +
> +static const struct iio_info mcp49x2_info = {
> +	.read_raw = &mcp49x2_read_raw,
> +	.write_raw = &mcp49x2_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int mcp49x2_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct mcp49x2_state *state;
> +	const struct spi_device_id *id;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	state = iio_priv(indio_dev);
> +	state->spi = spi;
> +	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
> +	if (IS_ERR(state->vref_reg)) {
> +		dev_err(&spi->dev, "Vref regulator not specified\n");
> +		return PTR_ERR(state->vref_reg);
> +	}
> +
> +	ret = regulator_enable(state->vref_reg);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_get_voltage(state->vref_reg);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
> +				ret);
> +		goto error_disable_reg;
> +	}
> +	state->vref_mv = ret / 1000;
> +
> +	spi_set_drvdata(spi, indio_dev);
> +	id = spi_get_device_id(spi);
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->info = &mcp49x2_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = mcp49x2_channels[id->driver_data];
> +	indio_dev->num_channels = MCP49X2_NUM_CHANNELS;
> +	indio_dev->name = id->name;
> +
You shouldn't be using devm_iio_device_register unless there
is nothing else to be done in the remove.

Unregistering the device is responsible for tearing down the userspace
interface. Until that happens, even if a remove is in progress, it
is possible for new reads to come in from userspace.

Jumping down to remove...
> +	ret = devm_iio_device_register(&spi->dev, indio_dev);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to register iio device: %d\n",
> +				ret);
> +		goto error_disable_reg;
> +	}
> +
> +	return 0;
> +
> +error_disable_reg:
> +	regulator_disable(state->vref_reg);
> +
> +	return ret;
> +}
> +
> +static int mcp49x2_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct mcp49x2_state *state;
> +
> +	state = iio_priv(indio_dev);
> +	regulator_disable(state->vref_reg);
Continuing from comment in probe...

Here you disable the regulator - at this precise point it would be possible
for a read to be going on, thus I presume resulting in some fairly
unpredictable behaviour!

Hence you need to use the unmanaged interfaces for iio_device_register
and iio_device_unregister with the unregister being the first thing
to happen in this remove function.

We had a reasonably involved debate on whether to introduce
devm_iio_device_register, precisely because this sort of issue can
happen, but in the end it was decided that it would be easy enough
to catch cases such as this one in review :)
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id mcp49x2_id[] = {
> +	{"mcp4902", ID_MCP4902},
> +	{"mcp4912", ID_MCP4912},
> +	{"mcp4922", ID_MCP4922},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, mcp49x2_id);
> +
> +static struct spi_driver mcp49x2_driver = {
> +	.driver = {
> +		   .name = "mcp49x2",
> +		   .owner = THIS_MODULE,
> +		   },
> +	.probe = mcp49x2_probe,
> +	.remove = mcp49x2_remove,
> +	.id_table = mcp49x2_id,
> +};
> +module_spi_driver(mcp49x2_driver);
> +
> +MODULE_AUTHOR("Michael Welling <mwelling@ieee.org>");
> +MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
> +MODULE_LICENSE("GPL v2");
>

[PATCH v5] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[Michael Welling]

This patch provides an iio device driver for the Microchip
MCP49x2 series DACs.

v5:
- Removes wildcard naming in filename and variables.
- Switches to using iio_device_register/unregister to avoid race condition.
- Removes --- around help in the Kconfig entry.
- Removes unused variable.
- Adds checking for val2 in write_raw function.

v4:
- Removes use of old IIO_ST macro.
- Moves mosi variable to the state struct.
- Removes default vref of 2.5v.

v3:
- Removes MCP49X2_RES_MASK and uses GENMASK directly.
- Switch from using address to channel in functions.

v2:
- Switched to using GENMASK instead of creating a mask directly.
- Switched from using address to channel.
- Removed unnecessary feilds from channel macro.
- Made SPI buffer cachline aligned.
- Removed some uncessary bounds checking.
- Report -EINVAL when writing a value beyond the range to DAC.
- Switched to devm_iio_device_alloc.
- Created a constant for the number of channels.
- Switched to devm_ioo_device_register.

Signed-off-by: Michael Welling <mwelling@ieee.org>
---
 drivers/iio/dac/Kconfig   |   10 +++
 drivers/iio/dac/Makefile  |    1 +
 drivers/iio/dac/mcp4922.c |  216 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 227 insertions(+)
 create mode 100644 drivers/iio/dac/mcp4922.c

diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index f378ca8..f278eff 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -163,4 +163,14 @@ config MCP4725
 	  To compile this driver as a module, choose M here: the module
 	  will be called mcp4725.
 
+config MCP4922
+	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
+	depends on SPI
+	help
+	  Say yes here to build the driver for the Microchip MCP4902
+	  MCP4912, and MCP4922 DAC devices.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mcp4922.
+
 endmenu
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index bb84ad6..1010764 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_AD5686) += ad5686.o
 obj-$(CONFIG_AD7303) += ad7303.o
 obj-$(CONFIG_MAX517) += max517.o
 obj-$(CONFIG_MCP4725) += mcp4725.o
+obj-$(CONFIG_MCP4922) += mcp4922.o
diff --git a/drivers/iio/dac/mcp4922.c b/drivers/iio/dac/mcp4922.c
new file mode 100644
index 0000000..92cf4ca
--- /dev/null
+++ b/drivers/iio/dac/mcp4922.c
@@ -0,0 +1,216 @@
+/*
+ * mcp4922.c
+ *
+ * Driver for Microchip Digital to Analog Converters.
+ * Supports MCP4902, MCP4912, and MCP4922.
+ *
+ * Copyright (c) 2014 EMAC Inc.
+ *
+ * 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/module.h>
+#include <linux/init.h>
+#include <linux/spi/spi.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/regulator/consumer.h>
+#include <linux/bitops.h>
+
+#define MCP4922_NUM_CHANNELS	2
+
+enum mcp4922_supported_device_ids {
+	ID_MCP4902,
+	ID_MCP4912,
+	ID_MCP4922,
+};
+
+struct mcp4922_state {
+	struct spi_device *spi;
+	unsigned int value[MCP4922_NUM_CHANNELS];
+	unsigned int vref_mv;
+	struct regulator *vref_reg;
+	u8 mosi[2] ____cacheline_aligned;
+};
+
+#define MCP4922_CHAN(chan, bits) {			\
+	.type = IIO_VOLTAGE,				\
+	.output = 1,					\
+	.indexed = 1,					\
+	.channel = chan,				\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.scan_type = {					\
+		.sign = 'u',				\
+		.realbits = (bits),			\
+		.storagebits = 16,			\
+		.shift = 12 - (bits),			\
+	},						\
+}
+
+static int mcp4922_spi_write(struct mcp4922_state *state, u8 addr, u32 val)
+{
+	state->mosi[1] = val & 0xff;
+	state->mosi[0] = (addr == 0) ? 0x00 : 0x80;
+	state->mosi[0] |= 0x30 | ((val >> 8) & 0x0f);
+
+	return spi_write(state->spi, state->mosi, 2);
+}
+
+static int mcp4922_read_raw(struct iio_dev *indio_dev,
+		struct iio_chan_spec const *chan,
+		int *val,
+		int *val2,
+		long mask)
+{
+	struct mcp4922_state *state = iio_priv(indio_dev);
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*val = state->value[chan->channel];
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = state->vref_mv;
+		*val2 = chan->scan_type.realbits;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	default:
+		return -EINVAL;
+	}
+}
+
+static int mcp4922_write_raw(struct iio_dev *indio_dev,
+		struct iio_chan_spec const *chan,
+		int val,
+		int val2,
+		long mask)
+{
+	struct mcp4922_state *state = iio_priv(indio_dev);
+
+	if (val2 != 0)
+		return -EINVAL;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (val > GENMASK(chan->scan_type.realbits-1, 0))
+			return -EINVAL;
+		val <<= chan->scan_type.shift;
+		state->value[chan->channel] = val;
+		return mcp4922_spi_write(state, chan->channel, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static const struct iio_chan_spec mcp4922_channels[3][MCP4922_NUM_CHANNELS] = {
+	[ID_MCP4902] = { MCP4922_CHAN(0, 8),	MCP4922_CHAN(1, 8) },
+	[ID_MCP4912] = { MCP4922_CHAN(0, 10),	MCP4922_CHAN(1, 10) },
+	[ID_MCP4922] = { MCP4922_CHAN(0, 12),	MCP4922_CHAN(1, 12) },
+};
+
+static const struct iio_info mcp4922_info = {
+	.read_raw = &mcp4922_read_raw,
+	.write_raw = &mcp4922_write_raw,
+	.driver_module = THIS_MODULE,
+};
+
+static int mcp4922_probe(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev;
+	struct mcp4922_state *state;
+	const struct spi_device_id *id;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
+	if (indio_dev == NULL)
+		return -ENOMEM;
+
+	state = iio_priv(indio_dev);
+	state->spi = spi;
+	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
+	if (IS_ERR(state->vref_reg)) {
+		dev_err(&spi->dev, "Vref regulator not specified\n");
+		return PTR_ERR(state->vref_reg);
+	}
+
+	ret = regulator_enable(state->vref_reg);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
+				ret);
+		return ret;
+	}
+
+	ret = regulator_get_voltage(state->vref_reg);
+	if (ret < 0) {
+		dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
+				ret);
+		goto error_disable_reg;
+	}
+	state->vref_mv = ret / 1000;
+
+	spi_set_drvdata(spi, indio_dev);
+	id = spi_get_device_id(spi);
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->info = &mcp4922_info;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = mcp4922_channels[id->driver_data];
+	indio_dev->num_channels = MCP4922_NUM_CHANNELS;
+	indio_dev->name = id->name;
+
+	ret = iio_device_register(indio_dev);
+	if (ret) {
+		dev_err(&spi->dev, "Failed to register iio device: %d\n",
+				ret);
+		goto error_disable_reg;
+	}
+
+	return 0;
+
+error_disable_reg:
+	regulator_disable(state->vref_reg);
+
+	return ret;
+}
+
+static int mcp4922_remove(struct spi_device *spi)
+{
+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+	struct mcp4922_state *state;
+
+	iio_device_unregister(indio_dev);
+	state = iio_priv(indio_dev);
+	regulator_disable(state->vref_reg);
+
+	return 0;
+}
+
+static const struct spi_device_id mcp4922_id[] = {
+	{"mcp4902", ID_MCP4902},
+	{"mcp4912", ID_MCP4912},
+	{"mcp4922", ID_MCP4922},
+	{}
+};
+MODULE_DEVICE_TABLE(spi, mcp4922_id);
+
+static struct spi_driver mcp4922_driver = {
+	.driver = {
+		   .name = "mcp4922",
+		   .owner = THIS_MODULE,
+		   },
+	.probe = mcp4922_probe,
+	.remove = mcp4922_remove,
+	.id_table = mcp4922_id,
+};
+module_spi_driver(mcp4922_driver);
+
+MODULE_AUTHOR("Michael Welling <mwelling@ieee.org>");
+MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

Re: [PATCH v5] iio: dac: mcp4902/mcp4912/mcp4922 dac driver

[Jonathan Cameron]

On 15/06/14 22:17, Michael Welling wrote:
> This patch provides an iio device driver for the Microchip
> MCP49x2 series DACs.
>
Normally this lot goes below the --- inorder that it doesn't then lead
to really long commit messages in git.  Anyhow, I just dropped this stuff
during the commit.  It's enough to have it in the list archives.
> v5:
> - Removes wildcard naming in filename and variables.
> - Switches to using iio_device_register/unregister to avoid race condition.
> - Removes --- around help in the Kconfig entry.
> - Removes unused variable.
> - Adds checking for val2 in write_raw function.
>
> v4:
> - Removes use of old IIO_ST macro.
> - Moves mosi variable to the state struct.
> - Removes default vref of 2.5v.
>
> v3:
> - Removes MCP49X2_RES_MASK and uses GENMASK directly.
> - Switch from using address to channel in functions.
>
> v2:
> - Switched to using GENMASK instead of creating a mask directly.
> - Switched from using address to channel.
> - Removed unnecessary feilds from channel macro.
> - Made SPI buffer cachline aligned.
> - Removed some uncessary bounds checking.
> - Report -EINVAL when writing a value beyond the range to DAC.
> - Switched to devm_iio_device_alloc.
> - Created a constant for the number of channels.
> - Switched to devm_ioo_device_register.
>
> Signed-off-by: Michael Welling <mwelling@ieee.org>
Applied to the togreg branch of iio.git.  Note this is initially pushed
out as testing to allow the autobuilders to do there work before I commit
to not rebasing the tree!

Thanks for your hard work on this.  It's a nice little driver!

Jonathan
> ---
>   drivers/iio/dac/Kconfig   |   10 +++
>   drivers/iio/dac/Makefile  |    1 +
>   drivers/iio/dac/mcp4922.c |  216 +++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 227 insertions(+)
>   create mode 100644 drivers/iio/dac/mcp4922.c
>
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index f378ca8..f278eff 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -163,4 +163,14 @@ config MCP4725
>   	  To compile this driver as a module, choose M here: the module
>   	  will be called mcp4725.
>
> +config MCP4922
> +	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build the driver for the Microchip MCP4902
> +	  MCP4912, and MCP4922 DAC devices.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called mcp4922.
> +
>   endmenu
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index bb84ad6..1010764 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -18,3 +18,4 @@ obj-$(CONFIG_AD5686) += ad5686.o
>   obj-$(CONFIG_AD7303) += ad7303.o
>   obj-$(CONFIG_MAX517) += max517.o
>   obj-$(CONFIG_MCP4725) += mcp4725.o
> +obj-$(CONFIG_MCP4922) += mcp4922.o
> diff --git a/drivers/iio/dac/mcp4922.c b/drivers/iio/dac/mcp4922.c
> new file mode 100644
> index 0000000..92cf4ca
> --- /dev/null
> +++ b/drivers/iio/dac/mcp4922.c
> @@ -0,0 +1,216 @@
> +/*
> + * mcp4922.c
> + *
> + * Driver for Microchip Digital to Analog Converters.
> + * Supports MCP4902, MCP4912, and MCP4922.
> + *
> + * Copyright (c) 2014 EMAC Inc.
> + *
> + * 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/module.h>
> +#include <linux/init.h>
> +#include <linux/spi/spi.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/bitops.h>
> +
> +#define MCP4922_NUM_CHANNELS	2
> +
> +enum mcp4922_supported_device_ids {
> +	ID_MCP4902,
> +	ID_MCP4912,
> +	ID_MCP4922,
> +};
> +
> +struct mcp4922_state {
> +	struct spi_device *spi;
> +	unsigned int value[MCP4922_NUM_CHANNELS];
> +	unsigned int vref_mv;
> +	struct regulator *vref_reg;
> +	u8 mosi[2] ____cacheline_aligned;
> +};
> +
> +#define MCP4922_CHAN(chan, bits) {			\
> +	.type = IIO_VOLTAGE,				\
> +	.output = 1,					\
> +	.indexed = 1,					\
> +	.channel = chan,				\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),	\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.scan_type = {					\
> +		.sign = 'u',				\
> +		.realbits = (bits),			\
> +		.storagebits = 16,			\
> +		.shift = 12 - (bits),			\
> +	},						\
> +}
> +
> +static int mcp4922_spi_write(struct mcp4922_state *state, u8 addr, u32 val)
> +{
> +	state->mosi[1] = val & 0xff;
> +	state->mosi[0] = (addr == 0) ? 0x00 : 0x80;
> +	state->mosi[0] |= 0x30 | ((val >> 8) & 0x0f);
> +
> +	return spi_write(state->spi, state->mosi, 2);
> +}
> +
> +static int mcp4922_read_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int *val,
> +		int *val2,
> +		long mask)
> +{
> +	struct mcp4922_state *state = iio_priv(indio_dev);
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = state->value[chan->channel];
> +		return IIO_VAL_INT;
> +	case IIO_CHAN_INFO_SCALE:
> +		*val = state->vref_mv;
> +		*val2 = chan->scan_type.realbits;
> +		return IIO_VAL_FRACTIONAL_LOG2;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int mcp4922_write_raw(struct iio_dev *indio_dev,
> +		struct iio_chan_spec const *chan,
> +		int val,
> +		int val2,
> +		long mask)
> +{
> +	struct mcp4922_state *state = iio_priv(indio_dev);
> +
> +	if (val2 != 0)
> +		return -EINVAL;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (val > GENMASK(chan->scan_type.realbits-1, 0))
> +			return -EINVAL;
> +		val <<= chan->scan_type.shift;
> +		state->value[chan->channel] = val;
> +		return mcp4922_spi_write(state, chan->channel, val);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static const struct iio_chan_spec mcp4922_channels[3][MCP4922_NUM_CHANNELS] = {
> +	[ID_MCP4902] = { MCP4922_CHAN(0, 8),	MCP4922_CHAN(1, 8) },
> +	[ID_MCP4912] = { MCP4922_CHAN(0, 10),	MCP4922_CHAN(1, 10) },
> +	[ID_MCP4922] = { MCP4922_CHAN(0, 12),	MCP4922_CHAN(1, 12) },
> +};
> +
> +static const struct iio_info mcp4922_info = {
> +	.read_raw = &mcp4922_read_raw,
> +	.write_raw = &mcp4922_write_raw,
> +	.driver_module = THIS_MODULE,
> +};
> +
> +static int mcp4922_probe(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev;
> +	struct mcp4922_state *state;
> +	const struct spi_device_id *id;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*state));
> +	if (indio_dev == NULL)
> +		return -ENOMEM;
> +
> +	state = iio_priv(indio_dev);
> +	state->spi = spi;
> +	state->vref_reg = devm_regulator_get(&spi->dev, "vref");
> +	if (IS_ERR(state->vref_reg)) {
> +		dev_err(&spi->dev, "Vref regulator not specified\n");
> +		return PTR_ERR(state->vref_reg);
> +	}
> +
> +	ret = regulator_enable(state->vref_reg);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to enable vref regulator: %d\n",
> +				ret);
> +		return ret;
> +	}
> +
> +	ret = regulator_get_voltage(state->vref_reg);
> +	if (ret < 0) {
> +		dev_err(&spi->dev, "Failed to read vref regulator: %d\n",
> +				ret);
> +		goto error_disable_reg;
> +	}
> +	state->vref_mv = ret / 1000;
> +
> +	spi_set_drvdata(spi, indio_dev);
> +	id = spi_get_device_id(spi);
> +	indio_dev->dev.parent = &spi->dev;
> +	indio_dev->info = &mcp4922_info;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = mcp4922_channels[id->driver_data];
> +	indio_dev->num_channels = MCP4922_NUM_CHANNELS;
> +	indio_dev->name = id->name;
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		dev_err(&spi->dev, "Failed to register iio device: %d\n",
> +				ret);
> +		goto error_disable_reg;
> +	}
> +
> +	return 0;
> +
> +error_disable_reg:
> +	regulator_disable(state->vref_reg);
> +
> +	return ret;
> +}
> +
> +static int mcp4922_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct mcp4922_state *state;
> +
> +	iio_device_unregister(indio_dev);
> +	state = iio_priv(indio_dev);
> +	regulator_disable(state->vref_reg);
> +
> +	return 0;
> +}
> +
> +static const struct spi_device_id mcp4922_id[] = {
> +	{"mcp4902", ID_MCP4902},
> +	{"mcp4912", ID_MCP4912},
> +	{"mcp4922", ID_MCP4922},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(spi, mcp4922_id);
> +
> +static struct spi_driver mcp4922_driver = {
> +	.driver = {
> +		   .name = "mcp4922",
> +		   .owner = THIS_MODULE,
> +		   },
> +	.probe = mcp4922_probe,
> +	.remove = mcp4922_remove,
> +	.id_table = mcp4922_id,
> +};
> +module_spi_driver(mcp4922_driver);
> +
> +MODULE_AUTHOR("Michael Welling <mwelling@ieee.org>");
> +MODULE_DESCRIPTION("Microchip MCP4902, MCP4912, MCP4922 DAC");
> +MODULE_LICENSE("GPL v2");
>