[PATCH v2 00/14] drm/msm/hdmi: rework and fix the HPD even generation

[PATCH v2 00/14] drm/msm/hdmi: rework and fix the HPD even generation

[Dmitry Baryshkov]

The MSM HDMI driver is plagued with the long-standing bug. If HDMI cable
is disconnected, in most of the cases cable reconnection will not be
detected properly. We have been carrying the patch from [1] in our
integration tree for ages. The time has come to fix the long-standing
bug and implement proper HPD handling.

This series was tested on msm8996 and apq8064 boards. Previously HPD
handling sometimes could trigger in the CRTC event handling, however I
can no longer reproduce it now.

[1] https://lore.kernel.org/linux-arm-msm/20171027105732.19235-2-architt@codeaurora.org/

---
Dmitry Baryshkov (14):
      drm/msm/hdmi: move the alt_iface clock to the hpd list
      drm/msm/hdmi: simplify extp clock handling
      drm/msm/hdmi: switch to atomic_pre_enable/post_disable
      drm/msm/hdmi: set infoframes on all pre_enable calls
      drm/msm/hdmi: drop clock frequency assignment
      drm/msm/hdmi: switch to clk_bulk API
      drm/msm/hdmi: switch to pm_runtime_resume_and_get()
      drm/msm/hdmi: add runtime PM calls to DDC transfer function
      drm/msm/hdmi: implement proper runtime PM handling
      drm/msm/hdmi: rename hpd_clks to pwr_clks
      drm/msm/hdmi: expand the HDMI_CFG macro
      drm/msm/hdmi: drop hpd-gpios support
      drm/msm/hdmi: ensure that HDMI is one if HPD is requested
      drm/msm/hdmi: wire in hpd_enable/hpd_disable bridge ops

 drivers/gpu/drm/msm/hdmi/hdmi.c        | 145 ++++++++++++++++-----------------
 drivers/gpu/drm/msm/hdmi/hdmi.h        |  26 ++----
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  80 +++++++++---------
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 142 ++++++--------------------------
 drivers/gpu/drm/msm/hdmi/hdmi_i2c.c    |  14 +++-
 drivers/gpu/drm/msm/hdmi/hdmi_phy.c    |   6 +-
 6 files changed, 157 insertions(+), 256 deletions(-)
---
base-commit: 8314289a8d50a4e05d8ece1ae0445a3b57bb4d3b
change-id: 20240522-fd-hdmi-hpd-e3868deb6ae0

Best regards,
-- 
Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[PATCH v2 01/14] drm/msm/hdmi: move the alt_iface clock to the hpd list

[Dmitry Baryshkov]

According to the vendor kernel [1] , the alt_iface clock should be
enabled together with the rest of HPD clocks, to make HPD to work
properly.

[1] https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/e07a5487e521e57f76083c0a6e2f995414ac6d03

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 24abcb7254cc..108c86925780 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -235,9 +235,9 @@ static const struct hdmi_platform_config hdmi_tx_8960_config = {
 };
 
 static const char *pwr_reg_names_8x74[] = {"core-vdda", "core-vcc"};
