The LAN8842 is a low-power, single port triple-speed (10BASE-T/ 100BASE-TX/
1000BASE-T) ethernet physical layer transceiver (PHY) that supports
transmission and reception of data on standard CAT-5, as well as CAT-5e and
CAT-6, Unshielded Twisted Pair (UTP) cables.

The LAN8842 supports industry-standard SGMII (Serial Gigabit Media
Independent Interface) providing chip-to-chip connection to a Gigabit
Ethernet MAC using a single serialized link (differential pair) in each
direction.

There are 2 variants of the lan8842. The one that supports timestamping
(lan8842) and one that doesn't have timestamping (lan8832).

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 drivers/net/phy/micrel.c   | 199 +++++++++++++++++++++++++++++++++++++
 include/linux/micrel_phy.h |   1 +
 2 files changed, 200 insertions(+)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index f678c1bdacdf0..c5887a23f9941 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -107,6 +107,7 @@
 #define LAN8814_INTC				0x18
 #define LAN8814_INTS				0x1B
 
+#define LAN8814_INT_FLF				BIT(15)
 #define LAN8814_INT_LINK_DOWN			BIT(2)
 #define LAN8814_INT_LINK_UP			BIT(0)
 #define LAN8814_INT_LINK			(LAN8814_INT_LINK_UP |\
@@ -448,6 +449,20 @@ struct kszphy_priv {
 	struct kszphy_phy_stats phy_stats;
 };
 
+struct lan8842_hw_stat {
+	const char *string;
+	u8 page;
+	u8 regs_count;
+	u8 regs[3];
+};
+
+static struct lan8842_hw_stat lan8842_hw_stats[] = {
+	{ "phy_rx_correct_count", 2, 3, {88, 61, 60}},
+	{ "phy_rx_crc_count", 2, 2, {63, 62}},
+	{ "phy_tx_correct_count", 2, 3, {89, 85, 84}},
+	{ "phy_tx_crc_count", 2, 2, {87, 86}},
+};
+
 static const struct kszphy_type lan8814_type = {
 	.led_mode_reg		= ~LAN8814_LED_CTRL_1,
 	.cable_diag_reg		= LAN8814_CABLE_DIAG,
@@ -5641,6 +5656,174 @@ static int ksz9131_resume(struct phy_device *phydev)
 	return kszphy_resume(phydev);
 }
 
+#define LAN8842_SELF_TEST			14 /* 0x0e */
+#define LAN8842_SELF_TEST_RX_CNT_ENA		BIT(8)
+#define LAN8842_SELF_TEST_TX_CNT_ENA		BIT(4)
+
+static int lan8842_probe(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Similar to lan8814 this PHY has a pin which needs to be pulled down
+	 * to enable to pass any traffic through it. Therefore use the same
+	 * function as lan8814
+	 */
+	ret = lan8814_release_coma_mode(phydev);
+	if (ret)
+		return ret;
+
+	/* Enable to count the RX and TX packets */
+	lanphy_write_page_reg(phydev, 2, LAN8842_SELF_TEST,
+			      LAN8842_SELF_TEST_RX_CNT_ENA |
+			      LAN8842_SELF_TEST_TX_CNT_ENA);
+
+	return 0;
+}
+
+#define LAN8842_SGMII_AUTO_ANEG_ENA		69 /* 0x45 */
+#define LAN8842_FLF				15 /* 0x0e */
+#define LAN8842_FLF_ENA				BIT(1)
+#define LAN8842_FLF_ENA_LINK_DOWN		BIT(0)
+
+static int lan8842_config_init(struct phy_device *phydev)
+{
+	int val;
+	int ret;
+
+	/* Reset the PHY */
+	val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
+	if (val < 0)
+		return val;
+	val |= LAN8814_QSGMII_SOFT_RESET_BIT;
+	lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);
+
+	/* Disable ANEG with QSGMII PCS Host side
+	 * It has the same address as lan8814
+	 */
+	val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
+	if (val < 0)
+		return val;
+	val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
+	ret = lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG,
+				    val);
+	if (ret < 0)
+		return ret;
+
+	/* Disable also the SGMII_AUTO_ANEG_ENA, this will determine what is the
+	 * PHY autoneg with the other end and then will update the host side
+	 */
+	lanphy_write_page_reg(phydev, 4, LAN8842_SGMII_AUTO_ANEG_ENA, 0);
+
+	/* To allow the PHY to control the LEDs the GPIOs of the PHY should have
+	 * a function mode and not the GPIO. Apparently by default the value is
+	 * GPIO and not function even though the datasheet it says that it is
+	 * function. Therefore set this value.
+	 */
+	lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_EN2, 0);
+
+	/* Enable the Fast link failure, at the top level, at the bottom level
+	 * it would be set/cleared inside lan8842_config_intr
+	 */
+	val = lanphy_read_page_reg(phydev, 0, LAN8842_FLF);
+	if (val < 0)
+		return val;
+	val |= LAN8842_FLF_ENA | LAN8842_FLF_ENA_LINK_DOWN;
+	lanphy_write_page_reg(phydev, 0, LAN8842_FLF, val);
+
+	return 0;
+}
+
+#define LAN8842_INTR_CTRL_REG			52 /* 0x34 */
+
+static int lan8842_config_intr(struct phy_device *phydev)
+{
+	int err;
+
+	lanphy_write_page_reg(phydev, 4, LAN8842_INTR_CTRL_REG,
+			      LAN8814_INTR_CTRL_REG_INTR_ENABLE);
+
+	/* enable / disable interrupts */
+	if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+		err = lan8814_ack_interrupt(phydev);
+		if (err)
+			return err;
+
+		err = phy_write(phydev, LAN8814_INTC,
+				LAN8814_INT_LINK | LAN8814_INT_FLF);
+	} else {
+		err = phy_write(phydev, LAN8814_INTC, 0);
+		if (err)
+			return err;
+
+		err = lan8814_ack_interrupt(phydev);
+	}
+
+	return err;
+}
+
+static irqreturn_t lan8842_handle_interrupt(struct phy_device *phydev)
+{
+	int ret = IRQ_NONE;
+	int irq_status;
+
+	irq_status = phy_read(phydev, LAN8814_INTS);
+	if (irq_status < 0) {
+		phy_error(phydev);
+		return IRQ_NONE;
+	}
+
+	if (irq_status & (LAN8814_INT_LINK | LAN8814_INT_FLF)) {
+		phy_trigger_machine(phydev);
+		ret = IRQ_HANDLED;
+	}
+
+	return ret;
+}
+
+static int lan8842_get_sset_count(struct phy_device *phydev)
+{
+	return ARRAY_SIZE(lan8842_hw_stats);
+}
+
+static void lan8842_get_strings(struct phy_device *phydev, u8 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lan8842_hw_stats); i++) {
+		strscpy(data + i * ETH_GSTRING_LEN,
+			lan8842_hw_stats[i].string, ETH_GSTRING_LEN);
+	}
+}
+
+static u64 lan8842_get_stat(struct phy_device *phydev, int i)
+{
+	struct lan8842_hw_stat stat = lan8842_hw_stats[i];
+	int val;
+	u64 ret = 0;
+
+	for (int j = 0; j < stat.regs_count; ++j) {
+		val = lanphy_read_page_reg(phydev,
+					   stat.page,
+					   stat.regs[j]);
+		if (val < 0)
+			return U64_MAX;
+
+		ret <<= 16;
+		ret += val;
+	}
+
+	return ret;
+}
+
+static void lan8842_get_stats(struct phy_device *phydev,
+			      struct ethtool_stats *stats, u64 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(lan8842_hw_stats); i++)
+		data[i] = lan8842_get_stat(phydev, i);
+}
+
 static struct phy_driver ksphy_driver[] = {
 {
 	.phy_id		= PHY_ID_KS8737,
@@ -5869,6 +6052,21 @@ static struct phy_driver ksphy_driver[] = {
 	.resume		= lan8841_resume,
 	.cable_test_start	= lan8814_cable_test_start,
 	.cable_test_get_status	= ksz886x_cable_test_get_status,
+}, {
+	.phy_id		= PHY_ID_LAN8842,
+	.phy_id_mask	= MICREL_PHY_ID_MASK,
+	.name		= "Microchip LAN8842 Gigabit PHY",
+	.flags		= PHY_POLL_CABLE_TEST,
+	.driver_data	= &lan8814_type,
+	.probe		= lan8842_probe,
+	.config_init	= lan8842_config_init,
+	.config_intr	= lan8842_config_intr,
+	.handle_interrupt = lan8842_handle_interrupt,
+	.get_sset_count	= lan8842_get_sset_count,
+	.get_strings	= lan8842_get_strings,
+	.get_stats	= lan8842_get_stats,
+	.cable_test_start	= lan8814_cable_test_start,
+	.cable_test_get_status	= ksz886x_cable_test_get_status,
 }, {
 	.phy_id		= PHY_ID_KSZ9131,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
@@ -5967,6 +6165,7 @@ static const struct mdio_device_id __maybe_unused micrel_tbl[] = {
 	{ PHY_ID_LAN8814, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8804, MICREL_PHY_ID_MASK },
 	{ PHY_ID_LAN8841, MICREL_PHY_ID_MASK },
+	{ PHY_ID_LAN8842, MICREL_PHY_ID_MASK },
 	{ }
 };
 
diff --git a/include/linux/micrel_phy.h b/include/linux/micrel_phy.h
index 9af01bdd86d26..ca691641788b8 100644
--- a/include/linux/micrel_phy.h
+++ b/include/linux/micrel_phy.h
@@ -32,6 +32,7 @@
 #define PHY_ID_LAN8814		0x00221660
 #define PHY_ID_LAN8804		0x00221670
 #define PHY_ID_LAN8841		0x00221650
+#define PHY_ID_LAN8842		0x002216C0
 
 #define PHY_ID_KSZ886X		0x00221430
 #define PHY_ID_KSZ8863		0x00221435
-- 
2.34.1

Hi Horatiu,

On Mon, Jul 21, 2025 at 09:14:05AM +0200, Horatiu Vultur wrote:

> +static int lan8842_config_init(struct phy_device *phydev)
> +{
> +	int val;
> +	int ret;
> +
> +	/* Reset the PHY */
> +	val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);

It would be good to use defines for MMD pages.

> +	if (val < 0)
> +		return val;
> +	val |= LAN8814_QSGMII_SOFT_RESET_BIT;
> +	lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);

Please, do not ignore return values.

> +
> +	/* Disable ANEG with QSGMII PCS Host side
> +	 * It has the same address as lan8814
> +	 */
> +	val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
> +	if (val < 0)
> +		return val;
> +	val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
> +	ret = lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG,
> +				    val);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Disable also the SGMII_AUTO_ANEG_ENA, this will determine what is the
> +	 * PHY autoneg with the other end and then will update the host side
> +	 */
> +	lanphy_write_page_reg(phydev, 4, LAN8842_SGMII_AUTO_ANEG_ENA, 0);
> +
> +	/* To allow the PHY to control the LEDs the GPIOs of the PHY should have
> +	 * a function mode and not the GPIO. Apparently by default the value is
> +	 * GPIO and not function even though the datasheet it says that it is
> +	 * function. Therefore set this value.
> +	 */
> +	lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_EN2, 0);
> +
> +	/* Enable the Fast link failure, at the top level, at the bottom level
> +	 * it would be set/cleared inside lan8842_config_intr
> +	 */
> +	val = lanphy_read_page_reg(phydev, 0, LAN8842_FLF);
> +	if (val < 0)
> +		return val;
> +	val |= LAN8842_FLF_ENA | LAN8842_FLF_ENA_LINK_DOWN;

If I see it correctly, FLF support will make link fail after ~1ms, while
IEEE 802.3 recommends 750ms. Since a link recovery of a PHY with autoneg
support usually takes multiple seconds, I see the benefit for FLF
support only mostly for SyncE environment at same time it seems to be
a disadvantage for other environments.

I would prefer to have IEEE 802.3 recommended link behavior by default
and have separate Netlink configuration interface for FLF.

Best Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

The 07/23/2025 07:34, Oleksij Rempel wrote:
> 
> Hi Horatiu,

Hi Olekij,

> 
> On Mon, Jul 21, 2025 at 09:14:05AM +0200, Horatiu Vultur wrote:
> 
> > +static int lan8842_config_init(struct phy_device *phydev)
> > +{
> > +     int val;
> > +     int ret;
> > +
> > +     /* Reset the PHY */
> > +     val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
> 
> It would be good to use defines for MMD pages.

Those are extended pages and not MMD pages. Currently in the entire
source code I can see we used hardcoded values, also in the register
description it looks like all these extended pages do not have really
meaningfull names: Extended Page 0, Extended Page 4, Extended Page 5...

> 
> > +     if (val < 0)
> > +             return val;
> > +     val |= LAN8814_QSGMII_SOFT_RESET_BIT;
> > +     lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);
> 
> Please, do not ignore return values.

