From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 1/2] drm/i915: implement hsw_write_infoframe Date: Sun, 13 May 2012 15:20:49 +0200 Message-ID: <20120513132049.GA4720@phenom.ffwll.local> References: <1336765699-11996-1-git-send-email-przanoni@gmail.com> <4FAD984D.3000609@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-wi0-f177.google.com (mail-wi0-f177.google.com [209.85.212.177]) by gabe.freedesktop.org (Postfix) with ESMTP id 5A2279E77C for ; Sun, 13 May 2012 06:19:43 -0700 (PDT) Received: by wibhm14 with SMTP id hm14so2138411wib.12 for ; Sun, 13 May 2012 06:19:42 -0700 (PDT) Content-Disposition: inline In-Reply-To: <4FAD984D.3000609@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: eugeni.dodonov@intel.com Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Fri, May 11, 2012 at 07:53:01PM -0300, Eugeni Dodonov wrote: > On 05/11/2012 04:48 PM, Paulo Zanoni wrote: > >From: Paulo Zanoni > > > >Both the control and data registers are completely different now. > > > >Signed-off-by: Paulo Zanoni > > Haswell for the win! :) > > Just some comments below, but nothing too critical. I kinda agree with Eugeni's bikesheds - fixing up style issues in the new code right away sounds better. -Daniel > > >+static u32 hsw_infoframe_enable(struct dip_infoframe *frame) > >+{ > >+ u32 flags = 0; > >+ > >+ switch (frame->type) { > >+ case DIP_TYPE_AVI: > >+ flags |= VIDEO_DIP_ENABLE_AVI_HSW; > >+ break; > >+ case DIP_TYPE_SPD: > >+ flags |= VIDEO_DIP_ENABLE_SPD_HSW; > >+ break; > >+ default: > >+ DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); > >+ break; > >+ } > >+ > >+ return flags; > > I think it makes sense to inverse order of patches, and simplify the > _infoframe stuff you do in your 2nd patch first; and then add this > part with new style directly. > > And I think it would be worth adding a comment with TODO saying that > we still need to support other frames (GCP, VSC and so on). We don't > have any use for them right now, but in the future we might. And we > have all the registers defined already anyway. > > >+} > >+ > >+static u32 hsw_infoframe_data_reg(struct dip_infoframe *frame, enum pipe pipe) > >+{ > >+ u32 reg = 0; > >+ > >+ switch (frame->type) { > >+ case DIP_TYPE_AVI: > >+ reg = HSW_TVIDEO_DIP_AVI_DATA(pipe); > >+ break; > >+ case DIP_TYPE_SPD: > >+ reg = HSW_TVIDEO_DIP_SPD_DATA(pipe); > >+ break; > >+ default: > >+ DRM_DEBUG_DRIVER("unknown info frame type %d\n", frame->type); > >+ break; > >+ } > >+ > >+ return reg; > >+} > > I think you could simplify it here as well, similarly to what you do > in your 2nd patch, and return the register directly. > > > static void hsw_write_infoframe(struct drm_encoder *encoder, > >- struct dip_infoframe *frame) > >+ struct dip_infoframe *frame) > > I think this should go into the other patch (which simplifies > tabbing and such), no? > > But other than that, very nice! > > Reviewed-by: Eugeni Dodonov > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Mail: daniel@ffwll.ch Mobile: +41 (0)79 365 57 48