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