ADF41513 Driver implementation #3012

rodrigo455 · 2025-11-05T09:15:55Z

PR Description

This PR is not meant to be merged, as it targets an upstream mirror
I want to collect feedback and run CI pipelines if possible
This PR adds the ADF41513 driver implementation and documentation
ADF41513 is a dual modulus PLL frequency synth
Current implementation is based on existing PLL drivers
the driver implements a clock provider/supplier, but clock-scales is not taken care here, because that feature is not available in this branch

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 compiled my changes, including the documentation
I have tested the changes on the relevant hardware
I have updated the documentation outside this repo accordingly
I have provided links for the relevant upstream lore

gastmaier · 2025-11-05T14:06:52Z

drivers/iio/frequency/adf41513.c

+	struct adf41513_platform_data *pdata;
+	struct iio_dev *indio_dev;
+	struct adf41513_state *st;
+	struct clk *ref_clk;


Suggested change

struct clk *ref_clk;

struct clk *ref_clk = NULL;

if (pdata->clkin), ref_clk is never set.

gastmaier · 2025-11-05T14:08:58Z

drivers/iio/frequency/adf41513.c

+	if (st->ref_clk)
+		st->ref_freq_hz = clk_get_rate(st->ref_clk);
+	else
+		st->ref_freq_hz = pdata->clkin;


Suggested change

if (st->ref_clk)

st->ref_freq_hz = clk_get_rate(st->ref_clk);

else

st->ref_freq_hz = pdata->clkin;

if (pdata->clkin)

st->ref_freq_hz = pdata->clkin;

else

st->ref_freq_hz = clk_get_rate(st->ref_clk);

flows a little more naturally IMO, since ref_clk is set if clkin is not set

artursartamonovsadi · 2025-11-06T15:51:04Z

drivers/iio/frequency/adf41513.c

+	struct mutex			lock;
+
+	/* Cached register values */
+	u32				regs[14];


array size is 14, can there array size used as define? Later on the REG0 and REG13 used in for loop. could be from MAX_REG number to 0. Assume no more the 14 registers are used?

drivers/iio/frequency/adf41513.c

nunojsa

Just some comments about using legacy platform_data. Once you mark this ready for reviewing I'll give a more in depth review.

nunojsa · 2025-11-07T09:52:12Z

include/linux/iio/frequency/adf41513.h

+ * @load_enable_sync:		Enables load enable sync with reference input.
+ * @freq_resolution_uhz:	Target frequency resolution in uHz (default = 1 Hz).
+ */
+struct adf41513_platform_data {


I see this is still a draft but I might just tell it now. Most likely you don't really need the header as I don't expecte anyone other than the driver itself to consume this. So, just move it to the source file.

Another comment is for struct adf41513_platform_data. Name it differently (like adf41513_state or adf41513_data). platform data is a thing from the past :)

nunojsa · 2025-11-07T09:53:56Z

drivers/iio/frequency/adf41513.c

+			return PTR_ERR(pdata);
+	} else {
+		pdata = spi->dev.platform_data;
+		if (!pdata) {


As I mentioned, drop the platform data stuff. We just want to support FW properties.

nunojsa

Ok, I'm done for now :). We'll need some iterations on this one and skipped more questionable/complicated things for now. We are also pushing the limits of what can go into a DT property so be ready to justify them when upstreaming.

nunojsa · 2025-11-07T15:46:50Z

Documentation/devicetree/bindings/iio/frequency/adi,adf41513.yaml

+maintainers:
+  - Rodrigo Alencar <[email protected]>
+
+description: |


Likely no need for formatting.

nunojsa · 2025-11-07T15:47:28Z

Documentation/devicetree/bindings/iio/frequency/adi,adf41513.yaml

+    description: Clock that provides the reference input frequency.
+
+  clock-names:
+    const: clkin


Given that you only have a clock, you might get questioned about the above

nunojsa · 2025-11-07T15:48:08Z

Documentation/devicetree/bindings/iio/frequency/adi,adf41513.yaml

+    description: Power supply for the device (3.3V)
+
+  chip-enable-gpios:
+    description: |


drop |. Same in all other places

Documentation/devicetree/bindings/iio/frequency/adi,adf41513.yaml

nunojsa · 2025-11-07T15:50:22Z

Documentation/devicetree/bindings/iio/frequency/adi,adf41513.yaml

+    maximum: 32
+    description:
+      Reference division factor (R Counter). If not specified, the driver
+      will calculate the optimal value automatically.


Again, we might need to better explain why we would need this as a DT property.

those are constraints to the input reference path, I've taken those from adf4350 driver.

nunojsa · 2025-11-07T16:24:51Z

drivers/iio/frequency/adf41513.c

+	/* get lock detect gpio */
+	st->lock_detect = devm_gpiod_get_optional(&spi->dev, "lock-detect", GPIOD_IN);
+	if (IS_ERR(st->lock_detect)) {
+		dev_err(&spi->dev, "fail to request lock detect GPIO\n");


Be consistent. dev_err_probe() in all probe path

nunojsa · 2025-11-07T16:26:25Z

drivers/iio/frequency/adf41513.c

+	}
+
+	/* configure IIO device */
+	indio_dev->name = st->data.name[0] ? st->data.name : "adf41513";


The name should be constant. Just use chip_info struct kind of thing if you need more than the name. Or just a const char pointer passed through of_id and spi_id tables.

nunojsa · 2025-11-07T16:26:35Z

drivers/iio/frequency/adf41513.c

+		return -EINVAL;
+	}
+
+	mutex_init(&st->lock);


devm_mutex_init()

drivers/iio/frequency/adf41513.c

nunojsa · 2025-11-07T16:28:11Z

drivers/iio/frequency/adf41513.c

+
+static int adf41513_pm_resume(struct device *dev)
+{
+	struct iio_dev *indio_dev = dev_get_drvdata(dev);


Pass in your device struct directly in spi_set_drvdata(). Then no need for the pointer dance

nunojsa

Here it goes my second iteration. As I said, this one will take some...

nunojsa · 2025-11-12T17:26:12Z

drivers/iio/frequency/adf41513.c

+/* REG2 Bit Definitions */
+#define ADF41513_REG2_PHASE_VAL_MSK		GENMASK_U32(15, 4)
+#define ADF41513_REG2_PHASE_VAL(x)		FIELD_PREP(ADF41513_REG2_PHASE_VAL_MSK, x)
+#define ADF41513_REG2_PHASE_ADJ_MSK		BIT_U32(31)


i get it but normally no one really cares about that and use the generic BIT() and GENMASK() macros

nunojsa · 2025-11-12T17:29:22Z

drivers/iio/frequency/adf41513.c

+#define ADF41513_REG12_LOGIC_LEVEL_MSK		BIT_U32(27)
+#define ADF41513_REG12_LOGIC_LEVEL(x)		FIELD_PREP(ADF41513_REG12_LOGIC_LEVEL_MSK, x)
+#define ADF41513_REG12_MUXOUT_MSK		GENMASK_U32(31, 28)
+#define ADF41513_REG12_MUXOUT(x)		FIELD_PREP(ADF41513_REG12_MUXOUT_MSK, x)


Still not addressed

drivers/iio/frequency/adf41513.c

nunojsa · 2025-11-12T17:35:02Z

drivers/iio/frequency/adf41513.c

+	st->settings.ref_div2 =
+		FIELD_GET(ADF41513_REG5_RDIV2_MSK, st->regs_hw[ADF41513_REG5]);
+	st->settings.prescaler =
+		FIELD_GET(ADF41513_REG5_PRESCALER_MSK, st->regs_hw[ADF41513_REG5]);


refactor the line break. Just break the line after the first argument of FIELD_PREP() for example

drivers/iio/frequency/adf41513.c

nunojsa · 2025-11-12T18:15:02Z

drivers/iio/frequency/adf41513.c

+	if (!device_property_read_u64(dev, "adi,freq-resolution",
+				      &st->data.freq_resolution_uhz)) {
+		if (!st->data.freq_resolution_uhz)
+			return dev_err_probe(dev, -ERANGE, "frequency resolution cannot be zero\n");


And upper limit? Do we have one?

nunojsa · 2025-11-12T18:16:59Z

drivers/iio/frequency/adf41513.c

+	}
+
+	st->data.phase_resync_clk_div[1] = 1;
+	if (!device_property_read_u32(dev, "adi,12bit-clk2-divider", &tmp)) {


I have a slight preference for the following patterh:

ret = device_property_read_u32() if (ret && ret != -EINVAL) //some meaningful error other than property not present return ret; if (!ret) { /* validate property */ }

documentation says:

* %-EINVAL if given arguments are not valid, * %-ENODATA if the property does not have a value, * %-EPROTO if the property is not an array of numbers, * %-EOVERFLOW if the size of the property is not as expected. * %-ENXIO if no suitable firmware interface is present.

I dont think the suggested pattern is correct

Well, sometimes you need to look further :). At least for OF:

https://elixir.bootlin.com/linux/v6.17.8/source/drivers/of/property.c#L136

But yes, often I also just go:

ret = device_property_read_u32() if (!ret) { // validate it... }

nunojsa · 2025-11-12T18:17:37Z

drivers/iio/frequency/adf41513.c

+		if (tmp > 7)
+			return dev_err_probe(dev, -ERANGE,
+					     "lock detect count setting too high %u\n", tmp);
+		else


redundant else. Ditto for the other places.

nunojsa · 2025-11-12T18:19:00Z

drivers/iio/frequency/adf41513.c

+
+	st->data.lock_detect_precision = 0; /* default 0 */
+	if (!device_property_read_u32(dev, "adi,lock-detect-precision", &tmp)) {
+		if (tmp > 3)


Here it's not too obvious what 3 means? Maybe a define for it? Im ok with plain literals when their meaning is straightforward but in this case I not seeing it 😅

nunojsa · 2025-11-12T18:21:28Z

drivers/iio/frequency/adf41513.c

+	/* lock detector settings */
+	st->data.lock_detect_bias = 0; /* default 40 uA */
+	if (!device_property_read_u32(dev, "lock-detect-bias-microamp", &tmp)) {
+		tmp = (ADF41513_MAX_LD_BIAS_uA - tmp) / ADF41513_LD_BIAS_STEP_uA;


The above is questionable? Why do we need it before validating the property? It seems to me you are validating the register value rather than the property.

The driver is based on existing PLL drivers in the IIO subsystem and implements the following key features: - Integer-N and fractional-N (fixed/variable modulus) synthesis modes - High-resolution frequency calculations using microhertz (µHz) precision to handle sub-Hz resolution across multi-GHz frequency ranges - Clock framework integration as a clock provider to expose the output signal for use by other kernel subsystems - IIO debugfs interface for direct register access - Platform data parsing from devicetree including charge pump settings, reference path configuration, lock detect parameters, and muxout options - Power management support with suspend/resume callbacks - Phase resynchronization and load-enable synchronization features - Lock detect GPIO monitoring The driver uses 64-bit microhertz values throughout PLL calculations to maintain precision when working with frequencies that exceed 32-bit Hz representation while requiring fractional Hz resolution. Signed-off-by: Rodrigo Alencar <[email protected]>

ultralow noise PLL frequency synthesizer that can be used to implement local oscillators (LOs) as high as 26.5 GHz Signed-off-by: Rodrigo Alencar <[email protected]>

add documentation for adf41513 driver which describes the device driver files and shows how userspace may consume the ABI for various tasks Signed-off-by: Rodrigo Alencar <[email protected]>

rodrigo455 requested review from CiprianRegus, amiclaus, btogorean, buha, chegbeli, danmois, dbogdan, machschmitt, mhennerich, nunojsa and rbolboac as code owners November 5, 2025 09:15

gastmaier marked this pull request as draft November 5, 2025 09:38

rodrigo455 force-pushed the staging/jic23_iio_adf41513 branch 4 times, most recently from ccd64ff to 76e3d30 Compare November 5, 2025 12:29

gastmaier reviewed Nov 5, 2025

View reviewed changes

github-actions bot force-pushed the mirror/jic23/iio/testing branch 2 times, most recently from 61944f0 to c9591d3 Compare November 6, 2025 00:00

artursartamonovsadi reviewed Nov 6, 2025

View reviewed changes

github-actions bot force-pushed the mirror/jic23/iio/testing branch from c9591d3 to 65e501e Compare November 7, 2025 00:00

nunojsa reviewed Nov 7, 2025

View reviewed changes

rodrigo455 force-pushed the staging/jic23_iio_adf41513 branch from 76e3d30 to fe10392 Compare November 7, 2025 13:00

rodrigo455 marked this pull request as ready for review November 7, 2025 13:10

rodrigo455 force-pushed the staging/jic23_iio_adf41513 branch from fe10392 to f98be31 Compare November 7, 2025 14:54

nunojsa reviewed Nov 7, 2025

View reviewed changes

github-actions bot force-pushed the mirror/jic23/iio/testing branch 2 times, most recently from 950132e to b60429f Compare November 10, 2025 00:10

rodrigo455 force-pushed the staging/jic23_iio_adf41513 branch from f98be31 to 3150872 Compare November 10, 2025 12:20

rodrigo455 force-pushed the staging/jic23_iio_adf41513 branch 4 times, most recently from 658fbf4 to b8cc6c4 Compare November 10, 2025 18:54

github-actions bot force-pushed the mirror/jic23/iio/testing branch 2 times, most recently from d967ebe to f0c3212 Compare November 11, 2025 23:59

nunojsa reviewed Nov 12, 2025

View reviewed changes

github-actions bot force-pushed the mirror/jic23/iio/testing branch 5 times, most recently from c416398 to 90a2673 Compare November 17, 2025 00:01

rodrigo455 added 3 commits November 17, 2025 14:19

dt-bindings: iio: frequency: add adf41513

3a75f85

ultralow noise PLL frequency synthesizer that can be used to implement local oscillators (LOs) as high as 26.5 GHz Signed-off-by: Rodrigo Alencar <[email protected]>

docs: iio: add documentation for adf41513 driver

d5d5fa9

add documentation for adf41513 driver which describes the device driver files and shows how userspace may consume the ABI for various tasks Signed-off-by: Rodrigo Alencar <[email protected]>

rodrigo455 force-pushed the staging/jic23_iio_adf41513 branch from b8cc6c4 to d5d5fa9 Compare November 17, 2025 14:25

ADF41513 Driver implementation #3012

Are you sure you want to change the base?

ADF41513 Driver implementation #3012

Uh oh!

Conversation

rodrigo455 commented Nov 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

nunojsa left a comment

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!

nunojsa left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

nunojsa left a comment

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!

Choose a reason for hiding this comment

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

rodrigo455 commented Nov 5, 2025 •

edited

Loading