Re: Linux driver for MAX517/518/519

Re: Linux driver for MAX517/518/519

[Roland Stigge]

[-- Attachment #1: Type: text/plain, Size: 1198 bytes --]

Hi,

On 03/01/11 23:28, Greg KH wrote:
>> I've written a driver for the Maxim MAX517/MAX518/MAX519 1/2-channel
>> DAC. See also the branch "max51x" of repository linux-2.6 at
>> git.antcom.de (alternatively the patch at
>> http://www.rolandstigge.de/dp/download/patches/max51x.patch).
>>
>> It's currently located in drivers/misc/max51x.c, implemented similarly
>> to the other drivers like e.g. ti_dac7512.c. The hwmon maintainer
>> pointed me to drivers/staging/iio/.
>>
>> Is it advisible now to port the driver to drivers/staging/iio or keep it
>> in drivers/misc? Where should I forward the code/patch for Linux
>> integration?
> 
> It's best to convert to the iio subsystem, as that is the way to go in
> the future.
> 
> As for where to send patches for it, send them here, and cc: the iio
> developers and me and we will be glad to comment on them and apply them
> to the kernel tree for submission to Linus's tree.

OK, done. Attaching the patch. (Also available at the above mentioned
git repository for merging.)

Thanks in advance,

Roland


BTW: Maybe we should also call it "Analog to digital converters" instead
of "Analog to digital convertors", see drivers/staging/iio/adc/Kconfig

[-- Attachment #2: max51x.patch --]
[-- Type: text/x-patch, Size: 10107 bytes --]

Added max51x iio driver

Signed-off-by: Roland Stigge <stigge@antcom.de>

diff --git a/drivers/staging/iio/Documentation/dac/max51x b/drivers/staging/iio/Documentation/dac/max51x
new file mode 100644
index 0000000..cf61a25
--- /dev/null
+++ b/drivers/staging/iio/Documentation/dac/max51x
@@ -0,0 +1,38 @@
+Kernel driver max51x
+====================
+
+Supported chips:
+  * Maxim MAX517, MAX518, MAX519
+    Prefix: 'max51x'
+    Datasheet: Publicly available at the Maxim website
+               http://www.maxim-ic.com/
+
+Author:
+        Roland Stigge <stigge@antcom.de>
+
+Description
+-----------
+
+The Maxim MAX51x is an 8-bit DAC on the I2C bus. The following table shows the
+different feature sets of the variants MAX517, MAX518 and MAX519:
+
+Feature                              MAX517 MAX518 MAX519
+--------------------------------------------------------------------------
+One output channel                   X
+Two output channels                         X      X
+Simultaneous output updates                 X      X
+Supply voltage as reference                 X
+Separate reference input             X
+Reference input for each DAC                       X
+
+Via the iio sysfs interface, there are three attributes available: value_1,
+value_2 and value_both. With value_1 and value_2, the current output values of
+the DACs can be read back from the driver and written to the device. value_both
+can be used to set both output channel values simultaneously.
+
+With MAX517, only value_1 is available.
+
+When the operating system goes to a power down state, the Power Down function
+of the chip is activated, reducing the supply current to 4uA.
+
+On power-up, the device is in 0V-output state.
diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig
index ed48815..6fc35ad 100644
--- a/drivers/staging/iio/Kconfig
+++ b/drivers/staging/iio/Kconfig
@@ -42,6 +42,7 @@ config IIO_TRIGGER
 
 source "drivers/staging/iio/accel/Kconfig"
 source "drivers/staging/iio/adc/Kconfig"
+source "drivers/staging/iio/dac/Kconfig"
 source "drivers/staging/iio/gyro/Kconfig"
 source "drivers/staging/iio/imu/Kconfig"
 source "drivers/staging/iio/light/Kconfig"
diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile
index e909674..8766e07 100644
--- a/drivers/staging/iio/Makefile
+++ b/drivers/staging/iio/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_IIO_SW_RING) += ring_sw.o
 
 obj-y += accel/
 obj-y += adc/
+obj-y += dac/
 obj-y += gyro/
 obj-y += imu/
 obj-y += light/
diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
new file mode 100644
index 0000000..f2c9c84
--- /dev/null
+++ b/drivers/staging/iio/dac/Kconfig
@@ -0,0 +1,14 @@
+#
+# DAC drivers
+#
+comment "Digital to analog converters"
+
+config MAX51X
+	tristate "Maxim MAX51X DAC driver"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the Maxim chips MAX517,
+	  MAX518 and MAX519 (I2C 8-Bit DACs with rail-to-rail outputs).
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called max51x.
diff --git a/drivers/staging/iio/dac/Makefile b/drivers/staging/iio/dac/Makefile
new file mode 100644
index 0000000..3caa871
--- /dev/null
+++ b/drivers/staging/iio/dac/Makefile
@@ -0,0 +1,5 @@
+#
+# Makefile for industrial I/O DAC drivers
+#
+
+obj-$(CONFIG_MAX51X) += max51x.o
diff --git a/drivers/staging/iio/dac/max51x.c b/drivers/staging/iio/dac/max51x.c
new file mode 100644
index 0000000..6d05428
--- /dev/null
+++ b/drivers/staging/iio/dac/max51x.c
@@ -0,0 +1,258 @@
+/*
+ *  max51x.c - Support for Maxim MAX517, MAX518 and MAX519
+ *
+ *  Copyright (C) 2010 Roland Stigge <stigge@antcom.de>
+ *
+ *  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.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+
+#include "../iio.h"
+
+#define MAX51X_DRV_NAME	"max51x"
+
+/* Commands */
+#define COMMAND_CHANNEL0	0x00
+#define COMMAND_CHANNEL1	0x01 /* for MAX518 and MAX519 */
+#define COMMAND_PD		0x08 /* Power Down */
+
+struct max51x_data {
+	u8			values[2];
+	struct iio_dev		*indio_dev;
+};
+
+/*
+ * channel: bit 0: channel 1
+ *          bit 1: channel 2
+ * (this way, it's possible to set both channels at once)
+ */
+static ssize_t max51x_set_value(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count, int channel)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct max51x_data *data = i2c_get_clientdata(client);
+	u8 outbuf[4]; /* 1x or 2x command + value */
+	int outbuf_size = 0;
+	int res;
+	long val;
+
+	res = strict_strtol(buf, 10, &val);
+
+	if (res)
+		return res;
+
+	if (val < 0 || val > 255)
+		return -EINVAL;
+
+	if (channel & 1) {
+		data->values[0] = val;
+		outbuf[outbuf_size++] = COMMAND_CHANNEL0;
+		outbuf[outbuf_size++] = val;
+	}
+	if (channel & 2) {
+		data->values[1] = val;
+		outbuf[outbuf_size++] = COMMAND_CHANNEL1;
+		outbuf[outbuf_size++] = val;
+	}
+
+	/*
+	 * at this point, there are always 1 or 2 two-byte commands in
+	 * outbuf
+	 */
+	i2c_smbus_xfer(client->adapter, client->addr, client->flags,
+			I2C_SMBUS_WRITE, outbuf[0], outbuf_size - 1,
+			(union i2c_smbus_data *) &outbuf[1]);
+	return count;
+}
+
+static ssize_t max51x_set_value_1(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	return max51x_set_value(dev, attr, buf, count, 1);
+}
+
+static ssize_t max51x_set_value_2(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	return max51x_set_value(dev, attr, buf, count, 2);
+}
+
+static ssize_t max51x_set_value_both(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	return max51x_set_value(dev, attr, buf, count, 3);
+}
+
+static ssize_t max51x_get_value(struct device *dev, struct device_attribute *da,
+				char *buf, int channel)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct max51x_data *data = i2c_get_clientdata(client);
+	u8 val;
+
+	if (channel == 1) {
+		val = data->values[0];
+	} else { /* channel == 2 */
+		val = data->values[1];
+	}
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t max51x_get_value_1(struct device *dev,
+				  struct device_attribute *da, char *buf)
+{
+	return max51x_get_value(dev, da, buf, 1);
+}
+
+static ssize_t max51x_get_value_2(struct device *dev,
+				  struct device_attribute *da, char *buf)
+{
+	return max51x_get_value(dev, da, buf, 2);
+}
+
+static IIO_DEVICE_ATTR(value_1, S_IWUSR | S_IRUGO,
+		max51x_get_value_1, max51x_set_value_1, 0);
+static IIO_DEVICE_ATTR(value_2, S_IWUSR | S_IRUGO,
+		max51x_get_value_2, max51x_set_value_2, 1);
+static IIO_DEVICE_ATTR(value_both, S_IWUSR, NULL, max51x_set_value_both, -1);
+
+static struct attribute *max51x_attributes[] = {
+	&iio_dev_attr_value_1.dev_attr.attr,
+	&iio_dev_attr_value_2.dev_attr.attr,
+	&iio_dev_attr_value_both.dev_attr.attr,
+	NULL
+};
+
+static struct attribute *max517_attributes[] = {
+	&iio_dev_attr_value_1.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group max51x_attribute_group = {
+	.attrs = max51x_attributes,
+};
+
+static int max51x_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	return i2c_smbus_write_byte(client, COMMAND_PD);
+}
+
+static int max51x_resume(struct i2c_client *client)
+{
+	return i2c_smbus_write_byte(client, 0);
+}
+
+static int max51x_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct max51x_data *data;
+	int err;
+
+	data = kzalloc(sizeof(struct max51x_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	i2c_set_clientdata(client, data);
+
+	/* reduced attribute set for max517 */
+	if (!strcmp(id->name, "max517"))
+		max51x_attribute_group.attrs = max517_attributes;
+
+	data->indio_dev = iio_allocate_device();
+	if (data->indio_dev == NULL) {
+		err = -ENOMEM;
+		goto exit_free_data;
+	}
+
+	/* establish that the iio_dev is a child of the i2c device */
+	data->indio_dev->dev.parent = &client->dev;
+	data->indio_dev->attrs = &max51x_attribute_group;
+	data->indio_dev->dev_data = (void *)(data);
+	data->indio_dev->driver_module = THIS_MODULE;
+	data->indio_dev->modes = INDIO_DIRECT_MODE;
+
+	err = iio_device_register(data->indio_dev);
+	if (err)
+		goto exit_free_device;
+
+	dev_info(&client->dev, "DAC registered\n");
+
+	return 0;
+
+exit_free_device:
+	iio_free_device(data->indio_dev);
+exit_free_data:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int max51x_remove(struct i2c_client *client)
+{
+	struct max51x_data *data = i2c_get_clientdata(client);
+
+	kfree(data);
+
+	return 0;
+}
+
+static const struct i2c_device_id max51x_id[] = {
+	{ "max517", 0 },
+	{ "max518", 0 },
+	{ "max519", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max51x_id);
+
+static struct i2c_driver max51x_driver = {
+	.driver = {
+		.name	= MAX51X_DRV_NAME,
+	},
+	.probe		= max51x_probe,
+	.remove		= max51x_remove,
+	.suspend	= max51x_suspend,
+	.resume		= max51x_resume,
+	.id_table	= max51x_id,
+};
+
+static int __init max51x_init(void)
+{
+	return i2c_add_driver(&max51x_driver);
+}
+
+static void __exit max51x_exit(void)
+{
+	i2c_del_driver(&max51x_driver);
+}
+
+MODULE_AUTHOR("Roland Stigge <stigge@antcom.de>");
+MODULE_DESCRIPTION("MAX51X 8-bit DAC");
+MODULE_LICENSE("GPL");
+
+module_init(max51x_init);
+module_exit(max51x_exit);

Re: Linux driver for MAX517/518/519

[Jonathan Cameron]

On 01/09/11 14:29, Roland Stigge wrote:
> Hi,
> 
> On 03/01/11 23:28, Greg KH wrote:
>>> I've written a driver for the Maxim MAX517/MAX518/MAX519 1/2-channel
>>> DAC. See also the branch "max51x" of repository linux-2.6 at
>>> git.antcom.de (alternatively the patch at
>>> http://www.rolandstigge.de/dp/download/patches/max51x.patch).
>>>
>>> It's currently located in drivers/misc/max51x.c, implemented similarly
>>> to the other drivers like e.g. ti_dac7512.c. The hwmon maintainer
>>> pointed me to drivers/staging/iio/.
>>>
>>> Is it advisible now to port the driver to drivers/staging/iio or keep it
>>> in drivers/misc? Where should I forward the code/patch for Linux
>>> integration?
>>
>> It's best to convert to the iio subsystem, as that is the way to go in
>> the future.
>>
>> As for where to send patches for it, send them here, and cc: the iio
>> developers and me and we will be glad to comment on them and apply them
>> to the kernel tree for submission to Linus's tree.
> 
> OK, done. Attaching the patch. (Also available at the above mentioned
> git repository for merging.)
> 
> Thanks in advance,
> 
> Roland
Hi Roland,

What the heck, nothing on iplayer so I'll do a review now.

Firstly, I find reviewing easier in reverse so some comments
may only make sense that way around!

Pretty clean and nice driver so it was an easy review and should
be trivial to fix up for a merge.

Main points of review are:

1) You are on an out of date tree, please use Greg's staging-next
tree for base of IIO patches (see below for addresses). This means
you haven't seen or matched interfaces of the DAC drivers now
in the tree.

2) Interfaces need to match those in standard.  We need to support
a wide range of devices, so things like 'value' don't give enough
information. Note, if possible, can we also have output_scale
defined so userspace knows how to convert to the device units.
(obviously this point is really a result of point 1!)

3) One new interface needed (I think?) See below. Have cc'd Michael
as he is much more familiar with this area than I am and I'd like
his input on this if possible.

One or two other quirks inline.

Thanks,

Jonathan
> 
> 
> BTW: Maybe we should also call it "Analog to digital converters" instead
> of "Analog to digital convertors", see drivers/staging/iio/adc/Kconfig

> Added max51x iio driver
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>
> 
> diff --git a/drivers/staging/iio/Documentation/dac/max51x b/drivers/staging/iio/Documentation/dac/max51x
> new file mode 100644
> index 0000000..cf61a25
> --- /dev/null
> +++ b/drivers/staging/iio/Documentation/dac/max51x
> @@ -0,0 +1,38 @@
> +Kernel driver max51x
> +====================
> +
> +Supported chips:
> +  * Maxim MAX517, MAX518, MAX519
> +    Prefix: 'max51x'
> +    Datasheet: Publicly available at the Maxim website
> +               http://www.maxim-ic.com/
> +
> +Author:
> +        Roland Stigge <stigge@antcom.de>
> +
> +Description
> +-----------
> +
> +The Maxim MAX51x is an 8-bit DAC on the I2C bus. The following table shows the
> +different feature sets of the variants MAX517, MAX518 and MAX519:
> +
> +Feature                              MAX517 MAX518 MAX519
> +--------------------------------------------------------------------------
> +One output channel                   X
> +Two output channels                         X      X
> +Simultaneous output updates                 X      X
> +Supply voltage as reference                 X
> +Separate reference input             X
> +Reference input for each DAC                       X
> +
> +Via the iio sysfs interface, there are three attributes available: value_1,
> +value_2 and value_both. With value_1 and value_2, the current output values of
> +the DACs can be read back from the driver and written to the device. value_both
> +can be used to set both output channel values simultaneously.
> +
> +With MAX517, only value_1 is available.
> +
> +When the operating system goes to a power down state, the Power Down function
> +of the chip is activated, reducing the supply current to 4uA.
> +
> +On power-up, the device is in 0V-output state.
> diff --git a/drivers/staging/iio/Kconfig b/drivers/staging/iio/Kconfig
> index ed48815..6fc35ad 100644
> --- a/drivers/staging/iio/Kconfig
> +++ b/drivers/staging/iio/Kconfig
> @@ -42,6 +42,7 @@ config IIO_TRIGGER
>  
>  source "drivers/staging/iio/accel/Kconfig"
>  source "drivers/staging/iio/adc/Kconfig"
> +source "drivers/staging/iio/dac/Kconfig"
>  source "drivers/staging/iio/gyro/Kconfig"
>  source "drivers/staging/iio/imu/Kconfig"
>  source "drivers/staging/iio/light/Kconfig"
> diff --git a/drivers/staging/iio/Makefile b/drivers/staging/iio/Makefile
> index e909674..8766e07 100644
> --- a/drivers/staging/iio/Makefile
> +++ b/drivers/staging/iio/Makefile
> @@ -11,6 +11,7 @@ obj-$(CONFIG_IIO_SW_RING) += ring_sw.o
>  
>  obj-y += accel/
>  obj-y += adc/
> +obj-y += dac/
Ah, looks like you have an old tree. All work on IIO drivers
needs to be done against the staging-next branch of
Greg's tree at:

http://git.kernel.org/?p=linux/kernel/git/gregkh/staging-2.6.git;a=shortlog;h=refs/heads/staging-next

git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging-2.6.git staging-next

There are a couple of DAC's in there now to copy interfaces
from.

>  obj-y += gyro/
>  obj-y += imu/
>  obj-y += light/
> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
> new file mode 100644
> index 0000000..f2c9c84
> --- /dev/null
> +++ b/drivers/staging/iio/dac/Kconfig
> @@ -0,0 +1,14 @@
> +#
> +# DAC drivers
> +#
> +comment "Digital to analog converters"
> +
> +config MAX51X

Please don't use a wild card in a driver name. For starters,
the MAX510 is another DAC not supported by this driver.

Pick one of the devices supported and use that as the name.
We may well find Maxim release more devices with the same
interface (more or less) that have radically different
numbers. For example, see the set we have ended up with in
adc/max1363!

> +	tristate "Maxim MAX51X DAC driver"
> +	depends on I2C && EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for the Maxim chips MAX517,
> +	  MAX518 and MAX519 (I2C 8-Bit DACs with rail-to-rail outputs).
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called max51x.
> diff --git a/drivers/staging/iio/dac/Makefile b/drivers/staging/iio/dac/Makefile
> new file mode 100644
> index 0000000..3caa871
> --- /dev/null
> +++ b/drivers/staging/iio/dac/Makefile
> @@ -0,0 +1,5 @@
> +#
> +# Makefile for industrial I/O DAC drivers
> +#
> +
> +obj-$(CONFIG_MAX51X) += max51x.o
> diff --git a/drivers/staging/iio/dac/max51x.c b/drivers/staging/iio/dac/max51x.c
> new file mode 100644
> index 0000000..6d05428
> --- /dev/null
> +++ b/drivers/staging/iio/dac/max51x.c
> @@ -0,0 +1,258 @@
> +/*
> + *  max51x.c - Support for Maxim MAX517, MAX518 and MAX519
> + *
> + *  Copyright (C) 2010 Roland Stigge <stigge@antcom.de>
> + *
> + *  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.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +
> +#include "../iio.h"
> +
> +#define MAX51X_DRV_NAME	"max51x"
> +
> +/* Commands */
> +#define COMMAND_CHANNEL0	0x00
> +#define COMMAND_CHANNEL1	0x01 /* for MAX518 and MAX519 */
> +#define COMMAND_PD		0x08 /* Power Down */
> +
> +struct max51x_data {
> +	u8			values[2];
> +	struct iio_dev		*indio_dev;
> +};
> +
> +/*
> + * channel: bit 0: channel 1
> + *          bit 1: channel 2
> + * (this way, it's possible to set both channels at once)
> + */
> +static ssize_t max51x_set_value(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count, int channel)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct max51x_data *data = i2c_get_clientdata(client);
> +	u8 outbuf[4]; /* 1x or 2x command + value */
> +	int outbuf_size = 0;
> +	int res;
> +	long val;
> +
> +	res = strict_strtol(buf, 10, &val);
> +
> +	if (res)
> +		return res;
> +
> +	if (val < 0 || val > 255)
> +		return -EINVAL;
> +
> +	if (channel & 1) {
> +		data->values[0] = val;
> +		outbuf[outbuf_size++] = COMMAND_CHANNEL0;
> +		outbuf[outbuf_size++] = val;
> +	}
> +	if (channel & 2) {
> +		data->values[1] = val;
> +		outbuf[outbuf_size++] = COMMAND_CHANNEL1;
> +		outbuf[outbuf_size++] = val;
> +	}
> +
> +	/*
> +	 * at this point, there are always 1 or 2 two-byte commands in
> +	 * outbuf
Perhaps expand this comment to explain how the double version is
being blugeoned into an smbus transfer... It's a little unusual!
Does doing this actually conform to the smbus spec? It would be fine
with generic i2c, but then you wouldn't have to manipulate the form
quite like this...
> +	 */
> +	i2c_smbus_xfer(client->adapter, client->addr, client->flags,
> +			I2C_SMBUS_WRITE, outbuf[0], outbuf_size - 1,
> +			(union i2c_smbus_data *) &outbuf[1]);
I'd like to see some handling of any errors from this function.


> +	return count;
> +}
> +
> +static ssize_t max51x_set_value_1(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	return max51x_set_value(dev, attr, buf, count, 1);
> +}
> +
> +static ssize_t max51x_set_value_2(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	return max51x_set_value(dev, attr, buf, count, 2);
> +}
> +
> +static ssize_t max51x_set_value_both(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	return max51x_set_value(dev, attr, buf, count, 3);
> +}
> +
> +static ssize_t max51x_get_value(struct device *dev, struct device_attribute *da,
> +				char *buf, int channel)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	struct max51x_data *data = i2c_get_clientdata(client);
> +	u8 val;
> +
> +	if (channel == 1) {
> +		val = data->values[0];
> +	} else { /* channel == 2 */
> +		val = data->values[1];
> +	}
> +
> +	return sprintf(buf, "%d\n", val);
return sprintf(buf, "%d\n", data->values[channel - 1]); ?
> +}
> +
> +static ssize_t max51x_get_value_1(struct device *dev,
> +				  struct device_attribute *da, char *buf)
> +{
> +	return max51x_get_value(dev, da, buf, 1);
> +}
> +
> +static ssize_t max51x_get_value_2(struct device *dev,
> +				  struct device_attribute *da, char *buf)
> +{
> +	return max51x_get_value(dev, da, buf, 2);
> +}
> +
> +static IIO_DEVICE_ATTR(value_1, S_IWUSR | S_IRUGO,
> +		max51x_get_value_1, max51x_set_value_1, 0);

