From mboxrd@z Thu Jan  1 00:00:00 1970
From: Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH 2/3] drm: add CRTC properties
Date: Tue, 17 Jan 2012 11:27:23 +0100
Message-ID: <20120117102723.GD4093@phenom.ffwll.local>
References: <A+gsUGSBzJtaBhnAxPfdV4uBSw01VvAeTYSeCOtgr+UVfy0GjQ@mail.gmail.com>
	<1326744167-3615-1-git-send-email-przanoni@gmail.com>
	<1326744167-3615-2-git-send-email-przanoni@gmail.com>
	<CA+gsUGShCbtGg4-4ScPcRxP30XjpPie8PP0_yd31LHLmnD3ANw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="iso-8859-1"
Content-Transfer-Encoding: quoted-printable
Return-path: <dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org>
Received: from mail-we0-f177.google.com (mail-we0-f177.google.com
	[74.125.82.177])
	by gabe.freedesktop.org (Postfix) with ESMTP id 58BC89F4D7
	for <dri-devel@lists.freedesktop.org>;
	Tue, 17 Jan 2012 02:27:27 -0800 (PST)
Received: by werm13 with SMTP id m13so1932842wer.36
	for <dri-devel@lists.freedesktop.org>;
	Tue, 17 Jan 2012 02:27:26 -0800 (PST)
Content-Disposition: inline
In-Reply-To: <CA+gsUGShCbtGg4-4ScPcRxP30XjpPie8PP0_yd31LHLmnD3ANw@mail.gmail.com>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/dri-devel>,
	<mailto:dri-devel-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/dri-devel>
List-Post: <mailto:dri-devel@lists.freedesktop.org>
List-Help: <mailto:dri-devel-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/dri-devel>,
	<mailto:dri-devel-request@lists.freedesktop.org?subject=subscribe>
Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org
Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org
To: Paulo Zanoni <przanoni@gmail.com>
Cc: dri-devel@lists.freedesktop.org
List-Id: dri-devel@lists.freedesktop.org

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,
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct drm_pr=
operty *property, uint64_t init_val)
> > +{
> > + =A0 =A0 =A0 int i;
> > +
> > + =A0 =A0 =A0 for (i =3D 0; i < DRM_CRTC_MAX_PROPERTY; i++) {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (crtc->property_ids[i] =3D=3D 0) {
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 crtc->property_ids[i] =3D=
 property->base.id;
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 crtc->property_values[i] =
=3D init_val;
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return;
> > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> > + =A0 =A0 =A0 }
> > + =A0 =A0 =A0 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