diff mbox

[RFC,1/2] iio: dac: add TI DAC5755 family support

Message ID 20180322102336.32268-1-sean.nyekjaer@prevas.dk (mailing list archive)
State New, archived
Headers show

Commit Message

Sean Nyekjær March 22, 2018, 10:23 a.m. UTC 
This patch adds support for the Texas Intruments DAC5755 Family.

Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
---
 drivers/iio/dac/Kconfig      |  10 ++
 drivers/iio/dac/Makefile     |   1 +
 drivers/iio/dac/ti-dac5571.c | 395 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 406 insertions(+)
 create mode 100644 drivers/iio/dac/ti-dac5571.c

Comments

Peter Meerwald-Stadler March 22, 2018, 11:25 a.m. UTC | #1 
> This patch adds support for the Texas Intruments DAC5755 Family.

comments below
 
> Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
> ---
>  drivers/iio/dac/Kconfig      |  10 ++
>  drivers/iio/dac/Makefile     |   1 +
>  drivers/iio/dac/ti-dac5571.c | 395 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 406 insertions(+)
>  create mode 100644 drivers/iio/dac/ti-dac5571.c
> 
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 965d5c0d2468..c78c02f455b9 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -320,6 +320,16 @@ config TI_DAC082S085
>  
>  	  If compiled as a module, it will be called ti-dac082s085.
>  
> +config TI_DAC5571
> +	tristate "Texas Instruments 8/10/12/16-bit 1/2/4-channel DAC driver"
> +	depends on I2C
> +	help
> +	  Driver for the Texas Instruments
> +	  DAC5571, DAC6571, DAC7571, DAC5574, DAC6574, DAC7574, DAC5573,
> +	  DAC6573, DAC7573.
> +
> +	  If compiled as a module, it will be called ti-dac5571.
> +
>  config VF610_DAC
>  	tristate "Vybrid vf610 DAC driver"
>  	depends on OF
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 81e710ed7491..1ab358d522ee 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -35,4 +35,5 @@ obj-$(CONFIG_MCP4922) += mcp4922.o
>  obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
>  obj-$(CONFIG_STM32_DAC) += stm32-dac.o
>  obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o
> +obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o
>  obj-$(CONFIG_VF610_DAC) += vf610_dac.o
> diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c
> new file mode 100644
> index 000000000000..cf4ba151ed7e
> --- /dev/null
> +++ b/drivers/iio/dac/ti-dac5571.c
> @@ -0,0 +1,395 @@
> +/*
> + * ti-dac5571.c - Texas Instruments 8/10/12-bit 1/4-channel DAC driver
> + *
> + * Copyright (C) 2018 Prevas A/S
> + *
> + * http://www.ti.com/lit/ds/symlink/dac5571.pdf
> + * http://www.ti.com/lit/ds/symlink/dac6571.pdf
> + * http://www.ti.com/lit/ds/symlink/dac7571.pdf
> + * http://www.ti.com/lit/ds/symlink/dac5574.pdf
> + * http://www.ti.com/lit/ds/symlink/dac6574.pdf
> + * http://www.ti.com/lit/ds/symlink/dac7574.pdf
> + * http://www.ti.com/lit/ds/symlink/dac5573.pdf
> + * http://www.ti.com/lit/ds/symlink/dac6573.pdf
> + * http://www.ti.com/lit/ds/symlink/dac7573.pdf
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License (version 2) as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/i2c.h>
> +#include <linux/of_device.h>
> +#include <linux/of.h>
> +
> +enum chip_id {
> +	single_8bit, single_10bit, single_12bit,
> +	quad_8bit, quad_10bit, quad_12bit
> +};
> +
> +struct dac5571_spec {
> +	u8 num_channels;
> +	u8 resolution;
> +};
> +
> +static const struct dac5571_spec dac5571_spec[] = {
> +	[single_8bit]  = {.num_channels = 1, .resolution =  8},
> +	[single_10bit] = {.num_channels = 1, .resolution = 10},
> +	[single_12bit] = {.num_channels = 1, .resolution = 12},
> +	[quad_8bit]    = {.num_channels = 4, .resolution =  8},
> +	[quad_10bit]   = {.num_channels = 4, .resolution = 10},
> +	[quad_12bit]   = {.num_channels = 4, .resolution = 12},
> +};
> +
> +struct dac5571_data {
> +	struct i2c_client *client;
> +	int id;
> +	struct mutex lock;
> +	struct regulator *vref;
> +	u16 val[4];
> +	bool powerdown;
> +	u8 powerdown_mode;
> +	u8 resolution;
> +	u8 channels;
> +	u8 buf[3] ____cacheline_aligned;
> +};
> +
> +#define POWERDOWN(mode)		((mode) + 1)

DAC5571_ prefix please

> +
> +static int dac5571_cmd(struct dac5571_data *data, int channel,
> +		       u8 pwrdwn, u16 val)
> +{
> +	int ret;
> +	u8 shift;

no need for shift to be u8, use unsigned int or int

> +
> +	switch (data->channels) {
> +	case 1:
> +		shift = 12 - data->resolution;
> +		data->buf[0] = (val << shift);

parenthesis not needed here

> +		data->buf[1] = pwrdwn << 4 | (val >> (8 - shift));

I suggest parenthesis around (pwrdwn << 4)

> +		ret = i2c_master_send(data->client, data->buf, 2);
> +		break;
> +	case 4:
> +		shift = 16 - data->resolution;
> +		data->buf[0] = val << shift;
> +		data->buf[1] = val >> (8 - shift) | pwrdwn << 6;
> +		data->buf[2] = channel << 1 | 1 << 4;

I suggest parenthesis for readability
(val >> (8 - shift))
(channel << 1) | (1 << 4)

> +		if (pwrdwn)
> +			data->buf[2] |= 1;
> +		ret = i2c_master_send(data->client, data->buf, 3);
> +		break;
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	};
> +
> +	return ret;
> +}
> +
> +static const char *const dac5571_powerdown_modes[] = {
> +	"1kohm_to_gnd", "100kohm_to_gnd", "three_state",
> +};
> +
> +static int dac5571_get_powerdown_mode(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan)
> +{
> +	struct dac5571_data *data = iio_priv(indio_dev);
> +
> +	return data->powerdown_mode;
> +}
> +
> +static int dac5571_set_powerdown_mode(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      unsigned int mode)

mode could/should be u8 as it is passed to the device, or an enum even

> +{
> +	struct dac5571_data *data = iio_priv(indio_dev);
> +	int ret = 0;
> +
> +	if (data->powerdown_mode == mode)
> +		return 0;
> +
> +	mutex_lock(&data->lock);
> +	if (data->powerdown) {
> +		ret = dac5571_cmd(data, chan->channel, POWERDOWN(mode), 0);
> +		if (ret)

i2c_master_send() and hence dac5571_cmd() return the number of bytes 
written!? should be 
if (ret <= 0)
?

> +			goto out;
> +	}
> +	data->powerdown_mode = mode;
> +
> + out:
> +	mutex_unlock(&data->lock);
> +	return ret;
> +}
> +
> +static const struct iio_enum dac5571_powerdown_mode = {
> +	.items = dac5571_powerdown_modes,
> +	.num_items = ARRAY_SIZE(dac5571_powerdown_modes),
> +	.get = dac5571_get_powerdown_mode,
> +	.set = dac5571_set_powerdown_mode,
> +};
> +
> +static ssize_t dac5571_read_powerdown(struct iio_dev *indio_dev,
> +				      uintptr_t private,
> +				      const struct iio_chan_spec *chan,
> +				      char *buf)
> +{
> +	struct dac5571_data *data = iio_priv(indio_dev);
> +
> +	return sprintf(buf, "%d\n", data->powerdown);
> +}
> +
> +static ssize_t dac5571_write_powerdown(struct iio_dev *indio_dev,
> +				       uintptr_t private,
> +				       const struct iio_chan_spec *chan,
> +				       const char *buf, size_t len)
> +{
> +	struct dac5571_data *data = iio_priv(indio_dev);
> +	bool powerdown;
> +	int ret;
> +
> +	ret = strtobool(buf, &powerdown);
> +	if (ret)
> +		return ret;
> +
> +	if (data->powerdown == powerdown)
> +		return len;
> +
> +	mutex_lock(&data->lock);
> +	if (powerdown)
> +		ret =
> +		    dac5571_cmd(data, chan->channel,
> +				POWERDOWN(data->powerdown_mode), 0);
> +	else
> +		ret = dac5571_cmd(data, chan->channel, 0, data->val[0]);

ret value of dac5571_cmd() is number of bytes written?

> +	if (!ret)
> +		data->powerdown = powerdown;
> +	mutex_unlock(&data->lock);
> +
> +	return ret ? ret : len;
> +}
> +
> +
> +static const struct iio_chan_spec_ext_info dac5571_ext_info[] = {
> +	{
> +		.name	   = "powerdown",
> +		.read	   = dac5571_read_powerdown,
> +		.write	   = dac5571_write_powerdown,
> +		.shared	   = IIO_SHARED_BY_TYPE,
> +	},
> +	IIO_ENUM("powerdown_mode", IIO_SHARED_BY_TYPE, &dac5571_powerdown_mode),
> +	IIO_ENUM_AVAILABLE("powerdown_mode", &dac5571_powerdown_mode),
> +	{},
> +};
> +
> +#define dac5571_CHANNEL(chan) {					\
> +	.type = IIO_VOLTAGE,					\
> +	.channel = (chan),					\
> +	.address = (chan),					\
> +	.indexed = true,					\
> +	.output = true,						\
> +	.datasheet_name = (const char[]){ 'A' + (chan), 0 },	\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.ext_info = dac5571_ext_info,				\
> +}
> +
> +static const struct iio_chan_spec dac5571_channels[] = {
> +	dac5571_CHANNEL(0),
> +	dac5571_CHANNEL(1),
> +	dac5571_CHANNEL(2),
> +	dac5571_CHANNEL(3),
> +};
> +
> +static int dac5571_read_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int *val, int *val2, long mask)
> +{
> +	struct dac5571_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = data->val[chan->channel];
> +		ret = IIO_VAL_INT;

return directly

> +		break;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regulator_get_voltage(data->vref);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret / 1000;
> +		*val2 = data->resolution;
> +		ret = IIO_VAL_FRACTIONAL_LOG2;

return directly

> +		break;
> +
> +	default:
> +		ret = -EINVAL;

return directly

> +	}
> +
> +	return ret;
> +}
> +
> +static int dac5571_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan,
> +			     int val, int val2, long mask)
> +{
> +	struct dac5571_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (data->val[chan->channel] == val)
> +			return 0;
> +
> +		if (val >= (1 << data->resolution) || val < 0)
> +			return -EINVAL;
> +
> +		if (data->powerdown)
> +			return -EBUSY;
> +
> +		mutex_lock(&data->lock);
> +		ret = dac5571_cmd(data, chan->channel, 0, val);

ret value of dac5571_cmd()...

> +		if (!ret)
> +			data->val[chan->channel] = val;
> +		mutex_unlock(&data->lock);
> +		break;

return directly

> +
> +	default:
> +		ret = -EINVAL;

return directly

> +	}
> +
> +	return ret;
> +}
> +
> +static int dac5571_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				     struct iio_chan_spec const *chan,
> +				     long mask)
> +{
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info dac5571_info = {
> +	.read_raw	   = dac5571_read_raw,
> +	.write_raw	   = dac5571_write_raw,
> +	.write_raw_get_fmt = dac5571_write_raw_get_fmt,
> +};
> +
> +static int dac5571_probe(struct i2c_client *client,
> +			 const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	const struct dac5571_spec *spec;
> +	struct dac5571_data *data;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +	data = iio_priv(indio_dev);
> +	i2c_set_clientdata(client, indio_dev);
> +	data->client = client;
> +	if (client->dev.of_node)
> +		data->id = (enum chip_id)of_device_get_match_data(&client->dev);
> +	else
> +		data->id = id->driver_data;
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->dev.of_node = client->dev.of_node;
> +	indio_dev->info = &dac5571_info;
> +	indio_dev->name = id->name;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = dac5571_channels;
> +
> +	spec = &dac5571_spec[id->driver_data];
> +	indio_dev->num_channels = spec->num_channels;
> +	data->resolution = spec->resolution;
> +	data->channels = spec->num_channels;
> +
> +	data->vref = devm_regulator_get(dev, "vref");
> +	if (IS_ERR(data->vref))
> +		return PTR_ERR(data->vref);
> +
> +	ret = regulator_enable(data->vref);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_init(&data->lock);
> +
> +	ret = dac5571_cmd(data, 0, 0, 0);
> +	if (ret) {

ret value is number of bytes written?

> +		dev_err(dev, "failed to initialize outputs to 0\n");
> +		goto err;
> +	}
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +
> + err:
> +	mutex_destroy(&data->lock);

no need to destroy mutex (at least most drivers don't)

> +	regulator_disable(data->vref);
> +	return ret;
> +}
> +
> +static int dac5571_remove(struct i2c_client *i2c)
> +{
> +	struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
> +	struct dac5571_data *data = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	mutex_destroy(&data->lock);

no need to destroy mutex

> +	regulator_disable(data->vref);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id dac5571_of_id[] = {
> +	{.compatible = "ti,dac5571"},
> +	{.compatible = "ti,dac6571"},
> +	{.compatible = "ti,dac7571"},
> +	{.compatible = "ti,dac5574"},
> +	{.compatible = "ti,dac6574"},
> +	{.compatible = "ti,dac7574"},
> +	{.compatible = "ti,dac5573"},
> +	{.compatible = "ti,dac6573"},
> +	{.compatible = "ti,dac7573"},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, dac5571_of_id);
> +#endif
> +
> +static const struct i2c_device_id dac5571_id[] = {
> +	{"dac5571", single_8bit},
> +	{"dac6571", single_10bit},
> +	{"dac7571", single_12bit},
> +	{"dac5574", quad_8bit},
> +	{"dac6574", quad_10bit},
> +	{"dac7574", quad_12bit},
> +	{"dac5573", quad_8bit},
> +	{"dac6573", quad_10bit},
> +	{"dac7573", quad_12bit},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(i2c, dac5571_id);
> +
> +static struct i2c_driver dac5571_driver = {
> +	.driver = {
> +		   .name = "ti-dac5571",
> +	},
> +	.probe	  = dac5571_probe,
> +	.remove   = dac5571_remove,
> +	.id_table = dac5571_id,
> +};
> +module_i2c_driver(dac5571_driver);
> +
> +MODULE_AUTHOR("Sean Nyekjaer <sean.nyekjaer@prevas.dk>");
> +MODULE_DESCRIPTION("Texas Instruments 8/10/12-bit 1/4-channel DAC driver");
> +MODULE_LICENSE("GPL v2");
>
Jonathan Cameron March 23, 2018, 12:27 p.m. UTC | #2 
On Thu, 22 Mar 2018 12:25:35 +0100
Peter Meerwald-Stadler <pmeerw@pmeerw.net> wrote:

> > This patch adds support for the Texas Intruments DAC5755 Family.  
> 
> comments below
If posting an RFC rather than a normal patch, I really want to see an
explanation of what you would like comments on.

A few minor comments from me.

Thanks,

Jonathan

>  
> > Signed-off-by: Sean Nyekjaer <sean.nyekjaer@prevas.dk>
> > ---
> >  drivers/iio/dac/Kconfig      |  10 ++
> >  drivers/iio/dac/Makefile     |   1 +
> >  drivers/iio/dac/ti-dac5571.c | 395 +++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 406 insertions(+)
> >  create mode 100644 drivers/iio/dac/ti-dac5571.c
> > 
> > diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> > index 965d5c0d2468..c78c02f455b9 100644
> > --- a/drivers/iio/dac/Kconfig
> > +++ b/drivers/iio/dac/Kconfig
> > @@ -320,6 +320,16 @@ config TI_DAC082S085
> >  
> >  	  If compiled as a module, it will be called ti-dac082s085.
> >  
> > +config TI_DAC5571
> > +	tristate "Texas Instruments 8/10/12/16-bit 1/2/4-channel DAC driver"
> > +	depends on I2C
> > +	help
> > +	  Driver for the Texas Instruments
> > +	  DAC5571, DAC6571, DAC7571, DAC5574, DAC6574, DAC7574, DAC5573,
> > +	  DAC6573, DAC7573.
> > +
> > +	  If compiled as a module, it will be called ti-dac5571.
> > +
> >  config VF610_DAC
> >  	tristate "Vybrid vf610 DAC driver"
> >  	depends on OF
> > diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> > index 81e710ed7491..1ab358d522ee 100644
> > --- a/drivers/iio/dac/Makefile
> > +++ b/drivers/iio/dac/Makefile
> > @@ -35,4 +35,5 @@ obj-$(CONFIG_MCP4922) += mcp4922.o
> >  obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
> >  obj-$(CONFIG_STM32_DAC) += stm32-dac.o
> >  obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o
> > +obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o
> >  obj-$(CONFIG_VF610_DAC) += vf610_dac.o
> > diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c
> > new file mode 100644
> > index 000000000000..cf4ba151ed7e
> > --- /dev/null
> > +++ b/drivers/iio/dac/ti-dac5571.c
> > @@ -0,0 +1,395 @@
> > +/*
> > + * ti-dac5571.c - Texas Instruments 8/10/12-bit 1/4-channel DAC driver
> > + *
> > + * Copyright (C) 2018 Prevas A/S
> > + *
> > + * http://www.ti.com/lit/ds/symlink/dac5571.pdf
> > + * http://www.ti.com/lit/ds/symlink/dac6571.pdf
> > + * http://www.ti.com/lit/ds/symlink/dac7571.pdf
> > + * http://www.ti.com/lit/ds/symlink/dac5574.pdf
> > + * http://www.ti.com/lit/ds/symlink/dac6574.pdf
> > + * http://www.ti.com/lit/ds/symlink/dac7574.pdf
> > + * http://www.ti.com/lit/ds/symlink/dac5573.pdf
> > + * http://www.ti.com/lit/ds/symlink/dac6573.pdf
> > + * http://www.ti.com/lit/ds/symlink/dac7573.pdf
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License (version 2) as
> > + * published by the Free Software Foundation.
Consider SPDX. It's up to you whether you want to use it or not.

> > + */
> > +
> > +#include <linux/iio/iio.h>
> > +#include <linux/module.h>
> > +#include <linux/regulator/consumer.h>
Unless there is a reason to do otherwise, convention is alphabetical order for
includes.

> > +#include <linux/i2c.h>
> > +#include <linux/of_device.h>
> > +#include <linux/of.h>
> > +
> > +enum chip_id {
> > +	single_8bit, single_10bit, single_12bit,
> > +	quad_8bit, quad_10bit, quad_12bit
> > +};
> > +
> > +struct dac5571_spec {
> > +	u8 num_channels;
> > +	u8 resolution;
> > +};
> > +
> > +static const struct dac5571_spec dac5571_spec[] = {
> > +	[single_8bit]  = {.num_channels = 1, .resolution =  8},
> > +	[single_10bit] = {.num_channels = 1, .resolution = 10},
> > +	[single_12bit] = {.num_channels = 1, .resolution = 12},
> > +	[quad_8bit]    = {.num_channels = 4, .resolution =  8},
> > +	[quad_10bit]   = {.num_channels = 4, .resolution = 10},
> > +	[quad_12bit]   = {.num_channels = 4, .resolution = 12},
> > +};
> > +
> > +struct dac5571_data {
> > +	struct i2c_client *client;
> > +	int id;
> > +	struct mutex lock;
As ever, the lock needs documentation.  What is it protecting...

> > +	struct regulator *vref;
> > +	u16 val[4];
> > +	bool powerdown;
> > +	u8 powerdown_mode;
> > +	u8 resolution;
> > +	u8 channels;
Rather than copying over the various elements, perhaps just
have a dac5571_spec structure pointer in here?

> > +	u8 buf[3] ____cacheline_aligned;
> > +};
> > +
> > +#define POWERDOWN(mode)		((mode) + 1)  
> 
> DAC5571_ prefix please
> 
> > +
> > +static int dac5571_cmd(struct dac5571_data *data, int channel,
> > +		       u8 pwrdwn, u16 val)
This seems to be combining several different things.

I would have a separate function for pwrdwn to simplify
this version which I think then becomes something that
is just used to set the value?

> > +{
> > +	int ret;
> > +	u8 shift;  
> 
> no need for shift to be u8, use unsigned int or int
> 
> > +
> > +	switch (data->channels) {
> > +	case 1:
> > +		shift = 12 - data->resolution;
> > +		data->buf[0] = (val << shift);  
> 
> parenthesis not needed here
> 
> > +		data->buf[1] = pwrdwn << 4 | (val >> (8 - shift));  
> 
> I suggest parenthesis around (pwrdwn << 4)
> 
> > +		ret = i2c_master_send(data->client, data->buf, 2);
> > +		break;
> > +	case 4:
> > +		shift = 16 - data->resolution;
> > +		data->buf[0] = val << shift;
> > +		data->buf[1] = val >> (8 - shift) | pwrdwn << 6;
> > +		data->buf[2] = channel << 1 | 1 << 4;  
> 
> I suggest parenthesis for readability
> (val >> (8 - shift))
> (channel << 1) | (1 << 4)
The 1 shifted by 4 is a magic number.  Use a define to give
it a meaningful name.

My temptation here would be to have two functions, one for
the single channel case and one for the 4 channel case.  Then
use the device type specific structure to provide the right one
for each type of device.  It moves any device type matching code
out to one place in the probe.
> 
> > +		if (pwrdwn)
> > +			data->buf[2] |= 1;
> > +		ret = i2c_master_send(data->client, data->buf, 3);
> > +		break;
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	};
> > +
> > +	return ret;
> > +}
> > +
> > +static const char *const dac5571_powerdown_modes[] = {
> > +	"1kohm_to_gnd", "100kohm_to_gnd", "three_state",
> > +};
> > +
> > +static int dac5571_get_powerdown_mode(struct iio_dev *indio_dev,
> > +				      const struct iio_chan_spec *chan)
> > +{
> > +	struct dac5571_data *data = iio_priv(indio_dev);
> > +
> > +	return data->powerdown_mode;
> > +}
> > +
> > +static int dac5571_set_powerdown_mode(struct iio_dev *indio_dev,
> > +				      const struct iio_chan_spec *chan,
> > +				      unsigned int mode)  
> 
> mode could/should be u8 as it is passed to the device, or an enum even
> 
> > +{
> > +	struct dac5571_data *data = iio_priv(indio_dev);
> > +	int ret = 0;
> > +
> > +	if (data->powerdown_mode == mode)
> > +		return 0;
> > +
> > +	mutex_lock(&data->lock);
> > +	if (data->powerdown) {
> > +		ret = dac5571_cmd(data, chan->channel, POWERDOWN(mode), 0);
> > +		if (ret)  
> 
> i2c_master_send() and hence dac5571_cmd() return the number of bytes 
> written!? should be 
> if (ret <= 0)
> ?
> 
> > +			goto out;
> > +	}
> > +	data->powerdown_mode = mode;
> > +
> > + out:
> > +	mutex_unlock(&data->lock);
> > +	return ret;
> > +}
> > +
> > +static const struct iio_enum dac5571_powerdown_mode = {
> > +	.items = dac5571_powerdown_modes,
> > +	.num_items = ARRAY_SIZE(dac5571_powerdown_modes),
> > +	.get = dac5571_get_powerdown_mode,
> > +	.set = dac5571_set_powerdown_mode,
> > +};
> > +
> > +static ssize_t dac5571_read_powerdown(struct iio_dev *indio_dev,
> > +				      uintptr_t private,
> > +				      const struct iio_chan_spec *chan,
> > +				      char *buf)
> > +{
> > +	struct dac5571_data *data = iio_priv(indio_dev);
> > +
> > +	return sprintf(buf, "%d\n", data->powerdown);
> > +}
> > +
> > +static ssize_t dac5571_write_powerdown(struct iio_dev *indio_dev,
> > +				       uintptr_t private,
> > +				       const struct iio_chan_spec *chan,
> > +				       const char *buf, size_t len)
> > +{
> > +	struct dac5571_data *data = iio_priv(indio_dev);
> > +	bool powerdown;
> > +	int ret;
> > +
> > +	ret = strtobool(buf, &powerdown);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (data->powerdown == powerdown)
> > +		return len;
> > +
> > +	mutex_lock(&data->lock);
> > +	if (powerdown)
> > +		ret =
> > +		    dac5571_cmd(data, chan->channel,
> > +				POWERDOWN(data->powerdown_mode), 0);
> > +	else
> > +		ret = dac5571_cmd(data, chan->channel, 0, data->val[0]);  
> 
> ret value of dac5571_cmd() is number of bytes written?
> 
> > +	if (!ret)
> > +		data->powerdown = powerdown;
> > +	mutex_unlock(&data->lock);
> > +
> > +	return ret ? ret : len;
> > +}
> > +
> > +
> > +static const struct iio_chan_spec_ext_info dac5571_ext_info[] = {
> > +	{
> > +		.name	   = "powerdown",
> > +		.read	   = dac5571_read_powerdown,
> > +		.write	   = dac5571_write_powerdown,
> > +		.shared	   = IIO_SHARED_BY_TYPE,
> > +	},
> > +	IIO_ENUM("powerdown_mode", IIO_SHARED_BY_TYPE, &dac5571_powerdown_mode),
> > +	IIO_ENUM_AVAILABLE("powerdown_mode", &dac5571_powerdown_mode),
> > +	{},
> > +};
> > +
> > +#define dac5571_CHANNEL(chan) {					\
> > +	.type = IIO_VOLTAGE,					\
> > +	.channel = (chan),					\
> > +	.address = (chan),					\
> > +	.indexed = true,					\
> > +	.output = true,						\
> > +	.datasheet_name = (const char[]){ 'A' + (chan), 0 },	\
Just add a parameter to the macro and make this explicit.
It is not saving enough code to make it worthwhile to do it
in a complex fashion.

> > +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> > +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> > +	.ext_info = dac5571_ext_info,				\
> > +}
> > +
> > +static const struct iio_chan_spec dac5571_channels[] = {
> > +	dac5571_CHANNEL(0),
> > +	dac5571_CHANNEL(1),
> > +	dac5571_CHANNEL(2),
> > +	dac5571_CHANNEL(3),
> > +};
> > +
> > +static int dac5571_read_raw(struct iio_dev *indio_dev,
> > +			    struct iio_chan_spec const *chan,
> > +			    int *val, int *val2, long mask)
> > +{
> > +	struct dac5571_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		*val = data->val[chan->channel];
> > +		ret = IIO_VAL_INT;  
> 
> return directly
> 
> > +		break;
> > +
> > +	case IIO_CHAN_INFO_SCALE:
> > +		ret = regulator_get_voltage(data->vref);
> > +		if (ret < 0)
> > +			return ret;
> > +
> > +		*val = ret / 1000;
> > +		*val2 = data->resolution;
> > +		ret = IIO_VAL_FRACTIONAL_LOG2;  
> 
> return directly
> 
> > +		break;
> > +
> > +	default:
> > +		ret = -EINVAL;  
> 
> return directly
> 
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int dac5571_write_raw(struct iio_dev *indio_dev,
> > +			     struct iio_chan_spec const *chan,
> > +			     int val, int val2, long mask)
> > +{
> > +	struct dac5571_data *data = iio_priv(indio_dev);
> > +	int ret;
> > +
> > +	switch (mask) {
> > +	case IIO_CHAN_INFO_RAW:
> > +		if (data->val[chan->channel] == val)
> > +			return 0;
> > +
> > +		if (val >= (1 << data->resolution) || val < 0)
> > +			return -EINVAL;
> > +
> > +		if (data->powerdown)
> > +			return -EBUSY;
> > +
> > +		mutex_lock(&data->lock);
> > +		ret = dac5571_cmd(data, chan->channel, 0, val);  
> 
> ret value of dac5571_cmd()...
> 
> > +		if (!ret)
> > +			data->val[chan->channel] = val;
> > +		mutex_unlock(&data->lock);
> > +		break;  
> 
> return directly
> 
> > +
> > +	default:
> > +		ret = -EINVAL;  
> 
> return directly
> 
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> > +static int dac5571_write_raw_get_fmt(struct iio_dev *indio_dev,
> > +				     struct iio_chan_spec const *chan,
> > +				     long mask)
> > +{
> > +	return IIO_VAL_INT;
> > +}
> > +
> > +static const struct iio_info dac5571_info = {
> > +	.read_raw	   = dac5571_read_raw,
> > +	.write_raw	   = dac5571_write_raw,
> > +	.write_raw_get_fmt = dac5571_write_raw_get_fmt,
> > +};
> > +
> > +static int dac5571_probe(struct i2c_client *client,
> > +			 const struct i2c_device_id *id)
> > +{
> > +	struct device *dev = &client->dev;
> > +	const struct dac5571_spec *spec;
> > +	struct dac5571_data *data;
> > +	struct iio_dev *indio_dev;
> > +	int ret;
> > +
> > +	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> > +	if (!indio_dev)
> > +		return -ENOMEM;
blank line here ever so slightly helps readability.

> > +	data = iio_priv(indio_dev);
> > +	i2c_set_clientdata(client, indio_dev);
> > +	data->client = client;
> > +	if (client->dev.of_node)
> > +		data->id = (enum chip_id)of_device_get_match_data(&client->dev);
> > +	else
> > +		data->id = id->driver_data;
Only used locally so no need to put in the structure.
> > +
> > +	indio_dev->dev.parent = dev;
> > +	indio_dev->dev.of_node = client->dev.of_node;
> > +	indio_dev->info = &dac5571_info;
> > +	indio_dev->name = id->name;
> > +	indio_dev->modes = INDIO_DIRECT_MODE;
> > +	indio_dev->channels = dac5571_channels;
> > +
> > +	spec = &dac5571_spec[id->driver_data];
> > +	indio_dev->num_channels = spec->num_channels;
> > +	data->resolution = spec->resolution;
> > +	data->channels = spec->num_channels;
> > +
> > +	data->vref = devm_regulator_get(dev, "vref");
> > +	if (IS_ERR(data->vref))
> > +		return PTR_ERR(data->vref);
> > +
> > +	ret = regulator_enable(data->vref);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	mutex_init(&data->lock);
> > +
> > +	ret = dac5571_cmd(data, 0, 0, 0);
> > +	if (ret) {  
> 
> ret value is number of bytes written?
> 
> > +		dev_err(dev, "failed to initialize outputs to 0\n");
> > +		goto err;
> > +	}
> > +
> > +	ret = iio_device_register(indio_dev);
> > +	if (ret)
> > +		goto err;
> > +
> > +	return 0;
> > +
> > + err:
> > +	mutex_destroy(&data->lock);  
> 
> no need to destroy mutex (at least most drivers don't)
> 
> > +	regulator_disable(data->vref);
> > +	return ret;
> > +}
> > +
> > +static int dac5571_remove(struct i2c_client *i2c)
> > +{
> > +	struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
> > +	struct dac5571_data *data = iio_priv(indio_dev);
> > +
> > +	iio_device_unregister(indio_dev);
> > +	mutex_destroy(&data->lock);  
> 
> no need to destroy mutex
> 
> > +	regulator_disable(data->vref);
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id dac5571_of_id[] = {
> > +	{.compatible = "ti,dac5571"},
> > +	{.compatible = "ti,dac6571"},
> > +	{.compatible = "ti,dac7571"},
> > +	{.compatible = "ti,dac5574"},
> > +	{.compatible = "ti,dac6574"},
> > +	{.compatible = "ti,dac7574"},
> > +	{.compatible = "ti,dac5573"},
> > +	{.compatible = "ti,dac6573"},
> > +	{.compatible = "ti,dac7573"},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, dac5571_of_id);
> > +#endif
> > +
> > +static const struct i2c_device_id dac5571_id[] = {
> > +	{"dac5571", single_8bit},
> > +	{"dac6571", single_10bit},
> > +	{"dac7571", single_12bit},
> > +	{"dac5574", quad_8bit},
> > +	{"dac6574", quad_10bit},
> > +	{"dac7574", quad_12bit},
> > +	{"dac5573", quad_8bit},
> > +	{"dac6573", quad_10bit},
> > +	{"dac7573", quad_12bit},
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(i2c, dac5571_id);
> > +
> > +static struct i2c_driver dac5571_driver = {
> > +	.driver = {
> > +		   .name = "ti-dac5571",
> > +	},
> > +	.probe	  = dac5571_probe,
> > +	.remove   = dac5571_remove,
> > +	.id_table = dac5571_id,
> > +};
> > +module_i2c_driver(dac5571_driver);
> > +
> > +MODULE_AUTHOR("Sean Nyekjaer <sean.nyekjaer@prevas.dk>");
> > +MODULE_DESCRIPTION("Texas Instruments 8/10/12-bit 1/4-channel DAC driver");
> > +MODULE_LICENSE("GPL v2");
> >   
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sean Nyekjær March 23, 2018, 12:35 p.m. UTC | #3 
On 23-03-2018 13:27, Jonathan Cameron wrote:
> If posting an RFC rather than a normal patch, I really want to see an
> explanation of what you would like comments on.
>
> A few minor comments from me.
>
> Thanks,
>
> Jonathan
I'll remember that :-)
But the reason for the RFC tag is that I don't have the hardware to test 
this on yet...
It will arrive sometime next month

Thanks for the review
/Sean
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Meerwald-Stadler March 23, 2018, 12:42 p.m. UTC | #4 
> But the reason for the RFC tag is that I don't have the hardware to test this
> on yet...
> It will arrive sometime next month

this explains the broken handling of the i2c_master_send() retval :)

p.
diff mbox

Patch

diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 965d5c0d2468..c78c02f455b9 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -320,6 +320,16 @@  config TI_DAC082S085
 
 	  If compiled as a module, it will be called ti-dac082s085.
 
+config TI_DAC5571
+	tristate "Texas Instruments 8/10/12/16-bit 1/2/4-channel DAC driver"
+	depends on I2C
+	help
+	  Driver for the Texas Instruments
+	  DAC5571, DAC6571, DAC7571, DAC5574, DAC6574, DAC7574, DAC5573,
+	  DAC6573, DAC7573.
+
+	  If compiled as a module, it will be called ti-dac5571.
+
 config VF610_DAC
 	tristate "Vybrid vf610 DAC driver"
 	depends on OF
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 81e710ed7491..1ab358d522ee 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -35,4 +35,5 @@  obj-$(CONFIG_MCP4922) += mcp4922.o
 obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
 obj-$(CONFIG_STM32_DAC) += stm32-dac.o
 obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o
+obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o
 obj-$(CONFIG_VF610_DAC) += vf610_dac.o
diff --git a/drivers/iio/dac/ti-dac5571.c b/drivers/iio/dac/ti-dac5571.c
new file mode 100644
index 000000000000..cf4ba151ed7e
--- /dev/null
+++ b/drivers/iio/dac/ti-dac5571.c
@@ -0,0 +1,395 @@ 
+/*
+ * ti-dac5571.c - Texas Instruments 8/10/12-bit 1/4-channel DAC driver
+ *
+ * Copyright (C) 2018 Prevas A/S
+ *
+ * http://www.ti.com/lit/ds/symlink/dac5571.pdf
+ * http://www.ti.com/lit/ds/symlink/dac6571.pdf
+ * http://www.ti.com/lit/ds/symlink/dac7571.pdf
+ * http://www.ti.com/lit/ds/symlink/dac5574.pdf
+ * http://www.ti.com/lit/ds/symlink/dac6574.pdf
+ * http://www.ti.com/lit/ds/symlink/dac7574.pdf
+ * http://www.ti.com/lit/ds/symlink/dac5573.pdf
+ * http://www.ti.com/lit/ds/symlink/dac6573.pdf
+ * http://www.ti.com/lit/ds/symlink/dac7573.pdf
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License (version 2) as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/iio/iio.h>
+#include <linux/module.h>
+#include <linux/regulator/consumer.h>
+#include <linux/i2c.h>
+#include <linux/of_device.h>
+#include <linux/of.h>
+
+enum chip_id {
+	single_8bit, single_10bit, single_12bit,
+	quad_8bit, quad_10bit, quad_12bit
+};
+
+struct dac5571_spec {
+	u8 num_channels;
+	u8 resolution;
+};
+
+static const struct dac5571_spec dac5571_spec[] = {
+	[single_8bit]  = {.num_channels = 1, .resolution =  8},
+	[single_10bit] = {.num_channels = 1, .resolution = 10},
+	[single_12bit] = {.num_channels = 1, .resolution = 12},
+	[quad_8bit]    = {.num_channels = 4, .resolution =  8},
+	[quad_10bit]   = {.num_channels = 4, .resolution = 10},
+	[quad_12bit]   = {.num_channels = 4, .resolution = 12},
+};
+
+struct dac5571_data {
+	struct i2c_client *client;
+	int id;
+	struct mutex lock;
+	struct regulator *vref;
+	u16 val[4];
+	bool powerdown;
+	u8 powerdown_mode;
+	u8 resolution;
+	u8 channels;
+	u8 buf[3] ____cacheline_aligned;
+};
+
+#define POWERDOWN(mode)		((mode) + 1)
+
+static int dac5571_cmd(struct dac5571_data *data, int channel,
+		       u8 pwrdwn, u16 val)
+{
+	int ret;
+	u8 shift;
+
+	switch (data->channels) {
+	case 1:
+		shift = 12 - data->resolution;
+		data->buf[0] = (val << shift);
+		data->buf[1] = pwrdwn << 4 | (val >> (8 - shift));
+		ret = i2c_master_send(data->client, data->buf, 2);
+		break;
+	case 4:
+		shift = 16 - data->resolution;
+		data->buf[0] = val << shift;
+		data->buf[1] = val >> (8 - shift) | pwrdwn << 6;
+		data->buf[2] = channel << 1 | 1 << 4;
+		if (pwrdwn)
+			data->buf[2] |= 1;
+		ret = i2c_master_send(data->client, data->buf, 3);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	};
+
+	return ret;
+}
+
+static const char *const dac5571_powerdown_modes[] = {
+	"1kohm_to_gnd", "100kohm_to_gnd", "three_state",
+};
+
+static int dac5571_get_powerdown_mode(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan)
+{
+	struct dac5571_data *data = iio_priv(indio_dev);
+
+	return data->powerdown_mode;
+}
+
+static int dac5571_set_powerdown_mode(struct iio_dev *indio_dev,
+				      const struct iio_chan_spec *chan,
+				      unsigned int mode)
+{
+	struct dac5571_data *data = iio_priv(indio_dev);
+	int ret = 0;
+
+	if (data->powerdown_mode == mode)
+		return 0;
+
+	mutex_lock(&data->lock);
+	if (data->powerdown) {
+		ret = dac5571_cmd(data, chan->channel, POWERDOWN(mode), 0);
+		if (ret)
+			goto out;
+	}
+	data->powerdown_mode = mode;
+
+ out:
+	mutex_unlock(&data->lock);
+	return ret;
+}
+
+static const struct iio_enum dac5571_powerdown_mode = {
+	.items = dac5571_powerdown_modes,
+	.num_items = ARRAY_SIZE(dac5571_powerdown_modes),
+	.get = dac5571_get_powerdown_mode,
+	.set = dac5571_set_powerdown_mode,
+};
+
+static ssize_t dac5571_read_powerdown(struct iio_dev *indio_dev,
+				      uintptr_t private,
+				      const struct iio_chan_spec *chan,
+				      char *buf)
+{
+	struct dac5571_data *data = iio_priv(indio_dev);
+
+	return sprintf(buf, "%d\n", data->powerdown);
+}
+
+static ssize_t dac5571_write_powerdown(struct iio_dev *indio_dev,
+				       uintptr_t private,
+				       const struct iio_chan_spec *chan,
+				       const char *buf, size_t len)
+{
+	struct dac5571_data *data = iio_priv(indio_dev);
+	bool powerdown;
+	int ret;
+
+	ret = strtobool(buf, &powerdown);
+	if (ret)
+		return ret;
+
+	if (data->powerdown == powerdown)
+		return len;
+
+	mutex_lock(&data->lock);
+	if (powerdown)
+		ret =
+		    dac5571_cmd(data, chan->channel,
+				POWERDOWN(data->powerdown_mode), 0);
+	else
+		ret = dac5571_cmd(data, chan->channel, 0, data->val[0]);
+	if (!ret)
+		data->powerdown = powerdown;
+	mutex_unlock(&data->lock);
+
+	return ret ? ret : len;
+}
+
+
+static const struct iio_chan_spec_ext_info dac5571_ext_info[] = {
+	{
+		.name	   = "powerdown",
+		.read	   = dac5571_read_powerdown,
+		.write	   = dac5571_write_powerdown,
+		.shared	   = IIO_SHARED_BY_TYPE,
+	},
+	IIO_ENUM("powerdown_mode", IIO_SHARED_BY_TYPE, &dac5571_powerdown_mode),
+	IIO_ENUM_AVAILABLE("powerdown_mode", &dac5571_powerdown_mode),
+	{},
+};
+
+#define dac5571_CHANNEL(chan) {					\
+	.type = IIO_VOLTAGE,					\
+	.channel = (chan),					\
+	.address = (chan),					\
+	.indexed = true,					\
+	.output = true,						\
+	.datasheet_name = (const char[]){ 'A' + (chan), 0 },	\
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
+	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
+	.ext_info = dac5571_ext_info,				\
+}
+
+static const struct iio_chan_spec dac5571_channels[] = {
+	dac5571_CHANNEL(0),
+	dac5571_CHANNEL(1),
+	dac5571_CHANNEL(2),
+	dac5571_CHANNEL(3),
+};
+
+static int dac5571_read_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int *val, int *val2, long mask)
+{
+	struct dac5571_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*val = data->val[chan->channel];
+		ret = IIO_VAL_INT;
+		break;
+
+	case IIO_CHAN_INFO_SCALE:
+		ret = regulator_get_voltage(data->vref);
+		if (ret < 0)
+			return ret;
+
+		*val = ret / 1000;
+		*val2 = data->resolution;
+		ret = IIO_VAL_FRACTIONAL_LOG2;
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int dac5571_write_raw(struct iio_dev *indio_dev,
+			     struct iio_chan_spec const *chan,
+			     int val, int val2, long mask)
+{
+	struct dac5571_data *data = iio_priv(indio_dev);
+	int ret;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		if (data->val[chan->channel] == val)
+			return 0;
+
+		if (val >= (1 << data->resolution) || val < 0)
+			return -EINVAL;
+
+		if (data->powerdown)
+			return -EBUSY;
+
+		mutex_lock(&data->lock);
+		ret = dac5571_cmd(data, chan->channel, 0, val);
+		if (!ret)
+			data->val[chan->channel] = val;
+		mutex_unlock(&data->lock);
+		break;
+
+	default:
+		ret = -EINVAL;
+	}
+
+	return ret;
+}
+
+static int dac5571_write_raw_get_fmt(struct iio_dev *indio_dev,
+				     struct iio_chan_spec const *chan,
+				     long mask)
+{
+	return IIO_VAL_INT;
+}
+
+static const struct iio_info dac5571_info = {
+	.read_raw	   = dac5571_read_raw,
+	.write_raw	   = dac5571_write_raw,
+	.write_raw_get_fmt = dac5571_write_raw_get_fmt,
+};
+
+static int dac5571_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	const struct dac5571_spec *spec;
+	struct dac5571_data *data;
+	struct iio_dev *indio_dev;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*data));
+	if (!indio_dev)
+		return -ENOMEM;
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+	if (client->dev.of_node)
+		data->id = (enum chip_id)of_device_get_match_data(&client->dev);
+	else
+		data->id = id->driver_data;
+
+	indio_dev->dev.parent = dev;
+	indio_dev->dev.of_node = client->dev.of_node;
+	indio_dev->info = &dac5571_info;
+	indio_dev->name = id->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->channels = dac5571_channels;
+
+	spec = &dac5571_spec[id->driver_data];
+	indio_dev->num_channels = spec->num_channels;
+	data->resolution = spec->resolution;
+	data->channels = spec->num_channels;
+
+	data->vref = devm_regulator_get(dev, "vref");
+	if (IS_ERR(data->vref))
+		return PTR_ERR(data->vref);
+
+	ret = regulator_enable(data->vref);
+	if (ret < 0)
+		return ret;
+
+	mutex_init(&data->lock);
+
+	ret = dac5571_cmd(data, 0, 0, 0);
+	if (ret) {
+		dev_err(dev, "failed to initialize outputs to 0\n");
+		goto err;
+	}
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto err;
+
+	return 0;
+
+ err:
+	mutex_destroy(&data->lock);
+	regulator_disable(data->vref);
+	return ret;
+}
+
+static int dac5571_remove(struct i2c_client *i2c)
+{
+	struct iio_dev *indio_dev = i2c_get_clientdata(i2c);
+	struct dac5571_data *data = iio_priv(indio_dev);
+
+	iio_device_unregister(indio_dev);
+	mutex_destroy(&data->lock);
+	regulator_disable(data->vref);
+
+	return 0;
+}
+
+#ifdef CONFIG_OF
+static const struct of_device_id dac5571_of_id[] = {
+	{.compatible = "ti,dac5571"},
+	{.compatible = "ti,dac6571"},
+	{.compatible = "ti,dac7571"},
+	{.compatible = "ti,dac5574"},
+	{.compatible = "ti,dac6574"},
+	{.compatible = "ti,dac7574"},
+	{.compatible = "ti,dac5573"},
+	{.compatible = "ti,dac6573"},
+	{.compatible = "ti,dac7573"},
+	{}
+};
+MODULE_DEVICE_TABLE(of, dac5571_of_id);
+#endif
+
+static const struct i2c_device_id dac5571_id[] = {
+	{"dac5571", single_8bit},
+	{"dac6571", single_10bit},
+	{"dac7571", single_12bit},
+	{"dac5574", quad_8bit},
+	{"dac6574", quad_10bit},
+	{"dac7574", quad_12bit},
+	{"dac5573", quad_8bit},
+	{"dac6573", quad_10bit},
+	{"dac7573", quad_12bit},
+	{}
+};
+MODULE_DEVICE_TABLE(i2c, dac5571_id);
+
+static struct i2c_driver dac5571_driver = {
+	.driver = {
+		   .name = "ti-dac5571",
+	},
+	.probe	  = dac5571_probe,
+	.remove   = dac5571_remove,
+	.id_table = dac5571_id,
+};
+module_i2c_driver(dac5571_driver);
+
+MODULE_AUTHOR("Sean Nyekjaer <sean.nyekjaer@prevas.dk>");
+MODULE_DESCRIPTION("Texas Instruments 8/10/12-bit 1/4-channel DAC driver");
+MODULE_LICENSE("GPL v2");