Re: [PATCH 2/2] iio: dac: Add Texas Instruments 8/10/12-bit 2/4-channel DAC driver

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 5 Sep 2017 11:27:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> The DACrrcS085 (rr = 08/10/12, c = 2/4) family of SPI DACs was
> inherited by TI when they acquired National Semiconductor in 2011.
> This driver was developed for and tested with the DAC082S085 built into
> the Revolution Pi by KUNBUS, but should work with any of the other
> chips as they share the same programming interface.
> 
> There is also a family of I2C DACs with just a single channel called
> DACrr1C08x (rr = 08/10/12, x = 1/5).  Their programming interface is
> very similar and it should be possible to extend the driver for these
> chips with moderate effort.  Alternatively they could be integrated into
> ad5446.c.  (The AD5301/AD5311/AD5321 use different power-down modes but
> otherwise appear to be comparable.)
> 
> Furthermore there is a family of 8-channel DACs called DACrr8S085
> (rr = 08/10/12) as well as two 16-bit DACs called DAC161Sxxx
> (xxx = 055/997).  These are more complicated devices with support for
> daisy-chaining and the ability to power down each channel separately.
> They could either be handled by a separate driver or integrated into the
> present driver with a larger effort.
> 
> Cc: Mathias Duckeck <m.duckeck@kunbus.de>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>

Very nice.  A few really minor comments inline.

Thanks,

