Re: [PATCH v2 4/6] phy: realtek: usb2: introduce reset controller struct

[Vladimir Oltean]

On Fri, Mar 27, 2026 at 09:06:36PM +0500, Rustam Adilov wrote:
> In RTL9607C, there is so called "IP Enable Controller" which resemble
> reset controller with reset lines and is used for various things like
> USB, PCIE, GMAC and such.
> 
> Introduce the reset_control struct to this driver to handle deasserting
> usb2 phy reset line.
> 
> Make use of the function devm_reset_control_array_get_optional_exclusive()
> function to get the reset controller and since existing RTD SoCs don't
> specify the resets we can have a cleaner code.
> 
> Co-developed-by: Michael Zavertkin <misha.zavertkin@xxxxxxx>
> Signed-off-by: Michael Zavertkin <misha.zavertkin@xxxxxxx>
> Signed-off-by: Rustam Adilov <adilov@xxxxxxxxxxx>
> ---
>  drivers/phy/realtek/phy-rtk-usb2.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/phy/realtek/phy-rtk-usb2.c b/drivers/phy/realtek/phy-rtk-usb2.c
> index e65b8525b88b..070cba1e0e0a 100644
> --- a/drivers/phy/realtek/phy-rtk-usb2.c
> +++ b/drivers/phy/realtek/phy-rtk-usb2.c
> @@ -17,6 +17,7 @@
>  #include <linux/sys_soc.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/phy/phy.h>
> +#include <linux/reset.h>
>  #include <linux/usb.h>
>  
>  /* GUSB2PHYACCn register */
> @@ -130,6 +131,7 @@ struct rtk_phy {
>  	struct phy_cfg *phy_cfg;
>  	int num_phy;
>  	struct phy_parameter *phy_parameter;
> +	struct reset_control *phy_rst;
>  
>  	struct dentry *debug_dir;
>  };
> @@ -602,6 +604,10 @@ static int do_rtk_phy_init(struct rtk_phy *rtk_phy, int index)
>  	phy_parameter = &((struct phy_parameter *)rtk_phy->phy_parameter)[index];
>  	phy_reg = &phy_parameter->phy_reg;
>  
> +	reset_control_deassert(rtk_phy->phy_rst);

LLM review says:

(less important)
Can reset_control_deassert() fail here? If there is a hardware communication
error with the reset controller, should this check the return value and
propagate the error up instead of proceeding to configure the PHY?
Additionally, since the exclusive reset line is deasserted here, does this
code need a corresponding reset_control_assert() in the driver's teardown
or exit path? Leaving the IP block permanently enabled after shutdown could
lead to power leaks and prevent proper hardware re-initialization.

> +
> +	mdelay(5);

(more important)
This code unnecessarily penalizes existing platforms. If rtk_phy->phy_rst
is NULL (as on older platforms where the optional reset is not defined), the
delay still executes.

Also, since PHY initialization callbacks run in a sleepable context, would it
be better to use a sleep-based delay like usleep_range(5000, 6000) to yield
the CPU instead of busy-waiting with mdelay(5)?

> +
>  	if (phy_cfg->use_default_parameter) {
>  		dev_dbg(rtk_phy->dev, "%s phy#%d use default parameter\n",
>  			__func__, index);
> @@ -1069,6 +1075,12 @@ static int rtk_usb2phy_probe(struct platform_device *pdev)
>  
>  	rtk_phy->num_phy = phy_cfg->num_phy;
>  
> +	rtk_phy->phy_rst = devm_reset_control_array_get_optional_exclusive(dev);
> +	if (IS_ERR(rtk_phy->phy_rst)) {
> +		dev_err(dev, "usb2 phy resets are not working\n");
> +		return PTR_ERR(rtk_phy->phy_rst);
> +	}
> +

(still LLM review)
If the reset controller driver is not yet ready, this will return
-EPROBE_DEFER and print an error message to the kernel log.
Should this use dev_err_probe() to silently handle probe deferral while
correctly logging actual errors?

>  	ret = parse_phy_data(rtk_phy);
>  	if (ret)
>  		goto err;
> -- 
> 2.53.0
> 
>