[RFC PATCH v1 0/3] regulator: qcom_smd: add list_voltage callback support.

[RFC PATCH v1 0/3] regulator: qcom_smd: add list_voltage callback support.

[Srinivas Kandagatla]

This patchset adds list_voltage callback support to qcom smd regulator driver.
One of the helper patch modifies the existing list_voltage_linear_range()
callback to consider voltage constriants, so that the consumers get the
actual supported voltage range information.

Without this patchset, I was unable to get SD card working with the
mainline kernel on DB410c.

Thanks,
srini

Srinivas Kandagatla (3):
  regulator: helpers: consider constriants in list_voltage_linear_range
  regulator: qcom_smd: add list_voltage callback
  regulator: qcom_smd: add linear range to pm8941 lnldo

 drivers/regulator/helpers.c            | 10 ++++++++--
 drivers/regulator/qcom_smd-regulator.c |  6 +++++-
 2 files changed, 13 insertions(+), 3 deletions(-)

--
2.8.2

[RFC PATCH v1 1/3] regulator: helpers: consider constriants in list_voltage_linear_range

[Srinivas Kandagatla]

Regulator drivers can have linear range which is according to the
regualtor hardware spec, however the board level device tree files
can restrict this range by adding constriants.
These constriants are not considered in the exsiting code, which
gives false supported voltage range to the consumers.

Fix this by adding constriants check in the helper function.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---

For now I have added this support for regulator_list_voltage_linear_range()
If it makes sense we can extend this to other list voltage helpers too.


 drivers/regulator/helpers.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/helpers.c b/drivers/regulator/helpers.c
index bcf38fd..379b2b3 100644
--- a/drivers/regulator/helpers.c
+++ b/drivers/regulator/helpers.c
@@ -16,6 +16,7 @@
 #include <linux/delay.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
+#include <linux/regulator/machine.h>
 #include <linux/regulator/driver.h>
 #include <linux/module.h>

@@ -374,7 +375,7 @@ int regulator_list_voltage_linear_range(struct regulator_dev *rdev,
 					unsigned int selector)
 {
 	const struct regulator_linear_range *range;
-	int i;
+	int i, v;

 	if (!rdev->desc->n_linear_ranges) {
 		BUG_ON(!rdev->desc->n_linear_ranges);
@@ -389,8 +390,13 @@ int regulator_list_voltage_linear_range(struct regulator_dev *rdev,
 			continue;

 		selector -= range->min_sel;
+		v = range->min_uV + (range->uV_step * selector);

-		return range->min_uV + (range->uV_step * selector);
+		if (v < rdev->constraints->min_uV ||
+		    v > rdev->constraints->max_uV)
+			return -EINVAL;
+		else
+			return v;
 	}

 	return -EINVAL;
--
2.8.2

Re: [RFC PATCH v1 1/3] regulator: helpers: consider constriants in list_voltage_linear_range

[Mark Brown]

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

On Thu, Jun 02, 2016 at 11:23:14AM +0100, Srinivas Kandagatla wrote:
> Regulator drivers can have linear range which is according to the
> regualtor hardware spec, however the board level device tree files
> can restrict this range by adding constriants.
> These constriants are not considered in the exsiting code, which
> gives false supported voltage range to the consumers.

...

> For now I have added this support for regulator_list_voltage_linear_range()
> If it makes sense we can extend this to other list voltage helpers too.

Why are you making this change?  The obvious problem here is that drivers
should not be looking at constraints - it would be silly to duplicate
constraint enforcing code in individual drivers and would lead to
inconsistent performance of constraints.  This is why we do this in the
core, in _regulator_list_voltage(), which means that if this change has
any effect there's something else going on that needs to be
investigated.

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

Re: [RFC PATCH v1 1/3] regulator: helpers: consider constriants in list_voltage_linear_range

[Srinivas Kandagatla]

On 02/06/16 12:05, Mark Brown wrote:
> On Thu, Jun 02, 2016 at 11:23:14AM +0100, Srinivas Kandagatla wrote:
>> Regulator drivers can have linear range which is according to the
>> regualtor hardware spec, however the board level device tree files
>> can restrict this range by adding constriants.
>> These constriants are not considered in the exsiting code, which
>> gives false supported voltage range to the consumers.
>
> ...
>
>> For now I have added this support for regulator_list_voltage_linear_range()
>> If it makes sense we can extend this to other list voltage helpers too.
>
> Why are you making this change?  The obvious problem here is that drivers
> should not be looking at constraints - it would be silly to duplicate
> constraint enforcing code in individual drivers and would lead to
> inconsistent performance of constraints.  This is why we do this in the
> core, in _regulator_list_voltage(),
Yep, you are right, I should have looked into _regulator_list_voltage() 
in more carefully, it already has this check in place.

--srini

[RFC PATCH v1 2/3] regulator: qcom_smd: add list_voltage callback

[Srinivas Kandagatla]

This patch adds support to list_voltage callback, so that consumers
like mmc core, can get information of supported voltage range.

Without this patch there is no way for mmc core to know this voltage range.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/regulator/qcom_smd-regulator.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
index 56a17ec..b11b627 100644
--- a/drivers/regulator/qcom_smd-regulator.c
+++ b/drivers/regulator/qcom_smd-regulator.c
@@ -140,6 +140,7 @@ static const struct regulator_ops rpm_smps_ldo_ops = {
 	.enable = rpm_reg_enable,
 	.disable = rpm_reg_disable,
 	.is_enabled = rpm_reg_is_enabled,
+	.list_voltage = regulator_list_voltage_linear_range,
 
 	.get_voltage = rpm_reg_get_voltage,
 	.set_voltage = rpm_reg_set_voltage,
-- 
2.8.2

Re: [RFC PATCH v1 2/3] regulator: qcom_smd: add list_voltage callback

[Mark Brown]

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

On Thu, Jun 02, 2016 at 11:23:15AM +0100, Srinivas Kandagatla wrote:
> This patch adds support to list_voltage callback, so that consumers
> like mmc core, can get information of supported voltage range.
> 
> Without this patch there is no way for mmc core to know this voltage range.

Something I applied yesterday, most likely this, seems to have broken
boot on a large proportion of (potentially all) 32 bit Qualcomm
platforms:

   https://kernelci.org/boot/all/job/broonie-regulator/kernel/v4.7-rc1-10-gafafb91512cf/

I suspect this is because the fixed voltage regulator is triggering a
crash as it has no linear ranges defined.  Please get me a fix ASAP.

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

Re: [RFC PATCH v1 2/3] regulator: qcom_smd: add list_voltage callback

[Srinivas Kandagatla]

On 03/06/16 11:40, Mark Brown wrote:
> On Thu, Jun 02, 2016 at 11:23:15AM +0100, Srinivas Kandagatla wrote:
>> This patch adds support to list_voltage callback, so that consumers
>> like mmc core, can get information of supported voltage range.
>>
>> Without this patch there is no way for mmc core to know this voltage range.
>
> Something I applied yesterday, most likely this, seems to have broken
> boot on a large proportion of (potentially all) 32 bit Qualcomm
> platforms:
>
>     https://kernelci.org/boot/all/job/broonie-regulator/kernel/v4.7-rc1-10-gafafb91512cf/
>
> I suspect this is because the fixed voltage regulator is triggering a
> crash as it has no linear ranges defined.  Please get me a fix ASAP.
Yes, looks like it is because of the fixed regulator thing, I send a 
patch with separate ops for this.

--srini
>

Applied "regulator: qcom_smd: add list_voltage callback" to the regulator tree

[Mark Brown]

The patch

   regulator: qcom_smd: add list_voltage callback

has been applied to the regulator tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git 

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 3bfbb4d1a480cc17f6ccfce13b76eb6c0dbeaf8c Mon Sep 17 00:00:00 2001
From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Date: Thu, 2 Jun 2016 11:23:15 +0100
Subject: [PATCH] regulator: qcom_smd: add list_voltage callback

This patch adds support to list_voltage callback, so that consumers
like mmc core, can get information of supported voltage range.

Without this patch there is no way for mmc core to know this voltage range.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/qcom_smd-regulator.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
index 6c7fe4778793..526bf23dcb49 100644
--- a/drivers/regulator/qcom_smd-regulator.c
+++ b/drivers/regulator/qcom_smd-regulator.c
@@ -152,6 +152,7 @@ static const struct regulator_ops rpm_smps_ldo_ops_fixed = {
 	.enable = rpm_reg_enable,
 	.disable = rpm_reg_disable,
 	.is_enabled = rpm_reg_is_enabled,
+	.list_voltage = regulator_list_voltage_linear_range,
 
 	.get_voltage = rpm_reg_get_voltage,
 	.set_voltage = rpm_reg_set_voltage,
-- 
2.8.1

[RFC PATCH v1 3/3] regulator: qcom_smd: add linear range to pm8941 lnldo

[Srinivas Kandagatla]

This patch converts a fixed voltage pm8941 lnldo to a single step
linear range regulator, so that we could use the same list_volatage
callback without BUG_ON from regulator core.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/regulator/qcom_smd-regulator.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
index b11b627..09c688d 100644
--- a/drivers/regulator/qcom_smd-regulator.c
+++ b/drivers/regulator/qcom_smd-regulator.c
@@ -246,7 +246,10 @@ static const struct regulator_desc pm8941_nldo = {
 };
 
 static const struct regulator_desc pm8941_lnldo = {
-	.fixed_uV = 1740000,
+	.linear_ranges = (struct regulator_linear_range[]) {
+		REGULATOR_LINEAR_RANGE(1740000, 0, 0, 1740000),
+	},
+	.n_linear_ranges = 1,
 	.n_voltages = 1,
 	.ops = &rpm_smps_ldo_ops,
 };
-- 
2.8.2

Re: [RFC PATCH v1 3/3] regulator: qcom_smd: add linear range to pm8941 lnldo

[Mark Brown]

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

On Thu, Jun 02, 2016 at 11:23:16AM +0100, Srinivas Kandagatla wrote:
> This patch converts a fixed voltage pm8941 lnldo to a single step
> linear range regulator, so that we could use the same list_volatage
> callback without BUG_ON from regulator core.

Why is this better than using a separate set of ops for the driver?

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

Re: [RFC PATCH v1 3/3] regulator: qcom_smd: add linear range to pm8941 lnldo

[Srinivas Kandagatla]

On 02/06/16 15:49, Mark Brown wrote:
> On Thu, Jun 02, 2016 at 11:23:16AM +0100, Srinivas Kandagatla wrote:
>> This patch converts a fixed voltage pm8941 lnldo to a single step
>> linear range regulator, so that we could use the same list_volatage
>> callback without BUG_ON from regulator core.
>
> Why is this better than using a separate set of ops for the driver?
Am ok either way, it would be just few more lines for separate set of ops.

--srini

Re: [RFC PATCH v1 3/3] regulator: qcom_smd: add linear range to pm8941 lnldo

[Mark Brown]

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

On Thu, Jun 02, 2016 at 03:57:42PM +0100, Srinivas Kandagatla wrote:
> On 02/06/16 15:49, Mark Brown wrote:

> > Why is this better than using a separate set of ops for the driver?

> Am ok either way, it would be just few more lines for separate set of ops.

It's more natural to use a separate set of ops, and we can optimise a
few things if we know the regulator is a fixed voltage one.

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

Re: [RFC PATCH v1 3/3] regulator: qcom_smd: add linear range to pm8941 lnldo

[Bjorn Andersson]

On Thu 02 Jun 08:50 PDT 2016, Mark Brown wrote:

> On Thu, Jun 02, 2016 at 03:57:42PM +0100, Srinivas Kandagatla wrote:
> > On 02/06/16 15:49, Mark Brown wrote:
> 
> > > Why is this better than using a separate set of ops for the driver?
> 
> > Am ok either way, it would be just few more lines for separate set of ops.
> 
> It's more natural to use a separate set of ops, and we can optimise a
> few things if we know the regulator is a fixed voltage one.

In my view a fixed regulator is a thing that when you turn it on you get
a predefined voltage, but iirc we actually need to send of a set-voltage
request for the singly supported voltage on this ldo (which both
implementations do today...).

That's why I suggested Srini to do it this way, but maybe my
interpretation of "fixed" is inaccurate?

Regards,
Bjorn

Re: [RFC PATCH v1 3/3] regulator: qcom_smd: add linear range to pm8941 lnldo

[Mark Brown]

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

On Thu, Jun 02, 2016 at 09:04:32AM -0700, Bjorn Andersson wrote:
> On Thu 02 Jun 08:50 PDT 2016, Mark Brown wrote:

> > It's more natural to use a separate set of ops, and we can optimise a
> > few things if we know the regulator is a fixed voltage one.

> In my view a fixed regulator is a thing that when you turn it on you get
> a predefined voltage, but iirc we actually need to send of a set-voltage
> request for the singly supported voltage on this ldo (which both
> implementations do today...).

> That's why I suggested Srini to do it this way, but maybe my
> interpretation of "fixed" is inaccurate?

If it only supports one voltage then shouldn't we just tell it that
voltage once at startup?  Doing a call in the probe routine would be
fine, the framework doesn't need to know about that really.

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