Re: [PATCH v10 1/3] platform/chrome: cros_ec_uart: Add cros-ec-uart transport layer

From: Prashant Malani
Date: Wed Dec 07 2022 - 17:30:17 EST


Hi Mark & Bhanu,

Mostly style nits; once addressed, please add my reviewed tag (FWIW):

Reviewed-by: Prashant Malani <pmalani@xxxxxxxxxxxx>

On Dec 07 10:40, Mark Hasemeyer wrote:
> +
> +/**
> + * struct response_info - Encapsulate EC response related
> + * information for passing between function
> + * cros_ec_uart_pkt_xfer() and cros_ec_uart_rx_bytes()
> + * callback.
> + * @data: Copy the data received from EC here.
> + * @max_size: Max size allocated for the @data buffer. If the
> + * received data exceeds this value, we log an error.
> + * @size: Actual size of data received from EC. This is also
> + * used to accumulate byte count with response is received
> + * in dma chunks.
> + * @exp_len: Expected bytes of response from EC including header.
> + * @status: Re-init to 0 before sending a cmd. Updated to 1 when
> + * a response is successfully received, or a negative
> + * integer on failure.
nit: You mean error number specifically, right? Otherwise it can just be
any negative integer, and the caller shouldn't bother checking anything other than " < 0".
> +
> + memcpy(ec_uart->response.data + ec_uart->response.size, data, count);
> +
> + ec_uart->response.size += count;
> +
> + /*
> + * Read data_len if we received response header and if exp_len
> + * was not read before.
> + */
nit: Comment can fit on 1 line.

> + if (ec_uart->response.size >= sizeof(*response) &&
> + ec_uart->response.exp_len == 0) {
> + response = (struct ec_host_response *) ec_uart->response.data;
> + ec_uart->response.exp_len = response->data_len + sizeof(*response);
> + }
> +
> + /*
> + * If driver received response header and payload from EC,
> + * Wake up the wait queue.
> + */
nit: Comment can fit on 1 line.

> + /* Setup for incoming response */
> + ec_uart->response.data = ec_dev->din;
> + ec_uart->response.max_size = ec_dev->din_size;
> + ec_uart->response.size = 0;
> + ec_uart->response.exp_len = 0;
> + ec_uart->response.status = 0;
> +
> + ret = serdev_device_write_buf(serdev, ec_dev->dout, len);
> + if (ret < len) {
> + dev_err(ec_dev->dev, "Unable to write data");
nit: Please end with a "\n".
> + ret = -EIO;
> + goto exit;
> + }
> +
> + ret = wait_event_timeout(ec_uart->response.wait_queue,
> + ec_uart->response.status,
> + msecs_to_jiffies(EC_MSG_DEADLINE_MS));
> + if (ret == 0) {
> + dev_warn(ec_dev->dev, "Timed out waiting for response.\n");
> + ret = -ETIMEDOUT;
> + goto exit;
> + }
> +
> + if (ec_uart->response.status < 0) {
> + dev_warn(ec_dev->dev, "Error response received: %d\n", ec_uart->response.status);
> + ret = ec_uart->response.status;
> + goto exit;
> + }
> +
> + response = (struct ec_host_response *)ec_dev->din;
> + ec_msg->result = response->result;
> +
> + if (response->data_len > ec_msg->insize) {
> + dev_err(ec_dev->dev, "Resp too long (%d bytes, expected %d)", response->data_len,
nit: Please end with a "\n".

Best regards,

-Prashant