Taking over this series with David Lechner's aproval, to add some
fixes, proper handling of pseudo differential parts and
some extra commits adding support for 4-channel compatible parts.
Here is David's cover letter:
This series is adding a new driver for the Analog Devices Inc. AD7380,
AD7381, AD7383, and AD7384 ADCs. These chips are part of a family of
simultaneous sampling SAR ADCs.
To keep things simple, the initial driver implementation only supports
the 2/4-channel differential chips listed above. There are also 4-channel
single-ended chips in the family that can be added later.
Furthermore, the driver is just implementing basic support for capturing
data. Additional features like interrupts, CRC, etc. can be added later.
This work is being done by BayLibre and on behalf of Analog Devices Inc.
hence the maintainers are @analog.com.
To: Lars-Peter Clausen <[email protected]>
To: Michael Hennerich <[email protected]>
To: Nuno Sá <[email protected]>
To: David Lechner <[email protected]>
To: Jonathan Cameron <[email protected]>
To: Rob Herring <[email protected]>
To: Krzysztof Kozlowski <[email protected]>
To: Conor Dooley <[email protected]>
To: Liam Girdwood <[email protected]>
To: Mark Brown <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Changes in v5:
- make ad7380_regmap_config static
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
- don't use bool in FIELD_PREP
Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
- fix rx/tx buffer for regmap access
- add datasheet links of supported parts
- move reading reference voltage to probe function
- removed DIFFERENTIAL from a few macro names
Adding the following commits on top of the v4
- add supplies for pseudo-differential chips
- prepare driver to add more compatible parts
- add support for 4-channel compatible parts
- Link to v4: https://lore.kernel.org/all/[email protected]
Changes in v4:
- Dropped SPI bindings patch.
- Removed `spi-rx-bus-channels` from the adi,ad7380 bindings.
- Link to v3: https://lore.kernel.org/r/[email protected]
Changes in v3:
- dt-bindings:
- Picked up Conor's Reviewed-By on the adi,ad7380 bindings
- driver:
- Removed extra indent in DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL macro
- Removed scan mask that included timestamp channel
- Removed parent device assignment
- Picked up Nuno's Reviewed-by
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- dt-bindings:
- Added new patch with generic spi-rx-bus-channels property
- Added maxItems to reg property
- Replaced adi,sdo-mode property with spi-rx-bus-channels
- Made spi-rx-bus-channels property optional with default value of 1
(this made the if: check more complex)
- Changed example to use gpio for interrupt
- driver:
- Fixed CONFIG_AD7380 in Makefile
- rx_buf = st->scan_data.raw instead of rx_buf = &st->scan_data
- Moved iio_push_to_buffers_with_timestamp() outside of if statement
- Removed extra blank lines
- Renamed regulator disable function
- Dropped checking of adi,sdo-mode property (regardless of the actual
wiring, we can always use 1-wire mode)
- Added available_scan_masks
- Added check for missing driver match data
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Signed-off-by: Julien Stephan <[email protected]>
---
David Lechner (4):
dt-bindings: iio: adc: Add binding for AD7380 ADCs
iio: adc: ad7380: new driver for AD7380 ADCs
dt-bindings: iio: adc: ad7380: add pseudo-differential parts
iio: adc: ad7380: add support for pseudo-differential parts
Julien Stephan (3):
iio: adc: ad7380: prepare for parts with more channels
dt-bindings: iio: adc: ad7380: add support for ad738x-4 4 channels variants
iio: adc: ad7380: add support for ad738x-4 4 channels variants
.../devicetree/bindings/iio/adc/adi,ad7380.yaml | 148 +++++
MAINTAINERS | 10 +
drivers/iio/adc/Kconfig | 16 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad7380.c | 614 +++++++++++++++++++++
5 files changed, 789 insertions(+)
---
base-commit: 1446d8ef48196409f811c25071b2cc510a49fc60
change-id: 20240311-adding-new-ad738x-driver-5f9b54ad7c74
Best regards,
--
Julien Stephan <[email protected]>
From: David Lechner <[email protected]>
This adds a binding specification for the Analog Devices Inc. AD7380
family of ADCs.
Signed-off-by: David Lechner <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
.../devicetree/bindings/iio/adc/adi,ad7380.yaml | 82 ++++++++++++++++++++++
MAINTAINERS | 9 +++
2 files changed, 91 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
new file mode 100644
index 000000000000..5e1ee0ebe0a2
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
@@ -0,0 +1,82 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iio/adc/adi,ad7380.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices Simultaneous Sampling Analog to Digital Converters
+
+maintainers:
+ - Michael Hennerich <[email protected]>
+ - Nuno Sá <[email protected]>
+
+description: |
+ * https://www.analog.com/en/products/ad7380.html
+ * https://www.analog.com/en/products/ad7381.html
+
+$ref: /schemas/spi/spi-peripheral-props.yaml#
+
+properties:
+ compatible:
+ enum:
+ - adi,ad7380
+ - adi,ad7381
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 80000000
+ spi-cpol: true
+ spi-cpha: true
+
+ vcc-supply:
+ description: A 3V to 3.6V supply that powers the chip.
+
+ vlogic-supply:
+ description:
+ A 1.65V to 3.6V supply for the logic pins.
+
+ refio-supply:
+ description:
+ A 2.5V to 3.3V supply for the external reference voltage. When omitted,
+ the internal 2.5V reference is used.
+
+ interrupts:
+ description:
+ When the device is using 1-wire mode, this property is used to optionally
+ specify the ALERT interrupt.
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+ - vcc-supply
+ - vlogic-supply
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+
+ spi {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ adc@0 {
+ compatible = "adi,ad7380";
+ reg = <0>;
+
+ spi-cpol;
+ spi-cpha;
+ spi-max-frequency = <80000000>;
+
+ interrupts = <27 IRQ_TYPE_EDGE_FALLING>;
+ interrupt-parent = <&gpio0>;
+
+ vcc-supply = <&supply_3_3V>;
+ vlogic-supply = <&supply_3_3V>;
+ refio-supply = <&supply_2_5V>;
+ };
+ };
diff --git a/MAINTAINERS b/MAINTAINERS
index 7b1a6f2d0c9c..f7c512f3bbda 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -427,6 +427,15 @@ W: http://wiki.analog.com/AD7142
W: https://ez.analog.com/linux-software-drivers
F: drivers/input/misc/ad714x.c
+AD738X ADC DRIVER (AD7380/1/2/4)
+M: Michael Hennerich <[email protected]>
+M: Nuno Sá <[email protected]>
+R: David Lechner <[email protected]>
+S: Supported
+W: https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
+W: https://ez.analog.com/linux-software-drivers
+F: Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
+
AD7877 TOUCHSCREEN DRIVER
M: Michael Hennerich <[email protected]>
S: Supported
--
2.44.0
From: David Lechner <[email protected]>
Adding AD7383 and AD7384 compatible parts that are pseudo-differential.
Pseudo-differential require common mode voltage supplies, so add them
conditionally
Signed-off-by: David Lechner <[email protected]>
Signed-off-by: Julien Stephan <[email protected]>
---
.../devicetree/bindings/iio/adc/adi,ad7380.yaml | 32 ++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
index 5e1ee0ebe0a2..de3d28a021ae 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
@@ -13,6 +13,8 @@ maintainers:
description: |
* https://www.analog.com/en/products/ad7380.html
* https://www.analog.com/en/products/ad7381.html
+ * https://www.analog.com/en/products/ad7383.html
+ * https://www.analog.com/en/products/ad7384.html
$ref: /schemas/spi/spi-peripheral-props.yaml#
@@ -21,6 +23,8 @@ properties:
enum:
- adi,ad7380
- adi,ad7381
+ - adi,ad7383
+ - adi,ad7384
reg:
maxItems: 1
@@ -42,6 +46,16 @@ properties:
A 2.5V to 3.3V supply for the external reference voltage. When omitted,
the internal 2.5V reference is used.
+ aina-supply:
+ description:
+ The common mode voltage supply for the AINA- pin on pseudo-differential
+ chips.
+
+ ainb-supply:
+ description:
+ The common mode voltage supply for the AINB- pin on pseudo-differential
+ chips.
+
interrupts:
description:
When the device is using 1-wire mode, this property is used to optionally
@@ -56,6 +70,24 @@ required:
unevaluatedProperties: false
+allOf:
+ # pseudo-differential chips require common mode voltage supplies,
+ # true differential chips don't use them
+ - if:
+ properties:
+ compatible:
+ enum:
+ - adi,ad7383
+ - adi,ad7384
+ then:
+ required:
+ - aina-supply
+ - ainb-supply
+ else:
+ properties:
+ aina-supply: false
+ ainb-supply: false
+
examples:
- |
#include <dt-bindings/interrupt-controller/irq.h>
--
2.44.0
From: David Lechner <[email protected]>
This adds a new driver for the AD7380 family ADCs.
The driver currently implements basic support for the AD7380, AD7381,
2-channel differential ADCs. Support for additional single-ended,
pseudo-differential and 4-channel chips that use the same register map
as well as additional features of the chip will be added in future patches.
Co-developed-by: Stefan Popa <[email protected]>
Signed-off-by: Stefan Popa <[email protected]>
Reviewed-by: Nuno Sa <[email protected]>
Signed-off-by: David Lechner <[email protected]>
[Julien Stephan: add datasheet links of supported parts]
[Julien Stephan: fix rx/tx buffer for regmap access]
Signed-off-by: Julien Stephan <[email protected]>
---
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 16 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad7380.c | 447 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 465 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index f7c512f3bbda..2277870853c7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -435,6 +435,7 @@ S: Supported
W: https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
W: https://ez.analog.com/linux-software-drivers
F: Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
+F: drivers/iio/adc/ad7380.c
AD7877 TOUCHSCREEN DRIVER
M: Michael Hennerich <[email protected]>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 8db68b80b391..631386b037ae 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -155,6 +155,22 @@ config AD7298
To compile this driver as a module, choose M here: the
module will be called ad7298.
+config AD7380
+ tristate "Analog Devices AD7380 ADC driver"
+ depends on SPI_MASTER
+ select IIO_BUFFER
+ select IIO_TRIGGER
+ select IIO_TRIGGERED_BUFFER
+ help
+ AD7380 is a family of simultaneous sampling ADCs that share the same
+ SPI register map and have similar pinouts.
+
+ Say yes here to build support for Analog Devices AD7380 ADC and
+ similar chips.
+
+ To compile this driver as a module, choose M here: the module will be
+ called ad7380.
+
config AD7476
tristate "Analog Devices AD7476 1-channel ADCs driver and other similar devices from AD and TI"
depends on SPI
diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
index edb32ce2af02..bd3cbbb178fa 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_AD7291) += ad7291.o
obj-$(CONFIG_AD7292) += ad7292.o
obj-$(CONFIG_AD7298) += ad7298.o
obj-$(CONFIG_AD7923) += ad7923.o
+obj-$(CONFIG_AD7380) += ad7380.o
obj-$(CONFIG_AD7476) += ad7476.o
obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
new file mode 100644
index 000000000000..caf6deb3a8b1
--- /dev/null
+++ b/drivers/iio/adc/ad7380.c
@@ -0,0 +1,447 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD738x Simultaneous Sampling SAR ADCs
+ *
+ * Copyright 2017 Analog Devices Inc.
+ * Copyright 2024 BayLibre, SAS
+ *
+ * Datasheets of supported parts:
+ * ad7380/1 : https://www.analog.com/media/en/technical-documentation/data-sheets/AD7380-7381.pdf
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bitops.h>
+#include <linux/cleanup.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/slab.h>
+#include <linux/spi/spi.h>
+#include <linux/sysfs.h>
+
+#include <linux/iio/buffer.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/sysfs.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/triggered_buffer.h>
+
+/* 2.5V internal reference voltage */
+#define AD7380_INTERNAL_REF_MV 2500
+
+/* reading and writing registers is more reliable at lower than max speed */
+#define AD7380_REG_WR_SPEED_HZ 10000000
+
+#define AD7380_REG_WR BIT(15)
+#define AD7380_REG_REGADDR GENMASK(14, 12)
+#define AD7380_REG_DATA GENMASK(11, 0)
+
+#define AD7380_REG_ADDR_NOP 0x0
+#define AD7380_REG_ADDR_CONFIG1 0x1
+#define AD7380_REG_ADDR_CONFIG2 0x2
+#define AD7380_REG_ADDR_ALERT 0x3
+#define AD7380_REG_ADDR_ALERT_LOW_TH 0x4
+#define AD7380_REG_ADDR_ALERT_HIGH_TH 0x5
+
+#define AD7380_CONFIG1_OS_MODE BIT(9)
+#define AD7380_CONFIG1_OSR GENMASK(8, 6)
+#define AD7380_CONFIG1_CRC_W BIT(5)
+#define AD7380_CONFIG1_CRC_R BIT(4)
+#define AD7380_CONFIG1_ALERTEN BIT(3)
+#define AD7380_CONFIG1_RES BIT(2)
+#define AD7380_CONFIG1_REFSEL BIT(1)
+#define AD7380_CONFIG1_PMODE BIT(0)
+
+#define AD7380_CONFIG2_SDO2 GENMASK(9, 8)
+#define AD7380_CONFIG2_SDO BIT(8)
+#define AD7380_CONFIG2_RESET GENMASK(7, 0)
+
+#define AD7380_CONFIG2_RESET_SOFT 0x3C
+#define AD7380_CONFIG2_RESET_HARD 0xFF
+
+#define AD7380_ALERT_LOW_TH GENMASK(11, 0)
+#define AD7380_ALERT_HIGH_TH GENMASK(11, 0)
+
+struct ad7380_chip_info {
+ const char *name;
+ const struct iio_chan_spec *channels;
+ unsigned int num_channels;
+};
+
+#define AD7380_CHANNEL(index, bits) { \
+ .type = IIO_VOLTAGE, \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
+ .indexed = 1, \
+ .differential = 1, \
+ .channel = 2 * (index), \
+ .channel2 = 2 * (index) + 1, \
+ .scan_index = (index), \
+ .scan_type = { \
+ .sign = 's', \
+ .realbits = (bits), \
+ .storagebits = 16, \
+ .endianness = IIO_CPU, \
+ }, \
+}
+
+#define DEFINE_AD7380_2_CHANNEL(name, bits) \
+static const struct iio_chan_spec name[] = { \
+ AD7380_CHANNEL(0, bits), \
+ AD7380_CHANNEL(1, bits), \
+ IIO_CHAN_SOFT_TIMESTAMP(2), \
+}
+
+DEFINE_AD7380_2_CHANNEL(ad7380_channels, 16);
+DEFINE_AD7380_2_CHANNEL(ad7381_channels, 14);
+
+/* Since this is simultaneous sampling, we don't allow individual channels. */
+static const unsigned long ad7380_2_channel_scan_masks[] = {
+ GENMASK(1, 0),
+ 0
+};
+
+static const struct ad7380_chip_info ad7380_chip_info = {
+ .name = "ad7380",
+ .channels = ad7380_channels,
+ .num_channels = ARRAY_SIZE(ad7380_channels),
+};
+
+static const struct ad7380_chip_info ad7381_chip_info = {
+ .name = "ad7381",
+ .channels = ad7381_channels,
+ .num_channels = ARRAY_SIZE(ad7381_channels),
+};
+
+struct ad7380_state {
+ const struct ad7380_chip_info *chip_info;
+ struct spi_device *spi;
+ struct regmap *regmap;
+ unsigned int vref_mv;
+ /*
+ * DMA (thus cache coherency maintenance) requires the
+ * transfer buffers to live in their own cache lines.
+ * Make the buffer large enough for 2 16-bit samples and one 64-bit
+ * aligned 64 bit timestamp.
+ */
+ struct {
+ u16 raw[2];
+
+ s64 ts __aligned(8);
+ } scan_data __aligned(IIO_DMA_MINALIGN);
+ u16 tx;
+ u16 rx;
+};
+
+static int ad7380_regmap_reg_write(void *context, unsigned int reg,
+ unsigned int val)
+{
+ struct ad7380_state *st = context;
+ struct spi_transfer xfer = {
+ .speed_hz = AD7380_REG_WR_SPEED_HZ,
+ .bits_per_word = 16,
+ .len = 2,
+ .tx_buf = &st->tx,
+ };
+
+ st->tx = FIELD_PREP(AD7380_REG_WR, 1) |
+ FIELD_PREP(AD7380_REG_REGADDR, reg) |
+ FIELD_PREP(AD7380_REG_DATA, val);
+
+ return spi_sync_transfer(st->spi, &xfer, 1);
+}
+
+static int ad7380_regmap_reg_read(void *context, unsigned int reg,
+ unsigned int *val)
+{
+ struct ad7380_state *st = context;
+ struct spi_transfer xfers[] = {
+ {
+ .speed_hz = AD7380_REG_WR_SPEED_HZ,
+ .bits_per_word = 16,
+ .len = 2,
+ .tx_buf = &st->tx,
+ .cs_change = 1,
+ .cs_change_delay = {
+ .value = 10, /* t[CSH] */
+ .unit = SPI_DELAY_UNIT_NSECS,
+ },
+ }, {
+ .speed_hz = AD7380_REG_WR_SPEED_HZ,
+ .bits_per_word = 16,
+ .len = 2,
+ .rx_buf = &st->rx,
+ },
+ };
+ int ret;
+
+ st->tx = FIELD_PREP(AD7380_REG_WR, 0) |
+ FIELD_PREP(AD7380_REG_REGADDR, reg) |
+ FIELD_PREP(AD7380_REG_DATA, 0);
+
+ ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+ if (ret < 0)
+ return ret;
+
+ *val = FIELD_GET(AD7380_REG_DATA, st->rx);
+
+ return 0;
+}
+
+static const struct regmap_config ad7380_regmap_config = {
+ .reg_bits = 3,
+ .val_bits = 12,
+ .reg_read = ad7380_regmap_reg_read,
+ .reg_write = ad7380_regmap_reg_write,
+ .max_register = AD7380_REG_ADDR_ALERT_HIGH_TH,
+ .can_sleep = true,
+};
+
+static int ad7380_debugfs_reg_access(struct iio_dev *indio_dev, u32 reg,
+ u32 writeval, u32 *readval)
+{
+ struct ad7380_state *st = iio_priv(indio_dev);
+ int ret;
+
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ if (readval)
+ ret = regmap_read(st->regmap, reg, readval);
+ else
+ ret = regmap_write(st->regmap, reg, writeval);
+
+ iio_device_release_direct_mode(indio_dev);
+
+ return ret;
+}
+
+static irqreturn_t ad7380_trigger_handler(int irq, void *p)
+{
+ struct iio_poll_func *pf = p;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct ad7380_state *st = iio_priv(indio_dev);
+ struct spi_transfer xfer = {
+ .bits_per_word = st->chip_info->channels[0].scan_type.realbits,
+ .len = 4,
+ .rx_buf = st->scan_data.raw,
+ };
+ int ret;
+
+ ret = spi_sync_transfer(st->spi, &xfer, 1);
+ if (ret)
+ goto out;
+
+ iio_push_to_buffers_with_timestamp(indio_dev, &st->scan_data,
+ pf->timestamp);
+
+out:
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static int ad7380_read_direct(struct ad7380_state *st,
+ struct iio_chan_spec const *chan, int *val)
+{
+ struct spi_transfer xfers[] = {
+ /* toggle CS (no data xfer) to trigger a conversion */
+ {
+ .speed_hz = AD7380_REG_WR_SPEED_HZ,
+ .bits_per_word = chan->scan_type.realbits,
+ .delay = {
+ .value = 190, /* t[CONVERT] */
+ .unit = SPI_DELAY_UNIT_NSECS,
+ },
+ .cs_change = 1,
+ .cs_change_delay = {
+ .value = 10, /* t[CSH] */
+ .unit = SPI_DELAY_UNIT_NSECS,
+ },
+ },
+ /* then read both channels */
+ {
+ .speed_hz = AD7380_REG_WR_SPEED_HZ,
+ .bits_per_word = chan->scan_type.realbits,
+ .rx_buf = st->scan_data.raw,
+ .len = 4,
+ },
+ };
+ int ret;
+
+ ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+ if (ret < 0)
+ return ret;
+
+ *val = sign_extend32(st->scan_data.raw[chan->scan_index],
+ chan->scan_type.realbits - 1);
+
+ return IIO_VAL_INT;
+}
+
+static int ad7380_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val, int *val2, long info)
+{
+ struct ad7380_state *st = iio_priv(indio_dev);
+ int ret;
+
+ switch (info) {
+ case IIO_CHAN_INFO_RAW:
+ ret = iio_device_claim_direct_mode(indio_dev);
+ if (ret)
+ return ret;
+
+ ret = ad7380_read_direct(st, chan, val);
+ iio_device_release_direct_mode(indio_dev);
+
+ return ret;
+ case IIO_CHAN_INFO_SCALE:
+ *val = st->vref_mv;
+ *val2 = chan->scan_type.realbits;
+
+ return IIO_VAL_FRACTIONAL_LOG2;
+ }
+
+ return -EINVAL;
+}
+
+static const struct iio_info ad7380_info = {
+ .read_raw = &ad7380_read_raw,
+ .debugfs_reg_access = &ad7380_debugfs_reg_access,
+};
+
+static int ad7380_init(struct ad7380_state *st, struct regulator *vref)
+{
+ int ret;
+
+ /* perform hard reset */
+ ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
+ AD7380_CONFIG2_RESET,
+ FIELD_PREP(AD7380_CONFIG2_RESET,
+ AD7380_CONFIG2_RESET_HARD));
+ if (ret < 0)
+ return ret;
+
+ /* select internal or external reference voltage */
+ ret = regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG1,
+ AD7380_CONFIG1_REFSEL,
+ FIELD_PREP(AD7380_CONFIG1_REFSEL,
+ vref ? 1 : 0));
+ if (ret < 0)
+ return ret;
+
+ /* SPI 1-wire mode */
+ return regmap_update_bits(st->regmap, AD7380_REG_ADDR_CONFIG2,
+ AD7380_CONFIG2_SDO,
+ FIELD_PREP(AD7380_CONFIG2_SDO, 1));
+}
+
+static void ad7380_regulator_disable(void *p)
+{
+ regulator_disable(p);
+}
+
+static int ad7380_probe(struct spi_device *spi)
+{
+ struct iio_dev *indio_dev;
+ struct ad7380_state *st;
+ struct regulator *vref;
+ int ret;
+
+ indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
+ if (!indio_dev)
+ return -ENOMEM;
+
+ st = iio_priv(indio_dev);
+ st->spi = spi;
+ st->chip_info = spi_get_device_match_data(spi);
+ if (!st->chip_info)
+ return dev_err_probe(&spi->dev, -EINVAL, "missing match data\n");
+
+ vref = devm_regulator_get_optional(&spi->dev, "refio");
+ if (IS_ERR(vref)) {
+ if (PTR_ERR(vref) != -ENODEV)
+ return dev_err_probe(&spi->dev, PTR_ERR(vref),
+ "Failed to get refio regulator\n");
+
+ vref = NULL;
+ }
+
+ /*
+ * If there is no REFIO supply, then it means that we are using
+ * the internal 2.5V reference, otherwise REFIO is reference voltage.
+ */
+ if (vref) {
+ ret = regulator_enable(vref);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(&spi->dev,
+ ad7380_regulator_disable, vref);
+ if (ret)
+ return ret;
+
+ ret = regulator_get_voltage(vref);
+ if (ret < 0)
+ return ret;
+
+ st->vref_mv = ret / 1000;
+ } else {
+ st->vref_mv = AD7380_INTERNAL_REF_MV;
+ }
+
+ st->regmap = devm_regmap_init(&spi->dev, NULL, st, &ad7380_regmap_config);
+ if (IS_ERR(st->regmap))
+ return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
+ "failed to allocate register map\n");
+
+ indio_dev->channels = st->chip_info->channels;
+ indio_dev->num_channels = st->chip_info->num_channels;
+ indio_dev->name = st->chip_info->name;
+ indio_dev->info = &ad7380_info;
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->available_scan_masks = ad7380_2_channel_scan_masks;
+
+ ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
+ iio_pollfunc_store_time,
+ ad7380_trigger_handler, NULL);
+ if (ret)
+ return ret;
+
+ ret = ad7380_init(st, vref);
+ if (ret)
+ return ret;
+
+ return devm_iio_device_register(&spi->dev, indio_dev);
+}
+
+static const struct of_device_id ad7380_of_match_table[] = {
+ { .compatible = "adi,ad7380", .data = &ad7380_chip_info },
+ { .compatible = "adi,ad7381", .data = &ad7381_chip_info },
+ { }
+};
+
+static const struct spi_device_id ad7380_id_table[] = {
+ { "ad7380", (kernel_ulong_t)&ad7380_chip_info },
+ { "ad7381", (kernel_ulong_t)&ad7381_chip_info },
+ { }
+};
+MODULE_DEVICE_TABLE(spi, ad7380_id_table);
+
+static struct spi_driver ad7380_driver = {
+ .driver = {
+ .name = "ad7380",
+ .of_match_table = ad7380_of_match_table,
+ },
+ .probe = ad7380_probe,
+ .id_table = ad7380_id_table,
+};
+module_spi_driver(ad7380_driver);
+
+MODULE_AUTHOR("Stefan Popa <[email protected]>");
+MODULE_DESCRIPTION("Analog Devices AD738x ADC driver");
+MODULE_LICENSE("GPL");
--
2.44.0
From: David Lechner <[email protected]>
Add support for AD7383, AD7384 pseudo-differential compatible parts.
Pseudo differential parts require common mode voltage supplies so add
the support for them and add the support of IIO_CHAN_INFO_OFFSET to
retrieve the offset
Signed-off-by: David Lechner <[email protected]>
Signed-off-by: Julien Stephan <[email protected]>
---
drivers/iio/adc/ad7380.c | 98 +++++++++++++++++++++++++++++++++++++++++-------
1 file changed, 85 insertions(+), 13 deletions(-)
diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index caf6deb3a8b1..996ca83feaed 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -7,6 +7,7 @@
*
* Datasheets of supported parts:
* ad7380/1 : https://www.analog.com/media/en/technical-documentation/data-sheets/AD7380-7381.pdf
+ * ad7383/4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7383-7384.pdf
*/
#include <linux/bitfield.h>
@@ -68,16 +69,19 @@ struct ad7380_chip_info {
const char *name;
const struct iio_chan_spec *channels;
unsigned int num_channels;
+ const char * const *vcm_supplies;
+ unsigned int num_vcm_supplies;
};
-#define AD7380_CHANNEL(index, bits) { \
+#define AD7380_CHANNEL(index, bits, diff) { \
.type = IIO_VOLTAGE, \
- .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
+ ((diff) ? 0 : BIT(IIO_CHAN_INFO_OFFSET)), \
.info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \
.indexed = 1, \
- .differential = 1, \
- .channel = 2 * (index), \
- .channel2 = 2 * (index) + 1, \
+ .differential = (diff), \
+ .channel = (diff) ? (2 * (index)) : (index), \
+ .channel2 = (diff) ? (2 * (index) + 1) : 0, \
.scan_index = (index), \
.scan_type = { \
.sign = 's', \
@@ -87,15 +91,23 @@ struct ad7380_chip_info {
}, \
}
-#define DEFINE_AD7380_2_CHANNEL(name, bits) \
-static const struct iio_chan_spec name[] = { \
- AD7380_CHANNEL(0, bits), \
- AD7380_CHANNEL(1, bits), \
- IIO_CHAN_SOFT_TIMESTAMP(2), \
+#define DEFINE_AD7380_2_CHANNEL(name, bits, diff) \
+static const struct iio_chan_spec name[] = { \
+ AD7380_CHANNEL(0, bits, diff), \
+ AD7380_CHANNEL(1, bits, diff), \
+ IIO_CHAN_SOFT_TIMESTAMP(2), \
}
-DEFINE_AD7380_2_CHANNEL(ad7380_channels, 16);
-DEFINE_AD7380_2_CHANNEL(ad7381_channels, 14);
+/* fully differential */
+DEFINE_AD7380_2_CHANNEL(ad7380_channels, 16, 1);
+DEFINE_AD7380_2_CHANNEL(ad7381_channels, 14, 1);
+/* pseudo differential */
+DEFINE_AD7380_2_CHANNEL(ad7383_channels, 16, 0);
+DEFINE_AD7380_2_CHANNEL(ad7384_channels, 14, 0);
+
+static const char * const ad7380_2_channel_vcm_supplies[] = {
+ "aina", "ainb",
+};
/* Since this is simultaneous sampling, we don't allow individual channels. */
static const unsigned long ad7380_2_channel_scan_masks[] = {
@@ -115,11 +127,28 @@ static const struct ad7380_chip_info ad7381_chip_info = {
.num_channels = ARRAY_SIZE(ad7381_channels),
};
+static const struct ad7380_chip_info ad7383_chip_info = {
+ .name = "ad7383",
+ .channels = ad7383_channels,
+ .num_channels = ARRAY_SIZE(ad7383_channels),
+ .vcm_supplies = ad7380_2_channel_vcm_supplies,
+ .num_vcm_supplies = ARRAY_SIZE(ad7380_2_channel_vcm_supplies),
+};
+
+static const struct ad7380_chip_info ad7384_chip_info = {
+ .name = "ad7384",
+ .channels = ad7384_channels,
+ .num_channels = ARRAY_SIZE(ad7384_channels),
+ .vcm_supplies = ad7380_2_channel_vcm_supplies,
+ .num_vcm_supplies = ARRAY_SIZE(ad7380_2_channel_vcm_supplies),
+};
+
struct ad7380_state {
const struct ad7380_chip_info *chip_info;
struct spi_device *spi;
struct regmap *regmap;
unsigned int vref_mv;
+ unsigned int vcm_mv[2];
/*
* DMA (thus cache coherency maintenance) requires the
* transfer buffers to live in their own cache lines.
@@ -304,6 +333,11 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
*val2 = chan->scan_type.realbits;
return IIO_VAL_FRACTIONAL_LOG2;
+ case IIO_CHAN_INFO_OFFSET:
+ *val = st->vcm_mv[chan->channel] * (1 << chan->scan_type.realbits)
+ / st->vref_mv;
+
+ return IIO_VAL_INT;
}
return -EINVAL;
@@ -350,7 +384,7 @@ static int ad7380_probe(struct spi_device *spi)
struct iio_dev *indio_dev;
struct ad7380_state *st;
struct regulator *vref;
- int ret;
+ int ret, i;
indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
if (!indio_dev)
@@ -394,6 +428,40 @@ static int ad7380_probe(struct spi_device *spi)
st->vref_mv = AD7380_INTERNAL_REF_MV;
}
+ if (st->chip_info->num_vcm_supplies > ARRAY_SIZE(st->vcm_mv))
+ return dev_err_probe(&spi->dev, -EINVAL,
+ "invalid number of VCM supplies\n");
+
+ /*
+ * pseudo-differential chips have common mode supplies for the negative
+ * input pin.
+ */
+ for (i = 0; i < st->chip_info->num_vcm_supplies; i++) {
+ struct regulator *vcm;
+
+ vcm = devm_regulator_get_optional(&spi->dev,
+ st->chip_info->vcm_supplies[i]);
+ if (IS_ERR(vcm))
+ return dev_err_probe(&spi->dev, PTR_ERR(vcm),
+ "Failed to get %s regulator\n",
+ st->chip_info->vcm_supplies[i]);
+
+ ret = regulator_enable(vcm);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(&spi->dev,
+ ad7380_regulator_disable, vcm);
+ if (ret)
+ return ret;
+
+ ret = regulator_get_voltage(vcm);
+ if (ret < 0)
+ return ret;
+
+ st->vcm_mv[i] = ret / 1000;
+ }
+
st->regmap = devm_regmap_init(&spi->dev, NULL, st, &ad7380_regmap_config);
if (IS_ERR(st->regmap))
return dev_err_probe(&spi->dev, PTR_ERR(st->regmap),
@@ -422,12 +490,16 @@ static int ad7380_probe(struct spi_device *spi)
static const struct of_device_id ad7380_of_match_table[] = {
{ .compatible = "adi,ad7380", .data = &ad7380_chip_info },
{ .compatible = "adi,ad7381", .data = &ad7381_chip_info },
+ { .compatible = "adi,ad7383", .data = &ad7383_chip_info },
+ { .compatible = "adi,ad7384", .data = &ad7384_chip_info },
{ }
};
static const struct spi_device_id ad7380_id_table[] = {
{ "ad7380", (kernel_ulong_t)&ad7380_chip_info },
{ "ad7381", (kernel_ulong_t)&ad7381_chip_info },
+ { "ad7383", (kernel_ulong_t)&ad7383_chip_info },
+ { "ad7384", (kernel_ulong_t)&ad7384_chip_info },
{ }
};
MODULE_DEVICE_TABLE(spi, ad7380_id_table);
--
2.44.0
The current driver supports only parts with 2 channels.
In order to prepare the support of new compatible ADCs with more
channels, this commit:
- defines MAX_NUM_CHANNEL to specify the maximum number of
channels currently supported by the driver
- adds available_scan_mask member in ad7380_chip_info structure
- fixes spi xfer struct len depending on number of channels
- fixes scan_data.raw buffer size to handle more channels
- adds a timing specifications structure in ad7380_chip_info structure
Signed-off-by: Julien Stephan <[email protected]>
---
drivers/iio/adc/ad7380.c | 42 ++++++++++++++++++++++++++++++++----------
1 file changed, 32 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index 996ca83feaed..3aca41ce9a14 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -29,6 +29,7 @@
#include <linux/iio/trigger_consumer.h>
#include <linux/iio/triggered_buffer.h>
+#define MAX_NUM_CHANNELS 2
/* 2.5V internal reference voltage */
#define AD7380_INTERNAL_REF_MV 2500
@@ -65,12 +66,19 @@
#define AD7380_ALERT_LOW_TH GENMASK(11, 0)
#define AD7380_ALERT_HIGH_TH GENMASK(11, 0)
+#define T_CONVERT_NS 190 /* conversion time */
+struct ad7380_timing_specs {
+ const unsigned int t_csh_ns; /* CS minimum high time */
+};
+
struct ad7380_chip_info {
const char *name;
const struct iio_chan_spec *channels;
unsigned int num_channels;
const char * const *vcm_supplies;
unsigned int num_vcm_supplies;
+ const unsigned long *available_scan_masks;
+ const struct ad7380_timing_specs *timing_specs;
};
#define AD7380_CHANNEL(index, bits, diff) { \
@@ -115,16 +123,24 @@ static const unsigned long ad7380_2_channel_scan_masks[] = {
0
};
+static const struct ad7380_timing_specs ad7380_timing = {
+ .t_csh_ns = 10,
+};
+
static const struct ad7380_chip_info ad7380_chip_info = {
.name = "ad7380",
.channels = ad7380_channels,
.num_channels = ARRAY_SIZE(ad7380_channels),
+ .available_scan_masks = ad7380_2_channel_scan_masks,
+ .timing_specs = &ad7380_timing,
};
static const struct ad7380_chip_info ad7381_chip_info = {
.name = "ad7381",
.channels = ad7381_channels,
.num_channels = ARRAY_SIZE(ad7381_channels),
+ .available_scan_masks = ad7380_2_channel_scan_masks,
+ .timing_specs = &ad7380_timing,
};
static const struct ad7380_chip_info ad7383_chip_info = {
@@ -133,6 +149,8 @@ static const struct ad7380_chip_info ad7383_chip_info = {
.num_channels = ARRAY_SIZE(ad7383_channels),
.vcm_supplies = ad7380_2_channel_vcm_supplies,
.num_vcm_supplies = ARRAY_SIZE(ad7380_2_channel_vcm_supplies),
+ .available_scan_masks = ad7380_2_channel_scan_masks,
+ .timing_specs = &ad7380_timing,
};
static const struct ad7380_chip_info ad7384_chip_info = {
@@ -141,6 +159,8 @@ static const struct ad7380_chip_info ad7384_chip_info = {
.num_channels = ARRAY_SIZE(ad7384_channels),
.vcm_supplies = ad7380_2_channel_vcm_supplies,
.num_vcm_supplies = ARRAY_SIZE(ad7380_2_channel_vcm_supplies),
+ .available_scan_masks = ad7380_2_channel_scan_masks,
+ .timing_specs = &ad7380_timing,
};
struct ad7380_state {
@@ -148,15 +168,15 @@ struct ad7380_state {
struct spi_device *spi;
struct regmap *regmap;
unsigned int vref_mv;
- unsigned int vcm_mv[2];
+ unsigned int vcm_mv[MAX_NUM_CHANNELS];
/*
* DMA (thus cache coherency maintenance) requires the
* transfer buffers to live in their own cache lines.
- * Make the buffer large enough for 2 16-bit samples and one 64-bit
+ * Make the buffer large enough for MAX_NUM_CHANNELS 16-bit samples and one 64-bit
* aligned 64 bit timestamp.
*/
struct {
- u16 raw[2];
+ u16 raw[MAX_NUM_CHANNELS];
s64 ts __aligned(8);
} scan_data __aligned(IIO_DMA_MINALIGN);
@@ -194,7 +214,7 @@ static int ad7380_regmap_reg_read(void *context, unsigned int reg,
.tx_buf = &st->tx,
.cs_change = 1,
.cs_change_delay = {
- .value = 10, /* t[CSH] */
+ .value = st->chip_info->timing_specs->t_csh_ns,
.unit = SPI_DELAY_UNIT_NSECS,
},
}, {
@@ -255,7 +275,8 @@ static irqreturn_t ad7380_trigger_handler(int irq, void *p)
struct ad7380_state *st = iio_priv(indio_dev);
struct spi_transfer xfer = {
.bits_per_word = st->chip_info->channels[0].scan_type.realbits,
- .len = 4,
+ .len = (st->chip_info->num_channels - 1) *
+ ((st->chip_info->channels->scan_type.storagebits > 16) ? 4 : 2),
.rx_buf = st->scan_data.raw,
};
int ret;
@@ -282,21 +303,22 @@ static int ad7380_read_direct(struct ad7380_state *st,
.speed_hz = AD7380_REG_WR_SPEED_HZ,
.bits_per_word = chan->scan_type.realbits,
.delay = {
- .value = 190, /* t[CONVERT] */
+ .value = T_CONVERT_NS,
.unit = SPI_DELAY_UNIT_NSECS,
},
.cs_change = 1,
.cs_change_delay = {
- .value = 10, /* t[CSH] */
+ .value = st->chip_info->timing_specs->t_csh_ns,
.unit = SPI_DELAY_UNIT_NSECS,
},
},
- /* then read both channels */
+ /* then read all channels */
{
.speed_hz = AD7380_REG_WR_SPEED_HZ,
.bits_per_word = chan->scan_type.realbits,
.rx_buf = st->scan_data.raw,
- .len = 4,
+ .len = (st->chip_info->num_channels - 1) *
+ ((chan->scan_type.storagebits > 16) ? 4 : 2),
},
};
int ret;
@@ -472,7 +494,7 @@ static int ad7380_probe(struct spi_device *spi)
indio_dev->name = st->chip_info->name;
indio_dev->info = &ad7380_info;
indio_dev->modes = INDIO_DIRECT_MODE;
- indio_dev->available_scan_masks = ad7380_2_channel_scan_masks;
+ indio_dev->available_scan_masks = st->chip_info->available_scan_masks;
ret = devm_iio_triggered_buffer_setup(&spi->dev, indio_dev,
iio_pollfunc_store_time,
--
2.44.0
Add compatible support for ad7380/1/3/4-4 parts which are 4 channels
variants from ad7380/1/3/4
Signed-off-by: Julien Stephan <[email protected]>
---
.../devicetree/bindings/iio/adc/adi,ad7380.yaml | 34 ++++++++++++++++++++++
1 file changed, 34 insertions(+)
diff --git a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
index de3d28a021ae..899b777017ce 100644
--- a/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
@@ -15,6 +15,10 @@ description: |
* https://www.analog.com/en/products/ad7381.html
* https://www.analog.com/en/products/ad7383.html
* https://www.analog.com/en/products/ad7384.html
+ * https://www.analog.com/en/products/ad7380-4.html
+ * https://www.analog.com/en/products/ad7381-4.html
+ * https://www.analog.com/en/products/ad7383-4.html
+ * https://www.analog.com/en/products/ad7384-4.html
$ref: /schemas/spi/spi-peripheral-props.yaml#
@@ -25,6 +29,10 @@ properties:
- adi,ad7381
- adi,ad7383
- adi,ad7384
+ - adi,ad7380-4
+ - adi,ad7381-4
+ - adi,ad7383-4
+ - adi,ad7384-4
reg:
maxItems: 1
@@ -56,6 +64,16 @@ properties:
The common mode voltage supply for the AINB- pin on pseudo-differential
chips.
+ ainc-supply:
+ description:
+ The common mode voltage supply for the AINC- pin on pseudo-differential
+ chips.
+
+ aind-supply:
+ description:
+ The common mode voltage supply for the AIND- pin on pseudo-differential
+ chips.
+
interrupts:
description:
When the device is using 1-wire mode, this property is used to optionally
@@ -79,6 +97,8 @@ allOf:
enum:
- adi,ad7383
- adi,ad7384
+ - adi,ad7383-4
+ - adi,ad7384-4
then:
required:
- aina-supply
@@ -87,6 +107,20 @@ allOf:
properties:
aina-supply: false
ainb-supply: false
+ - if:
+ properties:
+ compatible:
+ enum:
+ - adi,ad7383-4
+ - adi,ad7384-4
+ then:
+ required:
+ - ainc-supply
+ - aind-supply
+ else:
+ properties:
+ ainc-supply: false
+ aind-supply: false
examples:
- |
--
2.44.0
Add support for ad7380/1/2/3-4 parts which are 4 channels
variants from ad7380/1/2/3
Signed-off-by: Julien Stephan <[email protected]>
---
drivers/iio/adc/ad7380.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 74 insertions(+), 1 deletion(-)
diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
index 3aca41ce9a14..cf9d2ace5f20 100644
--- a/drivers/iio/adc/ad7380.c
+++ b/drivers/iio/adc/ad7380.c
@@ -8,6 +8,9 @@
* Datasheets of supported parts:
* ad7380/1 : https://www.analog.com/media/en/technical-documentation/data-sheets/AD7380-7381.pdf
* ad7383/4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7383-7384.pdf
+ * ad7380-4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7380-4.pdf
+ * ad7381-4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7381-4.pdf
+ * ad7383/4-4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7383-4-ad7384-4.pdf
*/
#include <linux/bitfield.h>
@@ -29,7 +32,7 @@
#include <linux/iio/trigger_consumer.h>
#include <linux/iio/triggered_buffer.h>
-#define MAX_NUM_CHANNELS 2
+#define MAX_NUM_CHANNELS 4
/* 2.5V internal reference voltage */
#define AD7380_INTERNAL_REF_MV 2500
@@ -106,27 +109,53 @@ static const struct iio_chan_spec name[] = { \
IIO_CHAN_SOFT_TIMESTAMP(2), \
}
+#define DEFINE_AD7380_4_CHANNEL(name, bits, diff) \
+static const struct iio_chan_spec name[] = { \
+ AD7380_CHANNEL(0, bits, diff), \
+ AD7380_CHANNEL(1, bits, diff), \
+ AD7380_CHANNEL(2, bits, diff), \
+ AD7380_CHANNEL(3, bits, diff), \
+ IIO_CHAN_SOFT_TIMESTAMP(4), \
+}
+
/* fully differential */
DEFINE_AD7380_2_CHANNEL(ad7380_channels, 16, 1);
DEFINE_AD7380_2_CHANNEL(ad7381_channels, 14, 1);
+DEFINE_AD7380_4_CHANNEL(ad7380_4_channels, 16, 1);
+DEFINE_AD7380_4_CHANNEL(ad7381_4_channels, 14, 1);
/* pseudo differential */
DEFINE_AD7380_2_CHANNEL(ad7383_channels, 16, 0);
DEFINE_AD7380_2_CHANNEL(ad7384_channels, 14, 0);
+DEFINE_AD7380_4_CHANNEL(ad7383_4_channels, 16, 0);
+DEFINE_AD7380_4_CHANNEL(ad7384_4_channels, 14, 0);
static const char * const ad7380_2_channel_vcm_supplies[] = {
"aina", "ainb",
};
+static const char * const ad7380_4_channel_vcm_supplies[] = {
+ "aina", "ainb", "ainc", "aind",
+};
+
/* Since this is simultaneous sampling, we don't allow individual channels. */
static const unsigned long ad7380_2_channel_scan_masks[] = {
GENMASK(1, 0),
0
};
+static const unsigned long ad7380_4_channel_scan_masks[] = {
+ GENMASK(3, 0),
+ 0
+};
+
static const struct ad7380_timing_specs ad7380_timing = {
.t_csh_ns = 10,
};
+static const struct ad7380_timing_specs ad7380_4_timing = {
+ .t_csh_ns = 20,
+};
+
static const struct ad7380_chip_info ad7380_chip_info = {
.name = "ad7380",
.channels = ad7380_channels,
@@ -163,6 +192,42 @@ static const struct ad7380_chip_info ad7384_chip_info = {
.timing_specs = &ad7380_timing,
};
+static const struct ad7380_chip_info ad7380_4_chip_info = {
+ .name = "ad7380-4",
+ .channels = ad7380_4_channels,
+ .num_channels = ARRAY_SIZE(ad7380_4_channels),
+ .available_scan_masks = ad7380_4_channel_scan_masks,
+ .timing_specs = &ad7380_4_timing,
+};
+
+static const struct ad7380_chip_info ad7381_4_chip_info = {
+ .name = "ad7381-4",
+ .channels = ad7381_4_channels,
+ .num_channels = ARRAY_SIZE(ad7381_4_channels),
+ .available_scan_masks = ad7380_4_channel_scan_masks,
+ .timing_specs = &ad7380_4_timing,
+};
+
+static const struct ad7380_chip_info ad7383_4_chip_info = {
+ .name = "ad7383-4",
+ .channels = ad7383_4_channels,
+ .num_channels = ARRAY_SIZE(ad7383_4_channels),
+ .vcm_supplies = ad7380_4_channel_vcm_supplies,
+ .num_vcm_supplies = ARRAY_SIZE(ad7380_4_channel_vcm_supplies),
+ .available_scan_masks = ad7380_4_channel_scan_masks,
+ .timing_specs = &ad7380_4_timing,
+};
+
+static const struct ad7380_chip_info ad7384_4_chip_info = {
+ .name = "ad7384-4",
+ .channels = ad7384_4_channels,
+ .num_channels = ARRAY_SIZE(ad7384_4_channels),
+ .vcm_supplies = ad7380_4_channel_vcm_supplies,
+ .num_vcm_supplies = ARRAY_SIZE(ad7380_4_channel_vcm_supplies),
+ .available_scan_masks = ad7380_4_channel_scan_masks,
+ .timing_specs = &ad7380_4_timing,
+};
+
struct ad7380_state {
const struct ad7380_chip_info *chip_info;
struct spi_device *spi;
@@ -514,6 +579,10 @@ static const struct of_device_id ad7380_of_match_table[] = {
{ .compatible = "adi,ad7381", .data = &ad7381_chip_info },
{ .compatible = "adi,ad7383", .data = &ad7383_chip_info },
{ .compatible = "adi,ad7384", .data = &ad7384_chip_info },
+ { .compatible = "adi,ad7380-4", .data = &ad7380_4_chip_info },
+ { .compatible = "adi,ad7381-4", .data = &ad7381_4_chip_info },
+ { .compatible = "adi,ad7383-4", .data = &ad7383_4_chip_info },
+ { .compatible = "adi,ad7384-4", .data = &ad7384_4_chip_info },
{ }
};
@@ -522,6 +591,10 @@ static const struct spi_device_id ad7380_id_table[] = {
{ "ad7381", (kernel_ulong_t)&ad7381_chip_info },
{ "ad7383", (kernel_ulong_t)&ad7383_chip_info },
{ "ad7384", (kernel_ulong_t)&ad7384_chip_info },
+ { "ad7380-4", (kernel_ulong_t)&ad7380_4_chip_info },
+ { "ad7381-4", (kernel_ulong_t)&ad7381_4_chip_info },
+ { "ad7383-4", (kernel_ulong_t)&ad7383_4_chip_info },
+ { "ad7384-4", (kernel_ulong_t)&ad7384_4_chip_info },
{ }
};
MODULE_DEVICE_TABLE(spi, ad7380_id_table);
--
2.44.0
On Tue, Mar 19, 2024 at 11:11:24AM +0100, Julien Stephan wrote:
> From: David Lechner <[email protected]>
>
> Adding AD7383 and AD7384 compatible parts that are pseudo-differential.
>
> Pseudo-differential require common mode voltage supplies, so add them
> conditionally
>
> Signed-off-by: David Lechner <[email protected]>
> Signed-off-by: Julien Stephan <[email protected]>
Acked-by: Conor Dooley <[email protected]>
Thanks,
Conor.
On Tue, Mar 19, 2024 at 11:11:27AM +0100, Julien Stephan wrote:
> Add compatible support for ad7380/1/3/4-4 parts which are 4 channels
> variants from ad7380/1/3/4
>
> Signed-off-by: Julien Stephan <[email protected]>
Acked-by: Conor Dooley <[email protected]>
On Tue, 19 Mar 2024 11:11:23 +0100
Julien Stephan <[email protected]> wrote:
> From: David Lechner <[email protected]>
>
> This adds a new driver for the AD7380 family ADCs.
>
> The driver currently implements basic support for the AD7380, AD7381,
> 2-channel differential ADCs. Support for additional single-ended,
> pseudo-differential and 4-channel chips that use the same register map
> as well as additional features of the chip will be added in future patches.
>
> Co-developed-by: Stefan Popa <[email protected]>
> Signed-off-by: Stefan Popa <[email protected]>
> Reviewed-by: Nuno Sa <[email protected]>
> Signed-off-by: David Lechner <[email protected]>
> [Julien Stephan: add datasheet links of supported parts]
> [Julien Stephan: fix rx/tx buffer for regmap access]
> Signed-off-by: Julien Stephan <[email protected]>
Looks good to me. One unrelated comment inline.
Jonathan
> ---
> MAINTAINERS | 1 +
> drivers/iio/adc/Kconfig | 16 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ad7380.c | 447 +++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 465 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index f7c512f3bbda..2277870853c7 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -435,6 +435,7 @@ S: Supported
> W: https://wiki.analog.com/resources/tools-software/linux-drivers/iio-adc/ad738x
> W: https://ez.analog.com/linux-software-drivers
> F: Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> +F: drivers/iio/adc/ad7380.c
>
> AD7877 TOUCHSCREEN DRIVER
> M: Michael Hennerich <[email protected]>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 8db68b80b391..631386b037ae 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -155,6 +155,22 @@ config AD7298
> To compile this driver as a module, choose M here: the
> module will be called ad7298.
>
> +config AD7380
> + tristate "Analog Devices AD7380 ADC driver"
> + depends on SPI_MASTER
> + select IIO_BUFFER
> + select IIO_TRIGGER
> + select IIO_TRIGGERED_BUFFER
> + help
> + AD7380 is a family of simultaneous sampling ADCs that share the same
> + SPI register map and have similar pinouts.
> +
> + Say yes here to build support for Analog Devices AD7380 ADC and
> + similar chips.
> +
> + To compile this driver as a module, choose M here: the module will be
> + called ad7380.
> +
> config AD7476
> tristate "Analog Devices AD7476 1-channel ADCs driver and other similar devices from AD and TI"
> depends on SPI
> diff --git a/drivers/iio/adc/Makefile b/drivers/iio/adc/Makefile
> index edb32ce2af02..bd3cbbb178fa 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_AD7291) += ad7291.o
> obj-$(CONFIG_AD7292) += ad7292.o
> obj-$(CONFIG_AD7298) += ad7298.o
> obj-$(CONFIG_AD7923) += ad7923.o
Oops these clearly got out of order a long time ago.
We should fix that up but nothing to do with this series.
> +obj-$(CONFIG_AD7380) += ad7380.o
> obj-$(CONFIG_AD7476) += ad7476.o
> obj-$(CONFIG_AD7606_IFACE_PARALLEL) += ad7606_par.o
> obj-$(CONFIG_AD7606_IFACE_SPI) += ad7606_spi.o
On Tue, 19 Mar 2024 11:11:25 +0100
Julien Stephan <[email protected]> wrote:
> From: David Lechner <[email protected]>
>
> Add support for AD7383, AD7384 pseudo-differential compatible parts.
> Pseudo differential parts require common mode voltage supplies so add
> the support for them and add the support of IIO_CHAN_INFO_OFFSET to
> retrieve the offset
>
> Signed-off-by: David Lechner <[email protected]>
> Signed-off-by: Julien Stephan <[email protected]>
Hi Julien,
A few aditional comments inline. The one about
optional regulators may be something others disagree with.
Mark, perhaps you have time to comment.
Is this usage of devm_regulator_get_optional() to check a real regulator
is supplied (as we are going to get the voltage) sensible? Feels wrong
given the regulator is the exact opposite of optional.
Jonathan
> struct ad7380_state {
> const struct ad7380_chip_info *chip_info;
> struct spi_device *spi;
> struct regmap *regmap;
> unsigned int vref_mv;
> + unsigned int vcm_mv[2];
> /*
> * DMA (thus cache coherency maintenance) requires the
> * transfer buffers to live in their own cache lines.
> @@ -304,6 +333,11 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
> *val2 = chan->scan_type.realbits;
>
> return IIO_VAL_FRACTIONAL_LOG2;
> + case IIO_CHAN_INFO_OFFSET:
> + *val = st->vcm_mv[chan->channel] * (1 << chan->scan_type.realbits)
> + / st->vref_mv;
So this maths seems to be right to me, but it took me a while to figure it out.
Perhaps a comment would help along the lines of this is transforming
(raw * scale) + vcm_mv
to
(raw + vcm_mv / scale) * scale
as IIO ABI says offset is applied before scale.
> +
> + return IIO_VAL_INT;
> }
>
> return -EINVAL;
> @@ -350,7 +384,7 @@ static int ad7380_probe(struct spi_device *spi)
> struct iio_dev *indio_dev;
> struct ad7380_state *st;
> struct regulator *vref;
> - int ret;
> + int ret, i;
>
> indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> if (!indio_dev)
> @@ -394,6 +428,40 @@ static int ad7380_probe(struct spi_device *spi)
> st->vref_mv = AD7380_INTERNAL_REF_MV;
> }
>
> + if (st->chip_info->num_vcm_supplies > ARRAY_SIZE(st->vcm_mv))
> + return dev_err_probe(&spi->dev, -EINVAL,
> + "invalid number of VCM supplies\n");
> +
> + /*
> + * pseudo-differential chips have common mode supplies for the negative
> + * input pin.
> + */
> + for (i = 0; i < st->chip_info->num_vcm_supplies; i++) {
> + struct regulator *vcm;
> +
> + vcm = devm_regulator_get_optional(&spi->dev,
Why optional?
> + st->chip_info->vcm_supplies[i]);
> + if (IS_ERR(vcm))
This will fail if it's not there, so I'm guessing you are using this to avoid
getting to the regulator_get_voltage? If it's not present I'd rely on that
failing rather than the confusing handling here.
When the read of voltage wasn't in probe this would have resulted in a problem
much later than initial setup, now it is, we are just pushing it down a few lines.
Arguably we could have a devm_regulator_get_not_dummy()
that had same implementation to as get_optional() but whilst it's called that
I think it's confusing to use like this.
> + return dev_err_probe(&spi->dev, PTR_ERR(vcm),
> + "Failed to get %s regulator\n",
> + st->chip_info->vcm_supplies[i]);
> +
> + ret = regulator_enable(vcm);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(&spi->dev,
> + ad7380_regulator_disable, vcm);
> + if (ret)
> + return ret;
> +
> + ret = regulator_get_voltage(vcm);
I'd let this fail if we have a dummy regulator.
> + if (ret < 0)
> + return ret;
> +
> + st->vcm_mv[i] = ret / 1000;
> + }
> +
On Tue, 19 Mar 2024 11:11:26 +0100
Julien Stephan <[email protected]> wrote:
> The current driver supports only parts with 2 channels.
> In order to prepare the support of new compatible ADCs with more
> channels, this commit:
> - defines MAX_NUM_CHANNEL to specify the maximum number of
> channels currently supported by the driver
> - adds available_scan_mask member in ad7380_chip_info structure
> - fixes spi xfer struct len depending on number of channels
> - fixes scan_data.raw buffer size to handle more channels
> - adds a timing specifications structure in ad7380_chip_info structure
>
> Signed-off-by: Julien Stephan <[email protected]>
> struct ad7380_state {
> @@ -148,15 +168,15 @@ struct ad7380_state {
> struct spi_device *spi;
> struct regmap *regmap;
> unsigned int vref_mv;
> - unsigned int vcm_mv[2];
> + unsigned int vcm_mv[MAX_NUM_CHANNELS];
> /*
> * DMA (thus cache coherency maintenance) requires the
> * transfer buffers to live in their own cache lines.
> - * Make the buffer large enough for 2 16-bit samples and one 64-bit
> + * Make the buffer large enough for MAX_NUM_CHANNELS 16-bit samples and one 64-bit
> * aligned 64 bit timestamp.
> */
> struct {
> - u16 raw[2];
> + u16 raw[MAX_NUM_CHANNELS];
This can get problematic for drivers supporting devices with varying numbers of channels.
You are fine up to 4 channels as the structure layout isn't changing. The reason is that
it implies the timestamp location, which if you were for instance to support 8 channels
would be incorrect if only 4 of them were enabled.
Now the timestamp location is only used implicitly in the driver (as the IIO core
handles the actual insertion of the data) so it's not a bug as such to do this, but it
can give people the wrong impression during testing etc.
As such, I'd like to see the comment mention that "as MAX_NUM_CHANNELS is 4
the layout of the structure is the same for all parts".
Note I've reviewed one driver today that did have different layouts depending on
which device was in use and ended up falsely indicating the timestamp was later
than it actually was in the buffer. Hence my request for a defensive comment!
>
> s64 ts __aligned(8);
> } scan_data __aligned(IIO_DMA_MINALIGN);
On Tue, 19 Mar 2024 11:11:28 +0100
Julien Stephan <[email protected]> wrote:
> Add support for ad7380/1/2/3-4 parts which are 4 channels
> variants from ad7380/1/2/3
>
> Signed-off-by: Julien Stephan <[email protected]>
This and other patches I didn't comment on all look good to me.
So just those minor few bits and bobs for v6 and I'll pick this up
if nothing else comes in.
Thanks,
Jonathan
> ---
> drivers/iio/adc/ad7380.c | 75 +++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iio/adc/ad7380.c b/drivers/iio/adc/ad7380.c
> index 3aca41ce9a14..cf9d2ace5f20 100644
> --- a/drivers/iio/adc/ad7380.c
> +++ b/drivers/iio/adc/ad7380.c
> @@ -8,6 +8,9 @@
> * Datasheets of supported parts:
> * ad7380/1 : https://www.analog.com/media/en/technical-documentation/data-sheets/AD7380-7381.pdf
> * ad7383/4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7383-7384.pdf
> + * ad7380-4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7380-4.pdf
> + * ad7381-4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7381-4.pdf
> + * ad7383/4-4 : https://www.analog.com/media/en/technical-documentation/data-sheets/ad7383-4-ad7384-4.pdf
> */
>
> #include <linux/bitfield.h>
> @@ -29,7 +32,7 @@
> #include <linux/iio/trigger_consumer.h>
> #include <linux/iio/triggered_buffer.h>
>
> -#define MAX_NUM_CHANNELS 2
> +#define MAX_NUM_CHANNELS 4
> /* 2.5V internal reference voltage */
> #define AD7380_INTERNAL_REF_MV 2500
>
> @@ -106,27 +109,53 @@ static const struct iio_chan_spec name[] = { \
> IIO_CHAN_SOFT_TIMESTAMP(2), \
> }
>
> +#define DEFINE_AD7380_4_CHANNEL(name, bits, diff) \
> +static const struct iio_chan_spec name[] = { \
> + AD7380_CHANNEL(0, bits, diff), \
> + AD7380_CHANNEL(1, bits, diff), \
> + AD7380_CHANNEL(2, bits, diff), \
> + AD7380_CHANNEL(3, bits, diff), \
> + IIO_CHAN_SOFT_TIMESTAMP(4), \
> +}
> +
> /* fully differential */
> DEFINE_AD7380_2_CHANNEL(ad7380_channels, 16, 1);
> DEFINE_AD7380_2_CHANNEL(ad7381_channels, 14, 1);
> +DEFINE_AD7380_4_CHANNEL(ad7380_4_channels, 16, 1);
> +DEFINE_AD7380_4_CHANNEL(ad7381_4_channels, 14, 1);
> /* pseudo differential */
> DEFINE_AD7380_2_CHANNEL(ad7383_channels, 16, 0);
> DEFINE_AD7380_2_CHANNEL(ad7384_channels, 14, 0);
> +DEFINE_AD7380_4_CHANNEL(ad7383_4_channels, 16, 0);
> +DEFINE_AD7380_4_CHANNEL(ad7384_4_channels, 14, 0);
>
> static const char * const ad7380_2_channel_vcm_supplies[] = {
> "aina", "ainb",
> };
>
> +static const char * const ad7380_4_channel_vcm_supplies[] = {
> + "aina", "ainb", "ainc", "aind",
> +};
> +
> /* Since this is simultaneous sampling, we don't allow individual channels. */
> static const unsigned long ad7380_2_channel_scan_masks[] = {
> GENMASK(1, 0),
> 0
> };
>
> +static const unsigned long ad7380_4_channel_scan_masks[] = {
> + GENMASK(3, 0),
> + 0
> +};
> +
> static const struct ad7380_timing_specs ad7380_timing = {
> .t_csh_ns = 10,
> };
>
> +static const struct ad7380_timing_specs ad7380_4_timing = {
> + .t_csh_ns = 20,
> +};
> +
> static const struct ad7380_chip_info ad7380_chip_info = {
> .name = "ad7380",
> .channels = ad7380_channels,
> @@ -163,6 +192,42 @@ static const struct ad7380_chip_info ad7384_chip_info = {
> .timing_specs = &ad7380_timing,
> };
>
> +static const struct ad7380_chip_info ad7380_4_chip_info = {
> + .name = "ad7380-4",
> + .channels = ad7380_4_channels,
> + .num_channels = ARRAY_SIZE(ad7380_4_channels),
> + .available_scan_masks = ad7380_4_channel_scan_masks,
> + .timing_specs = &ad7380_4_timing,
> +};
> +
> +static const struct ad7380_chip_info ad7381_4_chip_info = {
> + .name = "ad7381-4",
> + .channels = ad7381_4_channels,
> + .num_channels = ARRAY_SIZE(ad7381_4_channels),
> + .available_scan_masks = ad7380_4_channel_scan_masks,
> + .timing_specs = &ad7380_4_timing,
> +};
> +
> +static const struct ad7380_chip_info ad7383_4_chip_info = {
> + .name = "ad7383-4",
> + .channels = ad7383_4_channels,
> + .num_channels = ARRAY_SIZE(ad7383_4_channels),
> + .vcm_supplies = ad7380_4_channel_vcm_supplies,
> + .num_vcm_supplies = ARRAY_SIZE(ad7380_4_channel_vcm_supplies),
> + .available_scan_masks = ad7380_4_channel_scan_masks,
> + .timing_specs = &ad7380_4_timing,
> +};
> +
> +static const struct ad7380_chip_info ad7384_4_chip_info = {
> + .name = "ad7384-4",
> + .channels = ad7384_4_channels,
> + .num_channels = ARRAY_SIZE(ad7384_4_channels),
> + .vcm_supplies = ad7380_4_channel_vcm_supplies,
> + .num_vcm_supplies = ARRAY_SIZE(ad7380_4_channel_vcm_supplies),
> + .available_scan_masks = ad7380_4_channel_scan_masks,
> + .timing_specs = &ad7380_4_timing,
> +};
> +
> struct ad7380_state {
> const struct ad7380_chip_info *chip_info;
> struct spi_device *spi;
> @@ -514,6 +579,10 @@ static const struct of_device_id ad7380_of_match_table[] = {
> { .compatible = "adi,ad7381", .data = &ad7381_chip_info },
> { .compatible = "adi,ad7383", .data = &ad7383_chip_info },
> { .compatible = "adi,ad7384", .data = &ad7384_chip_info },
> + { .compatible = "adi,ad7380-4", .data = &ad7380_4_chip_info },
> + { .compatible = "adi,ad7381-4", .data = &ad7381_4_chip_info },
> + { .compatible = "adi,ad7383-4", .data = &ad7383_4_chip_info },
> + { .compatible = "adi,ad7384-4", .data = &ad7384_4_chip_info },
> { }
> };
>
> @@ -522,6 +591,10 @@ static const struct spi_device_id ad7380_id_table[] = {
> { "ad7381", (kernel_ulong_t)&ad7381_chip_info },
> { "ad7383", (kernel_ulong_t)&ad7383_chip_info },
> { "ad7384", (kernel_ulong_t)&ad7384_chip_info },
> + { "ad7380-4", (kernel_ulong_t)&ad7380_4_chip_info },
> + { "ad7381-4", (kernel_ulong_t)&ad7381_4_chip_info },
> + { "ad7383-4", (kernel_ulong_t)&ad7383_4_chip_info },
> + { "ad7384-4", (kernel_ulong_t)&ad7384_4_chip_info },
> { }
> };
> MODULE_DEVICE_TABLE(spi, ad7380_id_table);
>
On Sun, Mar 24, 2024 at 8:01 AM Jonathan Cameron <[email protected]> wrote:
>
> On Tue, 19 Mar 2024 11:11:25 +0100
> Julien Stephan <[email protected]> wrote:
>
> > From: David Lechner <[email protected]>
> >
> > Add support for AD7383, AD7384 pseudo-differential compatible parts.
> > Pseudo differential parts require common mode voltage supplies so add
> > the support for them and add the support of IIO_CHAN_INFO_OFFSET to
> > retrieve the offset
> >
> > Signed-off-by: David Lechner <[email protected]>
> > Signed-off-by: Julien Stephan <[email protected]>
>
> Hi Julien,
>
> A few aditional comments inline. The one about
> optional regulators may be something others disagree with.
> Mark, perhaps you have time to comment.
> Is this usage of devm_regulator_get_optional() to check a real regulator
> is supplied (as we are going to get the voltage) sensible? Feels wrong
> given the regulator is the exact opposite of optional.
>
> Jonathan
>
> > struct ad7380_state {
> > const struct ad7380_chip_info *chip_info;
> > struct spi_device *spi;
> > struct regmap *regmap;
> > unsigned int vref_mv;
> > + unsigned int vcm_mv[2];
> > /*
> > * DMA (thus cache coherency maintenance) requires the
> > * transfer buffers to live in their own cache lines.
> > @@ -304,6 +333,11 @@ static int ad7380_read_raw(struct iio_dev *indio_dev,
> > *val2 = chan->scan_type.realbits;
> >
> > return IIO_VAL_FRACTIONAL_LOG2;
> > + case IIO_CHAN_INFO_OFFSET:
> > + *val = st->vcm_mv[chan->channel] * (1 << chan->scan_type.realbits)
> > + / st->vref_mv;
>
> So this maths seems to be right to me, but it took me a while to figure it out.
> Perhaps a comment would help along the lines of this is transforming
>
> (raw * scale) + vcm_mv
> to
> (raw + vcm_mv / scale) * scale
> as IIO ABI says offset is applied before scale.
>
> > +
> > + return IIO_VAL_INT;
> > }
> >
> > return -EINVAL;
> > @@ -350,7 +384,7 @@ static int ad7380_probe(struct spi_device *spi)
> > struct iio_dev *indio_dev;
> > struct ad7380_state *st;
> > struct regulator *vref;
> > - int ret;
> > + int ret, i;
> >
> > indio_dev = devm_iio_device_alloc(&spi->dev, sizeof(*st));
> > if (!indio_dev)
> > @@ -394,6 +428,40 @@ static int ad7380_probe(struct spi_device *spi)
> > st->vref_mv = AD7380_INTERNAL_REF_MV;
> > }
> >
> > + if (st->chip_info->num_vcm_supplies > ARRAY_SIZE(st->vcm_mv))
> > + return dev_err_probe(&spi->dev, -EINVAL,
> > + "invalid number of VCM supplies\n");
> > +
> > + /*
> > + * pseudo-differential chips have common mode supplies for the negative
> > + * input pin.
> > + */
> > + for (i = 0; i < st->chip_info->num_vcm_supplies; i++) {
> > + struct regulator *vcm;
> > +
> > + vcm = devm_regulator_get_optional(&spi->dev,
>
> Why optional?
>
> > + st->chip_info->vcm_supplies[i]);
> > + if (IS_ERR(vcm))
>
> This will fail if it's not there, so I'm guessing you are using this to avoid
> getting to the regulator_get_voltage? If it's not present I'd rely on that
> failing rather than the confusing handling here.
>
> When the read of voltage wasn't in probe this would have resulted in a problem
> much later than initial setup, now it is, we are just pushing it down a few lines.
>
> Arguably we could have a devm_regulator_get_not_dummy()
> that had same implementation to as get_optional() but whilst it's called that
> I think it's confusing to use like this.
Despite the misleading naming, I guess I am used to
devm_regulator_get_optional() by now having used it enough times.
Since it fails either way though, technically both ways seem fine so I
can't really argue for one over the other.
But given that this is a common pattern in many IIO drivers, maybe we
make a devm_regulator_get_enable_get_voltage()? This would return the
voltage on success or an error code. (If the regulator subsystem
doesn't want this maybe we could have
devm_iio_regulator_get_enable_get_voltage()).
If the dev_err_probe() calls were included in
devm_regulator_get_enable_get_voltage(), then the 10+ lines of code
here and in many other drivers to get the regulator, enable it, add
the reset action and get the voltage could be reduced to 3 lines.
>
> > + return dev_err_probe(&spi->dev, PTR_ERR(vcm),
> > + "Failed to get %s regulator\n",
> > + st->chip_info->vcm_supplies[i]);
> > +
> > + ret = regulator_enable(vcm);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(&spi->dev,
> > + ad7380_regulator_disable, vcm);
> > + if (ret)
> > + return ret;
> > +
> > + ret = regulator_get_voltage(vcm);
>
> I'd let this fail if we have a dummy regulator.
>
> > + if (ret < 0)
> > + return ret;
> > +
> > + st->vcm_mv[i] = ret / 1000;
> > + }
> > +
On Sun, Mar 24, 2024 at 8:11 AM Jonathan Cameron <[email protected]> wrote:
>
> On Tue, 19 Mar 2024 11:11:28 +0100
> Julien Stephan <[email protected]> wrote:
>
> > Add support for ad7380/1/2/3-4 parts which are 4 channels
> > variants from ad7380/1/2/3
> >
> > Signed-off-by: Julien Stephan <[email protected]>
> This and other patches I didn't comment on all look good to me.
> So just those minor few bits and bobs for v6 and I'll pick this up
> if nothing else comes in.
>
Hi Jonathan, as a reminder, this is the driver we dropped from the 6.9
cycle. We still don't have a patch prepared for the resolution boost
feature that may require us to reconsider some of our userspace
interface choices here. Hopefully we can get that sorted out in the
next 6 weeks, but I just wanted to make you aware ahead of time so
that we don't end up in the same situation in case things don't go as
planned again. Do you have "usual" way you prefer to handle a
situation like this?
On Mon, 25 Mar 2024 10:01:29 -0500
David Lechner <[email protected]> wrote:
> On Sun, Mar 24, 2024 at 8:11 AM Jonathan Cameron <[email protected]> wrote:
> >
> > On Tue, 19 Mar 2024 11:11:28 +0100
> > Julien Stephan <[email protected]> wrote:
> >
> > > Add support for ad7380/1/2/3-4 parts which are 4 channels
> > > variants from ad7380/1/2/3
> > >
> > > Signed-off-by: Julien Stephan <[email protected]>
> > This and other patches I didn't comment on all look good to me.
> > So just those minor few bits and bobs for v6 and I'll pick this up
> > if nothing else comes in.
> >
>
> Hi Jonathan, as a reminder, this is the driver we dropped from the 6.9
> cycle. We still don't have a patch prepared for the resolution boost
> feature that may require us to reconsider some of our userspace
> interface choices here. Hopefully we can get that sorted out in the
> next 6 weeks, but I just wanted to make you aware ahead of time so
> that we don't end up in the same situation in case things don't go as
> planned again. Do you have "usual" way you prefer to handle a
> situation like this?
My preferences:
Post as an RFC with a comment on what is unresolved.
I'll still review the RFC but won't apply until you let me know it's
good to go (ideally by posting a non RFC version)
Jonathan
> > > + /*
> > > + * pseudo-differential chips have common mode supplies for the negative
> > > + * input pin.
> > > + */
> > > + for (i = 0; i < st->chip_info->num_vcm_supplies; i++) {
> > > + struct regulator *vcm;
> > > +
> > > + vcm = devm_regulator_get_optional(&spi->dev,
> >
> > Why optional?
> >
> > > + st->chip_info->vcm_supplies[i]);
> > > + if (IS_ERR(vcm))
> >
> > This will fail if it's not there, so I'm guessing you are using this to avoid
> > getting to the regulator_get_voltage? If it's not present I'd rely on that
> > failing rather than the confusing handling here.
> >
> > When the read of voltage wasn't in probe this would have resulted in a problem
> > much later than initial setup, now it is, we are just pushing it down a few lines.
> >
> > Arguably we could have a devm_regulator_get_not_dummy()
> > that had same implementation to as get_optional() but whilst it's called that
> > I think it's confusing to use like this.
>
> Despite the misleading naming, I guess I am used to
> devm_regulator_get_optional() by now having used it enough times.
> Since it fails either way though, technically both ways seem fine so I
> can't really argue for one over the other.
>
> But given that this is a common pattern in many IIO drivers, maybe we
> make a devm_regulator_get_enable_get_voltage()? This would return the
> voltage on success or an error code. (If the regulator subsystem
> doesn't want this maybe we could have
> devm_iio_regulator_get_enable_get_voltage()).
>
> If the dev_err_probe() calls were included in
> devm_regulator_get_enable_get_voltage(), then the 10+ lines of code
> here and in many other drivers to get the regulator, enable it, add
> the reset action and get the voltage could be reduced to 3 lines.
I like this proposal a lot. RFC, so it's visible outside the depths
of this thread?
Particularly good as it will keep the regulator opaque in the same
fashion as devm_regulator_get_enabled()
As you say, we have a 'lot' of instances of this (quick grep
suggests > 50 in IIO alone and smaller numbers elsewhere).
Jonathan
>
> >
> > > + return dev_err_probe(&spi->dev, PTR_ERR(vcm),
> > > + "Failed to get %s regulator\n",
> > > + st->chip_info->vcm_supplies[i]);
> > > +
> > > + ret = regulator_enable(vcm);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = devm_add_action_or_reset(&spi->dev,
> > > + ad7380_regulator_disable, vcm);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = regulator_get_voltage(vcm);
> >
> > I'd let this fail if we have a dummy regulator.
> >
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + st->vcm_mv[i] = ret / 1000;
> > > + }
> > > +
>
On Mon, Mar 25, 2024 at 3:06 PM Jonathan Cameron <[email protected]> wrote:
>
> >
> > But given that this is a common pattern in many IIO drivers, maybe we
> > make a devm_regulator_get_enable_get_voltage()? This would return the
> > voltage on success or an error code. (If the regulator subsystem
> > doesn't want this maybe we could have
> > devm_iio_regulator_get_enable_get_voltage()).
> >
> > If the dev_err_probe() calls were included in
> > devm_regulator_get_enable_get_voltage(), then the 10+ lines of code
> > here and in many other drivers to get the regulator, enable it, add
> > the reset action and get the voltage could be reduced to 3 lines.
>
> I like this proposal a lot. RFC, so it's visible outside the depths
> of this thread?
Yes, I can send an RFC separately so it doesn't hold up this patch/series.
> Particularly good as it will keep the regulator opaque in the same
> fashion as devm_regulator_get_enabled()
>
> As you say, we have a 'lot' of instances of this (quick grep
> suggests > 50 in IIO alone and smaller numbers elsewhere).
>