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

[Zhang, Sonic]

>-----Original Message-----
>From: Lars-Peter Clausen [mailto:lars@xxxxxxxxxx]
>Sent: Tuesday, July 03, 2012 4:06 PM
>To: Zhang, Sonic
>Cc: Axel Lin; Mark Brown; linux-kernel@xxxxxxxxxxxxxxx; 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@xxxxxxxxx]
>>> Sent: Tuesday, July 03, 2012 3:43 PM
>>> To: Mark Brown
>>> Cc: linux-kernel@xxxxxxxxxxxxxxx; 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@xxxxxxxxx>
>>> ~
>>> ---
>>> 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‹§²æìr¸›yúèšØb²X¬¶ÇvØ^–)Þ{.nÇ+‰·¥Š{±‘êçzX§¶›¡Ü}©ž²ÆzÚ&j:+v‰¨¾«‘êçzZ+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹®w¥¢¸?™¨è­Ú&¢)ßf”ù^jÇy§m…á@A«a¶Úÿ0¶ìh®å’i