Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220622003140.30365-1-pgwipeout@gmail.com> <f252c182-e306-174e-dd32-51c9a406d3f7@sholland.org>
In-Reply-To: <f252c182-e306-174e-dd32-51c9a406d3f7@sholland.org>
From:   Peter Geis <pgwipeout@gmail.com>
Date:   Wed, 22 Jun 2022 04:31:39 -0400
Message-ID: <CAMdYzYqUsr+orVtSbD023=GAyNLg4VdP9Wcrg6gON+Ut9FdzZA@mail.gmail.com>
Subject: Re: [PATCH] phy: rockchip-inno-usb2: Sync initial otg state
To:     Samuel Holland <samuel@sholland.org>
Cc:     Heiko Stuebner <heiko@sntech.de>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Vinod Koul <vkoul@kernel.org>,
        "open list:GENERIC PHY FRAMEWORK" <linux-phy@lists.infradead.org>,
        arm-mail-list <linux-arm-kernel@lists.infradead.org>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Jun 22, 2022 at 1:16 AM Samuel Holland <samuel@sholland.org> wrote:
>
> On 6/21/22 7:31 PM, Peter Geis wrote:
> > The initial otg state for the phy defaults to device mode. The actual
> > state isn't detected until an ID IRQ fires. Fix this by syncing the ID
> > state during initialization.
> >
> > Fixes: 51a9b2c03dd3 ("phy: rockchip-inno-usb2: Handle ID IRQ")
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>
> One comment below. Either way it fixes a bug, so:
>
> Reviewed-by: Samuel Holland <samuel@sholland.org>

Thanks!

>
> > ---
> >  drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > index 6711659f727c..6e44069617df 100644
> > --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
> > @@ -1162,6 +1162,12 @@ static int rockchip_usb2phy_otg_port_init(struct rockchip_usb2phy *rphy,
> >                                       EXTCON_USB_HOST, &rport->event_nb);
> >               if (ret)
> >                       dev_err(rphy->dev, "register USB HOST notifier failed\n");
> > +
> > +             if (!of_property_read_bool(rphy->dev->of_node, "extcon")) {
>
> Currently the IRQ handler does not care who created the extcon. We should at
> least be consistent between here and there.

We care in this instance because if there is an external extcon it is
responsible for setting the initial state. Our ID pin may or may not
be used in this case and if it isn't we would overwrite the initial
state with an invalid one.

>
> Regards,
> Samuel
>
> > +                     /* do initial sync of usb state */
> > +                     ret = property_enabled(rphy->grf, &rport->port_cfg->utmi_id);
> > +                     extcon_set_state_sync(rphy->edev, EXTCON_USB_HOST, !ret);
> > +             }
> >       }
> >
> >  out:
> >
>