Add ADF4382 #2096

chegbeli · 2024-02-08T12:14:10Z

Pull Request Description

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.

PR Type

Bug fix (change that fixes an issue)
New feature (change that adds new functionality)
Breaking change (has dependencies in other repos or will cause CI to fail)

PR Checklist

I have followed the Coding style guidelines
I have performed a self-review of the changes
I have commented my code, at least hard-to-understand parts
I have build all projects affected by the changes in this PR
I have tested in hardware affected projects, at the relevant boards
I have signed off all commits from this PR
I have updated the documentation (wiki pages, ReadMe etc), if applies

CLAassistant · 2024-02-08T12:14:16Z

All committers have signed the CLA.

amiclaus

some general comments:

please sign the CLA
for the documentation build - please add the documentation into the ToC (an example here: 5100411 )
for the xilinx build make sure that the hardware naming in the build.json file is corresponding to the hdl output name and the project is available on main hdl branch.
a project README.rst would be nice (see example here: 6673e20 )

chegbeli · 2024-02-08T12:45:54Z

I'll remove the Xilinx platform as it is not officially supported yet.

amiclaus

some comments on my side.

amiclaus · 2024-02-09T09:00:41Z

drivers/frequency/adf4382/adf4382.c

+ *   @brief  Implementation of adf4382 Driver.
+ *   @author Ciprian Hegbeli (ciprian.hegbeli@analog.com)
+********************************************************************************
+ * Copyright 2023(c) Analog Devices, Inc.


amiclaus · 2024-02-09T09:01:59Z

drivers/frequency/adf4382/adf4382.c

+#include "no_os_print_log.h"
+#include "no_os_delay.h"
+
+//Charge pump current values expressed in uA


nit: c-style comments are preferred even for single line.

amiclaus · 2024-02-09T09:02:32Z

drivers/frequency/adf4382/adf4382.c

+
+	ret = no_os_spi_write_and_read(dev->spi_desc, buff,
+				       ADF4382_BUFF_SIZE_BYTES);
+	if(ret < 0)


if (ret) same as it is done for linux drivers.

amiclaus · 2024-02-09T09:03:07Z

drivers/frequency/adf4382/adf4382.c

+
+	*data = buff[2];
+
+	return ret;


return 0; same as it is done for linux drivers.

amiclaus · 2024-02-09T09:08:18Z

drivers/frequency/adf4382/adf4382.c

+	tmp = orig & ~mask;
+	tmp |= data & mask;
+
+	if (tmp != orig)


this check is a bit uncommon. but i'm fine with it also.

adapted the function from the Linux implementation https://elixir.bootlin.com/linux/latest/source/drivers/base/regmap/regmap.c#L3137

amiclaus · 2024-02-09T09:31:40Z

drivers/frequency/adf4382/iio_adf4382.c

+		return -EINVAL;
+	adf4382_get_ref_clk(adf4382, &val);
+	return snprintf(buf, len, "%"PRIu64, val);
+//	return 0;


amiclaus · 2024-02-09T09:32:10Z

drivers/frequency/adf4382/iio_adf4382.c

+
+	val = en;
+	return iio_format_value(buf, len, IIO_VAL_INT, 1, &val);
+


drop extra line.

amiclaus · 2024-02-09T09:38:29Z

projects/adf4382/Makefile

+# Select the example you want to enable by chossing y for enabling and n for disabling
+BASIC_EXAMPLE = n
+IIO_EXAMPLE = y
+


nit: drop extra line

amiclaus · 2024-02-09T09:39:19Z

projects/adf4382/src/platform/mbed/parameters.c

+	.spi_clk_pin = SDP_SPI_SCK,
+	.use_sw_csb = false
+};
+


drop extra lines.

amiclaus · 2024-02-09T09:39:34Z

projects/adf4382/src/platform/stm32/platform_src.mk

+	$(PLATFORM_DRIVERS)/stm32_irq.c       \
+	$(PLATFORM_DRIVERS)/stm32_gpio_irq.c  \
+	$(PLATFORM_DRIVERS)/stm32_uart.c      \
+	$(PLATFORM_DRIVERS)/stm32_uart_stdio.c


add new line.

rbolboac

some remarks from my side, also please fix the documentation issues. you will have to provide extra files for the build to be successful, see this example: fd31fb2#diff-d7a16ebb8ac2559dcec7cfb925b059c3176c69cbe2debf591de4d671a11e65dd

also, project documentation can be useful, you may see this example: https://github.com/analogdevicesinc/no-OS/blob/main/projects/eval-adis1650x/README.rst

rbolboac · 2024-02-12T14:57:43Z

projects/adf4382/builds.json

+  },
+  "stm32": {
+    "basic_example": {
+       "flags" : "BASIC_EXAMPLE=y"


please add also the hardware flag: HARDWARE=sdp-ck1z.ioc

rbolboac · 2024-02-12T14:57:47Z

projects/adf4382/builds.json

+       "flags" : "BASIC_EXAMPLE=y"
+    },
+    "iio": {
+      "flags": "IIO_EXAMPLE=y"


please add also the hardware flag: HARDWARE=sdp-ck1z.ioc

rbolboac · 2024-02-12T14:58:30Z

projects/adf4382/sdp-ck1z.ioc

+MxCube.Version=6.8.1
+MxDb.Version=DB.6.0.81


MxCube.Version=6.5.0
MxDb.Version=DB.6.0.50

rbolboac · 2024-02-12T14:59:39Z

projects/adf4382/src/examples/basic/basic_example.c

+	return ret;
+error:
+	pr_info("Error!\n");
+	return ret;


adf4382_remove should be called before returning to free the allocated memory

rbolboac · 2024-02-12T15:00:57Z

projects/adf4382/src/examples/iio_example/iio_example.c

+	if (ret)
+		return ret;
+
+	return iio_app_run(app);


there are some memory leaks here, the according remove functions should be called if memory is allocated successfully. you may take a look at: https://github.com/analogdevicesinc/no-OS/blob/main/projects/max14916/src/examples/iio_example/iio_example.c and follow the example

drivers/frequency/adf4382/adf4382.c

rbolboac · 2024-02-12T15:20:09Z

drivers/frequency/adf4382/adf4382.c

+	if (pol < 0)
+		return pol;


this will never be true, pol is uint8_t, always >= 0, please adapt accordingly

rbolboac · 2024-02-12T15:22:15Z

drivers/frequency/adf4382/adf4382.c

+ * @return	   - 0 in case bits are not set, positive value if they are set
+ * 		     or negative error code otherwise.
+ */
+int adf4382_spi_test_bits(struct adf4382_dev *dev, uint16_t reg_addr,


the return type should be the execution status of the API. pass the result by pointer in the parameters list

rbolboac · 2024-02-12T15:25:27Z

drivers/frequency/adf4382/iio_adf4382.c

+				uint32_t *readval)
+{
+	return adf4382_spi_read(dev->adf4382_dev, (uint16_t)reg,
+				(uint8_t *)readval);


casting this pointer will result in a different value based on the endianness of the CPU. please do not make the cast

rbolboac · 2024-02-12T15:26:15Z

drivers/frequency/adf4382/iio_adf4382.c

+				 intptr_t priv)
+{
+	struct adf4382_iio_dev *iio_adf4382 = (struct adf4382_iio_dev *)dev;
+	struct adf4382_dev *adf4382 = iio_adf4382->adf4382_dev;


this should be done after the null check of iio_adf4382, please see all occurrences in the file

chegbeli · 2024-03-28T12:25:00Z

V1 -> V2

Removed Xilinx platform, as there is no official support for it
Updated sdp-ck1z.ioc to use version 6.5.0
Added missing driver and project documentation
Fixed wrong POR delay time
Removed warning messages as they brake the IIO over UART
Remove adf4382_spi_test_bits and replaced with no_os_field_get
Added adf4382_get_rfout which computes the current output frequency from register values
Replaced some MACROs with the ones from Use no_os_units.h
Removed ADF4382_EN_RDBLR(x), ADF4382_R_DIV(x) and ADF4382_CMOS_OV(x) MACROs
Added NULL checks to API functions
Moved dev NULL checks before attributing the dev in the IIO functions
Fixed pointer casting from u32 to u8
Added ID structure entries for ADF4382 and ADF4382A
Added missing no_os_uart_remove to the main files
Fixed missing adf4382_remove for error conditions in the IIO and basic example

amiclaus

there are some conflicts that need addressing.

buha · 2024-04-09T10:48:13Z

drivers/frequency/adf4382/adf4382.c

+		return EINVAL;
+	}
+
+	/*Phase adjustment can only be done if bleed is active, and a bleed


there is some inconsistency here in how you wrote comments with // or with /*, please make them look alike

buha · 2024-04-09T10:49:40Z

drivers/frequency/adf4382/adf4382.c

+		device->clkout_div_reg_val_max = ADF4382A_CLKOUT_DIV_REG_VAL_MAX;
+		break;
+	default:
+		return -EINVAL;


you've already allocated something, you'll need to goto to a label to avoid memory leaks

buha

Looks good, some minor remarks above.
One more thing, Ramona commented about the gcd() function, (like her) I think you should use the util.h one and remove your implementation unless there is a justification for keeping it which would go in a code comment.

chegbeli · 2024-04-09T12:01:35Z

Looks good, some minor remarks above. One more thing, Ramona commented about the gcd() function, (like her) I think you should use the util.h one and remove your implementation unless there is a justification for keeping it which would go in a code comment.

Unfortunately the function in no_os_util.h has a slow execution time which is visible in this driver due to the large numbers which are being processed. I could make a new PR for the gcd replacing the one we currently have, if that is something that would help us.

rbolboac · 2024-04-10T13:16:52Z

drivers/frequency/adf4382/adf4382.c

+
+	ret = no_os_spi_remove(dev->spi_desc);
+	if (ret)
+		return ret;


no_os_free(dev);

rbolboac · 2024-04-10T13:18:01Z

drivers/frequency/adf4382/iio_adf4382.c

+	if (index < 0)
+		return index;


index is unsigned, this will never be true

rbolboac · 2024-04-10T14:43:51Z

projects/adf4382/src/platform/stm32/platform_src.mk

+	$(PLATFORM_DRIVERS)/stm32_gpio_irq.c  \
+	$(PLATFORM_DRIVERS)/stm32_uart.c      \
+	$(PLATFORM_DRIVERS)/stm32_uart_stdio.c
+


missing new line at the end of file

tfcollins · 2024-04-16T18:27:40Z

@chegbeli It doesn't seem like the driver checks for the PLL actually locking. REG 0x58 (bit 0). After initialization it should be the final check.

amiclaus · 2024-04-22T08:25:55Z

drivers/frequency/adf4382/adf4382.c

+int adf4382_reg_dump(struct adf4382_dev *dev)
+{
+	uint8_t val;
+	uint8_t ret;


if ret can take negative values, then use int type. Check throughout the code, since this is occuring multiple times.

amiclaus · 2024-04-22T08:28:39Z

drivers/frequency/adf4382/adf4382.c

+
+	ret = no_os_spi_write_and_read(dev->spi_desc, buff,
+				       ADF4382_BUFF_SIZE_BYTES);
+	if(ret)


please follow the linux guidelines -> if (ret) Check the entire code since there are multiple occurences. Same applies for other statements (for, while, switch etc).

chegbeli · 2024-05-16T13:33:52Z

V2 -> V3

Fixed coding style
Updated return variables to proper data types
Add locked check
Switched to no_os_greatest_common_divisor
Fixed not freeing memory when failing checks

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: chegbeli <ciprian.hegbeli@analog.com>

The IIO driver will enable support for the ADF4382 to be used with the IIO framework over the serial interface. This will expose runtime configurable attributes of the IC using the IIO tools. Signed-off-by: chegbeli <ciprian.hegbeli@analog.com>

Add documentation for adf4382 drivers. Signed-off-by: chegbeli <ciprian.hegbeli@analog.com>

Add example projects for the adf4382 using stm32, and mbed platforms. This also includes the example project for IIO as well. The projects have been tested using the EV-ADF4382SD1Z and SDP-K1 for the stm32 and mbed platforms. Signed-off-by: chegbeli <ciprian.hegbeli@analog.com>

Add README.rst documentation file for project alongside other documentation related files. Signed-off-by: chegbeli <ciprian.hegbeli@analog.com>

CiprianRegus · 2024-05-23T07:01:40Z

drivers/frequency/adf4382/iio_adf4382.c

+ * @return i	- The index of the pair if found or an error if there is no
+ * 		  pair of values in the table.
+ */
+static int adf4382_iio_find_2d_row(const int (*tbl)[2], const int size,


You may use ci_table directly in this function without needing to pass it as a parameter.

chegbeli requested review from buha, CiprianRegus, dbogdan, amiclaus, rbolboac and danmois as code owners February 8, 2024 12:14

amiclaus reviewed Feb 8, 2024

View reviewed changes

amiclaus reviewed Feb 9, 2024

View reviewed changes

rbolboac reviewed Feb 12, 2024

View reviewed changes

chegbeli force-pushed the adf4382 branch from 1728c5e to 3001cdb Compare March 28, 2024 12:24

amiclaus reviewed Apr 2, 2024

View reviewed changes

chegbeli force-pushed the adf4382 branch 5 times, most recently from b11f7b5 to 053c4fb Compare April 9, 2024 10:45

buha reviewed Apr 9, 2024

View reviewed changes

rbolboac reviewed Apr 10, 2024

View reviewed changes

amiclaus reviewed Apr 22, 2024

View reviewed changes

chegbeli force-pushed the adf4382 branch from 053c4fb to be2b3e0 Compare May 16, 2024 13:33

chegbeli added 2 commits May 17, 2024 07:46

chegbeli added 3 commits May 17, 2024 07:46

drivers:frequency: Add ADF4382 README.rst

fe16c04

Add documentation for adf4382 drivers. Signed-off-by: chegbeli <ciprian.hegbeli@analog.com>

projects: Add ADF4382 README.rst

2c5f5c6

Add README.rst documentation file for project alongside other documentation related files. Signed-off-by: chegbeli <ciprian.hegbeli@analog.com>

chegbeli force-pushed the adf4382 branch from be2b3e0 to 2c5f5c6 Compare May 17, 2024 06:02

chegbeli requested review from buha, rbolboac and amiclaus May 17, 2024 07:51

CiprianRegus approved these changes May 23, 2024

View reviewed changes

CiprianRegus merged commit d345f16 into main May 23, 2024
14 checks passed

CiprianRegus deleted the adf4382 branch May 23, 2024 07:48


		val = en;
		return iio_format_value(buf, len, IIO_VAL_INT, 1, &val);

		MxCube.Version=6.8.1
		MxDb.Version=DB.6.0.81

Add ADF4382 #2096

Add ADF4382 #2096

Conversation

chegbeli commented Feb 8, 2024

Pull Request Description

PR Type

PR Checklist

CLAassistant commented Feb 8, 2024 • edited

amiclaus left a comment

Choose a reason for hiding this comment

chegbeli commented Feb 8, 2024

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

rbolboac 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

chegbeli commented Mar 28, 2024

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

buha left a comment • edited

Choose a reason for hiding this comment

chegbeli commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfcollins commented Apr 16, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chegbeli commented May 16, 2024 • edited

Choose a reason for hiding this comment

CLAassistant commented Feb 8, 2024 •

edited

buha left a comment •

edited

tfcollins commented Apr 16, 2024 •

edited

chegbeli commented May 16, 2024 •

edited