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

[Prashant Malani]

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