Hi Benjamin,
Few coding style nitpicks follow.
On Tue, 08 Mar 2005 18:11:59 +1100, Benjamin Herrenschmidt
<[EMAIL PROTECTED]> wrote:
> Index: linux-work/include/linux/pci.h
> ===================================================================
> --- linux-work.orig/include/linux/pci.h 2005-01-24 17:09:57.000000000
> +1100
> +++ linux-work/include/linux/pci.h 2005-03-08 15:26:25.000000000 +1100
> @@ -1064,5 +1064,6 @@
> #define PCIPCI_VSFX 16
> #define PCIPCI_ALIMAGIK 32
>
> +
> #endif /* __KERNEL__ */
> #endif /* LINUX_PCI_H */
Please drop whitespace noise from the patch.
> Index: linux-work/drivers/pci/vga.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ linux-work/drivers/pci/vga.c 2005-03-08 18:04:57.000000000 +1100
> @@ -0,0 +1,403 @@
> +static LIST_HEAD( vga_list);
Please remove whitespace damage.
> +static spinlock_t vga_lock;
> +static DECLARE_WAIT_QUEUE_HEAD( vga_wait_queue);
Ditto.
> +/* Architecture can override enabling/disabling of a given
> + * device resources here
> + */
> +#ifndef __ARCH_HAS_VGA_DISABLE_RESOURCES
> +static inline void vga_disable_resources(struct pci_dev *pdev,
> + unsigned int rsrc,
> + unsigned int change_bridge)
> +{
> + struct pci_bus *bus;
> + struct pci_dev *bridge;
> + u16 cmd;
> +
> + pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> + if (rsrc & VGA_RSRC_IO)
> + cmd &= ~PCI_COMMAND_IO;
> + if (rsrc & VGA_RSRC_MEM)
> + cmd &= ~PCI_COMMAND_MEMORY;
> + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +
> + if (!change_bridge)
> + return;
> +
> + bus = pdev->bus;
> + while (bus) {
> + bridge = bus->self;
> + if (bridge) {
> + pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &cmd);
> + if (!(cmd & PCI_BRIDGE_CTL_VGA))
> + continue;
> + cmd &= ~PCI_BRIDGE_CTL_VGA;
> + pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, cmd);
> + }
> + bus = bus->parent;
See comment below.
> + }
> +}
> +#endif
> +
> +#ifndef __ARCH_HAS_VGA_ENABLE_RESOURCES
> +static inline void vga_enable_resources(struct pci_dev *pdev,
> + unsigned int rsrc)
> +{
> + struct pci_bus *bus;
> + struct pci_dev *bridge;
> + u16 cmd;
> +
> + pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> + if (rsrc & VGA_RSRC_IO)
> + cmd |= PCI_COMMAND_IO;
> + if (rsrc & VGA_RSRC_MEM)
> + cmd |= PCI_COMMAND_MEMORY;
> + pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +
> + bus = pdev->bus;
> + while (bus) {
> + bridge = bus->self;
> + if (bridge) {
> + pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &cmd);
> + if (cmd & PCI_BRIDGE_CTL_VGA)
> + continue;
> + cmd |= PCI_BRIDGE_CTL_VGA;
> + pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, cmd);
> + }
> + bus = bus->parent;
Please consolidate both while loops into one function. One possible way would
be to do:
static void vga_update_bus(struct pci_bus *bus, unsigned int enable)
{
while (bus) {
bridge = bus->self;
if (bridge) {
pci_read_config_word(bridge, PCI_BRIDGE_CONTROL, &cmd);
if (cmd & PCI_BRIDGE_CTL_VGA)
continue;
if (enable)
cmd |= PCI_BRIDGE_CTL_VGA;
else
cmd &= ~PCI_BRIDGE_CTL_VGA;
pci_write_config_word(bridge, PCI_BRIDGE_CONTROL, cmd);
}
bus = bus->parent;
}
}
> +/*
> + * Currently, we assume that the "initial" setup of the system is
> + * sane, that is we don't come up with conflicting devices, which
> + * would be annoying. We could double check and be better at
> + * deciding who is the default here, but we don't.
> + */
> +void vga_arbiter_add_pci_device(struct pci_dev *pdev)
> +{
> + struct vga_device *vgadev;
> + unsigned long flags;
> + struct pci_bus *bus;
> + struct pci_dev *bridge;
> + u16 cmd;
> +
> + /* Only deal with VGA class devices */
> + if ((pdev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> + return;
> +
> + /* Allocate structure */
> + vgadev = kmalloc(sizeof(struct vga_device), GFP_KERNEL);
> + memset(vgadev, 0, sizeof(*vgadev));
Please consider using kcalloc() here.
Pekka
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
