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

[Jonathan Cameron <jic23@kernel.org>]

On 13/03/16 19:11, Evgeny Boger 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.
> 
> At present, only internal voltage reference is supported.
> 
> Signed-off-by: Evgeny Boger <boger@contactless.ru>
A couple of tiny points inline.  Nothing signficant.

Jonathan
> ---
>  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@contactless.ru>
> + *
> + * Based on mcp4728 by Peter Meerwald <pmeerw@pmeerw.net>
> + *
> + * 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;
> +};
> +
> +struct mcp4728_channel_state {
> +	u16 dac_value;
> +	bool powerdown;
> +	u8 gain;
> +	u8 vref;
> +	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];
Spacing a bit random looking here.  Just go with a single space
every time as it's a lot easier to maintain without noise if additional
elements are added to this structure.
> +
> +};
> +
> +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
> +*/
> +	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);
> +	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;
> +}
> +
> +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];
> +	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;
> +		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);
> +
One blank line is pretty much always enough.
> +
> +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) {
> +
The blank line here doesn't add much value in readability so drop it.
> +				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->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");
> +		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 */
ADC?
> +		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));
If there really isn't anything else to do in the remove, use
devm_iio_device_register and you can drop the remove function entirely.

> +	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@contactless.ru>");
> +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@contactless.ru>
> + *
> + * 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_ */
>