Hi Huacai, [...] > The ASpeed AST2500 hardward does not set the VGA Enable bit on its It would be "hardware" in the above sentence. > bridge control register, which causes vgaarb subsystem don't think the Probably better to say "don't consider the" rather than use "think". > VGA card behind the bridge as a valid boot vga device. It would be "VGA" in the above sentence, to be consistent. > So we provide a quirk to fix Xorg auto-detection. A nit pick. Technically it's "X.org", but I see that the canon in the commit messages is to use "Xorg", so either would be valid. > See similar bug: > > https://patchwork.kernel.org/project/linux-pci/patch/20170619023528.11532-1-dja@xxxxxxxxxx/ A small nit pick. Linking to https://lore.kernel.org/linux-pci/ is often preferred. [...] > + struct pci_dev *bridge; > + struct pci_bus *bus; > + struct pci_dev *vdevp = NULL; > + u16 config; > + > + bus = pdev->bus; > + bridge = bus->self; Preferred style would be: struct pci_bus *bus = pdev->bug; struct pci_dev *bridge = bus->self; > + /* Is VGA routed to us? */ > + if (bridge && (pci_is_bridge(bridge))) { > + pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &config); > + > + /* Yes, this bridge is PCI bridge-to-bridge spec compliant, > + * just return! > + */ Second line of the comment has some errand space. Also, wording of this comment could be improved a bit. > + if (config & PCI_BRIDGE_CTL_VGA) > + return; > + > + dev_warn(&pdev->dev, "VGA bridge control is not enabled\n"); > + } > + > + /* Just return if the system already have a default device */ Missing period at the end of the sentence in the comment. I am also not sure this comment is useful, as the if-statement immediately below is quite self-explanatory. > + if (vga_default_device()) > + return; > + > + /* No default vga device */ It would be "VGA" here, plus missing period at the end. > + while ((vdevp = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, vdevp))) { > + if (vdevp->vendor != 0x1a03) { > + /* Have other vga devcie in the system, do nothing */ It would be "VGA" and "device" in the comment above, missing period at the end. I am also not sure if this comment is useful, given the log line just below it. > + dev_info(&pdev->dev, "Another boot vga device: 0x%x:0x%x\n", It would be "VGA" in the message above. Also, using "Another" is a bit vague. What makes a VGA device "another" in this case? > + dev_info(&pdev->dev, "Boot vga device set as 0x%x:0x%x\n", > + pdev->vendor, pdev->device); You need to align the second line in the above with the open bracket, like in the log line above. > +DECLARE_PCI_FIXUP_CLASS_FINAL(0x1a03, 0x2000, PCI_CLASS_DISPLAY_VGA, 8, aspeed_fixup_vgaarb); You might need to wrap this line above, see: https://lore.kernel.org/linux-pci/20171026223701.GA25649@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ Krzysztof
