Enable lpadc dma feature #97414

ZhaoxiangJin · 2025-10-12T14:15:55Z

Add DMA transfer capability to LPADC

Fix adc_dt run issue on frdm_mcxn236 board, the default mode of vref is standby mode, the BUF21 is not enabled, so the LPADC can not work normally. Need to set the vref mode to 'NXP_VREF_MODE_LOW_POWER' or 'NXP_VREF_MODE_HIGH_POWER' to enable the BUF21. Signed-off-by: Zhaoxiang Jin <[email protected]>

Add dmas properties for lpadc node in nxp_mcxn23x_common.dtsi Signed-off-by: Zhaoxiang Jin <[email protected]>

Adding DMA transfer capability to LPADC Signed-off-by: Zhaoxiang Jin <[email protected]>

Enable DMA transfer of LPADC data in frdm_mcxn236 Signed-off-by: Zhaoxiang Jin <[email protected]>

sonarqubecloud · 2025-10-13T07:05:39Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

ZhaoxiangJin · 2025-10-15T02:36:50Z

Hello @hakehuang, could you please help to test samples/adc/adc_dt for frdm_mcxn236 board? Thanks.

hakehuang · 2025-10-15T05:48:06Z

Hello @hakehuang, could you please help to test samples/adc/adc_dt for frdm_mcxn236 board? Thanks.

samples/adc/adc_dt/samples.yaml need add frdm_mcxn236 in platform_allow, besides, why tests/drivers/adc is not enabled for dma test in this platform? @ZhaoxiangJin

decsny

I don't understand what's the point of this. This seems like an insane overcomplication of what is essentially reading one word. Previously, the CPU just reads the sample value from the register. Now, the CPU spends a ton of time to set up a DMA transfer to read still just one word from the register to a special one-word sized buffer, and then waits for DMA callback so that it can handle the dma interrupt from ADC driver, so that it can read the one word from the one-word buffer and restart the DMA transfer again. Can you explain what is the value of adding all this code, am I misunderstanding something?

decsny · 2025-10-15T12:32:00Z

drivers/adc/adc_mcux_lpadc.c

+		return;
+	}
+
+	(void)dma_stop(config->dma_dev, config->dma_channel);


why is return value ignored

decsny · 2025-10-15T12:32:49Z

drivers/adc/adc_mcux_lpadc.c

+	if (!data->use_dma) {
+		return;
+	}


seems like an unexpected situation, maybe log warning

@decsny there is a warning down in the mcux_lpadc_init() function if use_dma is false when the Kconfig is true. The flag will not be changed any other time so not needing a log message here as it would overwhelm the output.

no, this is the dma callback, which should only happen if dma is active, that's why it's unexpected and should log a warning

decsny · 2025-10-15T12:33:52Z

drivers/adc/adc_mcux_lpadc.c

+		if (data->ctx.sequence.resolution == 16) {
+			/* Keep raw 16-bit value; for differential, bit15 is sign. */
+			tmp = conv_result;
+		} else {
+			/* Right-align 12/13-bit payload from bit [3-14]. */
+			uint16_t value = (uint16_t)((conv_result >> 3) & 0xFFF);
+
+			if (is_diff && (conv_result & 0x8000)) {
+				/* 13-bit differential: apply two's complement sign
+				 * using 12-bit magnitude.
+				 */
+				value = (uint16_t)(value - 0x1000);
+			}
+			tmp = value;
+		}


turn this calculation into helper function

Why? It only seems to be used in one place?

reduce cognitive complexity of this function

decsny · 2025-10-15T12:34:34Z

drivers/adc/adc_mcux_lpadc.c

+	const struct device *dev = data->dev;
+	const struct mcux_lpadc_config *config = dev->config;
+
+	if (data->use_dma) {


reverse the condition to if (!data->use_dma) { return; } to remove a level of nesting /cognitive complexity

ZhaoxiangJin · 2025-10-15T13:13:23Z

I don't understand what's the point of this. This seems like an insane overcomplication of what is essentially reading one word. Previously, the CPU just reads the sample value from the register. Now, the CPU spends a ton of time to set up a DMA transfer to read still just one word from the register to a special one-word sized buffer, and then waits for DMA callback so that it can handle the dma interrupt from ADC driver, so that it can read the one word from the one-word buffer and restart the DMA transfer again. Can you explain what is the value of adding all this code, am I misunderstanding something?

Hello @decsny, in the zephyr samples, a sequence might only have one or two channels, but in real projects, many ADC channels might be used (this is why there are so many ADC channels in one SoC, for example, the MCXN236 has dozens of ADC channels). Without DMA, each channel's data would require an interrupt to retrieve it. If DMA is used, it can wait until all channels are converted, and then a single DMA callback is called. This operation reduces the number of ISRs and I think the overall efficiency will be improved.
Many ADCs can sample at rates up to 1M sps or even higher. At this sampling rate, DMA offers a greater advantage if there are many channels in a sequence. Furthermore, consider low-power scenarios. In tickless/low-power scenarios, DMA allows the core to sleep longer (the CPU doesn't have to wake up frequently to read registers), DMA wakes up the MCU after the transfer is complete, ultimately reducing energy consumption.

decsny · 2025-10-15T15:07:14Z

I am not talking about any sample, I'm talking about how your driver code works. Regardless of how many samples the application is doing, it looks like your code sets up a whole DMA transfer and callback for each 32 bit word. Do I misunderstand?

ZhaoxiangJin · 2025-10-15T15:35:27Z

I am not talking about any sample, I'm talking about how your driver code works. Regardless of how many samples the application is doing, it looks like your code sets up a whole DMA transfer and callback for each 32 bit word. Do I misunderstand?

No, the callback will occur after channel_counts words have been transmitted. please check the dma configuration.

dleach02 · 2025-10-16T02:54:30Z

Hello @hakehuang, could you please help to test samples/adc/adc_dt for frdm_mcxn236 board? Thanks.

samples/adc/adc_dt/samples.yaml need add frdm_mcxn236 in platform_allow, besides, why tests/drivers/adc is not enabled for dma test in this platform? @ZhaoxiangJin

@ZhaoxiangJin to ensure regression test coverage, we want to do this testing from the tests/drivers/adc and ensure enablement of DMA for the specific target. Can you update your PR to include this.

ZhaoxiangJin · 2025-10-16T03:16:59Z

Hello @hakehuang, could you please help to test samples/adc/adc_dt for frdm_mcxn236 board? Thanks.

samples/adc/adc_dt/samples.yaml need add frdm_mcxn236 in platform_allow, besides, why tests/drivers/adc is not enabled for dma test in this platform? @ZhaoxiangJin

@ZhaoxiangJin to ensure regression test coverage, we want to do this testing from the tests/drivers/adc and ensure enablement of DMA for the specific target. Can you update your PR to include this.

Hello @dleach02, yes, I will enable test for this board

decsny · 2025-10-16T13:08:23Z

I don't understand what's the point of this. This seems like an insane overcomplication of what is essentially reading one word. Previously, the CPU just reads the sample value from the register. Now, the CPU spends a ton of time to set up a DMA transfer to read still just one word from the register to a special one-word sized buffer, and then waits for DMA callback so that it can handle the dma interrupt from ADC driver, so that it can read the one word from the one-word buffer and restart the DMA transfer again. Can you explain what is the value of adding all this code, am I misunderstanding something?

Hello @decsny, in the zephyr samples, a sequence might only have one or two channels, but in real projects, many ADC channels might be used (this is why there are so many ADC channels in one SoC, for example, the MCXN236 has dozens of ADC channels). Without DMA, each channel's data would require an interrupt to retrieve it. If DMA is used, it can wait until all channels are converted, and then a single DMA callback is called. This operation reduces the number of ISRs and I think the overall efficiency will be improved. Many ADCs can sample at rates up to 1M sps or even higher. At this sampling rate, DMA offers a greater advantage if there are many channels in a sequence. Furthermore, consider low-power scenarios. In tickless/low-power scenarios, DMA allows the core to sleep longer (the CPU doesn't have to wake up frequently to read registers), DMA wakes up the MCU after the transfer is complete, ultimately reducing energy consumption.

I see I did misread a line and that the DMA transfer is for the number of channels. But I am still questioning the real value and "performance gain" of this code. It seems right now like we are doing this just to do it. Is there a specific requirement you are trying to meet (even if it's one you made up) that is clear about what actual benchmark/specification you are trying to accomplish and do you have testing proof that this actually meets it. Because right now, this is looking to me like a lot of new code and a new feature to maintain that I am not convinced of the real value yet, based on a hunch (you said you "think" the efficiency will be improved). Without real benchmarks and requirements, you could just as easily argue against this feature and say things like, it will increase latency and gaps in the ADC readings, it will cause the core to be awake for longer due to the complexity of setting up the DMA transfer and transferring the data between buffers with CPU at the end, I don't know. You are claiming it reduces energy consumption and improves efficiency, so where are the measurements of this.

ZhaoxiangJin · 2025-10-16T13:26:16Z

I don't understand what's the point of this. This seems like an insane overcomplication of what is essentially reading one word. Previously, the CPU just reads the sample value from the register. Now, the CPU spends a ton of time to set up a DMA transfer to read still just one word from the register to a special one-word sized buffer, and then waits for DMA callback so that it can handle the dma interrupt from ADC driver, so that it can read the one word from the one-word buffer and restart the DMA transfer again. Can you explain what is the value of adding all this code, am I misunderstanding something?

Hello @decsny, in the zephyr samples, a sequence might only have one or two channels, but in real projects, many ADC channels might be used (this is why there are so many ADC channels in one SoC, for example, the MCXN236 has dozens of ADC channels). Without DMA, each channel's data would require an interrupt to retrieve it. If DMA is used, it can wait until all channels are converted, and then a single DMA callback is called. This operation reduces the number of ISRs and I think the overall efficiency will be improved. Many ADCs can sample at rates up to 1M sps or even higher. At this sampling rate, DMA offers a greater advantage if there are many channels in a sequence. Furthermore, consider low-power scenarios. In tickless/low-power scenarios, DMA allows the core to sleep longer (the CPU doesn't have to wake up frequently to read registers), DMA wakes up the MCU after the transfer is complete, ultimately reducing energy consumption.

I see I did misread a line and that the DMA transfer is for the number of channels. But I am still questioning the real value and "performance gain" of this code. It seems right now like we are doing this just to do it. Is there a specific requirement you are trying to meet (even if it's one you made up) that is clear about what actual benchmark/specification you are trying to accomplish and do you have testing proof that this actually meets it. Because right now, this is looking to me like a lot of new code and a new feature to maintain that I am not convinced of the real value yet, based on a hunch (you said you "think" the efficiency will be improved). Without real benchmarks and requirements, you could just as easily argue against this feature and say things like, it will increase latency and gaps in the ADC readings, it will cause the core to be awake for longer due to the complexity of setting up the DMA transfer and transferring the data between buffers with CPU at the end, I don't know. You are claiming it reduces energy consumption and improves efficiency, so where are the measurements of this.

@decsny, I've already provided my response, and I don't wish to engage in endless debate on this matter. If you insist on blocking this PR, I have no objection to closing it. However, I am certain that in the future, there will definitely be requests for this feature from SE/FAE/customers, because the same thing has happened many times in the SDK. It is quite surprising that the support for ADC DMA functionality is being questioned, even after I've explained so much.

decsny · 2025-10-16T14:10:52Z

I don't want to have an endless debate either. I asked for numbers that are backing up your claims that this code is improving something. I'm not fundamentally against this, but you should be able to have evidence of your claims, otherwise I don't want to be adding code just for the sake of it. The only reason we would have an endless debate is if those numbers were never produced and we went in circles talking about guesses and hunches, which is exactly what I don't want to be doing things based on, because I don't want to be wasting time and effort, which are very precious resources.

In fact, if people are requesting it, that is promising for this PR, because it means there must be specific requirements that you are trying to meet?

ZhaoxiangJin · 2025-10-16T14:22:50Z

I don't want to have an endless debate either. I asked for numbers that are backing up your claims that this code is improving something. I'm not fundamentally against this, but you should be able to have evidence of your claims, otherwise I don't want to be adding code just for the sake of it. The only reason we would have an endless debate is if those numbers were never produced and we went in circles talking about guesses and hunches, which is exactly what I don't want to be doing things based on, because I don't want to be wasting time and effort, which are very precious resources.

In fact, if people are requesting it, that is promising for this PR, because it means there must be specific requirements that you are trying to meet?

Why do you always say I'm adding code just for the sake of adding code? Do you think I've really wasted so much time? I've already theoretically explained the advantages of DMA transfer, and there's no need for me to spend more time designing a case study to demonstrate the advantages of DMA transfer in a practical project. In my opinion, you haven't carefully read the code and my response.

ZhaoxiangJin force-pushed the enable-lpadc-edma-feature branch 4 times, most recently from 1769f55 to 4925d86 Compare October 13, 2025 06:22

ZhaoxiangJin added 4 commits October 13, 2025 14:58

dts: nxp: Add dma properties for lpadc node

feb94ad

Add dmas properties for lpadc node in nxp_mcxn23x_common.dtsi Signed-off-by: Zhaoxiang Jin <[email protected]>

drivers: adc: Adding DMA transfer capability to LPADC

c9a52d2

Adding DMA transfer capability to LPADC Signed-off-by: Zhaoxiang Jin <[email protected]>

samples: adc_dt: Enable DMA transfer of LPADC data in frdm_mcxn236

421bb34

Enable DMA transfer of LPADC data in frdm_mcxn236 Signed-off-by: Zhaoxiang Jin <[email protected]>

ZhaoxiangJin force-pushed the enable-lpadc-edma-feature branch from 4925d86 to 421bb34 Compare October 13, 2025 07:00

ZhaoxiangJin marked this pull request as ready for review October 13, 2025 08:08

zephyrbot added area: ADC Analog-to-Digital Converter (ADC) platform: NXP NXP platform: NXP MCU area: Boards/SoCs area: Samples Samples labels Oct 13, 2025

zephyrbot requested review from DerekSnell, EmilioCBen, JarmouniA, anangl, butok, dbaluta, decsny, dleach02, kartben, manuargue, mmahadevan108 and nashif October 13, 2025 08:10

zephyrbot assigned dleach02 and mmahadevan108 Oct 13, 2025

ZhaoxiangJin changed the title ~~Enable lpadc edma feature~~ Enable lpadc dma feature Oct 13, 2025

decsny requested changes Oct 15, 2025

View reviewed changes

ZhaoxiangJin closed this Oct 16, 2025

Enable lpadc dma feature #97414

Enable lpadc dma feature #97414

Conversation

ZhaoxiangJin commented Oct 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sonarqubecloud bot commented Oct 13, 2025

Quality Gate passed

Uh oh!

ZhaoxiangJin commented Oct 15, 2025

Uh oh!

hakehuang commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

decsny left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

ZhaoxiangJin commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

decsny commented Oct 15, 2025

Uh oh!

ZhaoxiangJin commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dleach02 commented Oct 16, 2025

Uh oh!

ZhaoxiangJin commented Oct 16, 2025

Uh oh!

decsny commented Oct 16, 2025

Uh oh!

ZhaoxiangJin commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

decsny commented Oct 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ZhaoxiangJin commented Oct 16, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ZhaoxiangJin commented Oct 12, 2025 •

edited

Loading

hakehuang commented Oct 15, 2025 •

edited

Loading

decsny left a comment •

edited

Loading

ZhaoxiangJin commented Oct 15, 2025 •

edited

Loading

ZhaoxiangJin commented Oct 15, 2025 •

edited

Loading

ZhaoxiangJin commented Oct 16, 2025 •

edited

Loading

decsny commented Oct 16, 2025 •

edited

Loading