Thread (10 messages) 10 messages, 4 authors, 2016-01-04

Re: [PATCH] pinctrl: mediatek: convert to arch_initcall

From: Matthias Brugger <matthias.bgg@gmail.com>
Date: 2016-01-01 14:27:36
Also in: linux-arm-kernel, linux-gpio, lkml


On January 1, 2016 3:56:01 AM EET, Daniel Kurtz [off-list ref] wrote:
Hi Mark,

Thanks for responding.

On Fri, Jan 1, 2016 at 6:07 AM, Mark Brown [off-list ref] wrote:
quoted
On Thu, Dec 31, 2015 at 09:45:51PM +0800, Daniel Kurtz wrote:
quoted
On Thu, Dec 31, 2015 at 1:22 AM, Mark Brown [off-list ref]
wrote:
quoted
quoted
quoted
I really don't think we should be applying this sort of stuff
unless
quoted
quoted
quoted
things are actively broken right now.  It's a bit of a rabbit hole
we
quoted
quoted
quoted
could spend a long time going down tweaking things for different
systems in the same way that tweaking the link order can be and it
masks
quoted
quoted
quoted
the underlying issues.
quoted
Things are actively broken right now, in the sense that there are
many
quoted
quoted
needless probe deferrals on boot.
That's just noisy, everything does end up loading OK.
It's not just noisy, it also adds to boot time.
quoted
 If the noise is a
problem working on fixing the underlying problem with not being able
to
quoted
figure out dependencies seems like a better thing.  When we discussed
this on the kernel summit list it wasn't clear everyone was convinced
this was even a problem (I think it is since it obscures real
information).  Actual breakage to me is something that never sorts
itself out.
quoted
These are pinctrl drivers, which are required to load before every
other driver that requests a gpio.
AFAICT, the pinctrl is part of the platform "architecture", hence
why
quoted
quoted
I suggest we move this to arch_initcall().
This is exactly the sort of hacking that leads to problems
What problems?
More patches as people adjust / tune / optimize boot time, or something
else?
quoted
you can
also make the same argument for a bunch of other things like
regulators
quoted
but then you find there's circular dependencies or extra devices with
different requirements on some systems that cause further issues and
need more special casing, or you find that some other device gets
pushed
quoted
earlier so you have to go round tweaking everything it uses.
For regulators, I think things are a bit more subtle.  Most regulators
are external components that can be used on different boards for
different purposes, so I can see an argument for treating them in a
more generic way by using a later device_initcall.  The exception
being architecture specific PMICs that only make sense when paired
with a specific small set of CPUs - and if you look, there are many
PMIC regulator drivers that are at earlier initcall levels, presumably
because they are required by cpufreq drivers, and/or their initcall
level is set as the same as the rest of the functions exposed by the
same PMIC MFD.
quoted
It's not
that the device is magic, it's that we don't understand how to handle
dependencies well enough.  Raphael did say he was going to work on
something for this, I'm not sure where it got to though.
Glad to hear it is a well recognized problem, and that people plan to
look into a fix.
quoted
quoted
arch_initcall() is also consistent with 39 other pinctrl drivers in
drivers/pinctrl.
19 others use subsys_initcall(), core_initcall() or
postcore_initcall(), any of which would also work.
It's fairly clear that there's at least a case for simplifying the
existing practice here, for example by moving everything into a
single
quoted
(perhaps aliased) initcall rather than by randomly picking a level
per
quoted
system or by actually fiddling with the link ordering if the case is
sufficiently clear that pinctrl in general ought to load earlier than
it
quoted
does.
Nothing above sounds like a reason not to merge this patch, however.
Why should we block useful patches that use existing tools to fix real
architecture-specific issues until new infrastructure is merged that
solves general problems?
I think what Mark means is, that we define some pinctrl_initcall which is a macro to subsys_initcall (or arch_initcall or similar). We apply this to all pinctrl drivers including the one from Mediatek. This way at least we have a common method and changing the behaviour in the future is easier to handle.

Regards,
Matthias
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help