Skip to content

imx9_lpuart: use small lock in arch/arm(64)/src/imx9/imx9_lpuart.c#15291

Merged
xiaoxiang781216 merged 1 commit into
apache:masterfrom
hujun260:apache_41
Dec 20, 2024
Merged

imx9_lpuart: use small lock in arch/arm(64)/src/imx9/imx9_lpuart.c#15291
xiaoxiang781216 merged 1 commit into
apache:masterfrom
hujun260:apache_41

Conversation

@hujun260
Copy link
Copy Markdown
Contributor

Summary

imx9_lpuart: use small lock in arch/arm(64)/src/imx9/imx9_lpuart.c
reason:
We would like to replace the big lock with a small lock.

Impact

imx9_lpuart

Testing

ci

@github-actions github-actions Bot added Arch: arm Issues related to ARM (32-bit) architecture Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: M The size of the change in this PR is medium labels Dec 19, 2024
@nuttxpr
Copy link
Copy Markdown

nuttxpr commented Dec 19, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details.

Here's what's missing:

  • Summary: The summary is too vague. It needs to explain why a smaller lock is desired (e.g., performance improvement, reduced interrupt latency). It also needs to describe how the change is implemented (e.g., replacing nxsem_wait with nxsem_trywait, using a spinlock instead of a semaphore).
  • Impact: The impact section is severely lacking. It simply repeats "imx9_lpuart." It needs to address all the points listed in the requirements template. For example:
    • Is new feature added? Is existing feature changed? This is a change to an existing feature.
    • Impact on user: Potentially yes, if the change affects performance. This needs to be investigated and documented.
    • Impact on build: Likely no, but this should be explicitly stated.
    • Impact on hardware: Specific to the imx9 architecture.
    • Impact on documentation: Potentially yes, if the locking behavior is documented.
    • Impact on security/compatibility: These need to be addressed, even if the answer is no.
  • Testing: "ci" is insufficient. The testing section needs to include:
    • Specific build host details (OS, compiler version, etc.).
    • Specific target details (board, configuration).
    • Actual logs demonstrating the behavior before and after the change. Simply stating "ci" doesn't provide any evidence that the change works as intended.

The PR needs significant revision to meet the NuttX requirements. It needs to be more specific and provide detailed information about the change, its impact, and the testing performed.

Comment thread arch/arm64/src/imx9/imx9_lpuart.c Outdated
Comment thread arch/arm64/src/imx9/imx9_lpuart.c Outdated
Comment thread arch/arm64/src/imx9/imx9_lpuart.c Outdated
Comment thread arch/arm64/src/imx9/imx9_lpuart.c Outdated
Comment thread arch/arm/src/imx9/imx9_lpuart.c Outdated
Comment thread arch/arm/src/imx9/imx9_lpuart.c Outdated
Comment thread arch/arm/src/imx9/imx9_lpuart.c Outdated
reason:
We would like to replace the big lock with a small lock.

Signed-off-by: hujun5 <hujun5@xiaomi.com>
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 Arch: arm64 Issues related to ARM64 (64-bit) architecture Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants