AD9213 driver #2804

danmois · May 26, 2025

PR Description

The AD9213 is a single, 12-bit, 6 GSPS/10.25 GSPS, radio frequency (RF) analog-to-digital converter (ADC) with a 6.5 GHz input bandwidth. The AD9213 supports high dynamic range frequency and time domain applications requiring wide instantaneous bandwidth and low conversion error rates (CER). The AD9213 features a 16-lane JESD204B interface to support maximum bandwidth capability.

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)

gastmaier · 2025-05-27T07:02:59Z

drivers/iio/adc/ad9213.c

+
+MODULE_AUTHOR("Dragos Bogdan <dragos.bogdan@analog.com>");
+MODULE_DESCRIPTION("Analog Devices AD9213 ADC");
+MODULE_LICENSE("GPL v2");


Suggested change

MODULE_LICENSE("GPL v2");

MODULE_LICENSE("GPL");

MODULE_IMPORT_NS("IIO_BACKEND")

nunojsa

The driver seems to be an old one that you're now adding. Please make sure to update it and make use of new kernel APIs and helpers.

Also, your commit subjects are not really good. Don't include file suffixes like .h or .c and do not include drivers: .... Just iio: ...

nunojsa · 2025-05-28T12:03:14Z

drivers/iio/frequency/adf4371.c

+	if (IS_ERR(st->clkin)) {
+		dev_err(&spi->dev, "failed to get clkin\n");
+		return PTR_ERR(st->clkin);
+	}


This makes the clock effectively required now... Shouldn't this still work without jesd?

Yes, it should. This if will be removed.

Don't remove the error check. AFAICT, the clock is indeed mandatory when this device is part of the JESD topology. Hence, I would move the above code into the if (jdev) block.

I wonder if the clock isn't in fact required, since we have below: st->clkin_freq = clk_get_rate(st->clkin);

Oh, I just did not looked to the rest of the code. The clock is indeed required but why are you getting it again? You have the above change and right below yo have a call to devm_clk_get_enabled(). So not sure why the above is needed.

True, not needed. Thank you!

nunojsa · 2025-05-28T12:04:48Z

drivers/iio/frequency/hmc7044.c

+	if (ch->num == 14)
+		return (hmc->vcxo_freq / ch->divider);
+	else
+		return hmc7044_get_clk_attr(hw, IIO_CHAN_INFO_FREQUENCY);


redundant else

nunojsa · 2025-05-28T12:05:04Z

drivers/iio/frequency/hmc7044.c

+
+		return 0;
+	} else {
+		return hmc7044_set_clk_attr(hw, IIO_CHAN_INFO_FREQUENCY, rate);


nunojsa · 2025-05-28T12:06:49Z

drivers/iio/frequency/hmc7044.c

+					      HMC7044_CH_EN);
+				if (ret)
+					return ret;
+			}


Hmm, oscout looks like an output clock signal. Or is it something else? This feels like it should be using the clock subsystem.

nit, please just use spaces for the fields. This kind of indentation often leads to unnecessary changes when new elements are added.

nunojsa · 2025-05-28T12:10:41Z

drivers/iio/adc/ad9213.c

+	/* Power up the AD9213. */
+	/* Assert a Pin Reset. */
+	gpiod_set_value_cansleep(adc->reset_gpio, 0);
+	mdelay(1);


do we really need mdelay() here? Also, it seems we have lots of comments in this function. Make sure they follow kernel style comments and are really needed

nunojsa · 2025-05-28T12:11:53Z

drivers/iio/adc/ad9213.c

+	ad9213_write(adc, AD9213_REG_PLL_ENABLE_CTRL, 0x00);
+
+	/* Write Register 0x570, Bit 0 = 1, powers up JESD204B PLL. */
+	ad9213_write(adc, AD9213_REG_PLL_ENABLE_CTRL, AD9213_PWRUP_LCPLL);


None of the calls to ad9213_write() check error codes

nunojsa · 2025-05-28T12:14:02Z

drivers/iio/adc/ad9213.c

+	}
+
+	adc->syncinb_cmos_en =
+		of_property_read_bool(np, "adi,syncinb-cmos-enable");


use device properties

nunojsa · 2025-05-28T12:14:27Z

drivers/iio/adc/ad9213.c

