Re: [PATCH 2/2] drm/msm: Add the eDP connector in msm drm driver

[Thierry Reding]

On Fri, Dec 05, 2014 at 04:30:01PM -0500, Hai Li wrote:
> Modified the hard-coded hdmi connector/encoder implementations in msm drm
> driver to support both edp and hdmi.

s/hdmi/HDMI/, s/msm/MSM/, s/drm/DRM/, s/edp/eDP/

[...]
> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_encoder.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_encoder.c
[...]
> @@ -177,7 +180,28 @@ static void mdp5_encoder_mode_set(struct drm_encoder *encoder,
>  	/* probably need to get DATA_EN polarity from panel.. */
>  
>  	dtv_hsync_skew = 0;  /* get this from panel? */
> -	format = 0x213f;     /* get this from panel? */
> +
> +	/* Get color format from panel, default is 8bpc */
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +		if (connector->encoder == encoder) {
> +			switch (connector->display_info.bpc) {
> +			case 4:
> +				format |= 0;
> +				break;
> +			case 5:
> +				format |= 0x15;
> +				break;
> +			case 6:
> +				format |= 0x2A;
> +				break;
> +			case 8:
> +			default:
> +				format |= 0x3F;
> +				break;
> +			}
> +			break;

I suppose that it might be obvious from the above switch statement, but
having symbolic names for the values of format might still be good for
readability.

> +		}
> +	}
>  
>  	hsync_start_x = (mode->htotal - mode->hsync_start);
>  	hsync_end_x = mode->htotal - (mode->hsync_start - mode->hdisplay) - 1;
> @@ -187,6 +211,16 @@ static void mdp5_encoder_mode_set(struct drm_encoder *encoder,
>  	display_v_start = (mode->vtotal - mode->vsync_start) * mode->htotal + dtv_hsync_skew;
>  	display_v_end = vsync_period - ((mode->vsync_start - mode->vdisplay) * mode->htotal) + dtv_hsync_skew - 1;
>  
> +	/*
> +	 * For edp only:
> +	 * DISPLAY_V_START = (VBP * HCYCLE) + HBP
> +	 * DISPLAY_V_END = (VBP + VACTIVE) * HCYCLE - 1 - HFP
> +	 */
> +	if (mdp5_encoder->intf_id == INTF_eDP) {
> +		display_v_start += (mode->htotal - mode->hsync_start);
> +		display_v_end -= (mode->hsync_start - mode->hdisplay);

No need for the parentheses.

> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.c
[...]
>  	struct drm_device *dev = mdp5_kms->dev;
>  	struct msm_drm_private *priv = dev->dev_private;
> -	struct drm_encoder *encoder;
> +	struct drm_encoder *edp_encoder = NULL, *hdmi_encoder = NULL;

Can't you simply reuse encoder? It's scope is limited to the
conditionals, so no need to have two separate variables.

Thierry

Attachment: pgphkc3_Gh8Xv.pgp
Description: PGP signature