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

[Andrew Lunn <andrew@lunn.ch>]

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