+static struct spi_driver ad9213_driver = {
+	.driver = {
+			.name = "ad9213",
+			.of_match_table = of_match_ptr(ad9213_of_match),


drop of_match_ptr()

nunojsa · 2025-05-28T12:14:29Z

drivers/iio/adc/ad9213.c

+	struct iio_dev *indio_dev = spi_get_drvdata(spi);
+
+	iio_device_unregister(indio_dev);
+}


no need for a .remove()

danmois · 2025-06-02T12:32:47Z

V2

remove devm_clk_get in ADF4371 driver
make OSCOUT1 IIO channel
update AD9213 driver to use regmap, device properties, cleanup.h, error handling

danmois · 2025-06-02T12:42:31Z

V3

remove HMC7044 unused code

danmois · 2025-06-03T08:08:08Z

V4

remove HMC7044 unused code in hmc7044_clk_set_rate

Add JESD FSM ops. These modifications are required if the device is added to a JESD topology. Required by the AD9213 driver, so that the initialization sequence is synchronized with the AD9213 operation. Signed-off-by: George Mois <george.mois@analog.com>

Add OSCOUT1 as an output channel to the driver. The OSCOUT1 output is used a an input for the ADF4371 on the AD9213_EVB, and is used for synchronizing the initalization of the two devices in this setup. Signed-off-by: George Mois <george.mois@analog.com>

Remove the adi,divider-ratio defines, as they are no longer used. Signed-off-by: George Mois <george.mois@analog.com>

This adds device tree bindings for the AD9213 ADC. Signed-off-by: George Mois <george.mois@analog.com>

Add driver for AD9213. Signed-off-by: AndrDragomir <andrei.dragomir@analog.com> Signed-off-by: George Mois <george.mois@analog.com>

Add AD9213 driver to adi_mb_defconfig. Signed-off-by: George Mois <george.mois@analog.com>

Make sure the AD9213 ADC is built. Signed-off-by: George Mois <george.mois@analog.com>

Add devicetree for AD9213_EVB. Signed-off-by: AndrDragomir <andrei.dragomir@analog.com> Signed-off-by: George Mois <george.mois@analog.com>

danmois · 2025-06-04T06:17:13Z

V5

formatting adjustments

gastmaier · 2025-06-10T12:04:51Z

drivers/iio/adc/ad9213.c

+
+	case AD9213_JESD204_FSM_STATE:
+		if (!adc->jdev)
+			ret = -EOPNOTSUPP;


This error case is overwritten right below on jesd204_get_active_links_num error or always by jesd204_get_links_data.
At jesd204_get_active_links_num a NULL pointer dereference would occur if the method itself didn't check if (!jdev), but it does.

I suggest to direct return on all ret = of this method, it saves some lines and avoids that.

Suggested change

ret = -EOPNOTSUPP;

return -EOPNOTSUPP;

nunojsa

Please look at my comment about commit subjects in the first version of the series. Also, can we have this without JESD FSM? What I'm thinking is to upstream the driver without FSM and then backport it with it. At least, the driver would not be completely out of tree

nunojsa · 2025-06-11T07:20:08Z

drivers/iio/frequency/hmc7044.c

-
-	hmc->oscout1_driver_impedance = HMC7044_DRIVER_IMPEDANCE_DISABLE;
-	of_property_read_u32(np, "adi,oscillator-output1-driver-impedance",
-			     &hmc->oscout1_driver_impedance);


Why removing the DT properties? I'm aware this is now done during runtime but removing these properties now will break old DTs (users) that rely on them to have an initial OSCOUT value. So, most likely having this controlled at runtime should have be done from the beginning but I would say that we're now stuck with these properties.

In the first version, this output was treated separately. Now, it is treated as an additional channel of HMC7044. This is the only project using OSCOUT1, so no old DTs have this property.

nunojsa · 2025-06-11T07:21:05Z

drivers/iio/adc/ad9213.c

+	.driver = {
+			.name = "ad9213",
+			.of_match_table = ad9213_of_match,
+		},


odd indentation...

nunojsa · 2025-06-11T07:21:22Z

drivers/iio/adc/ad9213.c

+	if (ret)
+		return ret;
+
+	dev_info(&adc->spi->dev, "%s Probed\n", indio_dev->name);


drop the above

nunojsa · 2025-06-11T07:21:48Z

drivers/iio/adc/ad9213.c

+	}
+
+	indio_dev->dev.parent = &spi->dev;
+	indio_dev->name = spi->dev.of_node->name;


drop the above. Give it the real part name

nunojsa · 2025-06-11T07:21:59Z

drivers/iio/adc/ad9213.c

+		if (ret)
+			return ret;
+
+		regmap_read(adc->regmap, AD9213_REG_PLL_STATUS, &status);


no error check

nunojsa · 2025-06-11T07:28:47Z

drivers/iio/adc/ad9213.c

+			break;
+		}
+
+		ret = sprintf(buf, "%d\n", adc->is_initialized);


return sysfs_emit(). Ditto for all other places

