Re: [PATCH v2 02/13] drm/msm/hdmi: switch to devm_drm_bridge_add()

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

 



On 09/10/2023 22:19, Abhinav Kumar wrote:


On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
Make MSM HDMI driver use devm_drm_bridge_add() instead of plain
drm_bridge_add(). As the driver doesn't require any additional cleanup,
stop adding created bridge to the priv->bridges array.

Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
---
  drivers/gpu/drm/msm/hdmi/hdmi.c        | 22 +++++--------------
  drivers/gpu/drm/msm/hdmi/hdmi.h        |  5 ++---
  drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 30 ++++++++------------------
  drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    |  3 +--
  4 files changed, 17 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c b/drivers/gpu/drm/msm/hdmi/hdmi.c
index b6bcb9f675fe..c8ebd75176bb 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
@@ -160,24 +160,16 @@ static int msm_hdmi_init(struct hdmi *hdmi)
  int msm_hdmi_modeset_init(struct hdmi *hdmi,
          struct drm_device *dev, struct drm_encoder *encoder)
  {
-    struct msm_drm_private *priv = dev->dev_private;
      int ret;
-    if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
-        DRM_DEV_ERROR(dev->dev, "too many bridges\n");
-        return -ENOSPC;
-    }
-
      hdmi->dev = dev;
      hdmi->encoder = encoder;
      hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
-    hdmi->bridge = msm_hdmi_bridge_init(hdmi);
-    if (IS_ERR(hdmi->bridge)) {
-        ret = PTR_ERR(hdmi->bridge);
+    ret = msm_hdmi_bridge_init(hdmi);
+    if (ret) {
          DRM_DEV_ERROR(dev->dev, "failed to create HDMI bridge: %d\n", ret);
-        hdmi->bridge = NULL;
          goto fail;
      }
@@ -215,16 +207,9 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
          goto fail;
      }
-    priv->bridges[priv->num_bridges++]       = hdmi->bridge;
-
      return 0;
  fail:
-    /* bridge is normally destroyed by drm: */
-    if (hdmi->bridge) {
-        msm_hdmi_bridge_destroy(hdmi->bridge);
-        hdmi->bridge = NULL;
-    }
      if (hdmi->connector) {
          hdmi->connector->funcs->destroy(hdmi->connector);
          hdmi->connector = NULL;
@@ -395,6 +380,9 @@ static void msm_hdmi_unbind(struct device *dev, struct device *master,
          if (priv->hdmi->audio_pdev)
              platform_device_unregister(priv->hdmi->audio_pdev);
+        if (priv->hdmi->bridge)
+            msm_hdmi_hpd_disable(priv->hdmi);
+

Now is this the only place where hdmi->bridge is used?

Why cant we just keep msm_hdmi_hpd_disable(priv->hdmi) here since its anyway protected by if (priv->hdmi) and drop hdmi->bridge completely?

Sure, sounds like a good idea, same followup as for the DSI.


          msm_hdmi_destroy(priv->hdmi);
          priv->hdmi = NULL;
      }
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h b/drivers/gpu/drm/msm/hdmi/hdmi.h
index e8dbee50637f..ec5786440391 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi.h
+++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
@@ -224,14 +224,13 @@ void msm_hdmi_audio_set_sample_rate(struct hdmi *hdmi, int rate);
   * hdmi bridge:
   */
-struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi);
-void msm_hdmi_bridge_destroy(struct drm_bridge *bridge);
+int msm_hdmi_bridge_init(struct hdmi *hdmi);
  void msm_hdmi_hpd_irq(struct drm_bridge *bridge);
  enum drm_connector_status msm_hdmi_bridge_detect(
          struct drm_bridge *bridge);
  int msm_hdmi_hpd_enable(struct drm_bridge *bridge);
-void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge);
+void msm_hdmi_hpd_disable(struct hdmi *hdmi);
  /*
   * i2c adapter for ddc:
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
index 9b1391d27ed3..0b7a6a56677e 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
@@ -11,14 +11,6 @@
  #include "msm_kms.h"
  #include "hdmi.h"
-void msm_hdmi_bridge_destroy(struct drm_bridge *bridge)
-{
-    struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
-
-    msm_hdmi_hpd_disable(hdmi_bridge);
-    drm_bridge_remove(bridge);
-}
-
  static void msm_hdmi_power_on(struct drm_bridge *bridge)
  {
      struct drm_device *dev = bridge->dev;
@@ -317,7 +309,7 @@ msm_hdmi_hotplug_work(struct work_struct *work)
  }
  /* initialize bridge */
-struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
+int msm_hdmi_bridge_init(struct hdmi *hdmi)
  {
      struct drm_bridge *bridge = NULL;
      struct hdmi_bridge *hdmi_bridge;
@@ -325,10 +317,8 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
      hdmi_bridge = devm_kzalloc(hdmi->dev->dev,
              sizeof(*hdmi_bridge), GFP_KERNEL);
-    if (!hdmi_bridge) {
-        ret = -ENOMEM;
-        goto fail;
-    }
+    if (!hdmi_bridge)
+        return -ENOMEM;
      hdmi_bridge->hdmi = hdmi;
      INIT_WORK(&hdmi_bridge->hpd_work, msm_hdmi_hotplug_work);
@@ -341,17 +331,15 @@ struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
          DRM_BRIDGE_OP_DETECT |
          DRM_BRIDGE_OP_EDID;
-    drm_bridge_add(bridge);
+    ret = devm_drm_bridge_add(&hdmi->pdev->dev, bridge);
+    if (ret)
+        return ret;
      ret = drm_bridge_attach(hdmi->encoder, bridge, NULL, DRM_BRIDGE_ATTACH_NO_CONNECTOR);
      if (ret)
-        goto fail;
+        return ret;
-    return bridge;
+    hdmi->bridge = bridge;
-fail:
-    if (bridge)
-        msm_hdmi_bridge_destroy(bridge);
-
-    return ERR_PTR(ret);
+    return 0;
  }
diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
index bfa827b47989..9ce0ffa35417 100644
--- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
+++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
@@ -147,9 +147,8 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
      return ret;
  }
-void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge)
+void msm_hdmi_hpd_disable(struct hdmi *hdmi)
  {
-    struct hdmi *hdmi = hdmi_bridge->hdmi;
      const struct hdmi_platform_config *config = hdmi->config;
      struct device *dev = &hdmi->pdev->dev;
      int ret;

--
With best wishes
Dmitry




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [Linux for Sparc]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux