ADE9113 driver for IMX6ULL by xvuko · Pull Request #670 · phoenix-rtos/phoenix-rtos-devices

xvuko · 2026-06-02T18:41:33Z

Driver for continuous sample reading from 4 x ADE9113 connected in SPI chain on IMX6ULL

Description

Motivation and Context

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Chore (refactoring, style fixes, git/CI config, submodule management, no code logic changes)

How Has This Been Tested?

Already covered by automatic testing.
New test added: (add PR link here).
Tested by hand on: (list targets here).

Checklist:

My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing linter checks and tests passed.
My changes generate no new compilation warnings for any of the targets.

Special treatment

This PR needs additional PRs to work (list the PRs, preferably in merge-order).
I will merge this PR by myself when appropriate.

gemini-code-assist

Code Review

This pull request introduces a Phoenix-RTOS driver for the ADE9113 ADC (4 chained chips), featuring command/response handling, IMX6ULL DMA support, a message API server, and a sample ring buffer. It also updates the IMX6ULL SDMA driver to support automatic buffer descriptor completion options and word-aligned memory access. Critical feedback identifies a potential deadlock in sampleRead due to a missing unlock on an error path, a null pointer dereference in the message API when handling inactive sessions, and an incorrect mmap failure check. Other high-to-medium severity issues include a compilation warning from returning a value in a void function, a resource leak in sample_init on allocation failure, a leftover debug condition in the monitor thread, and the need to use static inline void for sample_write to prevent linker issues.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist · 2026-06-02T18:46:59Z

+		struct sampleReader *reader = NULL;
+		if ((msg.oid.id > 0) && (msg.oid.id <= MSGAPI_READER_COUNT)) {
+			reader = &ctx->readers[msg.oid.id - 1];
+		}


If a client sends mtRead or mtClose with a valid msg.oid.id but for a session that is not open or already closed, reader->ctx is NULL. Calling sample_readBlock or sample_readerClose on such a reader will dereference NULL and crash the driver. We should validate that the reader is active before processing.

struct sampleReader *reader = NULL; if ((msg.oid.id > 0) && (msg.oid.id <= MSGAPI_READER_COUNT)) { reader = &ctx->readers[msg.oid.id - 1]; if (reader->ctx == NULL) { reader = NULL; } }

I added separate sample_readerInit function so that ctx is always valid

gemini-code-assist · 2026-06-02T18:47:00Z

+int sample_init(struct sampleCtx *ctx, unsigned expSize, unsigned sampleSize)
+{
+	if ((expSize > 31) || (((uint32_t)sampleSize >> expSize) > 0)) {
+		return -1;
+	}
+
+	*ctx = (struct sampleCtx) {
+		.written = 0,
+		.sampleSize = sampleSize,
+		.mask = ((uint32_t)1 << expSize) - 1,
+		.buffer = malloc((uint32_t)1 << expSize)
+	};
+
+	if (mutexCreate(&ctx->lock) < 0) {
+		fprintf(stderr, "mutex create failed");
+		assert(false);
+		return -1;
+	}
+	if (condCreate(&ctx->cond) < 0) {
+		fprintf(stderr, "cond create failed");
+		assert(false);
+		return -1;
+	}
+
+	if (ctx->buffer == NULL) {
+		fprintf(stderr, "failed to allocate sample buffer");
+		return -1;
+	}
+	return 0;


In sample_init, if malloc fails to allocate ctx->buffer, the function returns -1 but does not destroy the previously created ctx->lock and ctx->cond. Allocating the buffer first avoids this leak entirely.

int sample_init(struct sampleCtx *ctx, unsigned expSize, unsigned sampleSize) { if ((expSize > 31) || (((uint32_t)sampleSize >> expSize) > 0)) { return -1; } uint8_t *buffer = malloc((uint32_t)1 << expSize); if (buffer == NULL) { fprintf(stderr, "failed to allocate sample buffer"); return -1; } *ctx = (struct sampleCtx) { .written = 0, .sampleSize = sampleSize, .mask = ((uint32_t)1 << expSize) - 1, .buffer = buffer }; if (mutexCreate(&ctx->lock) < 0) { fprintf(stderr, "mutex create failed"); free(buffer); assert(false); return -1; } if (condCreate(&ctx->cond) < 0) { fprintf(stderr, "cond create failed"); free(buffer); assert(false); return -1; } return 0; }

github-actions · 2026-06-02T19:10:12Z

Unit Test Results

10 860 tests ±0 10 190 ✅ ±0 53m 22s ⏱️ +21s
670 suites ±0 670 💤 ±0
1 files ±0 0 ❌ ±0

Results for commit 59c2073. ± Comparison against base commit bccac7e.

♻️ This comment has been updated with latest results.

anglov

Commit with generated headers should be added before implementation

anglov · 2026-06-08T10:21:23Z

@@ -0,0 +1,1273 @@
+/* clang-format off */
+/* Auto-generated from iomux_enum_defs.xml */


Refering to non-existing file?

Removed, added header. For now added scripts to internal repo.

anglov · 2026-06-11T16:56:20Z


 		unsigned i;
 		for (i = 1; i < NUM_OF_SDMA_CHANNELS; i++) {
+			sdma_channel_t *channel = &cmn->channel[i];


Refactoring could be done in separate commit (ok as-is)

Added separate commit

anglov · 2026-06-16T12:54:42Z

+
+
+struct sdmaCtx {
+	volatile bool enabled;


note: using this flag in interrupt is safe because operation on bool is atomic on arm.

Changed to sig_atomic_t.

anglov · 2026-06-16T15:17:08Z

+	ade9113_writeRegs(&ade, ADE9113_SYNC_SNAP, ADE9113_SYNC_SNAP_ENC(.align = 1));
+	ade9113_writeRegs(&ade, ADE9113_SYNC_SNAP, ADE9113_SYNC_SNAP_ENC(.snapshot = 1));


why two transaction required?

Added comment. This sequence is required in datasheet

Please expand this comment. As described in datasheet doesn't provide much context. Explain what's happening here or cite the relevant part of the datasheet.

anglov · 2026-06-16T15:23:55Z

+			ADE9113_CONFIG0_ENC(.stream_dbg = args->dbgMode, .crc_en_spi_write = 1, .clkout_en = 0));
+
+	/* scratch values will be checked in DMA responses to ensure there is no shift */
+	ade9113_writeRegsDifferent(&ade, ADE9113_SCRATCH, 0xA0, 0xA1, 0xA2, 0xA3);


consider
`SCRATCH_VALUE, SCRATCH_VALUE + 1, SCRATCH_VALUE + 2, SCRATCH_VALUE + 3'

changed to

/* value written to SCRATCH registers to verify reads (arbitrary unique value for each ADC chip) */ static const uint8_t scratchValues[4] = { 0xa0, 0xa1, 0xa2, 0xa3 };

anglov · 2026-06-16T15:35:16Z

+
+	msgapi_init(&ctx->msgapi, &ctx->samples, "ade9113");
+
+	static uint8_t msgStacks[2][_PAGE_SIZE] __attribute__((aligned(16)));


why alignment needed?

Changed to 8. Kernel's HAL adds padding if to stack if not padded to 8.

anglov · 2026-06-16T15:36:55Z

+				msg.o.err = -EBUSY;
+
+				if (mutexLock(ctx->lock) < 0) {
+					msg.o.err = -ENOSYS;


weird errno?

changed to -EIO as there is no better code for internal (non temporary) errors

anglov · 2026-06-16T15:43:38Z

@@ -0,0 +1,557 @@
+/* ADE9113 register and field definitions. */
+/* Auto-generated from ade9113.svd - do not edit manually. */


how and where to find?

For now internal devtools repo. Proper way to upstream this and provide headers via library should be considered.

TASK: MSH-36

SDMA commands GET_DM and SET_DM take size in 32 bit words. TASK: MSH-36

Lock is always released inside condWait TASK: MSH-36

TASK: MSH-36

There is no need to `DSPOVR`o register. IMX6 does not have DSP core. It is set to 0xffffffff after reset and there is no need for any explicit writes from driver. Line `DSPOVR |= 1 << channel_id` looks like it might do something important. It does not. It makes `sdma_init_core` harder to understand without any benefit. TASK: MSH-36

TASK: MSH-36

BD_DONE flags indicate that SDMA "owns" buffer described in BD and can write into it. When SDMA returns ownership to processor/driver buffer content should be read *before* setting BD_DONE flag. This ensures data integrity (no read / write collisions). This description assumes RX transfer, but same applies to TX with read / writes swapped. Automatic BD_DONE set can still be useful in some cases like sending fixed buffer repeatedaly (like read requests from ADC). Backwards incompatible. Use `channelCfg.options = sdma_chOption__auto_bd_done` to keep old behaviour. TASK: MSH-36

TASK: MSH-36

ziemleszcz · 2026-07-01T14:02:03Z

+ *
+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+


Please add header guard.

ziemleszcz · 2026-07-01T14:02:39Z

+ * SPDX-License-Identifier: BSD-3-Clause
+ */
+
+


Please add header guard.

ziemleszcz · 2026-07-01T14:03:13Z

+ */
+
+
+#ifndef MSGAPI_H


ADE9113_MSGAPI_H?

ziemleszcz · 2026-07-01T14:03:26Z

+ */
+
+
+#ifndef SAMPLE_H


ADE9113_SAMPLE_H?

ziemleszcz · 2026-07-01T14:04:58Z

+
+#define log_error(fmt, ...) \
+	do { \
+		fprintf(stdout, LOG_TAG fmt "\n", ##__VA_ARGS__); \


ziemleszcz · 2026-07-01T14:05:24Z

+		fprintf(stdout, LOG_TAG fmt "\n", ##__VA_ARGS__); \
+	} while (0)
+
+#define log_info(fmt, ...) log_error(fmt, ##__VA_ARGS__);


fprintf(stdout, ...)?

ziemleszcz · 2026-07-01T14:09:15Z

+		0
+		| (0x01 << 23) /* RXDEN = 1 */
+		| (15 << 16)   /* RX_THRESHOLD = 15 */
+		| (0x01 << 7)  /* TXDEN = 1 */


TXDEN -> TEDEN

ziemleszcz · 2026-07-02T08:28:44Z

+		ade9113_readRegs(&ade, ADE9113_CONFIG_CRC, values, sizeof(values));
+		bool ready = true;
+		for (int i = 0; i < 4; ++i) {
+			if ((values[i] & 0x01) == 0) {


0x1 is CRC_FORCE. According to the datasheet, it's automatically cleared when CRC recalculation completes. Did you mean to check CRC_DONE (0x2) instead?

ziemleszcz · 2026-07-02T08:47:33Z

+	ade9113_writeRegs(&ade, ADE9113_SYNC_SNAP, ADE9113_SYNC_SNAP_ENC(.align = 1));
+	ade9113_writeRegs(&ade, ADE9113_SYNC_SNAP, ADE9113_SYNC_SNAP_ENC(.snapshot = 1));


Please expand this comment. As described in datasheet doesn't provide much context. Explain what's happening here or cite the relevant part of the datasheet.

ziemleszcz · 2026-07-02T08:51:25Z

+			ADE9113_SWRST__SOFTWARE_RESET_COMMAND,
+			ADE9113_SWRST__SOFTWARE_RESET_COMMAND,
+			ADE9113_SWRST__SOFTWARE_RESET_COMMAND);
+	usleep(300 * 1000);


Nitpick: consider polling STATUS0 for RESET_DONE instead of using a fixed usleep.

github-actions Bot reviewed Jun 2, 2026

View reviewed changes

Comment thread adc/ade9113/dma.c Outdated

xvuko changed the title ~~Xvuko/msh 36~~ ADE9113 driver for IMX6ULL Jun 2, 2026

gemini-code-assist Bot reviewed Jun 2, 2026

View reviewed changes

xvuko force-pushed the xvuko/msh-36 branch from 6921b7f to ca8db3b Compare June 2, 2026 18:53

xvuko force-pushed the xvuko/msh-36 branch 3 times, most recently from 63f4fb7 to e464da1 Compare June 2, 2026 19:33

xvuko requested a review from anglov June 2, 2026 19:35

xvuko force-pushed the xvuko/msh-36 branch 2 times, most recently from f67967c to 6247513 Compare June 9, 2026 15:45

anglov reviewed Jun 16, 2026

View reviewed changes

xvuko added 7 commits June 24, 2026 17:17

imx6ull-sdma: add missing const

bfd46e1

TASK: MSH-36

imx6ull-sdma: format

a2fea65

TASK: MSH-36

imx6ull-sdma: check size alignment in data read/write

5178ca2

TASK: MSH-36

imx6ull-sdma: fix data memory read/write size calculation

2cbb3fb

SDMA commands GET_DM and SET_DM take size in 32 bit words. TASK: MSH-36

imx6ull-sdma: optimize lock usage

378bcee

Lock is always released inside condWait TASK: MSH-36

imx6ull-sdma: add README

41578fe

TASK: MSH-36

xvuko force-pushed the xvuko/msh-36 branch from 6247513 to 142b75b Compare June 24, 2026 19:19

xvuko added 2 commits June 26, 2026 19:33

imx6ull-sdma: refactor sdma_intr

541d76b

TASK: MSH-36

xvuko force-pushed the xvuko/msh-36 branch from 142b75b to 6feba86 Compare June 26, 2026 17:34

adc/ade9113: add ADE9113 driver for IMX6ULL

59c2073

TASK: MSH-36

xvuko force-pushed the xvuko/msh-36 branch from 6feba86 to 59c2073 Compare June 26, 2026 17:45

xvuko requested a review from ziemleszcz June 26, 2026 17:54

xvuko marked this pull request as ready for review June 26, 2026 17:54

ziemleszcz reviewed Jul 2, 2026

View reviewed changes

		@@ -0,0 +1,1273 @@
		/* clang-format off */
		/* Auto-generated from iomux_enum_defs.xml */

		ade9113_writeRegs(&ade, ADE9113_SYNC_SNAP, ADE9113_SYNC_SNAP_ENC(.align = 1));
		ade9113_writeRegs(&ade, ADE9113_SYNC_SNAP, ADE9113_SYNC_SNAP_ENC(.snapshot = 1));


		msgapi_init(&ctx->msgapi, &ctx->samples, "ade9113");

		static uint8_t msgStacks[2][_PAGE_SIZE] __attribute__((aligned(16)));

		@@ -0,0 +1,557 @@
		/* ADE9113 register and field definitions. */
		/* Auto-generated from ade9113.svd - do not edit manually. */

		*/


		#ifndef MSGAPI_H

		*/


		#ifndef SAMPLE_H

Uh oh!

Conversation

xvuko commented Jun 2, 2026

Description

Motivation and Context

Types of changes

How Has This Been Tested?

Checklist:

Special treatment

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

gemini-code-assist Bot Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot Jun 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Jun 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unit Test Results

Uh oh!

anglov 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!

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!

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!

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!

github-actions Bot commented Jun 2, 2026 •

edited

Loading