ad796x: Add project and driver for ad796x-fmc #2139

ahaslam2 · 2024-04-04T12:48:25Z

This adds driver files and a project test the ad796x series of pulsar Differential ADC.

The HDL for this project is still not merged, and probably its why CI is failing.
perhaps review can start without the HDL

this was tested using the HDL binary at https://github.com/adi-ses/sea-misc/tree/main/AD796x-PRJ

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

rbolboac

some remarks from my side, also:
please create a different commit for the driver and in the same commit please also add the driver documentation
please use the new project structure

drivers/adc/ad796x/ad796x.c

drivers/adc/ad796x/ad796x.h

ahaslam2 · 2024-04-19T08:08:35Z

@rbolboac

ok ill separate driver and project. but
what do you mean by "new project structure"?

rbolboac · 2024-04-19T08:14:26Z

@rbolboac

ok ill separate driver and project. but what do you mean by "new project structure"?

By new project structure I mean the src folder should have platform independent examples, with common folder and platform specific folders. This is an example from iio_demo project:

├── src
│   ├── common
│   │   ├── app_config.h
│   │   ├── common_data.c
│   │   └── common_data.h
│   ├── examples
│   │   ├── examples_src.mk
│   │   ├── iio_example
│   │   │   ├── iio_example.c
│   │   │   └── iio_example.h
│   │   ├── iio_sw_trigger_example
│   │   │   ├── iio_sw_trigger_example.c
│   │   │   └── iio_sw_trigger_example.h
│   │   └── iio_timer_trigger_example
│   │       ├── iio_timer_trigger_example.c
│   │       └── iio_timer_trigger_example.h
│   └── platform
│       ├── aducm3029
│       │   ├── main.c
│       │   ├── parameters.c
│       │   ├── parameters.h
│       │   └── platform_src.mk
│       ├── linux
│       │   ├── main.c
│       │   ├── parameters.c
│       │   ├── parameters.h
│       │   └── platform_src.mk
│       ├── maxim
│       │   ├── main.c
│       │   ├── parameters.c
│       │   ├── parameters.h
│       │   └── platform_src.mk
│       ├── pico
│       │   ├── main.c
│       │   ├── parameters.c
│       │   ├── parameters.h
│       │   └── platform_src.mk
│       ├── platform_includes.h
│       ├── stm32
│       │   ├── main.c
│       │   ├── parameters.c
│       │   ├── parameters.h
│       │   └── platform_src.mk
│       └── xilinx
│           ├── main.c
│           ├── parameters.c
│           ├── parameters.h
│           └── platform_src.mk

ahaslam2 · 2024-04-19T08:30:21Z

Ok, i did not know this structure was a requirement, will do.

ahaslam2 · 2024-04-19T13:27:52Z

one question @rbolboac

i would suppose the structure requirement is to keep the examples platform
independant?

i see things like axi_dac_init_begin(&phy->tx_dac, &tx_dac_init);
int the example c files
that i think are tied to some specific fpga/platform ?

https://github.com/analogdevicesinc/no-OS/blob/main/projects/adrv902x/src/examples/iio_example/iio_example.c#L304

are there some rules as to what goes into platform/common/example?

rbolboac · 2024-04-19T13:34:03Z

well, the idea is to have the examples generic and platform independent, but if that is not possible I suppose it is alright.
in that case adrv902x project can be used as an example.

ahaslam2 · 2024-05-06T14:34:23Z

The failure of CI is currently due to the missing HDL:

 -> Building ad796x_fmcz (iio) -- xilinx -- ad796x_fmc_zed
 -> cp /no-OS_builds/builds_main/new_hardware/ad796x_fmc_zed/system_top.xsa /no-OS_builds/builds_main/hardware/ad796x_fmc_zed.xsa
 -> ERROR
 -> See log logs/ad796x_fmcz_xilinx_iio_ad796x_fmc_zed.txt -- Use cat (linux) or type (windows) to see colored output
