[PATCH v4 14/22] drm: omapdrm: Keep vblank interrupt enabled while CRTC is active

[Tomi Valkeinen]

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.

 Tomi

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/a142350b/attachment.sig>