Re: [PATCH net-next v3 2/3] net: phy: air_en8811h: add Airoha AN8811HB support

Jakub Kicinski [off-list ref] writes:

On Mon, 26 Jan 2026 07:57:48 +0100 Bjørn Mork wrote:

quoted

+		mdelay(300);

Did you mean msleep()? mdelay(300) is a lot of spinning.

Doh!  Thanks.  I will write that 300 times on the chalkboard.

quoted

+		air_buckpbus_reg_read(phydev, mon2, &pbus_value);
+
+		if (pbus_value & AN8811HB_CRC_ST) {
+			air_buckpbus_reg_read(phydev, mon3, &pbus_value);
+			phydev_dbg(phydev, "CRC Check %s!\n",
+				   pbus_value & AN8811HB_CRC_CHECK_PASS ?
+					"PASS" : "FAIL");

AI code review points out on failure you just print a FAIL and carry on.
Is this because this is what the vendor driver does? Or we know bad CRC
FWs exist in the wild? A comment would be useful here..

Spot on. Explaining this in a comment for you, me and the AI is a good
idea.

Please name labels after what they jump to. Per CodingStyle..

will do if there are any labels left after refactoring.

quoted

+	ret = air_buckpbus_reg_write(phydev, EN8811H_FW_CTRL_1,
+				     EN8811H_FW_CTRL_1_START);
+	if (ret < 0)
+		goto an8811hb_load_firmware_out;
+
+	ret = air_write_buf(phydev, AIR_FW_ADDR_DM,  fw1);
+	if (ret < 0)
+		goto an8811hb_load_firmware_out;
+
+	ret = an8811hb_check_crc(phydev, AN8811HB_CRC_DM_SET1,
+				 AN8811HB_CRC_DM_MON2,
+				 AN8811HB_CRC_DM_MON3);
+	if (ret < 0)
+		goto an8811hb_load_firmware_out;
+
+	ret = air_write_buf(phydev, AIR_FW_ADDR_DSP, fw2);
+	if (ret < 0)
+		goto an8811hb_load_firmware_out;
+
+	ret = an8811hb_check_crc(phydev, AN8811HB_CRC_PM_SET1,
+				 AN8811HB_CRC_PM_MON2,
+				 AN8811HB_CRC_PM_MON3);
+	if (ret < 0)
+		goto an8811hb_load_firmware_out;
+
+	ret = en8811h_wait_mcu_ready(phydev);
+	if (ret < 0)
+		goto an8811hb_load_firmware_out;

TBH the gotos are a bit hard to read, maybe factor out the logic that
can fail to a separate function? Then we can just capture ret here and
fall thru; and the helper itself can return ret; directly.

Yes, this was almost unreadable.

But fixing it turned out harder than I thought. There are so many steps
and we want to skip everything after the first failure.  Could be just
me being daft.  Probably is

Anyway, I have made an attempt.  At least got rid of the gotos.  Will
send a new version after some testing.


Bjørn