Error occured: 1

rbolboac

some remarks from my side

rbolboac · 2024-05-09T10:35:01Z

drivers/adc/ad796x/ad796x.c

+			 uint16_t samples)
+{
+	int ret;
+


blank line can be removed, no reason to leave blank lines between declarations. see everywhere where it applies

rbolboac · 2024-05-09T10:36:01Z

drivers/adc/ad796x/ad796x.c

+		dev->gpio_adc_en0_fmc,
+	};
+
+	for (i = 0; i < 4; i++) {


create a macro value for that magic value

rbolboac · 2024-05-09T10:38:15Z

drivers/adc/ad796x/ad796x.c

+	ret = axi_clkgen_set_rate(dev->clkgen, 125000000);
+	if (ret) {
+		pr_err("error: axi_clkgen_set_rate error: %d\n", ret);
+		goto err_set_rate;


no need for an extra label, can use err_clkgen

rbolboac · 2024-05-09T10:40:53Z

drivers/adc/ad796x/ad796x.c

+err_dmac:
+	axi_adc_remove(dev->ad796x_core);
+err_adc:
+	no_os_pwm_remove(dev->axi_pwm_1);
+err_pwm_1:
+	no_os_pwm_remove(dev->axi_pwm_0);
+err_pwm_0:
+err_set_rate:
+	axi_clkgen_remove(dev->clkgen);
+err_clkgen:
+	ad796x_gpio_remove(dev);
+error_gpio:
+	no_os_free(dev);


I think it is safe to have a single error label (but please check before).
basically, if the init function has not been called, the desc will be null and the remove functions should check whether the desc is null or not. if it is null nothing will happen, if it is not null it will be freed.

Ok, that just means that the convention is that if the desc returns something that
something has to be freed (even if the int func fails).

i guess i can add those checks in remove and just call remove then.

Well... the convention is that if something is allocated and ret is not 0, it should be freed before returning.
However, seeing how we use calloc, all pointers are initialized with null, so if we call the remove function, even if memory has not been allocated, then the remove function should check for null and free the memory only if the pointer is not null.
Assuming that all remove functions check for null pointer before attempting to free it, it is safe to call the remove function even if the init has not been called. This will make the code easier to follow and shorter (a single label can be used).

RE: if something is allocated and ret is not 0, it should be freed before returning

What i see from the no os-code is a bit confusing:

axi_dmac does not free before returning -1
(https://github.com/analogdevicesinc/no-OS/blob/main/drivers/axi_core/axi_dmac/axi_dmac.c#L349)

axi_adc_core frees the allocation before returning -1 But the the pointer is not checked for null in remove.
https://github.com/analogdevicesinc/no-OS/blob/main/drivers/axi_core/axi_adc_core/axi_adc_core.c#L677

So in case one of the int function call fails, im not sure we can trust the pointer to be freed, or
to be freed and null.

ill assume those are separate issues and make the change anyways.

regarding https://github.com/analogdevicesinc/no-OS/blob/main/drivers/axi_core/axi_dmac/axi_dmac.c#L349, it looks like the remove function should be called if ret is not 0. so the suggestion works great in this case. I do not know why this implementation was done like that. personally I would rework it into something like:

struct axi_dmac *dmac; dmac = (struct axi_dmac *)no_os_calloc(1, sizeof(*dmac)); if (!dmac) return -1; dmac->name = init->name; dmac->base = init->base; dmac->irq_option = init->irq_option; int32_t status = axi_dmac_detect_caps(dmac); if (status < 0) goto free; *dmac_core = dmac; return 0; free: no_os_free(dmac); return -1;

regarding https://github.com/analogdevicesinc/no-OS/blob/main/drivers/axi_core/axi_adc_core/axi_adc_core.c#L677,
the descriptor remains null in case of an error, only in case of success the descriptor is set: https://github.com/analogdevicesinc/no-OS/blob/main/drivers/axi_core/axi_adc_core/axi_adc_core.c#L673

so basically, dev->ad796x_core will be null if init is not successful, and when calling the remove function nothing will happen.
the problem I see is that axi_adc_remove does not check for null pointer, we should add that check I think.

yes, ok. with those changes.

BTW i have done it this way because the logic i was following was
to keep balanced number of init/remove.

this would mean that:

init will free its resources in case of fail, a failed init is not a init (like you mention)

remove should not be called if init was not called before

remove should not be called for a failed init

should not assume the state of the pointer

I understand... this is not mandatory. in my case the code would look cleaner...
also, to me there is no real value in adding a duplicate label: err_pwm_0

also, regarding my previous comment the problem I see is that axi_adc_remove does not check for null pointer, we should add that check I think. there is no problem with that API, it is safe to call free for null pointers.

drivers/adc/ad796x/ad796x.h

rbolboac · 2024-05-09T10:51:11Z

projects/ad796x_fmcz/src/examples/iio_example/iio_example.h

+/******************************************************************************/
+/***************************** Include Files **********************************/
+/******************************************************************************/
+


remove this

rbolboac · 2024-05-09T10:51:48Z

projects/ad796x_fmcz/src/platform/xilinx/parameters.c

+
+

remove extra blank lines

rbolboac · 2024-05-09T10:51:59Z

projects/ad796x_fmcz/src/platform/xilinx/parameters.h

+#include <xil_cache.h>
+#include <xilinx_uart.h>
+#include "xilinx_gpio.h"
+


remove extra blank line

rbolboac · 2024-05-09T10:52:20Z

projects/ad796x_fmcz/src/platform/xilinx/parameters.h

+#define UART_DEVICE_ID	XPAR_AXI_UART_DEVICE_ID
+#define INTC_DEVICE_ID	XPAR_INTC_SINGLE_DEVICE_ID
+#define UART_IRQ_ID		XPAR_AXI_INTC_AXI_UART_INTERRUPT_INTR


use the same indentation

projects/ad796x_fmcz/src/platform/xilinx/parameters.h

ahaslam2 · 2024-05-13T11:21:07Z

changes:

use remove function instead of err_ lables in driver init error path
call driver remove in case of error on project main
misc style fixes.

rbolboac · 2024-05-13T11:33:12Z

drivers/adc/ad796x/ad796x.c

+{
+	int ret;
+
+	if (dev->ad796x_core) {


the only thing I would check for null here is dev.
freeing a null pointer is allowed

rbolboac · 2024-05-13T11:34:51Z

drivers/adc/ad796x/ad796x.c

+	int ret, i;
+
+	struct no_os_gpio_desc *desc[] = {
+		dev->gpio_adc_en3_fmc,


dev should be checked for null before trying to access its members

ill use a table index instead to loop though the gpios.

rbolboac · 2024-05-13T11:36:01Z

drivers/adc/ad796x/ad796x.c

+		if (!desc[i])
+			continue;


no_os_gpio_remove already checks for null pointer, no need to do it here

amiclaus

minor comments, otherwise lgtm

amiclaus · 2024-05-21T09:49:21Z

drivers/axi_core/iio_axi_adc/iio_axi_adc.c

@@ -43,9 +43,10 @@
 /***************************** Include Files **********************************/
 /******************************************************************************/

-#include <inttypes.h>
-#include <stdio.h>
+


drop this extra line.

amiclaus · 2024-05-21T09:50:41Z

drivers/adc/ad796x/ad796x.c

+	[AD796X_MODE1_EXT_REF_5P0] =		{1, 0, 0, 1},
+	[AD796X_MODE2_INT_REF_4P0] =		{1, 0, 0, 1},
+	[AD796X_MODE3_EXT_REF_4P0] =		{0, 1, 0, 1},
+	[AD796X_MODE4_SNOOZE] =		{1, 1, 0, 1},


odd indentation: check raw file.

amiclaus · 2024-05-21T10:28:19Z

drivers/adc/ad796x/ad796x.c

+
+	return ret;
+}
+


drop extra line

amiclaus · 2024-05-21T10:28:30Z

drivers/adc/ad796x/ad796x.h

+/* Read samples from the device */
+int32_t ad796x_read_data(struct ad796x_dev *dev, uint32_t *buf,
+			 uint16_t samples);
+


drop extra line

rbolboac

some remarks from my side, also ad796x driver documentation is missing
same remark regarding commit message length, can have up to 72 characters per line.

rbolboac · 2024-05-22T07:12:27Z

projects/ad796x_fmcz/src/examples/iio_example/iio_example.c

+	if (ret)
+		goto err_app_run;
+
+	return 0;


I would remove these lines...if fore some reason iio_app_run returns (either with 0 or another value), the remove APIs should call before exiting this example.

rbolboac · 2024-05-22T07:13:01Z

projects/ad796x_fmcz/src/examples/iio_example/iio_example.c

+	iio_axi_adc_remove(iio_axi_adc_desc);
+err_adc_init:
+	ad796x_remove(adc_dev);
+	return ret;


if (ret) I would print an error message

Ok ill put print in after the ret that fails were i can show who produced the error.

rbolboac · 2024-05-22T07:13:48Z

projects/ad796x_fmcz/src/platform/xilinx/main.c

+	ret = iio_example_main();
+#elif BASIC_EXAMPLE
+	ret = basic_example_main();
+#endif


maybe add an #error message in case no example is defined

rbolboac · 2024-05-23T10:50:31Z

do you plan on adding the driver documentation in this PR or in a new one?

ahaslam2 · 2024-05-23T14:22:58Z

do you plan on adding the driver documentation in this PR or in a new one?

i will add it to this PR

if inttypes.h is decared at the start of the header a compilation error might happen because __int64_t_defined is not defined causing PRIi64 to not be defined either. By moving the inttypes.h header after sdio, __int64_t_defined is defiend and we can use PRIi64. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>

Memory should be freed when returning error from init. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>

This adds the driver for the ad796x series of chips. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>

This adds the project files for ad796x. It supports iio and basic example for the xilinx platform. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>

rbolboac · 2024-05-24T11:20:45Z

drivers/adc/ad796x/README.rst

+
+AD796X Device Configuration
+----------------------------
+


looks like device configuration documentation is missing...
in general, I think it will be useful to also describe the available APIs, just in case someone checks the documentation before the code... there are various examples here: http://analogdevicesinc.github.io/no-OS/drivers_doc.html
also, the commit message states that only project documentation is added, meanwhile you also add the driver documentation

i took the example from https://analogdevicesinc.github.io/no-OS/drivers/adm1177.html
where "driver initialization " is a subtopic of "device configuration "

in fact there is no device configuration for this driver. only driver initialization and reading data,
which i mention, but perhaps not clearly enough?

It is not clear that driver initialization is a subtopic of device configuration. they are using the same indentation. use ^^^^, instead of ---- for driver initialization if that is the case

ok , i just followed what was done in the other docs:

AD405X Device Configuration --------------------------- Driver Initialization --------------------- In order to be able to use the device, you will have to provide the support for the communication protocol (SPI) as well as 3 external GPIOs for the CNV pin and two

ADM1177 Device Configuration ---------------------------- Driver Initialization --------------------- In order to be able to use the device, you will have to provide the support for the communication protocol (I2C) as mentioned above. The first API to be called is **adm1177_init**. Make sure that it returns 0, which means that the driver was initialized correctly.

This adds project documentation for ad796x. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>

ahaslam2 force-pushed the axelh/add-ad796x branch 2 times, most recently from b24a10b to 0a3d1da Compare April 4, 2024 15:29

ahaslam2 marked this pull request as ready for review April 12, 2024 10:53

ahaslam2 requested review from buha, CiprianRegus, dbogdan, amiclaus, rbolboac and danmois as code owners April 12, 2024 10:53

rbolboac reviewed Apr 18, 2024

View reviewed changes

drivers/adc/ad796x/ad796x.c Show resolved Hide resolved

drivers/adc/ad796x/ad796x.c Show resolved Hide resolved

drivers/adc/ad796x/ad796x.h Show resolved Hide resolved

drivers/adc/ad796x/ad796x.h Outdated Show resolved Hide resolved

ahaslam2 force-pushed the axelh/add-ad796x branch from 0a3d1da to cfff8a0 Compare April 19, 2024 07:57

ahaslam2 force-pushed the axelh/add-ad796x branch 8 times, most recently from 576859c to 2bb46dc Compare May 6, 2024 13:28

ahaslam2 force-pushed the axelh/add-ad796x branch from 2bb46dc to 634a268 Compare May 6, 2024 13:42

rbolboac reviewed May 9, 2024

View reviewed changes

ahaslam2 force-pushed the axelh/add-ad796x branch from 634a268 to d477d84 Compare May 13, 2024 11:18

rbolboac reviewed May 13, 2024

View reviewed changes

ahaslam2 force-pushed the axelh/add-ad796x branch 4 times, most recently from 4f70f6d to 27fbc6a Compare May 15, 2024 16:39

amiclaus reviewed May 21, 2024

View reviewed changes

rbolboac reviewed May 22, 2024

View reviewed changes

ahaslam2 force-pushed the axelh/add-ad796x branch from 27fbc6a to 3b8c8a9 Compare May 23, 2024 09:01

ahaslam2 added 4 commits May 23, 2024 17:02

drivers: axi_dmac: Call free in case of error

68576f0

Memory should be freed when returning error from init. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>

drivers: adc: Add ad796x

7186d54

This adds the driver for the ad796x series of chips. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>

projects: Add ad796x_fmcz

671a919

This adds the project files for ad796x. It supports iio and basic example for the xilinx platform. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>

ahaslam2 force-pushed the axelh/add-ad796x branch 2 times, most recently from c5408f2 to 9520c43 Compare May 24, 2024 09:51

rbolboac reviewed May 24, 2024

View reviewed changes

projects: ad796x_fmcz: Add Documentation

176b39a

This adds project documentation for ad796x. Signed-off-by: Axel Haslam <ahaslam@baylibre.com>

ahaslam2 force-pushed the axelh/add-ad796x branch from 9520c43 to 176b39a Compare May 27, 2024 09:25

rbolboac approved these changes May 27, 2024

View reviewed changes

amiclaus approved these changes May 27, 2024

View reviewed changes

ahaslam2 merged commit b26434c into main May 28, 2024
14 checks passed

ahaslam2 deleted the axelh/add-ad796x branch May 28, 2024 05:13

ad796x: Add project and driver for ad796x-fmc #2139

ad796x: Add project and driver for ad796x-fmc #2139

Conversation

ahaslam2 commented Apr 4, 2024 • edited

PR Type

PR Checklist

rbolboac left a comment

Choose a reason for hiding this comment

ahaslam2 commented Apr 19, 2024

rbolboac commented Apr 19, 2024

ahaslam2 commented Apr 19, 2024

ahaslam2 commented Apr 19, 2024 • edited

rbolboac commented Apr 19, 2024

ahaslam2 commented May 6, 2024

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

ahaslam2 May 13, 2024 • edited

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

ahaslam2 commented May 13, 2024

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

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

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

rbolboac commented May 23, 2024

ahaslam2 commented May 23, 2024 • edited

rbolboac May 24, 2024 • edited

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

ahaslam2 commented Apr 4, 2024 •

edited

ahaslam2 commented Apr 19, 2024 •

edited

ahaslam2 May 13, 2024 •

edited

ahaslam2 commented May 23, 2024 •

edited

rbolboac May 24, 2024 •

edited