Jonathan
> ---
>  Documentation/ABI/testing/sysfs-bus-iio |   1 +
>  drivers/iio/dac/Kconfig                 |  10 +
>  drivers/iio/dac/Makefile                |   1 +
>  drivers/iio/dac/ti-dac082s085.c         | 347 ++++++++++++++++++++++++++++++++
>  4 files changed, 359 insertions(+)
>  create mode 100644 drivers/iio/dac/ti-dac082s085.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio b/Documentation/ABI/testing/sysfs-bus-iio
> index 7eead5f97e02..407e4d1024ee 100644
> --- a/Documentation/ABI/testing/sysfs-bus-iio
> +++ b/Documentation/ABI/testing/sysfs-bus-iio
> @@ -522,6 +522,7 @@ Description:
>  		Specifies the output powerdown mode.
>  		DAC output stage is disconnected from the amplifier and
>  		1kohm_to_gnd: connected to ground via an 1kOhm resistor,
> +		2.5kohm_to_gnd: connected to ground via a 2.5kOhm resistor,
>  		6kohm_to_gnd: connected to ground via a 6kOhm resistor,
>  		20kohm_to_gnd: connected to ground via a 20kOhm resistor,
>  		90kohm_to_gnd: connected to ground via a 90kOhm resistor,
> diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
> index 25bed2d7d2b9..716fcb1610dd 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -300,6 +300,16 @@ config STM32_DAC
>  config STM32_DAC_CORE
>  	tristate
>  
> +config TI_DAC082S085
> +	tristate "Texas Instruments 8/10/12-bit 2/4-channel DAC driver"
> +	depends on SPI_MASTER
> +	help
> +	  Driver for the Texas Instruments (formerly National Semiconductor)
> +	  DAC082S085, DAC102S085, DAC122S085, DAC084S085, DAC104S085 and
> +	  DAC124S085.
> +
> +	  If compiled as a module, it will be called ti-dac082s085.
> +
>  config VF610_DAC
>  	tristate "Vybrid vf610 DAC driver"
>  	depends on OF
> diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
> index 603587cc2f07..1a2dd507b4d8 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -32,4 +32,5 @@ obj-$(CONFIG_MCP4725) += mcp4725.o
>  obj-$(CONFIG_MCP4922) += mcp4922.o
>  obj-$(CONFIG_STM32_DAC_CORE) += stm32-dac-core.o
>  obj-$(CONFIG_STM32_DAC) += stm32-dac.o
> +obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o
>  obj-$(CONFIG_VF610_DAC) += vf610_dac.o
> diff --git a/drivers/iio/dac/ti-dac082s085.c b/drivers/iio/dac/ti-dac082s085.c
> new file mode 100644
> index 000000000000..22fdce512a09
> --- /dev/null
> +++ b/drivers/iio/dac/ti-dac082s085.c
> @@ -0,0 +1,347 @@
> +/*
> + * ti-dac082s085.c - Texas Instruments 8/10/12-bit 2/4-channel DAC driver
> + *
> + * Copyright (C) 2017 KUNBUS GmbH
> + *
> + * http://www.ti.com/lit/ds/symlink/dac082s085.pdf
> + * http://www.ti.com/lit/ds/symlink/dac102s085.pdf
> + * http://www.ti.com/lit/ds/symlink/dac122s085.pdf
> + * http://www.ti.com/lit/ds/symlink/dac084s085.pdf
> + * http://www.ti.com/lit/ds/symlink/dac104s085.pdf
> + * http://www.ti.com/lit/ds/symlink/dac124s085.pdf
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License (version 2) as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/iio/iio.h>
> +#include <linux/module.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +/**
> + * struct ti_dac_chip - TI DAC chip
> + * @lock: protects write sequences
> + * @vref: regulator generating Vref
> + * @mesg: SPI message to perform a write
> + * @xfer: SPI transfer used by @mesg
> + * @buf: buffer for @xfer
> + * @val: cached value of each output
> + * @powerdown: whether the chip is powered down
> + * @powerdown_mode: selected by the user
> + * @resolution: resolution of the chip
> + */
> +struct ti_dac_chip {
> +	struct mutex lock;
> +	struct regulator *vref;
> +	struct spi_message mesg;
> +	struct spi_transfer xfer;
> +	u8 buf[2] ____cacheline_aligned;
> +	u16 val[4];
> +	bool powerdown;
> +	u8 powerdown_mode;
> +	u8 resolution;
> +};
> +
> +#define WRITE_NOT_UPDATE(chan)	(0x00 | (chan) << 6)
> +#define WRITE_AND_UPDATE(chan)	(0x10 | (chan) << 6)
> +#define WRITE_ALL_UPDATE	 0x20
> +#define POWERDOWN(mode) 	(0x30 | ((mode) + 1) << 6)
> +
> +static int ti_dac_cmd(struct ti_dac_chip *ti_dac, u8 cmd, u16 val)
> +{
> +	u8 shift = 12 - ti_dac->resolution;
> +
> +	ti_dac->buf[0] = cmd | (val >> (8 - shift));
> +	ti_dac->buf[1] = (val << shift) & 0xff;
> +	return spi_sync(ti_dac->mesg.spi, &ti_dac->mesg);
> +}
> +
> +static const char * const ti_dac_powerdown_modes[] = {
> +	"2.5kohm_to_gnd", "100kohm_to_gnd", "three_state",
> +};
> +
> +static int ti_dac_get_powerdown_mode(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan)
> +{
> +	struct ti_dac_chip *ti_dac = iio_priv(indio_dev);
> +
> +	return ti_dac->powerdown_mode;
> +}
> +
> +static int ti_dac_set_powerdown_mode(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     unsigned int mode)
> +{
> +	struct ti_dac_chip *ti_dac = iio_priv(indio_dev);
> +	int ret;
> +
> +	if (ti_dac->powerdown_mode == mode)
> +		return 0;
> +
> +	mutex_lock(&ti_dac->lock);
> +	if (ti_dac->powerdown) {
> +		ret = ti_dac_cmd(ti_dac, POWERDOWN(mode), 0);
> +		if (ret) {

I'd slightly prefer a goto to use the unlock already found below.

> +			mutex_unlock(&ti_dac->lock);
> +			return ret;
> +		}
> +	}
> +	ti_dac->powerdown_mode = mode;
> +	mutex_unlock(&ti_dac->lock);

nitpick time : blank line here

> +	return 0;
> +}
> +
> +static const struct iio_enum ti_dac_powerdown_mode = {
> +	.items = ti_dac_powerdown_modes,
> +	.num_items = ARRAY_SIZE(ti_dac_powerdown_modes),
> +	.get = ti_dac_get_powerdown_mode,
> +	.set = ti_dac_set_powerdown_mode,
> +};
> +
> +static ssize_t ti_dac_read_powerdown(struct iio_dev *indio_dev,
> +				     uintptr_t private,
> +				     const struct iio_chan_spec *chan,
> +				     char *buf)
> +{
> +	struct ti_dac_chip *ti_dac = iio_priv(indio_dev);
> +
> +	return sprintf(buf, "%d\n", ti_dac->powerdown);
> +}
> +
> +static ssize_t ti_dac_write_powerdown(struct iio_dev *indio_dev,
> +				      uintptr_t private,
> +				      const struct iio_chan_spec *chan,
> +				      const char *buf, size_t len)
> +{
> +	struct ti_dac_chip *ti_dac = iio_priv(indio_dev);
> +	bool powerdown;
> +	int ret;
> +
> +	ret = strtobool(buf, &powerdown);
> +	if (ret)
> +		return ret;
> +
> +	if (ti_dac->powerdown == powerdown)
> +		return len;
> +
> +	mutex_lock(&ti_dac->lock);
> +	if (powerdown)
> +		ret = ti_dac_cmd(ti_dac, POWERDOWN(ti_dac->powerdown_mode), 0);
> +	else
> +		ret = ti_dac_cmd(ti_dac, WRITE_AND_UPDATE(0), ti_dac->val[0]);
> +	if (!ret)
> +		ti_dac->powerdown = powerdown;
> +	mutex_unlock(&ti_dac->lock);
> +
> +	return ret ? ret : len;
> +}
> +
> +static const struct iio_chan_spec_ext_info ti_dac_ext_info[] = {
> +	{
> +		.name	   = "powerdown",
> +		.read	   = ti_dac_read_powerdown,
> +		.write	   = ti_dac_write_powerdown,
> +		.shared	   = IIO_SHARED_BY_TYPE,
> +	},
> +	IIO_ENUM("powerdown_mode", IIO_SHARED_BY_TYPE, &ti_dac_powerdown_mode),
> +	IIO_ENUM_AVAILABLE("powerdown_mode", &ti_dac_powerdown_mode),
> +	{ },
> +};
> +
> +#define TI_DAC_CHANNEL(chan) {					\
> +	.type = IIO_VOLTAGE,					\
> +	.channel = (chan),					\
> +	.address = (chan),					\
> +	.indexed = true,					\
> +	.output = true,						\
> +	.info_mask_separate = BIT(IIO_CHAN_INFO_RAW),		\
> +	.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE),	\
> +	.ext_info = ti_dac_ext_info,				\
> +}
> +
> +static const struct iio_chan_spec ti_dac_channels[] = {
> +	TI_DAC_CHANNEL(0),
> +	TI_DAC_CHANNEL(1),
> +	TI_DAC_CHANNEL(2),
> +	TI_DAC_CHANNEL(3),
> +};
> +
> +static int ti_dac_read_raw(struct iio_dev *indio_dev,
> +			   struct iio_chan_spec const *chan,
> +			   int *val, int *val2, long mask)
> +{
> +	struct ti_dac_chip *ti_dac = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		*val = ti_dac->val[chan->channel];
> +		ret = IIO_VAL_INT;
> +		break;
> +
> +	case IIO_CHAN_INFO_SCALE:
> +		ret = regulator_get_voltage(ti_dac->vref);
> +		if (ret < 0)
> +			return ret;
> +
> +		*val = ret / 1000;
> +		*val2 = ti_dac->resolution;
> +		ret = IIO_VAL_FRACTIONAL_LOG2;
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int ti_dac_write_raw(struct iio_dev *indio_dev,
> +			    struct iio_chan_spec const *chan,
> +			    int val, int val2, long mask)
> +{
> +	struct ti_dac_chip *ti_dac = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_RAW:
> +		if (ti_dac->val[chan->channel] == val)
> +			return 0;
> +
> +		if (val >= (1 << ti_dac->resolution) || val < 0)
> +			return -EINVAL;
> +
> +		if (ti_dac->powerdown)
> +			return -EHOSTDOWN;

Hmm. That's a relatively unusual error to find in a driver.
I'd have gone with -EBUSY to indicate that you can't do it now, but might
be able to in future..

> +
> +		mutex_lock(&ti_dac->lock);
> +		ret = ti_dac_cmd(ti_dac, WRITE_AND_UPDATE(chan->channel), val);
> +		if (!ret)
> +			ti_dac->val[chan->channel] = val;
> +		mutex_unlock(&ti_dac->lock);
> +		break;
> +
> +	default:
> +		ret = -EINVAL;
> +	}
> +
> +	return ret;
> +}
> +
> +static int ti_dac_write_raw_get_fmt(struct iio_dev *indio_dev,
> +				    struct iio_chan_spec const *chan, long mask)
> +{
> +	return IIO_VAL_INT;
> +}
> +
> +static const struct iio_info ti_dac_info = {
> +	.read_raw	   = ti_dac_read_raw,
> +	.write_raw	   = ti_dac_write_raw,
> +	.write_raw_get_fmt = ti_dac_write_raw_get_fmt,
> +};
> +
> +static int ti_dac_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct ti_dac_chip *ti_dac;
> +	struct iio_dev *indio_dev;
> +	int ret;
> +
> +	indio_dev = devm_iio_device_alloc(dev, sizeof(*ti_dac));
> +	if (!indio_dev)
> +		return -ENOMEM;
> +
> +	indio_dev->dev.parent = dev;
> +	indio_dev->info = &ti_dac_info;
> +	indio_dev->name = spi->modalias;
> +	indio_dev->modes = INDIO_DIRECT_MODE;
> +	indio_dev->channels = ti_dac_channels;
> +	spi_set_drvdata(spi, indio_dev);
> +
> +	ti_dac = iio_priv(indio_dev);
> +	ti_dac->xfer.tx_buf = &ti_dac->buf;
> +	ti_dac->xfer.len = sizeof(ti_dac->buf);
> +	spi_message_init_with_transfers(&ti_dac->mesg, &ti_dac->xfer, 1);
> +	ti_dac->mesg.spi = spi;
> +
> +	ret = sscanf(spi->modalias, "dac%2hhu%1d",
> +		     &ti_dac->resolution, &indio_dev->num_channels);
> +	WARN_ON(ret != 2);

Whilst this seems nice and clear now, it may not work if this driver
had additional parts added in future. 

I would prefer an explicit table with this information in it and
use an enum to reference into it.

This is a bit 'too clever' :)

> +
> +	ret = ti_dac_cmd(ti_dac, WRITE_ALL_UPDATE, 0);
> +	if (ret) {
> +		dev_err(dev, "failed to initialize outputs to 0\n");
> +		return ret;
> +	}
> +
> +	ti_dac->vref = devm_regulator_get(dev, "vref");
> +	if (IS_ERR(ti_dac->vref))
> +		return PTR_ERR(ti_dac->vref);
> +
> +	ret = regulator_enable(ti_dac->vref);
> +	if (ret < 0)
> +		return ret;
> +
> +	mutex_init(&ti_dac->lock);
> +	ret = iio_device_register(indio_dev);
> +	if (ret) {
> +		mutex_destroy(&ti_dac->lock);
> +		regulator_disable(ti_dac->vref);
> +		return ret;
> +	}
> +
> +	return 0;
Trivial: 

return ret; here and get rid of it in the brackets above.

> +}
> +
> +static int ti_dac_remove(struct spi_device *spi)
> +{
> +	struct iio_dev *indio_dev = spi_get_drvdata(spi);
> +	struct ti_dac_chip *ti_dac = iio_priv(indio_dev);
> +
> +	iio_device_unregister(indio_dev);
> +	mutex_destroy(&ti_dac->lock);
> +	regulator_disable(ti_dac->vref);
> +
> +	return 0;
> +}
> +
> +#ifdef CONFIG_OF
> +static const struct of_device_id ti_dac_of_id[] = {
> +	{ .compatible = "ti,dac082s085" },
> +	{ .compatible = "ti,dac102s085" },
> +	{ .compatible = "ti,dac122s085" },
> +	{ .compatible = "ti,dac084s085" },
> +	{ .compatible = "ti,dac104s085" },
> +	{ .compatible = "ti,dac124s085" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ti_dac_of_id);
> +#endif
> +
> +static const struct spi_device_id ti_dac_spi_id[] = {
> +	{ "dac082s085" },
> +	{ "dac102s085" },
> +	{ "dac122s085" },
> +	{ "dac084s085" },
> +	{ "dac104s085" },
> +	{ "dac124s085" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(spi, ti_dac_spi_id);
> +
> +static struct spi_driver ti_dac_driver = {
> +	.driver = {
> +		.name		= "ti-dac082s085",
> +		.of_match_table	= of_match_ptr(ti_dac_of_id),
> +	},
> +	.probe	  = ti_dac_probe,
> +	.remove   = ti_dac_remove,
> +	.id_table = ti_dac_spi_id,
> +};
> +module_spi_driver(ti_dac_driver);
> +
> +MODULE_AUTHOR("Lukas Wunner <lukas@wunner.de>");
> +MODULE_DESCRIPTION("Texas Instruments 8/10/12-bit 2/4-channel DAC driver");
> +MODULE_LICENSE("GPL v2");