Messages in this thread | | | Date | Fri, 2 Dec 2022 14:27:43 +0100 | From | Andrew Lunn <> | Subject | Re: [PATCH net-next v2] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy |
| |
> +static bool mdio_is_locked(struct phy_device *phydev) > +{ > + return mutex_is_locked(&phydev->mdio.bus->mdio_lock); > +} > + > +#define ASSERT_MDIO(phydev) \ > + WARN_ONCE(!mdio_is_locked(phydev), \ > + "MDIO: assertion failed at %s (%d)\n", __FILE__, __LINE__) > +
Hi Frank
You are not the only one who gets locking wrong. This could be used in other drivers. Please add it to include/linux/phy.h,
> /** > * ytphy_read_ext() - read a PHY's extended register > * @phydev: a pointer to a &struct phy_device > @@ -258,6 +271,8 @@ static int ytphy_read_ext(struct phy_device *phydev, u16 regnum) > { > int ret; > > + ASSERT_MDIO(phydev); > + > ret = __phy_write(phydev, YTPHY_PAGE_SELECT, regnum); > if (ret < 0) > return ret; > @@ -297,6 +312,8 @@ static int ytphy_write_ext(struct phy_device *phydev, u16 regnum, u16 val) > { > int ret; > > + ASSERT_MDIO(phydev); > + > ret = __phy_write(phydev, YTPHY_PAGE_SELECT, regnum); > if (ret < 0) > return ret; > @@ -342,6 +359,8 @@ static int ytphy_modify_ext(struct phy_device *phydev, u16 regnum, u16 mask, > { > int ret; > > + ASSERT_MDIO(phydev); > + > ret = __phy_write(phydev, YTPHY_PAGE_SELECT, regnum); > if (ret < 0) > return ret; > @@ -479,6 +498,76 @@ static int ytphy_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol) > return phy_restore_page(phydev, old_page, ret); > }
Please make the above one patch, which adds the macro and its users. There are a couple more below as well.
Did it find any problems in the current code? Any fixes mixed in here?
Then add yt8531 is another patch.
> +/** > + * yt8531_set_wol() - turn wake-on-lan on or off > + * @phydev: a pointer to a &struct phy_device > + * @wol: a pointer to a &struct ethtool_wolinfo > + * > + * returns 0 or negative errno code > + */ > +static int yt8531_set_wol(struct phy_device *phydev, > + struct ethtool_wolinfo *wol) > +{ > + struct net_device *p_attached_dev; > + const u16 mac_addr_reg[] = { > + YTPHY_WOL_MACADDR2_REG, > + YTPHY_WOL_MACADDR1_REG, > + YTPHY_WOL_MACADDR0_REG, > + }; > + const u8 *mac_addr; > + u16 mask; > + u16 val; > + int ret; > + u8 i; > + > + if (wol->wolopts & WAKE_MAGIC) { > + p_attached_dev = phydev->attached_dev; > + if (!p_attached_dev) > + return -ENODEV; > + > + mac_addr = (const u8 *)p_attached_dev->dev_addr;
Why the cast?
> + if (!is_valid_ether_addr(mac_addr)) > + return -EINVAL;
Andrew
| |