diff mbox

hwmon: (pmbus) Add missing break statements

Message ID 20170503193140.zbxcdgda7k3rnr2m@mwanda (mailing list archive)
State Accepted
Headers show

Commit Message

Dan Carpenter May 3, 2017, 7:31 p.m. UTC 
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>

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Guenter Roeck May 3, 2017, 9:57 p.m. UTC | #1 
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;
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Walter Harms May 4, 2017, 7:28 a.m. UTC | #2 
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
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dan Carpenter May 4, 2017, 7:31 a.m. UTC | #3 
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

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Guenter Roeck May 4, 2017, 1:42 p.m. UTC | #4 
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

--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Walter Harms May 4, 2017, 2:36 p.m. UTC | #5 
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


--
To unsubscribe from this list: send the line "unsubscribe linux-hwmon" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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;