Re: [PATCH] iio: dac: mcp4728: Add support for MCP4728

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thank you for your comments! See below

On 03/16/2016 02:02 AM, Peter Meerwald-Stadler wrote:
Microchip MCP4728 is a 12-bit quad channel
digital-to-analog converter (DAC) with I2C interface.

This patch adds support for most of the features of MCP4728,
including per-channel gain, power state and power down mode control.
Driver also supports saving the current state to on-chip EEPROM.
since Jonathan seems to be OK with a separate driver, here are my review
comments...
Signed-off-by: Evgeny Boger <boger@xxxxxxxxxxxxxx>
---
  drivers/iio/dac/Kconfig         |  11 +
  drivers/iio/dac/Makefile        |   1 +
  drivers/iio/dac/mcp4728.c       | 479 ++++++++++++++++++++++++++++++++++++++++
  include/linux/iio/dac/mcp4728.h |  16 ++
  4 files changed, 507 insertions(+)
  create mode 100644 drivers/iio/dac/mcp4728.c
  create mode 100644 include/linux/iio/dac/mcp4728.h

diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index e701e28..8da6039 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -186,6 +186,17 @@ config MCP4725
  	  To compile this driver as a module, choose M here: the module
  	  will be called mcp4725.
+config MCP4728
+	tristate "MCP4728 DAC driver"
+	depends on I2C
+	---help---
+	  Say Y here if you want to build a driver for the Microchip
+	  MCP 4728 12-bit quad digital-to-analog converter (DAC) with I2C
+	  interface.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called mcp4728.
+
  config MCP4922
  	tristate "MCP4902, MCP4912, MCP4922 DAC driver"
  	depends on SPI
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 63ae056..2c9decc 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -20,4 +20,5 @@ obj-$(CONFIG_M62332) += m62332.o
  obj-$(CONFIG_MAX517) += max517.o
  obj-$(CONFIG_MAX5821) += max5821.o
  obj-$(CONFIG_MCP4725) += mcp4725.o
+obj-$(CONFIG_MCP4728) += mcp4728.o
  obj-$(CONFIG_MCP4922) += mcp4922.o
diff --git a/drivers/iio/dac/mcp4728.c b/drivers/iio/dac/mcp4728.c
new file mode 100644
index 0000000..6dbbfac
--- /dev/null
+++ b/drivers/iio/dac/mcp4728.c
@@ -0,0 +1,479 @@
+/*
+ * mcp4728.c - Support for Microchip MCP4728
+ *
+ * Copyright (C) 2016 Evgeny Boger <boger@xxxxxxxxxxxxxx>
+ *
+ * Based on mcp4728 by Peter Meerwald <pmeerw@xxxxxxxxxx>
+ *
+ * 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 MCP4728 I2C 12-bit quad digital-to-analog
+ * converter (DAC)
+ * (7-bit I2C slave address 0x60, the three LSBs can be configured in
+ * hardware)
+ */
+
+#include <linux/module.h>
+#include <linux/i2c.h>
+#include <linux/err.h>
+#include <linux/delay.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+
+#include <linux/iio/dac/mcp4728.h>
+
+#define MCP4728_DRV_NAME "mcp4728"
+#define MCP4728_NUM_CHANNELS 4
+#define MCP4728_INT_VREF_MV 2048
+#define MCP4728_RESOLUTION 12
+
+struct mcp4728_scale {
+	unsigned int		integer;
+	unsigned int		micro;
use just a single space

+};
+
+struct mcp4728_channel_state {
+	u16 dac_value;
+	bool powerdown;
+	u8 gain;
+	u8 vref;
could this be bool?
u8 vref => bool int_vref
to clarify it doesn't store the value of reference voltage

+	unsigned powerdown_mode;
+};
+
+struct mcp4728_data {
+	struct i2c_client *client;
+	struct mcp4728_channel_state channel_state[MCP4728_NUM_CHANNELS];
+	struct mcp4728_scale	scale_avail[MCP4728_NUM_CHANNELS][2];
+
+};
+
+static int mcp4728_write_reg(struct mcp4728_data *data, int ch, bool eeprom)
+{
+	struct mcp4728_channel_state *state = &data->channel_state[ch];
+	u8 outbuf[3];
+	u8 pd;
+	int ret;
+
+	if (state->dac_value >= (1 << MCP4728_RESOLUTION) ||
+				state->dac_value < 0)
+		return -EINVAL;
+/*
+  Write input register. EEPROM is not updated:
+  c2=0, c1=1, c0=0, w1=0, w0=0 : 0x40
+
+  Write input and EEPROM registers:
+  c2=0, c1=1, c0=0, w1=1, w0=1 : 0x58
+
+  b[0]: c2 | c1 | c0 | w1 | w0 | dac1 | dac0 | /udac
+  b[1]: vref | pd1 | pd0 | gain | d11 | d10 | d9 | d8
+  b[2]: d7-d0
+*/
maybe indent the block above

+	pd = state->powerdown ? (state->powerdown_mode + 1) : 0;
+
+	outbuf[0] = (eeprom ? 0x58 : 0x40) |
+				((ch & 0x03) << 1);
+	outbuf[1] = ((state->vref & 0x01) << 7) |
+				((pd & 0x3) << 5)    |
+				((state->gain & 0x01) << 4) |
+				((state->dac_value >> 8) & 0x0f);
+	outbuf[2] = state->dac_value & 0xff;
+
+	ret = i2c_master_send(data->client, outbuf, 3);
3 could be replaced by sizeof(outbuf), here and below

+	if (ret < 0)
+		return ret;
+	else if (ret != 3)
+		return -EIO;
+	else
+		return 0;
+}
+
+static int mcp4728_write_input_reg(struct mcp4728_data *data, int ch)
+{
+	return mcp4728_write_reg(data, ch, false);
+}
+
+static int mcp4728_suspend(struct device *dev)
+{
+	struct mcp4728_data *data = iio_priv(i2c_get_clientdata(
+		to_i2c_client(dev)));
+	int ch, ret;
+
+	for (ch = 0; ch < MCP4728_NUM_CHANNELS; ++ch) {
+		data->channel_state[ch].powerdown = true;
+		ret = mcp4728_write_input_reg(data, ch);
+		if (ret < 0)
+			return ret;
+	}
+
+	return ret;
+}
+
_suspend()/_resume() code is almost identical, split out common code?

On a second thought: it seems like the present _suspend/_resume logic is somewhat flawed. Suppose user has manually switched off some of the channels via sysfs. In this case, one would expect the driver to restore the configured state after suspend/resume cycle, i.e. keep some of the channels in powerdown state. So if there are no objections, I'm going to rewrite this part to store the current power state for each channel on suspend and restore it on resume.



+static int mcp4728_resume(struct device *dev)
+{
+	struct mcp4728_data *data = iio_priv(i2c_get_clientdata(
+		to_i2c_client(dev)));
+	int ch, ret;
+
+	for (ch = 0; ch < MCP4728_NUM_CHANNELS; ++ch) {
+		data->channel_state[ch].powerdown = false;
+		ret = mcp4728_write_input_reg(data, ch);
+		if (ret < 0)
+			return ret;
+	}
+
+	return ret;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static SIMPLE_DEV_PM_OPS(mcp4728_pm_ops, mcp4728_suspend, mcp4728_resume);
+#define MCP4728_PM_OPS (&mcp4728_pm_ops)
+#else
+#define MCP4728_PM_OPS NULL
+#endif
+
+static ssize_t mcp4728_store_eeprom(struct device *dev,
+	struct device_attribute *attr, const char *buf, size_t len)
+{
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct mcp4728_data *data = iio_priv(indio_dev);
+	int tries;
+	u8 inbuf[3];
actually just one byte is used?

+	bool state;
+	int ret, ch;
+
+	ret = strtobool(buf, &state);
+	if (ret < 0)
+		return ret;
+
+	if (!state)
+		return 0;
+
+	for (ch = 0; ch < MCP4728_NUM_CHANNELS; ++ch) {
+		tries = 20;
make the variable 'tries' local to the loop?
maybe 'inbuf' as well

+		mcp4728_write_reg(data, ch, true);
+
+
+		/* wait for write complete, takes up to 50ms */
+		while (tries--) {
+			msleep(20);
+			ret = i2c_master_recv(data->client, inbuf, 1);
+			if (ret < 0)
+				return ret;
+			else if (ret != 1)
+				return -EIO;
+
+			if (inbuf[0] & 0x80)
+				break;
+		}
+
+		if (tries < 0) {
+			dev_err(&data->client->dev,
+				"mcp4728_store_eeprom() failed, incomplete\n");
+			return -EIO;
+		}
+	}
+
+	return len;
+}
+
+static IIO_DEVICE_ATTR(store_eeprom, S_IWUSR, NULL, mcp4728_store_eeprom, 0);
+
+
+static ssize_t mcp4728_show_scale_available(struct device *dev,
+		struct device_attribute *attr,
+		char *buf)
+{
+	struct iio_dev_attr *iio_attr = to_iio_dev_attr(attr);
+	struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+	struct mcp4728_data *data = iio_priv(indio_dev);
+	u8 ch = iio_attr->address;
+	int i, len = 0;
+
+	for (i = 0; i < ARRAY_SIZE(data->scale_avail[ch]); i++)
+		len += sprintf(buf + len, "%u.%06u ",
+			       data->scale_avail[ch][i].integer,
+			       data->scale_avail[ch][i].micro);
+
+	len += sprintf(buf + len, "\n");
+
+	return len;
+}
+
+#define SHOW_SCALE_AVAILABLE_ATTR(ch)					\
+static IIO_DEVICE_ATTR(out_voltage##ch##_scale_available, S_IRUGO,	\
+		       mcp4728_show_scale_available, NULL, ch)
+
+SHOW_SCALE_AVAILABLE_ATTR(0);
+SHOW_SCALE_AVAILABLE_ATTR(1);
+SHOW_SCALE_AVAILABLE_ATTR(2);
+SHOW_SCALE_AVAILABLE_ATTR(3);
+
+static struct attribute *mcp4728_attributes[] = {
+	&iio_dev_attr_store_eeprom.dev_attr.attr,
+	&iio_dev_attr_out_voltage0_scale_available.dev_attr.attr,
+	&iio_dev_attr_out_voltage1_scale_available.dev_attr.attr,
+	&iio_dev_attr_out_voltage2_scale_available.dev_attr.attr,
+	&iio_dev_attr_out_voltage3_scale_available.dev_attr.attr,
+	NULL,
+};
+
+static const struct attribute_group mcp4728_attribute_group = {
+	.attrs = mcp4728_attributes,
+};
+
+static const char * const mcp4728_powerdown_modes[] = {
+	"1kohm_to_gnd",
+	"100kohm_to_gnd",
+	"500kohm_to_gnd"
+};
+
+static int mcp4728_get_powerdown_mode(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan)
+{
+	struct mcp4728_data *data = iio_priv(indio_dev);
+
+	return data->channel_state[chan->channel].powerdown_mode;
+}
+
+static int mcp4728_set_powerdown_mode(struct iio_dev *indio_dev,
+	const struct iio_chan_spec *chan, unsigned mode)
+{
+	struct mcp4728_data *data = iio_priv(indio_dev);
+
+	data->channel_state[chan->channel].powerdown_mode = mode;
+
+	return 0;
+}
+
+static ssize_t mcp4728_read_powerdown(struct iio_dev *indio_dev,
+	uintptr_t private, const struct iio_chan_spec *chan, char *buf)
+{
+	struct mcp4728_data *data = iio_priv(indio_dev);
+
+	return sprintf(buf, "%d\n",
+		data->channel_state[chan->channel].powerdown);
+}
+
+static ssize_t mcp4728_write_powerdown(struct iio_dev *indio_dev,
+	 uintptr_t private, const struct iio_chan_spec *chan,
+	 const char *buf, size_t len)
+{
+	struct mcp4728_data *data = iio_priv(indio_dev);
+	struct mcp4728_channel_state *state =
+			&data->channel_state[chan->channel];
+	bool powerdown;
+	int ret;
+
+	ret = strtobool(buf, &powerdown);
+	if (ret)
+		return ret;
+
+	state->powerdown = powerdown;
+	ret = mcp4728_write_input_reg(data, chan->channel);
+
+	if (ret < 0)
+		return ret;
+
+	return len;
+}
+
+static const struct iio_enum mcp4728_powerdown_mode_enum = {
+	.items = mcp4728_powerdown_modes,
+	.num_items = ARRAY_SIZE(mcp4728_powerdown_modes),
+	.get = mcp4728_get_powerdown_mode,
+	.set = mcp4728_set_powerdown_mode,
+};
+
+static const struct iio_chan_spec_ext_info mcp4728_ext_info[] = {
+	{
+		.name = "powerdown",
+		.read = mcp4728_read_powerdown,
+		.write = mcp4728_write_powerdown,
+		.shared = IIO_SEPARATE,
+	},
+	IIO_ENUM("powerdown_mode", IIO_SEPARATE, &mcp4728_powerdown_mode_enum),
+	IIO_ENUM_AVAILABLE("powerdown_mode", &mcp4728_powerdown_mode_enum),
+	{ },
+};
+
+#define MCP4728_CHAN(chan) {			\
+	.type		= IIO_VOLTAGE, \
+	.indexed	= 1, \
+	.output		= 1, \
+	.channel	= chan, \
+	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+			      BIT(IIO_CHAN_INFO_SCALE), \
+	.ext_info	= mcp4728_ext_info, \
+}
+
+
+static const struct iio_chan_spec mcp4728_channels[MCP4728_NUM_CHANNELS] = {
+	MCP4728_CHAN(0),
+	MCP4728_CHAN(1),
+	MCP4728_CHAN(2),
+	MCP4728_CHAN(3),
+};
+
+static int mcp4728_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long mask)
+{
+	struct mcp4728_data *data = iio_priv(indio_dev);
+	struct mcp4728_channel_state *state =
+			&data->channel_state[chan->channel];
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		*val = data->channel_state[chan->channel].dac_value;
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		*val = (state->gain + 1) * MCP4728_INT_VREF_MV;
+		*val2 = MCP4728_RESOLUTION;
+		return IIO_VAL_FRACTIONAL_LOG2;
+	}
+	return -EINVAL;
+}
+
+static int mcp4728_write_raw(struct iio_dev *indio_dev,
+			       struct iio_chan_spec const *chan,
+			       int val, int val2, long mask)
+{
+	struct mcp4728_data *data = iio_priv(indio_dev);
+	struct mcp4728_channel_state *state =
+			&data->channel_state[chan->channel];
+	struct mcp4728_scale *scale_avail =
+		data->scale_avail[chan->channel];
+
+	int ret, gain;
+
+	switch (mask) {
+	case IIO_CHAN_INFO_RAW:
+		state->dac_value = val;
+		ret = mcp4728_write_input_reg(data, chan->channel);
+		break;
+	case IIO_CHAN_INFO_SCALE:
+		ret = -EINVAL;
+
+		for (gain = 0; gain < 2; ++gain) {
+			if (val == scale_avail[gain].integer &&
+			val2 == scale_avail[gain].micro) {
+
+				state->gain = gain;
+				ret = mcp4728_write_input_reg(data,
+					chan->channel);
+			}
+		}
+
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
+static const struct iio_info mcp4728_info = {
+	.read_raw = mcp4728_read_raw,
+	.write_raw = mcp4728_write_raw,
+	.attrs = &mcp4728_attribute_group,
+	.driver_module = THIS_MODULE,
+};
+
+static int mcp4728_probe(struct i2c_client *client,
+			 const struct i2c_device_id *id)
+{
+	struct mcp4728_data *data;
+	struct iio_dev *indio_dev;
+	u8 inbuf[24];
+	u8 *reg;
+	u8 pd;
+	int err;
+	int i, gain;
+	u64 scale_uv;
+
+	indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
+	if (indio_dev == NULL)
+		return -ENOMEM;
+	data = iio_priv(indio_dev);
+	i2c_set_clientdata(client, indio_dev);
+	data->client = client;
+
+	indio_dev->dev.parent = &client->dev;
+	indio_dev->info = &mcp4728_info;
indio_dev->name ?

+	indio_dev->channels = mcp4728_channels;
+	indio_dev->num_channels = MCP4728_NUM_CHANNELS;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+
+	/* read current DAC value */
+	err = i2c_master_recv(client, inbuf, 24);
+	if (err < 0) {
+		dev_err(&client->dev, "failed to read DAC value");
missing \n

+		return err;
+	}
+
+	for (i = 0; i < MCP4728_NUM_CHANNELS; ++i) {
+		reg = &inbuf[6*i];
+		pd = (reg[1] >> 5) & 0x3;
+		data->channel_state[i].powerdown = pd > 0 ? true : false;
+
+		/* set to 500kohm_to_gnd if channel is now powered down */
+		data->channel_state[i].powerdown_mode = pd ? pd-1 : 2;
+		data->channel_state[i].dac_value = ((reg[1] & 0x0f) << 8) |
+						   (reg[2]);
+		data->channel_state[i].gain = (reg[1] >> 4) & 0x01;
+
+		/* Use internal reference */
+		data->channel_state[i].vref = 1;
+
+		/* Populate available ADC input ranges */
+		for (gain = 0; gain < 2; gain++) {
+			/*
+			  [gain=0] = Gain Setting 1, vref x 1
+			  [gain=1] = Gain Setting 2, vref x 2
+
+			  The scale is calculated by doing:
+			    Vref >> (realbits - gain)
+			  which multiplies by two on the second component
+			  of the array.
+			 */
+			scale_uv = ((u64) MCP4728_INT_VREF_MV * 100000) >>
+				   (MCP4728_RESOLUTION - gain);
+			data->scale_avail[i][gain].micro =
+					do_div(scale_uv, 100000) * 10;
+			data->scale_avail[i][gain].integer = scale_uv;
+		}
+	}
+
+	return iio_device_register(indio_dev);
+}
+
+static int mcp4728_remove(struct i2c_client *client)
+{
+	iio_device_unregister(i2c_get_clientdata(client));
+	return 0;
+}
+
+static const struct i2c_device_id mcp4728_id[] = {
+	{ "mcp4728", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, mcp4728_id);
+
+static struct i2c_driver mcp4728_driver = {
+	.driver = {
+		.name	= MCP4728_DRV_NAME,
+		.pm	= MCP4728_PM_OPS,
+	},
+	.probe		= mcp4728_probe,
+	.remove		= mcp4728_remove,
+	.id_table	= mcp4728_id,
+};
+module_i2c_driver(mcp4728_driver);
+
+MODULE_AUTHOR("Evgeny Boger <boger@xxxxxxxxxxxxxx>");
+MODULE_DESCRIPTION("MCP4728 quad 12-bit DAC");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/iio/dac/mcp4728.h b/include/linux/iio/dac/mcp4728.h
new file mode 100644
index 0000000..14adb0c
--- /dev/null
+++ b/include/linux/iio/dac/mcp4728.h
@@ -0,0 +1,16 @@
+/*
+ * MCP4728 DAC driver
+ *
+ * Copyright (C) 2016 Evgeny Boger <boger@xxxxxxxxxxxxxx>
+ *
+ * Licensed under the GPL-2 or later.
+ */
+
+#ifndef IIO_DAC_MCP4728_H_
+#define IIO_DAC_MCP4728_H_
+
+struct mcp4728_platform_data {
+	u16 vref_mv;
+};
+
+#endif /* IIO_DAC_MCP4728_H_ */



--
С уважением,
    Евгений Богер / Evgeny Boger
    Wiren Board Team
    ООО Бесконтактные устройства
    http://contactless.ru
    +7 919 965 88 36

--
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



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Input]     [Linux Kernel]     [Linux SCSI]     [X.org]

  Powered by Linux