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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
