On 4/29/2012 6:13 PM, Peter Meerwald wrote:
Signed-off-by: Peter Meerwald<pmeerw@xxxxxxxxxx>
Mostly fine. As I explain below you were a little unlucky in which
driver to base
on. Roland's driver is lovely and clean, but is lagging a bit in
interface terms.
Sometime soon we'll bring it up to the latest and greatest.
Sorry about that!
Anyhow, I've made some suggestions inline. What you do with them is
dependent on
how much time you want to spend on this. I'm happy to see it go into
staging/iio
as is, but would want to move to the chan_spec stuff to take it directly
into drivers/iio/
without bounchign through staging.
The only vital change is updating your header locations as some of them
have moved.
At somepoint I suspect someone will add regulator support for that
reference voltage.
Obviously if it is no use to you there is no reason for you to add it!
Jonathan
---
drivers/staging/iio/dac/mcp4725.c | 210 +++++++++++++++++++++++++++++++++++++
drivers/staging/iio/dac/mcp4725.h | 16 +++
2 files changed, 226 insertions(+)
create mode 100644 drivers/staging/iio/dac/mcp4725.c
create mode 100644 drivers/staging/iio/dac/mcp4725.h
diff --git a/drivers/staging/iio/dac/mcp4725.c b/drivers/staging/iio/dac/mcp4725.c
new file mode 100644
index 0000000..8603c66
--- /dev/null
+++ b/drivers/staging/iio/dac/mcp4725.c
@@ -0,0 +1,210 @@
+/*
+ * mcp4725.c - Support for Microchip MCP4725
+ *
+ * Copyright (C) 2012 Peter Meerwald<pmeerw@xxxxxxxxxx>
+ *
+ * Based on max517 by Roland Stigge<stigge@xxxxxxxxx>
That's a little unfortuante as Roland's driver is prior to the change
to doing pretty much everything through iio_chan_spec descriptions
of the channels and read_raw and write_raw callbacks.
The reasoning behind this is that it makes it possible for other drivers
within the kernel to make use of the facilities the device provides.
Still in a nice simple driver like this it won't take too long.
Without that change I'm not happy with this going into drivers/iio/dac
but it could go straight into drivers/staging/iio/dac
+ *
+ * This file is subject to the terms and conditions of version 2 of
+ * the GNU General Public License. See the file COPYING in the main
+ * directory of this archive for more details.
+ *
+ * driver for the Microchip I2C 12-bit digital-to-analog converter (DAC)
+ * (7-bit I2C slave address 0x60, the three LSBs can be configured in
+ * hardware)
+ *
+ * writing the DAC value to EEPROM is not supported
+ */
+
+#include<linux/module.h>
+#include<linux/init.h>
+#include<linux/i2c.h>
+#include<linux/err.h>
+
+#include "../iio.h"
+#include "../sysfs.h"
You are lagging a bit. Please test against the latest staging-next
branch of:
git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git
Note that the core of IIO is out of staging in that tree so we are happy to
take drivers directly into the non staging side if they have nothing messy
or controversial.
+#include "dac.h"
Hohum. That's a header we need to kill off with moving all the remaining
dac drivers over the chan_spec stuff.
+
+#include "mcp4725.h"
+
+#define MCP4725_DRV_NAME "mcp4725"
+
+struct mcp4725_data {
+ struct iio_dev *indio_dev;
+ struct i2c_client *client;
+ unsigned short vref_mv;
+ unsigned short dac_value;
+};
+
+static ssize_t mcp4725_set_value(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct mcp4725_data *data = iio_priv(indio_dev);
+ struct i2c_client *client = data->client;
General point: As you only use client once in the function, I'd just put
data->client into that call and drop this line.
+ u8 outbuf[2];
+ int res;
+ long val;
+
+ res = strict_strtol(buf, 10,&val);
Deprecated function. Please use kstrtol or similar. Otherwise someone
will just post a patch switching it over 2 days after this hits the tree!
+ if (res)
+ return res;
+
+ if (val< 0 || val> 0xfff)
+ return -EINVAL;
+
+ outbuf[0] = (val>> 8)& 0xf;
+ outbuf[1] = val& 0xff;
Might be marginally clearer with outbuf as a u16 then mask and
use cpu_to_le16 ( I think that's the right one - I've not had enough
coffee yet to be sure ;)
+
+ res = i2c_master_send(client, outbuf, 2);
+ if (res< 0)
+ return res;
+
+ data->dac_value = val;
+
+ return count;
+}
+
+static IIO_DEVICE_ATTR(out_voltage_raw, S_IWUSR,
+ NULL, mcp4725_set_value, 0);
+
+static ssize_t mcp4725_show_scale(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct mcp4725_data *data = iio_priv(indio_dev);
+
+ unsigned int scale_uv = (data->vref_mv * 1000)>> 12;
+
+ return sprintf(buf, "%d.%03d\n", scale_uv / 1000, scale_uv % 1000);
+}
+
+static IIO_DEVICE_ATTR(out_voltage_scale, S_IRUGO,
+ mcp4725_show_scale, NULL, 0);
+
+static struct attribute *mcp4725_attributes[] = {
+ &iio_dev_attr_out_voltage_raw.dev_attr.attr,
+ &iio_dev_attr_out_voltage_scale.dev_attr.attr,
+ NULL
+};
+
+static struct attribute_group mcp4725_attribute_group = {
+ .attrs = mcp4725_attributes,
+};
+
+#ifdef CONFIG_PM_SLEEP
+static int mcp4725_suspend(struct device *dev)
+{
+ u8 outbuf[2];
+
+ outbuf[0] = 0x3<< 4; /* power-down bits, 500 kOhm resistor */
+ outbuf[1] = 0;
+
+ return i2c_master_send(to_i2c_client(dev),&outbuf, 2);
+}
+
+static int mcp4725_resume(struct device *dev)
+{
+ struct iio_dev *indio_dev = dev_get_drvdata(dev);
+ struct mcp4725_data *data = iio_priv(indio_dev);
+ u8 outbuf[2];
+
+ /* restore previous DAC value */
+ outbuf[0] = (data->dac_value>> 8)& 0xf;
+ outbuf[1] = data->dac_value& 0xff;
+
+ return i2c_master_send(to_i2c_client(dev),&outbuf, 2);
+}
+
+static SIMPLE_DEV_PM_OPS(mcp4725_pm_ops, mcp4725_suspend, mcp4725_resume);
+#define MCP4725_PM_OPS (&mcp4725_pm_ops)
+#else
+#define MCP4725_PM_OPS NULL
+#endif
+
+static const struct iio_info mcp4725_info = {
+ .attrs =&mcp4725_attribute_group,
+ .driver_module = THIS_MODULE,
+};
+
+static int mcp4725_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct mcp4725_data *data;
+ struct iio_dev *indio_dev;
+ struct mcp4725_platform_data *platform_data = client->dev.platform_data;
+ u8 inbuf[3];
+ int err;
+
+ if (!platform_data || !platform_data->vref_mv) {
+ dev_err(&client->dev, "invalid platform data");
+ err = -EINVAL;
+ goto exit;
+ }
+
+ indio_dev = iio_allocate_device(sizeof(*data));
+ if (indio_dev == NULL) {
+ err = -ENOMEM;
+ goto exit;
+ }
+ data = iio_priv(indio_dev);
+ i2c_set_clientdata(client, indio_dev);
+ data->client = client;
+
+ indio_dev->dev.parent =&client->dev;
+ indio_dev->info =&mcp4725_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+
+ data->vref_mv = platform_data->vref_mv;
+
+ /* read current DAC value */
+ err = i2c_master_recv(client, inbuf, 3);
+ if (err< 0) {
+ dev_err(&client->dev, "failed to read DAC value");
+ goto exit_free_device;
+ }
+ data->dac_value = (inbuf[1]<< 4) | (inbuf[2]>> 4);
+
+ err = iio_device_register(indio_dev);
+ if (err)
+ goto exit_free_device;
+
+ dev_info(&client->dev, "MCP4725 DAC registered\n");
+
+ return 0;
+
+exit_free_device:
+ iio_free_device(indio_dev);
+exit:
+ return err;
+}
+
+static int mcp4725_remove(struct i2c_client *client)
+{
+ iio_free_device(i2c_get_clientdata(client));
+
+ return 0;
+}
+
+static const struct i2c_device_id mcp4725_id[] = {
+ { "mcp4725", 0 },
+ { }
+};
+MODULE_DEVICE_TABLE(i2c, mcp4725_id);
+
+static struct i2c_driver mcp4725_driver = {
+ .driver = {
+ .name = MCP4725_DRV_NAME,
+ .pm = MCP4725_PM_OPS,
+ },
+ .probe = mcp4725_probe,
+ .remove = mcp4725_remove,
+ .id_table = mcp4725_id,
+};
+module_i2c_driver(mcp4725_driver);
+
+MODULE_AUTHOR("Peter Meerwald<pmeerw@xxxxxxxxxx>");
+MODULE_DESCRIPTION("MCP4725 12-bit DAC");
+MODULE_LICENSE("GPL");
diff --git a/drivers/staging/iio/dac/mcp4725.h b/drivers/staging/iio/dac/mcp4725.h
new file mode 100644
index 0000000..91530e6
--- /dev/null
+++ b/drivers/staging/iio/dac/mcp4725.h
@@ -0,0 +1,16 @@
+/*
+ * MCP4725 DAC driver
+ *
+ * Copyright (C) 2012 Peter Meerwald<pmeerw@xxxxxxxxxx>
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#ifndef IIO_DAC_MCP4725_H_
+#define IIO_DAC_MCP4725_H_
+
+struct mcp4725_platform_data {
+ u16 vref_mv;
+};
+
+#endif /* IIO_DAC_MCP4725_H_ */
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
