+struct pd692x0_priv {
+	struct i2c_client *client;
+	struct pse_controller_dev pcdev;
+
+	enum pd692x0_fw_state fw_state;
+	struct fw_upload *fwl;
+	bool cancel_request:1;
+
+	u8 msg_id;
+	bool last_cmd_key:1;

Does a bool bit field of size 1 make any sense?  I would also put the
two bitfields next to each other, and the compiler might then pack
them into the same word. The base type of a u8 would allow the compile
to put it next to the msg_id without any padding.

+	unsigned long last_cmd_key_time;
+
+	enum ethtool_pse_admin_state admin_state[PD692X0_MAX_LOGICAL_PORTS];
+};
+
+/* Template list of the fixed bytes of the communication messages */
+static const struct pd692x0_msg pd692x0_msg_template_list[PD692X0_MSG_CNT] = {
+	[PD692X0_MSG_RESET] = {
+		.content = {
+			.key = PD692X0_KEY_CMD,
+			.sub = {0x07, 0x55, 0x00},
+			.data = {0x55, 0x00, 0x55, 0x4e,
+				 0x4e, 0x4e, 0x4e, 0x4e},
+		},
+	},

Is there any documentation about what all these magic number mean?

+/* Implementation of the i2c communication in particular when there is
+ * a communication loss. See the "Synchronization During Communication Loss"
+ * paragraph of the Communication Protocol document.
+ */

Is this document public?

+static int pd692x0_recv_msg(struct pd692x0_priv *priv,
+			    struct pd692x0_msg *msg,
+			    struct pd692x0_msg_content *buf)
+{
+	const struct i2c_client *client = priv->client;
+	int ret;
+
+	memset(buf, 0, sizeof(*buf));
+	if (msg->delay_recv)
+		msleep(msg->delay_recv);
+	else
+		msleep(30);
+
+	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
+	if (buf->key)
+		goto out;

This is the first attempt to receive the message. I assume buf->key
not being 0 indicates something has been received?

+
+	msleep(100);
+
+	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
+	if (buf->key)
+		goto out;

So this is a second attempt. Should there be another memset? Could the
first failed transfer fill the buffer with random junk in the higher
bytes, and a successful read here could be a partial read and the end
of the buffer still contains the junk.

+
+	ret = pd692x0_send_msg(priv, msg);
+	if (ret)
+		return ret;

So now we are re-transmitting the request.

+
+	if (msg->delay_recv)
+		msleep(msg->delay_recv);
+	else
+		msleep(30);
+
+	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
+	if (buf->key)
+		goto out;
+
+	msleep(100);
+
+	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
+	if (buf->key)
+		goto out;
+
+	msleep(10000);

And two more attemps to receive it.

+
+	ret = pd692x0_send_msg(priv, msg);
+	if (ret)
+		return ret;
+
+	if (msg->delay_recv)
+		msleep(msg->delay_recv);
+	else
+		msleep(30);
+
+	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
+	if (buf->key)
+		goto out;
+
+	msleep(100);
+
+	i2c_master_recv(client, (u8 *)buf, sizeof(*buf));
+	if (buf->key)
+		goto out;

Another resend and two more attempts to receive.

Is there a reason to not uses for loops here? And maybe put
send/receive/receive into a helper? And maybe make the first send part
of this, rather then separate? I think the code will be more readable
when restructured.

+static int pd692x0_ethtool_set_config(struct pse_controller_dev *pcdev,
+				      unsigned long id,
+				      struct netlink_ext_ack *extack,
+				      const struct pse_control_config *config)
+{
+	struct pd692x0_priv *priv = to_pd692x0_priv(pcdev);
+	struct pd692x0_msg_content buf = {0};
+	struct pd692x0_msg msg;
+	int ret;
+
+	ret = pd692x0_fw_unavailable(priv);
+	if (ret)
+		return ret;

It seems a bit late to check if the device has any firmware. I would
of expected probe to check that, and maybe attempt to download
firmware. If that fails, fail the probe, since the PSE is a brick.

+static struct pd692x0_msg_ver pd692x0_get_sw_version(struct pd692x0_priv *priv)
+{
+	struct pd692x0_msg msg = pd692x0_msg_template_list[PD692X0_MSG_GET_SW_VER];
+	struct device *dev = &priv->client->dev;
+	struct pd692x0_msg_content buf = {0};
+	struct pd692x0_msg_ver ver = {0};
+	int ret;
+
+	ret = pd692x0_sendrecv_msg(priv, &msg, &buf);
+	if (ret < 0) {
+		dev_err(dev, "Failed to get PSE version (%pe)\n", ERR_PTR(ret));
+		return ver;

I _think_ that return is wrong ???

+static enum fw_upload_err pd692x0_fw_write(struct fw_upload *fwl,
+					   const u8 *data, u32 offset,
+					   u32 size, u32 *written)
+{
+	struct pd692x0_priv *priv = fwl->dd_handle;
+	char line[PD692X0_FW_LINE_MAX_SZ];
+	const struct i2c_client *client;
+	int ret, i;
+	char cmd;
+
+	client = priv->client;
+	priv->fw_state = PD692X0_FW_WRITE;
+
+	/* Erase */
+	cmd = 'E';
+	ret = i2c_master_send(client, &cmd, 1);
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to boot programming mode (%pe)\n",
+			ERR_PTR(ret));
+		return FW_UPLOAD_ERR_RW_ERROR;
+	}
+
+	ret = pd692x0_fw_recv_resp(client, 100, "TOE\r\n", sizeof("TOE\r\n") - 1);
+	if (ret)
+		return ret;
+
+	ret = pd692x0_fw_recv_resp(client, 5000, "TE\r\n", sizeof("TE\r\n") - 1);
+	if (ret)
+		dev_warn(&client->dev,
+			 "Failed to erase internal memory, however still try to write Firmware\n");
+
+	ret = pd692x0_fw_recv_resp(client, 100, "TPE\r\n", sizeof("TPE\r\n") - 1);
+	if (ret)
+		dev_warn(&client->dev,
+			 "Failed to erase internal memory, however still try to write Firmware\n");
+
+	if (priv->cancel_request)
+		return FW_UPLOAD_ERR_CANCELED;
+
+	/* Program */
+	cmd = 'P';
+	ret = i2c_master_send(client, &cmd, sizeof(char));
+	if (ret < 0) {
+		dev_err(&client->dev,
+			"Failed to boot programming mode (%pe)\n",
+			ERR_PTR(ret));
+		return ret;
+	}
+
+	ret = pd692x0_fw_recv_resp(client, 100, "TOP\r\n", sizeof("TOP\r\n") - 1);
+	if (ret)
+		return ret;
+
+	i = 0;
+	while (i < size) {
+		ret = pd692x0_fw_get_next_line(data, line, size - i);
+		if (!ret) {
+			ret = FW_UPLOAD_ERR_FW_INVALID;
+			goto err;
+		}
+
+		i += ret;
+		data += ret;
+		if (line[0] == 'S' && line[1] == '0') {
+			continue;
+		} else if (line[0] == 'S' && line[1] == '7') {
+			ret = pd692x0_fw_write_line(client, line, true);
+			if (ret)
+				goto err;

Is the firmware in Motorola SREC format? I thought the kernel had a
helper for that, but a quick search did not find it. So maybe i'm
remembering wrongly. But it seems silly for every driver to implement
an SREC parser.

   Andrew