* [PATCH v2 0/3] iio: adc: add new ad7380 driver
@ 2023-12-13 11:21 David Lechner
2023-12-13 11:21 ` [PATCH v2 1/3] dt-bindings: spi: add spi-rx-bus-channels peripheral property David Lechner
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: David Lechner @ 2023-12-13 11:21 UTC (permalink / raw
To: linux-iio, devicetree
Cc: David Lechner, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Michael Hennerich, Nuno Sá, Liam Girdwood,
Mark Brown, linux-kernel
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.
One quirk of these chips is that since they are simultaneous sampling,
they have multiple SPI data output lines that allow transferring two
data words (one for each input channel) at the same time. So a new
generic devicetree binding is added to describe this sort of SPI bus
configuration.
To keep things simple, the initial driver implementation only supports
the 2-channel differential chips listed above. There are also 4-channel
differential chips and 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.
Also, FYI, this driver will also be used as the base for an upcoming
series adding AXI SPI Engine offload support for these chips along with
[1].
This work is being done by BayLibre and on behalf of Analog Devices Inc.
hence the maintainers are @analog.com.
[1]: https://lore.kernel.org/linux-spi/20231204-axi-spi-engine-series-2-v1-0-063672323fce@baylibre.com/
---
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
- Added available_scan_masks
- Added check for missing driver match data
- Link to v1: https://lore.kernel.org/r/20231208-ad7380-mainline-v1-0-2b33fe2f44ae@baylibre.com
---
David Lechner (3):
dt-bindings: spi: add spi-rx-bus-channels peripheral property
dt-bindings: iio: adc: Add binding for AD7380 ADCs
iio: adc: ad7380: new driver for AD7380 ADCs
.../devicetree/bindings/iio/adc/adi,ad7380.yaml | 107 +++++
.../bindings/spi/spi-peripheral-props.yaml | 12 +
MAINTAINERS | 10 +
drivers/iio/adc/Kconfig | 16 +
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad7380.c | 464 +++++++++++++++++++++
6 files changed, 610 insertions(+)
---
base-commit: 18f78b5e609b19b56237f0dae47068d44b8b0ecd
change-id: 20231208-ad7380-mainline-e6c4fa7dbedd
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 1/3] dt-bindings: spi: add spi-rx-bus-channels peripheral property
2023-12-13 11:21 [PATCH v2 0/3] iio: adc: add new ad7380 driver David Lechner
@ 2023-12-13 11:21 ` David Lechner
2023-12-13 11:21 ` [PATCH v2 2/3] dt-bindings: iio: adc: Add binding for AD7380 ADCs David Lechner
2023-12-13 11:21 ` [PATCH v2 3/3] iio: adc: ad7380: new driver " David Lechner
2 siblings, 0 replies; 14+ messages in thread
From: David Lechner @ 2023-12-13 11:21 UTC (permalink / raw
To: linux-iio, devicetree
Cc: David Lechner, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Michael Hennerich, Nuno Sá, Liam Girdwood,
Mark Brown, linux-kernel
This adds a new spi-rx-bus-channels property to the generic spi
peripheral property bindings. This property is used to describe
devices that have parallel data output channels.
This property is different from spi-rx-bus-width in that the latter
means that we are reading multiple bits of a single word at one time
while the former means that we are reading single bits of multiple words
at the same time.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v2 changes: new patch in v2
.../devicetree/bindings/spi/spi-peripheral-props.yaml | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
index 15938f81fdce..1c8e71c18234 100644
--- a/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml
@@ -67,6 +67,18 @@ properties:
enum: [0, 1, 2, 4, 8]
default: 1
+ spi-rx-bus-channels:
+ description:
+ The number of parallel channels for read transfers. The difference between
+ this and spi-rx-bus-width is that a value N for spi-rx-bus-channels means
+ the SPI bus is receiving one bit each of N different words at the same
+ time whereas a value M for spi-rx-bus-width means that the bus is
+ receiving M bits of a single word at the same time. It is also possible to
+ use both properties at the same time, meaning the bus is receiving M bits
+ of N different words at the same time.
+ $ref: /schemas/types.yaml#/definitions/uint32
+ default: 1
+
spi-rx-delay-us:
description:
Delay, in microseconds, after a read transfer.
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 2/3] dt-bindings: iio: adc: Add binding for AD7380 ADCs
2023-12-13 11:21 [PATCH v2 0/3] iio: adc: add new ad7380 driver David Lechner
2023-12-13 11:21 ` [PATCH v2 1/3] dt-bindings: spi: add spi-rx-bus-channels peripheral property David Lechner
@ 2023-12-13 11:21 ` David Lechner
2023-12-13 16:10 ` Conor Dooley
2023-12-13 16:15 ` Conor Dooley
2023-12-13 11:21 ` [PATCH v2 3/3] iio: adc: ad7380: new driver " David Lechner
2 siblings, 2 replies; 14+ messages in thread
From: David Lechner @ 2023-12-13 11:21 UTC (permalink / raw
To: linux-iio, devicetree
Cc: David Lechner, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Michael Hennerich, Nuno Sá, Liam Girdwood,
Mark Brown, linux-kernel
This adds a binding specification for the Analog Devices Inc. AD7380
family of ADCs.
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v2 changes:
- 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
.../devicetree/bindings/iio/adc/adi,ad7380.yaml | 107 +++++++++++++++++++++
MAINTAINERS | 9 ++
2 files changed, 116 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..43d58c52f7dd
--- /dev/null
+++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
@@ -0,0 +1,107 @@
+# 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 <Michael.Hennerich@analog.com>
+ - Nuno Sá <nuno.sa@analog.com>
+
+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#
+
+properties:
+ compatible:
+ enum:
+ - adi,ad7380
+ - adi,ad7381
+ - adi,ad7383
+ - adi,ad7384
+
+ reg:
+ maxItems: 1
+
+ spi-max-frequency:
+ maximum: 80000000
+ spi-cpol: true
+ spi-cpha: true
+
+ spi-rx-bus-channels:
+ description:
+ In 1-wire mode, the SDOA pin acts as the sole data line and the SDOB/ALERT
+ pin acts as the ALERT interrupt signal. In 2-wire mode, data for input A
+ is read from SDOA and data for input B is read from SDOB/ALERT (and the
+ ALERT interrupt signal is not available).
+ enum: [1, 2]
+
+ 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
+
+allOf:
+ - if:
+ required:
+ - spi-rx-bus-channels
+ then:
+ if:
+ properties:
+ spi-rx-bus-channels:
+ const: 2
+ then:
+ properties:
+ interrupts: false
+
+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 fe1f6f97f96a..e2a998be5879 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -430,6 +430,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 <michael.hennerich@analog.com>
+M: Nuno Sá <nuno.sa@analog.com>
+R: David Lechner <dlechner@baylibre.com>
+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 <michael.hennerich@analog.com>
S: Supported
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH v2 3/3] iio: adc: ad7380: new driver for AD7380 ADCs
2023-12-13 11:21 [PATCH v2 0/3] iio: adc: add new ad7380 driver David Lechner
2023-12-13 11:21 ` [PATCH v2 1/3] dt-bindings: spi: add spi-rx-bus-channels peripheral property David Lechner
2023-12-13 11:21 ` [PATCH v2 2/3] dt-bindings: iio: adc: Add binding for AD7380 ADCs David Lechner
@ 2023-12-13 11:21 ` David Lechner
2023-12-13 12:18 ` Nuno Sá
2023-12-14 10:14 ` Jonathan Cameron
2 siblings, 2 replies; 14+ messages in thread
From: David Lechner @ 2023-12-13 11:21 UTC (permalink / raw
To: linux-iio, devicetree
Cc: David Lechner, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Jonathan Cameron, Michael Hennerich, Nuno Sá, Liam Girdwood,
Mark Brown, linux-kernel, Stefan Popa
This adds a new driver for the AD7380 family ADCs.
The driver currently implements basic support for the AD7380, AD7381,
AD7383, and AD7384 2-channel differential ADCs. Support for additional
single-ended 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 <stefan.popa@analog.com>
Signed-off-by: Stefan Popa <stefan.popa@analog.com>
Signed-off-by: David Lechner <dlechner@baylibre.com>
---
v2 changes:
- 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 (we always sample two channels at the same time
so we need to let userspace know this)
- Added check for missing driver match data
MAINTAINERS | 1 +
drivers/iio/adc/Kconfig | 16 ++
drivers/iio/adc/Makefile | 1 +
drivers/iio/adc/ad7380.c | 464 +++++++++++++++++++++++++++++++++++++++++++++++
4 files changed, 482 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index e2a998be5879..5a54620a31b8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -438,6 +438,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 <michael.hennerich@analog.com>
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 35f9867da12c..cbfd626712e3 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -122,6 +122,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 bee11d442af4..9c921c497655 100644
--- a/drivers/iio/adc/Makefile
+++ b/drivers/iio/adc/Makefile
@@ -16,6 +16,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..b8025b636b67
--- /dev/null
+++ b/drivers/iio/adc/ad7380.c
@@ -0,0 +1,464 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Analog Devices AD738x Simultaneous Sampling SAR ADCs
+ *
+ * Copyright 2017 Analog Devices Inc.
+ * Copyright 2023 BayLibre, SAS
+ */
+
+#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_DIFFERENTIAL_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_DIFFERENTIAL_2_CHANNEL(name, bits) \
+static const struct iio_chan_spec name[] = { \
+ AD7380_DIFFERENTIAL_CHANNEL(0, bits), \
+ AD7380_DIFFERENTIAL_CHANNEL(1, bits), \
+ IIO_CHAN_SOFT_TIMESTAMP(2), \
+}
+
+/* fully differential */
+DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7380_channels, 16);
+DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7381_channels, 14);
+/* pseudo differential */
+DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7383_channels, 16);
+DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7384_channels, 14);
+
+/* Since this is simultaneous sampling, we don't allow individual channels. */
+static const unsigned long ad7380_2_channel_scan_masks[] = {
+ GENMASK(2, 0), /* both ADC channels and soft timestamp */
+ GENMASK(1, 0), /* both ADC channels, no timestamp */
+ 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),
+};
+
+static const struct ad7380_chip_info ad7383_chip_info = {
+ .name = "ad7383",
+ .channels = ad7383_channels,
+ .num_channels = ARRAY_SIZE(ad7383_channels),
+};
+
+static const struct ad7380_chip_info ad7384_chip_info = {
+ .name = "ad7384",
+ .channels = ad7384_channels,
+ .num_channels = ARRAY_SIZE(ad7384_channels),
+};
+
+struct ad7380_state {
+ const struct ad7380_chip_info *chip_info;
+ struct spi_device *spi;
+ struct regulator *vref;
+ struct regmap *regmap;
+ /*
+ * 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[2];
+ u16 rx[2];
+};
+
+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[0],
+ };
+
+ st->tx[0] = 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[0],
+ .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[0],
+ },
+ };
+ int ret;
+
+ st->tx[0] = 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[0]);
+
+ return 0;
+}
+
+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->rx[0],
+ .len = 4,
+ },
+ };
+ int ret;
+
+ ret = spi_sync_transfer(st->spi, xfers, ARRAY_SIZE(xfers));
+ if (ret < 0)
+ return ret;
+
+ *val = sign_extend32(st->rx[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:
+ if (st->vref) {
+ ret = regulator_get_voltage(st->vref);
+ if (ret < 0)
+ return ret;
+
+ *val = ret / 1000;
+ } else {
+ *val = AD7380_INTERNAL_REF_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)
+{
+ 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, !!st->vref));
+ 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;
+ 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");
+
+ st->vref = devm_regulator_get_optional(&spi->dev, "refio");
+ if (IS_ERR(st->vref)) {
+ /*
+ * If there is no REFIO supply, then it means that we are using
+ * the internal 2.5V reference.
+ */
+ if (PTR_ERR(st->vref) == -ENODEV)
+ st->vref = NULL;
+ else
+ return dev_err_probe(&spi->dev, PTR_ERR(st->vref),
+ "Failed to get refio regulator\n");
+ }
+
+ if (st->vref) {
+ ret = regulator_enable(st->vref);
+ if (ret)
+ return ret;
+
+ ret = devm_add_action_or_reset(&spi->dev, ad7380_regulator_disable,
+ st->vref);
+ if (ret)
+ return ret;
+ }
+
+ 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->dev.parent = &spi->dev;
+ 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);
+ 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 },
+ { .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);
+
+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 <stefan.popa@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD738x ADC driver");
+MODULE_LICENSE("GPL");
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] iio: adc: ad7380: new driver for AD7380 ADCs
2023-12-13 11:21 ` [PATCH v2 3/3] iio: adc: ad7380: new driver " David Lechner
@ 2023-12-13 12:18 ` Nuno Sá
2023-12-13 13:47 ` David Lechner
2023-12-14 10:14 ` Jonathan Cameron
1 sibling, 1 reply; 14+ messages in thread
From: Nuno Sá @ 2023-12-13 12:18 UTC (permalink / raw
To: David Lechner, linux-iio, devicetree
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Jonathan Cameron,
Michael Hennerich, Nuno Sá, Liam Girdwood, Mark Brown,
linux-kernel, Stefan Popa
On Wed, 2023-12-13 at 05:21 -0600, David Lechner wrote:
> This adds a new driver for the AD7380 family ADCs.
>
> The driver currently implements basic support for the AD7380, AD7381,
> AD7383, and AD7384 2-channel differential ADCs. Support for additional
> single-ended 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 <stefan.popa@analog.com>
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>
> v2 changes:
> - 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 (we always sample two channels at the same time
> so we need to let userspace know this)
> - Added check for missing driver match data
>
> MAINTAINERS | 1 +
> drivers/iio/adc/Kconfig | 16 ++
> drivers/iio/adc/Makefile | 1 +
> drivers/iio/adc/ad7380.c | 464 +++++++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 482 insertions(+)
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e2a998be5879..5a54620a31b8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -438,6 +438,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 <michael.hennerich@analog.com>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 35f9867da12c..cbfd626712e3 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -122,6 +122,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 bee11d442af4..9c921c497655 100644
> --- a/drivers/iio/adc/Makefile
> +++ b/drivers/iio/adc/Makefile
> @@ -16,6 +16,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..b8025b636b67
> --- /dev/null
> +++ b/drivers/iio/adc/ad7380.c
> @@ -0,0 +1,464 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Analog Devices AD738x Simultaneous Sampling SAR ADCs
> + *
> + * Copyright 2017 Analog Devices Inc.
> + * Copyright 2023 BayLibre, SAS
> + */
> +
> +#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>
> +
...
>
> +static int ad7380_probe(struct spi_device *spi)
> +{
> + struct iio_dev *indio_dev;
> + struct ad7380_state *st;
> + 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");
> +
> + st->vref = devm_regulator_get_optional(&spi->dev, "refio");
> + if (IS_ERR(st->vref)) {
> + /*
> + * If there is no REFIO supply, then it means that we are using
> + * the internal 2.5V reference.
> + */
> + if (PTR_ERR(st->vref) == -ENODEV)
> + st->vref = NULL;
> + else
> + return dev_err_probe(&spi->dev, PTR_ERR(st->vref),
> + "Failed to get refio regulator\n");
> + }
> +
> + if (st->vref) {
> + ret = regulator_enable(st->vref);
> + if (ret)
> + return ret;
> +
> + ret = devm_add_action_or_reset(&spi->dev, ad7380_regulator_disable,
> + st->vref);
> + if (ret)
> + return ret;
> + }
> +
> + 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");
Still not using a regmap_bus... You could at least argue in the last version why
you're not doing it rather than ignoring the comment :).
I'm asking for it because it already happened (in IIO) to me and I was asked for
implementing the bus. You also gain things like regmap core handling endianism and
formatting the work buffer for you (eg: regmap_bulk_read() could be more efficient),
> + indio_dev->channels = st->chip_info->channels;
> + indio_dev->num_channels = st->chip_info->num_channels;
> + indio_dev->dev.parent = &spi->dev;
still not addressed...
With at least the above (for the regmap_bus I'll leave the ultimate decision to
Jonathan - not a deal breaker for me):
Reviewed-by: Nuno Sa <nuno.sa@analog.com>
- Nuno Sá
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] iio: adc: ad7380: new driver for AD7380 ADCs
2023-12-13 12:18 ` Nuno Sá
@ 2023-12-13 13:47 ` David Lechner
2023-12-13 14:07 ` Nuno Sá
0 siblings, 1 reply; 14+ messages in thread
From: David Lechner @ 2023-12-13 13:47 UTC (permalink / raw
To: Nuno Sá
Cc: linux-iio, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Liam Girdwood, Mark Brown, linux-kernel, Stefan Popa
On Wed, Dec 13, 2023 at 1:18 PM Nuno Sá <noname.nuno@gmail.com> wrote:
>
> On Wed, 2023-12-13 at 05:21 -0600, David Lechner wrote:
> > This adds a new driver for the AD7380 family ADCs.
> >
> > The driver currently implements basic support for the AD7380, AD7381,
> > AD7383, and AD7384 2-channel differential ADCs. Support for additional
> > single-ended 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 <stefan.popa@analog.com>
> > Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > ---
> >
> > v2 changes:
> > - 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 (we always sample two channels at the same time
> > so we need to let userspace know this)
> > - Added check for missing driver match data
> >
> > MAINTAINERS | 1 +
> > drivers/iio/adc/Kconfig | 16 ++
> > drivers/iio/adc/Makefile | 1 +
> > drivers/iio/adc/ad7380.c | 464 +++++++++++++++++++++++++++++++++++++++++++++++
> > 4 files changed, 482 insertions(+)
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e2a998be5879..5a54620a31b8 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -438,6 +438,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 <michael.hennerich@analog.com>
> > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > index 35f9867da12c..cbfd626712e3 100644
> > --- a/drivers/iio/adc/Kconfig
> > +++ b/drivers/iio/adc/Kconfig
> > @@ -122,6 +122,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 bee11d442af4..9c921c497655 100644
> > --- a/drivers/iio/adc/Makefile
> > +++ b/drivers/iio/adc/Makefile
> > @@ -16,6 +16,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..b8025b636b67
> > --- /dev/null
> > +++ b/drivers/iio/adc/ad7380.c
> > @@ -0,0 +1,464 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Analog Devices AD738x Simultaneous Sampling SAR ADCs
> > + *
> > + * Copyright 2017 Analog Devices Inc.
> > + * Copyright 2023 BayLibre, SAS
> > + */
> > +
> > +#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>
> > +
>
> ...
>
> >
> > +static int ad7380_probe(struct spi_device *spi)
> > +{
> > + struct iio_dev *indio_dev;
> > + struct ad7380_state *st;
> > + 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");
> > +
> > + st->vref = devm_regulator_get_optional(&spi->dev, "refio");
> > + if (IS_ERR(st->vref)) {
> > + /*
> > + * If there is no REFIO supply, then it means that we are using
> > + * the internal 2.5V reference.
> > + */
> > + if (PTR_ERR(st->vref) == -ENODEV)
> > + st->vref = NULL;
> > + else
> > + return dev_err_probe(&spi->dev, PTR_ERR(st->vref),
> > + "Failed to get refio regulator\n");
> > + }
> > +
> > + if (st->vref) {
> > + ret = regulator_enable(st->vref);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_add_action_or_reset(&spi->dev, ad7380_regulator_disable,
> > + st->vref);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + 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");
>
> Still not using a regmap_bus... You could at least argue in the last version why
> you're not doing it rather than ignoring the comment :).
>
> I'm asking for it because it already happened (in IIO) to me and I was asked for
> implementing the bus. You also gain things like regmap core handling endianism and
> formatting the work buffer for you (eg: regmap_bulk_read() could be more efficient),
>
> > + indio_dev->channels = st->chip_info->channels;
> > + indio_dev->num_channels = st->chip_info->num_channels;
> > + indio_dev->dev.parent = &spi->dev;
>
> still not addressed...
>
> With at least the above (for the regmap_bus I'll leave the ultimate decision to
> Jonathan - not a deal breaker for me):
>
> Reviewed-by: Nuno Sa <nuno.sa@analog.com>
>
>
> - Nuno Sá
>
Sorry, I did not mean to ignore these. I just did a bad job of
double-checking that I addressed all comments before sending v2. :-(
If we need a v3, I will look into regmap_bus but at leas
superficially, I don't see much difference for this part, i.e not
really any case where bulk ops make sense and since it uses SPI bus
underneath, endianness isn't an issue.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] iio: adc: ad7380: new driver for AD7380 ADCs
2023-12-13 13:47 ` David Lechner
@ 2023-12-13 14:07 ` Nuno Sá
0 siblings, 0 replies; 14+ messages in thread
From: Nuno Sá @ 2023-12-13 14:07 UTC (permalink / raw
To: David Lechner
Cc: linux-iio, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Liam Girdwood, Mark Brown, linux-kernel, Stefan Popa
On Wed, 2023-12-13 at 14:47 +0100, David Lechner wrote:
> On Wed, Dec 13, 2023 at 1:18 PM Nuno Sá <noname.nuno@gmail.com> wrote:
> >
> > On Wed, 2023-12-13 at 05:21 -0600, David Lechner wrote:
> > > This adds a new driver for the AD7380 family ADCs.
> > >
> > > The driver currently implements basic support for the AD7380, AD7381,
> > > AD7383, and AD7384 2-channel differential ADCs. Support for additional
> > > single-ended 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 <stefan.popa@analog.com>
> > > Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > > ---
> > >
> > > v2 changes:
> > > - 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 (we always sample two channels at the same time
> > > so we need to let userspace know this)
> > > - Added check for missing driver match data
> > >
> > > MAINTAINERS | 1 +
> > > drivers/iio/adc/Kconfig | 16 ++
> > > drivers/iio/adc/Makefile | 1 +
> > > drivers/iio/adc/ad7380.c | 464 +++++++++++++++++++++++++++++++++++++++++++++++
> > > 4 files changed, 482 insertions(+)
> > >
> > > diff --git a/MAINTAINERS b/MAINTAINERS
> > > index e2a998be5879..5a54620a31b8 100644
> > > --- a/MAINTAINERS
> > > +++ b/MAINTAINERS
> > > @@ -438,6 +438,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 <michael.hennerich@analog.com>
> > > diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> > > index 35f9867da12c..cbfd626712e3 100644
> > > --- a/drivers/iio/adc/Kconfig
> > > +++ b/drivers/iio/adc/Kconfig
> > > @@ -122,6 +122,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 bee11d442af4..9c921c497655 100644
> > > --- a/drivers/iio/adc/Makefile
> > > +++ b/drivers/iio/adc/Makefile
> > > @@ -16,6 +16,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..b8025b636b67
> > > --- /dev/null
> > > +++ b/drivers/iio/adc/ad7380.c
> > > @@ -0,0 +1,464 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/*
> > > + * Analog Devices AD738x Simultaneous Sampling SAR ADCs
> > > + *
> > > + * Copyright 2017 Analog Devices Inc.
> > > + * Copyright 2023 BayLibre, SAS
> > > + */
> > > +
> > > +#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>
> > > +
> >
> > ...
> >
> > >
> > > +static int ad7380_probe(struct spi_device *spi)
> > > +{
> > > + struct iio_dev *indio_dev;
> > > + struct ad7380_state *st;
> > > + 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");
> > > +
> > > + st->vref = devm_regulator_get_optional(&spi->dev, "refio");
> > > + if (IS_ERR(st->vref)) {
> > > + /*
> > > + * If there is no REFIO supply, then it means that we are using
> > > + * the internal 2.5V reference.
> > > + */
> > > + if (PTR_ERR(st->vref) == -ENODEV)
> > > + st->vref = NULL;
> > > + else
> > > + return dev_err_probe(&spi->dev, PTR_ERR(st->vref),
> > > + "Failed to get refio
> > > regulator\n");
> > > + }
> > > +
> > > + if (st->vref) {
> > > + ret = regulator_enable(st->vref);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + ret = devm_add_action_or_reset(&spi->dev,
> > > ad7380_regulator_disable,
> > > + st->vref);
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + 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");
> >
> > Still not using a regmap_bus... You could at least argue in the last version why
> > you're not doing it rather than ignoring the comment :).
> >
> > I'm asking for it because it already happened (in IIO) to me and I was asked for
> > implementing the bus. You also gain things like regmap core handling endianism
> > and
> > formatting the work buffer for you (eg: regmap_bulk_read() could be more
> > efficient),
> >
> > > + indio_dev->channels = st->chip_info->channels;
> > > + indio_dev->num_channels = st->chip_info->num_channels;
> > > + indio_dev->dev.parent = &spi->dev;
> >
> > still not addressed...
> >
> > With at least the above (for the regmap_bus I'll leave the ultimate decision to
> > Jonathan - not a deal breaker for me):
> >
> > Reviewed-by: Nuno Sa <nuno.sa@analog.com>
> >
> >
> > - Nuno Sá
> >
>
> Sorry, I did not mean to ignore these. I just did a bad job of
> double-checking that I addressed all comments before sending v2. :-(
>
no worries (done that and will do it again) :)
> If we need a v3, I will look into regmap_bus but at leas
Well, at least 'indio_dev->dev.parent = &spi->dev;' should be removed. But yeah, if
the regmap_bus is not to be required then Jonathan might just do it while applying.
- Nuno Sá
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] dt-bindings: iio: adc: Add binding for AD7380 ADCs
2023-12-13 11:21 ` [PATCH v2 2/3] dt-bindings: iio: adc: Add binding for AD7380 ADCs David Lechner
@ 2023-12-13 16:10 ` Conor Dooley
2023-12-13 16:11 ` Conor Dooley
2023-12-13 16:15 ` Conor Dooley
1 sibling, 1 reply; 14+ messages in thread
From: Conor Dooley @ 2023-12-13 16:10 UTC (permalink / raw
To: David Lechner
Cc: linux-iio, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Liam Girdwood, Mark Brown, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5118 bytes --]
On Wed, Dec 13, 2023 at 05:21:19AM -0600, David Lechner wrote:
> This adds a binding specification for the Analog Devices Inc. AD7380
> family of ADCs.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
> ---
>
> v2 changes:
> - 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
>
> .../devicetree/bindings/iio/adc/adi,ad7380.yaml | 107 +++++++++++++++++++++
> MAINTAINERS | 9 ++
> 2 files changed, 116 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..43d58c52f7dd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> @@ -0,0 +1,107 @@
> +# 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 <Michael.Hennerich@analog.com>
> + - Nuno Sá <nuno.sa@analog.com>
> +
> +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#
> +
> +properties:
> + compatible:
> + enum:
> + - adi,ad7380
> + - adi,ad7381
> + - adi,ad7383
> + - adi,ad7384
> +
> + reg:
> + maxItems: 1
> +
> + spi-max-frequency:
> + maximum: 80000000
> + spi-cpol: true
> + spi-cpha: true
> +
> + spi-rx-bus-channels:
This is now being framed as a standard property, so I think it should be
in spi-peripheral-props, no? Granted, you'd need a rather more
generalised explanation of the property in that case.
> + description:
> + In 1-wire mode, the SDOA pin acts as the sole data line and the SDOB/ALERT
> + pin acts as the ALERT interrupt signal. In 2-wire mode, data for input A
> + is read from SDOA and data for input B is read from SDOB/ALERT (and the
> + ALERT interrupt signal is not available).
> + enum: [1, 2]
Jonathan also mentioned specifying that this defaults to 1-wire. I
didn't see a response or that implemented. Did you miss that comment
from him?
Cheers,
Conor.
> +
> + 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
> +
> +allOf:
> + - if:
> + required:
> + - spi-rx-bus-channels
> + then:
> + if:
> + properties:
> + spi-rx-bus-channels:
> + const: 2
> + then:
> + properties:
> + interrupts: false
> +
> +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 fe1f6f97f96a..e2a998be5879 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -430,6 +430,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 <michael.hennerich@analog.com>
> +M: Nuno Sá <nuno.sa@analog.com>
> +R: David Lechner <dlechner@baylibre.com>
> +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 <michael.hennerich@analog.com>
> S: Supported
>
> --
> 2.34.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] dt-bindings: iio: adc: Add binding for AD7380 ADCs
2023-12-13 16:10 ` Conor Dooley
@ 2023-12-13 16:11 ` Conor Dooley
0 siblings, 0 replies; 14+ messages in thread
From: Conor Dooley @ 2023-12-13 16:11 UTC (permalink / raw
To: David Lechner
Cc: linux-iio, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Liam Girdwood, Mark Brown, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 3021 bytes --]
On Wed, Dec 13, 2023 at 04:10:40PM +0000, Conor Dooley wrote:
> On Wed, Dec 13, 2023 at 05:21:19AM -0600, David Lechner wrote:
> > This adds a binding specification for the Analog Devices Inc. AD7380
> > family of ADCs.
> >
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > ---
> >
> > v2 changes:
> > - 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
> >
> > .../devicetree/bindings/iio/adc/adi,ad7380.yaml | 107 +++++++++++++++++++++
> > MAINTAINERS | 9 ++
> > 2 files changed, 116 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..43d58c52f7dd
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iio/adc/adi,ad7380.yaml
> > @@ -0,0 +1,107 @@
> > +# 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 <Michael.Hennerich@analog.com>
> > + - Nuno Sá <nuno.sa@analog.com>
> > +
> > +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#
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - adi,ad7380
> > + - adi,ad7381
> > + - adi,ad7383
> > + - adi,ad7384
> > +
> > + reg:
> > + maxItems: 1
> > +
> > + spi-max-frequency:
> > + maximum: 80000000
> > + spi-cpol: true
> > + spi-cpha: true
> > +
> > + spi-rx-bus-channels:
>
> This is now being framed as a standard property, so I think it should be
> in spi-peripheral-props, no? Granted, you'd need a rather more
> generalised explanation of the property in that case.
>
> > + description:
> > + In 1-wire mode, the SDOA pin acts as the sole data line and the SDOB/ALERT
> > + pin acts as the ALERT interrupt signal. In 2-wire mode, data for input A
> > + is read from SDOA and data for input B is read from SDOB/ALERT (and the
> > + ALERT interrupt signal is not available).
> > + enum: [1, 2]
>
> Jonathan also mentioned specifying that this defaults to 1-wire. I
> didn't see a response or that implemented. Did you miss that comment
> from him?
Ah, I read the patchset backwards, d'oh. I see you did in fact do both
of these things. Apologies!
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] dt-bindings: iio: adc: Add binding for AD7380 ADCs
2023-12-13 11:21 ` [PATCH v2 2/3] dt-bindings: iio: adc: Add binding for AD7380 ADCs David Lechner
2023-12-13 16:10 ` Conor Dooley
@ 2023-12-13 16:15 ` Conor Dooley
1 sibling, 0 replies; 14+ messages in thread
From: Conor Dooley @ 2023-12-13 16:15 UTC (permalink / raw
To: David Lechner
Cc: linux-iio, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Liam Girdwood, Mark Brown, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 374 bytes --]
On Wed, Dec 13, 2023 at 05:21:19AM -0600, David Lechner wrote:
> This adds a binding specification for the Analog Devices Inc. AD7380
> family of ADCs.
>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Having read the patchset in the correct order, this patch now
seems fine to me.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
Cheers,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] iio: adc: ad7380: new driver for AD7380 ADCs
2023-12-13 11:21 ` [PATCH v2 3/3] iio: adc: ad7380: new driver " David Lechner
2023-12-13 12:18 ` Nuno Sá
@ 2023-12-14 10:14 ` Jonathan Cameron
2023-12-14 10:33 ` David Lechner
1 sibling, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2023-12-14 10:14 UTC (permalink / raw
To: David Lechner
Cc: linux-iio, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Liam Girdwood, Mark Brown, linux-kernel, Stefan Popa
On Wed, 13 Dec 2023 05:21:20 -0600
David Lechner <dlechner@baylibre.com> wrote:
> This adds a new driver for the AD7380 family ADCs.
>
> The driver currently implements basic support for the AD7380, AD7381,
> AD7383, and AD7384 2-channel differential ADCs. Support for additional
> single-ended 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 <stefan.popa@analog.com>
> Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> Signed-off-by: David Lechner <dlechner@baylibre.com>
Just one additional comment. I 'might' sort both this an Nuno's comment
if Mark is fine with the SPI and no on else has review comments.
Feel free to send a v3 though if you like ;)
> +/* fully differential */
> +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7380_channels, 16);
> +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7381_channels, 14);
> +/* pseudo differential */
> +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7383_channels, 16);
> +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7384_channels, 14);
> +
> +/* Since this is simultaneous sampling, we don't allow individual channels. */
> +static const unsigned long ad7380_2_channel_scan_masks[] = {
> + GENMASK(2, 0), /* both ADC channels and soft timestamp */
> + GENMASK(1, 0), /* both ADC channels, no timestamp */
https://elixir.bootlin.com/linux/v6.7-rc5/source/include/linux/iio/iio.h#L567
See the comment (added recently!)
Also, if I remember how this works correctly there is no need to include
the timestamp in the mask. We do special handling for it to avoid having to double
the number of provided masks. The details being that it uses
iio_scan_el_ts_store rather than iio_scan_el_Store.
So as you have it I think you'll always end up with the first entry
and that will include a bonus bit that isn't a problem as it will match
anyway.
So just have the second entry and 0.
Jonathan
> + 0
> +};
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] iio: adc: ad7380: new driver for AD7380 ADCs
2023-12-14 10:14 ` Jonathan Cameron
@ 2023-12-14 10:33 ` David Lechner
2023-12-14 12:36 ` Jonathan Cameron
0 siblings, 1 reply; 14+ messages in thread
From: David Lechner @ 2023-12-14 10:33 UTC (permalink / raw
To: Jonathan Cameron
Cc: linux-iio, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Liam Girdwood, Mark Brown, linux-kernel, Stefan Popa
On Thu, Dec 14, 2023 at 11:14 AM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Wed, 13 Dec 2023 05:21:20 -0600
> David Lechner <dlechner@baylibre.com> wrote:
>
> > This adds a new driver for the AD7380 family ADCs.
> >
> > The driver currently implements basic support for the AD7380, AD7381,
> > AD7383, and AD7384 2-channel differential ADCs. Support for additional
> > single-ended 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 <stefan.popa@analog.com>
> > Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> > Signed-off-by: David Lechner <dlechner@baylibre.com>
>
> Just one additional comment. I 'might' sort both this an Nuno's comment
> if Mark is fine with the SPI and no on else has review comments.
> Feel free to send a v3 though if you like ;)
>
>
> > +/* fully differential */
> > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7380_channels, 16);
> > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7381_channels, 14);
> > +/* pseudo differential */
> > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7383_channels, 16);
> > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7384_channels, 14);
> > +
> > +/* Since this is simultaneous sampling, we don't allow individual channels. */
> > +static const unsigned long ad7380_2_channel_scan_masks[] = {
> > + GENMASK(2, 0), /* both ADC channels and soft timestamp */
> > + GENMASK(1, 0), /* both ADC channels, no timestamp */
>
> https://elixir.bootlin.com/linux/v6.7-rc5/source/include/linux/iio/iio.h#L567
> See the comment (added recently!)
I did see this comment but this is already sorted in order of
preference, so I'm not sure why you are calling it out. Just FYI, I
guess?
>
> Also, if I remember how this works correctly there is no need to include
> the timestamp in the mask. We do special handling for it to avoid having to double
> the number of provided masks. The details being that it uses
> iio_scan_el_ts_store rather than iio_scan_el_Store.
Indeed. I've been working ahead on adding more features and noticed
this. So we will need to find a way to say that we the timestamp
should not be allowed under certain conditions. But that will be a
discussion for a later series.
>
> So as you have it I think you'll always end up with the first entry
> and that will include a bonus bit that isn't a problem as it will match
> anyway.
>
> So just have the second entry and 0.
>
> Jonathan
>
> > + 0
> > +};
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] iio: adc: ad7380: new driver for AD7380 ADCs
2023-12-14 10:33 ` David Lechner
@ 2023-12-14 12:36 ` Jonathan Cameron
2023-12-14 15:03 ` David Lechner
0 siblings, 1 reply; 14+ messages in thread
From: Jonathan Cameron @ 2023-12-14 12:36 UTC (permalink / raw
To: David Lechner
Cc: linux-iio, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Liam Girdwood, Mark Brown, linux-kernel, Stefan Popa
On Thu, 14 Dec 2023 11:33:51 +0100
David Lechner <dlechner@baylibre.com> wrote:
> On Thu, Dec 14, 2023 at 11:14 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Wed, 13 Dec 2023 05:21:20 -0600
> > David Lechner <dlechner@baylibre.com> wrote:
> >
> > > This adds a new driver for the AD7380 family ADCs.
> > >
> > > The driver currently implements basic support for the AD7380, AD7381,
> > > AD7383, and AD7384 2-channel differential ADCs. Support for additional
> > > single-ended 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 <stefan.popa@analog.com>
> > > Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> >
> > Just one additional comment. I 'might' sort both this an Nuno's comment
> > if Mark is fine with the SPI and no on else has review comments.
> > Feel free to send a v3 though if you like ;)
> >
> >
> > > +/* fully differential */
> > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7380_channels, 16);
> > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7381_channels, 14);
> > > +/* pseudo differential */
> > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7383_channels, 16);
> > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7384_channels, 14);
> > > +
> > > +/* Since this is simultaneous sampling, we don't allow individual channels. */
> > > +static const unsigned long ad7380_2_channel_scan_masks[] = {
> > > + GENMASK(2, 0), /* both ADC channels and soft timestamp */
> > > + GENMASK(1, 0), /* both ADC channels, no timestamp */
> >
> > https://elixir.bootlin.com/linux/v6.7-rc5/source/include/linux/iio/iio.h#L567
> > See the comment (added recently!)
>
> I did see this comment but this is already sorted in order of
> preference, so I'm not sure why you are calling it out. Just FYI, I
> guess?
No. Order of preference would be turn on the minimal if that is enough.
First item is the highest preference (if the requested channels are a subset of
that we don't look any further). Here that means we always stop on the first
entry and never look at the second.
>
> >
> > Also, if I remember how this works correctly there is no need to include
> > the timestamp in the mask. We do special handling for it to avoid having to double
> > the number of provided masks. The details being that it uses
> > iio_scan_el_ts_store rather than iio_scan_el_Store.
>
> Indeed. I've been working ahead on adding more features and noticed
> this. So we will need to find a way to say that we the timestamp
> should not be allowed under certain conditions. But that will be a
> discussion for a later series.
Interesting - you have cases where it's not valid at all?
It sometimes becomes inaccurate because we are interpolating across
data from a fifo, but I've not seen a case where we can't provide anything
useful. Ah well - as you say I'll wait for that later series!
Jonathan
>
> >
> > So as you have it I think you'll always end up with the first entry
> > and that will include a bonus bit that isn't a problem as it will match
> > anyway.
> >
> > So just have the second entry and 0.
> >
> > Jonathan
> >
> > > + 0
> > > +};
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] iio: adc: ad7380: new driver for AD7380 ADCs
2023-12-14 12:36 ` Jonathan Cameron
@ 2023-12-14 15:03 ` David Lechner
0 siblings, 0 replies; 14+ messages in thread
From: David Lechner @ 2023-12-14 15:03 UTC (permalink / raw
To: Jonathan Cameron
Cc: linux-iio, devicetree, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Jonathan Cameron, Michael Hennerich, Nuno Sá,
Liam Girdwood, Mark Brown, linux-kernel, Stefan Popa
On Thu, Dec 14, 2023 at 1:36 PM Jonathan Cameron
<Jonathan.Cameron@huawei.com> wrote:
>
> On Thu, 14 Dec 2023 11:33:51 +0100
> David Lechner <dlechner@baylibre.com> wrote:
>
> > On Thu, Dec 14, 2023 at 11:14 AM Jonathan Cameron
> > <Jonathan.Cameron@huawei.com> wrote:
> > >
> > > On Wed, 13 Dec 2023 05:21:20 -0600
> > > David Lechner <dlechner@baylibre.com> wrote:
> > >
> > > > This adds a new driver for the AD7380 family ADCs.
> > > >
> > > > The driver currently implements basic support for the AD7380, AD7381,
> > > > AD7383, and AD7384 2-channel differential ADCs. Support for additional
> > > > single-ended 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 <stefan.popa@analog.com>
> > > > Signed-off-by: Stefan Popa <stefan.popa@analog.com>
> > > > Signed-off-by: David Lechner <dlechner@baylibre.com>
> > >
> > > Just one additional comment. I 'might' sort both this an Nuno's comment
> > > if Mark is fine with the SPI and no on else has review comments.
> > > Feel free to send a v3 though if you like ;)
> > >
> > >
> > > > +/* fully differential */
> > > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7380_channels, 16);
> > > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7381_channels, 14);
> > > > +/* pseudo differential */
> > > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7383_channels, 16);
> > > > +DEFINE_AD7380_DIFFERENTIAL_2_CHANNEL(ad7384_channels, 14);
> > > > +
> > > > +/* Since this is simultaneous sampling, we don't allow individual channels. */
> > > > +static const unsigned long ad7380_2_channel_scan_masks[] = {
> > > > + GENMASK(2, 0), /* both ADC channels and soft timestamp */
> > > > + GENMASK(1, 0), /* both ADC channels, no timestamp */
> > >
> > > https://elixir.bootlin.com/linux/v6.7-rc5/source/include/linux/iio/iio.h#L567
> > > See the comment (added recently!)
> >
> > I did see this comment but this is already sorted in order of
> > preference, so I'm not sure why you are calling it out. Just FYI, I
> > guess?
>
> No. Order of preference would be turn on the minimal if that is enough.
> First item is the highest preference (if the requested channels are a subset of
> that we don't look any further). Here that means we always stop on the first
> entry and never look at the second.
OK, I understand what you are getting at now. I thought the preference
could be my personal preference rather than the minimal case. :-)
But as you pointed out, the timestamp is handled separately, so it
doesn't make a difference here. The main point was to ensure that both
channels are always enabled since the ADC is doing simultaneous
sampling and always reading two channels at the same time.
>
> >
> > >
> > > Also, if I remember how this works correctly there is no need to include
> > > the timestamp in the mask. We do special handling for it to avoid having to double
> > > the number of provided masks. The details being that it uses
> > > iio_scan_el_ts_store rather than iio_scan_el_Store.
> >
> > Indeed. I've been working ahead on adding more features and noticed
> > this. So we will need to find a way to say that we the timestamp
> > should not be allowed under certain conditions. But that will be a
> > discussion for a later series.
>
> Interesting - you have cases where it's not valid at all?
> It sometimes becomes inaccurate because we are interpolating across
> data from a fifo, but I've not seen a case where we can't provide anything
> useful. Ah well - as you say I'll wait for that later series!
>
> Jonathan
>
> >
> > >
> > > So as you have it I think you'll always end up with the first entry
> > > and that will include a bonus bit that isn't a problem as it will match
> > > anyway.
> > >
> > > So just have the second entry and 0.
> > >
> > > Jonathan
> > >
> > > > + 0
> > > > +};
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-12-14 15:03 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-13 11:21 [PATCH v2 0/3] iio: adc: add new ad7380 driver David Lechner
2023-12-13 11:21 ` [PATCH v2 1/3] dt-bindings: spi: add spi-rx-bus-channels peripheral property David Lechner
2023-12-13 11:21 ` [PATCH v2 2/3] dt-bindings: iio: adc: Add binding for AD7380 ADCs David Lechner
2023-12-13 16:10 ` Conor Dooley
2023-12-13 16:11 ` Conor Dooley
2023-12-13 16:15 ` Conor Dooley
2023-12-13 11:21 ` [PATCH v2 3/3] iio: adc: ad7380: new driver " David Lechner
2023-12-13 12:18 ` Nuno Sá
2023-12-13 13:47 ` David Lechner
2023-12-13 14:07 ` Nuno Sá
2023-12-14 10:14 ` Jonathan Cameron
2023-12-14 10:33 ` David Lechner
2023-12-14 12:36 ` Jonathan Cameron
2023-12-14 15:03 ` David Lechner
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 read-only IMAP folder(s) and NNTP newsgroup(s).