Message-ID: <4F5F9F9E.7030706@wwwdotorg.org>
Date:	Tue, 13 Mar 2012 13:27:26 -0600
From:	Stephen Warren <swarren@...dotorg.org>
To:	Dong Aisheng <aisheng.dong@...escale.com>
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 03/12/2012 09:20 PM, Dong Aisheng wrote:
> 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.
...
>>>> +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.

Well, the entire point is that the driver for the IP block shouldn't
know anything about which SoC it's included in, or whether pinmux is
needed, or what pinmux is needed, beyond what's expressed in platform
data or device tree. The whole point of the pinctrl is to completely
remove this knowledge from the driver, and centralize it in the mapping
table.

>> 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 */
> }

Drivers aren't supposed to contain "is_soc_foo" or "is_machine_foo"
calls. Indeed, in the case of "is_soc_foo", I don't think such an API
even exists. Instead, platform data or device tree should represent the
information that drivers need.

> 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?

The compatible property would be the only way to implement anything like
is_soc_foo. However, it's a bad idea to overload the compatible property
in this way.

>> 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 driver contains code like:

if (is_soc_a) {
    ...
} else if (is_soc_b) {
   ...
}
...

Then in order to support a new SoC, even if the driver doesn't need to
do anything different, you'd need to go and edit the code to add an "if
(is_soc_c)" condition into that list.

>> 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.

I personally don't think it's that confusing. A zero-length list is
after all still a list. That said, let me see if I can add such an
example to the binding document; the documentation does talk about this
case, but we can certainly add another example to highlight it.
--
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