On Tue, Feb 28, 2023 at 03:13:59PM +0000, Ken Sloat wrote:

Hi Andrew,

Thanks for your quick reply!

quoted

-----Original Message-----
From: Andrew Lunn <andrew@lunn.ch>
Sent: Tuesday, February 28, 2023 9:53 AM
To: Ken Sloat <redacted>
Cc: Michael Hennerich <michael.hennerich@analog.com>; Heiner Kallweit
[off-list ref]; Russell King [off-list ref]; David S.
Miller [off-list ref]; Jakub Kicinski [off-list ref];
netdev@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] net: phy: adin: Add flags to disable enhanced link
detection

On Tue, Feb 28, 2023 at 09:40:56AM -0500, Ken Sloat wrote:

quoted

Enhanced link detection is an ADI PHY feature that allows for earlier
detection of link down if certain signal conditions are met. This
feature is for the most part enabled by default on the PHY. This is
not suitable for all applications and breaks the IEEE standard as
explained in the ADI datasheet.

To fix this, add override flags to disable enhanced link detection for
1000BASE-T and 100BASE-TX respectively by clearing any related feature
enable bits.

This new feature was tested on an ADIN1300 but according to the
datasheet applies equally for 100BASE-TX on the ADIN1200.

Signed-off-by: Ken Sloat <redacted>

Hi Ken

quoted

+static int adin_config_fld_en(struct phy_device *phydev)

Could we have a better name please. I guess it means Fast Link Down, but
the commit messages talks about Enhanced link detection. This function is
also not enabling fast link down, but disabling it, so _en seems wrong.

"Enhanced Link Detection" is the ADI term, but the associated register for controlling this feature is called "FLD_EN." I considered "ELD" as that makes more sense language wise but it did not match the datasheet and did not want to invent a new term. I was not sure what the F was but perhaps you are right, as the link is brought down as part of this feature when conditions are met. I am guessing then that this FLD is a carryover from some initial name of the feature that was later re-branded.

I am happy to change fld -> eld or something else that might make more sense for users and am open to any suggestions.

The Marvell PHYs also support a fast link down mode, so i think using
fast link down everywhere, in the code and the commit message would be
good. How about adin_fast_down_disable().

quoted

You need to document these two properties in the device tree binding.

I already have a separate patch for this. I will send both patches
when I re-submit and CC additional parties.

It is normal to submit them together as a patch set. What generally
happens is that the DT maintainers ACK the documentation patch, and
then it gets merged via the netdev tree.

   Andrew