Messages in this thread | | | Date | Mon, 13 Nov 2017 11:16:41 +0300 | From | Dan Carpenter <> | Subject | Re: [PATCH v4 3/5] staging: Introduce NVIDIA Tegra video decoder driver |
| |
On Sat, Nov 11, 2017 at 04:06:52PM +0200, Vladimir Zapolskiy wrote: > > + if (!wait_dma) > > + return 0; > > + > > + err = readl_relaxed_poll_timeout(vde->bsev + INTR_STATUS, value, > > + !(value & BSE_DMA_BUSY), 1, 100); > > + if (err) { > > + dev_err(dev, "BSEV DMA timeout\n"); > > + return err; > > + } > > + > > + return 0; > > if (err) > dev_err(dev, "BSEV DMA timeout\n"); > > return err; > > is two lines shorter. >
This is fine, but just watch out because getting clever with a last if statement is a common anti-pattern. For example, you often see it where people do success handling instead of failure handling. And it leads to static checker bugs, and makes the code slightly more subtle.
> > + err = tegra_vde_attach_dmabuf(dev, source->aux_fd, > > + source->aux_offset, csize, > > + &frame->aux_dmabuf_attachment, > > + &frame->aux_addr, > > + &frame->aux_sgt, > > + NULL, dma_dir); > > + if (err) > > + goto err_release_cr; > > + } > > + > > + return 0; > > if (!err) > return 0; > > and then remove a check above. >
Argh!!!! Success handling. Always do failure handling, never success handling.
The rest of your comments I agree with, though.
regards, dan carpenter
| |