Re: [PATCH 2/3] drm: add CRTC properties

[Daniel Vetter <daniel@ffwll.ch>]

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
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48