| Message ID | 20200806155322.GA25523@ola-jn9phv2.ad.garmin.com |
|---|---|
| State | New |
| Headers | show |
| Series | pinctrl: nomadik: Fix pull direction debug info | expand |
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
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
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,
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(-)