| Message ID | 1411614872-4009-2-git-send-email-wangyijing@huawei.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, Sep 25, 2014 at 11:14:11AM +0800, Yijing Wang wrote: > Msi_chip functions setup_irq/teardown_irq rarely use msi_chip > argument. That's not true. Out of the four drivers that you modify two use the parameter. And the two that don't probably should be using it too. 50% is not "rarely". =) > We can look up msi_chip pointer by the device pointer > or irq number, so clean up msi_chip argument. I don't like this particular change. The idea was to keep the API object oriented so that drivers wouldn't have to know where to get the MSI chip from. It also makes it more resilient against code reorganizations since the core code is the only place that needs to know where to get the chip from. Thierry
On Thu, 25 Sep 2014, Thierry Reding wrote: > On Thu, Sep 25, 2014 at 11:14:11AM +0800, Yijing Wang wrote: > > Msi_chip functions setup_irq/teardown_irq rarely use msi_chip > > argument. > > That's not true. Out of the four drivers that you modify two use the > parameter. And the two that don't probably should be using it too. > > 50% is not "rarely". =) > > > We can look up msi_chip pointer by the device pointer > > or irq number, so clean up msi_chip argument. > > I don't like this particular change. The idea was to keep the API object > oriented so that drivers wouldn't have to know where to get the MSI chip > from. It also makes it more resilient against code reorganizations since > the core code is the only place that needs to know where to get the chip > from. Right. We have the same thing in the irq_chip callbacks. All of them take "struct irq_data", because it's already available in the core code and it gives easy access to all information (chip, chipdata ...) which is necessary for the callback implementations. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2014/9/25 15:15, Thierry Reding wrote: > On Thu, Sep 25, 2014 at 11:14:11AM +0800, Yijing Wang wrote: >> Msi_chip functions setup_irq/teardown_irq rarely use msi_chip >> argument. > > That's not true. Out of the four drivers that you modify two use the > parameter. And the two that don't probably should be using it too. > > 50% is not "rarely". =) > >> We can look up msi_chip pointer by the device pointer >> or irq number, so clean up msi_chip argument. > > I don't like this particular change. The idea was to keep the API object > oriented so that drivers wouldn't have to know where to get the MSI chip > from. It also makes it more resilient against code reorganizations since > the core code is the only place that needs to know where to get the chip > from. Hm, ok, Thomas also don't like this change, I will drop this one, thanks. Thanks! Yijing. > > Thierry >
On 2014/9/25 18:20, Thomas Gleixner wrote: > On Thu, 25 Sep 2014, Thierry Reding wrote: > >> On Thu, Sep 25, 2014 at 11:14:11AM +0800, Yijing Wang wrote: >>> Msi_chip functions setup_irq/teardown_irq rarely use msi_chip >>> argument. >> >> That's not true. Out of the four drivers that you modify two use the >> parameter. And the two that don't probably should be using it too. >> >> 50% is not "rarely". =) >> >>> We can look up msi_chip pointer by the device pointer >>> or irq number, so clean up msi_chip argument. >> >> I don't like this particular change. The idea was to keep the API object >> oriented so that drivers wouldn't have to know where to get the MSI chip >> from. It also makes it more resilient against code reorganizations since >> the core code is the only place that needs to know where to get the chip >> from. > > Right. We have the same thing in the irq_chip callbacks. All of them > take "struct irq_data", because it's already available in the core > code and it gives easy access to all information (chip, chipdata ...) > which is necessary for the callback implementations. OK, I will drop this change, tglx, thanks for your review and comments! Thanks! Yijing. > > Thanks, > > tglx > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > . >
diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c index df60eab..3909d06 100644 --- a/drivers/irqchip/irq-armada-370-xp.c +++ b/drivers/irqchip/irq-armada-370-xp.c @@ -129,9 +129,8 @@ static void armada_370_xp_free_msi(int hwirq) mutex_unlock(&msi_used_lock); } -static int armada_370_xp_setup_msi_irq(struct msi_chip *chip, - struct pci_dev *pdev, - struct msi_desc *desc) +static int armada_370_xp_setup_msi_irq(struct pci_dev *pdev, + struct msi_desc *desc) { struct msi_msg msg; int virq, hwirq; @@ -160,8 +159,7 @@ static int armada_370_xp_setup_msi_irq(struct msi_chip *chip, return 0; } -static void armada_370_xp_teardown_msi_irq(struct msi_chip *chip, - unsigned int irq) +static void armada_370_xp_teardown_msi_irq(unsigned int irq) { struct irq_data *d = irq_get_irq_data(irq); unsigned long hwirq = d->hwirq; diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c index 0fb0fdb..edd4040 100644 --- a/drivers/pci/host/pci-tegra.c +++ b/drivers/pci/host/pci-tegra.c @@ -1157,9 +1157,10 @@ static irqreturn_t tegra_pcie_msi_irq(int irq, void *data) return processed > 0 ? IRQ_HANDLED : IRQ_NONE; } -static int tegra_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev, +static int tegra_msi_setup_irq(struct pci_dev *pdev, struct msi_desc *desc) { + struct msi_chip *chip = pdev->bus->msi; struct tegra_msi *msi = to_tegra_msi(chip); struct msi_msg msg; unsigned int irq; @@ -1185,10 +1186,11 @@ static int tegra_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev, return 0; } -static void tegra_msi_teardown_irq(struct msi_chip *chip, unsigned int irq) +static void tegra_msi_teardown_irq(unsigned int irq) { - struct tegra_msi *msi = to_tegra_msi(chip); struct irq_data *d = irq_get_irq_data(irq); + struct msi_chip *chip = irq_get_chip_data(irq); + struct tegra_msi *msi = to_tegra_msi(chip); tegra_msi_free(msi, d->hwirq); } diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index fa2fa45..517f1e1 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -342,7 +342,7 @@ static void clear_irq(unsigned int irq) msi->msi_attrib.multiple = 0; } -static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev, +static int dw_msi_setup_irq(struct pci_dev *pdev, struct msi_desc *desc) { int irq, pos, msgvec; @@ -383,7 +383,7 @@ static int dw_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev, return 0; } -static void dw_msi_teardown_irq(struct msi_chip *chip, unsigned int irq) +static void dw_msi_teardown_irq(unsigned int irq) { clear_irq(irq); } diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c index 4884ee5..647bc9f 100644 --- a/drivers/pci/host/pcie-rcar.c +++ b/drivers/pci/host/pcie-rcar.c @@ -615,9 +615,10 @@ static irqreturn_t rcar_pcie_msi_irq(int irq, void *data) return IRQ_HANDLED; } -static int rcar_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev, +static int rcar_msi_setup_irq(struct pci_dev *pdev, struct msi_desc *desc) { + struct msi_chip *chip = pdev->bus->msi; struct rcar_msi *msi = to_rcar_msi(chip); struct rcar_pcie *pcie = container_of(chip, struct rcar_pcie, msi.chip); struct msi_msg msg; @@ -645,10 +646,11 @@ static int rcar_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev, return 0; } -static void rcar_msi_teardown_irq(struct msi_chip *chip, unsigned int irq) +static void rcar_msi_teardown_irq(unsigned int irq) { - struct rcar_msi *msi = to_rcar_msi(chip); struct irq_data *d = irq_get_irq_data(irq); + struct msi_chip *chip = irq_get_chip_data(irq); + struct rcar_msi *msi = to_rcar_msi(chip); rcar_msi_free(msi, d->hwirq); } diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index aae2fc8..51d7e62 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -37,7 +37,7 @@ int __weak arch_setup_msi_irq(struct pci_dev *dev, struct msi_desc *desc) if (!chip || !chip->setup_irq) return -EINVAL; - err = chip->setup_irq(chip, dev, desc); + err = chip->setup_irq(dev, desc); if (err < 0) return err; @@ -53,7 +53,7 @@ void __weak arch_teardown_msi_irq(unsigned int irq) if (!chip || !chip->teardown_irq) return; - chip->teardown_irq(chip, irq); + chip->teardown_irq(irq); } int __weak arch_setup_msi_irqs(struct pci_dev *dev, int nvec, int type) diff --git a/include/linux/msi.h b/include/linux/msi.h index 36c63cf..45ef8cb 100644 --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -68,9 +68,8 @@ struct msi_chip { struct device_node *of_node; struct list_head list; - int (*setup_irq)(struct msi_chip *chip, struct pci_dev *dev, - struct msi_desc *desc); - void (*teardown_irq)(struct msi_chip *chip, unsigned int irq); + int (*setup_irq)(struct pci_dev *dev, struct msi_desc *desc); + void (*teardown_irq)(unsigned int irq); }; #endif /* LINUX_MSI_H */
Msi_chip functions setup_irq/teardown_irq rarely use msi_chip argument. We can look up msi_chip pointer by the device pointer or irq number, so clean up msi_chip argument. Signed-off-by: Yijing Wang <wangyijing@huawei.com> CC: Thierry Reding <thierry.reding@gmail.com> CC: Thomas Petazzoni <thomas.petazzoni@free-electrons.com> --- drivers/irqchip/irq-armada-370-xp.c | 8 +++----- drivers/pci/host/pci-tegra.c | 8 +++++--- drivers/pci/host/pcie-designware.c | 4 ++-- drivers/pci/host/pcie-rcar.c | 8 +++++--- drivers/pci/msi.c | 4 ++-- include/linux/msi.h | 5 ++--- 6 files changed, 19 insertions(+), 18 deletions(-)