-static const char *pwr_clk_names_8x74[] = {"extp", "alt_iface"};
-static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core"};
-static unsigned long hpd_clk_freq_8x74[] = {0, 19200000, 0};
+static const char *pwr_clk_names_8x74[] = {"extp"};
+static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
+static unsigned long hpd_clk_freq_8x74[] = {0, 19200000, 0, 0};
 
 static const struct hdmi_platform_config hdmi_tx_8974_config = {
 		HDMI_CFG(pwr_reg, 8x74),

-- 
2.39.2

Re: [PATCH v2 01/14] drm/msm/hdmi: move the alt_iface clock to the hpd list

[Jessica Zhang]

On 5/22/2024 3:50 AM, Dmitry Baryshkov wrote:
> According to the vendor kernel [1] , the alt_iface clock should be
> enabled together with the rest of HPD clocks, to make HPD to work
> properly.
> 
> [1] https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/e07a5487e521e57f76083c0a6e2f995414ac6d03
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Hi Dmitry,

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

Thanks,

Jessica Zhang

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 24abcb7254cc..108c86925780 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -235,9 +235,9 @@ static const struct hdmi_platform_config hdmi_tx_8960_config = {
>   };
>   
>   static const char *pwr_reg_names_8x74[] = {"core-vdda", "core-vcc"};
> -static const char *pwr_clk_names_8x74[] = {"extp", "alt_iface"};
> -static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core"};
> -static unsigned long hpd_clk_freq_8x74[] = {0, 19200000, 0};
> +static const char *pwr_clk_names_8x74[] = {"extp"};
> +static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
> +static unsigned long hpd_clk_freq_8x74[] = {0, 19200000, 0, 0};
>   
>   static const struct hdmi_platform_config hdmi_tx_8974_config = {
>   		HDMI_CFG(pwr_reg, 8x74),
> 
> -- 
> 2.39.2
> 

[PATCH v2 02/14] drm/msm/hdmi: simplify extp clock handling

[Dmitry Baryshkov]

With the extp being the only "power" clock left, remove the surrounding
loops and handle the extp clock directly.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c        | 24 ++++--------------------
 drivers/gpu/drm/msm/hdmi/hdmi.h        |  6 +-----
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 33 +++++++++++++--------------------
 3 files changed, 18 insertions(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 108c86925780..681265e29aa0 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -235,13 +235,11 @@ static const struct hdmi_platform_config hdmi_tx_8960_config = {
 };
 
 static const char *pwr_reg_names_8x74[] = {"core-vdda", "core-vcc"};
-static const char *pwr_clk_names_8x74[] = {"extp"};
 static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
 static unsigned long hpd_clk_freq_8x74[] = {0, 19200000, 0, 0};
 
 static const struct hdmi_platform_config hdmi_tx_8974_config = {
 		HDMI_CFG(pwr_reg, 8x74),
-		HDMI_CFG(pwr_clk, 8x74),
 		HDMI_CFG(hpd_clk, 8x74),
 		.hpd_freq      = hpd_clk_freq_8x74,
 };
@@ -485,24 +483,10 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
 		hdmi->hpd_clks[i] = clk;
 	}
 
-	hdmi->pwr_clks = devm_kcalloc(&pdev->dev,
-				      config->pwr_clk_cnt,
-				      sizeof(hdmi->pwr_clks[0]),
-				      GFP_KERNEL);
-	if (!hdmi->pwr_clks)
-		return -ENOMEM;
-
-	for (i = 0; i < config->pwr_clk_cnt; i++) {
-		struct clk *clk;
-
-		clk = msm_clk_get(pdev, config->pwr_clk_names[i]);
-		if (IS_ERR(clk))
-			return dev_err_probe(dev, PTR_ERR(clk),
-					     "failed to get pwr clk: %s\n",
-					     config->pwr_clk_names[i]);
-
-		hdmi->pwr_clks[i] = clk;
-	}
+	hdmi->extp_clk = devm_clk_get_optional(&pdev->dev, "extp");
+	if (IS_ERR(hdmi->extp_clk))
+		return dev_err_probe(dev, PTR_ERR(hdmi->extp_clk),
+				     "failed to get extp clock\n");
 
 	hdmi->hpd_gpiod = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN);
 	/* This will catch e.g. -EPROBE_DEFER */
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 4586baf36415..abdbe4779cf9 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -51,7 +51,7 @@ struct hdmi {
 	struct regulator_bulk_data *hpd_regs;
 	struct regulator_bulk_data *pwr_regs;
 	struct clk **hpd_clks;
-	struct clk **pwr_clks;
+	struct clk *extp_clk;
 
 	struct gpio_desc *hpd_gpiod;
 
@@ -98,10 +98,6 @@ struct hdmi_platform_config {
 	const char **hpd_clk_names;
 	const long unsigned *hpd_freq;
 	int hpd_clk_cnt;
-
-	/* clks that need to be on for screen pwr (ie pixel clk): */
-	const char **pwr_clk_names;
-	int pwr_clk_cnt;
 };
 
 struct hdmi_bridge {
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 4a5b5112227f..9eb4d06bdc0e 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -17,7 +17,7 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
 	const struct hdmi_platform_config *config = hdmi->config;
-	int i, ret;
+	int ret;
 
 	pm_runtime_get_sync(&hdmi->pdev->dev);
 
@@ -25,21 +25,15 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
 	if (ret)
 		DRM_DEV_ERROR(dev->dev, "failed to enable pwr regulator: %d\n", ret);
 
-	if (config->pwr_clk_cnt > 0) {
+	if (hdmi->extp_clk) {
 		DBG("pixclock: %lu", hdmi->pixclock);
-		ret = clk_set_rate(hdmi->pwr_clks[0], hdmi->pixclock);
-		if (ret) {
-			DRM_DEV_ERROR(dev->dev, "failed to set pixel clk: %s (%d)\n",
-					config->pwr_clk_names[0], ret);
-		}
-	}
+		ret = clk_set_rate(hdmi->extp_clk, hdmi->pixclock);
+		if (ret)
+			DRM_DEV_ERROR(dev->dev, "failed to set extp clk rate: %d\n", ret);
 
-	for (i = 0; i < config->pwr_clk_cnt; i++) {
-		ret = clk_prepare_enable(hdmi->pwr_clks[i]);
-		if (ret) {
-			DRM_DEV_ERROR(dev->dev, "failed to enable pwr clk: %s (%d)\n",
-					config->pwr_clk_names[i], ret);
-		}
+		ret = clk_prepare_enable(hdmi->extp_clk);
+		if (ret)
+			DRM_DEV_ERROR(dev->dev, "failed to enable extp clk: %d\n", ret);
 	}
 }
 
@@ -49,15 +43,15 @@ static void power_off(struct drm_bridge *bridge)
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
 	const struct hdmi_platform_config *config = hdmi->config;
-	int i, ret;
+	int ret;
 
 	/* TODO do we need to wait for final vblank somewhere before
 	 * cutting the clocks?
 	 */
 	mdelay(16 + 4);
 
-	for (i = 0; i < config->pwr_clk_cnt; i++)
-		clk_disable_unprepare(hdmi->pwr_clks[i]);
+	if (hdmi->extp_clk)
+		clk_disable_unprepare(hdmi->extp_clk);
 
 	ret = regulator_bulk_disable(config->pwr_reg_cnt, hdmi->pwr_regs);
 	if (ret)
@@ -271,7 +265,6 @@ static enum drm_mode_status msm_hdmi_bridge_mode_valid(struct drm_bridge *bridge
 {
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
-	const struct hdmi_platform_config *config = hdmi->config;
 	struct msm_drm_private *priv = bridge->dev->dev_private;
 	struct msm_kms *kms = priv->kms;
 	long actual, requested;
@@ -285,8 +278,8 @@ static enum drm_mode_status msm_hdmi_bridge_mode_valid(struct drm_bridge *bridge
 	if (kms->funcs->round_pixclk)
 		actual = kms->funcs->round_pixclk(kms,
 			requested, hdmi_bridge->hdmi->encoder);
-	else if (config->pwr_clk_cnt > 0)
-		actual = clk_round_rate(hdmi->pwr_clks[0], requested);
+	else if (hdmi->extp_clk)
+		actual = clk_round_rate(hdmi->extp_clk, requested);
 	else
 		actual = requested;
 

-- 
2.39.2

Re: [PATCH v2 02/14] drm/msm/hdmi: simplify extp clock handling

[Jessica Zhang]

On 5/22/2024 3:50 AM, Dmitry Baryshkov wrote:
> With the extp being the only "power" clock left, remove the surrounding
> loops and handle the extp clock directly.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c        | 24 ++++--------------------
>   drivers/gpu/drm/msm/hdmi/hdmi.h        |  6 +-----
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 33 +++++++++++++--------------------
>   3 files changed, 18 insertions(+), 45 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 108c86925780..681265e29aa0 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -235,13 +235,11 @@ static const struct hdmi_platform_config hdmi_tx_8960_config = {
>   };
>   
>   static const char *pwr_reg_names_8x74[] = {"core-vdda", "core-vcc"};
> -static const char *pwr_clk_names_8x74[] = {"extp"};
>   static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
>   static unsigned long hpd_clk_freq_8x74[] = {0, 19200000, 0, 0};
>   
>   static const struct hdmi_platform_config hdmi_tx_8974_config = {
>   		HDMI_CFG(pwr_reg, 8x74),
> -		HDMI_CFG(pwr_clk, 8x74),
>   		HDMI_CFG(hpd_clk, 8x74),
>   		.hpd_freq      = hpd_clk_freq_8x74,
>   };
> @@ -485,24 +483,10 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   		hdmi->hpd_clks[i] = clk;
>   	}
>   
> -	hdmi->pwr_clks = devm_kcalloc(&pdev->dev,
> -				      config->pwr_clk_cnt,
> -				      sizeof(hdmi->pwr_clks[0]),
> -				      GFP_KERNEL);
> -	if (!hdmi->pwr_clks)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < config->pwr_clk_cnt; i++) {
> -		struct clk *clk;
> -
> -		clk = msm_clk_get(pdev, config->pwr_clk_names[i]);
> -		if (IS_ERR(clk))
> -			return dev_err_probe(dev, PTR_ERR(clk),
> -					     "failed to get pwr clk: %s\n",
> -					     config->pwr_clk_names[i]);
> -
> -		hdmi->pwr_clks[i] = clk;
> -	}
> +	hdmi->extp_clk = devm_clk_get_optional(&pdev->dev, "extp");
> +	if (IS_ERR(hdmi->extp_clk))
> +		return dev_err_probe(dev, PTR_ERR(hdmi->extp_clk),
> +				     "failed to get extp clock\n");
>   
>   	hdmi->hpd_gpiod = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN);
>   	/* This will catch e.g. -EPROBE_DEFER */
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index 4586baf36415..abdbe4779cf9 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -51,7 +51,7 @@ struct hdmi {
>   	struct regulator_bulk_data *hpd_regs;
>   	struct regulator_bulk_data *pwr_regs;
>   	struct clk **hpd_clks;
> -	struct clk **pwr_clks;
> +	struct clk *extp_clk;
>   
>   	struct gpio_desc *hpd_gpiod;
>   
> @@ -98,10 +98,6 @@ struct hdmi_platform_config {
>   	const char **hpd_clk_names;
>   	const long unsigned *hpd_freq;
>   	int hpd_clk_cnt;
> -
> -	/* clks that need to be on for screen pwr (ie pixel clk): */
> -	const char **pwr_clk_names;
> -	int pwr_clk_cnt;
>   };
>   
>   struct hdmi_bridge {
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 4a5b5112227f..9eb4d06bdc0e 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -17,7 +17,7 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
>   	const struct hdmi_platform_config *config = hdmi->config;
> -	int i, ret;
> +	int ret;
>   
>   	pm_runtime_get_sync(&hdmi->pdev->dev);
>   
> @@ -25,21 +25,15 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
>   	if (ret)
>   		DRM_DEV_ERROR(dev->dev, "failed to enable pwr regulator: %d\n", ret);
>   
> -	if (config->pwr_clk_cnt > 0) {
> +	if (hdmi->extp_clk) {
>   		DBG("pixclock: %lu", hdmi->pixclock);
> -		ret = clk_set_rate(hdmi->pwr_clks[0], hdmi->pixclock);
> -		if (ret) {
> -			DRM_DEV_ERROR(dev->dev, "failed to set pixel clk: %s (%d)\n",
> -					config->pwr_clk_names[0], ret);
> -		}
> -	}
> +		ret = clk_set_rate(hdmi->extp_clk, hdmi->pixclock);
> +		if (ret)
> +			DRM_DEV_ERROR(dev->dev, "failed to set extp clk rate: %d\n", ret);
>   
> -	for (i = 0; i < config->pwr_clk_cnt; i++) {
> -		ret = clk_prepare_enable(hdmi->pwr_clks[i]);
> -		if (ret) {
> -			DRM_DEV_ERROR(dev->dev, "failed to enable pwr clk: %s (%d)\n",
> -					config->pwr_clk_names[i], ret);
> -		}
> +		ret = clk_prepare_enable(hdmi->extp_clk);
> +		if (ret)
> +			DRM_DEV_ERROR(dev->dev, "failed to enable extp clk: %d\n", ret);
>   	}
>   }
>   
> @@ -49,15 +43,15 @@ static void power_off(struct drm_bridge *bridge)
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
>   	const struct hdmi_platform_config *config = hdmi->config;
> -	int i, ret;
> +	int ret;
>   
>   	/* TODO do we need to wait for final vblank somewhere before
>   	 * cutting the clocks?
>   	 */
>   	mdelay(16 + 4);
>   
> -	for (i = 0; i < config->pwr_clk_cnt; i++)
> -		clk_disable_unprepare(hdmi->pwr_clks[i]);
> +	if (hdmi->extp_clk)
> +		clk_disable_unprepare(hdmi->extp_clk);
>   
>   	ret = regulator_bulk_disable(config->pwr_reg_cnt, hdmi->pwr_regs);
>   	if (ret)
> @@ -271,7 +265,6 @@ static enum drm_mode_status msm_hdmi_bridge_mode_valid(struct drm_bridge *bridge
>   {
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
> -	const struct hdmi_platform_config *config = hdmi->config;
>   	struct msm_drm_private *priv = bridge->dev->dev_private;
>   	struct msm_kms *kms = priv->kms;
>   	long actual, requested;
> @@ -285,8 +278,8 @@ static enum drm_mode_status msm_hdmi_bridge_mode_valid(struct drm_bridge *bridge
>   	if (kms->funcs->round_pixclk)
>   		actual = kms->funcs->round_pixclk(kms,
>   			requested, hdmi_bridge->hdmi->encoder);
> -	else if (config->pwr_clk_cnt > 0)
> -		actual = clk_round_rate(hdmi->pwr_clks[0], requested);
> +	else if (hdmi->extp_clk)
> +		actual = clk_round_rate(hdmi->extp_clk, requested);
>   	else
>   		actual = requested;
>   
> 
> -- 
> 2.39.2
> 

[PATCH v2 03/14] drm/msm/hdmi: switch to atomic_pre_enable/post_disable

[Dmitry Baryshkov]

In preparation of reworking the HDMI mode setting, switch pre_enable and
post_disable callbacks to their atomic variants.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 9eb4d06bdc0e..3c6121c57b01 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -120,7 +120,8 @@ static void msm_hdmi_config_avi_infoframe(struct hdmi *hdmi)
 	hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL1, val);
 }
 
-static void msm_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
+static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
+					      struct drm_bridge_state *old_bridge_state)
 {
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
@@ -146,7 +147,8 @@ static void msm_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
 		msm_hdmi_hdcp_on(hdmi->hdcp_ctrl);
 }
 
-static void msm_hdmi_bridge_post_disable(struct drm_bridge *bridge)
+static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
+						struct drm_bridge_state *old_bridge_state)
 {
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
@@ -292,8 +294,13 @@ static enum drm_mode_status msm_hdmi_bridge_mode_valid(struct drm_bridge *bridge
 }
 
 static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = {
-	.pre_enable = msm_hdmi_bridge_pre_enable,
-	.post_disable = msm_hdmi_bridge_post_disable,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
+
+	.atomic_pre_enable = msm_hdmi_bridge_atomic_pre_enable,
+	.atomic_post_disable = msm_hdmi_bridge_atomic_post_disable,
+
 	.mode_set = msm_hdmi_bridge_mode_set,
 	.mode_valid = msm_hdmi_bridge_mode_valid,
 	.edid_read = msm_hdmi_bridge_edid_read,

-- 
2.39.2

Re: [PATCH v2 03/14] drm/msm/hdmi: switch to atomic_pre_enable/post_disable

[Jessica Zhang]

On 5/22/2024 3:50 AM, Dmitry Baryshkov wrote:
> In preparation of reworking the HDMI mode setting, switch pre_enable and
> post_disable callbacks to their atomic variants.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 15 +++++++++++----
>   1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 9eb4d06bdc0e..3c6121c57b01 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -120,7 +120,8 @@ static void msm_hdmi_config_avi_infoframe(struct hdmi *hdmi)
>   	hdmi_write(hdmi, REG_HDMI_INFOFRAME_CTRL1, val);
>   }
>   
> -static void msm_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
> +static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> +					      struct drm_bridge_state *old_bridge_state)
>   {
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
> @@ -146,7 +147,8 @@ static void msm_hdmi_bridge_pre_enable(struct drm_bridge *bridge)
>   		msm_hdmi_hdcp_on(hdmi->hdcp_ctrl);
>   }
>   
> -static void msm_hdmi_bridge_post_disable(struct drm_bridge *bridge)
> +static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
> +						struct drm_bridge_state *old_bridge_state)
>   {
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
> @@ -292,8 +294,13 @@ static enum drm_mode_status msm_hdmi_bridge_mode_valid(struct drm_bridge *bridge
>   }
>   
>   static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = {
> -	.pre_enable = msm_hdmi_bridge_pre_enable,
> -	.post_disable = msm_hdmi_bridge_post_disable,
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +	.atomic_reset = drm_atomic_helper_bridge_reset,
> +
> +	.atomic_pre_enable = msm_hdmi_bridge_atomic_pre_enable,
> +	.atomic_post_disable = msm_hdmi_bridge_atomic_post_disable,
> +
>   	.mode_set = msm_hdmi_bridge_mode_set,
>   	.mode_valid = msm_hdmi_bridge_mode_valid,
>   	.edid_read = msm_hdmi_bridge_edid_read,
> 
> -- 
> 2.39.2
> 

[PATCH v2 04/14] drm/msm/hdmi: set infoframes on all pre_enable calls

[Dmitry Baryshkov]

In consequent modeset calls, the atomic_pre_enable() will be called
several times without calling atomic_post_disable() inbetween. Thus
iframes will not be updated for the next mode. Fix this by setting the
iframe outside of the !power_on check.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 3c6121c57b01..fb99328107dd 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -133,10 +133,11 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 		msm_hdmi_phy_resource_enable(phy);
 		msm_hdmi_power_on(bridge);
 		hdmi->power_on = true;
-		if (hdmi->hdmi_mode) {
-			msm_hdmi_config_avi_infoframe(hdmi);
-			msm_hdmi_audio_update(hdmi);
-		}
+	}
+
+	if (hdmi->hdmi_mode) {
+		msm_hdmi_config_avi_infoframe(hdmi);
+		msm_hdmi_audio_update(hdmi);
 	}
 
 	msm_hdmi_phy_powerup(phy, hdmi->pixclock);

-- 
2.39.2

Re: [PATCH v2 04/14] drm/msm/hdmi: set infoframes on all pre_enable calls

[Jessica Zhang]

On 5/22/2024 3:50 AM, Dmitry Baryshkov wrote:
> In consequent modeset calls, the atomic_pre_enable() will be called
> several times without calling atomic_post_disable() inbetween. Thus

Hi Dmitry,

Just wondering, where are you seeing multiple pre_enable() calls without 
a post_disable() happening?

IIRC, the msm commit_tail always calls commit_modeset_disables() before 
the commit_modeset_enables(). Also, doesn't the pre_enable() and 
post_disable() only happen once for bringing up/down the bridge?

Thanks,

Jessica Zhang

> iframes will not be updated for the next mode. Fix this by setting the
> iframe outside of the !power_on check.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 3c6121c57b01..fb99328107dd 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -133,10 +133,11 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>   		msm_hdmi_phy_resource_enable(phy);
>   		msm_hdmi_power_on(bridge);
>   		hdmi->power_on = true;
> -		if (hdmi->hdmi_mode) {
> -			msm_hdmi_config_avi_infoframe(hdmi);
> -			msm_hdmi_audio_update(hdmi);
> -		}
> +	}
> +
> +	if (hdmi->hdmi_mode) {
> +		msm_hdmi_config_avi_infoframe(hdmi);
> +		msm_hdmi_audio_update(hdmi);
>   	}
>   
>   	msm_hdmi_phy_powerup(phy, hdmi->pixclock);
> 
> -- 
> 2.39.2
> 

Re: [PATCH v2 04/14] drm/msm/hdmi: set infoframes on all pre_enable calls

[Dmitry Baryshkov]

On Wed, 12 Jun 2024 at 03:04, Jessica Zhang <quic_jesszhan@quicinc.com> wrote:
>
>
>
> On 5/22/2024 3:50 AM, Dmitry Baryshkov wrote:
> > In consequent modeset calls, the atomic_pre_enable() will be called
> > several times without calling atomic_post_disable() inbetween. Thus
>
> Hi Dmitry,
>
> Just wondering, where are you seeing multiple pre_enable() calls without
> a post_disable() happening?

I think that was with me hacking around, so the commit is indeed incorrect.

>
> IIRC, the msm commit_tail always calls commit_modeset_disables() before
> the commit_modeset_enables(). Also, doesn't the pre_enable() and
> post_disable() only happen once for bringing up/down the bridge?

I don't know if you mean it, but they are called each time the output
gets disabled and then enabled again, e.g. during modeswitch.

Last, but not least, I'm planning to land the HDMI rework ([1]) once
the drm-misc-next is merged into msm-next ([2]). This makes this
commit obsolete.

[1] https://lore.kernel.org/dri-devel/20240607-bridge-hdmi-connector-v5-0-ab384e6021af@linaro.org/
[2] https://gitlab.freedesktop.org/drm/msm/-/merge_requests/118


>
> Thanks,
>
> Jessica Zhang
>
> > iframes will not be updated for the next mode. Fix this by setting the
> > iframe outside of the !power_on check.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 9 +++++----
> >   1 file changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > index 3c6121c57b01..fb99328107dd 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > @@ -133,10 +133,11 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> >               msm_hdmi_phy_resource_enable(phy);
> >               msm_hdmi_power_on(bridge);
> >               hdmi->power_on = true;
> > -             if (hdmi->hdmi_mode) {
> > -                     msm_hdmi_config_avi_infoframe(hdmi);
> > -                     msm_hdmi_audio_update(hdmi);
> > -             }
> > +     }
> > +
> > +     if (hdmi->hdmi_mode) {
> > +             msm_hdmi_config_avi_infoframe(hdmi);
> > +             msm_hdmi_audio_update(hdmi);
> >       }
> >
> >       msm_hdmi_phy_powerup(phy, hdmi->pixclock);
> >
> > --
> > 2.39.2
> >



-- 
With best wishes
Dmitry

[PATCH v2 05/14] drm/msm/hdmi: drop clock frequency assignment

[Dmitry Baryshkov]

The only clock which has frequency being set through hpd_freqs is the
"core" aka MDSS_HDMI_CLK clock. It always has the specified frequency,
so we can drop corresponding clk_set_rate() call together with the
hpd_freq infrastructure.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c     | 2 --
 drivers/gpu/drm/msm/hdmi/hdmi.h     | 1 -
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c | 9 ---------
 3 files changed, 12 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 681265e29aa0..c14e009f38b1 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -236,12 +236,10 @@ static const struct hdmi_platform_config hdmi_tx_8960_config = {
 
 static const char *pwr_reg_names_8x74[] = {"core-vdda", "core-vcc"};
 static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
-static unsigned long hpd_clk_freq_8x74[] = {0, 19200000, 0, 0};
 
 static const struct hdmi_platform_config hdmi_tx_8974_config = {
 		HDMI_CFG(pwr_reg, 8x74),
 		HDMI_CFG(hpd_clk, 8x74),
-		.hpd_freq      = hpd_clk_freq_8x74,
 };
 
 /*
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index abdbe4779cf9..c0d60ed23b75 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -96,7 +96,6 @@ struct hdmi_platform_config {
 
 	/* clks that need to be on for hpd: */
 	const char **hpd_clk_names;
-	const long unsigned *hpd_freq;
 	int hpd_clk_cnt;
 };
 
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
index 9ce0ffa35417..7ae69b14e953 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
@@ -68,15 +68,6 @@ static void enable_hpd_clocks(struct hdmi *hdmi, bool enable)
 
 	if (enable) {
 		for (i = 0; i < config->hpd_clk_cnt; i++) {
-			if (config->hpd_freq && config->hpd_freq[i]) {
-				ret = clk_set_rate(hdmi->hpd_clks[i],
-						   config->hpd_freq[i]);
-				if (ret)
-					dev_warn(dev,
-						 "failed to set clk %s (%d)\n",
-						 config->hpd_clk_names[i], ret);
-			}
-
 			ret = clk_prepare_enable(hdmi->hpd_clks[i]);
 			if (ret) {
 				DRM_DEV_ERROR(dev,

-- 
2.39.2

Re: [PATCH v2 05/14] drm/msm/hdmi: drop clock frequency assignment

[Jessica Zhang]

On 5/22/2024 3:50 AM, Dmitry Baryshkov wrote:
> The only clock which has frequency being set through hpd_freqs is the
> "core" aka MDSS_HDMI_CLK clock. It always has the specified frequency,
> so we can drop corresponding clk_set_rate() call together with the
> hpd_freq infrastructure.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c     | 2 --
>   drivers/gpu/drm/msm/hdmi/hdmi.h     | 1 -
>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c | 9 ---------
>   3 files changed, 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 681265e29aa0..c14e009f38b1 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -236,12 +236,10 @@ static const struct hdmi_platform_config hdmi_tx_8960_config = {
>   
>   static const char *pwr_reg_names_8x74[] = {"core-vdda", "core-vcc"};
>   static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
> -static unsigned long hpd_clk_freq_8x74[] = {0, 19200000, 0, 0};
>   
>   static const struct hdmi_platform_config hdmi_tx_8974_config = {
>   		HDMI_CFG(pwr_reg, 8x74),
>   		HDMI_CFG(hpd_clk, 8x74),
> -		.hpd_freq      = hpd_clk_freq_8x74,
>   };
>   
>   /*
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index abdbe4779cf9..c0d60ed23b75 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -96,7 +96,6 @@ struct hdmi_platform_config {
>   
>   	/* clks that need to be on for hpd: */
>   	const char **hpd_clk_names;
> -	const long unsigned *hpd_freq;
>   	int hpd_clk_cnt;
>   };
>   
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> index 9ce0ffa35417..7ae69b14e953 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> @@ -68,15 +68,6 @@ static void enable_hpd_clocks(struct hdmi *hdmi, bool enable)
>   
>   	if (enable) {
>   		for (i = 0; i < config->hpd_clk_cnt; i++) {
> -			if (config->hpd_freq && config->hpd_freq[i]) {
> -				ret = clk_set_rate(hdmi->hpd_clks[i],
> -						   config->hpd_freq[i]);
> -				if (ret)
> -					dev_warn(dev,
> -						 "failed to set clk %s (%d)\n",
> -						 config->hpd_clk_names[i], ret);
> -			}
> -
>   			ret = clk_prepare_enable(hdmi->hpd_clks[i]);
>   			if (ret) {
>   				DRM_DEV_ERROR(dev,
> 
> -- 
> 2.39.2
> 

[PATCH v2 06/14] drm/msm/hdmi: switch to clk_bulk API

[Dmitry Baryshkov]

The last platform using legacy clock names for HDMI block (APQ8064)
switched to new clock names in 5.16. It's time to stop caring about old
DT, drop hand-coded helpers and switch to clk_bulk_* API.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c     | 15 +++++---------
 drivers/gpu/drm/msm/hdmi/hdmi.h     |  2 +-
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c | 39 +++++++++++++------------------------
 3 files changed, 19 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index c14e009f38b1..7ec4ca3b7597 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -469,17 +469,12 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
 	if (!hdmi->hpd_clks)
 		return -ENOMEM;
 
-	for (i = 0; i < config->hpd_clk_cnt; i++) {
-		struct clk *clk;
+	for (i = 0; i < config->hpd_clk_cnt; i++)
+		hdmi->hpd_clks[i].id = config->hpd_clk_names[i];
 
-		clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
-		if (IS_ERR(clk))
-			return dev_err_probe(dev, PTR_ERR(clk),
-					     "failed to get hpd clk: %s\n",
-					     config->hpd_clk_names[i]);
-
-		hdmi->hpd_clks[i] = clk;
-	}
+	ret = devm_clk_bulk_get(&pdev->dev, config->hpd_clk_cnt, hdmi->hpd_clks);
+	if (ret)
+		return ret;
 
 	hdmi->extp_clk = devm_clk_get_optional(&pdev->dev, "extp");
 	if (IS_ERR(hdmi->extp_clk))
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index c0d60ed23b75..eeba85ffef09 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -50,7 +50,7 @@ struct hdmi {
 
 	struct regulator_bulk_data *hpd_regs;
 	struct regulator_bulk_data *pwr_regs;
-	struct clk **hpd_clks;
+	struct clk_bulk_data *hpd_clks;
 	struct clk *extp_clk;
 
 	struct gpio_desc *hpd_gpiod;
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
index 7ae69b14e953..36266aa626dc 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
@@ -60,27 +60,6 @@ static void msm_hdmi_phy_reset(struct hdmi *hdmi)
 	}
 }
 
-static void enable_hpd_clocks(struct hdmi *hdmi, bool enable)
-{
-	const struct hdmi_platform_config *config = hdmi->config;
-	struct device *dev = &hdmi->pdev->dev;
-	int i, ret;
-
-	if (enable) {
-		for (i = 0; i < config->hpd_clk_cnt; i++) {
-			ret = clk_prepare_enable(hdmi->hpd_clks[i]);
-			if (ret) {
-				DRM_DEV_ERROR(dev,
-					"failed to enable hpd clk: %s (%d)\n",
-					config->hpd_clk_names[i], ret);
-			}
-		}
-	} else {
-		for (i = config->hpd_clk_cnt - 1; i >= 0; i--)
-			clk_disable_unprepare(hdmi->hpd_clks[i]);
-	}
-}
-
 int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
 {
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
@@ -107,7 +86,9 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
 		gpiod_set_value_cansleep(hdmi->hpd_gpiod, 1);
 
 	pm_runtime_get_sync(dev);
-	enable_hpd_clocks(hdmi, true);
+	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
+	if (ret)
+		goto fail;
 
 	msm_hdmi_set_mode(hdmi, false);
 	msm_hdmi_phy_reset(hdmi);
@@ -149,7 +130,7 @@ void msm_hdmi_hpd_disable(struct hdmi *hdmi)
 
 	msm_hdmi_set_mode(hdmi, false);
 
-	enable_hpd_clocks(hdmi, false);
+	clk_bulk_disable_unprepare(config->hpd_clk_cnt, hdmi->hpd_clks);
 	pm_runtime_put(dev);
 
 	ret = pinctrl_pm_select_sleep_state(dev);
@@ -193,14 +174,20 @@ void msm_hdmi_hpd_irq(struct drm_bridge *bridge)
 
 static enum drm_connector_status detect_reg(struct hdmi *hdmi)
 {
-	uint32_t hpd_int_status;
+	const struct hdmi_platform_config *config = hdmi->config;
+	uint32_t hpd_int_status = 0;
+	int ret;
 
 	pm_runtime_get_sync(&hdmi->pdev->dev);
-	enable_hpd_clocks(hdmi, true);
+	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
+	if (ret)
+		goto out;
 
 	hpd_int_status = hdmi_read(hdmi, REG_HDMI_HPD_INT_STATUS);
 
-	enable_hpd_clocks(hdmi, false);
+	clk_bulk_disable_unprepare(config->hpd_clk_cnt, hdmi->hpd_clks);
+
+out:
 	pm_runtime_put(&hdmi->pdev->dev);
 
 	return (hpd_int_status & HDMI_HPD_INT_STATUS_CABLE_DETECTED) ?

-- 
2.39.2

Re: [PATCH v2 06/14] drm/msm/hdmi: switch to clk_bulk API

[Jessica Zhang]

On 5/22/2024 3:50 AM, Dmitry Baryshkov wrote:
> The last platform using legacy clock names for HDMI block (APQ8064)
> switched to new clock names in 5.16. It's time to stop caring about old
> DT, drop hand-coded helpers and switch to clk_bulk_* API.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c     | 15 +++++---------
>   drivers/gpu/drm/msm/hdmi/hdmi.h     |  2 +-
>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c | 39 +++++++++++++------------------------
>   3 files changed, 19 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index c14e009f38b1..7ec4ca3b7597 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -469,17 +469,12 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   	if (!hdmi->hpd_clks)
>   		return -ENOMEM;
>   
> -	for (i = 0; i < config->hpd_clk_cnt; i++) {
> -		struct clk *clk;
> +	for (i = 0; i < config->hpd_clk_cnt; i++)
> +		hdmi->hpd_clks[i].id = config->hpd_clk_names[i];
>   
> -		clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
> -		if (IS_ERR(clk))
> -			return dev_err_probe(dev, PTR_ERR(clk),
> -					     "failed to get hpd clk: %s\n",
> -					     config->hpd_clk_names[i]);
> -
> -		hdmi->hpd_clks[i] = clk;
> -	}
> +	ret = devm_clk_bulk_get(&pdev->dev, config->hpd_clk_cnt, hdmi->hpd_clks);
> +	if (ret)
> +		return ret;
>   
>   	hdmi->extp_clk = devm_clk_get_optional(&pdev->dev, "extp");
>   	if (IS_ERR(hdmi->extp_clk))
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index c0d60ed23b75..eeba85ffef09 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -50,7 +50,7 @@ struct hdmi {
>   
>   	struct regulator_bulk_data *hpd_regs;
>   	struct regulator_bulk_data *pwr_regs;
> -	struct clk **hpd_clks;
> +	struct clk_bulk_data *hpd_clks;
>   	struct clk *extp_clk;
>   
>   	struct gpio_desc *hpd_gpiod;
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> index 7ae69b14e953..36266aa626dc 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> @@ -60,27 +60,6 @@ static void msm_hdmi_phy_reset(struct hdmi *hdmi)
>   	}
>   }
>   
> -static void enable_hpd_clocks(struct hdmi *hdmi, bool enable)
> -{
> -	const struct hdmi_platform_config *config = hdmi->config;
> -	struct device *dev = &hdmi->pdev->dev;
> -	int i, ret;
> -
> -	if (enable) {
> -		for (i = 0; i < config->hpd_clk_cnt; i++) {
> -			ret = clk_prepare_enable(hdmi->hpd_clks[i]);
> -			if (ret) {
> -				DRM_DEV_ERROR(dev,
> -					"failed to enable hpd clk: %s (%d)\n",
> -					config->hpd_clk_names[i], ret);
> -			}
> -		}
> -	} else {
> -		for (i = config->hpd_clk_cnt - 1; i >= 0; i--)
> -			clk_disable_unprepare(hdmi->hpd_clks[i]);
> -	}
> -}
> -
>   int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>   {
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
> @@ -107,7 +86,9 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>   		gpiod_set_value_cansleep(hdmi->hpd_gpiod, 1);
>   
>   	pm_runtime_get_sync(dev);
> -	enable_hpd_clocks(hdmi, true);
> +	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
> +	if (ret)
> +		goto fail;
>   
>   	msm_hdmi_set_mode(hdmi, false);
>   	msm_hdmi_phy_reset(hdmi);
> @@ -149,7 +130,7 @@ void msm_hdmi_hpd_disable(struct hdmi *hdmi)
>   
>   	msm_hdmi_set_mode(hdmi, false);
>   
> -	enable_hpd_clocks(hdmi, false);
> +	clk_bulk_disable_unprepare(config->hpd_clk_cnt, hdmi->hpd_clks);
>   	pm_runtime_put(dev);
>   
>   	ret = pinctrl_pm_select_sleep_state(dev);
> @@ -193,14 +174,20 @@ void msm_hdmi_hpd_irq(struct drm_bridge *bridge)
>   
>   static enum drm_connector_status detect_reg(struct hdmi *hdmi)
>   {
> -	uint32_t hpd_int_status;
> +	const struct hdmi_platform_config *config = hdmi->config;
> +	uint32_t hpd_int_status = 0;
> +	int ret;
>   
>   	pm_runtime_get_sync(&hdmi->pdev->dev);
> -	enable_hpd_clocks(hdmi, true);
> +	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
> +	if (ret)
> +		goto out;
>   
>   	hpd_int_status = hdmi_read(hdmi, REG_HDMI_HPD_INT_STATUS);
>   
> -	enable_hpd_clocks(hdmi, false);
> +	clk_bulk_disable_unprepare(config->hpd_clk_cnt, hdmi->hpd_clks);
> +
> +out:
>   	pm_runtime_put(&hdmi->pdev->dev);
>   
>   	return (hpd_int_status & HDMI_HPD_INT_STATUS_CABLE_DETECTED) ?
> 
> -- 
> 2.39.2
> 

[PATCH v2 07/14] drm/msm/hdmi: switch to pm_runtime_resume_and_get()

[Dmitry Baryshkov]

The pm_runtime_get_sync() function is a bad choise for runtime power
management. Switch HDMI driver to pm_runtime_resume_and_get() and add
proper error handling, while we are at it.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 12 ++++++++++--
 drivers/gpu/drm/msm/hdmi/hdmi_phy.c    |  6 +++++-
 3 files changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index fb99328107dd..d1b35328b6e8 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -19,7 +19,7 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
 	const struct hdmi_platform_config *config = hdmi->config;
 	int ret;
 
-	pm_runtime_get_sync(&hdmi->pdev->dev);
+	pm_runtime_resume_and_get(&hdmi->pdev->dev);
 
 	ret = regulator_bulk_enable(config->pwr_reg_cnt, hdmi->pwr_regs);
 	if (ret)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
index 36266aa626dc..fc21ad3b01dc 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
@@ -85,7 +85,12 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
 	if (hdmi->hpd_gpiod)
 		gpiod_set_value_cansleep(hdmi->hpd_gpiod, 1);
 
-	pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "runtime resume failed: %d\n", ret);
+		goto fail;
+	}
+
 	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
 	if (ret)
 		goto fail;
@@ -178,7 +183,10 @@ static enum drm_connector_status detect_reg(struct hdmi *hdmi)
 	uint32_t hpd_int_status = 0;
 	int ret;
 
-	pm_runtime_get_sync(&hdmi->pdev->dev);
+	ret = pm_runtime_resume_and_get(&hdmi->pdev->dev);
+	if (ret)
+		goto out;
+
 	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
 	if (ret)
 		goto out;
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_phy.c b/drivers/gpu/drm/msm/hdmi/hdmi_phy.c
index 88a3423b7f24..d5acae752300 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_phy.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_phy.c
@@ -58,7 +58,11 @@ int msm_hdmi_phy_resource_enable(struct hdmi_phy *phy)
 	struct device *dev = &phy->pdev->dev;
 	int i, ret = 0;
 
-	pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "runtime resume failed: %d\n", ret);
+		return ret;
+	}
 
 	ret = regulator_bulk_enable(cfg->num_regs, phy->regs);
 	if (ret) {

-- 
2.39.2

Re: [PATCH v2 07/14] drm/msm/hdmi: switch to pm_runtime_resume_and_get()

[Jessica Zhang]

On 5/22/2024 3:50 AM, Dmitry Baryshkov wrote:
> The pm_runtime_get_sync() function is a bad choise for runtime power

[nit: s/choise/choice/]

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> management. Switch HDMI driver to pm_runtime_resume_and_get() and add
> proper error handling, while we are at it.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  2 +-
>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 12 ++++++++++--
>   drivers/gpu/drm/msm/hdmi/hdmi_phy.c    |  6 +++++-
>   3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index fb99328107dd..d1b35328b6e8 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -19,7 +19,7 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
>   	const struct hdmi_platform_config *config = hdmi->config;
>   	int ret;
>   
> -	pm_runtime_get_sync(&hdmi->pdev->dev);
> +	pm_runtime_resume_and_get(&hdmi->pdev->dev);
>   
>   	ret = regulator_bulk_enable(config->pwr_reg_cnt, hdmi->pwr_regs);
>   	if (ret)
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> index 36266aa626dc..fc21ad3b01dc 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> @@ -85,7 +85,12 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>   	if (hdmi->hpd_gpiod)
>   		gpiod_set_value_cansleep(hdmi->hpd_gpiod, 1);
>   
> -	pm_runtime_get_sync(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "runtime resume failed: %d\n", ret);
> +		goto fail;
> +	}
> +
>   	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
>   	if (ret)
>   		goto fail;
> @@ -178,7 +183,10 @@ static enum drm_connector_status detect_reg(struct hdmi *hdmi)
>   	uint32_t hpd_int_status = 0;
>   	int ret;
>   
> -	pm_runtime_get_sync(&hdmi->pdev->dev);
> +	ret = pm_runtime_resume_and_get(&hdmi->pdev->dev);
> +	if (ret)
> +		goto out;
> +
>   	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
>   	if (ret)
>   		goto out;
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_phy.c b/drivers/gpu/drm/msm/hdmi/hdmi_phy.c
> index 88a3423b7f24..d5acae752300 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_phy.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_phy.c
> @@ -58,7 +58,11 @@ int msm_hdmi_phy_resource_enable(struct hdmi_phy *phy)
>   	struct device *dev = &phy->pdev->dev;
>   	int i, ret = 0;
>   
> -	pm_runtime_get_sync(dev);
> +	ret = pm_runtime_resume_and_get(dev);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "runtime resume failed: %d\n", ret);
> +		return ret;
> +	}
>   
>   	ret = regulator_bulk_enable(cfg->num_regs, phy->regs);
>   	if (ret) {
> 
> -- 
> 2.39.2
> 

[PATCH v2 08/14] drm/msm/hdmi: add runtime PM calls to DDC transfer function

[Dmitry Baryshkov]

We must be sure that the HDMI controller is powered on, while performing
the DDC transfer. Add corresponding runtime PM calls to
msm_hdmi_i2c_xfer().

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi_i2c.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_i2c.c b/drivers/gpu/drm/msm/hdmi/hdmi_i2c.c
index 7aa500d24240..ebefea4fb408 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_i2c.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_i2c.c
@@ -107,11 +107,15 @@ static int msm_hdmi_i2c_xfer(struct i2c_adapter *i2c,
 	if (num == 0)
 		return num;
 
+	ret = pm_runtime_resume_and_get(&hdmi->pdev->dev);
+	if (ret)
+		return ret;
+
 	init_ddc(hdmi_i2c);
 
 	ret = ddc_clear_irq(hdmi_i2c);
 	if (ret)
-		return ret;
+		goto fail;
 
 	for (i = 0; i < num; i++) {
 		struct i2c_msg *p = &msgs[i];
@@ -169,7 +173,7 @@ static int msm_hdmi_i2c_xfer(struct i2c_adapter *i2c,
 				hdmi_read(hdmi, REG_HDMI_DDC_SW_STATUS),
 				hdmi_read(hdmi, REG_HDMI_DDC_HW_STATUS),
 				hdmi_read(hdmi, REG_HDMI_DDC_INT_CTRL));
-		return ret;
+		goto fail;
 	}
 
 	ddc_status = hdmi_read(hdmi, REG_HDMI_DDC_SW_STATUS);
@@ -202,7 +206,13 @@ static int msm_hdmi_i2c_xfer(struct i2c_adapter *i2c,
 		}
 	}
 
+	pm_runtime_put(&hdmi->pdev->dev);
+
 	return i;
+
+fail:
+	pm_runtime_put(&hdmi->pdev->dev);
+	return ret;
 }
 
 static u32 msm_hdmi_i2c_func(struct i2c_adapter *adapter)

-- 
2.39.2

Re: [PATCH v2 08/14] drm/msm/hdmi: add runtime PM calls to DDC transfer function

[Jessica Zhang]

On 5/22/2024 3:51 AM, Dmitry Baryshkov wrote:
> We must be sure that the HDMI controller is powered on, while performing
> the DDC transfer. Add corresponding runtime PM calls to
> msm_hdmi_i2c_xfer().
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi_i2c.c | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_i2c.c b/drivers/gpu/drm/msm/hdmi/hdmi_i2c.c
> index 7aa500d24240..ebefea4fb408 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_i2c.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_i2c.c
> @@ -107,11 +107,15 @@ static int msm_hdmi_i2c_xfer(struct i2c_adapter *i2c,
>   	if (num == 0)
>   		return num;
>   
> +	ret = pm_runtime_resume_and_get(&hdmi->pdev->dev);
> +	if (ret)
> +		return ret;
> +
>   	init_ddc(hdmi_i2c);
>   
>   	ret = ddc_clear_irq(hdmi_i2c);
>   	if (ret)
> -		return ret;
> +		goto fail;
>   
>   	for (i = 0; i < num; i++) {
>   		struct i2c_msg *p = &msgs[i];
> @@ -169,7 +173,7 @@ static int msm_hdmi_i2c_xfer(struct i2c_adapter *i2c,
>   				hdmi_read(hdmi, REG_HDMI_DDC_SW_STATUS),
>   				hdmi_read(hdmi, REG_HDMI_DDC_HW_STATUS),
>   				hdmi_read(hdmi, REG_HDMI_DDC_INT_CTRL));
> -		return ret;
> +		goto fail;
>   	}
>   
>   	ddc_status = hdmi_read(hdmi, REG_HDMI_DDC_SW_STATUS);
> @@ -202,7 +206,13 @@ static int msm_hdmi_i2c_xfer(struct i2c_adapter *i2c,
>   		}
>   	}
>   
> +	pm_runtime_put(&hdmi->pdev->dev);
> +
>   	return i;
> +
> +fail:
> +	pm_runtime_put(&hdmi->pdev->dev);
> +	return ret;
>   }
>   
>   static u32 msm_hdmi_i2c_func(struct i2c_adapter *adapter)
> 
> -- 
> 2.39.2
> 

[PATCH v2 09/14] drm/msm/hdmi: implement proper runtime PM handling

[Dmitry Baryshkov]

It is completely not obvious, but the so-called 'hpd' clocks and
regulators are required for the HDMI host to function properly. Merge
pwr and hpd regulators. Use regulators, clocks and pinctrl to implement
proper runtime PM callbacks.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c        | 62 +++++++++++++++++++++++++---------
 drivers/gpu/drm/msm/hdmi/hdmi.h        |  7 +---
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 12 -------
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 42 +----------------------
 4 files changed, 48 insertions(+), 75 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 7ec4ca3b7597..cc671baad87b 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -8,6 +8,7 @@
 #include <linux/gpio/consumer.h>
 #include <linux/of_irq.h>
 #include <linux/of_platform.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/platform_device.h>
 
 #include <drm/drm_bridge_connector.h>
@@ -226,11 +227,11 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 	.item ## _names = item ##_names_ ## entry, \
 	.item ## _cnt   = ARRAY_SIZE(item ## _names_ ## entry)
 
-static const char *hpd_reg_names_8960[] = {"core-vdda"};
+static const char *pwr_reg_names_8960[] = {"core-vdda"};
 static const char *hpd_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
 
 static const struct hdmi_platform_config hdmi_tx_8960_config = {
-		HDMI_CFG(hpd_reg, 8960),
+		HDMI_CFG(pwr_reg, 8960),
 		HDMI_CFG(hpd_clk, 8960),
 };
 
@@ -434,20 +435,6 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
 	if (hdmi->irq < 0)
 		return hdmi->irq;
 
-	hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
-				      config->hpd_reg_cnt,
-				      sizeof(hdmi->hpd_regs[0]),
-				      GFP_KERNEL);
-	if (!hdmi->hpd_regs)
-		return -ENOMEM;
-
-	for (i = 0; i < config->hpd_reg_cnt; i++)
-		hdmi->hpd_regs[i].supply = config->hpd_reg_names[i];
-
-	ret = devm_regulator_bulk_get(&pdev->dev, config->hpd_reg_cnt, hdmi->hpd_regs);
-	if (ret)
-		return dev_err_probe(dev, ret, "failed to get hpd regulators\n");
-
 	hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
 				      config->pwr_reg_cnt,
 				      sizeof(hdmi->pwr_regs[0]),
@@ -525,6 +512,48 @@ static void msm_hdmi_dev_remove(struct platform_device *pdev)
 	msm_hdmi_put_phy(hdmi);
 }
 
+static int msm_hdmi_runtime_suspend(struct device *dev)
+{
+	struct hdmi *hdmi = dev_get_drvdata(dev);
+	const struct hdmi_platform_config *config = hdmi->config;
+
+	clk_bulk_disable_unprepare(config->hpd_clk_cnt, hdmi->hpd_clks);
+
+	pinctrl_pm_select_sleep_state(dev);
+
+	regulator_bulk_disable(config->pwr_reg_cnt, hdmi->pwr_regs);
+
+	return 0;
+}
+
+static int msm_hdmi_runtime_resume(struct device *dev)
+{
+	struct hdmi *hdmi = dev_get_drvdata(dev);
+	const struct hdmi_platform_config *config = hdmi->config;
+	int ret;
+
+	ret = regulator_bulk_enable(config->pwr_reg_cnt, hdmi->pwr_regs);
+	if (ret)
+		return ret;
+
+	ret = pinctrl_pm_select_default_state(dev);
+	if (ret)
+		goto fail;
+
+	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
+	if (ret)
+		goto fail;
+
+	return 0;
+
+fail:
+	pinctrl_pm_select_sleep_state(dev);
+
+	return ret;
+}
+
+DEFINE_RUNTIME_DEV_PM_OPS(msm_hdmi_pm_ops, msm_hdmi_runtime_suspend, msm_hdmi_runtime_resume, NULL);
+
 static const struct of_device_id msm_hdmi_dt_match[] = {
 	{ .compatible = "qcom,hdmi-tx-8996", .data = &hdmi_tx_8974_config },
 	{ .compatible = "qcom,hdmi-tx-8994", .data = &hdmi_tx_8974_config },
@@ -541,6 +570,7 @@ static struct platform_driver msm_hdmi_driver = {
 	.driver = {
 		.name = "hdmi_msm",
 		.of_match_table = msm_hdmi_dt_match,
+		.pm = &msm_hdmi_pm_ops,
 	},
 };
 
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index eeba85ffef09..ee5463eb41b6 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -48,7 +48,6 @@ struct hdmi {
 	void __iomem *qfprom_mmio;
 	phys_addr_t mmio_phy_addr;
 
-	struct regulator_bulk_data *hpd_regs;
 	struct regulator_bulk_data *pwr_regs;
 	struct clk_bulk_data *hpd_clks;
 	struct clk *extp_clk;
@@ -86,11 +85,7 @@ struct hdmi {
 
 /* platform config data (ie. from DT, or pdata) */
 struct hdmi_platform_config {
-	/* regulators that need to be on for hpd: */
-	const char **hpd_reg_names;
-	int hpd_reg_cnt;
-
-	/* regulators that need to be on for screen pwr: */
+	/* regulators that need to be on: */
 	const char **pwr_reg_names;
 	int pwr_reg_cnt;
 
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index d1b35328b6e8..cddba640d292 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -16,15 +16,10 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
 	struct drm_device *dev = bridge->dev;
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
-	const struct hdmi_platform_config *config = hdmi->config;
 	int ret;
 
 	pm_runtime_resume_and_get(&hdmi->pdev->dev);
 
-	ret = regulator_bulk_enable(config->pwr_reg_cnt, hdmi->pwr_regs);
-	if (ret)
-		DRM_DEV_ERROR(dev->dev, "failed to enable pwr regulator: %d\n", ret);
-
 	if (hdmi->extp_clk) {
 		DBG("pixclock: %lu", hdmi->pixclock);
 		ret = clk_set_rate(hdmi->extp_clk, hdmi->pixclock);
@@ -39,11 +34,8 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
 
 static void power_off(struct drm_bridge *bridge)
 {
-	struct drm_device *dev = bridge->dev;
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
-	const struct hdmi_platform_config *config = hdmi->config;
-	int ret;
 
 	/* TODO do we need to wait for final vblank somewhere before
 	 * cutting the clocks?
@@ -53,10 +45,6 @@ static void power_off(struct drm_bridge *bridge)
 	if (hdmi->extp_clk)
 		clk_disable_unprepare(hdmi->extp_clk);
 
-	ret = regulator_bulk_disable(config->pwr_reg_cnt, hdmi->pwr_regs);
-	if (ret)
-		DRM_DEV_ERROR(dev->dev, "failed to disable pwr regulator: %d\n", ret);
-
 	pm_runtime_put(&hdmi->pdev->dev);
 }
 
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
index fc21ad3b01dc..32e447267e3b 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
@@ -64,36 +64,17 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
 {
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
-	const struct hdmi_platform_config *config = hdmi->config;
 	struct device *dev = &hdmi->pdev->dev;
 	uint32_t hpd_ctrl;
 	int ret;
 	unsigned long flags;
 
-	ret = regulator_bulk_enable(config->hpd_reg_cnt, hdmi->hpd_regs);
-	if (ret) {
-		DRM_DEV_ERROR(dev, "failed to enable hpd regulators: %d\n", ret);
-		goto fail;
-	}
-
-	ret = pinctrl_pm_select_default_state(dev);
-	if (ret) {
-		DRM_DEV_ERROR(dev, "pinctrl state chg failed: %d\n", ret);
-		goto fail;
-	}
-
 	if (hdmi->hpd_gpiod)
 		gpiod_set_value_cansleep(hdmi->hpd_gpiod, 1);
 
 	ret = pm_runtime_resume_and_get(dev);
-	if (ret) {
-		DRM_DEV_ERROR(dev, "runtime resume failed: %d\n", ret);
-		goto fail;
-	}
-
-	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
 	if (ret)
-		goto fail;
+		return ret;
 
 	msm_hdmi_set_mode(hdmi, false);
 	msm_hdmi_phy_reset(hdmi);
@@ -119,32 +100,18 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
 	spin_unlock_irqrestore(&hdmi->reg_lock, flags);
 
 	return 0;
-
-fail:
-	return ret;
 }
 
 void msm_hdmi_hpd_disable(struct hdmi *hdmi)
 {
-	const struct hdmi_platform_config *config = hdmi->config;
 	struct device *dev = &hdmi->pdev->dev;
-	int ret;
 
 	/* Disable HPD interrupt */
 	hdmi_write(hdmi, REG_HDMI_HPD_INT_CTRL, 0);
 
 	msm_hdmi_set_mode(hdmi, false);
 
-	clk_bulk_disable_unprepare(config->hpd_clk_cnt, hdmi->hpd_clks);
 	pm_runtime_put(dev);
-
-	ret = pinctrl_pm_select_sleep_state(dev);
-	if (ret)
-		dev_warn(dev, "pinctrl state chg failed: %d\n", ret);
-
-	ret = regulator_bulk_disable(config->hpd_reg_cnt, hdmi->hpd_regs);
-	if (ret)
-		dev_warn(dev, "failed to disable hpd regulator: %d\n", ret);
 }
 
 void msm_hdmi_hpd_irq(struct drm_bridge *bridge)
@@ -179,7 +146,6 @@ void msm_hdmi_hpd_irq(struct drm_bridge *bridge)
 
 static enum drm_connector_status detect_reg(struct hdmi *hdmi)
 {
-	const struct hdmi_platform_config *config = hdmi->config;
 	uint32_t hpd_int_status = 0;
 	int ret;
 
@@ -187,14 +153,8 @@ static enum drm_connector_status detect_reg(struct hdmi *hdmi)
 	if (ret)
 		goto out;
 
-	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
-	if (ret)
-		goto out;
-
 	hpd_int_status = hdmi_read(hdmi, REG_HDMI_HPD_INT_STATUS);
 
-	clk_bulk_disable_unprepare(config->hpd_clk_cnt, hdmi->hpd_clks);
-
 out:
 	pm_runtime_put(&hdmi->pdev->dev);
 

-- 
2.39.2

Re: [PATCH v2 09/14] drm/msm/hdmi: implement proper runtime PM handling

[Jessica Zhang]

On 5/22/2024 3:51 AM, Dmitry Baryshkov wrote:
> It is completely not obvious, but the so-called 'hpd' clocks and
> regulators are required for the HDMI host to function properly. Merge
> pwr and hpd regulators. Use regulators, clocks and pinctrl to implement
> proper runtime PM callbacks.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c        | 62 +++++++++++++++++++++++++---------
>   drivers/gpu/drm/msm/hdmi/hdmi.h        |  7 +---
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 12 -------
>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 42 +----------------------
>   4 files changed, 48 insertions(+), 75 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 7ec4ca3b7597..cc671baad87b 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -8,6 +8,7 @@
>   #include <linux/gpio/consumer.h>
>   #include <linux/of_irq.h>
>   #include <linux/of_platform.h>
> +#include <linux/pinctrl/consumer.h>
>   #include <linux/platform_device.h>
>   
>   #include <drm/drm_bridge_connector.h>
> @@ -226,11 +227,11 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   	.item ## _names = item ##_names_ ## entry, \
>   	.item ## _cnt   = ARRAY_SIZE(item ## _names_ ## entry)
>   
> -static const char *hpd_reg_names_8960[] = {"core-vdda"};
> +static const char *pwr_reg_names_8960[] = {"core-vdda"};
>   static const char *hpd_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
>   
>   static const struct hdmi_platform_config hdmi_tx_8960_config = {
> -		HDMI_CFG(hpd_reg, 8960),
> +		HDMI_CFG(pwr_reg, 8960),
>   		HDMI_CFG(hpd_clk, 8960),
>   };
>   
> @@ -434,20 +435,6 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   	if (hdmi->irq < 0)
>   		return hdmi->irq;
>   
> -	hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
> -				      config->hpd_reg_cnt,
> -				      sizeof(hdmi->hpd_regs[0]),
> -				      GFP_KERNEL);
> -	if (!hdmi->hpd_regs)
> -		return -ENOMEM;
> -
> -	for (i = 0; i < config->hpd_reg_cnt; i++)
> -		hdmi->hpd_regs[i].supply = config->hpd_reg_names[i];
> -
> -	ret = devm_regulator_bulk_get(&pdev->dev, config->hpd_reg_cnt, hdmi->hpd_regs);
> -	if (ret)
> -		return dev_err_probe(dev, ret, "failed to get hpd regulators\n");
> -
>   	hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
>   				      config->pwr_reg_cnt,
>   				      sizeof(hdmi->pwr_regs[0]),
> @@ -525,6 +512,48 @@ static void msm_hdmi_dev_remove(struct platform_device *pdev)
>   	msm_hdmi_put_phy(hdmi);
>   }
>   
> +static int msm_hdmi_runtime_suspend(struct device *dev)
> +{
> +	struct hdmi *hdmi = dev_get_drvdata(dev);
> +	const struct hdmi_platform_config *config = hdmi->config;
> +
> +	clk_bulk_disable_unprepare(config->hpd_clk_cnt, hdmi->hpd_clks);
> +
> +	pinctrl_pm_select_sleep_state(dev);
> +
> +	regulator_bulk_disable(config->pwr_reg_cnt, hdmi->pwr_regs);
> +
> +	return 0;
> +}
> +
> +static int msm_hdmi_runtime_resume(struct device *dev)
> +{
> +	struct hdmi *hdmi = dev_get_drvdata(dev);
> +	const struct hdmi_platform_config *config = hdmi->config;
> +	int ret;
> +
> +	ret = regulator_bulk_enable(config->pwr_reg_cnt, hdmi->pwr_regs);
> +	if (ret)
> +		return ret;
> +
> +	ret = pinctrl_pm_select_default_state(dev);
> +	if (ret)
> +		goto fail;
> +
> +	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
> +	if (ret)
> +		goto fail;
> +
> +	return 0;
> +
> +fail:
> +	pinctrl_pm_select_sleep_state(dev);
> +
> +	return ret;
> +}
> +
> +DEFINE_RUNTIME_DEV_PM_OPS(msm_hdmi_pm_ops, msm_hdmi_runtime_suspend, msm_hdmi_runtime_resume, NULL);
> +
>   static const struct of_device_id msm_hdmi_dt_match[] = {
>   	{ .compatible = "qcom,hdmi-tx-8996", .data = &hdmi_tx_8974_config },
>   	{ .compatible = "qcom,hdmi-tx-8994", .data = &hdmi_tx_8974_config },
> @@ -541,6 +570,7 @@ static struct platform_driver msm_hdmi_driver = {
>   	.driver = {
>   		.name = "hdmi_msm",
>   		.of_match_table = msm_hdmi_dt_match,
> +		.pm = &msm_hdmi_pm_ops,
>   	},
>   };
>   
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index eeba85ffef09..ee5463eb41b6 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -48,7 +48,6 @@ struct hdmi {
>   	void __iomem *qfprom_mmio;
>   	phys_addr_t mmio_phy_addr;
>   
> -	struct regulator_bulk_data *hpd_regs;
>   	struct regulator_bulk_data *pwr_regs;
>   	struct clk_bulk_data *hpd_clks;
>   	struct clk *extp_clk;
> @@ -86,11 +85,7 @@ struct hdmi {
>   
>   /* platform config data (ie. from DT, or pdata) */
>   struct hdmi_platform_config {
> -	/* regulators that need to be on for hpd: */
> -	const char **hpd_reg_names;
> -	int hpd_reg_cnt;
> -
> -	/* regulators that need to be on for screen pwr: */
> +	/* regulators that need to be on: */
>   	const char **pwr_reg_names;
>   	int pwr_reg_cnt;
>   
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index d1b35328b6e8..cddba640d292 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -16,15 +16,10 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
>   	struct drm_device *dev = bridge->dev;
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
> -	const struct hdmi_platform_config *config = hdmi->config;
>   	int ret;
>   
>   	pm_runtime_resume_and_get(&hdmi->pdev->dev);
>   
> -	ret = regulator_bulk_enable(config->pwr_reg_cnt, hdmi->pwr_regs);
> -	if (ret)
> -		DRM_DEV_ERROR(dev->dev, "failed to enable pwr regulator: %d\n", ret);
> -
>   	if (hdmi->extp_clk) {
>   		DBG("pixclock: %lu", hdmi->pixclock);
>   		ret = clk_set_rate(hdmi->extp_clk, hdmi->pixclock);
> @@ -39,11 +34,8 @@ static void msm_hdmi_power_on(struct drm_bridge *bridge)
>   
>   static void power_off(struct drm_bridge *bridge)
>   {
> -	struct drm_device *dev = bridge->dev;
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
> -	const struct hdmi_platform_config *config = hdmi->config;
> -	int ret;
>   
>   	/* TODO do we need to wait for final vblank somewhere before
>   	 * cutting the clocks?
> @@ -53,10 +45,6 @@ static void power_off(struct drm_bridge *bridge)
>   	if (hdmi->extp_clk)
>   		clk_disable_unprepare(hdmi->extp_clk);
>   
> -	ret = regulator_bulk_disable(config->pwr_reg_cnt, hdmi->pwr_regs);
> -	if (ret)
> -		DRM_DEV_ERROR(dev->dev, "failed to disable pwr regulator: %d\n", ret);
> -
>   	pm_runtime_put(&hdmi->pdev->dev);
>   }
>   
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> index fc21ad3b01dc..32e447267e3b 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> @@ -64,36 +64,17 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>   {
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
> -	const struct hdmi_platform_config *config = hdmi->config;
>   	struct device *dev = &hdmi->pdev->dev;
>   	uint32_t hpd_ctrl;
>   	int ret;
>   	unsigned long flags;
>   
> -	ret = regulator_bulk_enable(config->hpd_reg_cnt, hdmi->hpd_regs);
> -	if (ret) {
> -		DRM_DEV_ERROR(dev, "failed to enable hpd regulators: %d\n", ret);
> -		goto fail;
> -	}
> -
> -	ret = pinctrl_pm_select_default_state(dev);
> -	if (ret) {
> -		DRM_DEV_ERROR(dev, "pinctrl state chg failed: %d\n", ret);
> -		goto fail;
> -	}
> -
>   	if (hdmi->hpd_gpiod)
>   		gpiod_set_value_cansleep(hdmi->hpd_gpiod, 1);
>   
>   	ret = pm_runtime_resume_and_get(dev);
> -	if (ret) {
> -		DRM_DEV_ERROR(dev, "runtime resume failed: %d\n", ret);
> -		goto fail;
> -	}
> -
> -	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
>   	if (ret)
> -		goto fail;
> +		return ret;
>   
>   	msm_hdmi_set_mode(hdmi, false);
>   	msm_hdmi_phy_reset(hdmi);
> @@ -119,32 +100,18 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>   	spin_unlock_irqrestore(&hdmi->reg_lock, flags);
>   
>   	return 0;
> -
> -fail:
> -	return ret;
>   }
>   
>   void msm_hdmi_hpd_disable(struct hdmi *hdmi)
>   {
> -	const struct hdmi_platform_config *config = hdmi->config;
>   	struct device *dev = &hdmi->pdev->dev;
> -	int ret;
>   
>   	/* Disable HPD interrupt */
>   	hdmi_write(hdmi, REG_HDMI_HPD_INT_CTRL, 0);
>   
>   	msm_hdmi_set_mode(hdmi, false);
>   
> -	clk_bulk_disable_unprepare(config->hpd_clk_cnt, hdmi->hpd_clks);
>   	pm_runtime_put(dev);
> -
> -	ret = pinctrl_pm_select_sleep_state(dev);
> -	if (ret)
> -		dev_warn(dev, "pinctrl state chg failed: %d\n", ret);
> -
> -	ret = regulator_bulk_disable(config->hpd_reg_cnt, hdmi->hpd_regs);
> -	if (ret)
> -		dev_warn(dev, "failed to disable hpd regulator: %d\n", ret);
>   }
>   
>   void msm_hdmi_hpd_irq(struct drm_bridge *bridge)
> @@ -179,7 +146,6 @@ void msm_hdmi_hpd_irq(struct drm_bridge *bridge)
>   
>   static enum drm_connector_status detect_reg(struct hdmi *hdmi)
>   {
> -	const struct hdmi_platform_config *config = hdmi->config;
>   	uint32_t hpd_int_status = 0;
>   	int ret;
>   
> @@ -187,14 +153,8 @@ static enum drm_connector_status detect_reg(struct hdmi *hdmi)
>   	if (ret)
>   		goto out;
>   
> -	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
> -	if (ret)
> -		goto out;
> -
>   	hpd_int_status = hdmi_read(hdmi, REG_HDMI_HPD_INT_STATUS);
>   
> -	clk_bulk_disable_unprepare(config->hpd_clk_cnt, hdmi->hpd_clks);
> -
>   out:
>   	pm_runtime_put(&hdmi->pdev->dev);
>   
> 
> -- 
> 2.39.2
> 

[PATCH v2 10/14] drm/msm/hdmi: rename hpd_clks to pwr_clks

[Dmitry Baryshkov]

As these clocks are now used in the runtime PM callbacks, they have no
connection to 'HPD'. Rename corresponding fields to follow clocks
purpose, to power up the HDMI controller.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c | 26 +++++++++++++-------------
 drivers/gpu/drm/msm/hdmi/hdmi.h |  6 +++---
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index cc671baad87b..c39a1f3a7505 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -228,19 +228,19 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 	.item ## _cnt   = ARRAY_SIZE(item ## _names_ ## entry)
 
 static const char *pwr_reg_names_8960[] = {"core-vdda"};
-static const char *hpd_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
+static const char *pwr_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
 
 static const struct hdmi_platform_config hdmi_tx_8960_config = {
 		HDMI_CFG(pwr_reg, 8960),
-		HDMI_CFG(hpd_clk, 8960),
+		HDMI_CFG(pwr_clk, 8960),
 };
 
 static const char *pwr_reg_names_8x74[] = {"core-vdda", "core-vcc"};
-static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
+static const char *pwr_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
 
 static const struct hdmi_platform_config hdmi_tx_8974_config = {
 		HDMI_CFG(pwr_reg, 8x74),
-		HDMI_CFG(hpd_clk, 8x74),
+		HDMI_CFG(pwr_clk, 8x74),
 };
 
 /*
@@ -449,17 +449,17 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
 	if (ret)
 		return dev_err_probe(dev, ret, "failed to get pwr regulators\n");
 
-	hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
-				      config->hpd_clk_cnt,
-				      sizeof(hdmi->hpd_clks[0]),
+	hdmi->pwr_clks = devm_kcalloc(&pdev->dev,
+				      config->pwr_clk_cnt,
+				      sizeof(hdmi->pwr_clks[0]),
 				      GFP_KERNEL);
-	if (!hdmi->hpd_clks)
+	if (!hdmi->pwr_clks)
 		return -ENOMEM;
 
-	for (i = 0; i < config->hpd_clk_cnt; i++)
-		hdmi->hpd_clks[i].id = config->hpd_clk_names[i];
+	for (i = 0; i < config->pwr_clk_cnt; i++)
+		hdmi->pwr_clks[i].id = config->pwr_clk_names[i];
 
-	ret = devm_clk_bulk_get(&pdev->dev, config->hpd_clk_cnt, hdmi->hpd_clks);
+	ret = devm_clk_bulk_get(&pdev->dev, config->pwr_clk_cnt, hdmi->pwr_clks);
 	if (ret)
 		return ret;
 
@@ -517,7 +517,7 @@ static int msm_hdmi_runtime_suspend(struct device *dev)
 	struct hdmi *hdmi = dev_get_drvdata(dev);
 	const struct hdmi_platform_config *config = hdmi->config;
 
-	clk_bulk_disable_unprepare(config->hpd_clk_cnt, hdmi->hpd_clks);
+	clk_bulk_disable_unprepare(config->pwr_clk_cnt, hdmi->pwr_clks);
 
 	pinctrl_pm_select_sleep_state(dev);
 
@@ -540,7 +540,7 @@ static int msm_hdmi_runtime_resume(struct device *dev)
 	if (ret)
 		goto fail;
 
-	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
+	ret = clk_bulk_prepare_enable(config->pwr_clk_cnt, hdmi->pwr_clks);
 	if (ret)
 		goto fail;
 
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index ee5463eb41b6..1e346e697f8e 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -49,7 +49,7 @@ struct hdmi {
 	phys_addr_t mmio_phy_addr;
 
 	struct regulator_bulk_data *pwr_regs;
-	struct clk_bulk_data *hpd_clks;
+	struct clk_bulk_data *pwr_clks;
 	struct clk *extp_clk;
 
 	struct gpio_desc *hpd_gpiod;
@@ -90,8 +90,8 @@ struct hdmi_platform_config {
 	int pwr_reg_cnt;
 
 	/* clks that need to be on for hpd: */
-	const char **hpd_clk_names;
-	int hpd_clk_cnt;
+	const char **pwr_clk_names;
+	int pwr_clk_cnt;
 };
 
 struct hdmi_bridge {

-- 
2.39.2

Re: [PATCH v2 10/14] drm/msm/hdmi: rename hpd_clks to pwr_clks

[Jessica Zhang]

On 5/22/2024 3:51 AM, Dmitry Baryshkov wrote:
> As these clocks are now used in the runtime PM callbacks, they have no
> connection to 'HPD'. Rename corresponding fields to follow clocks
> purpose, to power up the HDMI controller.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c | 26 +++++++++++++-------------
>   drivers/gpu/drm/msm/hdmi/hdmi.h |  6 +++---
>   2 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index cc671baad87b..c39a1f3a7505 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -228,19 +228,19 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   	.item ## _cnt   = ARRAY_SIZE(item ## _names_ ## entry)
>   
>   static const char *pwr_reg_names_8960[] = {"core-vdda"};
> -static const char *hpd_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
> +static const char *pwr_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
>   
>   static const struct hdmi_platform_config hdmi_tx_8960_config = {
>   		HDMI_CFG(pwr_reg, 8960),
> -		HDMI_CFG(hpd_clk, 8960),
> +		HDMI_CFG(pwr_clk, 8960),
>   };
>   
>   static const char *pwr_reg_names_8x74[] = {"core-vdda", "core-vcc"};
> -static const char *hpd_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
> +static const char *pwr_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
>   
>   static const struct hdmi_platform_config hdmi_tx_8974_config = {
>   		HDMI_CFG(pwr_reg, 8x74),
> -		HDMI_CFG(hpd_clk, 8x74),
> +		HDMI_CFG(pwr_clk, 8x74),
>   };
>   
>   /*
> @@ -449,17 +449,17 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   	if (ret)
>   		return dev_err_probe(dev, ret, "failed to get pwr regulators\n");
>   
> -	hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
> -				      config->hpd_clk_cnt,
> -				      sizeof(hdmi->hpd_clks[0]),
> +	hdmi->pwr_clks = devm_kcalloc(&pdev->dev,
> +				      config->pwr_clk_cnt,
> +				      sizeof(hdmi->pwr_clks[0]),
>   				      GFP_KERNEL);
> -	if (!hdmi->hpd_clks)
> +	if (!hdmi->pwr_clks)
>   		return -ENOMEM;
>   
> -	for (i = 0; i < config->hpd_clk_cnt; i++)
> -		hdmi->hpd_clks[i].id = config->hpd_clk_names[i];
> +	for (i = 0; i < config->pwr_clk_cnt; i++)
> +		hdmi->pwr_clks[i].id = config->pwr_clk_names[i];
>   
> -	ret = devm_clk_bulk_get(&pdev->dev, config->hpd_clk_cnt, hdmi->hpd_clks);
> +	ret = devm_clk_bulk_get(&pdev->dev, config->pwr_clk_cnt, hdmi->pwr_clks);
>   	if (ret)
>   		return ret;
>   
> @@ -517,7 +517,7 @@ static int msm_hdmi_runtime_suspend(struct device *dev)
>   	struct hdmi *hdmi = dev_get_drvdata(dev);
>   	const struct hdmi_platform_config *config = hdmi->config;
>   
> -	clk_bulk_disable_unprepare(config->hpd_clk_cnt, hdmi->hpd_clks);
> +	clk_bulk_disable_unprepare(config->pwr_clk_cnt, hdmi->pwr_clks);
>   
>   	pinctrl_pm_select_sleep_state(dev);
>   
> @@ -540,7 +540,7 @@ static int msm_hdmi_runtime_resume(struct device *dev)
>   	if (ret)
>   		goto fail;
>   
> -	ret = clk_bulk_prepare_enable(config->hpd_clk_cnt, hdmi->hpd_clks);
> +	ret = clk_bulk_prepare_enable(config->pwr_clk_cnt, hdmi->pwr_clks);
>   	if (ret)
>   		goto fail;
>   
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index ee5463eb41b6..1e346e697f8e 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -49,7 +49,7 @@ struct hdmi {
>   	phys_addr_t mmio_phy_addr;
>   
>   	struct regulator_bulk_data *pwr_regs;
> -	struct clk_bulk_data *hpd_clks;
> +	struct clk_bulk_data *pwr_clks;
>   	struct clk *extp_clk;
>   
>   	struct gpio_desc *hpd_gpiod;
> @@ -90,8 +90,8 @@ struct hdmi_platform_config {
>   	int pwr_reg_cnt;
>   
>   	/* clks that need to be on for hpd: */
> -	const char **hpd_clk_names;
> -	int hpd_clk_cnt;
> +	const char **pwr_clk_names;
> +	int pwr_clk_cnt;
>   };
>   
>   struct hdmi_bridge {
> 
> -- 
> 2.39.2
> 

[PATCH v2 11/14] drm/msm/hdmi: expand the HDMI_CFG macro

[Dmitry Baryshkov]

Expand the HDMI_CFG() macro in HDMI config description. It has no added
value other than hiding some boilerplate declarations.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c | 16 ++++++++--------
 drivers/gpu/drm/msm/hdmi/hdmi.h |  2 +-
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index c39a1f3a7505..e160a23e962e 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -223,24 +223,24 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
  * The hdmi device:
  */
 
-#define HDMI_CFG(item, entry) \
-	.item ## _names = item ##_names_ ## entry, \
-	.item ## _cnt   = ARRAY_SIZE(item ## _names_ ## entry)
-
 static const char *pwr_reg_names_8960[] = {"core-vdda"};
 static const char *pwr_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
 
 static const struct hdmi_platform_config hdmi_tx_8960_config = {
-		HDMI_CFG(pwr_reg, 8960),
-		HDMI_CFG(pwr_clk, 8960),
+	.pwr_reg_names = pwr_reg_names_8960,
+	.pwr_reg_cnt = ARRAY_SIZE(pwr_reg_names_8960),
+	.pwr_clk_names = pwr_clk_names_8960,
+	.pwr_clk_cnt = ARRAY_SIZE(pwr_clk_names_8960),
 };
 
 static const char *pwr_reg_names_8x74[] = {"core-vdda", "core-vcc"};
 static const char *pwr_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
 
 static const struct hdmi_platform_config hdmi_tx_8974_config = {
-		HDMI_CFG(pwr_reg, 8x74),
-		HDMI_CFG(pwr_clk, 8x74),
+	.pwr_reg_names = pwr_reg_names_8x74,
+	.pwr_reg_cnt = ARRAY_SIZE(pwr_reg_names_8x74),
+	.pwr_clk_names = pwr_clk_names_8x74,
+	.pwr_clk_cnt = ARRAY_SIZE(pwr_clk_names_8x74),
 };
 
 /*
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 1e346e697f8e..2a98efa8b6bd 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -89,7 +89,7 @@ struct hdmi_platform_config {
 	const char **pwr_reg_names;
 	int pwr_reg_cnt;
 
-	/* clks that need to be on for hpd: */
+	/* clks that need to be on: */
 	const char **pwr_clk_names;
 	int pwr_clk_cnt;
 };

-- 
2.39.2

Re: [PATCH v2 11/14] drm/msm/hdmi: expand the HDMI_CFG macro

[Jessica Zhang]

On 5/22/2024 3:51 AM, Dmitry Baryshkov wrote:
> Expand the HDMI_CFG() macro in HDMI config description. It has no added
> value other than hiding some boilerplate declarations.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c | 16 ++++++++--------
>   drivers/gpu/drm/msm/hdmi/hdmi.h |  2 +-
>   2 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index c39a1f3a7505..e160a23e962e 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -223,24 +223,24 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>    * The hdmi device:
>    */
>   
> -#define HDMI_CFG(item, entry) \
> -	.item ## _names = item ##_names_ ## entry, \
> -	.item ## _cnt   = ARRAY_SIZE(item ## _names_ ## entry)
> -
>   static const char *pwr_reg_names_8960[] = {"core-vdda"};
>   static const char *pwr_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
>   
>   static const struct hdmi_platform_config hdmi_tx_8960_config = {
> -		HDMI_CFG(pwr_reg, 8960),
> -		HDMI_CFG(pwr_clk, 8960),
> +	.pwr_reg_names = pwr_reg_names_8960,
> +	.pwr_reg_cnt = ARRAY_SIZE(pwr_reg_names_8960),
> +	.pwr_clk_names = pwr_clk_names_8960,
> +	.pwr_clk_cnt = ARRAY_SIZE(pwr_clk_names_8960),
>   };
>   
>   static const char *pwr_reg_names_8x74[] = {"core-vdda", "core-vcc"};
>   static const char *pwr_clk_names_8x74[] = {"iface", "core", "mdp_core", "alt_iface"};
>   
>   static const struct hdmi_platform_config hdmi_tx_8974_config = {
> -		HDMI_CFG(pwr_reg, 8x74),
> -		HDMI_CFG(pwr_clk, 8x74),
> +	.pwr_reg_names = pwr_reg_names_8x74,
> +	.pwr_reg_cnt = ARRAY_SIZE(pwr_reg_names_8x74),
> +	.pwr_clk_names = pwr_clk_names_8x74,
> +	.pwr_clk_cnt = ARRAY_SIZE(pwr_clk_names_8x74),
>   };
>   
>   /*
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index 1e346e697f8e..2a98efa8b6bd 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -89,7 +89,7 @@ struct hdmi_platform_config {
>   	const char **pwr_reg_names;
>   	int pwr_reg_cnt;
>   
> -	/* clks that need to be on for hpd: */
> +	/* clks that need to be on: */
>   	const char **pwr_clk_names;
>   	int pwr_clk_cnt;
>   };
> 
> -- 
> 2.39.2
> 

[PATCH v2 12/14] drm/msm/hdmi: drop hpd-gpios support

[Dmitry Baryshkov]

Supporting simultaneous check of native HPD and the external GPIO proved
to be less stable than just native HPD. Drop the hpd-gpios support,
leaving just the native HPD support. In case the native HPD doesn't work
the user is urged to switch to specifying the HPD property to the
hdmi-connector device.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c     | 14 +++-------
 drivers/gpu/drm/msm/hdmi/hdmi.h     |  2 --
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c | 53 +++----------------------------------
 3 files changed, 7 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index e160a23e962e..a9437054c015 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -468,17 +468,9 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, PTR_ERR(hdmi->extp_clk),
 				     "failed to get extp clock\n");
 
-	hdmi->hpd_gpiod = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN);
-	/* This will catch e.g. -EPROBE_DEFER */
-	if (IS_ERR(hdmi->hpd_gpiod))
-		return dev_err_probe(dev, PTR_ERR(hdmi->hpd_gpiod),
-				     "failed to get hpd gpio\n");
-
-	if (!hdmi->hpd_gpiod)
-		DBG("failed to get HPD gpio");
-
-	if (hdmi->hpd_gpiod)
-		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
+	if (of_find_property(dev->of_node, "hpd-gpios", NULL) ||
+	    of_find_property(dev->of_node, "hpd-gpio", NULL))
+		dev_warn(dev, "hpd-gpios is not supported anymore, please migrate to the hdmi-connector\n");
 
 	ret = msm_hdmi_get_phy(hdmi);
 	if (ret) {
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 2a98efa8b6bd..268ff8604423 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -52,8 +52,6 @@ struct hdmi {
 	struct clk_bulk_data *pwr_clks;
 	struct clk *extp_clk;
 
-	struct gpio_desc *hpd_gpiod;
-
 	struct hdmi_phy *phy;
 	struct device *phy_dev;
 
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
index 32e447267e3b..d3353c6148ed 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
@@ -69,9 +69,6 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
 	int ret;
 	unsigned long flags;
 
-	if (hdmi->hpd_gpiod)
-		gpiod_set_value_cansleep(hdmi->hpd_gpiod, 1);
-
 	ret = pm_runtime_resume_and_get(dev);
 	if (ret)
 		return ret;
@@ -144,8 +141,11 @@ void msm_hdmi_hpd_irq(struct drm_bridge *bridge)
 	}
 }
 
-static enum drm_connector_status detect_reg(struct hdmi *hdmi)
+enum drm_connector_status msm_hdmi_bridge_detect(
+		struct drm_bridge *bridge)
 {
+	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
+	struct hdmi *hdmi = hdmi_bridge->hdmi;
 	uint32_t hpd_int_status = 0;
 	int ret;
 
@@ -161,48 +161,3 @@ static enum drm_connector_status detect_reg(struct hdmi *hdmi)
 	return (hpd_int_status & HDMI_HPD_INT_STATUS_CABLE_DETECTED) ?
 			connector_status_connected : connector_status_disconnected;
 }
-
-#define HPD_GPIO_INDEX	2
-static enum drm_connector_status detect_gpio(struct hdmi *hdmi)
-{
-	return gpiod_get_value(hdmi->hpd_gpiod) ?
-			connector_status_connected :
-			connector_status_disconnected;
-}
-
-enum drm_connector_status msm_hdmi_bridge_detect(
-		struct drm_bridge *bridge)
-{
-	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
-	struct hdmi *hdmi = hdmi_bridge->hdmi;
-	enum drm_connector_status stat_gpio, stat_reg;
-	int retry = 20;
-
-	/*
-	 * some platforms may not have hpd gpio. Rely only on the status
-	 * provided by REG_HDMI_HPD_INT_STATUS in this case.
-	 */
-	if (!hdmi->hpd_gpiod)
-		return detect_reg(hdmi);
-
-	do {
-		stat_gpio = detect_gpio(hdmi);
-		stat_reg  = detect_reg(hdmi);
-
-		if (stat_gpio == stat_reg)
-			break;
-
-		mdelay(10);
-	} while (--retry);
-
-	/* the status we get from reading gpio seems to be more reliable,
-	 * so trust that one the most if we didn't manage to get hdmi and
-	 * gpio status to agree:
-	 */
-	if (stat_gpio != stat_reg) {
-		DBG("HDMI_HPD_INT_STATUS tells us: %d", stat_reg);
-		DBG("hpd gpio tells us: %d", stat_gpio);
-	}
-
-	return stat_gpio;
-}

-- 
2.39.2

Re: [PATCH v2 12/14] drm/msm/hdmi: drop hpd-gpios support

[Jessica Zhang]

On 5/22/2024 3:51 AM, Dmitry Baryshkov wrote:
> Supporting simultaneous check of native HPD and the external GPIO proved
> to be less stable than just native HPD. Drop the hpd-gpios support,
> leaving just the native HPD support. In case the native HPD doesn't work
> the user is urged to switch to specifying the HPD property to the
> hdmi-connector device.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c     | 14 +++-------
>   drivers/gpu/drm/msm/hdmi/hdmi.h     |  2 --
>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c | 53 +++----------------------------------
>   3 files changed, 7 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index e160a23e962e..a9437054c015 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -468,17 +468,9 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   		return dev_err_probe(dev, PTR_ERR(hdmi->extp_clk),
>   				     "failed to get extp clock\n");
>   
> -	hdmi->hpd_gpiod = devm_gpiod_get_optional(&pdev->dev, "hpd", GPIOD_IN);
> -	/* This will catch e.g. -EPROBE_DEFER */
> -	if (IS_ERR(hdmi->hpd_gpiod))
> -		return dev_err_probe(dev, PTR_ERR(hdmi->hpd_gpiod),
> -				     "failed to get hpd gpio\n");
> -
> -	if (!hdmi->hpd_gpiod)
> -		DBG("failed to get HPD gpio");
> -
> -	if (hdmi->hpd_gpiod)
> -		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
> +	if (of_find_property(dev->of_node, "hpd-gpios", NULL) ||
> +	    of_find_property(dev->of_node, "hpd-gpio", NULL))
> +		dev_warn(dev, "hpd-gpios is not supported anymore, please migrate to the hdmi-connector\n");
>   
>   	ret = msm_hdmi_get_phy(hdmi);
>   	if (ret) {
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index 2a98efa8b6bd..268ff8604423 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -52,8 +52,6 @@ struct hdmi {
>   	struct clk_bulk_data *pwr_clks;
>   	struct clk *extp_clk;
>   
> -	struct gpio_desc *hpd_gpiod;
> -
>   	struct hdmi_phy *phy;
>   	struct device *phy_dev;
>   
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> index 32e447267e3b..d3353c6148ed 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> @@ -69,9 +69,6 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>   	int ret;
>   	unsigned long flags;
>   
> -	if (hdmi->hpd_gpiod)
> -		gpiod_set_value_cansleep(hdmi->hpd_gpiod, 1);
> -
>   	ret = pm_runtime_resume_and_get(dev);
>   	if (ret)
>   		return ret;
> @@ -144,8 +141,11 @@ void msm_hdmi_hpd_irq(struct drm_bridge *bridge)
>   	}
>   }
>   
> -static enum drm_connector_status detect_reg(struct hdmi *hdmi)
> +enum drm_connector_status msm_hdmi_bridge_detect(
> +		struct drm_bridge *bridge)
>   {
> +	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
> +	struct hdmi *hdmi = hdmi_bridge->hdmi;
>   	uint32_t hpd_int_status = 0;
>   	int ret;
>   
> @@ -161,48 +161,3 @@ static enum drm_connector_status detect_reg(struct hdmi *hdmi)
>   	return (hpd_int_status & HDMI_HPD_INT_STATUS_CABLE_DETECTED) ?
>   			connector_status_connected : connector_status_disconnected;
>   }
> -
> -#define HPD_GPIO_INDEX	2
> -static enum drm_connector_status detect_gpio(struct hdmi *hdmi)
> -{
> -	return gpiod_get_value(hdmi->hpd_gpiod) ?
> -			connector_status_connected :
> -			connector_status_disconnected;
> -}
> -
> -enum drm_connector_status msm_hdmi_bridge_detect(
> -		struct drm_bridge *bridge)
> -{
> -	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
> -	struct hdmi *hdmi = hdmi_bridge->hdmi;
> -	enum drm_connector_status stat_gpio, stat_reg;
> -	int retry = 20;
> -
> -	/*
> -	 * some platforms may not have hpd gpio. Rely only on the status
> -	 * provided by REG_HDMI_HPD_INT_STATUS in this case.
> -	 */
> -	if (!hdmi->hpd_gpiod)
> -		return detect_reg(hdmi);
> -
> -	do {
> -		stat_gpio = detect_gpio(hdmi);
> -		stat_reg  = detect_reg(hdmi);
> -
> -		if (stat_gpio == stat_reg)
> -			break;
> -
> -		mdelay(10);
> -	} while (--retry);
> -
> -	/* the status we get from reading gpio seems to be more reliable,
> -	 * so trust that one the most if we didn't manage to get hdmi and
> -	 * gpio status to agree:
> -	 */
> -	if (stat_gpio != stat_reg) {
> -		DBG("HDMI_HPD_INT_STATUS tells us: %d", stat_reg);
> -		DBG("hpd gpio tells us: %d", stat_gpio);
> -	}
> -
> -	return stat_gpio;
> -}
> 
> -- 
> 2.39.2
> 

[PATCH v2 13/14] drm/msm/hdmi: ensure that HDMI is one if HPD is requested

[Dmitry Baryshkov]

The HDMI block needs to be enabled to properly generate HPD events. Make
sure it is not turned off in the disable paths if HPD delivery is enabled.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c        | 1 +
 drivers/gpu/drm/msm/hdmi/hdmi.h        | 2 ++
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 8 +++++++-
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 9 ++++++++-
 4 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index a9437054c015..2890196857f8 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -409,6 +409,7 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
 	hdmi->pdev = pdev;
 	hdmi->config = config;
 	spin_lock_init(&hdmi->reg_lock);
+	mutex_init(&hdmi->state_mutex);
 
 	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, &hdmi->next_bridge);
 	if (ret && ret != -ENODEV)
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 268ff8604423..7f0ca5252018 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -42,6 +42,8 @@ struct hdmi {
 
 	/* video state: */
 	bool power_on;
+	bool hpd_enabled;
+	struct mutex state_mutex; /* protects two booleans */
 	unsigned long int pixclock;
 
 	void __iomem *mmio;
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index cddba640d292..104107ed47d0 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -117,11 +117,13 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 
 	DBG("power up");
 
+	mutex_lock(&hdmi->state_mutex);
 	if (!hdmi->power_on) {
 		msm_hdmi_phy_resource_enable(phy);
 		msm_hdmi_power_on(bridge);
 		hdmi->power_on = true;
 	}
+	mutex_unlock(&hdmi->state_mutex);
 
 	if (hdmi->hdmi_mode) {
 		msm_hdmi_config_avi_infoframe(hdmi);
@@ -147,7 +149,10 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
 		msm_hdmi_hdcp_off(hdmi->hdcp_ctrl);
 
 	DBG("power down");
-	msm_hdmi_set_mode(hdmi, false);
+
+	/* Keep the HDMI enabled if the HPD is enabled */
+	mutex_lock(&hdmi->state_mutex);
+	msm_hdmi_set_mode(hdmi, hdmi->hpd_enabled);
 
 	msm_hdmi_phy_powerdown(phy);
 
@@ -158,6 +163,7 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
 			msm_hdmi_audio_update(hdmi);
 		msm_hdmi_phy_resource_disable(phy);
 	}
+	mutex_unlock(&hdmi->state_mutex);
 }
 
 static void msm_hdmi_bridge_mode_set(struct drm_bridge *bridge,
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
index d3353c6148ed..cb89e9e2c6ea 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
@@ -73,10 +73,14 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
 	if (ret)
 		return ret;
 
+	mutex_lock(&hdmi->state_mutex);
 	msm_hdmi_set_mode(hdmi, false);
 	msm_hdmi_phy_reset(hdmi);
 	msm_hdmi_set_mode(hdmi, true);
 
+	hdmi->hpd_enabled = true;
+	mutex_unlock(&hdmi->state_mutex);
+
 	hdmi_write(hdmi, REG_HDMI_USEC_REFTIMER, 0x0001001b);
 
 	/* enable HPD events: */
@@ -106,7 +110,10 @@ void msm_hdmi_hpd_disable(struct hdmi *hdmi)
 	/* Disable HPD interrupt */
 	hdmi_write(hdmi, REG_HDMI_HPD_INT_CTRL, 0);
 
-	msm_hdmi_set_mode(hdmi, false);
+	mutex_lock(&hdmi->state_mutex);
+	hdmi->hpd_enabled = false;
+	msm_hdmi_set_mode(hdmi, hdmi->power_on);
+	mutex_unlock(&hdmi->state_mutex);
 
 	pm_runtime_put(dev);
 }

-- 
2.39.2

Re: [PATCH v2 13/14] drm/msm/hdmi: ensure that HDMI is one if HPD is requested

[Markus Elfring]

…
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -117,11 +117,13 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>
>  	DBG("power up");
>
> +	mutex_lock(&hdmi->state_mutex);
>  	if (!hdmi->power_on) {
…
>  	}
> +	mutex_unlock(&hdmi->state_mutex);
>
>  	if (hdmi->hdmi_mode) {
>  		msm_hdmi_config_avi_infoframe(hdmi);
…

Would you become interested to apply a statement like “guard(mutex)(&hdmi->state_mutex);”?
https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/mutex.h#L196

Regards,
Markus

Re: [PATCH v2 13/14] drm/msm/hdmi: ensure that HDMI is one if HPD is requested

[Dmitry Baryshkov]

On Wed, 12 Jun 2024 at 16:01, Markus Elfring <Markus.Elfring@web.de> wrote:
>
> …
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > @@ -117,11 +117,13 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> >
> >       DBG("power up");
> >
> > +     mutex_lock(&hdmi->state_mutex);
> >       if (!hdmi->power_on) {
> …
> >       }
> > +     mutex_unlock(&hdmi->state_mutex);
> >
> >       if (hdmi->hdmi_mode) {
> >               msm_hdmi_config_avi_infoframe(hdmi);
> …
>
> Would you become interested to apply a statement like “guard(mutex)(&hdmi->state_mutex);”?
> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/mutex.h#L196

I am not.


-- 
With best wishes
Dmitry

Re: [v2 13/14] drm/msm/hdmi: ensure that HDMI is one if HPD is requested

[Markus Elfring]

>> Would you become interested to apply a statement like “guard(mutex)(&hdmi->state_mutex);”?
>> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/mutex.h#L196
>
> I am not.

Under which circumstances will development interests grow for scope-based resource management?
https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/cleanup.h#L124

Regards,
Markus

Re: [v2 13/14] drm/msm/hdmi: ensure that HDMI is one if HPD is requested

[Dmitry Baryshkov]

On Wed, 12 Jun 2024 at 17:32, Markus Elfring <Markus.Elfring@web.de> wrote:
>
> >> Would you become interested to apply a statement like “guard(mutex)(&hdmi->state_mutex);”?
> >> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/mutex.h#L196
> >
> > I am not.
>
> Under which circumstances will development interests grow for scope-based resource management?
> https://elixir.bootlin.com/linux/v6.10-rc3/source/include/linux/cleanup.h#L124

I consider guard() and free() to be counterintuitive, harder to follow
and semantically troublesome.

-- 
With best wishes
Dmitry

Re: [PATCH v2 13/14] drm/msm/hdmi: ensure that HDMI is one if HPD is requested

[Jessica Zhang]

On 5/22/2024 3:51 AM, Dmitry Baryshkov wrote:
> The HDMI block needs to be enabled to properly generate HPD events. Make
> sure it is not turned off in the disable paths if HPD delivery is enabled.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c        | 1 +
>   drivers/gpu/drm/msm/hdmi/hdmi.h        | 2 ++
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 8 +++++++-
>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 9 ++++++++-
>   4 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index a9437054c015..2890196857f8 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -409,6 +409,7 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   	hdmi->pdev = pdev;
>   	hdmi->config = config;
>   	spin_lock_init(&hdmi->reg_lock);
> +	mutex_init(&hdmi->state_mutex);
>   
>   	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, &hdmi->next_bridge);
>   	if (ret && ret != -ENODEV)
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index 268ff8604423..7f0ca5252018 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -42,6 +42,8 @@ struct hdmi {
>   
>   	/* video state: */
>   	bool power_on;
> +	bool hpd_enabled;
> +	struct mutex state_mutex; /* protects two booleans */
>   	unsigned long int pixclock;
>   
>   	void __iomem *mmio;
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index cddba640d292..104107ed47d0 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -117,11 +117,13 @@ static void msm_hdmi_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>   
>   	DBG("power up");
>   
> +	mutex_lock(&hdmi->state_mutex);
>   	if (!hdmi->power_on) {
>   		msm_hdmi_phy_resource_enable(phy);
>   		msm_hdmi_power_on(bridge);
>   		hdmi->power_on = true;
>   	}
> +	mutex_unlock(&hdmi->state_mutex);
>   
>   	if (hdmi->hdmi_mode) {
>   		msm_hdmi_config_avi_infoframe(hdmi);
> @@ -147,7 +149,10 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
>   		msm_hdmi_hdcp_off(hdmi->hdcp_ctrl);
>   
>   	DBG("power down");
> -	msm_hdmi_set_mode(hdmi, false);
> +
> +	/* Keep the HDMI enabled if the HPD is enabled */
> +	mutex_lock(&hdmi->state_mutex);
> +	msm_hdmi_set_mode(hdmi, hdmi->hpd_enabled);
>   
>   	msm_hdmi_phy_powerdown(phy);
>   
> @@ -158,6 +163,7 @@ static void msm_hdmi_bridge_atomic_post_disable(struct drm_bridge *bridge,
>   			msm_hdmi_audio_update(hdmi);
>   		msm_hdmi_phy_resource_disable(phy);
>   	}
> +	mutex_unlock(&hdmi->state_mutex);
>   }
>   
>   static void msm_hdmi_bridge_mode_set(struct drm_bridge *bridge,
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> index d3353c6148ed..cb89e9e2c6ea 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> @@ -73,10 +73,14 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>   	if (ret)
>   		return ret;
>   
> +	mutex_lock(&hdmi->state_mutex);
>   	msm_hdmi_set_mode(hdmi, false);
>   	msm_hdmi_phy_reset(hdmi);
>   	msm_hdmi_set_mode(hdmi, true);
>   
> +	hdmi->hpd_enabled = true;
> +	mutex_unlock(&hdmi->state_mutex);
> +
>   	hdmi_write(hdmi, REG_HDMI_USEC_REFTIMER, 0x0001001b);
>   
>   	/* enable HPD events: */
> @@ -106,7 +110,10 @@ void msm_hdmi_hpd_disable(struct hdmi *hdmi)
>   	/* Disable HPD interrupt */
>   	hdmi_write(hdmi, REG_HDMI_HPD_INT_CTRL, 0);
>   
> -	msm_hdmi_set_mode(hdmi, false);
> +	mutex_lock(&hdmi->state_mutex);
> +	hdmi->hpd_enabled = false;
> +	msm_hdmi_set_mode(hdmi, hdmi->power_on);
> +	mutex_unlock(&hdmi->state_mutex);
>   
>   	pm_runtime_put(dev);
>   }
> 
> -- 
> 2.39.2
> 

[PATCH v2 14/14] drm/msm/hdmi: wire in hpd_enable/hpd_disable bridge ops

[Dmitry Baryshkov]

The HDMI driver already has msm_hdmi_hpd_enable() and
msm_hdmi_hpd_disable() functions. Wire them into the
msm_hdmi_bridge_funcs, so that HPD  can be enabled and disabled
dynamically rather than always having HPD events generation enabled.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c        |  9 ---------
 drivers/gpu/drm/msm/hdmi/hdmi.h        |  4 ++--
 drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  3 +++
 drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 12 ++++++------
 4 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 2890196857f8..06adcf4a6544 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -202,12 +202,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 		goto fail;
 	}
 
-	ret = msm_hdmi_hpd_enable(hdmi->bridge);
-	if (ret < 0) {
-		DRM_DEV_ERROR(&hdmi->pdev->dev, "failed to enable HPD: %d\n", ret);
-		goto fail;
-	}
-
 	return 0;
 
 fail:
@@ -377,9 +371,6 @@ static void msm_hdmi_unbind(struct device *dev, struct device *master,
 		if (priv->hdmi->audio_pdev)
 			platform_device_unregister(priv->hdmi->audio_pdev);
 
-		if (priv->hdmi->bridge)
-			msm_hdmi_hpd_disable(priv->hdmi);
-
 		msm_hdmi_destroy(priv->hdmi);
 		priv->hdmi = NULL;
 	}
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 7f0ca5252018..c6519e6f7f2c 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -219,8 +219,8 @@ int msm_hdmi_bridge_init(struct hdmi *hdmi);
 void msm_hdmi_hpd_irq(struct drm_bridge *bridge);
 enum drm_connector_status msm_hdmi_bridge_detect(
 		struct drm_bridge *bridge);
-int msm_hdmi_hpd_enable(struct drm_bridge *bridge);
-void msm_hdmi_hpd_disable(struct hdmi *hdmi);
+void msm_hdmi_hpd_enable(struct drm_bridge *bridge);
+void msm_hdmi_hpd_disable(struct drm_bridge *bridge);
 
 /*
  * i2c adapter for ddc:
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 104107ed47d0..41722b2e6b44 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -300,6 +300,9 @@ static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = {
 	.mode_valid = msm_hdmi_bridge_mode_valid,
 	.edid_read = msm_hdmi_bridge_edid_read,
 	.detect = msm_hdmi_bridge_detect,
+
+	.hpd_enable = msm_hdmi_hpd_enable,
+	.hpd_disable = msm_hdmi_hpd_disable,
 };
 
 static void
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
index cb89e9e2c6ea..04d00b6f36fd 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
@@ -60,7 +60,7 @@ static void msm_hdmi_phy_reset(struct hdmi *hdmi)
 	}
 }
 
-int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
+void msm_hdmi_hpd_enable(struct drm_bridge *bridge)
 {
 	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
 	struct hdmi *hdmi = hdmi_bridge->hdmi;
@@ -70,8 +70,8 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
 	unsigned long flags;
 
 	ret = pm_runtime_resume_and_get(dev);
-	if (ret)
-		return ret;
+	if (WARN_ON(ret))
+		return;
 
 	mutex_lock(&hdmi->state_mutex);
 	msm_hdmi_set_mode(hdmi, false);
@@ -99,12 +99,12 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
 	hdmi_write(hdmi, REG_HDMI_HPD_CTRL,
 			HDMI_HPD_CTRL_ENABLE | hpd_ctrl);
 	spin_unlock_irqrestore(&hdmi->reg_lock, flags);
-
-	return 0;
 }
 
-void msm_hdmi_hpd_disable(struct hdmi *hdmi)
+void msm_hdmi_hpd_disable(struct drm_bridge *bridge)
 {
+	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
+	struct hdmi *hdmi = hdmi_bridge->hdmi;
 	struct device *dev = &hdmi->pdev->dev;
 
 	/* Disable HPD interrupt */

-- 
2.39.2

Re: [PATCH v2 14/14] drm/msm/hdmi: wire in hpd_enable/hpd_disable bridge ops

[Jessica Zhang]

On 5/22/2024 3:51 AM, Dmitry Baryshkov wrote:
> The HDMI driver already has msm_hdmi_hpd_enable() and
> msm_hdmi_hpd_disable() functions. Wire them into the
> msm_hdmi_bridge_funcs, so that HPD  can be enabled and disabled
> dynamically rather than always having HPD events generation enabled.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Jessica Zhang <quic_jesszhan@quicinc.com>

> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c        |  9 ---------
>   drivers/gpu/drm/msm/hdmi/hdmi.h        |  4 ++--
>   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c |  3 +++
>   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    | 12 ++++++------
>   4 files changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 2890196857f8..06adcf4a6544 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -202,12 +202,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   		goto fail;
>   	}
>   
> -	ret = msm_hdmi_hpd_enable(hdmi->bridge);
> -	if (ret < 0) {
> -		DRM_DEV_ERROR(&hdmi->pdev->dev, "failed to enable HPD: %d\n", ret);
> -		goto fail;
> -	}
> -
>   	return 0;
>   
>   fail:
> @@ -377,9 +371,6 @@ static void msm_hdmi_unbind(struct device *dev, struct device *master,
>   		if (priv->hdmi->audio_pdev)
>   			platform_device_unregister(priv->hdmi->audio_pdev);
>   
> -		if (priv->hdmi->bridge)
> -			msm_hdmi_hpd_disable(priv->hdmi);
> -
>   		msm_hdmi_destroy(priv->hdmi);
>   		priv->hdmi = NULL;
>   	}
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
> index 7f0ca5252018..c6519e6f7f2c 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> @@ -219,8 +219,8 @@ int msm_hdmi_bridge_init(struct hdmi *hdmi);
>   void msm_hdmi_hpd_irq(struct drm_bridge *bridge);
>   enum drm_connector_status msm_hdmi_bridge_detect(
>   		struct drm_bridge *bridge);
> -int msm_hdmi_hpd_enable(struct drm_bridge *bridge);
> -void msm_hdmi_hpd_disable(struct hdmi *hdmi);
> +void msm_hdmi_hpd_enable(struct drm_bridge *bridge);
> +void msm_hdmi_hpd_disable(struct drm_bridge *bridge);
>   
>   /*
>    * i2c adapter for ddc:
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> index 104107ed47d0..41722b2e6b44 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> @@ -300,6 +300,9 @@ static const struct drm_bridge_funcs msm_hdmi_bridge_funcs = {
>   	.mode_valid = msm_hdmi_bridge_mode_valid,
>   	.edid_read = msm_hdmi_bridge_edid_read,
>   	.detect = msm_hdmi_bridge_detect,
> +
> +	.hpd_enable = msm_hdmi_hpd_enable,
> +	.hpd_disable = msm_hdmi_hpd_disable,
>   };
>   
>   static void
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> index cb89e9e2c6ea..04d00b6f36fd 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> @@ -60,7 +60,7 @@ static void msm_hdmi_phy_reset(struct hdmi *hdmi)
>   	}
>   }
>   
> -int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
> +void msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>   {
>   	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
>   	struct hdmi *hdmi = hdmi_bridge->hdmi;
> @@ -70,8 +70,8 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>   	unsigned long flags;
>   
>   	ret = pm_runtime_resume_and_get(dev);
> -	if (ret)
> -		return ret;
> +	if (WARN_ON(ret))
> +		return;
>   
>   	mutex_lock(&hdmi->state_mutex);
>   	msm_hdmi_set_mode(hdmi, false);
> @@ -99,12 +99,12 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
>   	hdmi_write(hdmi, REG_HDMI_HPD_CTRL,
>   			HDMI_HPD_CTRL_ENABLE | hpd_ctrl);
>   	spin_unlock_irqrestore(&hdmi->reg_lock, flags);
> -
> -	return 0;
>   }
>   
> -void msm_hdmi_hpd_disable(struct hdmi *hdmi)
> +void msm_hdmi_hpd_disable(struct drm_bridge *bridge)
>   {
> +	struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
> +	struct hdmi *hdmi = hdmi_bridge->hdmi;
>   	struct device *dev = &hdmi->pdev->dev;
>   
>   	/* Disable HPD interrupt */
> 
> -- 
> 2.39.2
> 

Re: [v2 00/14] drm/msm/hdmi: rework and fix the HPD even generation

[Markus Elfring]

…
> This series was tested on msm8996 and apq8064 boards. Previously HPD
> handling sometimes could trigger in the CRTC event handling, …

Would you like to refer to the word “event” also in the message subject?

Regards,
Markus