| Message ID | 20190619185636.10831-1-jeffrey.l.hugo@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 61d7fdc49f03f4ec990974d1d2a8b05e64afeae4 |
| Headers | show |
| Series | regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel | expand |
On Wed 19 Jun 11:56 PDT 2019, Jeffrey Hugo wrote: > spmi_regulator_set_voltage_time_sel() calculates the amount of delay > needed as the result of setting a new voltage. Essentially this is the > absolute difference of the old and new voltages, divided by the slew rate. > > The implementation of spmi_regulator_set_voltage_time_sel() is wrong. > > It attempts to calculate the difference in voltages by using the > difference in selectors and multiplying by the voltage step between > selectors. This ignores the possibility that the old and new selectors > might be from different ranges, which have different step values. Also, > the difference between the selectors may encapsulate N ranges inbetween, > so a summation of each selector change from old to new would be needed. > > Lets avoid all of that complexity, and just get the actual voltage > represented by both the old and new selector, and use those to directly > compute the voltage delta. This is more straight forward, and has the > side benifit of avoiding issues with regulator implementations that don't > have hardware register support to get the current configured range. > > Fixes: e92a4047419c ("regulator: Add QCOM SPMI regulator driver") > Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Reported-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> > --- > drivers/regulator/qcom_spmi-regulator.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c > index 13f83be50076..877df33e0246 100644 > --- a/drivers/regulator/qcom_spmi-regulator.c > +++ b/drivers/regulator/qcom_spmi-regulator.c > @@ -813,14 +813,10 @@ static int spmi_regulator_set_voltage_time_sel(struct regulator_dev *rdev, > unsigned int old_selector, unsigned int new_selector) > { > struct spmi_regulator *vreg = rdev_get_drvdata(rdev); > - const struct spmi_voltage_range *range; > int diff_uV; > > - range = spmi_regulator_find_range(vreg); > - if (!range) > - return -EINVAL; > - > - diff_uV = abs(new_selector - old_selector) * range->step_uV; > + diff_uV = abs(spmi_regulator_common_list_voltage(rdev, new_selector) - > + spmi_regulator_common_list_voltage(rdev, old_selector)); > > return DIV_ROUND_UP(diff_uV, vreg->slew_rate); > } > -- > 2.17.1 >
On 6/19/19 21:05, Bjorn Andersson wrote: > On Wed 19 Jun 11:56 PDT 2019, Jeffrey Hugo wrote: > >> spmi_regulator_set_voltage_time_sel() calculates the amount of delay >> needed as the result of setting a new voltage. Essentially this is the >> absolute difference of the old and new voltages, divided by the slew rate. >> >> The implementation of spmi_regulator_set_voltage_time_sel() is wrong. >> >> It attempts to calculate the difference in voltages by using the >> difference in selectors and multiplying by the voltage step between >> selectors. This ignores the possibility that the old and new selectors >> might be from different ranges, which have different step values. Also, >> the difference between the selectors may encapsulate N ranges inbetween, >> so a summation of each selector change from old to new would be needed. >> >> Lets avoid all of that complexity, and just get the actual voltage >> represented by both the old and new selector, and use those to directly >> compute the voltage delta. This is more straight forward, and has the >> side benifit of avoiding issues with regulator implementations that don't >> have hardware register support to get the current configured range. >> >> Fixes: e92a4047419c ("regulator: Add QCOM SPMI regulator driver") >> Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org> >> Reported-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> >> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > > Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Tested on EVB-4000 using the cpufreq patchset that I still need to repost v3 [1] Tested-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> [1] https://patchwork.kernel.org/cover/10784383/ > >> --- >> drivers/regulator/qcom_spmi-regulator.c | 8 ++------ >> 1 file changed, 2 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c >> index 13f83be50076..877df33e0246 100644 >> --- a/drivers/regulator/qcom_spmi-regulator.c >> +++ b/drivers/regulator/qcom_spmi-regulator.c >> @@ -813,14 +813,10 @@ static int spmi_regulator_set_voltage_time_sel(struct regulator_dev *rdev, >> unsigned int old_selector, unsigned int new_selector) >> { >> struct spmi_regulator *vreg = rdev_get_drvdata(rdev); >> - const struct spmi_voltage_range *range; >> int diff_uV; >> >> - range = spmi_regulator_find_range(vreg); >> - if (!range) >> - return -EINVAL; >> - >> - diff_uV = abs(new_selector - old_selector) * range->step_uV; >> + diff_uV = abs(spmi_regulator_common_list_voltage(rdev, new_selector) - >> + spmi_regulator_common_list_voltage(rdev, old_selector)); >> >> return DIV_ROUND_UP(diff_uV, vreg->slew_rate); >> } >> -- >> 2.17.1 >> >
On Wed, Jun 19, 2019 at 11:56:36AM -0700, Jeffrey Hugo wrote: > Fixes: e92a4047419c ("regulator: Add QCOM SPMI regulator driver") > Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Reported-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> I remember pointing this out during reviews as well...
On Wed, Jun 19, 2019 at 11:56:36AM -0700, Jeffrey Hugo wrote: > spmi_regulator_set_voltage_time_sel() calculates the amount of delay > needed as the result of setting a new voltage. Essentially this is the > absolute difference of the old and new voltages, divided by the slew rate. > > The implementation of spmi_regulator_set_voltage_time_sel() is wrong. > > It attempts to calculate the difference in voltages by using the > difference in selectors and multiplying by the voltage step between > selectors. This ignores the possibility that the old and new selectors > might be from different ranges, which have different step values. Also, > the difference between the selectors may encapsulate N ranges inbetween, > so a summation of each selector change from old to new would be needed. > > Lets avoid all of that complexity, and just get the actual voltage > represented by both the old and new selector, and use those to directly > compute the voltage delta. This is more straight forward, and has the > side benifit of avoiding issues with regulator implementations that don't > have hardware register support to get the current configured range. > > Fixes: e92a4047419c ("regulator: Add QCOM SPMI regulator driver") > Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org> > Reported-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> > Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> > --- > drivers/regulator/qcom_spmi-regulator.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c > index 13f83be50076..877df33e0246 100644 > --- a/drivers/regulator/qcom_spmi-regulator.c > +++ b/drivers/regulator/qcom_spmi-regulator.c > @@ -813,14 +813,10 @@ static int spmi_regulator_set_voltage_time_sel(struct regulator_dev *rdev, > unsigned int old_selector, unsigned int new_selector) > { > struct spmi_regulator *vreg = rdev_get_drvdata(rdev); > - const struct spmi_voltage_range *range; > int diff_uV; > > - range = spmi_regulator_find_range(vreg); > - if (!range) > - return -EINVAL; > - > - diff_uV = abs(new_selector - old_selector) * range->step_uV; > + diff_uV = abs(spmi_regulator_common_list_voltage(rdev, new_selector) - > + spmi_regulator_common_list_voltage(rdev, old_selector)); > > return DIV_ROUND_UP(diff_uV, vreg->slew_rate); > } > -- > 2.17.1 > Tested-by: Niklas Cassel <niklas.cassel@linaro.org>
diff --git a/drivers/regulator/qcom_spmi-regulator.c b/drivers/regulator/qcom_spmi-regulator.c index 13f83be50076..877df33e0246 100644 --- a/drivers/regulator/qcom_spmi-regulator.c +++ b/drivers/regulator/qcom_spmi-regulator.c @@ -813,14 +813,10 @@ static int spmi_regulator_set_voltage_time_sel(struct regulator_dev *rdev, unsigned int old_selector, unsigned int new_selector) { struct spmi_regulator *vreg = rdev_get_drvdata(rdev); - const struct spmi_voltage_range *range; int diff_uV; - range = spmi_regulator_find_range(vreg); - if (!range) - return -EINVAL; - - diff_uV = abs(new_selector - old_selector) * range->step_uV; + diff_uV = abs(spmi_regulator_common_list_voltage(rdev, new_selector) - + spmi_regulator_common_list_voltage(rdev, old_selector)); return DIV_ROUND_UP(diff_uV, vreg->slew_rate); }
spmi_regulator_set_voltage_time_sel() calculates the amount of delay needed as the result of setting a new voltage. Essentially this is the absolute difference of the old and new voltages, divided by the slew rate. The implementation of spmi_regulator_set_voltage_time_sel() is wrong. It attempts to calculate the difference in voltages by using the difference in selectors and multiplying by the voltage step between selectors. This ignores the possibility that the old and new selectors might be from different ranges, which have different step values. Also, the difference between the selectors may encapsulate N ranges inbetween, so a summation of each selector change from old to new would be needed. Lets avoid all of that complexity, and just get the actual voltage represented by both the old and new selector, and use those to directly compute the voltage delta. This is more straight forward, and has the side benifit of avoiding issues with regulator implementations that don't have hardware register support to get the current configured range. Fixes: e92a4047419c ("regulator: Add QCOM SPMI regulator driver") Reported-by: Bjorn Andersson <bjorn.andersson@linaro.org> Reported-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org> Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com> --- drivers/regulator/qcom_spmi-regulator.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)