Linux-ARM-MSM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel
@ 2019-06-19 18:56 Jeffrey Hugo
  2019-06-19 19:05 ` Bjorn Andersson
                   ` (3 more replies)
  0 siblings, 4 replies; 6+ messages in thread
From: Jeffrey Hugo @ 2019-06-19 18:56 UTC (permalink / raw)
  To: bjorn.andersson, lgirdwood, broonie, jorge.ramirez-ortiz
  Cc: linux-arm-msm, linux-kernel, Jeffrey Hugo

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel
  2019-06-19 18:56 [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel Jeffrey Hugo
@ 2019-06-19 19:05 ` Bjorn Andersson
  2019-06-19 19:43   ` Jorge Ramirez
  2019-06-20 11:54 ` Mark Brown
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 6+ messages in thread
From: Bjorn Andersson @ 2019-06-19 19:05 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: lgirdwood, broonie, jorge.ramirez-ortiz, linux-arm-msm, linux-kernel

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
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel
  2019-06-19 19:05 ` Bjorn Andersson
@ 2019-06-19 19:43   ` Jorge Ramirez
  0 siblings, 0 replies; 6+ messages in thread
From: Jorge Ramirez @ 2019-06-19 19:43 UTC (permalink / raw)
  To: Bjorn Andersson, Jeffrey Hugo
  Cc: lgirdwood, broonie, linux-arm-msm, linux-kernel

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel
  2019-06-19 18:56 [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel Jeffrey Hugo
  2019-06-19 19:05 ` Bjorn Andersson
@ 2019-06-20 11:54 ` Mark Brown
  2019-06-20 13:32 ` Applied "regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel" to the regulator tree Mark Brown
  2019-06-20 13:55 ` [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel Niklas Cassel
  3 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2019-06-20 11:54 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: bjorn.andersson, lgirdwood, jorge.ramirez-ortiz, linux-arm-msm,
	linux-kernel

[-- Attachment #1: Type: text/plain, Size: 372 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Applied "regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel" to the regulator tree
  2019-06-19 18:56 [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel Jeffrey Hugo
  2019-06-19 19:05 ` Bjorn Andersson
  2019-06-20 11:54 ` Mark Brown
@ 2019-06-20 13:32 ` Mark Brown
  2019-06-20 13:55 ` [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel Niklas Cassel
  3 siblings, 0 replies; 6+ messages in thread
From: Mark Brown @ 2019-06-20 13:32 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: Bjorn Andersson, bjorn.andersson, broonie, Jorge Ramirez-Ortiz,
	jorge.ramirez-ortiz, lgirdwood, linux-arm-msm, linux-kernel,
	Mark Brown

The patch

   regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel

has been applied to the regulator tree at

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-5.3

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

From 61d7fdc49f03f4ec990974d1d2a8b05e64afeae4 Mon Sep 17 00:00:00 2001
From: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
Date: Wed, 19 Jun 2019 11:56:36 -0700
Subject: [PATCH] regulator: qcom_spmi: Fix math of
 spmi_regulator_set_voltage_time_sel

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>
Reported-by: Mark Brown <broonie@kernel.org>
Signed-off-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Tested-by: Jorge Ramirez-Ortiz <jorge.ramirez-ortiz@linaro.org>
Signed-off-by: Mark Brown <broonie@kernel.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.20.1


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel
  2019-06-19 18:56 [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel Jeffrey Hugo
                   ` (2 preceding siblings ...)
  2019-06-20 13:32 ` Applied "regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel" to the regulator tree Mark Brown
@ 2019-06-20 13:55 ` Niklas Cassel
  3 siblings, 0 replies; 6+ messages in thread
From: Niklas Cassel @ 2019-06-20 13:55 UTC (permalink / raw)
  To: Jeffrey Hugo
  Cc: bjorn.andersson, lgirdwood, broonie, jorge.ramirez-ortiz,
	linux-arm-msm, linux-kernel

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>

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, back to index

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-06-19 18:56 [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel Jeffrey Hugo
2019-06-19 19:05 ` Bjorn Andersson
2019-06-19 19:43   ` Jorge Ramirez
2019-06-20 11:54 ` Mark Brown
2019-06-20 13:32 ` Applied "regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel" to the regulator tree Mark Brown
2019-06-20 13:55 ` [PATCH] regulator: qcom_spmi: Fix math of spmi_regulator_set_voltage_time_sel Niklas Cassel

Linux-ARM-MSM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-msm/0 linux-arm-msm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-msm linux-arm-msm/ https://lore.kernel.org/linux-arm-msm \
		linux-arm-msm@vger.kernel.org linux-arm-msm@archiver.kernel.org
	public-inbox-index linux-arm-msm


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-arm-msm


AGPL code for this site: git clone https://public-inbox.org/ public-inbox