Re: [PATCH 2/6] regulator: anatop: only set supply regulator when it actually exists

Re: [PATCH 2/6] regulator: anatop: only set supply regulator when it actually exists

[Mark Brown]

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

On Wed, Apr 12, 2017 at 09:58:43AM +0800, Dong Aisheng wrote:
> Mandatorily set the initdata->supply_regulator while it actually not
> exist will cause regulator core to resolve supply each time whenever
> a new regulator registered which is meaningless and waste CPU mips.
> 
> We can observe more than one hundred times of iteration of resolving
> during a MX6Q SDB board booting up.
> 
> This patch adds the condition check for vin-supply to avoid the issue.

This is an obvious abstraction failure - there is nothing magical about
your driver which means that we need special casing in it to handle
badly written DTs that don't specify supplies.  Exactly the same
argument applies to all other regulators so if this is worth fixing it's
worth fixing in the core so we substitute in a dummy regulator if the
supply is genuinely missing.  Which is something we in fact have code to
do already though for some reason I can't see we bypass it, I'll send a
patch just now...

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

Re: [PATCH 5/6] regulator: anatop-regulator: make regulator-name using optionally

[Mark Brown]

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

On Wed, Apr 12, 2017 at 09:58:46AM +0800, Dong Aisheng wrote:
> rdesc->name/regulator-name is optional according to standard regulator
> binding doc. Use it conditionally to avoid a kernel NULL point crash.

It is optional in the standard binding because it is used to override
the name statically provided in the driver for the device.  Since the
anatop regulator is completely dynamic (there's no static list of
regulators in the device) it's mandatory for anatop regulators - you
should improve the error handling instead to detect a missing name.

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

Re: [PATCH 6/6] regulator: anatop: set default voltage selector for pcie

[Mark Brown]

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

On Wed, Apr 12, 2017 at 09:58:47AM +0800, Dong Aisheng wrote:
> Set the initial voltage selector for vddpcie in case it's disabled
> by default.

Why is this the only anatop regulator which can have this problem and
how do we know this is a good value?

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

Applied "regulator: anatop: remove unneeded name field of struct anatop_regulator" to the regulator tree

[Mark Brown]

The patch

   regulator: anatop: remove unneeded name field of struct anatop_regulator

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 aeb1404d68df62b0a1d277a4138dbd92a4330304 Mon Sep 17 00:00:00 2001
From: Dong Aisheng <aisheng.dong@nxp.com>
Date: Wed, 12 Apr 2017 09:58:45 +0800
Subject: [PATCH] regulator: anatop: remove unneeded name field of struct
 anatop_regulator

sreg->name is only used as an intermediate assign of rdesc->name, plus
another strcmp. Since we already have rdesc->name, no need it anymore.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/anatop-regulator.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 58141cbdf257..c6ce9745ffc8 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -39,7 +39,6 @@
 #define LDO_FET_FULL_ON			0x1f
 
 struct anatop_regulator {
-	const char *name;
 	u32 control_reg;
 	struct regmap *anatop;
 	int vol_bit_shift;
@@ -194,12 +193,12 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	if (!sreg)
 		return -ENOMEM;
 
-	of_property_read_string(np, "regulator-name", &sreg->name);
 	rdesc = &sreg->rdesc;
-	rdesc->name = sreg->name;
 	rdesc->type = REGULATOR_VOLTAGE;
 	rdesc->owner = THIS_MODULE;
 
+	of_property_read_string(np, "regulator-name", &rdesc->name);
+
 	initdata = of_get_regulator_init_data(dev, np, rdesc);
 	if (!initdata)
 		return -ENOMEM;
@@ -297,7 +296,7 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 		 * a sane default until imx6-cpufreq was probed and changes the
 		 * voltage to the correct value. In this case we set 1.25V.
 		 */
-		if (!sreg->sel && !strcmp(sreg->name, "vddpu"))
+		if (!sreg->sel && !strcmp(rdesc->name, "vddpu"))
 			sreg->sel = 22;
 
 		if (!sreg->bypass && !sreg->sel) {
-- 
2.11.0

Applied "regulator: anatop: use of_property_read_string to read the name" to the regulator tree

[Mark Brown]

The patch

   regulator: anatop: use of_property_read_string to read the name

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 5062e04711dbc4f67b24ffd926cc67060267792d Mon Sep 17 00:00:00 2001
From: Dong Aisheng <aisheng.dong@nxp.com>
Date: Wed, 12 Apr 2017 09:58:44 +0800
Subject: [PATCH] regulator: anatop: use of_property_read_string to read the
 name

sreg->name is a string, so use a more proper api to read back the string
instead of of_get_property.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/anatop-regulator.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index aa93f462ac6e..58141cbdf257 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -193,7 +193,8 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
 	if (!sreg)
 		return -ENOMEM;
-	sreg->name = of_get_property(np, "regulator-name", NULL);
+
+	of_property_read_string(np, "regulator-name", &sreg->name);
 	rdesc = &sreg->rdesc;
 	rdesc->name = sreg->name;
 	rdesc->type = REGULATOR_VOLTAGE;
-- 
2.11.0

Applied "regulator: anatop: check return value of of_get_regulator_init_data" to the regulator tree

[Mark Brown]

The patch

   regulator: anatop: check return value of of_get_regulator_init_data

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 7f51cf2ea7186e3f217e616a5522f1156678356f Mon Sep 17 00:00:00 2001
From: Dong Aisheng <aisheng.dong@nxp.com>
Date: Wed, 12 Apr 2017 09:58:42 +0800
Subject: [PATCH] regulator: anatop: check return value of
 of_get_regulator_init_data

Should check the return value of of_get_regulator_init_data before
using it.

Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/regulator/anatop-regulator.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 3a6d0290c54c..aa93f462ac6e 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -200,6 +200,9 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	rdesc->owner = THIS_MODULE;
 
 	initdata = of_get_regulator_init_data(dev, np, rdesc);
+	if (!initdata)
+		return -ENOMEM;
+
 	initdata->supply_regulator = "vin";
 	sreg->initdata = initdata;
 
-- 
2.11.0

[PATCH 1/6] regulator: anatop: check return value of of_get_regulator_init_data

[Dong Aisheng]

Should check the return value of of_get_regulator_init_data before
using it.

Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Robin Gong <yibin.gong@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/regulator/anatop-regulator.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index b041f27..46b9c2c 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -200,6 +200,9 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	rdesc->owner = THIS_MODULE;
 
 	initdata = of_get_regulator_init_data(dev, np, rdesc);
+	if (!initdata)
+		return -ENOMEM;
+
 	initdata->supply_regulator = "vin";
 	sreg->initdata = initdata;
 
-- 
2.7.4

[PATCH 2/6] regulator: anatop: only set supply regulator when it actually exists

[Dong Aisheng]

Mandatorily set the initdata->supply_regulator while it actually not
exist will cause regulator core to resolve supply each time whenever
a new regulator registered which is meaningless and waste CPU mips.

We can observe more than one hundred times of iteration of resolving
during a MX6Q SDB board booting up.

This patch adds the condition check for vin-supply to avoid the issue.

Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Robin Gong <yibin.gong@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/regulator/anatop-regulator.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 46b9c2c..2a97ada 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -203,7 +203,9 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	if (!initdata)
 		return -ENOMEM;
 
-	initdata->supply_regulator = "vin";
+	if (of_find_property(np, "vin-supply", NULL))
+		initdata->supply_regulator = "vin";
+
 	sreg->initdata = initdata;
 
 	anatop_np = of_get_parent(np);
-- 
2.7.4

[PATCH 3/6] regulator: anatop: use of_property_read_string to read the name

[Dong Aisheng]

sreg->name is a string, so use a more proper api to read back the string
instead of of_get_property.

Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Robin Gong <yibin.gong@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/regulator/anatop-regulator.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 2a97ada..9481730 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -193,7 +193,8 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
 	if (!sreg)
 		return -ENOMEM;
-	sreg->name = of_get_property(np, "regulator-name", NULL);
+
+	of_property_read_string(np, "regulator-name", &sreg->name);
 	rdesc = &sreg->rdesc;
 	rdesc->name = sreg->name;
 	rdesc->type = REGULATOR_VOLTAGE;
-- 
2.7.4

[PATCH 4/6] regulator: anatop: remove unneeded name field of struct anatop_regulator

[Dong Aisheng]

sreg->name is only used as an intermediate assign of rdesc->name, plus
another strcmp. Since we already have rdesc->name, no need it anymore.

Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Robin Gong <yibin.gong@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/regulator/anatop-regulator.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 9481730..19eb1f4 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -39,7 +39,6 @@
 #define LDO_FET_FULL_ON			0x1f
 
 struct anatop_regulator {
-	const char *name;
 	u32 control_reg;
 	struct regmap *anatop;
 	int vol_bit_shift;
@@ -194,12 +193,12 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 	if (!sreg)
 		return -ENOMEM;
 
-	of_property_read_string(np, "regulator-name", &sreg->name);
 	rdesc = &sreg->rdesc;
-	rdesc->name = sreg->name;
 	rdesc->type = REGULATOR_VOLTAGE;
 	rdesc->owner = THIS_MODULE;
 
+	of_property_read_string(np, "regulator-name", &rdesc->name);
+
 	initdata = of_get_regulator_init_data(dev, np, rdesc);
 	if (!initdata)
 		return -ENOMEM;
@@ -299,7 +298,7 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 		 * a sane default until imx6-cpufreq was probed and changes the
 		 * voltage to the correct value. In this case we set 1.25V.
 		 */
-		if (!sreg->sel && !strcmp(sreg->name, "vddpu"))
+		if (!sreg->sel && !strcmp(rdesc->name, "vddpu"))
 			sreg->sel = 22;
 
 		if (!sreg->bypass && !sreg->sel) {
-- 
2.7.4

[PATCH 5/6] regulator: anatop-regulator: make regulator-name using optionally

[Dong Aisheng]

rdesc->name/regulator-name is optional according to standard regulator
binding doc. Use it conditionally to avoid a kernel NULL point crash.

Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Robin Gong <yibin.gong@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/regulator/anatop-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 19eb1f4..6da0b20 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -298,7 +298,7 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 		 * a sane default until imx6-cpufreq was probed and changes the
 		 * voltage to the correct value. In this case we set 1.25V.
 		 */
-		if (!sreg->sel && !strcmp(rdesc->name, "vddpu"))
+		if (!sreg->sel && rdesc->name && !strcmp(rdesc->name, "vddpu"))
 			sreg->sel = 22;
 
 		if (!sreg->bypass && !sreg->sel) {
-- 
2.7.4

[PATCH 6/6] regulator: anatop: set default voltage selector for pcie

[Dong Aisheng]

Set the initial voltage selector for vddpcie in case it's disabled
by default.

This fixes the below warning:
20c8000.anatop:regulator-vddpcie: Failed to read a valid default voltage selector.
anatop_regulator: probe of 20c8000.anatop:regulator-vddpcie failed with error -22

Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: Robin Gong <yibin.gong@nxp.com>
Cc: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Richard Zhu <hongxing.zhu@nxp.com>
Signed-off-by: Dong Aisheng <aisheng.dong@nxp.com>
---
 drivers/regulator/anatop-regulator.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
index 6da0b20..910adfd 100644
--- a/drivers/regulator/anatop-regulator.c
+++ b/drivers/regulator/anatop-regulator.c
@@ -301,6 +301,11 @@ static int anatop_regulator_probe(struct platform_device *pdev)
 		if (!sreg->sel && rdesc->name && !strcmp(rdesc->name, "vddpu"))
 			sreg->sel = 22;
 
+		/* set the default voltage of the pcie phy to be 1.100v */
+		if (!sreg->sel && rdesc->name &&
+		    !strcmp(rdesc->name, "vddpcie"))
+			sreg->sel = 0x10;
+
 		if (!sreg->bypass && !sreg->sel) {
 			dev_err(&pdev->dev, "Failed to read a valid default voltage selector.\n");
 			return -EINVAL;
-- 
2.7.4

Re: [PATCH 6/6] regulator: anatop: set default voltage selector for pcie

[Mark Brown]

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

On Thu, Apr 13, 2017 at 03:41:03PM +0800, Dong Aisheng wrote:
> On Tue, Apr 11, 2017 at 09:40:03PM +0100, Mark Brown wrote:

> > Why is this the only anatop regulator which can have this problem and
> > how do we know this is a good value?

> Anatop regulator has no separate gate bit.
> e.g.
> 00000 Power gated off
> 00001 Target core voltage = 0.725V
> ...
> So it may have no valid default voltage in case it's disabled in
> bootloader.
> e.g. regulator_enable() may not work.

That doesn't answer my question.  What I'm asking is why another anatop
regulator might not end up disabled like this one.

> The default voltage 1.100v this patch sets is defined in reference
> manual.

For the SoC you're currently looking at...  might another have a
different value?

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

Re: [PATCH 2/6] regulator: anatop: only set supply regulator when it actually exists

[Mark Brown]

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

On Thu, Apr 13, 2017 at 03:11:01PM +0800, Dong Aisheng wrote:

> You're absolutely right!
> I did this because there're some where else did the same thing.
> e.g. drivers/regulator/fixed.c.

> But it's obviously none of any platform specific and is perfectly
> to be handled in regulator core.

Did my patch solve the problems you were seeing?  I just wrote it
quickly last thing before I finished for the evening.

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

Re: [PATCH 2/6] regulator: anatop: only set supply regulator when it actually exists

[Dong Aisheng]

On Wed, Apr 12, 2017 at 11:53 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Apr 13, 2017 at 03:11:01PM +0800, Dong Aisheng wrote:
>
>> You're absolutely right!
>> I did this because there're some where else did the same thing.
>> e.g. drivers/regulator/fixed.c.
>
>> But it's obviously none of any platform specific and is perfectly
>> to be handled in regulator core.
>
> Did my patch solve the problems you were seeing?  I just wrote it
> quickly last thing before I finished for the evening.

It can solve the problem.
But it breaks some thing and need a further tiny fix.
I just replied the mail in your patch thread.
Please check it!

Regards
Dong Aisheng

Re: [PATCH 2/6] regulator: anatop: only set supply regulator when it actually exists

[Mark Brown]

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

On Thu, Apr 13, 2017 at 12:00:36AM +0800, Dong Aisheng wrote:

> It can solve the problem.
> But it breaks some thing and need a further tiny fix.
> I just replied the mail in your patch thread.
> Please check it!

OK, I'll look out for your mail (I've not seen it yet, guess it's got
held up).  Thanks for testing.

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

Re: [PATCH 6/6] regulator: anatop: set default voltage selector for pcie

[Dong Aisheng]

On Wed, Apr 12, 2017 at 11:49 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Apr 13, 2017 at 03:41:03PM +0800, Dong Aisheng wrote:
>> On Tue, Apr 11, 2017 at 09:40:03PM +0100, Mark Brown wrote:
>
>> > Why is this the only anatop regulator which can have this problem and
>> > how do we know this is a good value?
>
>> Anatop regulator has no separate gate bit.
>> e.g.
>> 00000 Power gated off
>> 00001 Target core voltage = 0.725V
>> ...
>> So it may have no valid default voltage in case it's disabled in
>> bootloader.
>> e.g. regulator_enable() may not work.
>
> That doesn't answer my question.  What I'm asking is why another anatop
> regulator might not end up disabled like this one.
>

Well, that's true and i once thought of it.
Currently it is probably a quick fix and we did not see any others up till now
for all MX6&7 platforms based on NXP internal tree.

If we do see it in the future, then probably a better solution is constructing
a staticly defined default voltage table in anatop driver and do dynamically
check.

>> The default voltage 1.100v this patch sets is defined in reference
>> manual.
>
> For the SoC you're currently looking at...  might another have a
> different value?

No, only MX6SX has it currently.

Regards
Dong Aisheng

Re: [PATCH 6/6] regulator: anatop: set default voltage selector for pcie

[Lucas Stach]

Am Donnerstag, den 13.04.2017, 15:41 +0800 schrieb Dong Aisheng:
> On Tue, Apr 11, 2017 at 09:40:03PM +0100, Mark Brown wrote:
> > On Wed, Apr 12, 2017 at 09:58:47AM +0800, Dong Aisheng wrote:
> > > Set the initial voltage selector for vddpcie in case it's disabled
> > > by default.
> > 
> > Why is this the only anatop regulator which can have this problem and
> > how do we know this is a good value?
> 
> Anatop regulator has no separate gate bit.
> e.g.
> 00000 Power gated off
> 00001 Target core voltage = 0.725V
> ...
> So it may have no valid default voltage in case it's disabled in
> bootloader.
> e.g. regulator_enable() may not work.
> 
> The default voltage 1.100v this patch sets is defined in reference
> manual.

Huh? Shouldn't regulator_enable bring the voltage in a range defined by
the constraints in the DT?

Regards,
Lucas

Re: [PATCH 6/6] regulator: anatop: set default voltage selector for pcie

[Mark Brown]

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

On Wed, Apr 12, 2017 at 06:11:54PM +0200, Lucas Stach wrote:
> Am Donnerstag, den 13.04.2017, 15:41 +0800 schrieb Dong Aisheng:

> > The default voltage 1.100v this patch sets is defined in reference
> > manual.

> Huh? Shouldn't regulator_enable bring the voltage in a range defined by
> the constraints in the DT?

As part of that process it checks what the current voltage is but with
this regulator you can't read the voltage if the regulator is powered
off and we don't just ignore that error.

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

Re: [PATCH 2/6] regulator: anatop: only set supply regulator when it actually exists

[Dong Aisheng]

Hi Mark,

On Tue, Apr 11, 2017 at 09:31:24PM +0100, Mark Brown wrote:
> On Wed, Apr 12, 2017 at 09:58:43AM +0800, Dong Aisheng wrote:
> > Mandatorily set the initdata->supply_regulator while it actually not
> > exist will cause regulator core to resolve supply each time whenever
> > a new regulator registered which is meaningless and waste CPU mips.
> > 
> > We can observe more than one hundred times of iteration of resolving
> > during a MX6Q SDB board booting up.
> > 
> > This patch adds the condition check for vin-supply to avoid the issue.
> 
> This is an obvious abstraction failure - there is nothing magical about
> your driver which means that we need special casing in it to handle
> badly written DTs that don't specify supplies.  Exactly the same
> argument applies to all other regulators so if this is worth fixing it's
> worth fixing in the core so we substitute in a dummy regulator if the
> supply is genuinely missing.  Which is something we in fact have code to
> do already though for some reason I can't see we bypass it, I'll send a
> patch just now...

You're absolutely right!
I did this because there're some where else did the same thing.
e.g. drivers/regulator/fixed.c.

But it's obviously none of any platform specific and is perfectly
to be handled in regulator core.

Regards
Dong Aisheng

Re: [PATCH 5/6] regulator: anatop-regulator: make regulator-name using optionally

[Dong Aisheng]

On Tue, Apr 11, 2017 at 09:38:18PM +0100, Mark Brown wrote:
> On Wed, Apr 12, 2017 at 09:58:46AM +0800, Dong Aisheng wrote:
> > rdesc->name/regulator-name is optional according to standard regulator
> > binding doc. Use it conditionally to avoid a kernel NULL point crash.
> 
> It is optional in the standard binding because it is used to override
> the name statically provided in the driver for the device.  Since the
> anatop regulator is completely dynamic (there's no static list of
> regulators in the device) it's mandatory for anatop regulators - you
> should improve the error handling instead to detect a missing name.

Got it. Will change in v2.
Thanks for the suggestion.

Regards
Dong Aisheng

Re: [PATCH 6/6] regulator: anatop: set default voltage selector for pcie

[Dong Aisheng]

On Tue, Apr 11, 2017 at 09:40:03PM +0100, Mark Brown wrote:
> On Wed, Apr 12, 2017 at 09:58:47AM +0800, Dong Aisheng wrote:
> > Set the initial voltage selector for vddpcie in case it's disabled
> > by default.
> 
> Why is this the only anatop regulator which can have this problem and
> how do we know this is a good value?

Anatop regulator has no separate gate bit.
e.g.
00000 Power gated off
00001 Target core voltage = 0.725V
...
So it may have no valid default voltage in case it's disabled in
bootloader.
e.g. regulator_enable() may not work.

The default voltage 1.100v this patch sets is defined in reference
manual.

Regards
Dong Aisheng