Patchwork [2/4] rtc: m41t80: clean up error paths

login
register
mail settings
Submitter Wolfram Sang
Date April 2, 2014, 12:15 p.m.
Message ID <1396440913-7114-3-git-send-email-wsa@the-dreams.de>
Download mbox | patch
Permalink /patch/336327/
State New
Headers show

Comments

Wolfram Sang - April 2, 2014, 12:15 p.m.
From: Wolfram Sang <wsa@sang-engineering.com>

There is no cleanup needed when something fails in probe, so no need for
goto. Directly return when something fails.

Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
Cc: Jingoo Han <jg1.han@samsung.com>
---

@Jingoo: Please take also an eye on simplifying error paths when applying devm_*
conversions. Here, the error path was in a confusing state after that.

 drivers/rtc/rtc-m41t80.c | 64 +++++++++++++++++-------------------------------
 1 file changed, 23 insertions(+), 41 deletions(-)
Alessandro Zummo - April 2, 2014, 3:01 p.m.
On Wed,  2 Apr 2014 14:15:11 +0200
Wolfram Sang <wsa@the-dreams.de> wrote:

> From: Wolfram Sang <wsa@sang-engineering.com>
> 
> There is no cleanup needed when something fails in probe, so no need for
> goto. Directly return when something fails.
> 
> Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
> Cc: Jingoo Han <jg1.han@samsung.com>

 Acked-by: Alessandro Zummo <a.zummo@towertech.it>
Jingoo Han - April 3, 2014, 1:06 a.m.
On Wednesday, April 02, 2014 9:15 PM, Wolfram Sang wrote:
> 
> From: Wolfram Sang <wsa@sang-engineering.com>
> 
> There is no cleanup needed when something fails in probe, so no need for
> goto. Directly return when something fails.
> 
> Signed-off-by: Wolfram Sang <wsa@sang-engineering.com>
> Cc: Jingoo Han <jg1.han@samsung.com>

It looks good; however, when applying this patch to linux-next 20140402,
it makes conflict.

> ---
> 
> @Jingoo: Please take also an eye on simplifying error paths when applying devm_*
> conversions. Here, the error path was in a confusing state after that.

Before I changed devm_* conversions, the error paths of this driver
were already very confusing.

Best regards,
Jingoo Han

> 
>  drivers/rtc/rtc-m41t80.c | 64 +++++++++++++++++-------------------------------
>  1 file changed, 23 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
> index c287c6d5d1a9..86eccb15f524 100644
> --- a/drivers/rtc/rtc-m41t80.c
> +++ b/drivers/rtc/rtc-m41t80.c
> @@ -627,37 +627,28 @@ static int m41t80_probe(struct i2c_client *client,
>  	struct m41t80_data *clientdata = NULL;
> 
>  	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C
> -				     | I2C_FUNC_SMBUS_BYTE_DATA)) {
> -		rc = -ENODEV;
> -		goto exit;
> -	}
> +				     | I2C_FUNC_SMBUS_BYTE_DATA))
> +		return -ENODEV;
> 
>  	clientdata = devm_kzalloc(&client->dev, sizeof(*clientdata),
>  				GFP_KERNEL);
> -	if (!clientdata) {
> -		rc = -ENOMEM;
> -		goto exit;
> -	}
> +	if (!clientdata)
> +		return -ENOMEM;
> 
>  	clientdata->features = id->driver_data;
>  	i2c_set_clientdata(client, clientdata);
> 
>  	rtc = devm_rtc_device_register(&client->dev, client->name,
>  					&m41t80_rtc_ops, THIS_MODULE);
> -	if (IS_ERR(rtc)) {
> -		rc = PTR_ERR(rtc);
> -		rtc = NULL;
> -		goto exit;
> -	}
> +	if (IS_ERR(rtc))
> +		return PTR_ERR(rtc);
> 
>  	clientdata->rtc = rtc;
> 
>  	/* Make sure HT (Halt Update) bit is cleared */
>  	rc = i2c_smbus_read_byte_data(client, M41T80_REG_ALARM_HOUR);
> -	if (rc < 0)
> -		goto ht_err;
> 
> -	if (rc & M41T80_ALHOUR_HT) {
> +	if (rc >= 0 && rc & M41T80_ALHOUR_HT) {
>  		if (clientdata->features & M41T80_FEATURE_HT) {
>  			m41t80_get_datetime(client, &tm);
>  			dev_info(&client->dev, "HT bit was set!\n");
> @@ -668,53 +659,44 @@ static int m41t80_probe(struct i2c_client *client,
>  				 tm.tm_mon + 1, tm.tm_mday, tm.tm_hour,
>  				 tm.tm_min, tm.tm_sec);
>  		}
> -		if (i2c_smbus_write_byte_data(client,
> -					      M41T80_REG_ALARM_HOUR,
> -					      rc & ~M41T80_ALHOUR_HT) < 0)
> -			goto ht_err;
> +		rc = i2c_smbus_write_byte_data(client, M41T80_REG_ALARM_HOUR,
> +					      rc & ~M41T80_ALHOUR_HT);
> +	}
> +
> +	if (rc < 0) {
> +		dev_err(&client->dev, "Can't clear HT bit\n");
> +		return -EIO;
>  	}
> 
>  	/* Make sure ST (stop) bit is cleared */
>  	rc = i2c_smbus_read_byte_data(client, M41T80_REG_SEC);
> -	if (rc < 0)
> -		goto st_err;
> 
> -	if (rc & M41T80_SEC_ST) {
> -		if (i2c_smbus_write_byte_data(client, M41T80_REG_SEC,
> -					      rc & ~M41T80_SEC_ST) < 0)
> -			goto st_err;
> +	if (rc >= 0 && rc & M41T80_SEC_ST)
> +		rc = i2c_smbus_write_byte_data(client, M41T80_REG_SEC,
> +					      rc & ~M41T80_SEC_ST);
> +	if (rc < 0) {
> +		dev_err(&client->dev, "Can't clear ST bit\n");
> +		return -EIO;
>  	}
> 
>  	rc = m41t80_sysfs_register(&client->dev);
>  	if (rc)
> -		goto exit;
> +		return rc;
> 
>  #ifdef CONFIG_RTC_DRV_M41T80_WDT
>  	if (clientdata->features & M41T80_FEATURE_HT) {
>  		save_client = client;
>  		rc = misc_register(&wdt_dev);
>  		if (rc)
> -			goto exit;
> +			return rc;
>  		rc = register_reboot_notifier(&wdt_notifier);
>  		if (rc) {
>  			misc_deregister(&wdt_dev);
> -			goto exit;
> +			return rc;
>  		}
>  	}
>  #endif
>  	return 0;
> -
> -st_err:
> -	rc = -EIO;
> -	dev_err(&client->dev, "Can't clear ST bit\n");
> -	goto exit;
> -ht_err:
> -	rc = -EIO;
> -	dev_err(&client->dev, "Can't clear HT bit\n");
> -	goto exit;
> -
> -exit:
> -	return rc;
>  }
> 
>  static int m41t80_remove(struct i2c_client *client)
> --
> 1.9.0

Patch

diff --git a/drivers/rtc/rtc-m41t80.c b/drivers/rtc/rtc-m41t80.c
index c287c6d5d1a9..86eccb15f524 100644
--- a/drivers/rtc/rtc-m41t80.c
+++ b/drivers/rtc/rtc-m41t80.c
@@ -627,37 +627,28 @@  static int m41t80_probe(struct i2c_client *client,
 	struct m41t80_data *clientdata = NULL;
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C
-				     | I2C_FUNC_SMBUS_BYTE_DATA)) {
-		rc = -ENODEV;
-		goto exit;
-	}
+				     | I2C_FUNC_SMBUS_BYTE_DATA))
+		return -ENODEV;
 
 	clientdata = devm_kzalloc(&client->dev, sizeof(*clientdata),
 				GFP_KERNEL);
-	if (!clientdata) {
-		rc = -ENOMEM;
-		goto exit;
-	}
+	if (!clientdata)
+		return -ENOMEM;
 
 	clientdata->features = id->driver_data;
 	i2c_set_clientdata(client, clientdata);
 
 	rtc = devm_rtc_device_register(&client->dev, client->name,
 					&m41t80_rtc_ops, THIS_MODULE);
-	if (IS_ERR(rtc)) {
-		rc = PTR_ERR(rtc);
-		rtc = NULL;
-		goto exit;
-	}
+	if (IS_ERR(rtc))
+		return PTR_ERR(rtc);
 
 	clientdata->rtc = rtc;
 
 	/* Make sure HT (Halt Update) bit is cleared */
 	rc = i2c_smbus_read_byte_data(client, M41T80_REG_ALARM_HOUR);
-	if (rc < 0)
-		goto ht_err;
 
-	if (rc & M41T80_ALHOUR_HT) {
+	if (rc >= 0 && rc & M41T80_ALHOUR_HT) {
 		if (clientdata->features & M41T80_FEATURE_HT) {
 			m41t80_get_datetime(client, &tm);
 			dev_info(&client->dev, "HT bit was set!\n");
@@ -668,53 +659,44 @@  static int m41t80_probe(struct i2c_client *client,
 				 tm.tm_mon + 1, tm.tm_mday, tm.tm_hour,
 				 tm.tm_min, tm.tm_sec);
 		}
-		if (i2c_smbus_write_byte_data(client,
-					      M41T80_REG_ALARM_HOUR,
-					      rc & ~M41T80_ALHOUR_HT) < 0)
-			goto ht_err;
+		rc = i2c_smbus_write_byte_data(client, M41T80_REG_ALARM_HOUR,
+					      rc & ~M41T80_ALHOUR_HT);
+	}
+
+	if (rc < 0) {
+		dev_err(&client->dev, "Can't clear HT bit\n");
+		return -EIO;
 	}
 
 	/* Make sure ST (stop) bit is cleared */
 	rc = i2c_smbus_read_byte_data(client, M41T80_REG_SEC);
-	if (rc < 0)
-		goto st_err;
 
-	if (rc & M41T80_SEC_ST) {
-		if (i2c_smbus_write_byte_data(client, M41T80_REG_SEC,
-					      rc & ~M41T80_SEC_ST) < 0)
-			goto st_err;
+	if (rc >= 0 && rc & M41T80_SEC_ST)
+		rc = i2c_smbus_write_byte_data(client, M41T80_REG_SEC,
+					      rc & ~M41T80_SEC_ST);
+	if (rc < 0) {
+		dev_err(&client->dev, "Can't clear ST bit\n");
+		return -EIO;
 	}
 
 	rc = m41t80_sysfs_register(&client->dev);
 	if (rc)
-		goto exit;
+		return rc;
 
 #ifdef CONFIG_RTC_DRV_M41T80_WDT
 	if (clientdata->features & M41T80_FEATURE_HT) {
 		save_client = client;
 		rc = misc_register(&wdt_dev);
 		if (rc)
-			goto exit;
+			return rc;
 		rc = register_reboot_notifier(&wdt_notifier);
 		if (rc) {
 			misc_deregister(&wdt_dev);
-			goto exit;
+			return rc;
 		}
 	}
 #endif
 	return 0;
-
-st_err:
-	rc = -EIO;
-	dev_err(&client->dev, "Can't clear ST bit\n");
-	goto exit;
-ht_err:
-	rc = -EIO;
-	dev_err(&client->dev, "Can't clear HT bit\n");
-	goto exit;
-
-exit:
-	return rc;
 }
 
 static int m41t80_remove(struct i2c_client *client)