nunojsa · 2025-06-11T07:30:01Z

drivers/iio/adc/ad9213.c

+		break;
+
+	case AD9213_JESD204_FSM_CTRL:
+		if (!adc->jdev) {


I wonder if we can make this slightly better. Don't expose these attrs in the first place if jdev is NULL

nunojsa · 2025-06-11T07:30:39Z

drivers/iio/adc/ad9213.c

+				break;
+			}
+		}
+		ret = sprintf(buf, "%d\n", paused);


The above is big enough that deserves an helper function

nunojsa · 2025-06-11T07:30:56Z

drivers/iio/adc/ad9213.c

+			ret = -EOPNOTSUPP;
+			break;
+		}
+		ret = strtobool(buf, &enable);


kstrtobool()

nunojsa · 2025-06-11T07:31:46Z

drivers/iio/adc/ad9213.c

+		ret = regmap_read(adc->regmap, reg, read_val);
+	else
+		ret = regmap_write(adc->regmap, reg, write_val);
+


The above can be done much better. Will let you figure it out :)

danmois · 2025-06-11T08:05:41Z

Please look at my comment about commit subjects in the first version of the series. Also, can we have this without JESD FSM? What I'm thinking is to upstream the driver without FSM and then backport it with it. At least, the driver would not be completely out of tree

I doubt that this can be done without the FSM. It assures that the correct probing order is maintained. Will experiment if removing it is possible.

nunojsa · 2025-06-12T07:03:41Z

I doubt that this can be done without the FSM. It assures that the correct probing order is maintained. Will experiment if removing it is possible.

Yeah, it would be just so we could upstream a version of the driver without it :)

danmois force-pushed the 9213 branch 7 times, most recently from 6eab46b to a6a3f96 Compare May 27, 2025 06:53

gastmaier reviewed May 27, 2025

View reviewed changes

danmois force-pushed the 10000 9213 branch 15 times, most recently from d1d695c to a6753c4 Compare May 27, 2025 16:13

danmois marked this pull request as ready for review May 28, 2025 06:08

danmois requested review from nunojsa, mhennerich, dbogdan, btogorean, chegbeli and buha as code owners May 28, 2025 06:08

danmois requested review from amiclaus, rbolboac, CiprianRegus and machschmitt as code owners May 28, 2025 06:08

nunojsa reviewed May 28, 2025

View reviewed changes

danmois force-pushed the 9213 branch from a6753c4 to 3a7d85b Compare June 2, 2025 12:27

danmois force-pushed the 9213 branch from 3a7d85b to 256d23b Compare June 2, 2025 12:41

danmois force-pushed the 9213 branch from 256d23b to f9aa803 Compare June 3, 2025 08:07

danmois and others added 8 commits June 4, 2025 09:12

include: hmc7044: Remove unused defines

55e8216

Remove the adi,divider-ratio defines, as they are no longer used. Signed-off-by: George Mois <george.mois@analog.com>

dt-bindings: iio: adc: AD9213 yaml documentation

101b950

This adds device tree bindings for the AD9213 ADC. Signed-off-by: George Mois <george.mois@analog.com>

drivers: iio: adc: ad9213: Add support for AD9213

47a299d

Add driver for AD9213. Signed-off-by: AndrDragomir <andrei.dragomir@analog.com> Signed-off-by: George Mois <george.mois@analog.com>

microblaze: adi_mb_defconfig: Add AD9213

f255044

Add AD9213 driver to adi_mb_defconfig. Signed-off-by: George Mois <george.mois@analog.com>

drivers: iio: Kconfig.adi: imply AD9213

2859ff8

Make sure the AD9213 ADC is built. Signed-off-by: George Mois <george.mois@analog.com>

arch: microblaze: boot: dts: Add devicetree for AD9213

0161879

Add devicetree for AD9213_EVB. Signed-off-by: AndrDragomir <andrei.dragomir@analog.com> Signed-off-by: George Mois <george.mois@analog.com>

danmois force-pushed the 9213 branch from f9aa803 to 0161879 Compare June 4, 2025 06:15

gastmaier reviewed Jun 10, 2025

View reviewed changes

nunojsa reviewed Jun 11, 2025

View reviewed changes

+              	/* protect against device accesses */
+              	struct mutex		lock;
+              	u64			adc_frequency_hz;
+              	struct iio_backend	*back;

	MODULE_LICENSE("GPL v2");
	MODULE_LICENSE("GPL");
	MODULE_IMPORT_NS("IIO_BACKEND")

AD9213 driver #2804

Are you sure you want to change the base?

AD9213 driver #2804

Uh oh!

Conversation

PR Description

PR Type

PR Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!