* [PATCH] hwmon: (pmbus) Add missing break statements
@ 2017-05-03 19:31 Dan Carpenter
2017-05-03 21:57 ` Guenter Roeck
2017-05-04 7:28 ` walter harms
0 siblings, 2 replies; 6+ messages in thread
From: Dan Carpenter @ 2017-05-03 19:31 UTC (permalink / raw)
To: Guenter Roeck, Samuel Mendoza-Jonas
Cc: Jean Delvare, linux-hwmon, kernel-janitors
Static checkers complain about these missing break statements.
Fixes: 6eaaea144dc5 ("hwmon: (pmbus) Add client driver for IR35221")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
diff --git a/drivers/hwmon/pmbus/ir35221.c b/drivers/hwmon/pmbus/ir35221.c
index cc7b3b542531..00e4a1e264e2 100644
--- a/drivers/hwmon/pmbus/ir35221.c
+++ b/drivers/hwmon/pmbus/ir35221.c
@@ -129,6 +129,7 @@ static int ir35221_read_word_data(struct i2c_client *client, int page, int reg)
case PMBUS_IIN_OC_WARN_LIMIT:
ret = pmbus_read_word_data(client, page, reg);
ret = ir35221_scale_result(ret, -1, PSC_CURRENT_IN);
+ break;
case PMBUS_READ_VIN:
ret = pmbus_read_word_data(client, page, PMBUS_READ_VIN);
ret = ir35221_scale_result(ret, -5, PSC_VOLTAGE_IN);
@@ -222,6 +223,7 @@ static int ir35221_write_word_data(struct i2c_client *client, int page, int reg,
case PMBUS_IIN_OC_WARN_LIMIT:
val = ir35221_scale_result(word, 1, PSC_CURRENT_IN);
ret = pmbus_write_word_data(client, page, reg, val);
+ break;
default:
ret = -ENODATA;
break;
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] hwmon: (pmbus) Add missing break statements
2017-05-03 19:31 [PATCH] hwmon: (pmbus) Add missing break statements Dan Carpenter
@ 2017-05-03 21:57 ` Guenter Roeck
2017-05-04 7:28 ` walter harms
1 sibling, 0 replies; 6+ messages in thread
From: Guenter Roeck @ 2017-05-03 21:57 UTC (permalink / raw)
To: Dan Carpenter
Cc: Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon, kernel-janitors
On Wed, May 03, 2017 at 10:31:40PM +0300, Dan Carpenter wrote:
> Static checkers complain about these missing break statements.
>
> Fixes: 6eaaea144dc5 ("hwmon: (pmbus) Add client driver for IR35221")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied, thanks.
Guenter
>
> diff --git a/drivers/hwmon/pmbus/ir35221.c b/drivers/hwmon/pmbus/ir35221.c
> index cc7b3b542531..00e4a1e264e2 100644
> --- a/drivers/hwmon/pmbus/ir35221.c
> +++ b/drivers/hwmon/pmbus/ir35221.c
> @@ -129,6 +129,7 @@ static int ir35221_read_word_data(struct i2c_client *client, int page, int reg)
> case PMBUS_IIN_OC_WARN_LIMIT:
> ret = pmbus_read_word_data(client, page, reg);
> ret = ir35221_scale_result(ret, -1, PSC_CURRENT_IN);
> + break;
> case PMBUS_READ_VIN:
> ret = pmbus_read_word_data(client, page, PMBUS_READ_VIN);
> ret = ir35221_scale_result(ret, -5, PSC_VOLTAGE_IN);
> @@ -222,6 +223,7 @@ static int ir35221_write_word_data(struct i2c_client *client, int page, int reg,
> case PMBUS_IIN_OC_WARN_LIMIT:
> val = ir35221_scale_result(word, 1, PSC_CURRENT_IN);
> ret = pmbus_write_word_data(client, page, reg, val);
> + break;
> default:
> ret = -ENODATA;
> break;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hwmon: (pmbus) Add missing break statements
2017-05-03 19:31 [PATCH] hwmon: (pmbus) Add missing break statements Dan Carpenter
2017-05-03 21:57 ` Guenter Roeck
@ 2017-05-04 7:28 ` walter harms
2017-05-04 7:31 ` Dan Carpenter
1 sibling, 1 reply; 6+ messages in thread
From: walter harms @ 2017-05-04 7:28 UTC (permalink / raw)
To: Dan Carpenter
Cc: Guenter Roeck, Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon,
kernel-janitors
Am 03.05.2017 21:31, schrieb Dan Carpenter:
> Static checkers complain about these missing break statements.
>
> Fixes: 6eaaea144dc5 ("hwmon: (pmbus) Add client driver for IR35221")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> diff --git a/drivers/hwmon/pmbus/ir35221.c b/drivers/hwmon/pmbus/ir35221.c
> index cc7b3b542531..00e4a1e264e2 100644
> --- a/drivers/hwmon/pmbus/ir35221.c
> +++ b/drivers/hwmon/pmbus/ir35221.c
> @@ -129,6 +129,7 @@ static int ir35221_read_word_data(struct i2c_client *client, int page, int reg)
> case PMBUS_IIN_OC_WARN_LIMIT:
> ret = pmbus_read_word_data(client, page, reg);
> ret = ir35221_scale_result(ret, -1, PSC_CURRENT_IN);
> + break;
> case PMBUS_READ_VIN:
> ret = pmbus_read_word_data(client, page, PMBUS_READ_VIN);
> ret = ir35221_scale_result(ret, -5, PSC_VOLTAGE_IN);
Just a remark:
the naming of the variable for pmbus_read_word_data() is unfortunate.
It would be nice to have it like below: val
just my 2 cents,
re,
wh
> @@ -222,6 +223,7 @@ static int ir35221_write_word_data(struct i2c_client *client, int page, int reg,
> case PMBUS_IIN_OC_WARN_LIMIT:
> val = ir35221_scale_result(word, 1, PSC_CURRENT_IN);
> ret = pmbus_write_word_data(client, page, reg, val);
> + break;
> default:
> ret = -ENODATA;
> break;
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hwmon: (pmbus) Add missing break statements
2017-05-04 7:28 ` walter harms
@ 2017-05-04 7:31 ` Dan Carpenter
2017-05-04 13:42 ` Guenter Roeck
0 siblings, 1 reply; 6+ messages in thread
From: Dan Carpenter @ 2017-05-04 7:31 UTC (permalink / raw)
To: walter harms
Cc: Guenter Roeck, Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon,
kernel-janitors
On Thu, May 04, 2017 at 09:28:19AM +0200, walter harms wrote:
>
>
> Am 03.05.2017 21:31, schrieb Dan Carpenter:
> > Static checkers complain about these missing break statements.
> >
> > Fixes: 6eaaea144dc5 ("hwmon: (pmbus) Add client driver for IR35221")
> > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> >
> > diff --git a/drivers/hwmon/pmbus/ir35221.c b/drivers/hwmon/pmbus/ir35221.c
> > index cc7b3b542531..00e4a1e264e2 100644
> > --- a/drivers/hwmon/pmbus/ir35221.c
> > +++ b/drivers/hwmon/pmbus/ir35221.c
> > @@ -129,6 +129,7 @@ static int ir35221_read_word_data(struct i2c_client *client, int page, int reg)
> > case PMBUS_IIN_OC_WARN_LIMIT:
> > ret = pmbus_read_word_data(client, page, reg);
> > ret = ir35221_scale_result(ret, -1, PSC_CURRENT_IN);
> > + break;
> > case PMBUS_READ_VIN:
> > ret = pmbus_read_word_data(client, page, PMBUS_READ_VIN);
> > ret = ir35221_scale_result(ret, -5, PSC_VOLTAGE_IN);
>
> Just a remark:
> the naming of the variable for pmbus_read_word_data() is unfortunate.
> It would be nice to have it like below: val
>
Yeah. I thought so too.
regards,
dan carpenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hwmon: (pmbus) Add missing break statements
2017-05-04 7:31 ` Dan Carpenter
@ 2017-05-04 13:42 ` Guenter Roeck
2017-05-04 14:36 ` walter harms
0 siblings, 1 reply; 6+ messages in thread
From: Guenter Roeck @ 2017-05-04 13:42 UTC (permalink / raw)
To: Dan Carpenter, walter harms
Cc: Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon, kernel-janitors
On 05/04/2017 12:31 AM, Dan Carpenter wrote:
> On Thu, May 04, 2017 at 09:28:19AM +0200, walter harms wrote:
>>
>>
>> Am 03.05.2017 21:31, schrieb Dan Carpenter:
>>> Static checkers complain about these missing break statements.
>>>
>>> Fixes: 6eaaea144dc5 ("hwmon: (pmbus) Add client driver for IR35221")
>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>
>>> diff --git a/drivers/hwmon/pmbus/ir35221.c b/drivers/hwmon/pmbus/ir35221.c
>>> index cc7b3b542531..00e4a1e264e2 100644
>>> --- a/drivers/hwmon/pmbus/ir35221.c
>>> +++ b/drivers/hwmon/pmbus/ir35221.c
>>> @@ -129,6 +129,7 @@ static int ir35221_read_word_data(struct i2c_client *client, int page, int reg)
>>> case PMBUS_IIN_OC_WARN_LIMIT:
>>> ret = pmbus_read_word_data(client, page, reg);
>>> ret = ir35221_scale_result(ret, -1, PSC_CURRENT_IN);
>>> + break;
>>> case PMBUS_READ_VIN:
>>> ret = pmbus_read_word_data(client, page, PMBUS_READ_VIN);
>>> ret = ir35221_scale_result(ret, -5, PSC_VOLTAGE_IN);
>>
>> Just a remark:
>> the naming of the variable for pmbus_read_word_data() is unfortunate.
>> It would be nice to have it like below: val
>>
>
> Yeah. I thought so too.
>
The real problem here is that ret < 0 should return an error without rescale,
which I overlooked. That is a real bug, which isn't fixed by adding a new
variable to this function. So it would have to be
ret = pmbus_read_word_data();
if (ret < 0)
break; // or return ret;
ret = ir35221_scale_result();
break;
or
val = pmbus_read_word_data();
if (val < 0)
return val;
ret = ir35221_scale_result();
break;
or
val = pmbus_read_word_data();
if (val < 0) {
ret = val;
break;
}
ret = ir35221_scale_result();
break;
Out of those, I personally prefer the first. I don't really see how adding
a variable would improve the code.
Guenter
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] hwmon: (pmbus) Add missing break statements
2017-05-04 13:42 ` Guenter Roeck
@ 2017-05-04 14:36 ` walter harms
0 siblings, 0 replies; 6+ messages in thread
From: walter harms @ 2017-05-04 14:36 UTC (permalink / raw)
To: Guenter Roeck
Cc: Dan Carpenter, Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon,
kernel-janitors
Am 04.05.2017 15:42, schrieb Guenter Roeck:
> On 05/04/2017 12:31 AM, Dan Carpenter wrote:
>> On Thu, May 04, 2017 at 09:28:19AM +0200, walter harms wrote:
>>>
>>>
>>> Am 03.05.2017 21:31, schrieb Dan Carpenter:
>>>> Static checkers complain about these missing break statements.
>>>>
>>>> Fixes: 6eaaea144dc5 ("hwmon: (pmbus) Add client driver for IR35221")
>>>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>>>>
>>>> diff --git a/drivers/hwmon/pmbus/ir35221.c
>>>> b/drivers/hwmon/pmbus/ir35221.c
>>>> index cc7b3b542531..00e4a1e264e2 100644
>>>> --- a/drivers/hwmon/pmbus/ir35221.c
>>>> +++ b/drivers/hwmon/pmbus/ir35221.c
>>>> @@ -129,6 +129,7 @@ static int ir35221_read_word_data(struct
>>>> i2c_client *client, int page, int reg)
>>>> case PMBUS_IIN_OC_WARN_LIMIT:
>>>> ret = pmbus_read_word_data(client, page, reg);
>>>> ret = ir35221_scale_result(ret, -1, PSC_CURRENT_IN);
>>>> + break;
>>>> case PMBUS_READ_VIN:
>>>> ret = pmbus_read_word_data(client, page, PMBUS_READ_VIN);
>>>> ret = ir35221_scale_result(ret, -5, PSC_VOLTAGE_IN);
>>>
>>> Just a remark:
>>> the naming of the variable for pmbus_read_word_data() is unfortunate.
>>> It would be nice to have it like below: val
>>>
>>
>> Yeah. I thought so too.
>>
>
> The real problem here is that ret < 0 should return an error without
> rescale,
> which I overlooked. That is a real bug, which isn't fixed by adding a new
> variable to this function. So it would have to be
>
> ret = pmbus_read_word_data();
> if (ret < 0)
> break; // or return ret;
> ret = ir35221_scale_result();
> break;
>
> or
> val = pmbus_read_word_data();
> if (val < 0)
> return val;
> ret = ir35221_scale_result();
> break;
>
> or
> val = pmbus_read_word_data();
> if (val < 0) {
> ret = val;
> break;
> }
> ret = ir35221_scale_result();
> break;
>
> Out of those, I personally prefer the first. I don't really see how adding
> a variable would improve the code.
>
the "if" changes everything. Is all about naming the variables. With
ret you give the impression that it may contain an error indicator,
but with val you say "this is a value".
short: if "if" gets added everything is fine
hope that helps,
wh
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-05-04 14:36 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-05-03 19:31 [PATCH] hwmon: (pmbus) Add missing break statements Dan Carpenter
2017-05-03 21:57 ` Guenter Roeck
2017-05-04 7:28 ` walter harms
2017-05-04 7:31 ` Dan Carpenter
2017-05-04 13:42 ` Guenter Roeck
2017-05-04 14:36 ` walter harms
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox