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 at ffwll.ch
Mobile: +41 (0)79 365 57 48
