> +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
