Skip to content

arch/arm/rp23xx: Fix PWM registers access #16210

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

Merged
merged 1 commit into from
Apr 15, 2025

Conversation

shtirlic
Copy link
Contributor

Fix offset alignment and correct the PWM enable register name

Summary

Current PWM on rp23xx implementation is broken this pull fixes this.

Impact

Working PWM

Testing

Tested on pico2 with single channel and multichannel (2) setup via pwm app.

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

nuttxpr commented Apr 15, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details required for proper review. Here's a breakdown:

  • Summary: Too vague. Needs to specify why the current implementation is broken (what's the bug?), what offset alignment is being fixed, and which register name is incorrect. It mentions a functional part (PWM on rp23xx), but not clearly enough. No related issue references are provided.
  • Impact: Simply stating "Working PWM" isn't helpful. It needs to explicitly answer all the impact questions (user, build, hardware, documentation, security, compatibility) with "YES/NO" and provide descriptions where necessary. Even if the answer is "NO", it's good practice to explicitly state it.
  • Testing: Insufficient. Needs to specify the host OS, CPU, and compiler version used for building. The target information is slightly better, but could be more precise (e.g., "pico2 with xyz configuration"). The biggest issue is the lack of actual logs. Empty code blocks are useless. Provide real logs demonstrating the issue before the change and the corrected behavior after the change.

In short, the PR needs more detail and actual testing logs to be considered complete.

Fix offset alignment and correct the PWM enable register name

Signed-off-by: Serg Podtynnyi <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit 5ddc67b into apache:master Apr 15, 2025
25 checks passed
@shtirlic shtirlic deleted the rp23xxx-fixpwm branch April 15, 2025 09:11
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.

4 participants