Messages in this thread | | | Date | Thu, 15 Mar 2012 11:32:58 +0800 | From | Dong Aisheng <> | Subject | Re: [PATCH] dt: pinctrl: Document device tree binding |
| |
On Wed, Mar 14, 2012 at 03:27:26AM +0800, Stephen Warren wrote: > 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. > Good point to me. The driver does not need to know which SoC it's included in, but it has to support that SoC first.
> >> 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. > Hmm, whatever platform data or device tree or device id, we can use a way to tell driver which SoC it is running on. The driver private is_soc_a macro or function can be implemented based that information.
> > 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. > I guess i might not describe my idea clearly. My idea is that the compatible string does that work. For example: static const struct of_device_id mxs_mmc_dt_ids[] = { { .compatible = "fsl,imx23-mmc", .data = NULL, }, { .compatible = "fsl,imx28-mmc", .data = NULL, }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, mxs_mmc_dt_ids); Replace NULL to some special data like SOC_MX23 or SOC_MX28 can let driver driver know which SoC it's running on. Then driver can use private macro like is_soc_foo.
> >> 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. > No. No changes needed if the driver does not need to do anything different. Compatible string does that work For example, in a new soc which is fully compatible with current driver. It can just add device like: mmc@80010000 { compatible = "fsl, xxx-mmc", "fsl,imx28-mmc"; reg = <..>; ... }
> >> 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. > If dt does allow this, i'm also ok with it.
Regards Dong Aisheng
| |