Re: [PATCH v12] i2c: virtio: add a virtio i2c frontend driver

On 05-07-21, 14:21, Jie Deng wrote:

On 2021/7/5 10:43, Viresh Kumar wrote:

quoted

On 02-07-21, 12:58, Andy Shevchenko wrote:

quoted

On Fri, Jul 02, 2021 at 04:46:47PM +0800, Jie Deng wrote:

quoted

+static int virtio_i2c_complete_reqs(struct virtqueue *vq,
+				    struct virtio_i2c_req *reqs,
+				    struct i2c_msg *msgs, int nr,
+				    bool fail)
+{
+	struct virtio_i2c_req *req;
+	bool failed = fail;

Jie, you can actually get rid of this variable too. Jut rename fail to failed
and everything shall work as you want.

Oh, You are not right. I just found we can't remove this variable. The
"fail" and "failed" have different

meanings for this function. We need fail to return the result.

Ahh, yes. You are right. Maybe rename fail to timedout, it would make it more
readable, else fail and failed look very similar.
 

quoted

quoted

quoted

+	unsigned int len;
+	int i, j = 0;
+
+	for (i = 0; i < nr; i++) {
+		/* Detach the ith request from the vq */
+		req = virtqueue_get_buf(vq, &len);
+
+		/*
+		 * Condition (req && req == &reqs[i]) should always meet since
+		 * we have total nr requests in the vq.
+		 */
+		if (!failed && (WARN_ON(!(req && req == &reqs[i])) ||
+		    (req->in_hdr.status != VIRTIO_I2C_MSG_OK)))
+			failed = true;

...and after failed is true, we are continuing the loop, why?

Actually this function can be called with fail set to true. We proceed as we
need to call i2c_put_dma_safe_msg_buf() for all buffers we allocated earlier.

quoted

quoted

+		i2c_put_dma_safe_msg_buf(reqs[i].buf, &msgs[i], !failed);
+		if (!failed)
+			++j;

Besides better to read j++ the j itself can be renamed to something more
verbose.

quoted

+	}
+	return (fail ? -ETIMEDOUT : j);

Redundant parentheses.

quoted

+}

-- 
viresh
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization