[PATCH] pinctrl: make the pinmux-pins more helpful

[PATCH] pinctrl: make the pinmux-pins more helpful

[Linus Walleij]

From: Linus Walleij <linus.walleij@linaro.org>

The debugfs file pinmux-pins used to tell which function was
enabled but now states simply which device owns the pin. Being
owned by the pinctrl driver itself means just that it's hogged
so be a bit more helpful by printing that.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
I somewhat mourn the loss of being able to tell from the debugfs
which function is using a certain pin, does anyone have ideas on
how to go about fixing this properly? The root file
pinctrl-handles does tell it, but requires cross-referencing
which isn't helpful.
---
 drivers/pinctrl/pinmux.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 98b89d6..db5ed86 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -626,8 +626,8 @@ static int pinmux_pins_show(struct seq_file *s, void *what)
 
 	/* The pin number can be retrived from the pin controller descriptor */
 	for (i = 0; i < pctldev->desc->npins; i++) {
-
 		struct pin_desc *desc;
+		const char *owner;
 
 		pin = pctldev->desc->pins[i].number;
 		desc = pin_desc_get(pctldev, pin);
@@ -635,9 +635,16 @@ static int pinmux_pins_show(struct seq_file *s, void *what)
 		if (desc == NULL)
 			continue;
 
+		if (!desc->owner)
+			owner = "UNCLAIMED";
+		else if (!strcmp(desc->owner, pinctrl_dev_get_name(pctldev)))
+			owner = "HOG";
+		else
+			owner = desc->owner;
+
 		seq_printf(s, "pin %d (%s): %s\n", pin,
 			   desc->name ? desc->name : "unnamed",
-			   desc->owner ? desc->owner : "UNCLAIMED");
+			   owner);
 	}
 
 	return 0;
-- 
1.7.8

Re: [PATCH] pinctrl: make the pinmux-pins more helpful

[Dong Aisheng]

On Fri, Feb 24, 2012 at 06:56:09AM +0100, Linus Walleij wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
> 
> The debugfs file pinmux-pins used to tell which function was
> enabled but now states simply which device owns the pin. Being
> owned by the pinctrl driver itself means just that it's hogged
> so be a bit more helpful by printing that.
> 
It's useful.

Acked-by: Dong Aisheng <dong.aisheng@linaro.org>

BTW, one question below although it's not relatd to this patch itself.

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> I somewhat mourn the loss of being able to tell from the debugfs
> which function is using a certain pin, does anyone have ideas on
> how to go about fixing this properly? The root file
> pinctrl-handles does tell it, but requires cross-referencing
> which isn't helpful.
> ---
>  drivers/pinctrl/pinmux.c |   11 +++++++++--
>  1 files changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
> index 98b89d6..db5ed86 100644
> --- a/drivers/pinctrl/pinmux.c
> +++ b/drivers/pinctrl/pinmux.c
> @@ -626,8 +626,8 @@ static int pinmux_pins_show(struct seq_file *s, void *what)
>  
>  	/* The pin number can be retrived from the pin controller descriptor */
>  	for (i = 0; i < pctldev->desc->npins; i++) {
> -
>  		struct pin_desc *desc;
> +		const char *owner;
>  
>  		pin = pctldev->desc->pins[i].number;
>  		desc = pin_desc_get(pctldev, pin);
> @@ -635,9 +635,16 @@ static int pinmux_pins_show(struct seq_file *s, void *what)
>  		if (desc == NULL)
>  			continue;
>  
> +		if (!desc->owner)
> +			owner = "UNCLAIMED";
> +		else if (!strcmp(desc->owner, pinctrl_dev_get_name(pctldev)))
> +			owner = "HOG";
> +		else
> +			owner = desc->owner;
> +
>  		seq_printf(s, "pin %d (%s): %s\n", pin,
>  			   desc->name ? desc->name : "unnamed",
Is there a little issue?
For this line, i see some code:
static int pinctrl_register_one_pin(struct pinctrl_dev *pctldev,
                                    unsigned number, const char *name)
{
...
        pindesc = kzalloc(sizeof(*pindesc), GFP_KERNEL);
        if (pindesc == NULL)
                return -ENOMEM;
...
        /* Copy basic pin info */
        if (name) {
                pindesc->name = name;
        } else {
                pindesc->name = kasprintf(GFP_KERNEL, "PIN%u", number);
                if (pindesc->name == NULL)
                        return -ENOMEM;
                pindesc->dynamic_name = true;
        }
...
}
So is it possible the desc->name is NULL?

> -			   desc->owner ? desc->owner : "UNCLAIMED");
> +			   owner);
>  	}
>  
>  	return 0;

Regards
Dong Aisheng

RE: [PATCH] pinctrl: make the pinmux-pins more helpful

[Stephen Warren]

Linus Walleij wrote at Thursday, February 23, 2012 10:56 PM:
> The debugfs file pinmux-pins used to tell which function was
> enabled but now states simply which device owns the pin. Being
> owned by the pinctrl driver itself means just that it's hogged
> so be a bit more helpful by printing that.

> +		if (!desc->owner)
> +			owner = "UNCLAIMED";
> +		else if (!strcmp(desc->owner, pinctrl_dev_get_name(pctldev)))
> +			owner = "HOG";
> +		else
> +			owner = desc->owner;
> +
>  		seq_printf(s, "pin %d (%s): %s\n", pin,
>  			   desc->name ? desc->name : "unnamed",
> -			   desc->owner ? desc->owner : "UNCLAIMED");
> +			   owner);

Personally, I'd prefer not to make this change. I don't really like the
way we treat hogs as some kind of special-case; they work exactly like
any other pinctrl state (at least after the patch I posted to implemnt
pinctrl_select_state()), so I don't really see the need for special-casing
the debug information here.

If we do make a change like this, I'd prefer the format to be:

UNCLAIMED
"%s (HOG)", desc->owner
desc->owner

So that the ownership is always there in the standard format, but the
hog information is additional if you care about the special case.

> I somewhat mourn the loss of being able to tell from the debugfs
> which function is using a certain pin, does anyone have ideas on
> how to go about fixing this properly? The root file
> pinctrl-handles does tell it, but requires cross-referencing
> which isn't helpful.

This doesn't seem like a big deal to me; it's very easy to cross-
reference. That said, we could either:

a) Add a field to pin_desc which indicates current usage. This would be
set whenever the pin's mux function was set, i.e. in pinctrl_select_state()
or pinctrl_request_gpio().

b) Add a pinctrl driver ops function which reads and prints the current
state from HW.

(and note the fact that having the debug file list the current mux
function per pin doesn't really make sense on HW where the muxing is
per group...)

-- 
nvpublic

Re: [PATCH] pinctrl: make the pinmux-pins more helpful

[Linus Walleij]

On Fri, Feb 24, 2012 at 5:44 PM, Stephen Warren <swarren@nvidia.com> wrote:

> If we do make a change like this, I'd prefer the format to be:
>
> UNCLAIMED
> "%s (HOG)", desc->owner
> desc->owner

I don't see the point, the debugfs files are supposed to be
human-readable, a human is not interested in the fact that
the pinctrl device itself is owning the pin, what is interesting is
that it is hogged.

>> I somewhat mourn the loss of being able to tell from the debugfs
>> which function is using a certain pin, does anyone have ideas on
>> how to go about fixing this properly? The root file
>> pinctrl-handles does tell it, but requires cross-referencing
>> which isn't helpful.
>
> This doesn't seem like a big deal to me; it's very easy to cross-
> reference.

Not to me it isn't, looks like I would have to create scripts to
do that for a large pin controller and that's less helpful than
just having the information there.

> That said, we could either:
>
> a) Add a field to pin_desc which indicates current usage. This would be
> set whenever the pin's mux function was set, i.e. in pinctrl_select_state()
> or pinctrl_request_gpio().

That's like re-introducing the former "function" field I guess.

Simple if I just also #ifdef CONFIG_DEBUGFS... so I'd go
for this.

> b) Add a pinctrl driver ops function which reads and prints the current
> state from HW.
>
> (and note the fact that having the debug file list the current mux
> function per pin doesn't really make sense on HW where the muxing is
> per group...)

On U300 it makes a lot of sense even thogh it is essentially
group based. When sitting with the datasheet with the pin names
and use groups it's simple to see exactly how any one pin is muxed
for the moment and troubleshoot from there.

