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).
>> @@ -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.
>> @@ -230,15 +230,13 @@ void write_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;
>>
>> - writel(msg->address_lo,
>> - base + PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
>> - writel(msg->address_hi,
>> - base + PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET);
>> - writel(msg->data, base + PCI_MSIX_ENTRY_DATA_OFFSET);
>> + writel(msg->address_lo, ofs + PCI_MSIX_ENTRY_LOWER_ADDR_OFFSET);
>> + writel(msg->address_hi, ofs + PCI_MSIX_ENTRY_UPPER_ADDR_OFFSET);
>> + writel(msg->data, ofs + PCI_MSIX_ENTRY_DATA_OFFSET);
>> break;
>
> Ditto.
>
> cheers
Same to above.
Thanks,
H.Seto
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
