[v1] DesignWare PWM improvements

[PATCH v1 2/3] pwm: dwc: simplify error handling

Posted by Raag Jadav 1 year, 11 months ago

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(-)

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);
-- 
2.35.3

Re: [PATCH v1 2/3] pwm: dwc: simplify error handling

Posted by Andy Shevchenko 1 year, 11 months ago

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] pwm: dwc: simplify error handling

Posted by Raag Jadav 1 year, 10 months ago

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

Re: [PATCH v1 2/3] pwm: dwc: simplify error handling

Posted by Andy Shevchenko 1 year, 10 months ago

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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/3] pwm: dwc: simplify error handling

Posted by Uwe Kleine-König 1 year, 11 months ago

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH v1 2/3] pwm: dwc: simplify error handling

Posted by Andy Shevchenko 1 year, 10 months ago

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.

-- 
With Best Regards,
Andy Shevchenko