[PATCH] video: atmel_lcdfb: Use common error handling code in atmel_lcdfb_of_init()

[PATCH] video: atmel_lcdfb: Use common error handling code in atmel_lcdfb_of_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 5 Nov 2017 14:00:52 +0100

Add a jump target so that a specific error message is stored only once
at the end of this function implementation.
Replace two calls of the function "dev_err" by goto statements.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/video/fbdev/atmel_lcdfb.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
index e06358da4b99..3672c2e52ebd 100644
--- a/drivers/video/fbdev/atmel_lcdfb.c
+++ b/drivers/video/fbdev/atmel_lcdfb.c
@@ -1047,10 +1047,8 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
 	}
 
 	ret = of_property_read_u32(display_np, "bits-per-pixel", &var->bits_per_pixel);
-	if (ret < 0) {
-		dev_err(dev, "failed to get property bits-per-pixel\n");
-		goto put_display_node;
-	}
+	if (ret < 0)
+		goto report_bits_failure;
 
 	ret = of_property_read_u32(display_np, "atmel,guard-time", &pdata->guard_time);
 	if (ret < 0) {
@@ -1065,10 +1063,8 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
 	}
 
 	ret = of_property_read_u32(display_np, "atmel,dmacon", &pdata->default_dmacon);
-	if (ret < 0) {
-		dev_err(dev, "failed to get property bits-per-pixel\n");
-		goto put_display_node;
-	}
+	if (ret < 0)
+		goto report_bits_failure;
 
 	INIT_LIST_HEAD(&pdata->pwr_gpios);
 	ret = -ENOMEM;
@@ -1147,6 +1143,10 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
 put_display_node:
 	of_node_put(display_np);
 	return ret;
+
+report_bits_failure:
+	dev_err(dev, "failed to get property bits-per-pixel\n");
+	goto put_display_node;
 }
 #else
 static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
-- 
2.15.0

Re: [PATCH] video: atmel_lcdfb: Use common error handling code in atmel_lcdfb_of_init()

[Nicolas Ferre]

On 05/11/2017 at 14:10, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 5 Nov 2017 14:00:52 +0100
> 
> Add a jump target so that a specific error message is stored only once
> at the end of this function implementation.
> Replace two calls of the function "dev_err" by goto statements.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Sorry but NACK: the message was malformed and resulted in the
duplication of the error log that you spotted.

The proper way to fix this is to modify the second occurrence of this
message.
If you want to lower the size of strings in this driver, you can do it,
but not like this.

Regards,
  Nicolas


> ---
>  drivers/video/fbdev/atmel_lcdfb.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
> index e06358da4b99..3672c2e52ebd 100644
> --- a/drivers/video/fbdev/atmel_lcdfb.c
> +++ b/drivers/video/fbdev/atmel_lcdfb.c
> @@ -1047,10 +1047,8 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
>  	}
>  
>  	ret = of_property_read_u32(display_np, "bits-per-pixel", &var->bits_per_pixel);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to get property bits-per-pixel\n");
> -		goto put_display_node;
> -	}
> +	if (ret < 0)
> +		goto report_bits_failure;
>  
>  	ret = of_property_read_u32(display_np, "atmel,guard-time", &pdata->guard_time);
>  	if (ret < 0) {
> @@ -1065,10 +1063,8 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
>  	}
>  
>  	ret = of_property_read_u32(display_np, "atmel,dmacon", &pdata->default_dmacon);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to get property bits-per-pixel\n");
> -		goto put_display_node;
> -	}
> +	if (ret < 0)
> +		goto report_bits_failure;
>  
>  	INIT_LIST_HEAD(&pdata->pwr_gpios);
>  	ret = -ENOMEM;
> @@ -1147,6 +1143,10 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
>  put_display_node:
>  	of_node_put(display_np);
>  	return ret;
> +
> +report_bits_failure:
> +	dev_err(dev, "failed to get property bits-per-pixel\n");
> +	goto put_display_node;
>  }
>  #else
>  static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
> 


-- 
Nicolas Ferre

Re: [PATCH] video: atmel_lcdfb: Use common error handling code in atmel_lcdfb_of_init()

[Dan Carpenter]

On Mon, Nov 06, 2017 at 09:40:19AM +0100, Nicolas Ferre wrote:
> If you want to lower the size of strings in this driver, you can do it,
> but not like this.

Just so we're clear, GCC already detects and combines it when you use
the same string constant twice.

regards,
dan carpenter

Re: video: atmel_lcdfb: Use common error handling code in atmel_lcdfb_of_init()

[SF Markus Elfring]

>> If you want to lower the size of strings in this driver, you can do it,
>> but not like this.
> 
> Just so we're clear, GCC already detects and combines it when you use
> the same string constant twice.

Do you distinguish between merging of constants and the combination
of statements for such an use case?

Regards,
Markus

Re: video: atmel_lcdfb: Use common error handling code in atmel_lcdfb_of_init()

[Dan Carpenter]

On Mon, Nov 06, 2017 at 10:00:25AM +0100, SF Markus Elfring wrote:
> >> If you want to lower the size of strings in this driver, you can do it,
> >> but not like this.
> > 
> > Just so we're clear, GCC already detects and combines it when you use
> > the same string constant twice.
> 
> Do you distinguish between merging of constants and the combination
> of statements for such an use case?

I would have rejected the patch even if GCC didn't combine the strings
because the most important thing is that the code is readable.

regards,
dan carpenter

Re: video: atmel_lcdfb: Use common error handling code in atmel_lcdfb_of_init()

[SF Markus Elfring]

> Sorry but NACK: the message was malformed and resulted in the
> duplication of the error log that you spotted.
> 
> The proper way to fix this is to modify the second occurrence of this message.

* Would you like to achieve that a corresponding message will mention
  anything around a property “atmel,dmacon” (instead of “bits-per-pixel”)?

* Can it make sense to adjust the used message format then?

Regards,
Markus

Re: video: atmel_lcdfb: Use common error handling code in atmel_lcdfb_of_init()

[Nicolas Ferre]

On 06/11/2017 at 10:32, SF Markus Elfring wrote:
>> Sorry but NACK: the message was malformed and resulted in the
>> duplication of the error log that you spotted.
>>
>> The proper way to fix this is to modify the second occurrence of this message.
> 
> * Would you like to achieve that a corresponding message will mention
>   anything around a property “atmel,dmacon” (instead of “bits-per-pixel”)?

Yes: that would actually fix the wrong log message.

> * Can it make sense to adjust the used message format then?

In another patch, why not... Beware about the added complexity though.

Regards,
-- 
Nicolas Ferre

[PATCH v2] video: atmel_lcdfb: Use unique error messages in atmel_lcdfb_of_init()

[SF Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 6 Nov 2017 19:00:58 +0100

A duplicate error message was used so far in this function implementation.
Thus use a consistent message format instead together with property names
where constant merging can be applied by the compiler in four cases.

This issue was detected by using the Coccinelle software.


This software update supersedes the suggestion "video: atmel_lcdfb:
Use common error handling code in atmel_lcdfb_of_init()" from 2017-11-05.
https://patchwork.kernel.org/patch/10042131/

Fixes: b985172b328abc6de495a87ab998b267c5a09c16 ("video: atmel_lcdfb: add device tree suport")

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/video/fbdev/atmel_lcdfb.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
index e06358da4b99..44146aec3596 100644
--- a/drivers/video/fbdev/atmel_lcdfb.c
+++ b/drivers/video/fbdev/atmel_lcdfb.c
@@ -1021,6 +1021,8 @@ static void atmel_lcdfb_power_control_gpio(struct atmel_lcdfb_pdata *pdata, int
 		gpio_set_value(og->gpio, on);
 }
 
+static char const property_failure[] = "failed to get property %s\n";
+
 static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
 {
 	struct fb_info *info = sinfo->info;
@@ -1048,25 +1050,25 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
 
 	ret = of_property_read_u32(display_np, "bits-per-pixel", &var->bits_per_pixel);
 	if (ret < 0) {
-		dev_err(dev, "failed to get property bits-per-pixel\n");
+		dev_err(dev, property_failure, "bits-per-pixel");
 		goto put_display_node;
 	}
 
 	ret = of_property_read_u32(display_np, "atmel,guard-time", &pdata->guard_time);
 	if (ret < 0) {
-		dev_err(dev, "failed to get property atmel,guard-time\n");
+		dev_err(dev, property_failure, "atmel,guard-time");
 		goto put_display_node;
 	}
 
 	ret = of_property_read_u32(display_np, "atmel,lcdcon2", &pdata->default_lcdcon2);
 	if (ret < 0) {
-		dev_err(dev, "failed to get property atmel,lcdcon2\n");
+		dev_err(dev, property_failure, "atmel,lcdcon2");
 		goto put_display_node;
 	}
 
 	ret = of_property_read_u32(display_np, "atmel,dmacon", &pdata->default_dmacon);
 	if (ret < 0) {
-		dev_err(dev, "failed to get property bits-per-pixel\n");
+		dev_err(dev, property_failure, "atmel,dmacon");
 		goto put_display_node;
 	}
 
-- 
2.15.0

Re: [PATCH v2] video: atmel_lcdfb: Use unique error messages in atmel_lcdfb_of_init()

[Joe Perches]

On Mon, 2017-11-06 at 19:14 +0100, SF Markus Elfring wrote:
> A duplicate error message was used so far in this function implementation.
> Thus use a consistent message format instead together with property names
> where constant merging can be applied by the compiler in four cases.
[]
> diff --git a/drivers/video/fbdev/atmel_lcdfb.c b/drivers/video/fbdev/atmel_lcdfb.c
[]
> @@ -1021,6 +1021,8 @@ static void atmel_lcdfb_power_control_gpio(struct atmel_lcdfb_pdata *pdata, int
>  		gpio_set_value(og->gpio, on);
>  }
>  
> +static char const property_failure[] = "failed to get property %s\n";

Yuck.  This makes it harder to grep the sources.
Just use the normal format string in each place.

> +
>  static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
>  {
>  	struct fb_info *info = sinfo->info;
> @@ -1048,25 +1050,25 @@ static int atmel_lcdfb_of_init(struct atmel_lcdfb_info *sinfo)
>  
>  	ret = of_property_read_u32(display_np, "bits-per-pixel", &var->bits_per_pixel);
>  	if (ret < 0) {
> -		dev_err(dev, "failed to get property bits-per-pixel\n");
> +		dev_err(dev, property_failure, "bits-per-pixel");

This likely doesn't even save any code size.