Add support for adaq776x-1 series and add missing features #2587

jonathanns · 2024-08-27T23:09:44Z

PR Description

This adds support for ADAQ7768-1, ADAQ7769-1 and ADAQ7767-1 devices from the AD7768-1 family. It also includes:

Addition of missing IIO attributes
AAF gain configuration
PGA control
Fix on MOSI idle state.
Support for 16-bit mode, which allows to reach maximum sample rate (1.024 MSPS) with Sinc5 Dec8 filter.
Use of new spi-engine implementation.

PR Type

Bug fix (a change that fixes an issue)
New feature (a change that adds new functionality)
Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

I have conducted a self-review of my own code changes
I have tested the changes on the relevant hardware
I have updated the documentation outside this repo accordingly (if there is the case)

Add IIO attributes: - Filter type - Decimation rate for Sinc3 filter - Decimation rate for Sinc5 and Wideband filter - Power mode selection - MCLK divider - Common mode voltage Signed-off-by: PopPaul2021 <[email protected]>

This adds support for ADAQ7767/68/69-1 series, which includes PGIA and AAF gains, configurable through devicetree. It also fixes sampling frequency calculation, sync pulse using registers and data acquisition with iio. the driver now can work in low-latency mode. In low-latency mode the filter type is fixed in Sinc5 Dec8 and ADC data width is set to 16 bits to reach maximum sample rate. When disabled, the ADC data is 24 bits and it is possible to change between filters in runtime (except for Sinc5 Dec8). Signed-off-by: jonathanns <[email protected]>

All supported parts require that the MOSI line stays high while in idle. Signed-off-by: jonathanns <[email protected]>

Add support for new spi-engine implementation on ad7768-1 driver. This optimizes the offload transfers by reducing the CS delay and removing the number of commands on offload's FIFO, which makes the low latency mode viable. Without the otimization there's not enough time between the end of a transfer and the start of the next one, reducing the actual sample rate. Signed-off-by: jonathanns <[email protected]>

new required property Update sync-in GPIO number according to the current HDL version. Add low-latency property. Signed-off-by: jonathanns <[email protected]>

Enables using the ADAQ7767-1 device on ZedBoard with FMC connector. Signed-off-by: jonathanns <[email protected]>

Enables using the ADAQ7768-1 device on ZedBoard with FMC connector. Signed-off-by: jonathanns <[email protected]>

Enables using the ADAQ7769-1 device on ZedBoard with FMC connector. Signed-off-by: jonathanns <[email protected]>

Add compatibles for supported parts in the ad7768-1 family: ADAQ7767-1, ADAQ7768-1 and ADAQ7769-1 Add new properties: adi,low-latency and adi,gain-milli Signed-off-by: jonathanns <[email protected]>

nunojsa

First round of review... There#s some stuff with ABI that needs to be figured. In your second patch, you also have:

"It also fixes sampling frequency calculation, sync pulse using registers
and data acquisition with iio."

Please don't mix fixes with new code being added. Fixes need to come first and be as standalone as possible.

Also note that I'm aware this driver already diverged from the upstream version but it would very nice (and it would make a lot of sense) if you could send these patches upstream with the new devices and new ABI. Naturally you would have to ommit the spi offload part of it but the new ABI would be very important to have it discussed (and agreed) upstream. Otherwise, what it will most likely happen is that we merge something in here that will then change when upstreaming the missing bits of this driver upstream.

nunojsa · 2024-09-23T06:47:54Z

drivers/iio/adc/ad7768-1.c

-	{ AD7768_MCLK_DIV_4, AD7768_DEC_RATE_512, 2048, AD7768_MED_MODE },
-	{ AD7768_MCLK_DIV_4, AD7768_DEC_RATE_1024, 4096, AD7768_MED_MODE },
-	{ AD7768_MCLK_DIV_8, AD7768_DEC_RATE_1024, 8192, AD7768_MED_MODE },
-	{ AD7768_MCLK_DIV_16, AD7768_DEC_RATE_1024, 16384, AD7768_ECO_MODE },


Why are we removing this? I guess it's because we have mclk on a runtime attr but more on that below...

I guess we divided it into separete attributes

nunojsa · 2024-09-23T06:49:51Z

drivers/iio/adc/ad7768-1.c

+	[SINC5_DEC_X8] = "SINC5_DEC_X8",
+	[SINC5_DEC_X16] = "SINC5_DEC_X16",
+	[SINC3] = "SINC3",
+	[WIDEBAND] = "WIDEBAND",


This is something to bother when trying to upstream but take a look at this ABI:

https://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git/tree/Documentation/ABI/testing/sysfs-bus-iio-adc-ad4130?h=testing

I'm fairly sure that when upstreaming we will be asked to put this ABI in a more a generic file. Hence, it makes sense to see if there's something on that file that makes sense to be used in here.

nunojsa · 2024-09-23T06:50:23Z

drivers/iio/adc/ad7768-1.c

+	[AD7768_ECO_MODE]   = "AD7768_ECO_MODE",
+	[AD7768_MED_MODE]   = "AD7768_MED_MODE",
+	[AD7768_FAST_MODE]  = "AD7768_FAST_MODE",
+};


I guess we could implement the above with runtime PM but I'll defer this discussion for usptream...

I will take a look at the Power management module.

nunojsa · 2024-09-23T06:51:00Z

drivers/iio/adc/ad7768-1.c

-		 &ad7768_vcm_mode_enum),
-	IIO_ENUM_AVAILABLE("common_mode_voltage",
-			   IIO_SHARED_BY_ALL,
-			   &ad7768_vcm_mode_enum),


There's no point in changing the above... Minimizing the diffs makes the reviewer life easier

nunojsa · 2024-09-23T06:52:23Z

drivers/iio/adc/ad7768-1.c

+	IIO_ENUM("dec_rate", IIO_SHARED_BY_ALL, &ad7768_dec_rate_iio_enum),
+	IIO_ENUM_AVAILABLE("dec_rate", IIO_SHARED_BY_ALL, &ad7768_dec_rate_iio_enum),
+	IIO_ENUM("mclk_div", IIO_SHARED_BY_ALL, &ad7768_mclk_div_iio_enum),
+	IIO_ENUM_AVAILABLE("mclk_div", IIO_SHARED_BY_ALL, &ad7768_mclk_div_iio_enum),


What's the mclk_div about? Is this an internal divider for an output clock? Or is it for an input clk? If it's an input clock do we really need to change this at runtime or a DT parameter would be enough?

MCLK is an input clock generated by a Crystal. mclk_div is set by a configuration register to control the sampling frequency.

Could we then somehow implement this as part of the sampling_frequency attr?

nunojsa · 2024-09-23T07:16:34Z

drivers/iio/adc/ad7768-1.c

-	indio_dev->name = spi_get_device_id(spi)->name;
+	ret = device_property_read_u32(&spi->dev, "adi,low-latency", &val);
+	if (ret)
+		return ret;


I don't think this needs to be a mandatory property at all... Also seems like a boolean one. But more importantly, it raises the question about this being a DT property? It's at the very least not too coherent with the power_modes attr? Is this somehow related with those modes?

Yes, that could be a boolean. We need something like this in the devicetree to set the IIO channel to either 16 or 24 bits.

In Sinc5 Decimation x8 mode, the sample width is set to 16 bits, while in other filter modes, the sample width is 24 bits.. Since we cannot change the IIO channel in runtime, we need a property in the device tree to choose between the two cases.

When this property is "enabled," the device will operate exclusively in Sinc5 Dec8 mode (16-bit samples). If the property is disabled, the filter mode can be chosen between the other options, and the sample width will default to 24 bits.

In Sinc5 Decimation x8 mode, the sample width is set to 16 bits, while in other filter modes, the sample width is 24 bits.. Since we cannot change the IIO channel in runtime, we need a property in the device tree to choose between the two cases.

If I'm understanding correctly, you can now change the scan elements at run time... take a look at

https://elixir.bootlin.com/linux/v6.11/source/include/linux/iio/iio.h#L839

and how it's being used...

nunojsa · 2024-09-23T07:19:08Z

drivers/iio/adc/ad7768-1.c

