Re: [PATCH net-next 1/2] net: phy: realtek: disable broadcast address feature of rtl8211f

[Heiner Kallweit <hkallweit1@gmail.com>]

On 03.12.2024 09:35, Zhiyuan Wan wrote:
> On 2024/12/3 15:38, Heiner Kallweit wrote:
>>
>> Take care to remove the Rb tag if you make changes to the patch.
>>
> Roger that.
>>
>> This still uses the _changed version even if not needed.
>>
> I'm not sure is it okay to directly write PHYCR1 register, because not
> only it controls ALDPS and broadcast PHY address, but also controls
> MDI mode/Jabber detection.
> 

This was not my point. Just use phy_modify_paged().

> I'm afraid that maybe it causes problem if I don't use RMW to clear
> the PHYAD_EN bit. Because the following code in `rtl8211f_config_init`
> also utilizes `phy_modify_paged_changed` to change ALDPS setting
> without touching anything else.
> 
> But if you insist, I can modify code to this if you like:
> 
> 
> 	ret = phy_read_paged(phydev, 0xa43, RTL8211F_PHYCR1);
> 	if (ret < 0)
> 		return ret;
> 
> 	dev_dbg(dev, "disabling MDIO address 0 for this phy");
> 	priv->phycr1 = ret & (u16)~RTL8211F_PHYAD0_EN;
> 	ret = phy_write_paged(phydev, 0xa43, RTL8211F_PHYCR1,
> 			      priv->phycr1);
> 	if (ret < 0) {
> 		return dev_err_probe(dev, ret,
> 			             "disabling MDIO address 0 failed\n");
> 	}
> 	/* Don't allow using broadcast address as PHY address */
> 	if (phydev->mdio.addr == 0)
> 		return -ENODEV;
> 
>  	priv->phycr1 &= (RTL8211F_ALDPS_PLL_OFF | RTL8211F_ALDPS_ENABLE | RTL8211F_ALDPS_XTAL_OFF);
> ...
> 
> 
> 
>>> +	if (ret < 0) {
>>> +		dev_err(dev, "disabling MDIO address 0 failed: %pe\n",
>>> +			ERR_PTR(ret));
>>
>> You may want to use dev_err_probe() here. And is it by intent that
>> the error is ignored and you go on?
>>
> 
> I'm sorry that I made a mistake.
> 
>>
>> And one more formal issue:
>> You annotated the patch as 1/2, but submit it as single patch.
>>
> I have another patch to enable support optical/copper combo support
> of RTL8211FS PHY in this mail thread, but since Andrew said that patch
> (migrated from Rockchip SDK) is low quality and I'm too busy with my
> job, don't have much time to read and improve it, so I decided to
> suspend that patch's submission and I'll resume to submit that patch
> when I'm free. Could you please give me some advice or recommends on it?
> 

The patch here seems to be independent and should be properly submitted
as single patch. Regarding the other patch I have nothing to add to what
Andrew stated already.

> Sincerely,
> 
> Zhiyuan Wan