[PATCH v4 14/22] drm: omapdrm: Keep vblank interrupt enabled while CRTC is active
tomi.valkeinen at ti.com
Thu Dec 15 14:56:15 UTC 2016
On 15/12/16 16:51, Laurent Pinchart wrote:
>>> + WARN_ON(drm_crtc_vblank_get(crtc) != 0);
>> I don't like this style very much. I think WARN()s should be without
>> side effects.
>> Also, WARN only gives benefit when we don't know what the call stack is.
>> Afaik, there's only one way omap_crtc_atomic_flush can be called, so
>> it's just extra spam and dev_err or dev_warn should be enough.
> I've used it because the equivalent statements testing omap_crtc-
>> vblank_irq.registered used WARN_ON() too. WARN_ON() is also a bit more vocal,
> it really gets the point across. As the function really should not return an
> error unless in case of a driver bug, I don't think it will generate any spam.
> I don't care too much though, I can replace it with a dev_err() if you insist.
I don't mind using WARN_ON() that much. But that's the comment I've
received a few times, so I shared it =).
What I do care is the side effect inside WARN_ON. At least for me it's
quite easy to miss that it's actually having a side effect, as I expect
WARN_ON() to be just a check, like assert(). So when reading the code, I
skip the WARN_ONs.
-------------- next part --------------
A non-text attachment was scrubbed...
Size: 819 bytes
Desc: OpenPGP digital signature
More information about the dri-devel