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
