Staging/adf4030 support #2651

mhennerich · 2024-11-18T12:06:42Z

PR Description

New Linux IIO driver for Analog Devices ADF4030 10-Channel Precision Synchronizer IC.

The ADF4030 provides for 10 bidirectional synchronized clock (BSYNC) channels and
accepts a reference clock input (REFIN) signal as a frequency reference for
generating an output clock on any BSYNC channels that are configured as an output.
The hallmark feature of the ADF4030 is the ability to time align the clock edges
of any one or more BSYNC channels to <5 ps (at the device pins) with respect to
the BSYNC channel selected as the reference BSYNC channel.

https://www.analog.com/en/products/adf4030.html

PR Type

New feature (a change that adds new functionality)

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 · 2024-11-29T12:59:17Z

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

+
+      adi,extended-name:
+        description: Extended name for the channel.
+        $ref: /schemas/types.yaml#/definitions/string


This one is questionable..

Well it's handy, and avoids possible mix up.

nunojsa · 2024-11-29T12:59:41Z

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

+    properties:
+      reg:
+        description: Channel number.
+        $ref: /schemas/types.yaml#/definitions/uint32


This needs some constrains. I guess some of the other properties also need it.

nunojsa · 2024-11-29T13:00:53Z

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

+
+      adi,rcm:
+        description: RCM value for the channel.
+        $ref: /schemas/types.yaml#/definitions/uint32


A lot of these properties need a better description. Right now, the description is more or less the same what one can infer from the name.

You will always need the datasheet to properly set those and the naming follows the datasheet.
But I agree some more text doesn't harm. Will fix.

nunojsa · 2024-11-29T13:02:17Z

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

+    description: Number of iterations for auto alignment.
+
+  adi,bsync-autoalign-threshold-fs:
+    description: Threshold for auto alignment in femtoseconds.


I dunno the three above properties pass the binding check. They have no standard suffix and we're not defining a type for them.

Not sure why it does not fail in the check

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

drivers/iio/frequency/adf4030.c

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

drivers/iio/frequency/adf4030.c

+		.shared = true,
+		.private = BSYNC_ALIGN_ITER,
+	},
+	{},


The above four (at least) need an ABI doc. But I guess we can do it when upstreaming...

ABI documentation sysfs-bus-iio-frequency-adf4030

Yeah - the wiki page already has a link for it ...
https://wiki.analog.com/resources/tools-software/linux-drivers/iio-synchronizer/adf4030

drivers/iio/frequency/adf4030.c

Initial support... Signed-off-by: Nuno Sa <[email protected]>

Add dt-bindings for the Analog Devices ADF4030 Precision Synchronizer IC. Signed-off-by: Michael Hennerich <[email protected]>

Automatically imply ADF4030. Signed-off-by: Nuno Sa <[email protected]>

nunojsa

Looks mostly good to me...

Is there any strong reason to merge this right now or can we wait till we upstream this driver?

nunojsa · 2024-12-17T12:23:09Z

drivers/iio/frequency/adf4030.c

+		return dev_err_probe(&spi->dev, ret,
+				     "Failed to start JESD204 FSM\n");
+
+	return devm_add_action_or_reset(&spi->dev, adf4030_stop_jesd204,


We now have devm_jesd204_fsm_start()

nunojsa · 2024-12-17T12:25:16Z

drivers/iio/frequency/adf4030.c

+
+		fwnode_property_read_u32(child, "adi,rcm", &st->channels[cnt].rcm);
+		cnt++;
+	}


We should validate the properties that have constrains in the bindings to make sure dt values that we get are sane...

nunojsa · 2024-12-17T12:26:45Z

drivers/iio/frequency/adf4030.c

+						st->clk_out_names,
+						ARRAY_SIZE(st->clk_out_names));
+	if (ret < 0)
+		return ret;


This is still treating it as mandatory because we error out in case the property is not there. I guess we could do it in the same way as here:

https://elixir.bootlin.com/linux/v6.13-rc3/source/drivers/iio/frequency/adf4350.c#L467

nunojsa · 2024-12-17T12:27:21Z

drivers/iio/frequency/adf4030.c

+		dev_err(&st->spi->dev,
+			"Failed to compute R and N dividers for Fref=%u Hz amd VCO=%u Hz\n",
+			ref_input_freq_hz, vco_out_freq_hz);
+		return ret;


dev_err_probe()

nunojsa · 2024-12-17T12:27:30Z

drivers/iio/frequency/adf4030.c

+				       100, 1000);
+	if (ret) {
+		dev_err(&st->spi->dev, "PLL calibration failed\n");
+		return ret;


nunojsa · 2024-12-17T12:30:47Z

drivers/iio/frequency/adf4030.c

+		if (ret)
+			return ret;
+
+		regmap_read(st->regmap, ADF4030_REG(0x8F), &regval);


missing error handling....

nunojsa · 2024-12-17T12:30:55Z

drivers/iio/frequency/adf4030.c

+			dev_err(&st->spi->dev, "Autoalign failed FSM_BUSY\n");
+			return ret;
+		}
+		regmap_read(st->regmap, ADF4030_REG(0x90), &regval);


nunojsa · 2024-12-17T12:31:32Z

drivers/iio/frequency/adf4030.c

+	vco_period_in_fs = DIV_U64_ROUND_CLOSEST(vco_period_in_fs, 512);
+	tdc_offset = div64_s64(delay_in_fs, vco_period_in_fs);
+
+	/* Check regmap_bulk_write() */


Does it not work? Otherwise I guess we should remove the comment

drivers/iio/frequency/adf4030.c

nunojsa · 2024-12-17T12:37:20Z

drivers/iio/frequency/adf4030.c

+		.name = "duty_cycle",
+		.read = adf4030_ext_info_read,
+		.write = adf4030_ext_info_write,
+		.shared = false,


This needs to use the proper IIO_ENUMS.... This is the reason for CI failures

mhennerich requested review from nunojsa, dbogdan, btogorean, chegbeli, buha, amiclaus, rbolboac, danmois, CiprianRegus and machschmitt as code owners November 18, 2024 12:06

mhennerich force-pushed the staging/adf4030-support branch 5 times, most recently from 0d2c2cd to dbed104 Compare November 19, 2024 14:21

nunojsa reviewed Nov 29, 2024

View reviewed changes

mhennerich force-pushed the staging/adf4030-support branch from dbed104 to 6e2a7c7 Compare December 5, 2024 11:35

nunojsa and others added 3 commits December 5, 2024 12:37

iio: frequency: support the adf4030 Synchronizer

89a5139

Initial support... Signed-off-by: Nuno Sa <[email protected]>

devicetree: bindings: iio: adi,adf4030.yaml: Add dt-bindings doc

a0f082a

Add dt-bindings for the Analog Devices ADF4030 Precision Synchronizer IC. Signed-off-by: Michael Hennerich <[email protected]>

iio: Kconfig.adi: imply ADF4030

ea7284a

Automatically imply ADF4030. Signed-off-by: Nuno Sa <[email protected]>

mhennerich force-pushed the staging/adf4030-support branch from 6e2a7c7 to ea7284a Compare December 5, 2024 11:51

nunojsa reviewed Dec 17, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Staging/adf4030 support #2651

Staging/adf4030 support #2651

mhennerich commented Nov 18, 2024

nunojsa Nov 29, 2024

mhennerich Nov 29, 2024

nunojsa Nov 29, 2024

mhennerich Nov 29, 2024

nunojsa Nov 29, 2024

mhennerich Nov 29, 2024

nunojsa Nov 29, 2024

nunojsa Nov 29, 2024

mhennerich Nov 29, 2024

nunojsa left a comment

nunojsa Dec 17, 2024

nunojsa Dec 17, 2024

nunojsa Dec 17, 2024

nunojsa Dec 17, 2024

nunojsa Dec 17, 2024

nunojsa Dec 17, 2024

nunojsa Dec 17, 2024

nunojsa Dec 17, 2024

nunojsa Dec 17, 2024

Staging/adf4030 support #2651

Are you sure you want to change the base?

Staging/adf4030 support #2651

Conversation

mhennerich commented Nov 18, 2024

PR Description

PR Type

PR Checklist

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

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