[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20231124163054.GQ50352@kernel.org>
Date: Fri, 24 Nov 2023 16:30:54 +0000
From: Simon Horman <horms@...nel.org>
To: Kory Maincent <kory.maincent@...tlin.com>
Cc: "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Jonathan Corbet <corbet@....net>,
Luis Chamberlain <mcgrof@...nel.org>,
Russ Weight <russ.weight@...ux.dev>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH net-next 9/9] net: pse-pd: Add PD692x0 PSE controller
driver
On Thu, Nov 16, 2023 at 03:01:41PM +0100, Kory Maincent wrote:
> Add a new driver for the PD692x0 I2C Power Sourcing Equipment controller.
> This driver only support i2c communication for now.
>
> Signed-off-by: Kory Maincent <kory.maincent@...tlin.com>
Hi Kory,
some minor feedback from my side.
...
> diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
...
> +static int
> +pd692x0_get_of_matrix(struct device *dev,
> + struct matrix port_matrix[PD692X0_MAX_LOGICAL_PORTS])
> +{
> + int ret, i, ports_matrix_size;
> + u32 val[PD692X0_MAX_LOGICAL_PORTS * 3];
nit: reverse xmas tree please.
...
> +static int pd692x0_fw_write_line(const struct i2c_client *client,
> + const char line[PD692X0_FW_LINE_MAX_SZ],
> + const bool last_line)
> +{
> + int ret;
> +
> + while (*line != 0) {
> + ret = i2c_master_send(client, line, 1);
> + if (ret < 0)
> + return FW_UPLOAD_ERR_RW_ERROR;
> + line++;
> + }
> +
> + if (last_line) {
> + ret = pd692x0_fw_recv_resp(client, 100, "TP\r\n",
> + sizeof("TP\r\n") - 1);
> + if (ret)
> + return ret;
> + } else {
> + ret = pd692x0_fw_recv_resp(client, 100, "T*\r\n",
> + sizeof("T*\r\n") - 1);
> + if (ret)
> + return ret;
> + }
> +
> + return FW_UPLOAD_ERR_NONE;
> +}
...
> +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;
> + } else {
> + ret = pd692x0_fw_write_line(client, line, false);
> + if (ret)
> + goto err;
> + }
> +
> + if (priv->cancel_request) {
> + ret = FW_UPLOAD_ERR_CANCELED;
> + goto err;
> + }
> + }
> + *written = i;
> +
> + msleep(400);
> +
> + return FW_UPLOAD_ERR_NONE;
> +
> +err:
> + pd692x0_fw_write_line(client, "S7\r\n", true);
gcc-13 (W=1) seems a bit upset about this.
drivers/net/pse-pd/pd692x0.c: In function 'pd692x0_fw_write':
drivers/net/pse-pd/pd692x0.c:861:9: warning: 'pd692x0_fw_write_line' reading 128 bytes from a region of size 5 [-Wstringop-overread]
861 | pd692x0_fw_write_line(client, "S7\r\n", true);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
drivers/net/pse-pd/pd692x0.c:861:9: note: referencing argument 2 of type 'const char[128]'
drivers/net/pse-pd/pd692x0.c:642:12: note: in a call to function 'pd692x0_fw_write_line'
642 | static int pd692x0_fw_write_line(const struct i2c_client *client,
| ^~~~~~~~~~~~~~~~~~~~~
> + return ret;
> +}
...
Powered by blists - more mailing lists