On Tue, Jan 17, 2012 at 10:47:14AM -0800, Stephen Warren wrote:

Shawn Guo wrote at Saturday, January 14, 2012 12:09 AM:

quoted

On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:

...

quoted

quoted

                /* 1:n example: */
                pinmux =
                        <"default" &pmx_sdhci_mux_a>
                        <"default" &pmx_sdhci_pincfg_a>
                        <"suspend" &pmx_sdhci_mux_a>
                        <"suspend" &pmx_sdhci_pincfg_a_suspend>;

I personally do not like the 1:n binding.  To me, any particular pinctrl
configuration, e.g. pmx_sdhci_active, should consist of a pair of pinmux
and pinconf, which should be given by the pmx_sdhci_active node.

Just a further explanation on my original code: I always intended that
each entry in that list was a full pinmux configuration that could include
mux and pin configuration settings. Thus, the above is more like:

pinctrl = 
    <"default" &pmx_sdhci_a>
    <"default" &pmx_sdhci_overrides>

(overrides rather than explicit separate mux/config; the separate mux
And config were just an example use case).

My thoughts here:

With this binding, we can certainly define a lot of pre-defined/canned
configurations to a set of pins. However, there are so many pin config
options (at least on Tegra) for different aspects of drive strength, slew
rate, ... that I sincerely doubt every single board is going to be able
to use one of those pre-defined/canned *exactly* without changes. The ways
to cope with these small board-specific differences are:

a) Cut/paste the entire pre-defined/canned configuration and tweak it
as necessary. You can do this with the 1:1 model.

b) Use the pre-defined/canned as a base, then modify it to add extra
configuration options, or change existing configuration options, as
appropriate. I think the 1:n model works best for this. Given previous
comments, I'd now propose the following syntax for a 1:n model:


    pinctrl = <&pmx_sdhci_a>, <&pmx_sdhci_overrides>, <&pmx_sdhci_suspend>;
    pinctrl-names = "default", "default", "suspend";

This should be easy to implement; instead of roughly:

prop = get_prop(np, "pinctrl-names");
index = find_index(prop, "default");
handle_pinctrl_prop(np, "pinctrl", index);

something more like:

prop = get_prop(np, "pinctrl-names");
prev = NULL;
while (find_index(prop, "default", &prev))
    handle_pinctrl_prop(np, "pinctrl", index);

Ok, I get it.  It seems a comprehensive design to me then.

...

quoted

quoted

                /*
                 * The actual definition of the complete state of the
                 * pinmux as required by some driver.
                 *
                 * These can be either directly in the device node, or
                 * somewhere in tegra20.dtsi in order to provide pre-
                 * selected/common configurations. Hence, they're referred
                 * to by phandle above.
                 */
                pmx_sdhci_active: {
                        /*
                         * Pin mux settings. Mandatory?
                         * Not mandatory if the 1:1 mentioned above is
                         * extended to 1:n.
                         *
                         * Format is <&pmx_controller_phandle muxable_entity_id
                         * selected_function>.
                         *
                         * The pmx phandle is required since there may be more
                         * than one pinmux controller in the system. Even if
                         * this node is inside the pinmux controller itself, I
                         * think it's simpler to just always have this field
                         * present in the binding for consistency.
                         *

I prefer to just put such nodes inside pinctrl controller itself and
drop those phandles.

My rationale here:

Forcing those nodes to be inside the controller node forces us to store
any board-specific custom configurations or overrides in the controller
node too; I'd simply prefer the flexibility to put them anywhere.

Hmm, this type of flexibility does not make too much point to me.  On
the contrary, it's good to have a centralized place for these nodes,
so that they can be well organized and people can find them easily.

Equally, I want SoC vendors to be able to choose whether to use these
pre-defined/canned configuration nodes at all. If you do use them, putting
them into the controller node makes perfect sense. If you don't use them,
putting the pin configuration nodes into the individual device nodes (in
the board.dts file) makes much more sense.

Having 'pinctrl' phandle point to a configuration node which is in the
same device node looks odd to me.  I'd rather define these configuration
nodes in <board>.dts still under controller node.

Having each property start with the phandle of the relevant controller
is also far more consistent with the way all the GPIO, IRQ, ... bindings
work.

The GPIO and IRQ gets only one level phandle reference from device node
to the destination, while you are proposing two levels of phandle
reference for pinctrl.  Having 'pinctrl' phandle point to the
configuration node which sits under controller node seems well aligned
with GPIO and IRQ etc to me.

-- 
Regards,
Shawn