Good catch, I will fix that in the next version.
There are few others bellow, I will fix those also.

> 
> > +
> > +     /* Disable ANEG with QSGMII PCS Host side
> > +      * It has the same address as lan8814
> > +      */
> > +     val = lanphy_read_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG);
> > +     if (val < 0)
> > +             return val;
> > +     val &= ~LAN8814_QSGMII_PCS1G_ANEG_CONFIG_ANEG_ENA;
> > +     ret = lanphy_write_page_reg(phydev, 5, LAN8814_QSGMII_PCS1G_ANEG_CONFIG,
> > +                                 val);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     /* Disable also the SGMII_AUTO_ANEG_ENA, this will determine what is the
> > +      * PHY autoneg with the other end and then will update the host side
> > +      */
> > +     lanphy_write_page_reg(phydev, 4, LAN8842_SGMII_AUTO_ANEG_ENA, 0);
> > +
> > +     /* To allow the PHY to control the LEDs the GPIOs of the PHY should have
> > +      * a function mode and not the GPIO. Apparently by default the value is
> > +      * GPIO and not function even though the datasheet it says that it is
> > +      * function. Therefore set this value.
> > +      */
> > +     lanphy_write_page_reg(phydev, 4, LAN8814_GPIO_EN2, 0);
> > +
> > +     /* Enable the Fast link failure, at the top level, at the bottom level
> > +      * it would be set/cleared inside lan8842_config_intr
> > +      */
> > +     val = lanphy_read_page_reg(phydev, 0, LAN8842_FLF);
> > +     if (val < 0)
> > +             return val;
> > +     val |= LAN8842_FLF_ENA | LAN8842_FLF_ENA_LINK_DOWN;
> 
> If I see it correctly, FLF support will make link fail after ~1ms, while
> IEEE 802.3 recommends 750ms. Since a link recovery of a PHY with autoneg
> support usually takes multiple seconds, I see the benefit for FLF
> support only mostly for SyncE environment at same time it seems to be
> a disadvantage for other environments.

Why would be a disadvantage?
> 
> I would prefer to have IEEE 802.3 recommended link behavior by default
> and have separate Netlink configuration interface for FLF.
> 
> Best Regards,
> Oleksij
> --
> Pengutronix e.K.                           |                             |
> Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
> 31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

-- 
/Horatiu

