Re: [PATCH 2/2] iio: dac: Introduce the TI DAC8771 DAC driver
From: Dan Murphy
Date: Fri Jan 03 2020 - 08:46:35 EST
Jonathan
On 12/23/19 9:13 AM, Jonathan Cameron wrote:
On Tue, 17 Dec 2019 08:07:31 -0600
Dan Murphy <dmurphy@xxxxxx> wrote:
Adding the Texas Instruments single channel 16 bit DAC driver.
This device has configurable voltage and current output with 16 bit
resolution for offset and gain.
Signed-off-by: Dan Murphy <dmurphy@xxxxxx>
Various small things inline.
Thanks,
Jonathan
---
drivers/iio/dac/Kconfig | 7 +
drivers/iio/dac/Makefile | 1 +
drivers/iio/dac/ti-dac8771.c | 499 +++++++++++++++++++++++++++++++++++
3 files changed, 507 insertions(+)
create mode 100644 drivers/iio/dac/ti-dac8771.c
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 979070196da9..df7793113415 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -386,6 +386,13 @@ config TI_DAC7612
If compiled as a module, it will be called ti-dac7612.
+config TI_DAC8771
+ tristate "Texas Instruments 16-bit single channel DAC driver"
+ depends on SPI_MASTER && GPIOLIB
+ help
+ Driver for the Texas Instruments DAC8771.
+ If compiled as a module, it will be called ti-dac8771.
+
config VF610_DAC
tristate "Vybrid vf610 DAC driver"
depends on OF
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index 1369fa1d2f0e..4aa6e35348cf 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -42,4 +42,5 @@ obj-$(CONFIG_TI_DAC082S085) += ti-dac082s085.o
obj-$(CONFIG_TI_DAC5571) += ti-dac5571.o
obj-$(CONFIG_TI_DAC7311) += ti-dac7311.o
obj-$(CONFIG_TI_DAC7612) += ti-dac7612.o
+obj-$(CONFIG_TI_DAC8771) += ti-dac8771.o
obj-$(CONFIG_VF610_DAC) += vf610_dac.o
diff --git a/drivers/iio/dac/ti-dac8771.c b/drivers/iio/dac/ti-dac8771.c
new file mode 100644
index 000000000000..309728212a07
--- /dev/null
+++ b/drivers/iio/dac/ti-dac8771.c
@@ -0,0 +1,499 @@
+// SPDX-License-Identifier: GPL-2.0
+// DAC8771 DAC
+// Copyright (C) 2019 Texas Instruments Incorporated - http://www.ti.com/
+
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/gpio/consumer.h>
+#include <linux/of_gpio.h>
What is using of_gpio.h in here?
Ack that can be removed.
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <linux/iio/iio.h>
+#include <linux/iio/events.h>
+
+#define DAC8771_RESET 0x01
+#define DAC8771_RESET_CFG 0x02
+#define DAC8771_DAC_SELECT 0x03
+#define DAC8771_DAC_CFG 0x04
+#define DAC8771_DAC_DATA 0x05
+#define DAC8771_BUCK_BOOST_SEL 0x06
+#define DAC8771_BUCK_BOOST_CFG 0x07
+#define DAC8771_CH_CAL_EN 0x08
+#define DAC8771_CH_GAIN_CAL 0x09
+#define DAC8771_CH_OFF_CAL 0x0a
+#define DAC8771_STATUS 0x0b
+#define DAC8771_STATUS_MSK 0x0c
+#define DAC8771_ALARM_ACTION 0x0d
+#define DAC8771_ALARM_CODE 0x0e
+#define DAC8771_WDOG_RESET 0x10
+#define DAC8771_DEV_ID 0x11
+
+#define DAC8771_RESOLUTION 16
+
+/* Status register */
+#define DAC8771_OVERTEMP BIT(4)
+#define DAC8771_DAC_FAULT BIT(0)
+#define DAC8771_STATUS_MASK (DAC8771_OVERTEMP | DAC8771_DAC_FAULT)
+
+#define DAC8771_READ_EN BIT(7)
+
+#define DAC8771_SW_RESET BIT(0)
+#define DAC8771_SPI_ENABLE 0
+#define DAC8771_SPI_DISABLE 1
+
+#define DAC8771_CHA_SEL BIT(5)
+#define DAC8771_OUT_RANGE_MSK GENMASK(3, 0)
+#define DAC8771_MAX_GAIN_OFF 65535
+
+#define DAC8771_OUT_RANGE_DEF 0x0
+#define DAC8771_OUT_RANGE_CURR_LOW 4
+#define DAC8771_OUT_RANGE_CURR_HIGH 7
+#define DAC8771_OUT_RANGE_MAX 12
+
+#define DAC8771_SELECT_A BIT(0)
+
+/**
+ * struct dac8771_chip - TI DAC chip
+ * @lock: protects write sequences
+ * @vref: regulator generating Vref
+ * @spi: SPI device to send data to the device
+ * @ldac: load DAC GPIO
+ * @reset: reset GPIO
+ * @chan_type: Type of IIO channel either VOLTAGE or CURRENT
+ * @output_range: output range of the device
+ * @val: cached value
+ * @buf: buffer for transfer data
+ */
+struct dac8771_chip {
+ struct mutex lock;
+ struct regulator *vref;
+ struct spi_device *spi;
+ struct gpio_desc *ldac;
+ struct gpio_desc *reset;
+ enum iio_chan_type chan_type;
+ u8 output_range;
+ u16 val;
+ union {
+ __be32 d32;
+ __be16 d16;
Why? Only d8 seems to be used.
It was a development artifact it can be removed
+ u8 d8[3];
+ } data[3] ____cacheline_aligned;
+};
+
+static int dac8771_write(struct dac8771_chip *dac8771,
+ unsigned int reg, int val)
+{
+ u8 data[3];
Spi buffers need to be dma safe. Can't be on the stack because
you can't force stack variables to be in a cacheline without anything
else that might cause problems during dma transfers.
ok. I will need to fix the write
+
+ data[0] = reg;
+ data[1] = val >> 8;
+ data[2] = val & 0xff;
+
+ return spi_write(dac8771->spi, &data, sizeof(data));
+}
+
+static int dac8771_read(struct dac8771_chip *dac8771, unsigned int reg)
+{
+ int ret;
+ struct spi_transfer t[] = {
+ {
+ .tx_buf = &dac8771->data[0].d8[0],
+ .len = 3,
+ .cs_change = 1,
+ }, {
+ .tx_buf = &dac8771->data[1].d8[0],
I can't immediately see where anything is put into this buffer to transmit..
I will have to look into this.
The device requires some clocking NOOPs to shift the data into the buffers.
+ .rx_buf = &dac8771->data[2].d8[0],
+ .len = 3,
+ },
+ };
+
+ dac8771->data[0].d8[0] = (DAC8771_READ_EN | reg);
+
+ ret = spi_sync_transfer(dac8771->spi, t, ARRAY_SIZE(t));
+ if (ret < 0)
+ return ret;
+
+ return (dac8771->data[2].d8[1] << 8 | dac8771->data[2].d8[2]);
+}
+
+static int dac8771_sync_write(struct dac8771_chip *dac8771,
+ unsigned int reg, int val)
+{
+ int ret;
+
+ ret = dac8771_write(dac8771, reg, val);
+ if (ret)
+ return ret;
+
+ if (dac8771->ldac) {
+ gpiod_set_value(dac8771->ldac, 1);
+ gpiod_set_value(dac8771->ldac, 0);
+ }
+
+ return ret;
+}
+
+static int dac8771_reset(struct dac8771_chip *dac8771)
+{
+ if (dac8771->reset) {
+ gpiod_set_value(dac8771->reset, 0);
+ gpiod_set_value(dac8771->reset, 1);
+ } else {
+ return dac8771_sync_write(dac8771, DAC8771_RESET,
+ DAC8771_SW_RESET);
+ }
+
+ return 0;
+}
+
+static int dac8771_update_bits(struct dac8771_chip *dac8771, unsigned int reg,
+ int mask, int val)
+{
+ int write_val;
+ int tmp;
+
+ tmp = dac8771_read(dac8771, reg);
+ if (tmp < 0)
+ return tmp;
+
+ write_val = (tmp & ~mask);
+ write_val |= val & mask;
+
+ return dac8771_write(dac8771, reg, write_val);
+}
+
+static int dac8771_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long mask)
+{
+ struct dac8771_chip *dac8771 = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ *val = dac8771->val;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ ret = regulator_get_voltage(dac8771->vref);
+ if (ret < 0)
+ return ret;
+
+ *val = ret / 1000;
+ *val2 = DAC8771_RESOLUTION;
+ return IIO_VAL_FRACTIONAL_LOG2;
+ case IIO_CHAN_INFO_OFFSET:
+ mutex_lock(&dac8771->lock);
+ ret = dac8771_read(dac8771, DAC8771_CH_OFF_CAL);
+ mutex_unlock(&dac8771->lock);
+
+ if (ret < 0)
+ return ret;
+
+ *val = ret;
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_HARDWAREGAIN:
+ mutex_lock(&dac8771->lock);
+ ret = dac8771_read(dac8771, DAC8771_CH_GAIN_CAL);
+ mutex_unlock(&dac8771->lock);
+
+ if (ret < 0)
+ return ret;
+
+ *val = ret;
+ return IIO_VAL_INT;
+ default:
+ ret = -EINVAL;
+ }
+
+ return -EINVAL;
+}
+
+static int dac8771_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val, int val2, long mask)
+{
+ struct dac8771_chip *dac8771 = iio_priv(indio_dev);
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ mutex_lock(&dac8771->lock);
+ ret = dac8771_sync_write(dac8771, DAC8771_DAC_DATA, val);
+ if (!ret)
+ dac8771->val = val;
+ break;
+ case IIO_CHAN_INFO_ENABLE:
+ if (val)
+ val = DAC8771_CHA_SEL;
+ else
+ val = 0;
+
+ mutex_lock(&dac8771->lock);
+
+ ret = dac8771_update_bits(dac8771, DAC8771_DAC_SELECT,
+ DAC8771_CHA_SEL, val);
+ if (ret)
+ goto io_err;
+
+ ret = dac8771_update_bits(dac8771, DAC8771_DAC_CFG,
+ DAC8771_OUT_RANGE_MSK,
+ dac8771->output_range);
+ break;
+ case IIO_CHAN_INFO_OFFSET:
+ if (val > DAC8771_MAX_GAIN_OFF)
+ return -EINVAL;
+
+ mutex_lock(&dac8771->lock);
+ ret = dac8771_sync_write(dac8771, DAC8771_CH_OFF_CAL, val);
+ break;
+ case IIO_CHAN_INFO_HARDWAREGAIN:
Hmm. I wouldn't expect to see hardware gain on a DAC. It has some very
specific usecases, normally focused around device where the thing being
measured or generated is not an amplitude (time of flight sensors for example).
For a dac, we would normally have IIO_CHAN_INFO_SCALE controlling user
visible 'gain' by specifying the value of 1LSB. We might also have
IIO_CHAN_INFO_CALIBSCALE to reflect small correction controls, usually used
to compensate for manufacturing variability. So whilst they do move the
output value on a DAC, the intent is this is not a general user control
but more of a 'trimming control'. Given the register name of _CAL
I'm guessing this one should be IIO_CHAN_INFO_CALIBSCALE.
ACK. I originally had scale declared but thought HARDWARE_GAIN made
more sense.
I have no issue changing it to INFO_SCALE.
+ if (val > DAC8771_MAX_GAIN_OFF)
+ return -EINVAL;
+
+ mutex_lock(&dac8771->lock);
+ ret = dac8771_sync_write(dac8771, DAC8771_CH_GAIN_CAL, val);
+ break;
+ default:
+ return -EINVAL;
+ }
+io_err:
+ mutex_unlock(&dac8771->lock);
+ return ret;
+}
+
+static int dac8771_write_raw_get_fmt(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan, long mask)
+{
+ return IIO_VAL_INT;
+}
+
+static irqreturn_t dac8771_event_handler(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct dac8771_chip *dac8771 = iio_priv(indio_dev);
+ int clear_bits = 0;
+ int ret;
+
+ ret = dac8771_read(dac8771, DAC8771_STATUS);
+
+ if (ret & DAC8771_OVERTEMP) {
+ iio_push_event(private,
Given you have a local indio_dev variable, nicer to use that than private.
ACK
+ IIO_UNMOD_EVENT_CODE(IIO_TEMP,
+ 0, IIO_EV_TYPE_THRESH, IIO_EV_DIR_RISING),
+ iio_get_time_ns(private));
+ clear_bits = DAC8771_OVERTEMP;
+ }
+
+ if (ret & DAC8771_DAC_FAULT) {
+ iio_push_event(private,
+ IIO_UNMOD_EVENT_CODE(dac8771->chan_type,
+ 0, IIO_EV_TYPE_CHANGE, IIO_EV_DIR_NONE),
+ iio_get_time_ns(private));
We need some more discussion on why this event is done like this.
What does it signify? Would userspace expect to get it this way?
Unfortunately this is how the data sheet declares this fault just as DAC
Fault.
It is a generic fault condition unlike the TMP fault above.
In general, provision for fault cases is poor, at least partly because,
every now and then there is discussion on how to report hardware errors
like this in general across the kernel, but no good answer has ever
being figured out.
+
+ clear_bits |= DAC8771_DAC_FAULT;
+ }
+
+ ret = dac8771_update_bits(dac8771, DAC8771_STATUS, DAC8771_STATUS_MASK,
+ clear_bits);
+ if (ret)
+ dev_err(&dac8771->spi->dev,
+ "Failed to clear status register\n");
+
+ return IRQ_HANDLED;
+}
+
+static const struct iio_info dac8771_info = {
+ .read_raw = dac8771_read_raw,
+ .write_raw = dac8771_write_raw,
+ .write_raw_get_fmt = dac8771_write_raw_get_fmt,
+};
+
+static int dac8771_probe_dt(struct dac8771_chip *dac8771)
+{
+ u32 range;
+ int ret;
+
+ dac8771->ldac = devm_gpiod_get_optional(&dac8771->spi->dev, "loaddacs",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(dac8771->ldac)) {
+ ret = PTR_ERR(dac8771->ldac);
+ if (ret != -ENODEV) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(&dac8771->spi->dev,
+ "Failed to get loaddacs gpio: %d\n",
+ ret);
+ return ret;
+ }
+ dac8771->ldac = NULL;
+ }
+
+ dac8771->reset = devm_gpiod_get_optional(&dac8771->spi->dev, "reset",
+ GPIOD_OUT_HIGH);
+ if (IS_ERR(dac8771->reset)) {
+ ret = PTR_ERR(dac8771->reset);
+ if (ret != -ENODEV) {
+ if (ret != -EPROBE_DEFER)
+ dev_err(&dac8771->spi->dev,
+ "Failed to get reset gpio: %d\n",
+ ret);
+ return ret;
+ }
+ dac8771->reset = NULL;
+ }
+
+ ret = device_property_read_u32(&dac8771->spi->dev, "ti,output-range",
+ &range);
+ if (ret) {
+ dev_err(&dac8771->spi->dev,
+ "Output range is not set using default\n");
+ range = DAC8771_OUT_RANGE_DEF;
+ } else {
+ if (dac8771->output_range < DAC8771_OUT_RANGE_DEF ||
+ dac8771->output_range > DAC8771_OUT_RANGE_MAX) {
+ dev_err(&dac8771->spi->dev,
+ "Output range is invalid\n");
+ return -EINVAL;
+ }
+ }
+
+ dac8771->output_range = range;
+ if ((dac8771->output_range >= DAC8771_OUT_RANGE_CURR_LOW &&
+ dac8771->output_range <= DAC8771_OUT_RANGE_CURR_HIGH) ||
+ dac8771->output_range == DAC8771_OUT_RANGE_MAX)
+ dac8771->chan_type = IIO_CURRENT;
+ else
+ dac8771->chan_type = IIO_VOLTAGE;
+
+ return 0;
+}
+
+static int dac8771_init(struct dac8771_chip *dac8771)
+{
+ int ret;
+
+ ret = dac8771_reset(dac8771);
+ if (ret)
+ return ret;
+
+ return dac8771_write(dac8771, DAC8771_BUCK_BOOST_SEL, DAC8771_SELECT_A);
+}
+
+static int dac8771_probe(struct spi_device *spi)
+{
+ struct device *dev = &spi->dev;
+ struct dac8771_chip *dac8771;
+ struct iio_dev *indio_dev;
+ struct iio_chan_spec *chan;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(dev, sizeof(*dac8771));
+ if (!indio_dev) {
+ dev_err(dev, "can not allocate iio device\n");
+ return -ENOMEM;
+ }
+
+ spi->mode = SPI_MODE_2;
+ spi_setup(spi);
+ spi_set_drvdata(spi, indio_dev);
+
+ dac8771 = iio_priv(indio_dev);
+ dac8771->spi = spi;
+
+ ret = dac8771_probe_dt(dac8771);
+ if (ret)
+ return ret;
+
+ indio_dev->dev.parent = dev;
+ indio_dev->dev.of_node = spi->dev.of_node;
+ indio_dev->info = &dac8771_info;
+ indio_dev->name = spi_get_device_id(spi)->name;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->num_channels = 1;
+
+ chan = devm_kzalloc(&indio_dev->dev, sizeof(struct iio_chan_spec),
+ GFP_KERNEL);
If it's always the same size, why not embed it in the dac8711 structure?
Will avoid the need for an extra allocation.
+
+ chan->type = dac8771->chan_type;
As there are only two types (I think!) I'd prefer to see this done
as a pick between two static const structures rather than via
dynamic setup.
There are only two types VOLTAGE and CURRENT which is determined by the
DAC configuration register.
This is passed in via the binding to control what type of channel it is
and setup the appropriate RANGE of the output.
So I can do a static const structure that passes in the chan_type to
setup the correct channel type since the other values are constant.
+ chan->channel = 0;
+ chan->output = true;
+ chan->info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE);
+ chan->info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_OFFSET) |
+ BIT(IIO_CHAN_INFO_HARDWAREGAIN) |
+ BIT(IIO_CHAN_INFO_ENABLE) |
+ BIT(IIO_CHAN_INFO_SCALE);
+
+ indio_dev->channels = chan;
+
+ dac8771->vref = devm_regulator_get(&dac8771->spi->dev, "vref");
A quick glance at the datasheet suggests this device can be used with
an internal reference? Ideally we'd support that by making the provision
of a vref regulator optional.
Ack. Missed that.
It also seems that there are a number of other power supplies that should
be in the dt bindings even if not yet supported by the driver
This is true. I did not have the current need to manage those
additional supplies.
I think you are referring to AVDD, DVDD and PVDD.
.
+ if (IS_ERR(dac8771->vref)) {
+ dev_err(&dac8771->spi->dev, "error to get regulator\n");
+ return PTR_ERR(dac8771->vref);
+ }
+
+ ret = regulator_enable(dac8771->vref);
+ if (ret < 0) {
+ dev_err(dev, "can not enable regulator\n");
+ return ret;
+ }
+
+ mutex_init(&dac8771->lock);
+
+ if (spi->irq) {
+ ret = devm_request_threaded_irq(&spi->dev, spi->irq, NULL,
+ &dac8771_event_handler,
+ IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+ spi_get_device_id(dac8771->spi)->name,
+ indio_dev);
+ if (ret)
+ goto err;
+ }
+
+ ret = dac8771_init(dac8771);
+ if (ret)
+ goto err;
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
You can't safely mix managed and unmanaged code like this.
By the time the userspace interfacs are removed as a result
of the equivalent of iio_device_unregister being called as the device
managed code unwinds, we will have turned the reference voltage off.
Any readings that happen to occur in that narrow window will get
unpredictable results.
One option for that is to use devm_add_action_or_reset to register
an additional unwind function to deal with the regulator disable
in the remove path. The argument for not having a devm_regulator_enable
has been that it is almost always desirable to turn regulators off in
other paths (suspend or runtime suspend) so normally it doesn't
make that much sense to have a general always approach.
Note we hardly ever bother with mutex_destroy as it is only for
debug purposes and usually only relevant if complex handling is going
on within the driver. For that reason I guess, no one has ever
bothered with a devm_mutex_init().
Ack
Dan