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

[Viresh Kumar]

On 05-07-21, 14:21, Jie Deng wrote:
> 
> On 2021/7/5 10:43, Viresh Kumar wrote:
> > On 02-07-21, 12:58, Andy Shevchenko wrote:
> > > On Fri, Jul 02, 2021 at 04:46:47PM +0800, Jie Deng wrote:
> > > > +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.
 
> > > > +	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.
> > 
> > > > +		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.
> > > 
> > > > +	}
> > > > +	return (fail ? -ETIMEDOUT : j);
> > > Redundant parentheses.
> > > 
> > > > +}

-- 
viresh