All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/2] drm/bridge: anx7625: refactor power control to use runtime PM framework
@ 2021-05-07  6:44 ` Pi-Hsun Shih
  0 siblings, 0 replies; 7+ messages in thread
From: Pi-Hsun Shih @ 2021-05-07  6:44 UTC (permalink / raw)
  Cc: Pi-Hsun Shih, Tzung-Bi Shih, Andrzej Hajda, Neil Armstrong,
	Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
	David Airlie, Daniel Vetter, Xin Ji, Sam Ravnborg, Hsin-Yi Wang,
	open list:DRM DRIVERS, open list

The driver originally use an atomic_t for keep track of the power
status, which makes the driver more complicated than needed, and has
some race condition as it's possible to have the power on and power off
sequence going at the same time.

This patch remove the usage of the atomic_t power_status, and use the
kernel runtime power management framework instead.

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
 drivers/gpu/drm/bridge/analogix/anx7625.c | 148 +++++++++-------------
 drivers/gpu/drm/bridge/analogix/anx7625.h |   1 -
 2 files changed, 63 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 23283ba0c4f9..f56f8cf1f3bd 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
@@ -1005,33 +1006,6 @@ static void anx7625_power_on_init(struct anx7625_data *ctx)
 	}
 }
 
-static void anx7625_chip_control(struct anx7625_data *ctx, int state)
-{
-	struct device *dev = &ctx->client->dev;
-
-	DRM_DEV_DEBUG_DRIVER(dev, "before set, power_state(%d).\n",
-			     atomic_read(&ctx->power_status));
-
-	if (!ctx->pdata.low_power_mode)
-		return;
-
-	if (state) {
-		atomic_inc(&ctx->power_status);
-		if (atomic_read(&ctx->power_status) == 1)
-			anx7625_power_on_init(ctx);
-	} else {
-		if (atomic_read(&ctx->power_status)) {
-			atomic_dec(&ctx->power_status);
-
-			if (atomic_read(&ctx->power_status) == 0)
-				anx7625_power_standby(ctx);
-		}
-	}
-
-	DRM_DEV_DEBUG_DRIVER(dev, "after set, power_state(%d).\n",
-			     atomic_read(&ctx->power_status));
-}
-
 static void anx7625_init_gpio(struct anx7625_data *platform)
 {
 	struct device *dev = &platform->client->dev;
@@ -1061,9 +1035,6 @@ static void anx7625_stop_dp_work(struct anx7625_data *ctx)
 	ctx->hpd_status = 0;
 	ctx->hpd_high_cnt = 0;
 	ctx->display_timing_valid = 0;
-
-	if (ctx->pdata.low_power_mode == 0)
-		anx7625_disable_pd_protocol(ctx);
 }
 
 static void anx7625_start_dp_work(struct anx7625_data *ctx)
@@ -1105,49 +1076,26 @@ static void anx7625_hpd_polling(struct anx7625_data *ctx)
 	int ret, val;
 	struct device *dev = &ctx->client->dev;
 
-	if (atomic_read(&ctx->power_status) != 1) {
-		DRM_DEV_DEBUG_DRIVER(dev, "No need to poling HPD status.\n");
-		return;
-	}
-
 	ret = readx_poll_timeout(anx7625_read_hpd_status_p0,
 				 ctx, val,
 				 ((val & HPD_STATUS) || (val < 0)),
 				 5000,
 				 5000 * 100);
 	if (ret) {
-		DRM_DEV_ERROR(dev, "HPD polling timeout!\n");
-	} else {
-		DRM_DEV_DEBUG_DRIVER(dev, "HPD raise up.\n");
-		anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
-				  INTR_ALERT_1, 0xFF);
-		anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
-				  INTERFACE_CHANGE_INT, 0);
+		DRM_DEV_ERROR(dev, "no hpd.\n");
+		return;
 	}
 
-	anx7625_start_dp_work(ctx);
-}
-
-static void anx7625_disconnect_check(struct anx7625_data *ctx)
-{
-	if (atomic_read(&ctx->power_status) == 0)
-		anx7625_stop_dp_work(ctx);
-}
-
-static void anx7625_low_power_mode_check(struct anx7625_data *ctx,
-					 int state)
-{
-	struct device *dev = &ctx->client->dev;
+	DRM_DEV_DEBUG_DRIVER(dev, "system status: 0x%x. HPD raise up.\n", val);
+	anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
+			  INTR_ALERT_1, 0xFF);
+	anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
+			  INTERFACE_CHANGE_INT, 0);
 
-	DRM_DEV_DEBUG_DRIVER(dev, "low power mode check, state(%d).\n", state);
+	anx7625_start_dp_work(ctx);
 
-	if (ctx->pdata.low_power_mode) {
-		anx7625_chip_control(ctx, state);
-		if (state)
-			anx7625_hpd_polling(ctx);
-		else
-			anx7625_disconnect_check(ctx);
-	}
+	if (!ctx->pdata.panel_bridge && ctx->bridge_attached)
+		drm_helper_hpd_irq_event(ctx->bridge.dev);
 }
 
 static void anx7625_remove_edid(struct anx7625_data *ctx)
@@ -1180,9 +1128,6 @@ static int anx7625_hpd_change_detect(struct anx7625_data *ctx)
 	int intr_vector, status;
 	struct device *dev = &ctx->client->dev;
 
-	DRM_DEV_DEBUG_DRIVER(dev, "power_status=%d\n",
-			     (u32)atomic_read(&ctx->power_status));
-
 	status = anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
 				   INTR_ALERT_1, 0xFF);
 	if (status < 0) {
@@ -1228,22 +1173,25 @@ static void anx7625_work_func(struct work_struct *work)
 						struct anx7625_data, work);
 
 	mutex_lock(&ctx->lock);
+
+	if (pm_runtime_suspended(&ctx->client->dev))
+		goto unlock;
+
 	event = anx7625_hpd_change_detect(ctx);
-	mutex_unlock(&ctx->lock);
 	if (event < 0)
-		return;
+		goto unlock;
 
 	if (ctx->bridge_attached)
 		drm_helper_hpd_irq_event(ctx->bridge.dev);
+
+unlock:
+	mutex_unlock(&ctx->lock);
 }
 
 static irqreturn_t anx7625_intr_hpd_isr(int irq, void *data)
 {
 	struct anx7625_data *ctx = (struct anx7625_data *)data;
 
-	if (atomic_read(&ctx->power_status) != 1)
-		return IRQ_NONE;
-
 	queue_work(ctx->workqueue, &ctx->work);
 
 	return IRQ_HANDLED;
@@ -1305,9 +1253,9 @@ static struct edid *anx7625_get_edid(struct anx7625_data *ctx)
 		return (struct edid *)edid;
 	}
 
-	anx7625_low_power_mode_check(ctx, 1);
+	pm_runtime_get_sync(dev);
 	edid_num = sp_tx_edid_read(ctx, p_edid->edid_raw_data);
-	anx7625_low_power_mode_check(ctx, 0);
+	pm_runtime_put(dev);
 
 	if (edid_num < 1) {
 		DRM_DEV_ERROR(dev, "Fail to read EDID: %d\n", edid_num);
@@ -1611,10 +1559,7 @@ static void anx7625_bridge_enable(struct drm_bridge *bridge)
 
 	DRM_DEV_DEBUG_DRIVER(dev, "drm enable\n");
 
-	anx7625_low_power_mode_check(ctx, 1);
-
-	if (WARN_ON(!atomic_read(&ctx->power_status)))
-		return;
+	pm_runtime_get_sync(dev);
 
 	anx7625_dp_start(ctx);
 }
@@ -1624,14 +1569,11 @@ static void anx7625_bridge_disable(struct drm_bridge *bridge)
 	struct anx7625_data *ctx = bridge_to_anx7625(bridge);
 	struct device *dev = &ctx->client->dev;
 
-	if (WARN_ON(!atomic_read(&ctx->power_status)))
-		return;
-
 	DRM_DEV_DEBUG_DRIVER(dev, "drm disable\n");
 
 	anx7625_dp_stop(ctx);
 
-	anx7625_low_power_mode_check(ctx, 0);
+	pm_runtime_put(dev);
 }
 
 static enum drm_connector_status
@@ -1735,6 +1677,39 @@ static void anx7625_unregister_i2c_dummy_clients(struct anx7625_data *ctx)
 	i2c_unregister_device(ctx->i2c.tcpc_client);
 }
 
+static int __maybe_unused anx7625_runtime_pm_suspend(struct device *dev)
+{
+	struct anx7625_data *ctx = dev_get_drvdata(dev);
+
+	mutex_lock(&ctx->lock);
+
+	anx7625_stop_dp_work(ctx);
+	anx7625_power_standby(ctx);
+
+	mutex_unlock(&ctx->lock);
+
+	return 0;
+}
+
+static int __maybe_unused anx7625_runtime_pm_resume(struct device *dev)
+{
+	struct anx7625_data *ctx = dev_get_drvdata(dev);
+
+	mutex_lock(&ctx->lock);
+
+	anx7625_power_on_init(ctx);
+	anx7625_hpd_polling(ctx);
+
+	mutex_unlock(&ctx->lock);
+
+	return 0;
+}
+
+static const struct dev_pm_ops anx7625_pm_ops = {
+	SET_RUNTIME_PM_OPS(anx7625_runtime_pm_suspend,
+			   anx7625_runtime_pm_resume, NULL)
+};
+
 static int anx7625_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
@@ -1778,8 +1753,6 @@ static int anx7625_i2c_probe(struct i2c_client *client,
 	}
 	anx7625_init_gpio(platform);
 
-	atomic_set(&platform->power_status, 0);
-
 	mutex_init(&platform->lock);
 
 	platform->pdata.intp_irq = client->irq;
@@ -1809,9 +1782,11 @@ static int anx7625_i2c_probe(struct i2c_client *client,
 		goto free_wq;
 	}
 
-	if (platform->pdata.low_power_mode == 0) {
+	pm_runtime_enable(dev);
+
+	if (!platform->pdata.low_power_mode) {
 		anx7625_disable_pd_protocol(platform);
-		atomic_set(&platform->power_status, 1);
+		pm_runtime_get_sync(dev);
 	}
 
 	/* Add work function */
@@ -1847,6 +1822,9 @@ static int anx7625_i2c_remove(struct i2c_client *client)
 	if (platform->pdata.intp_irq)
 		destroy_workqueue(platform->workqueue);
 
+	if (!platform->pdata.low_power_mode)
+		pm_runtime_put_sync_suspend(&client->dev);
+
 	anx7625_unregister_i2c_dummy_clients(platform);
 
 	kfree(platform);
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
index e4a086b3a3d7..034c3840028f 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.h
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
@@ -369,7 +369,6 @@ struct anx7625_i2c_client {
 
 struct anx7625_data {
 	struct anx7625_platform_data pdata;
-	atomic_t power_status;
 	int hpd_status;
 	int hpd_high_cnt;
 	/* Lock for work queue */

base-commit: e48661230cc35b3d0f4367eddfc19f86463ab917
-- 
2.31.1.607.g51e8a6a459-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 1/2] drm/bridge: anx7625: refactor power control to use runtime PM framework
@ 2021-05-07  6:44 ` Pi-Hsun Shih
  0 siblings, 0 replies; 7+ messages in thread
From: Pi-Hsun Shih @ 2021-05-07  6:44 UTC (permalink / raw)
  Cc: Jernej Skrabec, Neil Armstrong, David Airlie,
	open list:DRM DRIVERS, Jonas Karlman, open list, Robert Foss,
	Andrzej Hajda, Tzung-Bi Shih, Laurent Pinchart, Pi-Hsun Shih,
	Hsin-Yi Wang, Sam Ravnborg, Xin Ji

The driver originally use an atomic_t for keep track of the power
status, which makes the driver more complicated than needed, and has
some race condition as it's possible to have the power on and power off
sequence going at the same time.

This patch remove the usage of the atomic_t power_status, and use the
kernel runtime power management framework instead.

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
 drivers/gpu/drm/bridge/analogix/anx7625.c | 148 +++++++++-------------
 drivers/gpu/drm/bridge/analogix/anx7625.h |   1 -
 2 files changed, 63 insertions(+), 86 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 23283ba0c4f9..f56f8cf1f3bd 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/pm_runtime.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 #include <linux/types.h>
@@ -1005,33 +1006,6 @@ static void anx7625_power_on_init(struct anx7625_data *ctx)
 	}
 }
 
-static void anx7625_chip_control(struct anx7625_data *ctx, int state)
-{
-	struct device *dev = &ctx->client->dev;
-
-	DRM_DEV_DEBUG_DRIVER(dev, "before set, power_state(%d).\n",
-			     atomic_read(&ctx->power_status));
-
-	if (!ctx->pdata.low_power_mode)
-		return;
-
-	if (state) {
-		atomic_inc(&ctx->power_status);
-		if (atomic_read(&ctx->power_status) == 1)
-			anx7625_power_on_init(ctx);
-	} else {
-		if (atomic_read(&ctx->power_status)) {
-			atomic_dec(&ctx->power_status);
-
-			if (atomic_read(&ctx->power_status) == 0)
-				anx7625_power_standby(ctx);
-		}
-	}
-
-	DRM_DEV_DEBUG_DRIVER(dev, "after set, power_state(%d).\n",
-			     atomic_read(&ctx->power_status));
-}
-
 static void anx7625_init_gpio(struct anx7625_data *platform)
 {
 	struct device *dev = &platform->client->dev;
@@ -1061,9 +1035,6 @@ static void anx7625_stop_dp_work(struct anx7625_data *ctx)
 	ctx->hpd_status = 0;
 	ctx->hpd_high_cnt = 0;
 	ctx->display_timing_valid = 0;
-
-	if (ctx->pdata.low_power_mode == 0)
-		anx7625_disable_pd_protocol(ctx);
 }
 
 static void anx7625_start_dp_work(struct anx7625_data *ctx)
