RE: [PATCH v2] regulator: ad5398: Fix min/max current limitboundary checking
From: Zhang, Sonic
Date: Tue Jul 03 2012 - 04:13:36 EST
>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
>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
>>> Subject: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary
>>> 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.
> chip-min chip-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.
>>> 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,