[PATCH 1/3] drm/msm/dp: Drop now unused dp_hpd module

[PATCH 1/3] drm/msm/dp: Drop now unused dp_hpd module

[Bjorn Andersson]

The dp_hpd module is a remnant from the downstream design and is now
completely unused. Drop it and all references to it.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_debug.c   |  8 ++-
 drivers/gpu/drm/msm/dp/dp_debug.h   | 12 ++---
 drivers/gpu/drm/msm/dp/dp_display.c | 35 +------------
 drivers/gpu/drm/msm/dp/dp_hpd.c     | 67 -------------------------
 drivers/gpu/drm/msm/dp/dp_hpd.h     | 78 -----------------------------
 drivers/gpu/drm/msm/dp/dp_panel.h   |  1 -
 6 files changed, 11 insertions(+), 190 deletions(-)
 delete mode 100644 drivers/gpu/drm/msm/dp/dp_hpd.c
 delete mode 100644 drivers/gpu/drm/msm/dp/dp_hpd.h

diff --git a/drivers/gpu/drm/msm/dp/dp_debug.c b/drivers/gpu/drm/msm/dp/dp_debug.c
index 5e35033ba3e4..cdd72474197b 100644
--- a/drivers/gpu/drm/msm/dp/dp_debug.c
+++ b/drivers/gpu/drm/msm/dp/dp_debug.c
@@ -21,7 +21,6 @@
 struct dp_debug_private {
 	struct dentry *root;
 
-	struct dp_usbpd *usbpd;
 	struct dp_link *link;
 	struct dp_panel *panel;
 	struct drm_connector *connector;
@@ -232,14 +231,14 @@ static void dp_debug_init(struct dp_debug *dp_debug, struct drm_minor *minor)
 }
 
 struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel *panel,
