Messages in this thread | | | From | Axel Lin <> | Date | Wed, 9 Jun 2021 11:32:30 +0800 | Subject | Re: [PATCH] regulator: rt6160: Convert to use regulator_set_ramp_delay_regmap |
| |
cy_huang(黃啟原) <cy_huang@richtek.com> 於 2021年6月4日 週五 下午2:29寫道: > > > 於 五,2021-06-04 於 13:59 +0800,Axel Lin 提到: > > cy_huang(黃啟原) <cy_huang@richtek.com> 於 2021年6月4日 週五 下午1:48寫道: > > > > > 於 五,2021-06-04 於 11:30 +0800,Axel Lin 提到: > > cy_huang(黃啟原) <cy_huang@richtek.com> 於 2021年6月4日 週五 上午10:26寫道: > > > > > cy_huang(黃啟原) <cy_huang@richtek.com> 於 2021年6月3日 週四 下午11:18寫道: > > > Hi,> > > > > cy_huang(黃啟原) <cy_huang@richtek.com> 於 2021年6月3日 週四 下午6:20寫道: > > > > > Hi, Axel:> Use regulator_set_ramp_delay_regmap instead of open-coded. > > > > > > There's some reason. > You can refer to https://lkml.org/lkml/2021/6/1/1145. > > It's because our ramp value order is from small to large, not large to > small. > It conflicts with find_closest_bigger value chosen logic. > > I have verified the rt6160_set_ramp_delay() behavior exactly the same as > regulator_set_ramp_delay_regmap. (both functions get the same selector > for a given delay) > > Could you check if this patch works? > > Sure. > > After my test sample code, below's the result. > ascending [1000 2500 5000 10000] > target = 1000 =>sel = 0 > target = 2500 =>sel = 1 > target = 5000 =>sel = 2 > target = 10000 =>sel = 3 > target = 1700 =>sel = 1 > target = 2750 =>sel = 2 > target = 7500 =>sel = 3 > target = 15000 =>failed to find best select, sel = 3 > target = 0 =>sel = 0 > descending [10000 5000 2500 1000] > target = 1000 =>sel = 3 > target = 2500 =>sel = 2 > target = 5000 =>sel = 1 > target = 10000 =>sel = 0 > target = 1700 =>sel = 2 > target = 2750 =>sel = 1 > target = 7500 =>sel = 0 > target = 15000 =>failed to find best select, sel = 0 > target = 0 =>sel = 3 > > > It means when target is in range or even over, the result are all correct. > But like as the ramp target is equal to 0, the selection will only choose the minimum one. > When the ramp target is equal to 0, it means the user want to disable the rammpping function. > > As I know, if target is equal to 0, it must find the fastest rampping value as the best selection. > > > If your table is [1000 2500 5000 10000], find_closest_bigger() will > choose sel=0 when ramp_delay=0. > If your table is [10000 5000 2500 1000], find_closest_bigger() will > choose sel=3 when ramp_delay=0. > i.e. In both cases, find_closest_bigger() chooses the fastest ramping value. > > This meets your exception. > > Actually, even if your table is [10000, 1000, 5000, 2500], > find_closest_bigger() still can choose the correct selector. > i.e. sel=1 when ramp_delay=0 in this case. > > This selection may be wrong. > ramp_delay=0 means ramp disabled, > If chip not support rampping disable, this selection must be configured as fastest rampping value, not the minimum one. > > > 0 does not mean ramp disable. > It could be explicitly set to zero or its unintialized (zero by default). > see > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/regulator/core.c?id=1653ccf4c52df6a4abe8ec2f33f2cb2896d129ea > > How about 'ramp_disable' falg is true? > > > My understanding is: > If the h/w does not support disabling ramp_delay, the regulator core > won't call ops->set_ramp_delay when ramp_delay=0. > If the h/w supports disabling ramp, i.e. ramp_disable flag is true, > the regulator core will > call ops->set_ramp_delay with ramp_delay=0. in this case, it should > have an exactly match and > write the register to disable ramp_delay. > > > If this can be guaranteed , to use the set_ramp_delay_regmap helper function would be better.
Hi ChiYuan, Can you add Reviewed-by or Acked-by if this patch works.
Regards, Axel
| |