Viresh Kumar
2021-Jul-05 02:43 UTC
[PATCH v12] i2c: virtio: add a virtio i2c frontend driver
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.> > + 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
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.Sure.
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.>>> + 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. >> >>> +}