-		struct dp_usbpd *usbpd, struct dp_link *link,
-		struct drm_connector *connector, struct drm_minor *minor)
+			      struct dp_link *link, struct drm_connector *connector,
+			      struct drm_minor *minor)
 {
 	struct dp_debug_private *debug;
 	struct dp_debug *dp_debug;
 	int rc;
 
-	if (!dev || !panel || !usbpd || !link) {
+	if (!dev || !panel || !link) {
 		DRM_ERROR("invalid input\n");
 		rc = -EINVAL;
 		goto error;
@@ -252,7 +251,6 @@ struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel *panel,
 	}
 
 	debug->dp_debug.debug_en = false;
-	debug->usbpd = usbpd;
 	debug->link = link;
 	debug->panel = panel;
 	debug->dev = dev;
diff --git a/drivers/gpu/drm/msm/dp/dp_debug.h b/drivers/gpu/drm/msm/dp/dp_debug.h
index 8c0d0b5178fd..d99ef4532c9c 100644
--- a/drivers/gpu/drm/msm/dp/dp_debug.h
+++ b/drivers/gpu/drm/msm/dp/dp_debug.h
@@ -32,7 +32,6 @@ struct dp_debug {
  *
  * @dev: device instance of the caller
  * @panel: instance of panel module
- * @usbpd: instance of usbpd module
  * @link: instance of link module
  * @connector: double pointer to display connector
  * @minor: pointer to drm minor number after device registration
@@ -42,9 +41,9 @@ struct dp_debug {
  * for debugfs input to be communicated with existing modules
  */
 struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel *panel,
-		struct dp_usbpd *usbpd, struct dp_link *link,
-		struct drm_connector *connector,
-		struct drm_minor *minor);
+			      struct dp_link *link,
+			      struct drm_connector *connector,
+			      struct drm_minor *minor);
 
 /**
  * dp_debug_put()
@@ -59,8 +58,9 @@ void dp_debug_put(struct dp_debug *dp_debug);
 
 static inline
 struct dp_debug *dp_debug_get(struct device *dev, struct dp_panel *panel,
-		struct dp_usbpd *usbpd, struct dp_link *link,
-		struct drm_connector *connector, struct drm_minor *minor)
+			      struct dp_link *link,
+			      struct drm_connector *connector,
+			      struct drm_minor *minor)
 {
 	return ERR_PTR(-EINVAL);
 }
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 0477263bb55c..4df52ad2b462 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -14,7 +14,6 @@
 
 #include "msm_drv.h"
 #include "msm_kms.h"
-#include "dp_hpd.h"
 #include "dp_parser.h"
 #include "dp_power.h"
 #include "dp_catalog.h"
@@ -92,7 +91,6 @@ struct dp_display_private {
 	struct platform_device *pdev;
 	struct dentry *root;
 
-	struct dp_usbpd   *usbpd;
 	struct dp_parser  *parser;
 	struct dp_power   *power;
 	struct dp_catalog *catalog;
@@ -102,7 +100,6 @@ struct dp_display_private {
 	struct dp_ctrl    *ctrl;
 	struct dp_debug   *debug;
 
-	struct dp_usbpd_cb usbpd_cb;
 	struct dp_display_mode dp_mode;
 	struct msm_dp dp_display;
 
@@ -493,11 +490,6 @@ static int dp_display_usbpd_configure_cb(struct device *dev)
 	return dp_display_process_hpd_high(dp);
 }
 
-static int dp_display_usbpd_disconnect_cb(struct device *dev)
-{
-	return 0;
-}
-
 static int dp_display_notify_disconnect(struct device *dev)
 {
 	struct dp_display_private *dp = dev_get_dp_display_private(dev);
@@ -582,13 +574,9 @@ static int dp_display_usbpd_attention_cb(struct device *dev)
 
 static int dp_hpd_plug_handle(struct dp_display_private *dp, u32 data)
 {
-	struct dp_usbpd *hpd = dp->usbpd;
 	u32 state;
 	int ret;
 
-	if (!hpd)
-		return 0;
-
 	mutex_lock(&dp->event_mutex);
 
 	state =  dp->hpd_state;
@@ -649,12 +637,8 @@ static void dp_display_handle_plugged_change(struct msm_dp *dp_display,
 
 static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data)
 {
-	struct dp_usbpd *hpd = dp->usbpd;
 	u32 state;
 
-	if (!hpd)
-		return 0;
-
 	mutex_lock(&dp->event_mutex);
 
 	state = dp->hpd_state;
@@ -767,24 +751,10 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
 {
 	int rc = 0;
 	struct device *dev = &dp->pdev->dev;
-	struct dp_usbpd_cb *cb = &dp->usbpd_cb;
 	struct dp_panel_in panel_in = {
 		.dev = dev,
 	};
 
-	/* Callback APIs used for cable status change event */
-	cb->configure  = dp_display_usbpd_configure_cb;
-	cb->disconnect = dp_display_usbpd_disconnect_cb;
-	cb->attention  = dp_display_usbpd_attention_cb;
-
-	dp->usbpd = dp_hpd_get(dev, cb);
-	if (IS_ERR(dp->usbpd)) {
-		rc = PTR_ERR(dp->usbpd);
-		DRM_ERROR("failed to initialize hpd, rc = %d\n", rc);
-		dp->usbpd = NULL;
-		goto error;
-	}
-
 	dp->parser = dp_parser_get(dp->pdev);
 	if (IS_ERR(dp->parser)) {
 		rc = PTR_ERR(dp->parser);
@@ -1544,9 +1514,8 @@ void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor *minor)
 	dp = container_of(dp_display, struct dp_display_private, dp_display);
 	dev = &dp->pdev->dev;
 
-	dp->debug = dp_debug_get(dev, dp->panel, dp->usbpd,
-					dp->link, dp->dp_display.connector,
-					minor);
+	dp->debug = dp_debug_get(dev, dp->panel, dp->link,
+				 dp->dp_display.connector, minor);
 	if (IS_ERR(dp->debug)) {
 		rc = PTR_ERR(dp->debug);
 		DRM_ERROR("failed to initialize debug, rc = %d\n", rc);
diff --git a/drivers/gpu/drm/msm/dp/dp_hpd.c b/drivers/gpu/drm/msm/dp/dp_hpd.c
deleted file mode 100644
index db98a1d431eb..000000000000
--- a/drivers/gpu/drm/msm/dp/dp_hpd.c
+++ /dev/null
@@ -1,67 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
- */
-
-#define pr_fmt(fmt)	"[drm-dp] %s: " fmt, __func__
-
-#include <linux/slab.h>
-#include <linux/device.h>
-
-#include "dp_hpd.h"
-
-/* DP specific VDM commands */
-#define DP_USBPD_VDM_STATUS	0x10
-#define DP_USBPD_VDM_CONFIGURE	0x11
-
-/* USBPD-TypeC specific Macros */
-#define VDM_VERSION		0x0
-#define USB_C_DP_SID		0xFF01
-
-struct dp_hpd_private {
-	struct device *dev;
-	struct dp_usbpd_cb *dp_cb;
-	struct dp_usbpd dp_usbpd;
-};
-
-int dp_hpd_connect(struct dp_usbpd *dp_usbpd, bool hpd)
-{
-	int rc = 0;
-	struct dp_hpd_private *hpd_priv;
-
-	hpd_priv = container_of(dp_usbpd, struct dp_hpd_private,
-					dp_usbpd);
-
-	if (!hpd_priv->dp_cb || !hpd_priv->dp_cb->configure
-				|| !hpd_priv->dp_cb->disconnect) {
-		pr_err("hpd dp_cb not initialized\n");
-		return -EINVAL;
-	}
-	if (hpd)
-		hpd_priv->dp_cb->configure(hpd_priv->dev);
-	else
-		hpd_priv->dp_cb->disconnect(hpd_priv->dev);
-
-	return rc;
-}
-
-struct dp_usbpd *dp_hpd_get(struct device *dev, struct dp_usbpd_cb *cb)
-{
-	struct dp_hpd_private *dp_hpd;
-
-	if (!cb) {
-		pr_err("invalid cb data\n");
-		return ERR_PTR(-EINVAL);
-	}
-
-	dp_hpd = devm_kzalloc(dev, sizeof(*dp_hpd), GFP_KERNEL);
-	if (!dp_hpd)
-		return ERR_PTR(-ENOMEM);
-
-	dp_hpd->dev = dev;
-	dp_hpd->dp_cb = cb;
-
-	dp_hpd->dp_usbpd.connect = dp_hpd_connect;
-
-	return &dp_hpd->dp_usbpd;
-}
diff --git a/drivers/gpu/drm/msm/dp/dp_hpd.h b/drivers/gpu/drm/msm/dp/dp_hpd.h
deleted file mode 100644
index 8feec5aa5027..000000000000
--- a/drivers/gpu/drm/msm/dp/dp_hpd.h
+++ /dev/null
@@ -1,78 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0-only */
-/*
- * Copyright (c) 2012-2020, The Linux Foundation. All rights reserved.
- */
-
-#ifndef _DP_HPD_H_
-#define _DP_HPD_H_
-
-//#include <linux/usb/usbpd.h>
-
-#include <linux/types.h>
-#include <linux/device.h>
-
-enum plug_orientation {
-	ORIENTATION_NONE,
-	ORIENTATION_CC1,
-	ORIENTATION_CC2,
-};
-
-/**
- * struct dp_usbpd - DisplayPort status
- *
- * @orientation: plug orientation configuration
- * @low_pow_st: low power state
- * @adaptor_dp_en: adaptor functionality enabled
- * @multi_func: multi-function preferred
- * @usb_config_req: request to switch to usb
- * @exit_dp_mode: request exit from displayport mode
- * @hpd_irq: Change in the status since last message
- * @alt_mode_cfg_done: bool to specify alt mode status
- * @debug_en: bool to specify debug mode
- * @connect: simulate disconnect or connect for debug mode
- */
-struct dp_usbpd {
-	enum plug_orientation orientation;
-	bool low_pow_st;
-	bool adaptor_dp_en;
-	bool multi_func;
-	bool usb_config_req;
-	bool exit_dp_mode;
-	bool hpd_irq;
-	bool alt_mode_cfg_done;
-	bool debug_en;
-
-	int (*connect)(struct dp_usbpd *dp_usbpd, bool hpd);
-};
-
-/**
- * struct dp_usbpd_cb - callback functions provided by the client
- *
- * @configure: called by usbpd module when PD communication has
- * been completed and the usb peripheral has been configured on
- * dp mode.
- * @disconnect: notify the cable disconnect issued by usb.
- * @attention: notify any attention message issued by usb.
- */
-struct dp_usbpd_cb {
-	int (*configure)(struct device *dev);
-	int (*disconnect)(struct device *dev);
-	int (*attention)(struct device *dev);
-};
-
-/**
- * dp_hpd_get() - setup hpd module
- *
- * @dev: device instance of the caller
- * @cb: struct containing callback function pointers.
- *
- * This function allows the client to initialize the usbpd
- * module. The module will communicate with HPD module.
- */
-struct dp_usbpd *dp_hpd_get(struct device *dev, struct dp_usbpd_cb *cb);
-
-int dp_hpd_register(struct dp_usbpd *dp_usbpd);
-void dp_hpd_unregister(struct dp_usbpd *dp_usbpd);
-int dp_hpd_connect(struct dp_usbpd *dp_usbpd, bool hpd);
-
-#endif /* _DP_HPD_H_ */
diff --git a/drivers/gpu/drm/msm/dp/dp_panel.h b/drivers/gpu/drm/msm/dp/dp_panel.h
index 45208b45eb53..ed1030e17e1b 100644
--- a/drivers/gpu/drm/msm/dp/dp_panel.h
+++ b/drivers/gpu/drm/msm/dp/dp_panel.h
@@ -10,7 +10,6 @@
 
 #include "dp_aux.h"
 #include "dp_link.h"
-#include "dp_hpd.h"
 
 struct edid;
 
-- 
2.39.2

[PATCH 2/3] drm/msm/dp: Clean up logs dp_power module

[Bjorn Andersson]

The clk_bulk API already provides error messages indicating which
specific clock in the request for which the operation failed, further
more these errors are associated with the specific DisplayPort
controller (rather than the shared drm_device). The additional error
messages int he dp_power module does thereby not provide any benefit.

While at it, none of the dp_power handles passed to these functions are
dynamic in nature, so there should not be any need for runtime checking
them. Drop these as well.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_power.c | 62 +++++--------------------------
 1 file changed, 9 insertions(+), 53 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
index c0aaabb03389..031d2eefef07 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -35,25 +35,16 @@ static int dp_power_clk_init(struct dp_power_private *power)
 	stream = &power->parser->mp[DP_STREAM_PM];
 
 	rc = devm_clk_bulk_get(dev, core->num_clk, core->clocks);
-	if (rc) {
-		DRM_ERROR("failed to get %s clk. err=%d\n",
-			dp_parser_pm_name(DP_CORE_PM), rc);
+	if (rc)
 		return rc;
-	}
 
 	rc = devm_clk_bulk_get(dev, ctrl->num_clk, ctrl->clocks);
-	if (rc) {
-		DRM_ERROR("failed to get %s clk. err=%d\n",
-			dp_parser_pm_name(DP_CTRL_PM), rc);
+	if (rc)
 		return -ENODEV;
-	}
 
 	rc = devm_clk_bulk_get(dev, stream->num_clk, stream->clocks);
-	if (rc) {
-		DRM_ERROR("failed to get %s clk. err=%d\n",
-			dp_parser_pm_name(DP_CTRL_PM), rc);
+	if (rc)
 		return -ENODEV;
-	}
 
 	return 0;
 }
@@ -121,11 +112,9 @@ int dp_power_clk_enable(struct dp_power *dp_power,
 			mp = &power->parser->mp[DP_CORE_PM];
 
 			rc = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
-			if (rc) {
-				DRM_ERROR("fail to enable clks: %s. err=%d\n",
-					dp_parser_pm_name(DP_CORE_PM), rc);
+			if (rc)
 				return rc;
-			}
+
 			dp_power->core_clks_on = true;
 		}
 	}
@@ -133,10 +122,8 @@ int dp_power_clk_enable(struct dp_power *dp_power,
 	mp = &power->parser->mp[pm_type];
 	if (enable) {
 		rc = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
-		if (rc) {
-			DRM_ERROR("failed to enable clks, err: %d\n", rc);
+		if (rc)
 			return rc;
-		}
 	} else {
 		clk_bulk_disable_unprepare(mp->num_clk, mp->clocks);
 	}
@@ -162,34 +149,19 @@ int dp_power_clk_enable(struct dp_power *dp_power,
 
 int dp_power_client_init(struct dp_power *dp_power)
 {
-	int rc = 0;
 	struct dp_power_private *power;
 
-	if (!dp_power) {
-		DRM_ERROR("invalid power data\n");
-		return -EINVAL;
-	}
-
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
 	pm_runtime_enable(&power->pdev->dev);
 
-	rc = dp_power_clk_init(power);
-	if (rc)
-		DRM_ERROR("failed to init clocks %d\n", rc);
-
-	return rc;
+	return dp_power_clk_init(power);
 }
 
 void dp_power_client_deinit(struct dp_power *dp_power)
 {
 	struct dp_power_private *power;
 
-	if (!dp_power) {
-		DRM_ERROR("invalid power data\n");
-		return;
-	}
-
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
 	pm_runtime_disable(&power->pdev->dev);
@@ -200,25 +172,14 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
 	int rc = 0;
 	struct dp_power_private *power = NULL;
 
-	if (!dp_power) {
-		DRM_ERROR("invalid power data\n");
-		return -EINVAL;
-	}
-
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
 	pm_runtime_get_sync(&power->pdev->dev);
 
 	rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
-	if (rc) {
-		DRM_ERROR("failed to enable DP core clocks, %d\n", rc);
-		goto exit;
-	}
-
-	return 0;
+	if (rc)
+		pm_runtime_put_sync(&power->pdev->dev);
 
-exit:
-	pm_runtime_put_sync(&power->pdev->dev);
 	return rc;
 }
 
@@ -238,11 +199,6 @@ struct dp_power *dp_power_get(struct device *dev, struct dp_parser *parser)
 	struct dp_power_private *power;
 	struct dp_power *dp_power;
 
-	if (!parser) {
-		DRM_ERROR("invalid input\n");
-		return ERR_PTR(-EINVAL);
-	}
-
 	power = devm_kzalloc(&parser->pdev->dev, sizeof(*power), GFP_KERNEL);
 	if (!power)
 		return ERR_PTR(-ENOMEM);
-- 
2.39.2

[PATCH 3/3] drm/msm/dp: Clean up pdev/dev duplication in dp_power

[Bjorn Andersson]

The dp_power module keeps track of both the DP controller's struct
platform_device and struct device - with the prior pulled out of the
dp_parser module.

Clean up the duplication by dropping the platform_device reference and
just track the passed struct device.

Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
---
 drivers/gpu/drm/msm/dp/dp_power.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
index 031d2eefef07..9be645f91211 100644
--- a/drivers/gpu/drm/msm/dp/dp_power.c
+++ b/drivers/gpu/drm/msm/dp/dp_power.c
@@ -14,7 +14,6 @@
 
 struct dp_power_private {
 	struct dp_parser *parser;
-	struct platform_device *pdev;
 	struct device *dev;
 	struct drm_device *drm_dev;
 	struct clk *link_clk_src;
@@ -28,7 +27,7 @@ static int dp_power_clk_init(struct dp_power_private *power)
 {
 	int rc = 0;
 	struct dss_module_power *core, *ctrl, *stream;
-	struct device *dev = &power->pdev->dev;
+	struct device *dev = power->dev;
 
 	core = &power->parser->mp[DP_CORE_PM];
 	ctrl = &power->parser->mp[DP_CTRL_PM];
@@ -153,7 +152,7 @@ int dp_power_client_init(struct dp_power *dp_power)
 
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
-	pm_runtime_enable(&power->pdev->dev);
+	pm_runtime_enable(power->dev);
 
 	return dp_power_clk_init(power);
 }
@@ -164,7 +163,7 @@ void dp_power_client_deinit(struct dp_power *dp_power)
 
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
-	pm_runtime_disable(&power->pdev->dev);
+	pm_runtime_disable(power->dev);
 }
 
 int dp_power_init(struct dp_power *dp_power, bool flip)
@@ -174,11 +173,11 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
 
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
-	pm_runtime_get_sync(&power->pdev->dev);
+	pm_runtime_get_sync(power->dev);
 
 	rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
 	if (rc)
-		pm_runtime_put_sync(&power->pdev->dev);
+		pm_runtime_put_sync(power->dev);
 
 	return rc;
 }
@@ -190,7 +189,7 @@ int dp_power_deinit(struct dp_power *dp_power)
 	power = container_of(dp_power, struct dp_power_private, dp_power);
 
 	dp_power_clk_enable(dp_power, DP_CORE_PM, false);
-	pm_runtime_put_sync(&power->pdev->dev);
+	pm_runtime_put_sync(power->dev);
 	return 0;
 }
 
@@ -199,12 +198,11 @@ struct dp_power *dp_power_get(struct device *dev, struct dp_parser *parser)
 	struct dp_power_private *power;
 	struct dp_power *dp_power;
 
-	power = devm_kzalloc(&parser->pdev->dev, sizeof(*power), GFP_KERNEL);
+	power = devm_kzalloc(dev, sizeof(*power), GFP_KERNEL);
 	if (!power)
 		return ERR_PTR(-ENOMEM);
 
 	power->parser = parser;
-	power->pdev = parser->pdev;
 	power->dev = dev;
 
 	dp_power = &power->dp_power;
-- 
2.39.2

Re: [PATCH 1/3] drm/msm/dp: Drop now unused dp_hpd module

[Dmitry Baryshkov]

On 15/05/2023 06:02, Bjorn Andersson wrote:
> The dp_hpd module is a remnant from the downstream design and is now
> completely unused. Drop it and all references to it.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_debug.c   |  8 ++-
>   drivers/gpu/drm/msm/dp/dp_debug.h   | 12 ++---
>   drivers/gpu/drm/msm/dp/dp_display.c | 35 +------------
>   drivers/gpu/drm/msm/dp/dp_hpd.c     | 67 -------------------------
>   drivers/gpu/drm/msm/dp/dp_hpd.h     | 78 -----------------------------
>   drivers/gpu/drm/msm/dp/dp_panel.h   |  1 -
>   6 files changed, 11 insertions(+), 190 deletions(-)
>   delete mode 100644 drivers/gpu/drm/msm/dp/dp_hpd.c
>   delete mode 100644 drivers/gpu/drm/msm/dp/dp_hpd.h

I think this is not complete. Could you please check if 
https://patchwork.freedesktop.org/patch/433883/?series=90198&rev=1 works 
for you?

-- 
With best wishes
Dmitry

Re: [PATCH 2/3] drm/msm/dp: Clean up logs dp_power module

[Dmitry Baryshkov]

On 15/05/2023 06:02, Bjorn Andersson wrote:
> The clk_bulk API already provides error messages indicating which
> specific clock in the request for which the operation failed, further
> more these errors are associated with the specific DisplayPort
> controller (rather than the shared drm_device). The additional error
> messages int he dp_power module does thereby not provide any benefit.
> 
> While at it, none of the dp_power handles passed to these functions are
> dynamic in nature, so there should not be any need for runtime checking
> them. Drop these as well.

It might have been slightly better to have this split into two patches, 
but as this is a debug/logs only, it's fine with me.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_power.c | 62 +++++--------------------------
>   1 file changed, 9 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index c0aaabb03389..031d2eefef07 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -35,25 +35,16 @@ static int dp_power_clk_init(struct dp_power_private *power)
>   	stream = &power->parser->mp[DP_STREAM_PM];
>   
>   	rc = devm_clk_bulk_get(dev, core->num_clk, core->clocks);
> -	if (rc) {
> -		DRM_ERROR("failed to get %s clk. err=%d\n",
> -			dp_parser_pm_name(DP_CORE_PM), rc);
> +	if (rc)
>   		return rc;
> -	}
>   
>   	rc = devm_clk_bulk_get(dev, ctrl->num_clk, ctrl->clocks);
> -	if (rc) {
> -		DRM_ERROR("failed to get %s clk. err=%d\n",
> -			dp_parser_pm_name(DP_CTRL_PM), rc);
> +	if (rc)
>   		return -ENODEV;
> -	}
>   
>   	rc = devm_clk_bulk_get(dev, stream->num_clk, stream->clocks);
> -	if (rc) {
> -		DRM_ERROR("failed to get %s clk. err=%d\n",
> -			dp_parser_pm_name(DP_CTRL_PM), rc);
> +	if (rc)
>   		return -ENODEV;
> -	}
>   
>   	return 0;
>   }
> @@ -121,11 +112,9 @@ int dp_power_clk_enable(struct dp_power *dp_power,
>   			mp = &power->parser->mp[DP_CORE_PM];
>   
>   			rc = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
> -			if (rc) {
> -				DRM_ERROR("fail to enable clks: %s. err=%d\n",
> -					dp_parser_pm_name(DP_CORE_PM), rc);
> +			if (rc)
>   				return rc;
> -			}
> +
>   			dp_power->core_clks_on = true;
>   		}
>   	}
> @@ -133,10 +122,8 @@ int dp_power_clk_enable(struct dp_power *dp_power,
>   	mp = &power->parser->mp[pm_type];
>   	if (enable) {
>   		rc = clk_bulk_prepare_enable(mp->num_clk, mp->clocks);
> -		if (rc) {
> -			DRM_ERROR("failed to enable clks, err: %d\n", rc);
> +		if (rc)
>   			return rc;
> -		}
>   	} else {
>   		clk_bulk_disable_unprepare(mp->num_clk, mp->clocks);
>   	}
> @@ -162,34 +149,19 @@ int dp_power_clk_enable(struct dp_power *dp_power,
>   
>   int dp_power_client_init(struct dp_power *dp_power)
>   {
> -	int rc = 0;
>   	struct dp_power_private *power;
>   
> -	if (!dp_power) {
> -		DRM_ERROR("invalid power data\n");
> -		return -EINVAL;
> -	}
> -
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
>   	pm_runtime_enable(&power->pdev->dev);
>   
> -	rc = dp_power_clk_init(power);
> -	if (rc)
> -		DRM_ERROR("failed to init clocks %d\n", rc);
> -
> -	return rc;
> +	return dp_power_clk_init(power);
>   }
>   
>   void dp_power_client_deinit(struct dp_power *dp_power)
>   {
>   	struct dp_power_private *power;
>   
> -	if (!dp_power) {
> -		DRM_ERROR("invalid power data\n");
> -		return;
> -	}
> -
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
>   	pm_runtime_disable(&power->pdev->dev);
> @@ -200,25 +172,14 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
>   	int rc = 0;
>   	struct dp_power_private *power = NULL;
>   
> -	if (!dp_power) {
> -		DRM_ERROR("invalid power data\n");
> -		return -EINVAL;
> -	}
> -
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
>   	pm_runtime_get_sync(&power->pdev->dev);
>   
>   	rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
> -	if (rc) {
> -		DRM_ERROR("failed to enable DP core clocks, %d\n", rc);
> -		goto exit;
> -	}
> -
> -	return 0;
> +	if (rc)
> +		pm_runtime_put_sync(&power->pdev->dev);
>   
> -exit:
> -	pm_runtime_put_sync(&power->pdev->dev);
>   	return rc;
>   }
>   
> @@ -238,11 +199,6 @@ struct dp_power *dp_power_get(struct device *dev, struct dp_parser *parser)
>   	struct dp_power_private *power;
>   	struct dp_power *dp_power;
>   
> -	if (!parser) {
> -		DRM_ERROR("invalid input\n");
> -		return ERR_PTR(-EINVAL);
> -	}
> -
>   	power = devm_kzalloc(&parser->pdev->dev, sizeof(*power), GFP_KERNEL);
>   	if (!power)
>   		return ERR_PTR(-ENOMEM);

-- 
With best wishes
Dmitry

Re: [PATCH 3/3] drm/msm/dp: Clean up pdev/dev duplication in dp_power

[Dmitry Baryshkov]

On 15/05/2023 06:02, Bjorn Andersson wrote:
> The dp_power module keeps track of both the DP controller's struct
> platform_device and struct device - with the prior pulled out of the
> dp_parser module.
> 
> Clean up the duplication by dropping the platform_device reference and
> just track the passed struct device.
> 
> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> ---
>   drivers/gpu/drm/msm/dp/dp_power.c | 16 +++++++---------
>   1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> index 031d2eefef07..9be645f91211 100644
> --- a/drivers/gpu/drm/msm/dp/dp_power.c
> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> @@ -14,7 +14,6 @@
>   
>   struct dp_power_private {
>   	struct dp_parser *parser;
> -	struct platform_device *pdev;
>   	struct device *dev;
>   	struct drm_device *drm_dev;
>   	struct clk *link_clk_src;
> @@ -28,7 +27,7 @@ static int dp_power_clk_init(struct dp_power_private *power)
>   {
>   	int rc = 0;
>   	struct dss_module_power *core, *ctrl, *stream;
> -	struct device *dev = &power->pdev->dev;
> +	struct device *dev = power->dev;
>   
>   	core = &power->parser->mp[DP_CORE_PM];
>   	ctrl = &power->parser->mp[DP_CTRL_PM];
> @@ -153,7 +152,7 @@ int dp_power_client_init(struct dp_power *dp_power)
>   
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
> -	pm_runtime_enable(&power->pdev->dev);
> +	pm_runtime_enable(power->dev);
>   
>   	return dp_power_clk_init(power);
>   }
> @@ -164,7 +163,7 @@ void dp_power_client_deinit(struct dp_power *dp_power)
>   
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
> -	pm_runtime_disable(&power->pdev->dev);
> +	pm_runtime_disable(power->dev);
>   }
>   
>   int dp_power_init(struct dp_power *dp_power, bool flip)
> @@ -174,11 +173,11 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
>   
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
> -	pm_runtime_get_sync(&power->pdev->dev);
> +	pm_runtime_get_sync(power->dev);
>   
>   	rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
>   	if (rc)
> -		pm_runtime_put_sync(&power->pdev->dev);
> +		pm_runtime_put_sync(power->dev);
>   
>   	return rc;
>   }
> @@ -190,7 +189,7 @@ int dp_power_deinit(struct dp_power *dp_power)
>   	power = container_of(dp_power, struct dp_power_private, dp_power);
>   
>   	dp_power_clk_enable(dp_power, DP_CORE_PM, false);
> -	pm_runtime_put_sync(&power->pdev->dev);
> +	pm_runtime_put_sync(power->dev);
>   	return 0;
>   }
>   
> @@ -199,12 +198,11 @@ struct dp_power *dp_power_get(struct device *dev, struct dp_parser *parser)

Technically we don't even need to pass struct device here, we can get it 
from parser->pdev->dev.

>   	struct dp_power_private *power;
>   	struct dp_power *dp_power;
>   
> -	power = devm_kzalloc(&parser->pdev->dev, sizeof(*power), GFP_KERNEL);
> +	power = devm_kzalloc(dev, sizeof(*power), GFP_KERNEL);
>   	if (!power)
>   		return ERR_PTR(-ENOMEM);
>   
>   	power->parser = parser;
> -	power->pdev = parser->pdev;
>   	power->dev = dev;
>   
>   	dp_power = &power->dp_power;

-- 
With best wishes
Dmitry

Re: [PATCH 3/3] drm/msm/dp: Clean up pdev/dev duplication in dp_power

[Bjorn Andersson]

On Sat, May 20, 2023 at 04:26:59AM +0300, Dmitry Baryshkov wrote:
> On 15/05/2023 06:02, Bjorn Andersson wrote:
> > The dp_power module keeps track of both the DP controller's struct
> > platform_device and struct device - with the prior pulled out of the
> > dp_parser module.
> > 
> > Clean up the duplication by dropping the platform_device reference and
> > just track the passed struct device.
> > 
> > Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
> > ---
> >   drivers/gpu/drm/msm/dp/dp_power.c | 16 +++++++---------
> >   1 file changed, 7 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
> > index 031d2eefef07..9be645f91211 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_power.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_power.c
> > @@ -14,7 +14,6 @@
> >   struct dp_power_private {
> >   	struct dp_parser *parser;
> > -	struct platform_device *pdev;
> >   	struct device *dev;
> >   	struct drm_device *drm_dev;
> >   	struct clk *link_clk_src;
> > @@ -28,7 +27,7 @@ static int dp_power_clk_init(struct dp_power_private *power)
> >   {
> >   	int rc = 0;
> >   	struct dss_module_power *core, *ctrl, *stream;
> > -	struct device *dev = &power->pdev->dev;
> > +	struct device *dev = power->dev;
> >   	core = &power->parser->mp[DP_CORE_PM];
> >   	ctrl = &power->parser->mp[DP_CTRL_PM];
> > @@ -153,7 +152,7 @@ int dp_power_client_init(struct dp_power *dp_power)
> >   	power = container_of(dp_power, struct dp_power_private, dp_power);
> > -	pm_runtime_enable(&power->pdev->dev);
> > +	pm_runtime_enable(power->dev);
> >   	return dp_power_clk_init(power);
> >   }
> > @@ -164,7 +163,7 @@ void dp_power_client_deinit(struct dp_power *dp_power)
> >   	power = container_of(dp_power, struct dp_power_private, dp_power);
> > -	pm_runtime_disable(&power->pdev->dev);
> > +	pm_runtime_disable(power->dev);
> >   }
> >   int dp_power_init(struct dp_power *dp_power, bool flip)
> > @@ -174,11 +173,11 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
> >   	power = container_of(dp_power, struct dp_power_private, dp_power);
> > -	pm_runtime_get_sync(&power->pdev->dev);
> > +	pm_runtime_get_sync(power->dev);
> >   	rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
> >   	if (rc)
> > -		pm_runtime_put_sync(&power->pdev->dev);
> > +		pm_runtime_put_sync(power->dev);
> >   	return rc;
> >   }
> > @@ -190,7 +189,7 @@ int dp_power_deinit(struct dp_power *dp_power)
> >   	power = container_of(dp_power, struct dp_power_private, dp_power);
> >   	dp_power_clk_enable(dp_power, DP_CORE_PM, false);
> > -	pm_runtime_put_sync(&power->pdev->dev);
> > +	pm_runtime_put_sync(power->dev);
> >   	return 0;
> >   }
> > @@ -199,12 +198,11 @@ struct dp_power *dp_power_get(struct device *dev, struct dp_parser *parser)
> 
> Technically we don't even need to pass struct device here, we can get it
> from parser->pdev->dev.
> 

Right, but afaict dp_init_sub_modules() passes struct device * as first
parameter to all the "module" initializers. So it feels reasonable to
keep it, for now, for symmetry.

What do you think?

Regards,
Bjorn

> >   	struct dp_power_private *power;
> >   	struct dp_power *dp_power;
> > -	power = devm_kzalloc(&parser->pdev->dev, sizeof(*power), GFP_KERNEL);
> > +	power = devm_kzalloc(dev, sizeof(*power), GFP_KERNEL);
> >   	if (!power)
> >   		return ERR_PTR(-ENOMEM);
> >   	power->parser = parser;
> > -	power->pdev = parser->pdev;
> >   	power->dev = dev;
> >   	dp_power = &power->dp_power;
> 
> -- 
> With best wishes
> Dmitry
> 

Re: [PATCH 3/3] drm/msm/dp: Clean up pdev/dev duplication in dp_power

[Dmitry Baryshkov]

On 21/05/2023 06:53, Bjorn Andersson wrote:
> On Sat, May 20, 2023 at 04:26:59AM +0300, Dmitry Baryshkov wrote:
>> On 15/05/2023 06:02, Bjorn Andersson wrote:
>>> The dp_power module keeps track of both the DP controller's struct
>>> platform_device and struct device - with the prior pulled out of the
>>> dp_parser module.
>>>
>>> Clean up the duplication by dropping the platform_device reference and
>>> just track the passed struct device.
>>>
>>> Signed-off-by: Bjorn Andersson <quic_bjorande@quicinc.com>
>>> ---
>>>    drivers/gpu/drm/msm/dp/dp_power.c | 16 +++++++---------
>>>    1 file changed, 7 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/dp/dp_power.c b/drivers/gpu/drm/msm/dp/dp_power.c
>>> index 031d2eefef07..9be645f91211 100644
>>> --- a/drivers/gpu/drm/msm/dp/dp_power.c
>>> +++ b/drivers/gpu/drm/msm/dp/dp_power.c
>>> @@ -14,7 +14,6 @@
>>>    struct dp_power_private {
>>>    	struct dp_parser *parser;
>>> -	struct platform_device *pdev;
>>>    	struct device *dev;
>>>    	struct drm_device *drm_dev;
>>>    	struct clk *link_clk_src;
>>> @@ -28,7 +27,7 @@ static int dp_power_clk_init(struct dp_power_private *power)
>>>    {
>>>    	int rc = 0;
>>>    	struct dss_module_power *core, *ctrl, *stream;
>>> -	struct device *dev = &power->pdev->dev;
>>> +	struct device *dev = power->dev;
>>>    	core = &power->parser->mp[DP_CORE_PM];
>>>    	ctrl = &power->parser->mp[DP_CTRL_PM];
>>> @@ -153,7 +152,7 @@ int dp_power_client_init(struct dp_power *dp_power)
>>>    	power = container_of(dp_power, struct dp_power_private, dp_power);
>>> -	pm_runtime_enable(&power->pdev->dev);
>>> +	pm_runtime_enable(power->dev);
>>>    	return dp_power_clk_init(power);
>>>    }
>>> @@ -164,7 +163,7 @@ void dp_power_client_deinit(struct dp_power *dp_power)
>>>    	power = container_of(dp_power, struct dp_power_private, dp_power);
>>> -	pm_runtime_disable(&power->pdev->dev);
>>> +	pm_runtime_disable(power->dev);
>>>    }
>>>    int dp_power_init(struct dp_power *dp_power, bool flip)
>>> @@ -174,11 +173,11 @@ int dp_power_init(struct dp_power *dp_power, bool flip)
>>>    	power = container_of(dp_power, struct dp_power_private, dp_power);
>>> -	pm_runtime_get_sync(&power->pdev->dev);
>>> +	pm_runtime_get_sync(power->dev);
>>>    	rc = dp_power_clk_enable(dp_power, DP_CORE_PM, true);
>>>    	if (rc)
>>> -		pm_runtime_put_sync(&power->pdev->dev);
>>> +		pm_runtime_put_sync(power->dev);
>>>    	return rc;
>>>    }
>>> @@ -190,7 +189,7 @@ int dp_power_deinit(struct dp_power *dp_power)
>>>    	power = container_of(dp_power, struct dp_power_private, dp_power);
>>>    	dp_power_clk_enable(dp_power, DP_CORE_PM, false);
>>> -	pm_runtime_put_sync(&power->pdev->dev);
>>> +	pm_runtime_put_sync(power->dev);
>>>    	return 0;
>>>    }
>>> @@ -199,12 +198,11 @@ struct dp_power *dp_power_get(struct device *dev, struct dp_parser *parser)
>>
>> Technically we don't even need to pass struct device here, we can get it
>> from parser->pdev->dev.
>>
> 
> Right, but afaict dp_init_sub_modules() passes struct device * as first
> parameter to all the "module" initializers. So it feels reasonable to
> keep it, for now, for symmetry.
> 
> What do you think?

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH 1/3] drm/msm/dp: Drop now unused dp_hpd module

[Dmitry Baryshkov]

On Sun, 14 May 2023 20:02:54 -0700, Bjorn Andersson wrote:
> The dp_hpd module is a remnant from the downstream design and is now
> completely unused. Drop it and all references to it.
> 
> 

Applied, thanks!

[2/3] drm/msm/dp: Clean up logs dp_power module
      https://gitlab.freedesktop.org/lumag/msm/-/commit/9f93258549db
[3/3] drm/msm/dp: Clean up pdev/dev duplication in dp_power
      https://gitlab.freedesktop.org/lumag/msm/-/commit/9edac2eec47c

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