Messages in this thread | | | From | Ken Sloat <> | Subject | RE: [PATCH v1] net: phy: adin: Add flags to disable enhanced link detection | Date | Tue, 28 Feb 2023 15:13:59 +0000 |
| |
Hi Andrew,
Thanks for your quick reply!
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Tuesday, February 28, 2023 9:53 AM > To: Ken Sloat <ken.s@variscite.com> > Cc: Michael Hennerich <michael.hennerich@analog.com>; Heiner Kallweit > <hkallweit1@gmail.com>; Russell King <linux@armlinux.org.uk>; David S. > Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; > 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: > > 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 <ken.s@variscite.com> > Hi Ken > > > +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.
> > +{ > > + struct device *dev = &phydev->mdio.dev; > > + int reg; > > + > > + reg = phy_read_mmd(phydev, MDIO_MMD_VEND1, > ADIN1300_FLD_EN_REG); > > + if (reg < 0) > > + return reg; > > + > > + if (device_property_read_bool(dev, "adi,disable-fld-1000base-t")) > > 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.
> Please also take a read of > https://www.kernel.org/doc/html/latest/process/maintainer- > netdev.html#netdev-faq > > Andrew
Sincerely, Ken Sloat
| |