Re: [v6,4/4] drm/i915/display: remove unnecessary PHY_NONE definition

On Wed, 2026-07-01 at 20:36 +0200, Michał Grzelak wrote:
> On Tue, 9 Jun 2026, Luca Coelho wrote:
> > PHY_NONE is not really used, but we define it and, thus, need to check
> > for it in a few places we use phy.  The only potential places where
> 
> double space: s/  / /
> 
> > phy may become PHY_NONE, is in intel_port_to_phy(), where it derives
> > from port, which can be PORT_NONE.  Many of its callers don't check
> 
> double space: s/  / /
> 
> > for PHY_NONE, which can cause unknown behavior.  Additionally, this
> 
> double space: s/  / /
> 
> > can only happen if the encoder used has PORT_NONE, which should not be
> > the case either, without unexpected consequences.
> > 
> > Remove the PHY_NONE definition entirely and add a couple of WARNs at
> > the relevant places, just to be sure.
> > 
> > Signed-off-by: Luca Coelho <[email protected]>
> > ---
> > drivers/gpu/drm/i915/display/intel_display.c           | 10 ++++++----
> > drivers/gpu/drm/i915/display/intel_display.h           |  2 --
> > .../gpu/drm/i915/display/intel_display_power_well.c    |  6 +++++-
> > drivers/gpu/drm/i915/display/intel_hti.c               |  3 ---
> > 4 files changed, 11 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > index bdf02b67c1d8..58713ad04d37 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -1810,9 +1810,7 @@ static void hsw_crtc_disable(struct intel_atomic_state *state,
> > /* Prefer intel_encoder_is_combo() */
> > bool intel_phy_is_combo(struct intel_display *display, enum phy phy)
> > {
> > -	if (phy == PHY_NONE)
> > -		return false;
> > -	else if (display->platform.alderlake_s)
> > +	if (display->platform.alderlake_s)
> > 		return phy <= PHY_E;
> > 	else if (display->platform.dg1 || display->platform.rocketlake)
> > 		return phy <= PHY_D;
> > @@ -1866,7 +1864,7 @@ bool intel_phy_is_snps(struct intel_display *display, enum phy phy)
> > 	 * For DG2, and for DG2 only, all four "combo" ports and the TC1 port
> > 	 * (PHY E) use Synopsis PHYs. See intel_phy_is_tc().
> > 	 */
> > -	return display->platform.dg2 && phy > PHY_NONE && phy <= PHY_E;
> > +	return display->platform.dg2 && phy <= PHY_E;
> > }
> > 
> > /* Prefer intel_encoder_to_phy() */
> > @@ -1884,6 +1882,10 @@ enum phy intel_port_to_phy(struct intel_display *display, enum port port)
> > 		 port == PORT_D)
> > 		return PHY_A;
> > 
> > +	if (drm_WARN(display->drm, port < 0,
> > +		     "PHY is invalid if port < 0 (%d), assuming PHY_A\n", port))
> > +		return PHY_A;
> > +
> > 	return PHY_A + port - PORT_A;
> > }
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.h b/drivers/gpu/drm/i915/display/intel_display.h
> > index 98b589e8360d..a4f621934b33 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.h
> > +++ b/drivers/gpu/drm/i915/display/intel_display.h
> > @@ -136,8 +136,6 @@ enum tc_port {
> > };
> > 
> > enum phy {
> > -	PHY_NONE = -1,
> > -
> > 	PHY_A = 0,
> 
> I'm wondering if with this change we could remove the assignment above
> (PHY_A = 0), since I think enums are starting from 0 by default;

Yeah, the assignment to 0 is now redundant, I'll remove it.


> but again I might be missing common codestyle guidelines. Anyways:
> 
> Reviewed-by: Michał Grzelak <[email protected]>

Thank you!

--
Cheers,
Luca.