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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxx>
---
  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);
  }



[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux