* [PATCH 0/2] extcon-max14577: Fine-tuning for max14577_muic_set_path()
@ 2017-10-22 17:50 SF Markus Elfring
2017-10-22 17:51 ` [PATCH 1/2] extcon: max14577: Use common error handling code in max14577_muic_set_path() SF Markus Elfring
0 siblings, 1 reply; 10+ messages in thread
From: SF Markus Elfring @ 2017-10-22 17:50 UTC (permalink / raw)
To: kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 22 Oct 2017 19:45:54 +0200
Two update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Use common error handling code
Delete an unnecessary variable initialisation
drivers/extcon/extcon-max14577.c | 24 +++++++++++-------------
1 file changed, 11 insertions(+), 13 deletions(-)
--
2.14.2
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] extcon: max14577: Use common error handling code in max14577_muic_set_path()
@ 2017-10-22 17:51 ` SF Markus Elfring
2017-10-22 18:59 ` Krzysztof Kozlowski
2017-10-23 0:57 ` Chanwoo Choi
0 siblings, 2 replies; 10+ messages in thread
From: SF Markus Elfring @ 2017-10-22 17:51 UTC (permalink / raw)
To: kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 22 Oct 2017 19:33:58 +0200
Add a jump target so that a bit of exception handling can be better reused
at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
drivers/extcon/extcon-max14577.c | 22 ++++++++++------------
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/drivers/extcon/extcon-max14577.c b/drivers/extcon/extcon-max14577.c
index f6414b7fa5bc..3d4bf5d23236 100644
--- a/drivers/extcon/extcon-max14577.c
+++ b/drivers/extcon/extcon-max14577.c
@@ -211,10 +211,8 @@ static int max14577_muic_set_path(struct max14577_muic_info *info,
ret = max14577_update_reg(info->max14577->regmap,
MAX14577_MUIC_REG_CONTROL1,
CLEAR_IDBEN_MICEN_MASK, CTRL1_SW_OPEN);
- if (ret < 0) {
- dev_err(info->dev, "failed to update MUIC register\n");
- return ret;
- }
+ if (ret < 0)
+ goto report_failure;
if (attached)
ctrl1 = val;
@@ -224,10 +222,8 @@ static int max14577_muic_set_path(struct max14577_muic_info *info,
ret = max14577_update_reg(info->max14577->regmap,
MAX14577_MUIC_REG_CONTROL1,
CLEAR_IDBEN_MICEN_MASK, ctrl1);
- if (ret < 0) {
- dev_err(info->dev, "failed to update MUIC register\n");
- return ret;
- }
+ if (ret < 0)
+ goto report_failure;
if (attached)
ctrl2 |= CTRL2_CPEN_MASK; /* LowPwr=0, CPEn=1 */
@@ -237,16 +233,18 @@ static int max14577_muic_set_path(struct max14577_muic_info *info,
ret = max14577_update_reg(info->max14577->regmap,
MAX14577_REG_CONTROL2,
CTRL2_LOWPWR_MASK | CTRL2_CPEN_MASK, ctrl2);
- if (ret < 0) {
- dev_err(info->dev, "failed to update MUIC register\n");
- return ret;
- }
+ if (ret < 0)
+ goto report_failure;
dev_dbg(info->dev,
"CONTROL1 : 0x%02x, CONTROL2 : 0x%02x, state : %s\n",
ctrl1, ctrl2, attached ? "attached" : "detached");
return 0;
+
+report_failure:
+ dev_err(info->dev, "failed to update MUIC register\n");
+ return ret;
}
/*
--
2.14.2
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] extcon: max14577: Use common error handling code in max14577_muic_set_path()
2017-10-22 17:51 ` [PATCH 1/2] extcon: max14577: Use common error handling code in max14577_muic_set_path() SF Markus Elfring
@ 2017-10-22 18:59 ` Krzysztof Kozlowski
2017-10-22 20:15 ` SF Markus Elfring
2017-10-23 0:57 ` Chanwoo Choi
1 sibling, 1 reply; 10+ messages in thread
From: Krzysztof Kozlowski @ 2017-10-22 18:59 UTC (permalink / raw)
To: kernel-janitors
On Sun, Oct 22, 2017 at 07:51:17PM +0200, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 22 Oct 2017 19:33:58 +0200
>
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/extcon/extcon-max14577.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/extcon/extcon-max14577.c b/drivers/extcon/extcon-max14577.c
> index f6414b7fa5bc..3d4bf5d23236 100644
> --- a/drivers/extcon/extcon-max14577.c
> +++ b/drivers/extcon/extcon-max14577.c
> @@ -211,10 +211,8 @@ static int max14577_muic_set_path(struct max14577_muic_info *info,
> ret = max14577_update_reg(info->max14577->regmap,
> MAX14577_MUIC_REG_CONTROL1,
> CLEAR_IDBEN_MICEN_MASK, CTRL1_SW_OPEN);
> - if (ret < 0) {
> - dev_err(info->dev, "failed to update MUIC register\n");
> - return ret;
> - }
> + if (ret < 0)
> + goto report_failure;
No, one exit path just to report error does not seem to be more
readable. Instead printing error after the register access looks to me
as common pattern, easy to maintain.
This patch does not bring improvement, in my opinion.
Best regards,
Krzysztof
>
> if (attached)
> ctrl1 = val;
> @@ -224,10 +222,8 @@ static int max14577_muic_set_path(struct max14577_muic_info *info,
> ret = max14577_update_reg(info->max14577->regmap,
> MAX14577_MUIC_REG_CONTROL1,
> CLEAR_IDBEN_MICEN_MASK, ctrl1);
> - if (ret < 0) {
> - dev_err(info->dev, "failed to update MUIC register\n");
> - return ret;
> - }
> + if (ret < 0)
> + goto report_failure;
>
> if (attached)
> ctrl2 |= CTRL2_CPEN_MASK; /* LowPwr=0, CPEn=1 */
> @@ -237,16 +233,18 @@ static int max14577_muic_set_path(struct max14577_muic_info *info,
> ret = max14577_update_reg(info->max14577->regmap,
> MAX14577_REG_CONTROL2,
> CTRL2_LOWPWR_MASK | CTRL2_CPEN_MASK, ctrl2);
> - if (ret < 0) {
> - dev_err(info->dev, "failed to update MUIC register\n");
> - return ret;
> - }
> + if (ret < 0)
> + goto report_failure;
>
> dev_dbg(info->dev,
> "CONTROL1 : 0x%02x, CONTROL2 : 0x%02x, state : %s\n",
> ctrl1, ctrl2, attached ? "attached" : "detached");
>
> return 0;
> +
> +report_failure:
> + dev_err(info->dev, "failed to update MUIC register\n");
> + return ret;
> }
>
> /*
> --
> 2.14.2
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] extcon: max14577: Use common error handling code in max14577_muic_set_path()
2017-10-22 18:59 ` Krzysztof Kozlowski
@ 2017-10-22 20:15 ` SF Markus Elfring
0 siblings, 0 replies; 10+ messages in thread
From: SF Markus Elfring @ 2017-10-22 20:15 UTC (permalink / raw)
To: Krzysztof Kozlowski, kernel-janitors
Cc: Bartlomiej Zolnierkiewicz, Chanwoo Choi, MyungJoo Ham, LKML
>> From: Markus Elfring <elfring@users.sourceforge.net>
>> Date: Sun, 22 Oct 2017 19:33:58 +0200
>>
>> Add a jump target so that a bit of exception handling can be better reused
>> at the end of this function.
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
>> ---
>> drivers/extcon/extcon-max14577.c | 22 ++++++++++------------
>> 1 file changed, 10 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/extcon/extcon-max14577.c b/drivers/extcon/extcon-max14577.c
>> index f6414b7fa5bc..3d4bf5d23236 100644
>> --- a/drivers/extcon/extcon-max14577.c
>> +++ b/drivers/extcon/extcon-max14577.c
>> @@ -211,10 +211,8 @@ static int max14577_muic_set_path(struct max14577_muic_info *info,
>> ret = max14577_update_reg(info->max14577->regmap,
>> MAX14577_MUIC_REG_CONTROL1,
>> CLEAR_IDBEN_MICEN_MASK, CTRL1_SW_OPEN);
>> - if (ret < 0) {
>> - dev_err(info->dev, "failed to update MUIC register\n");
>> - return ret;
>> - }
>> + if (ret < 0)
>> + goto report_failure;
>
> No, one exit path just to report error does not seem to be more readable.
I got an other development opinion on this aspect.
> Instead printing error after the register access looks to me
> as common pattern, easy to maintain.
Do you care if corresponding messages are different?
> This patch does not bring improvement, in my opinion.
How do you generally think about the change possibility for a bit of
code reduction?
>>
>> if (attached)
>> ctrl1 = val;
>> @@ -224,10 +222,8 @@ static int max14577_muic_set_path(struct max14577_muic_info *info,
>> ret = max14577_update_reg(info->max14577->regmap,
>> MAX14577_MUIC_REG_CONTROL1,
>> CLEAR_IDBEN_MICEN_MASK, ctrl1);
>> - if (ret < 0) {
>> - dev_err(info->dev, "failed to update MUIC register\n");
>> - return ret;
>> - }
>> + if (ret < 0)
>> + goto report_failure;
>>
>> if (attached)
>> ctrl2 |= CTRL2_CPEN_MASK; /* LowPwr=0, CPEn=1 */
>> @@ -237,16 +233,18 @@ static int max14577_muic_set_path(struct max14577_muic_info *info,
>> ret = max14577_update_reg(info->max14577->regmap,
>> MAX14577_REG_CONTROL2,
>> CTRL2_LOWPWR_MASK | CTRL2_CPEN_MASK, ctrl2);
>> - if (ret < 0) {
>> - dev_err(info->dev, "failed to update MUIC register\n");
>> - return ret;
>> - }
>> + if (ret < 0)
>> + goto report_failure;
>>
>> dev_dbg(info->dev,
>> "CONTROL1 : 0x%02x, CONTROL2 : 0x%02x, state : %s\n",
>> ctrl1, ctrl2, attached ? "attached" : "detached");
>>
>> return 0;
>> +
>> +report_failure:
>> + dev_err(info->dev, "failed to update MUIC register\n");
>> + return ret;
>> }
>>
>> /*
>> --
>> 2.14.2
>>
Would you like to take another look at remaining open issues
in source files from the pattern “extcon-max…”?
Regards,
Markus
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] extcon: max14577: Use common error handling code in max14577_muic_set_path()
2017-10-22 17:51 ` [PATCH 1/2] extcon: max14577: Use common error handling code in max14577_muic_set_path() SF Markus Elfring
2017-10-22 18:59 ` Krzysztof Kozlowski
@ 2017-10-23 0:57 ` Chanwoo Choi
2017-10-23 5:45 ` SF Markus Elfring
1 sibling, 1 reply; 10+ messages in thread
From: Chanwoo Choi @ 2017-10-23 0:57 UTC (permalink / raw)
To: kernel-janitors
Hi,
As you commented, this patch might remove the redundant error message.
But, it makes the code more complicated in side of readability.
If max1577_muic_set_path() have to handle the additional core and exception handling
int the future, this patch might make it more difficult. So, I prefer existing code.
Thanks for your effort.
Regards,
Chanwoo Choi
On 2017년 10월 23일 02:51, SF Markus Elfring wrote:
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 22 Oct 2017 19:33:58 +0200
>
> Add a jump target so that a bit of exception handling can be better reused
> at the end of this function.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
> drivers/extcon/extcon-max14577.c | 22 ++++++++++------------
> 1 file changed, 10 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/extcon/extcon-max14577.c b/drivers/extcon/extcon-max14577.c
> index f6414b7fa5bc..3d4bf5d23236 100644
> --- a/drivers/extcon/extcon-max14577.c
> +++ b/drivers/extcon/extcon-max14577.c
> @@ -211,10 +211,8 @@ static int max14577_muic_set_path(struct max14577_muic_info *info,
> ret = max14577_update_reg(info->max14577->regmap,
> MAX14577_MUIC_REG_CONTROL1,
> CLEAR_IDBEN_MICEN_MASK, CTRL1_SW_OPEN);
> - if (ret < 0) {
> - dev_err(info->dev, "failed to update MUIC register\n");
> - return ret;
> - }
> + if (ret < 0)
> + goto report_failure;
>
> if (attached)
> ctrl1 = val;
> @@ -224,10 +222,8 @@ static int max14577_muic_set_path(struct max14577_muic_info *info,
> ret = max14577_update_reg(info->max14577->regmap,
> MAX14577_MUIC_REG_CONTROL1,
> CLEAR_IDBEN_MICEN_MASK, ctrl1);
> - if (ret < 0) {
> - dev_err(info->dev, "failed to update MUIC register\n");
> - return ret;
> - }
> + if (ret < 0)
> + goto report_failure;
>
> if (attached)
> ctrl2 |= CTRL2_CPEN_MASK; /* LowPwr=0, CPEn=1 */
> @@ -237,16 +233,18 @@ static int max14577_muic_set_path(struct max14577_muic_info *info,
> ret = max14577_update_reg(info->max14577->regmap,
> MAX14577_REG_CONTROL2,
> CTRL2_LOWPWR_MASK | CTRL2_CPEN_MASK, ctrl2);
> - if (ret < 0) {
> - dev_err(info->dev, "failed to update MUIC register\n");
> - return ret;
> - }
> + if (ret < 0)
> + goto report_failure;
>
> dev_dbg(info->dev,
> "CONTROL1 : 0x%02x, CONTROL2 : 0x%02x, state : %s\n",
> ctrl1, ctrl2, attached ? "attached" : "detached");
>
> return 0;
> +
> +report_failure:
> + dev_err(info->dev, "failed to update MUIC register\n");
> + return ret;
> }
>
> /*
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] extcon: max14577: Use common error handling code in max14577_muic_set_path()
2017-10-23 0:57 ` Chanwoo Choi
@ 2017-10-23 5:45 ` SF Markus Elfring
2017-10-23 5:54 ` Chanwoo Choi
0 siblings, 1 reply; 10+ messages in thread
From: SF Markus Elfring @ 2017-10-23 5:45 UTC (permalink / raw)
To: Chanwoo Choi, kernel-janitors
Cc: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, MyungJoo Ham,
LKML
> As you commented, this patch might remove the redundant error message.
> But, it makes the code more complicated in side of readability.
Do you try to avoid duplicated code any more in other circumstances?
Regards,
Markus
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] extcon: max14577: Use common error handling code in max14577_muic_set_path()
2017-10-23 5:45 ` SF Markus Elfring
@ 2017-10-23 5:54 ` Chanwoo Choi
2017-10-23 5:57 ` SF Markus Elfring
0 siblings, 1 reply; 10+ messages in thread
From: Chanwoo Choi @ 2017-10-23 5:54 UTC (permalink / raw)
To: SF Markus Elfring, kernel-janitors
Cc: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, MyungJoo Ham,
LKML
On 2017년 10월 23일 14:45, SF Markus Elfring wrote:
>> As you commented, this patch might remove the redundant error message.
>> But, it makes the code more complicated in side of readability.
>
> Do you try to avoid duplicated code any more in other circumstances?
I usually used the goto statement on following cases:
- Return the value (error number if fail or 0 if success)
- Do free or unregister or remove operations when error happen.
I think that there is any benefit of this patch.
Also, as I commented, it make the code more complicated.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: extcon: max14577: Use common error handling code in max14577_muic_set_path()
2017-10-23 5:54 ` Chanwoo Choi
@ 2017-10-23 5:57 ` SF Markus Elfring
2017-10-23 6:03 ` Chanwoo Choi
0 siblings, 1 reply; 10+ messages in thread
From: SF Markus Elfring @ 2017-10-23 5:57 UTC (permalink / raw)
To: Chanwoo Choi, kernel-janitors
Cc: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, MyungJoo Ham,
LKML
> I think that there is any benefit of this patch.
> Also, as I commented, it make the code more complicated.
We have got different software development opinions about
the shown change possibilities then.
Regards,
Markus
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: extcon: max14577: Use common error handling code in max14577_muic_set_path()
2017-10-23 5:57 ` SF Markus Elfring
@ 2017-10-23 6:03 ` Chanwoo Choi
2017-10-23 6:08 ` SF Markus Elfring
0 siblings, 1 reply; 10+ messages in thread
From: Chanwoo Choi @ 2017-10-23 6:03 UTC (permalink / raw)
To: SF Markus Elfring, kernel-janitors
Cc: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, MyungJoo Ham,
LKML
On 2017년 10월 23일 14:57, SF Markus Elfring wrote:
>> I think that there is any benefit of this patch.
>> Also, as I commented, it make the code more complicated.
>
> We have got different software development opinions about
I agree absolutely. So, anyone can suggest the opinion and send patches.
But, all patches have to get the review from maintainer, reviewer
or the mailing-list developer.
(Please don't remove the part of my comment when you reply.)
> the shown change possibilities then.
--
Best Regards,
Chanwoo Choi
Samsung Electronics
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: extcon: max14577: Use common error handling code in max14577_muic_set_path()
2017-10-23 6:03 ` Chanwoo Choi
@ 2017-10-23 6:08 ` SF Markus Elfring
0 siblings, 0 replies; 10+ messages in thread
From: SF Markus Elfring @ 2017-10-23 6:08 UTC (permalink / raw)
To: Chanwoo Choi, kernel-janitors
Cc: Bartlomiej Zolnierkiewicz, Krzysztof Kozlowski, MyungJoo Ham,
LKML
>>> I think that there is any benefit of this patch.
>>> Also, as I commented, it make the code more complicated.
>>
>> We have got different software development opinions about
>
> I agree absolutely. So, anyone can suggest the opinion and send patches.
> But, all patches have to get the review from maintainer, reviewer
> or the mailing-list developer.
Can an other change acceptance evolve over time?
> (Please don't remove the part of my comment when you reply.)
>
>> the shown change possibilities then.
Do you find similar updates more useful for any other software modules?
Regards,
Markus
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-10-23 6:08 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-22 17:50 [PATCH 0/2] extcon-max14577: Fine-tuning for max14577_muic_set_path() SF Markus Elfring
2017-10-22 17:51 ` [PATCH 1/2] extcon: max14577: Use common error handling code in max14577_muic_set_path() SF Markus Elfring
2017-10-22 18:59 ` Krzysztof Kozlowski
2017-10-22 20:15 ` SF Markus Elfring
2017-10-23 0:57 ` Chanwoo Choi
2017-10-23 5:45 ` SF Markus Elfring
2017-10-23 5:54 ` Chanwoo Choi
2017-10-23 5:57 ` SF Markus Elfring
2017-10-23 6:03 ` Chanwoo Choi
2017-10-23 6:08 ` SF Markus Elfring
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).