From mboxrd@z Thu Jan  1 00:00:00 1970
From: Eugeni Dodonov <eugeni.dodonov@linux.intel.com>
Subject: Re: [PATCH 1/2] drm/i915: implement hsw_write_infoframe
Date: Fri, 11 May 2012 19:53:01 -0300
Message-ID: <4FAD984D.3000609@linux.intel.com>
References: <1336765699-11996-1-git-send-email-przanoni@gmail.com>
Reply-To: eugeni.dodonov@intel.com
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org>
Received: from mga02.intel.com (mga02.intel.com [134.134.136.20])
	by gabe.freedesktop.org (Postfix) with ESMTP id 21CABA0CAC
	for <intel-gfx@lists.freedesktop.org>;
	Fri, 11 May 2012 15:52:30 -0700 (PDT)
In-Reply-To: <1336765699-11996-1-git-send-email-przanoni@gmail.com>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=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: Paulo Zanoni <przanoni@gmail.com>
Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni <paulo.r.zanoni@intel.com>
List-Id: intel-gfx@lists.freedesktop.org

On 05/11/2012 04:48 PM, Paulo Zanoni wrote:
> From: Paulo Zanoni<paulo.r.zanoni@intel.com>
>
> Both the control and data registers are completely different now.
>
> Signed-off-by: Paulo Zanoni<paulo.r.zanoni@intel.com>

Haswell for the win! :)

Just some comments below, but nothing too critical.

> +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@intel.com>