Re: [PATCH 5/7] mfd: pcf50633: Cleanup pcf50633_probe error handling

[Samuel Ortiz <sameo@linux.intel.com>]

On Tue, Oct 20, 2009 at 03:09:52AM +0400, Paul Fertser wrote:
> Hi Samuel,
> 
> Big thanks for your comments, next time i send anything upstream i
> will certainly provide a minimal changelog for every patch :)
> 
> On Mon, Oct 19, 2009 at 05:08:13PM +0200, Samuel Ortiz wrote:
> > On Wed, Oct 14, 2009 at 02:12:34AM +0400, Paul Fertser wrote:
> > >  	if (enable_irq_wake(client->irq) < 0)
> > > -		dev_err(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
> > > +		dev_info(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
> > >  			"in this hardware revision", client->irq);
> > 2 things here: This doesnt really belong to this patch, and also I'd prefer to
> > keep that as an error message.
> [...]
> > >  	ret = sysfs_create_group(&client->dev.kobj, &pcf_attr_group);
> > >  	if (ret)
> > > -		dev_err(pcf->dev, "error creating sysfs entries\n");
> > > +		dev_info(pcf->dev, "Failed to create sysfs entries\n");
> > Same here.
> 
> Sure. Please find amended version in attachement.
Patch applied, thanks a lot.

Cheers,
Samuel.
 
> -- 
> Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
> mailto:fercerpav@gmail.com

> From 7f982d01515371fcbab0086bf77448f351b09a42 Mon Sep 17 00:00:00 2001
> From: Lars-Peter Clausen <lars@metafoo.de>
> Date: Thu, 8 Oct 2009 00:24:54 +0200
> Subject: [PATCH] mfd: pcf50633: Cleanup pcf50633_probe error handling
> 
> Currently the child devices were not freed if the irq could not be requested.
> This patch restructures the function, that in case of an error all previously
> allocated resources are freed.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Paul Fertser <fercerpav@gmail.com>
> ---
>  drivers/mfd/pcf50633-core.c |   43 ++++++++++++++++++++++++++-----------------
>  1 files changed, 26 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mfd/pcf50633-core.c b/drivers/mfd/pcf50633-core.c
> index 69cdbdc..5aed527 100644
> --- a/drivers/mfd/pcf50633-core.c
> +++ b/drivers/mfd/pcf50633-core.c
> @@ -553,9 +553,14 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  {
>  	struct pcf50633 *pcf;
>  	struct pcf50633_platform_data *pdata = client->dev.platform_data;
> -	int i, ret = 0;
> +	int i, ret;
>  	int version, variant;
>  
> +	if (!client->irq) {
> +		dev_err(&client->dev, "Missing IRQ\n");
> +		return -ENOENT;
> +	}
> +
>  	pcf = kzalloc(sizeof(*pcf), GFP_KERNEL);
>  	if (!pcf)
>  		return -ENOMEM;
> @@ -570,6 +575,12 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  	pcf->irq = client->irq;
>  	pcf->work_queue = create_singlethread_workqueue("pcf50633");
>  
> +	if (!pcf->work_queue) {
> +		dev_err(&client->dev, "Failed to alloc workqueue\n");
> +		ret = -ENOMEM;
> +		goto err_free;
> +	}
> +
>  	INIT_WORK(&pcf->irq_work, pcf50633_irq_worker);
>  
>  	version = pcf50633_reg_read(pcf, 0);
> @@ -577,7 +588,7 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  	if (version < 0 || variant < 0) {
>  		dev_err(pcf->dev, "Unable to probe pcf50633\n");
>  		ret = -ENODEV;
> -		goto err;
> +		goto err_destroy_workqueue;
>  	}
>  
>  	dev_info(pcf->dev, "Probed device version %d variant %d\n",
> @@ -591,6 +602,14 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  	pcf50633_reg_write(pcf, PCF50633_REG_INT4M, 0x00);
>  	pcf50633_reg_write(pcf, PCF50633_REG_INT5M, 0x00);
>  
> +	ret = request_irq(client->irq, pcf50633_irq,
> +					IRQF_TRIGGER_LOW, "pcf50633", pcf);
> +
> +	if (ret) {
> +		dev_err(pcf->dev, "Failed to request IRQ %d\n", ret);
> +		goto err_destroy_workqueue;
> +	}
> +
>  	/* Create sub devices */
>  	pcf50633_client_dev_register(pcf, "pcf50633-input",
>  						&pcf->input_pdev);
> @@ -606,7 +625,7 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  
>  		pdev = platform_device_alloc("pcf50633-regltr", i);
>  		if (!pdev) {
> -			dev_err(pcf->dev, "Cannot create regulator\n");
> +			dev_err(pcf->dev, "Cannot create regulator %d\n", i);
>  			continue;
>  		}
>  
> @@ -618,19 +637,6 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  		platform_device_add(pdev);
>  	}
>  
> -	if (client->irq) {
> -		ret = request_irq(client->irq, pcf50633_irq,
> -				IRQF_TRIGGER_LOW, "pcf50633", pcf);
> -
> -		if (ret) {
> -			dev_err(pcf->dev, "Failed to request IRQ %d\n", ret);
> -			goto err;
> -		}
> -	} else {
> -		dev_err(pcf->dev, "No IRQ configured\n");
> -		goto err;
> -	}
> -
>  	if (enable_irq_wake(client->irq) < 0)
>  		dev_err(pcf->dev, "IRQ %u cannot be enabled as wake-up source"
>  			"in this hardware revision", client->irq);
> @@ -644,9 +650,12 @@ static int __devinit pcf50633_probe(struct i2c_client *client,
>  
>  	return 0;
>  
> -err:
> +err_destroy_workqueue:
>  	destroy_workqueue(pcf->work_queue);
> +err_free:
> +	i2c_set_clientdata(client, NULL);
>  	kfree(pcf);
> +
>  	return ret;
>  }
>  
> -- 
> 1.6.0.6
> 


-- 
Intel Open Source Technology Centre
http://oss.intel.com/