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
