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

Rustam Adilov
2026-03-31T16:35:38Z

On 2026-03-30 21:39, Vladimir Oltean wrote:
> 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@mail.ru>
>> Signed-off-by: Michael Zavertkin <misha.zavertkin@mail.ru>
>> Signed-off-by: Rustam Adilov <adilov@disroot.org>
>> ---
>> 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.

It realistically shouldn't fail. But I can add the return error for this.
And no, it doesn't need the reset_control_assert.

>> +
>> + 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)?

I can change mdelay to msleep and wrap it around something like if (rtk_phy->phy_rst).

>> +
>> 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?

I can change it to dev_err_probe, no problem with that.

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