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@xxxxxxxxxx>
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 kernel-janitors" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
