From: Lad, Prabhakar <[email protected]>
This patch series cleanup's the VPSS clock enabling.
The first patch removes vpss clock enabling from the capture
drivers and moves it to the VPSS driver itself.
The second patch moves the venc_enable_vpss_clock() to the driver
which was being done in platform code.
Lad, Prabhakar (2):
media: davinci: vpss: enable vpss clocks
media: davinci: vpbe: venc: move the enabling of vpss clocks to
driver
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/vpbe_venc.c | 26 +++++++++++
drivers/media/platform/davinci/vpss.c | 60 ++++++++++++++++++++++++++
8 files changed, 98 insertions(+), 116 deletions(-)
--
1.7.4.1
From: Lad, Prabhakar <[email protected]>
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 protected]>
---
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(-)
diff --git a/arch/arm/mach-davinci/dm355.c b/arch/arm/mach-davinci/dm355.c
index b49c3b7..a917983 100644
--- a/arch/arm/mach-davinci/dm355.c
+++ b/arch/arm/mach-davinci/dm355.c
@@ -873,9 +873,6 @@ static int __init dm355_init_devices(void)
if (!cpu_is_davinci_dm355())
return 0;
- /* Add ccdc clock aliases */
- clk_add_alias("master", dm355_ccdc_dev.name, "vpss_master", NULL);
- clk_add_alias("slave", dm355_ccdc_dev.name, "vpss_master", NULL);
davinci_cfg_reg(DM355_INT_EDMA_CC);
platform_device_register(&dm355_edma_device);
platform_device_register(&dm355_vpss_device);
diff --git a/arch/arm/mach-davinci/dm365.c b/arch/arm/mach-davinci/dm365.c
index 6c39805..f4c19f7 100644
--- a/arch/arm/mach-davinci/dm365.c
+++ b/arch/arm/mach-davinci/dm365.c
@@ -257,6 +257,12 @@ static struct clk vpss_master_clk = {
.flags = CLK_PSC,
};
+static struct clk vpss_slave_clk = {
+ .name = "vpss_slave",
+ .parent = &pll1_sysclk5,
+ .lpsc = DAVINCI_LPSC_VPSSSLV,
+};
+
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),
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;
+
/*
* 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");
+ 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,
+};
+
static struct platform_driver vpss_driver = {
.driver = {
.name = "vpss",
.owner = THIS_MODULE,
+ .pm = &vpss_pm_ops,
},
.remove = vpss_remove,
.probe = vpss_probe,
--
1.7.4.1
From: Lad, Prabhakar <[email protected]>
The vpss clocks were enabled by calling a exported function from a driver
in a machine code. calling driver code from platform code is incorrect way.
This patch fixes this issue and calls the function from driver code itself.
Signed-off-by: Lad, Prabhakar <[email protected]>
---
Note: This patch is based on the comment from Sekhar
(https://patchwork-mail1.kernel.org/patch/2278441/).
Shekar I haven't completely removed the callback, I just added
the function calls after the callback. As you mentioned just to
pass the VPSS_CLK_CTRL as a resource to venc but the VPSS_CLK_CTRL
is already being used by VPSS driver. I'll take this cleanup task later
point of time.
drivers/media/platform/davinci/vpbe_venc.c | 26 ++++++++++++++++++++++++++
1 files changed, 26 insertions(+), 0 deletions(-)
diff --git a/drivers/media/platform/davinci/vpbe_venc.c b/drivers/media/platform/davinci/vpbe_venc.c
index f15f211..26bdf9b 100644
--- a/drivers/media/platform/davinci/vpbe_venc.c
+++ b/drivers/media/platform/davinci/vpbe_venc.c
@@ -202,6 +202,26 @@ static void venc_enabledigitaloutput(struct v4l2_subdev *sd, int benable)
}
}
+static void
+venc_enable_vpss_clock(int venc_type,
+ enum vpbe_enc_timings_type type,
+ unsigned int pclock)
+{
+ if (venc_type == VPBE_VERSION_1)
+ return;
+
+ if (venc_type == VPBE_VERSION_2 && (type == VPBE_ENC_STD ||
+ (type == VPBE_ENC_DV_TIMINGS && pclock <= 27000000))) {
+ vpss_enable_clock(VPSS_VENC_CLOCK_SEL, 1);
+ vpss_enable_clock(VPSS_VPBE_CLOCK, 1);
+ return;
+ }
+
+ if (venc_type == VPBE_VERSION_3 && type == VPBE_ENC_STD)
+ vpss_enable_clock(VPSS_VENC_CLOCK_SEL, 0);
+
+}
+
#define VDAC_CONFIG_SD_V3 0x0E21A6B6
#define VDAC_CONFIG_SD_V2 0x081141CF
/*
@@ -220,6 +240,7 @@ static int venc_set_ntsc(struct v4l2_subdev *sd)
if (pdata->setup_clock(VPBE_ENC_STD, V4L2_STD_525_60) < 0)
return -EINVAL;
+ venc_enable_vpss_clock(venc->venc_type, VPBE_ENC_STD, V4L2_STD_525_60);
venc_enabledigitaloutput(sd, 0);
if (venc->venc_type == VPBE_VERSION_3) {
@@ -265,6 +286,7 @@ static int venc_set_pal(struct v4l2_subdev *sd)
if (venc->pdata->setup_clock(VPBE_ENC_STD, V4L2_STD_625_50) < 0)
return -EINVAL;
+ venc_enable_vpss_clock(venc->venc_type, VPBE_ENC_STD, V4L2_STD_625_50);
venc_enabledigitaloutput(sd, 0);
if (venc->venc_type == VPBE_VERSION_3) {
@@ -319,6 +341,7 @@ static int venc_set_480p59_94(struct v4l2_subdev *sd)
if (pdata->setup_clock(VPBE_ENC_DV_TIMINGS, 27000000) < 0)
return -EINVAL;
+ venc_enable_vpss_clock(venc->venc_type, VPBE_ENC_DV_TIMINGS, 27000000);
venc_enabledigitaloutput(sd, 0);
if (venc->venc_type == VPBE_VERSION_2)
@@ -366,6 +389,7 @@ static int venc_set_576p50(struct v4l2_subdev *sd)
if (pdata->setup_clock(VPBE_ENC_DV_TIMINGS, 27000000) < 0)
return -EINVAL;
+ venc_enable_vpss_clock(venc->venc_type, VPBE_ENC_DV_TIMINGS, 27000000);
venc_enabledigitaloutput(sd, 0);
if (venc->venc_type == VPBE_VERSION_2)
@@ -406,6 +430,7 @@ static int venc_set_720p60_internal(struct v4l2_subdev *sd)
if (pdata->setup_clock(VPBE_ENC_DV_TIMINGS, 74250000) < 0)
return -EINVAL;
+ venc_enable_vpss_clock(venc->venc_type, VPBE_ENC_DV_TIMINGS, 74250000);
venc_enabledigitaloutput(sd, 0);
venc_write(sd, VENC_OSDCLK0, 0);
@@ -434,6 +459,7 @@ static int venc_set_1080i30_internal(struct v4l2_subdev *sd)
if (pdata->setup_clock(VPBE_ENC_DV_TIMINGS, 74250000) < 0)
return -EINVAL;
+ venc_enable_vpss_clock(venc->venc_type, VPBE_ENC_DV_TIMINGS, 74250000);
venc_enabledigitaloutput(sd, 0);
venc_write(sd, VENC_OSDCLK0, 0);
--
1.7.4.1
On 3/22/2013 1:23 PM, Prabhakar lad wrote:
> From: Lad, Prabhakar <[email protected]>
>
> 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 protected]>
> ---
> 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
On 3/22/2013 1:23 PM, Prabhakar lad wrote:
> From: Lad, Prabhakar <[email protected]>
>
> The vpss clocks were enabled by calling a exported function from a driver
> in a machine code. calling driver code from platform code is incorrect way.
>
> This patch fixes this issue and calls the function from driver code itself.
>
> Signed-off-by: Lad, Prabhakar <[email protected]>
> ---
> Note: This patch is based on the comment from Sekhar
> (https://patchwork-mail1.kernel.org/patch/2278441/).
> Shekar I haven't completely removed the callback, I just added
> the function calls after the callback. As you mentioned just to
> pass the VPSS_CLK_CTRL as a resource to venc but the VPSS_CLK_CTRL
> is already being used by VPSS driver. I'll take this cleanup task later
> point of time.
Fine by me.
Thanks,
Sekhar
Hi Sekhar,
Thanks for the review!
On Mon, Mar 25, 2013 at 11:02 AM, Sekhar Nori <[email protected]> wrote:
> On 3/22/2013 1:23 PM, Prabhakar lad wrote:
>> From: Lad, Prabhakar <[email protected]>
>>
>> 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 protected]>
>> ---
>> 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),
>
OK
>> 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.
>
Its just a movement, while fixing the cleanups. I'll add some
description about it.
>> /*
>> * 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.
>
OK
>> + 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.
>
I felt having in same patch would be a good idea, since the clock
enabling/disabling
where removed from dm644x_ccdc.c for suspend/resume and add it in
vpss. If you still
feel it needs to be separate patch let me know.
> 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?
>
I want this series to go through the media tree because of few dependencies
(dependency on ths7353 video amplifier driver which recently got
merged into media tree)
Regards,
--Prabhakar
> Thanks,
> Sekhar
Hi Sekhar,
On Mon, Mar 25, 2013 at 11:09 AM, Sekhar Nori <[email protected]> wrote:
> On 3/22/2013 1:23 PM, Prabhakar lad wrote:
>> From: Lad, Prabhakar <[email protected]>
>>
>> The vpss clocks were enabled by calling a exported function from a driver
>> in a machine code. calling driver code from platform code is incorrect way.
>>
>> This patch fixes this issue and calls the function from driver code itself.
>>
>> Signed-off-by: Lad, Prabhakar <[email protected]>
>> ---
>> Note: This patch is based on the comment from Sekhar
>> (https://patchwork-mail1.kernel.org/patch/2278441/).
>> Shekar I haven't completely removed the callback, I just added
>> the function calls after the callback. As you mentioned just to
>> pass the VPSS_CLK_CTRL as a resource to venc but the VPSS_CLK_CTRL
>> is already being used by VPSS driver. I'll take this cleanup task later
>> point of time.
>
> Fine by me.
>
Can I have your ACK on this patch ?
Regards,
--Prabhakar
> Thanks,
> Sekhar
On 3/25/2013 3:44 PM, Prabhakar Lad wrote:
> Hi Sekhar,
>
> Thanks for the review!
>
> On Mon, Mar 25, 2013 at 11:02 AM, Sekhar Nori <[email protected]> wrote:
>> On 3/22/2013 1:23 PM, Prabhakar lad wrote:
>>> From: Lad, Prabhakar <[email protected]>
>>>
>>> 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 protected]>
>>> ---
>>> 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),
>>
> OK
>
>>> 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.
>>
> Its just a movement, while fixing the cleanups. I'll add some
> description about it.
This is fine as is. I failed to notice the movement.
>
>>> /*
>>> * 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.
>>
> OK
>
>>> + 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.
>>
> I felt having in same patch would be a good idea, since the clock
> enabling/disabling
> where removed from dm644x_ccdc.c for suspend/resume and add it in
> vpss. If you still
> feel it needs to be separate patch let me know.
Okay, please add some description for this. Its not terribly obvious.
>
>> 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?
>>
> I want this series to go through the media tree because of few dependencies
> (dependency on ths7353 video amplifier driver which recently got
> merged into media tree)
Hmm, this patch does not touch ths7353 so are you sure there is a merge
dependency? There are a lot of platform changes you are doing in some of
the recent patches you submitted based on this patch and ideally those
go through my tree to ease merging for upstream. That said, if there is
a good reason for this and the other platform changes to go through
media tree, I am okay with that too.
Thanks,
Sekhar
Sekhar,
On Mon, Mar 25, 2013 at 3:54 PM, Sekhar Nori <[email protected]> wrote:
> On 3/25/2013 3:44 PM, Prabhakar Lad wrote:
>> Hi Sekhar,
>>
>> Thanks for the review!
>>
>> On Mon, Mar 25, 2013 at 11:02 AM, Sekhar Nori <[email protected]> wrote:
>>> On 3/22/2013 1:23 PM, Prabhakar lad wrote:
>>>> From: Lad, Prabhakar <[email protected]>
>>>>
>>>> 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 protected]>
>>>> ---
>>>> 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),
>>>
>> OK
>>
>>>> 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.
>>>
>> Its just a movement, while fixing the cleanups. I'll add some
>> description about it.
>
> This is fine as is. I failed to notice the movement.
>
>>
>>>> /*
>>>> * 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.
>>>
>> OK
>>
>>>> + 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.
>>>
>> I felt having in same patch would be a good idea, since the clock
>> enabling/disabling
>> where removed from dm644x_ccdc.c for suspend/resume and add it in
>> vpss. If you still
>> feel it needs to be separate patch let me know.
>
> Okay, please add some description for this. Its not terribly obvious.
>
>>
>>> 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?
>>>
>> I want this series to go through the media tree because of few dependencies
>> (dependency on ths7353 video amplifier driver which recently got
>> merged into media tree)
>
> Hmm, this patch does not touch ths7353 so are you sure there is a merge
> dependency? There are a lot of platform changes you are doing in some of
> the recent patches you submitted based on this patch and ideally those
> go through my tree to ease merging for upstream. That said, if there is
> a good reason for this and the other platform changes to go through
> media tree, I am okay with that too.
>
Yes this patch doesnt touch ths7353, but the machine patches for DM365 which
I posted earlier have a dependency on it. And also I have planned to
take the machine
patches for DM365 and DM355 through media tree itself. [1] this is one
is also of dependent
patch which is already merged into media tree. So to avoid
build/dependency I fell this series and
the machine patches for DM365/DM355 should go through media tree.
[1] http://git.linuxtv.org/media_tree.git/commitdiff/ef2d41b19b8100ce63eabba9ee87953aa685921a
Regards,
--Prabhakar
> Thanks,
> Sekhar
On 3/25/2013 3:45 PM, Prabhakar Lad wrote:
> Hi Sekhar,
>
> On Mon, Mar 25, 2013 at 11:09 AM, Sekhar Nori <[email protected]> wrote:
>> On 3/22/2013 1:23 PM, Prabhakar lad wrote:
>>> From: Lad, Prabhakar <[email protected]>
>>>
>>> The vpss clocks were enabled by calling a exported function from a driver
>>> in a machine code. calling driver code from platform code is incorrect way.
>>>
>>> This patch fixes this issue and calls the function from driver code itself.
>>>
>>> Signed-off-by: Lad, Prabhakar <[email protected]>
>>> ---
>>> Note: This patch is based on the comment from Sekhar
>>> (https://patchwork-mail1.kernel.org/patch/2278441/).
>>> Shekar I haven't completely removed the callback, I just added
>>> the function calls after the callback. As you mentioned just to
>>> pass the VPSS_CLK_CTRL as a resource to venc but the VPSS_CLK_CTRL
>>> is already being used by VPSS driver. I'll take this cleanup task later
>>> point of time.
>>
>> Fine by me.
>>
> Can I have your ACK on this patch ?
The 'fine' from me was for the approach, not not patch ;). Seriously
though, since this patch is only touching media/ I haven't really done a
detailed enough review of it. In any case, it should be OK to merge this
without my ack.
Thanks,
Sekhar