Re: [PATCH v4 net-next 5/5] drivers/net/phy: add driver for the onsemi NCN26000 10BASE-T1S PHY

+// module parameter: if set, the link status is derived from the PLCA status
+// default: false
+static bool link_status_plca;
+module_param(link_status_plca, bool, 0644);

No module parameters, they are considered a bad user interface.

+static int ncn26000_get_features(struct phy_device *phydev)
+{
+	linkmode_zero(phydev->supported);
+	linkmode_set_bit(ETHTOOL_LINK_MODE_MII_BIT, phydev->supported);
+
+	linkmode_set_bit(ETHTOOL_LINK_MODE_10baseT1S_P2MP_Half_BIT,
+			 phydev->supported);
+
+	linkmode_copy(phydev->advertising, phydev->supported);

That should not be needed.

Also, look at PHY_BASIC_T1_FEATURES, and how it is used in
microchip_t1.c.

+static int ncn26000_read_status(struct phy_device *phydev)
+{
+	// The NCN26000 reports NCN26000_LINK_STATUS_BIT if the link status of
+	// the PHY is up. It further reports the logical AND of the link status
+	// and the PLCA status in the BMSR_LSTATUS bit. Thus, report the link
+	// status by testing the appropriate BMSR bit according to the module's
+	// parameter configuration.
+	const int lstatus_flag = link_status_plca ?
+		BMSR_LSTATUS : NCN26000_BMSR_LINK_STATUS_BIT;
+
+	int ret;
+
+	ret = phy_read(phydev, MII_BMSR);
+	if (unlikely(ret < 0))
+		return ret;
+
+	// update link status
+	phydev->link = (ret & lstatus_flag) ? 1 : 0;

What about the latching behaviour of LSTATUS?

https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy_device.c#L2289

+
+	// handle more IRQs here

You are not in an IRQ handler...

You should also be setting speed and duplex. I don't think they are
guaranteed to have any specific value if you don't set them.

+
+	return 0;
+}
+
+static irqreturn_t ncn26000_handle_interrupt(struct phy_device *phydev)
+{
+	const struct ncn26000_priv *const priv = phydev->priv;
+	int ret;
+
+	// clear the latched bits in MII_BMSR
+	phy_read(phydev, MII_BMSR);

Why?

+
+	// read and aknowledge the IRQ status register
+	ret = phy_read(phydev, NCN26000_REG_IRQ_STATUS);
+
+	if (unlikely(ret < 0) || (ret & priv->enabled_irqs) == 0)

How does NCN26000_REG_IRQ_STATUS work? Can it have bits set which are
not in NCN26000_REG_IRQ_CTL ? That does happen sometimes, but is
pretty unusual. If not, you don't need to track priv->enabled_irqs,
just ensure ret is not 0.

     Andrew