| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 25 Mar 2013 15:54:35 +0530
From: Sekhar Nori <nsekhar@...com>
To: Prabhakar Lad <prabhakar.csengg@...il.com>
CC: LMML <linux-media@...r.kernel.org>,
DLOS <davinci-linux-open-source@...ux.davincidsp.com>,
LAK <linux-arm-kernel@...ts.infradead.org>,
LKML <linux-kernel@...r.kernel.org>,
Mauro Carvalho Chehab <mchehab@...hat.com>
Subject: Re: [PATCH 1/2] media: davinci: vpss: enable vpss clocks
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 <nsekhar@...com> wrote:
>> On 3/22/2013 1:23 PM, Prabhakar lad wrote:
>>> From: Lad, Prabhakar <prabhakar.csengg@...il.com>
>>>
>>> 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 <prabhakar.csengg@...il.com>
>>> ---
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists