linux-mips
[Top] [All Lists]

Re: [PATCH] enable PCI bridges in MIPS ip32

To: Giuseppe Sacco <giuseppe@eppesuigoccas.homedns.org>
Subject: Re: [PATCH] enable PCI bridges in MIPS ip32
From: "Maciej W. Rozycki" <macro@linux-mips.org>
Date: Thu, 4 Oct 2007 13:27:47 +0100 (BST)
Cc: Ralf Baechle <ralf@linux-mips.org>, linux-mips@linux-mips.org
In-reply-to: <E1IdO0a-0000n7-Cg@eppesuigoccas.homedns.org>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <E1IdO0a-0000n7-Cg@eppesuigoccas.homedns.org>
Sender: linux-mips-bounce@linux-mips.org
On Thu, 4 Oct 2007, Giuseppe Sacco wrote:

> I managed to create a patch against current 2.6.23-rc9 git tree
> for supporting PCI bridges on SGI ip32 machines.
> This is my first kernel patch, so I am usure about the correct way
> to send a patch. Please let me know if anything is wrong.

 I am glad you have succeeded.  A couple of minor notes below.

> @@ -31,20 +31,21 @@
>  
>  #define chkslot(_bus,_devfn)                                 \
>  do {                                                         \
> -     if ((_bus)->number > 0 || PCI_SLOT (_devfn) < 1 \
> -         || PCI_SLOT (_devfn) > 3)                           \
> +     if ((_bus)->number > 1 ||                               \
> +             ((_bus)->number == 0 && (PCI_SLOT (_devfn) < 1  \
> +             || PCI_SLOT (_devfn) > 3)))                     \
>               return PCIBIOS_DEVICE_NOT_FOUND;                \

 I think you should allow any bus numbers, not only 0 and 1 -- while 
possibly unlikely, you may have a tree of bridges on an option card.  The 
generic code should handle it fine -- you need not care.

> -#define mkaddr(_devfn, _reg) \
> -((((_devfn) & 0xffUL) << 8) | ((_reg) & 0xfcUL))
> +#define mkaddr(_bus, _devfn, _reg) \
> +((((_bus)->number & 0xffUL) << 16) | (((_devfn) & 0xffUL) << 8) | ((_reg) & 
> 0xfcUL))

 Please fit your lines in 80 characters.

> -     mace->pci.config_addr = mkaddr(devfn, reg);
> +     mace->pci.config_addr = mkaddr(bus, devfn, reg);

 It may be more consistent if you pass just "bus->number".  You may neatly 
avoid the line wrap above this way too.

 Have you run your change through `scripts/checkpatch.pl'?

  Maciej

<Prev in Thread] Current Thread [Next in Thread>