Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bp-16079} arch/arm/src/samv7: enable GPIO clock even if interrupt not enabled #16114

Merged
merged 1 commit into from
Apr 2, 2025

Conversation

jerpelea
Copy link
Contributor

@jerpelea jerpelea commented Apr 2, 2025

Summary

GPIO peripheral clocks have to be enabled even if interrupt support on the peripheral is not enabled, otherwise GPIO input will not work. This is probably caused by input filters that need clock signal.

This commit moves the clock enable call from interrupt specific function to common initialization function that is called from _start entry point function.

Impact

RELEASE

Testing

CI

GPIO peripheral clocks have to be enabled even if interrupt support
on the peripheral is not enabled, otherwise GPIO input will not work.
This is probably caused by input filters that need clock signal.

This commit moves the clock enable call from interrupt specific function
to common initialization function that is called from _start entry
point function.

Signed-off-by: Michal Lenc <[email protected]>
@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture Size: S The size of the change in this PR is small labels Apr 2, 2025
@nuttxpr
Copy link

nuttxpr commented Apr 2, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although it's missing some specifics. Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the "why," "what," and "how" of the change effectively.
  • Impact Section Mostly Addressed: The "RELEASE" tag in the Impact section implies significant impact, but lacks detail. Marking specific sub-sections as "NO" or providing more context about the "RELEASE" impact would improve clarity.
  • Testing Performed: The "CI" mention suggests Continuous Integration testing was done, which is good.

Weaknesses:

  • Missing Issue References: No related issue numbers are provided. Even if no issue exists, stating "N/A" would be helpful.
  • Impact Section Lacks Detail: "RELEASE" is vague. Explicitly stating the impact on users, build, hardware, documentation, security, and compatibility with "YES" or "NO" and further explanations is crucial, even if the answer is "NO." For a release-worthy change, there's likely some impact.
  • Insufficient Testing Detail: "CI" is insufficient. While CI is important, the PR should mention specific build hosts and targets tested. Generic examples are provided in the template, which should be replaced with actual details. The "Testing logs before/after" sections are also empty, making it difficult to assess the effectiveness of the testing.

Recommendation:

The PR needs more detail, particularly in the Impact and Testing sections. Specifically:

  • Add any relevant issue numbers, or "N/A."
  • Replace "RELEASE" with specific impact details (even if "NO") for each impact category. Justify why this is a release-worthy change.
  • Provide specific build host and target information (OS, architecture, board, configuration).
  • Include relevant testing logs or explain why they are not included. If logs are too extensive, refer to CI logs with specific links/references.

By addressing these points, the PR will be significantly strengthened and more likely to be accepted.

@lupyuen lupyuen merged commit 526a8ea into apache:releases/12.9 Apr 2, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants