[PATCH] pinctrl: nomadik: Fix pull direction debug info

[PATCH] pinctrl: nomadik: Fix pull direction debug info

[Andrew Halaney]

The nomadik pinctrl hardware doesn't have any way to
determine if the active pull is up or down. Reading
the bit currently used to report if the pull is up/down
indicates if the gpio input is reading high or low, it
doesn't reflect the pull state.

For this reason change the output from "pull up"/"pull down" to
"pull enabled". This avoids confusing developers who were using
the output to determine what the pull state is.

Signed-off-by: Andrew Halaney <ajhalaney@gmail.com>
---
 drivers/pinctrl/nomadik/pinctrl-nomadik.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/nomadik/pinctrl-nomadik.c b/drivers/pinctrl/nomadik/pinctrl-nomadik.c
index ba25c4654391..657e35a75d84 100644
--- a/drivers/pinctrl/nomadik/pinctrl-nomadik.c
+++ b/drivers/pinctrl/nomadik/pinctrl-nomadik.c
@@ -931,11 +931,6 @@ static void nmk_gpio_dbg_show_one(struct seq_file *s,
 		[NMK_GPIO_ALT_C+3]	= "altC3",
 		[NMK_GPIO_ALT_C+4]	= "altC4",
 	};
-	const char *pulls[] = {
-		"none     ",
-		"pull down",
-		"pull up  ",
-	};
 
 	clk_enable(nmk_chip->clk);
 	is_out = !!(readl(nmk_chip->addr + NMK_GPIO_DIR) & BIT(offset));
@@ -946,7 +941,7 @@ static void nmk_gpio_dbg_show_one(struct seq_file *s,
 		mode = nmk_prcm_gpiocr_get_mode(pctldev, gpio);
 
 	if (is_out) {
-		seq_printf(s, " gpio-%-3d (%-20.20s) out %s        %s",
+		seq_printf(s, " gpio-%-3d (%-20.20s) out %s           %s",
 			   gpio,
 			   label ?: "(none)",
 			   data_out ? "hi" : "lo",
@@ -954,11 +949,12 @@ static void nmk_gpio_dbg_show_one(struct seq_file *s,
 	} else {
 		int irq = chip->to_irq(chip, offset);
 		struct irq_desc	*desc = irq_to_desc(irq);
-		int pullidx = 0;
+		const int pullidx = pull ? 1 : 0;
 		int val;
-
-		if (pull)
-			pullidx = data_out ? 2 : 1;
+		static const char * const pulls[] = {
+			"none        ",
+			"pull enabled",
+		};
 
 		seq_printf(s, " gpio-%-3d (%-20.20s) in  %s %s",
 			   gpio,
-- 
2.24.1

Re: [PATCH] pinctrl: nomadik: Fix pull direction debug info

[Linus Walleij]

On Thu, Aug 6, 2020 at 5:53 PM Andrew Halaney <ajhalaney@gmail.com> wrote:

> The nomadik pinctrl hardware doesn't have any way to
> determine if the active pull is up or down. Reading
> the bit currently used to report if the pull is up/down
> indicates if the gpio input is reading high or low, it
> doesn't reflect the pull state.
>
> For this reason change the output from "pull up"/"pull down" to
> "pull enabled". This avoids confusing developers who were using
> the output to determine what the pull state is.
>
> Signed-off-by: Andrew Halaney <ajhalaney@gmail.com>

Patch applied. It's clearly better like this. A plain bug.

Do you think we can fix it properly by reading both the
DATS and DATC registers  to do the inverse of what
__nmk_gpio_set_pull() does and give a proper readout
of pull up/down?

Out of curiosity: what hardware are you testing this on?

Yours,
Linus Walleij

Re: [PATCH] pinctrl: nomadik: Fix pull direction debug info

[Andrew Halaney]

On Thu, Aug 13, 2020 at 10:06:55AM +0200, Linus Walleij wrote:
> Patch applied. It's clearly better like this. A plain bug.
Thanks!

> Do you think we can fix it properly by reading both the
> DATS and DATC registers  to do the inverse of what
> __nmk_gpio_set_pull() does and give a proper readout
> of pull up/down?
Sadly I don't think this will work. Both the DATS and DATC registers
return the contents of the DAT register on a read, so we'd be back
to where we are without the patch I posted. To me it seems like this
pin controller just doesn't support any method to probe the hardware
for the current pull state. I had considered seeing if this state
was stored in software anywhere, but decided against it since there's
a good chance a bootloader setup some pin configurations as well,
and that would cause the output to not match up with
the actual hardware state.

> Out of curiosity: what hardware are you testing this on?
I have access to an Accordo5 (sta1295 - not upstreamed).

Thanks,
Andrew