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:24 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 10:13 AM, Zhang, Sonic wrote:
>>
>>
>>> -----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?
>
>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@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Ç+‰·Ÿ®‰­†+%ŠËlzwm…ébëæìr¸›zX§»®w¥Š{ayºÊÚë,j­¢f£¢·hš‹àz¹®w¥¢¸¢·¦j:+v‰¨ŠwèjØm¶Ÿÿ¾«‘êçzZ+ƒùšŽŠÝj"ú!¶iO•æ¬z·švØ^¶m§ÿðÃnÆàþY&—