Re: [PATCH net-next] net: phy: micrel: Add support for PTP_PF_PEROUT for lan8841

[Russell King (Oracle)]

On Fri, Feb 17, 2023 at 01:30:06PM +0000, Russell King (Oracle) wrote:
> On Fri, Feb 17, 2023 at 08:52:13AM +0100, Horatiu Vultur wrote:
> > +static void lan8841_ptp_perout_off(struct kszphy_ptp_priv *ptp_priv, int pin)
> > +{
> > +	struct phy_device *phydev = ptp_priv->phydev;
> > +	u16 tmp;
> > +
> > +	tmp = phy_read_mmd(phydev, 2, LAN8841_GPIO_EN) & LAN8841_PTP_GPIO_MASK;
> > +	tmp &= ~BIT(pin);
> > +	phy_write_mmd(phydev, 2, LAN8841_GPIO_EN, tmp);
> 
> Problem 1: doesn't check the return value of phy_read_mmd(), so a
> spurious error results in an error code written back to the register.
> 
> Issue 2: please use phy_modify_mmd() and definitions for the MMD. It
> probably also makes sense to cache the mask. Thus, this whole thing
> becomes:
> 
> 	u16 mask = ~(LAN8841_PTP_GPIO_MASK | BIT(pin));
> 
> 	phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_EN, mask, 0);
> 	phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_DIR, mask, 0);
> 	phy_modify_mmd(phydev, MDIO_MMD_WIS, LAN8841_GPIO_BUF, mask, 0);
> 
> although I'm not sure why you need to mask off bits 15:11.

Or even use phy_clear_bits_mmd(). There's also phy_set_bits_mmd() which I
think would also be useful elsewhere in this driver.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!