On Wed, 2008-06-04 at 16:17 +0900, Hidetoshi Seto wrote:
> Thank you for good comments.
>
> Michael Ellerman wrote:
> >> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> >> index f62f941..b5decf3 100644
> >> --- a/drivers/pci/msi.c
> >> +++ b/drivers/pci/msi.c
> >> @@ -54,7 +53,8 @@ arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type)
> >> return 0;
> >> }
> >>
> >> -void __attribute__ ((weak)) arch_teardown_msi_irq(unsigned int irq)
> >> +void __attribute__ ((weak))
> >> +arch_teardown_msi_irq(unsigned int irq)
> >> {
> >> return;
> >> }
> >
> > If you're going to bother touching it, at least use __weak? And then put
> > it back on one line.
>
> There are 5 weak functions, and only this one was on one line.
> I know this arch_teardown_msi_irq and the neighbor arch_teardown_msi_irqs
> can be on one line each, but IMHO it looks good to me to place all of them
> in 2 lines each (since it make slightly emphasize "weak", I guess).
Yeah I guess, I like __weak better than the attribute mess though.
> >> @@ -192,16 +192,16 @@ void read_msi_msg(unsigned int irq, struct msi_msg *msg)
> >> }
> >> case PCI_CAP_ID_MSIX:
> >> {
> >> - void __iomem *base;
> >> - base = entry->mask_base +
> >> + void __iomem *ofs;
> >> + ofs = entry->mask_base +
> >> entry->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE;
> >>
> >> - msg->address_lo = readl(base + PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
> >> - msg->address_hi = readl(base + PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET);
> >> - msg->data = readl(base + PCI_MSIX_ENTRY_DATA_OFFSET);
> >> - break;
> >> - }
> >> - default:
> >> + msg->address_lo = readl(ofs + PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
> >> + msg->address_hi = readl(ofs + PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET);
> >> + msg->data = readl(ofs + PCI_MSIX_ENTRY_DATA_OFFSET);
> >> + break;
> >> + }
> >
> > Is that just s/base/ofs/? If so that's just pointless churn IMHO.
>
> There was violation of 80 column limit, even though it was 81.
> The name "base" is a bit confusing because it does not equal to mask_base,
> so I employ "ofs" as offset of the entry.
I guess base != mask_base is fair enough, except "ofs" doesn't mean
anything. If you want it be short for offset it needs to be "offs" I
think, which breaks 80 columns again :)
cheers
--
Michael Ellerman
OzLabs, IBM Australia Development Lab
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
Attachment:
signature.asc
Description: This is a digitally signed message part
