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

Add GPIO support for Rockchip RV1103 #2303

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ZhangGaoxing
Copy link
Contributor

@ZhangGaoxing ZhangGaoxing commented Apr 4, 2024

Add GPIO support for Rockchip RV1103 and some boards like LuckFox Pico.

I do some test in LuckFox Pico with pin 137, the OS is Ubuntu 22.04, Linux kernel version is 5.10.110, .NET version is 8.0.203. The GPIO speed is 219KHz

1

Microsoft Reviewers: Open in CodeFlow

@raffaeler
Copy link
Contributor

Hi @ZhangGaoxing great contribution, thank you.
On my side I would love to see comments including:

  • a link to the official specifications (including relevant pages, where possible)
  • a bit more descriptions in the constants (Rv1103Driver offsets)

@ZhangGaoxing
Copy link
Contributor Author

Some datasheets can refer to the following link: https://wiki.luckfox.com/Luckfox-Pico/Datasheets/
and the register offsets I wrote in the code comments 😄

@raffaeler
Copy link
Contributor

Some datasheets can refer to the following link: https://wiki.luckfox.com/Luckfox-Pico/Datasheets/ and the register offsets I wrote in the code comments 😄

I meant something a bit more detailed :-)

The general guidelines applied to the rest of the repository are:

  • The link should go to the README file, specifying the paragraph or page numbers related to the driver
  • In the comments, specify the paragraph or page of the datasheet where to find the numbers
  • Where relevant, create a const int for the numbers that used more than a single time (no need for numbers that are signatures, prefixes, etc)

Thank you for the great work!

@Ellerbach
Copy link
Member

[Triage] @ZhangGaoxing it would be perfect if you can address the feedback from @raffaeler. And we don't have the device to test or do anything with it. So please adjust a bit your PR and we will be super happy to review it and approve and of course merge it after. Thanks.

@joperezr
Copy link
Member

[Triage] Hey @ZhangGaoxing your PR seems to be in a good state code-wise (thanks for the contribution!), but we think it needs some work in adding a few comments just to explain where some of the registers come from and where can we look at the docs. This is very important to us as it will help us maintain this driver going forward. Do you think you'll have cycles to do a quick pass over those? That way we can go ahead and take the change.

@joperezr joperezr added the Needs: Author Feedback We are waiting for author to react to feedback (action required) label May 23, 2024
Copy link
Contributor

This pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs: Author Feedback We are waiting for author to react to feedback (action required) Status: No Recent Activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants