Hello Carl-Daniel,
all-in-all, your patch looks good. Still I have some comments to that mail
Am Sonntag, den 06.01.2013, 22:13 +0100 schrieb Carl-Daniel Hailfinger:
- Use the PCI command word for autoskip
How would that work?
I still haven't figured that out.
If a PCI device is completely disabled (and thus not eligible as flasher), I expected the PCI command word to not enable I/O or memory decoding while it is needed for flashing.
- Implement a set-of-BAR (bitmask, array) anyway
Depending on the PCI header type, BARs can be at completely different locations. Still, set-of-BAR sounds like a good idea.
That doesn't work for satasii which uses different BARs for different PCI IDs.
That could be solved by moving the bitmask into the pcidev_status structure. Which means the mask can be different for each ID.
- Probe that all BARs (except ROM perhaps) that are not unused contain
sensible values
How do we determine which values are sensible?
That doesn't work either for external programmers which need more than one BAR, and one of those BARs may be disabled by default (i.e. unusable).
If things like that really happen, this does not work. I expected a device to either get ressources assigned by the BIOS or PCI core of the OS or none at all.
- Hand in a "is_usable" callback (a standard callback for testing a
single BAR could be provided)
A standard pcidev_validator_bar0 in pcidev.c still might make sense to avoid reimplement it in every second programmer driver.
Do you really trust new driver authors to get that right?
I think this one is the only viable solution, and with my automatic driver writer script (and plenty of good code to copy from) I think that there is a good chance new driver authors will get it right. I have named the callback "validator" instead of "is_usable", but I have no preference for either name.
validator also sounds nice. go with it.
+static int ogp_spi_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BAR. */
- return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
+}
this would be pcidev_validator_bar0 (or pcidev_validate_bar0, depending on whether you want a nominal clause or an action as function name).
-struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar) +struct pci_dev *pcidev_init(const struct dev_entry *devs, int (*validator) (struct pci_dev *dev))
Function declarations including function pointer types are quite difficult to read in my oppinion. What about "typedef int (*pcidev_validator)(struct pci_dev *dev)"?
if ((addr = pcidev_readbar(dev, bar)) != 0) {
if (!(*validator)(dev)) {
The star and the parenthesis are not needed in C. "if (!validator(dev))" would do as well. Furthermore, we don't use this explicit indirection syntax for all those function pointers in structures.
+static int gfxnvidia_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BAR. */
- return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
+}
A second instance of pcidev_validator_bar0
+static int nicrealtek_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BAR. */
- return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
+}
A third one
+static int nicintel_spi_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BAR. */
- return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
+}
A fourth one
+static int nicnatsemi_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BAR. */
- return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
+}
Another one, no sense in counting them anymore ;)
+static int nic3com_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BAR. */
- return !pcidev_readbar(dev, PCI_BASE_ADDRESS_0);
+}
Well, yes.
- dev = pcidev_init(satas_sii, PCI_BASE_ADDRESS_0);
dev = pcidev_init(satas_sii, satasii_validator); if (!dev) return 1;
id = dev->device_id;
- if ((id == 0x3132) || (id == 0x3124)) {
Well, this works for sure, but couldn't we somehow associate this info into the pci device ID list, instead of repeating some IDs here? Of course this would also mean that pcidev_init needs to return a pointer to the matched pcidev_status entry. Possibly not worth the hassle.
+static int nicintel_validator(struct pci_dev *dev) +{
- /* Return 0 if we have a valid BARs. */
- return !(pcidev_readbar(dev, PCI_BASE_ADDRESS_0) && pcidev_readbar(dev, PCI_BASE_ADDRESS_2));
+}
int nicintel_init(void) { struct pci_dev *dev = NULL; @@ -78,7 +84,7 @@ return 1;
/* FIXME: BAR2 is not available if the device uses the CardBus function. */
Maybe you want to move the FIXME comment into the validator declaration.
-struct pci_dev *pcidev_init(const struct dev_entry *devs, int bar); +struct pci_dev *pcidev_init(const struct dev_entry *devs, int (*validator) (struct pci_dev *dev));
typedef, as above.
Regards, Michael Karcher