Message-ID: <20120313032010.GB4513@shlinux2.ap.freescale.net>
Date:	Tue, 13 Mar 2012 11:20:10 +0800
From:	Dong Aisheng <aisheng.dong@...escale.com>
To:	Stephen Warren <swarren@...dotorg.org>
CC:	Dong Aisheng-B29396 <B29396@...escale.com>,
	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	"devicetree-discuss@...ts.ozlabs.org" 
	<devicetree-discuss@...ts.ozlabs.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Linus Walleij <linus.walleij@...ricsson.com>,
	"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
	"dongas86@...il.com" <dongas86@...il.com>,
	"shawn.guo@...aro.org" <shawn.guo@...aro.org>,
	"thomas.abraham@...aro.org" <thomas.abraham@...aro.org>,
	"tony@...mide.com" <tony@...mide.com>
Subject: Re: [PATCH] dt: pinctrl: Document device tree binding

On Tue, Mar 13, 2012 at 01:16:19AM +0800, Stephen Warren wrote:
> On 03/12/2012 08:34 AM, Dong Aisheng wrote:
> > On Sat, Mar 10, 2012 at 02:14:33AM +0800, Stephen Warren wrote:
> >> The core pin controller bindings define:
> >> * The fact that pin controllers expose pin configurations as nodes in
> >>   device tree.
> >> * That the bindings for those pin configuration nodes is defined by the
> >>   individual pin controller drivers.
> >> * A standardized set of properties for client devices to define numbered
> >>   or named pin configuration states, each referring to some number of the
> >>   afore-mentioned pin configuration nodes.
> >> * That the bindings for the client devices determines the set of numbered
> >>   or named states that must exist.
> 
> >> diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
> 
> >> +Required properties:
> >> +pinctrl-0:	List of phandles, each pointing at a pin configuration
> >> +		node. These referenced pin configuration nodes must be child
> >> +		nodes of the pin controller that they configure. Multiple
> >> +		entries may exist in this list so that multiple pin
> >> +		controllers may be configured, or so that a state may be built
> >> +		from multiple nodes for a single pin controller, each
> >> +		contributing part of the overall configuration. See the next
> >> +		section of this document for details of the format of these
> >> +		pin configuration nodes.
> >> +
> >> +		In some cases, it may be useful to define a state, but for it
> >> +		to be empty. This may be required when a common IP block is
> >> +		used in an SoC either without a pin controller, or where the
> >> +		pin controller does not affect the HW module in question. If
> >> +		the binding for that IP block requires certain pin states to
> >> +		exist, they must still be defined, but may be left empty.
> >> +
> >
> > It looks this functions similar as the PIN_MAP_DUMMY_STATE you introduced
> > before to address the issues that the shared IP block may need or not need
> > pinctrl configuration on different platforms(correct me if wrong).
> 
> Yes, it's to generate the dummy states.
> 
> > Then, there may be cases like below which may look a bit confusing
> > to people.
> > device {
> > 	pinctrl-names = "active", "idle";
> > 	pinctrl-0;
> > 	pinctrl-1;
> > };
> 
> I'd personally expect the syntax to look like:
> 
> device {
> 	pinctrl-names = "active", "idle";
> 	pinctrl-0 = <>;
> 	pinctrl-1 = <>;
> };
> 
> which has an explicitly empty value. Admittedly, these would both
> compile down to the exact same thing in the DTB, but I think the
> interpretation of the above is pretty readable.
> 
> > I'm wondering if we can let each individual driver to handle this special case?
> > Like checking device id then make decision whether call pinctrl_* APIs.
> > Then we can just do not define those properties for devices who
> > do not need pin configurations.
> 
> The individual client drivers certainly could work that way.
> 
> However, the disadvantage is that the client driver then needs explicit
> code to deal with this case, and this needs to be triggered by using a
Since this is purely specific to IP block(e.g. the driver knows this ip used
in which platform does not need pin configuration), so i guess it's natural
that the driver can also handle it instead of device tree, just like
a lot of existing drivers in kernel do similar things for tricks
on different SoCs.

> different compatible flag (or perhaps some other explicit property).
> You'd have to write this code over and over for each individual driver.
> 
I still do not understand why need a more special compatible flag?
My understanding is that in device driver side, they can do:
if (is_soc_a || is_soc_b) {
	pinctrl_get(dev, "default");
	....
} else if (is_soc_c) {
	/* do nothing */
}
I can't see why we still need a special compatible flag to tell driver.
Just do not define pinctrl-* properties for that devices in device tree.
Did i understand wrong?

> That also means that if you were the first user of an IP block in a
> system which didn't need pin muxing for it, you'd have to modify the
> kernel to support pinctrl being optional before you could use that device.
> 
Why need modify the kernel?
Assuming above example.
I'm a bit confused.

> If the pinctrl subsystem itself hides this from the client driver, then
> you'd never need to add any code to any driver to support this case, and
> all you'd need to do is write a few lines of device tree to use the
> driver; no code changes.
> 
Yes, that is the benefit.
My only concern is that if this may make people confuse when see
such code in device tree since we,i guess, still do not have such examples
in device tree. And i'm afraid this is a bit not HW oriented.
device {
	pinctrl-names = "active", "idle";
	pinctrl-0 = <>;
	pinctrl-1 = <>;
};
So i'm asking if we do it in driver.
Maybe device tree people can give some comments.

Regards
Dong Aisheng

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists