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

[PATCH v2] 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.

We need to set min_uA = chip->min_uA if (min_uA < chip->min_uA),
this ensures the equation to calcuate selator does not return negative number.

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

diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
index 46d05f3..84fdcda 100644
--- a/drivers/regulator/ad5398.c
+++ b/drivers/regulator/ad5398.c
@@ -89,9 +89,10 @@ 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->min_uA)
+		min_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

RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking

[Zhang, Sonic]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1688 bytes --]



>-----Original Message-----
>From: Axel Lin [mailto:axel.lin@gmail.com]
>Sent: Tuesday, July 03, 2012 3:43 PM
>To: Mark Brown
>Cc: linux-kernel@vger.kernel.org; Zhang, Sonic; Lars-Peter Clausen; Liam
>Girdwood
>Subject: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>checking
>
>It is ok to request current limit with min_uA < chip->min_uA and
>max_uA > chip->max_uA.
>
>We need to set min_uA = chip->min_uA if (min_uA < chip->min_uA),
>this ensures the equation to calcuate selator does not return negative number.
>

You should not do it in driver. Set a correct min_uA value in your application.

Regards,

Sonic

>Signed-off-by: Axel Lin <axel.lin@gmail.com>
>~
>---
> drivers/regulator/ad5398.c |    7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
>index 46d05f3..84fdcda 100644
>--- a/drivers/regulator/ad5398.c
>+++ b/drivers/regulator/ad5398.c
>@@ -89,9 +89,10 @@ 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->min_uA)
>+              min_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
>
>
>

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking

[Lars-Peter Clausen]

On 07/03/2012 09:54 AM, Zhang, Sonic wrote:
> 
> 
>> -----Original Message-----
>> From: Axel Lin [mailto:axel.lin@gmail.com]
>> Sent: Tuesday, July 03, 2012 3:43 PM
>> To: Mark Brown
>> Cc: linux-kernel@vger.kernel.org; Zhang, Sonic; Lars-Peter Clausen; Liam
>> Girdwood
>> Subject: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>> checking
>>
>> It is ok to request current limit with min_uA < chip->min_uA and
>> max_uA > chip->max_uA.
>>
>> We need to set min_uA = chip->min_uA if (min_uA < chip->min_uA),
>> this ensures the equation to calcuate selator does not return negative number.
>>
> 
> You should not do it in driver. Set a correct min_uA value in your application.

I think the patch makes sense. If a application request a current range
which overlaps with the range support by the chip, but either the requested
min is smaller than the supported min or the requested max is larger than
the supported max the driver will fail with an error. E.g.

req-min     req-max
   |-----------|
        |------------|
     chip-min  chip-max

or even

req-min                req-max
   |----------------------|
        |------------|
     chip-min  chip-max


While it is obviously possible for the chip to fulfill this request.
Axel's patch takes care of this situation and ensures that the request is
satisfied and the output current is set to a current within the requested
range and the supported range.

- Lars

> 
> Regards,
> 
> Sonic
> 
>> Signed-off-by: Axel Lin <axel.lin@gmail.com>
>> ~
>> ---
>> drivers/regulator/ad5398.c |    7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
>> index 46d05f3..84fdcda 100644
>> --- a/drivers/regulator/ad5398.c
>> +++ b/drivers/regulator/ad5398.c
>> @@ -89,9 +89,10 @@ 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->min_uA)
>> +              min_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
>>
>>
>>
> 

RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking

[Zhang, Sonic]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3139 bytes --]



>-----Original Message-----
>From: Lars-Peter Clausen [mailto:lars@metafoo.de]
>Sent: Tuesday, July 03, 2012 4:06 PM
>To: Zhang, Sonic
>Cc: Axel Lin; Mark Brown; linux-kernel@vger.kernel.org; Liam Girdwood
>Subject: Re: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>checking
>
>On 07/03/2012 09:54 AM, Zhang, Sonic wrote:
>>
>>
>>> -----Original Message-----
>>> From: Axel Lin [mailto:axel.lin@gmail.com]
>>> Sent: Tuesday, July 03, 2012 3:43 PM
>>> To: Mark Brown
>>> Cc: linux-kernel@vger.kernel.org; Zhang, Sonic; Lars-Peter Clausen; Liam
>>> Girdwood
>>> Subject: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>>> checking
>>>
>>> It is ok to request current limit with min_uA < chip->min_uA and
>>> max_uA > chip->max_uA.
>>>
>>> We need to set min_uA = chip->min_uA if (min_uA < chip->min_uA),
>>> this ensures the equation to calcuate selator does not return negative number.
>>>
>>
>> You should not do it in driver. Set a correct min_uA value in your application.
>
>I think the patch makes sense. If a application request a current range
>which overlaps with the range support by the chip, but either the requested
>min is smaller than the supported min or the requested max is larger than
>the supported max the driver will fail with an error. E.g.
>
>req-min     req-max
>   |-----------|
>        |------------|
>     chip-min  chip-max
>
>or even
>
>req-min                req-max
>   |----------------------|
>        |------------|
>     chip-min  chip-max
>
>
>While it is obviously possible for the chip to fulfill this request.
>Axel's patch takes care of this situation and ensures that the request is
>satisfied and the output current is set to a current within the requested
>range and the supported range.

If the requested minimum current is smaller than the capability of the hardware, does a bigger min value fulfill this request?

If this logic is correct, I am fine to ACK.


Regards,

Sonic


>
>- Lars
>
>>
>> Regards,
>>
>> Sonic
>>
>>> Signed-off-by: Axel Lin <axel.lin@gmail.com>
>>> ~
>>> ---
>>> drivers/regulator/ad5398.c |    7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
>>> index 46d05f3..84fdcda 100644
>>> --- a/drivers/regulator/ad5398.c
>>> +++ b/drivers/regulator/ad5398.c
>>> @@ -89,9 +89,10 @@ 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->min_uA)
>>> +              min_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
>>>
>>>
>>>
>>
>

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking

[Lars-Peter Clausen]

On 07/03/2012 10:13 AM, Zhang, Sonic wrote:
> 
> 
>> -----Original Message-----
>> From: Lars-Peter Clausen [mailto:lars@metafoo.de]
>> Sent: Tuesday, July 03, 2012 4:06 PM
>> To: Zhang, Sonic
>> Cc: Axel Lin; Mark Brown; linux-kernel@vger.kernel.org; Liam Girdwood
>> Subject: Re: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>> checking
>>
>> On 07/03/2012 09:54 AM, Zhang, Sonic wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Axel Lin [mailto:axel.lin@gmail.com]
>>>> Sent: Tuesday, July 03, 2012 3:43 PM
>>>> To: Mark Brown
>>>> Cc: linux-kernel@vger.kernel.org; Zhang, Sonic; Lars-Peter Clausen; Liam
>>>> Girdwood
>>>> Subject: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>>>> checking
>>>>
>>>> It is ok to request current limit with min_uA < chip->min_uA and
>>>> max_uA > chip->max_uA.
>>>>
>>>> We need to set min_uA = chip->min_uA if (min_uA < chip->min_uA),
>>>> this ensures the equation to calcuate selator does not return negative number.
>>>>
>>>
>>> You should not do it in driver. Set a correct min_uA value in your application.
>>
>> I think the patch makes sense. If a application request a current range
>> which overlaps with the range support by the chip, but either the requested
>> min is smaller than the supported min or the requested max is larger than
>> the supported max the driver will fail with an error. E.g.
>>
>> req-min     req-max
>>   |-----------|
>>        |------------|
>>     chip-min  chip-max
>>
>> or even
>>
>> req-min                req-max
>>   |----------------------|
>>        |------------|
>>     chip-min  chip-max
>>
>>
>> While it is obviously possible for the chip to fulfill this request.
>> Axel's patch takes care of this situation and ensures that the request is
>> satisfied and the output current is set to a current within the requested
>> range and the supported range.
> 
> If the requested minimum current is smaller than the capability of the hardware, does a bigger min value fulfill this request?

As long as it is smaller than the maximum requested current, yes. You
request a current range with the regulator API and any value within this
range is fine as the actual output current.

> 
> If this logic is correct, I am fine to ACK.
> 
> 
> Regards,
> 
> Sonic
> 
> 
>>
>> - Lars
>>
>>>
>>> Regards,
>>>
>>> Sonic
>>>
>>>> Signed-off-by: Axel Lin <axel.lin@gmail.com>
>>>> ~
>>>> ---
>>>> drivers/regulator/ad5398.c |    7 ++++---
>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
>>>> index 46d05f3..84fdcda 100644
>>>> --- a/drivers/regulator/ad5398.c
>>>> +++ b/drivers/regulator/ad5398.c
>>>> @@ -89,9 +89,10 @@ 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->min_uA)
>>>> +              min_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
>>>>
>>>>
>>>>
>>>
>>
> 

RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking

[Zhang, Sonic]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3971 bytes --]



