On Fri, 31 Jan 2020 11:35:35 +0200
Jani Nikula <jani.nik...@intel.com> wrote:
Hi Jani,
> On Thu, 30 Jan 2020, Vivek Kasireddy <vivek.kasire...@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.nik...@intel.com>
> > Cc: Matt Roper <matthew.d.ro...@intel.com>
> > Cc: José Roberto de Souza <jose.so...@intel.com>
> > Signed-off-by: Vivek Kasireddy <vivek.kasire...@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;
> > }
> > }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
