no need to typedef this enum

Removed typedef as suggested.

no need to typedef this enum

Removed typedef as suggested.

I'm seeing a pattern, but yeah like the others, don't typedef enums/structs please. We somewhat follow linux coding style and typedefs like this aren't warranted.

https://kernel.org/doc/html/latest/process/coding-style.html#typedefs

Thanks for the reference. Removed typedef as suggested.

use the WAIT_FOR macro here with a time limit if the hardware has some guarantees on time to reset you can follow and fail on

Used WAIT_FOR macro with timeout

not needed if you use goto and labels or early returns, please fix and drop do { } while(0);

We follow MISRA C rule 15.5, which recommends having a single point of exit at the end of a function. While there are multiple ways to achieve this, we generally avoid using goto, as it’s often discouraged. Using the do { } while (0) construct is one valid way to meet this rule.
What’s your opinion on this approach within Zephyr’s coding style?

@teburd We have already discussed the use of do { } while(0) in another PR: #93450 (comment) , which has now been approved and merged.

MISRA C:2012 Rule 15.1 – The goto statement should not be used:
https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_15_01.c

MISRA C:2012 Rule 15.5 – A function should have a single point of exit at the end:
https://gitlab.com/MISRA/MISRA-C/MISRA-C-2012/Example-Suite/-/blob/master/R_15_05.c

We aim to comply with both rules wherever possible, which is why we use the do { } while(0) style here.

Zephyr is massively not compliant then already as almost everything uses goto, including the kernel in large numbers of functions.

do {} while(0) is a relatively new approach to this in the zephyr codebase, and not one that seems to add readability in my view, @nashif @kartben do we have any guidelines on these misra rules? I'd argue readability matters a great deal more and this is less readable to me at least than the much more commonly practiced style that we have throughout Zephyr.

if (some_err_branch_check) {
   ret = -EIO;
   goto out;
}

// more code

out
   return ret;

As far as I can see the rule 15.1 is "unrestricted use of goto is bad" but I don't know for certain as the rules are not published publicly.

But Misra has a few rules around usage of goto according to mathworks.

https://www.mathworks.com/help/bugfinder/ref/misrac2023rule15.2.html
https://www.mathworks.com/help/bugfinder/ref/misrac2023rule15.3.html

Why have any rules around using goto if you can't use it, makes no sense.

I understand that the intention here is to avoid multiple exit point from a function (https://in.mathworks.com/help/bugfinder/ref/misrac2023rule15.5.html). This is an advisory Misra C rule. To avoid multiple returns, there are multiple methods.
Some are

1. Restructure the logic
2. encapsulate inside do-while(0) with break statement
3. use goto statement.

First preference should be option 1. Second option can be option 2, do-while(0). The usage of goto is not recommended as it violates another advisory Misra C rule(https://in.mathworks.com/help/bugfinder/ref/misrac2023rule15.1.html) . Usage of goto is like violating one advisory rule to fix another advisory rule.
So I would suggest to live with multiple returns whenever do-while(0) doesn't solve the multiple return issue.

@ArunMCHP, in this particular case, i could see that the issue can be solved by option 1, by restructuring the code without using do-while. Could you please explore that option?

@teburd , the Misra rules you mentioned (15.2 and 15.3) are required rules.
I hope the zephyr will not prevent anyone from following Misra rules as long as it doesn't violates any of the zephyr coding guidelines.

@carlescufi can we add this topic, specifically around the usage of do { } while(0) in place of goto for single exit?

@ArunMCHP this whole conversation is absurd, let's put aside that many MISRA rules are ambiguous and there's no publicly available documentation for them, 15.1 and 15.5 are not even mentioned in https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html. So that is not an argument and even less so is the fact that you managed to drive maintainers to exhaustion in #93450 and get an instance of that merged.

What you are doing here is abusing do{} while() loops and going against the coding style and common practice of keeping the normal execution flow in line and limiting the indentation in functions, and making these drivers incoherent with the rest of the code base as well. Plus wasting reviewers time since you got told that from multiple person already.

I understand the good intentions, but this has to stop. Please fix the coding style, here and in https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/clock_control/clock_control_mchp_sam_d5x_e5x.c as well. We don't need more anti-patterns in the code base.

Thanks.

And probably drivers/serial/uart_mchp_sercom_g1.c while you are at it.

@ArunMCHP this whole conversation is absurd, let's put aside that many MISRA rules are ambiguous and there's no publicly available documentation for them, 15.1 and 15.5 are not even mentioned in https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html. So that is not an argument and even less so is the fact that you managed to drive maintainers to exhaustion in #93450 and get an instance of that merged.

What you are doing here is abusing do{} while() loops and going against the coding style and common practice of keeping the normal execution flow in line and limiting the indentation in functions, and making these drivers incoherent with the rest of the code base as well. Plus wasting reviewers time since you got told that from multiple person already.

I understand the good intentions, but this has to stop. Please fix the coding style, here and in https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/clock_control/clock_control_mchp_sam_d5x_e5x.c as well. We don't need more anti-patterns in the code base.

Thanks.

This topic is under Architecture WG discussion. The changes will be done based on the decision made in WG.

Hi @fabiobaltieri,

Thanks for the feedback. In the Architecture WG meeting, we concluded not to introduce new do { } while (0) constructs going forward.

I’ve already updated the code in this PR to align with Zephyr’s coding style. The file https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/clock_control/clock_control_mchp_sam_d5x_e5x.c will be updated in the next PR.

Thanks for the clarification.

k_fifo really isn't the right tool for this, either use a k_mem_slab, sys_block, or array with atomic bitmap signaling used/unused. If you want the cheapest computational route make an array of arrays, one array for each channel with some number of descriptors. No locks, no lists, no checks needed at all since channels are single owner.

This undef looks suspicious, if nothing else its going to lead to confusion if someone greps around for DMAC_REGS and sees there's two defines for it. If the token is already defined then use a new one, no need to undef and redef.

Used a new macro.

extra unnecessary spacing here

Removed extra line.

Really a goto here is just fine, we use them everywhere in Zephyr for single exits, you can drop the irq_locked flag as well and have two exit labels.

Replaced the do { } while (0) single-exit construct with if-else based flow for better clarity wherever possible.
Also removed irq_lock() since no shared resources are accessed among channels.

irq_lock still here it appears, if the register that is being manipulated isn't shared (it doesn't appear to be) among channels, then it is safe to do so without locking.

nit: drop the blank line

dropped

same

dropped

the pointer themselves are not changing right? so they can go in config, save few bytes of sram

These pointers are constant, so moving them to config would save about 8 bytes, but it would make the code a bit less readable and harder to maintain. Do you still want me to move them?

no need to initialize this, drop, then the compiler can do its job and warn if there's a code path where this is used uninitialized

Thanks, I’ve removed the variable initialization.

return -EINVAL, return 0 at the end of the function and drop ret

dropped ret and used the return as suggested. Thanks.

same

same

dropped.

use \ consistently, preferably aligned on the right but at least consistently

updated.

same

updated

nit: this should be .dma_ctx = {, bracket inline, pass if you are using clang-format since that's a lost battle but in that case you probably forgot to rerun it?

Used clang-format, still the bracket goes to next line.