On Wed, Jul 23, 2025 at 11:01:45AM +0200, Horatiu Vultur wrote:
> The 07/23/2025 07:34, Oleksij Rempel wrote:
> > 
> > Hi Horatiu,
> 
> Hi Olekij,
> 
> > 
> > On Mon, Jul 21, 2025 at 09:14:05AM +0200, Horatiu Vultur wrote:
> > 
> > > +static int lan8842_config_init(struct phy_device *phydev)
> > > +{
> > > +     int val;
> > > +     int ret;
> > > +
> > > +     /* Reset the PHY */
> > > +     val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
> > 
> > It would be good to use defines for MMD pages.
> 
> Those are extended pages and not MMD pages. Currently in the entire
> source code I can see we used hardcoded values, also in the register
> description it looks like all these extended pages do not have really
> meaningfull names: Extended Page 0, Extended Page 4, Extended Page 5...

I'll be happy with xxxx_EXT_PAGE_0, etc 

> > > +     val = lanphy_read_page_reg(phydev, 0, LAN8842_FLF);
> > > +     if (val < 0)
> > > +             return val;
> > > +     val |= LAN8842_FLF_ENA | LAN8842_FLF_ENA_LINK_DOWN;
> > 
> > If I see it correctly, FLF support will make link fail after ~1ms, while
> > IEEE 802.3 recommends 750ms. Since a link recovery of a PHY with autoneg
> > support usually takes multiple seconds, I see the benefit for FLF
> > support only mostly for SyncE environment at same time it seems to be
> > a disadvantage for other environments.
> 
> Why would be a disadvantage?

The disadvantage in a standard network without a backup link (like one
using RSTP) comes from how the system handles recoverable, temporary
errors like a short burst of noise.

# Standard PHY Behavior (Grace Period)
When a standard 1000BASE-T link becomes unstable, the IEEE 802.3
standard requires the PHY to attempt to retrain and recover the
connection on its own. It has a timeout window of up to 750 ms to do
this, which acts as a grace period.

If the link issue was temporary and the PHY recovers within this window,
the operating system never sees a "link down" event. Applications only
experience a brief moment of packet loss, which is often handled
gracefully by protocols like TCP.

# FLF Behavior (Immediate Failure)
An FLF-enabled PHY is designed to report link instability almost
immediately (~1 ms). Instead of trying to recover silently, it
immediately reports a hard link failure to the operating system.

# The Disadvantage in a Single-Link System

For a system with only one link, this "fail-fast" approach can be a
disadvantage. Consider a short, recoverable noise burst:

- Without FLF: The PHY uses its 750 ms grace period to recover. The
link stays up, and the service interruption is limited to brief packet
loss.

- With FLF: The PHY reports "link down" after ~1 ms. The operating
system tears down the network interface. Even if the hardware recovers
quickly, the OS has to bring the interface back up, re-run DHCP, and
re-establish all application connections. This system-level recovery
often takes much longer than the original glitch.

In short, FLF can turn a minor, recoverable physical-layer glitch into a
more disruptive, longer-lasting outage at the application level when
there is no backup link to switch to.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

> # FLF Behavior (Immediate Failure)
> An FLF-enabled PHY is designed to report link instability almost
> immediately (~1 ms). Instead of trying to recover silently, it
> immediately reports a hard link failure to the operating system.
> 
> # The Disadvantage in a Single-Link System
> 
> For a system with only one link, this "fail-fast" approach can be a
> disadvantage. Consider a short, recoverable noise burst:
> 
> - Without FLF: The PHY uses its 750 ms grace period to recover. The
> link stays up, and the service interruption is limited to brief packet
> loss.
> 
> - With FLF: The PHY reports "link down" after ~1 ms. The operating
> system tears down the network interface. Even if the hardware recovers
> quickly, the OS has to bring the interface back up, re-run DHCP, and
> re-establish all application connections. This system-level recovery
> often takes much longer than the original glitch.
> 
> In short, FLF can turn a minor, recoverable physical-layer glitch into a
> more disruptive, longer-lasting outage at the application level when
> there is no backup link to switch to.

Fast link down can be a useful feature to have, but the PHY should
default to what 802.3 says. There is however:

ETHTOOL_PHY_FAST_LINK_DOWN

which two drivers support.

	Andrew

The 07/23/2025 16:28, Andrew Lunn wrote:

Hi,

> 
> > # FLF Behavior (Immediate Failure)
> > An FLF-enabled PHY is designed to report link instability almost
> > immediately (~1 ms). Instead of trying to recover silently, it
> > immediately reports a hard link failure to the operating system.
> >
> > # The Disadvantage in a Single-Link System
> >
> > For a system with only one link, this "fail-fast" approach can be a
> > disadvantage. Consider a short, recoverable noise burst:
> >
> > - Without FLF: The PHY uses its 750 ms grace period to recover. The
> > link stays up, and the service interruption is limited to brief packet
> > loss.
> >
> > - With FLF: The PHY reports "link down" after ~1 ms. The operating
> > system tears down the network interface. Even if the hardware recovers
> > quickly, the OS has to bring the interface back up, re-run DHCP, and
> > re-establish all application connections. This system-level recovery
> > often takes much longer than the original glitch.
> >
> > In short, FLF can turn a minor, recoverable physical-layer glitch into a
> > more disruptive, longer-lasting outage at the application level when
> > there is no backup link to switch to.
> 
> Fast link down can be a useful feature to have, but the PHY should
> default to what 802.3 says. There is however:
> 
> ETHTOOL_PHY_FAST_LINK_DOWN
> 
> which two drivers support.

OK, by default the FLF support will not be enabled and then in a
different patch I will allow to configure the FLF using
ETHTOOL_PHY_FAST_LINK_DOWN.


> 
>         Andrew

-- 
/Horatiu

> +static struct lan8842_hw_stat lan8842_hw_stats[] = {
> +	{ "phy_rx_correct_count", 2, 3, {88, 61, 60}},
> +	{ "phy_rx_crc_count", 2, 2, {63, 62}},
> +	{ "phy_tx_correct_count", 2, 3, {89, 85, 84}},
> +	{ "phy_tx_crc_count", 2, 2, {87, 86}},
> +};

Hi Horatiu

Please could you look at using ethtool_phy_stats via the
.get_phy_stats() phy driver op.

> +static int lan8842_config_init(struct phy_device *phydev)
> +{
> +	int val;
> +	int ret;
> +
> +	/* Reset the PHY */
> +	val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
> +	if (val < 0)
> +		return val;
> +	val |= LAN8814_QSGMII_SOFT_RESET_BIT;
> +	lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);

It looks like there are sufficient pairs of
lanphy_read_page_reg()/lanphy_write_page_reg() you would be justified
adding a lanphy_modify_page_reg().

> +}, {
> +	.phy_id		= PHY_ID_LAN8842,
> +	.phy_id_mask	= MICREL_PHY_ID_MASK,

I think you could use PHY_ID_MATCH_MODEL() here.  It would make it
different to every other PHY, but maybe you could start with some
cleanup patches, adding the _modify_ call etc?

	Andrew

The 07/21/2025 16:22, Andrew Lunn wrote:

Hi Andrew,

> 
> > +static struct lan8842_hw_stat lan8842_hw_stats[] = {
> > +     { "phy_rx_correct_count", 2, 3, {88, 61, 60}},
> > +     { "phy_rx_crc_count", 2, 2, {63, 62}},
> > +     { "phy_tx_correct_count", 2, 3, {89, 85, 84}},
> > +     { "phy_tx_crc_count", 2, 2, {87, 86}},
> > +};
> 
> Hi Horatiu
> 
> Please could you look at using ethtool_phy_stats via the
> .get_phy_stats() phy driver op.

Yes, definetly I will update this in the next version.

> 
> > +static int lan8842_config_init(struct phy_device *phydev)
> > +{
> > +     int val;
> > +     int ret;
> > +
> > +     /* Reset the PHY */
> > +     val = lanphy_read_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET);
> > +     if (val < 0)
> > +             return val;
> > +     val |= LAN8814_QSGMII_SOFT_RESET_BIT;
> > +     lanphy_write_page_reg(phydev, 4, LAN8814_QSGMII_SOFT_RESET, val);
> 
> It looks like there are sufficient pairs of
> lanphy_read_page_reg()/lanphy_write_page_reg() you would be justified
> adding a lanphy_modify_page_reg().

I agree, I can do that.

> 
> > +}, {
> > +     .phy_id         = PHY_ID_LAN8842,
> > +     .phy_id_mask    = MICREL_PHY_ID_MASK,
> 
> I think you could use PHY_ID_MATCH_MODEL() here.  It would make it
> different to every other PHY, but maybe you could start with some
> cleanup patches, adding the _modify_ call etc?

Yes, I will use PHY_ID_MATCH_MODEL().
I was thinking to start with 2 cleanup patches, one to use
PHY_ID_MATCH_MODEL in the driver and one to introduce
lanphy_modify_page_reg and put it to use.
Do you have other suggestions?

> 
>         Andrew

-- 
/Horatiu

> Yes, I will use PHY_ID_MATCH_MODEL().
> I was thinking to start with 2 cleanup patches, one to use
> PHY_ID_MATCH_MODEL in the driver and one to introduce
> lanphy_modify_page_reg and put it to use.
> Do you have other suggestions?

I did not look at the driver too closely. There might be other
cleanups you can do, but those two make sense in the context of what
you are doing.

	Andrew