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)

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.

drivers/iio/adc/ad7768-1.c

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.

drivers/iio/adc/ad7768-1.c

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 · 2024-10-14T21:19:25Z

v2:

Re-arranged commits to prioritize fixes and include preparatory patches.
Ported Multiple scan type feature from upstream.
Added support for the Multiple Scan Type feature, allowing IIO context changes at runtime. This replaces the low-latency property in the device tree.
Restored the Sampling Frequency Available attribute. While it still controls decimation and the MCLK divider, it no longer affects filter mode, which is now managed by a separate attribute. Consequently, the decimation and MCLK divider attributes have been removed.
The Sampling Frequency Available attribute now shows a range of values instead of a list, to include all supported sampling frequencies, including those for the Sinc3 filter.
Filter mode attribute more aligned to AD4130's ABI.
Dropped unrelated and unecessary changes.

machschmitt

This PR adds support for a lot of stuff.
In some patch I saw something like Co-authored-by when the official tag is Co-developed-by. See Documentation/process/submitting-patches.rst.
The dt-bindings patch should come before the updates to the dts files.
dts files should follow the documentation provided by the dt (binding) doc.
Patches can probably become more concise and cleaner.
Anyways, looks like you're on the right track for it.

machschmitt · 2024-10-18T20:14:31Z

drivers/iio/adc/ad7768-1.c

@@ -898,14 +1141,19 @@ static int ad7768_setup(struct ad7768_state *st)
 		return ret;

 	st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in",
-					  GPIOD_OUT_LOW);
+					  GPIOD_IN);


Hi @jonathanns, sync_in (adi,sync-in-gpios) is already defined in upstream dt doc and driver as an output pin (GPO). Changing it to input now would break users of the current upstream implementation (if any). Datasheet page 21 says "SYNC_IN receives the synchronization signal from SYNC_OUT or from the main controller." So, maybe define some dt property e.g. adi,sync-out-to-sync-in (name up to you), then check for that. If adi,sync-out-to-sync-in is absent, get and activate gpio_sync_in when appropriate (current implementation). If the property is present, do not touch gpio_sync_in and do things the new way.

machschmitt · 2024-10-18T20:28:47Z

drivers/iio/adc/ad7768-1.c

+	unsigned int val, mclk_div_value;
+	int ret;
+
+	mutex_unlock(&st->lock);


unlocking the mutex before reg read? mutex_lock(&st->lock);?

My mistake. I will fix that

machschmitt · 2024-10-18T20:38:33Z

drivers/iio/adc/ad7768-1.c

+	ret = ad7768_spi_reg_read(st, AD7768_REG_POWER_CLOCK, &val, 1);
+	if (ret)
+		goto out;
+	mclk_div_value = (val & 0xcf) | AD7768_PWR_MCLK_DIV(mclk_div);


Why do we bit and with 0xCF?
Wondering if FIELD_PREP() could be used here.
e.g.

#define AD7768_IMPORTANT_REG_FIELD 0xCF ... ret = ad7768_spi_reg_read(st, AD7768_REG_POWER_CLOCK, &val, 1); if (ret) goto out; mclk_div_value &= ~AD7768_IMPORTANT_REG_FIELD mclk_div_value |= FIELD_PREP(AD7768_IMPORTANT_REG_FIELD, val);

see include/linux/bitfield.h for more examples.

machschmitt · 2024-10-18T20:45:38Z

drivers/iio/adc/ad7768-1.c

+	if (ret < 0)
+		goto out;
+	ret = ad7768_spi_reg_write(st, AD7768_REG_SINC3_DEC_RATE_LSB, dec_rate_lsb);
+
+out:
+	mutex_unlock(&st->lock);
+
+	return ret;


nitpick: blank lines after error returns make it easier to spot what is error handling code and what is not.
If you don't want too many blank lines in the code, it's better to bring statements at same indentation level closer. e.g.

if (ret < 0) goto out; ret = ad7768_spi_reg_write(st, AD7768_REG_SINC3_DEC_RATE_LSB, dec_rate_lsb); out: mutex_unlock(&st->lock); return ret;

machschmitt · 2024-10-18T20:50:07Z

drivers/iio/adc/ad7768-1.c

+	if (ret)
+		goto out;
+	mode = (val & 0x8f) | AD7768_DIG_FIL_FIL(filter_mode);


What about using FIELD_PREP() here too?

machschmitt · 2024-10-18T22:04:39Z

arch/arm/boot/dts/zynq-zed-adv7511-ad7768-1-evb.dts

@@ -83,7 +83,7 @@
 			spi-cpol;
 			spi-cpha;
 			vref-supply = <&vref>;
-			adi,sync-in-gpios = <&gpio0 88 GPIO_ACTIVE_LOW>;
+			adi,sync-in-gpios = <&gpio0 92 GPIO_ACTIVE_LOW>;


Why do you need to update this?
Didn't you say sync-out was connected to sync-in?

machschmitt · 2024-10-18T22:06:25Z

arch/arm/boot/dts/zynq-zed-adv7511-adaq7767-1-evb.dts

+			spi-cpol;
+			spi-cpha;
+			vref-supply = <&vref>;
+			adi,sync-in-gpios = <&gpio0 92 GPIO_ACTIVE_LOW>;


Will the sync-in GPIO be needed if SYNC_OUT is connected to SYNC_IN?

