From mboxrd@z Thu Jan 1 00:00:00 1970 From: walter harms Date: Thu, 04 May 2017 14:36:04 +0000 Subject: Re: [PATCH] hwmon: (pmbus) Add missing break statements Message-Id: <590B3C54.1060700@bfs.de> List-Id: References: <20170503193140.zbxcdgda7k3rnr2m@mwanda> <590AD813.6030805@bfs.de> <20170504073109.b2etej6kg3dzt4vb@mwanda> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Guenter Roeck Cc: Dan Carpenter , Samuel Mendoza-Jonas , Jean Delvare , linux-hwmon@vger.kernel.org, kernel-janitors@vger.kernel.org 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