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

[Kunihiko Hayashi]

Hi Marek,

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.

>> +	return ret;
>> +}
>> +
>> +static int uniphier_usb3phy_probe(struct udevice *dev)
>> +{
>> +	struct uniphier_usb3phy_priv *priv = dev_get_priv(dev);
>> +	int ret;
>> +
>> +	ret = clk_get_bulk(dev, &priv->clks);
>> +	if (ret) {
>> +		if (ret != -ENOSYS && ret != -ENOENT) {
> 
> This can be single line:
> 
> if (ret && ret != -ENOSYS && ret != -ENOENT)
>     return ret;

OK, This will be fixed.

>> +			printf("Failed to get clocks\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>> +	ret = reset_get_bulk(dev, &priv->rsts);
>> +	if (ret) {
>> +		if (ret != -ENOSYS && ret != -ENOENT) {
> 
> Same here.
> 
>> +			printf("Failed to get resets\n");
>> +			clk_release_bulk(&priv->clks);
> 
> Better use goto out_clk fail path.
> 
>> +			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.

Thank you,

---
Best Regards
Kunihiko Hayashi