[PATCH 1/2] regulator: ad5398: Fix min/max current limit boundary checking

[PATCH 1/2] regulator: ad5398: Fix min/max current limit boundary checking

[Axel Lin]

It is ok to request current limit with min_uA < chip->min_uA and
max_uA > chip->max_uA.

Signed-off-by: Axel Lin <axel.lin@gmail.com>
---
 drivers/regulator/ad5398.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
index 46d05f3..6a357dc 100644
--- a/drivers/regulator/ad5398.c
+++ b/drivers/regulator/ad5398.c
@@ -89,9 +89,7 @@ static int ad5398_set_current_limit(struct regulator_dev *rdev, int min_uA, int
 	unsigned short data;
 	int ret;
 
-	if (min_uA > chip->max_uA || min_uA < chip->min_uA)
-		return -EINVAL;
-	if (max_uA > chip->max_uA || max_uA < chip->min_uA)
+	if (min_uA > chip->max_uA || max_uA < chip->min_uA)
 		return -EINVAL;
 
 	selector = DIV_ROUND_UP((min_uA - chip->min_uA) * chip->current_level,
-- 
1.7.9.5

[PATCH 2/2] regulator: tps65217: Fix voltage boundary checking in tps65217_pmic_map_voltage

[Axel Lin]

It is ok to request voltage with min_uV < tps->info[rid]->min_uV and
max_uV > tps->info[rid]->max_uV

Signed-off-by: Axel Lin <axel.lin@gmail.com>
---
 drivers/regulator/tps65217-regulator.c |    5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/regulator/tps65217-regulator.c b/drivers/regulator/tps65217-regulator.c
index 0a3df5b..0947a09 100644
--- a/drivers/regulator/tps65217-regulator.c
+++ b/drivers/regulator/tps65217-regulator.c
@@ -206,10 +206,7 @@ static int tps65217_pmic_map_voltage(struct regulator_dev *dev,
 	if (rid < TPS65217_DCDC_1 || rid > TPS65217_LDO_4)
 		return -EINVAL;
 
-	if (min_uV < tps->info[rid]->min_uV || min_uV > tps->info[rid]->max_uV)
-		return -EINVAL;
-
-	if (max_uV < tps->info[rid]->min_uV || max_uV > tps->info[rid]->max_uV)
+	if (max_uV < tps->info[rid]->min_uV || min_uV > tps->info[rid]->max_uV)
 		return -EINVAL;
 
 	ret = tps->info[rid]->uv_to_vsel(min_uV, &sel);
-- 
1.7.9.5

RE: [PATCH 2/2] regulator: tps65217: Fix voltage boundary checking in tps65217_pmic_map_voltage

[AnilKumar, Chimata]

Hi Axel,

On Tue, Jul 03, 2012 at 08:35:55, Axel Lin wrote:
> It is ok to request voltage with min_uV < tps->info[rid]->min_uV and
> max_uV > tps->info[rid]->max_uV
> 
> Signed-off-by: Axel Lin <axel.lin@gmail.com>
> ---
>  drivers/regulator/tps65217-regulator.c |    5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/regulator/tps65217-regulator.c b/drivers/regulator/tps65217-regulator.c
> index 0a3df5b..0947a09 100644
> --- a/drivers/regulator/tps65217-regulator.c
> +++ b/drivers/regulator/tps65217-regulator.c
> @@ -206,10 +206,7 @@ static int tps65217_pmic_map_voltage(struct regulator_dev *dev,
>  	if (rid < TPS65217_DCDC_1 || rid > TPS65217_LDO_4)
>  		return -EINVAL;
>  
> -	if (min_uV < tps->info[rid]->min_uV || min_uV > tps->info[rid]->max_uV)
> -		return -EINVAL;
> -
> -	if (max_uV < tps->info[rid]->min_uV || max_uV > tps->info[rid]->max_uV)
> +	if (max_uV < tps->info[rid]->min_uV || min_uV > tps->info[rid]->max_uV)
>  		return -EINVAL;

Case 1:-
------

tps->info[rid]->min_uV = 600000;
tps->info[rid]->max_uV = 1100000;

If we do regulator_set_voltage(reg, 550000, 1100000);

This results into error condition and how can we handle with this?

Case 2:-
------

I think the current code handles this case as well.

There might be a case where board/DT data is false like

tps->info[rid]->min_uV = 1100000;
tps->info[rid]->max_uV = 600000;

regulator_set_voltage(reg, 650000, 1100000);

Regards
AnilKumar

RE: [PATCH 2/2] regulator: tps65217: Fix voltage boundary checking in tps65217_pmic_map_voltage

[Axel Lin]

> Case 1:-
> ------
> 
> tps->info[rid]->min_uV = 600000;
> tps->info[rid]->max_uV = 1100000;
> 
> If we do regulator_set_voltage(reg, 550000, 1100000);
> 
> This results into error condition and how can we handle with this?

In original code, it returns error. ( I think which is wrong.)

What we want is to set the smallest voltage within the specified voltage
range.
With this patch, calling regulator_set_voltage(reg, 550000, 1100000);
is the same as calling regulator_set_voltage(reg, 600000, 1100000);
It will set the voltage to 600000 uV.

> 
> Case 2:-
> ------
> 
> I think the current code handles this case as well.
> 
> There might be a case where board/DT data is false like
> 
> tps->info[rid]->min_uV = 1100000;
> tps->info[rid]->max_uV = 600000;
> 
I don't get it.
You mean the case min_uV is greater than max_uV?

My understanding of current implementation is that the
tps->info[rid]->min_uV and tps->info[rid]->max_uV are not controlled by
board/DT data. They are defined in tps65217_pmic_regs[].

> regulator_set_voltage(reg, 650000, 1100000);
> 
> Regards
> AnilKumar

RE: [PATCH 2/2] regulator: tps65217: Fix voltage boundary checking in tps65217_pmic_map_voltage

[AnilKumar, Chimata]

On Tue, Jul 03, 2012 at 10:56:08, Axel Lin wrote:
> 
> > Case 1:-
> > ------
> > 
> > tps->info[rid]->min_uV = 600000;
> > tps->info[rid]->max_uV = 1100000;
> > 
> > If we do regulator_set_voltage(reg, 550000, 1100000);
> > 
> > This results into error condition and how can we handle with this?
> 
> In original code, it returns error. ( I think which is wrong.)
> 
> What we want is to set the smallest voltage within the specified voltage
> range.
> With this patch, calling regulator_set_voltage(reg, 550000, 1100000);
> is the same as calling regulator_set_voltage(reg, 600000, 1100000);
> It will set the voltage to 600000 uV.

Suppose
rdev->constraints->min_uV = 600000; /* wrong data entered by user as DT/plat
						data */
rdev->constraints->max_uV = 1500000;

And tps limits (little modification from my previous mail "actual values")

tps->info[rid]->min_uV = 900000;
tps->info[rid]->max_uV = 1800000;

regulator_set_voltage(reg, 850000, 1100000);

In side tps65217_pmic_map_voltage() we have

tps->info[rid]->uv_to_vsel(850000, &sel) nothing but
tps65217_uv_to_vsel1(850000, &sel);

sel = (((850000 - 900000) + (25000) - 1) / (25000));
sel = ((-25001)/(25000));
sel = -1; /* Which is not expected */

Ideally this would be the change

	if (min_uV < tps->info[rid]->min_uV)
		min_uV = tps->info[rid]->min_uV;

	if (max_uV > tps->info[rid]->max_uV)
		max_uV = tps->info[rid]->max_uV;

	if (max_uV < tps->info[rid]->min_uV ||
			min_uV > tps->info[rid]->max_uV)
		return -EINVAL;

If this is not the case then I am completely in a wrong direction.

> 
> > 
> > Case 2:-
> > ------
> > 
> > I think the current code handles this case as well.
> > 
> > There might be a case where board/DT data is false like
> > 
> > tps->info[rid]->min_uV = 1100000;
> > tps->info[rid]->max_uV = 600000;
> > 
> I don't get it.
> You mean the case min_uV is greater than max_uV?
> 
> My understanding of current implementation is that the
> tps->info[rid]->min_uV and tps->info[rid]->max_uV are not controlled by
> board/DT data. They are defined in tps65217_pmic_regs[].

My point here is rdev->constraints->min_uV and rdev->constraints->max_uV
values set from DT/board file. Sorry about the confusion.
This case is taken care by regulator core.

Regards
AnilKumar

RE: [PATCH 2/2] regulator: tps65217: Fix voltage boundary checking in tps65217_pmic_map_voltage

[Axel Lin]

> In side tps65217_pmic_map_voltage() we have
> 
> tps->info[rid]->uv_to_vsel(850000, &sel) nothing but
> tps65217_uv_to_vsel1(850000, &sel);
> 
> sel = (((850000 - 900000) + (25000) - 1) / (25000));
> sel = ((-25001)/(25000));
> sel = -1; /* Which is not expected */
> 
> Ideally this would be the change
> 
> 	if (min_uV < tps->info[rid]->min_uV)
> 		min_uV = tps->info[rid]->min_uV;
> 
ok. I got your point now.

So if min_uV < tps->info[rid]->min_uV, we need to set
min_uV = tps->info[rid]->min_uV.
This is because the equation we used in uv_to_vsel() does not allow
min_uV < tps->info[rid]->min_uV, otherwise it returns negative selector.


> 	if (max_uV > tps->info[rid]->max_uV)
> 		max_uV = tps->info[rid]->max_uV;

This looks not necessary. the equation in uv_to_vsel() does not use
max_uV.

> 
> 	if (max_uV < tps->info[rid]->min_uV ||
> 			min_uV > tps->info[rid]->max_uV)
> 		return -EINVAL;
> 
> If this is not the case then I am completely in a wrong direction.
You are right in this case. I'll send a v2 for review.

Thanks for the review,
Axel

RE: [PATCH 2/2] regulator: tps65217: Fix voltage boundary checking in tps65217_pmic_map_voltage

[AnilKumar, Chimata]

On Tue, Jul 03, 2012 at 12:39:45, Axel Lin wrote:
> 
> > In side tps65217_pmic_map_voltage() we have
> > 
> > tps->info[rid]->uv_to_vsel(850000, &sel) nothing but
> > tps65217_uv_to_vsel1(850000, &sel);
> > 
> > sel = (((850000 - 900000) + (25000) - 1) / (25000));
> > sel = ((-25001)/(25000));
> > sel = -1; /* Which is not expected */
> > 
> > Ideally this would be the change
> > 
> > 	if (min_uV < tps->info[rid]->min_uV)
> > 		min_uV = tps->info[rid]->min_uV;
> > 
> ok. I got your point now.
> 
> So if min_uV < tps->info[rid]->min_uV, we need to set
> min_uV = tps->info[rid]->min_uV.
> This is because the equation we used in uv_to_vsel() does not allow
> min_uV < tps->info[rid]->min_uV, otherwise it returns negative selector.
> 
> 
> > 	if (max_uV > tps->info[rid]->max_uV)
> > 		max_uV = tps->info[rid]->max_uV;
> 
> This looks not necessary. the equation in uv_to_vsel() does not use
> max_uV.
> 

Yes, I agree.

Regards
AnilKumar