Re: [PATCH] davinci: vpss: enable vpss clocks

[Sekhar Nori <nsekhar@ti.com>]

Hi Prabhakar,

On 7/20/2012 7:41 PM, Prabhakar Lad wrote:
> From: Lad, Prabhakar <prabhakar.lad@ti.com>
> 
> By default the VPSS clocks are only enabled in capture driver.
> and display wont work if the capture is not enabled. This
> patch adds support to enable the VPSS clocks in VPSS driver.
> This way we can enable/disable capture and display and use it
> independently.

With this description, I would expect to see some lines related to clock
enable being removed from capture driver, but I don't see that. Are you
sure there is no duplicate enable of of clocks happening after this patch?

> 
> Signed-off-by: Lad, Prabhakar <prabhakar.lad@ti.com>
> Signed-off-by: Manjunath Hadli <manjunath.hadli@ti.com>
> ---
>  drivers/media/video/davinci/vpss.c |   38 ++++++++++++++++++++++++++++++++++++
>  1 files changed, 38 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/davinci/vpss.c b/drivers/media/video/davinci/vpss.c
> index 3e5cf27..30283bb 100644
> --- a/drivers/media/video/davinci/vpss.c
> +++ b/drivers/media/video/davinci/vpss.c
> @@ -25,6 +25,9 @@
>  #include <linux/spinlock.h>
>  #include <linux/compiler.h>
>  #include <linux/io.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +
>  #include <mach/hardware.h>
>  #include <media/davinci/vpss.h>
>  
> @@ -104,6 +107,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;
> @@ -381,6 +388,29 @@ static int __init vpss_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> +	/* Get and enable Master clock */
> +	oper_cfg.mclk = clk_get(&pdev->dev, "master");
> +	if (IS_ERR(oper_cfg.mclk)) {
> +		status = PTR_ERR(oper_cfg.mclk);
> +		goto fail_getclk;
> +	}
> +	if (clk_enable(oper_cfg.mclk)) {
> +		status = -ENODEV;
> +		goto fail_mclk;
> +	}
> +	if (oper_cfg.platform == DM355 || oper_cfg.platform == DM644X) {
> +		/* Get and enable Slave clock */
> +		oper_cfg.sclk = clk_get(&pdev->dev, "slave");

Clock API is already a platform abstraction. So, to check for device
type here is incorrect. This way, you are not actually using the
abstraction.

Why are you enabling slave clock only for DM355 and DM644X? I suspect
since the IP is reused between these devices, the IP still has a slave
clock on all other platforms - only it may be a constant clock on those
platforms. If that's the case, the driver should still request for both
master and slave clocks on all platforms and have the platform define
the slave clock as a constant clock where required.

> +		if (IS_ERR(oper_cfg.sclk)) {
> +			status = PTR_ERR(oper_cfg.sclk);
> +			goto fail_mclk;
> +		}
> +		if (clk_enable(oper_cfg.sclk)) {
> +			status = -ENODEV;

Why override the status that clk_enable() is giving you?

Thanks,
Sekhar