[PATCH v3 08/10] phy: socionext: Add UniPhier USB3 PHY driver

[Marek Vasut]

On 2/13/23 04:08, Kunihiko Hayashi wrote:
> Hi Marek,

Hello Hayashi-san,

> On 2023/02/10 23:09, Marek Vasut wrote:
>> On 2/8/23 10:15, Kunihiko Hayashi wrote:
>>
>> [...]
>>
>>> +static int uniphier_usb3phy_init(struct phy *phy)
>>> +{
>>> +    struct uniphier_usb3phy_priv *priv = dev_get_priv(phy->dev);
>>> +    int ret;
>>> +
>>> +    ret = clk_enable_bulk(&priv->clks);
>>> +    if (ret)
>>> +        goto out_clk;
>>
>> This should be just 'return ret;'
>>
>>> +    ret = reset_deassert_bulk(&priv->rsts);
>>> +    if (ret)
>>> +        goto out_rst;
>>
>> This should be goto out_rst, however ...
>>
>>> +    return 0;
>>> +
>>> +out_rst:
>>> +    reset_release_bulk(&priv->rsts);
>>> +out_clk:
>>> +    clk_release_bulk(&priv->clks);
>>
>> ... the out_rst: should only do:
>>
>> out_rst:
>>     clk_disable_bulk();
>>     return ret
>>
>> out_clk part can be removed.
> 
> I see. These operations are unpaired.
> I'll fix them.

Thank you

>>> +            return ret;
>>> +        }
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static struct phy_ops uniphier_usb3phy_ops = {
>>> +    .init = uniphier_usb3phy_init,
>>
>> You should implement .exit callback too, that one should do these two 
>> steps:
>>
>> reset_assert_bulk()
>> clk_disable_bulk()
> 
> I think so, however, when I added .exit() and executed "usb stop;usb 
> start",
> unfortunately the command got stuck.
> 
> Currently uniphier clk doesn't support CLK_CCF and can't be nested.
> I think more changes are needed.

Do you know where exactly the system hangs ?

Do you expect to add the CCF support ?

If so, then add at least a FIXME code comment that the exit callback 
must be implemented, so that this is not forgotten once CCF is in place. 
I don't want this series to grow much further into some "rework 
everything at once" thing.