Re: [PATCH] hwmon: (pmbus) Add missing break statements
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
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
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: (pmbus) Add missing break statements
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
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
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: (it87) Remove useless test during device detection
On Thu, 4 May 2017 11:35:06 +0200, Jean Delvare wrote: > There is no reason to treat the IT8705F differently during device > detection. If a single IT8705F chip indeed answers to both Super-IO > addresses, we have code in place to detect the duplicate device > address and skip the second one. > (...) Bah, scratch this. I can't even convince myself that this is a good idea. Sure, the rest of the code is enough to deal with the situation, but why keep looking for something when we already know we will find and discard a duplicate... Sorry for the noise, -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: (pmbus) Add missing break statements
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
> >
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hwmon: (pmbus) Add missing break statements
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
>
> 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 majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
