| Message ID | 20211112162827.128319-4-aouledameur@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: meson: fix shared reset control use | expand |
Hi Amjad, Thanks for working on this! On Fri, Nov 12, 2021 at 5:33 PM Amjad Ouled-Ameur <aouledameur@baylibre.com> wrote: [...] > + reset_control_rearm(priv->reset); Using priv->reset in this driver currently requires an IS_ERR check beforehand. When I wrote the driver originally I used the following code in phy_meson8b_usb2_probe: priv->reset = ... if (PTR_ERR(priv->reset) == -EPROBE_DEFER) return PTR_ERR(priv->reset); That means: priv->reset can (in theory) be an error pointer at runtime. Since your patch is valid: can you please add another one (before this one) in the series and change the priv->reset error checking to use something like: if (IS_ERR(priv->reset)) return dev_err_probe(&pdev->dev, PTR_ERR(priv->reset), "Failed to get the reset line"); With such a patch you can consider this one as: Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Best regards, Martin
Hi Martin, Thank you for reviewing this. On 21/11/2021 00:57, Martin Blumenstingl wrote: > Hi Amjad, > > Thanks for working on this! > > On Fri, Nov 12, 2021 at 5:33 PM Amjad Ouled-Ameur > <aouledameur@baylibre.com> wrote: > [...] >> + reset_control_rearm(priv->reset); > Using priv->reset in this driver currently requires an IS_ERR check beforehand. > When I wrote the driver originally I used the following code in > phy_meson8b_usb2_probe: > priv->reset = ... > if (PTR_ERR(priv->reset) == -EPROBE_DEFER) > return PTR_ERR(priv->reset); > > That means: priv->reset can (in theory) be an error pointer at runtime. > Since your patch is valid: can you please add another one (before this > one) in the series and change the priv->reset error checking to use > something like: > if (IS_ERR(priv->reset)) > return dev_err_probe(&pdev->dev, PTR_ERR(priv->reset), "Failed to > get the reset line"); No worries, will do. Regards, Amjad > With such a patch you can consider this one as: > Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > > > Best regards, > Martin
diff --git a/drivers/phy/amlogic/phy-meson8b-usb2.c b/drivers/phy/amlogic/phy-meson8b-usb2.c index cf10bed40528..a6e74288ca8f 100644 --- a/drivers/phy/amlogic/phy-meson8b-usb2.c +++ b/drivers/phy/amlogic/phy-meson8b-usb2.c @@ -154,6 +154,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy) ret = clk_prepare_enable(priv->clk_usb_general); if (ret) { dev_err(&phy->dev, "Failed to enable USB general clock\n"); + reset_control_rearm(priv->reset); return ret; } @@ -161,6 +162,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy) if (ret) { dev_err(&phy->dev, "Failed to enable USB DDR clock\n"); clk_disable_unprepare(priv->clk_usb_general); + reset_control_rearm(priv->reset); return ret; } @@ -199,6 +201,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy) dev_warn(&phy->dev, "USB ID detect failed!\n"); clk_disable_unprepare(priv->clk_usb); clk_disable_unprepare(priv->clk_usb_general); + reset_control_rearm(priv->reset); return -EINVAL; } } @@ -218,6 +221,7 @@ static int phy_meson8b_usb2_power_off(struct phy *phy) clk_disable_unprepare(priv->clk_usb); clk_disable_unprepare(priv->clk_usb_general); + reset_control_rearm(priv->reset); /* power off the PHY by putting it into reset mode */ regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
Use reset_control_rearm() call if an error occurs in case phy_meson8b_usb2_power_on() fails after reset() has been called, or in case phy_meson8b_usb2_power_off() is called i.e the resource is no longer used and the reset line may be triggered again by other devices. reset_control_rearm() keeps use of triggered_count sane in the reset framework, use of reset_control_reset() on shared reset line should be balanced with reset_control_rearm(). Signed-off-by: Amjad Ouled-Ameur <aouledameur@baylibre.com> Reported-by: Jerome Brunet <jbrunet@baylibre.com> --- drivers/phy/amlogic/phy-meson8b-usb2.c | 4 ++++ 1 file changed, 4 insertions(+)