[PATCH] pinctrl: phandle entries will be applied sequentially

[PATCH] pinctrl: phandle entries will be applied sequentially

[Shawn Guo]

It's naturally expected that when there are multiple phandle entries
specified in pinctrl-* property, these entries will be applied
sequentially.  And this is how Linux kernel works.  So let's define
this behavior in the binding doc.

The behavior is useful when people want to reuse a group of predefined
pins with only minor configuration adjustment on one particular pin.
They will only need to add another entry after the predefined one with
needed configuration on that particular pin to overwrite the predefined
configuration.

Signed-off-by: Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
---
 .../bindings/pinctrl/pinctrl-bindings.txt          |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
index 1958ca9..404ba32 100644
--- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
+++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt
@@ -50,7 +50,13 @@ pinctrl-0:	List of phandles, each pointing at a pin configuration
 		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
+		contributing part of the overall configuration. These entries
+		will be applied sequentially. If there are multiple entries
+		contributing the configuration of the same pin, the latter
+		will overwrite the former. However, this 'overwrite' mechanism
+		should be used with the caution that it could cause some ill
+		effect, e.g. a glitch on the pin when pull down/up setting
+		gets flipped in this 'overwrite'.  See the next
 		section of this document for details of the format of these
 		pin configuration nodes.
 
-- 
1.7.9.5


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Linus Walleij]

On Wed, Oct 9, 2013 at 7:42 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> It's naturally expected that when there are multiple phandle entries
> specified in pinctrl-* property, these entries will be applied
> sequentially.  And this is how Linux kernel works.  So let's define
> this behavior in the binding doc.

NAK this this a Linux kernel implementation detail. Sherman Yin
fixed this up so the drivers does not have to imply any sequence
semantic for this.

Study commit 03b054e9696c3cbd3d5905ec96da15acd0a2fe8d
"pinctrl: Pass all configs to driver on pin_config_set()"

It is perfectly legal for a driver to accumulate the settings into
e.g. a single register write if it can, e.g.:

u32 val = 0;

for (i = 0; i < num_configs; i++) {
  switch() {
  FOO:
     val |= 1;
     break;
   BAR:
     val |= 2:
     break;
   BAZ:
     val |=4;
     break;
};

writel(val, base+pinreg);

I.e. the driver may choose to apply configs sequentially, but that is
not at all necessary.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Russell King - ARM Linux]

On Wed, Oct 09, 2013 at 02:40:49PM +0200, Linus Walleij wrote:
> NAK this this a Linux kernel implementation detail. Sherman Yin
> fixed this up so the drivers does not have to imply any sequence
> semantic for this.
> 
> Study commit 03b054e9696c3cbd3d5905ec96da15acd0a2fe8d
> "pinctrl: Pass all configs to driver on pin_config_set()"
> 
> It is perfectly legal for a driver to accumulate the settings into
> e.g. a single register write if it can, e.g.:
> 
> u32 val = 0;
> 
> for (i = 0; i < num_configs; i++) {
>   switch() {
>   FOO:
>      val |= 1;
>      break;
>    BAR:
>      val |= 2:
>      break;
>    BAZ:
>      val |=4;
>      break;
> };
> 
> writel(val, base+pinreg);
> 
> I.e. the driver may choose to apply configs sequentially, but that is
> not at all necessary.

So, just to be clear, what you're saying is that specifying two settings
in a pinctrl declaration which provide different values results in
undefined behaviour?
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Linus Walleij]

On Wed, Oct 9, 2013 at 2:44 PM, Russell King - ARM Linux
<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
> On Wed, Oct 09, 2013 at 02:40:49PM +0200, Linus Walleij wrote:

