[Intel-gfx] [PATCH] drm/i915/hotplug: Use phy to get the hpd_pin instead of the port (v3)

[Vivek Kasireddy]

On Fri, 31 Jan 2020 11:35:35 +0200
Jani Nikula <jani.nikula at intel.com> wrote:
Hi Jani,

> On Thu, 30 Jan 2020, Vivek Kasireddy <vivek.kasireddy at intel.com>
> wrote:
> > On some platforms such as Elkhart Lake, although we may use DDI D
> > to drive a connector, we have to use PHY A (Combo Phy PORT A) to
> > detect the hotplug interrupts as per the spec because there is no
> > one-to-one mapping between DDIs and PHYs. Therefore, use the
> > function intel_port_to_phy() which contains the logic for such
> > mapping(s) to find the correct hpd_pin.
> >
> > This change should not affect other platforms as there is always
> > a one-to-one mapping between DDIs and PHYs.
> >
> > v2:
> > - Convert the case statements to use PHYs instead of PORTs (Jani)
> >
> > v3:
> > - Refactor the function to reduce the number of return statements by
> >   lumping all the case statements together except PHY_F which needs
> >   special handling (Jose)
> >
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Cc: José Roberto de Souza <jose.souza at intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy at intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_hotplug.c | 37
> > ++++++++------------ 1 file changed, 15 insertions(+), 22
> > deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c
> > b/drivers/gpu/drm/i915/display/intel_hotplug.c index
> > 042d98bae1ea..27e3033278a0 100644 ---
> > a/drivers/gpu/drm/i915/display/intel_hotplug.c +++
> > b/drivers/gpu/drm/i915/display/intel_hotplug.c @@ -89,29 +89,22 @@
> >  enum hpd_pin intel_hpd_pin_default(struct drm_i915_private
> > *dev_priv, enum port port)
> >  {
> > -	switch (port) {
> > -	case PORT_A:
> > -		return HPD_PORT_A;
> > -	case PORT_B:
> > -		return HPD_PORT_B;
> > -	case PORT_C:
> > -		return HPD_PORT_C;
> > -	case PORT_D:
> > -		return HPD_PORT_D;
> > -	case PORT_E:
> > -		return HPD_PORT_E;
> > -	case PORT_F:
> > -		if (IS_CNL_WITH_PORT_F(dev_priv))
> > -			return HPD_PORT_E;
> > -		return HPD_PORT_F;
> > -	case PORT_G:
> > -		return HPD_PORT_G;
> > -	case PORT_H:
> > -		return HPD_PORT_H;
> > -	case PORT_I:
> > -		return HPD_PORT_I;
> > +	enum phy phy = intel_port_to_phy(dev_priv, port);
> > +
> > +	switch (phy) {
> > +	case PHY_F:
> > +		return IS_CNL_WITH_PORT_F(dev_priv) ? HPD_PORT_E :
> > HPD_PORT_F;
> > +	case PHY_A:
> > +	case PHY_B:
> > +	case PHY_C:
> > +	case PHY_D:
> > +	case PHY_E:
> > +	case PHY_G:
> > +	case PHY_H:
> > +	case PHY_I:
> > +		return HPD_PORT_A + phy;  
> 
> I know José asked you to do this, but now you've tied two enum
> sequences together without explaining it anywhere. Before this,
> AFAICT, enum hpd_pin was just an abstract enumeration where the
> actual values of the enums didn't mean a thing, apart from 0 for
> HPD_NONE.
> 
> Maybe this is what we want to do, but we should never be so casual
> about it.
Do you suggest that I explain this in the description associated
with v3 that we now have a switch/case fallthrough in this function?
Or, do you want me to send a v4 to include this in a comment?

Thanks,
Vivek

> 
> 
> BR,
> Jani.
> 
> 
> >  	default:
> > -		MISSING_CASE(port);
> > +		MISSING_CASE(phy);
> >  		return HPD_NONE;
> >  	}
> >  }  
>