Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210617194154.2397-1-linux.amoon@gmail.com> <20210617194154.2397-7-linux.amoon@gmail.com>
In-Reply-To: <20210617194154.2397-7-linux.amoon@gmail.com>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Fri, 18 Jun 2021 00:24:24 +0200
Message-ID: <CAFBinCCE+6G7QtDoxfbcZVVRf-MDX-epSo=_k_PJZgWX+_Whvg@mail.gmail.com>
Subject: Re: [RFCv1 6/8] phy: amlogic: meson8b-usb2: Use phy reset callback function
To:     Anand Moon <linux.amoon@gmail.com>
Cc:     Kishon Vijay Abraham I <kishon@ti.com>,
        Vinod Koul <vkoul@kernel.org>,
        Neil Armstrong <narmstrong@baylibre.com>,
        Kevin Hilman <khilman@baylibre.com>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        linux-phy@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Anand,

On Thu, Jun 17, 2021 at 9:44 PM Anand Moon <linux.amoon@gmail.com> wrote:
[...]
> +static int phy_meson8b_usb2_reset(struct phy *phy)
> +{
> +       struct phy_meson8b_usb2_priv *priv = phy_get_drvdata(phy);
> +       int ret;
> +
> +       if (priv->is_enabled) {
> +               ret = reset_control_reset(priv->reset);
> +               if (ret) {
> +                       dev_err(&phy->dev, "Failed to trigger USB reset\n");
> +                       return ret;
> +               }
> +       }
> +
> +       return 0;
> +}
will this reset change the register values back to some kind of default?
If it does then we're not re-applying our desired register values
afterwards - which is probably asking for trouble.

For shared resets (like the one we have here) reset_control_reset will
only trigger the reset line once until all drivers using that reset
line are unloaded.
So effectively this new phy_ops.reset callback will be a no-op.

[...]
> -       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, NULL);
> +       priv->reset = devm_reset_control_get_optional_shared(&pdev->dev, "phy");
I think this breaks compatibility with existing .dtbs and our
dt-bindings (as we're not documenting a "reset-names" property).
What is the goal of this one?


Best regards,
Martin