@@ -1105,49 +1076,26 @@ static void anx7625_hpd_polling(struct anx7625_data *ctx)
 	int ret, val;
 	struct device *dev = &ctx->client->dev;
 
-	if (atomic_read(&ctx->power_status) != 1) {
-		DRM_DEV_DEBUG_DRIVER(dev, "No need to poling HPD status.\n");
-		return;
-	}
-
 	ret = readx_poll_timeout(anx7625_read_hpd_status_p0,
 				 ctx, val,
 				 ((val & HPD_STATUS) || (val < 0)),
 				 5000,
 				 5000 * 100);
 	if (ret) {
-		DRM_DEV_ERROR(dev, "HPD polling timeout!\n");
-	} else {
-		DRM_DEV_DEBUG_DRIVER(dev, "HPD raise up.\n");
-		anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
-				  INTR_ALERT_1, 0xFF);
-		anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
-				  INTERFACE_CHANGE_INT, 0);
+		DRM_DEV_ERROR(dev, "no hpd.\n");
+		return;
 	}
 
-	anx7625_start_dp_work(ctx);
-}
-
-static void anx7625_disconnect_check(struct anx7625_data *ctx)
-{
-	if (atomic_read(&ctx->power_status) == 0)
-		anx7625_stop_dp_work(ctx);
-}
-
-static void anx7625_low_power_mode_check(struct anx7625_data *ctx,
-					 int state)
-{
-	struct device *dev = &ctx->client->dev;
+	DRM_DEV_DEBUG_DRIVER(dev, "system status: 0x%x. HPD raise up.\n", val);
+	anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
+			  INTR_ALERT_1, 0xFF);
+	anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
+			  INTERFACE_CHANGE_INT, 0);
 
-	DRM_DEV_DEBUG_DRIVER(dev, "low power mode check, state(%d).\n", state);
+	anx7625_start_dp_work(ctx);
 
-	if (ctx->pdata.low_power_mode) {
-		anx7625_chip_control(ctx, state);
-		if (state)
-			anx7625_hpd_polling(ctx);
-		else
-			anx7625_disconnect_check(ctx);
-	}
+	if (!ctx->pdata.panel_bridge && ctx->bridge_attached)
+		drm_helper_hpd_irq_event(ctx->bridge.dev);
 }
 
 static void anx7625_remove_edid(struct anx7625_data *ctx)
@@ -1180,9 +1128,6 @@ static int anx7625_hpd_change_detect(struct anx7625_data *ctx)
 	int intr_vector, status;
 	struct device *dev = &ctx->client->dev;
 
-	DRM_DEV_DEBUG_DRIVER(dev, "power_status=%d\n",
-			     (u32)atomic_read(&ctx->power_status));
-
 	status = anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
 				   INTR_ALERT_1, 0xFF);
 	if (status < 0) {
@@ -1228,22 +1173,25 @@ static void anx7625_work_func(struct work_struct *work)
 						struct anx7625_data, work);
 
 	mutex_lock(&ctx->lock);
+
+	if (pm_runtime_suspended(&ctx->client->dev))
+		goto unlock;
+
 	event = anx7625_hpd_change_detect(ctx);
-	mutex_unlock(&ctx->lock);
 	if (event < 0)
-		return;
+		goto unlock;
 
 	if (ctx->bridge_attached)
 		drm_helper_hpd_irq_event(ctx->bridge.dev);
+
+unlock:
+	mutex_unlock(&ctx->lock);
 }
 
 static irqreturn_t anx7625_intr_hpd_isr(int irq, void *data)
 {
 	struct anx7625_data *ctx = (struct anx7625_data *)data;
 
-	if (atomic_read(&ctx->power_status) != 1)
-		return IRQ_NONE;
-
 	queue_work(ctx->workqueue, &ctx->work);
 
 	return IRQ_HANDLED;
@@ -1305,9 +1253,9 @@ static struct edid *anx7625_get_edid(struct anx7625_data *ctx)
 		return (struct edid *)edid;
 	}
 
-	anx7625_low_power_mode_check(ctx, 1);
+	pm_runtime_get_sync(dev);
 	edid_num = sp_tx_edid_read(ctx, p_edid->edid_raw_data);
-	anx7625_low_power_mode_check(ctx, 0);
+	pm_runtime_put(dev);
 
 	if (edid_num < 1) {
 		DRM_DEV_ERROR(dev, "Fail to read EDID: %d\n", edid_num);
@@ -1611,10 +1559,7 @@ static void anx7625_bridge_enable(struct drm_bridge *bridge)
 
 	DRM_DEV_DEBUG_DRIVER(dev, "drm enable\n");
 
-	anx7625_low_power_mode_check(ctx, 1);
-
-	if (WARN_ON(!atomic_read(&ctx->power_status)))
-		return;
+	pm_runtime_get_sync(dev);
 
 	anx7625_dp_start(ctx);
 }
@@ -1624,14 +1569,11 @@ static void anx7625_bridge_disable(struct drm_bridge *bridge)
 	struct anx7625_data *ctx = bridge_to_anx7625(bridge);
 	struct device *dev = &ctx->client->dev;
 
-	if (WARN_ON(!atomic_read(&ctx->power_status)))
-		return;
-
 	DRM_DEV_DEBUG_DRIVER(dev, "drm disable\n");
 
 	anx7625_dp_stop(ctx);
 
-	anx7625_low_power_mode_check(ctx, 0);
+	pm_runtime_put(dev);
 }
 
 static enum drm_connector_status
@@ -1735,6 +1677,39 @@ static void anx7625_unregister_i2c_dummy_clients(struct anx7625_data *ctx)
 	i2c_unregister_device(ctx->i2c.tcpc_client);
 }
 
+static int __maybe_unused anx7625_runtime_pm_suspend(struct device *dev)
+{
+	struct anx7625_data *ctx = dev_get_drvdata(dev);
+
+	mutex_lock(&ctx->lock);
+
+	anx7625_stop_dp_work(ctx);
+	anx7625_power_standby(ctx);
+
+	mutex_unlock(&ctx->lock);
+
+	return 0;
+}
+
+static int __maybe_unused anx7625_runtime_pm_resume(struct device *dev)
+{
+	struct anx7625_data *ctx = dev_get_drvdata(dev);
+
+	mutex_lock(&ctx->lock);
+
+	anx7625_power_on_init(ctx);
+	anx7625_hpd_polling(ctx);
+
+	mutex_unlock(&ctx->lock);
+
+	return 0;
+}
+
+static const struct dev_pm_ops anx7625_pm_ops = {
+	SET_RUNTIME_PM_OPS(anx7625_runtime_pm_suspend,
+			   anx7625_runtime_pm_resume, NULL)
+};
+
 static int anx7625_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
@@ -1778,8 +1753,6 @@ static int anx7625_i2c_probe(struct i2c_client *client,
 	}
 	anx7625_init_gpio(platform);
 
-	atomic_set(&platform->power_status, 0);
-
 	mutex_init(&platform->lock);
 
 	platform->pdata.intp_irq = client->irq;
@@ -1809,9 +1782,11 @@ static int anx7625_i2c_probe(struct i2c_client *client,
 		goto free_wq;
 	}
 
-	if (platform->pdata.low_power_mode == 0) {
+	pm_runtime_enable(dev);
+
+	if (!platform->pdata.low_power_mode) {
 		anx7625_disable_pd_protocol(platform);
-		atomic_set(&platform->power_status, 1);
+		pm_runtime_get_sync(dev);
 	}
 
 	/* Add work function */
@@ -1847,6 +1822,9 @@ static int anx7625_i2c_remove(struct i2c_client *client)
 	if (platform->pdata.intp_irq)
 		destroy_workqueue(platform->workqueue);
 
+	if (!platform->pdata.low_power_mode)
+		pm_runtime_put_sync_suspend(&client->dev);
+
 	anx7625_unregister_i2c_dummy_clients(platform);
 
 	kfree(platform);
diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
index e4a086b3a3d7..034c3840028f 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.h
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
@@ -369,7 +369,6 @@ struct anx7625_i2c_client {
 
 struct anx7625_data {
 	struct anx7625_platform_data pdata;
-	atomic_t power_status;
 	int hpd_status;
 	int hpd_high_cnt;
 	/* Lock for work queue */

base-commit: e48661230cc35b3d0f4367eddfc19f86463ab917
-- 
2.31.1.607.g51e8a6a459-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/2] drm/bridge: anx7625: add suspend / resume hooks
  2021-05-07  6:44 ` Pi-Hsun Shih
@ 2021-05-07  6:44   ` Pi-Hsun Shih
  -1 siblings, 0 replies; 7+ messages in thread
From: Pi-Hsun Shih @ 2021-05-07  6:44 UTC (permalink / raw)
  Cc: Pi-Hsun Shih, Tzung-Bi Shih, Andrzej Hajda, Neil Armstrong,
	Robert Foss, Laurent Pinchart, Jonas Karlman, Jernej Skrabec,
	David Airlie, Daniel Vetter, Xin Ji, Hsin-Yi Wang, Sam Ravnborg,
	open list:DRM DRIVERS, open list

Add suspend / resume hooks for anx7625 driver, that power off the device
on suspend and power on the device on resume if it was previously
powered.

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
 drivers/gpu/drm/bridge/analogix/anx7625.c | 27 +++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index f56f8cf1f3bd..176d395c1a9f 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -1705,7 +1705,34 @@ static int __maybe_unused anx7625_runtime_pm_resume(struct device *dev)
 	return 0;
 }
 
+static int __maybe_unused anx7625_resume(struct device *dev)
+{
+	struct anx7625_data *ctx = dev_get_drvdata(dev);
+
+	if (!ctx->pdata.intp_irq)
+		return 0;
+
+	if (!pm_runtime_enabled(dev) || !pm_runtime_suspended(dev))
+		anx7625_runtime_pm_resume(dev);
+
+	return 0;
+}
+
+static int __maybe_unused anx7625_suspend(struct device *dev)
+{
+	struct anx7625_data *ctx = dev_get_drvdata(dev);
+
+	if (!ctx->pdata.intp_irq)
+		return 0;
+
+	if (!pm_runtime_enabled(dev) || !pm_runtime_suspended(dev))
+		anx7625_runtime_pm_suspend(dev);
+
+	return 0;
+}
+
 static const struct dev_pm_ops anx7625_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(anx7625_suspend, anx7625_resume)
 	SET_RUNTIME_PM_OPS(anx7625_runtime_pm_suspend,
 			   anx7625_runtime_pm_resume, NULL)
 };
-- 
2.31.1.607.g51e8a6a459-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v2 2/2] drm/bridge: anx7625: add suspend / resume hooks
@ 2021-05-07  6:44   ` Pi-Hsun Shih
  0 siblings, 0 replies; 7+ messages in thread
From: Pi-Hsun Shih @ 2021-05-07  6:44 UTC (permalink / raw)
  Cc: Jernej Skrabec, Neil Armstrong, David Airlie,
	open list:DRM DRIVERS, Jonas Karlman, open list, Robert Foss,
	Andrzej Hajda, Tzung-Bi Shih, Laurent Pinchart, Pi-Hsun Shih,
	Hsin-Yi Wang, Sam Ravnborg, Xin Ji

Add suspend / resume hooks for anx7625 driver, that power off the device
on suspend and power on the device on resume if it was previously
powered.

Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
---
 drivers/gpu/drm/bridge/analogix/anx7625.c | 27 +++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index f56f8cf1f3bd..176d395c1a9f 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -1705,7 +1705,34 @@ static int __maybe_unused anx7625_runtime_pm_resume(struct device *dev)
 	return 0;
 }
 
+static int __maybe_unused anx7625_resume(struct device *dev)
+{
+	struct anx7625_data *ctx = dev_get_drvdata(dev);
+
+	if (!ctx->pdata.intp_irq)
+		return 0;
+
+	if (!pm_runtime_enabled(dev) || !pm_runtime_suspended(dev))
+		anx7625_runtime_pm_resume(dev);
+
+	return 0;
+}
+
+static int __maybe_unused anx7625_suspend(struct device *dev)
+{
+	struct anx7625_data *ctx = dev_get_drvdata(dev);
+
+	if (!ctx->pdata.intp_irq)
+		return 0;
+
+	if (!pm_runtime_enabled(dev) || !pm_runtime_suspended(dev))
+		anx7625_runtime_pm_suspend(dev);
+
+	return 0;
+}
+
 static const struct dev_pm_ops anx7625_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(anx7625_suspend, anx7625_resume)
 	SET_RUNTIME_PM_OPS(anx7625_runtime_pm_suspend,
 			   anx7625_runtime_pm_resume, NULL)
 };
-- 
2.31.1.607.g51e8a6a459-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] drm/bridge: anx7625: refactor power control to use runtime PM framework
  2021-05-07  6:44 ` Pi-Hsun Shih
  (?)
  (?)
