linux-kernel - Re: [RFC PATCH] i2c: core: dont change pinmux state to GPIO during recovery setup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZPivHKd0LWWnhPr/@shell.armlinux.org.uk>
Date:   Wed, 6 Sep 2023 17:55:56 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Robert Marko <robert.marko@...tura.hr>
Cc:     wsa@...nel.org, codrin.ciubotariu@...rochip.com,
        linux-i2c@...r.kernel.org, linux-kernel@...r.kernel.org,
        luka.perkov@...tura.hr
Subject: Re: [RFC PATCH] i2c: core: dont change pinmux state to GPIO during
 recovery setup

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@...tura.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@...tura.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!