| Message ID | 20200131001816.32741-1-vivek.kasireddy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/hotplug: Use phy to get the hpd_pin instead of the port (v3) | expand |
On Thu, 2020-01-30 at 16:18 -0800, Vivek Kasireddy 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) Thanks Reviewed-by: José Roberto de Souza <jose.souza@intel.com> > > Cc: Jani Nikula <jani.nikula@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@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; > default: > - MISSING_CASE(port); > + MISSING_CASE(phy); > return HPD_NONE; > } > }
On Thu, 30 Jan 2020, Vivek Kasireddy <vivek.kasireddy@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@intel.com> > Cc: Matt Roper <matthew.d.roper@intel.com> > Cc: José Roberto de Souza <jose.souza@intel.com> > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@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. BR, Jani. > default: > - MISSING_CASE(port); > + MISSING_CASE(phy); > return HPD_NONE; > } > }
On Fri, 31 Jan 2020 11:35:35 +0200 Jani Nikula <jani.nikula@intel.com> wrote: Hi Jani, > On Thu, 30 Jan 2020, Vivek Kasireddy <vivek.kasireddy@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@intel.com> > > Cc: Matt Roper <matthew.d.roper@intel.com> > > Cc: José Roberto de Souza <jose.souza@intel.com> > > Signed-off-by: Vivek Kasireddy <vivek.kasireddy@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; > > } > > } >
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; default: - MISSING_CASE(port); + MISSING_CASE(phy); return HPD_NONE; } }
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@intel.com> Cc: Matt Roper <matthew.d.roper@intel.com> Cc: José Roberto de Souza <jose.souza@intel.com> Signed-off-by: Vivek Kasireddy <vivek.kasireddy@intel.com> --- drivers/gpu/drm/i915/display/intel_hotplug.c | 37 ++++++++------------ 1 file changed, 15 insertions(+), 22 deletions(-)