On Thu, Aug 28, 2025 at 03:09:51PM +0200, Andrew Lunn wrote:

quoted

+/**
+ * mucse_mbx_get_capability - Get hw abilities from fw
+ * @hw: pointer to the HW structure
+ *
+ * mucse_mbx_get_capability tries to get capabities from
+ * hw. Many retrys will do if it is failed.
+ *
+ * Return: 0 on success, negative errno on failure
+ **/
+int mucse_mbx_get_capability(struct mucse_hw *hw)
+{
+	struct hw_abilities ability = {};
+	int try_cnt = 3;
+	int err;
+	/* It is called once in probe, if failed nothing
+	 * (register network) todo. Try more times to get driver
+	 * and firmware in sync.
+	 */
+	do {
+		err = mucse_fw_get_capability(hw, &ability);
+		if (err)
+			continue;
+		break;
+	} while (try_cnt--);
+
+	if (!err)
+		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
+	return err;
+}

I still think this should be a dedicated function to get the MAC
driver and firmware in sync, using a NOP or version request to the
firmware. The name mucse_mbx_get_capability() does not indicate this
function is special in any way, which is it.

Maybe I should rename it like this?

/**
 * mucse_mbx_sync_fw_by_get_capability - Try to sync driver and fw
 * @hw: pointer to the HW structure
 *
 * mucse_mbx_sync_fw_by_get_capability tries to sync driver and fw
 * by get capabitiy mbx cmd. Many retrys will do if it is failed.
 *
 * Return: 0 on success, negative errno on failure
 **/
int mucse_mbx_sync_fw_by_get_capability(struct mucse_hw *hw)
{
	struct hw_abilities ability = {};
	int try_cnt = 3;
	int err;
	/* It is called once in probe, if failed nothing
	 * (register network) todo. Try more times to get driver
	 * and firmware in sync.
	 */
	do {
		err = mucse_fw_get_capability(hw, &ability);
		if (err)
			continue;
		break;
	} while (try_cnt--);

	if (!err)
		hw->pfvfnum = le16_to_cpu(ability.pfnum) & GENMASK_U16(7, 0);
	return err;
}

quoted

+/**
+ * build_ifinsmod - build req with insmod opcode
+ * @req: pointer to the cmd req structure
+ * @is_insmod: true for insmod, false for rmmod
+ **/
+static void build_ifinsmod(struct mbx_fw_cmd_req *req,
+			   bool is_insmod)
+{
+	req->flags = 0;
+	req->opcode = cpu_to_le16(DRIVER_INSMOD);
+	req->datalen = cpu_to_le16(sizeof(req->ifinsmod) +
+				   MBX_REQ_HDR_LEN);
+	req->reply_lo = 0;
+	req->reply_hi = 0;
+#define FIXED_VERSION 0xFFFFFFFF
+	req->ifinsmod.version = cpu_to_le32(FIXED_VERSION);
+	if (is_insmod)
+		req->ifinsmod.status = cpu_to_le32(1);
+	else
+		req->ifinsmod.status = cpu_to_le32(0);
+}

Why does the firmware care? What does the firmware do when there is no
kernel driver? How does it behaviour change when the driver loads?

fw reduce working frequency to save power if no driver is probed to this
chip. And fw change frequency to normal after recieve insmod mbx cmd.

Maybe I should add the comment to func "mucse_mbx_ifinsmod"? 
/**
 * mucse_mbx_ifinsmod - Echo driver insmod status to fw
 * @hw: pointer to the HW structure
 * @is_insmod: true for insmod, false for rmmod
 *
 * mucse_mbx_ifinsmod echo driver insmod status to fw. fw changes working
 * frequency to normal after recieve insmod status, and reduce working
 * frequency if no driver is probed.
 *
 * Return: 0 on success, negative errno on failure
 **/
int mucse_mbx_ifinsmod(struct mucse_hw *hw, bool is_insmod)
{


}

Please try to ensure comment say why you are doing something, not what
you are doing.


    Andrew

---
pw-bot: cr

Thanks for your feedback.