Re: [PATCH] drm/i915: Split pin mapping into per platform functions

[Paulo Zanoni <paulo.r.zanoni@intel.com>]

Em Qui, 2017-08-10 às 11:42 -0700, Anusha Srivatsa escreveu:
> Cleanup the code. Map the pins in accordance to
> individual platforms rather than according to ports.
> Create separate functions for platforms.
> 
> v2:
>  - Add missing condition for CoffeeLake. Make platform
>  specific functions static. Add function
>  i915_ddc_pin_mapping().
> 
> Sugested-by Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Clinton Tayloe <clinton.a.taylor@intel.com>
> Signed-off-by: Anusha Srivatsa <anusha.srivatsa@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 115
> ++++++++++++++++++++++++++++++--------
>  1 file changed, 92 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> b/drivers/gpu/drm/i915/intel_hdmi.c
> index 2ef1ee85129d..41e6ba01ec1c 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -1843,49 +1843,118 @@ void
> intel_hdmi_handle_sink_scrambling(struct intel_encoder *encoder,
>  	DRM_DEBUG_KMS("sink scrambling handled\n");
>  }
>  
> -static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
> -			     enum port port)
> +static u8 chv_ddc_pin_mapping(struct drm_i915_private *dev_priv, 

Bikeshedding: what these functions do is pretty much map ports to DDC
pins, so I'd have named them x_port_to_ddc_pin() since that's the most
usual naming when converting "units", but I'm not opposed to the
current naming. Your choice.


> enum port port)
>  {
> -	const struct ddi_vbt_port_info *info =
> -		&dev_priv->vbt.ddi_port_info[port];
>  	u8 ddc_pin;
>  
> -	if (info->alternate_ddc_pin) {
> -		DRM_DEBUG_KMS("Using DDC pin 0x%x for port %c
> (VBT)\n",
> -			      info->alternate_ddc_pin,
> port_name(port));
> -		return info->alternate_ddc_pin;
> +	switch (port) {
> +

This is the only switch statement where you added a blank line at this
point. Doing this is not our usual coding style.


> +	case PORT_B:
> +		ddc_pin = GMBUS_PIN_DPB;
> +		break;
> +	case PORT_C:
> +		ddc_pin = GMBUS_PIN_DPC;
> +		break;
> +	case PORT_D:
> +		ddc_pin = GMBUS_PIN_DPD_CHV;
> +		break;
> +	default:
> +		MISSING_CASE(port);
> +		ddc_pin = GMBUS_PIN_DPB;
> +		break;
>  	}
> +	return ddc_pin;

Another bikeshedding which you don't need to implement: just returning
the pins inside the switch statements (return GMBUX_PIN_XYZ) without
the ddc_pin variable would have made the functions much shorter.


> +}
> +
> +static u8 bxt_ddc_pin_mapping(struct drm_i915_private *dev_priv,
> enum port port)
> +{
> +	u8 ddc_pin;
>  
>  	switch (port) {
>  	case PORT_B:
> -		if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv))
> -			ddc_pin = GMBUS_PIN_1_BXT;
> -		else
> -			ddc_pin = GMBUS_PIN_DPB;
> +		ddc_pin = GMBUS_PIN_1_BXT;
>  		break;
>  	case PORT_C:
> -		if (IS_GEN9_LP(dev_priv) || HAS_PCH_CNP(dev_priv))
> -			ddc_pin = GMBUS_PIN_2_BXT;
> -		else
> -			ddc_pin = GMBUS_PIN_DPC;
> +		ddc_pin = GMBUS_PIN_2_BXT;
> +		break;
> +	default:
> +		MISSING_CASE(port);
> +		ddc_pin = GMBUS_PIN_DPB;

Now that you've reorganized the code it's very easy to notice that on
bxt/cnp the default value doesn't even match what we return from port
B. I wouldn't complain if you addressed this issue in a follow-up
patch.


> +		break;
> +	}
> +	return ddc_pin;
> +}
> +
> +static u8 cnp_ddc_pin_mapping(struct drm_i915_private *dev_priv,
> +		enum port port)
> +{
> +	u8 ddc_pin;
> +
> +	switch (port) {
> +	case PORT_B:
> +		ddc_pin = GMBUS_PIN_1_BXT;
> +		break;
> +	case PORT_C:
> +		ddc_pin = GMBUS_PIN_2_BXT;
>  		break;
>  	case PORT_D:
> -		if (HAS_PCH_CNP(dev_priv))
> -			ddc_pin = GMBUS_PIN_4_CNP;
> -		else if (IS_CHERRYVIEW(dev_priv))
> -			ddc_pin = GMBUS_PIN_DPD_CHV;
> -		else
> -			ddc_pin = GMBUS_PIN_DPD;
> +		ddc_pin = GMBUS_PIN_4_CNP;
> +		break;
> +	default:
> +		MISSING_CASE(port);
> +		ddc_pin = GMBUS_PIN_DPB;
> +		break;
> +	}
> +	return ddc_pin;
> +}
> +
> +static u8 i915_ddc_pin_mapping(struct drm_i915_private *dev_priv,
> +		enum port port)

The first platform to run this is g4x, so please make the function name
start with g4x_


> +{
> +	u8 ddc_pin;
> +
> +	switch (port) {
> +	case PORT_B:
> +		ddc_pin = GMBUS_PIN_DPB;
> +		break;
> +	case PORT_C:
> +		ddc_pin = GMBUS_PIN_DPC;
> +		break;
> +	case PORT_D:
> +		ddc_pin = GMBUS_PIN_DPD;
>  		break;
>  	default:
>  		MISSING_CASE(port);
>  		ddc_pin = GMBUS_PIN_DPB;
>  		break;
>  	}
> +	return ddc_pin;
> +
> +}
> +static u8 intel_hdmi_ddc_pin(struct drm_i915_private *dev_priv,
> +			     enum port port)

Missing blank line between functions.


> +{
> +	const struct ddi_vbt_port_info *info =
> +		&dev_priv->vbt.ddi_port_info[port];
> +	u8 ddc_pin = 0;

In our coding style we usually don't zero-initialize things when they
don't need to be zero-initialized. The advantage of not zero-
initializing when we don't need is that if some rework happens and we
suddenly start forgetting to set a sane value, the compiler will tell
us.


> +
> +	if (info->alternate_ddc_pin) {
> +		DRM_DEBUG_KMS("Using DDC pin 0x%x for port %c
> (VBT)\n",
> +			      info->alternate_ddc_pin,
> port_name(port));
> +		return info->alternate_ddc_pin;
> +	}
> +
> +	if (IS_CHERRYVIEW(dev_priv))
> +		ddc_pin = chv_ddc_pin_mapping(dev_priv, port);
> +	else if (IS_GEN9_LP(dev_priv))
> +		ddc_pin = bxt_ddc_pin_mapping(dev_priv, port);
> +	else if (HAS_PCH_CNP(dev_priv))
> +		ddc_pin = cnp_ddc_pin_mapping(dev_priv, port);
> +	else
> +		ddc_pin = i915_ddc_pin_mapping(dev_priv, port);
 
>  	DRM_DEBUG_KMS("Using DDC pin 0x%x for port %c (platform
> default)\n",
>  		      ddc_pin, port_name(port));
> -

Please don't remove this line.


>  	return ddc_pin;
>  }
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx