Subject: [PATCH 1/2] media: davinci: vpss: enable vpss clocks

From: [email-masked] (Sekhar Nori)
Date: Mon, 25 Mar 2013 11:02:02 +0530

On 3/22/2013 1:23 PM, Prabhakar lad wrote:
> From: Lad, Prabhakar <[email-masked]>
> 
> By default the VPSS clocks are only enabled in capture driver
> for davinci family which creates duplicates. This
> patch adds support to enable the VPSS clocks in VPSS driver.
> This avoids duplication of code and also adding clock aliases.
> This patch cleanups the VPSS clock enabling in the capture driver,
> and also removes the clock alias in machine file. Along side adds
> a vpss slave clock for DM365 as mentioned by Sekhar
> (https://patchwork.kernel.org/patch/1221261/).
> 
> Signed-off-by: Lad, Prabhakar <[email-masked]>
> ---
>  arch/arm/mach-davinci/dm355.c                |    3 -
>  arch/arm/mach-davinci/dm365.c                |    9 +++-
>  arch/arm/mach-davinci/dm644x.c               |    5 --
>  drivers/media/platform/davinci/dm355_ccdc.c  |   39 +----------------
>  drivers/media/platform/davinci/dm644x_ccdc.c |   44 -------------------
>  drivers/media/platform/davinci/isif.c        |   28 ++----------
>  drivers/media/platform/davinci/vpss.c        |   60 ++++++++++++++++++++++++++
>  7 files changed, 72 insertions(+), 116 deletions(-)
> 
>  static struct clk arm_clk = {
>  	.name		= "arm_clk",
>  	.parent		= &pll2_sysclk2,
> @@ -450,6 +456,7 @@ static struct clk_lookup dm365_clks[] = {
>  	CLK(NULL, "pll2_sysclk9", &pll2_sysclk9),
>  	CLK(NULL, "vpss_dac", &vpss_dac_clk),
>  	CLK(NULL, "vpss_master", &vpss_master_clk),
> +	CLK(NULL, "vpss_slave", &vpss_slave_clk),

These should use device name for look-up instead of relying just on
con_id. So the entry should look like:

CLK("vpss", "slave", &vpss_slave_clk),

>  	CLK(NULL, "arm", &arm_clk),
>  	CLK(NULL, "uart0", &uart0_clk),
>  	CLK(NULL, "uart1", &uart1_clk),
> @@ -1239,8 +1246,6 @@ static int __init dm365_init_devices(void)
>  	clk_add_alias(NULL, dev_name(&dm365_mdio_device.dev),
>  		      NULL, &dm365_emac_device.dev);
>  
> -	/* Add isif clock alias */
> -	clk_add_alias("master", dm365_isif_dev.name, "vpss_master", NULL);
>  	platform_device_register(&dm365_vpss_device);
>  	platform_device_register(&dm365_isif_dev);
>  	platform_device_register(&vpfe_capture_dev);
> diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach-davinci/dm644x.c
> index ee0e994..026e7e3 100644
> --- a/arch/arm/mach-davinci/dm644x.c
> +++ b/arch/arm/mach-davinci/dm644x.c
> @@ -901,11 +901,6 @@ int __init dm644x_init_video(struct vpfe_config *vpfe_cfg,
>  		dm644x_vpfe_dev.dev.platform_data = vpfe_cfg;
>  		platform_device_register(&dm644x_ccdc_dev);
>  		platform_device_register(&dm644x_vpfe_dev);
> -		/* Add ccdc clock aliases */
> -		clk_add_alias("master", dm644x_ccdc_dev.name,
> -			      "vpss_master", NULL);
> -		clk_add_alias("slave", dm644x_ccdc_dev.name,
> -			      "vpss_slave", NULL);
>  	}
>  
>  	if (vpbe_cfg) {
> diff --git a/drivers/media/platform/davinci/dm355_ccdc.c b/drivers/media/platform/davinci/dm355_ccdc.c
> index 2364dba..05f8fb7 100644
> --- a/drivers/media/platform/davinci/dm355_ccdc.c
> +++ b/drivers/media/platform/davinci/dm355_ccdc.c
> @@ -37,7 +37,6 @@
>  #include <linux/platform_device.h>
>  #include <linux/uaccess.h>
>  #include <linux/videodev2.h>
> -#include <linux/clk.h>
>  #include <linux/err.h>
>  #include <linux/module.h>
>  
> @@ -59,10 +58,6 @@ static struct ccdc_oper_config {
>  	struct ccdc_params_raw bayer;
>  	/* YCbCr configuration */
>  	struct ccdc_params_ycbcr ycbcr;
> -	/* Master clock */
> -	struct clk *mclk;
> -	/* slave clock */
> -	struct clk *sclk;
>  	/* ccdc base address */
>  	void __iomem *base_addr;
>  } ccdc_cfg = {
> @@ -997,32 +992,10 @@ static int dm355_ccdc_probe(struct platform_device *pdev)
>  		goto fail_nomem;
>  	}
>  
> -	/* Get and enable Master clock */
> -	ccdc_cfg.mclk = clk_get(&pdev->dev, "master");
> -	if (IS_ERR(ccdc_cfg.mclk)) {
> -		status = PTR_ERR(ccdc_cfg.mclk);
> -		goto fail_nomap;
> -	}
> -	if (clk_prepare_enable(ccdc_cfg.mclk)) {
> -		status = -ENODEV;
> -		goto fail_mclk;
> -	}
> -
> -	/* Get and enable Slave clock */
> -	ccdc_cfg.sclk = clk_get(&pdev->dev, "slave");
> -	if (IS_ERR(ccdc_cfg.sclk)) {
> -		status = PTR_ERR(ccdc_cfg.sclk);
> -		goto fail_mclk;
> -	}
> -	if (clk_prepare_enable(ccdc_cfg.sclk)) {
> -		status = -ENODEV;
> -		goto fail_sclk;
> -	}
> -
>  	/* Platform data holds setup_pinmux function ptr */
>  	if (NULL == pdev->dev.platform_data) {
>  		status = -ENODEV;
> -		goto fail_sclk;
> +		goto fail_nomap;
>  	}
>  	setup_pinmux = pdev->dev.platform_data;
>  	/*
> @@ -1033,12 +1006,6 @@ static int dm355_ccdc_probe(struct platform_device *pdev)
>  	ccdc_cfg.dev = &pdev->dev;
>  	printk(KERN_NOTICE "%s is registered with vpfe.\n", ccdc_hw_dev.name);
>  	return 0;
> -fail_sclk:
> -	clk_disable_unprepare(ccdc_cfg.sclk);
> -	clk_put(ccdc_cfg.sclk);
> -fail_mclk:
> -	clk_disable_unprepare(ccdc_cfg.mclk);
> -	clk_put(ccdc_cfg.mclk);
>  fail_nomap:
>  	iounmap(ccdc_cfg.base_addr);
>  fail_nomem:
> @@ -1052,10 +1019,6 @@ static int dm355_ccdc_remove(struct platform_device *pdev)
>  {
>  	struct resource	*res;
>  
> -	clk_disable_unprepare(ccdc_cfg.sclk);
> -	clk_disable_unprepare(ccdc_cfg.mclk);
> -	clk_put(ccdc_cfg.mclk);
> -	clk_put(ccdc_cfg.sclk);
>  	iounmap(ccdc_cfg.base_addr);
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (res)
> diff --git a/drivers/media/platform/davinci/dm644x_ccdc.c b/drivers/media/platform/davinci/dm644x_ccdc.c
> index 971d639..30fa084 100644
> --- a/drivers/media/platform/davinci/dm644x_ccdc.c
> +++ b/drivers/media/platform/davinci/dm644x_ccdc.c
> @@ -38,7 +38,6 @@
>  #include <linux/uaccess.h>
>  #include <linux/videodev2.h>
>  #include <linux/gfp.h>
> -#include <linux/clk.h>
>  #include <linux/err.h>
>  #include <linux/module.h>
>  
> @@ -60,10 +59,6 @@ static struct ccdc_oper_config {
>  	struct ccdc_params_raw bayer;
>  	/* YCbCr configuration */
>  	struct ccdc_params_ycbcr ycbcr;
> -	/* Master clock */
> -	struct clk *mclk;
> -	/* slave clock */
> -	struct clk *sclk;
>  	/* ccdc base address */
>  	void __iomem *base_addr;
>  } ccdc_cfg = {
> @@ -991,38 +986,9 @@ static int dm644x_ccdc_probe(struct platform_device *pdev)
>  		goto fail_nomem;
>  	}
>  
> -	/* Get and enable Master clock */
> -	ccdc_cfg.mclk = clk_get(&pdev->dev, "master");
> -	if (IS_ERR(ccdc_cfg.mclk)) {
> -		status = PTR_ERR(ccdc_cfg.mclk);
> -		goto fail_nomap;
> -	}
> -	if (clk_prepare_enable(ccdc_cfg.mclk)) {
> -		status = -ENODEV;
> -		goto fail_mclk;
> -	}
> -
> -	/* Get and enable Slave clock */
> -	ccdc_cfg.sclk = clk_get(&pdev->dev, "slave");
> -	if (IS_ERR(ccdc_cfg.sclk)) {
> -		status = PTR_ERR(ccdc_cfg.sclk);
> -		goto fail_mclk;
> -	}
> -	if (clk_prepare_enable(ccdc_cfg.sclk)) {
> -		status = -ENODEV;
> -		goto fail_sclk;
> -	}
>  	ccdc_cfg.dev = &pdev->dev;
>  	printk(KERN_NOTICE "%s is registered with vpfe.\n", ccdc_hw_dev.name);
>  	return 0;
> -fail_sclk:
> -	clk_disable_unprepare(ccdc_cfg.sclk);
> -	clk_put(ccdc_cfg.sclk);
> -fail_mclk:
> -	clk_disable_unprepare(ccdc_cfg.mclk);
> -	clk_put(ccdc_cfg.mclk);
> -fail_nomap:
> -	iounmap(ccdc_cfg.base_addr);
>  fail_nomem:
>  	release_mem_region(res->start, resource_size(res));
>  fail_nores:
> @@ -1034,10 +1000,6 @@ static int dm644x_ccdc_remove(struct platform_device *pdev)
>  {
>  	struct resource	*res;
>  
> -	clk_disable_unprepare(ccdc_cfg.mclk);
> -	clk_disable_unprepare(ccdc_cfg.sclk);
> -	clk_put(ccdc_cfg.mclk);
> -	clk_put(ccdc_cfg.sclk);
>  	iounmap(ccdc_cfg.base_addr);
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (res)
> @@ -1052,18 +1014,12 @@ static int dm644x_ccdc_suspend(struct device *dev)
>  	ccdc_save_context();
>  	/* Disable CCDC */
>  	ccdc_enable(0);
> -	/* Disable both master and slave clock */
> -	clk_disable_unprepare(ccdc_cfg.mclk);
> -	clk_disable_unprepare(ccdc_cfg.sclk);
>  
>  	return 0;
>  }
>  
>  static int dm644x_ccdc_resume(struct device *dev)
>  {
> -	/* Enable both master and slave clock */
> -	clk_prepare_enable(ccdc_cfg.mclk);
> -	clk_prepare_enable(ccdc_cfg.sclk);
>  	/* Restore CCDC context */
>  	ccdc_restore_context();
>  
> diff --git a/drivers/media/platform/davinci/isif.c b/drivers/media/platform/davinci/isif.c
> index abc3ae3..3332cca 100644
> --- a/drivers/media/platform/davinci/isif.c
> +++ b/drivers/media/platform/davinci/isif.c
> @@ -32,7 +32,6 @@
>  #include <linux/uaccess.h>
>  #include <linux/io.h>
>  #include <linux/videodev2.h>
> -#include <linux/clk.h>
>  #include <linux/err.h>
>  #include <linux/module.h>
>  
> @@ -88,8 +87,6 @@ static struct isif_oper_config {
>  	struct isif_ycbcr_config ycbcr;
>  	struct isif_params_raw bayer;
>  	enum isif_data_pack data_pack;
> -	/* Master clock */
> -	struct clk *mclk;
>  	/* ISIF base address */
>  	void __iomem *base_addr;
>  	/* ISIF Linear Table 0 */
> @@ -1039,6 +1036,10 @@ static int isif_probe(struct platform_device *pdev)
>  	void *__iomem addr;
>  	int status = 0, i;
>  
> +	/* Platform data holds setup_pinmux function ptr */
> +	if (!pdev->dev.platform_data)
> +		return -ENODEV;
> +

This change seems unrelated. I suggest moving it to a different patch or
atleast note it in the description.

>  	/*
>  	 * first try to register with vpfe. If not correct platform, then we
>  	 * don't have to iomap
> @@ -1047,22 +1048,6 @@ static int isif_probe(struct platform_device *pdev)
>  	if (status < 0)
>  		return status;
>  
> -	/* Get and enable Master clock */
> -	isif_cfg.mclk = clk_get(&pdev->dev, "master");
> -	if (IS_ERR(isif_cfg.mclk)) {
> -		status = PTR_ERR(isif_cfg.mclk);
> -		goto fail_mclk;
> -	}
> -	if (clk_prepare_enable(isif_cfg.mclk)) {
> -		status = -ENODEV;
> -		goto fail_mclk;
> -	}
> -
> -	/* Platform data holds setup_pinmux function ptr */
> -	if (NULL == pdev->dev.platform_data) {
> -		status = -ENODEV;
> -		goto fail_mclk;
> -	}
>  	setup_pinmux = pdev->dev.platform_data;
>  	/*
>  	 * setup Mux configuration for ccdc which may be different for
> @@ -1124,9 +1109,6 @@ fail_nobase_res:
>  		release_mem_region(res->start, resource_size(res));
>  		i--;
>  	}
> -fail_mclk:
> -	clk_disable_unprepare(isif_cfg.mclk);
> -	clk_put(isif_cfg.mclk);
>  	vpfe_unregister_ccdc_device(&isif_hw_dev);
>  	return status;
>  }
> @@ -1146,8 +1128,6 @@ static int isif_remove(struct platform_device *pdev)
>  		i++;
>  	}
>  	vpfe_unregister_ccdc_device(&isif_hw_dev);
> -	clk_disable_unprepare(isif_cfg.mclk);
> -	clk_put(isif_cfg.mclk);
>  	return 0;
>  }
>  
> diff --git a/drivers/media/platform/davinci/vpss.c b/drivers/media/platform/davinci/vpss.c
> index a19c552..db69317 100644
> --- a/drivers/media/platform/davinci/vpss.c
> +++ b/drivers/media/platform/davinci/vpss.c
> @@ -17,6 +17,7 @@
>   *
>   * common vpss system module platform driver for all video drivers.
>   */
> +#include <linux/clk.h>
>  #include <linux/kernel.h>
>  #include <linux/sched.h>
>  #include <linux/init.h>
> @@ -126,6 +127,10 @@ struct vpss_oper_config {
>  	enum vpss_platform_type platform;
>  	spinlock_t vpss_lock;
>  	struct vpss_hw_ops hw_ops;
> +	/* Master clock */
> +	struct clk *mclk;
> +	/* slave clock */
> +	struct clk *sclk;
>  };
>  
>  static struct vpss_oper_config oper_cfg;
> @@ -429,6 +434,26 @@ static int vpss_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	/* Get and enable Master clock */
> +	oper_cfg.mclk = clk_get(&pdev->dev, "vpss_master");

use devm_clk_get() here to simplify the error handling.

> +	if (IS_ERR(oper_cfg.mclk)) {
> +		status = PTR_ERR(oper_cfg.mclk);
> +		goto fail_getclk;
> +	}
> +	status = clk_prepare_enable(oper_cfg.mclk);
> +	if (status)
> +		goto fail_mclk;
> +
> +	/* Get and enable Slave clock */
> +	oper_cfg.sclk = clk_get(&pdev->dev, "vpss_slave");
> +	if (IS_ERR(oper_cfg.sclk)) {
> +		status = PTR_ERR(oper_cfg.sclk);
> +		goto fail_mclk;
> +	}
> +	status = clk_prepare_enable(oper_cfg.sclk);
> +	if (status)
> +		goto fail_sclk;
> +
>  	dev_info(&pdev->dev, "%s vpss probed\n", platform_name);
>  	r1 = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!r1)
> @@ -500,6 +525,13 @@ fail2:
>  	iounmap(oper_cfg.vpss_regs_base0);
>  fail1:
>  	release_mem_region(r1->start, resource_size(r1));
> +fail_sclk:
> +	clk_disable_unprepare(oper_cfg.sclk);
> +	clk_put(oper_cfg.sclk);
> +fail_mclk:
> +	clk_disable_unprepare(oper_cfg.mclk);
> +	clk_put(oper_cfg.mclk);
> +fail_getclk:
>  	return status;
>  }
>  
> @@ -510,6 +542,10 @@ static int vpss_remove(struct platform_device *pdev)
>  	iounmap(oper_cfg.vpss_regs_base0);
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	release_mem_region(res->start, resource_size(res));
> +	clk_disable_unprepare(oper_cfg.mclk);
> +	clk_disable_unprepare(oper_cfg.sclk);
> +	clk_put(oper_cfg.mclk);
> +	clk_put(oper_cfg.sclk);
>  	if (oper_cfg.platform == DM355 || oper_cfg.platform == DM365) {
>  		iounmap(oper_cfg.vpss_regs_base1);
>  		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> @@ -518,10 +554,34 @@ static int vpss_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  

> +static int vpss_suspend(struct device *dev)
> +{
> +	/* Disable both master and slave clock */
> +	clk_disable_unprepare(oper_cfg.mclk);
> +	clk_disable_unprepare(oper_cfg.sclk);
> +
> +	return 0;
> +}
> +
> +static int vpss_resume(struct device *dev)
> +{
> +	/* Enable both master and slave clock */
> +	clk_prepare_enable(oper_cfg.mclk);
> +	clk_prepare_enable(oper_cfg.sclk);
> +
> +	return 0;
> +}
> +
> +static const struct dev_pm_ops vpss_pm_ops = {
> +	.suspend = vpss_suspend,
> +	.resume = vpss_resume,
> +};

Addition of suspend support seems unrelated to this patch. May be make a
seperate patch for it and while at it, please use PM runtime instead of
direct clock enable/disable. Have a look at the davinci_emac driver
which was converted to use PM runtime recently.

Let me know how you want to handle this patch. I suppose you intend this
should go through my tree because of other dependent platform changes?

Thanks,
Sekhar