Re: [PATCH 3/3] phy: amlogic: meson8b-usb2: fix shared reset control use

[Amjad Ouled-Ameur <aouledameur@baylibre.com>]

Hi Martin,

Thank you for the review !

On 14/11/2020 20:11, Martin Blumenstingl wrote:

> Hi Amjad,
>
> On Fri, Nov 13, 2020 at 1:07 AM Amjad Ouled-Ameur
> <aouledameur@baylibre.com> wrote:
> [...]
>>          ret = clk_prepare_enable(priv->clk_usb);
>>          if (ret) {
>>                  dev_err(&phy->dev, "Failed to enable USB DDR clock\n");
>> +               reset_control_rearm(priv->reset);
> this should come after reset_control_rearm so we're cleaning up in
> reverse order of initializing things
> (in this case it probably makes no difference since
> reset_control_rearm is not touching any registers, but I'd still have
> it in the correct order to not confuse future developers)

Agreed, it works in this current order since the two lines do not
interfere with each other, but it is cleaner to do it in the reverse
order of initialization. Will fix it in next change.

>>                  clk_disable_unprepare(priv->clk_usb_general);
>>                  return ret;
>>          }
>> @@ -197,6 +199,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
>>                          regmap_read(priv->regmap, REG_ADP_BC, &reg);
>>                          if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
>>                                  dev_warn(&phy->dev, "USB ID detect failed!\n");
>> +                               reset_control_rearm(priv->reset);
> same here, reset_control_rearm should be after clk_disable_unprepare

Ditto, will fix it in next change.

>
>>                                  clk_disable_unprepare(priv->clk_usb);
>>                                  clk_disable_unprepare(priv->clk_usb_general);
>>                                  return -EINVAL;
>> @@ -216,6 +219,7 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
>>                                     REG_DBG_UART_SET_IDDQ,
>>                                     REG_DBG_UART_SET_IDDQ);
>>
>> +       reset_control_rearm(priv->reset);
> same here, reset_control_rearm should be after clk_disable_unprepare

Ditto, will fix it in next change.

>
>>          clk_disable_unprepare(priv->clk_usb);
>>          clk_disable_unprepare(priv->clk_usb_general);
>
> Best regards,
> Martin
>

Sincerely,
Amjad


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic