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

[Lars-Peter Clausen <lars@metafoo.de>]

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
>>>>
>>>>
>>>>
>>>
>>
>