| 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
| ||
|
Message-ID: <CA+V-a8tZGh8y_eC52XC-7J41inARjrq9KD2bHCfZ+j1npo+VYw@mail.gmail.com>
Date: Mon, 25 Mar 2013 15:44:24 +0530
From: Prabhakar Lad <prabhakar.csengg@...il.com>
To: Sekhar Nori <nsekhar@...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
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.
>> /*
>> * 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
--
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