Re: [PATCH v7 4/4] clk: samsung: exynos-audss: Use runtime PM

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

 



On Fri, Aug 04, 2017 at 11:34:54AM +0200, Marek Szyprowski wrote:
> This patch adds support for runtime PM to Exynos Audio SubSystem driver to
> enable full support for audio power domain on Exynos5 SoCs. The main change
> is moving register saving and restoring code from system sleep PM ops to
> runtime PM ops and implementing system sleep PM ops with generic
> pm_runtime_force_suspend/resume helpers. Runtime PM of the Exynos AudSS
> device is managed from clock core depending on the preparation status
> of the provided clocks.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> ---
>  .../devicetree/bindings/clock/clk-exynos-audss.txt |  6 ++
>  drivers/clk/samsung/clk-exynos-audss.c             | 68 +++++++++++++---------
>  2 files changed, 45 insertions(+), 29 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/clock/clk-exynos-audss.txt b/Documentation/devicetree/bindings/clock/clk-exynos-audss.txt
> index 0c3d6015868d..f3635d5aeba4 100644
> --- a/Documentation/devicetree/bindings/clock/clk-exynos-audss.txt
> +++ b/Documentation/devicetree/bindings/clock/clk-exynos-audss.txt
> @@ -33,6 +33,12 @@ Required Properties:
>  - clock-names: Aliases for the above clocks. They should be "pll_ref",
>    "pll_in", "cdclk", "sclk_audio", and "sclk_pcm_in" respectively.
>  
> +Optional Properties:
> +
> +  - power-domains: a phandle to respective power domain node as described by
> +    generic PM domain bindings (see power/power_domain.txt for more
> +    information).
> +
>  The following is the list of clocks generated by the controller. Each clock is
>  assigned an identifier and client nodes use this identifier to specify the
>  clock which they consume. Some of the clocks are available only on a particular
> diff --git a/drivers/clk/samsung/clk-exynos-audss.c b/drivers/clk/samsung/clk-exynos-audss.c
> index 1fab56f396d4..a754c91d6ef3 100644
> --- a/drivers/clk/samsung/clk-exynos-audss.c
> +++ b/drivers/clk/samsung/clk-exynos-audss.c
> @@ -18,6 +18,7 @@
>  #include <linux/syscore_ops.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <dt-bindings/clock/exynos-audss-clk.h>
>  
> @@ -36,14 +37,13 @@
>  #define ASS_CLK_DIV 0x4
>  #define ASS_CLK_GATE 0x8
>  
> -#ifdef CONFIG_PM_SLEEP
>  static unsigned long reg_save[][2] = {
>  	{ ASS_CLK_SRC,  0 },
>  	{ ASS_CLK_DIV,  0 },
>  	{ ASS_CLK_GATE, 0 },
>  };
>  
> -static int exynos_audss_clk_suspend(struct device *dev)
> +static int __maybe_unused exynos_audss_clk_suspend(struct device *dev)
>  {
>  	int i;
>  
> @@ -53,7 +53,7 @@ static int exynos_audss_clk_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int exynos_audss_clk_resume(struct device *dev)
> +static int __maybe_unused exynos_audss_clk_resume(struct device *dev)
>  {
>  	int i;
>  
> @@ -62,7 +62,6 @@ static int exynos_audss_clk_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif /* CONFIG_PM_SLEEP */
>  
>  struct exynos_audss_clk_drvdata {
>  	unsigned int has_adma_clk:1;
> @@ -135,6 +134,7 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
>  	const struct exynos_audss_clk_drvdata *variant;
>  	struct clk_hw **clk_table;
>  	struct resource *res;
> +	struct device *dev = &pdev->dev;

I would prefer all these pdev->dev changes to be a separate patch
(simplifying the code before adding new feature). This would reduce this
patch itself a lot and make it easier to review.

>  	int i, ret = 0;
>  
>  	variant = of_device_get_match_data(&pdev->dev);
> @@ -142,15 +142,15 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
>  		return -EINVAL;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	reg_base = devm_ioremap_resource(&pdev->dev, res);
> +	reg_base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(reg_base)) {
> -		dev_err(&pdev->dev, "failed to map audss registers\n");
> +		dev_err(dev, "failed to map audss registers\n");
>  		return PTR_ERR(reg_base);
>  	}
>  
>  	epll = ERR_PTR(-ENODEV);
>  
> -	clk_data = devm_kzalloc(&pdev->dev,
> +	clk_data = devm_kzalloc(dev,
>  				sizeof(*clk_data) +
>  				sizeof(*clk_data->hws) * EXYNOS_AUDSS_MAX_CLKS,
>  				GFP_KERNEL);
> @@ -160,8 +160,8 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
>  	clk_data->num = variant->num_clks;
>  	clk_table = clk_data->hws;
>  
> -	pll_ref = devm_clk_get(&pdev->dev, "pll_ref");
> -	pll_in = devm_clk_get(&pdev->dev, "pll_in");
> +	pll_ref = devm_clk_get(dev, "pll_ref");
> +	pll_in = devm_clk_get(dev, "pll_in");
>  	if (!IS_ERR(pll_ref))
>  		mout_audss_p[0] = __clk_get_name(pll_ref);
>  	if (!IS_ERR(pll_in)) {
> @@ -172,81 +172,89 @@ static int exynos_audss_clk_probe(struct platform_device *pdev)
>  
>  			ret = clk_prepare_enable(epll);
>  			if (ret) {
> -				dev_err(&pdev->dev,
> +				dev_err(dev,
>  					"failed to prepare the epll clock\n");
>  				return ret;
>  			}
>  		}
>  	}
> -	clk_table[EXYNOS_MOUT_AUDSS] = clk_hw_register_mux(NULL, "mout_audss",
> +
> +	/*
> +	 * Enable runtime PM here to allow the clock core using runtime PM
> +	 * for the registered clocks.
> +	 */
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
> +	clk_table[EXYNOS_MOUT_AUDSS] = clk_hw_register_mux(dev, "mout_audss",
>  				mout_audss_p, ARRAY_SIZE(mout_audss_p),
>  				CLK_SET_RATE_NO_REPARENT,
>  				reg_base + ASS_CLK_SRC, 0, 1, 0, &lock);
>  
> -	cdclk = devm_clk_get(&pdev->dev, "cdclk");
> -	sclk_audio = devm_clk_get(&pdev->dev, "sclk_audio");
> +	cdclk = devm_clk_get(dev, "cdclk");
> +	sclk_audio = devm_clk_get(dev, "sclk_audio");
>  	if (!IS_ERR(cdclk))
>  		mout_i2s_p[1] = __clk_get_name(cdclk);
>  	if (!IS_ERR(sclk_audio))
>  		mout_i2s_p[2] = __clk_get_name(sclk_audio);
> -	clk_table[EXYNOS_MOUT_I2S] = clk_hw_register_mux(NULL, "mout_i2s",
> +	clk_table[EXYNOS_MOUT_I2S] = clk_hw_register_mux(dev, "mout_i2s",
>  				mout_i2s_p, ARRAY_SIZE(mout_i2s_p),
>  				CLK_SET_RATE_NO_REPARENT,
>  				reg_base + ASS_CLK_SRC, 2, 2, 0, &lock);
>  
> -	clk_table[EXYNOS_DOUT_SRP] = clk_hw_register_divider(NULL, "dout_srp",
> +	clk_table[EXYNOS_DOUT_SRP] = clk_hw_register_divider(dev, "dout_srp",
>  				"mout_audss", 0, reg_base + ASS_CLK_DIV, 0, 4,
>  				0, &lock);
>  
> -	clk_table[EXYNOS_DOUT_AUD_BUS] = clk_hw_register_divider(NULL,
> +	clk_table[EXYNOS_DOUT_AUD_BUS] = clk_hw_register_divider(dev,
>  				"dout_aud_bus", "dout_srp", 0,
>  				reg_base + ASS_CLK_DIV, 4, 4, 0, &lock);
>  
> -	clk_table[EXYNOS_DOUT_I2S] = clk_hw_register_divider(NULL, "dout_i2s",
> +	clk_table[EXYNOS_DOUT_I2S] = clk_hw_register_divider(dev, "dout_i2s",
>  				"mout_i2s", 0, reg_base + ASS_CLK_DIV, 8, 4, 0,
>  				&lock);
>  
> -	clk_table[EXYNOS_SRP_CLK] = clk_hw_register_gate(NULL, "srp_clk",
> +	clk_table[EXYNOS_SRP_CLK] = clk_hw_register_gate(dev, "srp_clk",
>  				"dout_srp", CLK_SET_RATE_PARENT,
>  				reg_base + ASS_CLK_GATE, 0, 0, &lock);
>  
> -	clk_table[EXYNOS_I2S_BUS] = clk_hw_register_gate(NULL, "i2s_bus",
> +	clk_table[EXYNOS_I2S_BUS] = clk_hw_register_gate(dev, "i2s_bus",
>  				"dout_aud_bus", CLK_SET_RATE_PARENT,
>  				reg_base + ASS_CLK_GATE, 2, 0, &lock);
>  
> -	clk_table[EXYNOS_SCLK_I2S] = clk_hw_register_gate(NULL, "sclk_i2s",
> +	clk_table[EXYNOS_SCLK_I2S] = clk_hw_register_gate(dev, "sclk_i2s",
>  				"dout_i2s", CLK_SET_RATE_PARENT,
>  				reg_base + ASS_CLK_GATE, 3, 0, &lock);
>  
> -	clk_table[EXYNOS_PCM_BUS] = clk_hw_register_gate(NULL, "pcm_bus",
> +	clk_table[EXYNOS_PCM_BUS] = clk_hw_register_gate(dev, "pcm_bus",
>  				 "sclk_pcm", CLK_SET_RATE_PARENT,
>  				reg_base + ASS_CLK_GATE, 4, 0, &lock);
>  
> -	sclk_pcm_in = devm_clk_get(&pdev->dev, "sclk_pcm_in");
> +	sclk_pcm_in = devm_clk_get(dev, "sclk_pcm_in");
>  	if (!IS_ERR(sclk_pcm_in))
>  		sclk_pcm_p = __clk_get_name(sclk_pcm_in);
> -	clk_table[EXYNOS_SCLK_PCM] = clk_hw_register_gate(NULL, "sclk_pcm",
> +	clk_table[EXYNOS_SCLK_PCM] = clk_hw_register_gate(dev, "sclk_pcm",
>  				sclk_pcm_p, CLK_SET_RATE_PARENT,
>  				reg_base + ASS_CLK_GATE, 5, 0, &lock);
>  
>  	if (variant->has_adma_clk) {
> -		clk_table[EXYNOS_ADMA] = clk_hw_register_gate(NULL, "adma",
> +		clk_table[EXYNOS_ADMA] = clk_hw_register_gate(dev, "adma",
>  				"dout_srp", CLK_SET_RATE_PARENT,
>  				reg_base + ASS_CLK_GATE, 9, 0, &lock);
>  	}
>  
>  	for (i = 0; i < clk_data->num; i++) {
>  		if (IS_ERR(clk_table[i])) {
> -			dev_err(&pdev->dev, "failed to register clock %d\n", i);
> +			dev_err(dev, "failed to register clock %d\n", i);
>  			ret = PTR_ERR(clk_table[i]);
>  			goto unregister;
>  		}
>  	}
>  
> -	ret = of_clk_add_hw_provider(pdev->dev.of_node, of_clk_hw_onecell_get,
> +	ret = of_clk_add_hw_provider(dev->of_node, of_clk_hw_onecell_get,
>  				     clk_data);
>  	if (ret) {
> -		dev_err(&pdev->dev, "failed to add clock provider\n");
> +		dev_err(dev, "failed to add clock provider\n");
>  		goto unregister;

Don't you need to upadte the error path here? pm_runtime_disable()?

Best regards,
Krzysztof

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



[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux