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

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

 



Tomi Valkeinen <tomi.valkeinen@xxxxxx> 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 <tomi.valkeinen@xxxxxx>

[...]

> +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().

> +		WARN_ON(r);
> +
> +		clk_disable(dispc.dss_clk);
> +
> +		dss_runtime_put();
> +	}
> +
> +	mutex_unlock(&dispc.runtime_lock);
> +}
> +
> +
>  bool dispc_go_busy(enum omap_channel channel)
>  {
>  	int bit;
> @@ -530,7 +645,7 @@ void dispc_go(enum omap_channel channel)
>  	int bit;
>  	bool enable_bit, go_bit;
>  
> -	enable_clocks(1);
> +	dispc_runtime_get();

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

>  	if (channel == OMAP_DSS_CHANNEL_LCD ||
>  			channel == OMAP_DSS_CHANNEL_LCD2)
> @@ -571,7 +686,7 @@ void dispc_go(enum omap_channel channel)
>  	else
>  		REG_FLD_MOD(DISPC_CONTROL, 1, bit, bit);
>  end:
> -	enable_clocks(0);
> +	dispc_runtime_put();

...and the pm_runtime_put() here.


Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Arm (vger)]     [ARM Kernel]     [ARM MSM]     [Linux Tegra]     [Linux WPAN Networking]     [Linux Wireless Networking]     [Maemo Users]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Trails]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux