Shawn Guo wrote at Tuesday, January 17, 2012 1:24 AM:

On Mon, Jan 16, 2012 at 12:50:02PM +0000, Dong Aisheng-B29396 wrote:

...

quoted

quoted

                         * 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.
                         *
                         * Alternative: Format is <&pmx_controller_phandle
                         * pmx_controller_specific_data>. In this case, the
                         * pmx controller needs to define #pinmux-mux-cells,
                         * and provide the pinctrl core with a mapping
                         * function to handle the rest of the data in the
                         * property. This is how GPIOs and interrupts work.
                         * However, this will probably interact badly with
                         * wanting to parse the entire pinmux map early in
                         * boot, when perhaps the pinctrl core is initialized,
                         * but the pinctrl driver itself is not.
                         */
                        mux =
                                <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
                                <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
                                /* Syntax example */
                                <&foo_pmx FOO_PMX_PG_X FOO_PMX_MUX_0>;

I'm still think how do we construct the pinmux map for such binding.
The format you're using is:
<&pmx_controller_phandle muxable_entity_id selected_function>
For contruct pinmux map, we need to know at least 3 things for a device:
a) pinctrl device b) function name c) group name.
For a, we can get it from this binding.
But for b and c, since they are constants, how to convert to name string?

I guess, for function name, it should be retrieved from the client
device node, and for the group name, it should be retrieved from the
node here.

For above example, the function name can be picked from sdhci device
node pinctr-names property I proposed, and the group name can just be
'pmx_sdhci_active', which is not a very nice name here and reminds me
the following point.

I responded directly to Dong's email re: how to map the DT values into
something for pinctrl. The mapping being described here isn't quite
what I had in mind....

Considering the different pinctrl configurations for the same client
device usually share the same pinmux and only pinconf varies.  It may
worth introducing another level phandle reference.  Something like
the following:

I don't think there's a need for another level of indirection. The 1:n
model I was talking about already handles this, I believe. See below.

	pinmux_sdhci: pinmux-sdhci {
		mux =
			<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
			<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
	};

	pinconf_sdhci_active: pinconf-sdhci-active {
		config =
			<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
			<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
			<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
			<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
	};

	pinconf_sdhci_suspend: pinconf-sdhci-suspend {
		config =
			<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
			<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
			<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
			<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
			<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
			<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
	};

Those 3 nodes make sense to me.

	pinctrl_sdhci_active: pinctrl-sdhci-active {
		pinmux = <&pinmux_sdhci>;
		pinconf = <&pinconf_sdhci_active>;
	};

	pinctrl_sdhci_suspend: pinctrl-sdhci-suspend {
		pinmux = <&pinmux_sdhci>;
		pinconf = <&pinconf_sdhci_suspend>;
	};

I think we can avoid those 2 nodes.

	sdhci@c8000200 {
		...
		pinctrl = <&pinctrl_sdhci_active> <&pinctrl_sdhci_suspend>;
		pinctrl-names = "active", "suspend";
	};

And rewrite that node as:

sdhci@c8000200 {
    ...
    pinctrl = <&pinmux_sdhci> <&pinconf_sdhci_active>
              <&pinmux_sdhci> <&pinconf_sdhci_suspend>;
    pinctrl-names = "active", "active", "suspend", "suspend";
};

The only slight disadvantage here is that the person constructing the
pinctrl/pinctrl-names properties needs to know to explicitly list both
the separate mux/config phandles for each value in pinctrl-names. Still,
this seems a reasonable compromise; the user is still picking from a
bunch of pre-defined/canned nodes, they simply need to list 2 (or n in
general) of them for each state.

This will be pretty useful for imx6 usdhc case, which will have 3
pinctrl configuration for each usdhc device (imx6 has 4 usdhc devices),
pinctrl-50mhz, pinctrl-100mhz and pinctrl-200mhz.  All these 3 states
have the exactly same pinmux settings, and only varies on pinconf.

Yes, I definitely agree there's a need for this.

As an aside, I wonder if the following would be any better:

sdhci@c8000200 {
    ...
    pinctrl = <&pinmux_sdhci> <&pinconf_sdhci_active>
              <&pinmux_sdhci> <&pinconf_sdhci_suspend>;
    /* Number of entries in pinctrl for each in pinctrl-names */
    pinctrl-entries = <2 2>;
    pinctrl-names = "active", "suspend";
};

That seems more complex though.

-- 
nvpublic