diff mbox series

[net-next,v1] lan78xx: lan7801 MAC support with lan8841

Message ID 20240611094233.865234-1-rengarajan.s@microchip.com
State New
Headers show
Series [net-next,v1] lan78xx: lan7801 MAC support with lan8841 | expand

Commit Message

Rengarajan S June 11, 2024, 9:42 a.m. UTC
Add lan7801 MAC only support with lan8841. The PHY fixup is registered
for lan8841 and the initializations are done using lan8835_fixup since
the register configs are similar for both lann8841 and lan8835. The PHY
is unregistered at two instances, one during init and other during
disconnect.

Signed-off-by: Rengarajan S <rengarajan.s@microchip.com>
---
 drivers/net/usb/lan78xx.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Jakub Kicinski June 13, 2024, 1:33 a.m. UTC | #1
On Tue, 11 Jun 2024 15:12:33 +0530 Rengarajan S wrote:
>  /* define external phy id */
>  #define	PHY_LAN8835			(0x0007C130)
> +#define PHY_LAN8841			(0x00221650)

For whatever reason the existing code uses a tab between define and its
name, so let's stick to that?

>  #define	PHY_KSZ9031RNX			(0x00221620)
>  
>  /* use ethtool to change the level for any given device */
> @@ -2327,6 +2328,13 @@ static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
>  			netdev_err(dev->net, "Failed to register fixup for PHY_LAN8835\n");
>  			return NULL;
>  		}
> +		/* external PHY fixup for LAN8841 */
> +		ret = phy_register_fixup_for_uid(PHY_LAN8841, 0xfffffff0,
> +						 lan8835_fixup);
> +		if (ret < 0) {
> +			netdev_err(dev->net, "Failed to register fixup for PHY_LAN8841\n");

Don't you have to unregister the previous fixup on the error path here?
In fact the existing error path for PHY_LAN8835 is missing an unregsiter
for PHY_KSZ9031RNX.

Could you please send a separate fix for that with a Fixes tag?

> +			return NULL;
> +		}
Andrew Lunn June 13, 2024, 8:46 p.m. UTC | #2
On Tue, Jun 11, 2024 at 03:12:33PM +0530, Rengarajan S wrote:
> Add lan7801 MAC only support with lan8841. The PHY fixup is registered
> for lan8841 and the initializations are done using lan8835_fixup since
> the register configs are similar for both lann8841 and lan8835.

What exactly does this fixup do?

Looking at it, what protects it from being used on some other device
which also happens to use the same PHY? Is there something to
guarantee:

struct lan78xx_net *dev = netdev_priv(phydev->attached_dev);

really is a lan78xx_net * ?

   Andrew
diff mbox series

Patch

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 5add4145d9fc..ab6f0c42b4d9 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -479,6 +479,7 @@  struct lan78xx_net {
 
 /* define external phy id */
 #define	PHY_LAN8835			(0x0007C130)
+#define PHY_LAN8841			(0x00221650)
 #define	PHY_KSZ9031RNX			(0x00221620)
 
 /* use ethtool to change the level for any given device */
@@ -2327,6 +2328,13 @@  static struct phy_device *lan7801_phy_init(struct lan78xx_net *dev)
 			netdev_err(dev->net, "Failed to register fixup for PHY_LAN8835\n");
 			return NULL;
 		}
+		/* external PHY fixup for LAN8841 */
+		ret = phy_register_fixup_for_uid(PHY_LAN8841, 0xfffffff0,
+						 lan8835_fixup);
+		if (ret < 0) {
+			netdev_err(dev->net, "Failed to register fixup for PHY_LAN8841\n");
+			return NULL;
+		}
 		/* add more external PHY fixup here if needed */
 
 		phydev->is_internal = false;
@@ -2390,6 +2398,8 @@  static int lan78xx_phy_init(struct lan78xx_net *dev)
 							     0xfffffff0);
 				phy_unregister_fixup_for_uid(PHY_LAN8835,
 							     0xfffffff0);
+				phy_unregister_fixup_for_uid(PHY_LAN8841,
+							     0xfffffff0);
 			}
 		}
 		return -EIO;
@@ -4239,6 +4249,7 @@  static void lan78xx_disconnect(struct usb_interface *intf)
 
 	phy_unregister_fixup_for_uid(PHY_KSZ9031RNX, 0xfffffff0);
 	phy_unregister_fixup_for_uid(PHY_LAN8835, 0xfffffff0);
+	phy_unregister_fixup_for_uid(PHY_LAN8841, 0xfffffff0);
 
 	phy_disconnect(net->phydev);