>-----Original Message-----
>From: Lars-Peter Clausen [mailto:lars@metafoo.de]
>Sent: Tuesday, July 03, 2012 4:24 PM
>To: Zhang, Sonic
>Cc: Axel Lin; Mark Brown; linux-kernel@vger.kernel.org; Liam Girdwood
>Subject: Re: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>checking
>
>On 07/03/2012 10:13 AM, Zhang, Sonic wrote:
>>
>>
>>> -----Original Message-----
>>> From: Lars-Peter Clausen [mailto:lars@metafoo.de]
>>> Sent: Tuesday, July 03, 2012 4:06 PM
>>> To: Zhang, Sonic
>>> Cc: Axel Lin; Mark Brown; linux-kernel@vger.kernel.org; Liam Girdwood
>>> Subject: Re: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>>> checking
>>>
>>> On 07/03/2012 09:54 AM, Zhang, Sonic wrote:
>>>>
>>>>
>>>>> -----Original Message-----
>>>>> From: Axel Lin [mailto:axel.lin@gmail.com]
>>>>> Sent: Tuesday, July 03, 2012 3:43 PM
>>>>> To: Mark Brown
>>>>> Cc: linux-kernel@vger.kernel.org; Zhang, Sonic; Lars-Peter Clausen; Liam
>>>>> Girdwood
>>>>> Subject: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>>>>> checking
>>>>>
>>>>> It is ok to request current limit with min_uA < chip->min_uA and
>>>>> max_uA > chip->max_uA.
>>>>>
>>>>> We need to set min_uA = chip->min_uA if (min_uA < chip->min_uA),
>>>>> this ensures the equation to calcuate selator does not return negative
>number.
>>>>>
>>>>
>>>> You should not do it in driver. Set a correct min_uA value in your application.
>>>
>>> I think the patch makes sense. If a application request a current range
>>> which overlaps with the range support by the chip, but either the requested
>>> min is smaller than the supported min or the requested max is larger than
>>> the supported max the driver will fail with an error. E.g.
>>>
>>> req-min     req-max
>>>   |-----------|
>>>        |------------|
>>>     chip-min  chip-max
>>>
>>> or even
>>>
>>> req-min                req-max
>>>   |----------------------|
>>>        |------------|
>>>     chip-min  chip-max
>>>
>>>
>>> While it is obviously possible for the chip to fulfill this request.
>>> Axel's patch takes care of this situation and ensures that the request is
>>> satisfied and the output current is set to a current within the requested
>>> range and the supported range.
>>
>> If the requested minimum current is smaller than the capability of the hardware,
>does a bigger min value fulfill this request?
>
>As long as it is smaller than the maximum requested current, yes. You
>request a current range with the regulator API and any value within this
>range is fine as the actual output current.
>

If so, please also set the max_uA as well.

Sonic

>>
>> If this logic is correct, I am fine to ACK.
>>
>>
>> Regards,
>>
>> Sonic
>>
>>
>>>
>>> - Lars
>>>
>>>>
>>>> Regards,
>>>>
>>>> Sonic
>>>>
>>>>> Signed-off-by: Axel Lin <axel.lin@gmail.com>
>>>>> ~
>>>>> ---
>>>>> drivers/regulator/ad5398.c |    7 ++++---
>>>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/regulator/ad5398.c b/drivers/regulator/ad5398.c
>>>>> index 46d05f3..84fdcda 100644
>>>>> --- a/drivers/regulator/ad5398.c
>>>>> +++ b/drivers/regulator/ad5398.c
>>>>> @@ -89,9 +89,10 @@ 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->min_uA)
>>>>> +              min_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
>>>>>
>>>>>
>>>>>
>>>>
>>>
>>
>

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking

[Axel Lin]

>>As long as it is smaller than the maximum requested current, yes. You
>>request a current range with the regulator API and any value within this
>>range is fine as the actual output current.
>>
>
> If so, please also set the max_uA as well.

The equation to calculate the selector does not depend on max_uA.
So I think we don't need to set the requested max_uA.

Current does check the selected current by:
if (ad5398_calc_current(chip, selector) > max_uA)
        return -EINVAL;

This ensures the selected current falls within specified range.

Regards,
Axel

RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking

[Zhang, Sonic]

>-----Original Message-----
>From: Axel Lin [mailto:axel.lin@gmail.com]
>Sent: Tuesday, July 03, 2012 5:44 PM
>To: Zhang, Sonic
>Cc: Lars-Peter Clausen; Mark Brown; linux-kernel@vger.kernel.org; Liam
>Girdwood
>Subject: Re: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>checking
>
>>>As long as it is smaller than the maximum requested current, yes. You
>>>request a current range with the regulator API and any value within this
>>>range is fine as the actual output current.
>>>
>>
>> If so, please also set the max_uA as well.
>
>The equation to calculate the selector does not depend on max_uA.
>So I think we don't need to set the requested max_uA.
>

But, ad5398_set_current_limit() behaves different for min_uA and max_uA with you patch. Is this expected?

Sonic

>Current does check the selected current by:
>if (ad5398_calc_current(chip, selector) > max_uA)
>        return -EINVAL;
>
>This ensures the selected current falls within specified range.
>
>Regards,
>Axel

RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking

[Axel Lin]

> >The equation to calculate the selector does not depend on max_uA.
> >So I think we don't need to set the requested max_uA.
> >
> 
> But, ad5398_set_current_limit() behaves different for min_uA and max_uA with you patch. Is this expected?
> 

What we want is to set the smallest current supported by this hardware
within the range you requested.

Current code uses below equation to choose selector:
selector = DIV_ROUND_UP((min_uA - chip->min_uA) * chip->current_level,
                        range_uA);

With this equation, we need to ensure min_uA >= chip->min_uA,
otherwise it returns a negative selector.

That is why we need to add:
if (min_uA < chip->min_uA)
        min_uA = chip->min_uA;


Axel

RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking

[Zhang, Sonic]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1342 bytes --]



>-----Original Message-----
>From: Axel Lin [mailto:axel.lin@gmail.com]
>Sent: Tuesday, July 03, 2012 7:36 PM
>To: Zhang, Sonic
>Cc: Lars-Peter Clausen; Mark Brown; linux-kernel@vger.kernel.org; Liam
>Girdwood
>Subject: RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>checking
>
>
>> >The equation to calculate the selector does not depend on max_uA.
>> >So I think we don't need to set the requested max_uA.
>> >
>>
>> But, ad5398_set_current_limit() behaves different for min_uA and max_uA with
>you patch. Is this expected?
>>
>
>What we want is to set the smallest current supported by this hardware
>within the range you requested.
>
>Current code uses below equation to choose selector:
>selector = DIV_ROUND_UP((min_uA - chip->min_uA) * chip->current_level,
>                        range_uA);
>
>With this equation, we need to ensure min_uA >= chip->min_uA,
>otherwise it returns a negative selector.
>
>That is why we need to add:
>if (min_uA < chip->min_uA)
>        min_uA = chip->min_uA;
>

Yes, but if you apply this logic to min_uA, you should apply the same logic to max_uA, even though it is not used in your application.


Sonic

>
>Axel
>

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking

[Axel Lin]

> >That is why we need to add:
> >if (min_uA < chip->min_uA)
> >        min_uA = chip->min_uA;
> >
> 
> Yes, but if you apply this logic to min_uA, you should apply the same logic to max_uA, even though it is not used in your application.

Actually, the logic is the same:
to find a supported (minmal) current in specified range.

The question is the equation used in current code does not allow
min_uA < chip->min_uA.
Setting min_uA = chip->min_uA if min_uA < chip->min_uA does make sense
because botch request actually returns the same current value.
( I mean no user visible change )

Adding below logic is not necessary.
( Note: Adding this or not does not have any user visible change, it's
just not necessary)

if (max_uA > chip->min_uA)
        max_uA = chip->max_uA;

It is not necessary because the equation to choose selector does not
depends on max_uA. No matter if we set max_uA = chip->max_uA or not in
this case, it does not impact the equation to choose the selector.

But, well, if you really prefer adding it. I'll send a v3 for it.
Just let me know how do you think.

Regards,
Axel

RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking

[Zhang, Sonic]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1630 bytes --]



>-----Original Message-----
>From: Axel Lin [mailto:axel.lin@gmail.com]
>Sent: Wednesday, July 04, 2012 11:19 AM
>To: Zhang, Sonic
>Cc: Lars-Peter Clausen; Mark Brown; linux-kernel@vger.kernel.org; Liam
>Girdwood
>Subject: RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>checking
>
>
>> >That is why we need to add:
>> >if (min_uA < chip->min_uA)
>> >        min_uA = chip->min_uA;
>> >
>>
>> Yes, but if you apply this logic to min_uA, you should apply the same logic to
>max_uA, even though it is not used in your application.
>
>Actually, the logic is the same:
>to find a supported (minmal) current in specified range.
>
>The question is the equation used in current code does not allow
>min_uA < chip->min_uA.
>Setting min_uA = chip->min_uA if min_uA < chip->min_uA does make sense
>because botch request actually returns the same current value.
>( I mean no user visible change )
>
>Adding below logic is not necessary.
>( Note: Adding this or not does not have any user visible change, it's
>just not necessary)
>
>if (max_uA > chip->min_uA)
>        max_uA = chip->max_uA;
>
>It is not necessary because the equation to choose selector does not
>depends on max_uA. No matter if we set max_uA = chip->max_uA or not in
>this case, it does not impact the equation to choose the selector.
>
>But, well, if you really prefer adding it. I'll send a v3 for it.
>Just let me know how do you think.

Yes, v3 please.

Sonic

>
>Regards,
>Axel
>
>

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