+	st->aaf_gain = AD7768_AAF_IN1;
+
+	if (device_property_present(&spi->dev, "adi,aaf-gain")
+	    && st->chip->has_variable_aaf) {


Don't check for presence... Instead, do it like this:

ret = device_property_read_u32(&spi->dev, "adi,aaf-gain", &val); if (!ret) { if (!st->chip->has_variable_aaf) return dev_err_probe(...); // proceed with code validating the prop value ... }

nunojsa · 2024-09-23T07:20:50Z

drivers/iio/adc/ad7768-1.c

+	* Request the SPI controller to make MOSI idle high.
+	*/
+	spi->mode |= SPI_MOSI_IDLE_HIGH;
+	ret = spi_setup(spi);


This should have a Fixes tag and be the first patch in the pull. Fixes should always come first in case we want to backport them.

Also wondering why this was never caught...

Ok, i will separate the fixes and move them to the beginning.
without that the device works, but not consistently. So I guess this slipped through.

nunojsa · 2024-09-23T07:21:40Z

drivers/iio/adc/ad7768-1.c

@@ -4,6 +4,7 @@
 *
 * Copyright 2017 Analog Devices Inc.
 */
+#include "linux/byteorder/little_endian.h"


this seems like an header being auto added by clangd 😅

Indeed, i will check this out

nunojsa · 2024-09-23T07:22:26Z

drivers/iio/adc/ad7768-1.c

 	tx_data[0] = AD7768_RD_FLAG_MSK(addr);
 	xfer.tx_buf = tx_data;
 	ret = spi_sync_transfer(st->spi, &xfer, 1);
 	if (ret < 0)
 		return ret;
-	*data = (len == 1 ? st->data.buf[0] : st->data.word);
+	*data = (len == 1 ? st->data.buf[1] : cpu_to_be32(st->data.word));


are the above changes needed?

It is necessary for the new spi-engine integration

jonathanns · 2024-09-24T15:24:51Z

First round of review... There#s some stuff with ABI that needs to be figured. In your second patch, you also have:

"It also fixes sampling frequency calculation, sync pulse using registers and data acquisition with iio."

Please don't mix fixes with new code being added. Fixes need to come first and be as standalone as possible.

Also note that I'm aware this driver already diverged from the upstream version but it would very nice (and it would make a lot of sense) if you could send these patches upstream with the new devices and new ABI. Naturally you would have to ommit the spi offload part of it but the new ABI would be very important to have it discussed (and agreed) upstream. Otherwise, what it will most likely happen is that we merge something in here that will then change when upstreaming the missing bits of this driver upstream.

Thanks for the feedback, I will re-arrange the commits and take a look at the ABI issue. I am sending these patches now to get a preliminar review before sending them upstream. So, when it is good enough, we can hold off this PR until they are upstreamed.

jonathanns requested review from nunojsa, mhennerich, dbogdan, btogorean, chegbeli, buha, amiclaus, rbolboac, danmois, CiprianRegus and machschmitt as code owners August 27, 2024 23:09

PopPaul2021 and others added 9 commits August 30, 2024 18:04

iio: adc: ad7768-1: add missing IIO attributes

cfcf83f

Add IIO attributes: - Filter type - Decimation rate for Sinc3 filter - Decimation rate for Sinc5 and Wideband filter - Power mode selection - MCLK divider - Common mode voltage Signed-off-by: PopPaul2021 <[email protected]>

iio: adc: ad7768-1: fix MOSI idle state

b21a7fd

All supported parts require that the MOSI line stays high while in idle. Signed-off-by: jonathanns <[email protected]>

arm: dts: zynq-zed-adv7511-ad7768-1-evb: Fix sync-in GPIO number and add

3b64cf2

new required property Update sync-in GPIO number according to the current HDL version. Add low-latency property. Signed-off-by: jonathanns <[email protected]>

arm: dts: Add device tree for ADAQ7767-1 on ZedBoard

dce6e5a

Enables using the ADAQ7767-1 device on ZedBoard with FMC connector. Signed-off-by: jonathanns <[email protected]>

arm: dts: Add device tree for ADAQ7768-1 on ZedBoard

9ca6733

Enables using the ADAQ7768-1 device on ZedBoard with FMC connector. Signed-off-by: jonathanns <[email protected]>

arm: dts: Add device tree for ADAQ7769-1 on ZedBoard

9a4ca47

Enables using the ADAQ7769-1 device on ZedBoard with FMC connector. Signed-off-by: jonathanns <[email protected]>

dt-bindings: iio: adc: ad7768-1: add new supported parts and properties

9432dff

Add compatibles for supported parts in the ad7768-1 family: ADAQ7767-1, ADAQ7768-1 and ADAQ7769-1 Add new properties: adi,low-latency and adi,gain-milli Signed-off-by: jonathanns <[email protected]>

jonathanns force-pushed the dev_adaq77682 branch from 4a14e55 to 9432dff Compare August 30, 2024 21:11

nunojsa reviewed Sep 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for adaq776x-1 series and add missing features #2587

Add support for adaq776x-1 series and add missing features #2587

jonathanns commented Aug 27, 2024

nunojsa left a comment

nunojsa Sep 23, 2024

jonathanns Sep 24, 2024

nunojsa Sep 23, 2024

nunojsa Sep 23, 2024

jonathanns Sep 24, 2024

nunojsa Sep 23, 2024

nunojsa Sep 23, 2024

jonathanns Sep 24, 2024

nunojsa Oct 1, 2024

nunojsa Sep 23, 2024

jonathanns Sep 24, 2024

nunojsa Oct 1, 2024

nunojsa Sep 23, 2024

jonathanns Sep 24, 2024

nunojsa Sep 23, 2024

jonathanns Sep 24, 2024 •

edited

Loading

nunojsa Sep 23, 2024

jonathanns Sep 24, 2024

nunojsa Sep 23, 2024

jonathanns Sep 24, 2024

jonathanns commented Sep 24, 2024

Add support for adaq776x-1 series and add missing features #2587

Are you sure you want to change the base?

Add support for adaq776x-1 series and add missing features #2587

Conversation

jonathanns commented Aug 27, 2024

PR Description

PR Type

PR Checklist

nunojsa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanns Sep 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanns commented Sep 24, 2024

jonathanns Sep 24, 2024 •

edited

Loading