| Message ID | 20240122030238.29437-3-raag.jadav@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | DesignWare PWM improvements | expand |
On Mon, Jan 22, 2024 at 08:32:37AM +0530, Raag Jadav wrote: > Simplify error handling in ->probe() function using dev_err_probe() helper. ... > ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci)); > - if (ret) { > - dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret)); > - return ret; > - } > + if (ret) > + return dev_err_probe(dev, ret, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret)); > > base = pcim_iomap_table(pci)[0]; > - if (!base) { > - dev_err(dev, "Base address missing\n"); > - return -ENOMEM; > - } > + if (!base) > + return dev_err_probe(dev, -ENOMEM, "Base address missing\n"); This check is bogus. Just remove it completely. The pcim_iomap_table() fails IFF pcim_iomap_regions() fails. You have checked the latter already.
Hello, On Sun, Jan 28, 2024 at 04:48:16PM +0200, Andy Shevchenko wrote: > On Mon, Jan 22, 2024 at 08:32:37AM +0530, Raag Jadav wrote: > > Simplify error handling in ->probe() function using dev_err_probe() helper. > > ... > > > ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci)); > > - if (ret) { > > - dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret)); > > - return ret; > > - } > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret)); > > > > base = pcim_iomap_table(pci)[0]; > > > - if (!base) { > > - dev_err(dev, "Base address missing\n"); > > - return -ENOMEM; > > - } > > + if (!base) > > + return dev_err_probe(dev, -ENOMEM, "Base address missing\n"); > > This check is bogus. Just remove it completely. This would be a separate patch though. IMHO mechanically converting to dev_err_probe() is fine. Best regards Uwe
On Sun, Jan 28, 2024 at 04:48:16PM +0200, Andy Shevchenko wrote: > On Mon, Jan 22, 2024 at 08:32:37AM +0530, Raag Jadav wrote: > > Simplify error handling in ->probe() function using dev_err_probe() helper. > > ... > > > ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci)); > > - if (ret) { > > - dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret)); > > - return ret; > > - } > > + if (ret) > > + return dev_err_probe(dev, ret, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret)); > > > > base = pcim_iomap_table(pci)[0]; > > > - if (!base) { > > - dev_err(dev, "Base address missing\n"); > > - return -ENOMEM; > > - } > > + if (!base) > > + return dev_err_probe(dev, -ENOMEM, "Base address missing\n"); > > This check is bogus. Just remove it completely. > > The pcim_iomap_table() fails IFF pcim_iomap_regions() fails. > You have checked the latter already. I'm no expert on devres but I found a few NULL returns in alloc_dr() call path. In the interest of learning more about iomap, wouldn't we need to handle them (just in some odd case)? Raag
On Sun, Jan 28, 2024 at 05:58:38PM +0100, Uwe Kleine-König wrote: > On Sun, Jan 28, 2024 at 04:48:16PM +0200, Andy Shevchenko wrote: > > On Mon, Jan 22, 2024 at 08:32:37AM +0530, Raag Jadav wrote: > > > Simplify error handling in ->probe() function using dev_err_probe() helper. ... > > > ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci)); > > > - if (ret) { > > > - dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret)); > > > - return ret; > > > - } > > > + if (ret) > > > + return dev_err_probe(dev, ret, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret)); > > > > > > base = pcim_iomap_table(pci)[0]; > > > > > - if (!base) { > > > - dev_err(dev, "Base address missing\n"); > > > - return -ENOMEM; > > > - } > > > + if (!base) > > > + return dev_err_probe(dev, -ENOMEM, "Base address missing\n"); > > > > This check is bogus. Just remove it completely. > > This would be a separate patch though. IMHO mechanically converting to > dev_err_probe() is fine. Sure, that's what I meant. First patch to remove, followed by dev_err_probe() conversion.
On Tue, Jan 30, 2024 at 10:31:02AM +0200, Raag Jadav wrote: > On Sun, Jan 28, 2024 at 04:48:16PM +0200, Andy Shevchenko wrote: > > On Mon, Jan 22, 2024 at 08:32:37AM +0530, Raag Jadav wrote: > > > Simplify error handling in ->probe() function using dev_err_probe() helper. ... > > > ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci)); > > > - if (ret) { > > > - dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret)); > > > - return ret; > > > - } > > > + if (ret) > > > + return dev_err_probe(dev, ret, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret)); > > > > > > base = pcim_iomap_table(pci)[0]; > > > > > - if (!base) { > > > - dev_err(dev, "Base address missing\n"); > > > - return -ENOMEM; > > > - } > > > + if (!base) > > > + return dev_err_probe(dev, -ENOMEM, "Base address missing\n"); > > > > This check is bogus. Just remove it completely. > > > > The pcim_iomap_table() fails IFF pcim_iomap_regions() fails. > > You have checked the latter already. > > I'm no expert on devres but I found a few NULL returns in alloc_dr() > call path. In the interest of learning more about iomap, wouldn't we > need to handle them (just in some odd case)? It has nothing to do with devres. The logic is implemented in PCI core.
diff --git a/drivers/pwm/pwm-dwc.c b/drivers/pwm/pwm-dwc.c index fd64313cb38d..8f8035b047c1 100644 --- a/drivers/pwm/pwm-dwc.c +++ b/drivers/pwm/pwm-dwc.c @@ -68,24 +68,18 @@ static int dwc_pwm_probe(struct pci_dev *pci, const struct pci_device_id *id) int ret; ret = pcim_enable_device(pci); - if (ret) { - dev_err(dev, "Failed to enable device (%pe)\n", ERR_PTR(ret)); - return ret; - } + if (ret) + return dev_err_probe(dev, ret, "Failed to enable device (%pe)\n", ERR_PTR(ret)); pci_set_master(pci); ret = pcim_iomap_regions(pci, BIT(0), pci_name(pci)); - if (ret) { - dev_err(dev, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret)); - return ret; - } + if (ret) + return dev_err_probe(dev, ret, "Failed to iomap PCI BAR (%pe)\n", ERR_PTR(ret)); base = pcim_iomap_table(pci)[0]; - if (!base) { - dev_err(dev, "Base address missing\n"); - return -ENOMEM; - } + if (!base) + return dev_err_probe(dev, -ENOMEM, "Base address missing\n"); info = (const struct dwc_pwm_info *)id->driver_data; ret = dwc_pwm_init(dev, info, base);
Simplify error handling in ->probe() function using dev_err_probe() helper. Signed-off-by: Raag Jadav <raag.jadav@intel.com> --- drivers/pwm/pwm-dwc.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-)