Re: [PATCH v7 2/2] phy: Add driver for EyeQ5 Ethernet PHY wrapper

[Théo Lebrun]

Hello Vladimir,

On Fri Feb 27, 2026 at 6:14 PM CET, Vladimir Oltean wrote:
> On Wed, Feb 25, 2026 at 05:54:41PM +0100, Théo Lebrun wrote:
>> +static int eq5_phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>> +{
>> +	struct eq5_phy_inst *inst = phy_get_drvdata(phy);
>> +
>> +	if (eq5_phy_validate(phy, mode, submode, NULL))
>> +		return -EOPNOTSUPP;
>
> Propagate the phy_validate() return code, don't generate your own.
> -EINVAL should be preferable to -EOPNOTSUPP, so that callers can
> distinguish between "phy_set_mode() not implemented" and "phy_set_mode()
> failed".

ACK. I had made the decision to explicitely override the return value
but indeed EOPNOTSUPP isn't the cleverest option. Will fix.

> (yeah, phy_set_mode() was made optional a while ago, IMO incorrectly,
> but that's another story)
>
>> +
>> +	if (submode == inst->phy_interface)
>> +		return 0;
>
> I think this simple comparison fails to serve its intended purpose
> (avoid PHY reset when not changing modes) for RGMII modes, of which
> there exist 4 variants.

Yes!

> Maybe:
> 	if ((phy_interface_mode_is_rgmii(submode) &&
> 	     phy_interface_mode_is_rgmii(inst->phy_interface)) ||
> 	    submode == inst->phy_interface)
> 		return 0;
>
> Does the EyeQ5 platform support internal RGMII delays? If yes, which
> layer enables them? The Generic PHY?

You are on point. We shouldn't care about the RGMII delays inside the
generic PHY driver. What we deal with here is a wrapper to the actual
net PHY behind the scenes. The net PHY is dealing with delays, we can
ignore them in the generic PHY driver.

Will fix, either with your solution or with a custom two state enum that
can do SGMII or RGMII (will represent all RGMII delay variants). I'll
experiment with both and send what looks better.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com