RE: [RFC PATCH v3 2/5] pinctrl: add dt binding support for pinmux mappings

[Stephen Warren <swarren@nvidia.com>]

Dong Aisheng-B29396 wrote at Tuesday, December 27, 2011 7:41 AM:
> Stephen Warren wrote at Sunday, December 25, 2011 11:37 AM:
> > Dong Aisheng-B29396 wrote at Thursday, December 22, 2011 1:18 AM:
> > > Stephen Warren wrote at Wednesday, December 21, 2011 8:39 AM:
> > > > Dong Aisheng wrote at Tuesday, December 20, 2011 10:41 AM:
> > > > > This patch provies a common API for driver to use to register
> > > > > pinmux mappings from dts file. It is needed for dt support.
> > > >
> > > > Dong,
> > > >
> > > > Thanks for providing a concrete binding to get discussion started. I
> > > > had intended to do this myself, but got side-tracked on other things.
> > > >
> > > > I'll comment exclusively on the documentation in this patch, since
> > > > any changes there will require the code to be changed.
...
> > > > b) The top-level node ("imx6q-sabreauto-map" below) is not tied to
> > > > any particular piece of hardware (no compatible flag, not reg
> > > > property etc.)
> > >
> > > My original purpose was that it was not a hw device node, it was just
> > > created for containing pin map information and referenced by a phandle
> > > in iomuxc deivce node
> > > like:
> > >
> > > iomuxc@020e0000 {
> > >         compatible = "fsl,imx6q-iomuxc";
> > >         reg = <0x020e0000 0x4000>;
> > >         fsl,pinmux-map = <&pinmux>;
> > > 	  ...
> > > }
> > >
> > > I could also make it as a sub node of iomuxc, but original design is
> > > that sub nodes of Ioxmuc are already representing each pinmux functions.
> > > To avoid conflict, I put the imx6q-sabreauto-map node out of iomuxc
> > > since a phandle (fsl,pinmux-map ) does the same work well.
> >
> > You could have nested sub-nodes, say something like:
> >
> > iomuxc@020e0000 {
> >     compatible = "fsl,imx6q-iomuxc";
> >     reg = <0x020e0000 0x4000>;
> >     fsl,pins {
> >         ... // fsl-specific pin properties
> >     };
> >     fsl,groups {
> >         ... // fsl-specific group properties
> >     };
> >     fsl,functions {
> >         ... // fsl-specific mux function properties
> >     };
> >     pinmux-usage {
> >         // standardized pinmux "table" properties
> >         sd4 { // pin group name
> >             function = "sdio4"; // this function active on it
> >             ...
> >         };
> >         ...
> >     }
> >     ...
> > }
> >
> Yes, I could do that.
> The extra effort is that we have to manually exclude one pinmux-usage
> node in pinmux driver since originally i take the child node count of
> iomuxc as the function count since all child nodes are functions,
> that why I firstly took the mapping node out of iomuxc, in addition
> the old way i used seems to be more brief and clear.
> 
> I tried adding pinmux-usage as a sub node of iomuxc and got two issues:
> Taking imx6q as an example:
>
> iomuxc@020e0000 {
>         uart4 {
>                 func-name = "uart4";
>                 grp-name = "uart4grp";
>                 grp-pins = <107 108>;
>                 num-pins = <2>;
>                 grp-mux = <4 4>;
>                 num-mux = <2>;
>         };
> 
>         sd4 {
>                 func-name = "sd4";
>                 grp-name = "sd4grp";
>                 grp-pins = <170 171 180 181 182 183 184 185 186 187>;
>                 num-pins = <10>;
>                 grp-mux = <0 0 1 1 1 1 1 1 1 1>;
>                 num-mux = <10>;
>         };
> 
>         pinmux-mapping {
>                 uart4 {
>                         function = "uart4";
>                         dev = <&uart4>;
>                 };
> 
>                 usdhc4 {
> /*			    ctrl-dev-name = "20e0000.iomuxc"; */
>                         function = "sd4";
>                         dev = <&usdhc4>;
>                 };
>         };
> };
> 
> If we remove ctrl-dev-name and get it from its parent node in drivers as
> you suggested,
> first, since this work will be done in the common API (pinmux_of_register_mappings
> Requires pass the pinmux mapping node to it for construct the mapping table)
> It will introduce a restriction that all platforms should define this node
> under their pinctrl device node.

This seems reasonable to me; the pinmux controller would be defining the
mux options/... that it's providing to the rest of the DT.

> Second, it seems we cannot get its parent node device or device name in driver
> (can only get node Name which is not sufficient for construct the pin map table
> required by pinctrl core) and current device tree subsystem seems do not support
> get its associated device from a device node.
> We may not be able to construct ctrl-dev-name or ctrl-dev for struct pinmux_map.
> I'm not sure if I missed something, if missed please let me know.
> To support it, we may need to add support for converting device node
> to device. Not sure it's applicable since I still have not tried it.

That functionality is pretty easy to implement; it already exists for
other subsystems that refer to parent/controller/... nodes using phandles.
for example, take a look at the implementation of of_node_to_gpiochip()
in drivers/of/gpio.c, which calls gpiochip_find() with a custom match
function that simply compares the gpio_chip's of_node field. Once you
have the chip, you have the device, and can store that in the constructed
mapping table, or call dev_name() on it. I assume there's something similar
for interrupts.

...
> > > The same question applies to clock and regulator: does each device
> > > need Define its clock and regulator properties?
> >
> > Yes, in those cases I believe each device does contain a phandle to the
> > resources it uses.
>
> I was ever thought putting a phandle of pinmux function in each device,
> Then pinmux mapping table seems not need anymore. Like:
> 
> usdhc4: usdhc@0219c000 { /* uSDHC4 */
>         compatible = "fsl,imx6q-usdhc";
> 	....
>         pinmux = <&pinmux-sd4>;
> };
> 
> iomuxc@020e0000 {
>         pinmux-sd4 : sd4 {
>                 func-name = "sd4";
>                 grp-name = "sd4grp";
>                 grp-pins = <170 171 180 181 182 183 184 185 186 187>;
>                 num-pins = <10>;
>                 grp-mux = <0 0 1 1 1 1 1 1 1 1>;
>                 num-mux = <10>;
>         };
> 
> It is a pure hw point of view to define node.
> And it may need to implement a of_pinmux_get.

> But what about the pin maps without device associated?

Indeed; that's why I'd tend towards defining a table of pinmux usage in
the pinmux node, and having other devices refer to that table.

Still, if the pinmux definitions are in the device nodes, we could simply
make the pinmux controller have such a definition itself too, for the
"system hog" case.

-- 
nvpublic