drivers/regulator/core.c: Fixes mapping inside regulator_mode_to_status() and makes it returning -EINVAL on invalid input.

drivers/regulator/core.c: Fixes mapping inside regulator_mode_to_status() and makes it returning -EINVAL on invalid input.

[Krystian Garbaciak]

Minor fix that makes the function working correctly with
REGULATOR_MODE_STANDBY as parameter.
Also, on invalid input (bad mode), it is better to return -EINVAL,
instead of meaningless 0 value (which actually is interpreted as
REGULATOR_STATUS_OFF).

--- 
diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 938398f..45aa874 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2484,10 +2484,10 @@ int regulator_mode_to_status(unsigned int mode)
 		return REGULATOR_STATUS_NORMAL;
 	case REGULATOR_MODE_IDLE:
 		return REGULATOR_STATUS_IDLE;
-	case REGULATOR_STATUS_STANDBY:
+	case REGULATOR_MODE_STANDBY:
 		return REGULATOR_STATUS_STANDBY;
 	default:
-		return 0;
+		return -EINVAL;
 	}
 }
 EXPORT_SYMBOL_GPL(regulator_mode_to_status);

Re: drivers/regulator/core.c: Fixes mapping inside regulator_mode_to_status() and makes it returning -EINVAL on invalid input.

[Mark Brown]

On Mon, Jan 09, 2012 at 12:12:20PM +0000, Krystian Garbaciak wrote:
> Minor fix that makes the function working correctly with
> REGULATOR_MODE_STANDBY as parameter.
> Also, on invalid input (bad mode), it is better to return -EINVAL,
> instead of meaningless 0 value (which actually is interpreted as
> REGULATOR_STATUS_OFF).
> 
> --- 

As I previously said you need to follow the patch submission process in
SubmittingPatches.  Here you need to:

 - Sign off your patch.  Without this your patch cannot be applied.
 - Send the patches to the maintainers (not just one of them) and the
   list.

You should also make an effort to use subject lines for your patch that
match up with what the rest of the subsystem is doing.

>  	default:
> -		return 0;
> +		return -EINVAL;

This behaviour is deliberate.

Re: drivers/regulator/core.c: Fixes mapping inside regulator_mode_to_status() and makes it returning -EINVAL on invalid input.

[dd diasemi]

On Mon, Jan 9, 2012 at 4:14 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Jan 09, 2012 at 12:12:20PM +0000, Krystian Garbaciak wrote:
>> Minor fix that makes the function working correctly with
>> REGULATOR_MODE_STANDBY as parameter.
>> Also, on invalid input (bad mode), it is better to return -EINVAL,
>> instead of meaningless 0 value (which actually is interpreted as
>> REGULATOR_STATUS_OFF).
>>
>> ---
>
>>       default:
>> -             return 0;
>> +             return -EINVAL;
>
> This behaviour is deliberate.

For this moment, the only usage is in one file in
wm831x_aldo_get_status() and wm831x_gp_ldo_get_status():
	ret = wm831x_aldo_get_mode(rdev);
	if (ret < 0)
		return ret;
	else
		return regulator_mode_to_status(ret);

The usage mistreats the signed value 'ret' as wm831x_aldo_get_mode()
always returns 0 on communication error. It effects in response as if
the regulator was off.

If that behaviour is deliberate, I would suggest to make it explicit:
 	default:
-		return 0;
+		return REGULATOR_STATUS_OFF;

Re: drivers/regulator/core.c: Fixes mapping inside regulator_mode_to_status() and makes it returning -EINVAL on invalid input.

[Mark Brown]

On Mon, Jan 09, 2012 at 07:20:50PM +0000, dd diasemi wrote:
> On Mon, Jan 9, 2012 at 4:14 PM, Mark Brown

> >>       default:
> >> -             return 0;
> >> +             return -EINVAL;

> > This behaviour is deliberate.

> For this moment, the only usage is in one file in
> wm831x_aldo_get_status() and wm831x_gp_ldo_get_status():
> 	ret = wm831x_aldo_get_mode(rdev);
> 	if (ret < 0)
> 		return ret;
> 	else
> 		return regulator_mode_to_status(ret);

> The usage mistreats the signed value 'ret' as wm831x_aldo_get_mode()
> always returns 0 on communication error. It effects in response as if
> the regulator was off.

> If that behaviour is deliberate, I would suggest to make it explicit:
>  	default:
> -		return 0;
> +		return REGULATOR_STATUS_OFF;

That's not the deliberate behaviour, the deliberate behaviour is to
return no mode if we didn't find one.

Re: drivers/regulator/core.c: Fixes mapping inside regulator_mode_to_status() and makes it returning -EINVAL on invalid input.

[dd diasemi]

On Mon, Jan 9, 2012 at 8:07 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Jan 09, 2012 at 07:20:50PM +0000, dd diasemi wrote:
>> On Mon, Jan 9, 2012 at 4:14 PM, Mark Brown
>
>> >>       default:
>> >> -             return 0;
>> >> +             return -EINVAL;
>
>> > This behaviour is deliberate.
>
>> For this moment, the only usage is in one file in
>> wm831x_aldo_get_status() and wm831x_gp_ldo_get_status():
>>       ret = wm831x_aldo_get_mode(rdev);
>>       if (ret < 0)
>>               return ret;
>>       else
>>               return regulator_mode_to_status(ret);
>
>> The usage mistreats the signed value 'ret' as wm831x_aldo_get_mode()
>> always returns 0 on communication error. It effects in response as if
>> the regulator was off.
>
>> If that behaviour is deliberate, I would suggest to make it explicit:
>>       default:
>> -             return 0;
>> +             return REGULATOR_STATUS_OFF;
>
> That's not the deliberate behaviour, the deliberate behaviour is to
> return no mode if we didn't find one.

I am focusing now on regulator_mode_to_status(). It returns regulator
status inside int value. If driver passes 0 to the function (eg. in
case of communication failure), which is not a valid mode, either it
may return REGULATOR_STATUS_OFF (0) as a valid status or it may return
an error code.

I am aware, that dealing with communication problem is rather in
driver responsibility, than the framework itself, but keeping things
clear is also important, especially for framework.

Making regulator_mode_to_status() returning an error allows to
simplify its usage:
	ret = regulator_mode_to_status(regulator_get_mode(rdev));
	if (ret < 0)
		ret = -EIO;

Re: drivers/regulator/core.c: Fixes mapping inside regulator_mode_to_status() and makes it returning -EINVAL on invalid input.

[Mark Brown]

On Tue, Jan 10, 2012 at 12:11:33AM +0000, dd diasemi wrote:

> Making regulator_mode_to_status() returning an error allows to
> simplify its usage:
> 	ret = regulator_mode_to_status(regulator_get_mode(rdev));
> 	if (ret < 0)
> 		ret = -EIO;

That code would definitely be less than ideal - if we got an error back
from the attempt to read the mode we ought to be returning that error
not squashing it down to a single value.

Re: drivers/regulator/core.c: Fixes mapping inside regulator_mode_to_status() and makes it returning -EINVAL on invalid input.

[dd diasemi]

On Tue, Jan 10, 2012 at 12:14 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Jan 10, 2012 at 12:11:33AM +0000, dd diasemi wrote:
>
>> Making regulator_mode_to_status() returning an error allows to
>> simplify its usage:
>>       ret = regulator_mode_to_status(regulator_get_mode(rdev));
>>       if (ret < 0)
>>               ret = -EIO;
>
> That code would definitely be less than ideal - if we got an error back
> from the attempt to read the mode we ought to be returning that error
> not squashing it down to a single value.

Yes, indeed.

>> If that behaviour is deliberate, I would suggest to make it explicit:
>>       default:
>> -             return 0;
>> +             return REGULATOR_STATUS_OFF;
>
> That's not the deliberate behaviour, the deliberate behaviour is to
> return no mode if we didn't find one.

The behaviour is exactly the same in both cases, because
REGULATOR_STATUS_OFF == 0.

>From linux/regulator/driver.h:
enum regulator_status {
       REGULATOR_STATUS_OFF,
       REGULATOR_STATUS_ON,
       REGULATOR_STATUS_ERROR,
       /* fast/normal/idle/standby are flavors of "on" */
       REGULATOR_STATUS_FAST,
       REGULATOR_STATUS_NORMAL,
       REGULATOR_STATUS_IDLE,
       REGULATOR_STATUS_STANDBY,
};

So the only difference is that, the code:
       return 0;
is not obvious but still it will be interpreted as:
       return REGULATOR_STATUS_OFF;
by the caller of regulator_mode_to_status() when incorrect mode is passed.
Is it correct way to hide this behaviour rather than to make it explicit ?

And to set things up, should regulator_get_status() return negative
error code or REGULATOR_STATUS_OFF (0) on communication failure?

Re: drivers/regulator/core.c: Fixes mapping inside regulator_mode_to_status() and makes it returning -EINVAL on invalid input.

[Mark Brown]

On Tue, Jan 10, 2012 at 05:09:40PM +0000, dd diasemi wrote:
> On Tue, Jan 10, 2012 at 12:14 AM, Mark Brown

> The behaviour is exactly the same in both cases, because
> REGULATOR_STATUS_OFF == 0.

That's not really the intended behaviour...

> From linux/regulator/driver.h:
> enum regulator_status {
>        REGULATOR_STATUS_OFF,

...if it were intended we'd explicitly set the value here to be zero
here.  The intention is to return an "I don't know" value rather than an
error.

> And to set things up, should regulator_get_status() return negative
> error code or REGULATOR_STATUS_OFF (0) on communication failure?

It should return an error when the I/O fails.