Add ADF4382 #2387

chegbeli · 2023-12-22T13:07:53Z

The ADF4382A is a high performance, ultralow jitter, Frac-N PLL
with integrated VCO ideally suited for LO generation for 5G applications
or data converter clock applications. The high performance
PLL has a figure of merit of -239 dBc/Hz, low 1/f Noise and
high PFD frequency of 625MHz in integer mode that can achieve
ultralow in-band noise and integrated jitter. The ADF4382A can
generate frequencies in a fundamental octave range of 11.5 GHz to
21 GHz, thereby eliminating the need for sub-harmonic filters. The
divide by 2 and 4 output dividers on the part allow frequencies to
be generated from 5.75GHz to 10.5GHz and 2.875GHz to 5.25GHz
respectively.

The ADF4382A is a high performance, ultralow jitter, Frac-N PLL with integrated VCO ideally suited for LO generation for 5G applications or data converter clock applications. The high performance PLL has a figure of merit of -239 dBc/Hz, low 1/f Noise and high PFD frequency of 625MHz in integer mode that can achieve ultralow in-band noise and integrated jitter. The ADF4382A can generate frequencies in a fundamental octave range of 11.5 GHz to 21 GHz, thereby eliminating the need for sub-harmonic filters. The divide by 2 and 4 output dividers on the part allow frequencies to be generated from 5.75GHz to 10.5GHz and 2.875GHz to 5.25GHz respectively. Signed-off-by: Ciprian Hegbeli <ciprian.hegbeli@analog.com>

Add ADF4382 to ADI Kconfig. Signed-off-by: Ciprian Hegbeli <ciprian.hegbeli@analog.com>

amiclaus

some initial comments on my side. I'd suggest sending it upstream (although i noticed the datasheet not public yet, so the process might be harsher).

amiclaus · 2024-01-09T10:58:12Z

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

+    spi {
+        #address-cells = <1>;
+        #size-cells = <0>;
+        adf4382@0 {


please use generic naming for the binding example: frequency

amiclaus · 2024-01-09T11:00:43Z

drivers/iio/frequency/adf4382.c

+
+/* ADF4382_REG0 */
+#define ADF4382_ADDR_ASC_MSK			BIT(2)
+#define ADF4382_ADDR_ASC(x)			FIELD_PREP(ADF4382_ADDR_ASC_MSK, x)


drop all these parametrized definitions. i think it is preferred using FIELD_PREP/FIELD_GET inline.

amiclaus · 2024-01-09T11:02:18Z

drivers/iio/frequency/adf4382.c

+#define ADF4382_VCO_CAL_VTUNE			124
+#define ADF4382_VCO_CAL_ALC			250
+
+#define UA_TO_A					1000000


These should go in units.h, if not already there :)

they should be there, yes

amiclaus · 2024-01-09T11:10:57Z

drivers/iio/frequency/adf4382.c

+	ret = regmap_write(st->regmap, 0x1A, var);
+	if (ret)
+		return ret;
+	var = (mod2_word >> 8) & ADF4382_MOD2WORD_MID_MSK;


instead of defining multiple GENMASK(7,0) for ADF4382_MOD2WORD_MID_MSK, ADF4382_MOD2WORD_MSB_MSK etc, i'd create some generic macros:
#define ADF4382_WORD_LSB_MSK GENMASK(7, 0)
#define ADF4382_WORD_MID_MSK GENMASK(15, 8)
#define ADF4382_WORD_MSB_MSK GENMASK(23, 16)

In this manner you drop multiple identical macro definitions and also get rid of explicit shifting by using FIELD_GET instead.

amiclaus · 2024-01-09T11:14:17Z

drivers/iio/frequency/adf4382.c

+
+static int adf4382_get_freq(struct adf4382_state *st, u64 *freq)
+{
+	*freq = st->freq;


we should obtain the frequency from hardware instead of cached values.

amiclaus · 2024-01-09T11:17:13Z

drivers/iio/frequency/adf4382.c

+	if (ret)
+		return ret;
+
+	mutex_lock(&st->lock);


i'd drop this mutex handling. it should go inside adf4382_set_freq. Moreover, adf4382_set_out_power doesn't seem to need locking.

amiclaus · 2024-01-09T11:18:45Z

drivers/iio/frequency/adf4382.c

+
+	ret = device_property_read_u32(&st->spi->dev, "adi,ref-divider",
+				       &tmp);
+	if (ret || !tmp)


hmm, you treat "adi,ref-divider" property as optional, although you made it a required property in the dt bindings.

amiclaus · 2024-01-09T11:20:24Z

drivers/iio/frequency/adf4382.c

+
+	ret = device_property_read_u32(&st->spi->dev, "adi,charge-pump-current",
+				       &tmp);
+	if (ret)


same here: property is optional in driver, while required in the dt bindings.

amiclaus · 2024-01-09T11:20:40Z

drivers/iio/frequency/adf4382.c

+		return ret;
+
+	if (val != ADF4382_SCRATCHPAD_VAL) {
+		dev_err(&st->spi->dev, "Scratch pad test failed please check SPI connection");


dev_err_probe

amiclaus · 2024-01-09T11:21:26Z

drivers/iio/frequency/adf4382.c

+
+	ret = adf4382_init(st);
+	if (ret) {
+		dev_err(&spi->dev, "adf4382 init failed\n");


ditto: dev_err_probe.

nunojsa

First set of comments on my side.

The most tricky stuff is thje CCF abuse together with the IIO userspace interface. I know we do it in more drivers but... Anyways, as always, I'm expecting this driver to be upstreamed first (as I don't see any reason why we can't upstream it) so let's see what they have to say about this.

nunojsa · 2024-01-15T08:58:25Z

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

+      - $ref: /schemas/types.yaml#/definitions/uint32
+      - enum: [0, 1, 2, 3, 6, 7, 9, 10, 11, 14, 15]
+      - default: 15
+    maxItems: 1


Drop maxItems (I think). Ditto for all other places where we don't really have an array. On top of it, make this a proper current property. That means this should be adi,charge-pump-microamp. The translation for proper register values should then be done in the driver. In dts, we should be clear about the properties (when possible).

nunojsa · 2024-01-15T08:59:00Z

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

+  adi,ref-divider:
+    description: |
+      Input divider of the reference frequency, cannot be lower then 1 or
+      higher then 63.


Don't think you need formatted text in all the properties. So you can drop the |

nunojsa · 2024-01-15T09:02:22Z

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

+      - minimum: 687500000
+      - maximum: 22000000000
+      - default: 2305000000
+    maxItems: 1


Not sure if the the above two properties belong in dts. The frequency one will be an hard sell upstream since I suppose that is something you can do already at runtime (changing the frequency). So you need to have a very good justification for this.

The adi,bleed-word also seems like something we could/should have at runtime? Or is this word something dependent on the system/design and that we typically just want to program once?

nunojsa · 2024-01-15T09:03:24Z

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

+  adi,cmos-3v3:
+    description: |
+      Sets the SPI logic to 3.3V, by defautl it uses 1,8V.
+    type: boolean


Make sure the above two properties are not already present in:

https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/spi/spi-peripheral-props.yaml

nunojsa · 2024-01-15T09:04:00Z

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

+  - adi,ref-divider
+
+additionalProperties: false
+


The driver looks to be a clock provider but I'm not seeing any provider properties in the bindings...

nunojsa · 2024-01-15T09:16:50Z

drivers/iio/frequency/adf4382.c

+	if (IS_ERR(st->clkin))
+		return PTR_ERR(st->clkin);
+
+	return 0;


return PTR_ERR_OR_ZERO()?

nunojsa · 2024-01-15T09:18:05Z

drivers/iio/frequency/adf4382.c

+	if (IS_ERR(clk))
+		return PTR_ERR(clk);
+
+	return of_clk_add_provider(of_node, of_clk_src_simple_get, clk);


devm_of_clk_add_hw_provider()

nunojsa · 2024-01-15T09:19:54Z

drivers/iio/frequency/adf4382.c

+	st = iio_priv(indio_dev);
+
+	indio_dev->info = &adf4382_info;
+	indio_dev->name = "adf4382";


you support two devices so ideally you would have the proper name when using adf4382a. Maybe have a chip_info struct only with const char *name

nunojsa · 2024-01-15T09:23:36Z

drivers/iio/frequency/adf4382.c

+
+	ret = clk_prepare_enable(st->clkin);
+	if (ret)
+		return ret;


you can drop the above with one of my previous comments

nunojsa · 2024-01-15T09:27:32Z

drivers/iio/frequency/adf4382.c

+
+	adf4382_setup_clk(st);
+	if (ret)
+		return ret;


The above is questionable. We do it for a couple of drivers and remember there was a reason for having both clock provider and an IIO userspace interface (like having different waveforms through IIO). But if the only reason is to have both userspace access top changing the frequency and having the clock exported as part of CCF at the same time, that will be more difficult to justify.

Anyways, one thing that should be protected is that if the channel is being exported through CCF, we should not allow to change it's frequency though the IIO interface as that could breaks potential consumers of the clock,

Anyways, let's see what upstream has to say about this.

chegbeli force-pushed the adf4382 branch 2 times, most recently from f622291 to 5acfa48 Compare January 8, 2024 09:36

chegbeli force-pushed the adf4382 branch 2 times, most recently from 92f53a7 to e7a4153 Compare January 8, 2024 11:32

chegbeli added 2 commits January 8, 2024 16:10

iio: Kconfig.adi: imply ADF4382

562586b

Add ADF4382 to ADI Kconfig. Signed-off-by: Ciprian Hegbeli <ciprian.hegbeli@analog.com>

chegbeli force-pushed the adf4382 branch from e7a4153 to 562586b Compare January 8, 2024 14:10

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

amiclaus reviewed Jan 9, 2024

View reviewed changes

nunojsa requested changes Jan 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ADF4382 #2387

Add ADF4382 #2387

chegbeli commented Dec 22, 2023

amiclaus left a comment

amiclaus Jan 9, 2024

amiclaus Jan 9, 2024

amiclaus Jan 9, 2024

nunojsa Jan 15, 2024

amiclaus Jan 9, 2024

amiclaus Jan 9, 2024

amiclaus Jan 9, 2024

amiclaus Jan 9, 2024

amiclaus Jan 9, 2024

amiclaus Jan 9, 2024

amiclaus Jan 9, 2024

nunojsa left a comment

nunojsa Jan 15, 2024

nunojsa Jan 15, 2024

nunojsa Jan 15, 2024

nunojsa Jan 15, 2024

nunojsa Jan 15, 2024

nunojsa Jan 15, 2024

nunojsa Jan 15, 2024

nunojsa Jan 15, 2024

nunojsa Jan 15, 2024

nunojsa Jan 15, 2024

Add ADF4382 #2387

Are you sure you want to change the base?

Add ADF4382 #2387

Conversation

chegbeli commented Dec 22, 2023

amiclaus 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

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