devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/2] Add support for the AD5754 DAC
@ 2022-11-18 17:24 Ciprian Regus
  2022-11-18 17:24 ` [PATCH v3 1/2] dt-bindings: iio: dac: add adi,ad5754.yaml Ciprian Regus
  2022-11-18 17:24 ` [PATCH v3 2/2] drivers: iio: dac: Add AD5754 DAC driver Ciprian Regus
  0 siblings, 2 replies; 6+ messages in thread
From: Ciprian Regus @ 2022-11-18 17:24 UTC (permalink / raw)
  To: jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree,
	linux-kernel
  Cc: Ciprian Regus

These patches implement support for the AD5754 DAC (both dual
and quad channel variants).

The AD5724/AD5734/AD5754 are quad, 12-/14-/16-bit, serial
input, voltage output DACs. The devices operate from single-
supply voltages from +4.5 V up to +16.5 V or dual-supply
voltages from ±4.5 V up to ±16.5 V. The input coding is
user-selectable twos complement or offset binary for a bipolar
output (depending on the state of Pin BIN/2sComp), and straight
binary for a unipolar output.

Ciprian Regus (2):
  dt-bindings: iio: dac: add adi,ad5754.yaml
  drivers: iio: dac: Add AD5754 DAC driver

 .../bindings/iio/dac/adi,ad5754.yaml          | 181 +++++
 MAINTAINERS                                   |   8 +
 drivers/iio/dac/Kconfig                       |  12 +
 drivers/iio/dac/Makefile                      |   1 +
 drivers/iio/dac/ad5754.c                      | 672 ++++++++++++++++++
 5 files changed, 874 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml
 create mode 100644 drivers/iio/dac/ad5754.c

-- 
2.30.2


^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH v3 1/2] dt-bindings: iio: dac: add adi,ad5754.yaml
  2022-11-18 17:24 [PATCH v3 0/2] Add support for the AD5754 DAC Ciprian Regus
@ 2022-11-18 17:24 ` Ciprian Regus
  2022-11-21 11:20   ` Krzysztof Kozlowski
  2022-11-18 17:24 ` [PATCH v3 2/2] drivers: iio: dac: Add AD5754 DAC driver Ciprian Regus
  1 sibling, 1 reply; 6+ messages in thread
From: Ciprian Regus @ 2022-11-18 17:24 UTC (permalink / raw)
  To: jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree,
	linux-kernel
  Cc: Ciprian Regus

Add devicetree bindings documentation for the AD5754 DAC driver.

Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
---
 changes in v3:
  - added additionalProperties: false to patternProperties
  - dropped status in the example.
  - added different values for adi,output-range-microvolt in the example.
    Negative values cannot be set since that will create a dt_bindings_check error.
 .../bindings/iio/dac/adi,ad5754.yaml          | 181 ++++++++++++++++++
 1 file changed, 181 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml

diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml
new file mode 100644
index 000000000000..de0f6fab82b8
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml
@@ -0,0 +1,181 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/dac/adi,ad5754.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices AD5754 DAC
+
+maintainers:
+  - Ciprian Regus <ciprian.regus@analog.com>
+
+description: |
+  Bindings for the AD5754 and other chip variants digital-to-analog
+  converters.
+
+  https://www.analog.com/media/en/technical-documentation/data-sheets/AD5724_5734_5754.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad5722_5732_5752.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ad5724r_5734r_5754r.pdf
+  https://www.analog.com/media/en/technical-documentation/data-sheets/AD5722R_5732R_5752R.pdf
+
+properties:
+  compatible:
+    enum:
+      - adi,ad5722
+      - adi,ad5732
+      - adi,ad5752
+      - adi,ad5724
+      - adi,ad5734
+      - adi,ad5754
+      - adi,ad5722r
+      - adi,ad5732r
+      - adi,ad5752r
+      - adi,ad5724r
+      - adi,ad5734r
+      - adi,ad5754r
+
+  reg:
+    maxItems: 1
+
+  spi-max-frequency:
+    maximum: 30000000
+
+  spi-cpol: true
+
+  vref-supply:
+    description:
+      The regulator to use as an external reference. If this is not provided,
+      the internal reference will be used for chips that have this feature.
+      The external reference must be 2.5V.
+
+  clr-gpios:
+    description: DAC output clear GPIO (CLR pin). If specified, it will be set
+      to high during probe, thus allowing the DAC output to be updated.
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+patternProperties:
+  "^channel@([0-3])$":
+    type: object
+    additionalProperties: false
+    description: Configurations for the DAC channels
+
+    properties:
+      reg:
+        description: Channel number
+        maxItems: 1
+
+      adi,output-range-microvolt:
+        description: |
+          Voltage range of a channel as <minimum, maximum>.
+        oneOf:
+          - items:
+              - const: 0
+              - enum: [5000000, 10000000, 10800000]
+          - items:
+              - const: -5000000
+              - const: 5000000
+          - items:
+              - const: -10000000
+              - const: 10000000
+          - items:
+              - const: -10800000
+              - const: 10800000
+
+    required:
+      - reg
+      - adi,output-range-microvolt
+
+allOf:
+  - $ref: /schemas/spi/spi-peripheral-props.yaml#
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad5722
+              - adi,ad5732
+              - adi,ad5752
+              - adi,ad5722r
+              - adi,ad5732r
+              - adi,ad5752r
+    then:
+      patternProperties:
+        "^channel@([0-3])$":
+          type: object
+          properties:
+            reg:
+              description: Channel number
+              enum: [0, 1]
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - adi,ad5724
+              - adi,ad5734
+              - adi,ad5754
+              - adi,ad5724r
+              - adi,ad5734r
+              - adi,ad5754r
+    then:
+      patternProperties:
+        "^channel@([0-3])$":
+          type: object
+          properties:
+            reg:
+              description: Channel number
+              enum: [0, 1, 2, 3]
+
+required:
+  - compatible
+  - reg
+  - spi-max-frequency
+  - spi-cpol
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+
+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        dac@0 {
+            compatible = "adi,ad5754r";
+            reg = <0>;
+            spi-max-frequency = <1000000>;
+            spi-cpol;
+
+            clr-gpios = <&gpio 25 GPIO_ACTIVE_LOW>;
+
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            channel@0 {
+                reg = <0>;
+                adi,output-range-microvolt = <0 5000000>;
+            };
+            channel@1 {
+                reg = <1>;
+                adi,output-range-microvolt = <0 10000000>;
+            };
+            channel@2 {
+                reg = <2>;
+                adi,output-range-microvolt = <0 5000000>;
+            };
+            channel@3 {
+                reg = <3>;
+                adi,output-range-microvolt = <0 10000000>;
+            };
+        };
+    };
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/2] drivers: iio: dac: Add AD5754 DAC driver
  2022-11-18 17:24 [PATCH v3 0/2] Add support for the AD5754 DAC Ciprian Regus
  2022-11-18 17:24 ` [PATCH v3 1/2] dt-bindings: iio: dac: add adi,ad5754.yaml Ciprian Regus
@ 2022-11-18 17:24 ` Ciprian Regus
  2022-11-23 17:45   ` Jonathan Cameron
  1 sibling, 1 reply; 6+ messages in thread
From: Ciprian Regus @ 2022-11-18 17:24 UTC (permalink / raw)
  To: jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree,
	linux-kernel
  Cc: Ciprian Regus

The AD5724/AD5734/AD5754 are quad, 12-/14-/16-bit, serial input, voltage
output DACs. The devices operate from single-supply voltages from +4.5 V
up to +16.5 V or dual-supply voltages from ±4.5 V up to ±16.5 V. The
input coding is user-selectable twos complement or offset binary for a
bipolar output (depending on the state of Pin BIN/2sComp), and straight
binary for a unipolar output.

Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5724_5734_5754.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5722_5732_5752.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5724r_5734r_5754r.pdf
Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5722R_5732R_5752R.pdf
Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
---
 changes in v3:
  - wrapped the commit message to 70-75 characters.
  - added a lock for the reg_read function in order to synchronize sequential transfers.
  - replaced explicit size setting with ARRAY_SIZE() in ad5754_set_dac_code().
  - enabled all channels at once, thus also removing the side effects of the
    ad5754_enable_channels().
  - removed the vref field from the ad5754_state struct, since it's not necessary to cache
    the vref value.
  - fixed the scale formula, since the one used in v2 would only work for a 2.5V reference.
  - split the devm_ unwind path into 3 functions (instead of a single one).
 MAINTAINERS              |   8 +
 drivers/iio/dac/Kconfig  |  12 +
 drivers/iio/dac/Makefile |   1 +
 drivers/iio/dac/ad5754.c | 672 +++++++++++++++++++++++++++++++++++++++
 4 files changed, 693 insertions(+)
 create mode 100644 drivers/iio/dac/ad5754.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4e5d1d737cfb..6fa48820c969 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1091,6 +1091,14 @@ T:	git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net.git
 T:	git git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git
 F:	drivers/net/amt.c
 
+ANALOG DEVICES INC AD5754 DRIVER
+M:	Ciprian Regus <ciprian.regus@analog.com>
+L:	linux-iio@vger.kernel.org
+S:	Supported
+W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml
+F:	drivers/iio/dac/ad5754.c
+
 ANALOG DEVICES INC AD7192 DRIVER
 M:	Alexandru Tachici <alexandru.tachici@analog.com>
 L:	linux-iio@vger.kernel.org
diff --git a/drivers/iio/dac/Kconfig b/drivers/iio/dac/Kconfig
index 80521bd28d0f..561bfbedeecb 100644
--- a/drivers/iio/dac/Kconfig
+++ b/drivers/iio/dac/Kconfig
@@ -170,6 +170,18 @@ config AD5696_I2C
 	  To compile this driver as a module, choose M here: the module will be
 	  called ad5696.
 
+config AD5754
+	tristate "Analog Devices AD5754 and similar DACs"
+	depends on SPI
+	select REGMAP_SPI
+	help
+	  Say yes here to build support for Analog Devices AD5752, AD5732,
+	  AD5722, AD5754, AD5734, AD5724, AD5752R, AD5732R, AD5722R, AD5754R,
+	  AD5734R, AD5724R dual/quad channel Digital to Analog Converters.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ad5754.
+
 config AD5755
 	tristate "Analog Devices AD5755/AD5755-1/AD5757/AD5735/AD5737 DAC driver"
 	depends on SPI_MASTER
diff --git a/drivers/iio/dac/Makefile b/drivers/iio/dac/Makefile
index ec3e42713f00..d6c18deed1f7 100644
--- a/drivers/iio/dac/Makefile
+++ b/drivers/iio/dac/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_AD5449) += ad5449.o
 obj-$(CONFIG_AD5592R_BASE) += ad5592r-base.o
 obj-$(CONFIG_AD5592R) += ad5592r.o
 obj-$(CONFIG_AD5593R) += ad5593r.o
+obj-$(CONFIG_AD5754) += ad5754.o
 obj-$(CONFIG_AD5755) += ad5755.o
 obj-$(CONFIG_AD5755) += ad5758.o
 obj-$(CONFIG_AD5761) += ad5761.o
diff --git a/drivers/iio/dac/ad5754.c b/drivers/iio/dac/ad5754.c
new file mode 100644
index 000000000000..81cfda2efa0f
--- /dev/null
+++ b/drivers/iio/dac/ad5754.c
@@ -0,0 +1,672 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2022 Analog Devices, Inc.
+ * Author: Ciprian Regus <ciprian.regus@analog.com>
+ */
+
+#include <linux/bitfield.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/iio/iio.h>
+#include <linux/kernel.h>
+#include <linux/linear_range.h>
+#include <linux/mod_devicetable.h>
+#include <linux/property.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/spi/spi.h>
+
+#include <asm/unaligned.h>
+
+#define AD5754_INT_VREF_mV			2500
+#define AD5754_FRAME_SIZE			3
+#define AD5754_MAX_CHANNELS			4
+#define AD5754_MAX_RESOLUTION			16
+#define uV_PER_mV				1000
+
+#define AD5754_DATA_MASK(_lsb)		GENMASK(15, _lsb)
+#define AD5754_RANGE_MASK		GENMASK(2, 0)
+
+#define AD5754_REG_RD			BIT(7)
+
+#define AD5754_CLEAR_FUNC		0x4
+#define AD5754_LOAD_FUNC		0x5
+#define AD5754_NOOP_FUNC		0x0
+
+#define AD5754_PU_ADDR			0x0
+#define AD5754_PU_MASK			GENMASK(3, 0)
+#define AD5754_PU_CH(x)			BIT(x)
+#define AD5754_INT_REF_MASK		BIT(4)
+
+#define AD5754_DAC_REG			0x0
+#define AD5754_RANGE_REG		0x1
+#define AD5754_PWR_REG			0x2
+#define AD5754_CTRL_REG			0x3
+
+#define AD5754_REG_ADDR(reg, addr)	(((reg) << 3) | (addr))
+
+#define AD5754_CHANNEL(_channel)					\
+	{								\
+		.type = IIO_VOLTAGE,					\
+		.indexed = 1,						\
+		.channel = _channel,					\
+		.output = 1,						\
+		.info_mask_separate = BIT(IIO_CHAN_INFO_RAW)    |	\
+				      BIT(IIO_CHAN_INFO_SCALE)  |	\
+				      BIT(IIO_CHAN_INFO_OFFSET)		\
+	}
+
+struct ad5754_span_tbl {
+	int min;
+	int max;
+};
+
+static const struct ad5754_span_tbl ad5754_range[] = {
+	{0, 5000000},
+	{0, 10000000},
+	{0, 10800000},
+	{-5000000, 5000000},
+	{-10000000, 10000000},
+	{-10800000, 10800000},
+};
+
+enum AD5754_TYPE {
+	AD5722,
+	AD5732,
+	AD5752,
+	AD5724,
+	AD5734,
+	AD5754,
+	AD5722R,
+	AD5732R,
+	AD5752R,
+	AD5724R,
+	AD5734R,
+	AD5754R,
+};
+
+struct ad5754_chip_info {
+	const char *name;
+	u32 resolution;
+	bool internal_vref;
+	const u32 data_mask;
+	const struct iio_chan_spec *channels;
+	u32 num_channels;
+};
+
+static const struct iio_chan_spec ad5754_2_channels[2] = {
+		AD5754_CHANNEL(0),
+		AD5754_CHANNEL(1),
+};
+
+static const struct iio_chan_spec ad5754_4_channels[4] = {
+		AD5754_CHANNEL(0),
+		AD5754_CHANNEL(1),
+		AD5754_CHANNEL(2),
+		AD5754_CHANNEL(3),
+};
+
+static const struct ad5754_chip_info ad5754_chip_info_data[] = {
+	[AD5722] = {
+		.name = "ad5722",
+		.resolution = 12,
+		.data_mask = AD5754_DATA_MASK(4),
+		.internal_vref = false,
+		.num_channels = 2,
+		.channels = ad5754_2_channels,
+	},
+	[AD5732] = {
+		.name = "ad5732",
+		.resolution = 14,
+		.data_mask = AD5754_DATA_MASK(2),
+		.internal_vref = false,
+		.num_channels = 2,
+		.channels = ad5754_2_channels,
+	},
+	[AD5752] = {
+		.name = "ad5752",
+		.resolution = 16,
+		.data_mask = AD5754_DATA_MASK(0),
+		.internal_vref = false,
+		.num_channels = 2,
+		.channels = ad5754_2_channels,
+	},
+	[AD5724] = {
+		.name = "ad5724",
+		.resolution = 12,
+		.data_mask = AD5754_DATA_MASK(4),
+		.internal_vref = false,
+		.num_channels = 4,
+		.channels = ad5754_4_channels,
+	},
+	[AD5734] = {
+		.name = "ad5734",
+		.resolution = 14,
+		.data_mask = AD5754_DATA_MASK(2),
+		.internal_vref = false,
+		.num_channels = 4,
+		.channels = ad5754_4_channels,
+	},
+	[AD5754] = {
+		.name = "ad5754",
+		.resolution = 16,
+		.data_mask = AD5754_DATA_MASK(0),
+		.internal_vref = false,
+		.num_channels = 4,
+		.channels = ad5754_4_channels,
+	},
+	[AD5722R] = {
+		.name = "ad5722r",
+		.resolution = 12,
+		.data_mask = AD5754_DATA_MASK(4),
+		.internal_vref = true,
+		.num_channels = 2,
+		.channels = ad5754_2_channels,
+	},
+	[AD5732R] = {
+		.name = "ad5732r",
+		.resolution = 14,
+		.data_mask = AD5754_DATA_MASK(2),
+		.internal_vref = true,
+		.num_channels = 2,
+		.channels = ad5754_2_channels,
+	},
+	[AD5752R] = {
+		.name = "ad5752r",
+		.resolution = 16,
+		.data_mask = AD5754_DATA_MASK(0),
+		.internal_vref = true,
+		.num_channels = 2,
+		.channels = ad5754_2_channels,
+	},
+	[AD5724R] = {
+		.name = "ad5724r",
+		.resolution = 12,
+		.data_mask = AD5754_DATA_MASK(4),
+		.internal_vref = true,
+		.num_channels = 4,
+		.channels = ad5754_4_channels,
+	},
+	[AD5734R] = {
+		.name = "ad5734r",
+		.resolution = 14,
+		.data_mask = AD5754_DATA_MASK(2),
+		.internal_vref = true,
+		.num_channels = 4,
+		.channels = ad5754_4_channels,
+	},
+	[AD5754R] = {
+		.name = "ad5754r",
+		.resolution = 16,
+		.data_mask = AD5754_DATA_MASK(0),
+		.internal_vref = true,
+		.num_channels = 4,
+		.channels = ad5754_4_channels,
+	},
+};
+
+struct ad5754_state {
+	struct regmap *regmap;
+	struct spi_device *spi;
+
+	const struct ad5754_chip_info *chip_info;
+
+	/* Used for sequential SPI transfers synchronization */
+	struct mutex lock;
+
+	struct gpio_desc *clr_gpio;
+	struct regulator *vref_reg;
+	u32 range_idx[AD5754_MAX_CHANNELS];
+	int offset[AD5754_MAX_CHANNELS];
+	u32 data_mask;
+
+	/*
+	 * DMA (thus cache coherency maintenance) may require the
+	 * transfer buffers to live in their own cache lines.
+	 */
+	u8 buff[AD5754_FRAME_SIZE] __aligned(IIO_DMA_MINALIGN);
+};
+
+/*
+ * The channel addresses for 2 channel chip variants are not sequential:
+ *      A2 A1 A0 Channel
+ *	0  0  0   DAC A
+ *	0  1  0   DAC B
+ *
+ * This is not the case for 4 channel chips:
+ *	A2 A1 A0 Channel
+ *	0  0  0   DAC A
+ *	0  0  1   DAC B
+ *	0  1  0   DAC C
+ *	0  1  1   DAC D
+ */
+static unsigned int ad5754_real_ch(struct ad5754_state *st,
+				   u32 channel,
+				   u32 *real_channel)
+{
+	switch (st->chip_info->num_channels) {
+	case 2:
+		if (channel == 0)
+			*real_channel = 0;
+		else
+			*real_channel = 2;
+		break;
+	case 4:
+		*real_channel = channel;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int ad5754_get_output_range(struct ad5754_state *st,
+				   struct fwnode_handle *channel_node,
+				   u32 ch_idx)
+{
+	u32 range[2];
+	int min, max;
+	int ret;
+	u32 i;
+
+	ret = fwnode_property_read_u32_array(channel_node,
+					     "adi,output-range-microvolt",
+					     range, 2);
+	if (ret)
+		return ret;
+
+	min = range[0];
+	max = range[1];
+
+	for (i = 0; i < ARRAY_SIZE(ad5754_range); i++) {
+		if (ad5754_range[i].min != min || ad5754_range[i].max != max)
+			continue;
+
+		st->range_idx[ch_idx] = i;
+		if (min < 0)
+			st->offset[ch_idx] = -BIT(st->chip_info->resolution - 1);
+
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
+static int ad5754_reg_write(void *context, unsigned int reg, unsigned int val)
+{
+	struct ad5754_state *st = context;
+	struct spi_transfer xfer = {
+		.tx_buf = st->buff,
+		.len = 3,
+	};
+
+	st->buff[0] = reg;
+	put_unaligned_be16(val, &st->buff[1]);
+
+	return spi_sync_transfer(st->spi, &xfer, 1);
+};
+
+static int ad5754_reg_read(void *context, unsigned int reg, unsigned int *val)
+{
+	struct ad5754_state *st = context;
+	struct spi_transfer xfer[] = {
+		{
+			.tx_buf = st->buff,
+			.len = AD5754_FRAME_SIZE,
+		},
+	};
+	int ret;
+
+	st->buff[0] = AD5754_REG_RD | reg;
+
+	mutex_lock(&st->lock);
+	ret = spi_sync_transfer(st->spi, xfer, 1);
+	if (ret)
+		goto unlock;
+
+	xfer->rx_buf = st->buff;
+	st->buff[0] = AD5754_REG_ADDR(AD5754_CTRL_REG, AD5754_NOOP_FUNC);
+	st->buff[1] = 0;
+	st->buff[2] = 0;
+	ret = spi_sync_transfer(st->spi, xfer, 1);
+	if (ret)
+		goto unlock;
+
+	*val = get_unaligned_be16(&st->buff[1]);
+
+unlock:
+	mutex_unlock(&st->lock);
+
+	return ret;
+};
+
+static const struct regmap_config ad5754_regmap_config = {
+	.reg_bits = 8,
+	.val_bits = 16,
+	.reg_write = ad5754_reg_write,
+	.reg_read = ad5754_reg_read,
+	.max_register = 0x23,
+};
+
+static int ad5754_set_dac_code(struct ad5754_state *st, u32 channel, u32 code)
+{
+	u32 sub_lsb = AD5754_MAX_RESOLUTION - st->chip_info->resolution;
+	struct reg_sequence xfer_seq[2] = {
+		{ AD5754_REG_ADDR(AD5754_DAC_REG, channel), code << sub_lsb },
+		{ AD5754_REG_ADDR(AD5754_CTRL_REG, AD5754_LOAD_FUNC), 0 },
+	};
+
+	return regmap_multi_reg_write(st->regmap, xfer_seq, ARRAY_SIZE(xfer_seq));
+}
+
+static int ad5754_enable_channels(struct ad5754_state *st)
+{
+	struct fwnode_handle *channel_node;
+	u32 en_channels = 0;
+	u32 real_channel;
+	u32 index;
+	int ret;
+
+	device_for_each_child_node(&st->spi->dev, channel_node) {
+		ret = fwnode_property_read_u32(channel_node, "reg", &index);
+		if (ret) {
+			dev_err(&st->spi->dev, "Failed to read channel reg: %d\n", ret);
+			goto free_node;
+		}
+		if (index >= st->chip_info->num_channels) {
+			dev_err(&st->spi->dev, "Channel index %u is too large\n", index);
+			goto free_node;
+		}
+
+		ret = ad5754_real_ch(st, index, &real_channel);
+		if (ret)
+			goto free_node;
+
+		ret = ad5754_get_output_range(st, channel_node, index);
+		if (ret)
+			goto free_node;
+
+		ret = regmap_write_bits(st->regmap,
+					AD5754_REG_ADDR(AD5754_RANGE_REG, real_channel),
+					AD5754_RANGE_MASK, st->range_idx[index]);
+		if (ret)
+			goto free_node;
+
+		en_channels |= AD5754_PU_CH(real_channel);
+	}
+
+	ret = regmap_update_bits(st->regmap,
+				 AD5754_REG_ADDR(AD5754_PWR_REG, AD5754_PU_ADDR),
+				 AD5754_PU_MASK,
+				 FIELD_PREP(AD5754_PU_MASK, en_channels));
+	if (ret)
+		return ret;
+
+	/* Channel power up delay */
+	fsleep(10);
+
+	return 0;
+
+free_node:
+	fwnode_handle_put(channel_node);
+
+	return ret;
+}
+
+static int ad5754_write_raw(struct iio_dev *indio_dev,
+			    struct iio_chan_spec const *chan,
+			    int val, int val2, long info)
+{
+	struct ad5754_state *st = iio_priv(indio_dev);
+	u32 real_channel;
+	int ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		if (val < 0 || val >= BIT(st->chip_info->resolution)) {
+			dev_err(&st->spi->dev, "Invalid DAC code %d\n", val);
+			return -EINVAL;
+		}
+		ret = ad5754_real_ch(st, chan->channel, &real_channel);
+		if (ret)
+			return ret;
+
+		return ad5754_set_dac_code(st, real_channel, val);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int ad5754_read_raw(struct iio_dev *indio_dev,
+			   struct iio_chan_spec const *chan,
+			   int *val, int *val2, long info)
+{
+	struct ad5754_state *st = iio_priv(indio_dev);
+	const struct ad5754_span_tbl *range;
+	u32 real_channel;
+	u32 gain;
+	int vref;
+	int ret;
+
+	ret = ad5754_real_ch(st, chan->channel, &real_channel);
+	if (ret)
+		return ret;
+
+	switch (info) {
+	case IIO_CHAN_INFO_RAW:
+		ret = regmap_read(st->regmap, real_channel, val);
+		if (ret)
+			return ret;
+
+		*val >>= AD5754_MAX_RESOLUTION - st->chip_info->resolution;
+
+		return IIO_VAL_INT;
+	case IIO_CHAN_INFO_SCALE:
+		if (st->chip_info->internal_vref) {
+			vref = AD5754_INT_VREF_mV;
+		} else {
+			vref = regulator_get_voltage(st->vref_reg);
+			if (vref < 0)
+				return vref;
+
+			vref /= uV_PER_mV;
+		}
+
+		range = &ad5754_range[st->range_idx[chan->channel]];
+		gain = (range->max - range->min) / AD5754_INT_VREF_mV;
+		*val = vref * gain / uV_PER_mV;
+		*val2 = st->chip_info->resolution;
+
+		return IIO_VAL_FRACTIONAL_LOG2;
+	case IIO_CHAN_INFO_OFFSET:
+		*val = st->offset[chan->channel];
+
+		return IIO_VAL_INT;
+	default:
+		return -EINVAL;
+	}
+}
+
+static void ad5754_channels_disable(void *state)
+{
+	struct ad5754_state *st = state;
+
+	regmap_update_bits(st->regmap,
+			   AD5754_REG_ADDR(AD5754_PWR_REG, AD5754_PU_ADDR),
+			   AD5754_PU_MASK, FIELD_PREP(AD5754_PU_MASK, 0));
+}
+
+static void ad5754_disable_int_ref(void *state)
+{
+	struct ad5754_state *st = state;
+
+	regmap_update_bits(st->regmap,
+			   AD5754_REG_ADDR(AD5754_PWR_REG, AD5754_PU_ADDR),
+			   AD5754_INT_REF_MASK,
+			   FIELD_PREP(AD5754_INT_REF_MASK, 0));
+}
+
+static void ad5754_regulator_disable(void *regulator)
+{
+	regulator_disable(regulator);
+}
+
+static const struct iio_info ad5754_info = {
+	.read_raw = &ad5754_read_raw,
+	.write_raw = &ad5754_write_raw,
+};
+
+static int ad5754_probe(struct spi_device *spi)
+{
+	struct device *dev = &spi->dev;
+	struct iio_dev *indio_dev;
+	struct ad5754_state *st;
+	int ret;
+
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*st));
+	if (!indio_dev)
+		return -ENOMEM;
+
+	st = iio_priv(indio_dev);
+
+	st->spi = spi;
+	st->chip_info = device_get_match_data(dev);
+	if (!st->chip_info)
+		st->chip_info = (void *)spi_get_device_id(spi)->driver_data;
+
+	mutex_init(&st->lock);
+
+	st->regmap = devm_regmap_init(dev, NULL, st, &ad5754_regmap_config);
+	if (IS_ERR(st->regmap))
+		return dev_err_probe(dev, PTR_ERR(st->regmap),
+				     "Regmap init error\n");
+
+	st->clr_gpio = devm_gpiod_get_optional(dev, "clr", GPIOD_OUT_LOW);
+	if (IS_ERR(st->clr_gpio))
+		return PTR_ERR(st->clr_gpio);
+
+	/* Clear DAC outputs */
+	ret = regmap_write(st->regmap,
+			   AD5754_REG_ADDR(AD5754_CTRL_REG, AD5754_CLEAR_FUNC),
+			   0);
+	if (ret)
+		return ret;
+
+	st->vref_reg = devm_regulator_get_optional(dev, "vref");
+	if (IS_ERR(st->vref_reg)) {
+		if (!st->chip_info->internal_vref)
+			return dev_err_probe(dev, PTR_ERR(st->vref_reg),
+					     "Failed to get the vref regulator\n");
+
+		ret = regmap_update_bits(st->regmap,
+					 AD5754_REG_ADDR(AD5754_PWR_REG, AD5754_PU_ADDR),
+					 AD5754_INT_REF_MASK,
+					 FIELD_PREP(AD5754_INT_REF_MASK, 1));
+		if (ret)
+			return ret;
+
+		ret = devm_add_action_or_reset(dev, ad5754_disable_int_ref, st);
+		if (ret)
+			return ret;
+	} else {
+		ret = regulator_enable(st->vref_reg);
+		if (ret)
+			return dev_err_probe(dev, ret,
+					     "Failed to enable the vref regulator\n");
+
+		ret = devm_add_action_or_reset(dev, ad5754_regulator_disable, st->vref_reg);
+		if (ret)
+			return ret;
+	}
+
+	indio_dev->name = st->chip_info->name;
+	indio_dev->modes = INDIO_DIRECT_MODE;
+	indio_dev->info = &ad5754_info;
+	indio_dev->channels = st->chip_info->channels;
+	indio_dev->num_channels = st->chip_info->num_channels;
+
+	ret = ad5754_enable_channels(st);
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, ad5754_channels_disable, st);
+	if (ret)
+		return ret;
+
+	return devm_iio_device_register(dev, indio_dev);
+}
+
+static const struct spi_device_id ad5754_id[] = {
+	{ "ad5722", (kernel_ulong_t)&ad5754_chip_info_data[AD5722], },
+	{ "ad5732", (kernel_ulong_t)&ad5754_chip_info_data[AD5732], },
+	{ "ad5752", (kernel_ulong_t)&ad5754_chip_info_data[AD5752], },
+	{ "ad5724", (kernel_ulong_t)&ad5754_chip_info_data[AD5724], },
+	{ "ad5734", (kernel_ulong_t)&ad5754_chip_info_data[AD5734], },
+	{ "ad5754", (kernel_ulong_t)&ad5754_chip_info_data[AD5754], },
+	{ "ad5722r", (kernel_ulong_t)&ad5754_chip_info_data[AD5722R], },
+	{ "ad5732r", (kernel_ulong_t)&ad5754_chip_info_data[AD5732R], },
+	{ "ad5752r", (kernel_ulong_t)&ad5754_chip_info_data[AD5752R], },
+	{ "ad5724r", (kernel_ulong_t)&ad5754_chip_info_data[AD5724R], },
+	{ "ad5734r", (kernel_ulong_t)&ad5754_chip_info_data[AD5734R], },
+	{ "ad5754r", (kernel_ulong_t)&ad5754_chip_info_data[AD5754R], },
+	{}
+};
+
+static const struct of_device_id ad5754_dt_id[] = {
+	{
+		.compatible = "adi,ad5722",
+		.data = &ad5754_chip_info_data[AD5722],
+	}, {
+		.compatible = "adi,ad5732",
+		.data = &ad5754_chip_info_data[AD5732],
+	}, {
+		.compatible = "adi,ad5752",
+		.data = &ad5754_chip_info_data[AD5752],
+	}, {
+		.compatible = "adi,ad5724",
+		.data = &ad5754_chip_info_data[AD5724],
+	}, {
+		.compatible = "adi,ad5734",
+		.data = &ad5754_chip_info_data[AD5734],
+	}, {
+		.compatible = "adi,ad5754",
+		.data = &ad5754_chip_info_data[AD5754],
+	}, {
+		.compatible = "adi,ad5722r",
+		.data = &ad5754_chip_info_data[AD5722R],
+	}, {
+		.compatible = "adi,ad5732r",
+		.data = &ad5754_chip_info_data[AD5732R],
+	}, {
+		.compatible = "adi,ad5752r",
+		.data = &ad5754_chip_info_data[AD5752R],
+	}, {
+		.compatible = "adi,ad5724r",
+		.data = &ad5754_chip_info_data[AD5724R],
+	}, {
+		.compatible = "adi,ad5734r",
+		.data = &ad5754_chip_info_data[AD5734R],
+	}, {
+		.compatible = "adi,ad5754r",
+		.data = &ad5754_chip_info_data[AD5754R],
+	},
+	{}
+};
+MODULE_DEVICE_TABLE(of, ad5754_dt_id);
+
+static struct spi_driver ad5754_driver = {
+	.driver = {
+		.name = "ad5754",
+		.of_match_table = ad5754_dt_id,
+	},
+	.probe = ad5754_probe,
+	.id_table = ad5754_id,
+};
+
+module_spi_driver(ad5754_driver);
+
+MODULE_AUTHOR("Ciprian Regus <ciprian.regus@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD5754 DAC");
+MODULE_LICENSE("GPL");
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: iio: dac: add adi,ad5754.yaml
  2022-11-18 17:24 ` [PATCH v3 1/2] dt-bindings: iio: dac: add adi,ad5754.yaml Ciprian Regus
@ 2022-11-21 11:20   ` Krzysztof Kozlowski
  2022-11-23 17:29     ` Jonathan Cameron
  0 siblings, 1 reply; 6+ messages in thread
From: Krzysztof Kozlowski @ 2022-11-21 11:20 UTC (permalink / raw)
  To: Ciprian Regus, jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio,
	devicetree, linux-kernel

On 18/11/2022 18:24, Ciprian Regus wrote:
> Add devicetree bindings documentation for the AD5754 DAC driver.
> 
> Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
> ---
>  changes in v3:
>   - added additionalProperties: false to patternProperties
>   - dropped status in the example.
>   - added different values for adi,output-range-microvolt in the example.
>     Negative values cannot be set since that will create a dt_bindings_check error.
>  .../bindings/iio/dac/adi,ad5754.yaml          | 181 ++++++++++++++++++
>  1 file changed, 181 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml
> new file mode 100644
> index 000000000000..de0f6fab82b8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml
> @@ -0,0 +1,181 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iio/dac/adi,ad5754.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices AD5754 DAC
> +
> +maintainers:
> +  - Ciprian Regus <ciprian.regus@analog.com>
> +
> +description: |
> +  Bindings for the AD5754 and other chip variants digital-to-analog
> +  converters.
> +
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/AD5724_5734_5754.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad5722_5732_5752.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad5724r_5734r_5754r.pdf
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/AD5722R_5732R_5752R.pdf
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,ad5722
> +      - adi,ad5732
> +      - adi,ad5752
> +      - adi,ad5724
> +      - adi,ad5734
> +      - adi,ad5754

Keep the list sorted.

> +      - adi,ad5722r

I would even suggest sorted entirely, so 5722r follows 5722, but I don't
mind some combo-sorting (logical + alphabetical).

> +      - adi,ad5732r
> +      - adi,ad5752r
> +      - adi,ad5724r
> +      - adi,ad5734r
> +      - adi,ad5754r
> +
> +  reg:
> +    maxItems: 1
> +
> +  spi-max-frequency:
> +    maximum: 30000000
> +
> +  spi-cpol: true
> +
> +  vref-supply:
> +    description:
> +      The regulator to use as an external reference. If this is not provided,
> +      the internal reference will be used for chips that have this feature.
> +      The external reference must be 2.5V.
> +
> +  clr-gpios:
> +    description: DAC output clear GPIO (CLR pin). If specified, it will be set
> +      to high during probe, thus allowing the DAC output to be updated.
> +    maxItems: 1
> +
> +  '#address-cells':
> +    const: 1
> +
> +  '#size-cells':
> +    const: 0
> +
> +patternProperties:
> +  "^channel@([0-3])$":

No need for ().

> +    type: object
> +    additionalProperties: false
> +    description: Configurations for the DAC channels
> +
> +    properties:
> +      reg:
> +        description: Channel number
> +        maxItems: 1
> +
> +      adi,output-range-microvolt:
> +        description: |
> +          Voltage range of a channel as <minimum, maximum>.
> +        oneOf:
> +          - items:
> +              - const: 0
> +              - enum: [5000000, 10000000, 10800000]
> +          - items:
> +              - const: -5000000
> +              - const: 5000000
> +          - items:
> +              - const: -10000000
> +              - const: 10000000
> +          - items:
> +              - const: -10800000
> +              - const: 10800000
> +
> +    required:
> +      - reg
> +      - adi,output-range-microvolt
> +
> +allOf:
> +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,ad5722
> +              - adi,ad5732
> +              - adi,ad5752
> +              - adi,ad5722r
> +              - adi,ad5732r
> +              - adi,ad5752r
> +    then:
> +      patternProperties:
> +        "^channel@([0-3])$":

No need for ().

> +          type: object
> +          properties:
> +            reg:
> +              description: Channel number
> +              enum: [0, 1]
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - adi,ad5724
> +              - adi,ad5734
> +              - adi,ad5754
> +              - adi,ad5724r
> +              - adi,ad5734r
> +              - adi,ad5754r
> +    then:
> +      patternProperties:
> +        "^channel@([0-3])$":

No need for ().

> +          type: object
> +          properties:
> +            reg:
> +              description: Channel number
> +              enum: [0, 1, 2, 3]
> +


Best regards,
Krzysztof


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 1/2] dt-bindings: iio: dac: add adi,ad5754.yaml
  2022-11-21 11:20   ` Krzysztof Kozlowski
@ 2022-11-23 17:29     ` Jonathan Cameron
  0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2022-11-23 17:29 UTC (permalink / raw)
  To: Krzysztof Kozlowski
  Cc: Ciprian Regus, jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio,
	devicetree, linux-kernel

On Mon, 21 Nov 2022 12:20:39 +0100
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 18/11/2022 18:24, Ciprian Regus wrote:
> > Add devicetree bindings documentation for the AD5754 DAC driver.
> > 
> > Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>
> > ---
> >  changes in v3:
> >   - added additionalProperties: false to patternProperties
> >   - dropped status in the example.
> >   - added different values for adi,output-range-microvolt in the example.
> >     Negative values cannot be set since that will create a dt_bindings_check error.
> >  .../bindings/iio/dac/adi,ad5754.yaml          | 181 ++++++++++++++++++
> >  1 file changed, 181 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml b/Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml
> > new file mode 100644
> > index 000000000000..de0f6fab82b8
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/dac/adi,ad5754.yaml
> > @@ -0,0 +1,181 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/iio/dac/adi,ad5754.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices AD5754 DAC
> > +
> > +maintainers:
> > +  - Ciprian Regus <ciprian.regus@analog.com>
> > +
> > +description: |
> > +  Bindings for the AD5754 and other chip variants digital-to-analog
> > +  converters.
> > +
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/AD5724_5734_5754.pdf
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad5722_5732_5752.pdf
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/ad5724r_5734r_5754r.pdf
> > +  https://www.analog.com/media/en/technical-documentation/data-sheets/AD5722R_5732R_5752R.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,ad5722
> > +      - adi,ad5732
> > +      - adi,ad5752
> > +      - adi,ad5724
> > +      - adi,ad5734
> > +      - adi,ad5754  
> 
> Keep the list sorted.
> 
> > +      - adi,ad5722r  
> 
> I would even suggest sorted entirely, so 5722r follows 5722, but I don't
> mind some combo-sorting (logical + alphabetical).

I was curious about this as well, so went digging.

Wonderfully they are grouped by logical set here - with each group of
3 being on a different datasheet (visible datasheet names above),
with associated commonality.

Still I'm fine with whatever ordering makes sense. Maybe it's just easier
to smash them into simple alphabetical order both here and in the driver
and not worry about where the order comes from.

If doing that, make sure you also do it for the sub lists below.

> 
> > +      - adi,ad5732r
> > +      - adi,ad5752r
> > +      - adi,ad5724r
> > +      - adi,ad5734r
> > +      - adi,ad5754r
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  spi-max-frequency:
> > +    maximum: 30000000
> > +
> > +  spi-cpol: true
> > +
> > +  vref-supply:
> > +    description:
> > +      The regulator to use as an external reference. If this is not provided,
> > +      the internal reference will be used for chips that have this feature.
> > +      The external reference must be 2.5V.
> > +
> > +  clr-gpios:
> > +    description: DAC output clear GPIO (CLR pin). If specified, it will be set
> > +      to high during probe, thus allowing the DAC output to be updated.
> > +    maxItems: 1
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^channel@([0-3])$":  
> 
> No need for ().
> 
> > +    type: object
> > +    additionalProperties: false
> > +    description: Configurations for the DAC channels
> > +
> > +    properties:
> > +      reg:
> > +        description: Channel number
> > +        maxItems: 1
> > +
> > +      adi,output-range-microvolt:
> > +        description: |
> > +          Voltage range of a channel as <minimum, maximum>.
> > +        oneOf:
> > +          - items:
> > +              - const: 0
> > +              - enum: [5000000, 10000000, 10800000]
> > +          - items:
> > +              - const: -5000000
> > +              - const: 5000000
> > +          - items:
> > +              - const: -10000000
> > +              - const: 10000000
> > +          - items:
> > +              - const: -10800000
> > +              - const: 10800000
> > +
> > +    required:
> > +      - reg
> > +      - adi,output-range-microvolt
> > +
> > +allOf:
> > +  - $ref: /schemas/spi/spi-peripheral-props.yaml#
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - adi,ad5722
> > +              - adi,ad5732
> > +              - adi,ad5752
> > +              - adi,ad5722r
> > +              - adi,ad5732r
> > +              - adi,ad5752r
> > +    then:
> > +      patternProperties:
> > +        "^channel@([0-3])$":  
> 
> No need for ().
> 
> > +          type: object
> > +          properties:
> > +            reg:
> > +              description: Channel number
> > +              enum: [0, 1]
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - adi,ad5724
> > +              - adi,ad5734
> > +              - adi,ad5754
> > +              - adi,ad5724r
> > +              - adi,ad5734r
> > +              - adi,ad5754r
> > +    then:
> > +      patternProperties:
> > +        "^channel@([0-3])$":  
> 
> No need for ().
> 
> > +          type: object
> > +          properties:
> > +            reg:
> > +              description: Channel number
> > +              enum: [0, 1, 2, 3]
> > +  
> 
> 
> Best regards,
> Krzysztof
> 


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/2] drivers: iio: dac: Add AD5754 DAC driver
  2022-11-18 17:24 ` [PATCH v3 2/2] drivers: iio: dac: Add AD5754 DAC driver Ciprian Regus
@ 2022-11-23 17:45   ` Jonathan Cameron
  0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2022-11-23 17:45 UTC (permalink / raw)
  To: Ciprian Regus
  Cc: jic23, krzysztof.kozlowski+dt, robh+dt, linux-iio, devicetree,
	linux-kernel

On Fri, 18 Nov 2022 19:24:07 +0200
Ciprian Regus <ciprian.regus@analog.com> wrote:

> The AD5724/AD5734/AD5754 are quad, 12-/14-/16-bit, serial input, voltage
> output DACs. The devices operate from single-supply voltages from +4.5 V
> up to +16.5 V or dual-supply voltages from ±4.5 V up to ±16.5 V. The
> input coding is user-selectable twos complement or offset binary for a
> bipolar output (depending on the state of Pin BIN/2sComp), and straight
> binary for a unipolar output.
> 
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5724_5734_5754.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5722_5732_5752.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/ad5724r_5734r_5754r.pdf
> Datasheet: https://www.analog.com/media/en/technical-documentation/data-sheets/AD5722R_5732R_5752R.pdf
> Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>

Hi Ciprian,

Took another look and found a few more things given you'll be spinning
again for the dt binding.

Only significant one is the handling of error returns form the optional regulator.

Jonathan

> diff --git a/drivers/iio/dac/ad5754.c b/drivers/iio/dac/ad5754.c
> new file mode 100644
> index 000000000000..81cfda2efa0f
> --- /dev/null
> +++ b/drivers/iio/dac/ad5754.c
> @@ -0,0 +1,672 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2022 Analog Devices, Inc.
> + * Author: Ciprian Regus <ciprian.regus@analog.com>
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/iio/iio.h>
> +#include <linux/kernel.h>
> +#include <linux/linear_range.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/spi/spi.h>
> +
> +#include <asm/unaligned.h>
> +
> +#define AD5754_INT_VREF_mV			2500
> +#define AD5754_FRAME_SIZE			3
> +#define AD5754_MAX_CHANNELS			4
> +#define AD5754_MAX_RESOLUTION			16
> +#define uV_PER_mV				1000
In common with units.h probably better to spell out than mix case
(as someone will come along and 'fix' this for you ;)

#define MICROVOLT_PER_MILLIVOLT

Or just use MICRO/MILLI inline from units.h and rely on compiler
work the maths out for you.


> +
> +static const struct iio_chan_spec ad5754_2_channels[2] = {
> +		AD5754_CHANNEL(0),
> +		AD5754_CHANNEL(1),
> +};
> +
> +static const struct iio_chan_spec ad5754_4_channels[4] = {
> +		AD5754_CHANNEL(0),
> +		AD5754_CHANNEL(1),
> +		AD5754_CHANNEL(2),
> +		AD5754_CHANNEL(3),

Why the double tab for alignment of these?

> +};
..

> +
> +static int ad5754_reg_write(void *context, unsigned int reg, unsigned int val)
> +{
> +	struct ad5754_state *st = context;
> +	struct spi_transfer xfer = {
> +		.tx_buf = st->buff,
> +		.len = 3,
> +	};
> +
> +	st->buff[0] = reg;
> +	put_unaligned_be16(val, &st->buff[1]);
> +
> +	return spi_sync_transfer(st->spi, &xfer, 1);

spi_write()?
Same thing under the hood but slightly shorter code here.

> +};


> +
> +static int ad5754_probe(struct spi_device *spi)
> +{
> +	struct device *dev = &spi->dev;
> +	struct iio_dev *indio_dev;
> +	struct ad5754_state *st;
> +	int ret;
> +

> +
> +	st->vref_reg = devm_regulator_get_optional(dev, "vref");
> +	if (IS_ERR(st->vref_reg)) {

Subtle corner here.
The return may not indicate that vref was not provided, it might return
-EPROBEDEFER for example to indicate that we need to back off until
another driver is loaded.

As such, you need separate handling for -ENODEV (I think that's the return
for not specified) which is what you have here, and other error codes which
should be a probe failure via a dev_err_probe() to deal with the deferred case
debug info.

> +		if (!st->chip_info->internal_vref)
> +			return dev_err_probe(dev, PTR_ERR(st->vref_reg),
> +					     "Failed to get the vref regulator\n");
> +
> +		ret = regmap_update_bits(st->regmap,
> +					 AD5754_REG_ADDR(AD5754_PWR_REG, AD5754_PU_ADDR),
> +					 AD5754_INT_REF_MASK,
> +					 FIELD_PREP(AD5754_INT_REF_MASK, 1));
> +		if (ret)
> +			return ret;
> +
> +		ret = devm_add_action_or_reset(dev, ad5754_disable_int_ref, st);
> +		if (ret)
> +			return ret;
> +	} else {
> +		ret = regulator_enable(st->vref_reg);
> +		if (ret)
> +			return dev_err_probe(dev, ret,
> +					     "Failed to enable the vref regulator\n");
> +
> +		ret = devm_add_action_or_reset(dev, ad5754_regulator_disable, st->vref_reg);
> +		if (ret)
> +			return ret;
> +	}
...

> +static const struct spi_device_id ad5754_id[] = {
> +	{ "ad5722", (kernel_ulong_t)&ad5754_chip_info_data[AD5722], },
> +	{ "ad5732", (kernel_ulong_t)&ad5754_chip_info_data[AD5732], },
> +	{ "ad5752", (kernel_ulong_t)&ad5754_chip_info_data[AD5752], },
> +	{ "ad5724", (kernel_ulong_t)&ad5754_chip_info_data[AD5724], },
> +	{ "ad5734", (kernel_ulong_t)&ad5754_chip_info_data[AD5734], },
> +	{ "ad5754", (kernel_ulong_t)&ad5754_chip_info_data[AD5754], },
> +	{ "ad5722r", (kernel_ulong_t)&ad5754_chip_info_data[AD5722R], },
> +	{ "ad5732r", (kernel_ulong_t)&ad5754_chip_info_data[AD5732R], },
> +	{ "ad5752r", (kernel_ulong_t)&ad5754_chip_info_data[AD5752R], },
> +	{ "ad5724r", (kernel_ulong_t)&ad5754_chip_info_data[AD5724R], },
> +	{ "ad5734r", (kernel_ulong_t)&ad5754_chip_info_data[AD5734R], },
> +	{ "ad5754r", (kernel_ulong_t)&ad5754_chip_info_data[AD5754R], },
> +	{}
> +};
> +
> +static const struct of_device_id ad5754_dt_id[] = {
> +	{
> +		.compatible = "adi,ad5722",
> +		.data = &ad5754_chip_info_data[AD5722],

Whatever order you end up with in the DT binding after Krzysztof's review
replicate here and for the spi_device_ids above.

> +	}, {
> +		.compatible = "adi,ad5732",
> +		.data = &ad5754_chip_info_data[AD5732],
> +	}, {
> +		.compatible = "adi,ad5752",
> +		.data = &ad5754_chip_info_data[AD5752],
> +	}, {
> +		.compatible = "adi,ad5724",
> +		.data = &ad5754_chip_info_data[AD5724],
> +	}, {
> +		.compatible = "adi,ad5734",
> +		.data = &ad5754_chip_info_data[AD5734],
> +	}, {
> +		.compatible = "adi,ad5754",
> +		.data = &ad5754_chip_info_data[AD5754],
> +	}, {
> +		.compatible = "adi,ad5722r",
> +		.data = &ad5754_chip_info_data[AD5722R],
> +	}, {
> +		.compatible = "adi,ad5732r",
> +		.data = &ad5754_chip_info_data[AD5732R],
> +	}, {
> +		.compatible = "adi,ad5752r",
> +		.data = &ad5754_chip_info_data[AD5752R],
> +	}, {
> +		.compatible = "adi,ad5724r",
> +		.data = &ad5754_chip_info_data[AD5724R],
> +	}, {
> +		.compatible = "adi,ad5734r",
> +		.data = &ad5754_chip_info_data[AD5734R],
> +	}, {
> +		.compatible = "adi,ad5754r",
> +		.data = &ad5754_chip_info_data[AD5754R],
> +	},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, ad5754_dt_id);


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-11-23 17:45 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-11-18 17:24 [PATCH v3 0/2] Add support for the AD5754 DAC Ciprian Regus
2022-11-18 17:24 ` [PATCH v3 1/2] dt-bindings: iio: dac: add adi,ad5754.yaml Ciprian Regus
2022-11-21 11:20   ` Krzysztof Kozlowski
2022-11-23 17:29     ` Jonathan Cameron
2022-11-18 17:24 ` [PATCH v3 2/2] drivers: iio: dac: Add AD5754 DAC driver Ciprian Regus
2022-11-23 17:45   ` Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).