[Intel-gfx] [PATCH 1/2] drm/i915: implement hsw_write_infoframe

[Daniel Vetter]

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<paulo.r.zanoni at intel.com>
> >
> >Both the control and data registers are completely different now.
> >
> >Signed-off-by: Paulo Zanoni<paulo.r.zanoni at intel.com>
> 
> 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 <eugeni.dodonov at intel.com>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel at ffwll.ch
Mobile: +41 (0)79 365 57 48