@ 2021-05-07 10:56 ` kernel test robot
  -1 siblings, 0 replies; 7+ messages in thread
From: kernel test robot @ 2021-05-07 10:56 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 2103 bytes --]

Hi Pi-Hsun,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on e48661230cc35b3d0f4367eddfc19f86463ab917]

url:    https://github.com/0day-ci/linux/commits/Pi-Hsun-Shih/drm-bridge-anx7625-refactor-power-control-to-use-runtime-PM-framework/20210507-145024
base:   e48661230cc35b3d0f4367eddfc19f86463ab917
config: x86_64-randconfig-a005-20210506 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project a3a8a1a15b524d91b5308db68e9d293b34cd88dd)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/5c7b6ff4145b6168c6b54de5569d6888518f5e12
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Pi-Hsun-Shih/drm-bridge-anx7625-refactor-power-control-to-use-runtime-PM-framework/20210507-145024
        git checkout 5c7b6ff4145b6168c6b54de5569d6888518f5e12
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/gpu/drm/bridge/analogix/anx7625.c:1708:32: warning: unused variable 'anx7625_pm_ops' [-Wunused-const-variable]
   static const struct dev_pm_ops anx7625_pm_ops = {
                                  ^
   1 warning generated.


vim +/anx7625_pm_ops +1708 drivers/gpu/drm/bridge/analogix/anx7625.c

  1707	
> 1708	static const struct dev_pm_ops anx7625_pm_ops = {
  1709		SET_RUNTIME_PM_OPS(anx7625_runtime_pm_suspend,
  1710				   anx7625_runtime_pm_resume, NULL)
  1711	};
  1712	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 39584 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] drm/bridge: anx7625: refactor power control to use runtime PM framework
  2021-05-07  6:44 ` Pi-Hsun Shih
@ 2021-05-07 12:15   ` Hsin-Yi Wang
  -1 siblings, 0 replies; 7+ messages in thread
From: Hsin-Yi Wang @ 2021-05-07 12:15 UTC (permalink / raw)
  To: Pi-Hsun Shih
  Cc: Tzung-Bi Shih, Andrzej Hajda, Neil Armstrong, Robert Foss,
	Laurent Pinchart, Jonas Karlman, Jernej Skrabec, David Airlie,
	Daniel Vetter, Xin Ji, Sam Ravnborg, open list:DRM DRIVERS,
	open list

On Fri, May 7, 2021 at 2:44 PM Pi-Hsun Shih <pihsun@chromium.org> wrote:
>
> The driver originally use an atomic_t for keep track of the power
> status, which makes the driver more complicated than needed, and has
> some race condition as it's possible to have the power on and power off
> sequence going at the same time.
>
> This patch remove the usage of the atomic_t power_status, and use the
> kernel runtime power management framework instead.
>
> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> ---
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 148 +++++++++-------------
>  drivers/gpu/drm/bridge/analogix/anx7625.h |   1 -
>  2 files changed, 63 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 23283ba0c4f9..f56f8cf1f3bd 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -11,6 +11,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> @@ -1005,33 +1006,6 @@ static void anx7625_power_on_init(struct anx7625_data *ctx)
>         }
>  }
>
> -static void anx7625_chip_control(struct anx7625_data *ctx, int state)
> -{
> -       struct device *dev = &ctx->client->dev;
> -
> -       DRM_DEV_DEBUG_DRIVER(dev, "before set, power_state(%d).\n",
> -                            atomic_read(&ctx->power_status));
> -
> -       if (!ctx->pdata.low_power_mode)
> -               return;
> -
> -       if (state) {
> -               atomic_inc(&ctx->power_status);
> -               if (atomic_read(&ctx->power_status) == 1)
> -                       anx7625_power_on_init(ctx);
> -       } else {
> -               if (atomic_read(&ctx->power_status)) {
> -                       atomic_dec(&ctx->power_status);
> -
> -                       if (atomic_read(&ctx->power_status) == 0)
> -                               anx7625_power_standby(ctx);
> -               }
> -       }
> -
> -       DRM_DEV_DEBUG_DRIVER(dev, "after set, power_state(%d).\n",
> -                            atomic_read(&ctx->power_status));
> -}
> -
>  static void anx7625_init_gpio(struct anx7625_data *platform)
>  {
>         struct device *dev = &platform->client->dev;
> @@ -1061,9 +1035,6 @@ static void anx7625_stop_dp_work(struct anx7625_data *ctx)
>         ctx->hpd_status = 0;
>         ctx->hpd_high_cnt = 0;
>         ctx->display_timing_valid = 0;
> -
> -       if (ctx->pdata.low_power_mode == 0)
> -               anx7625_disable_pd_protocol(ctx);
>  }
>
>  static void anx7625_start_dp_work(struct anx7625_data *ctx)
> @@ -1105,49 +1076,26 @@ static void anx7625_hpd_polling(struct anx7625_data *ctx)
>         int ret, val;
>         struct device *dev = &ctx->client->dev;
>
> -       if (atomic_read(&ctx->power_status) != 1) {
> -               DRM_DEV_DEBUG_DRIVER(dev, "No need to poling HPD status.\n");
> -               return;
> -       }
> -
>         ret = readx_poll_timeout(anx7625_read_hpd_status_p0,
>                                  ctx, val,
>                                  ((val & HPD_STATUS) || (val < 0)),
>                                  5000,
>                                  5000 * 100);
>         if (ret) {
> -               DRM_DEV_ERROR(dev, "HPD polling timeout!\n");
> -       } else {
> -               DRM_DEV_DEBUG_DRIVER(dev, "HPD raise up.\n");
> -               anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
> -                                 INTR_ALERT_1, 0xFF);
> -               anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> -                                 INTERFACE_CHANGE_INT, 0);
> +               DRM_DEV_ERROR(dev, "no hpd.\n");
> +               return;
>         }
>
> -       anx7625_start_dp_work(ctx);
> -}
> -
> -static void anx7625_disconnect_check(struct anx7625_data *ctx)
> -{
> -       if (atomic_read(&ctx->power_status) == 0)
> -               anx7625_stop_dp_work(ctx);
> -}
> -
> -static void anx7625_low_power_mode_check(struct anx7625_data *ctx,
> -                                        int state)
> -{
> -       struct device *dev = &ctx->client->dev;
> +       DRM_DEV_DEBUG_DRIVER(dev, "system status: 0x%x. HPD raise up.\n", val);
> +       anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
> +                         INTR_ALERT_1, 0xFF);
> +       anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +                         INTERFACE_CHANGE_INT, 0);
>
> -       DRM_DEV_DEBUG_DRIVER(dev, "low power mode check, state(%d).\n", state);
> +       anx7625_start_dp_work(ctx);
>
> -       if (ctx->pdata.low_power_mode) {
> -               anx7625_chip_control(ctx, state);
> -               if (state)
> -                       anx7625_hpd_polling(ctx);
> -               else
> -                       anx7625_disconnect_check(ctx);
> -       }
> +       if (!ctx->pdata.panel_bridge && ctx->bridge_attached)
> +               drm_helper_hpd_irq_event(ctx->bridge.dev);
>  }
>
>  static void anx7625_remove_edid(struct anx7625_data *ctx)
> @@ -1180,9 +1128,6 @@ static int anx7625_hpd_change_detect(struct anx7625_data *ctx)
>         int intr_vector, status;
>         struct device *dev = &ctx->client->dev;
>
> -       DRM_DEV_DEBUG_DRIVER(dev, "power_status=%d\n",
> -                            (u32)atomic_read(&ctx->power_status));
> -
>         status = anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
>                                    INTR_ALERT_1, 0xFF);
>         if (status < 0) {
> @@ -1228,22 +1173,25 @@ static void anx7625_work_func(struct work_struct *work)
>                                                 struct anx7625_data, work);
>
>         mutex_lock(&ctx->lock);
> +
> +       if (pm_runtime_suspended(&ctx->client->dev))
> +               goto unlock;
> +
>         event = anx7625_hpd_change_detect(ctx);
> -       mutex_unlock(&ctx->lock);
>         if (event < 0)
> -               return;
> +               goto unlock;
>
>         if (ctx->bridge_attached)
>                 drm_helper_hpd_irq_event(ctx->bridge.dev);
> +
> +unlock:
> +       mutex_unlock(&ctx->lock);
>  }
>
>  static irqreturn_t anx7625_intr_hpd_isr(int irq, void *data)
>  {
>         struct anx7625_data *ctx = (struct anx7625_data *)data;
>
> -       if (atomic_read(&ctx->power_status) != 1)
> -               return IRQ_NONE;
> -
>         queue_work(ctx->workqueue, &ctx->work);
>
>         return IRQ_HANDLED;
> @@ -1305,9 +1253,9 @@ static struct edid *anx7625_get_edid(struct anx7625_data *ctx)
>                 return (struct edid *)edid;
>         }
>
> -       anx7625_low_power_mode_check(ctx, 1);
> +       pm_runtime_get_sync(dev);
>         edid_num = sp_tx_edid_read(ctx, p_edid->edid_raw_data);
> -       anx7625_low_power_mode_check(ctx, 0);
> +       pm_runtime_put(dev);
>
>         if (edid_num < 1) {
>                 DRM_DEV_ERROR(dev, "Fail to read EDID: %d\n", edid_num);
> @@ -1611,10 +1559,7 @@ static void anx7625_bridge_enable(struct drm_bridge *bridge)
>
>         DRM_DEV_DEBUG_DRIVER(dev, "drm enable\n");
>
> -       anx7625_low_power_mode_check(ctx, 1);
> -
> -       if (WARN_ON(!atomic_read(&ctx->power_status)))
> -               return;
> +       pm_runtime_get_sync(dev);
>
>         anx7625_dp_start(ctx);
>  }
> @@ -1624,14 +1569,11 @@ static void anx7625_bridge_disable(struct drm_bridge *bridge)
>         struct anx7625_data *ctx = bridge_to_anx7625(bridge);
>         struct device *dev = &ctx->client->dev;
>
> -       if (WARN_ON(!atomic_read(&ctx->power_status)))
> -               return;
> -
>         DRM_DEV_DEBUG_DRIVER(dev, "drm disable\n");
>
>         anx7625_dp_stop(ctx);
>
> -       anx7625_low_power_mode_check(ctx, 0);
> +       pm_runtime_put(dev);
>  }
>
>  static enum drm_connector_status
> @@ -1735,6 +1677,39 @@ static void anx7625_unregister_i2c_dummy_clients(struct anx7625_data *ctx)
>         i2c_unregister_device(ctx->i2c.tcpc_client);
>  }
>
> +static int __maybe_unused anx7625_runtime_pm_suspend(struct device *dev)
> +{
> +       struct anx7625_data *ctx = dev_get_drvdata(dev);
> +
> +       mutex_lock(&ctx->lock);
> +
> +       anx7625_stop_dp_work(ctx);
> +       anx7625_power_standby(ctx);
> +
> +       mutex_unlock(&ctx->lock);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused anx7625_runtime_pm_resume(struct device *dev)
> +{
> +       struct anx7625_data *ctx = dev_get_drvdata(dev);
> +
> +       mutex_lock(&ctx->lock);
> +
> +       anx7625_power_on_init(ctx);
> +       anx7625_hpd_polling(ctx);
> +
> +       mutex_unlock(&ctx->lock);
> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops anx7625_pm_ops = {
> +       SET_RUNTIME_PM_OPS(anx7625_runtime_pm_suspend,
> +                          anx7625_runtime_pm_resume, NULL)
> +};
> +

.pm = &anx7625_pm_ops, is missing in static struct i2c_driver
anx7625_driver{...}

>  static int anx7625_i2c_probe(struct i2c_client *client,
>                              const struct i2c_device_id *id)
>  {
> @@ -1778,8 +1753,6 @@ static int anx7625_i2c_probe(struct i2c_client *client,
>         }
>         anx7625_init_gpio(platform);
>
> -       atomic_set(&platform->power_status, 0);
> -
>         mutex_init(&platform->lock);
>
>         platform->pdata.intp_irq = client->irq;
> @@ -1809,9 +1782,11 @@ static int anx7625_i2c_probe(struct i2c_client *client,
>                 goto free_wq;
>         }
>
> -       if (platform->pdata.low_power_mode == 0) {
> +       pm_runtime_enable(dev);
> +
> +       if (!platform->pdata.low_power_mode) {
>                 anx7625_disable_pd_protocol(platform);
> -               atomic_set(&platform->power_status, 1);
> +               pm_runtime_get_sync(dev);
>         }
>
>         /* Add work function */
> @@ -1847,6 +1822,9 @@ static int anx7625_i2c_remove(struct i2c_client *client)
>         if (platform->pdata.intp_irq)
>                 destroy_workqueue(platform->workqueue);
>
> +       if (!platform->pdata.low_power_mode)
> +               pm_runtime_put_sync_suspend(&client->dev);
> +
>         anx7625_unregister_i2c_dummy_clients(platform);
>
>         kfree(platform);
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> index e4a086b3a3d7..034c3840028f 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> @@ -369,7 +369,6 @@ struct anx7625_i2c_client {
>
>  struct anx7625_data {
>         struct anx7625_platform_data pdata;
> -       atomic_t power_status;
>         int hpd_status;
>         int hpd_high_cnt;
>         /* Lock for work queue */
>
> base-commit: e48661230cc35b3d0f4367eddfc19f86463ab917
> --
> 2.31.1.607.g51e8a6a459-goog
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v2 1/2] drm/bridge: anx7625: refactor power control to use runtime PM framework
@ 2021-05-07 12:15   ` Hsin-Yi Wang
  0 siblings, 0 replies; 7+ messages in thread
From: Hsin-Yi Wang @ 2021-05-07 12:15 UTC (permalink / raw)
  To: Pi-Hsun Shih
  Cc: Jernej Skrabec, Jonas Karlman, David Airlie,
	open list:DRM DRIVERS, Neil Armstrong, open list, Robert Foss,
	Andrzej Hajda, Tzung-Bi Shih, Laurent Pinchart, Sam Ravnborg,
	Xin Ji

On Fri, May 7, 2021 at 2:44 PM Pi-Hsun Shih <pihsun@chromium.org> wrote:
>
> The driver originally use an atomic_t for keep track of the power
> status, which makes the driver more complicated than needed, and has
> some race condition as it's possible to have the power on and power off
> sequence going at the same time.
>
> This patch remove the usage of the atomic_t power_status, and use the
> kernel runtime power management framework instead.
>
> Signed-off-by: Pi-Hsun Shih <pihsun@chromium.org>
> ---
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 148 +++++++++-------------
>  drivers/gpu/drm/bridge/analogix/anx7625.h |   1 -
>  2 files changed, 63 insertions(+), 86 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
> index 23283ba0c4f9..f56f8cf1f3bd 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.c
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
> @@ -11,6 +11,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> @@ -1005,33 +1006,6 @@ static void anx7625_power_on_init(struct anx7625_data *ctx)
>         }
>  }
>
> -static void anx7625_chip_control(struct anx7625_data *ctx, int state)
> -{
> -       struct device *dev = &ctx->client->dev;
> -
> -       DRM_DEV_DEBUG_DRIVER(dev, "before set, power_state(%d).\n",
> -                            atomic_read(&ctx->power_status));
> -
> -       if (!ctx->pdata.low_power_mode)
> -               return;
> -
> -       if (state) {
> -               atomic_inc(&ctx->power_status);
> -               if (atomic_read(&ctx->power_status) == 1)
> -                       anx7625_power_on_init(ctx);
> -       } else {
> -               if (atomic_read(&ctx->power_status)) {
> -                       atomic_dec(&ctx->power_status);
> -
> -                       if (atomic_read(&ctx->power_status) == 0)
> -                               anx7625_power_standby(ctx);
> -               }
> -       }
> -
> -       DRM_DEV_DEBUG_DRIVER(dev, "after set, power_state(%d).\n",
> -                            atomic_read(&ctx->power_status));
> -}
> -
>  static void anx7625_init_gpio(struct anx7625_data *platform)
>  {
>         struct device *dev = &platform->client->dev;
> @@ -1061,9 +1035,6 @@ static void anx7625_stop_dp_work(struct anx7625_data *ctx)
>         ctx->hpd_status = 0;
>         ctx->hpd_high_cnt = 0;
>         ctx->display_timing_valid = 0;
> -
> -       if (ctx->pdata.low_power_mode == 0)
> -               anx7625_disable_pd_protocol(ctx);
>  }
>
>  static void anx7625_start_dp_work(struct anx7625_data *ctx)
> @@ -1105,49 +1076,26 @@ static void anx7625_hpd_polling(struct anx7625_data *ctx)
>         int ret, val;
>         struct device *dev = &ctx->client->dev;
>
> -       if (atomic_read(&ctx->power_status) != 1) {
> -               DRM_DEV_DEBUG_DRIVER(dev, "No need to poling HPD status.\n");
> -               return;
> -       }
> -
>         ret = readx_poll_timeout(anx7625_read_hpd_status_p0,
>                                  ctx, val,
>                                  ((val & HPD_STATUS) || (val < 0)),
>                                  5000,
>                                  5000 * 100);
>         if (ret) {
> -               DRM_DEV_ERROR(dev, "HPD polling timeout!\n");
> -       } else {
> -               DRM_DEV_DEBUG_DRIVER(dev, "HPD raise up.\n");
> -               anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
> -                                 INTR_ALERT_1, 0xFF);
> -               anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> -                                 INTERFACE_CHANGE_INT, 0);
> +               DRM_DEV_ERROR(dev, "no hpd.\n");
> +               return;
>         }
>
> -       anx7625_start_dp_work(ctx);
> -}
> -
> -static void anx7625_disconnect_check(struct anx7625_data *ctx)
> -{
> -       if (atomic_read(&ctx->power_status) == 0)
> -               anx7625_stop_dp_work(ctx);
> -}
> -
> -static void anx7625_low_power_mode_check(struct anx7625_data *ctx,
> -                                        int state)
> -{
> -       struct device *dev = &ctx->client->dev;
> +       DRM_DEV_DEBUG_DRIVER(dev, "system status: 0x%x. HPD raise up.\n", val);
> +       anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
> +                         INTR_ALERT_1, 0xFF);
> +       anx7625_reg_write(ctx, ctx->i2c.rx_p0_client,
> +                         INTERFACE_CHANGE_INT, 0);
>
> -       DRM_DEV_DEBUG_DRIVER(dev, "low power mode check, state(%d).\n", state);
> +       anx7625_start_dp_work(ctx);
>
> -       if (ctx->pdata.low_power_mode) {
> -               anx7625_chip_control(ctx, state);
> -               if (state)
> -                       anx7625_hpd_polling(ctx);
> -               else
> -                       anx7625_disconnect_check(ctx);
> -       }
> +       if (!ctx->pdata.panel_bridge && ctx->bridge_attached)
> +               drm_helper_hpd_irq_event(ctx->bridge.dev);
>  }
>
>  static void anx7625_remove_edid(struct anx7625_data *ctx)
> @@ -1180,9 +1128,6 @@ static int anx7625_hpd_change_detect(struct anx7625_data *ctx)
>         int intr_vector, status;
>         struct device *dev = &ctx->client->dev;
>
> -       DRM_DEV_DEBUG_DRIVER(dev, "power_status=%d\n",
> -                            (u32)atomic_read(&ctx->power_status));
> -
>         status = anx7625_reg_write(ctx, ctx->i2c.tcpc_client,
>                                    INTR_ALERT_1, 0xFF);
>         if (status < 0) {
> @@ -1228,22 +1173,25 @@ static void anx7625_work_func(struct work_struct *work)
>                                                 struct anx7625_data, work);
>
>         mutex_lock(&ctx->lock);
> +
> +       if (pm_runtime_suspended(&ctx->client->dev))
> +               goto unlock;
> +
>         event = anx7625_hpd_change_detect(ctx);
> -       mutex_unlock(&ctx->lock);
>         if (event < 0)
> -               return;
> +               goto unlock;
>
>         if (ctx->bridge_attached)
>                 drm_helper_hpd_irq_event(ctx->bridge.dev);
> +
> +unlock:
> +       mutex_unlock(&ctx->lock);
>  }
>
>  static irqreturn_t anx7625_intr_hpd_isr(int irq, void *data)
>  {
>         struct anx7625_data *ctx = (struct anx7625_data *)data;
>
> -       if (atomic_read(&ctx->power_status) != 1)
> -               return IRQ_NONE;
> -
>         queue_work(ctx->workqueue, &ctx->work);
>
>         return IRQ_HANDLED;
> @@ -1305,9 +1253,9 @@ static struct edid *anx7625_get_edid(struct anx7625_data *ctx)
>                 return (struct edid *)edid;
>         }
>
> -       anx7625_low_power_mode_check(ctx, 1);
> +       pm_runtime_get_sync(dev);
>         edid_num = sp_tx_edid_read(ctx, p_edid->edid_raw_data);
> -       anx7625_low_power_mode_check(ctx, 0);
> +       pm_runtime_put(dev);
>
>         if (edid_num < 1) {
>                 DRM_DEV_ERROR(dev, "Fail to read EDID: %d\n", edid_num);
> @@ -1611,10 +1559,7 @@ static void anx7625_bridge_enable(struct drm_bridge *bridge)
>
>         DRM_DEV_DEBUG_DRIVER(dev, "drm enable\n");
>
> -       anx7625_low_power_mode_check(ctx, 1);
> -
> -       if (WARN_ON(!atomic_read(&ctx->power_status)))
> -               return;
> +       pm_runtime_get_sync(dev);
>
>         anx7625_dp_start(ctx);
>  }
> @@ -1624,14 +1569,11 @@ static void anx7625_bridge_disable(struct drm_bridge *bridge)
>         struct anx7625_data *ctx = bridge_to_anx7625(bridge);
>         struct device *dev = &ctx->client->dev;
>
> -       if (WARN_ON(!atomic_read(&ctx->power_status)))
> -               return;
> -
>         DRM_DEV_DEBUG_DRIVER(dev, "drm disable\n");
>
>         anx7625_dp_stop(ctx);
>
> -       anx7625_low_power_mode_check(ctx, 0);
> +       pm_runtime_put(dev);
>  }
>
>  static enum drm_connector_status
> @@ -1735,6 +1677,39 @@ static void anx7625_unregister_i2c_dummy_clients(struct anx7625_data *ctx)
>         i2c_unregister_device(ctx->i2c.tcpc_client);
>  }
>
> +static int __maybe_unused anx7625_runtime_pm_suspend(struct device *dev)
> +{
> +       struct anx7625_data *ctx = dev_get_drvdata(dev);
> +
> +       mutex_lock(&ctx->lock);
> +
> +       anx7625_stop_dp_work(ctx);
> +       anx7625_power_standby(ctx);
> +
> +       mutex_unlock(&ctx->lock);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused anx7625_runtime_pm_resume(struct device *dev)
> +{
> +       struct anx7625_data *ctx = dev_get_drvdata(dev);
> +
> +       mutex_lock(&ctx->lock);
> +
> +       anx7625_power_on_init(ctx);
> +       anx7625_hpd_polling(ctx);
> +
> +       mutex_unlock(&ctx->lock);
> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops anx7625_pm_ops = {
> +       SET_RUNTIME_PM_OPS(anx7625_runtime_pm_suspend,
> +                          anx7625_runtime_pm_resume, NULL)
> +};
> +

.pm = &anx7625_pm_ops, is missing in static struct i2c_driver
anx7625_driver{...}

>  static int anx7625_i2c_probe(struct i2c_client *client,
>                              const struct i2c_device_id *id)
>  {
> @@ -1778,8 +1753,6 @@ static int anx7625_i2c_probe(struct i2c_client *client,
>         }
>         anx7625_init_gpio(platform);
>
> -       atomic_set(&platform->power_status, 0);
> -
>         mutex_init(&platform->lock);
>
>         platform->pdata.intp_irq = client->irq;
> @@ -1809,9 +1782,11 @@ static int anx7625_i2c_probe(struct i2c_client *client,
>                 goto free_wq;
>         }
>
> -       if (platform->pdata.low_power_mode == 0) {
> +       pm_runtime_enable(dev);
> +
> +       if (!platform->pdata.low_power_mode) {
>                 anx7625_disable_pd_protocol(platform);
> -               atomic_set(&platform->power_status, 1);
> +               pm_runtime_get_sync(dev);
>         }
>
>         /* Add work function */
> @@ -1847,6 +1822,9 @@ static int anx7625_i2c_remove(struct i2c_client *client)
>         if (platform->pdata.intp_irq)
>                 destroy_workqueue(platform->workqueue);
>
> +       if (!platform->pdata.low_power_mode)
> +               pm_runtime_put_sync_suspend(&client->dev);
> +
>         anx7625_unregister_i2c_dummy_clients(platform);
>
>         kfree(platform);
> diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.h b/drivers/gpu/drm/bridge/analogix/anx7625.h
> index e4a086b3a3d7..034c3840028f 100644
> --- a/drivers/gpu/drm/bridge/analogix/anx7625.h
> +++ b/drivers/gpu/drm/bridge/analogix/anx7625.h
> @@ -369,7 +369,6 @@ struct anx7625_i2c_client {
>
>  struct anx7625_data {
>         struct anx7625_platform_data pdata;
> -       atomic_t power_status;
>         int hpd_status;
>         int hpd_high_cnt;
>         /* Lock for work queue */
>
> base-commit: e48661230cc35b3d0f4367eddfc19f86463ab917
> --
> 2.31.1.607.g51e8a6a459-goog
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-05-07 12:15 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-07  6:44 [PATCH v2 1/2] drm/bridge: anx7625: refactor power control to use runtime PM framework Pi-Hsun Shih
2021-05-07  6:44 ` Pi-Hsun Shih
2021-05-07  6:44 ` [PATCH v2 2/2] drm/bridge: anx7625: add suspend / resume hooks Pi-Hsun Shih
2021-05-07  6:44   ` Pi-Hsun Shih
2021-05-07 10:56 ` [PATCH v2 1/2] drm/bridge: anx7625: refactor power control to use runtime PM framework kernel test robot
2021-05-07 12:15 ` Hsin-Yi Wang
2021-05-07 12:15   ` Hsin-Yi Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.