[RFC][PATCH] regulator: Fix _regulator_get_voltage if get_voltage callback is NULL

[RFC][PATCH] regulator: Fix _regulator_get_voltage if get_voltage callback is NULL

[Axel Lin]

In the case of get_voltage callback is NULL, current implementation in
_regulator_get_voltage will return -EINVAL.

Also returns proper error if ret is negative value.

Signed-off-by: Axel Lin <axel.lin@gmail.com>
---
hi Mark,
I just found in commit 6b8e70 "regulator: Convert WM8400 to get_voltage_sel()",
you added get_voltage_sel callback and remove get_voltage callback.
I suspect you will hit this bug. Can you check it?

Thanks,
Axel

 drivers/regulator/core.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9493f61..2262474 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1886,13 +1886,15 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
 		if (sel < 0)
 			return sel;
 		ret = rdev->desc->ops->list_voltage(rdev, sel);
+		goto out;
 	}
 	if (rdev->desc->ops->get_voltage)
 		ret = rdev->desc->ops->get_voltage(rdev);
 	else
 		return -EINVAL;
 
-	return ret - rdev->constraints->uV_offset;
+out:
+	return ret < 0 ? ret : ret - rdev->constraints->uV_offset;
 }
 
 /**
-- 
1.7.1

Re: [RFC][PATCH] regulator: Fix _regulator_get_voltage if get_voltage callback is NULL

[Mark Brown]

On Wed, May 18, 2011 at 09:01:58PM +0800, Axel Lin wrote:

> @@ -1886,13 +1886,15 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
>  		if (sel < 0)
>  			return sel;
>  		ret = rdev->desc->ops->list_voltage(rdev, sel);
> +		goto out;
>  	}

Why are you adding this goto?  It's not going to change anything...

Re: [RFC][PATCH] regulator: Fix _regulator_get_voltage if get_voltage callback is NULL

[Axel Lin]

2011/5/19 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Wed, May 18, 2011 at 09:01:58PM +0800, Axel Lin wrote:
>
>> @@ -1886,13 +1886,15 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
>>               if (sel < 0)
>>                       return sel;
>>               ret = rdev->desc->ops->list_voltage(rdev, sel);
>> +             goto out;
>>       }
>
> Why are you adding this goto?  It's not going to change anything...
>
no matter what ret value returns from rdev->desc->ops->list_voltage(rdev, sel);

In the case of rdev->desc->ops->get_voltage is NULL,
current implementation will always return -EINVAL;

Regards,
Axel

Re: [RFC][PATCH] regulator: Fix _regulator_get_voltage if get_voltage callback is NULL

[Axel Lin]

2011/5/19 Axel Lin <axel.lin@gmail.com>:
> 2011/5/19 Mark Brown <broonie@opensource.wolfsonmicro.com>:
>> On Wed, May 18, 2011 at 09:01:58PM +0800, Axel Lin wrote:
>>
>>> @@ -1886,13 +1886,15 @@ static int _regulator_get_voltage(struct regulator_dev *rdev)
>>>               if (sel < 0)
>>>                       return sel;
>>>               ret = rdev->desc->ops->list_voltage(rdev, sel);
>>> +             goto out;
>>>       }
>>
>> Why are you adding this goto?  It's not going to change anything...
>>
> no matter what ret value returns from rdev->desc->ops->list_voltage(rdev, sel);
>
> In the case of rdev->desc->ops->get_voltage is NULL,
> current implementation will always return -EINVAL;
>
hi Mark,
I'm thinking maybe you prefer changes like below:
How do you think?

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 9493f61..2a2e927 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1887,12 +1887,12 @@ static int _regulator_get_voltage(struct
regulator_dev *rdev)
                        return sel;
                ret = rdev->desc->ops->list_voltage(rdev, sel);
        }
-       if (rdev->desc->ops->get_voltage)
+       else if (rdev->desc->ops->get_voltage)
                ret = rdev->desc->ops->get_voltage(rdev);
        else
                return -EINVAL;

-       return ret - rdev->constraints->uV_offset;
+       return ret < 0 ? ret : ret - rdev->constraints->uV_offset;
 }

 /**


Regards,
Axel

Re: [RFC][PATCH] regulator: Fix _regulator_get_voltage if get_voltage callback is NULL

[Mark Brown]

On Thu, May 19, 2011 at 05:58:06PM +0800, Axel Lin wrote:

> I'm thinking maybe you prefer changes like below:
> How do you think?

Yes, something like that.  The goto is obscure and hard to read.