| Message ID | 1491962327-12477-2-git-send-email-aisheng.dong@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 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 <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
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.
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
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);
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(-)