Add ad7606x_pi driver #2158

alin724 · 2023-03-30T08:24:32Z

Supported AD7606x family devices in this driver:

Digital interface mode - Parallel:

ad7606b - datasheet;
ad7606c-16 - datasheet;
ad7606c-18 - datasheet;

Eval boards - digital interface mode - parallel:

EVAL-AD7606B-FMCZ - guide, page;
EVAL-AD7606CFMCZ - 16b and 18b variants guide, page;

HDL project and control module for parallel interface - link-project, link-control_module;

amiclaus

please address first the build issues (cppcheck/dt-bindings check) and also rearrange the commits:

dt-bindings
driver support
Kconfig.adi
dts

alin724 · 2023-04-11T06:58:42Z

V2:

changed the commits' order;
solved issues from the checkpatch part, related to code warnings;

Documentation/devicetree/bindings/iio/adc/adi,ad7606x_pi.yaml

drivers/iio/adc/ad7606x_pi.c

amiclaus · 2023-04-11T08:58:34Z

drivers/iio/adc/ad7606x_pi.c

+	if (ret)
+		return ret;
+
+	dev_info(&pdev->dev, "ADI AIM (0x%X) at 0x%08llX mapped to 0x%p, probed ADC %s as %s\n",


is this dev_info really needed? We can reduce the number of code lines and return directly: return iio_device_register(indio_dev);

This can be removed, but in this case the user won't have a message in dmesg when the probe function is successfully completed, as in the case of using cf_axi_adc_core for ADCs connected to a SPI bus.

amiclaus · 2023-04-11T09:00:15Z

drivers/iio/adc/ad7606x_pi.c

+
+	ad7606x_pi_gpio_request(pdev, ad7606x_pi);
+
+	ad7606x_pi->vref = devm_regulator_get(&pdev->dev, "vref");


not sure if we have this in our tree, but you might want to have a look at devm_regulator_get_enable

I think that devm_regulator_get_enable function is not available in the current version of the master branch - link.

amiclaus · 2023-04-18T11:30:29Z

Documentation/devicetree/bindings/iio/adc/adi,ad7606x_pi.yaml

+                dma-names = "ad7606x-pi-adc-dma";
+                pwms = <&axi_pwm_gen 0 0>;
+                pwm-names = "cnvst_n";
+                vref = "ad7606x_pi_vref";


is this a regulator? if so, please use the -supply suffix. (example: fc747a3)

And it needs a handle, not a string.

Removed, is not necessary in FMC-based evaluation board.

Documentation/devicetree/bindings/iio/adc/adi,ad7606x_pi.yaml

pcercuei · 2023-04-18T12:19:06Z

drivers/iio/adc/ad7606x_pi.c

+
+static int ad7606x_pi_gpio_request(struct platform_device *pdev, struct ad7606x_pi_state *ad7606x_pi)
+{
+	ad7606x_pi->adc_serpar = devm_gpiod_get_optional(&pdev->dev, "adc_serpar", GPIOD_OUT_LOW);


All of these should be vendor properties, e.g. adi,adc_serpar.
Unless they are already used like that in the wild, and in that case it's too late. But DT maintainers won't be happy.

alin724 · 2023-04-25T15:15:10Z

V3:

Updated indentation for gpio array [adi,adc_os (dts, bindings)];
Updated properties to a vendor-based version [driver, dts files];

Documentation/devicetree/bindings/iio/adc/adi,ad7606x_pi.yaml

nunojsa · 2023-06-07T14:53:58Z

Documentation/devicetree/bindings/iio/adc/adi,ad7606x_pi.yaml

+      Must be the device tree identifier of the over-sampling
+      mode pins. As the line is active high, it should be marked
+      GPIO_ACTIVE_HIGH to enter ADC Software Mode.
+    maxItems: 3


all the above are gpios.... So remove the vendor prefix. And you should also have the -gpios suffix. Look at other examples.

The description is also a bit confusing. Avoid using things like device tree identifier. Call it property or something else.

"it should be marked GPIO_ACTIVE_HIGH to enter ADC Software Mode."

The above also seems to be redundant to me.... Saying active high should be more than enough

nunojsa · 2023-06-07T14:54:19Z

Documentation/devicetree/bindings/iio/adc/adi,ad7606x_pi.yaml

+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/pwm/pwm.h>
+    ad7606x_pi: adc@44a00000 {


just adc@...

nunojsa · 2023-06-07T14:56:17Z

drivers/iio/adc/Kconfig

@@ -211,6 +211,17 @@ config AD7606_IFACE_SPI
 	  To compile this driver as a module, choose M here: the
 	  module will be called ad7606_spi.

+config AD7606X_PI


remove the PI... Also It's not that usual to have names like AD7606X. The 'X' is the odd part of it. Typically one just picks one of the devices (supported by the family) and uses that for the driver. Maybe use ad7606 as no-Os?

https://wiki.analog.com/resources/tools-software/uc-drivers/ad7606

nunojsa · 2023-06-07T14:56:48Z

drivers/iio/adc/ad7606x_pi.c

@@ -0,0 +1,477 @@
+// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)


BSD-3-Clause is not usually in here (only on the bindings). Was this on purpose?

nunojsa · 2023-06-07T14:57:18Z

drivers/iio/adc/ad7606x_pi.c

+	.probe          = ad7606x_pi_probe,
+	.driver         = {
+		.name   = "ad7606x_pi",
+		.owner = THIS_MODULE,


not needed...

nunojsa · 2023-06-07T14:59:52Z

drivers/iio/adc/ad7606x_pi.c

+
+	/* Reset all HDL Cores */
+	axiadc_write(ad7606x_pi, ADI_REG_RSTN, 0);
+	axiadc_write(ad7606x_pi, ADI_REG_RSTN, ADI_RSTN);


Before jumping in more in the review I need to bring this up... Why do we have this driver like this? Typically these drivers are connected to:

https://github.com/analogdevicesinc/linux/blob/master/drivers/iio/adc/cf_axi_adc_core.c

Any reason for not doing it here? It looks to me that we are duplicating code (as the above reset)

@nunojsa This one would be a driver for interfacing the AD7606B/C-16/18 devices parallel interface, in terms of data path using specific data pins and read/write registers by using RD/WR and data pins of the interface (the configuration using the registers would be done by using the parallel interface operation mode).

I see the design is just using the parallel interface (implemented in HW) so a platform device is likely the only way to go.

However, my question is another one... Why don't we have the platform device just doing the specifs things it has to (for this project) and attaches to the cf_axi_adc_core.c as we do in most of the designs?

For instance, the implementation you have in ad7606x_pi_configure_ring_stream() is pretty much duplicating what we already have in ad7606x_pi_configure_ring_stream() and that is far from being great

Yes, implementing only the specific things for the parallel interface and attaching the common part to the cf_axi_adc_core would be better.

Hi @nunojsa, we reworked the driver for ad7606 parallel interface so now it integrates with the linux cf_axi_adc_core.c driver as suggested.
Do you have some time to have a look at it?

machschmitt · 2023-08-15T18:22:17Z

Driver has been reworked to integrate with the AXI ADC IP core 1 in linked mode.

First patches update the AXI ADC core driver itself to enable it to link with platform device drivers.
Then we add device tree doc, AD7606C-16/-18 support, and dts files.

The AXI ADC interface update was driven by a spatch semantic patch 2 with a few manual tweaks to pass checkpatch.

This set provides support for AD7606C-16/-18 as converter frontend ADCs integrated with the AXI ADC IP core Linux driver.

The semantic patch can be applied over ongoing/unmerged work to update code to the new interface.

drivers/iio/adc/ad7606_conv.c

machschmitt · 2023-09-22T12:53:07Z

The latest push has changes to make it pass dt_binding_check (at least locally, I don't understand why it's not passing dt_binding_check pipeline). There is a checkpatch CHECK: Macro argument reuse which doesn't seem to be a problem but Azure pipeline complains anyway.

Change log V5 -> V6:

Added of compatible string "adi,axi-ad7606-1.0" in cf_axi_adc_core.c
Updated dt binding doc and dts files to match the new compatible string

drivers/iio/adc/cf_axi_adc_core.c

Documentation/devicetree/bindings/iio/adc/adi,axi-ad7606.yaml

drivers/iio/adc/ad7606_conv.c

bspguy · 2023-11-29T08:49:44Z

@alin724
I tried building this branch as you suggested here

I got kernel build errors from the ad7606_conv.c file, about undefined reference to axiadc_read/axiadc_write calls.
I resolved those errors by adding the CF_AXI_ADC symbol to the kernel .config.
It adds the Analog Devices High-Speed AXI driver core.
Once added the kernel is built successfully.
Perhaps you overlooked it because IIO_ALL_ADI_DRIVERS was selected in your configuration.
Anyway it should be selected also if AD7606_CONV=y

Thanks
Roy

mhennerich · 2023-12-04T15:21:57Z

Wondering what is blocking this PR at the moment.
Mainline work? Did we start sending it, I don't think I have seen anything on the mailing list.

Change the axiadc_converter device field to store a generic Linux device instead of a SPI device so converters that do not probe as SPI devices will also be able to integrate with the Linux AXI ADC IP CORE driver. Most changes generated with spatch semantic patch. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

Search for platform devices if the AXI ADC core fails to find a converter frontend on the SPI bus. This allows the AXI ADC core to link with converters that probe as platform devices. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

Add update_scan_mode() hook function for converters that may need to do something different while updating the buffer scan mask before buffered readings. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

Add device tree documentation for the axi-ad7606. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> Signed-off-by: alin724 <alin-tudor.sferle@analog.com>

Add support for the parallel interface of AD7606B,C-16/18_FMCZ boards. AD7606 devices will be supported as converter frontend ADCs integrated with the AXI ADC IP core Linux driver. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> Signed-off-by: alin724 <alin-tudor.sferle@analog.com>

Add device tree source files for using ad7606C-16/-18 integrated with the AXI ADC IP core. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> Signed-off-by: Alin-Tudor Sferle <alin-tudor.sferle@analog.com>

Add AD7606_CONV to the IIO_ALL_ADI_DRIVERS Kconfig. Signed-off-by: alin724 <alin-tudor.sferle@analog.com>

machschmitt · 2023-12-04T21:36:38Z

Applied all suggested changes.
The first patch(es) update the AXI ADC core driver to enable it to link with platform device drivers which required a lot of collateral changes in other ADC drivers. If possible, I think it would be sensible to check at least a few of them in case I have missed anything. If everything is ok, I can work on upstreaming it.

Change log v6 -> v7:

Dropped MAINTAINERS file changes
Assigned axiadc_dev->owner
Merged GPIO and reset into a single function and released GPIO after use
Code style tidy up
Device tree doc nits

bspguy · 2023-12-14T10:14:30Z

Hi
What is the correlated HDL branch for testing this PR?
Is it https://github.com/analogdevicesinc/hdl/tree/update_ad7606_spi_engine ?
Should I use it only in parallel mode? (with the AD7606c-16).

machschmitt · 2023-12-14T12:41:45Z

Hi @bspguy,

From PR description, it looks like the HDL branch for the parallel interface is ad7606x_fmc.

bspguy · 2023-12-14T13:30:04Z

I think this PR uses the spi engine which this https://github.com/analogdevicesinc/hdl/tree/master/projects/ad7606x_fmc hdl design doesn't.
The https://github.com/analogdevicesinc/hdl/tree/update_ad7606_spi_engine does use the spi engine.
Am I wrong and the PR description is correct?
@alin724

alin724 · 2023-12-14T13:55:32Z

I think this PR uses the spi engine which this https://github.com/analogdevicesinc/hdl/tree/master/projects/ad7606x_fmc hdl design doesn't. The https://github.com/analogdevicesinc/hdl/tree/update_ad7606_spi_engine does use the spi engine. Am I wrong and the PR description is correct? @alin724

Hi @bspguy,
This PR supports only the parallel interface that is present in the ad7606x project from the HDL repository. [this driver and related devicetrees for 16/18b will have the support for the parallel interface and different configurations using the device's regmap

using the commands indicated in the https://wiki.analog.com/resources/fpga/docs/axi_adc_ip
ADC_CONFIG_CTRL register for read/write operations and ADC_CONFIG_WR for data that will be written to the device's register using the parallel interface's data pins).

bspguy · 2023-12-14T16:08:43Z

Thanks @alin724 !
I would appreciate your assistance on this issue - https://ez.analog.com/data_converters/precision_adcs/f/q-a/575949/ad7606c-16-device-tree-for-zcu102

Or perhaps refer me to someone who could help.

Thanks!

machschmitt · 2023-12-20T17:33:55Z

Hi, if there are no more suggestions about this patch series maybe we could merge it into master.
@nunojsa, would you like me to make any other change to this?

nunojsa · 2024-01-15T11:41:31Z

Hi, if there are no more suggestions about this patch series maybe we could merge it into master.
@nunojsa, would you like me to make any other change to this?

Maybe this one can wait on the backend stuff I'm working on. Then, we don't need more unnecessary changes in the cf drivers. Plus, we can upstream it

machschmitt · 2024-01-15T12:55:06Z

Maybe this one can wait on the backend stuff I'm working on. Then, we don't need more unnecessary changes in the cf drivers. Plus, we can upstream it

Sure, I've worked with these converter front ends enough to not be willing to rework the drivers again after the iio-backend framework gets accepted upstream :)

For note, AD3552R (CN0585 project) (PR #2194) is another driver that shall benefit from the iio-backend fw (the DDS backend part of it).

nunojsa · 2024-01-15T13:02:30Z

For note, AD3552R (CN0585 project) (PR #2194) is another driver that shall benefit from the iio-backend fw (the DDS backend part of it).

Yeps, I noticed we have some pending PRs that can use it. So better wait for it and rework those PULLs afterwards

bspguy · 2024-01-25T09:20:23Z

I have tested @alin724's PR on the Xilinx ZCU102 board with the EVAL-ad7606CFMCZ.
The driver works as expected 💯
If you wish I can prepare a PR for the HDL project and the device-tree modifications needed for it to work on the Xilinx board.
Thanks!

machschmitt · 2024-01-25T14:23:21Z

I have tested @alin724's PR on the Xilinx ZCU102 board with the EVAL-ad7606CFMCZ. The driver works as expected 💯 If you wish I can prepare a PR for the HDL project and the device-tree modifications needed for it to work on the Xilinx board. Thanks!

Hi @bspguy, that's great.
Though, if you made any changes to files included in the PR (e.g. devicetree files) you should comment here (do a review) and tell what the changes/fixes are so we can have it all in this PR (not spamming PRs about the same project).

Thanks for reviewing/testing ad7606 parallel interface project.

bspguy · 2024-03-31T07:04:18Z

Hi @machschmitt
I have not made any changes to files in this PR but actually made a new separate dts file for the AD7606c-16 to support the ZCU102, which should be placed under /arch/arm64/boot/dts/xilinx/ in the kernel tree.
Shouldn't that be on a separate PR?
Thanks
Roy

bspguy · 2024-04-11T20:38:39Z

Hi @machschmitt

I have not made any changes to files in this PR but actually made a new separate dts file for the AD7606c-16 to support the ZCU102, which should be placed under /arch/arm64/boot/dts/xilinx/ in the kernel tree.

Shouldn't that be on a separate PR?

Thanks

Roy

Waiting for you guys to answer..
Thanks
Roy

machschmitt · 2024-04-11T22:11:38Z

Hello @bspguy,

I have not made any changes to files in this PR but actually made a new separate dts file for the AD7606c-16 to support the ZCU102, which should be placed under /arch/arm64/boot/dts/xilinx/ in the kernel tree.
Shouldn't that be on a separate PR?

Oh, okay. Yes, device tree and device tree overlay files are often submitted in separate PRs. Although, it looks like this PR is not going to be merged because we are going to have new kernel interfaces between the DAC IP core and DAC devices upstream. We (I) must update the ad3552r driver to use the new DAC IP interface (adi-axi-dac) and submit the changes upstream.
Because of this update, the device tree nodes for these devices might change a little bit and so I would recommend waiting until the changes get upstream to then update the additional dts files and create a PR with them.

alin724 force-pushed the dev_ad7606_pi branch 3 times, most recently from a58ad48 to ffa9148 Compare March 30, 2023 11:05

alin724 marked this pull request as ready for review March 30, 2023 12:33

alin724 force-pushed the dev_ad7606_pi branch 2 times, most recently from 2b8d805 to d225814 Compare March 31, 2023 06:27

amiclaus requested a review from a team April 7, 2023 10:08

amiclaus reviewed Apr 7, 2023

View reviewed changes

alin724 force-pushed the dev_ad7606_pi branch 3 times, most recently from 8dd579a to 39ab137 Compare April 10, 2023 05:18

amiclaus reviewed Apr 11, 2023

View reviewed changes

alin724 force-pushed the dev_ad7606_pi branch from 39ab137 to f7c9a64 Compare April 11, 2023 19:36

amiclaus reviewed Apr 18, 2023

View reviewed changes

pcercuei reviewed Apr 18, 2023

View reviewed changes

alin724 force-pushed the dev_ad7606_pi branch 2 times, most recently from 5d9459b to edfed0b Compare April 25, 2023 14:43

alin724 requested review from amiclaus and pcercuei May 2, 2023 11:29

nunojsa reviewed Jun 7, 2023

View reviewed changes

machschmitt force-pushed the dev_ad7606_pi branch 2 times, most recently from 6ef2fbd to 805030f Compare August 15, 2023 18:11

machschmitt requested a review from nunojsa August 15, 2023 18:25

commodo reviewed Aug 16, 2023

View reviewed changes

drivers/iio/adc/ad7606_conv.c Outdated Show resolved Hide resolved

commodo reviewed Aug 16, 2023

View reviewed changes

drivers/iio/adc/ad7606_conv.c Show resolved Hide resolved

commodo reviewed Aug 16, 2023

View reviewed changes

drivers/iio/adc/ad7606_conv.c Outdated Show resolved Hide resolved

commodo reviewed Aug 16, 2023

View reviewed changes

drivers/iio/adc/ad7606_conv.c Outdated Show resolved Hide resolved

nunojsa reviewed Oct 31, 2023

View reviewed changes

alin724 force-pushed the dev_ad7606_pi branch from bf52a5a to 2c7ff04 Compare November 29, 2023 09:09

machschmitt and others added 4 commits December 4, 2023 15:29

iio: adc: cf_axi_adc: Add update_scan_mode hook for converters

58ca39e

Add update_scan_mode() hook function for converters that may need to do something different while updating the buffer scan mask before buffered readings. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com>

dt-bindings: iio: adc: Add axi-ad7606 doc

be03e64

Add device tree documentation for the axi-ad7606. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> Signed-off-by: alin724 <alin-tudor.sferle@analog.com>

machschmitt force-pushed the dev_ad7606_pi branch from 2c7ff04 to 930e62f Compare December 4, 2023 21:01

alin724 and others added 3 commits December 4, 2023 18:07

arm: dts: Add device tree files for axi-ad7606

5863210

Add device tree source files for using ad7606C-16/-18 integrated with the AXI ADC IP core. Signed-off-by: Marcelo Schmitt <marcelo.schmitt@analog.com> Signed-off-by: Alin-Tudor Sferle <alin-tudor.sferle@analog.com>

drivers: iio: Kconfig.adi: add ad7606_conv

6ece30f

Add AD7606_CONV to the IIO_ALL_ADI_DRIVERS Kconfig. Signed-off-by: alin724 <alin-tudor.sferle@analog.com>

machschmitt force-pushed the dev_ad7606_pi branch from 930e62f to 6ece30f Compare December 4, 2023 21:08

SRaus changed the base branch from master to main January 9, 2024 09:49


		ad7606x_pi_gpio_request(pdev, ad7606x_pi);

		ad7606x_pi->vref = devm_regulator_get(&pdev->dev, "vref");

		@@ -0,0 +1,477 @@
		// SPDX-License-Identifier: (GPL-2.0-only OR BSD-3-Clause)

Add ad7606x_pi driver #2158

Are you sure you want to change the base?

Add ad7606x_pi driver #2158

Conversation

alin724 commented Mar 30, 2023

amiclaus left a comment

Choose a reason for hiding this comment

alin724 commented Apr 11, 2023

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

alin724 commented Apr 25, 2023 • edited

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

alin724 Jun 10, 2023 • edited

Choose a reason for hiding this comment

nunojsa Jun 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

machschmitt commented Aug 15, 2023

machschmitt commented Sep 22, 2023

bspguy commented Nov 29, 2023

mhennerich commented Dec 4, 2023

machschmitt commented Dec 4, 2023

bspguy commented Dec 14, 2023 • edited

machschmitt commented Dec 14, 2023

bspguy commented Dec 14, 2023 • edited

alin724 commented Dec 14, 2023 • edited

bspguy commented Dec 14, 2023

machschmitt commented Dec 20, 2023

nunojsa commented Jan 15, 2024

machschmitt commented Jan 15, 2024 • edited

nunojsa commented Jan 15, 2024

bspguy commented Jan 25, 2024

machschmitt commented Jan 25, 2024

bspguy commented Mar 31, 2024

bspguy commented Apr 11, 2024 • edited

machschmitt commented Apr 11, 2024

alin724 commented Apr 25, 2023 •

edited

alin724 Jun 10, 2023 •

edited

nunojsa Jun 12, 2023 •

edited

bspguy commented Dec 14, 2023 •

edited

bspguy commented Dec 14, 2023 •

edited

alin724 commented Dec 14, 2023 •

edited

machschmitt commented Jan 15, 2024 •

edited

bspguy commented Apr 11, 2024 •

edited