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

risc-v/bl808: Add I2C driver #15332

Merged
merged 1 commit into from
Dec 25, 2024
Merged

Conversation

henryrov
Copy link
Contributor

Summary

This change implements a driver with support for all four I2C blocks on the BL808.

Impact

Improved hardware support.

Testing

Tested by communicating with another MCU using i2ctool. Waveforms and timing validated against RP2040 I2C driver using a logic analyzer.
To replicate testing, select one or more I2C blocks under BL808 Peripheral Support and enable i2ctool.

This change implements a driver with support for all four I2C blocks on the BL808.
@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Board: risc-v Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. labels Dec 24, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 24, 2024

[Experimental Bot, please feedback here]

Yes, this PR appears to mostly meet the requirements, but is missing some crucial details. Here's a breakdown of what's good and what needs improvement:

Good:

  • Clear Summary of the Change: The summary clearly states the purpose and scope of the change (new I2C driver for BL808).
  • Impact Summary: It highlights the improved hardware support, which is the primary impact.
  • Testing Methodology: The testing approach is described, including tools (i2ctool, logic analyzer) and comparison with a known-good implementation (RP2040). Instructions on how to replicate the testing are also provided.

Needs Improvement:

  • Summary (Details): While the "what" is addressed, the "how" is missing. Briefly describe how the driver interacts with the hardware and any specific techniques used. Mention any relevant NuttX issues, even if there isn't a directly related one. For example, was this driver modeled after another I2C driver in NuttX?
  • Impact (Details): While improved hardware support is mentioned, expand on this. Does this affect any existing drivers or configurations? Are there any new Kconfig options? All the "NO/YES" questions in the template must be answered explicitly. Pay particular attention to these:
    • Impact on build: Does the build system need any modifications to support this new driver? New Kconfig options?
    • Impact on hardware: Specifically which BL808 boards are supported? Be explicit.
    • Impact on documentation: At a minimum, the driver should be documented in the code. Does any other documentation need updating (e.g., board configuration files, user guides)? Confirm YES/NO.
  • Testing (Details): The testing description is a good start, but actual log output is required. Include snippets showing successful I2C communication before and after the change. Also provide details about the Build Host and Target(s) used for testing. Just saying "Tested by communicating" isn't sufficient. Show the commands used, the expected output, and the actual output. Be specific about the BL808 board and configuration used.

Example of improved testing section:

Testing

I confirm that changes are verified on local setup and works as intended:

* Build Host(s): Ubuntu 22.04, x86_64, GCC 11.2.0
* Target(s): BL808 (PineCone BL602), `nsh` configuration

Testing logs before change: (No I2C driver available)

nsh> i2ctool -b 0 -s 0x42
i2ctool: failed to register I2C driver


Testing logs after change:

nsh> i2ctool -b 0 -s 0x42 # Write 0x12 to register 0x00
nsh> i2ctool -b 0 -r 0x42 -l 1 # Read 1 byte from register 0x00
0x12



By filling in the missing details, the PR will be much stronger and easier to review. This demonstrates due diligence and increases the chances of quick acceptance.

@xiaoxiang781216 xiaoxiang781216 merged commit 923dc37 into apache:master Dec 25, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Area: Documentation Improvements or additions to documentation Board: risc-v Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants