Skip to content

Conversation

@michallenc
Copy link
Contributor

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

GPIO inputs now work even if the peripheral is not configured with interrupt support (option CONFIG_SAMV7_GPIOx_IRQ).

Testing

Tested on custom SAMv7 board with GPIOD peripheral. Reading pins now work even if CONFIG_SAMV7_GPIOD_IRQ is disabled.

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 Mar 26, 2025
@nuttxpr
Copy link

nuttxpr commented Mar 26, 2025

[Experimental Bot, please feedback here]

Yes, this PR appears to meet the NuttX requirements, although it could be slightly improved. Here's a breakdown:

Strengths:

  • Clear Summary: The summary explains the problem, the solution, and the affected code area.
  • Impact Description: The impact on the user is clearly stated.
  • Testing Provided: Evidence of testing is included, mentioning the specific target and the observed improvement.

Areas for Improvement (Minor):

  • Missing Issue References: While not strictly required if no related issue exists, it's generally good practice to create one and link it. This helps with tracking and searchability.
  • Incomplete Testing Details: The "Testing" section could be more robust. While the target is mentioned, providing more context about the Build Host would be helpful (OS, compiler, etc.). Also, the "logs" are empty. While full logs might be excessive, a snippet demonstrating the change in behavior (e.g., reading a GPIO value before and after the change) would be beneficial.
  • Missing "NO" in Impact Sections: Explicitly stating "NO" for the unaffected impact categories (build process, hardware changes other than GPIOD usage, documentation, security, compatibility) would improve clarity and completeness.

Example of improved "Impact" section:

## Impact
* Is new feature added? NO
* Is existing feature changed? YES (GPIO input functionality corrected)
* Impact on user (will user need to adapt to change)? NO 
* Impact on build (will build process change)? NO
* Impact on hardware (will arch(s) / board(s) / driver(s) change)? YES (SAMv7 GPIOD peripheral usage will now function correctly without IRQ configuration)
* Impact on documentation (is update required / provided)? NO
* Impact on security (any sort of implications)? NO
* Impact on compatibility (backward/forward/interoperability)? NO
* Anything else to consider? NO

Example of improved "Testing" section:

## Testing
I confirm that changes are verified on local setup and works as intended:
* Build Host(s): Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
* Target(s): SAMV7, Custom Board (board_config), sim

Testing logs before change (CONFIG_SAMV7_GPIOD_IRQ=n):

GPIOD pin X read: -1 (or some error indicating failure)


Testing logs after change (CONFIG_SAMV7_GPIOD_IRQ=n):

GPIOD pin X read: 1

By addressing these minor points, the PR would be even stronger and easier to review.

@lupyuen lupyuen merged commit bd62888 into apache:master Mar 29, 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