Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
MIME-Version: 1.0
References: <20230901114936.1319844-1-robert.marko@sartura.hr>
 <CA+HBbNEM6AfwX87DLRNAuJSWPKboGuuJJDRK_E+G3sJDF73oZA@mail.gmail.com> <ZPivHKd0LWWnhPr/@shell.armlinux.org.uk>
In-Reply-To: <ZPivHKd0LWWnhPr/@shell.armlinux.org.uk>
From:   Robert Marko <robert.marko@sartura.hr>
Date:   Wed, 13 Sep 2023 20:49:25 +0200
Message-ID: <CA+HBbNG9uXEhYjaKR6ixhN1uESmypgjqmCTayvWknUt03BYQNg@mail.gmail.com>
Subject: Re: [RFC PATCH] i2c: core: dont change pinmux state to GPIO during
 recovery setup
To:     "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc:     wsa@kernel.org, codrin.ciubotariu@microchip.com,
        linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org,
        luka.perkov@sartura.hr
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Wed, Sep 6, 2023 at 6:55=E2=80=AFPM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Wed, Sep 06, 2023 at 04:41:33PM +0200, Robert Marko wrote:
> > On Fri, Sep 1, 2023 at 1:49=E2=80=AFPM Robert Marko <robert.marko@sartu=
ra.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 i=
n DTS.
> > >
> > > After a while it was traced down to the only difference being that PX=
A
> > > driver did not change the pinmux state to GPIO before trying to acqui=
re 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_ge=
t()
> > > 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 unaffec=
ted
> -        * 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.

Hi Russel,
Sorry for the late reply, its been a busy week.

I have tried doing exactly that and calling:

if (bri->pinctrl)
pinctrl_select_state(bri->pinctrl, bri->pins_gpio);
/* change the state of the pins back to their default state */
if (bri->pinctrl)
pinctrl_select_state(bri->pinctrl, bri->pins_default);

However doing that in the generic code does not work, only thing that has
worked so far is removing the GPIO pinctrl call completely.

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


--=20
Robert Marko
Staff Embedded Linux Engineer
Sartura Ltd.
Lendavska ulica 16a
10000 Zagreb, Croatia
Email: robert.marko@sartura.hr
Web: www.sartura.hr