* [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking @ 2012-07-03 7:42 Axel Lin 2012-07-03 7:54 ` Zhang, Sonic 0 siblings, 1 reply; 12+ messages in thread From: Axel Lin @ 2012-07-03 7:42 UTC (permalink / raw) To: Mark Brown; +Cc: linux-kernel, Sonic Zhang, Lars-Peter Clausen, Liam Girdwood 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking 2012-07-03 7:42 [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking Axel Lin @ 2012-07-03 7:54 ` Zhang, Sonic 2012-07-03 8:06 ` Lars-Peter Clausen 0 siblings, 1 reply; 12+ messages in thread From: Zhang, Sonic @ 2012-07-03 7:54 UTC (permalink / raw) To: Axel Lin, Mark Brown Cc: linux-kernel@vger.kernel.org, Lars-Peter Clausen, Liam Girdwood [-- 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¥ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking 2012-07-03 7:54 ` Zhang, Sonic @ 2012-07-03 8:06 ` Lars-Peter Clausen 2012-07-03 8:13 ` Zhang, Sonic 0 siblings, 1 reply; 12+ messages in thread From: Lars-Peter Clausen @ 2012-07-03 8:06 UTC (permalink / raw) To: Zhang, Sonic Cc: Axel Lin, Mark Brown, linux-kernel@vger.kernel.org, Liam Girdwood 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 >> >> >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking 2012-07-03 8:06 ` Lars-Peter Clausen @ 2012-07-03 8:13 ` Zhang, Sonic 2012-07-03 8:24 ` Lars-Peter Clausen 0 siblings, 1 reply; 12+ messages in thread From: Zhang, Sonic @ 2012-07-03 8:13 UTC (permalink / raw) To: Lars-Peter Clausen Cc: Axel Lin, Mark Brown, linux-kernel@vger.kernel.org, Liam Girdwood [-- 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¥ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking 2012-07-03 8:13 ` Zhang, Sonic @ 2012-07-03 8:24 ` Lars-Peter Clausen 2012-07-03 8:33 ` Zhang, Sonic 0 siblings, 1 reply; 12+ messages in thread From: Lars-Peter Clausen @ 2012-07-03 8:24 UTC (permalink / raw) To: Zhang, Sonic Cc: Axel Lin, Mark Brown, linux-kernel@vger.kernel.org, Liam Girdwood 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 >>>> >>>> >>>> >>> >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking 2012-07-03 8:24 ` Lars-Peter Clausen @ 2012-07-03 8:33 ` Zhang, Sonic 2012-07-03 9:44 ` Axel Lin 0 siblings, 1 reply; 12+ messages in thread From: Zhang, Sonic @ 2012-07-03 8:33 UTC (permalink / raw) To: Lars-Peter Clausen Cc: Axel Lin, Mark Brown, linux-kernel@vger.kernel.org, Liam Girdwood [-- 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¥ ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking 2012-07-03 8:33 ` Zhang, Sonic @ 2012-07-03 9:44 ` Axel Lin 2012-07-03 9:51 ` Zhang, Sonic 0 siblings, 1 reply; 12+ messages in thread From: Axel Lin @ 2012-07-03 9:44 UTC (permalink / raw) To: Zhang, Sonic Cc: Lars-Peter Clausen, Mark Brown, linux-kernel@vger.kernel.org, Liam Girdwood >>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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking 2012-07-03 9:44 ` Axel Lin @ 2012-07-03 9:51 ` Zhang, Sonic 2012-07-03 11:36 ` Axel Lin 0 siblings, 1 reply; 12+ messages in thread From: Zhang, Sonic @ 2012-07-03 9:51 UTC (permalink / raw) To: axel.lin@gmail.com Cc: Lars-Peter Clausen, Mark Brown, linux-kernel@vger.kernel.org, Liam Girdwood >-----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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking 2012-07-03 9:51 ` Zhang, Sonic @ 2012-07-03 11:36 ` Axel Lin 2012-07-04 2:56 ` Zhang, Sonic 0 siblings, 1 reply; 12+ messages in thread From: Axel Lin @ 2012-07-03 11:36 UTC (permalink / raw) To: Zhang, Sonic Cc: Lars-Peter Clausen, Mark Brown, linux-kernel@vger.kernel.org, Liam Girdwood > >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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking 2012-07-03 11:36 ` Axel Lin @ 2012-07-04 2:56 ` Zhang, Sonic 2012-07-04 3:18 ` Axel Lin 0 siblings, 1 reply; 12+ messages in thread From: Zhang, Sonic @ 2012-07-04 2:56 UTC (permalink / raw) To: Axel Lin Cc: Lars-Peter Clausen, Mark Brown, linux-kernel@vger.kernel.org, Liam Girdwood [-- 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¥ ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking 2012-07-04 2:56 ` Zhang, Sonic @ 2012-07-04 3:18 ` Axel Lin 2012-07-04 3:44 ` Zhang, Sonic 0 siblings, 1 reply; 12+ messages in thread From: Axel Lin @ 2012-07-04 3:18 UTC (permalink / raw) To: Zhang, Sonic Cc: Lars-Peter Clausen, Mark Brown, linux-kernel@vger.kernel.org, Liam Girdwood > >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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* RE: [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking 2012-07-04 3:18 ` Axel Lin @ 2012-07-04 3:44 ` Zhang, Sonic 0 siblings, 0 replies; 12+ messages in thread From: Zhang, Sonic @ 2012-07-04 3:44 UTC (permalink / raw) To: Axel Lin Cc: Lars-Peter Clausen, Mark Brown, linux-kernel@vger.kernel.org, Liam Girdwood [-- 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¥ ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-07-04 3:45 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-07-03 7:42 [PATCH v2] regulator: ad5398: Fix min/max current limit boundary checking Axel Lin 2012-07-03 7:54 ` Zhang, Sonic 2012-07-03 8:06 ` Lars-Peter Clausen 2012-07-03 8:13 ` Zhang, Sonic 2012-07-03 8:24 ` Lars-Peter Clausen 2012-07-03 8:33 ` Zhang, Sonic 2012-07-03 9:44 ` Axel Lin 2012-07-03 9:51 ` Zhang, Sonic 2012-07-03 11:36 ` Axel Lin 2012-07-04 2:56 ` Zhang, Sonic 2012-07-04 3:18 ` Axel Lin 2012-07-04 3:44 ` Zhang, Sonic
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox