* [PATCH] bq2415x_charger: Use common error handling code in bq2415x_timer_work()
@ 2017-10-30 20:50 SF Markus Elfring
2017-11-09 12:08 ` Pavel Machek
0 siblings, 1 reply; 5+ messages in thread
From: SF Markus Elfring @ 2017-10-30 20:50 UTC (permalink / raw)
To: linux-pm, Pali Rohár, Sebastian Reichel; +Cc: LKML, kernel-janitors
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Mon, 30 Oct 2017 21:45:09 +0100
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/power/supply/bq2415x_charger.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/power/supply/bq2415x_charger.c b/drivers/power/supply/bq2415x_charger.c
index c4770a94cc8e..8385e02e1634 100644
--- a/drivers/power/supply/bq2415x_charger.c
+++ b/drivers/power/supply/bq2415x_charger.c
@@ -914,16 +914,12 @@ static void bq2415x_timer_work(struct work_struct *work)
}
boost = bq2415x_exec_command(bq, BQ2415X_BOOST_MODE_STATUS);
- if (boost < 0) {
- bq2415x_timer_error(bq, "Unknown error");
- return;
- }
+ if (boost < 0)
+ goto report_timer_error;
error = bq2415x_exec_command(bq, BQ2415X_FAULT_STATUS);
- if (error < 0) {
- bq2415x_timer_error(bq, "Unknown error");
- return;
- }
+ if (error < 0)
+ goto report_timer_error;
if (boost) {
switch (error) {
@@ -992,6 +988,10 @@ static void bq2415x_timer_work(struct work_struct *work)
}
schedule_delayed_work(&bq->work, BQ2415X_TIMER_TIMEOUT * HZ);
+ return;
+
+report_timer_error:
+ bq2415x_timer_error(bq, "Unknown error");
}
/**** power supply interface code ****/
--
2.14.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] bq2415x_charger: Use common error handling code in bq2415x_timer_work()
2017-10-30 20:50 [PATCH] bq2415x_charger: Use common error handling code in bq2415x_timer_work() SF Markus Elfring
@ 2017-11-09 12:08 ` Pavel Machek
2017-11-09 13:04 ` SF Markus Elfring
0 siblings, 1 reply; 5+ messages in thread
From: Pavel Machek @ 2017-11-09 12:08 UTC (permalink / raw)
To: SF Markus Elfring
Cc: linux-pm, Pali Roh?r, Sebastian Reichel, LKML, kernel-janitors
> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Mon, 30 Oct 2017 21:45:09 +0100
>
> 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>
Better fix would be to display separate messages; user is probably
interested in what failed...
Pavel
> ---
> drivers/power/supply/bq2415x_charger.c | 16 ++++++++--------
> 1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/power/supply/bq2415x_charger.c b/drivers/power/supply/bq2415x_charger.c
> index c4770a94cc8e..8385e02e1634 100644
> --- a/drivers/power/supply/bq2415x_charger.c
> +++ b/drivers/power/supply/bq2415x_charger.c
> @@ -914,16 +914,12 @@ static void bq2415x_timer_work(struct work_struct *work)
> }
>
> boost = bq2415x_exec_command(bq, BQ2415X_BOOST_MODE_STATUS);
> - if (boost < 0) {
> - bq2415x_timer_error(bq, "Unknown error");
> - return;
> - }
> + if (boost < 0)
> + goto report_timer_error;
>
> error = bq2415x_exec_command(bq, BQ2415X_FAULT_STATUS);
> - if (error < 0) {
> - bq2415x_timer_error(bq, "Unknown error");
> - return;
> - }
> + if (error < 0)
> + goto report_timer_error;
>
> if (boost) {
> switch (error) {
> @@ -992,6 +988,10 @@ static void bq2415x_timer_work(struct work_struct *work)
> }
>
> schedule_delayed_work(&bq->work, BQ2415X_TIMER_TIMEOUT * HZ);
> + return;
> +
> +report_timer_error:
> + bq2415x_timer_error(bq, "Unknown error");
> }
>
> /**** power supply interface code ****/
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: bq2415x_charger: Use common error handling code in bq2415x_timer_work()
2017-11-09 12:08 ` Pavel Machek
@ 2017-11-09 13:04 ` SF Markus Elfring
2017-11-09 13:14 ` Pali Rohár
0 siblings, 1 reply; 5+ messages in thread
From: SF Markus Elfring @ 2017-11-09 13:04 UTC (permalink / raw)
To: Pavel Machek, linux-pm
Cc: Pali Rohár, Sebastian Reichel, LKML, kernel-janitors
> Better fix would be to display separate messages; user is probably
> interested in what failed...
Which information (or wording) would you find more appropriate
at these places?
Regards,
Markus
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: bq2415x_charger: Use common error handling code in bq2415x_timer_work()
2017-11-09 13:04 ` SF Markus Elfring
@ 2017-11-09 13:14 ` Pali Rohár
2017-11-09 13:22 ` SF Markus Elfring
0 siblings, 1 reply; 5+ messages in thread
From: Pali Rohár @ 2017-11-09 13:14 UTC (permalink / raw)
To: SF Markus Elfring
Cc: Pavel Machek, linux-pm, Sebastian Reichel, LKML, kernel-janitors
On Thursday 09 November 2017 14:04:19 SF Markus Elfring wrote:
> > Better fix would be to display separate messages; user is probably
> > interested in what failed...
>
> Which information (or wording) would you find more appropriate
> at these places?
Hi! Basically dropping your patch and instead of the "Unknown error"
return to user reason why BQ2415X_BOOST_MODE_STATUS or
BQ2415X_FAULT_STATUS commands failed. Or at least instead of the
"Unknown error" write "Unknown error during BQ2415X_FAULT_STATUS".
Basically I do not see any value in your patch. Current coding style
pattern in that function is:
do_something;
if failed:
print error;
return;
And your patch just changed some, but not *all* parts of code to:
do_something;
if failed:
goto end_of_function
If you are changing coding style, I would really suggest to change it on
all places to let it consistent. Because your change introduces just
inconsistency.
--
Pali Rohár
pali.rohar@gmail.com
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: bq2415x_charger: Use common error handling code in bq2415x_timer_work()
2017-11-09 13:14 ` Pali Rohár
@ 2017-11-09 13:22 ` SF Markus Elfring
0 siblings, 0 replies; 5+ messages in thread
From: SF Markus Elfring @ 2017-11-09 13:22 UTC (permalink / raw)
To: Pali Rohár, linux-pm
Cc: Pavel Machek, Sebastian Reichel, LKML, kernel-janitors
>> Which information (or wording) would you find more appropriate
>> at these places?
>
> Hi! Basically dropping your patch and instead of the "Unknown error"
> return to user reason why BQ2415X_BOOST_MODE_STATUS or
> BQ2415X_FAULT_STATUS commands failed. Or at least instead of the
> "Unknown error" write "Unknown error during BQ2415X_FAULT_STATUS".
Can the error description become better?
Regards,
Markus
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-11-09 13:22 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-30 20:50 [PATCH] bq2415x_charger: Use common error handling code in bq2415x_timer_work() SF Markus Elfring
2017-11-09 12:08 ` Pavel Machek
2017-11-09 13:04 ` SF Markus Elfring
2017-11-09 13:14 ` Pali Rohár
2017-11-09 13:22 ` 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).