Should check the return value of of_get_regulator_init_data before
using it.
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Robin Gong <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
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
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 <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Robin Gong <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
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
sreg->name is a string, so use a more proper api to read back the string
instead of of_get_property.
Cc: Liam Girdwood <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Robin Gong <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
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
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 <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Robin Gong <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
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
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 <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Robin Gong <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
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
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 <[email protected]>
Cc: Mark Brown <[email protected]>
Cc: Shawn Guo <[email protected]>
Cc: Sascha Hauer <[email protected]>
Cc: Robin Gong <[email protected]>
Cc: Richard Zhu <[email protected]>
Signed-off-by: Richard Zhu <[email protected]>
Signed-off-by: Dong Aisheng <[email protected]>
---
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
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...
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.
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?
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
Signed-off-by: Mark Brown <[email protected]>
---
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
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
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
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
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?
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.
On Wed, Apr 12, 2017 at 11:53 PM, Mark Brown <[email protected]> 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
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.
On Wed, Apr 12, 2017 at 11:49 PM, Mark Brown <[email protected]> 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
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
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.