[PATCH v2 0/5] drm/msm/hdmi: move resource allocation to probe function

[PATCH v2 0/5] drm/msm/hdmi: move resource allocation to probe function

[Dmitry Baryshkov]

As pointed several times in the discussions, start moving resource
allocation from component bind to the probe function. This simplifies
boot process, as the component will not be registered until all
resources (clocks, regulators, IRQ, etc.) are not registered.

Changes since v1:
 - Moved a call to msm_hdmi_get_phy() to msm_hdmi_dev_probe() too.

Dmitry Baryshkov (5):
  drm/msm/hdmi: use devres helper for runtime PM management
  drm/msm/hdmi: drop constant resource names from platform config
  drm/msm/hdmi: move resource allocation to probe function
  drm/msm/hdmi: don't take extra reference on PHY device
  drm/msm/hdmi: move msm_hdmi_get_phy() to msm_hdmi_dev_probe()

 drivers/gpu/drm/msm/hdmi/hdmi.c | 348 +++++++++++++++-----------------
 drivers/gpu/drm/msm/hdmi/hdmi.h |   3 -
 2 files changed, 161 insertions(+), 190 deletions(-)

-- 
2.35.1

[PATCH v2 0/5] drm/msm/hdmi: move resource allocation to probe function

[Dmitry Baryshkov]

As pointed several times in the discussions, start moving resource
allocation from component bind to the probe function. This simplifies
boot process, as the component will not be registered until all
resources (clocks, regulators, IRQ, etc.) are not registered.

Changes since v1:
 - Moved a call to msm_hdmi_get_phy() to msm_hdmi_dev_probe() too.

Dmitry Baryshkov (5):
  drm/msm/hdmi: use devres helper for runtime PM management
  drm/msm/hdmi: drop constant resource names from platform config
  drm/msm/hdmi: move resource allocation to probe function
  drm/msm/hdmi: don't take extra reference on PHY device
  drm/msm/hdmi: move msm_hdmi_get_phy() to msm_hdmi_dev_probe()

 drivers/gpu/drm/msm/hdmi/hdmi.c | 348 +++++++++++++++-----------------
 drivers/gpu/drm/msm/hdmi/hdmi.h |   3 -
 2 files changed, 161 insertions(+), 190 deletions(-)

-- 
2.35.1

[PATCH v2 1/5] drm/msm/hdmi: use devres helper for runtime PM management

[Dmitry Baryshkov]

Use devm_pm_runtime_enable() to enable runtime PM. This way its effect
will be reverted on device unbind/destruction.

Fixes: 6ed9ed484d04 ("drm/msm/hdmi: Set up runtime PM for HDMI")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 93fe61b86967..1d4557de6872 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -252,7 +252,7 @@ static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
 	if (hdmi->hpd_gpiod)
 		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
 
-	pm_runtime_enable(&pdev->dev);
+	devm_pm_runtime_enable(&pdev->dev);
 
 	hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
 
-- 
2.35.1

[PATCH v2 1/5] drm/msm/hdmi: use devres helper for runtime PM management

[Dmitry Baryshkov]

Use devm_pm_runtime_enable() to enable runtime PM. This way its effect
will be reverted on device unbind/destruction.

Fixes: 6ed9ed484d04 ("drm/msm/hdmi: Set up runtime PM for HDMI")
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
---
 drivers/gpu/drm/msm/hdmi/hdmi.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 93fe61b86967..1d4557de6872 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -252,7 +252,7 @@ static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
 	if (hdmi->hpd_gpiod)
 		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
 
-	pm_runtime_enable(&pdev->dev);
+	devm_pm_runtime_enable(&pdev->dev);
 
 	hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
 
-- 
2.35.1

[PATCH v2 2/5] drm/msm/hdmi: drop constant resource names from platform config

[Dmitry Baryshkov]

All MSM HDMI devices use "core_physical" and "qfprom_physical" names for
register areas. Drop them from the platform config.

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

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 1d4557de6872..4a364d8f4c0b 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -138,7 +138,7 @@ static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
 	if (ret && ret != -ENODEV)
 		goto fail;
 
-	hdmi->mmio = msm_ioremap(pdev, config->mmio_name);
+	hdmi->mmio = msm_ioremap(pdev, "core_physical");
 	if (IS_ERR(hdmi->mmio)) {
 		ret = PTR_ERR(hdmi->mmio);
 		goto fail;
@@ -146,14 +146,14 @@ static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
 
 	/* HDCP needs physical address of hdmi register */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-		config->mmio_name);
+		"core_physical");
 	if (!res) {
 		ret = -EINVAL;
 		goto fail;
 	}
 	hdmi->mmio_phy_addr = res->start;
 
-	hdmi->qfprom_mmio = msm_ioremap(pdev, config->qfprom_mmio_name);
+	hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
 	if (IS_ERR(hdmi->qfprom_mmio)) {
 		DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
 		hdmi->qfprom_mmio = NULL;
@@ -524,9 +524,6 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
 		return -ENXIO;
 	}
 
-	hdmi_cfg->mmio_name     = "core_physical";
-	hdmi_cfg->qfprom_mmio_name = "qfprom_physical";
-
 	dev->platform_data = hdmi_cfg;
 
 	hdmi = msm_hdmi_init(to_platform_device(dev));
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 04a74381aaf7..e8dbee50637f 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -86,9 +86,6 @@ struct hdmi {
 
 /* platform config data (ie. from DT, or pdata) */
 struct hdmi_platform_config {
-	const char *mmio_name;
-	const char *qfprom_mmio_name;
-
 	/* regulators that need to be on for hpd: */
 	const char **hpd_reg_names;
 	int hpd_reg_cnt;
-- 
2.35.1

[PATCH v2 2/5] drm/msm/hdmi: drop constant resource names from platform config

[Dmitry Baryshkov]

All MSM HDMI devices use "core_physical" and "qfprom_physical" names for
register areas. Drop them from the platform config.

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

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 1d4557de6872..4a364d8f4c0b 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -138,7 +138,7 @@ static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
 	if (ret && ret != -ENODEV)
 		goto fail;
 
-	hdmi->mmio = msm_ioremap(pdev, config->mmio_name);
+	hdmi->mmio = msm_ioremap(pdev, "core_physical");
 	if (IS_ERR(hdmi->mmio)) {
 		ret = PTR_ERR(hdmi->mmio);
 		goto fail;
@@ -146,14 +146,14 @@ static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
 
 	/* HDCP needs physical address of hdmi register */
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-		config->mmio_name);
+		"core_physical");
 	if (!res) {
 		ret = -EINVAL;
 		goto fail;
 	}
 	hdmi->mmio_phy_addr = res->start;
 
-	hdmi->qfprom_mmio = msm_ioremap(pdev, config->qfprom_mmio_name);
+	hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
 	if (IS_ERR(hdmi->qfprom_mmio)) {
 		DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
 		hdmi->qfprom_mmio = NULL;
@@ -524,9 +524,6 @@ static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
 		return -ENXIO;
 	}
 
-	hdmi_cfg->mmio_name     = "core_physical";
-	hdmi_cfg->qfprom_mmio_name = "qfprom_physical";
-
 	dev->platform_data = hdmi_cfg;
 
 	hdmi = msm_hdmi_init(to_platform_device(dev));
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index 04a74381aaf7..e8dbee50637f 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -86,9 +86,6 @@ struct hdmi {
 
 /* platform config data (ie. from DT, or pdata) */
 struct hdmi_platform_config {
-	const char *mmio_name;
-	const char *qfprom_mmio_name;
-
 	/* regulators that need to be on for hpd: */
 	const char **hpd_reg_names;
 	int hpd_reg_cnt;
-- 
2.35.1

[PATCH v2 3/5] drm/msm/hdmi: move resource allocation to probe function

[Dmitry Baryshkov]

Rather than having all resource allocation happen in the _bind function
(resulting in possible EPROBE_DEFER returns and component bind/unbind
cycles) allocate and check all resources in _probe function. While we
are at it, use platform_get_irq() to get the IRQ rather than going
through the irq_of_parse_and_map().

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

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 4a364d8f4c0b..c298a36f3b42 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -76,8 +76,6 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
 
 	if (hdmi->i2c)
 		msm_hdmi_i2c_destroy(hdmi->i2c);
-
-	platform_set_drvdata(hdmi->pdev, NULL);
 }
 
 static int msm_hdmi_get_phy(struct hdmi *hdmi)
@@ -117,142 +115,10 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
  * we are to EPROBE_DEFER we want to do it here, rather than later
  * at modeset_init() time
  */
-static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
+static int msm_hdmi_init(struct hdmi *hdmi)
 {
-	struct hdmi_platform_config *config = pdev->dev.platform_data;
-	struct hdmi *hdmi = NULL;
-	struct resource *res;
-	int i, ret;
-
-	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
-	if (!hdmi) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-
-	hdmi->pdev = pdev;
-	hdmi->config = config;
-	spin_lock_init(&hdmi->reg_lock);
-
-	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, &hdmi->next_bridge);
-	if (ret && ret != -ENODEV)
-		goto fail;
-
-	hdmi->mmio = msm_ioremap(pdev, "core_physical");
-	if (IS_ERR(hdmi->mmio)) {
-		ret = PTR_ERR(hdmi->mmio);
-		goto fail;
-	}
-
-	/* HDCP needs physical address of hdmi register */
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-		"core_physical");
-	if (!res) {
-		ret = -EINVAL;
-		goto fail;
-	}
-	hdmi->mmio_phy_addr = res->start;
-
-	hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
-	if (IS_ERR(hdmi->qfprom_mmio)) {
-		DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
-		hdmi->qfprom_mmio = NULL;
-	}
-
-	hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
-				      config->hpd_reg_cnt,
-				      sizeof(hdmi->hpd_regs[0]),
-				      GFP_KERNEL);
-	if (!hdmi->hpd_regs) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-	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) {
-		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %d\n", ret);
-		goto fail;
-	}
-
-	hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
-				      config->pwr_reg_cnt,
-				      sizeof(hdmi->pwr_regs[0]),
-				      GFP_KERNEL);
-	if (!hdmi->pwr_regs) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-
-	for (i = 0; i < config->pwr_reg_cnt; i++)
-		hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
-
-	ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, hdmi->pwr_regs);
-	if (ret) {
-		DRM_DEV_ERROR(&pdev->dev, "failed to get pwr regulator: %d\n", ret);
-		goto fail;
-	}
-
-	hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
-				      config->hpd_clk_cnt,
-				      sizeof(hdmi->hpd_clks[0]),
-				      GFP_KERNEL);
-	if (!hdmi->hpd_clks) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-	for (i = 0; i < config->hpd_clk_cnt; i++) {
-		struct clk *clk;
-
-		clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
-		if (IS_ERR(clk)) {
-			ret = PTR_ERR(clk);
-			DRM_DEV_ERROR(&pdev->dev, "failed to get hpd clk: %s (%d)\n",
-					config->hpd_clk_names[i], ret);
-			goto fail;
-		}
-
-		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) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-	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)) {
-			ret = PTR_ERR(clk);
-			DRM_DEV_ERROR(&pdev->dev, "failed to get pwr clk: %s (%d)\n",
-					config->pwr_clk_names[i], ret);
-			goto fail;
-		}
-
-		hdmi->pwr_clks[i] = clk;
-	}
-
-	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)) {
-		ret = PTR_ERR(hdmi->hpd_gpiod);
-		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd gpio: (%d)\n", ret);
-		goto fail;
-	}
-
-	if (!hdmi->hpd_gpiod)
-		DBG("failed to get HPD gpio");
-
-	if (hdmi->hpd_gpiod)
-		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
-
-	devm_pm_runtime_enable(&pdev->dev);
+	struct platform_device *pdev = hdmi->pdev;
+	int ret;
 
 	hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
 
@@ -276,13 +142,13 @@ static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
 		hdmi->hdcp_ctrl = NULL;
 	}
 
-	return hdmi;
+	return 0;
 
 fail:
 	if (hdmi)
 		msm_hdmi_destroy(hdmi);
 
-	return ERR_PTR(ret);
+	return ret;
 }
 
 /* Second part of initialization, the drm/kms level modeset_init,
@@ -332,13 +198,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 
 	drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
 
-	hdmi->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
-	if (!hdmi->irq) {
-		ret = -EINVAL;
-		DRM_DEV_ERROR(dev->dev, "failed to get irq\n");
-		goto fail;
-	}
-
 	ret = devm_request_irq(&pdev->dev, hdmi->irq,
 			msm_hdmi_irq, IRQF_TRIGGER_HIGH,
 			"hdmi_isr", hdmi);
@@ -358,8 +217,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 
 	priv->bridges[priv->num_bridges++]       = hdmi->bridge;
 
-	platform_set_drvdata(pdev, hdmi);
-
 	return 0;
 
 fail:
@@ -387,7 +244,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 static const char *hpd_reg_names_8960[] = {"core-vdda"};
 static const char *hpd_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
 
-static struct hdmi_platform_config hdmi_tx_8960_config = {
+const static struct hdmi_platform_config hdmi_tx_8960_config = {
 		HDMI_CFG(hpd_reg, 8960),
 		HDMI_CFG(hpd_clk, 8960),
 };
@@ -397,7 +254,7 @@ 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 struct hdmi_platform_config hdmi_tx_8974_config = {
+const static struct hdmi_platform_config hdmi_tx_8974_config = {
 		HDMI_CFG(pwr_reg, 8x74),
 		HDMI_CFG(pwr_clk, 8x74),
 		HDMI_CFG(hpd_clk, 8x74),
@@ -512,23 +369,12 @@ static int msm_hdmi_register_audio_driver(struct hdmi *hdmi, struct device *dev)
 static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
 {
 	struct msm_drm_private *priv = dev_get_drvdata(master);
-	struct hdmi_platform_config *hdmi_cfg;
-	struct hdmi *hdmi;
-	struct device_node *of_node = dev->of_node;
+	struct hdmi *hdmi = dev_get_drvdata(dev);
 	int err;
 
-	hdmi_cfg = (struct hdmi_platform_config *)
-			of_device_get_match_data(dev);
-	if (!hdmi_cfg) {
-		DRM_DEV_ERROR(dev, "unknown hdmi_cfg: %pOFn\n", of_node);
-		return -ENXIO;
-	}
-
-	dev->platform_data = hdmi_cfg;
-
-	hdmi = msm_hdmi_init(to_platform_device(dev));
-	if (IS_ERR(hdmi))
-		return PTR_ERR(hdmi);
+	err = msm_hdmi_init(hdmi);
+	if (err)
+		return err;
 	priv->hdmi = hdmi;
 
 	err = msm_hdmi_register_audio_driver(hdmi, dev);
@@ -561,6 +407,133 @@ static const struct component_ops msm_hdmi_ops = {
 
 static int msm_hdmi_dev_probe(struct platform_device *pdev)
 {
+	const struct hdmi_platform_config *config;
+	struct device *dev = &pdev->dev;
+	struct hdmi *hdmi;
+	struct resource *res;
+	int i, ret;
+
+	config = of_device_get_match_data(dev);
+	if (!config)
+		return -EINVAL;
+
+	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
+	if (!hdmi)
+		return -ENOMEM;
+
+	hdmi->pdev = pdev;
+	hdmi->config = config;
+	spin_lock_init(&hdmi->reg_lock);
+
+	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, &hdmi->next_bridge);
+	if (ret && ret != -ENODEV)
+		return ret;
+
+	hdmi->mmio = msm_ioremap(pdev, "core_physical");
+	if (IS_ERR(hdmi->mmio))
+		return PTR_ERR(hdmi->mmio);
+
+	/* HDCP needs physical address of hdmi register */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+		"core_physical");
+	if (!res)
+		return -EINVAL;
+	hdmi->mmio_phy_addr = res->start;
+
+	hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
+	if (IS_ERR(hdmi->qfprom_mmio)) {
+		DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
+		hdmi->qfprom_mmio = NULL;
+	}
+
+	hdmi->irq = platform_get_irq(pdev, 0);
+	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]),
+				      GFP_KERNEL);
+	if (!hdmi->pwr_regs)
+		return -ENOMEM;
+
+	for (i = 0; i < config->pwr_reg_cnt; i++)
+		hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
+
+	ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, hdmi->pwr_regs);
+	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]),
+				      GFP_KERNEL);
+	if (!hdmi->hpd_clks)
+		return -ENOMEM;
+
+	for (i = 0; i < config->hpd_clk_cnt; i++) {
+		struct clk *clk;
+
+		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;
+	}
+
+	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->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");
+
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, hdmi);
+
 	return component_add(&pdev->dev, &msm_hdmi_ops);
 }
 
