Re: [PATCH 19/27] OMAP: DSS2: Use PM runtime & HWMOD support

Tomi Valkeinen [off-list ref] writes:

Use PM runtime and HWMOD support to handle enabling and disabling of DSS
modules.

Each DSS module will have get and put functions which can be used to
enable and disable that module. The functions use pm_runtime and hwmod
opt-clocks to enable the hardware.

Signed-off-by: Tomi Valkeinen <redacted>

[...]

+int dispc_runtime_get(void)
+{
+	int r;
+
+	mutex_lock(&dispc.runtime_lock);

It's not clear to me what the lock is trying to protect.  I guess it's
the counter?  I don't think it should be needed...

+	if (dispc.runtime_count++ = 0) {

You shouldn't need your own use-counting here.  The runtime PM core is
already doing usage counting.

Instead, you need to use the ->runtime_suspend() and ->runtime_resume()
callbacks (in dev_pm_ops).  These callbacks are called by the runtime PM
core when the usage count goes to/from zero.

IOW, this function should simply be a pm_runtime_get_sync(), and the
rest of the stuff in this function should be done in the
->runtime_resume() callback, which is only executed when the usage_count
transitions from zero.

+		DSSDBG("dispc_runtime_get\n");
+
+		r = dss_runtime_get();
+		if (r)
+			goto err_dss_get;
+
+		/* XXX dispc fclk can also come from DSI PLL */
+		clk_enable(dispc.dss_clk);
+
+		r = pm_runtime_get_sync(&dispc.pdev->dev);
+		WARN_ON(r);
+		if (r < 0)
+			goto err_runtime_get;
+
+		if (dispc_need_ctx_restore())
+			dispc_restore_context();
+	}
+
+	mutex_unlock(&dispc.runtime_lock);
+
+	return 0;
+
+err_runtime_get:
+	clk_disable(dispc.dss_clk);
+	dss_runtime_put();
+err_dss_get:
+	mutex_unlock(&dispc.runtime_lock);
+
+	return r;
 }
 
+void dispc_runtime_put(void)
+{
+	mutex_lock(&dispc.runtime_lock);
+
+	if (--dispc.runtime_count = 0) {
+		int r;

Same problem here.  

No usage counting is required (and probably no locking either.)  This
function should simply be a pm_runtime_put(), and the rest of the stuff
should be in the driver's ->runtime_suspend() callback.

+		DSSDBG("dispc_runtime_put\n");
+
+		dispc_save_context();
+
+		r = pm_runtime_put_sync(&dispc.pdev->dev);

Does this need to be the _sync() version?  It looks like it could be use
the "normal" (async) version.: pm_runtime_put().

Based on the above suggestions, you probably don't need dedicated
functions for this.  Just call pm_runtime_get_sync() here...

...and the pm_runtime_put() here.


Kevin