Re: [PATCH net-next v2] net: phy: Add driver for Motorcomm yt8531 gigabit ethernet phy

[Andrew Lunn]

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