If you see Greg's staging tree, we've recently pinned
down the interface for DAC devices as part of merging
various Analog Devices drivers.

There is a convenient macro in staging/iio/dac/dac.h

http://git.kernel.org/?p=linux/kernel/git/gregkh/staging-2.6.git;a=blob;f=drivers/staging/iio/dac/dac.h;h=1d82f353241c2ea5d3dacc240d8f56821aa0f4a8;hb=refs/heads/staging-next

Ooops, it seems the docs didn't get updated. Michael, do you
want to do the docs for this?

Basically, voltage dacs have

outputX_raw

I don't think we have previously had a device that
allows setting multiple inputs together.
Two options come to mind that will generalize more
than your _both.

output1&2_raw

output_raw (suppress the index hence indicating that it sets both).

What do you think is the clearest approach?
Which ever we pick it will also need proper documentation.  Whilst we
are here, please can you explain your use case?  From a datasheet
read I think the first channel is latched after the value byte is passed
then the second only after it's value has been passed over?
I appreciate there are plenty of cases where a true simultaneous latch
would make sense, but would like an example of when the behaviour seen
here makes sense. (I have no problem with this support, just would
like an example for documentation purposes as it isn't immediately
obvious to me.) 

> +static IIO_DEVICE_ATTR(value_2, S_IWUSR | S_IRUGO,
> +		max51x_get_value_2, max51x_set_value_2, 1);
> +static IIO_DEVICE_ATTR(value_both, S_IWUSR, NULL, max51x_set_value_both, -1);
> +
> +static struct attribute *max51x_attributes[] = {
> +	&iio_dev_attr_value_1.dev_attr.attr,
> +	&iio_dev_attr_value_2.dev_attr.attr,
> +	&iio_dev_attr_value_both.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute *max517_attributes[] = {
> +	&iio_dev_attr_value_1.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group max51x_attribute_group = {
> +	.attrs = max51x_attributes,
> +};
> +
> +static int max51x_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +	return i2c_smbus_write_byte(client, COMMAND_PD);
> +}
> +
> +static int max51x_resume(struct i2c_client *client)
> +{
> +	return i2c_smbus_write_byte(client, 0);
> +}
> +
> +static int max51x_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct max51x_data *data;
> +	int err;
> +
> +	data = kzalloc(sizeof(struct max51x_data), GFP_KERNEL);
> +	if (!data) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	i2c_set_clientdata(client, data);
> +
> +	/* reduced attribute set for max517 */
> +	if (!strcmp(id->name, "max517"))
Use the other parameter in the table to do this. Typically create
a suitable enum overing the variants, then set the second column
in the table equal to the relevant enum values.  These are then
available in id->driver_id.  Clearly cheaper to use that than to
do a strcmp.

> +		max51x_attribute_group.attrs = max517_attributes;
I'd also prefer to see two groups rather than switching the
attributes in one of them.  Then you can just set
data->indio_dev->dev_data equal to the relevant one.  The
form used here requires looking back to see what the value
of group.attrs otherwise would be and hence makes the code
marginally harder to read.
> +
> +	data->indio_dev = iio_allocate_device();
> +	if (data->indio_dev == NULL) {
> +		err = -ENOMEM;
> +		goto exit_free_data;
> +	}
> +
> +	/* establish that the iio_dev is a child of the i2c device */
> +	data->indio_dev->dev.parent = &client->dev;
> +	data->indio_dev->attrs = &max51x_attribute_group;
> +	data->indio_dev->dev_data = (void *)(data);
> +	data->indio_dev->driver_module = THIS_MODULE;
> +	data->indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	err = iio_device_register(data->indio_dev);
> +	if (err)
> +		goto exit_free_device;
> +
> +	dev_info(&client->dev, "DAC registered\n");
> +
> +	return 0;
> +
> +exit_free_device:
> +	iio_free_device(data->indio_dev);
> +exit_free_data:
> +	kfree(data);
> +exit:
> +	return err;
> +}
> +
> +static int max51x_remove(struct i2c_client *client)
> +{
> +	struct max51x_data *data = i2c_get_clientdata(client);
> +
Memory leak as in this path I can't see a call to iio_free_device
> +	kfree(data);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id max51x_id[] = {
> +	{ "max517", 0 },
> +	{ "max518", 0 },
> +	{ "max519", 0 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max51x_id);
> +
> +static struct i2c_driver max51x_driver = {
> +	.driver = {
> +		.name	= MAX51X_DRV_NAME,
> +	},
> +	.probe		= max51x_probe,
Check this builds with suspend support not built in... I think
the protective ifdef's are still needed.
> +	.remove		= max51x_remove,
> +	.suspend	= max51x_suspend,
> +	.resume		= max51x_resume,
> +	.id_table	= max51x_id,
> +};
> +
> +static int __init max51x_init(void)
> +{
> +	return i2c_add_driver(&max51x_driver);
> +}
> +
> +static void __exit max51x_exit(void)
> +{
> +	i2c_del_driver(&max51x_driver);
> +}
> +
> +MODULE_AUTHOR("Roland Stigge <stigge@antcom.de>");
> +MODULE_DESCRIPTION("MAX51X 8-bit DAC");
> +MODULE_LICENSE("GPL");
> +
> +module_init(max51x_init);
> +module_exit(max51x_exit);

Re: Linux driver for MAX517/518/519

[Roland Stigge]

Hi,

On 01/09/2011 11:31 PM, Jonathan Cameron wrote:
> Pretty clean and nice driver so it was an easy review and should
> be trivial to fix up for a merge.

Thank you for your review - I will include the suggested changes in the
next update.

> I don't think we have previously had a device that
> allows setting multiple inputs together.
> Two options come to mind that will generalize more
> than your _both.
>=20
> output1&2_raw
>=20
> output_raw (suppress the index hence indicating that it sets both).
>=20
> What do you think is the clearest approach?
> Which ever we pick it will also need proper documentation.  Whilst we
> are here, please can you explain your use case?  From a datasheet
> read I think the first channel is latched after the value byte is passe=
d
> then the second only after it's value has been passed over?

It's actually latched after the _complete_ transmission. See datasheet p.=
9:

"The data is transferred to the DAC=E2=80=99s output
latch during the STOP condition following the transmis-
sion. This allows both DACs of the MAX518/MAX519 to
be updated simultaneously."

I will also document this in the driver to make it clearer.

That's also why I'm constructing the I2C transfer in this nonstandard
way. Datasheet doesn't claim to support smbus, so I will change to the
_ic2_ (non _smbus_) interface.

bye,
  Roland

Re: Linux driver for MAX517/518/519

[Jonathan Cameron]

On 01/10/11 09:27, Roland Stigge wrote:
> Hi,
>=20
> On 01/09/2011 11:31 PM, Jonathan Cameron wrote:
>> Pretty clean and nice driver so it was an easy review and should
>> be trivial to fix up for a merge.
>=20
> Thank you for your review - I will include the suggested changes in t=
he
> next update.
>=20
>> I don't think we have previously had a device that
>> allows setting multiple inputs together.
>> Two options come to mind that will generalize more
>> than your _both.
>>
>> output1&2_raw
>>
>> output_raw (suppress the index hence indicating that it sets both).
>>
>> What do you think is the clearest approach?
>> Which ever we pick it will also need proper documentation.  Whilst w=
e
>> are here, please can you explain your use case?  From a datasheet
>> read I think the first channel is latched after the value byte is pa=
ssed
>> then the second only after it's value has been passed over?
>=20
> It's actually latched after the _complete_ transmission. See datashee=
t p.9:
>=20
> "The data is transferred to the DAC=E2=80=99s output
> latch during the STOP condition following the transmis-
> sion. This allows both DACs of the MAX518/MAX519 to
> be updated simultaneously."
Ah, I missed that bit of text.  I guess the label (DAC0 INPUT LATCH set=
 to full scale)
on figure 8b is just confusing then. Now you mention it, there is anoth=
er
label at the stop condition saying the outputs change at the end. I'm n=
ot entirely
sure why they differentiate between DAC input latching and DAC output l=
atching.
Surely people only care about the output.  Ah well, the delights of dat=
asheet
interpretation.

Thanks for clearing that up!=20
>=20
> I will also document this in the driver to make it clearer.
Thanks,
>=20
> That's also why I'm constructing the I2C transfer in this nonstandard
> way. Datasheet doesn't claim to support smbus, so I will change to th=
e
> _ic2_ (non _smbus_) interface.
Good idea. If anyone really needs to get it to work on an smbus only ad=
apter
we can put this back in, but until then readability is more important.
>=20
> bye,
>   Roland
>=20

Re: Linux driver for MAX517/518/519

[Roland Stigge]

[-- Attachment #1: Type: text/plain, Size: 587 bytes --]

Hi,

On 01/09/2011 11:31 PM, Jonathan Cameron wrote:
> Pretty clean and nice driver so it was an easy review and should
> be trivial to fix up for a merge.

Thanks for your review!

I'm attaching my update. Some notes:

* I used the attributes out1_* how I found it in the other DAC driver
already available
* suspend doesn't need the protective #ifdefs
* As for other drivers, drivers/staging/iio/dac/max517.h needs to go to
include/linux/iio as soon as available

I think I addressed everything from your notes. Let me know if there is
still some blocker.

Thanks in advance!

Roland


[-- Attachment #2: max517.patch --]
[-- Type: text/x-patch, Size: 11197 bytes --]

IIO Driver for Maxim MAX517, MAX518 and MAX519 DAC

Signed-off-by: Roland Stigge <stigge@antcom.de>
diff --git a/drivers/staging/iio/Documentation/dac/max517 b/drivers/staging/iio/Documentation/dac/max517
new file mode 100644
index 0000000..e60ec2f
--- /dev/null
+++ b/drivers/staging/iio/Documentation/dac/max517
@@ -0,0 +1,41 @@
+Kernel driver max517
+====================
+
+Supported chips:
+  * Maxim MAX517, MAX518, MAX519
+    Prefix: 'max517'
+    Datasheet: Publicly available at the Maxim website
+               http://www.maxim-ic.com/
+
+Author:
+        Roland Stigge <stigge@antcom.de>
+
+Description
+-----------
+
+The Maxim MAX517/518/519 is an 8-bit DAC on the I2C bus. The following table
+shows the different feature sets of the variants MAX517, MAX518 and MAX519:
+
+Feature                              MAX517 MAX518 MAX519
+--------------------------------------------------------------------------
+One output channel                   X
+Two output channels                         X      X
+Simultaneous output updates                 X      X
+Supply voltage as reference                 X
+Separate reference input             X
+Reference input for each DAC                       X
+
+Via the iio sysfs interface, there are three attributes available: out1_raw,
+out2_raw and out12_raw. With out1_raw and out2_raw, the current output values
+(0..255) of the DACs can be written to the device. out12_raw can be used to set
+both output channel values simultaneously.
+
+With MAX517, only out1_raw is available.
+
+Via out1_scale (and where appropriate, out2_scale), the current scaling factor
+in mV can be read.
+
+When the operating system goes to a power down state, the Power Down function
+of the chip is activated, reducing the supply current to 4uA.
+
+On power-up, the device is in 0V-output state.
diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
index 9191bd2..2120904 100644
--- a/drivers/staging/iio/dac/Kconfig
+++ b/drivers/staging/iio/dac/Kconfig
@@ -19,3 +19,13 @@ config AD5446
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad5446.
+
+config MAX517
+	tristate "Maxim MAX517/518/519 DAC driver"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the Maxim chips MAX517,
+	  MAX518 and MAX519 (I2C 8-Bit DACs with rail-to-rail outputs).
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called max517.
diff --git a/drivers/staging/iio/dac/Makefile b/drivers/staging/iio/dac/Makefile
index 7cf331b..1197aef 100644
--- a/drivers/staging/iio/dac/Makefile
+++ b/drivers/staging/iio/dac/Makefile
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_AD5624R_SPI) += ad5624r_spi.o
 obj-$(CONFIG_AD5446) += ad5446.o
+obj-$(CONFIG_MAX517) += max517.o
diff --git a/drivers/staging/iio/dac/max517.c b/drivers/staging/iio/dac/max517.c
new file mode 100644
index 0000000..5c57523
--- /dev/null
+++ b/drivers/staging/iio/dac/max517.c
@@ -0,0 +1,294 @@
+/*
+ *  max517.c - Support for Maxim MAX517, MAX518 and MAX519
+ *
+ *  Copyright (C) 2010, 2011 Roland Stigge <stigge@antcom.de>
+ *
+ *  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.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+
+#include "../iio.h"
+#include "dac.h"
+
+#include "max517.h"
+
+#define MAX517_DRV_NAME	"max517"
+
+/* Commands */
+#define COMMAND_CHANNEL0	0x00
+#define COMMAND_CHANNEL1	0x01 /* for MAX518 and MAX519 */
+#define COMMAND_PD		0x08 /* Power Down */
+
+enum max517_device_ids {
+	ID_MAX517,
+	ID_MAX518,
+	ID_MAX519,
+};
+
+struct max517_data {
+	struct iio_dev		*indio_dev;
+	unsigned short		vref_mv[2];
+};
+
+/*
+ * channel: bit 0: channel 1
+ *          bit 1: channel 2
+ * (this way, it's possible to set both channels at once)
+ */
+static ssize_t max517_set_value(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count, int channel)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	u8 outbuf[4]; /* 1x or 2x command + value */
+	int outbuf_size = 0;
+	int res;
+	long val;
+
+	res = strict_strtol(buf, 10, &val);
+
+	if (res)
+		return res;
+
+	if (val < 0 || val > 255)
+		return -EINVAL;
+
+	if (channel & 1) {
+		outbuf[outbuf_size++] = COMMAND_CHANNEL0;
+		outbuf[outbuf_size++] = val;
+	}
+	if (channel & 2) {
+		outbuf[outbuf_size++] = COMMAND_CHANNEL1;
+		outbuf[outbuf_size++] = val;
+	}
+
+	/*
+	 * At this point, there are always 1 or 2 two-byte commands in
+	 * outbuf. With 2 commands, the device can set two outputs
+	 * simultaneously, latching the values upon the end of the I2C
+	 * transfer.
+	 */
+
+	res = i2c_master_send(client, outbuf, outbuf_size);
+	if (res < 0)
+		return res;
+
+	return count;
+}
+
+static ssize_t max517_set_value_1(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	return max517_set_value(dev, attr, buf, count, 1);
+}
+static IIO_DEV_ATTR_OUT_RAW(1, max517_set_value_1, 0);
+
+static ssize_t max517_set_value_2(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	return max517_set_value(dev, attr, buf, count, 2);
+}
+static IIO_DEV_ATTR_OUT_RAW(2, max517_set_value_2, 1);
+
+static ssize_t max517_set_value_both(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	return max517_set_value(dev, attr, buf, count, 3);
+}
+static IIO_DEV_ATTR_OUT_RAW(12, max517_set_value_both, -1);
+
+static ssize_t max517_show_scale(struct device *dev,
+				struct device_attribute *attr,
+				char *buf, int channel)
+{
+	struct iio_dev *dev_info = dev_get_drvdata(dev);
+	struct max517_data *data = iio_dev_get_devdata(dev_info);
+	/* Corresponds to Vref / 2^(bits) */
+	unsigned int scale_uv = (data->vref_mv[channel - 1] * 1000) >> 8;
+
+	return sprintf(buf, "%d.%03d\n", scale_uv / 1000, scale_uv % 1000);
+}
+
+static ssize_t max517_show_scale1(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return max517_show_scale(dev, attr, buf, 1);
+}
+static IIO_DEVICE_ATTR(out1_scale, S_IRUGO, max517_show_scale1, NULL, 0);
+
+static ssize_t max517_show_scale2(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return max517_show_scale(dev, attr, buf, 2);
+}
+static IIO_DEVICE_ATTR(out2_scale, S_IRUGO, max517_show_scale2, NULL, 0);
+
+/* On MAX517 variant, we have two outputs */
+static struct attribute *max517_attributes[] = {
+	&iio_dev_attr_out1_raw.dev_attr.attr,
+	&iio_dev_attr_out1_scale.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group max517_attribute_group = {
+	.attrs = max517_attributes,
+};
+
+/* On MAX518 and MAX518 variant, we have two outputs */
+static struct attribute *max518_attributes[] = {
+	&iio_dev_attr_out1_raw.dev_attr.attr,
+	&iio_dev_attr_out1_scale.dev_attr.attr,
+	&iio_dev_attr_out2_raw.dev_attr.attr,
+	&iio_dev_attr_out2_scale.dev_attr.attr,
+	&iio_dev_attr_out12_raw.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group max518_attribute_group = {
+	.attrs = max518_attributes,
+};
+
+static int max517_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	u8 outbuf = COMMAND_PD;
+
+	return i2c_master_send(client, &outbuf, 1);
+}
+
+static int max517_resume(struct i2c_client *client)
+{
+	u8 outbuf = 0;
+
+	return i2c_master_send(client, &outbuf, 1);
+}
+
+static int max517_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct max517_data *data;
+	struct max517_platform_data *platform_data = client->dev.platform_data;
+	int err;
+
+	data = kzalloc(sizeof(struct max517_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	i2c_set_clientdata(client, data);
+
+	data->indio_dev = iio_allocate_device();
+	if (data->indio_dev == NULL) {
+		err = -ENOMEM;
+		goto exit_free_data;
+	}
+
+	/* establish that the iio_dev is a child of the i2c device */
+	data->indio_dev->dev.parent = &client->dev;
+
+	/* reduced attribute set for MAX517 */
+	if (id->driver_data == ID_MAX517)
+		data->indio_dev->attrs = &max517_attribute_group;
+	else
+		data->indio_dev->attrs = &max518_attribute_group;
+	data->indio_dev->dev_data = (void *)(data);
+	data->indio_dev->driver_module = THIS_MODULE;
+	data->indio_dev->modes = INDIO_DIRECT_MODE;
+
+	/*
+	 * Reference voltage on MAX518 is 5V, else take vref_mv from
+	 * platform_data
+	 */
+	if (id->driver_data == ID_MAX518)
+		data->vref_mv[0] = data->vref_mv[1] = 5000; /* mV */
+	else
+		if (platform_data) {
+			data->vref_mv[0] = platform_data->vref_mv[0];
+			data->vref_mv[1] = platform_data->vref_mv[1];
+		} else /* 5V by default */
+			data->vref_mv[0] = data->vref_mv[1] = 5000;
+
+	err = iio_device_register(data->indio_dev);
+	if (err)
+		goto exit_free_device;
+
+	dev_info(&client->dev, "DAC registered\n");
+
+	return 0;
+
+exit_free_device:
+	iio_free_device(data->indio_dev);
+exit_free_data:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int max517_remove(struct i2c_client *client)
+{
+	struct max517_data *data = i2c_get_clientdata(client);
+
+	iio_free_device(data->indio_dev);
+	kfree(data);
+
+	return 0;
+}
+
+static const struct i2c_device_id max517_id[] = {
+	{ "max517", ID_MAX517 },
+	{ "max518", ID_MAX518 },
+	{ "max519", ID_MAX519 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max517_id);
+
+static struct i2c_driver max517_driver = {
+	.driver = {
+		.name	= MAX517_DRV_NAME,
+	},
+	.probe		= max517_probe,
+	.remove		= max517_remove,
+	.suspend	= max517_suspend,
+	.resume		= max517_resume,
+	.id_table	= max517_id,
+};
+
+static int __init max517_init(void)
+{
+	return i2c_add_driver(&max517_driver);
+}
+
+static void __exit max517_exit(void)
+{
+	i2c_del_driver(&max517_driver);
+}
+
+MODULE_AUTHOR("Roland Stigge <stigge@antcom.de>");
+MODULE_DESCRIPTION("MAX517/MAX518/MAX519 8-bit DAC");
+MODULE_LICENSE("GPL");
+
+module_init(max517_init);
+module_exit(max517_exit);
diff --git a/drivers/staging/iio/dac/max517.h b/drivers/staging/iio/dac/max517.h
new file mode 100644
index 0000000..8106cf2
--- /dev/null
+++ b/drivers/staging/iio/dac/max517.h
@@ -0,0 +1,19 @@
+/*
+ * MAX517 DAC driver
+ *
+ * Copyright 2011 Roland Stigge <stigge@antcom.de>
+ *
+ * Licensed under the GPL-2 or later.
+ */
+#ifndef IIO_DAC_MAX517_H_
+#define IIO_DAC_MAX517_H_
+
+/*
+ * TODO: struct max517_platform_data needs to go into include/linux/iio
+ */
+
+struct max517_platform_data {
+	u16				vref_mv[2];
+};
+
+#endif /* IIO_DAC_MAX517_H_ */

Re: Linux driver for MAX517/518/519

[Jonathan Cameron]

On 01/11/11 16:20, Roland Stigge wrote:
> Hi,
> 
> On 01/09/2011 11:31 PM, Jonathan Cameron wrote:
>> Pretty clean and nice driver so it was an easy review and should
>> be trivial to fix up for a merge.
> 
> Thanks for your review!
> 
> I'm attaching my update. Some notes:
> 
> * I used the attributes out1_* how I found it in the other DAC driver
> already available
Cool, except for the out12_raw which could easily be the 12th channel.
This has to be one of the two options suggested the other day.
May require some macro magic. IIRC the relevant macro would be.

IIO_DEVICE_ATTR_NAMED(out1and2_raw, out1&2_raw, S_IWUSR, ... 

These sort of macros are probably too specific to want to put them
in the dac.h header so just keep it local to the driver.  It will
however want to be documented sometime soon.  Perhaps either you
or Michael could propose some suitable additions to sysfs-bus-iio-*
docs? If not I'll get to it at some point, but it may be a week or so.

> * suspend doesn't need the protective #ifdefs
> * As for other drivers, drivers/staging/iio/dac/max517.h needs to go to
> include/linux/iio as soon as available
Sure.
> 
> I think I addressed everything from your notes. Let me know if there is
> still some blocker.
> 
> Thanks in advance!
> 
> Roland
> 

Below ack is subject to fixing that one small interface issue.
Send a copy with that fixed on to Greg for merging. If you want
to do that as a separate follow up patch that would also be fine
with me.

> IIO Driver for Maxim MAX517, MAX518 and MAX519 DAC
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

> diff --git a/drivers/staging/iio/Documentation/dac/max517 b/drivers/staging/iio/Documentation/dac/max517
> new file mode 100644
> index 0000000..e60ec2f
> --- /dev/null
> +++ b/drivers/staging/iio/Documentation/dac/max517
> @@ -0,0 +1,41 @@
> +Kernel driver max517
> +====================
> +
> +Supported chips:
> +  * Maxim MAX517, MAX518, MAX519
> +    Prefix: 'max517'
> +    Datasheet: Publicly available at the Maxim website
> +               http://www.maxim-ic.com/
> +
> +Author:
> +        Roland Stigge <stigge@antcom.de>
> +
> +Description
> +-----------
> +
> +The Maxim MAX517/518/519 is an 8-bit DAC on the I2C bus. The following table
> +shows the different feature sets of the variants MAX517, MAX518 and MAX519:
> +
> +Feature                              MAX517 MAX518 MAX519
> +--------------------------------------------------------------------------
> +One output channel                   X
> +Two output channels                         X      X
> +Simultaneous output updates                 X      X
> +Supply voltage as reference                 X
> +Separate reference input             X
> +Reference input for each DAC                       X
> +
> +Via the iio sysfs interface, there are three attributes available: out1_raw,
> +out2_raw and out12_raw. With out1_raw and out2_raw, the current output values
> +(0..255) of the DACs can be written to the device. out12_raw can be used to set
> +both output channel values simultaneously.
> +
> +With MAX517, only out1_raw is available.
> +
> +Via out1_scale (and where appropriate, out2_scale), the current scaling factor
> +in mV can be read.
> +
> +When the operating system goes to a power down state, the Power Down function
> +of the chip is activated, reducing the supply current to 4uA.
> +
> +On power-up, the device is in 0V-output state.
> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
> index 9191bd2..2120904 100644
> --- a/drivers/staging/iio/dac/Kconfig
> +++ b/drivers/staging/iio/dac/Kconfig
> @@ -19,3 +19,13 @@ config AD5446
>  
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad5446.
> +
> +config MAX517
> +	tristate "Maxim MAX517/518/519 DAC driver"
> +	depends on I2C && EXPERIMENTAL
> +	help
> +	  If you say yes here you get support for the Maxim chips MAX517,
> +	  MAX518 and MAX519 (I2C 8-Bit DACs with rail-to-rail outputs).
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called max517.
> diff --git a/drivers/staging/iio/dac/Makefile b/drivers/staging/iio/dac/Makefile
> index 7cf331b..1197aef 100644
> --- a/drivers/staging/iio/dac/Makefile
> +++ b/drivers/staging/iio/dac/Makefile
> @@ -4,3 +4,4 @@
>  
>  obj-$(CONFIG_AD5624R_SPI) += ad5624r_spi.o
>  obj-$(CONFIG_AD5446) += ad5446.o
> +obj-$(CONFIG_MAX517) += max517.o
> diff --git a/drivers/staging/iio/dac/max517.c b/drivers/staging/iio/dac/max517.c
> new file mode 100644
> index 0000000..5c57523
> --- /dev/null
> +++ b/drivers/staging/iio/dac/max517.c
> @@ -0,0 +1,294 @@
> +/*
> + *  max517.c - Support for Maxim MAX517, MAX518 and MAX519
> + *
> + *  Copyright (C) 2010, 2011 Roland Stigge <stigge@antcom.de>
> + *
> + *  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.
> + *
> + *  You should have received a copy of the GNU General Public License
> + *  along with this program; if not, write to the Free Software
> + *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/jiffies.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +
> +#include "../iio.h"
> +#include "dac.h"
> +
> +#include "max517.h"
> +
> +#define MAX517_DRV_NAME	"max517"
> +
> +/* Commands */
> +#define COMMAND_CHANNEL0	0x00
> +#define COMMAND_CHANNEL1	0x01 /* for MAX518 and MAX519 */
> +#define COMMAND_PD		0x08 /* Power Down */
> +
> +enum max517_device_ids {
> +	ID_MAX517,
> +	ID_MAX518,
> +	ID_MAX519,
> +};
> +
> +struct max517_data {
> +	struct iio_dev		*indio_dev;
> +	unsigned short		vref_mv[2];
> +};
> +
> +/*
> + * channel: bit 0: channel 1
> + *          bit 1: channel 2
> + * (this way, it's possible to set both channels at once)
> + */
> +static ssize_t max517_set_value(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count, int channel)
> +{
> +	struct i2c_client *client = to_i2c_client(dev);
> +	u8 outbuf[4]; /* 1x or 2x command + value */
> +	int outbuf_size = 0;
> +	int res;
> +	long val;
> +
> +	res = strict_strtol(buf, 10, &val);
> +
> +	if (res)
> +		return res;
> +
> +	if (val < 0 || val > 255)
> +		return -EINVAL;
> +
> +	if (channel & 1) {
> +		outbuf[outbuf_size++] = COMMAND_CHANNEL0;
> +		outbuf[outbuf_size++] = val;
> +	}
> +	if (channel & 2) {
> +		outbuf[outbuf_size++] = COMMAND_CHANNEL1;
> +		outbuf[outbuf_size++] = val;
> +	}
> +
> +	/*
> +	 * At this point, there are always 1 or 2 two-byte commands in
> +	 * outbuf. With 2 commands, the device can set two outputs
> +	 * simultaneously, latching the values upon the end of the I2C
> +	 * transfer.
> +	 */
> +
> +	res = i2c_master_send(client, outbuf, outbuf_size);
> +	if (res < 0)
> +		return res;
> +
> +	return count;
> +}
> +
> +static ssize_t max517_set_value_1(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	return max517_set_value(dev, attr, buf, count, 1);
> +}
> +static IIO_DEV_ATTR_OUT_RAW(1, max517_set_value_1, 0);
> +
> +static ssize_t max517_set_value_2(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	return max517_set_value(dev, attr, buf, count, 2);
> +}
> +static IIO_DEV_ATTR_OUT_RAW(2, max517_set_value_2, 1);
> +
> +static ssize_t max517_set_value_both(struct device *dev,
> +				 struct device_attribute *attr,
> +				 const char *buf, size_t count)
> +{
> +	return max517_set_value(dev, attr, buf, count, 3);
> +}
> +static IIO_DEV_ATTR_OUT_RAW(12, max517_set_value_both, -1);
> +
> +static ssize_t max517_show_scale(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf, int channel)
> +{
> +	struct iio_dev *dev_info = dev_get_drvdata(dev);
> +	struct max517_data *data = iio_dev_get_devdata(dev_info);
> +	/* Corresponds to Vref / 2^(bits) */
> +	unsigned int scale_uv = (data->vref_mv[channel - 1] * 1000) >> 8;
> +
> +	return sprintf(buf, "%d.%03d\n", scale_uv / 1000, scale_uv % 1000);
> +}
> +
> +static ssize_t max517_show_scale1(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	return max517_show_scale(dev, attr, buf, 1);
> +}
> +static IIO_DEVICE_ATTR(out1_scale, S_IRUGO, max517_show_scale1, NULL, 0);
> +
> +static ssize_t max517_show_scale2(struct device *dev,
> +				struct device_attribute *attr,
> +				char *buf)
> +{
> +	return max517_show_scale(dev, attr, buf, 2);
> +}
> +static IIO_DEVICE_ATTR(out2_scale, S_IRUGO, max517_show_scale2, NULL, 0);
> +
> +/* On MAX517 variant, we have two outputs */
> +static struct attribute *max517_attributes[] = {
> +	&iio_dev_attr_out1_raw.dev_attr.attr,
> +	&iio_dev_attr_out1_scale.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group max517_attribute_group = {
> +	.attrs = max517_attributes,
> +};
> +
> +/* On MAX518 and MAX518 variant, we have two outputs */
> +static struct attribute *max518_attributes[] = {
> +	&iio_dev_attr_out1_raw.dev_attr.attr,
> +	&iio_dev_attr_out1_scale.dev_attr.attr,
> +	&iio_dev_attr_out2_raw.dev_attr.attr,
> +	&iio_dev_attr_out2_scale.dev_attr.attr,
> +	&iio_dev_attr_out12_raw.dev_attr.attr,
> +	NULL
> +};
> +
> +static struct attribute_group max518_attribute_group = {
> +	.attrs = max518_attributes,
> +};
> +
> +static int max517_suspend(struct i2c_client *client, pm_message_t mesg)
> +{
> +	u8 outbuf = COMMAND_PD;
> +
> +	return i2c_master_send(client, &outbuf, 1);
> +}
> +
> +static int max517_resume(struct i2c_client *client)
> +{
> +	u8 outbuf = 0;
> +
> +	return i2c_master_send(client, &outbuf, 1);
> +}
> +
> +static int max517_probe(struct i2c_client *client,
> +			const struct i2c_device_id *id)
> +{
> +	struct max517_data *data;
> +	struct max517_platform_data *platform_data = client->dev.platform_data;
> +	int err;
> +
> +	data = kzalloc(sizeof(struct max517_data), GFP_KERNEL);
> +	if (!data) {
> +		err = -ENOMEM;
> +		goto exit;
> +	}
> +
> +	i2c_set_clientdata(client, data);
> +
> +	data->indio_dev = iio_allocate_device();
> +	if (data->indio_dev == NULL) {
> +		err = -ENOMEM;
> +		goto exit_free_data;
> +	}
> +
> +	/* establish that the iio_dev is a child of the i2c device */
> +	data->indio_dev->dev.parent = &client->dev;
> +
> +	/* reduced attribute set for MAX517 */
> +	if (id->driver_data == ID_MAX517)
> +		data->indio_dev->attrs = &max517_attribute_group;
> +	else
> +		data->indio_dev->attrs = &max518_attribute_group;
> +	data->indio_dev->dev_data = (void *)(data);
> +	data->indio_dev->driver_module = THIS_MODULE;
> +	data->indio_dev->modes = INDIO_DIRECT_MODE;
> +
> +	/*
> +	 * Reference voltage on MAX518 is 5V, else take vref_mv from
> +	 * platform_data
> +	 */
Could munge this into slightly neater/shorter form with
if (id->driver_data == ID_MAX518 || !platform_data) {
	data->vref_mv[0] = data->vref_mv[1] = 5000; /* mV */
} else {
	data->vref_mv[0] = platform_data->vref_mv[0];
	data->vref_mv[1] = platform_data->vref_mv[1];
}
> +	if (id->driver_data == ID_MAX518)
> +		data->vref_mv[0] = data->vref_mv[1] = 5000; /* mV */
> +	else
> +		if (platform_data) {
> +			data->vref_mv[0] = platform_data->vref_mv[0];
> +			data->vref_mv[1] = platform_data->vref_mv[1];
> +		} else /* 5V by default */
> +			data->vref_mv[0] = data->vref_mv[1] = 5000;
> +
> +	err = iio_device_register(data->indio_dev);
> +	if (err)
> +		goto exit_free_device;
> +
> +	dev_info(&client->dev, "DAC registered\n");
> +
> +	return 0;
> +
> +exit_free_device:
> +	iio_free_device(data->indio_dev);
> +exit_free_data:
> +	kfree(data);
> +exit:
> +	return err;
> +}
> +
> +static int max517_remove(struct i2c_client *client)
> +{
> +	struct max517_data *data = i2c_get_clientdata(client);
> +
> +	iio_free_device(data->indio_dev);
> +	kfree(data);
> +
> +	return 0;
> +}
> +
> +static const struct i2c_device_id max517_id[] = {
> +	{ "max517", ID_MAX517 },
> +	{ "max518", ID_MAX518 },
> +	{ "max519", ID_MAX519 },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(i2c, max517_id);
> +
> +static struct i2c_driver max517_driver = {
> +	.driver = {
> +		.name	= MAX517_DRV_NAME,
> +	},
> +	.probe		= max517_probe,
> +	.remove		= max517_remove,
> +	.suspend	= max517_suspend,
> +	.resume		= max517_resume,
> +	.id_table	= max517_id,
> +};
> +
> +static int __init max517_init(void)
> +{
> +	return i2c_add_driver(&max517_driver);
> +}
> +
> +static void __exit max517_exit(void)
> +{
> +	i2c_del_driver(&max517_driver);
> +}
> +
> +MODULE_AUTHOR("Roland Stigge <stigge@antcom.de>");
> +MODULE_DESCRIPTION("MAX517/MAX518/MAX519 8-bit DAC");
> +MODULE_LICENSE("GPL");
> +
> +module_init(max517_init);
> +module_exit(max517_exit);
> diff --git a/drivers/staging/iio/dac/max517.h b/drivers/staging/iio/dac/max517.h
> new file mode 100644
> index 0000000..8106cf2
> --- /dev/null
> +++ b/drivers/staging/iio/dac/max517.h
> @@ -0,0 +1,19 @@
> +/*
> + * MAX517 DAC driver
> + *
> + * Copyright 2011 Roland Stigge <stigge@antcom.de>
> + *
> + * Licensed under the GPL-2 or later.
> + */
> +#ifndef IIO_DAC_MAX517_H_
> +#define IIO_DAC_MAX517_H_
> +
> +/*
> + * TODO: struct max517_platform_data needs to go into include/linux/iio
> + */
> +
> +struct max517_platform_data {
> +	u16				vref_mv[2];
> +};
> +
> +#endif /* IIO_DAC_MAX517_H_ */

Re: Linux driver for MAX517/518/519

[Roland Stigge]

[-- Attachment #1: Type: text/plain, Size: 704 bytes --]

Hi,

On 01/11/2011 08:34 PM, Jonathan Cameron wrote:
>> On 01/09/2011 11:31 PM, Jonathan Cameron wrote:
>>> Pretty clean and nice driver so it was an easy review and should
>>> be trivial to fix up for a merge.
>>
>> Thanks for your review!
>>
>> I'm attaching my update. Some notes:
>>
>> * I used the attributes out1_* how I found it in the other DAC driver
>> already available
> Cool, except for the out12_raw which could easily be the 12th channel.
> This has to be one of the two options suggested the other day.
> May require some macro magic. IIRC the relevant macro would be.
> 
> IIO_DEVICE_ATTR_NAMED(out1and2_raw, out1&2_raw, S_IWUSR, ... 

Update attached as suggested. :-)

Thanks!

Roland

[-- Attachment #2: max517.patch --]
[-- Type: text/x-patch, Size: 11223 bytes --]

IIO Driver for Maxim MAX517, MAX518 and MAX519 DAC

Signed-off-by: Roland Stigge <stigge@antcom.de>
Acked-by: Jonathan Cameron <jic23@cam.ac.uk>

diff --git a/drivers/staging/iio/Documentation/dac/max517 b/drivers/staging/iio/Documentation/dac/max517
new file mode 100644
index 0000000..e60ec2f
--- /dev/null
+++ b/drivers/staging/iio/Documentation/dac/max517
@@ -0,0 +1,41 @@
+Kernel driver max517
+====================
+
+Supported chips:
+  * Maxim MAX517, MAX518, MAX519
+    Prefix: 'max517'
+    Datasheet: Publicly available at the Maxim website
+               http://www.maxim-ic.com/
+
+Author:
+        Roland Stigge <stigge@antcom.de>
+
+Description
+-----------
+
+The Maxim MAX517/518/519 is an 8-bit DAC on the I2C bus. The following table
+shows the different feature sets of the variants MAX517, MAX518 and MAX519:
+
+Feature                              MAX517 MAX518 MAX519
+--------------------------------------------------------------------------
+One output channel                   X
+Two output channels                         X      X
+Simultaneous output updates                 X      X
+Supply voltage as reference                 X
+Separate reference input             X
+Reference input for each DAC                       X
+
+Via the iio sysfs interface, there are three attributes available: out1_raw,
+out2_raw and out12_raw. With out1_raw and out2_raw, the current output values
+(0..255) of the DACs can be written to the device. out12_raw can be used to set
+both output channel values simultaneously.
+
+With MAX517, only out1_raw is available.
+
+Via out1_scale (and where appropriate, out2_scale), the current scaling factor
+in mV can be read.
+
+When the operating system goes to a power down state, the Power Down function
+of the chip is activated, reducing the supply current to 4uA.
+
+On power-up, the device is in 0V-output state.
diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
index 9191bd2..2120904 100644
--- a/drivers/staging/iio/dac/Kconfig
+++ b/drivers/staging/iio/dac/Kconfig
@@ -19,3 +19,13 @@ config AD5446
 
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad5446.
+
+config MAX517
+	tristate "Maxim MAX517/518/519 DAC driver"
+	depends on I2C && EXPERIMENTAL
+	help
+	  If you say yes here you get support for the Maxim chips MAX517,
+	  MAX518 and MAX519 (I2C 8-Bit DACs with rail-to-rail outputs).
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called max517.
diff --git a/drivers/staging/iio/dac/Makefile b/drivers/staging/iio/dac/Makefile
index 7cf331b..1197aef 100644
--- a/drivers/staging/iio/dac/Makefile
+++ b/drivers/staging/iio/dac/Makefile
@@ -4,3 +4,4 @@
 
 obj-$(CONFIG_AD5624R_SPI) += ad5624r_spi.o
 obj-$(CONFIG_AD5446) += ad5446.o
+obj-$(CONFIG_MAX517) += max517.o
diff --git a/drivers/staging/iio/dac/max517.c b/drivers/staging/iio/dac/max517.c
new file mode 100644
index 0000000..4974e70
--- /dev/null
+++ b/drivers/staging/iio/dac/max517.c
@@ -0,0 +1,293 @@
+/*
+ *  max517.c - Support for Maxim MAX517, MAX518 and MAX519
+ *
+ *  Copyright (C) 2010, 2011 Roland Stigge <stigge@antcom.de>
+ *
+ *  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.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ */
+
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/slab.h>
+#include <linux/jiffies.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+
+#include "../iio.h"
+#include "dac.h"
+
+#include "max517.h"
+
+#define MAX517_DRV_NAME	"max517"
+
+/* Commands */
+#define COMMAND_CHANNEL0	0x00
+#define COMMAND_CHANNEL1	0x01 /* for MAX518 and MAX519 */
+#define COMMAND_PD		0x08 /* Power Down */
+
+enum max517_device_ids {
+	ID_MAX517,
+	ID_MAX518,
+	ID_MAX519,
+};
+
+struct max517_data {
+	struct iio_dev		*indio_dev;
+	unsigned short		vref_mv[2];
+};
+
+/*
+ * channel: bit 0: channel 1
+ *          bit 1: channel 2
+ * (this way, it's possible to set both channels at once)
+ */
+static ssize_t max517_set_value(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count, int channel)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	u8 outbuf[4]; /* 1x or 2x command + value */
+	int outbuf_size = 0;
+	int res;
+	long val;
+
+	res = strict_strtol(buf, 10, &val);
+
+	if (res)
+		return res;
+
+	if (val < 0 || val > 255)
+		return -EINVAL;
+
+	if (channel & 1) {
+		outbuf[outbuf_size++] = COMMAND_CHANNEL0;
+		outbuf[outbuf_size++] = val;
+	}
+	if (channel & 2) {
+		outbuf[outbuf_size++] = COMMAND_CHANNEL1;
+		outbuf[outbuf_size++] = val;
+	}
+
+	/*
+	 * At this point, there are always 1 or 2 two-byte commands in
+	 * outbuf. With 2 commands, the device can set two outputs
+	 * simultaneously, latching the values upon the end of the I2C
+	 * transfer.
+	 */
+
+	res = i2c_master_send(client, outbuf, outbuf_size);
+	if (res < 0)
+		return res;
+
+	return count;
+}
+
+static ssize_t max517_set_value_1(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	return max517_set_value(dev, attr, buf, count, 1);
+}
+static IIO_DEV_ATTR_OUT_RAW(1, max517_set_value_1, 0);
+
+static ssize_t max517_set_value_2(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	return max517_set_value(dev, attr, buf, count, 2);
+}
+static IIO_DEV_ATTR_OUT_RAW(2, max517_set_value_2, 1);
+
+static ssize_t max517_set_value_both(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t count)
+{
+	return max517_set_value(dev, attr, buf, count, 3);
+}
+static IIO_DEVICE_ATTR_NAMED(out1and2_raw, out1&2_raw, S_IWUSR, NULL,
+		max517_set_value_both, -1);
+
+static ssize_t max517_show_scale(struct device *dev,
+				struct device_attribute *attr,
+				char *buf, int channel)
+{
+	struct iio_dev *dev_info = dev_get_drvdata(dev);
+	struct max517_data *data = iio_dev_get_devdata(dev_info);
+	/* Corresponds to Vref / 2^(bits) */
+	unsigned int scale_uv = (data->vref_mv[channel - 1] * 1000) >> 8;
+
+	return sprintf(buf, "%d.%03d\n", scale_uv / 1000, scale_uv % 1000);
+}
+
+static ssize_t max517_show_scale1(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return max517_show_scale(dev, attr, buf, 1);
+}
+static IIO_DEVICE_ATTR(out1_scale, S_IRUGO, max517_show_scale1, NULL, 0);
+
+static ssize_t max517_show_scale2(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	return max517_show_scale(dev, attr, buf, 2);
+}
+static IIO_DEVICE_ATTR(out2_scale, S_IRUGO, max517_show_scale2, NULL, 0);
+
+/* On MAX517 variant, we have two outputs */
+static struct attribute *max517_attributes[] = {
+	&iio_dev_attr_out1_raw.dev_attr.attr,
+	&iio_dev_attr_out1_scale.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group max517_attribute_group = {
+	.attrs = max517_attributes,
+};
+
+/* On MAX518 and MAX518 variant, we have two outputs */
+static struct attribute *max518_attributes[] = {
+	&iio_dev_attr_out1_raw.dev_attr.attr,
+	&iio_dev_attr_out1_scale.dev_attr.attr,
+	&iio_dev_attr_out2_raw.dev_attr.attr,
+	&iio_dev_attr_out2_scale.dev_attr.attr,
+	&iio_dev_attr_out1and2_raw.dev_attr.attr,
+	NULL
+};
+
+static struct attribute_group max518_attribute_group = {
+	.attrs = max518_attributes,
+};
+
+static int max517_suspend(struct i2c_client *client, pm_message_t mesg)
+{
+	u8 outbuf = COMMAND_PD;
+
+	return i2c_master_send(client, &outbuf, 1);
+}
+
+static int max517_resume(struct i2c_client *client)
+{
+	u8 outbuf = 0;
+
+	return i2c_master_send(client, &outbuf, 1);
+}
+
+static int max517_probe(struct i2c_client *client,
+			const struct i2c_device_id *id)
+{
+	struct max517_data *data;
+	struct max517_platform_data *platform_data = client->dev.platform_data;
+	int err;
+
+	data = kzalloc(sizeof(struct max517_data), GFP_KERNEL);
+	if (!data) {
+		err = -ENOMEM;
+		goto exit;
+	}
+
+	i2c_set_clientdata(client, data);
+
+	data->indio_dev = iio_allocate_device();
+	if (data->indio_dev == NULL) {
+		err = -ENOMEM;
+		goto exit_free_data;
+	}
+
+	/* establish that the iio_dev is a child of the i2c device */
+	data->indio_dev->dev.parent = &client->dev;
+
+	/* reduced attribute set for MAX517 */
+	if (id->driver_data == ID_MAX517)
+		data->indio_dev->attrs = &max517_attribute_group;
+	else
+		data->indio_dev->attrs = &max518_attribute_group;
+	data->indio_dev->dev_data = (void *)(data);
+	data->indio_dev->driver_module = THIS_MODULE;
+	data->indio_dev->modes = INDIO_DIRECT_MODE;
+
+	/*
+	 * Reference voltage on MAX518 and default is 5V, else take vref_mv
+	 * from platform_data
+	 */
+	if (id->driver_data == ID_MAX518 || !platform_data) {
+		data->vref_mv[0] = data->vref_mv[1] = 5000; /* mV */
+	} else {
+		data->vref_mv[0] = platform_data->vref_mv[0];
+		data->vref_mv[1] = platform_data->vref_mv[1];
+	}
+
+	err = iio_device_register(data->indio_dev);
+	if (err)
+		goto exit_free_device;
+
+	dev_info(&client->dev, "DAC registered\n");
+
+	return 0;
+
+exit_free_device:
+	iio_free_device(data->indio_dev);
+exit_free_data:
+	kfree(data);
+exit:
+	return err;
+}
+
+static int max517_remove(struct i2c_client *client)
+{
+	struct max517_data *data = i2c_get_clientdata(client);
+
+	iio_free_device(data->indio_dev);
+	kfree(data);
+
+	return 0;
+}
+
+static const struct i2c_device_id max517_id[] = {
+	{ "max517", ID_MAX517 },
+	{ "max518", ID_MAX518 },
+	{ "max519", ID_MAX519 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, max517_id);
+
+static struct i2c_driver max517_driver = {
+	.driver = {
+		.name	= MAX517_DRV_NAME,
+	},
+	.probe		= max517_probe,
+	.remove		= max517_remove,
+	.suspend	= max517_suspend,
+	.resume		= max517_resume,
+	.id_table	= max517_id,
+};
+
+static int __init max517_init(void)
+{
+	return i2c_add_driver(&max517_driver);
+}
+
+static void __exit max517_exit(void)
+{
+	i2c_del_driver(&max517_driver);
+}
+
+MODULE_AUTHOR("Roland Stigge <stigge@antcom.de>");
+MODULE_DESCRIPTION("MAX517/MAX518/MAX519 8-bit DAC");
+MODULE_LICENSE("GPL");
+
+module_init(max517_init);
+module_exit(max517_exit);
diff --git a/drivers/staging/iio/dac/max517.h b/drivers/staging/iio/dac/max517.h
new file mode 100644
index 0000000..8106cf2
--- /dev/null
+++ b/drivers/staging/iio/dac/max517.h
@@ -0,0 +1,19 @@
+/*
+ * MAX517 DAC driver
+ *
+ * Copyright 2011 Roland Stigge <stigge@antcom.de>
+ *
+ * Licensed under the GPL-2 or later.
+ */
+#ifndef IIO_DAC_MAX517_H_
+#define IIO_DAC_MAX517_H_
+
+/*
+ * TODO: struct max517_platform_data needs to go into include/linux/iio
+ */
+
+struct max517_platform_data {
+	u16				vref_mv[2];
+};
+
+#endif /* IIO_DAC_MAX517_H_ */

Re: Linux driver for MAX517/518/519

[Jonathan Cameron]

On 01/12/11 09:44, Roland Stigge wrote:
> Hi,
> 
> On 01/11/2011 08:34 PM, Jonathan Cameron wrote:
>>> On 01/09/2011 11:31 PM, Jonathan Cameron wrote:
>>>> Pretty clean and nice driver so it was an easy review and should
>>>> be trivial to fix up for a merge.
>>>
>>> Thanks for your review!
>>>
>>> I'm attaching my update. Some notes:
>>>
>>> * I used the attributes out1_* how I found it in the other DAC driver
>>> already available
>> Cool, except for the out12_raw which could easily be the 12th channel.
>> This has to be one of the two options suggested the other day.
>> May require some macro magic. IIRC the relevant macro would be.
>>
>> IIO_DEVICE_ATTR_NAMED(out1and2_raw, out1&2_raw, S_IWUSR, ... 
> 
> Update attached as suggested. :-)
> 
> Thanks!
> 
> Roland

Probably worth sending it to Greg KH in a new thread so that he
picks up on the fact that it is ready to merge. Given the volume
of email he probably gets I doubt he is still reading this thread!
No need to cc that to the list given we have this email.

Thanks,

Jonathan

Re: Linux driver for MAX517/518/519

[Greg KH]

On Wed, Jan 12, 2011 at 11:20:38AM +0000, Jonathan Cameron wrote:
> On 01/12/11 09:44, Roland Stigge wrote:
> > Hi,
> > 
> > On 01/11/2011 08:34 PM, Jonathan Cameron wrote:
> >>> On 01/09/2011 11:31 PM, Jonathan Cameron wrote:
> >>>> Pretty clean and nice driver so it was an easy review and should
> >>>> be trivial to fix up for a merge.
> >>>
> >>> Thanks for your review!
> >>>
> >>> I'm attaching my update. Some notes:
> >>>
> >>> * I used the attributes out1_* how I found it in the other DAC driver
> >>> already available
> >> Cool, except for the out12_raw which could easily be the 12th channel.
> >> This has to be one of the two options suggested the other day.
> >> May require some macro magic. IIRC the relevant macro would be.
> >>
> >> IIO_DEVICE_ATTR_NAMED(out1and2_raw, out1&2_raw, S_IWUSR, ... 
> > 
> > Update attached as suggested. :-)
> > 
> > Thanks!
> > 
> > Roland
> 
> Probably worth sending it to Greg KH in a new thread so that he
> picks up on the fact that it is ready to merge. Given the volume
> of email he probably gets I doubt he is still reading this thread!
> No need to cc that to the list given we have this email.

Heh, I'm still here reading this :)

Roland sent me an updated version, I'll queue that up after .38-rc1 is
out.

thanks,

greg k-h

Re: Linux driver for MAX517/518/519

[Jonathan Cameron]

On 01/12/11 15:34, Greg KH wrote:
> On Wed, Jan 12, 2011 at 11:20:38AM +0000, Jonathan Cameron wrote:
>> On 01/12/11 09:44, Roland Stigge wrote:
>>> Hi,
>>>
>>> On 01/11/2011 08:34 PM, Jonathan Cameron wrote:
>>>>> On 01/09/2011 11:31 PM, Jonathan Cameron wrote:
>>>>>> Pretty clean and nice driver so it was an easy review and should
>>>>>> be trivial to fix up for a merge.
>>>>>
>>>>> Thanks for your review!
>>>>>
>>>>> I'm attaching my update. Some notes:
>>>>>
>>>>> * I used the attributes out1_* how I found it in the other DAC driver
>>>>> already available
>>>> Cool, except for the out12_raw which could easily be the 12th channel.
>>>> This has to be one of the two options suggested the other day.
>>>> May require some macro magic. IIRC the relevant macro would be.
>>>>
>>>> IIO_DEVICE_ATTR_NAMED(out1and2_raw, out1&2_raw, S_IWUSR, ... 
>>>
>>> Update attached as suggested. :-)
>>>
>>> Thanks!
>>>
>>> Roland
>>
>> Probably worth sending it to Greg KH in a new thread so that he
>> picks up on the fact that it is ready to merge. Given the volume
>> of email he probably gets I doubt he is still reading this thread!
>> No need to cc that to the list given we have this email.
> 
> Heh, I'm still here reading this :)
> 
> Roland sent me an updated version, I'll queue that up after .38-rc1 is
> out.
> 
> thanks,
> 
> greg k-h
Thanks. I'm seriously impressed at the number of mails you
must read every day :)

Jonathan
 

Re: Linux driver for MAX517/518/519

[Greg KH]

On Wed, Jan 12, 2011 at 03:44:41PM +0000, Jonathan Cameron wrote:
> > Heh, I'm still here reading this :)
> > 
> > Roland sent me an updated version, I'll queue that up after .38-rc1 is
> > out.
> > 
> > thanks,
> > 
> > greg k-h
> Thanks. I'm seriously impressed at the number of mails you
> must read every day :)

See:
	http://www.kroah.com/log/linux/get_lots_of_email.html
for some details as to my email load on a "slow" day...

I'm sure most other kernel subsystem maintainers have the same load.

thanks,

greg k-h

Re: Linux driver for MAX517/518/519

[Roland Stigge]

Hi,

On 03/01/11 23:28, Greg KH wrote:
> It's best to convert to the iio subsystem, as that is the way to go in
> the future.

I'm also preparing a plain PWM driver for the LPC3250 SoC.

Would IIO be an appropriate place?

Thanks in advance,

Roland

Re: Linux driver for MAX517/518/519

[J.I. Cameron]

On Jan 9 2011, Roland Stigge wrote:

>Hi,
>
>On 03/01/11 23:28, Greg KH wrote:
>> It's best to convert to the iio subsystem, as that is the way to go in
>> the future.
>
>I'm also preparing a plain PWM driver for the LPC3250 SoC.
>
>Would IIO be an appropriate place?
No. There is a pwm subsystem. I think thee actual drivers tend to go in the 
relevant arch directory. So far I think all of the are all arch specific. 
We do have some recently added support for direct digital synthesis (from 
Analog devices). There's obviously some common functionality, though the 
DDS devices tend to be for modulation purposes. Obviously pwm's have loads 
of uses, but tend to only need a very simple interface. I have wondered if 
we ought to support this as part of the dds interface, but so far I don't 
think we have a device anyone would actually use as simply a pwm.

Anyhow, see include/linux/pwm.h for pwm interface. Examples of 
implementations in arch/arm/plat-pxa and a few others in arch arm. Not that 
many users yet based on a quick grep.

Anyhow, the code in place looks to have come from Russell King (arm 
maintainer). I know Bill Gatliff proposed something similar... 
http://lists.openwall.net/linux-kernel/2010/02/02/353

Hope that helps. Will take a look at ADC driver sometime tomorrow with
a bit of luck.

Thanks,

Jonathan

>
>Thanks in advance,
>
>Roland
>--
>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
>

Re: Linux driver for MAX517/518/519

[J.I. Cameron]

On Jan 9 2011, J.I. Cameron wrote:

>On Jan 9 2011, Roland Stigge wrote:
>
>>Hi,
>>
>>On 03/01/11 23:28, Greg KH wrote:
>>> It's best to convert to the iio subsystem, as that is the way to go in
>>> the future.
>>
>>I'm also preparing a plain PWM driver for the LPC3250 SoC.
>>
>>Would IIO be an appropriate place?
> No. There is a pwm subsystem. I think thee actual drivers tend to go in 
> the relevant arch directory. So far I think all of the are all arch 
> specific. We do have some recently added support for direct digital 
> synthesis (from Analog devices). There's obviously some common 
> functionality, though the DDS devices tend to be for modulation purposes. 
> Obviously pwm's have loads of uses, but tend to only need a very simple 
> interface. I have wondered if we ought to support this as part of the dds 
> interface, but so far I don't think we have a device anyone would 
> actually use as simply a pwm.
>
> Anyhow, see include/linux/pwm.h for pwm interface. Examples of 
> implementations in arch/arm/plat-pxa and a few others in arch arm. Not 
> that many users yet based on a quick grep.
>
>Anyhow, the code in place looks to have come from Russell King (arm 
>maintainer). I know Bill Gatliff proposed something similar... 
>http://lists.openwall.net/linux-kernel/2010/02/02/353
>
>Hope that helps. Will take a look at ADC driver sometime tomorrow with
>a bit of luck.
Doh! DAC...  
>
>Thanks,
>
>Jonathan
>
>>
>>Thanks in advance,
>>
>>Roland
>>--
>>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
>>
>

Re: Linux driver for MAX517/518/519

[Roland Stigge]

On 09/01/11 22:05, J.I. Cameron wrote:
>> Hope that helps. Will take a look at ADC driver sometime tomorrow with
>> a bit of luck.
> Doh! DAC... 

Yes, that was my original reason for asking here - the hwmon maintainer
and Greg KH suggested to port my DAC driver as part of the IIO subsystem.

Thanks,

Roland