Linus Walleij

RE: [PATCH] pinctrl: make the pinmux-pins more helpful

[Stephen Warren]

Linus Walleij wrote at Wednesday, February 29, 2012 2:39 AM:
> On Fri, Feb 24, 2012 at 5:44 PM, Stephen Warren <swarren@nvidia.com> wrote:
> 
> > If we do make a change like this, I'd prefer the format to be:
> >
> > UNCLAIMED
> > "%s (HOG)", desc->owner
> > desc->owner
> 
> I don't see the point, the debugfs files are supposed to be
> human-readable, a human is not interested in the fact that
> the pinctrl device itself is owning the pin, what is interesting is
> that it is hogged.

Well, I'm a human and I care far more that the pin controller device is
what owns the configuration rather than some artificial "hog" concept
is present...

> >> I somewhat mourn the loss of being able to tell from the debugfs
> >> which function is using a certain pin, does anyone have ideas on
> >> how to go about fixing this properly? The root file
> >> pinctrl-handles does tell it, but requires cross-referencing
> >> which isn't helpful.
> >
> > This doesn't seem like a big deal to me; it's very easy to cross-
> > reference.
> 
> Not to me it isn't, looks like I would have to create scripts to
> do that for a large pin controller and that's less helpful than
> just having the information there.
> 
> > That said, we could either:
> >
> > a) Add a field to pin_desc which indicates current usage. This would be
> > set whenever the pin's mux function was set, i.e. in pinctrl_select_state()
> > or pinctrl_request_gpio().
> 
> That's like re-introducing the former "function" field I guess.
> 
> Simple if I just also #ifdef CONFIG_DEBUGFS... so I'd go
> for this.
> 
> > b) Add a pinctrl driver ops function which reads and prints the current
> > state from HW.
> >
> > (and note the fact that having the debug file list the current mux
> > function per pin doesn't really make sense on HW where the muxing is
> > per group...)
> 
> On U300 it makes a lot of sense even thogh it is essentially
> group based. When sitting with the datasheet with the pin names
> and use groups it's simple to see exactly how any one pin is muxed
> for the moment and troubleshoot from there.

If the HW muxes per group, then there's no concept of "which function is
selected for a pin", since functions are selected for a group not for a
pin.

So, the datasheet will be written in terms of "group X supports functions
A, B, C, D", not pins P, Q, R support functions "A, B, C, D". So,
representing mux function in debugfs at the group level would make a lot
more sense given that HW design.

-- 
nvpublic

Re: [PATCH] pinctrl: make the pinmux-pins more helpful

[Linus Walleij]

On Wed, Feb 29, 2012 at 6:27 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Linus Walleij wrote at Wednesday, February 29, 2012 2:39 AM:
>> On Fri, Feb 24, 2012 at 5:44 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>
>> > If we do make a change like this, I'd prefer the format to be:
>> >
>> > UNCLAIMED
>> > "%s (HOG)", desc->owner
>> > desc->owner
>>
>> I don't see the point, the debugfs files are supposed to be
>> human-readable, a human is not interested in the fact that
>> the pinctrl device itself is owning the pin, what is interesting is
>> that it is hogged.
>
> Well, I'm a human and I care far more that the pin controller device is
> what owns the configuration rather than some artificial "hog" concept
> is present...

Yeah OK I'll try to add both then to please both instances of
human...

>> On U300 it makes a lot of sense even thogh it is essentially
>> group based. When sitting with the datasheet with the pin names
>> and use groups it's simple to see exactly how any one pin is muxed
>> for the moment and troubleshoot from there.
>
> If the HW muxes per group, then there's no concept of "which function is
> selected for a pin", since functions are selected for a group not for a
> pin.
>
> So, the datasheet will be written in terms of "group X supports functions
> A, B, C, D", not pins P, Q, R support functions "A, B, C, D". So,
> representing mux function in debugfs at the group level would make a lot
> more sense given that HW design.

I think we're talking past each other, it's that old thing with group
concepts again. I'd just say that for debugging
and understanding the muxing on U300 style hardware this
is very useful. It may be pointless in Tegra style HW.

Yours,
Linus Walleij