[PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Guennadi Liakhovetski]

This patch adds a new function to allow clients to verify, whether a
certain group is selected in the currently active setting or not.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

Maybe there is already a way to do this without adding a new function, I 
haven't found one, hence this patch.

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index ec3b8cc18..4e768d4 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -310,6 +310,32 @@ void pinctrl_remove_gpio_range(struct pinctrl_dev *pctldev,
 EXPORT_SYMBOL_GPL(pinctrl_remove_gpio_range);
 
 /**
+ * pinctrl_mux_group_selected() - check, whether a group is currently selected
+ */
+bool pinctrl_mux_group_selected(struct pinctrl *p, const char *group)
+{
+	struct pinctrl_setting *setting;
+
+	list_for_each_entry(setting, &p->state->settings, node) {
+		struct pinctrl_dev *pctldev = setting->pctldev;
+		const struct pinctrl_ops *ops = pctldev->desc->pctlops;
+		const char *gname;
+
+		if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
+			continue;
+
+		gname = ops->get_group_name(pctldev, setting->data.mux.group);
+		if (!strcmp(gname, group)) {
+			dev_dbg(setting->pctldev->dev, "%s(): group %s found\n", __func__, group);
+			return true;
+		}
+	}
+	pr_debug("%s(): group %s not found\n", __func__, group);
+	return false;
+}
+EXPORT_SYMBOL_GPL(pinctrl_mux_group_selected);
+
+/**
  * pinctrl_get_group_selector() - returns the group selector for a group
  * @pctldev: the pin controller handling the group
  * @pin_group: the pin group to look up
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
index 191e726..31d6c6f 100644
--- a/include/linux/pinctrl/consumer.h
+++ b/include/linux/pinctrl/consumer.h
@@ -35,6 +35,7 @@ extern struct pinctrl_state * __must_check pinctrl_lookup_state(
 							struct pinctrl *p,
 							const char *name);
 extern int pinctrl_select_state(struct pinctrl *p, struct pinctrl_state *s);
+extern bool pinctrl_mux_group_selected(struct pinctrl *p, const char *group);
 
 extern struct pinctrl * __must_check devm_pinctrl_get(struct device *dev);
 extern void devm_pinctrl_put(struct pinctrl *p);

Re: [PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Stephen Warren]

On 05/22/2012 04:23 PM, Guennadi Liakhovetski wrote:
> This patch adds a new function to allow clients to verify, whether a
> certain group is selected in the currently active setting or not.
> 
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
> 
> Maybe there is already a way to do this without adding a new function, I 
> haven't found one, hence this patch.

This function doesn't currently exist because it breaks the pinctrl
conceptual model, which is that devices ask pinctrl for certain settings
whenever they need them, rather than information flowing the other way.

What's the use case for this new functionality?

Re: [PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Guennadi Liakhovetski]

On Wed, 23 May 2012, Stephen Warren wrote:

> On 05/22/2012 04:23 PM, Guennadi Liakhovetski wrote:
> > This patch adds a new function to allow clients to verify, whether a
> > certain group is selected in the currently active setting or not.
> > 
> > Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> > ---
> > 
> > Maybe there is already a way to do this without adding a new function, I 
> > haven't found one, hence this patch.
> 
> This function doesn't currently exist because it breaks the pinctrl
> conceptual model, which is that devices ask pinctrl for certain settings
> whenever they need them, rather than information flowing the other way.

Well, yes, we could tra to ask for each thinkable configuration and see 
which one(s) succeed, but that doesn't seem very optimal either, even if 
we only have 3 possibilities for now.

> What's the use case for this new functionality?

It's MMC. MMC interfaces can use 1, 4, or 8 data lines, depending on the 
board configuration. The board knows, that it has, say, only 4 data lines 
routed to the interface, so, it specifies the respective pinctrl 
configuration as default. Now in the driver we have to know how many 
data-lines are connected.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Stephen Warren]

On 05/23/2012 09:03 AM, Guennadi Liakhovetski wrote:
> On Wed, 23 May 2012, Stephen Warren wrote:
> 
>> On 05/22/2012 04:23 PM, Guennadi Liakhovetski wrote:
>>> This patch adds a new function to allow clients to verify, whether a
>>> certain group is selected in the currently active setting or not.
>>>
>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>> ---
>>>
>>> Maybe there is already a way to do this without adding a new function, I 
>>> haven't found one, hence this patch.
>>
>> This function doesn't currently exist because it breaks the pinctrl
>> conceptual model, which is that devices ask pinctrl for certain settings
>> whenever they need them, rather than information flowing the other way.
> 
> Well, yes, we could tra to ask for each thinkable configuration and see 
> which one(s) succeed, but that doesn't seem very optimal either, even if 
> we only have 3 possibilities for now.
> 
>> What's the use case for this new functionality?
> 
> It's MMC. MMC interfaces can use 1, 4, or 8 data lines, depending on the 
> board configuration. The board knows, that it has, say, only 4 data lines 
> routed to the interface, so, it specifies the respective pinctrl 
> configuration as default. Now in the driver we have to know how many 
> data-lines are connected.

If using device tree, the bus-width property should be used. If not
using device tree, presumably you'd add an equivalent field to the
platform data.

An alternative (only when not using DT, since there's already a property
for DT) is that the driver doesn't select pinctrl state "default", but
instead first looks for e.g. "8bit" and if found uses it, then falls
back to "4bit", then falls back to "1bit". Whichever state name is
defined indicates which bus width is available.

Re: [PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Linus Walleij]

On Wed, May 23, 2012 at 5:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/23/2012 09:03 AM, Guennadi Liakhovetski wrote:
>> On Wed, 23 May 2012, Stephen Warren wrote:
>>
>>> On 05/22/2012 04:23 PM, Guennadi Liakhovetski wrote:
>>>> This patch adds a new function to allow clients to verify, whether a
>>>> certain group is selected in the currently active setting or not.
>>>>
>>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>>> ---
>>>>
>>>> Maybe there is already a way to do this without adding a new function, I
>>>> haven't found one, hence this patch.
>>>
>>> This function doesn't currently exist because it breaks the pinctrl
>>> conceptual model, which is that devices ask pinctrl for certain settings
>>> whenever they need them, rather than information flowing the other way.
>>
>> Well, yes, we could tra to ask for each thinkable configuration and see
>> which one(s) succeed, but that doesn't seem very optimal either, even if
>> we only have 3 possibilities for now.
>>
>>> What's the use case for this new functionality?
>>
>> It's MMC. MMC interfaces can use 1, 4, or 8 data lines, depending on the
>> board configuration. The board knows, that it has, say, only 4 data lines
>> routed to the interface, so, it specifies the respective pinctrl
>> configuration as default. Now in the driver we have to know how many
>> data-lines are connected.

I discussed the same scenario the other day, and I sort of understand
this dilemma.

It's popping up when using different default modes for the same
device, the only one I've heard of is MMC that wants to be informed
on how many bits of the bus it should use.

[Stephen]
> If using device tree, the bus-width property should be used. If not
> using device tree, presumably you'd add an equivalent field to the
> platform data.
>
> An alternative (only when not using DT, since there's already a property
> for DT) is that the driver doesn't select pinctrl state "default", but
> instead first looks for e.g. "8bit" and if found uses it, then falls
> back to "4bit", then falls back to "1bit". Whichever state name is
> defined indicates which bus width is available.

But the DT approach seems to actually be less flexible than the old
style platform data here, whereas the method mentioned last here
can probe its way downward. DT just hardcodes it. But maybe this
is usually what you want to do anyway.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Stephen Warren]

On 06/05/2012 06:40 AM, Linus Walleij wrote:
> On Wed, May 23, 2012 at 5:26 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 05/23/2012 09:03 AM, Guennadi Liakhovetski wrote:
>>> On Wed, 23 May 2012, Stephen Warren wrote:
>>>
>>>> On 05/22/2012 04:23 PM, Guennadi Liakhovetski wrote:
>>>>> This patch adds a new function to allow clients to verify, whether a
>>>>> certain group is selected in the currently active setting or not.
>>>>>
>>>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>>>> ---
>>>>>
>>>>> Maybe there is already a way to do this without adding a new function, I
>>>>> haven't found one, hence this patch.
>>>>
>>>> This function doesn't currently exist because it breaks the pinctrl
>>>> conceptual model, which is that devices ask pinctrl for certain settings
>>>> whenever they need them, rather than information flowing the other way.
>>>
>>> Well, yes, we could tra to ask for each thinkable configuration and see
>>> which one(s) succeed, but that doesn't seem very optimal either, even if
>>> we only have 3 possibilities for now.
>>>
>>>> What's the use case for this new functionality?
>>>
>>> It's MMC. MMC interfaces can use 1, 4, or 8 data lines, depending on the
>>> board configuration. The board knows, that it has, say, only 4 data lines
>>> routed to the interface, so, it specifies the respective pinctrl
>>> configuration as default. Now in the driver we have to know how many
>>> data-lines are connected.
> 
> I discussed the same scenario the other day, and I sort of understand
> this dilemma.
> 
> It's popping up when using different default modes for the same
> device, the only one I've heard of is MMC that wants to be informed
> on how many bits of the bus it should use.

Well, you can't really have different "default" modes - "default" should
be used when there truly is a single state that the device/driver just
wants to get programmed. When the device/driver itself can support
multiple different configurations, "default" isn't the right answer if
the driver needs to derive information from the selected pinctrl state;
there should be different pinctrl states for each possible configuration.

> [Stephen]
>> If using device tree, the bus-width property should be used. If not
>> using device tree, presumably you'd add an equivalent field to the
>> platform data.
>>
>> An alternative (only when not using DT, since there's already a property
>> for DT) is that the driver doesn't select pinctrl state "default", but
>> instead first looks for e.g. "8bit" and if found uses it, then falls
>> back to "4bit", then falls back to "1bit". Whichever state name is
>> defined indicates which bus width is available.
> 
> But the DT approach seems to actually be less flexible than the old
> style platform data here, whereas the method mentioned last here
> can probe its way downward. DT just hardcodes it. But maybe this
> is usually what you want to do anyway.

I'm not sure what "probe its way downward" means. At least on Tegra, the
platform data also hard-coded the bus width (albeit in a slightly odd
way), so the bus-width DT property is semantically identical.

It's quite unlikely you'd ever want to switch between different states
at runtime, or program the HW to be anything other than what the board
supports, right?

Re: [PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Linus Walleij]

On Tue, Jun 5, 2012 at 6:07 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:

> Well, you can't really have different "default" modes - "default" should
> be used when there truly is a single state that the device/driver just
> wants to get programmed. When the device/driver itself can support
> multiple different configurations, "default" isn't the right answer if
> the driver needs to derive information from the selected pinctrl state;
> there should be different pinctrl states for each possible configuration.

This is true...

> It's quite unlikely you'd ever want to switch between different states
> at runtime, or program the HW to be anything other than what the board
> supports, right?

I think so too, but I'm not sure if I once heard an argument to the
contrary. Anyway, let's assume so.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Guennadi Liakhovetski]

Hi Stephen

Thanks for your comments and sorry for a delay.

On Wed, 23 May 2012, Stephen Warren wrote:

> On 05/23/2012 09:03 AM, Guennadi Liakhovetski wrote:
> > On Wed, 23 May 2012, Stephen Warren wrote:
> > 
> >> On 05/22/2012 04:23 PM, Guennadi Liakhovetski wrote:
> >>> This patch adds a new function to allow clients to verify, whether a
> >>> certain group is selected in the currently active setting or not.
> >>>
> >>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> >>> ---
> >>>
> >>> Maybe there is already a way to do this without adding a new function, I 
> >>> haven't found one, hence this patch.
> >>
> >> This function doesn't currently exist because it breaks the pinctrl
> >> conceptual model, which is that devices ask pinctrl for certain settings
> >> whenever they need them, rather than information flowing the other way.
> > 
> > Well, yes, we could tra to ask for each thinkable configuration and see 
> > which one(s) succeed, but that doesn't seem very optimal either, even if 
> > we only have 3 possibilities for now.
> > 
> >> What's the use case for this new functionality?
> > 
> > It's MMC. MMC interfaces can use 1, 4, or 8 data lines, depending on the 
> > board configuration. The board knows, that it has, say, only 4 data lines 
> > routed to the interface, so, it specifies the respective pinctrl 
> > configuration as default. Now in the driver we have to know how many 
> > data-lines are connected.
> 
> If using device tree, the bus-width property should be used. If not
> using device tree, presumably you'd add an equivalent field to the
> platform data.

Wouldn't adding a bus-width field to the platform data be redundant, 
considering it's already available in the pinctrl configuration?

> An alternative (only when not using DT, since there's already a property
> for DT) is that the driver doesn't select pinctrl state "default", but
> instead first looks for e.g. "8bit" and if found uses it, then falls
> back to "4bit", then falls back to "1bit". Whichever state name is
> defined indicates which bus width is available.

Sorry for not mentioning this in my previous reply, but there's more to 
it, not just data lines can optionally be present. Each of the 3 possible 
data-bus widths can also have Card Detection and Write Protect lines 
present, which brings the number of possible configurations to 4 * 3 = 
12... We don't want to try all of them, right? Whereas just querying the 
pinctrl framework about which pins have been selected and configured seems 
to be quite straight forward to me. Or should we request several 
configurations from the driver? One of the 3 bus widthes, and additionally 
a card-detection and a write-protect configurations?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Linus Walleij]

On Thu, Jun 7, 2012 at 4:05 AM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:

> [Stephen]
>> If using device tree, the bus-width property should be used. If not
>> using device tree, presumably you'd add an equivalent field to the
>> platform data.
>
> Wouldn't adding a bus-width field to the platform data be redundant,
> considering it's already available in the pinctrl configuration?

Maybe, but Documentation/devicetree/bindings/mmc/mmc.txt
mandates that you specify this width anyway I think, it's
stated as "requiered".

The bindings should be useable also for systems which does not
have pinctrl.

> Or should we request several configurations from the driver?

Do you mean several states?

> One of the 3 bus widthes, and additionally
> a card-detection and a write-protect configurations?

Isn't the card detection and write protect GPIO lines that
will be requested with gpio_request()? This is usually the case,
so I need to ask. GPIOs are usually muxed in on a per-pin basis
as a result of the gpio_request() call.

I don't know how your setup works but we have something like
this for an external SD card:

NB: I use these macros to shorten the lines, it's just simple hogs
on our primary pin controller:

#define DB8500_MUX(group,func,dev) \
        PIN_MAP_MUX_GROUP_DEFAULT(dev, "pinctrl-db8500", group, func)
#define DB8500_PIN(pin,conf,dev) \
        PIN_MAP_CONFIGS_PIN_DEFAULT(dev, "pinctrl-db8500", pin, conf)

/* Mux in SDI0 (here called MC0) used for removable MMC/SD/SDIO cards */
DB8500_MUX("mc0_a_1", "mc0", "sdi0"),
DB8500_PIN("GPIO18_AC2", out_hi, "sdi0"), /* CMDDIR */
DB8500_PIN("GPIO19_AC1", out_hi, "sdi0"), /* DAT0DIR */
DB8500_PIN("GPIO20_AB4", out_hi, "sdi0"), /* DAT2DIR */
DB8500_PIN("GPIO22_AA3", in_nopull, "sdi0"), /* FBCLK */
DB8500_PIN("GPIO23_AA4", out_lo, "sdi0"), /* CLK */
DB8500_PIN("GPIO24_AB2", in_pu, "sdi0"), /* CMD */
DB8500_PIN("GPIO25_Y4", in_pu, "sdi0"), /* DAT0 */
DB8500_PIN("GPIO26_Y2", in_pu, "sdi0"), /* DAT1 */
DB8500_PIN("GPIO27_AA2", in_pu, "sdi0"), /* DAT2 */
DB8500_PIN("GPIO28_AA1", in_pu, "sdi0"), /* DAT3 */

When "default" is selected, the group mc0_a_1 is muxed in
on function mc0, and the 10 pins are configured.(out_hi etc
are lists of configuration values).

We coul name this state "4bitsd" or so, and have another state
called "8bitemmc". Nothing stops you from also configuring pull-ups
on GPIO pins etc in this table.

We have several ports, here is an eMMC:

/* Mux in SDI2 (here called MC2) used for for PoP eMMC */
DB8500_MUX("mc2_a_1", "mc2", "sdi2"),
DB8500_PIN("GPIO128_A5", out_lo, "sdi2"), /* CLK */
DB8500_PIN("GPIO129_B4", in_pu, "sdi2"), /* CMD */
DB8500_PIN("GPIO130_C8", in_nopull, "sdi2"), /* FBCLK */
DB8500_PIN("GPIO131_A12", in_pu, "sdi2"), /* DAT0 */
DB8500_PIN("GPIO132_C10", in_pu, "sdi2"), /* DAT1 */
DB8500_PIN("GPIO133_B10", in_pu, "sdi2"), /* DAT2 */
DB8500_PIN("GPIO134_B9", in_pu, "sdi2"), /* DAT3 */
DB8500_PIN("GPIO135_A9", in_pu, "sdi2"), /* DAT4 */
DB8500_PIN("GPIO136_C7", in_pu, "sdi2"), /* DAT5 */
DB8500_PIN("GPIO137_A7", in_pu, "sdi2"), /* DAT6 */
DB8500_PIN("GPIO138_C5", in_pu, "sdi2"), /* DAT7 */

Isn't you board similarly hard-coded from the platform or
device tree? It seems less kludgy to ask DT or platform data
about the bus width if that is all.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Guennadi Liakhovetski]

On Thu, 7 Jun 2012, Linus Walleij wrote:

> On Thu, Jun 7, 2012 at 4:05 AM, Guennadi Liakhovetski
> <g.liakhovetski@gmx.de> wrote:
> 
> > [Stephen]
> >> If using device tree, the bus-width property should be used. If not
> >> using device tree, presumably you'd add an equivalent field to the
> >> platform data.
> >
> > Wouldn't adding a bus-width field to the platform data be redundant,
> > considering it's already available in the pinctrl configuration?
> 
> Maybe, but Documentation/devicetree/bindings/mmc/mmc.txt
> mandates that you specify this width anyway I think, it's
> stated as "requiered".
> 
> The bindings should be useable also for systems which does not
> have pinctrl.
> 
> > Or should we request several configurations from the driver?
> 
> Do you mean several states?

yes, sorry

> > One of the 3 bus widthes, and additionally
> > a card-detection and a write-protect configurations?
> 
> Isn't the card detection and write protect GPIO lines that
> will be requested with gpio_request()? This is usually the case,
> so I need to ask. GPIOs are usually muxed in on a per-pin basis
> as a result of the gpio_request() call.

They can be, but they don't have to be. On some SDHI implementations CD is 
a dedicated pin, belonging to the controller, but in those cases it is 
usually preferable to configure it as a GPIO. On some boards it is just a 
GPIO, on others yet it is just missing. Similarly, WP can be a GPIO or 
absent.

> I don't know how your setup works but we have something like
> this for an external SD card:

Yes, thanks, I understand that. But as I said, we can well have all 12 
configurations: 1, 4, or 8 bits, each of them with any or both of CD and 
WP or without them. Only the board knows about that.

The idea was to make the driver use the same pin-discovery method, 
independent whether DT is present or not: the pinctrl. And the pinctrl 
would be either specified in the platform data or in DT. This would avoid 
special handling of the no-DT case in the driver.

Thanks
Guennadi

> NB: I use these macros to shorten the lines, it's just simple hogs
> on our primary pin controller:
> 
> #define DB8500_MUX(group,func,dev) \
>         PIN_MAP_MUX_GROUP_DEFAULT(dev, "pinctrl-db8500", group, func)
> #define DB8500_PIN(pin,conf,dev) \
>         PIN_MAP_CONFIGS_PIN_DEFAULT(dev, "pinctrl-db8500", pin, conf)
> 
> /* Mux in SDI0 (here called MC0) used for removable MMC/SD/SDIO cards */
> DB8500_MUX("mc0_a_1", "mc0", "sdi0"),
> DB8500_PIN("GPIO18_AC2", out_hi, "sdi0"), /* CMDDIR */
> DB8500_PIN("GPIO19_AC1", out_hi, "sdi0"), /* DAT0DIR */
> DB8500_PIN("GPIO20_AB4", out_hi, "sdi0"), /* DAT2DIR */
> DB8500_PIN("GPIO22_AA3", in_nopull, "sdi0"), /* FBCLK */
> DB8500_PIN("GPIO23_AA4", out_lo, "sdi0"), /* CLK */
> DB8500_PIN("GPIO24_AB2", in_pu, "sdi0"), /* CMD */
> DB8500_PIN("GPIO25_Y4", in_pu, "sdi0"), /* DAT0 */
> DB8500_PIN("GPIO26_Y2", in_pu, "sdi0"), /* DAT1 */
> DB8500_PIN("GPIO27_AA2", in_pu, "sdi0"), /* DAT2 */
> DB8500_PIN("GPIO28_AA1", in_pu, "sdi0"), /* DAT3 */
> 
> When "default" is selected, the group mc0_a_1 is muxed in
> on function mc0, and the 10 pins are configured.(out_hi etc
> are lists of configuration values).
> 
> We coul name this state "4bitsd" or so, and have another state
> called "8bitemmc". Nothing stops you from also configuring pull-ups
> on GPIO pins etc in this table.
> 
> We have several ports, here is an eMMC:
> 
> /* Mux in SDI2 (here called MC2) used for for PoP eMMC */
> DB8500_MUX("mc2_a_1", "mc2", "sdi2"),
> DB8500_PIN("GPIO128_A5", out_lo, "sdi2"), /* CLK */
> DB8500_PIN("GPIO129_B4", in_pu, "sdi2"), /* CMD */
> DB8500_PIN("GPIO130_C8", in_nopull, "sdi2"), /* FBCLK */
> DB8500_PIN("GPIO131_A12", in_pu, "sdi2"), /* DAT0 */
> DB8500_PIN("GPIO132_C10", in_pu, "sdi2"), /* DAT1 */
> DB8500_PIN("GPIO133_B10", in_pu, "sdi2"), /* DAT2 */
> DB8500_PIN("GPIO134_B9", in_pu, "sdi2"), /* DAT3 */
> DB8500_PIN("GPIO135_A9", in_pu, "sdi2"), /* DAT4 */
> DB8500_PIN("GPIO136_C7", in_pu, "sdi2"), /* DAT5 */
> DB8500_PIN("GPIO137_A7", in_pu, "sdi2"), /* DAT6 */
> DB8500_PIN("GPIO138_C5", in_pu, "sdi2"), /* DAT7 */
> 
> Isn't you board similarly hard-coded from the platform or
> device tree? It seems less kludgy to ask DT or platform data
> about the bus width if that is all.
> 
> Yours,
> Linus Walleij
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Stephen Warren]

On 06/06/2012 08:05 PM, Guennadi Liakhovetski wrote:
> Hi Stephen
> 
> Thanks for your comments and sorry for a delay.
> 
> On Wed, 23 May 2012, Stephen Warren wrote:
> 
>> On 05/23/2012 09:03 AM, Guennadi Liakhovetski wrote:
>>> On Wed, 23 May 2012, Stephen Warren wrote:
>>>
>>>> On 05/22/2012 04:23 PM, Guennadi Liakhovetski wrote:
>>>>> This patch adds a new function to allow clients to verify, whether a
>>>>> certain group is selected in the currently active setting or not.
>>>>>
>>>>> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
>>>>> ---
>>>>>
>>>>> Maybe there is already a way to do this without adding a new function, I 
>>>>> haven't found one, hence this patch.
>>>>
>>>> This function doesn't currently exist because it breaks the pinctrl
>>>> conceptual model, which is that devices ask pinctrl for certain settings
>>>> whenever they need them, rather than information flowing the other way.
>>>
>>> Well, yes, we could tra to ask for each thinkable configuration and see 
>>> which one(s) succeed, but that doesn't seem very optimal either, even if 
>>> we only have 3 possibilities for now.
>>>
>>>> What's the use case for this new functionality?
>>>
>>> It's MMC. MMC interfaces can use 1, 4, or 8 data lines, depending on the 
>>> board configuration. The board knows, that it has, say, only 4 data lines 
>>> routed to the interface, so, it specifies the respective pinctrl 
>>> configuration as default. Now in the driver we have to know how many 
>>> data-lines are connected.
>>
>> If using device tree, the bus-width property should be used. If not
>> using device tree, presumably you'd add an equivalent field to the
>> platform data.
> 
> Wouldn't adding a bus-width field to the platform data be redundant, 
> considering it's already available in the pinctrl configuration?

That's not necessarily true in general. On Tegra at least, where pins
are muxed in groups rather than individually, I believe it's possible to
mux some SD controller's signals onto pin groups that support 8-bit data
width (since the pin group is 8 bits wide) but still only actually use 4
bits of data, since the board only hooked up that many signals. This
might be a waste of pins (but perhaps you can use the reset as random
GPIOs), but if the alternative pinmux locations for the SD signals have
to be used for something else, then you may have no choice.

>> An alternative (only when not using DT, since there's already a property
>> for DT) is that the driver doesn't select pinctrl state "default", but
>> instead first looks for e.g. "8bit" and if found uses it, then falls
>> back to "4bit", then falls back to "1bit". Whichever state name is
>> defined indicates which bus width is available.
> 
> Sorry for not mentioning this in my previous reply, but there's more to 
> it, not just data lines can optionally be present. Each of the 3 possible 
> data-bus widths can also have Card Detection and Write Protect lines 
> present, which brings the number of possible configurations to 4 * 3 = 
> 12... We don't want to try all of them, right? Whereas just querying the 
> pinctrl framework about which pins have been selected and configured seems 
> to be quite straight forward to me. Or should we request several 
> configurations from the driver? One of the 3 bus widthes, and additionally 
> a card-detection and a write-protect configurations?

I would personally recommend not deriving any information from the
selected pinctrl configuration at all. That way, you /can/ just use a
"default" state, and never have to search through multiple pinctrl
states. The board file will define that one state as setting up pinctrl
however it needs to be for that board - 1/4/8 bit, with/without
CD/WP/... signals/GPIOs etc.

If the CD and WP signals are truly GPIOs, there are already a common (if
not standardized) cd-gpios and wp-gpios DT properties to communicate the
GPIOs that are used to the driver.

If you're using built-in CD/WP logic on the SD host controller and don't
have the ability to make those pins plain GPIOs, I think it's entirely
reasonable to require DT to contain properties to enable/disable those
features, e.g. I see fsl-imx-esdhc.txt already has fsl,cd-internal and
fsl,wp-internal properties for this purpose.

Representing the configuration explicitly this way means that the SD
driver need have zero knowledge of the pinctrl settings for the SoC that
contains it, which keeps things nice and orthogonal. That'll help if the
SD controller HW module shows up in a different SoC or SoC version that
changes the pinctrl HW; the SD driver won't have to change at all this way.

Re: [PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Linus Walleij]

On Thu, Jun 7, 2012 at 5:14 PM, Guennadi Liakhovetski
<g.liakhovetski@gmx.de> wrote:
> On Thu, 7 Jun 2012, Linus Walleij wrote:
>> Isn't the card detection and write protect GPIO lines that
>> will be requested with gpio_request()? This is usually the case,
>> so I need to ask. GPIOs are usually muxed in on a per-pin basis
>> as a result of the gpio_request() call.
>
> They can be, but they don't have to be. On some SDHI implementations CD is
> a dedicated pin, belonging to the controller, but in those cases it is
> usually preferable to configure it as a GPIO. On some boards it is just a
> GPIO, on others yet it is just missing. Similarly, WP can be a GPIO or
> absent.

Sorry just to be clear on what we mean here:

"Configuring it as GPIO" in Linux means that you use the GPIO
subsystem and gpiolib to access it.

If you mean that you set some electrical properties that are named
as "the GPIO configuration" in the datasheet that is something else.

So that you mux it in as "gpio" (a name of the function from the
datasheet) does not make it into a GPIO in the Linux sense if it's not
handled by gpiolib.

In the case where such a pin is used by e.g. the MMC driver while
put into what the datasheet calls "gpio mode" this is still just one
pin in the MMC pinctrl function from the Linux point of view.

But I guess this was what you meant all the time, I'm probably just
confused by names being the same for similar but unrelated
things ... :-)

Yours,
Linus Walleij

Re: [PATCH] pinctrl: add a pinctrl_mux_group_selected() function

[Linus Walleij]

On Thu, Jun 7, 2012 at 6:25 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/06/2012 08:05 PM, Guennadi Liakhovetski wrote:

>> Sorry for not mentioning this in my previous reply, but there's more to
>> it, not just data lines can optionally be present. Each of the 3 possible
>> data-bus widths can also have Card Detection and Write Protect lines
>> present, which brings the number of possible configurations to 4 * 3 =
>> 12... We don't want to try all of them, right?

No that seems tiresome. And as noted elsewhere pinctrl is not
about maximum exercise of combinatorics. Nor should you encode
all possible combinations in you map, only put in a "default" map which
applied to that very board.

