iio: adc: ad4134: Enable greater sample rate data capture
Subsystems (6)
Marcelo Schmitt
maintainerv2 cover iio: adc: ad4134: Enable greater sample rate data capture
Extend AD4134 support to enable greater sample rate data capture. To achieve that, provide control of digital filter type selection. The default filter type on device power up only supports sample rates up to 374 kSPS. In addition to that, enable interfacing with AD4134 in SPI 4-wire mode that can support reading ADC sample data through multiple wires. Finally, use SPI offloading to speed up data transfers at up to 1.49 MSPS.
Features being added:
- Data filter type selection.
- Triggered buffer data read.
- SPI 4-wire mode.
- High speed data capture with SPI offloading.
- Device tree doc updates to reflect previously missing chip characteristics.
Minor improvements (introduced before the new features):
- Sign extend sample data. [NEW FIXES PATCH]
- Fix typo in vendor name.
- Drop import to empty name space.
- Update include list to comply with IWYU principles.
- Serialize single-read operations.
- Run shorter transfers when CRC is disabled.
The updates in v2 tackle all applicable suggestions from reviewers and from automated tests/reviews/AI.
Change log v1 -> v2:
[device tree]
- Set default for adi,spi-mode property.
- Added '#daisy-chained-devices' property.
- Documented constraints for adi,spi-mode + #daisy-chained-devices combinations.
- Documented external multiplexer usage. [NEW PATCH]
[IIO]
- Sign extended ADC sample data for correct delivery of negative ADC input difference.
- Now also fixing Analog Devices possessive suffix usage.
- Added {} to delimit scoped mutex usage context in single-sample read.
- Tidy up code for filter select support.
- Fixed IIO scan buffer size and single-read loop.
- No longer memcpy() with same pointer for source and destination.
- Always delimit guard()() with a blank line.
- Fail on unsupported adi,spi-mode.
- Now properly initializing access mode mutex.
- Highlighted usage of hardware external to AD4134 chip.
- Added comment about mux state switch logic.
- Suggestive name for auxiliary local variables.
- Updated preparation flow for ad4134_data_read().
- Error out on min I/O + offload config (currently not being supported).
- Preserved triggered buffer setup callbacks.
- Used pahole to reduce memory holes in ad4134_state struct.
- Improved PWM setup readability, update allowed scan_mask codestyle.
- Docs: Improved AD4134 connection diagrams.
Previous submissions:
v1: https://lore.kernel.org/linux-iio/[email protected]/
With best regards,
Marcelo
Marcelo Schmitt (15):
iio: adc: ad4134: Sign extend sample data
iio: Fix typo in vendor name
iio: adc: ad4134: Drop import to empty name space
iio: adc: ad4134: Update include list to comply with IWYU principles
iio: adc: ad4134: Serialize single-read operations
iio: adc: ad4134: Run shorter transfers when CRC is disabled
iio: adc: ad4134: Add support for digital filter type selection
iio: adc: ad4134: Support buffered data read
dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode
dt-bindings: iio: adc: adi,ad4134: Document external multiplexer usage
iio: adc: ad4134: Support SPI 4-wire mode
dt-bindings: iio: adc: adi,ad4134: Document PWM usage
dt-bindings: iio: adc: adi,ad4134: Add high data throughput example
iio: adc: ad4134: Support high-speed data capture
Docs: iio: Add AD4134
.../bindings/iio/adc/adi,ad4134.yaml | 111 ++- Documentation/iio/ad4134.rst | 130 +++ Documentation/iio/ade9000.rst | 2 +- Documentation/iio/adis16475.rst | 2 +- Documentation/iio/adis16480.rst | 2 +- Documentation/iio/adis16550.rst | 2 +- Documentation/iio/adxl313.rst | 2 +- Documentation/iio/adxl345.rst | 2 +- Documentation/iio/adxl380.rst | 2 +- Documentation/iio/index.rst | 1 + MAINTAINERS | 1 + drivers/iio/adc/Kconfig | 20 +- drivers/iio/adc/ad4134.c | 751 +++++++++++++++++- drivers/iio/dac/ad3552r.c | 2 +- 14 files changed, 985 insertions(+), 45 deletions(-) create mode 100644 Documentation/iio/ad4134.rst
base-commit: 9c464e7473f762cee4c031a09da04f65a1c65d49
Signature · 1 line
2.53.0
Marcelo Schmitt
maintainerPATCH v2 1/15 iio: adc: ad4134: Sign extend sample data
Sign extend ADC sample data for correct delivery of negative ADC input difference.
---
New patch in v2.
drivers/iio/adc/ad4134.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/iio/adc/ad4134.c +3 -1
@@ -172,6 +172,7 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg, unsigned int *val) {unsigned int i; +u32 sample;int ret;/*
@@ -196,8 +197,9 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,* Clock out data from all channels to avoid that. */ if (i ==AD4134_VREG_CH (reg)) - *val =get_unaligned_be24 (st->rx_buf); + sample =get_unaligned_be24 (st->rx_buf); } + *val =sign_extend32 (sample,AD4134_CHAN_PRECISION_BITS -1 );return 0 ; }
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v2 01/15] iio: adc: ad4134: Sign extend sample data
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] iio: adc: ad4134: info leak via uninitialized sample in ad4134_data_read() --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] iio: adc: ad4134: info leak via uninitialized sample in ad4134_data_read() --
commit 56b8f43cabdc9a50d1d532dbfddf01a627e253d4
Author: Marcelo Schmitt <[email protected]>
iio: adc: ad4134: Sign extend sample data
Sign extend ADC sample data for correct delivery of negative ADC input difference.
Quoted 12 lines
drivers/iio/adc/ad4134.c +1
-0@@ -172,6 +172,7 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg, unsigned int *val) {unsigned int i; +u32 sample;int ret;/*
[ ... ]
drivers/iio/adc/ad4134.c static int ad4134_data_read(struct ad4134_state *st, unsigned int reg, · line 202
Quoted 8 lines
drivers/iio/adc/ad4134.c +2
-1@@ -196,8 +197,9 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,* Clock out data from all channels to avoid that. */ if (i ==AD4134_VREG_CH (reg)) - *val =get_unaligned_be24 (st->rx_buf); + sample =get_unaligned_be24 (st->rx_buf); } + *val =sign_extend32 (sample,AD4134_CHAN_PRECISION_BITS -1 );
[Severity: High]
Does this code leak uninitialized stack memory to userspace?
If a privileged user reads register 0x54 via the IIO debugfs direct_reg_access file, ad4134_data_read() is invoked with reg=0x54. This is possible because of a pre-existing off-by-one error in the regmap configuration (where AD4134_CH_VREG(AD4134_NUM_CHANNELS) evaluates to 0x54) that allows read access to this virtual register.
Because the loop index 'i' only iterates up to 3, the condition 'i == AD4134_VREG_CH(0x54)' is never met and 'sample' is left uninitialized. The newly introduced code then unconditionally sign-extends this uninitialized memory and writes it back to '*val', which is then returned to the user.
Quoted 3 lines
drivers/iio/adc/ad4134.c +0
-0return 0 ; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Jonathan Cameron
maintainerRe: [PATCH v2 01/15] iio: adc: ad4134: Sign extend sample data
In reply to Marcelo SchmittQuoted 5 lines
Sign extend ADC sample data for correct delivery of negative ADC input difference.
Fixes e0bc6d7e2584 ("iio: adc: Initial support for AD4134")Signed-off-by Marcelo Schmitt <[email protected]>
Hi Marcelo, Sashiko made a comment and I have no idea if it is right or not!
Quoted 14 lines
drivers/iio/adc/ad4134.c +1
-0@@ -172,6 +172,7 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg, unsigned int *val) {unsigned int i; +u32 sample;int ret;/*
drivers/iio/adc/ad4134.c static int ad4134_data_read(struct ad4134_state *st, unsigned int reg, · line 202
Quoted 7 lines
drivers/iio/adc/ad4134.c +2
-1@@ -196,8 +197,9 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,* Clock out data from all channels to avoid that. */ if (i ==AD4134_VREG_CH (reg)) - *val =get_unaligned_be24 (st->rx_buf); + sample =get_unaligned_be24 (st->rx_buf); } + *val =sign_extend32 (sample,AD4134_CHAN_PRECISION_BITS -1 );
Sashiko.
[Severity: High]
Does this code leak uninitialized stack memory to userspace?
If a privileged user reads register 0x54 via the IIO debugfs direct_reg_access file, ad4134_data_read() is invoked with reg=0x54. This is possible because of a pre-existing off-by-one error in the regmap configuration (where AD4134_CH_VREG(AD4134_NUM_CHANNELS) evaluates to 0x54) that allows read access to this virtual register.
Because the loop index 'i' only iterates up to 3, the condition 'i == AD4134_VREG_CH(0x54)' is never met and 'sample' is left uninitialized. The newly introduced code then unconditionally sign-extends this uninitialized memory and writes it back to '*val', which is then returned to the user. -
Smells potentially valid givne previously val was not set unless we had a match and now it is always set.
Signature · 1 line
Jonathan Cameron <[email protected]>
Marcelo Schmitt
maintainerPATCH v2 2/15 iio: Fix typo in vendor name
In reply to Marcelo SchmittCorrect a typo in vendor company name: "Analog Device" -> "Analog Devices".
---
Change log v1 -> v2:
- Now also fixing Analog Devices possessive suffix usage.
Documentation/iio/ade9000.rst | 2 +- Documentation/iio/adis16475.rst | 2 +- Documentation/iio/adis16480.rst | 2 +- Documentation/iio/adis16550.rst | 2 +- Documentation/iio/adxl313.rst | 2 +- Documentation/iio/adxl345.rst | 2 +- Documentation/iio/adxl380.rst | 2 +- drivers/iio/adc/Kconfig | 12 ++++++------ drivers/iio/dac/ad3552r.c | 2 +- 9 files changed, 14 insertions(+), 14 deletions(-)
Documentation/iio/ade9000.rst +1 -1
@@ -4,7 +4,7 @@ ADE9000/ADE9078 driver ====================== -This driver supports Analog Device's ADE9000 and ADE9078 energy measurement +This driver supports Analog Devices' ADE9000 and ADE9078 energy measurement ICs on SPI bus. 1. Supported devices
Documentation/iio/adis16475.rst +1 -1
@@ -4,7 +4,7 @@ ADIS16475 driver ================ -This driver supports Analog Device's IMUs on SPI bus. +This driver supports Analog Devices' IMUs on SPI bus. 1. Supported devices ====================
Documentation/iio/adis16480.rst +1 -1
@@ -4,7 +4,7 @@ ADIS16480 driver ================ -This driver supports Analog Device's IMUs on SPI bus. +This driver supports Analog Devices' IMUs on SPI bus. 1. Supported devices ====================
Documentation/iio/adis16550.rst +1 -1
@@ -4,7 +4,7 @@ ADIS16550 driver ================ -This driver supports Analog Device's IMUs on SPI bus. +This driver supports Analog Devices' IMUs on SPI bus. 1. Supported devices ====================
Documentation/iio/adxl313.rst +1 -1
@@ -4,7 +4,7 @@ ADXL313 driver =============== -This driver supports Analog Device's ADXL313 on SPI/I2C bus. +This driver supports Analog Devices' ADXL313 on SPI/I2C bus. 1. Supported devices ====================
Documentation/iio/adxl345.rst +1 -1
@@ -4,7 +4,7 @@ ADXL345 driver =============== -This driver supports Analog Device's ADXL345/375 on SPI/I2C bus. +This driver supports Analog Devices' ADXL345/375 on SPI/I2C bus. 1. Supported Devices ====================
Documentation/iio/adxl380.rst +1 -1
@@ -4,7 +4,7 @@ ADXL380 driver =============== -This driver supports Analog Device's ADXL380/382 on SPI/I2C bus. +This driver supports Analog Devices' ADXL380/382 on SPI/I2C bus. 1. Supported devices ====================
drivers/iio/adc/Kconfig +6 -6
@@ -105,7 +105,7 @@ config AD4080 called ad4080. config AD4130 - tristate "Analog Device AD4130 ADC Driver" + tristate "Analog Devices AD4130 ADC Driver" depends on SPI depends on GPIOLIB select IIO_BUFFER
@@ -121,7 +121,7 @@ config AD4130 called ad4130. config AD4134 - tristate "Analog Device AD4134 ADC Driver" + tristate "Analog Devices AD4134 ADC Driver" depends on SPI select REGMAP_SPI select CRC8
@@ -133,7 +133,7 @@ config AD4134 called ad4134_spi. config AD4170_4 - tristate "Analog Device AD4170-4 ADC Driver" + tristate "Analog Devices AD4170-4 ADC Driver" depends on SPI select REGMAP_SPI select IIO_BUFFER
@@ -164,7 +164,7 @@ config AD4691 called ad4691. config AD4695 - tristate "Analog Device AD4695 ADC Driver" + tristate "Analog Devices AD4695 ADC Driver" depends on SPI select IIO_BUFFER select IIO_BUFFER_DMAENGINE
@@ -179,7 +179,7 @@ config AD4695 called ad4695. config AD4851 - tristate "Analog Device AD4851 DAS Driver" + tristate "Analog Devices AD4851 DAS Driver" depends on SPI depends on PWM select REGMAP_SPI
@@ -347,7 +347,7 @@ config AD7380 called ad7380. config AD7405 - tristate "Analog Device AD7405 ADC Driver" + tristate "Analog Devices AD7405 ADC Driver" depends on IIO_BACKEND help Say yes here to build support for Analog Devices AD7405, ADUM7701,
drivers/iio/dac/ad3552r.c +1 -1
@@ -696,6 +696,6 @@ static struct spi_driver ad3552r_driver = {module_spi_driver (ad3552r_driver);MODULE_AUTHOR ("Mihail Chindris <[email protected]>" ); -MODULE_DESCRIPTION ("Analog Device AD3552R DAC" ); +MODULE_DESCRIPTION ("Analog Devices AD3552R DAC" );MODULE_LICENSE ("GPL v2" );MODULE_IMPORT_NS ("IIO_AD3552R" );
Signature · 1 line
2.53.0
Jonathan Cameron
maintainerRe: [PATCH v2 02/15] iio: Fix typo in vendor name
Quoted 4 lines
Correct a typo in vendor company name: "Analog Device" -> "Analog Devices".
Reviewed-by Andy Shevchenko <[email protected]>Signed-off-by Marcelo Schmitt <[email protected]>
Not connected to the rest of the series. Applied
Signature · 1 line
Jonathan Cameron <[email protected]>
Marcelo Schmitt
maintainerPATCH v2 3/15 iio: adc: ad4134: Drop import to empty name space
In reply to Marcelo SchmittDrop unused name space that somehow remained being included by the device driver.
---
No changes since v1.
drivers/iio/adc/ad4134.c | 1 - 1 file changed, 1 deletion(-)
drivers/iio/adc/ad4134.c +0 -1
@@ -498,4 +498,3 @@ module_spi_driver(ad4134_driver);MODULE_AUTHOR ("Marcelo Schmitt <[email protected]>" );MODULE_DESCRIPTION ("Analog Devices AD4134 SPI driver" );MODULE_LICENSE ("GPL" ); -MODULE_IMPORT_NS ("IIO_AD4134" );
Signature · 1 line
2.53.0
Re: [PATCH v2 03/15] iio: adc: ad4134: Drop import to empty name space
Quoted 4 lines · On Tue, 15 Sep 2026 16:12:38 -0300 Marcelo Schmitt…
Drop unused name space that somehow remained being included by the device driver.
Signed-off-by Marcelo Schmitt <[email protected]>
I'll pick this one up now even if the rest need more time
Applied, thanks,
Jonathan
Quoted 15 lines
---
No changes since v1.drivers/iio/adc/ad4134.c | 1 - 1 file changed, 1 deletion(-)drivers/iio/adc/ad4134.c +0
-1@@ -498,4 +498,3 @@ module_spi_driver(ad4134_driver);MODULE_AUTHOR ("Marcelo Schmitt <[email protected]>" );MODULE_DESCRIPTION ("Analog Devices AD4134 SPI driver" );MODULE_LICENSE ("GPL" ); -MODULE_IMPORT_NS ("IIO_AD4134" );
Marcelo Schmitt
maintainerPATCH v2 4/15 iio: adc: ad4134: Update include list to comply with IWYU principles
In reply to Marcelo SchmittAdd includes that were missing (according to include-what-you-use tool) to comply with include what is used principles. Namely, add time64.h for USEC_PER_SEC.
Remove time.h which is covered by time64.h and iio/types.h which is covered by iio/iio.h.
While updating the list, organize it to follow current IIO include list convention.
---
Change log v1 -> v2:
- bits.h is implied by bitops.h so not adding that to the include list.
drivers/iio/adc/ad4134.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/iio/adc/ad4134.c +3 -3
@@ -14,18 +14,18 @@#include <linux/err.h> #include <linux/export.h> #include <linux/gpio/consumer.h> -#include <linux/iio/iio.h> -#include <linux/iio/types.h> #include <linux/module.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> #include <linux/reset.h> #include <linux/spi/spi.h> -#include <linux/time.h> +#include <linux/time64.h> #include <linux/types.h> #include <linux/unaligned.h> #include <linux/units.h> +#include <linux/iio/iio.h> +#define AD4134_RESET_TIME_US (10 * USEC_PER_SEC)#define AD4134_REG_READ_MASK BIT(7)
Signature · 1 line
2.53.0
Marcelo Schmitt
maintainerPATCH v2 5/15 iio: adc: ad4134: Serialize single-read operations
In reply to Marcelo SchmittProtect single-sample read operations from going wrong if two or more of them run concurrently. Serialize the data read operation so to avoid single-sample read requests disrupting each other.
---
Change log v1 -> v2:
- Added {} to delimit scoped mutex usage context in single-sample read.
- Used pahole to reduce memory holes in ad4134_state struct.
drivers/iio/adc/ad4134.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
drivers/iio/adc/ad4134.c +15 -1
@@ -7,6 +7,7 @@#include <linux/array_size.h> #include <linux/bitfield.h> #include <linux/bitops.h> +#include <linux/cleanup.h> #include <linux/clk.h> #include <linux/crc8.h> #include <linux/delay.h>
@@ -15,6 +16,7 @@#include <linux/export.h> #include <linux/gpio/consumer.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> #include <linux/reset.h>
@@ -98,6 +100,11 @@ struct ad4134_state {struct regmap *regmap;unsigned long sys_clk_hz;struct gpio_desc *odr_gpio; +/* +* Synchronize access to members the of driver state, and ensure +* atomicity of consecutive register access operations. +*/ +struct mutex lock;int refin_mv;/* * DMA (thus cache coherency maintenance) requires the transfer buffers
@@ -257,7 +264,9 @@ static int ad4134_read_raw(struct iio_dev *indio_dev,int ret;switch (info) { -case IIO_CHAN_INFO_RAW : +case IIO_CHAN_INFO_RAW : { +guard (mutex)(&st->lock); +gpiod_set_value_cansleep (st->odr_gpio,1 );/* * For slave mode gated DCLK (data sheet page 11), the minimum
@@ -276,6 +285,7 @@ static int ad4134_read_raw(struct iio_dev *indio_dev,return ret;return IIO_VAL_INT ; + }case IIO_CHAN_INFO_SCALE : *val = st->refin_mv; *val2 =AD4134_CHAN_PRECISION_BITS -1 ;
@@ -444,6 +454,10 @@ static int ad4134_probe(struct spi_device *spi)crc8_populate_msb (ad4134_spi_crc_table,AD4134_SPI_CRC_POLYNOM ); + ret =devm_mutex_init (dev, &st->lock); +if (ret) +return ret; + st->regmap =devm_regmap_init (dev,NULL , st, &ad4134_regmap_config);if (IS_ERR (st->regmap)) return dev_err_probe(dev, PTR_ERR(st->regmap),
Signature · 1 line
2.53.0
Marcelo Schmitt
maintainerPATCH v2 6/15 iio: adc: ad4134: Run shorter transfers when CRC is disabled
In reply to Marcelo SchmittWhen AD4134 has its SPI CRC feature enabled, one extra byte (the CRC code) is sent through SPI for both ends to verify the integrity of data transfers. Though, when CRC is not enabled, SPI transfers can be shorter.
---
No changes since v1.
drivers/iio/adc/ad4134.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-)
drivers/iio/adc/ad4134.c +6 -4
@@ -106,6 +106,7 @@ struct ad4134_state {*/ struct mutex lock;int refin_mv; +bool crc_en;/* * DMA (thus cache coherency maintenance) requires the transfer buffers * to live in their own cache lines.
@@ -159,7 +160,7 @@ static int ad4134_reg_write(void *context, unsigned int reg, unsigned int val)struct spi_transfer xfer = { .tx_buf = st->tx_buf, .rx_buf = st->rx_buf, - .len =AD4134_SPI_MAX_XFER_LEN , + .len = st->crc_en ?AD4134_SPI_MAX_XFER_LEN :2 , };int ret;
@@ -169,7 +170,7 @@ static int ad4134_reg_write(void *context, unsigned int reg, unsigned int val)if (ret)return ret; -if (st->rx_buf[2 ] != st->tx_buf[2 ]) +if (st->crc_en && st->rx_buf[2 ] != st->tx_buf[2 ])dev_dbg (&st->spi->dev,"reg write CRC check failed\n" );return 0 ;
@@ -217,7 +218,7 @@ static int ad4134_register_read(struct ad4134_state *st, unsigned int reg,struct spi_transfer xfer = { .tx_buf = st->tx_buf, .rx_buf = st->rx_buf, - .len =AD4134_SPI_MAX_XFER_LEN , + .len = st->crc_en ?AD4134_SPI_MAX_XFER_LEN :2 , };unsigned int inst;int ret;
@@ -232,7 +233,7 @@ static int ad4134_register_read(struct ad4134_state *st, unsigned int reg, *val = st->rx_buf[1 ];/* Check CRC */ -if (st->rx_buf[2 ] != st->tx_buf[2 ]) +if (st->crc_en && st->rx_buf[2 ] != st->tx_buf[2 ])dev_dbg (&st->spi->dev,"reg read CRC check failed\n" );return 0 ;
@@ -313,6 +314,7 @@ static int ad4134_min_io_mode_setup(struct ad4134_state *st)struct device *dev = &st->spi->dev;int ret; + st->crc_en = true;/* In minimum I/O mode CRC cannot be disabled */ st->odr_gpio =devm_gpiod_get (dev,"odr" ,GPIOD_OUT_LOW );if (IS_ERR (st->odr_gpio)) return dev_err_probe(dev, PTR_ERR(st->odr_gpio),
Signature · 1 line
2.53.0
Marcelo Schmitt
maintainerPATCH v2 7/15 iio: adc: ad4134: Add support for digital filter type selection
In reply to Marcelo SchmittAD4134 supports four types of programmable digital filters that allow users to choose between different output data rates and input bandwidths. Enable basic digital filter configuration by providing a way to select which filter type is activated for each channel.
---
Change log v1 -> v2:
- Tidy up code for filter select support.
drivers/iio/adc/ad4134.c | 76 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+)
drivers/iio/adc/ad4134.c +76 -0
@@ -63,6 +63,9 @@#define AD4134_PW_DOWN_CTRL_REG 0x13#define AD4134_DEVICE_STATUS_REG 0x15#define AD4134_ODR_VAL_INT_LSB_REG 0x16 +#define AD4134_CHAN_DIG_FILTER_SEL_REG 0x1E +#define AD4134_CHAN_DIG_FILTER_SEL_CH_MASK (ch) (GENMASK(1, 0) << 2 * (ch)) +#define AD4134_CH3_OFFSET_MSB_REG 0x3E#define AD4134_AIN_OR_ERROR_REG 0x48
@@ -80,12 +83,47 @@#define AD4134_SPI_CRC_INIT_VALUE 0xA5static unsigned char ad4134_spi_crc_table[CRC8_TABLE_SIZE ]; +enum ad4134_filter_type { +AD4134_WIDEBAND , +AD4134_SINC6 , +AD4134_SINC3 , +AD4134_SINC3_REJ60 , +}; + +static const char *const ad4134_filt_names[] = { + [AD4134_WIDEBAND ] ="wideband" , + [AD4134_SINC6 ] ="sinc6" , + [AD4134_SINC3 ] ="sinc3" , + [AD4134_SINC3_REJ60 ] ="sinc3+rej60" , +}; + +static int ad4134_get_filter_type (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan); + +static int ad4134_set_filter_type (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +unsigned int val); + +static const struct iio_enum ad4134_filter_type_enum = { + .items = ad4134_filt_names, + .num_items =ARRAY_SIZE (ad4134_filt_names), + .get = ad4134_get_filter_type, + .set = ad4134_set_filter_type, +}; + +static const struct iio_chan_spec_ext_info ad4134_filter_type_ext_info[] = { +IIO_ENUM ("filter_type" ,IIO_SEPARATE , &ad4134_filter_type_enum), +IIO_ENUM_AVAILABLE ("filter_type" ,IIO_SEPARATE , &ad4134_filter_type_enum), + { } +}; +#define AD4134_CHANNEL (_index) { \ .type = IIO_VOLTAGE, \ .indexed = 1, \ .channel = (_index), \ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \ .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE), \ + .ext_info = ad4134_filter_type_ext_info, \ }static const struct iio_chan_spec ad4134_chan_set[] = {
@@ -115,6 +153,44 @@ struct ad4134_state {u8 tx_buf[AD4134_SPI_MAX_XFER_LEN ]; }; +static int ad4134_set_filter_type (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan, +unsigned int val) +{ +struct ad4134_state *st =iio_priv (indio_dev); +unsigned int mask, reg_val; + +IIO_DEV_ACQUIRE_DIRECT_MODE (indio_dev, claim); +if (IIO_DEV_ACQUIRE_FAILED (claim)) +return -EBUSY ; + +guard (mutex)(&st->lock); + + mask =AD4134_CHAN_DIG_FILTER_SEL_CH_MASK (chan->channel); + reg_val =field_prep (mask, val); +return regmap_update_bits (st->regmap,AD4134_CHAN_DIG_FILTER_SEL_REG , + mask, reg_val); +} + +static int ad4134_get_filter_type (struct iio_dev *indio_dev, +struct iio_chan_spec const *chan) +{ +struct ad4134_state *st =iio_priv (indio_dev); +unsigned int mask, reg_val; +int ret; + +IIO_DEV_ACQUIRE_DIRECT_MODE (indio_dev, claim); +if (IIO_DEV_ACQUIRE_FAILED (claim)) +return -EBUSY ; + + ret =regmap_read (st->regmap,AD4134_CHAN_DIG_FILTER_SEL_REG , ®_val); +if (ret) +return ret; + + mask =AD4134_CHAN_DIG_FILTER_SEL_CH_MASK (chan->channel); +return field_get (mask, reg_val); +} +static const struct regmap_range ad4134_regmap_rd_range[] = {regmap_reg_range (AD4134_IFACE_CONFIG_A_REG ,AD4134_SILICON_REV_REG ),regmap_reg_range (AD4134_SCRATCH_PAD_REG ,AD4134_PW_DOWN_CTRL_REG ),
Signature · 1 line
2.53.0
Marcelo Schmitt
maintainerPATCH v2 8/15 iio: adc: ad4134: Support buffered data read
In reply to Marcelo SchmittEnable users to run buffered data captures triggered by IIO trigger device. Add an IIO timestamp channel so each data scan is provided with measurement time information. Require single-read operations to be in IIO device direct access mode to prevent buffered and single-shot data captures to disrupt each other.
---
Change log v1 -> v2:
- Fixed IIO scan buffer size and single-read loop.
- No longer memcpy() with same pointer for source and destination.
- Always delimit guard()() with a blank line.
drivers/iio/adc/Kconfig | 2 ++ drivers/iio/adc/ad4134.c | 75 +++++++++++++++++++++++++++++++++++----- 2 files changed, 68 insertions(+), 9 deletions(-)
drivers/iio/adc/Kconfig +2 -0
@@ -125,6 +125,8 @@ config AD4134 depends on SPI select REGMAP_SPI select CRC8 + select IIO_BUFFER + select IIO_TRIGGERED_BUFFER help Say yes here to build support for Analog Devices AD4134 SPI analog to digital converters (ADC).
drivers/iio/adc/ad4134.c +66 -9
156 lines not on this page · raw message
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v2 08/15] iio: adc: ad4134: Support buffered data read
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] iio: adc: ad4134: buffer compaction overwrites active channels due to skipped index increment…
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] iio: adc: ad4134: buffer compaction overwrites active channels due to skipped index increment
- [High] iio: adc: ad4134: undersized scan buffer causes -ENOSPC during triggered captures
- [High] iio: adc: ad4134: data corruption due to concurrent lockless access to shared rx_buf union
- [Medium] iio: adc: ad4134: uninitialized stack memory leak via out-of-bounds virtual register read --
commit 67ed10376d89a2413bb4cf43e7a24f52e73e8a76
Author: Marcelo Schmitt <[email protected]>
iio: adc: ad4134: Support buffered data read
Enable users to run buffered data captures triggered by IIO trigger device. Add an IIO timestamp channel so each data scan is provided with measurement time information. Require single-read operations to be in IIO device direct access mode to prevent buffered and single-shot data captures to disrupt each other.
Quoted 4 lines
drivers/iio/adc/ad4134.c +0
-0
[ ... ]
drivers/iio/adc/ad4134.c struct ad4134_state { · line 166
Quoted 8 lines
drivers/iio/adc/ad4134.c +3
-1@@ -149,7 +161,10 @@ struct ad4134_state {* DMA (thus cache coherency maintenance) requires the transfer buffers * to live in their own cache lines. */ -u8 rx_buf[AD4134_SPI_MAX_XFER_LEN ]__aligned(IIO_DMA_MINALIGN) ; +union { +u8 reg[AD4134_SPI_MAX_XFER_LEN ]; +u32 scan[ARRAY_SIZE (ad4134_chan_set)];
[Severity: High]
Does this array size fail to account for the timestamp's alignment padding?
The array is fixed at 20 bytes (5 elements). When all 4 channels and the 64-bit timestamp are active, the IIO core computes scan_bytes as 24 bytes due to the timestamp's 8-byte alignment requirement (16 bytes data + 8 bytes padding and timestamp). Passing sizeof(st->rx_buf.scan) to iio_push_to_buffers_with_ts() later in the trigger handler will cause the IIO core's safety check to fail, aborting the buffer push with -ENOSPC on full-scan captures.
[Severity: High]
Can concurrent lockless accesses corrupt the rx_buf shared union?
The ad4134_trigger_handler() executes locklessly in a pollfunc context, while regmap debugfs reads/writes (which do not acquire the IIO direct mode lock) can concurrently use st->rx_buf.reg as a DMA buffer. This can result in corrupted SPI scan data, corrupted regmap read/write data, and potentially invalid hardware configurations or ADC samples if debugfs is accessed while a triggered buffer capture is running.
Quoted 3 lines
drivers/iio/adc/ad4134.c +1
-0+ } rx_buf__aligned(IIO_DMA_MINALIGN) ;u8 tx_buf[AD4134_SPI_MAX_XFER_LEN ]; };
[ ... ]
Quoted 12 lines
drivers/iio/adc/ad4134.c +2
-2@@ -269,8 +284,8 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,* interface. Now we read data from all channels but keep only the bits * from the requested one. */ -for (i =0 ; i <ARRAY_SIZE (ad4134_chan_set); i++) { - ret =spi_write_then_read (st->spi,NULL ,0 , st->rx_buf, +for (i =0 ; i <AD4134_NUM_CHANNELS ; i++) { + ret =spi_write_then_read (st->spi,NULL ,0 , st->rx_buf.reg,BITS_TO_BYTES (AD4134_CHAN_PRECISION_BITS ));if (ret)return ret;
drivers/iio/adc/ad4134.c static int ad4134_data_read(struct ad4134_state *st, unsigned int reg, · line 301
Quoted 7 lines
drivers/iio/adc/ad4134.c +1
-1@@ -281,7 +296,7 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,* Clock out data from all channels to avoid that. */ if (i ==AD4134_VREG_CH (reg)) - sample =get_unaligned_be24 (st->rx_buf); + sample =get_unaligned_be24 (st->rx_buf.reg); } *val =sign_extend32 (sample,AD4134_CHAN_PRECISION_BITS -1 );
[Severity: Medium]
Does this leak uninitialized stack memory if the soft timestamp channel is read as a virtual register?
Because ad4134_regmap_config sets max_register to include the soft timestamp channel (ARRAY_SIZE(ad4134_chan_set) includes the timestamp, making index 4 valid), userspace could read virtual register index 4 via regmap debugfs. In that case, this loop iterates only up to AD4134_NUM_CHANNELS (indices 0 to 3), meaning i == AD4134_VREG_CH(reg) (where AD4134_VREG_CH(reg) would be 4) is never true. The sample variable is left uninitialized, and an uninitialized 32-bit stack chunk is processed and returned to userspace.
[ ... ]
Quoted 14 lines
drivers/iio/adc/ad4134.c +10
-0@@ -333,6 +348,37 @@ static const struct regmap_config ad4134_regmap_config = { .max_register = AD4134_CH_VREG(ARRAY_SIZE(ad4134_chan_set)), }; +static irqreturn_t ad4134_trigger_handler (int irq,void *p) +{ +struct iio_poll_func *pf = p; +struct iio_dev *indio_dev = pf->indio_dev; +struct ad4134_state *st =iio_priv (indio_dev); +unsigned int i =0 ; +int ret; + +gpiod_set_value_cansleep (st->odr_gpio,1 ); +fsleep (1 );
drivers/iio/adc/ad4134.c static const struct regmap_config ad4134_regmap_config = { · line 370
Quoted 10 lines
drivers/iio/adc/ad4134.c +10
-0+gpiod_set_value_cansleep (st->odr_gpio,0 ); + +for (unsigned int ch =0 ; ch <AD4134_NUM_CHANNELS ; ch++) { + ret =spi_write_then_read (st->spi,NULL ,0 , &st->rx_buf.scan[ch], +BITS_TO_BYTES (AD4134_CHAN_PRECISION_BITS )); +if (ret) + goto err_out; + +if (test_bit (ch, indio_dev->active_scan_mask) && ch != i) + memcpy(&st->rx_buf.scan[i++], &st->rx_buf.scan[ch],
[Severity: High]
Does this conditional logic permanently corrupt the scan data by overwriting active channels?
When the first active channel has the same index as the destination (for example, both are 0), the condition ch != i evaluates to false, short-circuiting the if statement and entirely skipping the post-increment i++. The next active channel will then overwrite the previous data by being written to scan[0] again. This triggers on any standard buffered capture involving multiple channels.
Quoted 10 lines
drivers/iio/adc/ad4134.c +10
-0+ sizeof(st->rx_buf.scan[ch])); + } + +iio_push_to_buffers_with_ts (indio_dev, &st->rx_buf.scan, +sizeof (st->rx_buf.scan), pf->timestamp); + +err_out: +iio_trigger_notify_done (indio_dev->trig); +return IRQ_HANDLED ; +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
Jonathan Cameron
maintainerRe: [PATCH v2 08/15] iio: adc: ad4134: Support buffered data read
In reply to Marcelo SchmittQuoted 7 lines
Enable users to run buffered data captures triggered by IIO trigger device. Add an IIO timestamp channel so each data scan is provided with measurement time information. Require single-read operations to be in IIO device direct access mode to prevent buffered and single-shot data captures to disrupt each other.
Signed-off-by Marcelo Schmitt <[email protected]>
Hi Marcelo
Sashiko had a bunch of feedback on this one. Note that you can save time if you check out what it comes up with and reply to the thread to say what is valid and not.
I haven't yet asked for those emails to go directly to linux-iio as the quality is mixed and I am reluctant to confuse newcomers, but most like I will ask them to turn those emails on in a month or so if not before. In meantime more experienced folk can look at the website directly.
Quoted 8 lines
drivers/iio/adc/ad4134.c +0
-0struct ad4134_state {
drivers/iio/adc/ad4134.c struct ad4134_state { · line 166
Quoted 7 lines
drivers/iio/adc/ad4134.c +3
-1@@ -149,7 +161,10 @@ struct ad4134_state {* DMA (thus cache coherency maintenance) requires the transfer buffers * to live in their own cache lines. */ -u8 rx_buf[AD4134_SPI_MAX_XFER_LEN ]__aligned(IIO_DMA_MINALIGN) ; +union { +u8 reg[AD4134_SPI_MAX_XFER_LEN ]; +u32 scan[ARRAY_SIZE (ad4134_chan_set)];
Sashiko:
[Severity: High]
Does this array size fail to account for the timestamp's alignment padding?
The array is fixed at 20 bytes (5 elements). When all 4 channels and the 64-bit timestamp are active, the IIO core computes scan_bytes as 24 bytes due to the timestamp's 8-byte alignment requirement (16 bytes data + 8 bytes padding and timestamp). Passing sizeof(st->rx_buf.scan) to iio_push_to_buffers_with_ts() later in the trigger handler will cause the IIO core's safety check to fail, aborting the buffer push with -ENOSPC on full-scan captures.
-
That does indeed look wrong. I'd expect to see one of the DECLARE buffer macros used here to ensure a large enough buffer. This should have shown up in testing so I'm curious why it didn't.
[Severity: High]
Can concurrent lockless accesses corrupt the rx_buf shared union?
The ad4134_trigger_handler() executes locklessly in a pollfunc context, while regmap debugfs reads/writes (which do not acquire the IIO direct mode lock) can concurrently use st->rx_buf.reg as a DMA buffer. This can result in corrupted SPI scan data, corrupted regmap read/write data, and potentially invalid hardware configurations or ADC samples if debugfs is accessed while a triggered buffer capture is running.
-
Also sounds valid though I haven't confirmed everything stated. A buffer like this should be protected by a local lock, not rely on serialization via the mode claim mechanism.
drivers/iio/adc/ad4134.c static int ad4134_data_read(struct ad4134_state *st, unsigned int reg, · line 301
Quoted 13 lines
drivers/iio/adc/ad4134.c +2
-1+ } rx_buf__aligned(IIO_DMA_MINALIGN) ;u8 tx_buf[AD4134_SPI_MAX_XFER_LEN ]; };@@ -281,7 +296,7 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,* Clock out data from all channels to avoid that. */ if (i ==AD4134_VREG_CH (reg)) - sample =get_unaligned_be24 (st->rx_buf); + sample =get_unaligned_be24 (st->rx_buf.reg); } *val =sign_extend32 (sample,AD4134_CHAN_PRECISION_BITS -1 );
[Severity: Medium]
Does this leak uninitialized stack memory if the soft timestamp channel is read as a virtual register?
Because ad4134_regmap_config sets max_register to include the soft timestamp channel (ARRAY_SIZE(ad4134_chan_set) includes the timestamp, making index 4 valid), userspace could read virtual register index 4 via regmap debugfs. In that case, this loop iterates only up to AD4134_NUM_CHANNELS (indices 0 to 3), meaning i == AD4134_VREG_CH(reg) (where AD4134_VREG_CH(reg) would be 4) is never true. The sample variable is left uninitialized, and an uninitialized 32-bit stack chunk is processed and returned to userspace.
-
Please check this one.
Quoted 18 lines
drivers/iio/adc/ad4134.c +10
-0@@ -333,6 +348,37 @@ static const struct regmap_config ad4134_regmap_config = { .max_register = AD4134_CH_VREG(ARRAY_SIZE(ad4134_chan_set)), }; +static irqreturn_t ad4134_trigger_handler (int irq,void *p) +{ +struct iio_poll_func *pf = p; +struct iio_dev *indio_dev = pf->indio_dev; +struct ad4134_state *st =iio_priv (indio_dev); +unsigned int i =0 ; +int ret; + +gpiod_set_value_cansleep (st->odr_gpio,1 ); +fsleep (1 );
drivers/iio/adc/ad4134.c static const struct regmap_config ad4134_regmap_config = { · line 370
Quoted 10 lines
drivers/iio/adc/ad4134.c +10
-0+gpiod_set_value_cansleep (st->odr_gpio,0 ); + +for (unsigned int ch =0 ; ch <AD4134_NUM_CHANNELS ; ch++) { + ret =spi_write_then_read (st->spi,NULL ,0 , &st->rx_buf.scan[ch], +BITS_TO_BYTES (AD4134_CHAN_PRECISION_BITS )); +if (ret) + goto err_out; + +if (test_bit (ch, indio_dev->active_scan_mask) && ch != i) + memcpy(&st->rx_buf.scan[i++], &st->rx_buf.scan[ch],
[Severity: High]
Does this conditional logic permanently corrupt the scan data by overwriting active channels?
When the first active channel has the same index as the destination (for example, both are 0), the condition ch != i evaluates to false, short-circuiting the if statement and entirely skipping the post-increment i++. The next active channel will then overwrite the previous data by being written to scan[0] again. This triggers on any standard buffered capture involving multiple channels. -
Check this one as well. Looks plausible to me.
Signature · 1 line
Jonathan Cameron <[email protected]>
Re: [PATCH v2 08/15] iio: adc: ad4134: Support buffered data read
Hi Jonathan,
Quoted 6 lines · Jonathan Cameron
Quoted 6 lines
Enable users to run buffered data captures triggered by IIO trigger device. Add an IIO timestamp channel so each data scan is provided with measurement time information. Require single-read operations to be in IIO device direct access mode to prevent buffered and single-shot data captures to disrupt each other.
Quoted 7 lines
Quoted 1 line
Signed-off-by Marcelo Schmitt <[email protected]>Hi Marcelo
Sashiko had a bunch of feedback on this one. Note that you can save time if you check out what it comes up with and reply to the thread to say what is valid and not.
Yes, I've been working on fixes for the issues that seem legitimate. Most of them do seem to make sense. I was to reply only to the ones that seem off, but didn't yet manage to fully assess them all. Will start providing preliminary reply to automated reviews.
Quoted 36 lines
I haven't yet asked for those emails to go directly to linux-iio as the quality is mixed and I am reluctant to confuse newcomers, but most like I will ask them to turn those emails on in a month or so if not before. In meantime more experienced folk can look at the website directly.
Quoted 8 lines
drivers/iio/adc/ad4134.c +0
-0struct ad4134_state {drivers/iio/adc/ad4134.c struct ad4134_state { · line 166
Quoted 7 lines
drivers/iio/adc/ad4134.c +3
-1@@ -149,7 +161,10 @@ struct ad4134_state {* DMA (thus cache coherency maintenance) requires the transfer buffers * to live in their own cache lines. */ -u8 rx_buf[AD4134_SPI_MAX_XFER_LEN ]__aligned(IIO_DMA_MINALIGN) ; +union { +u8 reg[AD4134_SPI_MAX_XFER_LEN ]; +u32 scan[ARRAY_SIZE (ad4134_chan_set)];Sashiko:
[Severity: High]
Does this array size fail to account for the timestamp's alignment padding?The array is fixed at 20 bytes (5 elements). When all 4 channels and the 64-bit timestamp are active, the IIO core computes
scan_bytesas 24 bytes due to the timestamp's 8-byte alignment requirement (16 bytes data + 8 bytes padding and timestamp). Passingsizeof(st->rx_buf.scan)toiio_push_to_buffers_with_ts()later in the trigger handler will cause the IIO core's safety check to fail, aborting the buffer push with-ENOSPCon full-scan captures.
-
Quoted 3 lines
That does indeed look wrong. I'd expect to see one of the DECLARE buffer macros used here to ensure a large enough buffer. This should have shown up in testing so I'm curious why it didn't.
I had initially tested with only one or two channels enabled while requesting timestamp. This issue is going to be fixed in v3.
Quoted 11 lines
[Severity: High]
Can concurrent lockless accesses corrupt therx_bufshared union?The
ad4134_trigger_handler()executes locklessly in a pollfunc context, while regmap debugfs reads/writes (which do not acquire the IIO direct mode lock) can concurrently usest->rx_buf.regas a DMA buffer. This can result in corrupted SPI scan data, corrupted regmap read/write data, and potentially invalid hardware configurations or ADC samples if debugfs is accessed while a triggered buffer capture is running.
Quoted 4 lines
-
Also sounds valid though I haven't confirmed everything stated. A buffer like this should be protected by a local lock, not rely on serialization via the mode claim mechanism.
Yes, that's a valid one that is going to be fixed.
Quoted 19 lines
Quoted 7 lines
drivers/iio/adc/ad4134.c +1
-0+ } rx_buf__aligned(IIO_DMA_MINALIGN) ;u8 tx_buf[AD4134_SPI_MAX_XFER_LEN ]; };drivers/iio/adc/ad4134.c static int ad4134_data_read(struct ad4134_state *st, unsigned int reg, · line 301
Quoted 7 lines
drivers/iio/adc/ad4134.c +1
-1@@ -281,7 +296,7 @@ static int ad4134_data_read(struct ad4134_state *st, unsigned int reg,* Clock out data from all channels to avoid that. */ if (i ==AD4134_VREG_CH (reg)) - sample =get_unaligned_be24 (st->rx_buf); + sample =get_unaligned_be24 (st->rx_buf.reg); } *val =sign_extend32 (sample,AD4134_CHAN_PRECISION_BITS -1 );[Severity: Medium]
Does this leak uninitialized stack memory if the soft timestamp channel is read as a virtual register?
Quoted 11 lines
Because
ad4134_regmap_configsetsmax_registerto include the soft timestamp channel (ARRAY_SIZE(ad4134_chan_set)includes the timestamp, making index 4 valid), userspace could read virtual register index 4 via regmap debugfs. In that case, this loop iterates only up toAD4134_NUM_CHANNELS(indices 0 to 3), meaningi == AD4134_VREG_CH(reg)(whereAD4134_VREG_CH(reg)would be 4) is never true. The sample variable is left uninitialized, and an uninitialized 32-bit stack chunk is processed and returned to userspace.
-
Please check this one.Quoted 1 line
Ack
Quoted 31 lines
Quoted 16 lines
drivers/iio/adc/ad4134.c +10
-0@@ -333,6 +348,37 @@ static const struct regmap_config ad4134_regmap_config = { .max_register = AD4134_CH_VREG(ARRAY_SIZE(ad4134_chan_set)), }; +static irqreturn_t ad4134_trigger_handler (int irq,void *p) +{ +struct iio_poll_func *pf = p; +struct iio_dev *indio_dev = pf->indio_dev; +struct ad4134_state *st =iio_priv (indio_dev); +unsigned int i =0 ; +int ret; + +gpiod_set_value_cansleep (st->odr_gpio,1 ); +fsleep (1 );drivers/iio/adc/ad4134.c static const struct regmap_config ad4134_regmap_config = { · line 370
Quoted 10 lines
drivers/iio/adc/ad4134.c +10
-0+gpiod_set_value_cansleep (st->odr_gpio,0 ); + +for (unsigned int ch =0 ; ch <AD4134_NUM_CHANNELS ; ch++) { + ret =spi_write_then_read (st->spi,NULL ,0 , &st->rx_buf.scan[ch], +BITS_TO_BYTES (AD4134_CHAN_PRECISION_BITS )); +if (ret) + goto err_out; + +if (test_bit (ch, indio_dev->active_scan_mask) && ch != i) + memcpy(&st->rx_buf.scan[i++], &st->rx_buf.scan[ch],[Severity: High]
Does this conditional logic permanently corrupt the scan data by overwriting active channels?
Quoted 8 lines
When the first active channel has the same index as the destination (for example, both are 0), the condition
ch != ievaluates to false, short-circuiting the if statement and entirely skipping the post-incrementi++. The next active channel will then overwrite the previous data by being written to scan[0] again. This triggers on any standard buffered capture involving multiple channels. -Check this one as well. Looks plausible to me.
Ack.
Thanks,
Marcelo
Marcelo Schmitt
maintainerPATCH v2 9/15 dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode
In reply to Marcelo SchmittDocument how AD4134 chips are connected to the host SPI controller according to different wiring configurations.
---
Change log v1 -> v2:
- Set default for adi,spi-mode property.
- Added '#daisy-chained-devices' property.
- Documented constraints for adi,spi-mode + #daisy-chained-devices combinations.
.../bindings/iio/adc/adi,ad4134.yaml | 51 ++++++++++++++++--- 1 file changed, 44 insertions(+), 7 deletions(-)
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +44 -7
@@ -131,6 +131,26 @@ properties: enum: [ free-running, gated ] default: gated + adi,spi-mode: + $ref: /schemas/types.yaml#/definitions/string + enum: [ no-cs, 4-wire, one-channel-chain, two-channel-chain ] + default: no-cs + description: | + This property indicates the SPI wiring configuration. + * no-cs: The datasheet calls this "minimum I/O mode". In this mode, the CS + line is not used (tied to GND). + * 4-wire: Specifies a conventional SPI wiring configuration. + * one-channel-chain: In this mode, the DOUT0 line of one chip is connected + to DOUT2 of the next chip upstream in the chain. Only the data interface + of the upmost device in the stream is in direct connection with the + digital host. + * two-channel-chain: In this mode, the DOUT0 and DOUT1 lines of one chip + are respectively connected to DOUT2 and DOUT3 of the next chip upstream + in the chain. Only the data interface of the upmost device in the stream + is in direct connection with the digital host. + + '#daisy-chained-devices': true + required: - compatible - reg
@@ -141,13 +161,30 @@ required: - clocks - clock-names -oneOf: - - required: - - ldoin-supply - - required: - - avdd1v8-supply - - dvdd1v8-supply - - clkvdd-supply +allOf: + - oneOf: + - required: + - ldoin-supply + - required: + - avdd1v8-supply + - dvdd1v8-supply + - clkvdd-supply + + - oneOf: + - required: [adi,spi-mode, '#daisy-chained-devices'] + properties: + adi,spi-mode: + const: one-channel-chain + spi-rx-bus-width: + const: <1> + - required: [adi,spi-mode, '#daisy-chained-devices'] + properties: + adi,spi-mode: + const: two-channel-chain + spi-rx-bus-width: + const: <1>, <1> + - properties: + '#daisy-chained-devices': false unevaluatedProperties: false
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v2 09/15] dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] dt-bindings: invalid DTS syntax in spi-rx-bus-width const constraints - [Medium] dt-bindings…
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] dt-bindings: invalid DTS syntax in spi-rx-bus-width const constraints
- [Medium] dt-bindings: schema allows bypassing daisy-chained-devices requirement --
commit 879c8beee9c21345a2925282a985c10001e710eb
Author: Marcelo Schmitt <[email protected]>
dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode
Document how AD4134 chips are connected to the host SPI controller according to different wiring configurations.
Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +0
-0
[ ... ]
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml required: · line 179
Quoted 7 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +7
-0+ - oneOf: + - required: [adi,spi-mode, '#daisy-chained-devices'] + properties: + adi,spi-mode: + const: one-channel-chain + spi-rx-bus-width: + const: <1>
[Severity: Medium]
Will this assignment of <1> to const cause a type mismatch during schema validation?
Since the YAML parser evaluates <1> as a literal string instead of an integer array, it appears that when a device tree specifies a mode like adi,spi-mode = "one-channel-chain" and provides a valid integer for spi-rx-bus-width = <1>, it will unconditionally fail validation.
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml required: · line 185
Quoted 6 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +6
-0+ - required: [adi,spi-mode, '#daisy-chained-devices'] + properties: + adi,spi-mode: + const: two-channel-chain + spi-rx-bus-width: + const: <1>, <1>
[Severity: Medium]
Does this string literal assignment also trigger a type mismatch when validating a two-channel-chain configuration?
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml required: · line 187
Quoted 2 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +2
-0+ - properties: + '#daisy-chained-devices': false
[Severity: Medium]
Can the fallback branch in this oneOf block inadvertently bypass the required constraints?
If a device tree completely omits the #daisy-chained-devices property, this catch-all branch seems to trivially evaluate to true, because property constraints in JSON Schema only evaluate if the property is actually present.
If the branch evaluates to true, the outer oneOf block succeeds. This appears to circumvent the mandatory #daisy-chained-devices requirement, allowing incomplete configurations to pass validation when adi,spi-mode is set to one of the chain modes.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=9
Rob Herring (Arm)
maintainerRe: [PATCH v2 09/15] dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode
In reply to Marcelo SchmittQuoted 4 lines · Marcelo Schmitt
Document how AD4134 chips are connected to the host SPI controller according to different wiring configurations.
Signed-off-by Marcelo Schmitt <[email protected]>
Quoted 9 lines
---
Change log v1 -> v2:
- Set default for adi,spi-mode property.
- Added '#daisy-chained-devices' property.
- Documented constraints for adi,spi-mode + #daisy-chained-devices combinations..../bindings/iio/adc/adi,ad4134.yaml | 51 ++++++++++++++++--- 1 file changed, 44 insertions(+), 7 deletions(-)
My bot found errors running 'make dt_binding_check' on your patch:
yamllint warnings/errors:./Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml:174:24: [warning] too few spaces after comma (commas) ./Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml:180:24: [warning] too few spaces after comma (commas)
dtschema/dtc warnings/errors:
doc reference errors (make refcheckdocs):
The base for the series is generally the latest rc1. A different dependency should be noted in *this* patch.
If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date:
pip3 install dtschema --upgrade
Please check and re-submit after running the above command yourself. Note that DT_SCHEMA_FILES can be set to your schema file to speed up checking your schema. However, it must be unset to test all examples with your schema.
Rob Herring
maintainerRe: [PATCH v2 09/15] dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode
In reply to Marcelo SchmittQuoted 20 lines · Marcelo Schmitt
Document how AD4134 chips are connected to the host SPI controller according to different wiring configurations.
Signed-off-by Marcelo Schmitt <[email protected]>---
Change log v1 -> v2:
- Set default for adi,spi-mode property.
- Added '#daisy-chained-devices' property.
- Documented constraints for adi,spi-mode + #daisy-chained-devices combinations..../bindings/iio/adc/adi,ad4134.yaml | 51 ++++++++++++++++--- 1 file changed, 44 insertions(+), 7 deletions(-)Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +0
-0@@ -131,6 +131,26 @@ properties: enum: [ free-running, gated ] default: gated
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml properties: · line 142
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +9
-0+ adi,spi-mode: + $ref: /schemas/types.yaml#/definitions/string + enum: [ no-cs, 4-wire, one-channel-chain, two-channel-chain ] + default: no-cs + description: | + This property indicates the SPI wiring configuration. + * no-cs: The datasheet calls this "minimum I/O mode". In this mode, the CS + line is not used (tied to GND). + * 4-wire: Specifies a conventional SPI wiring configuration.
Absence of this property should be conventional mode.
Quoted 8 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +8
-0+ * one-channel-chain: In this mode, the DOUT0 line of one chip is connected + to DOUT2 of the next chip upstream in the chain. Only the data interface + of the upmost device in the stream is in direct connection with the + digital host. + * two-channel-chain: In this mode, the DOUT0 and DOUT1 lines of one chip + are respectively connected to DOUT2 and DOUT3 of the next chip upstream + in the chain. Only the data interface of the upmost device in the stream + is in direct connection with the digital host.
Jonathan Cameron
maintainerRe: [PATCH v2 09/15] dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode
Quoted 22 lines · On Wed, 16 Sep 2026 11:00:25 -0500 Rob Herring <[email protected]> wrote:
Quoted 21 lines · Marcelo Schmitt
Document how AD4134 chips are connected to the host SPI controller according to different wiring configurations.
Signed-off-by Marcelo Schmitt <[email protected]>---
Change log v1 -> v2:
- Set default for adi,spi-mode property.
- Added '#daisy-chained-devices' property.
- Documented constraints for adi,spi-mode + #daisy-chained-devices combinations..../bindings/iio/adc/adi,ad4134.yaml | 51 ++++++++++++++++--- 1 file changed, 44 insertions(+), 7 deletions(-)Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +0
-0@@ -131,6 +131,26 @@ properties: enum: [ free-running, gated ] default: gated
Quoted 11 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml properties: · line 142
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +9
-0+ adi,spi-mode: + $ref: /schemas/types.yaml#/definitions/string + enum: [ no-cs, 4-wire, one-channel-chain, two-channel-chain ] + default: no-cs + description: | + This property indicates the SPI wiring configuration. + * no-cs: The datasheet calls this "minimum I/O mode". In this mode, the CS + line is not used (tied to GND). + * 4-wire: Specifies a conventional SPI wiring configuration.Absence of this property should be conventional mode.
Hi Rob,
Unfortunately we missed this in the binding originally so we are left with the default of what the driver did at the time.
I wouldn't be against merging a change in that default as a fix though if we don't think this is yet in heavy use (merged in 7.0, but given not a complete feature set yet, maybe?)
Jonathan
Quoted 9 lines
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +8
-0+ * one-channel-chain: In this mode, the DOUT0 line of one chip is connected + to DOUT2 of the next chip upstream in the chain. Only the data interface + of the upmost device in the stream is in direct connection with the + digital host. + * two-channel-chain: In this mode, the DOUT0 and DOUT1 lines of one chip + are respectively connected to DOUT2 and DOUT3 of the next chip upstream + in the chain. Only the data interface of the upmost device in the stream + is in direct connection with the digital host.
Rob Herring
maintainerRe: [PATCH v2 09/15] dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode
Quoted 36 lines · Jonathan Cameron
Quoted 22 lines · On Wed, 16 Sep 2026 11:00:25 -0500 Rob Herring <[email protected]> wrote:
Quoted 21 lines · Marcelo Schmitt
Document how AD4134 chips are connected to the host SPI controller according to different wiring configurations.
Signed-off-by Marcelo Schmitt <[email protected]>---
Change log v1 -> v2:
- Set default for adi,spi-mode property.
- Added '#daisy-chained-devices' property.
- Documented constraints for adi,spi-mode + #daisy-chained-devices combinations..../bindings/iio/adc/adi,ad4134.yaml | 51 ++++++++++++++++--- 1 file changed, 44 insertions(+), 7 deletions(-)Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +0
-0@@ -131,6 +131,26 @@ properties: enum: [ free-running, gated ] default: gatedQuoted 11 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml properties: · line 142
Quoted 9 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +9
-0+ adi,spi-mode: + $ref: /schemas/types.yaml#/definitions/string + enum: [ no-cs, 4-wire, one-channel-chain, two-channel-chain ] + default: no-cs + description: | + This property indicates the SPI wiring configuration. + * no-cs: The datasheet calls this "minimum I/O mode". In this mode, the CS + line is not used (tied to GND). + * 4-wire: Specifies a conventional SPI wiring configuration.Absence of this property should be conventional mode.
Quoted 8 lines
Hi Rob,
Unfortunately we missed this in the binding originally so we are left with the default of what the driver did at the time.
I wouldn't be against merging a change in that default as a fix though if we don't think this is yet in heavy use (merged in 7.0, but given not a complete feature set yet, maybe?)
Ok, it's fine then. Just make sure the commit msg has this detail.
Rob
Re: [PATCH v2 09/15] dt-bindings: iio: adc: adi,ad4134: Document SPI connection mode
Quoted 37 lines · Rob Herring
Quoted 36 lines · Jonathan Cameron
Quoted 33 lines · On Wed, 16 Sep 2026 11:00:25 -0500 Rob Herring <[email protected]> wrote:
Quoted 20 lines · Marcelo Schmitt
Document how AD4134 chips are connected to the host SPI controller according to different wiring configurations.
Signed-off-by Marcelo Schmitt <[email protected]>---
Change log v1 -> v2:
- Set default for adi,spi-mode property.
- Added '#daisy-chained-devices' property.
- Documented constraints for adi,spi-mode + #daisy-chained-devices combinations..../bindings/iio/adc/adi,ad4134.yaml | 51 ++++++++++++++++--- 1 file changed, 44 insertions(+), 7 deletions(-)Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +0
-0@@ -131,6 +131,26 @@ properties: enum: [ free-running, gated ] default: gatedDocumentation/devicetree/bindings/iio/adc/adi,ad4134.yaml properties: · line 142
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +9
-0+ adi,spi-mode: + $ref: /schemas/types.yaml#/definitions/string + enum: [ no-cs, 4-wire, one-channel-chain, two-channel-chain ] + default: no-cs + description: | + This property indicates the SPI wiring configuration. + * no-cs: The datasheet calls this "minimum I/O mode". In this mode, the CS + line is not used (tied to GND). + * 4-wire: Specifies a conventional SPI wiring configuration.Absence of this property should be conventional mode.
Quoted 9 lines
Quoted 8 lines
Hi Rob,
Unfortunately we missed this in the binding originally so we are left with the default of what the driver did at the time.
I wouldn't be against merging a change in that default as a fix though if we don't think this is yet in heavy use (merged in 7.0, but given not a complete feature set yet, maybe?)
I don't believe this part is being extensively used. Going with the conventional 4-wire default is the more natural choice so I'm growing my TODO list with a note for a follow up patch to update this default.
Quoted 1 line
Ok, it's fine then. Just make sure the commit msg has this detail.
Ok, will add.
Thanks,
Marcelo
Marcelo Schmitt
maintainerPATCH v2 10/15 dt-bindings: iio: adc: adi,ad4134: Document external multiplexer usage
In reply to Marcelo SchmittThe AD4134 design has two data interfaces. One interface allows register access for device configuration while the other (separate interface) provides ADC sample data. One way of handling both peripheral interfaces is to merge them into a single SPI interface by switching between register access and sample access according to device user requests. Though, such solution requires extra hardware, external to the ADC chip. The access mode switch can be done with an external multiplexer selecting either AD4134 SDO or AD4134 DOUT0 to connect to the controller. The external multiplexer becomes part of hardware requested for AD4134 device operation and thus must be provided for operating the peripheral in such merged interface schema. Still, there are alternative ways of handling the two AD4134 data interfaces so the multiplexer is not always required.
---
New patch.
Detailed reasoning for the external multiplexer usage.
Before coming to the current solution, the following configuration was tried.
+-----------------------+ +-----------------+
| AD4134 | | SPI Controller |
| | | |
| SPI interface | | |
| for register SCLK |<--------------------------| SCLK |
| access CS |<--------------------------| CS |
| SDI |<--------------------------| SDO |
| SDO |---+ | |
| | | | |
| Data interface DOUT0 |---+---------------------->| SDI0 |
| for ADC data DOUT1 |-------------------------->| SDI1 |
| read back DOUT2 |-------------------------->| SDI2 |
| DOUT3 |-------------------------->| SDI3 |
| DCLK |<--------------------------| DCLK
| ODR |<------------------+ +->| Offload Trigger |
+-----------------------+ | | +-----------------+
| +--| PWM1 |
+-------| PWM0 |
+-------| GPIO |
+-----------------+Though, because DOUT0 never goes high-Z, the DOUT0 pin keeps driving the data line, causing register reads to fail.
Alternatively, we could have something like
+-----------------------+ +-----------------+
| AD4134 | | SPI Controller |
| | | |
| SPI interface | | |
| for register SCLK |<--------------------------| SCLK |
| access CS |<--------------------------| CS |
| SDI |<--------------------------| SDO |
| SDO |-------------------------->| SDI0 |
| | | |
| Data interface DOUT0 |-------------------------->| SDI1 |
| for ADC data DOUT1 |-------------------------->| SDI2 |
| read back DOUT2 |-------------------------->| SDI3 |
| DOUT3 |-------------------------->| SDI4 |
| DCLK |<--------------------------| DCLK
| ODR |<------------------+ +->| Offload Trigger |
+-----------------------+ | | +-----------------+
| +--| PWM1 |
+-------| PWM0 |
+-------| GPIO |
+-----------------+The downside of the above is the peripheral would need fine-grained config of controller SDI lines to only read SDI0 for register access, and only read SDI1, SDI2, SDI3, SDI4 for ADC sample data (currently available SPI_MULTI_LANE_MODE_STRIPE reads from all SDI lines).
The currently proposed solution looks like the following
+-----------------------+ +-----------------+
| AD4134 | | SPI Controller |
| | | |
| SPI interface SCLK |<------------------------| SCLK |
| for register CS |<------------------------| CS |
| access SDI |<------------------------| SDO |
| SDO |------->|¯¯¯¯\ | |
| | |MUX >--------->| SDI0 |
| Data interface DOUT0 |------->|____/ | |
| for ADC sample | ^ | |
| data read DOUT1 |------------------------>| SDI1 |
| DOUT2 |------------------------>| SDI2 |
| DOUT3 |------------------------>| SDI3 |
| DCLK |<------------------------| DCLK |
| ODR |<----------------+ +->| Offload Trigger |
+-----------------------+ | | | +-----------------+
| | +--| PWM1 |
| +-------| PWM0 |
| +-------| GPIO0 |
+--------------| GPIO1 |
+-----------------+By being able to mux between AD4134 SDO and AD4134 DOUT0, the peripheral can be connected to a single bus such that controllers able to read from multiple lines will be able to gather ADC sample data from all SDI lines (SPI_MULTI_LANE_MODE_STRIPE). With that, AD4134 maximum data throughput can be supported with what is already available from the SPI core. See additional details on the AD4134 IIO documentation (patch 15).
Yet another possibility would be to have the peripheral connecting to two buses.
+-----------------------+ +-----------------+
| AD4134 | | SPI Controller A|
| | | |
| SPI interface SCLK |<--------------------------| SCLK |
| for register CS |<--------------------------| CS |
| access SDI |<--------------------------| SDO |
| SDO |-------------------------->| SDI |
| | +-----------------+
| | | SPI Controller B|
| | | |
| Data interface DOUT0 |-------------------------->| SDI0 |
| for ADC sample DOUT1 |-------------------------->| SDI1 |
| data read DOUT2 |-------------------------->| SDI2 |
| DOUT3 |-------------------------->| SDI3 |
| DCLK |<--------------------------| DCLK |
| ODR |<------------------+ +->| Offload Trigger |
+-----------------------+ | | +-----------------+
| +--| PWM1 |
+-------| PWM0 |
+-------| GPIO |
+-----------------+That may be a fallback option if what's currently being proposed fails to comply to Linux code standards.
.../devicetree/bindings/iio/adc/adi,ad4134.yaml | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +14 -0
@@ -104,6 +104,20 @@ properties: GPIO connected to ODR pin. Used to sample ADC data in minimum I/O mode. maxItems: 1 + mux-states: + description: + mux controller node to route SDO and DOUT0 signals from peripheral to + controller. + maxItems: 2 + + mux-state-names: + description: + mux state names for routing SDO and DOUT0 signals from peripheral to + controller. + items: + - const: data_read + - const: reg_access + adi,asrc-mode: $ref: /schemas/types.yaml#/definitions/string description:
Signature · 1 line
2.53.0
Jonathan Cameron
maintainerRe: [PATCH v2 10/15] dt-bindings: iio: adc: adi,ad4134: Document external multiplexer usage
Quoted 128 lines · On Tue, 15 Sep 2026 16:15:05 -0300 Marcelo Schmitt…
The AD4134 design has two data interfaces. One interface allows register access for device configuration while the other (separate interface) provides ADC sample data. One way of handling both peripheral interfaces is to merge them into a single SPI interface by switching between register access and sample access according to device user requests. Though, such solution requires extra hardware, external to the ADC chip. The access mode switch can be done with an external multiplexer selecting either AD4134 SDO or AD4134 DOUT0 to connect to the controller. The external multiplexer becomes part of hardware requested for AD4134 device operation and thus must be provided for operating the peripheral in such merged interface schema. Still, there are alternative ways of handling the two AD4134 data interfaces so the multiplexer is not always required.
Signed-off-by Marcelo Schmitt <[email protected]>---
New patch.Detailed reasoning for the external multiplexer usage.
Before coming to the current solution, the following configuration was tried.
+-----------------------+ +-----------------+ | AD4134 | | SPI Controller | | | | | | SPI interface | | | | for register SCLK |<--------------------------| SCLK | | access CS |<--------------------------| CS | | SDI |<--------------------------| SDO | | SDO |---+ | | | | | | | | Data interface DOUT0 |---+---------------------->| SDI0 | | for ADC data DOUT1 |-------------------------->| SDI1 | | read back DOUT2 |-------------------------->| SDI2 | | DOUT3 |-------------------------->| SDI3 | | DCLK |<--------------------------| DCLK | ODR |<------------------+ +->| Offload Trigger | +-----------------------+ | | +-----------------+ | +--| PWM1 | +-------| PWM0 | +-------| GPIO | +-----------------+Though, because DOUT0 never goes high-Z, the DOUT0 pin keeps driving the data line, causing register reads to fail.
Alternatively, we could have something like
+-----------------------+ +-----------------+ | AD4134 | | SPI Controller | | | | | | SPI interface | | | | for register SCLK |<--------------------------| SCLK | | access CS |<--------------------------| CS | | SDI |<--------------------------| SDO | | SDO |-------------------------->| SDI0 | | | | | | Data interface DOUT0 |-------------------------->| SDI1 | | for ADC data DOUT1 |-------------------------->| SDI2 | | read back DOUT2 |-------------------------->| SDI3 | | DOUT3 |-------------------------->| SDI4 | | DCLK |<--------------------------| DCLK | ODR |<------------------+ +->| Offload Trigger | +-----------------------+ | | +-----------------+ | +--| PWM1 | +-------| PWM0 | +-------| GPIO | +-----------------+The downside of the above is the peripheral would need fine-grained config of controller SDI lines to only read SDI0 for register access, and only read SDI1, SDI2, SDI3, SDI4 for ADC sample data (currently available
SPI_MULTI_LANE_MODE_STRIPEreads from all SDI lines).The currently proposed solution looks like the following
+-----------------------+ +-----------------+ | AD4134 | | SPI Controller | | | | | | SPI interface SCLK |<------------------------| SCLK | | for register CS |<------------------------| CS | | access SDI |<------------------------| SDO | | SDO |------->|¯¯¯¯\ | | | | |MUX >--------->| SDI0 | | Data interface DOUT0 |------->|____/ | | | for ADC sample | ^ | | | data read DOUT1 |------------------------>| SDI1 | | DOUT2 |------------------------>| SDI2 | | DOUT3 |------------------------>| SDI3 | | DCLK |<------------------------| DCLK | | ODR |<----------------+ +->| Offload Trigger | +-----------------------+ | | | +-----------------+ | | +--| PWM1 | | +-------| PWM0 | | +-------| GPIO0 | +--------------| GPIO1 | +-----------------+By being able to mux between AD4134 SDO and AD4134 DOUT0, the peripheral can be connected to a single bus such that controllers able to read from multiple lines will be able to gather ADC sample data from all SDI lines (
SPI_MULTI_LANE_MODE_STRIPE). With that, AD4134 maximum data throughput can be supported with what is already available from the SPI core. See additional details on the AD4134 IIO documentation (patch 15).Yet another possibility would be to have the peripheral connecting to two buses.
+-----------------------+ +-----------------+ | AD4134 | | SPI Controller A| | | | | | SPI interface SCLK |<--------------------------| SCLK | | for register CS |<--------------------------| CS | | access SDI |<--------------------------| SDO | | SDO |-------------------------->| SDI | | | +-----------------+ | | | SPI Controller B| | | | | | Data interface DOUT0 |-------------------------->| SDI0 | | for ADC sample DOUT1 |-------------------------->| SDI1 | | data read DOUT2 |-------------------------->| SDI2 | | DOUT3 |-------------------------->| SDI3 | | DCLK |<--------------------------| DCLK | | ODR |<------------------+ +->| Offload Trigger | +-----------------------+ | | +-----------------+ | +--| PWM1 | +-------| PWM0 | +-------| GPIO | +-----------------+
Quoted 2 lines
That may be a fallback option if what's currently being proposed fails to comply to Linux code standards.
I'm not against the MUX option but would imagine anyone reading the datasheet is at least someone likely to choose this last version. Still until we have a user for that as far as I am concerned the mux one is fine.
Quoted 16 lines
.../devicetree/bindings/iio/adc/adi,ad4134.yaml | 14 ++++++++++++++ 1 file changed, 14 insertions(+)Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +3
-0@@ -104,6 +104,20 @@ properties: GPIO connected to ODR pin. Used to sample ADC data in minimum I/O mode. maxItems: 1 + mux-states: + description: + mux controller node to route SDO and DOUT0 signals from peripheral to
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml properties: · line 119
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +10
-0+ controller. + maxItems: 2 + + mux-state-names: + description: + mux state names for routing SDO and DOUT0 signals from peripheral to + controller. + items: + - const: data_read + - const: reg_access
Throw in an example.
Jonathan
Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +1
-0+ adi,asrc-mode: $ref: /schemas/types.yaml#/definitions/string description:
Marcelo Schmitt
maintainerPATCH v2 11/15 iio: adc: ad4134: Support SPI 4-wire mode
In reply to Marcelo SchmittAD4134 devices can be wired in a few different ways. So far, only minimum I/O mode was supported. While minimum I/O mode allows interfacing with AD4134 with a reduced number of wires, that wiring configuration is not optimal for high-throughput data acquisition.
Extend AD4134 support to enable interfacing in SPI 4-wire configuration.
---
Change log v1 -> v2:
- Fail on unsupported adi,spi-mode.
- Now properly initializing access mode mutex.
- Highlighted usage of hardware external to AD4134 chip.
- Added comment about mux state switch logic.
- Suggestive name for auxiliary local variables.
- Updated preparation flow for ad4134_data_read().
- Used pahole to reduce memory holes in ad4134_state struct.
Detailed reasoning for the external multiplexer usage.
Before coming to the current solution, the following configuration was tried.
+-----------------------+ +-----------------+
| AD4134 | | SPI Controller |
| | | |
| SPI interface | | |
| for register SCLK |<--------------------------| SCLK |
| access CS |<--------------------------| CS |
| SDI |<--------------------------| SDO |
| SDO |---+ | |
| | | | |
| Data interface DOUT0 |---+---------------------->| SDI0 |
| for ADC data DOUT1 |-------------------------->| SDI1 |
| read back DOUT2 |-------------------------->| SDI2 |
| DOUT3 |-------------------------->| SDI3 |
| DCLK |<--------------------------| DCLK
| ODR |<------------------+ +->| Offload Trigger |
+-----------------------+ | | +-----------------+
| +--| PWM1 |
+-------| PWM0 |
+-------| GPIO |
+-----------------+Though, because DOUT0 never goes high-Z, the DOUT0 pin keeps driving the data line, causing register reads to fail.
Alternatively, we could have something like
+-----------------------+ +-----------------+
| AD4134 | | SPI Controller |
| | | |
| SPI interface | | |
| for register SCLK |<--------------------------| SCLK |
| access CS |<--------------------------| CS |
| SDI |<--------------------------| SDO |
| SDO |-------------------------->| SDI0 |
| | | |
| Data interface DOUT0 |-------------------------->| SDI1 |
| for ADC data DOUT1 |-------------------------->| SDI2 |
| read back DOUT2 |-------------------------->| SDI3 |
| DOUT3 |-------------------------->| SDI4 |
| DCLK |<--------------------------| DCLK
| ODR |<------------------+ +->| Offload Trigger |
+-----------------------+ | | +-----------------+
| +--| PWM1 |
+-------| PWM0 |
+-------| GPIO |
+-----------------+The downside of the above is the peripheral would need fine-grained config of controller SDI lines to only read SDI0 for register access, and only read SDI1, SDI2, SDI3, SDI4 for ADC sample data (currently available SPI_MULTI_LANE_MODE_STRIPE reads from all SDI lines).
The currently proposed solution looks like the following
+-----------------------+ +-----------------+
| AD4134 | | SPI Controller |
| | | |
| SPI interface SCLK |<------------------------| SCLK |
| for register CS |<------------------------| CS |
| access SDI |<------------------------| SDO |
| SDO |------->|¯¯¯¯\ | |
| | |MUX >--------->| SDI0 |
| Data interface DOUT0 |------->|____/ | |
| for ADC sample | ^ | |
| data read DOUT1 |------------------------>| SDI1 |
| DOUT2 |------------------------>| SDI2 |
| DOUT3 |------------------------>| SDI3 |
| DCLK |<------------------------| DCLK |
| ODR |<----------------+ +->| Offload Trigger |
+-----------------------+ | | | +-----------------+
| | +--| PWM1 |
| +-------| PWM0 |
| +-------| GPIO0 |
+--------------| GPIO1 |
+-----------------+By being able to mux between AD4134 SDO and AD4134 DOUT0, the peripheral can be connected to a single bus such that controllers able to read from multiple lines will be able to gather ADC sample data from all SDI lines (SPI_MULTI_LANE_MODE_STRIPE). With that, AD4134 maximum data throughput can be supported with what is already available from the SPI core. See additional details on the AD4134 IIO documentation (patch 15).
Yet another possibility would be to have the peripheral connecting to two buses.
+-----------------------+ +-----------------+
| AD4134 | | SPI Controller A|
| | | |
| SPI interface SCLK |<--------------------------| SCLK |
| for register CS |<--------------------------| CS |
| access SDI |<--------------------------| SDO |
| SDO |-------------------------->| SDI |
| | +-----------------+
| | | SPI Controller B|
| | | |
| Data interface DOUT0 |-------------------------->| SDI0 |
| for ADC sample DOUT1 |-------------------------->| SDI1 |
| data read DOUT2 |-------------------------->| SDI2 |
| DOUT3 |-------------------------->| SDI3 |
| DCLK |<--------------------------| DCLK |
| ODR |<------------------+ +->| Offload Trigger |
+-----------------------+ | | +-----------------+
| +--| PWM1 |
+-------| PWM0 |
+-------| GPIO |
+-----------------+That may be a fallback option if what's currently being proposed fails to comply to Linux code standards.
drivers/iio/adc/Kconfig | 1 + drivers/iio/adc/ad4134.c | 194 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 190 insertions(+), 5 deletions(-)
drivers/iio/adc/Kconfig +1 -0
@@ -127,6 +127,7 @@ config AD4134 select CRC8 select IIO_BUFFER select IIO_TRIGGERED_BUFFER + select MULTIPLEXER help Say yes here to build support for Analog Devices AD4134 SPI analog to digital converters (ADC).
drivers/iio/adc/ad4134.c +189 -5
249 lines not on this page · raw message
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v2 11/15] iio: adc: ad4134: Support SPI 4-wire mode
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] ad4134: ignored mux_state_try_select() errors mask failures and cause bus contention - [High]…
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] ad4134: ignored mux_state_try_select() errors mask failures and cause bus contention
- [High] ad4134: unconditional mux state deselection corrupts multiplexer semaphores
- [Medium] ad4134: concurrent regmap accesses corrupt IIO buffer hardware state
- [Medium] ad4134: direct assignment to spi->mode clobbers devicetree SPI flags --
commit 9b8961eadc30a7239344b617959f3ffedd9a5a63
Author: Marcelo Schmitt <[email protected]>
iio: adc: ad4134: Support SPI 4-wire mode
AD4134 devices can be wired in a few different ways. So far, only minimum I/O mode was supported. While minimum I/O mode allows interfacing with AD4134 with a reduced number of wires, that wiring configuration is not optimal for high-throughput data acquisition.
Extend AD4134 support to enable interfacing in SPI 4-wire configuration.
Quoted 4 lines
drivers/iio/adc/ad4134.c +0
-0
[ ... ]
Quoted 7 lines
drivers/iio/adc/ad4134.c +3
-0@@ -230,6 +252,85 @@ static const struct regmap_access_table ad4134_regmap_wr_table = { .n_yes_ranges = ARRAY_SIZE(ad4134_regmap_wr_range), }; +/* +* This function controls a multiplexer OUTSIDE OF AD4134 SILICON. +* When AD4134 SDO and DOUT0 pins are multiplexed, this function changes the
drivers/iio/adc/ad4134.c static const struct regmap_access_table ad4134_regmap_wr_table = { · line 267
Quoted 10 lines
drivers/iio/adc/ad4134.c +10
-0+* multiplexer state to route SDO to the SPI controller. See AD4134 IIO +* documentation for details. +*/ +static int ad4134_set_register_access (struct ad4134_state *st) +{ +int ret; + +guard (mutex)(&st->access_mode_lock); + + st->spi->mode =SPI_MODE_0 ;
[Severity: Medium]
Do these direct assignments clear all other SPI flags populated by the SPI core from devicetree properties, such as SPI_CS_HIGH, SPI_LSB_FIRST, or SPI_3WIRE?
Should this instead use SPI_MODE_X_MASK to safely update only the CPOL/CPHA bits?
drivers/iio/adc/ad4134.c static const struct regmap_access_table ad4134_regmap_wr_table = { · line 272
Quoted 5 lines
drivers/iio/adc/ad4134.c +5
-0+ ret =spi_setup (st->spi); +if (ret) +return ret; + + ret =mux_state_deselect (st->mux_st[AD4134_DOUT0_INPUT ]);
[Severity: High]
Does this unconditionally deselect the DOUT0 multiplexer state without verifying it is currently selected?
This could call up() on the semaphore and cause a semaphore count imbalance.
drivers/iio/adc/ad4134.c static const struct regmap_access_table ad4134_regmap_wr_table = { · line 283
Quoted 11 lines
drivers/iio/adc/ad4134.c +11
-0+if (ret) +dev_err (&st->spi->dev,"error on DOUT0 deselect: %d\n" , ret); + +/* +* Try to update the multiplexer state to route SDO to the SPI +* controller. If the SDO mux state is BUSY (i.e. locked), that means +* the desired state is already selected and we should not block here. +*/ + ret =mux_state_try_select (st->mux_st[AD4134_SDO_INPUT ]); +if (ret && ret != -EBUSY ) +return ret;
[Severity: High]
Is it safe to explicitly ignore the -EBUSY error here?
If the lock is actually held by another driver sharing the multiplexer, proceeding could result in doing SPI transfers on a misrouted bus, leading to contention and corrupted data transfers.
Quoted 9 lines
drivers/iio/adc/ad4134.c +9
-0+ +return 0 ; +} + +/* +* This function controls a multiplexer OUTSIDE OF AD4134 SILICON. +* When AD4134 SDO and DOUT0 pins are multiplexed, this function changes the +* multiplexer state to route DOUT0 to the SPI controller. On failure, fall +* back to routing SDO to the controller and return an errno. See AD4134 IIO
drivers/iio/adc/ad4134.c static const struct regmap_access_table ad4134_regmap_wr_table = { · line 302
Quoted 10 lines
drivers/iio/adc/ad4134.c +10
-0+* documentation for details. +*/ +static int ad4134_set_sample_access (struct ad4134_state *st) +{ +struct device *dev = &st->spi->dev; +int mux_state_ret, ret; + +guard (mutex)(&st->access_mode_lock); + + ret =mux_state_deselect (st->mux_st[AD4134_SDO_INPUT ]);
[Severity: High]
Is it safe to manually unlock the devres-managed mux state unconditionally here?
The SDO mux is acquired using devm_mux_state_get_selected() during probe, which registers a devm cleanup to deselect it on unbind. Manually unlocking it could risk a semaphore imbalance when devm cleanup runs.
drivers/iio/adc/ad4134.c static const struct regmap_access_table ad4134_regmap_wr_table = { · line 310
Quoted 8 lines
drivers/iio/adc/ad4134.c +8
-0+if (ret) +dev_err (&st->spi->dev,"error on SDO deselect: %d\n" , ret); + + ret =mux_state_try_select (st->mux_st[AD4134_DOUT0_INPUT ]); +if (ret) { +dev_err (&st->spi->dev,"error on DOUT0 select: %d\n" , ret); +return mux_state_select (st->mux_st[AD4134_SDO_INPUT ]); + }
[Severity: High]
If DOUT0 selection fails, does returning the success code of the recovery function (mux_state_select) swallow the original error code?
This might allow IIO buffer enablement to succeed while the hardware is incorrectly routed to SDO, resulting in garbage data.
drivers/iio/adc/ad4134.c static const struct regmap_access_table ad4134_regmap_wr_table = { · line 316
Quoted 6 lines
drivers/iio/adc/ad4134.c +6
-0+ +/* +* Data output on the DOUT lines is sampled on the falling edge +* (SPI mode 1). +*/ + st->spi->mode =SPI_MODE_1 ;
[Severity: Medium]
Similar to register access mode, does this direct assignment clear all other SPI flags, potentially breaking SPI communication on platforms requiring extra SPI flags?
Quoted 1 line
drivers/iio/adc/ad4134.c +1
-0+ ret =spi_setup (st->spi);
[ ... ]
Quoted 9 lines
drivers/iio/adc/ad4134.c +5
-0@@ -333,9 +434,24 @@ static int ad4134_register_read(struct ad4134_state *st, unsigned int reg,static int ad4134_reg_read (void *context,unsigned int reg,unsigned int *val) {struct ad4134_state *st = context; +int ret, access_ret; + +if (reg >=AD4134_CH_VREG (0 )) { +if (st->spi_mode ==AD4134_SPI_MODE_NO_CS ) +return ad4134_data_read (st, reg, val);
drivers/iio/adc/ad4134.c static int ad4134_register_read(struct ad4134_state *st, unsigned int reg, · line 449
Quoted 10 lines
drivers/iio/adc/ad4134.c +7
-2+ + ret =ad4134_set_sample_access (st); +if (ret) +return ret; -if (reg >=AD4134_CH_VREG (0 )) -return ad4134_data_read (st, reg, val); + ret =ad4134_data_read (st, reg, val); + + access_ret =ad4134_set_register_access (st);
[Severity: Medium]
If a user reads a register via debugfs while the IIO buffer is active, does this unconditionally revert the SPI and multiplexer state to register access mode?
This could clobber the active IIO buffer's expected hardware state (SPI_MODE_1 and DOUT0), causing subsequent trigger handler reads to fail or receive garbage.
Also, for standard registers, does ad4134_reg_read() fall through to ad4134_register_read(), where SPI transfers are initiated without holding access_mode_lock and without verifying the hardware is in register access mode?
Quoted 8 lines
drivers/iio/adc/ad4134.c +5
-0+if (access_ret) +dev_err (&st->spi->dev,"access mode error: %d\n" , access_ret); + +return ret; + }return ad4134_register_read (st, reg, val); }
[ ... ]
Quoted 1 line
drivers/iio/adc/ad4134.c +0
-0@@ -586,15 +717,68 @@ static int ad4134_probe(struct spi_device *spi)
[ ... ]
drivers/iio/adc/ad4134.c static int ad4134_probe(struct spi_device *spi) · line 770
Quoted 6 lines
drivers/iio/adc/ad4134.c +6
-0+ }else { +/* +* This controls a multiplexer OUTSIDE OF AD4134 SILICON. +* See AD4134 IIO documentation for details. +*/ + st->mux_st[AD4134_SDO_INPUT ] =devm_mux_state_get_selected (dev,"reg_access" );
[Severity: High]
Does this lock the mux control and tie its unlock to driver unbind, making the manual unlock seen in ad4134_set_sample_access() unsafe?
Quoted 4 lines
drivers/iio/adc/ad4134.c +4
-0+if (IS_ERR (st->mux_st[AD4134_SDO_INPUT ])) +return dev_err_probe (dev,PTR_ERR (st->mux_st[AD4134_SDO_INPUT ]), +"failed to get reg_access mux-state\n" ); +
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=11
Jonathan Cameron
maintainerRe: [PATCH v2 11/15] iio: adc: ad4134: Support SPI 4-wire mode
In reply to Marcelo SchmittQuoted 8 lines
AD4134 devices can be wired in a few different ways. So far, only minimum I/O mode was supported. While minimum I/O mode allows interfacing with AD4134 with a reduced number of wires, that wiring configuration is not optimal for high-throughput data acquisition.
Extend AD4134 support to enable interfacing in SPI 4-wire configuration.
Signed-off-by Marcelo Schmitt <[email protected]>
There is a lot of sashiko feedback on this one. Make sure to take a close look.
https://sashiko.dev/#/patchset/cover.1789494473.git.marcelo.schmitt%40analog.com
Quoted 10 lines
drivers/iio/adc/ad4134.c +0
-1--- a/drivers/iio/adc/ad4134.cenum ad4134_filter_type {AD4134_WIDEBAND ,AD4134_SINC6 ,
drivers/iio/adc/ad4134.c struct ad4134_state { · line 176
Quoted 6 lines
drivers/iio/adc/ad4134.c +3
-0@@ -155,6 +171,12 @@ struct ad4134_state {* atomicity of consecutive register access operations. */ struct mutex lock; +/* +* Ensure atomicity of access mode switch operations. +*/
One line comment seems like enough.
Quoted 13 lines
drivers/iio/adc/ad4134.c +6
-0+struct mutex access_mode_lock; +struct mux_state *mux_st[2 ];/* For external multiplexer control */ +enum ad4134_spi_mode spi_mode;int refin_mv;bool crc_en;/* @@ -230,6 +252,85 @@ static const struct regmap_access_table ad4134_regmap_wr_table = { .n_yes_ranges = ARRAY_SIZE(ad4134_regmap_wr_range), }; +/* +* This function controls a multiplexer OUTSIDE OF AD4134 SILICON. +* When AD4134 SDO and DOUT0 pins are multiplexed, this function changes the
drivers/iio/adc/ad4134.c static const struct regmap_access_table ad4134_regmap_wr_table = { · line 267
Quoted 10 lines
drivers/iio/adc/ad4134.c +10
-0+* multiplexer state to route SDO to the SPI controller. See AD4134 IIO +* documentation for details. +*/ +static int ad4134_set_register_access (struct ad4134_state *st) +{ +int ret; + +guard (mutex)(&st->access_mode_lock); + + st->spi->mode =SPI_MODE_0 ;
Oh goody we are flipping the spi bus type and that is not describable in firmware.
drivers/iio/adc/ad4134.c static const struct regmap_access_table ad4134_regmap_wr_table = { · line 280
Quoted 13 lines
drivers/iio/adc/ad4134.c +13
-0+ ret =spi_setup (st->spi); +if (ret) +return ret; + + ret =mux_state_deselect (st->mux_st[AD4134_DOUT0_INPUT ]); +if (ret) +dev_err (&st->spi->dev,"error on DOUT0 deselect: %d\n" , ret); + +/* +* Try to update the multiplexer state to route SDO to the SPI +* controller. If the SDO mux state is BUSY (i.e. locked), that means +* the desired state is already selected and we should not block here. +*/
I'm confused whether this is an expected path or not. Is there some race after the deselect above? If so call out how that occurs.
Signature · 1 line
Jonathan Cameron <[email protected]>
Marcelo Schmitt
maintainerPATCH v2 12/15 dt-bindings: iio: adc: adi,ad4134: Document PWM usage
In reply to Marcelo SchmittAdvanced setups may connect a PWM signal to AD4134 ODR pin for faster output data rates.
---
No changes since v1.
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml | 4 ++++ 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +4 -0
@@ -104,6 +104,10 @@ properties: GPIO connected to ODR pin. Used to sample ADC data in minimum I/O mode. maxItems: 1 + pwms: + description: PWM signal connected to the ODR pin. + maxItems: 1 + mux-states: description: mux controller node to route SDO and DOUT0 signals from peripheral to
Signature · 1 line
2.53.0
Marcelo Schmitt
maintainerPATCH v2 13/15 dt-bindings: iio: adc: adi,ad4134: Add high data throughput example
In reply to Marcelo SchmittAdd device tree example for AD4134 operation in 4-wire SPI mode with 4 lines to transfer data samples to the host controller.
---
No changes since v1.
.../bindings/iio/adc/adi,ad4134.yaml | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+)
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +42 -0
@@ -243,4 +243,46 @@ examples: }; }; + - | + #include <dt-bindings/gpio/gpio.h> + + ad4134_sdo_dout0_mux: mux-controller { + compatible = "gpio-mux"; + #mux-state-cells = <1>; + mux-gpios = <&gpio0 100 GPIO_ACTIVE_HIGH>; + }; + + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad4134"; + reg = <0>; + + spi-max-frequency = <1000000>; + spi-rx-bus-width = <1>, <1>, <1>, <1>; /* 4 lanes of 1 bit each */ + + pwms = <&ad4134_odr_generator 1 1000000 0>; + reset-gpios = <&gpio0 86 GPIO_ACTIVE_LOW>; + odr-gpios = <&gpio0 87 GPIO_ACTIVE_HIGH>; + powerdown-gpios = <&gpio0 88 GPIO_ACTIVE_LOW>; + + clocks = <&sys_clk>; + clock-names = "clkin"; + + avdd5-supply = <&avdd5>; + dvdd5-supply = <&dvdd5>; + iovdd-supply = <&iovdd>; + refin-supply = <&refin>; + avdd1v8-supply = <&avdd1v8>; + dvdd1v8-supply = <&dvdd1v8>; + clkvdd-supply = <&clkvdd>; + + mux-states = <&ad4134_sdo_dout0_mux 0>, <&ad4134_sdo_dout0_mux 1>; + mux-state-names = "data_read", "reg_access"; + + adi,spi-mode = "4-wire"; + }; + }; ...
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v2 13/15] dt-bindings: iio: adc: adi,ad4134: Add high data throughput example
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Low] dt-bindings: iio: adi,ad4134: conflicting ODR pin configuration in example --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] dt-bindings: iio: adi,ad4134: conflicting ODR pin configuration in example --
commit be1a4d1c0eae8c4f9247540fa11bc671216046b8
Author: Marcelo Schmitt <[email protected]>
dt-bindings: iio: adc: adi,ad4134: Add high data throughput example
Add device tree example for AD4134 operation in 4-wire SPI mode with 4 lines to transfer data samples to the host controller.
Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +0
-0
[ ... ]
Quoted 4 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +4
-0+ spi { + #address-cells = <1>; + #size-cells = <0>; +
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml examples: · line 268
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +10
-0+ adc@0 { + compatible = "adi,ad4134"; + reg = <0>; + + spi-max-frequency = <1000000>; + spi-rx-bus-width = <1>, <1>, <1>, <1>; /* 4 lanes of 1 bit each */ + + pwms = <&ad4134_odr_generator 1 1000000 0>; + reset-gpios = <&gpio0 86 GPIO_ACTIVE_LOW>; + odr-gpios = <&gpio0 87 GPIO_ACTIVE_HIGH>;
[Severity: Low]
Does this example create a logical hardware conflict?
Both a PWM (pwms) and a GPIO (odr-gpios) are assigned to the exact same physical ODR pin. Additionally, odr-gpios is explicitly documented to be used for minimum I/O mode, but this example configures adi,spi-mode = "4-wire".
Could this cause developers copying this example to create device trees with conflicting and semantically incorrect properties for the chosen SPI mode?
Quoted 20 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +19
-0+ powerdown-gpios = <&gpio0 88 GPIO_ACTIVE_LOW>; + + clocks = <&sys_clk>; + clock-names = "clkin"; + + avdd5-supply = <&avdd5>; + dvdd5-supply = <&dvdd5>; + iovdd-supply = <&iovdd>; + refin-supply = <&refin>; + avdd1v8-supply = <&avdd1v8>; + dvdd1v8-supply = <&dvdd1v8>; + clkvdd-supply = <&clkvdd>; + + mux-states = <&ad4134_sdo_dout0_mux 0>, <&ad4134_sdo_dout0_mux 1>; + mux-state-names = "data_read", "reg_access"; + + adi,spi-mode = "4-wire"; + }; + };
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=13
Jonathan Cameron
maintainerRe: [PATCH v2 13/15] dt-bindings: iio: adc: adi,ad4134: Add high data throughput example
In reply to Marcelo SchmittQuoted 4 lines
Add device tree example for AD4134 operation in 4-wire SPI mode with 4 lines to transfer data samples to the host controller.
Signed-off-by Marcelo Schmitt <[email protected]>
Why not do this earlier in the patch series - before introducing 4 wire support?
Signature · 1 line
Jonathan Cameron <[email protected]>
Rob Herring
maintainerRe: [PATCH v2 13/15] dt-bindings: iio: adc: adi,ad4134: Add high data throughput example
In reply to Marcelo SchmittQuoted 16 lines · Marcelo Schmitt
Add device tree example for AD4134 operation in 4-wire SPI mode with 4 lines to transfer data samples to the host controller.
Signed-off-by Marcelo Schmitt <[email protected]>---
No changes since v1..../bindings/iio/adc/adi,ad4134.yaml | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+)Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +0
-0@@ -243,4 +243,46 @@ examples:
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml examples: · line 253
Quoted 10 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +8
-0}; }; + - | + #include <dt-bindings/gpio/gpio.h> + + ad4134_sdo_dout0_mux: mux-controller { + compatible = "gpio-mux"; + #mux-state-cells = <1>; + mux-gpios = <&gpio0 100 GPIO_ACTIVE_HIGH>; + };
Drop. We don't put providers in examples.
Quoted 38 lines
Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +34
-0+ + spi { + #address-cells = <1>; + #size-cells = <0>; + + adc@0 { + compatible = "adi,ad4134"; + reg = <0>; + + spi-max-frequency = <1000000>; + spi-rx-bus-width = <1>, <1>, <1>, <1>; /* 4 lanes of 1 bit each */ + + pwms = <&ad4134_odr_generator 1 1000000 0>; + reset-gpios = <&gpio0 86 GPIO_ACTIVE_LOW>; + odr-gpios = <&gpio0 87 GPIO_ACTIVE_HIGH>; + powerdown-gpios = <&gpio0 88 GPIO_ACTIVE_LOW>; + + clocks = <&sys_clk>; + clock-names = "clkin"; + + avdd5-supply = <&avdd5>; + dvdd5-supply = <&dvdd5>; + iovdd-supply = <&iovdd>; + refin-supply = <&refin>; + avdd1v8-supply = <&avdd1v8>; + dvdd1v8-supply = <&dvdd1v8>; + clkvdd-supply = <&clkvdd>; + + mux-states = <&ad4134_sdo_dout0_mux 0>, <&ad4134_sdo_dout0_mux 1>; + mux-state-names = "data_read", "reg_access"; + + adi,spi-mode = "4-wire"; + }; + };Signature · 1 line
2.53.0
Marcelo Schmitt
maintainerPATCH v2 14/15 iio: adc: ad4134: Support high-speed data capture
In reply to Marcelo SchmittMake use of SPI transfer offloading to speed up data capture, enabling data acquisition at faster sample rates (up to 1.496 MSPS).
---
Change log v1 -> v2:
- Error out on min I/O + offload config (currently not being supported).
- Improved PWM setup readability, update allowed scan_mask codestyle.
- Preserved triggered buffer setup callbacks.
- Used pahole to reduce memory holes in ad4134_state struct.
We can't drop target_ns and use odr_wf.duty_length_ns directly because pwm_..._waveform...() may round waveform parameters. Same reasoning applies to offload_offset_ns.
drivers/iio/adc/Kconfig | 5 + drivers/iio/adc/ad4134.c | 387 ++++++++++++++++++++++++++++++++++++++- 2 files changed, 383 insertions(+), 9 deletions(-)
drivers/iio/adc/Kconfig +5 -0
@@ -123,9 +123,14 @@ config AD4130 config AD4134 tristate "Analog Devices AD4134 ADC Driver" depends on SPI + depends on PWM select REGMAP_SPI + select SPI_OFFLOAD + select SPI_OFFLOAD_TRIGGER_PWM select CRC8 select IIO_BUFFER + select IIO_BUFFER_DMA + select IIO_BUFFER_DMAENGINE select IIO_TRIGGERED_BUFFER select MULTIPLEXER help
drivers/iio/adc/ad4134.c +378 -9
471 lines not on this page · raw message
Signature · 1 line
2.53.0
[email protected]
AI reviewRe: [PATCH v2 14/15] iio: adc: ad4134: Support high-speed data capture
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider: - [High] iio: adc: ad4134: division by zero panic in probe if optional clocks are missing - [High] iio…
Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] iio: adc: ad4134: division by zero panic in probe if optional clocks are missing
- [High] iio: adc: ad4134: incorrect SPI transfer length drops channels in single-lane mode
- [High] iio: adc: ad4134: data corruption in ad4134_trigger_handler() due to missed increment
- [High] iio: adc: ad4134: triggered buffer samples dropped due to undersized buffer
- [Medium] iio: adc: ad4134: loop failure with valid slow clocks in ad4134_update_conversion_rate()
- [Medium] iio: adc: ad4134: PWM hardware resource leak on driver unbind
- [Medium] iio: adc: ad4134: LIFO teardown order violation in ad4134_offload_buffer_predisable()
- [Low] iio: adc: ad4134: off-by-one bounds check in in_range() validation
- [Low] iio: adc: ad4134: data race on odr_hz in ad4134_read_raw()
--
commit a29ef6f09380617b32619e37a1eab7692d45d1b3
Author: Marcelo Schmitt <[email protected]>
iio: adc: ad4134: Support high-speed data capture
This commit adds support for high-speed data capture to the AD4134 driver using SPI transfer offloading. It adds PWM triggering for the ADC ODR pin and DMA support for higher sample rates.
Quoted 4 lines
drivers/iio/adc/ad4134.c +0
-0
[ ... ]
Quoted 1 line
drivers/iio/adc/ad4134.c +0
-0@@ -178,6 +225,14 @@ struct ad4134_state {
[ ... ]
drivers/iio/adc/ad4134.c
Quoted 7 lines
drivers/iio/adc/ad4134.c +0
-0/* * DMA (thus cache coherency maintenance) requires the transfer buffers * to live in their own cache lines. */ union {u8 reg[AD4134_SPI_MAX_XFER_LEN ];u32 scan[ARRAY_SIZE (ad4134_chan_set)];
[Severity: High]
Will this array be large enough to hold the timestamp channel? The size is calculated as 5 elements (20 bytes). However, the timestamp channel requires 8 bytes, which would make the total required buffer size 24 bytes when all channels and the timestamp are active.
Quoted 3 lines
drivers/iio/adc/ad4134.c +0
-0} rx_buf__aligned(IIO_DMA_MINALIGN) ;u8 tx_buf[AD4134_SPI_MAX_XFER_LEN ]; };
[ ... ]
Quoted 5 lines
drivers/iio/adc/ad4134.c +4
-0@@ -464,6 +519,91 @@ static const struct regmap_config ad4134_regmap_config = { +static int ad4134_update_conversion_rate (struct ad4134_state *st, +unsigned int freq_Hz) +{ +struct spi_offload_trigger_config config = st->offload_trigger_config;
drivers/iio/adc/ad4134.c static const struct regmap_config ad4134_regmap_config = { · line 532
Quoted 10 lines
drivers/iio/adc/ad4134.c +10
-0+struct pwm_waveform odr_wf = { }; +u64 offload_period_ns; +u64 offload_offset_ns; +u64 odr_high_time_ns; +unsigned int count; +u64 target_ns; +int ret; + +if (!in_range (freq_Hz,AD4134_MIN_ODR_FREQ_HZ ,AD4134_MAX_ODR_FREQ_HZ )) +return -ERANGE ;
[Severity: Low]
Does this accurately validate the frequency upper bound? The in_range() macro takes the length of the range as its third parameter. By passing the absolute maximum frequency (1496000), it seems to allow frequencies up to 1496010 Hz rather than capping strictly at 1496000 Hz.
drivers/iio/adc/ad4134.c static const struct regmap_config ad4134_regmap_config = { · line 544
Quoted 9 lines
drivers/iio/adc/ad4134.c +9
-0+ + odr_wf.period_length_ns =DIV_ROUND_UP_ULL (NSEC_PER_SEC , freq_Hz); +/* +* Set the PWM duty cycle to keep ODR high for at least minimum required +* time. If the rounded PWM's value is less than the minimum required, +* increase the target value by 10 and attempt to round the waveform +* again, until the minimum (or try count limit) is reached. +*/ + odr_high_time_ns =div64_ul (6ULL *NSEC_PER_SEC , st->sys_clk_hz);
[Severity: High]
Could this cause a division by zero panic? If the optional xtal and clkin clocks are missing in the device tree, ad4134_clock_select() can fall back to clk_get_rate(NULL) == 0, resulting in st->sys_clk_hz being 0.
drivers/iio/adc/ad4134.c static const struct regmap_config ad4134_regmap_config = { · line 553
Quoted 9 lines
drivers/iio/adc/ad4134.c +9
-0+ target_ns =0 ; + count =100 ; +do { + target_ns +=10 ;/* Increment by PWM duty cycle period */ + odr_wf.duty_length_ns = target_ns; + ret =pwm_round_waveform_might_sleep (st->odr_pwm, &odr_wf); +if (ret) +return ret; + }while (count-- && odr_wf.duty_length_ns < odr_high_time_ns);
[Severity: Medium]
Will this loop artificially fail for valid slow-clock hardware configurations? If sys_clk_hz is low (e.g. 2 MHz), odr_high_time_ns will exceed 1000 ns. Since this loop starts target_ns at 0 and increments by 10 for a maximum of 100 iterations, it can never reach a duty cycle above 1000 ns.
[ ... ]
drivers/iio/adc/ad4134.c static const struct regmap_config ad4134_regmap_config = { · line 598
Quoted 3 lines
drivers/iio/adc/ad4134.c +3
-0+ ret =pwm_set_waveform_might_sleep (st->odr_pwm, &odr_wf, false); +if (ret) +return ret;
[Severity: Medium]
Does this leak the PWM hardware resource on driver unbind? The PWM waveform is started here, but it doesn't look like there is a cleanup callback registered (e.g. via devm_add_action_or_reset()) to stop the waveform when the module is removed or if a later probe step fails.
Quoted 19 lines
drivers/iio/adc/ad4134.c +19
-0+ + st->offload_trigger_config = config; + st->odr_wf = odr_wf; + st->odr_hz =DIV_ROUND_UP_ULL (NSEC_PER_SEC , odr_wf.period_length_ns); + +return 0 ; +} + +static irqreturn_t ad4134_trigger_handler (int irq,void *p) +{ +struct iio_poll_func *pf = p; +struct iio_dev *indio_dev = pf->indio_dev; +struct ad4134_state *st =iio_priv (indio_dev); +unsigned int i =0 ; +int ret; + +gpiod_set_value_cansleep (st->odr_gpio,1 ); +fsleep (1 ); +gpiod_set_value_cansleep (st->odr_gpio,0 );
drivers/iio/adc/ad4134.c static const struct regmap_config ad4134_regmap_config = { · line 582
Quoted 10 lines
drivers/iio/adc/ad4134.c +10
-0+ +for (unsigned int ch =0 ; ch <AD4134_NUM_CHANNELS ; ch++) { + ret =spi_write_then_read (st->spi,NULL ,0 , &st->rx_buf.scan[ch], +BITS_TO_BYTES (AD4134_CHAN_PRECISION_BITS )); +if (ret) + goto err_out; + +if (test_bit (ch, indio_dev->active_scan_mask) && ch != i) +memcpy (&st->rx_buf.scan[i++], &st->rx_buf.scan[ch], +sizeof (st->rx_buf.scan[ch]));
[Severity: High]
Can this logic cause channels to overwrite each other? If the test_bit() check is true and ch == i, the memcpy is avoided but i is never incremented. The next active channel will then overwrite the previous one at index i.
drivers/iio/adc/ad4134.c static const struct iio_chan_spec ad4134_chan_set[] = {
Quoted 4 lines
drivers/iio/adc/ad4134.c +4
-0+ } + +iio_push_to_buffers_with_ts (indio_dev, &st->rx_buf.scan, +sizeof (st->rx_buf.scan), pf->timestamp);
[Severity: High]
Will samples be dropped when all channels and the timestamp are active? sizeof(st->rx_buf.scan) is 20 bytes, but when 4 channels (16 bytes) and the timestamp (8 bytes) are active, the IIO core expects 24 bytes and may reject the sample with -ENOSPC.
Quoted 5 lines
drivers/iio/adc/ad4134.c +5
-0+ +err_out: +iio_trigger_notify_done (indio_dev->trig); +return IRQ_HANDLED ; +}
[ ... ]
Quoted 1 line
drivers/iio/adc/ad4134.c +0
-0@@ -549,6 +689,44 @@ static int ad4134_read_raw(struct iio_dev *indio_dev,
[ ... ]
drivers/iio/adc/ad4134.c static int ad4134_read_raw(struct iio_dev *indio_dev, · line 694
Quoted 6 lines
drivers/iio/adc/ad4134.c +3
-0*val2 =AD4134_CHAN_PRECISION_BITS -1 ;return IIO_VAL_FRACTIONAL_LOG2 ; +case IIO_CHAN_INFO_SAMP_FREQ : + *val = st->odr_hz; +return IIO_VAL_INT ;
[Severity: Low]
Is it safe to read st->odr_hz here without claiming direct mode or holding st->lock? It appears ad4134_update_conversion_rate() modifies it concurrently under the lock, which could result in a data race or a torn read.
Quoted 4 lines
drivers/iio/adc/ad4134.c +4
-0+default : +return -EINVAL ; + } +}
[ ... ]
Quoted 1 line
drivers/iio/adc/ad4134.c +0
-0@@ -600,6 +778,176 @@ static const struct iio_info ad4134_info = {
[ ... ]
drivers/iio/adc/ad4134.c static const struct iio_info ad4134_info = { · line 796
Quoted 9 lines
drivers/iio/adc/ad4134.c +9
-0+static void ad4134_prepare_offload_msg (struct iio_dev *indio_dev) +{ +struct ad4134_state *st =iio_priv (indio_dev); +unsigned int base_len =roundup_pow_of_two (BITS_TO_BYTES (AD4134_CHAN_PRECISION_BITS )); +unsigned int bpw =AD4134_CHAN_PRECISION_BITS ; + + st->xfers.cs_off =1 ; + st->xfers.bits_per_word = bpw; + st->xfers.len = base_len * st->spi->num_rx_lanes;
[Severity: High]
Could this drop channels when operating in single-lane mode?
If num_rx_lanes is 1, the transfer length computes to 4 bytes instead of the required 16 bytes. However, ad4134_scan_masks forces all 4 channels to be read per sample, which might leave the rest of the IIO buffer unpopulated.
Quoted 7 lines
drivers/iio/adc/ad4134.c +7
-0+if (st->spi->num_rx_lanes >1 ) + st->xfers.multi_lane_mode =SPI_MULTI_LANE_MODE_STRIPE ; + + st->xfers.offload_flags =SPI_OFFLOAD_XFER_RX_STREAM ; + +spi_message_init_with_transfers (&st->msg, &st->xfers,1 ); +}
[ ... ]
Quoted 4 lines
drivers/iio/adc/ad4134.c +4
-0+static int ad4134_offload_buffer_predisable (struct iio_dev *indio_dev) +{ +struct ad4134_state *st =iio_priv (indio_dev); +int ret =0 ;
drivers/iio/adc/ad4134.c static const struct iio_info ad4134_info = { · line 853
Quoted 10 lines
drivers/iio/adc/ad4134.c +10
-0+ +spi_offload_trigger_disable (st->offload, st->offload_trigger); + +if (st->spi_mode ==AD4134_SPI_MODE_4_WIRE ) + ret =ad4134_set_register_access (st); + +spi_unoptimize_message (&st->msg); + +return ret; +}
[Severity: Medium]
Is this violating LIFO teardown order?
In ad4134_offload_buffer_postenable(), spi_optimize_message() is called before changing access modes, but here ad4134_set_register_access() (which alters the SPI mode) is called before spi_unoptimize_message(). Altering the mode while a message is actively optimized might cause undefined behavior in the SPI controller.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=14
Jonathan Cameron
maintainerRe: [PATCH v2 14/15] iio: adc: ad4134: Support high-speed data capture
In reply to Marcelo SchmittQuoted 4 lines
Make use of SPI transfer offloading to speed up data capture, enabling data acquisition at faster sample rates (up to 1.496 MSPS).
Signed-off-by Marcelo Schmitt <[email protected]>
A lot of sashiko feedback so I only took a cursory look. Please address all that (or point out why it is wrong!)
Jonathan
Quoted 4 lines
drivers/iio/adc/ad4134.c +0
-0
...
Quoted 33 lines
drivers/iio/adc/ad4134.c +33
-0+static int ad4134_offload_setup (struct iio_dev *indio_dev,struct ad4134_state *st) +{ +struct device *dev = &st->spi->dev; +int ret; + + ret =ad4134_pwm_get (st); +if (ret) +return ret; + + ret =ad4134_offload_buffer_setup (indio_dev, st->spi); +if (ret) +return ret; + +/* +* Start with a sampling rate slower than 374 kSPS because that is the +* maximum rate supported with wideband filter (default on power up). +*/ + st->odr_hz =250 *HZ_PER_KHZ ; + ret =ad4134_update_conversion_rate (st, st->odr_hz); +if (ret) +return dev_err_probe (dev, ret,"failed to set sampling freq\n" ); + + indio_dev->setup_ops = &ad4134_offload_buffer_setup_ops; + indio_dev->available_scan_masks = ad4134_scan_masks; + +switch (st->spi->num_rx_lanes) { +case 1 : +return regmap_update_bits (st->regmap,AD4134_DIG_IF_CFG_REG , +AD4134_DIF_IF_CFG_FORMAT_MASK , +FIELD_PREP (AD4134_DIF_IF_CFG_FORMAT_MASK , +AD4134_DATA_FORMAT_SINGLE_CH_MODE )); +case 4 : + return regmap_update_bits(st->regmap, AD4134_DIG_IF_CFG_REG,
drivers/iio/adc/ad4134.c static const struct iio_info ad4134_info = { · line 948
Quoted 10 lines
drivers/iio/adc/ad4134.c +10
-0+AD4134_DIF_IF_CFG_FORMAT_MASK , +FIELD_PREP (AD4134_DIF_IF_CFG_FORMAT_MASK , + AD4134_DATA_FORMAT_QUAD_CH_PARALLEL)); +default : +return dev_err_probe (dev, -EINVAL , +"unsupported number of spi-rx-bus-width elements: %d\n" , + st->spi->num_rx_lanes); + } + +return 0 ;
Unreachable code so drop it.
Signature · 1 line
Jonathan Cameron <[email protected]>
Marcelo Schmitt
maintainerPATCH v2 15/15 Docs: iio: Add AD4134
In reply to Marcelo SchmittAdd initial documentation for the ad4134 IIO driver.
---
Change log v1 -> v2:
- More accurate multiplexer control signal portrayal in connection diagrams.
Documentation/iio/ad4134.rst | 130 +++++++++++++++++++++++++++++++++++ Documentation/iio/index.rst | 1 + MAINTAINERS | 1 + 3 files changed, 132 insertions(+) create mode 100644 Documentation/iio/ad4134.rst
Documentation/iio/ad4134.rst new file +130 -0
@@ -0,0 +1,130 @@ +.. SPDX-License-Identifier: GPL-2.0-only + +============= +AD4134 driver +============= + +Device driver for Analog Devices Inc. AD4134 and similar ADCs. + +Supported devices +================= + +* `AD4134 <https://www.analog.com/AD4134>`_ +* `AD7134 <https://www.analog.com/AD7134>`_ + +Wiring connections +------------------ + +AD4134 and similar ADCs can operate in a few different wiring configurations. +Because AD4134 also supports control modes other than through SPI protocol, +the following pins must be statically set for all of the wiring modes described +below. + ++----------------------------+----------------------+--------------------+ +| Pin Function | Level | Description | ++============================+======================+====================+ +| PIN/SPI | High | SPI control mode | ++----------------------------+----------------------+--------------------+ +| MODE | Low | ASRC slave mode | ++----------------------------+----------------------+--------------------+ +| DEC0/DCLKI | Low | DCLK input | ++----------------------------+----------------------+--------------------+ +| DEC1/DCLKMODE | Low | Gated DCLK | ++----------------------------+----------------------+--------------------+ + +Minimum I/O mode +^^^^^^^^^^^^^^^^ + +The minimum I/O mode wiring allows AD4134 register and data access with the +conventional set of SPI bus lines. In this mode, the peripheral CS pin must be +tied to GND. A possible connection schema that sets AD4134 digital interface for +minimum I/O mode is:: + + +------------+ +-------------+ + | AD4134 | | SPI | + | | | Controller | + | DCLK |<--+ | | + | SCLK |<--+---- | SCLK | + | SDI |<--------| SDO | + | SDO |-------->| SDI | + | ODR |<--------| GPIO | + | CS |----+ | | + +------------+ | +-------------+ + V + GND + +Set ``adi,spi-mode = "no-cs"`` in device tree to indicate the device is +connected in minimum I/O wiring configuration. + +SPI 4-wire mode - single lane +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +The conventional SPI 4-wire configuration except a multiplexer is used to +select between SDO (for register data) and DOUT0 (for ADC sample data).:: + + +-----------------------+ +-----------------+ + | AD4134 | | SPI Controller | + | | | | + | SPI interface | | | + | for register SCLK |<------------------------| SCLK | + | access CS |<------------------------| CS | + | SDI |<------------------------| SDO | + | SDO |------->|¯¯¯¯\ | | + | Data interface | |MUX >--------->| SDI | + | for ADC sample DOUT0 |------->|____/ | | + | data read | ^ | | + | | | | | + | DCLK |<------------------------| DCLK | + | ODR |<----------------+ +->| Offload Trigger | + +-----------------------+ | | | +-----------------+ + | | +--| PWM1 | + | +-------| PWM0 | + | +-------| GPIO0 | + +--------------| GPIO1 | + +-----------------+ + +Set ``adi,spi-mode = "4-wire"`` in device tree to indicate the device is +connected in SPI 4-wire configuration. + +For SPI offload operation, a PWM is connected to the ADC ODR pin. The ``pwms`` +firmware property specifies the PWM that is connected to the ADC. The SPI +offload will have a ``trigger-sources`` property to indicate the SPI offload +(PWM) trigger source. + +To achieve high sample rates while meeting ADC timing requirements, an offset +must be added between the rising edges of PWM0 and PWM1 to delay the SPI +transfer until ADC sample data is available. This requires a specialized PWM +controller that can provide such an offset. The `AD4134-FMC HDL project`_, for +example, can be configured to sample AD4134 at high speed sample rates. + +.. _AD4134-FMC HDL project: https://analogdevicesinc.github.io/hdl/projects/ad4134_fmc/index.html + +SPI 4-wire mode - multi-lane +^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +Maximum throughput rate can be achieved by reading 4 input lines simultaneously.:: + + +-----------------------+ +-----------------+ + | AD4134 | | SPI Controller | + | | | | + | SPI interface SCLK |<------------------------| SCLK | + | for register CS |<------------------------| CS | + | access SDI |<------------------------| SDO | + | SDO |------->|¯¯¯¯\ | | + | | |MUX >--------->| SDI0 | + | Data interface DOUT0 |------->|____/ | | + | for ADC sample | ^ | | + | data read DOUT1 |------------------------>| SDI1 | + | DOUT2 |------------------------>| SDI2 | + | DOUT3 |------------------------>| SDI3 | + | DCLK |<------------------------| DCLK | + | ODR |<----------------+ +->| Offload Trigger | + +-----------------------+ | | | +-----------------+ + | | +--| PWM1 | + | +-------| PWM0 | + | +-------| GPIO0 | + +--------------| GPIO1 | + +-----------------+ + +Each of SDI0 to SDI3 connect to a different SPI controller deserializer +(``spi-rx-bus-width = <1>, <1>, <1>, <1>``).
Documentation/iio/index.rst +1 -0
@@ -23,6 +23,7 @@ Industrial I/O Kernel Drivers ad4000 ad4030 ad4062 + ad4134 ad4691 ad4695 ad7191
MAINTAINERS +1 -0
@@ -1533,6 +1533,7 @@ L: [email protected] S: Supported W: https://ez.analog.com/linux-software-drivers F: Documentation/devicetree/bindings/iio/adc/adi,ad4134.yaml +F: Documentation/iio/ad4134.rst F: drivers/iio/adc/ad4134.c ANALOG DEVICES INC AD4170-4 DRIVER
Signature · 1 line
2.53.0