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

PCI: Use u8 as in the spec, add port docs #827

Merged
merged 2 commits into from
Feb 9, 2023

Conversation

hecatia-elegua
Copy link
Contributor

@hecatia-elegua hecatia-elegua commented Jan 26, 2023

  • also adds generic read/write methods for one of the ports

Please merge after #825.
I'm unsure if this is the right way to, like, do a follow-up PR?
I guess if the content is unrelated, I can start a completely different branch..
would be wild if one could specify that this PR depends on the previous one being done (which is very natural, similar to the git branch concept)

Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this looks pretty good! I'm not sure why we were using u16s instead of u8s in the first place.

One request -- I'm not sure how I feel about #825, I may not merge that in yet. But the changes from commit 815484f are generally good, and also independent from #825. So let's keep this PR strictly about u16 --> u8 such that we can address #825 separately.

Basically, please remove all changes related to #825, e.g., the determine_mem_base and BaseAddress thing.

kernel/pci/src/lib.rs Outdated Show resolved Hide resolved
kernel/pci/src/lib.rs Outdated Show resolved Hide resolved
kernel/pci/src/lib.rs Outdated Show resolved Hide resolved
@hecatia-elegua
Copy link
Contributor Author

About #825: I guess you mean that the API for getting the base address is bad? I have more changes to come so it will not stay that way and I can read up on pci BARs to find out if returning an enum is sensible

@hecatia-elegua
Copy link
Contributor Author

Sorry for the second force-push, now it should be good though. The docs are better too now.

Copy link
Member

@kevinaboos kevinaboos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, everything looks good now.

@kevinaboos kevinaboos merged commit 346a116 into theseus-os:theseus_main Feb 9, 2023
github-actions bot pushed a commit that referenced this pull request Feb 9, 2023
* No change in functionality -- just adhering to PCI spec.

Co-authored-by: Kevin Boos <[email protected]> 346a116
github-actions bot pushed a commit to kevinaboos/Theseus that referenced this pull request Feb 9, 2023
* No change in functionality -- just adhering to PCI spec.

Co-authored-by: Kevin Boos <[email protected]> 346a116
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants