| Message ID | 2db677d2-ad4e-1f79-344b-dfa38ce948c9@users.sourceforge.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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) >
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
>> 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
> 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
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
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,
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)