| Message ID | 20210214064529.481341-1-zhengdejin5@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [i2c-next] i2c: designware: Consolidate pci_free_irq_vectors to a single place | expand |
On 2/14/21 8:45 AM, Dejin Zheng wrote: > Consolidate pci_free_irq_vectors to a single place using "goto free_irq" > for simplify the code. > > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> > --- > drivers/i2c/busses/i2c-designware-pcidrv.c | 22 ++++++++++------------ > 1 file changed, 10 insertions(+), 12 deletions(-) > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
On Mon, Feb 15, 2021 at 11:36:27AM +0200, Jarkko Nikula wrote: > On 2/14/21 8:45 AM, Dejin Zheng wrote: > > Consolidate pci_free_irq_vectors to a single place using "goto free_irq" > > for simplify the code. > > > > Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> > > --- > > drivers/i2c/busses/i2c-designware-pcidrv.c | 22 ++++++++++------------ > > 1 file changed, 10 insertions(+), 12 deletions(-) > > > Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> Jarkko, Thanks! Dejin
On Sun, Feb 14, 2021 at 02:45:29PM +0800, Dejin Zheng wrote: > Consolidate pci_free_irq_vectors to a single place using "goto free_irq" > for simplify the code. One nit below, after addressing: Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> ... > +free_irq: It's quite confusing name. I would expect to see here (with current proposal) *free_irq() call, but no, I don't see a such. Please, change to something like err_pci_free_irq_vectors: or alternatively err_free_irq_vectors: however I think first one is disambiguous. > + pci_free_irq_vectors(pdev); > + return r;
On Mon, Feb 15, 2021 at 03:20:40PM +0200, Andy Shevchenko wrote: > On Sun, Feb 14, 2021 at 02:45:29PM +0800, Dejin Zheng wrote: > > Consolidate pci_free_irq_vectors to a single place using "goto free_irq" > > for simplify the code. FYI, you may rather to introduce a pcim_alloc_irq_vectors() and drop all these calls altogether. Note to everybody: PCIm is not ideal, but this piece is being called whenever pcim_enable_device() makes resources manageable. The problem here is the naming.
diff --git a/drivers/i2c/busses/i2c-designware-pcidrv.c b/drivers/i2c/busses/i2c-designware-pcidrv.c index 55c83a7a24f3..f0c82e91870a 100644 --- a/drivers/i2c/busses/i2c-designware-pcidrv.c +++ b/drivers/i2c/busses/i2c-designware-pcidrv.c @@ -234,10 +234,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, if (controller->setup) { r = controller->setup(pdev, controller); - if (r) { - pci_free_irq_vectors(pdev); - return r; - } + if (r) + goto free_irq; } i2c_dw_adjust_bus_speed(dev); @@ -246,10 +244,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, i2c_dw_acpi_configure(&pdev->dev); r = i2c_dw_validate_speed(dev); - if (r) { - pci_free_irq_vectors(pdev); - return r; - } + if (r) + goto free_irq; i2c_dw_configure(dev); @@ -269,10 +265,8 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, adap->nr = controller->bus_num; r = i2c_dw_probe(dev); - if (r) { - pci_free_irq_vectors(pdev); - return r; - } + if (r) + goto free_irq; pm_runtime_set_autosuspend_delay(&pdev->dev, 1000); pm_runtime_use_autosuspend(&pdev->dev); @@ -280,6 +274,10 @@ static int i2c_dw_pci_probe(struct pci_dev *pdev, pm_runtime_allow(&pdev->dev); return 0; + +free_irq: + pci_free_irq_vectors(pdev); + return r; } static void i2c_dw_pci_remove(struct pci_dev *pdev)
Consolidate pci_free_irq_vectors to a single place using "goto free_irq" for simplify the code. Signed-off-by: Dejin Zheng <zhengdejin5@gmail.com> --- drivers/i2c/busses/i2c-designware-pcidrv.c | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-)