-
Notifications
You must be signed in to change notification settings - Fork 551
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
RTL8168 Implementation #1652
base: master
Are you sure you want to change the base?
RTL8168 Implementation #1652
Conversation
/// </summary> | ||
/// <param name="port"></param> | ||
/// <returns></returns> | ||
public static byte InB(ushort port) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer we go with the same approach we have for other drivers as well, which is that we define all the the IOPorts we need and then access the one we actually need. For example see
Cosmos/source/Cosmos.Core/IOGroup/ATA.cs
Line 13 in a3d0a07
public class ATA : IOGroup |
That approach should be quicker since we arnt constantly creating new objects and less error prone than calculating the port address every time.
|
||
if (((GetMacVersion() & 0x7cf00000) == 0x54100000) || ((GetMacVersion() & 0x7cf00000) == 0x54000000)) | ||
{ | ||
Console.WriteLine("8168H Detected!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove this Console.WriteLine
public bool Reset() | ||
{ | ||
Ports.OutB((ushort)(BaseAddress + 0x37), 0x10); /* Send the Reset bit to the Command register */ | ||
while ((Ports.InB((ushort)(BaseAddress + 0x37)) & 0x10) != 0) { } /* Wait for the chip to finish resetting */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have timeout in this method, in which case we return false?
} | ||
|
||
protected void HandleNetworkInterrupt(ref IRQContext aContext) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are all the Console.WriteLines in this method required? They seem like debug output and should be removable
|
||
Console.WriteLine("NIC IRQ: " + device.InterruptLine); | ||
|
||
var RTL8168Device = new RTL8168(device); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is the device accessible from outside the class? Again I dont like the Console.WriteLines, maybe make the Init(bool aShowOutput) have a toggle for console output?
No description provided.