>> NAK this this a Linux kernel implementation detail. Sherman Yin
>> fixed this up so the drivers does not have to imply any sequence
>> semantic for this.
>>
>> Study commit 03b054e9696c3cbd3d5905ec96da15acd0a2fe8d
>> "pinctrl: Pass all configs to driver on pin_config_set()"
>>
>> It is perfectly legal for a driver to accumulate the settings into
>> e.g. a single register write if it can, e.g.:
>>
>> u32 val = 0;
>>
>> for (i = 0; i < num_configs; i++) {
>>   switch() {
>>   FOO:
>>      val |= 1;
>>      break;
>>    BAR:
>>      val |= 2:
>>      break;
>>    BAZ:
>>      val |=4;
>>      break;
>> };
>>
>> writel(val, base+pinreg);
>>
>> I.e. the driver may choose to apply configs sequentially, but that is
>> not at all necessary.
>
> So, just to be clear, what you're saying is that specifying two settings
> in a pinctrl declaration which provide different values results in
> undefined behaviour?

It's more like the pin control core is passing the array of settings
to the driver and the behaviour is specified per-driver.

So that is from the kernels point of view, no matter whether
device tree is used or not. A specific driver may instill specific
behaviour - sequential or not.

Sherman's driver for the Broadcom Capri had the requirement that
the configuration of all parameters be done with a single register
write to avoid side-effects. So it needs to iterate the configs and
build up a mask and value and write it in one go.

When I look at the i.MX driver (I guess this is what Shawn
is working on here) it can actually be augmented to do it
the same way and avoid this mess altogether:

       for (i = 0; i < num_configs; i++) {
                if (info->flags & SHARE_MUX_CONF_REG) {
                        u32 reg;
                        reg = readl(ipctl->base + pin_reg->conf_reg);
                        reg &= ~0xffff;
                        reg |= configs[i];
                        writel(reg, ipctl->base + pin_reg->conf_reg);
                } else {
                        writel(configs[i], ipctl->base + pin_reg->conf_reg);
                }
                dev_dbg(ipctl->dev, "write: offset 0x%x val 0x%lx\n",
                        pin_reg->conf_reg, configs[i]);
        } /* for each config */

As can be seen it will just writel() the config into the register
for each config, it seems more like there is a bug that also
the else-clause here should be read-modify-write, or am
I getting it all wrong?

In some cases some configs may be electrically conflicting
and completely unsound, like enabling pull-up and pull-down
at the same time - maybe possible in the hardware
but insane, so the driver should detect that state in this
loop and reject such configs, print a warning or make a
best effort.

I am aware that maybe not all drivers are handling this in
the most ultimate way today :-/

Then as this patch was against the device tree documentation:
since the device tree isn't really about sequencing things
(Grant has been onto us in the past for wanting to put sequences
in there) we should probably avoid defining such things as
sequences at all, as that basically moves the hardware
description toward a sequence description language (jam table),
and for that the full open firmware or other BIOS thing (ACPI?)
should be used instead. (If I understood the point correctly.)

I tend to think about the DT language as functional i.e. there
is no way to say anything about sequencing order from it,
I may be wrong here though.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Russell King - ARM Linux]

On Wed, Oct 09, 2013 at 03:09:21PM +0200, Linus Walleij wrote:
> It's more like the pin control core is passing the array of settings
> to the driver and the behaviour is specified per-driver.
> 
> So that is from the kernels point of view, no matter whether
> device tree is used or not. A specific driver may instill specific
> behaviour - sequential or not.

Right, so that means doing this:

                        pinctrl_usdhc1_1: usdhc1grp-1 {
                                fsl,pins = <
...
                                        MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x17059
...
                                >;
                        };

                        pinctrl_usdhc1_1_dat3cd: usdhc1grp-3 {
                                fsl,pins = <
                                        MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x13059
                                >;
                        };

and then:

	pinctrl-0 = <&pinctrl_usdhc1_1 &pinctrl_usdhc1_1_dat3cd>;

can result in either "MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x17059" or
"MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x13059" being the final configuration
for that pin.

What that means is that for any pinctrl setting, pins to be configured
must be mentioned exactly once and once only.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Linus Walleij]

On Wed, Oct 9, 2013 at 3:45 PM, Russell King - ARM Linux
<linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:

> Right, so that means doing this:
>
>                         pinctrl_usdhc1_1: usdhc1grp-1 {
>                                 fsl,pins = <
> ...
>                                         MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x17059
> ...
>                                 >;
>                         };
>
>                         pinctrl_usdhc1_1_dat3cd: usdhc1grp-3 {
>                                 fsl,pins = <
>                                         MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x13059
>                                 >;
>                         };
>
> and then:
>
>         pinctrl-0 = <&pinctrl_usdhc1_1 &pinctrl_usdhc1_1_dat3cd>;
>
> can result in either "MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x17059" or
> "MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x13059" being the final configuration
> for that pin.

I'm uncertain about the device tree case. I guess it is indeed
an array of handles indexed [0,1], the framework will only
send that array down to the driver, no strings attached.

> What that means is that for any pinctrl setting, pins to be configured
> must be mentioned exactly once and once only.

Either that (and the above does seem very ambigous) or the driver
need to combine and handle the conflicting configurations,
maintaining a state for the pin and so on ...

This seems like two *complete* configs fighting over the same pin
rather than two complementary configs that we could | together
and write, and that is indeed ambigous. The array passed to
the config function is supposed to be individual settings like
[ pull-up, schmitt-trigger ] not two complete sets of config.

The generic pin config binding is luckily  more clear than this
i.MX-specific handling.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Stephen Warren]

On 10/09/2013 06:44 AM, Russell King - ARM Linux wrote:
> On Wed, Oct 09, 2013 at 02:40:49PM +0200, Linus Walleij wrote:
>> NAK this this a Linux kernel implementation detail. Sherman Yin
>> fixed this up so the drivers does not have to imply any sequence
>> semantic for this.
>>
>> Study commit 03b054e9696c3cbd3d5905ec96da15acd0a2fe8d
>> "pinctrl: Pass all configs to driver on pin_config_set()"
>>
>> It is perfectly legal for a driver to accumulate the settings into
>> e.g. a single register write if it can, e.g.:
>>
>> u32 val = 0;
>>
>> for (i = 0; i < num_configs; i++) {
>>   switch() {
>>   FOO:
>>      val |= 1;
>>      break;
>>    BAR:
>>      val |= 2:
>>      break;
>>    BAZ:
>>      val |=4;
>>      break;
>> };
>>
>> writel(val, base+pinreg);
>>
>> I.e. the driver may choose to apply configs sequentially, but that is
>> not at all necessary.
> 
> So, just to be clear, what you're saying is that specifying two settings
> in a pinctrl declaration which provide different values results in
> undefined behaviour?

That makes sense to me, yes. It should be simple to separate out the
common/shared parts of a configuration, vs. other unique parts, and put
them into separate "pin configuration nodes", and hence avoid the
situation completely.

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Shawn Guo]

On Wed, Oct 09, 2013 at 02:40:49PM +0200, Linus Walleij wrote:
> On Wed, Oct 9, 2013 at 7:42 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> > It's naturally expected that when there are multiple phandle entries
> > specified in pinctrl-* property, these entries will be applied
> > sequentially.  And this is how Linux kernel works.  So let's define
> > this behavior in the binding doc.
> 
> NAK this this a Linux kernel implementation detail. Sherman Yin
> fixed this up so the drivers does not have to imply any sequence
> semantic for this.
> 
> Study commit 03b054e9696c3cbd3d5905ec96da15acd0a2fe8d
> "pinctrl: Pass all configs to driver on pin_config_set()"
> 
> It is perfectly legal for a driver to accumulate the settings into
> e.g. a single register write if it can, e.g.:
> 
> u32 val = 0;
> 
> for (i = 0; i < num_configs; i++) {
>   switch() {
>   FOO:
>      val |= 1;
>      break;
>    BAR:
>      val |= 2:
>      break;
>    BAZ:
>      val |=4;
>      break;
> };
> 
> writel(val, base+pinreg);
> 
> I.e. the driver may choose to apply configs sequentially, but that is
> not at all necessary.

Yes, that's the case when pinconf_apply_setting() is called to apply a
struct pinctrl_setting.  This pinctrl_setting may contain multiple
configs (pull, drive, open drain etc.) for a pin, and driver may choose
to apply these configs sequentially or accumulatedly.  In case of device
tree, all these configs must be coming from a single pin configuration
node, as per my understanding of how pinctrl core code maps pin
configuration in device tree node to struct pinctrl_setting.

However, my patch is talking about a different thing.  For example, we
have a device whose pinctrl-0 consists of two phandle entries that point
to pin configuration nodes foo and bar.

	pinctrl-0 = <&foo &bar>;

	foo {
		...
	};

	bar {
		...
	};

My patch only wants to make it clear that the configuration specified by
node foo will be applied to pin controller first, and the configuration
defined in node bar will be applied after that.  When both nodes have
configuration for a pin, these two configs for the same pin go to two
different pinctrl_setting structures.  And these two pinctrl_settings
can not be applied accumulatedly but only sequentially.  That's what my
patch talks about.

Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Linus Walleij]

On Thu, Oct 10, 2013 at 9:26 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:

> However, my patch is talking about a different thing.  For example, we
> have a device whose pinctrl-0 consists of two phandle entries that point
> to pin configuration nodes foo and bar.
>
>         pinctrl-0 = <&foo &bar>;
>
>         foo {
>                 ...
>         };
>
>         bar {
>                 ...
>         };
>
> My patch only wants to make it clear that the configuration specified by
> node foo will be applied to pin controller first, and the configuration
> defined in node bar will be applied after that.  When both nodes have
> configuration for a pin, these two configs for the same pin go to two
> different pinctrl_setting structures.  And these two pinctrl_settings
> can not be applied accumulatedly but only sequentially.  That's what my
> patch talks about.

OK I get it (I think).

However isn't this aspect rather a property of how the specific driver
parses and uses the device tree?

Remember that with drivers that do not use generic pin config,
we have left the handling of the device tree node up to the driver
by the callback dt_node_to_map().

But I do understand the ambition to specify this behaviour for all
pin control drivers using device tree.

But we need to be careful since DT isn't very much in the
buisiness of defining sequence semantics for stuff, and that
is why I'm a bit hesitant.

So, maybe we can add this as it only concerns the binding
(but then I want that example to be part of the documentation patch
so that it is crystal clear what we're talking about), but it would be
proper if we also helped enforce this semantic across the drivers,
i.e. centralize more of the DT parsing code...

BTW on a related subject: have you examined if the i.MX driver
can use the new utility functions from pinctrl-utils.h:

pinctrl_utils_reserve_map()
pinctrl_utils_add_map_mux()
pinctrl_utils_add_map_configs()
pinctrl_utils_add_config()
pinctrl_utils_dt_free_map()

It helps a lot the more boilerplate we can move out of the
drivers and get a clear picture of the device tree parsing code,
and see what really needs to be different across the drivers.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Shawn Guo]

On Wed, Oct 09, 2013 at 03:09:21PM +0200, Linus Walleij wrote:
> When I look at the i.MX driver (I guess this is what Shawn
> is working on here) it can actually be augmented to do it
> the same way and avoid this mess altogether:
> 
>        for (i = 0; i < num_configs; i++) {
>                 if (info->flags & SHARE_MUX_CONF_REG) {
>                         u32 reg;
>                         reg = readl(ipctl->base + pin_reg->conf_reg);
>                         reg &= ~0xffff;
>                         reg |= configs[i];
>                         writel(reg, ipctl->base + pin_reg->conf_reg);
>                 } else {
>                         writel(configs[i], ipctl->base + pin_reg->conf_reg);
>                 }
>                 dev_dbg(ipctl->dev, "write: offset 0x%x val 0x%lx\n",
>                         pin_reg->conf_reg, configs[i]);
>         } /* for each config */
> 
> As can be seen it will just writel() the config into the register
> for each config, it seems more like there is a bug that also
> the else-clause here should be read-modify-write, or am
> I getting it all wrong?

On imx, num_configs is always 1 (new_map[j].data.configs.num_configs
is hard-coded as 1 in imx_dt_node_to_map() function).  We have all
configs for given pin encoded in the last integer of fsl,pin entry.
And this integer can be directly written into conf_reg.

Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Russell King - ARM Linux]

On Thu, Oct 10, 2013 at 03:26:26PM +0800, Shawn Guo wrote:
> However, my patch is talking about a different thing.  For example, we
> have a device whose pinctrl-0 consists of two phandle entries that point
> to pin configuration nodes foo and bar.
> 
> 	pinctrl-0 = <&foo &bar>;
> 
> 	foo {
> 		...
> 	};
> 
> 	bar {
> 		...
> 	};
> 
> My patch only wants to make it clear that the configuration specified by
> node foo will be applied to pin controller first, and the configuration
> defined in node bar will be applied after that.  When both nodes have
> configuration for a pin, these two configs for the same pin go to two
> different pinctrl_setting structures.  And these two pinctrl_settings
> can not be applied accumulatedly but only sequentially.  That's what my
> patch talks about.

I still say this is a potentially dangerous thing, and in my case of
overriding the DAT3 pull-sense, it will cause the pin to glitch if
nothing is connected to it.

So even if you do get this clarified, I am *not* happy to change my
patch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Shawn Guo]

On Thu, Oct 10, 2013 at 10:03:40AM +0200, Linus Walleij wrote:
> On Thu, Oct 10, 2013 at 9:26 AM, Shawn Guo <shawn.guo-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> 
> > However, my patch is talking about a different thing.  For example, we
> > have a device whose pinctrl-0 consists of two phandle entries that point
> > to pin configuration nodes foo and bar.
> >
> >         pinctrl-0 = <&foo &bar>;
> >
> >         foo {
> >                 ...
> >         };
> >
> >         bar {
> >                 ...
> >         };
> >
> > My patch only wants to make it clear that the configuration specified by
> > node foo will be applied to pin controller first, and the configuration
> > defined in node bar will be applied after that.  When both nodes have
> > configuration for a pin, these two configs for the same pin go to two
> > different pinctrl_setting structures.  And these two pinctrl_settings
> > can not be applied accumulatedly but only sequentially.  That's what my
> > patch talks about.
> 
> OK I get it (I think).
> 
> However isn't this aspect rather a property of how the specific driver
> parses and uses the device tree?

The 'pinctrl-0' property is defined by common pinctrl device tree
binding and is parsed by pinctrl core, while pin configuration node
is platform specific and handled by controller driver.

> Remember that with drivers that do not use generic pin config,
> we have left the handling of the device tree node up to the driver
> by the callback dt_node_to_map().

Yes.  dt_node_to_map() takes pin configuration node pointer as input and
return a set of struct pinctrl_map.  But how pinctrl_map eventually gets
turned into pinctrl_setting as the input to pinconf_apply_setting() call
are all handled by pinctrl core and common across different controller
drivers.

> But I do understand the ambition to specify this behaviour for all
> pin control drivers using device tree.
> 
> But we need to be careful since DT isn't very much in the
> buisiness of defining sequence semantics for stuff, and that
> is why I'm a bit hesitant.
> 
> So, maybe we can add this as it only concerns the binding
> (but then I want that example to be part of the documentation patch
> so that it is crystal clear what we're talking about), but it would be
> proper if we also helped enforce this semantic across the drivers,
> i.e. centralize more of the DT parsing code...

Ok.  I would hold it off for a while and consider Stephen's comment
first for solving the problem we have on i.MX.

> BTW on a related subject: have you examined if the i.MX driver
> can use the new utility functions from pinctrl-utils.h:
> 
> pinctrl_utils_reserve_map()
> pinctrl_utils_add_map_mux()
> pinctrl_utils_add_map_configs()
> pinctrl_utils_add_config()
> pinctrl_utils_dt_free_map()

Thanks for the info.  Just had a quick look at them, and it seems that
they don't save too much code for imx.  Will take another deep look
though.

Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Shawn Guo]

On Thu, Oct 10, 2013 at 11:08:40AM +0100, Russell King - ARM Linux wrote:
> On Thu, Oct 10, 2013 at 03:26:26PM +0800, Shawn Guo wrote:
> > However, my patch is talking about a different thing.  For example, we
> > have a device whose pinctrl-0 consists of two phandle entries that point
> > to pin configuration nodes foo and bar.
> > 
> > 	pinctrl-0 = <&foo &bar>;
> > 
> > 	foo {
> > 		...
> > 	};
> > 
> > 	bar {
> > 		...
> > 	};
> > 
> > My patch only wants to make it clear that the configuration specified by
> > node foo will be applied to pin controller first, and the configuration
> > defined in node bar will be applied after that.  When both nodes have
> > configuration for a pin, these two configs for the same pin go to two
> > different pinctrl_setting structures.  And these two pinctrl_settings
> > can not be applied accumulatedly but only sequentially.  That's what my
> > patch talks about.
> 
> I still say this is a potentially dangerous thing, and in my case of
> overriding the DAT3 pull-sense, it will cause the pin to glitch if
> nothing is connected to it.
> 
> So even if you do get this clarified, I am *not* happy to change my
> patch.

What about the solution suggested by Stephen, moving
MX6QDL_PAD_SD1_DAT3__SD1_DATA3 out from pinctrl_usdhc1_1 and having
additional nodes/phandle for DAT3 with different settings?

diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
index 59154dc..fd52f4e 100644
--- a/arch/arm/boot/dts/imx6qdl.dtsi
+++ b/arch/arm/boot/dts/imx6qdl.dtsi
@@ -1157,7 +1157,6 @@
                                                        MX6QDL_PAD_SD1_DAT0__SD1_DATA0 0x17059
                                                        MX6QDL_PAD_SD1_DAT1__SD1_DATA1 0x17059
                                                        MX6QDL_PAD_SD1_DAT2__SD1_DATA2 0x17059
-                                                       MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x17059
                                                        MX6QDL_PAD_NANDF_D0__SD1_DATA4 0x17059
                                                        MX6QDL_PAD_NANDF_D1__SD1_DATA5 0x17059
                                                        MX6QDL_PAD_NANDF_D2__SD1_DATA6 0x17059
@@ -1165,6 +1164,18 @@
                                                >;
                                        };

+                                       pinctrl_usdhc1_1_dat3: usdhc1dat3-1 {
+                                               fsl,pins = <
+                                                       MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x17059
+                                               >;
+                                       };
+
+                                       pinctrl_usdhc1_1_dat3cd: usdhc1dat3cd-1 {
+                                               fsl,pins = <
+                                                       MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x13059
+                                               >;
+                                       };

Then pinctrl-0 = <&pinctrl_usdhc1_1 &pinctrl_usdhc1_1_dat3> for existing
boards, and <&pinctrl_usdhc1_1 &pinctrl_usdhc1_1_dat3cd> for boards that
want to use DAT3 for card detection.

Shawn

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pinctrl: phandle entries will be applied sequentially

[Russell King - ARM Linux]

On Thu, Oct 10, 2013 at 06:22:07PM +0800, Shawn Guo wrote:
> What about the solution suggested by Stephen, moving
> MX6QDL_PAD_SD1_DAT3__SD1_DATA3 out from pinctrl_usdhc1_1 and having
> additional nodes/phandle for DAT3 with different settings?
> 
> diff --git a/arch/arm/boot/dts/imx6qdl.dtsi b/arch/arm/boot/dts/imx6qdl.dtsi
> index 59154dc..fd52f4e 100644
> --- a/arch/arm/boot/dts/imx6qdl.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl.dtsi
> @@ -1157,7 +1157,6 @@
>                                                         MX6QDL_PAD_SD1_DAT0__SD1_DATA0 0x17059
>                                                         MX6QDL_PAD_SD1_DAT1__SD1_DATA1 0x17059
>                                                         MX6QDL_PAD_SD1_DAT2__SD1_DATA2 0x17059
> -                                                       MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x17059
>                                                         MX6QDL_PAD_NANDF_D0__SD1_DATA4 0x17059
>                                                         MX6QDL_PAD_NANDF_D1__SD1_DATA5 0x17059
>                                                         MX6QDL_PAD_NANDF_D2__SD1_DATA6 0x17059
> @@ -1165,6 +1164,18 @@
>                                                 >;
>                                         };
> 
> +                                       pinctrl_usdhc1_1_dat3: usdhc1dat3-1 {
> +                                               fsl,pins = <
> +                                                       MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x17059
> +                                               >;
> +                                       };
> +
> +                                       pinctrl_usdhc1_1_dat3cd: usdhc1dat3cd-1 {
> +                                               fsl,pins = <
> +                                                       MX6QDL_PAD_SD1_DAT3__SD1_DATA3 0x13059
> +                                               >;
> +                                       };
> 
> Then pinctrl-0 = <&pinctrl_usdhc1_1 &pinctrl_usdhc1_1_dat3> for existing
> boards, and <&pinctrl_usdhc1_1 &pinctrl_usdhc1_1_dat3cd> for boards that
> want to use DAT3 for card detection.

Yes, that avoids the issue entirely, and is a much better solution IMHO.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html