On Wed, 2005-03-09 at 12:45 +0200, Pekka Enberg wrote:
> 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.
Oh sure, will do. I'm not about to submit anything yet anyway, and it
will go through a cleanup phase. The above is just residual of quilt
picking up a file where I added something, then removed it.
> > 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);
The above isn't whitespace damage, it's aligning of the 3 variable
names properly in a column :) I dislike those DECLARE_*() macros because
of that btw. That one is a matter of style, I'm experiencing a bit with
this, but it's definitely intentional.
>
> 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;
> }
> }
I think you are beeing anal here, but I'll think about it ;)
> > +/*
> > + * 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.
Will do.
Ben.
-
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/
