On Mon, Jan 16, 2012 at 06:29:36PM -0200, Paulo Zanoni wrote:
Three comments about the design are inline:
+void drm_crtc_attach_property(struct drm_crtc *crtc,
- struct drm_property *property, uint64_t init_val)
+{
- int i;
- for (i = 0; i < DRM_CRTC_MAX_PROPERTY; i++) {
- if (crtc->property_ids[i] == 0) {
- crtc->property_ids[i] = property->base.id;
- crtc->property_values[i] = init_val;
- return;
- }
- }
- BUG_ON(1);
I looked at drm_connector_attach_property and saw that instead of BUG_ON(1), it tries to return -EINVAL. The problem is that only zero callers check for the return value of drm_connector_attach_property. I can provide a patch for drm_connector_attach_property changing the -EINVAL for BUG_ON(1) if no one objects. Or I can also add -EINVAL to drm_crtc_attach_property and, to be consistent, not check for it :)
Just a quick comment: WARN is generally highly preferred over BUG. Use the latter only when you know that the kernel _will_ go down in a horribly way and it's better to stop it doing so (e.g. for NULL pointer checks). -Daniel