Support AS5304T and AS6302T #42
Conversation
based on patch from mafredri#37 added detection logic based on PCI devices
mafredri
left a comment
mafredri
left a comment
There was a problem hiding this comment.
Nice work, thanks!
Minor nits and suggestions, but looks good overall.
(I noticed after reviewing that I commented on some old comments that were simply moved, but I still think they're valid.)
Do you btw want us to wait for testing before merging?
| GPIO_LOOKUP_IDX(GPIO_AS6300, 22, NULL, 11, GPIO_ACTIVE_HIGH), // sata2:green:disk ++ | ||
| GPIO_LOOKUP_IDX(GPIO_AS6300, 21, NULL, 12, GPIO_ACTIVE_HIGH), // sata2:red:disk ++ | ||
| {} | ||
| // GPIO_LOOKUP_IDX(GPIO_IT87, 53, NULL, 15, GPIO_ACTIVE_HIGH), // beep:status <<-- It can beep too!!! |
There was a problem hiding this comment.
What's the purpose of these commented lines, remove?
There was a problem hiding this comment.
I guess while probing for LED GPIOs, the author figured out that GPIO 53 beeps and wanted to document this - sensible idea IMHO
| GPIO_LOOKUP_IDX(GPIO_IT87, 21, NULL, 7, GPIO_ACTIVE_LOW), // green:usb | ||
| GPIO_LOOKUP_IDX(GPIO_IT87, 55, NULL, 8, GPIO_ACTIVE_HIGH), // blue:lan | ||
|
|
||
| GPIO_LOOKUP_IDX(GPIO_AS5304, 1, NULL, 9, GPIO_ACTIVE_HIGH), // sata1:green:disk ++ |
There was a problem hiding this comment.
This line and below seem weird, [tab]++?
| GPIO_LOOKUP_IDX(GPIO_AS5304, 1, NULL, 9, GPIO_ACTIVE_HIGH), // sata1:green:disk ++ | |
| GPIO_LOOKUP_IDX(GPIO_AS5304, 1, NULL, 9, GPIO_ACTIVE_HIGH), // sata1:green:disk |
|
|
||
| static struct asustor_driver_data asustor_6300_driver_data = { | ||
| .name = "AS63xx", | ||
| // This (and the remaining systems) don't need to match PCI devices to be detected, |
There was a problem hiding this comment.
Nit: This comment raises rather than answers a question, please see if you can make it answer "why", or consider if it's useful at all.
There was a problem hiding this comment.
it explains why, in contrast to the previous driver data definitions, .pci_matches is not initialized here or in the remaining ones
| // ASMedia ASM2806 4-Port PCIe x2 Gen3 Packet Switch | ||
| // (exists on AS602T and AS604T, but not AS5304T) | ||
| // in the lspci outputs I've seen both had 5 such devices, | ||
| // but let's be a bit more flexible here.. |
There was a problem hiding this comment.
Is this to cover known 6604T but unknown 6602T, or are both known? If it's always 5 on both, I'd err on strictness, a false positive could be worse (changing random GPIOs) than a false negative.
There was a problem hiding this comment.
I'm not sure if all switches are always visible, or only if e.g. all m.2 ports are used
There was a problem hiding this comment.
Also, we generally don't worry about false positives - after all, most devices are only matched by DMI which has an even higher chance of false positives.
So unless an incompatible device turns up that is wrongly matched here (and can be told apart by the number of PCIe switches) I wouldn't worry about that
|
yes, I think we should wait for testing - I'm especially curious if 53xx and 66xx even have to be distinguished or if they work the same after all, because there have been reports that the with the existing code controlling LEDs doesn't work did the first round of changes according to review |
2 participants
based on patch from
#37
added detection logic based on PCI devices
Needs testing by people owning AS5304T, AS6302T (@Tki2000), as well as people owning AS66xxT (because AS5304T was formerly wrongly detected as AS66xx).