-- 
2.35.1

[PATCH v2 3/5] drm/msm/hdmi: move resource allocation to probe function

[Dmitry Baryshkov]

Rather than having all resource allocation happen in the _bind function
(resulting in possible EPROBE_DEFER returns and component bind/unbind
cycles) allocate and check all resources in _probe function. While we
are at it, use platform_get_irq() to get the IRQ rather than going
through the irq_of_parse_and_map().

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

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 4a364d8f4c0b..c298a36f3b42 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -76,8 +76,6 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
 
 	if (hdmi->i2c)
 		msm_hdmi_i2c_destroy(hdmi->i2c);
-
-	platform_set_drvdata(hdmi->pdev, NULL);
 }
 
 static int msm_hdmi_get_phy(struct hdmi *hdmi)
@@ -117,142 +115,10 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
  * we are to EPROBE_DEFER we want to do it here, rather than later
  * at modeset_init() time
  */
-static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
+static int msm_hdmi_init(struct hdmi *hdmi)
 {
-	struct hdmi_platform_config *config = pdev->dev.platform_data;
-	struct hdmi *hdmi = NULL;
-	struct resource *res;
-	int i, ret;
-
-	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
-	if (!hdmi) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-
-	hdmi->pdev = pdev;
-	hdmi->config = config;
-	spin_lock_init(&hdmi->reg_lock);
-
-	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, &hdmi->next_bridge);
-	if (ret && ret != -ENODEV)
-		goto fail;
-
-	hdmi->mmio = msm_ioremap(pdev, "core_physical");
-	if (IS_ERR(hdmi->mmio)) {
-		ret = PTR_ERR(hdmi->mmio);
-		goto fail;
-	}
-
-	/* HDCP needs physical address of hdmi register */
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-		"core_physical");
-	if (!res) {
-		ret = -EINVAL;
-		goto fail;
-	}
-	hdmi->mmio_phy_addr = res->start;
-
-	hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
-	if (IS_ERR(hdmi->qfprom_mmio)) {
-		DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
-		hdmi->qfprom_mmio = NULL;
-	}
-
-	hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
-				      config->hpd_reg_cnt,
-				      sizeof(hdmi->hpd_regs[0]),
-				      GFP_KERNEL);
-	if (!hdmi->hpd_regs) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-	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) {
-		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %d\n", ret);
-		goto fail;
-	}
-
-	hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
-				      config->pwr_reg_cnt,
-				      sizeof(hdmi->pwr_regs[0]),
-				      GFP_KERNEL);
-	if (!hdmi->pwr_regs) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-
-	for (i = 0; i < config->pwr_reg_cnt; i++)
-		hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
-
-	ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, hdmi->pwr_regs);
-	if (ret) {
-		DRM_DEV_ERROR(&pdev->dev, "failed to get pwr regulator: %d\n", ret);
-		goto fail;
-	}
-
-	hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
-				      config->hpd_clk_cnt,
-				      sizeof(hdmi->hpd_clks[0]),
-				      GFP_KERNEL);
-	if (!hdmi->hpd_clks) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-	for (i = 0; i < config->hpd_clk_cnt; i++) {
-		struct clk *clk;
-
-		clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
-		if (IS_ERR(clk)) {
-			ret = PTR_ERR(clk);
-			DRM_DEV_ERROR(&pdev->dev, "failed to get hpd clk: %s (%d)\n",
-					config->hpd_clk_names[i], ret);
-			goto fail;
-		}
-
-		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) {
-		ret = -ENOMEM;
-		goto fail;
-	}
-	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)) {
-			ret = PTR_ERR(clk);
-			DRM_DEV_ERROR(&pdev->dev, "failed to get pwr clk: %s (%d)\n",
-					config->pwr_clk_names[i], ret);
-			goto fail;
-		}
-
-		hdmi->pwr_clks[i] = clk;
-	}
-
-	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)) {
-		ret = PTR_ERR(hdmi->hpd_gpiod);
-		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd gpio: (%d)\n", ret);
-		goto fail;
-	}
-
-	if (!hdmi->hpd_gpiod)
-		DBG("failed to get HPD gpio");
-
-	if (hdmi->hpd_gpiod)
-		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
-
-	devm_pm_runtime_enable(&pdev->dev);
+	struct platform_device *pdev = hdmi->pdev;
+	int ret;
 
 	hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
 
@@ -276,13 +142,13 @@ static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
 		hdmi->hdcp_ctrl = NULL;
 	}
 
-	return hdmi;
+	return 0;
 
 fail:
 	if (hdmi)
 		msm_hdmi_destroy(hdmi);
 
-	return ERR_PTR(ret);
+	return ret;
 }
 
 /* Second part of initialization, the drm/kms level modeset_init,
@@ -332,13 +198,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 
 	drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
 
-	hdmi->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
-	if (!hdmi->irq) {
-		ret = -EINVAL;
-		DRM_DEV_ERROR(dev->dev, "failed to get irq\n");
-		goto fail;
-	}
-
 	ret = devm_request_irq(&pdev->dev, hdmi->irq,
 			msm_hdmi_irq, IRQF_TRIGGER_HIGH,
 			"hdmi_isr", hdmi);
@@ -358,8 +217,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 
 	priv->bridges[priv->num_bridges++]       = hdmi->bridge;
 
-	platform_set_drvdata(pdev, hdmi);
-
 	return 0;
 
 fail:
@@ -387,7 +244,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
 static const char *hpd_reg_names_8960[] = {"core-vdda"};
 static const char *hpd_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
 
-static struct hdmi_platform_config hdmi_tx_8960_config = {
+const static struct hdmi_platform_config hdmi_tx_8960_config = {
 		HDMI_CFG(hpd_reg, 8960),
 		HDMI_CFG(hpd_clk, 8960),
 };
@@ -397,7 +254,7 @@ 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 struct hdmi_platform_config hdmi_tx_8974_config = {
+const static struct hdmi_platform_config hdmi_tx_8974_config = {
 		HDMI_CFG(pwr_reg, 8x74),
 		HDMI_CFG(pwr_clk, 8x74),
 		HDMI_CFG(hpd_clk, 8x74),
@@ -512,23 +369,12 @@ static int msm_hdmi_register_audio_driver(struct hdmi *hdmi, struct device *dev)
 static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
 {
 	struct msm_drm_private *priv = dev_get_drvdata(master);
-	struct hdmi_platform_config *hdmi_cfg;
-	struct hdmi *hdmi;
-	struct device_node *of_node = dev->of_node;
+	struct hdmi *hdmi = dev_get_drvdata(dev);
 	int err;
 
-	hdmi_cfg = (struct hdmi_platform_config *)
-			of_device_get_match_data(dev);
-	if (!hdmi_cfg) {
-		DRM_DEV_ERROR(dev, "unknown hdmi_cfg: %pOFn\n", of_node);
-		return -ENXIO;
-	}
-
-	dev->platform_data = hdmi_cfg;
-
-	hdmi = msm_hdmi_init(to_platform_device(dev));
-	if (IS_ERR(hdmi))
-		return PTR_ERR(hdmi);
+	err = msm_hdmi_init(hdmi);
+	if (err)
+		return err;
 	priv->hdmi = hdmi;
 
 	err = msm_hdmi_register_audio_driver(hdmi, dev);
@@ -561,6 +407,133 @@ static const struct component_ops msm_hdmi_ops = {
 
 static int msm_hdmi_dev_probe(struct platform_device *pdev)
 {
+	const struct hdmi_platform_config *config;
+	struct device *dev = &pdev->dev;
+	struct hdmi *hdmi;
+	struct resource *res;
+	int i, ret;
+
+	config = of_device_get_match_data(dev);
+	if (!config)
+		return -EINVAL;
+
+	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
+	if (!hdmi)
+		return -ENOMEM;
+
+	hdmi->pdev = pdev;
+	hdmi->config = config;
+	spin_lock_init(&hdmi->reg_lock);
+
+	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, &hdmi->next_bridge);
+	if (ret && ret != -ENODEV)
+		return ret;
+
+	hdmi->mmio = msm_ioremap(pdev, "core_physical");
+	if (IS_ERR(hdmi->mmio))
+		return PTR_ERR(hdmi->mmio);
+
+	/* HDCP needs physical address of hdmi register */
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+		"core_physical");
+	if (!res)
+		return -EINVAL;
+	hdmi->mmio_phy_addr = res->start;
+
+	hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
+	if (IS_ERR(hdmi->qfprom_mmio)) {
+		DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
+		hdmi->qfprom_mmio = NULL;
+	}
+
+	hdmi->irq = platform_get_irq(pdev, 0);
+	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]),
+				      GFP_KERNEL);
+	if (!hdmi->pwr_regs)
+		return -ENOMEM;
+
+	for (i = 0; i < config->pwr_reg_cnt; i++)
+		hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
+
+	ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, hdmi->pwr_regs);
+	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]),
+				      GFP_KERNEL);
+	if (!hdmi->hpd_clks)
+		return -ENOMEM;
+
+	for (i = 0; i < config->hpd_clk_cnt; i++) {
+		struct clk *clk;
+
+		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;
+	}
+
+	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->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");
+
+	ret = devm_pm_runtime_enable(&pdev->dev);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, hdmi);
+
 	return component_add(&pdev->dev, &msm_hdmi_ops);
 }
 
-- 
2.35.1

[PATCH v2 4/5] drm/msm/hdmi: don't take extra reference on PHY device

[Dmitry Baryshkov]

The of_find_device_by_node() already increments the device's usage
count, so there is no need to increment it again using get_device().
Drop this extra get_device().

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

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index c298a36f3b42..926274eeee25 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -106,7 +106,7 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
 		return -EPROBE_DEFER;
 	}
 
-	hdmi->phy_dev = get_device(&phy_pdev->dev);
+	hdmi->phy_dev = &phy_pdev->dev;
 
 	return 0;
 }
-- 
2.35.1

[PATCH v2 4/5] drm/msm/hdmi: don't take extra reference on PHY device

[Dmitry Baryshkov]

The of_find_device_by_node() already increments the device's usage
count, so there is no need to increment it again using get_device().
Drop this extra get_device().

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

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index c298a36f3b42..926274eeee25 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -106,7 +106,7 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
 		return -EPROBE_DEFER;
 	}
 
-	hdmi->phy_dev = get_device(&phy_pdev->dev);
+	hdmi->phy_dev = &phy_pdev->dev;
 
 	return 0;
 }
-- 
2.35.1

[PATCH v2 5/5] drm/msm/hdmi: move msm_hdmi_get_phy() to msm_hdmi_dev_probe()

[Dmitry Baryshkov]

To continue the idea of failing the probe() rather than failing the
bind(), move the call to msm_hdmi_get_phy() function to
msm_hdmi_dev_probe(), so that the driver fails the probe if PHY is not
yet available rather than succeeding the probe and then failing the
bind() with -EPROBE_DEFER.

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

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 926274eeee25..adaa67d9a78d 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -68,14 +68,17 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
 		destroy_workqueue(hdmi->workq);
 	msm_hdmi_hdcp_destroy(hdmi);
 
+	if (hdmi->i2c)
+		msm_hdmi_i2c_destroy(hdmi->i2c);
+}
+
+static void msm_hdmi_put_phy(struct hdmi *hdmi)
+{
 	if (hdmi->phy_dev) {
 		put_device(hdmi->phy_dev);
 		hdmi->phy = NULL;
 		hdmi->phy_dev = NULL;
 	}
-
-	if (hdmi->i2c)
-		msm_hdmi_i2c_destroy(hdmi->i2c);
 }
 
 static int msm_hdmi_get_phy(struct hdmi *hdmi)
@@ -91,19 +94,15 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
 	}
 
 	phy_pdev = of_find_device_by_node(phy_node);
-	if (phy_pdev)
-		hdmi->phy = platform_get_drvdata(phy_pdev);
-
 	of_node_put(phy_node);
 
-	if (!phy_pdev) {
-		DRM_DEV_ERROR(&pdev->dev, "phy driver is not ready\n");
-		return -EPROBE_DEFER;
-	}
+	if (!phy_pdev)
+		return dev_err_probe(&pdev->dev, -EPROBE_DEFER, "phy driver is not ready\n");
+
+	hdmi->phy = platform_get_drvdata(phy_pdev);
 	if (!hdmi->phy) {
-		DRM_DEV_ERROR(&pdev->dev, "phy driver is not ready\n");
 		put_device(&phy_pdev->dev);
-		return -EPROBE_DEFER;
+		return dev_err_probe(&pdev->dev, -EPROBE_DEFER, "phy driver is not ready\n");
 	}
 
 	hdmi->phy_dev = &phy_pdev->dev;
@@ -130,12 +129,6 @@ static int msm_hdmi_init(struct hdmi *hdmi)
 		goto fail;
 	}
 
-	ret = msm_hdmi_get_phy(hdmi);
-	if (ret) {
-		DRM_DEV_ERROR(&pdev->dev, "failed to get phy\n");
-		goto fail;
-	}
-
 	hdmi->hdcp_ctrl = msm_hdmi_hdcp_init(hdmi);
 	if (IS_ERR(hdmi->hdcp_ctrl)) {
 		dev_warn(&pdev->dev, "failed to init hdcp: disabled\n");
@@ -528,6 +521,12 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
 	if (hdmi->hpd_gpiod)
 		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
 
+	ret = msm_hdmi_get_phy(hdmi);
+	if (ret) {
+		DRM_DEV_ERROR(&pdev->dev, "failed to get phy\n");
+		return ret;
+	}
+
 	ret = devm_pm_runtime_enable(&pdev->dev);
 	if (ret)
 		return ret;
@@ -539,7 +538,12 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
 
 static int msm_hdmi_dev_remove(struct platform_device *pdev)
 {
+	struct hdmi *hdmi = dev_get_drvdata(&pdev->dev);
+
 	component_del(&pdev->dev, &msm_hdmi_ops);
+
+	msm_hdmi_put_phy(hdmi);
+
 	return 0;
 }
 
-- 
2.35.1

[PATCH v2 5/5] drm/msm/hdmi: move msm_hdmi_get_phy() to msm_hdmi_dev_probe()

[Dmitry Baryshkov]

To continue the idea of failing the probe() rather than failing the
bind(), move the call to msm_hdmi_get_phy() function to
msm_hdmi_dev_probe(), so that the driver fails the probe if PHY is not
yet available rather than succeeding the probe and then failing the
bind() with -EPROBE_DEFER.

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

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index 926274eeee25..adaa67d9a78d 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -68,14 +68,17 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
 		destroy_workqueue(hdmi->workq);
 	msm_hdmi_hdcp_destroy(hdmi);
 
+	if (hdmi->i2c)
+		msm_hdmi_i2c_destroy(hdmi->i2c);
+}
+
+static void msm_hdmi_put_phy(struct hdmi *hdmi)
+{
 	if (hdmi->phy_dev) {
 		put_device(hdmi->phy_dev);
 		hdmi->phy = NULL;
 		hdmi->phy_dev = NULL;
 	}
-
-	if (hdmi->i2c)
-		msm_hdmi_i2c_destroy(hdmi->i2c);
 }
 
 static int msm_hdmi_get_phy(struct hdmi *hdmi)
@@ -91,19 +94,15 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
 	}
 
 	phy_pdev = of_find_device_by_node(phy_node);
-	if (phy_pdev)
-		hdmi->phy = platform_get_drvdata(phy_pdev);
-
 	of_node_put(phy_node);
 
-	if (!phy_pdev) {
-		DRM_DEV_ERROR(&pdev->dev, "phy driver is not ready\n");
-		return -EPROBE_DEFER;
-	}
+	if (!phy_pdev)
+		return dev_err_probe(&pdev->dev, -EPROBE_DEFER, "phy driver is not ready\n");
+
+	hdmi->phy = platform_get_drvdata(phy_pdev);
 	if (!hdmi->phy) {
-		DRM_DEV_ERROR(&pdev->dev, "phy driver is not ready\n");
 		put_device(&phy_pdev->dev);
-		return -EPROBE_DEFER;
+		return dev_err_probe(&pdev->dev, -EPROBE_DEFER, "phy driver is not ready\n");
 	}
 
 	hdmi->phy_dev = &phy_pdev->dev;
@@ -130,12 +129,6 @@ static int msm_hdmi_init(struct hdmi *hdmi)
 		goto fail;
 	}
 
-	ret = msm_hdmi_get_phy(hdmi);
-	if (ret) {
-		DRM_DEV_ERROR(&pdev->dev, "failed to get phy\n");
-		goto fail;
-	}
-
 	hdmi->hdcp_ctrl = msm_hdmi_hdcp_init(hdmi);
 	if (IS_ERR(hdmi->hdcp_ctrl)) {
 		dev_warn(&pdev->dev, "failed to init hdcp: disabled\n");
@@ -528,6 +521,12 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
 	if (hdmi->hpd_gpiod)
 		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
 
+	ret = msm_hdmi_get_phy(hdmi);
+	if (ret) {
+		DRM_DEV_ERROR(&pdev->dev, "failed to get phy\n");
+		return ret;
+	}
+
 	ret = devm_pm_runtime_enable(&pdev->dev);
 	if (ret)
 		return ret;
@@ -539,7 +538,12 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
 
 static int msm_hdmi_dev_remove(struct platform_device *pdev)
 {
+	struct hdmi *hdmi = dev_get_drvdata(&pdev->dev);
+
 	component_del(&pdev->dev, &msm_hdmi_ops);
+
+	msm_hdmi_put_phy(hdmi);
+
 	return 0;
 }
 
-- 
2.35.1

Re: [PATCH v2 3/5] drm/msm/hdmi: move resource allocation to probe function

[Abhinav Kumar]

On 8/26/2022 2:39 AM, Dmitry Baryshkov wrote:
> Rather than having all resource allocation happen in the _bind function
> (resulting in possible EPROBE_DEFER returns and component bind/unbind
> cycles) allocate and check all resources in _probe function. While we
> are at it, use platform_get_irq() to get the IRQ rather than going
> through the irq_of_parse_and_map().
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c | 303 +++++++++++++++-----------------
>   1 file changed, 138 insertions(+), 165 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 4a364d8f4c0b..c298a36f3b42 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -76,8 +76,6 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>   
>   	if (hdmi->i2c)
>   		msm_hdmi_i2c_destroy(hdmi->i2c);
> -
> -	platform_set_drvdata(hdmi->pdev, NULL);
>   }
>   
>   static int msm_hdmi_get_phy(struct hdmi *hdmi)

Between v1 and v2, it just seems like a rebase to me on top of the 6.1 
MR. But what about moving msm_hdmi_get_phy() to probe(). I thought you 
were going to check that as well for v2.

A change log would have been nice here. Because as part of the rebase 
looks like we even migrate to using panel bridge for hdmi driver.

Usage of drm_of_find_panel_or_bridge was not present in v1 and wasnt 
obvious from the commit text either.

> @@ -117,142 +115,10 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
>    * we are to EPROBE_DEFER we want to do it here, rather than later
>    * at modeset_init() time
>    */
> -static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
> +static int msm_hdmi_init(struct hdmi *hdmi)
>   {
> -	struct hdmi_platform_config *config = pdev->dev.platform_data;
> -	struct hdmi *hdmi = NULL;
> -	struct resource *res;
> -	int i, ret;
> -
> -	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> -	if (!hdmi) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -
> -	hdmi->pdev = pdev;
> -	hdmi->config = config;
> -	spin_lock_init(&hdmi->reg_lock);
> -
> -	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, &hdmi->next_bridge);
> -	if (ret && ret != -ENODEV)
> -		goto fail;
> -
> -	hdmi->mmio = msm_ioremap(pdev, "core_physical");
> -	if (IS_ERR(hdmi->mmio)) {
> -		ret = PTR_ERR(hdmi->mmio);
> -		goto fail;
> -	}
> -
> -	/* HDCP needs physical address of hdmi register */
> -	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> -		"core_physical");
> -	if (!res) {
> -		ret = -EINVAL;
> -		goto fail;
> -	}
> -	hdmi->mmio_phy_addr = res->start;
> -
> -	hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
> -	if (IS_ERR(hdmi->qfprom_mmio)) {
> -		DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
> -		hdmi->qfprom_mmio = NULL;
> -	}
> -
> -	hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
> -				      config->hpd_reg_cnt,
> -				      sizeof(hdmi->hpd_regs[0]),
> -				      GFP_KERNEL);
> -	if (!hdmi->hpd_regs) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -	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) {
> -		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %d\n", ret);
> -		goto fail;
> -	}
> -
> -	hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
> -				      config->pwr_reg_cnt,
> -				      sizeof(hdmi->pwr_regs[0]),
> -				      GFP_KERNEL);
> -	if (!hdmi->pwr_regs) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -
> -	for (i = 0; i < config->pwr_reg_cnt; i++)
> -		hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
> -
> -	ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, hdmi->pwr_regs);
> -	if (ret) {
> -		DRM_DEV_ERROR(&pdev->dev, "failed to get pwr regulator: %d\n", ret);
> -		goto fail;
> -	}
> -
> -	hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
> -				      config->hpd_clk_cnt,
> -				      sizeof(hdmi->hpd_clks[0]),
> -				      GFP_KERNEL);
> -	if (!hdmi->hpd_clks) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -	for (i = 0; i < config->hpd_clk_cnt; i++) {
> -		struct clk *clk;
> -
> -		clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
> -		if (IS_ERR(clk)) {
> -			ret = PTR_ERR(clk);
> -			DRM_DEV_ERROR(&pdev->dev, "failed to get hpd clk: %s (%d)\n",
> -					config->hpd_clk_names[i], ret);
> -			goto fail;
> -		}
> -
> -		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) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -	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)) {
> -			ret = PTR_ERR(clk);
> -			DRM_DEV_ERROR(&pdev->dev, "failed to get pwr clk: %s (%d)\n",
> -					config->pwr_clk_names[i], ret);
> -			goto fail;
> -		}
> -
> -		hdmi->pwr_clks[i] = clk;
> -	}
> -
> -	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)) {
> -		ret = PTR_ERR(hdmi->hpd_gpiod);
> -		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd gpio: (%d)\n", ret);
> -		goto fail;
> -	}
> -
> -	if (!hdmi->hpd_gpiod)
> -		DBG("failed to get HPD gpio");
> -
> -	if (hdmi->hpd_gpiod)
> -		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
> -
> -	devm_pm_runtime_enable(&pdev->dev);
> +	struct platform_device *pdev = hdmi->pdev;
> +	int ret;
>   
>   	hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
>   
> @@ -276,13 +142,13 @@ static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
>   		hdmi->hdcp_ctrl = NULL;
>   	}
>   
> -	return hdmi;
> +	return 0;
>   
>   fail:
>   	if (hdmi)
>   		msm_hdmi_destroy(hdmi);
>   
> -	return ERR_PTR(ret);
> +	return ret;
>   }
>   
>   /* Second part of initialization, the drm/kms level modeset_init,
> @@ -332,13 +198,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   
>   	drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
>   
> -	hdmi->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> -	if (!hdmi->irq) {
> -		ret = -EINVAL;
> -		DRM_DEV_ERROR(dev->dev, "failed to get irq\n");
> -		goto fail;
> -	}
> -
>   	ret = devm_request_irq(&pdev->dev, hdmi->irq,
>   			msm_hdmi_irq, IRQF_TRIGGER_HIGH,
>   			"hdmi_isr", hdmi);
> @@ -358,8 +217,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   
>   	priv->bridges[priv->num_bridges++]       = hdmi->bridge;
>   
> -	platform_set_drvdata(pdev, hdmi);
> -
>   	return 0;
>   
>   fail:
> @@ -387,7 +244,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   static const char *hpd_reg_names_8960[] = {"core-vdda"};
>   static const char *hpd_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
>   
> -static struct hdmi_platform_config hdmi_tx_8960_config = {
> +const static struct hdmi_platform_config hdmi_tx_8960_config = {
>   		HDMI_CFG(hpd_reg, 8960),
>   		HDMI_CFG(hpd_clk, 8960),
>   };
> @@ -397,7 +254,7 @@ 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 struct hdmi_platform_config hdmi_tx_8974_config = {
> +const static struct hdmi_platform_config hdmi_tx_8974_config = {
>   		HDMI_CFG(pwr_reg, 8x74),
>   		HDMI_CFG(pwr_clk, 8x74),
>   		HDMI_CFG(hpd_clk, 8x74),
> @@ -512,23 +369,12 @@ static int msm_hdmi_register_audio_driver(struct hdmi *hdmi, struct device *dev)
>   static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
>   {
>   	struct msm_drm_private *priv = dev_get_drvdata(master);
> -	struct hdmi_platform_config *hdmi_cfg;
> -	struct hdmi *hdmi;
> -	struct device_node *of_node = dev->of_node;
> +	struct hdmi *hdmi = dev_get_drvdata(dev);
>   	int err;
>   
> -	hdmi_cfg = (struct hdmi_platform_config *)
> -			of_device_get_match_data(dev);
> -	if (!hdmi_cfg) {
> -		DRM_DEV_ERROR(dev, "unknown hdmi_cfg: %pOFn\n", of_node);
> -		return -ENXIO;
> -	}
> -
> -	dev->platform_data = hdmi_cfg;
> -
> -	hdmi = msm_hdmi_init(to_platform_device(dev));
> -	if (IS_ERR(hdmi))
> -		return PTR_ERR(hdmi);
> +	err = msm_hdmi_init(hdmi);
> +	if (err)
> +		return err;
>   	priv->hdmi = hdmi;
>   
>   	err = msm_hdmi_register_audio_driver(hdmi, dev);
> @@ -561,6 +407,133 @@ static const struct component_ops msm_hdmi_ops = {
>   
>   static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   {
> +	const struct hdmi_platform_config *config;
> +	struct device *dev = &pdev->dev;
> +	struct hdmi *hdmi;
> +	struct resource *res;
> +	int i, ret;
> +
> +	config = of_device_get_match_data(dev);
> +	if (!config)
> +		return -EINVAL;
> +
> +	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> +	if (!hdmi)
> +		return -ENOMEM;
> +
> +	hdmi->pdev = pdev;
> +	hdmi->config = config;
> +	spin_lock_init(&hdmi->reg_lock);
> +
> +	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, &hdmi->next_bridge);
> +	if (ret && ret != -ENODEV)
> +		return ret;
> +
> +	hdmi->mmio = msm_ioremap(pdev, "core_physical");
> +	if (IS_ERR(hdmi->mmio))
> +		return PTR_ERR(hdmi->mmio);
> +
> +	/* HDCP needs physical address of hdmi register */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +		"core_physical");
> +	if (!res)
> +		return -EINVAL;
> +	hdmi->mmio_phy_addr = res->start;
> +
> +	hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
> +	if (IS_ERR(hdmi->qfprom_mmio)) {
> +		DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
> +		hdmi->qfprom_mmio = NULL;
> +	}
> +
> +	hdmi->irq = platform_get_irq(pdev, 0);
> +	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]),
> +				      GFP_KERNEL);
> +	if (!hdmi->pwr_regs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < config->pwr_reg_cnt; i++)
> +		hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
> +
> +	ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, hdmi->pwr_regs);
> +	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]),
> +				      GFP_KERNEL);
> +	if (!hdmi->hpd_clks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < config->hpd_clk_cnt; i++) {
> +		struct clk *clk;
> +
> +		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;
> +	}
> +
> +	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->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");
> +
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, hdmi);
> +
>   	return component_add(&pdev->dev, &msm_hdmi_ops);
>   }
>   

Re: [PATCH v2 3/5] drm/msm/hdmi: move resource allocation to probe function

[Abhinav Kumar]

On 8/26/2022 2:39 AM, Dmitry Baryshkov wrote:
> Rather than having all resource allocation happen in the _bind function
> (resulting in possible EPROBE_DEFER returns and component bind/unbind
> cycles) allocate and check all resources in _probe function. While we
> are at it, use platform_get_irq() to get the IRQ rather than going
> through the irq_of_parse_and_map().
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c | 303 +++++++++++++++-----------------
>   1 file changed, 138 insertions(+), 165 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 4a364d8f4c0b..c298a36f3b42 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -76,8 +76,6 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>   
>   	if (hdmi->i2c)
>   		msm_hdmi_i2c_destroy(hdmi->i2c);
> -
> -	platform_set_drvdata(hdmi->pdev, NULL);
>   }
>   
>   static int msm_hdmi_get_phy(struct hdmi *hdmi)

Between v1 and v2, it just seems like a rebase to me on top of the 6.1 
MR. But what about moving msm_hdmi_get_phy() to probe(). I thought you 
were going to check that as well for v2.

A change log would have been nice here. Because as part of the rebase 
looks like we even migrate to using panel bridge for hdmi driver.

Usage of drm_of_find_panel_or_bridge was not present in v1 and wasnt 
obvious from the commit text either.

> @@ -117,142 +115,10 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
>    * we are to EPROBE_DEFER we want to do it here, rather than later
>    * at modeset_init() time
>    */
> -static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
> +static int msm_hdmi_init(struct hdmi *hdmi)
>   {
> -	struct hdmi_platform_config *config = pdev->dev.platform_data;
> -	struct hdmi *hdmi = NULL;
> -	struct resource *res;
> -	int i, ret;
> -
> -	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> -	if (!hdmi) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -
> -	hdmi->pdev = pdev;
> -	hdmi->config = config;
> -	spin_lock_init(&hdmi->reg_lock);
> -
> -	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, &hdmi->next_bridge);
> -	if (ret && ret != -ENODEV)
> -		goto fail;
> -
> -	hdmi->mmio = msm_ioremap(pdev, "core_physical");
> -	if (IS_ERR(hdmi->mmio)) {
> -		ret = PTR_ERR(hdmi->mmio);
> -		goto fail;
> -	}
> -
> -	/* HDCP needs physical address of hdmi register */
> -	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> -		"core_physical");
> -	if (!res) {
> -		ret = -EINVAL;
> -		goto fail;
> -	}
> -	hdmi->mmio_phy_addr = res->start;
> -
> -	hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
> -	if (IS_ERR(hdmi->qfprom_mmio)) {
> -		DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
> -		hdmi->qfprom_mmio = NULL;
> -	}
> -
> -	hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
> -				      config->hpd_reg_cnt,
> -				      sizeof(hdmi->hpd_regs[0]),
> -				      GFP_KERNEL);
> -	if (!hdmi->hpd_regs) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -	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) {
> -		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: %d\n", ret);
> -		goto fail;
> -	}
> -
> -	hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
> -				      config->pwr_reg_cnt,
> -				      sizeof(hdmi->pwr_regs[0]),
> -				      GFP_KERNEL);
> -	if (!hdmi->pwr_regs) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -
> -	for (i = 0; i < config->pwr_reg_cnt; i++)
> -		hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
> -
> -	ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, hdmi->pwr_regs);
> -	if (ret) {
> -		DRM_DEV_ERROR(&pdev->dev, "failed to get pwr regulator: %d\n", ret);
> -		goto fail;
> -	}
> -
> -	hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
> -				      config->hpd_clk_cnt,
> -				      sizeof(hdmi->hpd_clks[0]),
> -				      GFP_KERNEL);
> -	if (!hdmi->hpd_clks) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -	for (i = 0; i < config->hpd_clk_cnt; i++) {
> -		struct clk *clk;
> -
> -		clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
> -		if (IS_ERR(clk)) {
> -			ret = PTR_ERR(clk);
> -			DRM_DEV_ERROR(&pdev->dev, "failed to get hpd clk: %s (%d)\n",
> -					config->hpd_clk_names[i], ret);
> -			goto fail;
> -		}
> -
> -		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) {
> -		ret = -ENOMEM;
> -		goto fail;
> -	}
> -	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)) {
> -			ret = PTR_ERR(clk);
> -			DRM_DEV_ERROR(&pdev->dev, "failed to get pwr clk: %s (%d)\n",
> -					config->pwr_clk_names[i], ret);
> -			goto fail;
> -		}
> -
> -		hdmi->pwr_clks[i] = clk;
> -	}
> -
> -	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)) {
> -		ret = PTR_ERR(hdmi->hpd_gpiod);
> -		DRM_DEV_ERROR(&pdev->dev, "failed to get hpd gpio: (%d)\n", ret);
> -		goto fail;
> -	}
> -
> -	if (!hdmi->hpd_gpiod)
> -		DBG("failed to get HPD gpio");
> -
> -	if (hdmi->hpd_gpiod)
> -		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
> -
> -	devm_pm_runtime_enable(&pdev->dev);
> +	struct platform_device *pdev = hdmi->pdev;
> +	int ret;
>   
>   	hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
>   
> @@ -276,13 +142,13 @@ static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
>   		hdmi->hdcp_ctrl = NULL;
>   	}
>   
> -	return hdmi;
> +	return 0;
>   
>   fail:
>   	if (hdmi)
>   		msm_hdmi_destroy(hdmi);
>   
> -	return ERR_PTR(ret);
> +	return ret;
>   }
>   
>   /* Second part of initialization, the drm/kms level modeset_init,
> @@ -332,13 +198,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   
>   	drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
>   
> -	hdmi->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
> -	if (!hdmi->irq) {
> -		ret = -EINVAL;
> -		DRM_DEV_ERROR(dev->dev, "failed to get irq\n");
> -		goto fail;
> -	}
> -
>   	ret = devm_request_irq(&pdev->dev, hdmi->irq,
>   			msm_hdmi_irq, IRQF_TRIGGER_HIGH,
>   			"hdmi_isr", hdmi);
> @@ -358,8 +217,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   
>   	priv->bridges[priv->num_bridges++]       = hdmi->bridge;
>   
> -	platform_set_drvdata(pdev, hdmi);
> -
>   	return 0;
>   
>   fail:
> @@ -387,7 +244,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>   static const char *hpd_reg_names_8960[] = {"core-vdda"};
>   static const char *hpd_clk_names_8960[] = {"core", "master_iface", "slave_iface"};
>   
> -static struct hdmi_platform_config hdmi_tx_8960_config = {
> +const static struct hdmi_platform_config hdmi_tx_8960_config = {
>   		HDMI_CFG(hpd_reg, 8960),
>   		HDMI_CFG(hpd_clk, 8960),
>   };
> @@ -397,7 +254,7 @@ 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 struct hdmi_platform_config hdmi_tx_8974_config = {
> +const static struct hdmi_platform_config hdmi_tx_8974_config = {
>   		HDMI_CFG(pwr_reg, 8x74),
>   		HDMI_CFG(pwr_clk, 8x74),
>   		HDMI_CFG(hpd_clk, 8x74),
> @@ -512,23 +369,12 @@ static int msm_hdmi_register_audio_driver(struct hdmi *hdmi, struct device *dev)
>   static int msm_hdmi_bind(struct device *dev, struct device *master, void *data)
>   {
>   	struct msm_drm_private *priv = dev_get_drvdata(master);
> -	struct hdmi_platform_config *hdmi_cfg;
> -	struct hdmi *hdmi;
> -	struct device_node *of_node = dev->of_node;
> +	struct hdmi *hdmi = dev_get_drvdata(dev);
>   	int err;
>   
> -	hdmi_cfg = (struct hdmi_platform_config *)
> -			of_device_get_match_data(dev);
> -	if (!hdmi_cfg) {
> -		DRM_DEV_ERROR(dev, "unknown hdmi_cfg: %pOFn\n", of_node);
> -		return -ENXIO;
> -	}
> -
> -	dev->platform_data = hdmi_cfg;
> -
> -	hdmi = msm_hdmi_init(to_platform_device(dev));
> -	if (IS_ERR(hdmi))
> -		return PTR_ERR(hdmi);
> +	err = msm_hdmi_init(hdmi);
> +	if (err)
> +		return err;
>   	priv->hdmi = hdmi;
>   
>   	err = msm_hdmi_register_audio_driver(hdmi, dev);
> @@ -561,6 +407,133 @@ static const struct component_ops msm_hdmi_ops = {
>   
>   static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   {
> +	const struct hdmi_platform_config *config;
> +	struct device *dev = &pdev->dev;
> +	struct hdmi *hdmi;
> +	struct resource *res;
> +	int i, ret;
> +
> +	config = of_device_get_match_data(dev);
> +	if (!config)
> +		return -EINVAL;
> +
> +	hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
> +	if (!hdmi)
> +		return -ENOMEM;
> +
> +	hdmi->pdev = pdev;
> +	hdmi->config = config;
> +	spin_lock_init(&hdmi->reg_lock);
> +
> +	ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, &hdmi->next_bridge);
> +	if (ret && ret != -ENODEV)
> +		return ret;
> +
> +	hdmi->mmio = msm_ioremap(pdev, "core_physical");
> +	if (IS_ERR(hdmi->mmio))
> +		return PTR_ERR(hdmi->mmio);
> +
> +	/* HDCP needs physical address of hdmi register */
> +	res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +		"core_physical");
> +	if (!res)
> +		return -EINVAL;
> +	hdmi->mmio_phy_addr = res->start;
> +
> +	hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
> +	if (IS_ERR(hdmi->qfprom_mmio)) {
> +		DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
> +		hdmi->qfprom_mmio = NULL;
> +	}
> +
> +	hdmi->irq = platform_get_irq(pdev, 0);
> +	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]),
> +				      GFP_KERNEL);
> +	if (!hdmi->pwr_regs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < config->pwr_reg_cnt; i++)
> +		hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
> +
> +	ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, hdmi->pwr_regs);
> +	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]),
> +				      GFP_KERNEL);
> +	if (!hdmi->hpd_clks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < config->hpd_clk_cnt; i++) {
> +		struct clk *clk;
> +
> +		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;
> +	}
> +
> +	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->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");
> +
> +	ret = devm_pm_runtime_enable(&pdev->dev);
> +	if (ret)
> +		return ret;
> +
> +	platform_set_drvdata(pdev, hdmi);
> +
>   	return component_add(&pdev->dev, &msm_hdmi_ops);
>   }
>   

Re: [PATCH v2 4/5] drm/msm/hdmi: don't take extra reference on PHY device

[Abhinav Kumar]

On 8/26/2022 2:39 AM, Dmitry Baryshkov wrote:
> The of_find_device_by_node() already increments the device's usage
> count, so there is no need to increment it again using get_device().
> Drop this extra get_device().
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index c298a36f3b42..926274eeee25 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -106,7 +106,7 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
>   		return -EPROBE_DEFER;
>   	}
>   
> -	hdmi->phy_dev = get_device(&phy_pdev->dev);
> +	hdmi->phy_dev = &phy_pdev->dev;
>   
>   	return 0;
>   }

Re: [PATCH v2 4/5] drm/msm/hdmi: don't take extra reference on PHY device

[Abhinav Kumar]

On 8/26/2022 2:39 AM, Dmitry Baryshkov wrote:
> The of_find_device_by_node() already increments the device's usage
> count, so there is no need to increment it again using get_device().
> Drop this extra get_device().
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index c298a36f3b42..926274eeee25 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -106,7 +106,7 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
>   		return -EPROBE_DEFER;
>   	}
>   
> -	hdmi->phy_dev = get_device(&phy_pdev->dev);
> +	hdmi->phy_dev = &phy_pdev->dev;
>   
>   	return 0;
>   }

Re: [PATCH v2 5/5] drm/msm/hdmi: move msm_hdmi_get_phy() to msm_hdmi_dev_probe()

[Abhinav Kumar]

On 8/26/2022 2:39 AM, Dmitry Baryshkov wrote:
> To continue the idea of failing the probe() rather than failing the
> bind(), move the call to msm_hdmi_get_phy() function to
> msm_hdmi_dev_probe(), so that the driver fails the probe if PHY is not
> yet available rather than succeeding the probe and then failing the
> bind() with -EPROBE_DEFER.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c | 40 ++++++++++++++++++---------------
>   1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 926274eeee25..adaa67d9a78d 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -68,14 +68,17 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>   		destroy_workqueue(hdmi->workq);
>   	msm_hdmi_hdcp_destroy(hdmi);
>   
> +	if (hdmi->i2c)
> +		msm_hdmi_i2c_destroy(hdmi->i2c);
> +}
> +
> +static void msm_hdmi_put_phy(struct hdmi *hdmi)
> +{
>   	if (hdmi->phy_dev) {
>   		put_device(hdmi->phy_dev);
>   		hdmi->phy = NULL;
>   		hdmi->phy_dev = NULL;
>   	}
> -
> -	if (hdmi->i2c)
> -		msm_hdmi_i2c_destroy(hdmi->i2c);
>   }
>   
>   static int msm_hdmi_get_phy(struct hdmi *hdmi)
> @@ -91,19 +94,15 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
>   	}
>   
>   	phy_pdev = of_find_device_by_node(phy_node);
> -	if (phy_pdev)
> -		hdmi->phy = platform_get_drvdata(phy_pdev);
> -
>   	of_node_put(phy_node);
>   
> -	if (!phy_pdev) {
> -		DRM_DEV_ERROR(&pdev->dev, "phy driver is not ready\n");
> -		return -EPROBE_DEFER;
> -	}
> +	if (!phy_pdev)
> +		return dev_err_probe(&pdev->dev, -EPROBE_DEFER, "phy driver is not ready\n");
> +
> +	hdmi->phy = platform_get_drvdata(phy_pdev);
>   	if (!hdmi->phy) {
> -		DRM_DEV_ERROR(&pdev->dev, "phy driver is not ready\n");
>   		put_device(&phy_pdev->dev);
> -		return -EPROBE_DEFER;
> +		return dev_err_probe(&pdev->dev, -EPROBE_DEFER, "phy driver is not ready\n");
>   	}
>   
>   	hdmi->phy_dev = &phy_pdev->dev;
> @@ -130,12 +129,6 @@ static int msm_hdmi_init(struct hdmi *hdmi)
>   		goto fail;
>   	}
>   
> -	ret = msm_hdmi_get_phy(hdmi);
> -	if (ret) {
> -		DRM_DEV_ERROR(&pdev->dev, "failed to get phy\n");
> -		goto fail;
> -	}
> -
>   	hdmi->hdcp_ctrl = msm_hdmi_hdcp_init(hdmi);
>   	if (IS_ERR(hdmi->hdcp_ctrl)) {
>   		dev_warn(&pdev->dev, "failed to init hdcp: disabled\n");
> @@ -528,6 +521,12 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   	if (hdmi->hpd_gpiod)
>   		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
>   
> +	ret = msm_hdmi_get_phy(hdmi);
> +	if (ret) {
> +		DRM_DEV_ERROR(&pdev->dev, "failed to get phy\n");
> +		return ret;
> +	}
> +
>   	ret = devm_pm_runtime_enable(&pdev->dev);
>   	if (ret)
>   		return ret;
> @@ -539,7 +538,12 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   
>   static int msm_hdmi_dev_remove(struct platform_device *pdev)
>   {
> +	struct hdmi *hdmi = dev_get_drvdata(&pdev->dev);
> +
>   	component_del(&pdev->dev, &msm_hdmi_ops);
> +
> +	msm_hdmi_put_phy(hdmi);
> +
>   	return 0;
>   }
>   

Re: [PATCH v2 5/5] drm/msm/hdmi: move msm_hdmi_get_phy() to msm_hdmi_dev_probe()

[Abhinav Kumar]

On 8/26/2022 2:39 AM, Dmitry Baryshkov wrote:
> To continue the idea of failing the probe() rather than failing the
> bind(), move the call to msm_hdmi_get_phy() function to
> msm_hdmi_dev_probe(), so that the driver fails the probe if PHY is not
> yet available rather than succeeding the probe and then failing the
> bind() with -EPROBE_DEFER.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
> ---
>   drivers/gpu/drm/msm/hdmi/hdmi.c | 40 ++++++++++++++++++---------------
>   1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
> index 926274eeee25..adaa67d9a78d 100644
> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> @@ -68,14 +68,17 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>   		destroy_workqueue(hdmi->workq);
>   	msm_hdmi_hdcp_destroy(hdmi);
>   
> +	if (hdmi->i2c)
> +		msm_hdmi_i2c_destroy(hdmi->i2c);
> +}
> +
> +static void msm_hdmi_put_phy(struct hdmi *hdmi)
> +{
>   	if (hdmi->phy_dev) {
>   		put_device(hdmi->phy_dev);
>   		hdmi->phy = NULL;
>   		hdmi->phy_dev = NULL;
>   	}
> -
> -	if (hdmi->i2c)
> -		msm_hdmi_i2c_destroy(hdmi->i2c);
>   }
>   
>   static int msm_hdmi_get_phy(struct hdmi *hdmi)
> @@ -91,19 +94,15 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
>   	}
>   
>   	phy_pdev = of_find_device_by_node(phy_node);
> -	if (phy_pdev)
> -		hdmi->phy = platform_get_drvdata(phy_pdev);
> -
>   	of_node_put(phy_node);
>   
> -	if (!phy_pdev) {
> -		DRM_DEV_ERROR(&pdev->dev, "phy driver is not ready\n");
> -		return -EPROBE_DEFER;
> -	}
> +	if (!phy_pdev)
> +		return dev_err_probe(&pdev->dev, -EPROBE_DEFER, "phy driver is not ready\n");
> +
> +	hdmi->phy = platform_get_drvdata(phy_pdev);
>   	if (!hdmi->phy) {
> -		DRM_DEV_ERROR(&pdev->dev, "phy driver is not ready\n");
>   		put_device(&phy_pdev->dev);
> -		return -EPROBE_DEFER;
> +		return dev_err_probe(&pdev->dev, -EPROBE_DEFER, "phy driver is not ready\n");
>   	}
>   
>   	hdmi->phy_dev = &phy_pdev->dev;
> @@ -130,12 +129,6 @@ static int msm_hdmi_init(struct hdmi *hdmi)
>   		goto fail;
>   	}
>   
> -	ret = msm_hdmi_get_phy(hdmi);
> -	if (ret) {
> -		DRM_DEV_ERROR(&pdev->dev, "failed to get phy\n");
> -		goto fail;
> -	}
> -
>   	hdmi->hdcp_ctrl = msm_hdmi_hdcp_init(hdmi);
>   	if (IS_ERR(hdmi->hdcp_ctrl)) {
>   		dev_warn(&pdev->dev, "failed to init hdcp: disabled\n");
> @@ -528,6 +521,12 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   	if (hdmi->hpd_gpiod)
>   		gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
>   
> +	ret = msm_hdmi_get_phy(hdmi);
> +	if (ret) {
> +		DRM_DEV_ERROR(&pdev->dev, "failed to get phy\n");
> +		return ret;
> +	}
> +
>   	ret = devm_pm_runtime_enable(&pdev->dev);
>   	if (ret)
>   		return ret;
> @@ -539,7 +538,12 @@ static int msm_hdmi_dev_probe(struct platform_device *pdev)
>   
>   static int msm_hdmi_dev_remove(struct platform_device *pdev)
>   {
> +	struct hdmi *hdmi = dev_get_drvdata(&pdev->dev);
> +
>   	component_del(&pdev->dev, &msm_hdmi_ops);
> +
> +	msm_hdmi_put_phy(hdmi);
> +
>   	return 0;
>   }
>   

Re: [Freedreno] [PATCH v2 3/5] drm/msm/hdmi: move resource allocation to probe function

[Abhinav Kumar]

On 9/7/2022 11:54 AM, Abhinav Kumar wrote:
> 
> 
> On 8/26/2022 2:39 AM, Dmitry Baryshkov wrote:
>> Rather than having all resource allocation happen in the _bind function
>> (resulting in possible EPROBE_DEFER returns and component bind/unbind
>> cycles) allocate and check all resources in _probe function. While we
>> are at it, use platform_get_irq() to get the IRQ rather than going
>> through the irq_of_parse_and_map().
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/hdmi/hdmi.c | 303 +++++++++++++++-----------------
>>   1 file changed, 138 insertions(+), 165 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c 
>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> index 4a364d8f4c0b..c298a36f3b42 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> @@ -76,8 +76,6 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>>       if (hdmi->i2c)
>>           msm_hdmi_i2c_destroy(hdmi->i2c);
>> -
>> -    platform_set_drvdata(hdmi->pdev, NULL);
>>   }
>>   static int msm_hdmi_get_phy(struct hdmi *hdmi)
> 
> Between v1 and v2, it just seems like a rebase to me on top of the 6.1 
> MR. But what about moving msm_hdmi_get_phy() to probe(). I thought you 
> were going to check that as well for v2.

Please ignore this part of the comment, I see that moving 
msm_hdmi_get_phy() to probe() is in the patch 5 of this series.

Thanks for making that change.

The below comment still holds true.

> 
> A change log would have been nice here. Because as part of the rebase 
> looks like we even migrate to using panel bridge for hdmi driver.
> 
> Usage of drm_of_find_panel_or_bridge was not present in v1 and wasnt 
> obvious from the commit text either.
> 
>> @@ -117,142 +115,10 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
>>    * we are to EPROBE_DEFER we want to do it here, rather than later
>>    * at modeset_init() time
>>    */
>> -static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
>> +static int msm_hdmi_init(struct hdmi *hdmi)
>>   {
>> -    struct hdmi_platform_config *config = pdev->dev.platform_data;
>> -    struct hdmi *hdmi = NULL;
>> -    struct resource *res;
>> -    int i, ret;
>> -
>> -    hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
>> -    if (!hdmi) {
>> -        ret = -ENOMEM;
>> -        goto fail;
>> -    }
>> -
>> -    hdmi->pdev = pdev;
>> -    hdmi->config = config;
>> -    spin_lock_init(&hdmi->reg_lock);
>> -
>> -    ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, 
>> &hdmi->next_bridge);
>> -    if (ret && ret != -ENODEV)
>> -        goto fail;
>> -
>> -    hdmi->mmio = msm_ioremap(pdev, "core_physical");
>> -    if (IS_ERR(hdmi->mmio)) {
>> -        ret = PTR_ERR(hdmi->mmio);
>> -        goto fail;
>> -    }
>> -
>> -    /* HDCP needs physical address of hdmi register */
>> -    res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> -        "core_physical");
>> -    if (!res) {
>> -        ret = -EINVAL;
>> -        goto fail;
>> -    }
>> -    hdmi->mmio_phy_addr = res->start;
>> -
>> -    hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
>> -    if (IS_ERR(hdmi->qfprom_mmio)) {
>> -        DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
>> -        hdmi->qfprom_mmio = NULL;
>> -    }
>> -
>> -    hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
>> -                      config->hpd_reg_cnt,
>> -                      sizeof(hdmi->hpd_regs[0]),
>> -                      GFP_KERNEL);
>> -    if (!hdmi->hpd_regs) {
>> -        ret = -ENOMEM;
>> -        goto fail;
>> -    }
>> -    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) {
>> -        DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: 
>> %d\n", ret);
>> -        goto fail;
>> -    }
>> -
>> -    hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
>> -                      config->pwr_reg_cnt,
>> -                      sizeof(hdmi->pwr_regs[0]),
>> -                      GFP_KERNEL);
>> -    if (!hdmi->pwr_regs) {
>> -        ret = -ENOMEM;
>> -        goto fail;
>> -    }
>> -
>> -    for (i = 0; i < config->pwr_reg_cnt; i++)
>> -        hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
>> -
>> -    ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, 
>> hdmi->pwr_regs);
>> -    if (ret) {
>> -        DRM_DEV_ERROR(&pdev->dev, "failed to get pwr regulator: 
>> %d\n", ret);
>> -        goto fail;
>> -    }
>> -
>> -    hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
>> -                      config->hpd_clk_cnt,
>> -                      sizeof(hdmi->hpd_clks[0]),
>> -                      GFP_KERNEL);
>> -    if (!hdmi->hpd_clks) {
>> -        ret = -ENOMEM;
>> -        goto fail;
>> -    }
>> -    for (i = 0; i < config->hpd_clk_cnt; i++) {
>> -        struct clk *clk;
>> -
>> -        clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
>> -        if (IS_ERR(clk)) {
>> -            ret = PTR_ERR(clk);
>> -            DRM_DEV_ERROR(&pdev->dev, "failed to get hpd clk: %s 
>> (%d)\n",
>> -                    config->hpd_clk_names[i], ret);
>> -            goto fail;
>> -        }
>> -
>> -        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) {
>> -        ret = -ENOMEM;
>> -        goto fail;
>> -    }
>> -    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)) {
>> -            ret = PTR_ERR(clk);
>> -            DRM_DEV_ERROR(&pdev->dev, "failed to get pwr clk: %s 
>> (%d)\n",
>> -                    config->pwr_clk_names[i], ret);
>> -            goto fail;
>> -        }
>> -
>> -        hdmi->pwr_clks[i] = clk;
>> -    }
>> -
>> -    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)) {
>> -        ret = PTR_ERR(hdmi->hpd_gpiod);
>> -        DRM_DEV_ERROR(&pdev->dev, "failed to get hpd gpio: (%d)\n", 
>> ret);
>> -        goto fail;
>> -    }
>> -
>> -    if (!hdmi->hpd_gpiod)
>> -        DBG("failed to get HPD gpio");
>> -
>> -    if (hdmi->hpd_gpiod)
>> -        gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
>> -
>> -    devm_pm_runtime_enable(&pdev->dev);
>> +    struct platform_device *pdev = hdmi->pdev;
>> +    int ret;
>>       hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
>> @@ -276,13 +142,13 @@ static struct hdmi *msm_hdmi_init(struct 
>> platform_device *pdev)
>>           hdmi->hdcp_ctrl = NULL;
>>       }
>> -    return hdmi;
>> +    return 0;
>>   fail:
>>       if (hdmi)
>>           msm_hdmi_destroy(hdmi);
>> -    return ERR_PTR(ret);
>> +    return ret;
>>   }
>>   /* Second part of initialization, the drm/kms level modeset_init,
>> @@ -332,13 +198,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>       drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
>> -    hdmi->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>> -    if (!hdmi->irq) {
>> -        ret = -EINVAL;
>> -        DRM_DEV_ERROR(dev->dev, "failed to get irq\n");
>> -        goto fail;
>> -    }
>> -
>>       ret = devm_request_irq(&pdev->dev, hdmi->irq,
>>               msm_hdmi_irq, IRQF_TRIGGER_HIGH,
>>               "hdmi_isr", hdmi);
>> @@ -358,8 +217,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>       priv->bridges[priv->num_bridges++]       = hdmi->bridge;
>> -    platform_set_drvdata(pdev, hdmi);
>> -
>>       return 0;
>>   fail:
>> @@ -387,7 +244,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>   static const char *hpd_reg_names_8960[] = {"core-vdda"};
>>   static const char *hpd_clk_names_8960[] = {"core", "master_iface", 
>> "slave_iface"};
>> -static struct hdmi_platform_config hdmi_tx_8960_config = {
>> +const static struct hdmi_platform_config hdmi_tx_8960_config = {
>>           HDMI_CFG(hpd_reg, 8960),
>>           HDMI_CFG(hpd_clk, 8960),
>>   };
>> @@ -397,7 +254,7 @@ 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 struct hdmi_platform_config hdmi_tx_8974_config = {
>> +const static struct hdmi_platform_config hdmi_tx_8974_config = {
>>           HDMI_CFG(pwr_reg, 8x74),
>>           HDMI_CFG(pwr_clk, 8x74),
>>           HDMI_CFG(hpd_clk, 8x74),
>> @@ -512,23 +369,12 @@ static int msm_hdmi_register_audio_driver(struct 
>> hdmi *hdmi, struct device *dev)
>>   static int msm_hdmi_bind(struct device *dev, struct device *master, 
>> void *data)
>>   {
>>       struct msm_drm_private *priv = dev_get_drvdata(master);
>> -    struct hdmi_platform_config *hdmi_cfg;
>> -    struct hdmi *hdmi;
>> -    struct device_node *of_node = dev->of_node;
>> +    struct hdmi *hdmi = dev_get_drvdata(dev);
>>       int err;
>> -    hdmi_cfg = (struct hdmi_platform_config *)
>> -            of_device_get_match_data(dev);
>> -    if (!hdmi_cfg) {
>> -        DRM_DEV_ERROR(dev, "unknown hdmi_cfg: %pOFn\n", of_node);
>> -        return -ENXIO;
>> -    }
>> -
>> -    dev->platform_data = hdmi_cfg;
>> -
>> -    hdmi = msm_hdmi_init(to_platform_device(dev));
>> -    if (IS_ERR(hdmi))
>> -        return PTR_ERR(hdmi);
>> +    err = msm_hdmi_init(hdmi);
>> +    if (err)
>> +        return err;
>>       priv->hdmi = hdmi;
>>       err = msm_hdmi_register_audio_driver(hdmi, dev);
>> @@ -561,6 +407,133 @@ static const struct component_ops msm_hdmi_ops = {
>>   static int msm_hdmi_dev_probe(struct platform_device *pdev)
>>   {
>> +    const struct hdmi_platform_config *config;
>> +    struct device *dev = &pdev->dev;
>> +    struct hdmi *hdmi;
>> +    struct resource *res;
>> +    int i, ret;
>> +
>> +    config = of_device_get_match_data(dev);
>> +    if (!config)
>> +        return -EINVAL;
>> +
>> +    hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
>> +    if (!hdmi)
>> +        return -ENOMEM;
>> +
>> +    hdmi->pdev = pdev;
>> +    hdmi->config = config;
>> +    spin_lock_init(&hdmi->reg_lock);
>> +
>> +    ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, 
>> &hdmi->next_bridge);
>> +    if (ret && ret != -ENODEV)
>> +        return ret;
>> +
>> +    hdmi->mmio = msm_ioremap(pdev, "core_physical");
>> +    if (IS_ERR(hdmi->mmio))
>> +        return PTR_ERR(hdmi->mmio);
>> +
>> +    /* HDCP needs physical address of hdmi register */
>> +    res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +        "core_physical");
>> +    if (!res)
>> +        return -EINVAL;
>> +    hdmi->mmio_phy_addr = res->start;
>> +
>> +    hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
>> +    if (IS_ERR(hdmi->qfprom_mmio)) {
>> +        DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
>> +        hdmi->qfprom_mmio = NULL;
>> +    }
>> +
>> +    hdmi->irq = platform_get_irq(pdev, 0);
>> +    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]),
>> +                      GFP_KERNEL);
>> +    if (!hdmi->pwr_regs)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < config->pwr_reg_cnt; i++)
>> +        hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
>> +
>> +    ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, 
>> hdmi->pwr_regs);
>> +    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]),
>> +                      GFP_KERNEL);
>> +    if (!hdmi->hpd_clks)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < config->hpd_clk_cnt; i++) {
>> +        struct clk *clk;
>> +
>> +        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;
>> +    }
>> +
>> +    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->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");
>> +
>> +    ret = devm_pm_runtime_enable(&pdev->dev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    platform_set_drvdata(pdev, hdmi);
>> +
>>       return component_add(&pdev->dev, &msm_hdmi_ops);
>>   }

Re: [Freedreno] [PATCH v2 3/5] drm/msm/hdmi: move resource allocation to probe function

[Abhinav Kumar]

On 9/7/2022 11:54 AM, Abhinav Kumar wrote:
> 
> 
> On 8/26/2022 2:39 AM, Dmitry Baryshkov wrote:
>> Rather than having all resource allocation happen in the _bind function
>> (resulting in possible EPROBE_DEFER returns and component bind/unbind
>> cycles) allocate and check all resources in _probe function. While we
>> are at it, use platform_get_irq() to get the IRQ rather than going
>> through the irq_of_parse_and_map().
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/gpu/drm/msm/hdmi/hdmi.c | 303 +++++++++++++++-----------------
>>   1 file changed, 138 insertions(+), 165 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c 
>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> index 4a364d8f4c0b..c298a36f3b42 100644
>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>> @@ -76,8 +76,6 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>>       if (hdmi->i2c)
>>           msm_hdmi_i2c_destroy(hdmi->i2c);
>> -
>> -    platform_set_drvdata(hdmi->pdev, NULL);
>>   }
>>   static int msm_hdmi_get_phy(struct hdmi *hdmi)
> 
> Between v1 and v2, it just seems like a rebase to me on top of the 6.1 
> MR. But what about moving msm_hdmi_get_phy() to probe(). I thought you 
> were going to check that as well for v2.

Please ignore this part of the comment, I see that moving 
msm_hdmi_get_phy() to probe() is in the patch 5 of this series.

Thanks for making that change.

The below comment still holds true.

> 
> A change log would have been nice here. Because as part of the rebase 
> looks like we even migrate to using panel bridge for hdmi driver.
> 
> Usage of drm_of_find_panel_or_bridge was not present in v1 and wasnt 
> obvious from the commit text either.
> 
>> @@ -117,142 +115,10 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
>>    * we are to EPROBE_DEFER we want to do it here, rather than later
>>    * at modeset_init() time
>>    */
>> -static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
>> +static int msm_hdmi_init(struct hdmi *hdmi)
>>   {
>> -    struct hdmi_platform_config *config = pdev->dev.platform_data;
>> -    struct hdmi *hdmi = NULL;
>> -    struct resource *res;
>> -    int i, ret;
>> -
>> -    hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
>> -    if (!hdmi) {
>> -        ret = -ENOMEM;
>> -        goto fail;
>> -    }
>> -
>> -    hdmi->pdev = pdev;
>> -    hdmi->config = config;
>> -    spin_lock_init(&hdmi->reg_lock);
>> -
>> -    ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, 
>> &hdmi->next_bridge);
>> -    if (ret && ret != -ENODEV)
>> -        goto fail;
>> -
>> -    hdmi->mmio = msm_ioremap(pdev, "core_physical");
>> -    if (IS_ERR(hdmi->mmio)) {
>> -        ret = PTR_ERR(hdmi->mmio);
>> -        goto fail;
>> -    }
>> -
>> -    /* HDCP needs physical address of hdmi register */
>> -    res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> -        "core_physical");
>> -    if (!res) {
>> -        ret = -EINVAL;
>> -        goto fail;
>> -    }
>> -    hdmi->mmio_phy_addr = res->start;
>> -
>> -    hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
>> -    if (IS_ERR(hdmi->qfprom_mmio)) {
>> -        DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
>> -        hdmi->qfprom_mmio = NULL;
>> -    }
>> -
>> -    hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
>> -                      config->hpd_reg_cnt,
>> -                      sizeof(hdmi->hpd_regs[0]),
>> -                      GFP_KERNEL);
>> -    if (!hdmi->hpd_regs) {
>> -        ret = -ENOMEM;
>> -        goto fail;
>> -    }
>> -    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) {
>> -        DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: 
>> %d\n", ret);
>> -        goto fail;
>> -    }
>> -
>> -    hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
>> -                      config->pwr_reg_cnt,
>> -                      sizeof(hdmi->pwr_regs[0]),
>> -                      GFP_KERNEL);
>> -    if (!hdmi->pwr_regs) {
>> -        ret = -ENOMEM;
>> -        goto fail;
>> -    }
>> -
>> -    for (i = 0; i < config->pwr_reg_cnt; i++)
>> -        hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
>> -
>> -    ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, 
>> hdmi->pwr_regs);
>> -    if (ret) {
>> -        DRM_DEV_ERROR(&pdev->dev, "failed to get pwr regulator: 
>> %d\n", ret);
>> -        goto fail;
>> -    }
>> -
>> -    hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
>> -                      config->hpd_clk_cnt,
>> -                      sizeof(hdmi->hpd_clks[0]),
>> -                      GFP_KERNEL);
>> -    if (!hdmi->hpd_clks) {
>> -        ret = -ENOMEM;
>> -        goto fail;
>> -    }
>> -    for (i = 0; i < config->hpd_clk_cnt; i++) {
>> -        struct clk *clk;
>> -
>> -        clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
>> -        if (IS_ERR(clk)) {
>> -            ret = PTR_ERR(clk);
>> -            DRM_DEV_ERROR(&pdev->dev, "failed to get hpd clk: %s 
>> (%d)\n",
>> -                    config->hpd_clk_names[i], ret);
>> -            goto fail;
>> -        }
>> -
>> -        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) {
>> -        ret = -ENOMEM;
>> -        goto fail;
>> -    }
>> -    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)) {
>> -            ret = PTR_ERR(clk);
>> -            DRM_DEV_ERROR(&pdev->dev, "failed to get pwr clk: %s 
>> (%d)\n",
>> -                    config->pwr_clk_names[i], ret);
>> -            goto fail;
>> -        }
>> -
>> -        hdmi->pwr_clks[i] = clk;
>> -    }
>> -
>> -    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)) {
>> -        ret = PTR_ERR(hdmi->hpd_gpiod);
>> -        DRM_DEV_ERROR(&pdev->dev, "failed to get hpd gpio: (%d)\n", 
>> ret);
>> -        goto fail;
>> -    }
>> -
>> -    if (!hdmi->hpd_gpiod)
>> -        DBG("failed to get HPD gpio");
>> -
>> -    if (hdmi->hpd_gpiod)
>> -        gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
>> -
>> -    devm_pm_runtime_enable(&pdev->dev);
>> +    struct platform_device *pdev = hdmi->pdev;
>> +    int ret;
>>       hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
>> @@ -276,13 +142,13 @@ static struct hdmi *msm_hdmi_init(struct 
>> platform_device *pdev)
>>           hdmi->hdcp_ctrl = NULL;
>>       }
>> -    return hdmi;
>> +    return 0;
>>   fail:
>>       if (hdmi)
>>           msm_hdmi_destroy(hdmi);
>> -    return ERR_PTR(ret);
>> +    return ret;
>>   }
>>   /* Second part of initialization, the drm/kms level modeset_init,
>> @@ -332,13 +198,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>       drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
>> -    hdmi->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>> -    if (!hdmi->irq) {
>> -        ret = -EINVAL;
>> -        DRM_DEV_ERROR(dev->dev, "failed to get irq\n");
>> -        goto fail;
>> -    }
>> -
>>       ret = devm_request_irq(&pdev->dev, hdmi->irq,
>>               msm_hdmi_irq, IRQF_TRIGGER_HIGH,
>>               "hdmi_isr", hdmi);
>> @@ -358,8 +217,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>       priv->bridges[priv->num_bridges++]       = hdmi->bridge;
>> -    platform_set_drvdata(pdev, hdmi);
>> -
>>       return 0;
>>   fail:
>> @@ -387,7 +244,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>   static const char *hpd_reg_names_8960[] = {"core-vdda"};
>>   static const char *hpd_clk_names_8960[] = {"core", "master_iface", 
>> "slave_iface"};
>> -static struct hdmi_platform_config hdmi_tx_8960_config = {
>> +const static struct hdmi_platform_config hdmi_tx_8960_config = {
>>           HDMI_CFG(hpd_reg, 8960),
>>           HDMI_CFG(hpd_clk, 8960),
>>   };
>> @@ -397,7 +254,7 @@ 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 struct hdmi_platform_config hdmi_tx_8974_config = {
>> +const static struct hdmi_platform_config hdmi_tx_8974_config = {
>>           HDMI_CFG(pwr_reg, 8x74),
>>           HDMI_CFG(pwr_clk, 8x74),
>>           HDMI_CFG(hpd_clk, 8x74),
>> @@ -512,23 +369,12 @@ static int msm_hdmi_register_audio_driver(struct 
>> hdmi *hdmi, struct device *dev)
>>   static int msm_hdmi_bind(struct device *dev, struct device *master, 
>> void *data)
>>   {
>>       struct msm_drm_private *priv = dev_get_drvdata(master);
>> -    struct hdmi_platform_config *hdmi_cfg;
>> -    struct hdmi *hdmi;
>> -    struct device_node *of_node = dev->of_node;
>> +    struct hdmi *hdmi = dev_get_drvdata(dev);
>>       int err;
>> -    hdmi_cfg = (struct hdmi_platform_config *)
>> -            of_device_get_match_data(dev);
>> -    if (!hdmi_cfg) {
>> -        DRM_DEV_ERROR(dev, "unknown hdmi_cfg: %pOFn\n", of_node);
>> -        return -ENXIO;
>> -    }
>> -
>> -    dev->platform_data = hdmi_cfg;
>> -
>> -    hdmi = msm_hdmi_init(to_platform_device(dev));
>> -    if (IS_ERR(hdmi))
>> -        return PTR_ERR(hdmi);
>> +    err = msm_hdmi_init(hdmi);
>> +    if (err)
>> +        return err;
>>       priv->hdmi = hdmi;
>>       err = msm_hdmi_register_audio_driver(hdmi, dev);
>> @@ -561,6 +407,133 @@ static const struct component_ops msm_hdmi_ops = {
>>   static int msm_hdmi_dev_probe(struct platform_device *pdev)
>>   {
>> +    const struct hdmi_platform_config *config;
>> +    struct device *dev = &pdev->dev;
>> +    struct hdmi *hdmi;
>> +    struct resource *res;
>> +    int i, ret;
>> +
>> +    config = of_device_get_match_data(dev);
>> +    if (!config)
>> +        return -EINVAL;
>> +
>> +    hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
>> +    if (!hdmi)
>> +        return -ENOMEM;
>> +
>> +    hdmi->pdev = pdev;
>> +    hdmi->config = config;
>> +    spin_lock_init(&hdmi->reg_lock);
>> +
>> +    ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, 
>> &hdmi->next_bridge);
>> +    if (ret && ret != -ENODEV)
>> +        return ret;
>> +
>> +    hdmi->mmio = msm_ioremap(pdev, "core_physical");
>> +    if (IS_ERR(hdmi->mmio))
>> +        return PTR_ERR(hdmi->mmio);
>> +
>> +    /* HDCP needs physical address of hdmi register */
>> +    res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +        "core_physical");
>> +    if (!res)
>> +        return -EINVAL;
>> +    hdmi->mmio_phy_addr = res->start;
>> +
>> +    hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
>> +    if (IS_ERR(hdmi->qfprom_mmio)) {
>> +        DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
>> +        hdmi->qfprom_mmio = NULL;
>> +    }
>> +
>> +    hdmi->irq = platform_get_irq(pdev, 0);
>> +    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]),
>> +                      GFP_KERNEL);
>> +    if (!hdmi->pwr_regs)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < config->pwr_reg_cnt; i++)
>> +        hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
>> +
>> +    ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, 
>> hdmi->pwr_regs);
>> +    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]),
>> +                      GFP_KERNEL);
>> +    if (!hdmi->hpd_clks)
>> +        return -ENOMEM;
>> +
>> +    for (i = 0; i < config->hpd_clk_cnt; i++) {
>> +        struct clk *clk;
>> +
>> +        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;
>> +    }
>> +
>> +    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->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");
>> +
>> +    ret = devm_pm_runtime_enable(&pdev->dev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    platform_set_drvdata(pdev, hdmi);
>> +
>>       return component_add(&pdev->dev, &msm_hdmi_ops);
>>   }