>> Whereas just querying the
>> pinctrl framework about which pins have been selected and configured seems
>> to be quite straight forward to me. Or should we request several
>> configurations from the driver? One of the 3 bus widthes, and additionally
>> a card-detection and a write-protect configurations?
>
> I would personally recommend not deriving any information from the
> selected pinctrl configuration at all. That way, you /can/ just use a
> "default" state, and never have to search through multiple pinctrl
> states. The board file will define that one state as setting up pinctrl
> however it needs to be for that board - 1/4/8 bit, with/without
> CD/WP/... signals/GPIOs etc.

I second this, if possible.

> If you're using built-in CD/WP logic on the SD host controller and don't
> have the ability to make those pins plain GPIOs, I think it's entirely
> reasonable to require DT to contain properties to enable/disable those
> features, e.g. I see fsl-imx-esdhc.txt already has fsl,cd-internal and
> fsl,wp-internal properties for this purpose.

This looks good to me.

I'm just guessing that the case is such that the SD controller has
built-in lines for CD and WP, but on its way to the external pin it
passes a piece of hardware that has pin configuration capabilities,
and in this case I'm semi-guessing that this piece of hardware
has a mode for that pin/line called "gpio" and that is where all
confusion comes from. But actually that has nothing to do with
the Linux concept of GPIOs (which is just gpiolib and a big
numberspace) as far as Linux is concerned, this is a pin config
setting part of the "default" state and nothing else.

Yours,
Linus Walleij