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

[Dong Aisheng <dongas86@gmail.com>]

On Thu, Dec 29, 2011 at 10:46 AM, Shawn Guo <shawn.guo@freescale.com> wrote:
> On Tue, Dec 27, 2011 at 02:41:25PM +0000, Dong Aisheng-B29396 wrote:
> [...]
>> > 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.
>>
> Stephen suggested you put all the function nodes under node
> 'fsl,functions' (not sure prefix fsl is necessary).  In that case,
> of_get_child_count() on 'fsl,functions' still get you the function
> count you want, right?
>
> But we do not bother with it, if we do not have pinmux-usage at all.
> See below ...
>
>> 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.
>> 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.
>>
>> The same issue applies to dev using a phandle.
>>
> We do not bother with it, if we do not have pinmux-usage at all.
> See below ...
>
> [...]
>
>> 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.
>
> That's the way we should go for.  This is exactly the same thing that
> clk and regulator DT support is doing.  And right, in that way, we do
> not need pinmux mapping for DT at all.
>
I tried this way and basically it works but has a few limitations due to
via dt.
I will send out patch in this thread for discussion.

>> And it may need to implement a of_pinmux_get.
>
> To make the pinmux api generic for both dt and non-dt users, the pinmux
> client driver should still see/call pinmux_get, something like
> of_pinmux_get should be sorted out behind pinmux_get.
>
>> But what about the pin maps without device associated?
>>
> Do we have such cases?
>
It's hog_on_boot function.

Regards
Dong Aisheng