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

["Michał Grzelak" <michal.grzelak@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4483 bytes --]

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 <luciano.coelho@intel.com>
> ---
> 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; but
again I might be missing common codestyle guidelines. Anyways:

Reviewed-by: Michał Grzelak <michal.grzelak@intel.com>

BR,
Michał

> 	PHY_B,
> 	PHY_C,
> diff --git a/drivers/gpu/drm/i915/display/intel_display_power_well.c b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> index 04bd0dde5bed..daea2452a19e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display_power_well.c
> +++ b/drivers/gpu/drm/i915/display/intel_display_power_well.c
> @@ -325,7 +325,11 @@ static enum phy icl_aux_pw_to_phy(struct intel_display *display,
> {
> 	struct intel_encoder *encoder = icl_aux_pw_to_encoder(display, power_well);
>
> -	return encoder ? intel_encoder_to_phy(encoder) : PHY_NONE;
> +	if (drm_WARN(display->drm, !encoder,
> +		     "PHY is invalid if encoder is NULL, assuming PHY_A\n"))
> +		return PHY_A;
> +
> +	return intel_encoder_to_phy(encoder);
> }
>
> static bool icl_aux_pw_is_tc_phy(struct intel_display *display,
> diff --git a/drivers/gpu/drm/i915/display/intel_hti.c b/drivers/gpu/drm/i915/display/intel_hti.c
> index dc454420c134..56602240ceff 100644
> --- a/drivers/gpu/drm/i915/display/intel_hti.c
> +++ b/drivers/gpu/drm/i915/display/intel_hti.c
> @@ -23,9 +23,6 @@ void intel_hti_init(struct intel_display *display)
>
> bool intel_hti_uses_phy(struct intel_display *display, enum phy phy)
> {
> -	if (drm_WARN_ON(display->drm, phy == PHY_NONE))
> -		return false;
> -
> 	return display->hti.state & HDPORT_ENABLED &&
> 		display->hti.state & HDPORT_DDI_USED(phy);
> }
>