Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 7 Dec 2022 22:30:09 +0000
From:   Prashant Malani <pmalani@chromium.org>
To:     Mark Hasemeyer <markhas@chromium.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Raul Rangel <rrangel@chromium.org>,
        Bhanu Prakash Maiya <bhanumaiya@chromium.org>,
        Benson Leung <bleung@chromium.org>,
        Guenter Roeck <groeck@chromium.org>,
        Tzung-Bi Shih <tzungbi@kernel.org>,
        chrome-platform@lists.linux.dev
Subject: Re: [PATCH v10 1/3] platform/chrome: cros_ec_uart: Add cros-ec-uart
 transport layer
Message-ID: <Y5ET8a3WBntlOfU5@chromium.org>
References: <20221207104005.v10.1.If7926fcbad397bc6990dd725690229bed403948c@changeid>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221207104005.v10.1.If7926fcbad397bc6990dd725690229bed403948c@changeid>
Precedence: bulk

Hi Mark & Bhanu,

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

Reviewed-by: Prashant Malani <pmalani@chromium.org>

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