machschmitt · 2024-10-18T22:06:59Z

arch/arm/boot/dts/zynq-zed-adv7511-adaq7768-1-evb.dts

+			spi-cpol;
+			spi-cpha;
+			vref-supply = <&vref>;
+			adi,sync-in-gpios = <&gpio0 92 GPIO_ACTIVE_LOW>;


same question here, sync-in needed?

machschmitt · 2024-10-18T22:07:31Z

arch/arm/boot/dts/zynq-zed-adv7511-adaq7769-1-evb.dts

+			spi-cpol;
+			spi-cpha;
+			vref-supply = <&vref>;
+			adi,sync-in-gpios = <&gpio0 92 GPIO_ACTIVE_LOW>;


sync-in needed?

machschmitt · 2024-10-18T22:10:45Z

Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml

+      with IN1_AAF, IN2_AAF, or IN3_AAF on the ADAQ7769-1.
+    $ref: /schemas/types.yaml#/definitions/uint16
+    enum: [143, 364, 1000]
+
  adi,sync-in-gpios:
    maxItems: 1
    description:


good to also have a dt check for the gain property.

allOf: # Gain property only applies to ADAQ devices - if: properties: compatible: not: contains: enum: - adi,adaq7767-1 - adi,adaq7768-1 - adi,adaq7769-1 then: properties: adi,gain-milli: false

machschmitt · 2024-10-18T22:23:00Z

Also, FWIW, the updates to adaq7768-1 driver use the new MOSI idle feature which is not included in this PR but is included in #2621. So that's probably the cause of some of the CI checks never passing here.

machschmitt · 2024-10-23T02:34:59Z

Get Outlook for Android<https://aka.ms/AAb9ysg>

________________________________ From: Jonathan Santos ***@***.***> Sent: Monday, October 21, 2024 12:40:54 PM To: analogdevicesinc/linux ***@***.***> Cc: Schmitt, Marcelo ***@***.***>; Review requested ***@***.***> Subject: Re: [analogdevicesinc/linux] Add support for adaq776x-1 series and add missing features (PR #2587) [External] @jonathanns commented on this pull request.

________________________________ In drivers/iio/adc/ad7768-1.c<https://urldefense.com/v3/__https://github.com/analogdevicesinc/linux/pull/2587*discussion_r1809063399__;Iw!!A3Ni8CS0y2Y!_gq45BQs1W4FdBJIyq6hlxteUGYfF3_B7SOw931waHjWBSgxpYR3SsAkJqR6ow7DhpNRHK-nuOSGyvVHylUE8I_vUnB2$>:

@@ -898,14 +1141,19 @@ static int ad7768_setup(struct ad7768_state *st)

return ret; st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in", - GPIOD_OUT_LOW); + GPIOD_IN); I see. What if I remove the adi,sync-in-gpios from the required properties and if it is not present, I use the SPI SYNC instead? Of course that would require the SYNC_IN to be connected to SYNC_OUT in the board. The dt-binding can have a allOf/oneOf clause. Something like: required: allOf: - reg - other required props ... - oneOf: - adi,sync-in-gpios - adi,sync-out-to-sync-in correct syntax may be different from the above. Additional checks may be included if needed or wanted. Example: allOf: - if adi,sync-in-gpios; - then adi,sync-out-to-sync-in: false; Have a look at other bindings to get the correct syntax for those checks. — Reply to this email directly, view it on GitHub<https://urldefense.com/v3/__https://github.com/analogdevicesinc/linux/pull/2587*discussion_r1809063399__;Iw!!A3Ni8CS0y2Y!_gq45BQs1W4FdBJIyq6hlxteUGYfF3_B7SOw931waHjWBSgxpYR3SsAkJqR6ow7DhpNRHK-nuOSGyvVHylUE8I_vUnB2$>, or unsubscribe<https://urldefense.com/v3/__https://github.com/notifications/unsubscribe-auth/A5OAY4CBDX3CQKMIA6VGYIDZ4UOABAVCNFSM6AAAAABNHCV5FWVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDGOBSGUYTEMBUGA__;!!A3Ni8CS0y2Y!_gq45BQs1W4FdBJIyq6hlxteUGYfF3_B7SOw931waHjWBSgxpYR3SsAkJqR6ow7DhpNRHK-nuOSGyvVHylUE8McodytN$>. You are receiving this because your review was requested.Message ID: ***@***.***>

jonathanns · 2024-10-25T20:54:03Z

v3:

Added a new adi,sync-in-spi spi property. Now the user can choose between GPIO and SPI synchronization. The option to omit both is also supported for cases with multiple devices, where the synchronization signal comes from a external source, such as a "master" device.
Introduced additional checks in the DT bindings.
Removed changes to the sync-in gpio number. Turns out that there was no reason for that and this has been reverted in the HDL.
Addressed some code style issues.
Removed the multiplaction of vref by 2 and reduced the precision by one. But still, how do I check if the input is Bipolar or not? From what i've seen in similar devices, we just assume the Bipolar case, unless the ADC have some configuration register.
Replaced Co-authored-by for Co-developed-by

machschmitt · 2024-10-29T11:59:36Z

drivers/iio/adc/ad7768-1.c

+	if (st->en_spi_sync)
+		ret = ad7768_spi_reg_write(st, AD7768_REG_SYNC_RESET, 0x00);


Why not returning here?
return ad7768_spi_reg_write(st, AD7768_REG_SYNC_RESET, 0x00);

machschmitt · 2024-10-29T12:00:14Z

drivers/iio/adc/ad7768-1.c

+		gpiod_set_value(st->gpio_sync_in, 1);
+		gpiod_set_value(st->gpio_sync_in, 0);


Use gpiod_set_value_cansleep(), unless this can be called from interrupt context.

drivers/iio/adc/ad7768-1.c

machschmitt · 2024-10-29T12:26:17Z

drivers/iio/adc/ad7768-1.c

+	st->filter_mode = filter_mode;
+	ret = ad7768_spi_reg_read(st, AD7768_REG_DIGITAL_FILTER, &mode, 1);
+	if (ret)
+		goto out;


If the read fails a filter mode different from the actual one would the shown to user space.
My suggestion is to update ad7768_state only after reg write succeeded.

machschmitt · 2024-10-29T12:31:16Z

drivers/iio/adc/ad7768-1.c

+
+	mutex_lock(&st->lock);
+
+	st->pwrmode = pwr_mode;


same here, updating the state prior to running the transfers can lead to inconsistent device state in software.

machschmitt · 2024-10-29T12:43:49Z

drivers/iio/adc/ad7768-1.c

-		tx_data[0] = AD7768_RD_FLAG_MSK(AD7768_REG_ADC_DATA) << 24;
-		xfer.tx_buf = tx_data;


Why? high-speed / 16-bit transfers don't need a write buffer? It would be nice to have a comment nearby explaining it if that's the case. Also, what if the transfer is 24-bit? Is the tx buffer set and shifter somewhere else?

Both cases actually don't require the address byte if continuous read mode is enabled. However, for 16-bit mode to work correctly, it’s mandatory to remove the address byte, so we had to adjust for that.

There is already a comment in this same function regarding the continuos read mode, so I don't know why we were using the tx_data before

/* * Write a 1 to the LSB of the INTERFACE_FORMAT register to enter * continuous read mode. Subsequent data reads do not require an * initial 8-bit write to query the ADC_DATA register. */ ret = ad7768_spi_reg_write(st, AD7768_REG_INTERFACE_FORMAT, 0x01);

ok, makes sense. I would do the tx_buf remove in a separate patch since this isn't really dependent on the scan_type and add this explanation you provided as commit description/body.

machschmitt · 2024-10-29T12:59:21Z

drivers/iio/adc/ad7768-1.c

-	tx_data[0] = AD7768_WR_FLAG_MSK(addr);
-	tx_data[1] = val & 0xFF;
+	tx_data[1] = AD7768_WR_FLAG_MSK(addr);
+	tx_data[0] = val & 0xFF;


hmmm, suspicious. This changes the tx buffer from big endian to little endian. I think you want to do it because of the DMA buffer format that is in CPU endianness (little endian for our arm based test boards). Note, though, this would break users who connect the ADC to conventional SPI controllers (without offload or DMA support).
In the patch adding multiple scan_type, set BE endianness to the scan_type used without spi-engine.

.scan_type = { \ .endianness = IIO_BE \

and set CPU endianness in the scan_type that uses spi-engine with offload/DMA support.

.scan_type = { \ .endianness = IIO_CPU \

Yes, indeed. I will try this, Thanks!

machschmitt · 2024-10-29T13:00:18Z

drivers/iio/adc/ad7768-1.c

-	tx_data[0] = AD7768_RD_FLAG_MSK(addr);
+	tx_data[len] = AD7768_RD_FLAG_MSK(addr);


Undo this too when proper endianness handling is set in scan_type struct.

machschmitt · 2024-10-29T13:07:40Z

drivers/iio/adc/ad7768-1.c

@@ -1276,6 +1525,7 @@ static int ad7768_probe(struct spi_device *spi)
 	st->mclk_freq = clk_get_rate(st->mclk);
 	st->spi_is_dma_mapped = spi_engine_ex_offload_supported(spi);
 	st->irq = spi->irq;
+	st->vref = regulator_get_voltage(st->regulator);


looks like st->regulator is only to get the ADC reference voltage (vref). If that's the case, you can simplify this with devm_regulator_get_enable_read_voltage() now in ADI main.

machschmitt · 2024-10-29T13:10:48Z

Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml

@@ -65,7 +106,21 @@ required:
  - vref-supply
  - spi-cpol
  - spi-cpha
-  - adi,sync-in-gpios


don't do it all together. One patch for the new parts, another one for the updates to sync pins.
Also, the dt-binding patches should come before the driver changes that use the new dt properties.

put the gain property and related checks in the same patch that adds the new parts and put the changes related to adi,sync-in-spi property in a separate patch. That's what I wanted to say previously.

machschmitt · 2024-10-29T13:38:34Z

Hi @jonathanns, the series is looking better. A few more comments:

The patches for multiple scan_types have been ported to ADI main. Rebase on top of main so you won't need to carry them in this PR anymore.

Tip: try to add at least one imperative sentence to concisely state what patches do.
For example, the "fix MOSI idle state" patch looks good, but it's log/body doesn't really say what it's doing.
Add something like "Set SPI MOSI idle high configuration to keep the data line high when idle." to the end of the description to make it much more clear.

* Removed the multiplaction of vref by 2 and reduced the precision by one. But still, how do I check if the input is Bipolar or not? From what i've  seen in similar devices, we just assume the Bipolar case, unless the ADC have some configuration register.

ADC input type naming is confusing. I've been trying to clarify the relation between ADC input configuration and IIO support for them in this candidate doc 1373835. I still need to verify the how accurate (or not) those concepts are with other ADI engineers though. The goal is to have something to help us be less uncertain about IIO channel configuration in the future. Answering your question, an ADC channel is considered bipolar if its configuration allows it to output data that represents a negative quantity. For example, if a differential ADC has a pair of inputs (IN+ and IN-) working as a differential input, then a negative value must be provided to user space when IN+ < IN- to indicate that IN+ is below/smaller than IN-. Note that can happen when inputs are above or even below GND. When inputs can go below GND they are called true bipolar but the most important characteristic is whether the input configuration allows/requires negative values to be expressed.

jonathanns · 2024-11-06T21:21:53Z

v4:

Splitted the dt-binding patch: one for the new supported parts and other for new properties and checks.
moved dt-related patches to the start of the tree.
changed the commit messages to include at least one imperative sentence.
Moved the removal of tx_buf from spi-engine transfers in the offload to a separate patch.
Added Decimation rate attribute: preivous sampling frequency attribute obscured the trade-offs by forcing a combination of mclk_div and decimation rate.
Sampling frequency now calculates only the mclk_div value.
Power mode attribute removed. this value is set based on the value of mclk_div (as the datasheet recommends).
Changed spi register write and read to fix endianness problem.

machschmitt

Looking much better this version.

Though, there is still room for improvement.
minor commit log neat:
would avoid the word "fix" and try to be more informative when possible. e.g.
"iio: adc: ad7768-1: fix MOSI idle state" -> "iio: adc: ad7768-1: Set MOSI idle state to high"

machschmitt · 2024-11-08T12:31:11Z

arch/arm/boot/dts/zynq-zed-adv7511-adaq7767-1-evb.dts

+			clock-names = "mclk";
+			dmas = <&rx_dma 0>;
+			dma-names = "rx";
+			#io-channel-cells = <1>;


Add

#address-cells = <1>; #size-cells = <0>; channel@0 { reg = <0>; label = "channel_0"; };

as documented in Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml

machschmitt · 2024-11-08T12:32:09Z

arch/arm/boot/dts/zynq-zed-adv7511-adaq7768-1-evb.dts

+			clock-names = "mclk";
+			dmas = <&rx_dma 0>;
+			dma-names = "rx";
+			#io-channel-cells = <1>;


same here, add channel subnode.

machschmitt · 2024-11-08T12:32:21Z

arch/arm/boot/dts/zynq-zed-adv7511-adaq7769-1-evb.dts

+			clock-names = "mclk";
+			dmas = <&rx_dma 0>;
+			dma-names = "rx";
+			#io-channel-cells = <1>;


machschmitt · 2024-11-08T13:03:16Z

drivers/iio/adc/ad7768-1.c

-	st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in",
+	if (device_property_present(&st->spi->dev, "adi,sync-in-gpios")) {
+		st->en_gpio_sync = true;
+		st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in",
 					  GPIOD_OUT_LOW);
-	if (IS_ERR(st->gpio_sync_in))
-		return PTR_ERR(st->gpio_sync_in);
+		if (IS_ERR(st->gpio_sync_in))
+			return PTR_ERR(st->gpio_sync_in);
+
+	}
+
+	if (device_property_present(&st->spi->dev, "adi,sync-in-spi"))
+		st->en_spi_sync = true;


Your dt doc comment says "adi,sync-in-gpios and adi,sync-in-spi are mutually exclusive".
The code here does not guarantee they are mutually exclusive, though.
Does this fulfill the requirements?

st->en_gpio_sync = false; st->gpio_sync_in = devm_gpiod_get(&st->spi->dev, "adi,sync-in", GPIOD_OUT_LOW); if (!IS_ERR(st->gpio_sync_in)) st->en_gpio_sync = true; if (!st->en_gpio_sync && device_property_present(&st->spi->dev, "adi,sync-in-spi")) st->en_spi_sync = true;

machschmitt · 2024-11-08T14:01:59Z

drivers/iio/adc/ad7768-1.c

+	mclk_div_value &= ~(AD7768_PWR_MCLK_DIV_MSK | AD7768_PWR_PWRMODE_MSK);
+	/* Set mclk_div value */
+	mclk_div_value |= AD7768_PWR_MCLK_DIV(mclk_div);
+	/* Set power mode based on mclk_div value


neat, IIO maintainer prefers multiline comments with an empty starting line.

/* * comments ... */

https://lore.kernel.org/linux-iio/20241005174151.4bcd55f6@jic23-huawei/ (first patch that appeared when searching the string multiline comment in IIO mailing list).

machschmitt · 2024-11-08T17:47:04Z

drivers/iio/adc/ad7768-1.c


 	switch (info) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		return ad7768_set_freq(st, val);
+	case IIO_CHAN_INFO_SCALE:
+		if (!st->chip->has_pga)
+			return -EPERM;


add a blank line after return -EPERM for better code readability.

actually, I think -EOPNOTSUPP would be more appropriate error code here.

machschmitt · 2024-11-08T17:47:33Z

drivers/iio/adc/ad7768-1.c

+static const struct ad7768_chip_info adaq7768_chip_info = {
+	.name = "adaq7768-1",
+	.channel_spec = adaq776x_channels,
+	.num_channels = 1,


use ARRAY_SIZE()

machschmitt · 2024-11-08T17:52:03Z

drivers/iio/adc/ad7768-1.c

+	st->aaf_gain = AD7768_AAF_IN1;
+	ret = device_property_read_u32(&spi->dev, "adi,aaf-gain", &val);
+	if (!ret) {
+		if (!st->chip->has_variable_aaf)
+			return dev_err_probe(&spi->dev, -EINVAL,
+					     "This device does not support AAF");
+
+		switch (val) {


does it makes to first check for the AAF support and ignore adi,aaf-gain if device doesn't support it?

st->aaf_gain = AD7768_AAF_IN1; if (st->chip->has_variable_aaf) { ret = device_property_read_u32(&spi->dev, "adi,aaf-gain", &val); if (!ret) { ... } }

humm... We could go this way too. since we are setting a default value, this property is not required. The only check is for parts that do not support this feature, but I guess we could also avoid it.

machschmitt · 2024-11-08T17:54:25Z

drivers/iio/adc/ad7768-1.c

 	struct spi_device *spi;
-	struct regulator *vref;
+	int vref;


vref_uv is preferred so it is clear what voltage unit is stored.

machschmitt · 2024-11-08T18:01:41Z

Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml

@@ -65,7 +106,21 @@ required:
  - vref-supply
  - spi-cpol
  - spi-cpha
-  - adi,sync-in-gpios


put the gain property and related checks in the same patch that adds the new parts and put the changes related to adi,sync-in-spi property in a separate patch. That's what I wanted to say previously.

jonathanns · 2024-11-18T12:18:36Z

v5:

Improved some commit messages.
Added channel subnode to new devicetrees as recommended.
Made SPI and GPIO Synchronization mutually exclusive in the driver.
Fixed multiline comments.
Fixed error handling in some functions.
Fixed readability issues.
Replaced sprintf and scnprintf by sysfs_emit() and sysfs_emit_at().
Use of ARRAY_SIZE() to represent number of channels.
Added Scan type signal check to fix scale computation.
Replaced vref by vref_uv so the voltage unit is more clear.
Reorganized Documentation patches as requested: First new supported parts and aff gain, followed by adi,sync-in-spi support.

machschmitt

A few more comments. Most of them would be minor changes to the driver.

machschmitt · 2024-11-18T20:08:44Z

Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml

@@ -1,21 +1,30 @@
 # SPDX-License-Identifier: GPL-2.0
+# Copyright 2024 Analog Devices Inc.


remove the copyright line

machschmitt · 2024-11-18T20:21:02Z

arch/arm/boot/dts/zynq-zed-adv7511-adaq7767-1-evb.dts

+			spi-cpha;
+			vref-supply = <&vref>;
+			adi,sync-in-spi;
+			adi,aaf-gain = <143>;


I may have missed it but, where is the documentation for the adi,aaf-gain property?

Sorry, At some point I changed from adi,gain-milli to adi,aaf-gain. Is the first option more adequate? I can replace it

It depends on how the gain is applied. adi,gain-milli is used to describe the hardware gain applied to the ADC inputs. For ad4000 series, the gain provided by the ADC input scaler is defined by the hardware connections between chip pins OUT+, R1K-, R1K1-, R1K+, R1K1+, and OUT-. For gains set through register configuration or runtime (software) settings, IIO interfaces should be used (_scale, _callibscale). What is this AAF about? Is it related to datasheet AFE_GAIN?

AFE_gain is the combination of Anti-Aliasing Filter (AAF) gain and PGA gain. AAF is an analogic filter with three inputs, each one lead to a specific gain and bandwidth rejection. The user must connect the output of the first filter (PGA), for the case of adaq7769-1, to one of these AAF inputs by inserting a resistor in the corresponding pin:

Only the ADAQ7769-1 and ADAQ7767-1 have variable AAF gain. The second one does not have PGIA, so the AAF input is directly connected to the signal input and AFE gain is the AAF gain.

Hmm, that's a bit different than adi,gain-milli in that it also defines a bandwidth rejection. Though, to our luck, there is no IIO ABI for exposing bandwidth rejection settings to user space so I think it's okay to reuse adi,gain-milli to describe the AFF gain. Alternatively, if you want, you can instead provide a property with a different name (e.g. adi,aaf-gain) documenting it specifies both input gain and bandwidth rejection. In either case, include the documentation for it in Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml.

Edit: just recall linux is not the only user of device tree docs. Other systems may export different properties to users. Better to fully describe the analog gain and bandwidth rejection with a specific property name for it.

About the device that doesn't have variable AAF gain, I think it may be nice to add a check for it. e.g.

# Variable AAF only applicable to ADAQ7769-1 and ADAQ7767-1 - if: properties: compatible: not: contains: enum: - adi,adaq7767-1 - adi,adaq7769-1 then: properties: adi,aaf-gain: false

machschmitt · 2024-11-19T12:23:28Z

drivers/iio/adc/ad7768-1.c

+	long long val;
+	struct ad7768_state *st = iio_priv(indio_dev);
+
+	mutex_lock(&st->lock);


Also wouldn't lock on anything here. It's just a read from memory. If some other thread changes st->dec_rate after the user issues an attribute read, then they will just get a more up to date value.

machschmitt · 2024-11-19T13:06:33Z

drivers/iio/adc/ad7768-1.c

+		if (ret)
+			goto out;
+
+		if (st->filter_mode == SINC3) {


SINC3, SINC5, WIDEBAND are also enum elements. Does it make any difference if we use a switch here too?

SINC5 also includes SINC5_DEC_X8 and SINC5_DEC_X16 cases, if i use switch i have to move it to "default: ". The "else" is considering the those three values of filter_mode.

machschmitt · 2024-11-19T13:16:54Z

drivers/iio/adc/ad7768-1.c

+	/* Set Default Decimation rate */
+	ret = ad7768_set_dec_rate(st, 32);
+	if (ret < 0)
+		return ret;
+
+	/* Set Default Filter mode */
+	ret = ad7768_set_dig_fil(indio_dev, SINC5);
+	if (ret < 0)
+		return ret;
+


Didn't read much into the datasheet but, if I understand it correctly, the device can apply one of 3 digital filters to the signal, sinc5, sinc3, or wideband. Then, the decimation rates available depend on the choice of filter. Wouldn't it be more natural to first have ad7768_set_dig_fil() then ad7768_set_dec_rate(). Probalbly no practical difference in changing the call order, just trying to better understand how the device works.

Yes, we have two type of decimation, one for sinc5 and wideband and other for sinc3. We can call them in this order as well, it makes sense

machschmitt · 2024-11-19T13:19:46Z

drivers/iio/adc/ad7768-1.c

+struct ad7768_clk_configuration {
+	enum ad7768_mclk_div mclk_div;
+	enum ad7768_dec_rate dec_rate;
+	unsigned int clk_div;
+};


Hmm, this is not used in this commit. Should it have gone into a previous commit?

This must be a leftover from the time I remade all commits. I will remove it, thanks

machschmitt · 2024-11-19T13:54:35Z

drivers/iio/adc/ad7768-1.c

+	.name = "ad7768-1",
+	.channel_spec = ad7768_channels,
+	.num_channels = ARRAY_SIZE(ad7768_channels),
+	.available_masks = ad7768_channel_masks,


where is this available_masks read from chip_info?

Rigth, I am going to add it

machschmitt · 2024-11-19T14:09:34Z

drivers/iio/adc/ad7768-1.c

+			goto out;
+	}
+
+	/* Write GPIOs 0-2 with the gain mode equivalente value */


minor neat: write English comments
"Write GPIOs 0-2 with the gain mode equivalente value" -> "Write the respective gain values to GPIOs 0, 1, 2"

machschmitt · 2024-11-19T14:16:00Z

drivers/iio/adc/ad7768-1.c


 	switch (info) {
 	case IIO_CHAN_INFO_SAMP_FREQ:
 		return ad7768_set_freq(st, val);
+	case IIO_CHAN_INFO_SCALE:
+		if (!st->chip->has_pga)
+			return -EPERM;


actually, I think -EOPNOTSUPP would be more appropriate error code here.

Add compatibles for supported parts in the ad7768-1 family: ADAQ7767-1, ADAQ7768-1 and ADAQ7769-1 Add property and checks for AFF gain, supported by ADAQ7767-1 and ADAQ7769-1 parts: adi,aaf-gain Signed-off-by: Jonathan Santos <[email protected]>

Add adi,sync-in-spi property to enable synchronization over SPI. This should be used in the case when the GPIO cannot provide a pulse synchronous with the base MCLK signal. User can choose between SPI, GPIO synchronization or neither of them, but only if a external pulse can be provided, for example, by another device in a multidevice setup. Signed-off-by: Jonathan Santos <[email protected]>

Use compatible for new spi-engine implementation. Signed-off-by: Jonathan Santos <[email protected]>

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

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

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

All supported parts require that the MOSI line stays high while in idle. Configure SPI controller to set MOSI idle state to high. Fixes: 8a15c73 ("iio: adc: Add AD7768-1 ADC basic support") Signed-off-by: Jonathan Santos <[email protected]>

The synchronization method using GPIO requires the generated pulse to be truly synchronous with the base MCLK signal. When it is not possible to do that in hardware, the datasheet recommends using synchronization over SPI, where the generated pulse is already synchronous with MCLK. This requires the SYNC_OUT pin to be connected to SYNC_IN pin. Add the option to handle device synchronization over SPI. Signed-off-by: Jonathan Santos <[email protected]>

Separate filter mode and decimation rate from the sampling frequency attribute. The new filter mode attribute enables SINC3 and WIDEBAND filters, which were previously unavailable. Previously, combining decimation and MCLK divider in the sampling frequency obscured performance trade-offs. Lower MCLK divider settings increase power usage, while lower decimation rates reduce precision by decreasing averaging. By creating a decimation attribute, users gain finer control over performance. The addition of those attributes allows a wider range of sampling frequencies and more access to the device features. Co-developed-by: PopPaul2021 <[email protected]> Signed-off-by: PopPaul2021 <[email protected]> Signed-off-by: Jonathan Santos <[email protected]>

When the device is configured to Sinc5 filter and decimation x8, output data is reduced to 16-bits in order to support 1 MHz of sampling frequency due to clock limitation. Use multiple scan types feature to enable the driver to switch scan type in runtime, making possible to support both 24-bit and 16-bit resolution. Signed-off-by: Jonathan Santos <[email protected]>

The address byte is not needed in the SPI transfer if continuous read mode is enabled. In this case, data readback occurs on the application of SCLK. Remove tx_buf from IIO buffer read transfer to reduce the duration of each transfer and enable the sample rate of 1.024 MHz at 16-bit resolution, given the maximum supported SCLK rate of 20 MHz: - With address byte: (16-bit data + 8-bit address) * 1.024 MHz > 20 MHZ. Insufficient SCLK cycles to drive out the output. - Without address byte: (16-bit data) * 1.024 MHz < 20 MHz. Signed-off-by: Jonathan Santos <[email protected]>

Add support for new spi-engine implementation on ad7768-1 driver, optimizing offload transfers by reducing CS delay and minimizing the number of commands in the offload FIFO. 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. Adjust 'tx_data' buffer handling in SPI register read and write functions to ensure endianness consistency with the new spi-engine. Signed-off-by: Jonathan Santos <[email protected]>

Add Chip info struct in SPI device to store channel information for each supported part. Signed-off-by: Jonathan Santos <[email protected]>

Use devm_regulator_get_enable_read_voltage function as a standard and concise way of reading the voltage from the regulator and keep the regulator enabled. Replace the regulator descriptor with the direct voltage value in the device struct. Signed-off-by: Jonathan Santos <[email protected]>

Add support for ADAQ7767/68/69-1 series, which includes PGIA and Anti-aliasing filter (AAF) gains. The PGA gain is configured in run-time through the scale attribute, if supported by the device. The scale options are updated according to the output data width. The AAF gain is configured in the devicetree and it should correspond to the AAF channel selected in hardware. Signed-off-by: Jonathan Santos <[email protected]>

jonathanns · 2024-11-27T17:10:25Z

v6:

Replaced adi-gain-milli for adi-aaf-gain on dts binding.
Fixed dts bindings check - grouped them into the same allOf key.
Added read of available_masks on probe.
Removed copyrigth line from ad7768-1 devicetree
Removed unsed ad7768_clk_configuration struct.
Addressed other nits.

machschmitt

Looks good overall.
It will certainly change much more if submitted upstream but, for now, I think this is fairly good. With the last of my comments addressed,
Reviewed-by: Marcelo Schmitt [email protected]

machschmitt · 2024-11-28T19:43:30Z

arch/arm/boot/dts/zynq-zed-adv7511-adaq7767-1-evb.dts

+			spi-cpha;
+			vref-supply = <&vref>;
+			adi,sync-in-spi;
+			adi,aaf-gain = <143>;


adi,aaf-gain = /bits/ 16 <143>; since dt-binding specifies it is 16-bit

machschmitt · 2024-11-28T20:30:28Z

drivers/iio/adc/ad7768-1.c

@@ -1168,14 +1167,19 @@ static int ad7768_buffer_postenable(struct iio_dev *indio_dev)
 		return ret;

 	if (st->spi_is_dma_mapped) {
+		st->offload_xfer.rx_buf = rx_data;


Don't know the detailed explanation for it but I've been told to set buffers with a hacky pointer when the data is expected to travel through DMA. I suppose that would apply to the offload buffer here as well.

/* HACK: invalid pointer indicates read data from SPI offload DMA */ st->offload_xfer.rx_buf = (void*)(-1);

machschmitt · 2024-11-28T20:49:55Z

drivers/iio/adc/ad7768-1.c

+		*val = st->vref_uv / 1000;
+		if (st->chip->has_variable_aaf)
+			*val = (*val * MILLI) / ad7768_aaf_gains[st->aaf_gain];
+		*val2 = scan_type->realbits - 1;


Was about to suggest something like

if (scan_type->sign == 's') *val2 = scan_type->realbits - 1; else *val2 = scan_type->realbits;

but the ADC output code is always in two's complement format so the current *val2 assignment is correct.
If you think appropriate, add a comment like

/* ADC output code is two's complement so only (realbits - 1) bits express voltage magnitude */

nunojsa

Just some comments on top of @machschmitt . Please go ahead and usptream the bits that can be upstreamed.

nunojsa · 2024-11-29T13:44:13Z

Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml

+      properties:
+        compatible:
+          not:
+            contains:


not if the contains is needed...

nunojsa · 2024-11-29T13:45:25Z

Documentation/devicetree/bindings/iio/adc/adi,ad7768-1.yaml

+        adi,sync-in-spi: false
+  - if:
+      required:
+        - adi,sync-in-spi


Would this be something that we could assume in the absence of adi,sync-in-gpios or do we really need to be explicit?

Here's what we have decided for the synchronization:

Using GPIO: We define adi,sync-in-gpios and route the GPIO pin to the SYNC_IN pin on the board.

Using SPI command: We define the adi,sync-in-spi and route the SYNC_OUT pin to the SYNC_IN pin in the eval board.

We also have the option to do use none of them, but in this case the SYNC_IN pulse should be providaded by an external device or another source (e.g., when multiple devices are involved).

If you believe the third option doesn’t need support, we could consider the absence of adi,sync-in-gpios as indicative of the SPI command synchronization. I think it’s good to include the adi,sync-in-spi option, because it clarifies the need to route SYNC_OUT to SYNC_IN for proper operation.

nunojsa · 2024-11-29T13:47:20Z

arch/arm/boot/dts/zynq-zed-adv7511-adaq7768-1-evb.dts

+		#size-cells = <0x0>;
+
+		adaq7768_1: adc@0 {
+			compatible = "adi,adaq7768-1";


I didn't really checked but can we have a common dtsi and then dedicated dts files just for different pieces?

nunojsa · 2024-11-29T13:50:48Z

drivers/iio/adc/ad7768-1.c

@@ -689,10 +701,17 @@ static int ad7768_setup(struct ad7768_state *st)
 	if (ret)
 		return ret;

+	st->en_gpio_sync = false;
+	st->en_spi_sync = false;


The above should not be needed assuming you don't touch it after allocating your structure...

nunojsa · 2024-11-29T13:52:12Z

drivers/iio/adc/ad7768-1.c

-	if (IS_ERR(st->gpio_sync_in))
-		return PTR_ERR(st->gpio_sync_in);
+	if (!IS_ERR(st->gpio_sync_in))
+		st->en_gpio_sync = true;


You're ignoring possible error. Seems to me the gpio is now optional so treat it as such and use devm_gpiod_get_optional(). Then check for errors and for the pointer not being NULL.

You are right, that is better

nunojsa · 2024-11-29T13:59:32Z

drivers/iio/adc/ad7768-1.c

+		val = st->dec_rate;
+		break;
+	default:
+		ret = -EINVAL;


return -EINVAL

nunojsa · 2024-11-29T13:59:59Z

drivers/iio/adc/ad7768-1.c

+			goto out;
+
+		/* update clk_div and power mode */
+		ret = ad7768_set_freq(st, st->samp_freq);


I would likely have the above in an helper function...

nunojsa · 2024-11-29T14:00:57Z

drivers/iio/adc/ad7768-1.c

+	if (st->spi_is_dma_mapped)
+		return st->filter_mode == SINC5_DEC_X8 ? AD7768_SCAN_TYPE_DMA_HIGH_SPEED :
+		       AD7768_SCAN_TYPE_DMA_NORMAL;
+	else


redundant else

nunojsa · 2024-11-29T14:05:58Z

drivers/iio/adc/ad7768-1.c

+		.storagebits = 32,
+		.endianness = IIO_CPU,
+	},
+};


Just one thing that I don't understand... The filter mode was implemented in the previous patch right? So if I'm not missing nothing it seems the previous patch has a bug for some filter or decimation values? If I'm right, this should be a precursor patch...

nunojsa · 2024-11-29T14:09:06Z

drivers/iio/adc/ad7768-1.c

+		if (st->chip->has_variable_aaf) {
+			numerator *= ad7768_aaf_gains[st->aaf_gain];
+			denominator *= MILLI;
+		}


nit: newline

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

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

jonathanns force-pushed the dev_adaq77682 branch 2 times, most recently from b8a52d0 to c9f4f32 Compare October 14, 2024 21:19

machschmitt reviewed Oct 18, 2024

View reviewed changes

jonathanns force-pushed the dev_adaq77682 branch from c9f4f32 to fb7cb23 Compare October 25, 2024 20:13

machschmitt reviewed Oct 29, 2024

View reviewed changes

jonathanns force-pushed the dev_adaq77682 branch from fb7cb23 to a105f7c Compare November 6, 2024 21:11

machschmitt reviewed Nov 8, 2024

View reviewed changes

jonathanns force-pushed the dev_adaq77682 branch from a105f7c to fa7bfca Compare November 18, 2024 12:05

machschmitt reviewed Nov 19, 2024

View reviewed changes

jonathanns added 15 commits November 27, 2024 13:20

arm: dts: zynq-zed-adv7511-ad7768-1-evb: add support for new spi-engine

8a67013

Use compatible for new spi-engine implementation. Signed-off-by: Jonathan Santos <[email protected]>

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

ae77771

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

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

9ea71cc

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

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

f05aa91

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

iio: adc: ad7768-1: introduce chip info for future multidevice support

593b88a

Add Chip info struct in SPI device to store channel information for each supported part. Signed-off-by: Jonathan Santos <[email protected]>

jonathanns force-pushed the dev_adaq77682 branch from fa7bfca to fd9c009 Compare November 27, 2024 16:58

machschmitt reviewed Nov 28, 2024

View reviewed changes

nunojsa reviewed Nov 29, 2024

View reviewed changes

		if (st->en_spi_sync)
		ret = ad7768_spi_reg_write(st, AD7768_REG_SYNC_RESET, 0x00);

		gpiod_set_value(st->gpio_sync_in, 1);
		gpiod_set_value(st->gpio_sync_in, 0);

		tx_data[0] = AD7768_RD_FLAG_MSK(AD7768_REG_ADC_DATA) << 24;
		xfer.tx_buf = tx_data;

		tx_data[0] = AD7768_RD_FLAG_MSK(addr);
		tx_data[len] = AD7768_RD_FLAG_MSK(addr);

		@@ -1,21 +1,30 @@
		# SPDX-License-Identifier: GPL-2.0
		# Copyright 2024 Analog Devices Inc.

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

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 commented Oct 14, 2024

machschmitt 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

machschmitt commented Oct 18, 2024

machschmitt commented Oct 23, 2024 via email

jonathanns commented Oct 25, 2024

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

machschmitt commented Oct 29, 2024 • edited Loading

jonathanns commented Nov 6, 2024

machschmitt left a comment • 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

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

machschmitt commented Oct 29, 2024 •

edited

Loading

machschmitt left a comment •

edited

Loading