All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] hwmon: (pmbus) Add missing break statements
@ 2017-05-03 19:31 ` Dan Carpenter
  0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2017-05-03 19:31 UTC (permalink / raw)
  To: Guenter Roeck, Samuel Mendoza-Jonas
  Cc: Jean Delvare, linux-hwmon, kernel-janitors

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);
@@ -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;

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* [PATCH] hwmon: (pmbus) Add missing break statements
@ 2017-05-03 19:31 ` Dan Carpenter
  0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2017-05-03 19:31 UTC (permalink / raw)
  To: Guenter Roeck, Samuel Mendoza-Jonas
  Cc: Jean Delvare, linux-hwmon, kernel-janitors

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);
@@ -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;

^ permalink raw reply related	[flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: (pmbus) Add missing break statements
  2017-05-03 19:31 ` Dan Carpenter
@ 2017-05-03 21:57   ` Guenter Roeck
  -1 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2017-05-03 21:57 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon, kernel-janitors

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;

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: (pmbus) Add missing break statements
@ 2017-05-03 21:57   ` Guenter Roeck
  0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2017-05-03 21:57 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon, kernel-janitors

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;

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: (pmbus) Add missing break statements
  2017-05-03 19:31 ` Dan Carpenter
@ 2017-05-04  7:28   ` walter harms
  -1 siblings, 0 replies; 12+ messages in thread
From: walter harms @ 2017-05-04  7:28 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Guenter Roeck, Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon,
	kernel-janitors



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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: (pmbus) Add missing break statements
@ 2017-05-04  7:28   ` walter harms
  0 siblings, 0 replies; 12+ messages in thread
From: walter harms @ 2017-05-04  7:28 UTC (permalink / raw)
  To: Dan Carpenter
  Cc: Guenter Roeck, Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon,
	kernel-janitors



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
> 

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: (pmbus) Add missing break statements
  2017-05-04  7:28   ` walter harms
@ 2017-05-04  7:31     ` Dan Carpenter
  -1 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2017-05-04  7:31 UTC (permalink / raw)
  To: walter harms
  Cc: Guenter Roeck, Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon,
	kernel-janitors

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

^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: (pmbus) Add missing break statements
@ 2017-05-04  7:31     ` Dan Carpenter
  0 siblings, 0 replies; 12+ messages in thread
From: Dan Carpenter @ 2017-05-04  7:31 UTC (permalink / raw)
  To: walter harms
  Cc: Guenter Roeck, Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon,
	kernel-janitors

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


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: (pmbus) Add missing break statements
  2017-05-04  7:31     ` Dan Carpenter
@ 2017-05-04 13:42       ` Guenter Roeck
  -1 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2017-05-04 13:42 UTC (permalink / raw)
  To: Dan Carpenter, walter harms
  Cc: Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon, kernel-janitors

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


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: (pmbus) Add missing break statements
@ 2017-05-04 13:42       ` Guenter Roeck
  0 siblings, 0 replies; 12+ messages in thread
From: Guenter Roeck @ 2017-05-04 13:42 UTC (permalink / raw)
  To: Dan Carpenter, walter harms
  Cc: Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon, kernel-janitors

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


^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: (pmbus) Add missing break statements
  2017-05-04 13:42       ` Guenter Roeck
@ 2017-05-04 14:36         ` walter harms
  -1 siblings, 0 replies; 12+ messages in thread
From: walter harms @ 2017-05-04 14:36 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Dan Carpenter, Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon,
	kernel-janitors



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



^ permalink raw reply	[flat|nested] 12+ messages in thread
* Re: [PATCH] hwmon: (pmbus) Add missing break statements
@ 2017-05-04 14:36         ` walter harms
  0 siblings, 0 replies; 12+ messages in thread
From: walter harms @ 2017-05-04 14:36 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Dan Carpenter, Samuel Mendoza-Jonas, Jean Delvare, linux-hwmon,
	kernel-janitors



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



^ permalink raw reply	[flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-05-04 14:36 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-05-03 19:31 [PATCH] hwmon: (pmbus) Add missing break statements Dan Carpenter
2017-05-03 19:31 ` Dan Carpenter
2017-05-03 21:57 ` Guenter Roeck
2017-05-03 21:57   ` Guenter Roeck
2017-05-04  7:28 ` walter harms
2017-05-04  7:28   ` walter harms
2017-05-04  7:31   ` Dan Carpenter
2017-05-04  7:31     ` Dan Carpenter
2017-05-04 13:42     ` Guenter Roeck
2017-05-04 13:42       ` Guenter Roeck
2017-05-04 14:36       ` walter harms
2017-05-04 14:36         ` walter harms

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.