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

Andrew Lunn
2022-12-06T13:48:22Z

> +// 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