Messages in this thread | | | Date | Fri, 17 Feb 2023 13:33:42 +0000 | From | "Russell King (Oracle)" <> | Subject | Re: [PATCH net-next] net: phy: micrel: Add support for PTP_PF_PEROUT for lan8841 |
| |
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!
| |