Re: [PATCH net-next v4 1/2] net: dsa: microchip: ksz8: Make flow control, speed, and duplex on CPU port configurable

[Vladimir Oltean]

On Sun, May 21, 2023 at 06:38:41AM +0200, Oleksij Rempel wrote:
> Looks good, I like the idea with "wacky" registers!
> 
> Would you prefer that I start working on adapting your patch set to the
> KSZ8873? Or should I make a review to move forward the existing patch set?
> 
> Just a heads up, I don't have access to the KSZ87xx series switches, so
> I won't be able to test the changes on these models.
> 
> Let me know what you think and how we should proceed.

If we convert to regfields, the entire driver will need to be converted
(all switch families). I'd say make a best effort full conversion, and
if something breaks on the families which you could not test, surely
someone will jump to correct it. Since your KSZ8873 also has wacky registers
(btw, feel free to rename the concept to something more serious), I think
that not a lot can go wrong with a blind conversion as long as it's tested
on other hardware.

BTW, revisiting, I already found a bug in the conversion (patch 2/4):

+	} else if (mii_sel == bitval[P_RMII_SEL]) {
 		interface = PHY_INTERFACE_MODE_RGMII;
 	} else {
+		ret = ksz_regfields_read(dev, port, RF_RGMII_ID_IG_ENABLE, &ig);
+		if (WARN_ON(ret))
+			return PHY_INTERFACE_MODE_NA;
+
+		ret = ksz_regfields_read(dev, port, RF_RGMII_ID_IG_ENABLE, &eg);
						    ~~~~~~~~~~~~~~~~~~~~~
						    should have been RF_RGMII_ID_EG_ENABLE
+		if (WARN_ON(ret))
+			return PHY_INTERFACE_MODE_NA;
+
 		interface = PHY_INTERFACE_MODE_RGMII;
-		if (data8 & P_RGMII_ID_EG_ENABLE)
+		if (eg)
 			interface = PHY_INTERFACE_MODE_RGMII_TXID;
-		if (data8 & P_RGMII_ID_IG_ENABLE) {
+		if (ig) {
 			interface = PHY_INTERFACE_MODE_RGMII_RXID;
-			if (data8 & P_RGMII_ID_EG_ENABLE)
+			if (eg)
 				interface = PHY_INTERFACE_MODE_RGMII_ID;
 		}
 	}