All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Robert Marko <robert.marko@sartura.hr>
Cc: wsa@kernel.org, codrin.ciubotariu@microchip.com,
	linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
	luka.perkov@sartura.hr
Subject: Re: [RFC PATCH] i2c: core: dont change pinmux state to GPIO during recovery setup
Date: Wed, 6 Sep 2023 17:55:56 +0100	[thread overview]
Message-ID: <ZPivHKd0LWWnhPr/@shell.armlinux.org.uk> (raw)
In-Reply-To: <CA+HBbNEM6AfwX87DLRNAuJSWPKboGuuJJDRK_E+G3sJDF73oZA@mail.gmail.com>

On Wed, Sep 06, 2023 at 04:41:33PM +0200, Robert Marko wrote:
> On Fri, Sep 1, 2023 at 1:49 PM Robert Marko <robert.marko@sartura.hr> wrote:
> >
> > Ever since PXA I2C driver was moved to the generic I2C recovery, I2C has
> > stopped working completely on Armada 3720 if the pins are specified in DTS.
> >
> > After a while it was traced down to the only difference being that PXA
> > driver did not change the pinmux state to GPIO before trying to acquire the
> > GPIO pins.
> > And indeed as soon as this call is removed I2C starts working.
> >
> > To me it seems that this call is not required at all as devm_gpiod_get()
> > will result in the pinmux state being changed to GPIO via the pinmux
> > set_mux() op.
> >
> > Fixes: 0b01392c18b9 ("i2c: pxa: move to generic GPIO recovery")
> > Signed-off-by: Robert Marko <robert.marko@sartura.hr>
> > ---
> > I am aware this probably isnt the correct fix, so I am sending it as RFC
> > cause I have ran out of ideas.
> 
> CC-ing Russel as well since I forgot him.

So the generic recovery decided to set the pinmux state before calling
devm_gpiod_get(), where as the driver (and my code) originally did this
after calling devm_gpiod_get():

-       /*
-        * Claiming GPIOs can change the pinmux state, which confuses the
-        * pinctrl since pinctrl's idea of the current setting is unaffected
-        * by the pinmux change caused by claiming the GPIO. Work around that
-        * by switching pinctrl to the GPIO state here. We do it this way to
-        * avoid glitching the I2C bus.
-        */
-       pinctrl_select_state(i2c->pinctrl, i2c->pinctrl_recovery);
-
-       return pinctrl_select_state(i2c->pinctrl, i2c->pinctrl_default);

I'd suggest re-implementing my original scheme in the generic code
because this _does_ work on Armada 3720 hardware.

Removing the pinmux frobbing is likely to break stuff.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

  reply	other threads:[~2023-09-06 16:56 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-01 11:49 [RFC PATCH] i2c: core: dont change pinmux state to GPIO during recovery setup Robert Marko
2023-09-06 14:41 ` Robert Marko
2023-09-06 16:55   ` Russell King (Oracle) [this message]
2023-09-13 18:49     ` Robert Marko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZPivHKd0LWWnhPr/@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=codrin.ciubotariu@microchip.com \
    --cc=linux-i2c@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=luka.perkov@sartura.hr \
    --cc=robert.marko@sartura.hr \
    --cc=wsa@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.