[PATCH v2 0/3] Texas Instruments 8/10/12-bit 2/4-channel DAC driver

[PATCH v2 0/3] Texas Instruments 8/10/12-bit 2/4-channel DAC driver

[Lukas Wunner]

Changes v1 -> v2:

Patch [1/3] was acked by Rob Herring.

Patch [2/3]:
- Enable vref regulator before initializing outputs to 0 in ti_dac_probe().
- Add channel datasheet names "A", "B", "C", "D".
- Use goto in ti_dac_set_powerdown_mode(). (Jonathan)
- Return -EBUSY instead of -ESHUTDOWN in ti_dac_write_raw(). (Jonathan)

Patch [3/3]:
- Look up chip properties in table indexed by the modalias instead of
  gleaning them from the modalias directly. (Jonathan)
  Separate patch for opinionatedness. ;-)


Link to v1:

https://www.spinics.net/lists/linux-iio/msg34885.html


Lukas Wunner (3):
  dt-bindings: iio: dac: ti-dac082s085: Document new driver
  iio: dac: Add Texas Instruments 8/10/12-bit 2/4-channel DAC driver
  iio: dac: ti-dac082s085: Read chip spec from device table

 Documentation/ABI/testing/sysfs-bus-iio            |   1 +
 .../devicetree/bindings/iio/dac/ti-dac082s085.txt  |  34 ++
 drivers/iio/dac/Kconfig                            |  10 +
 drivers/iio/dac/Makefile                           |   1 +
 drivers/iio/dac/ti-dac082s085.c                    | 368 +++++++++++++++++++++
 5 files changed, 414 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/ti-dac082s085.txt
 create mode 100644 drivers/iio/dac/ti-dac082s085.c

-- 
2.11.0

[PATCH v2 1/3] dt-bindings: iio: dac: ti-dac082s085: Document new driver

[Lukas Wunner]

Add device tree bindings for Texas Instruments 8/10/12-bit 2/4-channel
DAC driver.

Cc: Mathias Duckeck <m.duckeck@kunbus.de>
Acked-by: Rob Herring <robh@kernel.org>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 .../devicetree/bindings/iio/dac/ti-dac082s085.txt  | 34 ++++++++++++++++++++++
 1 file changed, 34 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/ti-dac082s085.txt

diff --git a/Documentation/devicetree/bindings/iio/dac/ti-dac082s085.txt b/Documentation/devicetree/bindings/iio/dac/ti-dac082s085.txt
new file mode 100644
index 000000000000..9cb0e10df704
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/ti-dac082s085.txt
@@ -0,0 +1,34 @@
+Texas Instruments 8/10/12-bit 2/4-channel DAC driver
+
+Required properties:
+ - compatible:		Must be one of:
+			"ti,dac082s085"
+			"ti,dac102s085"
+			"ti,dac122s085"
+			"ti,dac084s085"
+			"ti,dac104s085"
+			"ti,dac124s085"
+ - reg: 		Chip select number.
+ - spi-cpha, spi-cpol:	SPI mode (0,1) or (1,0) must be used, so specify
+			either spi-cpha or spi-cpol (but not both).
+ - vref-supply: 	Phandle to the external reference voltage supply.
+
+For other required and optional properties of SPI slave nodes please refer to
+../../spi/spi-bus.txt.
+
+Example:
+	vref_2v5_reg: regulator-vref {
+		compatible = "regulator-fixed";
+		regulator-name = "2v5";
+		regulator-min-microvolt = <2500000>;
+		regulator-max-microvolt = <2500000>;
+		regulator-always-on;
+	};
+
+	dac@0 {
+		compatible = "ti,dac082s085";
+		reg = <0>;
+		spi-max-frequency = <40000000>;
+		spi-cpol;
+		vref-supply = <&vref_2v5_reg>;
+	};
-- 
2.11.0

Re: [PATCH v2 1/3] dt-bindings: iio: dac: ti-dac082s085: Document new driver

[Jonathan Cameron]

On Tue, 17 Oct 2017 12:42:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> Add device tree bindings for Texas Instruments 8/10/12-bit 2/4-channel
> DAC driver.
> 
> Cc: Mathias Duckeck <m.duckeck@kunbus.de>
> Acked-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
Applied to the togreg branch of iio.git and pushed out as testing.

Thanks,

Jonathan
> ---
>  .../devicetree/bindings/iio/dac/ti-dac082s085.txt  | 34 ++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/ti-dac082s085.txt
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/ti-dac082s085.txt b/Documentation/devicetree/bindings/iio/dac/ti-dac082s085.txt
> new file mode 100644
> index 000000000000..9cb0e10df704
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/ti-dac082s085.txt
> @@ -0,0 +1,34 @@
> +Texas Instruments 8/10/12-bit 2/4-channel DAC driver
> +
> +Required properties:
> + - compatible:		Must be one of:
> +			"ti,dac082s085"
> +			"ti,dac102s085"
> +			"ti,dac122s085"
> +			"ti,dac084s085"
> +			"ti,dac104s085"
> +			"ti,dac124s085"
> + - reg: 		Chip select number.
> + - spi-cpha, spi-cpol:	SPI mode (0,1) or (1,0) must be used, so specify
> +			either spi-cpha or spi-cpol (but not both).
> + - vref-supply: 	Phandle to the external reference voltage supply.
> +
> +For other required and optional properties of SPI slave nodes please refer to
> +../../spi/spi-bus.txt.
> +
> +Example:
> +	vref_2v5_reg: regulator-vref {
> +		compatible = "regulator-fixed";
> +		regulator-name = "2v5";
> +		regulator-min-microvolt = <2500000>;
> +		regulator-max-microvolt = <2500000>;
> +		regulator-always-on;
> +	};
> +
> +	dac@0 {
> +		compatible = "ti,dac082s085";
> +		reg = <0>;
> +		spi-max-frequency = <40000000>;
> +		spi-cpol;
> +		vref-supply = <&vref_2v5_reg>;
> +	};

[PATCH v2 3/3] iio: dac: ti-dac082s085: Read chip spec from device table

[Lukas Wunner]

The two properties unique to each supported chip, resolution and number
of channels, are currently gleaned from the chip's name.
E.g. dac102s085 is a dual channel 10-bit DAC.
        ^^^
This was deemed unmaintainable by the subsystem maintainer once the
driver is extended to support further chips, hence it was requested
to add an explicit table for chip-specific information and use an
enum to reference into it.

This adds 17 LoC without any immediate gain, so make the change in a
separate commit which can be reverted if we determine in 10 years that
it was unnecessary.

Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/iio/dac/ti-dac082s085.c | 35 ++++++++++++++++++++++++++---------
 1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/dac/ti-dac082s085.c b/drivers/iio/dac/ti-dac082s085.c
index 0fefb110be69..2c1556f32574 100644
--- a/drivers/iio/dac/ti-dac082s085.c
+++ b/drivers/iio/dac/ti-dac082s085.c
@@ -20,6 +20,22 @@
 #include <linux/regulator/consumer.h>
 #include <linux/spi/spi.h>
 
+enum { dual_8bit, dual_10bit, dual_12bit, quad_8bit, quad_10bit, quad_12bit };
+
+struct ti_dac_spec {
+	u8 num_channels;
+	u8 resolution;
+};
+
+static const struct ti_dac_spec ti_dac_spec[] = {
+	[dual_8bit]  = { .num_channels = 2, .resolution = 8  },
+	[dual_10bit] = { .num_channels = 2, .resolution = 10 },
+	[dual_12bit] = { .num_channels = 2, .resolution = 12 },
+	[quad_8bit]  = { .num_channels = 4, .resolution = 8  },
+	[quad_10bit] = { .num_channels = 4, .resolution = 10 },
+	[quad_12bit] = { .num_channels = 4, .resolution = 12 },
+};
+
 /**
  * struct ti_dac_chip - TI DAC chip
  * @lock: protects write sequences
@@ -246,6 +262,7 @@ static const struct iio_info ti_dac_info = {
 static int ti_dac_probe(struct spi_device *spi)
 {
 	struct device *dev = &spi->dev;
+	const struct ti_dac_spec *spec;
 	struct ti_dac_chip *ti_dac;
 	struct iio_dev *indio_dev;
 	int ret;
@@ -267,9 +284,9 @@ static int ti_dac_probe(struct spi_device *spi)
 	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);
+	spec = &ti_dac_spec[spi_get_device_id(spi)->driver_data];
+	indio_dev->num_channels = spec->num_channels;
+	ti_dac->resolution = spec->resolution;
 
 	ti_dac->vref = devm_regulator_get(dev, "vref");
 	if (IS_ERR(ti_dac->vref))
@@ -325,12 +342,12 @@ 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" },
+	{ "dac082s085", dual_8bit  },
+	{ "dac102s085", dual_10bit },
+	{ "dac122s085", dual_12bit },
+	{ "dac084s085", quad_8bit  },
+	{ "dac104s085", quad_10bit },
+	{ "dac124s085", quad_12bit },
 	{ }
 };
 MODULE_DEVICE_TABLE(spi, ti_dac_spi_id);
-- 
2.11.0

Re: [PATCH v2 3/3] iio: dac: ti-dac082s085: Read chip spec from device table

[Jonathan Cameron]

On Tue, 17 Oct 2017 12:42:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> The two properties unique to each supported chip, resolution and number
> of channels, are currently gleaned from the chip's name.
> E.g. dac102s085 is a dual channel 10-bit DAC.
>         ^^^
> This was deemed unmaintainable by the subsystem maintainer once the
> driver is extended to support further chips, hence it was requested
> to add an explicit table for chip-specific information and use an
> enum to reference into it.
> 
> This adds 17 LoC without any immediate gain, so make the change in a
> separate commit which can be reverted if we determine in 10 years that
> it was unnecessary.
> 
Good luck with that ;) 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

Thanks,

Jonathan
> ---
>  drivers/iio/dac/ti-dac082s085.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/dac/ti-dac082s085.c b/drivers/iio/dac/ti-dac082s085.c
> index 0fefb110be69..2c1556f32574 100644
> --- a/drivers/iio/dac/ti-dac082s085.c
> +++ b/drivers/iio/dac/ti-dac082s085.c
> @@ -20,6 +20,22 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/spi/spi.h>
>  
> +enum { dual_8bit, dual_10bit, dual_12bit, quad_8bit, quad_10bit, quad_12bit };
> +
> +struct ti_dac_spec {
> +	u8 num_channels;
> +	u8 resolution;
> +};
> +
> +static const struct ti_dac_spec ti_dac_spec[] = {
> +	[dual_8bit]  = { .num_channels = 2, .resolution = 8  },
> +	[dual_10bit] = { .num_channels = 2, .resolution = 10 },
> +	[dual_12bit] = { .num_channels = 2, .resolution = 12 },
> +	[quad_8bit]  = { .num_channels = 4, .resolution = 8  },
> +	[quad_10bit] = { .num_channels = 4, .resolution = 10 },
> +	[quad_12bit] = { .num_channels = 4, .resolution = 12 },
> +};
> +
>  /**
>   * struct ti_dac_chip - TI DAC chip
>   * @lock: protects write sequences
> @@ -246,6 +262,7 @@ static const struct iio_info ti_dac_info = {
>  static int ti_dac_probe(struct spi_device *spi)
>  {
>  	struct device *dev = &spi->dev;
> +	const struct ti_dac_spec *spec;
>  	struct ti_dac_chip *ti_dac;
>  	struct iio_dev *indio_dev;
>  	int ret;
> @@ -267,9 +284,9 @@ static int ti_dac_probe(struct spi_device *spi)
>  	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);
> +	spec = &ti_dac_spec[spi_get_device_id(spi)->driver_data];
> +	indio_dev->num_channels = spec->num_channels;
> +	ti_dac->resolution = spec->resolution;
>  
>  	ti_dac->vref = devm_regulator_get(dev, "vref");
>  	if (IS_ERR(ti_dac->vref))
> @@ -325,12 +342,12 @@ 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" },
> +	{ "dac082s085", dual_8bit  },
> +	{ "dac102s085", dual_10bit },
> +	{ "dac122s085", dual_12bit },
> +	{ "dac084s085", quad_8bit  },
> +	{ "dac104s085", quad_10bit },
> +	{ "dac124s085", quad_12bit },
>  	{ }
>  };
>  MODULE_DEVICE_TABLE(spi, ti_dac_spi_id);

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

[Lukas Wunner]

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>
---
 Documentation/ABI/testing/sysfs-bus-iio |   1 +
 drivers/iio/dac/Kconfig                 |  10 +
 drivers/iio/dac/Makefile                |   1 +
 drivers/iio/dac/ti-dac082s085.c         | 351 ++++++++++++++++++++++++++++++++
 4 files changed, 363 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 3fc79185cc56..2e3f919485f4 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 d187233dec3a..965d5c0d2468 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -310,6 +310,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 8fe5af231698..4785858e04cc 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -33,4 +33,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..0fefb110be69
--- /dev/null
+++ b/drivers/iio/dac/ti-dac082s085.c
@@ -0,0 +1,351 @@
+/*
+ * 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 = 0;
+
+	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)
+			goto out;
+	}
+	ti_dac->powerdown_mode = mode;
+
+out:
+	mutex_unlock(&ti_dac->lock);
+	return ret;
+}
+
+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,						\
+	.datasheet_name = (const char[]){ 'A' + (chan), 0 },	\
+	.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 -EBUSY;
+
+		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);
+
+	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 = ti_dac_cmd(ti_dac, WRITE_ALL_UPDATE, 0);
+	if (ret) {
+		dev_err(dev, "failed to initialize outputs to 0\n");
+		goto err;
+	}
+
+	ret = iio_device_register(indio_dev);
+	if (ret)
+		goto err;
+
+	return 0;
+
+err:
+	mutex_destroy(&ti_dac->lock);
+	regulator_disable(ti_dac->vref);
+	return ret;
+}
+
+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");
-- 
2.11.0

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

[Jonathan Cameron]

On Tue, 17 Oct 2017 12:42: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>
One issue inline, but I'll fix it.

Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to play with it.

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         | 351 ++++++++++++++++++++++++++++++++
>  4 files changed, 363 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 3fc79185cc56..2e3f919485f4 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 d187233dec3a..965d5c0d2468 100644
> --- a/drivers/iio/dac/Kconfig
> +++ b/drivers/iio/dac/Kconfig
> @@ -310,6 +310,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 8fe5af231698..4785858e04cc 100644
> --- a/drivers/iio/dac/Makefile
> +++ b/drivers/iio/dac/Makefile
> @@ -33,4 +33,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..0fefb110be69
> --- /dev/null
> +++ b/drivers/iio/dac/ti-dac082s085.c
> @@ -0,0 +1,351 @@
> +/*
> + * 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;
I missed this the first time.  The whole point of the fun
of ____cacheline_aligned is to ensure nothing shares a
cacheline with the buffer used for DMA.  We take care
when allocating these private structures that this
will be true, but it relies on nothing that might be
changed during dma being after the buffer.

Simple reordering fix so I'll do it whilst applying.
> +	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 = 0;
> +
> +	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)
> +			goto out;
> +	}
> +	ti_dac->powerdown_mode = mode;
> +
> +out:
> +	mutex_unlock(&ti_dac->lock);
> +	return ret;
> +}
> +
> +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,						\
> +	.datasheet_name = (const char[]){ 'A' + (chan), 0 },	\
> +	.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 -EBUSY;
> +
> +		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);
> +
> +	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 = ti_dac_cmd(ti_dac, WRITE_ALL_UPDATE, 0);
> +	if (ret) {
> +		dev_err(dev, "failed to initialize outputs to 0\n");
> +		goto err;
> +	}
> +
> +	ret = iio_device_register(indio_dev);
> +	if (ret)
> +		goto err;
> +
> +	return 0;
> +
> +err:
> +	mutex_destroy(&ti_dac->lock);
> +	regulator_disable(ti_dac->vref);
> +	return ret;
> +}
> +
> +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");

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

[Lukas Wunner]

On Sat, Oct 21, 2017 at 08:33:08PM +0100, Jonathan Cameron wrote:
> On Tue, 17 Oct 2017 12:42:00 +0200 Lukas Wunner <lukas@wunner.de> wrote:
> > +struct ti_dac_chip {
> > +	struct mutex lock;
> > +	struct regulator *vref;
> > +	struct spi_message mesg;
> > +	struct spi_transfer xfer;
> > +	u8 buf[2] ____cacheline_aligned;
> 
> I missed this the first time.  The whole point of the fun
> of ____cacheline_aligned is to ensure nothing shares a
> cacheline with the buffer used for DMA.  We take care
> when allocating these private structures that this
> will be true, but it relies on nothing that might be
> changed during dma being after the buffer.
> 
> Simple reordering fix so I'll do it whilst applying.

TBH I was a bit confused about how to achieve proper cacheline
alignment.  My limited understanding of the slab allocator is
that it neither guarantees that an allocation begins on a cacheline
nor that no another allocation immediately starts after it.

However taking a closer look at iio_device_alloc() I realize now
that it implicitly takes care of both by inserting the necessary
padding.  Neat.  I was already wondering why the IIO subsystem
takes this somewhat unusual approach to allocate the private struct
behind struct iio_dev, instead of letting the driver embed it
within its private struct and use container_of() to yield the
private struct corresponding to a struct iio_dev, as most other
subsystems do.  But that explains it.

Thanks,

Lukas

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

[Jonathan Cameron]

On Sun, 22 Oct 2017 11:48:54 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> On Sat, Oct 21, 2017 at 08:33:08PM +0100, Jonathan Cameron wrote:
> > On Tue, 17 Oct 2017 12:42:00 +0200 Lukas Wunner <lukas@wunner.de> wrote:  
> > > +struct ti_dac_chip {
> > > +	struct mutex lock;
> > > +	struct regulator *vref;
> > > +	struct spi_message mesg;
> > > +	struct spi_transfer xfer;
> > > +	u8 buf[2] ____cacheline_aligned;  
> > 
> > I missed this the first time.  The whole point of the fun
> > of ____cacheline_aligned is to ensure nothing shares a
> > cacheline with the buffer used for DMA.  We take care
> > when allocating these private structures that this
> > will be true, but it relies on nothing that might be
> > changed during dma being after the buffer.
> > 
> > Simple reordering fix so I'll do it whilst applying.  
> 
> TBH I was a bit confused about how to achieve proper cacheline
> alignment.  My limited understanding of the slab allocator is
> that it neither guarantees that an allocation begins on a cacheline
> nor that no another allocation immediately starts after it.
> 
> However taking a closer look at iio_device_alloc() I realize now
> that it implicitly takes care of both by inserting the necessary
> padding.  Neat.  I was already wondering why the IIO subsystem
> takes this somewhat unusual approach to allocate the private struct
> behind struct iio_dev, instead of letting the driver embed it
> within its private struct and use container_of() to yield the
> private struct corresponding to a struct iio_dev, as most other
> subsystems do.  But that explains it.
Kind of - if we didn't play that game, the drivers could do it
themselves in their private structures.

Back when we got started with IIO it was actually a pretty
even split in kernel subsystems between those who did the embedding
one way around and those that went the other way around - there
are advantages to both approaches but it doesn't really matter.
Honestly an early reviewer (can't remember who) said do it this
way (before that we were separately allocating both) and I never
thought about it at the time ;)

I now wonder if we made the right choice sometimes - particularly
as it makes drivers embedding both IIO and other interfaces a bit
messy.  I always planned to put in the various
initializers to handle the embedding in the private structure case
correctly, but haven't gotten around to it yet :) Should be easy
enough to do I think...  From a review point of view though it
is nice to keep all drivers looking roughly the same so perhaps
introducing another option would be a bad idea...

Malloc calls are always guaranteed to be cacheline aligned (though
largely through implementation than I think an explicit rule - though
they are guaranteed to be safe for DMA as long as the whole memory
space is accessible for DMA - so this enforces cacheline alignment
rather indirectly).

A lot of drivers rely on this for dma buffers though so it won't
change without a 'lot' of work.  Much nicer than in userspace
where you have to go through the dance of alloc_aligned to
guarantee anything at all...

Jonathan
> 
> Thanks,
> 
> Lukas