[PATCH] IIO: DAC: New driver for the AD5504 and AD5501 High Voltage DACs

[PATCH] IIO: DAC: New driver for the AD5504 and AD5501 High Voltage DACs

[michael.hennerich]

From: Michael Hennerich <michael.hennerich@analog.com>


Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
---
 drivers/staging/iio/dac/Kconfig  |   10 +
 drivers/staging/iio/dac/Makefile |    1 +
 drivers/staging/iio/dac/ad5504.c |  429 ++++++++++++++++++++++++++++++++++++++
 drivers/staging/iio/dac/ad5504.h |   74 +++++++
 drivers/staging/iio/sysfs.h      |    1 +
 5 files changed, 515 insertions(+), 0 deletions(-)
 create mode 100644 drivers/staging/iio/dac/ad5504.c
 create mode 100644 drivers/staging/iio/dac/ad5504.h

diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
index 67defcb..1b0188a 100644
--- a/drivers/staging/iio/dac/Kconfig
+++ b/drivers/staging/iio/dac/Kconfig
@@ -21,6 +21,16 @@ config AD5446
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad5446.
 
+config AD5504
+	tristate "Analog Devices AD5504/AD5501 DAC SPI driver"
+	depends on SPI
+	help
+	  Say yes here to build support for Analog Devices AD5504, AD5501,
+	  High Voltage Digital to Analog Converter.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad5504.
+
 config MAX517
 	tristate "Maxim MAX517/518/519 DAC driver"
 	depends on I2C && EXPERIMENTAL
diff --git a/drivers/staging/iio/dac/Makefile b/drivers/staging/iio/dac/Makefile
index 1197aef..020df4a 100644
--- a/drivers/staging/iio/dac/Makefile
+++ b/drivers/staging/iio/dac/Makefile
@@ -3,5 +3,6 @@
 #
 
 obj-$(CONFIG_AD5624R_SPI) += ad5624r_spi.o
+obj-$(CONFIG_AD5504) += ad5504.o
 obj-$(CONFIG_AD5446) += ad5446.o
 obj-$(CONFIG_MAX517) += max517.o
diff --git a/drivers/staging/iio/dac/ad5504.c b/drivers/staging/iio/dac/ad5504.c
new file mode 100644
index 0000000..0c6f2dc
--- /dev/null
+++ b/drivers/staging/iio/dac/ad5504.c
@@ -0,0 +1,429 @@
+/*
+ * AD5504, AD5501 High Voltage Digital to Analog Converter
+ *
+ * Copyright 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/gpio.h>
+#include <linux/fs.h>
+#include <linux/device.h>
+#include <linux/kernel.h>
+#include <linux/spi/spi.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+#include <linux/regulator/consumer.h>
+
+#include "../iio.h"
+#include "../sysfs.h"
+#include "dac.h"
+#include "ad5504.h"
+
+static int ad5504_spi_write(struct spi_device *spi, u8 addr, u16 val)
+{
+	u16 tmp = cpu_to_be16(AD5504_CMD_WRITE |
+			      AD5504_ADDR(addr) |
+			      (val & AD5504_RES_MASK));
+
+	return spi_write(spi, (u8 *)&tmp, 2);
+}
+
+static int ad5504_spi_read(struct spi_device *spi, u8 addr, u16 *val)
+{
+	u16 tmp = cpu_to_be16(AD5504_CMD_READ | AD5504_ADDR(addr));
+	int ret;
+	struct spi_transfer	t = {
+			.tx_buf		= &tmp,
+			.rx_buf		= val,
+			.len		= 2,
+		};
+	struct spi_message	m;
+
+	spi_message_init(&m);
+	spi_message_add_tail(&t, &m);
+	ret = spi_sync(spi, &m);
+
+	*val = be16_to_cpu(*val) & AD5504_RES_MASK;
+
+	return ret;
+}
+
+static ssize_t ad5504_write_dac(struct device *dev,
+				 struct device_attribute *attr,
+				 const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	long readin;
+	int ret;
+
+	ret = strict_strtol(buf, 10, &readin);
+	if (ret)
+		return ret;
+
+	ret = ad5504_spi_write(st->spi, this_attr->address, readin);
+	return ret ? ret : len;
+}
+
+static ssize_t ad5504_read_dac(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+	int ret;
+	u16 val;
+
+	ret = ad5504_spi_read(st->spi, this_attr->address, &val);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "%d\n", val);
+}
+
+static ssize_t ad5504_read_powerdown_mode(struct device *dev,
+				      struct device_attribute *attr, char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
+
+	char mode[][15] = {"20kohm_to_gnd", "three_state"};
+
+	return sprintf(buf, "%s\n", mode[st->pwr_down_mode]);
+}
+
+static ssize_t ad5504_write_powerdown_mode(struct device *dev,
+				       struct device_attribute *attr,
+				       const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
+	int ret;
+
+	if (sysfs_streq(buf, "20kohm_to_gnd"))
+		st->pwr_down_mode = AD5504_DAC_PWRDN_20K;
+	else if (sysfs_streq(buf, "three_state"))
+		st->pwr_down_mode = AD5504_DAC_PWRDN_3STATE;
+	else
+		ret = -EINVAL;
+
+	return ret ? ret : len;
+}
+
+static ssize_t ad5504_read_dac_powerdown(struct device *dev,
+					   struct device_attribute *attr,
+					   char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+
+	return sprintf(buf, "%d\n",
+			!!(st->pwr_down_mask & (1 << this_attr->address)));
+}
+
+static ssize_t ad5504_write_dac_powerdown(struct device *dev,
+					    struct device_attribute *attr,
+					    const char *buf, size_t len)
+{
+	long readin;
+	int ret;
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
+	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
+
+	ret = strict_strtol(buf, 10, &readin);
+	if (ret)
+		return ret;
+
+	if (readin == 0)
+		st->pwr_down_mask |= (1 << this_attr->address);
+	else if (readin == 1)
+		st->pwr_down_mask &= ~(1 << this_attr->address);
+	else
+		ret = -EINVAL;
+
+	ret = ad5504_spi_write(st->spi, AD5504_ADDR_CTRL,
+				AD5504_DAC_PWRDWN_MODE(st->pwr_down_mode) |
+				AD5504_DAC_PWR(st->pwr_down_mask));
+
+	/* writes to the CTRL register must be followed by a NOOP */
+	ad5504_spi_write(st->spi, AD5504_ADDR_NOOP, 0);
+
+	return ret ? ret : len;
+}
+
+static ssize_t ad5504_show_scale(struct device *dev,
+				struct device_attribute *attr,
+				char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
+	/* Corresponds to Vref / 2^(bits) */
+	unsigned int scale_uv = (st->vref_mv * 1000) >> AD5505_BITS;
+
+	return sprintf(buf, "%d.%03d\n", scale_uv / 1000, scale_uv % 1000);
+}
+static IIO_DEVICE_ATTR(out_scale, S_IRUGO, ad5504_show_scale, NULL, 0);
+
+static ssize_t ad5504_show_name(struct device *dev,
+				 struct device_attribute *attr,
+				 char *buf)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
+
+	return sprintf(buf, "%s\n", spi_get_device_id(st->spi)->name);
+}
+static IIO_DEVICE_ATTR(name, S_IRUGO, ad5504_show_name, NULL, 0);
+
+#define IIO_DEV_ATTR_OUT_RW_RAW(_num, _show, _store, _addr)		\
+	IIO_DEVICE_ATTR(out##_num##_raw,				\
+			S_IRUGO | S_IWUSR, _show, _store, _addr)
+
+static IIO_DEV_ATTR_OUT_RW_RAW(0, ad5504_read_dac,
+	ad5504_write_dac, AD5504_ADDR_DAC0);
+static IIO_DEV_ATTR_OUT_RW_RAW(1, ad5504_read_dac,
+	ad5504_write_dac, AD5504_ADDR_DAC1);
+static IIO_DEV_ATTR_OUT_RW_RAW(2, ad5504_read_dac,
+	ad5504_write_dac, AD5504_ADDR_DAC2);
+static IIO_DEV_ATTR_OUT_RW_RAW(3, ad5504_read_dac,
+	ad5504_write_dac, AD5504_ADDR_DAC3);
+
+static IIO_DEVICE_ATTR(out_powerdown_mode, S_IRUGO |
+			S_IWUSR, ad5504_read_powerdown_mode,
+			ad5504_write_powerdown_mode, 0);
+
+static IIO_CONST_ATTR(out_powerdown_mode_available,
+			"20kohm_to_gnd three_state");
+
+#define IIO_DEV_ATTR_DAC_POWERDOWN(_num, _show, _store, _addr)		\
+	IIO_DEVICE_ATTR(out##_num##_powerdown,				\
+			S_IRUGO | S_IWUSR, _show, _store, _addr)
+
+static IIO_DEV_ATTR_DAC_POWERDOWN(0, ad5504_read_dac_powerdown,
+				   ad5504_write_dac_powerdown, 0);
+static IIO_DEV_ATTR_DAC_POWERDOWN(1, ad5504_read_dac_powerdown,
+				   ad5504_write_dac_powerdown, 1);
+static IIO_DEV_ATTR_DAC_POWERDOWN(2, ad5504_read_dac_powerdown,
+				   ad5504_write_dac_powerdown, 2);
+static IIO_DEV_ATTR_DAC_POWERDOWN(3, ad5504_read_dac_powerdown,
+				   ad5504_write_dac_powerdown, 3);
+
+static struct attribute *ad5504_attributes[] = {
+	&iio_dev_attr_out0_raw.dev_attr.attr,
+	&iio_dev_attr_out1_raw.dev_attr.attr,
+	&iio_dev_attr_out2_raw.dev_attr.attr,
+	&iio_dev_attr_out3_raw.dev_attr.attr,
+	&iio_dev_attr_out0_powerdown.dev_attr.attr,
+	&iio_dev_attr_out1_powerdown.dev_attr.attr,
+	&iio_dev_attr_out2_powerdown.dev_attr.attr,
+	&iio_dev_attr_out3_powerdown.dev_attr.attr,
+	&iio_dev_attr_out_powerdown_mode.dev_attr.attr,
+	&iio_const_attr_out_powerdown_mode_available.dev_attr.attr,
+	&iio_dev_attr_out_scale.dev_attr.attr,
+	&iio_dev_attr_name.dev_attr.attr,
+	NULL,
+};
+
+static mode_t ad5504_attr_is_visible(struct kobject *kobj,
+				     struct attribute *attr, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);
+	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
+
+	mode_t mode = attr->mode;
+
+	if (spi_get_device_id(st->spi)->driver_data == ID_AD5501 &&
+		(attr == &iio_dev_attr_out1_raw.dev_attr.attr ||
+		attr == &iio_dev_attr_out2_raw.dev_attr.attr ||
+		attr == &iio_dev_attr_out3_raw.dev_attr.attr ||
+		attr == &iio_dev_attr_out1_powerdown.dev_attr.attr ||
+		attr == &iio_dev_attr_out2_powerdown.dev_attr.attr ||
+		attr == &iio_dev_attr_out3_powerdown.dev_attr.attr))
+		mode = 0;
+
+	return mode;
+}
+
+static const struct attribute_group ad5504_attribute_group = {
+	.attrs = ad5504_attributes,
+	.is_visible = ad5504_attr_is_visible,
+};
+
+static IIO_CONST_ATTR(out_alarm_overtemp_thresh_high_value, "110000");
+
+static struct attribute *ad5504_ev_attributes[] = {
+	&iio_const_attr_out_alarm_overtemp_thresh_high_value.dev_attr.attr,
+	NULL,
+};
+
+static struct attribute_group ad5504_ev_attribute_group = {
+	.attrs = ad5504_ev_attributes,
+};
+
+static void ad5504_interrupt_bh(struct work_struct *work_s)
+{
+	struct ad5504_state *st = container_of(work_s,
+		struct ad5504_state, work_alarm);
+
+	iio_push_event(st->indio_dev, 0,
+			IIO_UNMOD_EVENT_CODE(IIO_EV_CLASS_OUT,
+			0,
+			IIO_EV_TYPE_THRESH,
+			IIO_EV_DIR_RISING),
+			st->last_timestamp);
+
+	enable_irq(st->spi->irq);
+}
+
+static int ad5504_interrupt(struct iio_dev *dev_info,
+		int index,
+		s64 timestamp,
+		int no_test)
+{
+	struct ad5504_state *st = dev_info->dev_data;
+
+	st->last_timestamp = timestamp;
+	schedule_work(&st->work_alarm);
+	return 0;
+}
+
+IIO_EVENT_SH(ad5504, &ad5504_interrupt);
+
+static int __devinit ad5504_probe(struct spi_device *spi)
+{
+	struct ad5504_platform_data *pdata = spi->dev.platform_data;
+	struct ad5504_state *st;
+	int ret, voltage_uv = 0;
+
+	st = kzalloc(sizeof(*st), GFP_KERNEL);
+	if (st == NULL) {
+		ret = -ENOMEM;
+		goto error_ret;
+	}
+
+	spi_set_drvdata(spi, st);
+
+	st->reg = regulator_get(&spi->dev, "vcc");
+	if (!IS_ERR(st->reg)) {
+		ret = regulator_enable(st->reg);
+		if (ret)
+			goto error_put_reg;
+
+		voltage_uv = regulator_get_voltage(st->reg);
+	}
+
+	if (voltage_uv)
+		st->vref_mv = voltage_uv / 1000;
+	else if (pdata)
+		st->vref_mv = pdata->vref_mv;
+	else
+		dev_warn(&spi->dev, "reference voltage unspecified\n");
+
+	st->spi = spi;
+	st->indio_dev = iio_allocate_device();
+	if (st->indio_dev == NULL) {
+		ret = -ENOMEM;
+		goto error_disable_reg;
+	}
+	st->indio_dev->dev.parent = &spi->dev;
+	st->indio_dev->attrs = &ad5504_attribute_group;
+	st->indio_dev->dev_data = (void *)(st);
+	st->indio_dev->driver_module = THIS_MODULE;
+	st->indio_dev->modes = INDIO_DIRECT_MODE;
+	st->indio_dev->num_interrupt_lines = 1;
+	st->indio_dev->event_attrs = &ad5504_ev_attribute_group,
+
+	ret = iio_device_register(st->indio_dev);
+	if (ret)
+		goto error_free_dev;
+
+	if (spi->irq) {
+		INIT_WORK(&st->work_alarm, ad5504_interrupt_bh);
+
+		ret = iio_register_interrupt_line(spi->irq,
+				st->indio_dev,
+				0,
+				IRQF_TRIGGER_FALLING,
+				spi_get_device_id(st->spi)->name);
+		if (ret)
+			goto error_unreg_iio_device;
+
+		iio_add_event_to_list(&iio_event_ad5504,
+				&st->indio_dev->interrupts[0]->ev_list);
+	}
+
+	return 0;
+
+error_unreg_iio_device:
+	iio_device_unregister(st->indio_dev);
+error_free_dev:
+	iio_free_device(st->indio_dev);
+error_disable_reg:
+	if (!IS_ERR(st->reg))
+		regulator_disable(st->reg);
+error_put_reg:
+	if (!IS_ERR(st->reg))
+		regulator_put(st->reg);
+
+	kfree(st);
+error_ret:
+	return ret;
+}
+
+static int __devexit ad5504_remove(struct spi_device *spi)
+{
+	struct ad5504_state *st = spi_get_drvdata(spi);
+
+	if (spi->irq)
+		iio_unregister_interrupt_line(st->indio_dev, 0);
+
+	iio_device_unregister(st->indio_dev);
+
+	if (!IS_ERR(st->reg)) {
+		regulator_disable(st->reg);
+		regulator_put(st->reg);
+	}
+
+	kfree(st);
+
+	return 0;
+}
+
+static const struct spi_device_id ad5504_id[] = {
+	{"ad5504", ID_AD5504},
+	{"ad5501", ID_AD5501},
+	{}
+};
+
+static struct spi_driver ad5504_driver = {
+	.driver = {
+		   .name = "ad5504",
+		   .owner = THIS_MODULE,
+		   },
+	.probe = ad5504_probe,
+	.remove = __devexit_p(ad5504_remove),
+	.id_table = ad5504_id,
+};
+
+static __init int ad5504_spi_init(void)
+{
+	return spi_register_driver(&ad5504_driver);
+}
+module_init(ad5504_spi_init);
+
+static __exit void ad5504_spi_exit(void)
+{
+	spi_unregister_driver(&ad5504_driver);
+}
+module_exit(ad5504_spi_exit);
+
+MODULE_AUTHOR("Michael Hennerich <hennerich@blackfin.uclinux.org>");
+MODULE_DESCRIPTION("Analog Devices AD5501/AD5501 DAC");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/staging/iio/dac/ad5504.h b/drivers/staging/iio/dac/ad5504.h
new file mode 100644
index 0000000..d2fac63
--- /dev/null
+++ b/drivers/staging/iio/dac/ad5504.h
@@ -0,0 +1,74 @@
+/*
+ * AD5504 SPI DAC driver
+ *
+ * Copyright 2011 Analog Devices Inc.
+ *
+ * Licensed under the GPL-2.
+ */
+
+#ifndef SPI_AD5504_H_
+#define SPI_AD5504_H_
+
+#define AD5505_BITS			12
+#define AD5504_RES_MASK			((1 << (AD5505_BITS)) - 1)
+
+#define AD5504_CMD_READ			(1 << 15)
+#define AD5504_CMD_WRITE		(0 << 15)
+#define AD5504_ADDR(addr)		((addr) << 12)
+
+/* Registers */
+#define AD5504_ADDR_NOOP		0
+#define AD5504_ADDR_DAC0		1
+#define AD5504_ADDR_DAC1		2
+#define AD5504_ADDR_DAC2		3
+#define AD5504_ADDR_DAC3		4
+#define AD5504_ADDR_ALL_DAC		5
+#define AD5504_ADDR_CTRL		7
+
+/* Control Register */
+#define AD5504_DAC_PWR(ch)		((ch) << 2)
+#define AD5504_DAC_PWRDWN_MODE(mode)	((mode) << 6)
+#define AD5504_DAC_PWRDN_20K		0
+#define AD5504_DAC_PWRDN_3STATE		1
+
+/*
+ * TODO: struct ad5504_platform_data needs to go into include/linux/iio
+ */
+
+struct ad5504_platform_data {
+	u16				vref_mv;
+};
+
+/**
+ * struct ad5446_state - driver instance specific data
+ * @indio_dev:		the industrial I/O device
+ * @us:			spi_device
+ * @reg:		supply regulator
+ * @vref_mv:		actual reference voltage used
+ * @work_alarm:		bh work structure for event handling
+ * @last_timestamp:	timestamp of last event interrupt
+ * @pwr_down_mask	power down mask
+ * @pwr_down_mode	current power down mode
+ */
+
+struct ad5504_state {
+	struct iio_dev			*indio_dev;
+	struct spi_device		*spi;
+	struct regulator		*reg;
+	unsigned short			vref_mv;
+	struct work_struct		work_alarm;
+	s64				last_timestamp;
+	unsigned			pwr_down_mask;
+	unsigned			pwr_down_mode;
+};
+
+/**
+ * ad5504_supported_device_ids:
+ */
+
+enum ad5504_supported_device_ids {
+	ID_AD5504,
+	ID_AD5501,
+};
+
+#endif /* SPI_AD5504_H_ */
diff --git a/drivers/staging/iio/sysfs.h b/drivers/staging/iio/sysfs.h
index 24b74dd..9e3b784 100644
--- a/drivers/staging/iio/sysfs.h
+++ b/drivers/staging/iio/sysfs.h
@@ -266,6 +266,7 @@ struct iio_const_attr {
 #define IIO_EV_CLASS_MAGN		4
 #define IIO_EV_CLASS_LIGHT		5
 #define IIO_EV_CLASS_PROXIMITY		6
+#define IIO_EV_CLASS_OUT		7
 
 #define IIO_EV_MOD_X			0
 #define IIO_EV_MOD_Y			1
-- 
1.6.0.2

Re: [PATCH] IIO: DAC: New driver for the AD5504 and AD5501 High Voltage DACs

[Jonathan Cameron]

On 03/31/11 15:56, michael.hennerich@analog.com wrote:
> From: Michael Hennerich <michael.hennerich@analog.com>
> 
> 
Hi Michael,

I wonder if it makes sense to handle an over temp warning
as you have done here with a new rather complex threshold type.
What do we loose by 'defining' a separate temperature channel?
That would have no direct access and only exist for the purposes
of the threshold.

Thus we would have
temp0_thresh_rising_value and temp0_thresh_rising_en (always 1)
The event code is then just a standard temperature one.  What
you currently have to my mind implies that the threshold is
on the output voltage rather than the temperature.

Also need documentation for the powerdown attributes.
(powerdown_mode seems to be specified, but se haven't had explicit
per channel controls on this before).

I'm a little confused about what the powerdown attributes do consistent.
 Looks like read gives 1 if the relevant bit of pwr_down_mask is set.
If you write a 1 it seems to unset that bit. Thus write 1 and
you'll read back a 0.

As ever, pretty clean. Thanks!

Jonathan
> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
> ---
>  drivers/staging/iio/dac/Kconfig  |   10 +
>  drivers/staging/iio/dac/Makefile |    1 +
>  drivers/staging/iio/dac/ad5504.c |  429 ++++++++++++++++++++++++++++++++++++++
>  drivers/staging/iio/dac/ad5504.h |   74 +++++++
>  drivers/staging/iio/sysfs.h      |    1 +
>  5 files changed, 515 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/staging/iio/dac/ad5504.c
>  create mode 100644 drivers/staging/iio/dac/ad5504.h
> 
> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
> index 67defcb..1b0188a 100644
> --- a/drivers/staging/iio/dac/Kconfig
> +++ b/drivers/staging/iio/dac/Kconfig
> @@ -21,6 +21,16 @@ config AD5446
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called ad5446.
>  
> +config AD5504
> +	tristate "Analog Devices AD5504/AD5501 DAC SPI driver"
> +	depends on SPI
> +	help
> +	  Say yes here to build support for Analog Devices AD5504, AD5501,
Why reverse numerical order?  (not that I really care, just curious ;)
> +	  High Voltage Digital to Analog Converter.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ad5504.
> +
...
> +static ssize_t ad5504_read_powerdown_mode(struct device *dev,
> +				      struct device_attribute *attr, char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
> +
I count 14 characters needed.  Also, why not const char* for these
and share them across multiple driver instances.
> +	char mode[][15] = {"20kohm_to_gnd", "three_state"};
> +
> +	return sprintf(buf, "%s\n", mode[st->pwr_down_mode]);
> +}
> +
...


> +}
> +
> +static ssize_t ad5504_read_dac_powerdown(struct device *dev,
> +					   struct device_attribute *attr,
> +					   char *buf)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> +	return sprintf(buf, "%d\n",
So, if mask is set, the channel is powered down?
I wonder if flipping the logic here to have dac0_en etc might be
more consistent with everything else?  Perhaps not given it
would disassociate this from the powerdown_mode attributes...
> +			!!(st->pwr_down_mask & (1 << this_attr->address)));
> +}
> +
> +static ssize_t ad5504_write_dac_powerdown(struct device *dev,
> +					    struct device_attribute *attr,
> +					    const char *buf, size_t len)
> +{
> +	long readin;
> +	int ret;
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
> +	struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +
> +	ret = strict_strtol(buf, 10, &readin);
> +	if (ret)
> +		return ret;
> +
> +	if (readin == 0)
> +		st->pwr_down_mask |= (1 << this_attr->address);
> +	else if (readin == 1)
> +		st->pwr_down_mask &= ~(1 << this_attr->address);
Doesn't this technically mean this is a power up mask?  If we right
1 to power down we unset the bit in this mask.  
> +	else
> +		ret = -EINVAL;
> +
> +	ret = ad5504_spi_write(st->spi, AD5504_ADDR_CTRL,
> +				AD5504_DAC_PWRDWN_MODE(st->pwr_down_mode) |
> +				AD5504_DAC_PWR(st->pwr_down_mask));
> +
> +	/* writes to the CTRL register must be followed by a NOOP */
> +	ad5504_spi_write(st->spi, AD5504_ADDR_NOOP, 0);
> +
> +	return ret ? ret : len;
> +}
> +
...

> +static struct attribute *ad5504_attributes[] = {
> +	&iio_dev_attr_out0_raw.dev_attr.attr,
> +	&iio_dev_attr_out1_raw.dev_attr.attr,
> +	&iio_dev_attr_out2_raw.dev_attr.attr,
> +	&iio_dev_attr_out3_raw.dev_attr.attr,
> +	&iio_dev_attr_out0_powerdown.dev_attr.attr,
Not actually documented as yet (I think). Please add that as well.
> +	&iio_dev_attr_out1_powerdown.dev_attr.attr,
> +	&iio_dev_attr_out2_powerdown.dev_attr.attr,
> +	&iio_dev_attr_out3_powerdown.dev_attr.attr,
> +	&iio_dev_attr_out_powerdown_mode.dev_attr.attr,
> +	&iio_const_attr_out_powerdown_mode_available.dev_attr.attr,
> +	&iio_dev_attr_out_scale.dev_attr.attr,
> +	&iio_dev_attr_name.dev_attr.attr,
> +	NULL,
> +};
> +
Not convinced you are gaining much here vs just having two attribute
tables and picking that based on the id...
> +static mode_t ad5504_attr_is_visible(struct kobject *kobj,
> +				     struct attribute *attr, int n)
> +{
> +	struct device *dev = container_of(kobj, struct device, kobj);
> +	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> +	struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
> +
> +	mode_t mode = attr->mode;
> +
> +	if (spi_get_device_id(st->spi)->driver_data == ID_AD5501 &&
> +		(attr == &iio_dev_attr_out1_raw.dev_attr.attr ||
> +		attr == &iio_dev_attr_out2_raw.dev_attr.attr ||
> +		attr == &iio_dev_attr_out3_raw.dev_attr.attr ||
> +		attr == &iio_dev_attr_out1_powerdown.dev_attr.attr ||
> +		attr == &iio_dev_attr_out2_powerdown.dev_attr.attr ||
> +		attr == &iio_dev_attr_out3_powerdown.dev_attr.attr))
> +		mode = 0;
> +
> +	return mode;
> +}
> +
> +static const struct attribute_group ad5504_attribute_group = {
> +	.attrs = ad5504_attributes,
> +	.is_visible = ad5504_attr_is_visible,
> +};
> +
> +static IIO_CONST_ATTR(out_alarm_overtemp_thresh_high_value, "110000");
> +
> +static struct attribute *ad5504_ev_attributes[] = {
> +	&iio_const_attr_out_alarm_overtemp_thresh_high_value.dev_attr.attr,
For consistency we also want the _en element, (always 1 of course).

> +	NULL,
> +};
> +
> +static struct attribute_group ad5504_ev_attribute_group = {
> +	.attrs = ad5504_ev_attributes,
> +};
> +
> +static void ad5504_interrupt_bh(struct work_struct *work_s)
> +{
> +	struct ad5504_state *st = container_of(work_s,
> +		struct ad5504_state, work_alarm);
> +
> +	iio_push_event(st->indio_dev, 0,
> +			IIO_UNMOD_EVENT_CODE(IIO_EV_CLASS_OUT,
> +			0,
> +			IIO_EV_TYPE_THRESH,
> +			IIO_EV_DIR_RISING),
> +			st->last_timestamp);
Not happy with this event type. See intro.
> +
> +	enable_irq(st->spi->irq);
> +}
> +
...

> +#define AD5504_ADDR_ALL_DAC		5
> +#define AD5504_ADDR_CTRL		7
> +
> +/* Control Register */
> +#define AD5504_DAC_PWR(ch)		((ch) << 2)
> +#define AD5504_DAC_PWRDWN_MODE(mode)	((mode) << 6)
> +#define AD5504_DAC_PWRDN_20K		0
> +#define AD5504_DAC_PWRDN_3STATE		1
> +
> +/*
> + * TODO: struct ad5504_platform_data needs to go into include/linux/iio
> + */
> +
> +struct ad5504_platform_data {
> +	u16				vref_mv;
> +};
Would it matter to you if we stopped doing this vref passing in platform
data and started insisting on regulator always being queriable?  (came up in
the max1363 discussion of yesterday?) When we first started doing this
almost no boards had regulators specified. Is this still true?
> +
> +/**
> + * struct ad5446_state - driver instance specific data
> + * @indio_dev:		the industrial I/O device
> + * @us:			spi_device
> + * @reg:		supply regulator
> + * @vref_mv:		actual reference voltage used
> + * @work_alarm:		bh work structure for event handling
> + * @last_timestamp:	timestamp of last event interrupt
> + * @pwr_down_mask	power down mask
> + * @pwr_down_mode	current power down mode
> + */
> +
> +struct ad5504_state {
> +	struct iio_dev			*indio_dev;
> +	struct spi_device		*spi;
> +	struct regulator		*reg;
> +	unsigned short			vref_mv;
> +	struct work_struct		work_alarm;
> +	s64				last_timestamp;
> +	unsigned			pwr_down_mask;
> +	unsigned			pwr_down_mode;
> +};
> +
> +/**
> + * ad5504_supported_device_ids:
> + */
> +
> +enum ad5504_supported_device_ids {
> +	ID_AD5504,
> +	ID_AD5501,
> +};
> +
> +#endif /* SPI_AD5504_H_ */
> diff --git a/drivers/staging/iio/sysfs.h b/drivers/staging/iio/sysfs.h
> index 24b74dd..9e3b784 100644
> --- a/drivers/staging/iio/sysfs.h
> +++ b/drivers/staging/iio/sysfs.h
> @@ -266,6 +266,7 @@ struct iio_const_attr {
>  #define IIO_EV_CLASS_MAGN		4
>  #define IIO_EV_CLASS_LIGHT		5
>  #define IIO_EV_CLASS_PROXIMITY		6
> +#define IIO_EV_CLASS_OUT		7
This is fine but I think shouldn't be used here so shouldn't be in the patch!
>  
>  #define IIO_EV_MOD_X			0
>  #define IIO_EV_MOD_Y			1

Re: [PATCH] IIO: DAC: New driver for the AD5504 and AD5501 High Voltage DACs

[Michael Hennerich]

On 03/31/2011 06:18 PM, Jonathan Cameron wrote:
> On 03/31/11 15:56, michael.hennerich@analog.com wrote:
>   
>> From: Michael Hennerich <michael.hennerich@analog.com>
>>
>>
>>     
> Hi Michael,
>
> I wonder if it makes sense to handle an over temp warning
> as you have done here with a new rather complex threshold type.
> What do we loose by 'defining' a separate temperature channel?
> That would have no direct access and only exist for the purposes
> of the threshold.
>   
Hi Jonathan,
I think you know that this threshold event is just an ALARM interrupt,
that signals
the junction temperature exceeded 110°C, and that the part therefore
automatically
entered thermal shutdown mode?
So there is no temperature that can be read...
> Thus we would have
> temp0_thresh_rising_value
Always 110000 mDeg C.
> and temp0_thresh_rising_en (always 1)
> The event code is then just a standard temperature one.  What
> you currently have to my mind implies that the threshold is
> on the output voltage rather than the temperature.
>   
I see you point here.
But can you tell me how the standard temperature would look like?

> Also need documentation for the powerdown attributes.
> (powerdown_mode seems to be specified, but se haven't had explicit
> per channel controls on this before).
>
>   
There are no channel controls for powerdown_mode.
There are channel controls for outY_powerdown, but not for the mode.

All of them are already documented.

> I'm a little confused about what the powerdown attributes do consistent.
>  Looks like read gives 1 if the relevant bit of pwr_down_mask is set.
> If you write a 1 it seems to unset that bit. Thus write 1 and
> you'll read back a 0.
>   
Good catch, that's a bug.
> As ever, pretty clean. Thanks!
>
> Jonathan
>   
>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>> ---
>>  drivers/staging/iio/dac/Kconfig  |   10 +
>>  drivers/staging/iio/dac/Makefile |    1 +
>>  drivers/staging/iio/dac/ad5504.c |  429 ++++++++++++++++++++++++++++++++++++++
>>  drivers/staging/iio/dac/ad5504.h |   74 +++++++
>>  drivers/staging/iio/sysfs.h      |    1 +
>>  5 files changed, 515 insertions(+), 0 deletions(-)
>>  create mode 100644 drivers/staging/iio/dac/ad5504.c
>>  create mode 100644 drivers/staging/iio/dac/ad5504.h
>>
>> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/dac/Kconfig
>> index 67defcb..1b0188a 100644
>> --- a/drivers/staging/iio/dac/Kconfig
>> +++ b/drivers/staging/iio/dac/Kconfig
>> @@ -21,6 +21,16 @@ config AD5446
>>         To compile this driver as a module, choose M here: the
>>         module will be called ad5446.
>>
>> +config AD5504
>> +     tristate "Analog Devices AD5504/AD5501 DAC SPI driver"
>> +     depends on SPI
>> +     help
>> +       Say yes here to build support for Analog Devices AD5504, AD5501,
>>     
> Why reverse numerical order?  (not that I really care, just curious ;)
>   
The driver is named after the AD5504, so I thought I list it first.
Actually I don't really care, too.

>> +       High Voltage Digital to Analog Converter.
>> +
>> +       To compile this driver as a module, choose M here: the
>> +       module will be called ad5504.
>> +
>>     
> ...
>   
>> +static ssize_t ad5504_read_powerdown_mode(struct device *dev,
>> +                                   struct device_attribute *attr, char *buf)
>> +{
>> +     struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +     struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
>> +
>>     
> I count 14 characters needed.  Also, why not const char* for these
> and share them across multiple driver instances.
>   
ok
>> +     char mode[][15] = {"20kohm_to_gnd", "three_state"};
>> +
>> +     return sprintf(buf, "%s\n", mode[st->pwr_down_mode]);
>> +}
>> +
>>     
> ...
>
>
>   
>> +}
>> +
>> +static ssize_t ad5504_read_dac_powerdown(struct device *dev,
>> +                                        struct device_attribute *attr,
>> +                                        char *buf)
>> +{
>> +     struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +     struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
>> +     struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> +     return sprintf(buf, "%d\n",
>>     
> So, if mask is set, the channel is powered down?
> I wonder if flipping the logic here to have dac0_en etc might be
> more consistent with everything else?  Perhaps not given it
> would disassociate this from the powerdown_mode attributes...
>   
As I said the reversed logic is bug here.
I don't really like to call it enable. Since it is actually a power
down, associated with the
various modes.
>> +                     !!(st->pwr_down_mask & (1 << this_attr->address)));
>> +}
>> +
>> +static ssize_t ad5504_write_dac_powerdown(struct device *dev,
>> +                                         struct device_attribute *attr,
>> +                                         const char *buf, size_t len)
>> +{
>> +     long readin;
>> +     int ret;
>> +     struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +     struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
>> +     struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
>> +
>> +     ret = strict_strtol(buf, 10, &readin);
>> +     if (ret)
>> +             return ret;
>> +
>> +     if (readin == 0)
>> +             st->pwr_down_mask |= (1 << this_attr->address);
>> +     else if (readin == 1)
>> +             st->pwr_down_mask &= ~(1 << this_attr->address);
>>     
> Doesn't this technically mean this is a power up mask?  If we right
> 1 to power down we unset the bit in this mask.
>   
Unlike to the other DACs having this feature, it's here used as an power
up mask.
>> +     else
>> +             ret = -EINVAL;
>> +
>> +     ret = ad5504_spi_write(st->spi, AD5504_ADDR_CTRL,
>> +                             AD5504_DAC_PWRDWN_MODE(st->pwr_down_mode) |
>> +                             AD5504_DAC_PWR(st->pwr_down_mask));
>> +
>> +     /* writes to the CTRL register must be followed by a NOOP */
>> +     ad5504_spi_write(st->spi, AD5504_ADDR_NOOP, 0);
>> +
>> +     return ret ? ret : len;
>> +}
>> +
>>     
> ...
>
>   
>> +static struct attribute *ad5504_attributes[] = {
>> +     &iio_dev_attr_out0_raw.dev_attr.attr,
>> +     &iio_dev_attr_out1_raw.dev_attr.attr,
>> +     &iio_dev_attr_out2_raw.dev_attr.attr,
>> +     &iio_dev_attr_out3_raw.dev_attr.attr,
>> +     &iio_dev_attr_out0_powerdown.dev_attr.attr,
>>     
> Not actually documented as yet (I think). Please add that as well.
>   

It is -
+What:        /sys/bus/iio/devices/deviceX/outY_powerdown
+What:        /sys/bus/iio/devices/deviceX/out_powerdown
+KernelVersion:    2.6.38
+Contact:    linux-iio@vger.kernel.org
+Description:
+        Writing 1 causes output Y to enter the power down mode specified
+        by the corresponding outY_powerdown_mode. Clearing returns to
+        normal operation. Y may be suppressed if all outputs are
+        controlled together.


>> +     &iio_dev_attr_out1_powerdown.dev_attr.attr,
>> +     &iio_dev_attr_out2_powerdown.dev_attr.attr,
>> +     &iio_dev_attr_out3_powerdown.dev_attr.attr,
>> +     &iio_dev_attr_out_powerdown_mode.dev_attr.attr,
>> +     &iio_const_attr_out_powerdown_mode_available.dev_attr.attr,
>> +     &iio_dev_attr_out_scale.dev_attr.attr,
>> +     &iio_dev_attr_name.dev_attr.attr,
>> +     NULL,
>> +};
>> +
>>     
> Not convinced you are gaining much here vs just having two attribute
> tables and picking that based on the id...
>   
ok
>> +static mode_t ad5504_attr_is_visible(struct kobject *kobj,
>> +                                  struct attribute *attr, int n)
>> +{
>> +     struct device *dev = container_of(kobj, struct device, kobj);
>> +     struct iio_dev *indio_dev = dev_get_drvdata(dev);
>> +     struct ad5504_state *st = iio_dev_get_devdata(indio_dev);
>> +
>> +     mode_t mode = attr->mode;
>> +
>> +     if (spi_get_device_id(st->spi)->driver_data == ID_AD5501 &&
>> +             (attr == &iio_dev_attr_out1_raw.dev_attr.attr ||
>> +             attr == &iio_dev_attr_out2_raw.dev_attr.attr ||
>> +             attr == &iio_dev_attr_out3_raw.dev_attr.attr ||
>> +             attr == &iio_dev_attr_out1_powerdown.dev_attr.attr ||
>> +             attr == &iio_dev_attr_out2_powerdown.dev_attr.attr ||
>> +             attr == &iio_dev_attr_out3_powerdown.dev_attr.attr))
>> +             mode = 0;
>> +
>> +     return mode;
>> +}
>> +
>> +static const struct attribute_group ad5504_attribute_group = {
>> +     .attrs = ad5504_attributes,
>> +     .is_visible = ad5504_attr_is_visible,
>> +};
>> +
>> +static IIO_CONST_ATTR(out_alarm_overtemp_thresh_high_value, "110000");
>> +
>> +static struct attribute *ad5504_ev_attributes[] = {
>> +     &iio_const_attr_out_alarm_overtemp_thresh_high_value.dev_attr.attr,
>>     
> For consistency we also want the _en element, (always 1 of course).
>   
ok
>   
>> +     NULL,
>> +};
>> +
>> +static struct attribute_group ad5504_ev_attribute_group = {
>> +     .attrs = ad5504_ev_attributes,
>> +};
>> +
>> +static void ad5504_interrupt_bh(struct work_struct *work_s)
>> +{
>> +     struct ad5504_state *st = container_of(work_s,
>> +             struct ad5504_state, work_alarm);
>> +
>> +     iio_push_event(st->indio_dev, 0,
>> +                     IIO_UNMOD_EVENT_CODE(IIO_EV_CLASS_OUT,
>> +                     0,
>> +                     IIO_EV_TYPE_THRESH,
>> +                     IIO_EV_DIR_RISING),
>> +                     st->last_timestamp);
>>     
> Not happy with this event type. See intro.
>   
Please propose a better one.

>> +
>> +     enable_irq(st->spi->irq);
>> +}
>> +
>>     
> ...
>
>   
>> +#define AD5504_ADDR_ALL_DAC          5
>> +#define AD5504_ADDR_CTRL             7
>> +
>> +/* Control Register */
>> +#define AD5504_DAC_PWR(ch)           ((ch) << 2)
>> +#define AD5504_DAC_PWRDWN_MODE(mode) ((mode) << 6)
>> +#define AD5504_DAC_PWRDN_20K         0
>> +#define AD5504_DAC_PWRDN_3STATE              1
>> +
>> +/*
>> + * TODO: struct ad5504_platform_data needs to go into include/linux/iio
>> + */
>> +
>> +struct ad5504_platform_data {
>> +     u16                             vref_mv;
>> +};
>>     
> Would it matter to you if we stopped doing this vref passing in platform
> data and started insisting on regulator always being queriable?  (came up in
> the max1363 discussion of yesterday?) When we first started doing this
> almost no boards had regulators specified. Is this still true?
>   
The regulator stuff is not really common.
I always try to support it as an alternative way, since I know passing
platform dependent information
through platform_data is a bit inconvenient on systems using devicetree. 
A bandgap reference directly connected to an ADC or DAC, can be
considered as a fixed voltage regulator.
However there is no way to enable or disable it. it's a pretty
complicated way telling the driver it's reference.
I would propose to always support the regulator framework, however I
don't mind having the platform_data approach as well.
 
 
>> +
>> +/**
>> + * struct ad5446_state - driver instance specific data
>> + * @indio_dev:               the industrial I/O device
>> + * @us:                      spi_device
>> + * @reg:             supply regulator
>> + * @vref_mv:         actual reference voltage used
>> + * @work_alarm:              bh work structure for event handling
>> + * @last_timestamp:  timestamp of last event interrupt
>> + * @pwr_down_mask    power down mask
>> + * @pwr_down_mode    current power down mode
>> + */
>> +
>> +struct ad5504_state {
>> +     struct iio_dev                  *indio_dev;
>> +     struct spi_device               *spi;
>> +     struct regulator                *reg;
>> +     unsigned short                  vref_mv;
>> +     struct work_struct              work_alarm;
>> +     s64                             last_timestamp;
>> +     unsigned                        pwr_down_mask;
>> +     unsigned                        pwr_down_mode;
>> +};
>> +
>> +/**
>> + * ad5504_supported_device_ids:
>> + */
>> +
>> +enum ad5504_supported_device_ids {
>> +     ID_AD5504,
>> +     ID_AD5501,
>> +};
>> +
>> +#endif /* SPI_AD5504_H_ */
>> diff --git a/drivers/staging/iio/sysfs.h b/drivers/staging/iio/sysfs.h
>> index 24b74dd..9e3b784 100644
>> --- a/drivers/staging/iio/sysfs.h
>> +++ b/drivers/staging/iio/sysfs.h
>> @@ -266,6 +266,7 @@ struct iio_const_attr {
>>  #define IIO_EV_CLASS_MAGN            4
>>  #define IIO_EV_CLASS_LIGHT           5
>>  #define IIO_EV_CLASS_PROXIMITY               6
>> +#define IIO_EV_CLASS_OUT             7
>>     
> This is fine but I think shouldn't be used here so shouldn't be in the patch!
>   
ok
>>  #define IIO_EV_MOD_X                 0
>>  #define IIO_EV_MOD_Y                 1
>>     
>   


-- 
Greetings,
Michael

--
Analog Devices GmbH      Wilhelm-Wagenfeld-Str. 6      80807 Muenchen
Sitz der Gesellschaft: Muenchen; Registergericht: Muenchen HRB 40368;
Geschaeftsfuehrer:Dr.Carsten Suckrow, Thomas Wessel, William A. Martin,
Margaret Seif

Re: [PATCH] IIO: DAC: New driver for the AD5504 and AD5501 High Voltage DACs

[Jonathan Cameron]

On 04/01/11 09:55, Michael Hennerich wrote:
> On 03/31/2011 06:18 PM, Jonathan Cameron wrote:
>> On 03/31/11 15:56, michael.hennerich@analog.com wrote:
>>  =20
>>> From: Michael Hennerich <michael.hennerich@analog.com>
>>>
>>>
>>>    =20
>> Hi Michael,
>>
>> I wonder if it makes sense to handle an over temp warning
>> as you have done here with a new rather complex threshold type.
>> What do we loose by 'defining' a separate temperature channel?
>> That would have no direct access and only exist for the purposes
>> of the threshold.
>>  =20
> Hi Jonathan,
> I think you know that this threshold event is just an ALARM interrupt=
,
> that signals
> the junction temperature exceeded 110=B0C, and that the part therefor=
e
> automatically
> entered thermal shutdown mode?
> So there is no temperature that can be read...
Sure.
>> Thus we would have
>> temp0_thresh_rising_value
> Always 110000 mDeg C.
>> and temp0_thresh_rising_en (always 1)
>> The event code is then just a standard temperature one.  What
>> you currently have to my mind implies that the threshold is
>> on the output voltage rather than the temperature.
>>  =20
> I see you point here.
> But can you tell me how the standard temperature would look like?
Simply have the event attributes
temp0_thresh_rising_en (always 1)
temp0_thresh_rising_value (always 110,000).

Nothing says we have to be able to read the value of a given channel to
have events for it.  The other approach would be to register the temp
stuff as a hwmon device.  It's basically doing hardware monitoring of t=
he
DAC.=20

>=20
>> Also need documentation for the powerdown attributes.
>> (powerdown_mode seems to be specified, but se haven't had explicit
>> per channel controls on this before).
>>
>>  =20
> There are no channel controls for powerdown_mode.
> There are channel controls for outY_powerdown, but not for the mode.
>=20
> All of them are already documented.
Err. Some how I missed that. Sorry, must have been a typo in my search =
string!
>=20
>> I'm a little confused about what the powerdown attributes do consist=
ent.
>>  Looks like read gives 1 if the relevant bit of pwr_down_mask is set=
=2E
>> If you write a 1 it seems to unset that bit. Thus write 1 and
>> you'll read back a 0.
>>  =20
> Good catch, that's a bug.
>> As ever, pretty clean. Thanks!
>>
>> Jonathan
>>  =20
>>> Signed-off-by: Michael Hennerich <michael.hennerich@analog.com>
>>> ---
>>>  drivers/staging/iio/dac/Kconfig  |   10 +
>>>  drivers/staging/iio/dac/Makefile |    1 +
>>>  drivers/staging/iio/dac/ad5504.c |  429 ++++++++++++++++++++++++++=
++++++++++++
>>>  drivers/staging/iio/dac/ad5504.h |   74 +++++++
>>>  drivers/staging/iio/sysfs.h      |    1 +
>>>  5 files changed, 515 insertions(+), 0 deletions(-)
>>>  create mode 100644 drivers/staging/iio/dac/ad5504.c
>>>  create mode 100644 drivers/staging/iio/dac/ad5504.h
>>>
>>> diff --git a/drivers/staging/iio/dac/Kconfig b/drivers/staging/iio/=
dac/Kconfig
>>> index 67defcb..1b0188a 100644
>>> --- a/drivers/staging/iio/dac/Kconfig
>>> +++ b/drivers/staging/iio/dac/Kconfig
>>> @@ -21,6 +21,16 @@ config AD5446
>>>         To compile this driver as a module, choose M here: the
>>>         module will be called ad5446.
>>>
>>> +config AD5504
>>> +     tristate "Analog Devices AD5504/AD5501 DAC SPI driver"
>>> +     depends on SPI
>>> +     help
>>> +       Say yes here to build support for Analog Devices AD5504, AD=
5501,
>>>    =20
>> Why reverse numerical order?  (not that I really care, just curious =
;)
>>  =20
> The driver is named after the AD5504, so I thought I list it first.
> Actually I don't really care, too.
>=20
>>> +       High Voltage Digital to Analog Converter.
>>> +
>>> +       To compile this driver as a module, choose M here: the
>>> +       module will be called ad5504.
>>> +
>>>    =20
>> ...
>>  =20
>>> +static ssize_t ad5504_read_powerdown_mode(struct device *dev,
>>> +                                   struct device_attribute *attr, =
char *buf)
>>> +{
>>> +     struct iio_dev *indio_dev =3D dev_get_drvdata(dev);
>>> +     struct ad5504_state *st =3D iio_dev_get_devdata(indio_dev);
>>> +
>>>    =20
>> I count 14 characters needed.  Also, why not const char* for these
>> and share them across multiple driver instances.
>>  =20
> ok
>>> +     char mode[][15] =3D {"20kohm_to_gnd", "three_state"};
>>> +
>>> +     return sprintf(buf, "%s\n", mode[st->pwr_down_mode]);
>>> +}
>>> +
>>>    =20
>> ...
>>
>>
>>  =20
>>> +}
>>> +
>>> +static ssize_t ad5504_read_dac_powerdown(struct device *dev,
>>> +                                        struct device_attribute *a=
ttr,
>>> +                                        char *buf)
>>> +{
>>> +     struct iio_dev *indio_dev =3D dev_get_drvdata(dev);
>>> +     struct ad5504_state *st =3D iio_dev_get_devdata(indio_dev);
>>> +     struct iio_dev_attr *this_attr =3D to_iio_dev_attr(attr);
>>> +
>>> +     return sprintf(buf, "%d\n",
>>>    =20
>> So, if mask is set, the channel is powered down?
>> I wonder if flipping the logic here to have dac0_en etc might be
>> more consistent with everything else?  Perhaps not given it
>> would disassociate this from the powerdown_mode attributes...
>>  =20
> As I said the reversed logic is bug here.
> I don't really like to call it enable. Since it is actually a power
> down, associated with the
> various modes.
=46ine.
>>> +                     !!(st->pwr_down_mask & (1 << this_attr->addre=
ss)));
>>> +}
>>> +
>>> +static ssize_t ad5504_write_dac_powerdown(struct device *dev,
>>> +                                         struct device_attribute *=
attr,
>>> +                                         const char *buf, size_t l=
en)
>>> +{
>>> +     long readin;
>>> +     int ret;
>>> +     struct iio_dev *indio_dev =3D dev_get_drvdata(dev);
>>> +     struct ad5504_state *st =3D iio_dev_get_devdata(indio_dev);
>>> +     struct iio_dev_attr *this_attr =3D to_iio_dev_attr(attr);
>>> +
>>> +     ret =3D strict_strtol(buf, 10, &readin);
>>> +     if (ret)
>>> +             return ret;
>>> +
>>> +     if (readin =3D=3D 0)
>>> +             st->pwr_down_mask |=3D (1 << this_attr->address);
>>> +     else if (readin =3D=3D 1)
>>> +             st->pwr_down_mask &=3D ~(1 << this_attr->address);
>>>    =20
>> Doesn't this technically mean this is a power up mask?  If we right
>> 1 to power down we unset the bit in this mask.
>>  =20
> Unlike to the other DACs having this feature, it's here used as an po=
wer
> up mask.
>>> +     else
>>> +             ret =3D -EINVAL;
>>> +
>>> +     ret =3D ad5504_spi_write(st->spi, AD5504_ADDR_CTRL,
>>> +                             AD5504_DAC_PWRDWN_MODE(st->pwr_down_m=
ode) |
>>> +                             AD5504_DAC_PWR(st->pwr_down_mask));
>>> +
>>> +     /* writes to the CTRL register must be followed by a NOOP */
>>> +     ad5504_spi_write(st->spi, AD5504_ADDR_NOOP, 0);
>>> +
>>> +     return ret ? ret : len;
>>> +}
>>> +
>>>    =20
>> ...
>>
>>  =20
>>> +static struct attribute *ad5504_attributes[] =3D {
>>> +     &iio_dev_attr_out0_raw.dev_attr.attr,
>>> +     &iio_dev_attr_out1_raw.dev_attr.attr,
>>> +     &iio_dev_attr_out2_raw.dev_attr.attr,
>>> +     &iio_dev_attr_out3_raw.dev_attr.attr,
>>> +     &iio_dev_attr_out0_powerdown.dev_attr.attr,
>>>    =20
>> Not actually documented as yet (I think). Please add that as well.
>>  =20
>=20
> It is -
> +What:        /sys/bus/iio/devices/deviceX/outY_powerdown
> +What:        /sys/bus/iio/devices/deviceX/out_powerdown
> +KernelVersion:    2.6.38
> +Contact:    linux-iio@vger.kernel.org
> +Description:
> +        Writing 1 causes output Y to enter the power down mode speci=
fied
> +        by the corresponding outY_powerdown_mode. Clearing returns t=
o
> +        normal operation. Y may be suppressed if all outputs are
> +        controlled together.
Indeed, sorry for the noise!
>=20
>=20
>>> +     &iio_dev_attr_out1_powerdown.dev_attr.attr,
>>> +     &iio_dev_attr_out2_powerdown.dev_attr.attr,
>>> +     &iio_dev_attr_out3_powerdown.dev_attr.attr,
>>> +     &iio_dev_attr_out_powerdown_mode.dev_attr.attr,
>>> +     &iio_const_attr_out_powerdown_mode_available.dev_attr.attr,
>>> +     &iio_dev_attr_out_scale.dev_attr.attr,
>>> +     &iio_dev_attr_name.dev_attr.attr,
>>> +     NULL,
>>> +};
>>> +
>>>    =20
>> Not convinced you are gaining much here vs just having two attribute
>> tables and picking that based on the id...
>>  =20
> ok
>>> +static mode_t ad5504_attr_is_visible(struct kobject *kobj,
>>> +                                  struct attribute *attr, int n)
>>> +{
>>> +     struct device *dev =3D container_of(kobj, struct device, kobj=
);
>>> +     struct iio_dev *indio_dev =3D dev_get_drvdata(dev);
>>> +     struct ad5504_state *st =3D iio_dev_get_devdata(indio_dev);
>>> +
>>> +     mode_t mode =3D attr->mode;
>>> +
>>> +     if (spi_get_device_id(st->spi)->driver_data =3D=3D ID_AD5501 =
&&
>>> +             (attr =3D=3D &iio_dev_attr_out1_raw.dev_attr.attr ||
>>> +             attr =3D=3D &iio_dev_attr_out2_raw.dev_attr.attr ||
>>> +             attr =3D=3D &iio_dev_attr_out3_raw.dev_attr.attr ||
>>> +             attr =3D=3D &iio_dev_attr_out1_powerdown.dev_attr.att=
r ||
>>> +             attr =3D=3D &iio_dev_attr_out2_powerdown.dev_attr.att=
r ||
>>> +             attr =3D=3D &iio_dev_attr_out3_powerdown.dev_attr.att=
r))
>>> +             mode =3D 0;
>>> +
>>> +     return mode;
>>> +}
>>> +
>>> +static const struct attribute_group ad5504_attribute_group =3D {
>>> +     .attrs =3D ad5504_attributes,
>>> +     .is_visible =3D ad5504_attr_is_visible,
>>> +};
>>> +
>>> +static IIO_CONST_ATTR(out_alarm_overtemp_thresh_high_value, "11000=
0");
>>> +
>>> +static struct attribute *ad5504_ev_attributes[] =3D {
>>> +     &iio_const_attr_out_alarm_overtemp_thresh_high_value.dev_attr=
=2Eattr,
>>>    =20
>> For consistency we also want the _en element, (always 1 of course).
>>  =20
> ok
>>  =20
>>> +     NULL,
>>> +};
>>> +
>>> +static struct attribute_group ad5504_ev_attribute_group =3D {
>>> +     .attrs =3D ad5504_ev_attributes,
>>> +};
>>> +
>>> +static void ad5504_interrupt_bh(struct work_struct *work_s)
>>> +{
>>> +     struct ad5504_state *st =3D container_of(work_s,
>>> +             struct ad5504_state, work_alarm);
>>> +
>>> +     iio_push_event(st->indio_dev, 0,
>>> +                     IIO_UNMOD_EVENT_CODE(IIO_EV_CLASS_OUT,
>>> +                     0,
>>> +                     IIO_EV_TYPE_THRESH,
>>> +                     IIO_EV_DIR_RISING),
>>> +                     st->last_timestamp);
>>>    =20
>> Not happy with this event type. See intro.
>>  =20
> Please propose a better one.
>=20
>>> +
>>> +     enable_irq(st->spi->irq);
>>> +}
>>> +
>>>    =20
>> ...
>>
>>  =20
>>> +#define AD5504_ADDR_ALL_DAC          5
>>> +#define AD5504_ADDR_CTRL             7
>>> +
>>> +/* Control Register */
>>> +#define AD5504_DAC_PWR(ch)           ((ch) << 2)
>>> +#define AD5504_DAC_PWRDWN_MODE(mode) ((mode) << 6)
>>> +#define AD5504_DAC_PWRDN_20K         0
>>> +#define AD5504_DAC_PWRDN_3STATE              1
>>> +
>>> +/*
>>> + * TODO: struct ad5504_platform_data needs to go into include/linu=
x/iio
>>> + */
>>> +
>>> +struct ad5504_platform_data {
>>> +     u16                             vref_mv;
>>> +};
>>>    =20
>> Would it matter to you if we stopped doing this vref passing in plat=
form
>> data and started insisting on regulator always being queriable?  (ca=
me up in
>> the max1363 discussion of yesterday?) When we first started doing th=
is
>> almost no boards had regulators specified. Is this still true?
>>  =20
> The regulator stuff is not really common.
> I always try to support it as an alternative way, since I know passin=
g
> platform dependent information
> through platform_data is a bit inconvenient on systems using devicetr=
ee.=20
> A bandgap reference directly connected to an ADC or DAC, can be
> considered as a fixed voltage regulator.
> However there is no way to enable or disable it. it's a pretty
> complicated way telling the driver it's reference.
> I would propose to always support the regulator framework, however I
> don't mind having the platform_data approach as well.
Ok, lets leave it for now, but definitely do our best to encourage user=
s
to use the regulator framework where possible.  That way we can hopeful=
ly
stop putting this in new drives at some point in the future!
> =20
> =20
>>> +
>>> +/**
>>> + * struct ad5446_state - driver instance specific data
>>> + * @indio_dev:               the industrial I/O device
>>> + * @us:                      spi_device
>>> + * @reg:             supply regulator
>>> + * @vref_mv:         actual reference voltage used
>>> + * @work_alarm:              bh work structure for event handling
>>> + * @last_timestamp:  timestamp of last event interrupt
>>> + * @pwr_down_mask    power down mask
>>> + * @pwr_down_mode    current power down mode
>>> + */
>>> +
>>> +struct ad5504_state {
>>> +     struct iio_dev                  *indio_dev;
>>> +     struct spi_device               *spi;
>>> +     struct regulator                *reg;
>>> +     unsigned short                  vref_mv;
>>> +     struct work_struct              work_alarm;
>>> +     s64                             last_timestamp;
>>> +     unsigned                        pwr_down_mask;
>>> +     unsigned                        pwr_down_mode;
>>> +};
>>> +
>>> +/**
>>> + * ad5504_supported_device_ids:
>>> + */
>>> +
>>> +enum ad5504_supported_device_ids {
>>> +     ID_AD5504,
>>> +     ID_AD5501,
>>> +};
>>> +
>>> +#endif /* SPI_AD5504_H_ */
>>> diff --git a/drivers/staging/iio/sysfs.h b/drivers/staging/iio/sysf=
s.h
>>> index 24b74dd..9e3b784 100644
>>> --- a/drivers/staging/iio/sysfs.h
>>> +++ b/drivers/staging/iio/sysfs.h
>>> @@ -266,6 +266,7 @@ struct iio_const_attr {
>>>  #define IIO_EV_CLASS_MAGN            4
>>>  #define IIO_EV_CLASS_LIGHT           5
>>>  #define IIO_EV_CLASS_PROXIMITY               6
>>> +#define IIO_EV_CLASS_OUT             7
>>>    =20
>> This is fine but I think shouldn't be used here so shouldn't be in t=
he patch!
>>  =20
> ok
>>>  #define IIO_EV_MOD_X                 0
>>>  #define IIO_EV_MOD_Y                 1
>>>    =20
>>  =20
>=20
>=20