Re: [Freedreno] [PATCH v2 3/5] drm/msm/hdmi: move resource allocation to probe function

[Abhinav Kumar]

On 9/7/2022 12:09 PM, Abhinav Kumar wrote:
> 
> 
> On 9/7/2022 11:54 AM, Abhinav Kumar wrote:
>>
>>
>> On 8/26/2022 2:39 AM, Dmitry Baryshkov wrote:
>>> Rather than having all resource allocation happen in the _bind function
>>> (resulting in possible EPROBE_DEFER returns and component bind/unbind
>>> cycles) allocate and check all resources in _probe function. While we
>>> are at it, use platform_get_irq() to get the IRQ rather than going
>>> through the irq_of_parse_and_map().
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
The change itself LGTM, so with some change log added, this is
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/hdmi/hdmi.c | 303 +++++++++++++++-----------------
>>>   1 file changed, 138 insertions(+), 165 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c 
>>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> index 4a364d8f4c0b..c298a36f3b42 100644
>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> @@ -76,8 +76,6 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>>>       if (hdmi->i2c)
>>>           msm_hdmi_i2c_destroy(hdmi->i2c);
>>> -
>>> -    platform_set_drvdata(hdmi->pdev, NULL);
>>>   }
>>>   static int msm_hdmi_get_phy(struct hdmi *hdmi)
>>
>> Between v1 and v2, it just seems like a rebase to me on top of the 6.1 
>> MR. But what about moving msm_hdmi_get_phy() to probe(). I thought you 
>> were going to check that as well for v2.
> 
> Please ignore this part of the comment, I see that moving 
> msm_hdmi_get_phy() to probe() is in the patch 5 of this series.
> 
> Thanks for making that change.
> 
> The below comment still holds true.
> 
>>
>> A change log would have been nice here. Because as part of the rebase 
>> looks like we even migrate to using panel bridge for hdmi driver.
>>
>> Usage of drm_of_find_panel_or_bridge was not present in v1 and wasnt 
>> obvious from the commit text either.
>>
>>> @@ -117,142 +115,10 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
>>>    * we are to EPROBE_DEFER we want to do it here, rather than later
>>>    * at modeset_init() time
>>>    */
>>> -static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
>>> +static int msm_hdmi_init(struct hdmi *hdmi)
>>>   {
>>> -    struct hdmi_platform_config *config = pdev->dev.platform_data;
>>> -    struct hdmi *hdmi = NULL;
>>> -    struct resource *res;
>>> -    int i, ret;
>>> -
>>> -    hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
>>> -    if (!hdmi) {
>>> -        ret = -ENOMEM;
>>> -        goto fail;
>>> -    }
>>> -
>>> -    hdmi->pdev = pdev;
>>> -    hdmi->config = config;
>>> -    spin_lock_init(&hdmi->reg_lock);
>>> -
>>> -    ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, 
>>> &hdmi->next_bridge);
>>> -    if (ret && ret != -ENODEV)
>>> -        goto fail;
>>> -
>>> -    hdmi->mmio = msm_ioremap(pdev, "core_physical");
>>> -    if (IS_ERR(hdmi->mmio)) {
>>> -        ret = PTR_ERR(hdmi->mmio);
>>> -        goto fail;
>>> -    }
>>> -
>>> -    /* HDCP needs physical address of hdmi register */
>>> -    res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>> -        "core_physical");
>>> -    if (!res) {
>>> -        ret = -EINVAL;
>>> -        goto fail;
>>> -    }
>>> -    hdmi->mmio_phy_addr = res->start;
>>> -
>>> -    hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
>>> -    if (IS_ERR(hdmi->qfprom_mmio)) {
>>> -        DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
>>> -        hdmi->qfprom_mmio = NULL;
>>> -    }
>>> -
>>> -    hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
>>> -                      config->hpd_reg_cnt,
>>> -                      sizeof(hdmi->hpd_regs[0]),
>>> -                      GFP_KERNEL);
>>> -    if (!hdmi->hpd_regs) {
>>> -        ret = -ENOMEM;
>>> -        goto fail;
>>> -    }
>>> -    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) {
>>> -        DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: 
>>> %d\n", ret);
>>> -        goto fail;
>>> -    }
>>> -
>>> -    hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
>>> -                      config->pwr_reg_cnt,
>>> -                      sizeof(hdmi->pwr_regs[0]),
>>> -                      GFP_KERNEL);
>>> -    if (!hdmi->pwr_regs) {
>>> -        ret = -ENOMEM;
>>> -        goto fail;
>>> -    }
>>> -
>>> -    for (i = 0; i < config->pwr_reg_cnt; i++)
>>> -        hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
>>> -
>>> -    ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, 
>>> hdmi->pwr_regs);
>>> -    if (ret) {
>>> -        DRM_DEV_ERROR(&pdev->dev, "failed to get pwr regulator: 
>>> %d\n", ret);
>>> -        goto fail;
>>> -    }
>>> -
>>> -    hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
>>> -                      config->hpd_clk_cnt,
>>> -                      sizeof(hdmi->hpd_clks[0]),
>>> -                      GFP_KERNEL);
>>> -    if (!hdmi->hpd_clks) {
>>> -        ret = -ENOMEM;
>>> -        goto fail;
>>> -    }
>>> -    for (i = 0; i < config->hpd_clk_cnt; i++) {
>>> -        struct clk *clk;
>>> -
>>> -        clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
>>> -        if (IS_ERR(clk)) {
>>> -            ret = PTR_ERR(clk);
>>> -            DRM_DEV_ERROR(&pdev->dev, "failed to get hpd clk: %s 
>>> (%d)\n",
>>> -                    config->hpd_clk_names[i], ret);
>>> -            goto fail;
>>> -        }
>>> -
>>> -        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) {
>>> -        ret = -ENOMEM;
>>> -        goto fail;
>>> -    }
>>> -    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)) {
>>> -            ret = PTR_ERR(clk);
>>> -            DRM_DEV_ERROR(&pdev->dev, "failed to get pwr clk: %s 
>>> (%d)\n",
>>> -                    config->pwr_clk_names[i], ret);
>>> -            goto fail;
>>> -        }
>>> -
>>> -        hdmi->pwr_clks[i] = clk;
>>> -    }
>>> -
>>> -    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)) {
>>> -        ret = PTR_ERR(hdmi->hpd_gpiod);
>>> -        DRM_DEV_ERROR(&pdev->dev, "failed to get hpd gpio: (%d)\n", 
>>> ret);
>>> -        goto fail;
>>> -    }
>>> -
>>> -    if (!hdmi->hpd_gpiod)
>>> -        DBG("failed to get HPD gpio");
>>> -
>>> -    if (hdmi->hpd_gpiod)
>>> -        gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
>>> -
>>> -    devm_pm_runtime_enable(&pdev->dev);
>>> +    struct platform_device *pdev = hdmi->pdev;
>>> +    int ret;
>>>       hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
>>> @@ -276,13 +142,13 @@ static struct hdmi *msm_hdmi_init(struct 
>>> platform_device *pdev)
>>>           hdmi->hdcp_ctrl = NULL;
>>>       }
>>> -    return hdmi;
>>> +    return 0;
>>>   fail:
>>>       if (hdmi)
>>>           msm_hdmi_destroy(hdmi);
>>> -    return ERR_PTR(ret);
>>> +    return ret;
>>>   }
>>>   /* Second part of initialization, the drm/kms level modeset_init,
>>> @@ -332,13 +198,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>       drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
>>> -    hdmi->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>>> -    if (!hdmi->irq) {
>>> -        ret = -EINVAL;
>>> -        DRM_DEV_ERROR(dev->dev, "failed to get irq\n");
>>> -        goto fail;
>>> -    }
>>> -
>>>       ret = devm_request_irq(&pdev->dev, hdmi->irq,
>>>               msm_hdmi_irq, IRQF_TRIGGER_HIGH,
>>>               "hdmi_isr", hdmi);
>>> @@ -358,8 +217,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>       priv->bridges[priv->num_bridges++]       = hdmi->bridge;
>>> -    platform_set_drvdata(pdev, hdmi);
>>> -
>>>       return 0;
>>>   fail:
>>> @@ -387,7 +244,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>   static const char *hpd_reg_names_8960[] = {"core-vdda"};
>>>   static const char *hpd_clk_names_8960[] = {"core", "master_iface", 
>>> "slave_iface"};
>>> -static struct hdmi_platform_config hdmi_tx_8960_config = {
>>> +const static struct hdmi_platform_config hdmi_tx_8960_config = {
>>>           HDMI_CFG(hpd_reg, 8960),
>>>           HDMI_CFG(hpd_clk, 8960),
>>>   };
>>> @@ -397,7 +254,7 @@ 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 struct hdmi_platform_config hdmi_tx_8974_config = {
>>> +const static struct hdmi_platform_config hdmi_tx_8974_config = {
>>>           HDMI_CFG(pwr_reg, 8x74),
>>>           HDMI_CFG(pwr_clk, 8x74),
>>>           HDMI_CFG(hpd_clk, 8x74),
>>> @@ -512,23 +369,12 @@ static int 
>>> msm_hdmi_register_audio_driver(struct hdmi *hdmi, struct device *dev)
>>>   static int msm_hdmi_bind(struct device *dev, struct device *master, 
>>> void *data)
>>>   {
>>>       struct msm_drm_private *priv = dev_get_drvdata(master);
>>> -    struct hdmi_platform_config *hdmi_cfg;
>>> -    struct hdmi *hdmi;
>>> -    struct device_node *of_node = dev->of_node;
>>> +    struct hdmi *hdmi = dev_get_drvdata(dev);
>>>       int err;
>>> -    hdmi_cfg = (struct hdmi_platform_config *)
>>> -            of_device_get_match_data(dev);
>>> -    if (!hdmi_cfg) {
>>> -        DRM_DEV_ERROR(dev, "unknown hdmi_cfg: %pOFn\n", of_node);
>>> -        return -ENXIO;
>>> -    }
>>> -
>>> -    dev->platform_data = hdmi_cfg;
>>> -
>>> -    hdmi = msm_hdmi_init(to_platform_device(dev));
>>> -    if (IS_ERR(hdmi))
>>> -        return PTR_ERR(hdmi);
>>> +    err = msm_hdmi_init(hdmi);
>>> +    if (err)
>>> +        return err;
>>>       priv->hdmi = hdmi;
>>>       err = msm_hdmi_register_audio_driver(hdmi, dev);
>>> @@ -561,6 +407,133 @@ static const struct component_ops msm_hdmi_ops = {
>>>   static int msm_hdmi_dev_probe(struct platform_device *pdev)
>>>   {
>>> +    const struct hdmi_platform_config *config;
>>> +    struct device *dev = &pdev->dev;
>>> +    struct hdmi *hdmi;
>>> +    struct resource *res;
>>> +    int i, ret;
>>> +
>>> +    config = of_device_get_match_data(dev);
>>> +    if (!config)
>>> +        return -EINVAL;
>>> +
>>> +    hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
>>> +    if (!hdmi)
>>> +        return -ENOMEM;
>>> +
>>> +    hdmi->pdev = pdev;
>>> +    hdmi->config = config;
>>> +    spin_lock_init(&hdmi->reg_lock);
>>> +
>>> +    ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, 
>>> &hdmi->next_bridge);
>>> +    if (ret && ret != -ENODEV)
>>> +        return ret;
>>> +
>>> +    hdmi->mmio = msm_ioremap(pdev, "core_physical");
>>> +    if (IS_ERR(hdmi->mmio))
>>> +        return PTR_ERR(hdmi->mmio);
>>> +
>>> +    /* HDCP needs physical address of hdmi register */
>>> +    res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>> +        "core_physical");
>>> +    if (!res)
>>> +        return -EINVAL;
>>> +    hdmi->mmio_phy_addr = res->start;
>>> +
>>> +    hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
>>> +    if (IS_ERR(hdmi->qfprom_mmio)) {
>>> +        DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
>>> +        hdmi->qfprom_mmio = NULL;
>>> +    }
>>> +
>>> +    hdmi->irq = platform_get_irq(pdev, 0);
>>> +    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]),
>>> +                      GFP_KERNEL);
>>> +    if (!hdmi->pwr_regs)
>>> +        return -ENOMEM;
>>> +
>>> +    for (i = 0; i < config->pwr_reg_cnt; i++)
>>> +        hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
>>> +
>>> +    ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, 
>>> hdmi->pwr_regs);
>>> +    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]),
>>> +                      GFP_KERNEL);
>>> +    if (!hdmi->hpd_clks)
>>> +        return -ENOMEM;
>>> +
>>> +    for (i = 0; i < config->hpd_clk_cnt; i++) {
>>> +        struct clk *clk;
>>> +
>>> +        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;
>>> +    }
>>> +
>>> +    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->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");
>>> +
>>> +    ret = devm_pm_runtime_enable(&pdev->dev);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    platform_set_drvdata(pdev, hdmi);
>>> +
>>>       return component_add(&pdev->dev, &msm_hdmi_ops);
>>>   }

Re: [Freedreno] [PATCH v2 3/5] drm/msm/hdmi: move resource allocation to probe function

[Abhinav Kumar]

On 9/7/2022 12:09 PM, Abhinav Kumar wrote:
> 
> 
> On 9/7/2022 11:54 AM, Abhinav Kumar wrote:
>>
>>
>> On 8/26/2022 2:39 AM, Dmitry Baryshkov wrote:
>>> Rather than having all resource allocation happen in the _bind function
>>> (resulting in possible EPROBE_DEFER returns and component bind/unbind
>>> cycles) allocate and check all resources in _probe function. While we
>>> are at it, use platform_get_irq() to get the IRQ rather than going
>>> through the irq_of_parse_and_map().
>>>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
The change itself LGTM, so with some change log added, this is
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/hdmi/hdmi.c | 303 +++++++++++++++-----------------
>>>   1 file changed, 138 insertions(+), 165 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c 
>>> b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> index 4a364d8f4c0b..c298a36f3b42 100644
>>> --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
>>> @@ -76,8 +76,6 @@ static void msm_hdmi_destroy(struct hdmi *hdmi)
>>>       if (hdmi->i2c)
>>>           msm_hdmi_i2c_destroy(hdmi->i2c);
>>> -
>>> -    platform_set_drvdata(hdmi->pdev, NULL);
>>>   }
>>>   static int msm_hdmi_get_phy(struct hdmi *hdmi)
>>
>> Between v1 and v2, it just seems like a rebase to me on top of the 6.1 
>> MR. But what about moving msm_hdmi_get_phy() to probe(). I thought you 
>> were going to check that as well for v2.
> 
> Please ignore this part of the comment, I see that moving 
> msm_hdmi_get_phy() to probe() is in the patch 5 of this series.
> 
> Thanks for making that change.
> 
> The below comment still holds true.
> 
>>
>> A change log would have been nice here. Because as part of the rebase 
>> looks like we even migrate to using panel bridge for hdmi driver.
>>
>> Usage of drm_of_find_panel_or_bridge was not present in v1 and wasnt 
>> obvious from the commit text either.
>>
>>> @@ -117,142 +115,10 @@ static int msm_hdmi_get_phy(struct hdmi *hdmi)
>>>    * we are to EPROBE_DEFER we want to do it here, rather than later
>>>    * at modeset_init() time
>>>    */
>>> -static struct hdmi *msm_hdmi_init(struct platform_device *pdev)
>>> +static int msm_hdmi_init(struct hdmi *hdmi)
>>>   {
>>> -    struct hdmi_platform_config *config = pdev->dev.platform_data;
>>> -    struct hdmi *hdmi = NULL;
>>> -    struct resource *res;
>>> -    int i, ret;
>>> -
>>> -    hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
>>> -    if (!hdmi) {
>>> -        ret = -ENOMEM;
>>> -        goto fail;
>>> -    }
>>> -
>>> -    hdmi->pdev = pdev;
>>> -    hdmi->config = config;
>>> -    spin_lock_init(&hdmi->reg_lock);
>>> -
>>> -    ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, 
>>> &hdmi->next_bridge);
>>> -    if (ret && ret != -ENODEV)
>>> -        goto fail;
>>> -
>>> -    hdmi->mmio = msm_ioremap(pdev, "core_physical");
>>> -    if (IS_ERR(hdmi->mmio)) {
>>> -        ret = PTR_ERR(hdmi->mmio);
>>> -        goto fail;
>>> -    }
>>> -
>>> -    /* HDCP needs physical address of hdmi register */
>>> -    res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>> -        "core_physical");
>>> -    if (!res) {
>>> -        ret = -EINVAL;
>>> -        goto fail;
>>> -    }
>>> -    hdmi->mmio_phy_addr = res->start;
>>> -
>>> -    hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
>>> -    if (IS_ERR(hdmi->qfprom_mmio)) {
>>> -        DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
>>> -        hdmi->qfprom_mmio = NULL;
>>> -    }
>>> -
>>> -    hdmi->hpd_regs = devm_kcalloc(&pdev->dev,
>>> -                      config->hpd_reg_cnt,
>>> -                      sizeof(hdmi->hpd_regs[0]),
>>> -                      GFP_KERNEL);
>>> -    if (!hdmi->hpd_regs) {
>>> -        ret = -ENOMEM;
>>> -        goto fail;
>>> -    }
>>> -    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) {
>>> -        DRM_DEV_ERROR(&pdev->dev, "failed to get hpd regulator: 
>>> %d\n", ret);
>>> -        goto fail;
>>> -    }
>>> -
>>> -    hdmi->pwr_regs = devm_kcalloc(&pdev->dev,
>>> -                      config->pwr_reg_cnt,
>>> -                      sizeof(hdmi->pwr_regs[0]),
>>> -                      GFP_KERNEL);
>>> -    if (!hdmi->pwr_regs) {
>>> -        ret = -ENOMEM;
>>> -        goto fail;
>>> -    }
>>> -
>>> -    for (i = 0; i < config->pwr_reg_cnt; i++)
>>> -        hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
>>> -
>>> -    ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, 
>>> hdmi->pwr_regs);
>>> -    if (ret) {
>>> -        DRM_DEV_ERROR(&pdev->dev, "failed to get pwr regulator: 
>>> %d\n", ret);
>>> -        goto fail;
>>> -    }
>>> -
>>> -    hdmi->hpd_clks = devm_kcalloc(&pdev->dev,
>>> -                      config->hpd_clk_cnt,
>>> -                      sizeof(hdmi->hpd_clks[0]),
>>> -                      GFP_KERNEL);
>>> -    if (!hdmi->hpd_clks) {
>>> -        ret = -ENOMEM;
>>> -        goto fail;
>>> -    }
>>> -    for (i = 0; i < config->hpd_clk_cnt; i++) {
>>> -        struct clk *clk;
>>> -
>>> -        clk = msm_clk_get(pdev, config->hpd_clk_names[i]);
>>> -        if (IS_ERR(clk)) {
>>> -            ret = PTR_ERR(clk);
>>> -            DRM_DEV_ERROR(&pdev->dev, "failed to get hpd clk: %s 
>>> (%d)\n",
>>> -                    config->hpd_clk_names[i], ret);
>>> -            goto fail;
>>> -        }
>>> -
>>> -        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) {
>>> -        ret = -ENOMEM;
>>> -        goto fail;
>>> -    }
>>> -    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)) {
>>> -            ret = PTR_ERR(clk);
>>> -            DRM_DEV_ERROR(&pdev->dev, "failed to get pwr clk: %s 
>>> (%d)\n",
>>> -                    config->pwr_clk_names[i], ret);
>>> -            goto fail;
>>> -        }
>>> -
>>> -        hdmi->pwr_clks[i] = clk;
>>> -    }
>>> -
>>> -    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)) {
>>> -        ret = PTR_ERR(hdmi->hpd_gpiod);
>>> -        DRM_DEV_ERROR(&pdev->dev, "failed to get hpd gpio: (%d)\n", 
>>> ret);
>>> -        goto fail;
>>> -    }
>>> -
>>> -    if (!hdmi->hpd_gpiod)
>>> -        DBG("failed to get HPD gpio");
>>> -
>>> -    if (hdmi->hpd_gpiod)
>>> -        gpiod_set_consumer_name(hdmi->hpd_gpiod, "HDMI_HPD");
>>> -
>>> -    devm_pm_runtime_enable(&pdev->dev);
>>> +    struct platform_device *pdev = hdmi->pdev;
>>> +    int ret;
>>>       hdmi->workq = alloc_ordered_workqueue("msm_hdmi", 0);
>>> @@ -276,13 +142,13 @@ static struct hdmi *msm_hdmi_init(struct 
>>> platform_device *pdev)
>>>           hdmi->hdcp_ctrl = NULL;
>>>       }
>>> -    return hdmi;
>>> +    return 0;
>>>   fail:
>>>       if (hdmi)
>>>           msm_hdmi_destroy(hdmi);
>>> -    return ERR_PTR(ret);
>>> +    return ret;
>>>   }
>>>   /* Second part of initialization, the drm/kms level modeset_init,
>>> @@ -332,13 +198,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>       drm_connector_attach_encoder(hdmi->connector, hdmi->encoder);
>>> -    hdmi->irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>>> -    if (!hdmi->irq) {
>>> -        ret = -EINVAL;
>>> -        DRM_DEV_ERROR(dev->dev, "failed to get irq\n");
>>> -        goto fail;
>>> -    }
>>> -
>>>       ret = devm_request_irq(&pdev->dev, hdmi->irq,
>>>               msm_hdmi_irq, IRQF_TRIGGER_HIGH,
>>>               "hdmi_isr", hdmi);
>>> @@ -358,8 +217,6 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>       priv->bridges[priv->num_bridges++]       = hdmi->bridge;
>>> -    platform_set_drvdata(pdev, hdmi);
>>> -
>>>       return 0;
>>>   fail:
>>> @@ -387,7 +244,7 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
>>>   static const char *hpd_reg_names_8960[] = {"core-vdda"};
>>>   static const char *hpd_clk_names_8960[] = {"core", "master_iface", 
>>> "slave_iface"};
>>> -static struct hdmi_platform_config hdmi_tx_8960_config = {
>>> +const static struct hdmi_platform_config hdmi_tx_8960_config = {
>>>           HDMI_CFG(hpd_reg, 8960),
>>>           HDMI_CFG(hpd_clk, 8960),
>>>   };
>>> @@ -397,7 +254,7 @@ 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 struct hdmi_platform_config hdmi_tx_8974_config = {
>>> +const static struct hdmi_platform_config hdmi_tx_8974_config = {
>>>           HDMI_CFG(pwr_reg, 8x74),
>>>           HDMI_CFG(pwr_clk, 8x74),
>>>           HDMI_CFG(hpd_clk, 8x74),
>>> @@ -512,23 +369,12 @@ static int 
>>> msm_hdmi_register_audio_driver(struct hdmi *hdmi, struct device *dev)
>>>   static int msm_hdmi_bind(struct device *dev, struct device *master, 
>>> void *data)
>>>   {
>>>       struct msm_drm_private *priv = dev_get_drvdata(master);
>>> -    struct hdmi_platform_config *hdmi_cfg;
>>> -    struct hdmi *hdmi;
>>> -    struct device_node *of_node = dev->of_node;
>>> +    struct hdmi *hdmi = dev_get_drvdata(dev);
>>>       int err;
>>> -    hdmi_cfg = (struct hdmi_platform_config *)
>>> -            of_device_get_match_data(dev);
>>> -    if (!hdmi_cfg) {
>>> -        DRM_DEV_ERROR(dev, "unknown hdmi_cfg: %pOFn\n", of_node);
>>> -        return -ENXIO;
>>> -    }
>>> -
>>> -    dev->platform_data = hdmi_cfg;
>>> -
>>> -    hdmi = msm_hdmi_init(to_platform_device(dev));
>>> -    if (IS_ERR(hdmi))
>>> -        return PTR_ERR(hdmi);
>>> +    err = msm_hdmi_init(hdmi);
>>> +    if (err)
>>> +        return err;
>>>       priv->hdmi = hdmi;
>>>       err = msm_hdmi_register_audio_driver(hdmi, dev);
>>> @@ -561,6 +407,133 @@ static const struct component_ops msm_hdmi_ops = {
>>>   static int msm_hdmi_dev_probe(struct platform_device *pdev)
>>>   {
>>> +    const struct hdmi_platform_config *config;
>>> +    struct device *dev = &pdev->dev;
>>> +    struct hdmi *hdmi;
>>> +    struct resource *res;
>>> +    int i, ret;
>>> +
>>> +    config = of_device_get_match_data(dev);
>>> +    if (!config)
>>> +        return -EINVAL;
>>> +
>>> +    hdmi = devm_kzalloc(&pdev->dev, sizeof(*hdmi), GFP_KERNEL);
>>> +    if (!hdmi)
>>> +        return -ENOMEM;
>>> +
>>> +    hdmi->pdev = pdev;
>>> +    hdmi->config = config;
>>> +    spin_lock_init(&hdmi->reg_lock);
>>> +
>>> +    ret = drm_of_find_panel_or_bridge(pdev->dev.of_node, 1, 0, NULL, 
>>> &hdmi->next_bridge);
>>> +    if (ret && ret != -ENODEV)
>>> +        return ret;
>>> +
>>> +    hdmi->mmio = msm_ioremap(pdev, "core_physical");
>>> +    if (IS_ERR(hdmi->mmio))
>>> +        return PTR_ERR(hdmi->mmio);
>>> +
>>> +    /* HDCP needs physical address of hdmi register */
>>> +    res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>>> +        "core_physical");
>>> +    if (!res)
>>> +        return -EINVAL;
>>> +    hdmi->mmio_phy_addr = res->start;
>>> +
>>> +    hdmi->qfprom_mmio = msm_ioremap(pdev, "qfprom_physical");
>>> +    if (IS_ERR(hdmi->qfprom_mmio)) {
>>> +        DRM_DEV_INFO(&pdev->dev, "can't find qfprom resource\n");
>>> +        hdmi->qfprom_mmio = NULL;
>>> +    }
>>> +
>>> +    hdmi->irq = platform_get_irq(pdev, 0);
>>> +    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]),
>>> +                      GFP_KERNEL);
>>> +    if (!hdmi->pwr_regs)
>>> +        return -ENOMEM;
>>> +
>>> +    for (i = 0; i < config->pwr_reg_cnt; i++)
>>> +        hdmi->pwr_regs[i].supply = config->pwr_reg_names[i];
>>> +
>>> +    ret = devm_regulator_bulk_get(&pdev->dev, config->pwr_reg_cnt, 
>>> hdmi->pwr_regs);
>>> +    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]),
>>> +                      GFP_KERNEL);
>>> +    if (!hdmi->hpd_clks)
>>> +        return -ENOMEM;
>>> +
>>> +    for (i = 0; i < config->hpd_clk_cnt; i++) {
>>> +        struct clk *clk;
>>> +
>>> +        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;
>>> +    }
>>> +
>>> +    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->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");
>>> +
>>> +    ret = devm_pm_runtime_enable(&pdev->dev);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    platform_set_drvdata(pdev, hdmi);
>>> +
>>>       return component_add(&pdev->dev, &msm_hdmi_ops);
>>>   }