RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
Kevin, That is what I had seen. I will check it again on Monday and let you know. Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 phone: 301-407-9583 email: m-kariche...@ti.com >-Original Message- >From: Kevin Hilman [mailto:khil...@deeprootsystems.com] >Sent: Friday, December 11, 2009 1:35 PM >To: Karicheri, Muralidharan >Cc: davinci-linux-open-sou...@linux.davincidsp.com; linux- >me...@vger.kernel.org >Subject: Re: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable > >"Karicheri, Muralidharan" writes: > >>>> Kevin, >>>> >>>> I think I have figured it out... >>>> >>>> First issue was that I was adding my entry at the end of dm644x_clks[] >>>> array. I need to add it before the CLK(NULL, NULL, NULL) >>>> >>>> secondly, your suggestion didn't work as is. This is what I had to >>>> do to get it working... >>>> >>>> static struct clk ccdc_master_clk = { >>>>.name = "dm644x_ccdc", >>>>.parent = &vpss_master_clk, >>>> }; >>>> >>>> static struct clk ccdc_slave_clk = { >>>>.name = "dm644x_ccdc", >>>>.parent = &vpss_slave_clk, >>>> }; >> >> It doesn't work with out doing this. The cat /proc/davinci_clocks hangs >with >> your suggestion implemented... > >Can you track down the hang. It sounds like a bug in the walking of >the clock tree for davinci_clocks. > >>> >>>You should not need to add new clocks with new names. I don't thinke >>>the name field of the struct clk is used anywhere in the matching. >>>I think it's only used in /proc/davinci_clocks >>> >>>> static struct davinci_clk dm365_clks = { >>>> >>>> >>>> CLK("dm644x_ccdc", "master", &ccdc_master_clk), >>>> CLK("dm644x_ccdc", "slave", &ccdc_slave_clk), >>> >>>Looks like the drivers name is 'dm644x_ccdc', not 'isif'. I'm >>>guessing just this should work without having to add new clock names. >>> >> No. I have mixed up the names. ISIF is for the new ISIF driver on DM365. >> Below are for DM644x ccdc driver. With just these entries added, two >> things observed >> >> 1) Only one clock is shown disabled (usually many are shown disabled) >during bootup >> 2) cat /proc/davinci_clocks hangs. >> >> So this is the only way I got it working. > >Hmm, it worked just fine for me without any of these side effects. I >applied the simple patch below on top of current master branch. It booted >fine showing all the unused clocks being disabled, and I was able to >see davinci_clocks just fine: > > >diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach- >davinci/dm644x.c >index e65e29e..e6f3570 100644 >--- a/arch/arm/mach-davinci/dm644x.c >+++ b/arch/arm/mach-davinci/dm644x.c >@@ -293,8 +293,8 @@ struct davinci_clk dm644x_clks[] = { >CLK(NULL, "dsp", &dsp_clk), >CLK(NULL, "arm", &arm_clk), >CLK(NULL, "vicp", &vicp_clk), >- CLK(NULL, "vpss_master", &vpss_master_clk), >- CLK(NULL, "vpss_slave", &vpss_slave_clk), >+ CLK("dm644x_ccdc", "master", &vpss_master_clk), >+ CLK("dm644x_ccdc", "slave", &vpss_slave_clk), >CLK(NULL, "arm", &arm_clk), >CLK(NULL, "uart0", &uart0_clk), >CLK(NULL, "uart1", &uart1_clk), > > >[...] >Clocks: disable unused uart1 >Clocks: disable unused uart2 >Clocks: disable unused emac >Clocks: disable unused ide >Clocks: disable unused asp0 >Clocks: disable unused mmcsd >Clocks: disable unused spi >Clocks: disable unused usb >Clocks: disable unused vlynq >Clocks: disable unused pwm0 >Clocks: disable unused pwm1 >Clocks: disable unused pwm2 >Clocks: disable unused timer1 >[...] > >r...@dm644x:~# uname -r >2.6.32-arm-davinci-default-06873-g1a7277b-dirty >r...@dm644x:~# cat /debug/davinci_clocks >ref_clk users= 8 2700 Hz > pll1users= 8 pll 59400 Hz >pll1_sysclk1 users= 0 pll 59400 Hz > dsp users= 1 psc 59400 Hz >pll1_sysclk2 users= 2 pll 29700 Hz > arm users= 2 psc 29700 Hz >pll1_sysclk3 users= 0 pll 19800 Hz > vpss_master users= 0 psc 19800 Hz > vpss_slave users=
Re: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
"Karicheri, Muralidharan" writes: >>> Kevin, >>> >>> I think I have figured it out... >>> >>> First issue was that I was adding my entry at the end of dm644x_clks[] >>> array. I need to add it before the CLK(NULL, NULL, NULL) >>> >>> secondly, your suggestion didn't work as is. This is what I had to >>> do to get it working... >>> >>> static struct clk ccdc_master_clk = { >>> .name = "dm644x_ccdc", >>> .parent = &vpss_master_clk, >>> }; >>> >>> static struct clk ccdc_slave_clk = { >>> .name = "dm644x_ccdc", >>> .parent = &vpss_slave_clk, >>> }; > > It doesn't work with out doing this. The cat /proc/davinci_clocks hangs with > your suggestion implemented... Can you track down the hang. It sounds like a bug in the walking of the clock tree for davinci_clocks. >> >>You should not need to add new clocks with new names. I don't thinke >>the name field of the struct clk is used anywhere in the matching. >>I think it's only used in /proc/davinci_clocks >> >>> static struct davinci_clk dm365_clks = { >>> >>> >>> CLK("dm644x_ccdc", "master", &ccdc_master_clk), >>> CLK("dm644x_ccdc", "slave", &ccdc_slave_clk), >> >>Looks like the drivers name is 'dm644x_ccdc', not 'isif'. I'm >>guessing just this should work without having to add new clock names. >> > No. I have mixed up the names. ISIF is for the new ISIF driver on DM365. > Below are for DM644x ccdc driver. With just these entries added, two > things observed > > 1) Only one clock is shown disabled (usually many are shown disabled) during > bootup > 2) cat /proc/davinci_clocks hangs. > > So this is the only way I got it working. Hmm, it worked just fine for me without any of these side effects. I applied the simple patch below on top of current master branch. It booted fine showing all the unused clocks being disabled, and I was able to see davinci_clocks just fine: diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach-davinci/dm644x.c index e65e29e..e6f3570 100644 --- a/arch/arm/mach-davinci/dm644x.c +++ b/arch/arm/mach-davinci/dm644x.c @@ -293,8 +293,8 @@ struct davinci_clk dm644x_clks[] = { CLK(NULL, "dsp", &dsp_clk), CLK(NULL, "arm", &arm_clk), CLK(NULL, "vicp", &vicp_clk), - CLK(NULL, "vpss_master", &vpss_master_clk), - CLK(NULL, "vpss_slave", &vpss_slave_clk), + CLK("dm644x_ccdc", "master", &vpss_master_clk), + CLK("dm644x_ccdc", "slave", &vpss_slave_clk), CLK(NULL, "arm", &arm_clk), CLK(NULL, "uart0", &uart0_clk), CLK(NULL, "uart1", &uart1_clk), [...] Clocks: disable unused uart1 Clocks: disable unused uart2 Clocks: disable unused emac Clocks: disable unused ide Clocks: disable unused asp0 Clocks: disable unused mmcsd Clocks: disable unused spi Clocks: disable unused usb Clocks: disable unused vlynq Clocks: disable unused pwm0 Clocks: disable unused pwm1 Clocks: disable unused pwm2 Clocks: disable unused timer1 [...] r...@dm644x:~# uname -r 2.6.32-arm-davinci-default-06873-g1a7277b-dirty r...@dm644x:~# cat /debug/davinci_clocks ref_clk users= 8 2700 Hz pll1users= 8 pll 59400 Hz pll1_sysclk1 users= 0 pll 59400 Hz dsp users= 1 psc 59400 Hz pll1_sysclk2 users= 2 pll 29700 Hz arm users= 2 psc 29700 Hz pll1_sysclk3 users= 0 pll 19800 Hz vpss_master users= 0 psc 19800 Hz vpss_slave users= 0 psc 19800 Hz pll1_sysclk5 users= 3 pll 9900 Hz emacusers= 1 psc 9900 Hz ide users= 0 psc 9900 Hz asp0users= 0 psc 9900 Hz mmcsd users= 0 psc 9900 Hz spi users= 0 psc 9900 H
RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
>> Kevin, >> >> I think I have figured it out... >> >> First issue was that I was adding my entry at the end of dm644x_clks[] >> array. I need to add it before the CLK(NULL, NULL, NULL) >> >> secondly, your suggestion didn't work as is. This is what I had to >> do to get it working... >> >> static struct clk ccdc_master_clk = { >> .name = "dm644x_ccdc", >> .parent = &vpss_master_clk, >> }; >> >> static struct clk ccdc_slave_clk = { >> .name = "dm644x_ccdc", >> .parent = &vpss_slave_clk, >> }; It doesn't work with out doing this. The cat /proc/davinci_clocks hangs with your suggestion implemented... > >You should not need to add new clocks with new names. I don't thinke >the name field of the struct clk is used anywhere in the matching. >I think it's only used in /proc/davinci_clocks > >> static struct davinci_clk dm365_clks = { >> >> >> CLK("dm644x_ccdc", "master", &ccdc_master_clk), >> CLK("dm644x_ccdc", "slave", &ccdc_slave_clk), > >Looks like the drivers name is 'dm644x_ccdc', not 'isif'. I'm >guessing just this should work without having to add new clock names. > No. I have mixed up the names. ISIF is for the new ISIF driver on DM365. Below are for DM644x ccdc driver. With just these entries added, two things observed 1) Only one clock is shown disabled (usually many are shown disabled) during bootup 2) cat /proc/davinci_clocks hangs. So this is the only way I got it working. >CLK("dm644x_ccdc", "master", &vpss_master_clk), >CLK("dm644x_ccdc", "slave", &vpss_slave_clk), > >> CLK(NULL, NULL, NULL); >> >> Let me know if you think there is anything wrong with the above scheme. > >Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
>> I thought following is correct:- >> Probe() >> clk_get() followed by clk_enable() >> Remove() >> clk_disable() followed by clk_put() >> Suspend() >> clk_disable() >> Resume() >> clk_enable() > >Yes, that is correct. > >I didn't look at the whole driver. My concern was that if the driver >was enhanced to more aggressive clock management, you shouldn't do a >clk_get() every time you do a clk_enable(), same for put. Got you. I am in sync with your thinking. -Murali > >Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
"Karicheri, Muralidharan" writes: > Kevin, > > I think I have figured it out... > > First issue was that I was adding my entry at the end of dm644x_clks[] > array. I need to add it before the CLK(NULL, NULL, NULL) > > secondly, your suggestion didn't work as is. This is what I had to > do to get it working... > > static struct clk ccdc_master_clk = { > .name = "dm644x_ccdc", > .parent = &vpss_master_clk, > }; > > static struct clk ccdc_slave_clk = { > .name = "dm644x_ccdc", > .parent = &vpss_slave_clk, > }; You should not need to add new clocks with new names. I don't thinke the name field of the struct clk is used anywhere in the matching. I think it's only used in /proc/davinci_clocks > static struct davinci_clk dm365_clks = { > > > CLK("dm644x_ccdc", "master", &ccdc_master_clk), > CLK("dm644x_ccdc", "slave", &ccdc_slave_clk), Looks like the drivers name is 'dm644x_ccdc', not 'isif'. I'm guessing just this should work without having to add new clock names. CLK("dm644x_ccdc", "master", &vpss_master_clk), CLK("dm644x_ccdc", "slave", &vpss_slave_clk), > CLK(NULL, NULL, NULL); > > Let me know if you think there is anything wrong with the above scheme. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
"Karicheri, Muralidharan" writes: > Kevin, > >>> +/** >>> + * vpfe_disable_clock() - Disable clocks for vpfe capture driver >>> + * @vpfe_dev - ptr to vpfe capture device >>> + * >>> + * Disables clocks defined in vpfe configuration. >>> + */ >>> static void vpfe_disable_clock(struct vpfe_device *vpfe_dev) >>> { >>> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; >>> + int i; >>> >>> - clk_disable(vpfe_cfg->vpssclk); >>> - clk_put(vpfe_cfg->vpssclk); >>> - clk_disable(vpfe_cfg->slaveclk); >>> - clk_put(vpfe_cfg->slaveclk); >>> - v4l2_info(vpfe_dev->pdev->driver, >>> -"vpfe vpss master & slave clocks disabled\n"); >>> + for (i = 0; i < vpfe_cfg->num_clocks; i++) { >>> + clk_disable(vpfe_dev->clks[i]); >>> + clk_put(vpfe_dev->clks[i]); >> >>While cleaning this up, you should move the clk_put() to module >>disable/unload time. > > [MK] vpfe_disable_clock() is called from remove(). In the new > patch, from ccdc driver remove() function, clk_put() will be called. > Why do you think it should be moved to exit() function of the module? > >>You dont' need to put he clock on every disable. >>The same for clk_get(). You don't need to get the clock for every >>enable. Just do a clk_get() at init time. > > Are you suggesting to call clk_get() during init() and call clk_put() > from exit()? What is wrong with calling clk_get() from probe()? > I thought following is correct:- > Probe() > clk_get() followed by clk_enable() > Remove() > clk_disable() followed by clk_put() > Suspend() > clk_disable() > Resume() > clk_enable() Yes, that is correct. I didn't look at the whole driver. My concern was that if the driver was enhanced to more aggressive clock management, you shouldn't do a clk_get() every time you do a clk_enable(), same for put. Kevin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
Kevin, I think I have figured it out... First issue was that I was adding my entry at the end of dm644x_clks[] array. I need to add it before the CLK(NULL, NULL, NULL) secondly, your suggestion didn't work as is. This is what I had to do to get it working... static struct clk ccdc_master_clk = { .name = "dm644x_ccdc", .parent = &vpss_master_clk, }; static struct clk ccdc_slave_clk = { .name = "dm644x_ccdc", .parent = &vpss_slave_clk, }; static struct davinci_clk dm365_clks = { CLK("dm644x_ccdc", "master", &ccdc_master_clk), CLK("dm644x_ccdc", "slave", &ccdc_slave_clk), CLK(NULL, NULL, NULL); Let me know if you think there is anything wrong with the above scheme. Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 phone: 301-407-9583 email: m-kariche...@ti.com >-Original Message- >From: Karicheri, Muralidharan >Sent: Wednesday, December 09, 2009 1:22 PM >To: Karicheri, Muralidharan; Kevin Hilman >Cc: davinci-linux-open-sou...@linux.davincidsp.com; linux- >me...@vger.kernel.org >Subject: RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable > >Kevin, > >I tried the following and I get error in clk_enable(). Do you know what >might be wrong? > >in DM365.c > >CLK("isif", "master", &vpss_master_clk) > >The driver name is isif. I call clk_get(&pdev->dev, "master") from isif >driver. The platform device name is "isif". This call succeeds, but >clk_enable() fails... > >clk_ptr = clk_get(&pdev->dev, "master"); >clk_enable(clk_ptr); > >r...@dm355-evm:~# cat /proc/davinci_clocks >ref_clk users= 7 2400 Hz > pll1users= 6 pll 48600 Hz >pll1_aux_clk users= 3 pll 2400 Hz > uart0 users= 1 psc 2400 Hz > i2c users= 1 psc 2400 Hz > spi4users= 0 psc 2400 Hz > pwm0users= 0 psc 2400 Hz > pwm1users= 0 psc 2400 Hz > pwm2users= 0 psc 2400 Hz > timer0 users= 1 psc 2400 Hz > timer1 users= 0 psc 2400 Hz > timer2 users= 1 psc 2400 Hz > timer3 users= 0 psc 2400 Hz > usb users= 0 psc 2400 Hz >pll1_sysclkbp users= 0 pll 2400 Hz >clkout0 users= 0 pll 2400 Hz >pll1_sysclk1 users= 0 pll 48600 Hz >pll1_sysclk2 users= 0 pll 24300 Hz >pll1_sysclk3 users= 0 pll 24300 Hz > vpss_dacusers= 0 psc 24300 Hz > mjcpusers= 0 psc 24300 Hz >pll1_sysclk4 users= 3 pll 12150 Hz > uart1 users= 0 psc 12150 Hz > mmcsd1 users= 0 psc 12150 Hz > spi0users= 0 psc 12150 Hz > spi1users= 0 psc 12150 Hz > spi2users= 0 psc 12150 Hz > spi3users= 0 psc 12150 Hz > gpiousers= 1 psc 12150 Hz > aemif users= 1 psc 12150 Hz > emacusers= 1 psc 12150 Hz > asp0users= 0 psc 12150 Hz > rto users= 0 psc 12150 Hz >pll1_sysclk5 users= 0 pll 24300 Hz > vpss_master users= 0 psc 24300 Hz >pll1_sysclk6 users= 0 pll 2700 Hz >pll1_sysclk7 users= 0 pll 48600 Hz >pll1_sysclk8 users= 0 pll 12150 Hz > mmcsd0 users= 0 psc 12150 Hz >pll1_sysclk9 users= 0 pll 24300 Hz > pll2users= 1 pll 59400 Hz >pll2_aux_clk users= 0 pll 2400 Hz >clkout1 users= 0 pll 2400 Hz >pll2_sysclk1 users= 0 pll 59400 Hz >pll2_sysclk2 users= 1 pll 29700 Hz > arm_clk users= 1 psc 29700 Hz >pll2_sysclk3 users= 0 pll 59400 Hz >pll2_sysclk4 users= 0 pll 20482758 Hz > voice_codec users= 0 psc 20482758 Hz >pll2_sysclk5 users= 0 pll 7425 Hz >pll2_sysclk6 users= 0 pll 59400 Hz >pll2_sysclk7 users= 0 pll 59400 Hz >pll2_sysclk8 users= 0 pll 59400 Hz >pll2_sysclk9 users= 0 pll 59400 Hz > pwm3users= 0 psc 2400 Hz >r...@dm355-evm:~# > > > >Murali Karicheri >Software Design Engineer >Texas Instruments Inc. >Germantown, MD 20874 >phone: 301-407-9583 >email: m-kariche...@ti.com > >>-Original Message- >>From: davinci-linux-open-source-boun...@linux.davincidsp.com >>[mailto:davinci-linux-open-source-boun...@linux.davincidsp.com] On Behalf >>Of Karicheri, Muralidharan >>Sent: Wednesday, December 09, 2009 12:45 PM >>To: Kevin Hilman >>Cc: davinci-linux-open-sou..
RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
Kevin, I tried the following and I get error in clk_enable(). Do you know what might be wrong? in DM365.c CLK("isif", "master", &vpss_master_clk) The driver name is isif. I call clk_get(&pdev->dev, "master") from isif driver. The platform device name is "isif". This call succeeds, but clk_enable() fails... clk_ptr = clk_get(&pdev->dev, "master"); clk_enable(clk_ptr); r...@dm355-evm:~# cat /proc/davinci_clocks ref_clk users= 7 2400 Hz pll1users= 6 pll 48600 Hz pll1_aux_clk users= 3 pll 2400 Hz uart0 users= 1 psc 2400 Hz i2c users= 1 psc 2400 Hz spi4users= 0 psc 2400 Hz pwm0users= 0 psc 2400 Hz pwm1users= 0 psc 2400 Hz pwm2users= 0 psc 2400 Hz timer0 users= 1 psc 2400 Hz timer1 users= 0 psc 2400 Hz timer2 users= 1 psc 2400 Hz timer3 users= 0 psc 2400 Hz usb users= 0 psc 2400 Hz pll1_sysclkbp users= 0 pll 2400 Hz clkout0 users= 0 pll 2400 Hz pll1_sysclk1 users= 0 pll 48600 Hz pll1_sysclk2 users= 0 pll 24300 Hz pll1_sysclk3 users= 0 pll 24300 Hz vpss_dacusers= 0 psc 24300 Hz mjcpusers= 0 psc 24300 Hz pll1_sysclk4 users= 3 pll 12150 Hz uart1 users= 0 psc 12150 Hz mmcsd1 users= 0 psc 12150 Hz spi0users= 0 psc 12150 Hz spi1users= 0 psc 12150 Hz spi2users= 0 psc 12150 Hz spi3users= 0 psc 12150 Hz gpiousers= 1 psc 12150 Hz aemif users= 1 psc 12150 Hz emacusers= 1 psc 12150 Hz asp0users= 0 psc 12150 Hz rto users= 0 psc 12150 Hz pll1_sysclk5 users= 0 pll 24300 Hz vpss_master users= 0 psc 24300 Hz pll1_sysclk6 users= 0 pll 2700 Hz pll1_sysclk7 users= 0 pll 48600 Hz pll1_sysclk8 users= 0 pll 12150 Hz mmcsd0 users= 0 psc 12150 Hz pll1_sysclk9 users= 0 pll 24300 Hz pll2users= 1 pll 59400 Hz pll2_aux_clk users= 0 pll 2400 Hz clkout1 users= 0 pll 2400 Hz pll2_sysclk1 users= 0 pll 59400 Hz pll2_sysclk2 users= 1 pll 29700 Hz arm_clk users= 1 psc 29700 Hz pll2_sysclk3 users= 0 pll 59400 Hz pll2_sysclk4 users= 0 pll 20482758 Hz voice_codec users= 0 psc 20482758 Hz pll2_sysclk5 users= 0 pll 7425 Hz pll2_sysclk6 users= 0 pll 59400 Hz pll2_sysclk7 users= 0 pll 59400 Hz pll2_sysclk8 users= 0 pll 59400 Hz pll2_sysclk9 users= 0 pll 59400 Hz pwm3users= 0 psc 2400 Hz r...@dm355-evm:~# Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 phone: 301-407-9583 email: m-kariche...@ti.com >-Original Message- >From: davinci-linux-open-source-boun...@linux.davincidsp.com >[mailto:davinci-linux-open-source-boun...@linux.davincidsp.com] On Behalf >Of Karicheri, Muralidharan >Sent: Wednesday, December 09, 2009 12:45 PM >To: Kevin Hilman >Cc: davinci-linux-open-sou...@linux.davincidsp.com; linux- >me...@vger.kernel.org >Subject: RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable > >Kevin, > >>> +/** >>> + * vpfe_disable_clock() - Disable clocks for vpfe capture driver >>> + * @vpfe_dev - ptr to vpfe capture device >>> + * >>> + * Disables clocks defined in vpfe configuration. >>> + */ >>> static void vpfe_disable_clock(struct vpfe_device *vpfe_dev) >>> { >>> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; >>> + int i; >>> >>> - clk_disable(vpfe_cfg->vpssclk); >>> - clk_put(vpfe_cfg->vpssclk); >>> - clk_disable(vpfe_cfg->slaveclk); >>> - clk_put(vpfe_cfg->slaveclk); >>> - v4l2_info(vpfe_dev->pdev->driver, >>> -"vpfe vpss master & slave clocks disabled\n"); >>> + for (i = 0; i < vpfe_cfg->num_clocks; i++) { >>> + clk_disable(vpfe_dev->clks[i]); >>> + clk_put(vpfe_dev->clks[i]); >> >>While cleaning this up, you should move the clk_put() to module >>disable/unload time. > >[MK] vpfe_disable_clock() is called from remove(). In the new >patch, from ccdc driver remove() function, clk_put() will be called. >Why do you think it should be moved to exit() function of the module? > >>You dont' need to put he clock on every disable. >>The same for clk_get(). You don't need to get the cloc
RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
Kevin, >> +/** >> + * vpfe_disable_clock() - Disable clocks for vpfe capture driver >> + * @vpfe_dev - ptr to vpfe capture device >> + * >> + * Disables clocks defined in vpfe configuration. >> + */ >> static void vpfe_disable_clock(struct vpfe_device *vpfe_dev) >> { >> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; >> +int i; >> >> -clk_disable(vpfe_cfg->vpssclk); >> -clk_put(vpfe_cfg->vpssclk); >> -clk_disable(vpfe_cfg->slaveclk); >> -clk_put(vpfe_cfg->slaveclk); >> -v4l2_info(vpfe_dev->pdev->driver, >> - "vpfe vpss master & slave clocks disabled\n"); >> +for (i = 0; i < vpfe_cfg->num_clocks; i++) { >> +clk_disable(vpfe_dev->clks[i]); >> +clk_put(vpfe_dev->clks[i]); > >While cleaning this up, you should move the clk_put() to module >disable/unload time. [MK] vpfe_disable_clock() is called from remove(). In the new patch, from ccdc driver remove() function, clk_put() will be called. Why do you think it should be moved to exit() function of the module? >You dont' need to put he clock on every disable. >The same for clk_get(). You don't need to get the clock for every >enable. Just do a clk_get() at init time. Are you suggesting to call clk_get() during init() and call clk_put() from exit()? What is wrong with calling clk_get() from probe()? I thought following is correct:- Probe() clk_get() followed by clk_enable() Remove() clk_disable() followed by clk_put() Suspend() clk_disable() Resume() clk_enable() Please confirm. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
Kevin, After sending out my last patch (moving clocks to ccdc driver), I thought of reworking it in similar lines. I will re-send the patch after incorporating this. Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 phone: 301-407-9583 email: m-kariche...@ti.com >-Original Message- >From: Kevin Hilman [mailto:khil...@deeprootsystems.com] >Sent: Wednesday, December 09, 2009 11:00 AM >To: Karicheri, Muralidharan >Cc: linux-media@vger.kernel.org; hverk...@xs4all.nl; davinci-linux-open- >sou...@linux.davincidsp.com; Hiremath, Vaibhav >Subject: Re: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable > >m-kariche...@ti.com writes: > >> From: Muralidharan Karicheri >> >> v1 - updated based on comments from Vaibhav Hiremath. >> >> On DM365 we use only vpss master clock, where as on DM355 and >> DM6446, we use vpss master and slave clocks for vpfe capture and AM3517 >> we use internal clock and pixel clock. So this patch makes it >configurable >> on a per platform basis. This is needed for supporting DM365 for which >patches >> will be available soon. >> >> Tested-by: Vaibhav Hiremath , Muralidharan Karicheri kariche...@ti.com> >> Acked-by: Vaibhav Hiremath >> Signed-off-by: Muralidharan Karicheri > >Sorry for jumping late into this thread, but I have a couple comments. > >First, we should *never* pass clock names from platform code to driver >code. We have the clkdevs for this. Using clkdev, the right clock >is determined from the driver being used and any additional info. > >Rather than passing the strings along, you should add the driver name >to the 'dev_id' field of the clkdev node, and then use the 'con_id' field >to distinguish between the two clocks: > >- CLK(NULL, "vpss_master", &vpss_master_clk), >- CLK(NULL, "vpss_slave", &vpss_slave_clk), >+ CLK("vpfe-capture", "master", &vpss_master_clk), >+ CLK("vpfe-capture", "slave", &vpss_slave_clk), > >NOTE: this assumes clks are used from VPFE. When you move to CCDC, this >should change accordingly. > >More on the usage below... > > > >> --- >> drivers/media/video/davinci/vpfe_capture.c | 98 +- >- >> include/media/davinci/vpfe_capture.h | 11 ++- >> 2 files changed, 70 insertions(+), 39 deletions(-) >> >> diff --git a/drivers/media/video/davinci/vpfe_capture.c >b/drivers/media/video/davinci/vpfe_capture.c >> index 12a1b3d..c3468ee 100644 >> --- a/drivers/media/video/davinci/vpfe_capture.c >> +++ b/drivers/media/video/davinci/vpfe_capture.c >> @@ -1787,61 +1787,87 @@ static struct vpfe_device *vpfe_initialize(void) >> return vpfe_dev; >> } >> >> +/** >> + * vpfe_disable_clock() - Disable clocks for vpfe capture driver >> + * @vpfe_dev - ptr to vpfe capture device >> + * >> + * Disables clocks defined in vpfe configuration. >> + */ >> static void vpfe_disable_clock(struct vpfe_device *vpfe_dev) >> { >> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; >> +int i; >> >> -clk_disable(vpfe_cfg->vpssclk); >> -clk_put(vpfe_cfg->vpssclk); >> -clk_disable(vpfe_cfg->slaveclk); >> -clk_put(vpfe_cfg->slaveclk); >> -v4l2_info(vpfe_dev->pdev->driver, >> - "vpfe vpss master & slave clocks disabled\n"); >> +for (i = 0; i < vpfe_cfg->num_clocks; i++) { >> +clk_disable(vpfe_dev->clks[i]); >> +clk_put(vpfe_dev->clks[i]); > >While cleaning this up, you should move the clk_put() to module >disable/unload time. You dont' need to put he clock on every disable. >The same for clk_get(). You don't need to get the clock for every >enable. Just do a clk_get() at init time. > >> +} >> +kfree(vpfe_dev->clks); >> +v4l2_info(vpfe_dev->pdev->driver, "vpfe capture clocks disabled\n"); >> } >> >> +/** >> + * vpfe_enable_clock() - Enable clocks for vpfe capture driver >> + * @vpfe_dev - ptr to vpfe capture device >> + * >> + * Enables clocks defined in vpfe configuration. The function >> + * assumes that at least one clock is to be defined which is >> + * true as of now. re-visit this if this assumption is not true >> + */ >> static int vpfe_enable_clock(struct vpfe_device *vpfe_dev) >> { >> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; >> -int ret = -ENOENT; >> +int i; >> >> -vpfe
Re: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
m-kariche...@ti.com writes: > From: Muralidharan Karicheri > > v1 - updated based on comments from Vaibhav Hiremath. > > On DM365 we use only vpss master clock, where as on DM355 and > DM6446, we use vpss master and slave clocks for vpfe capture and AM3517 > we use internal clock and pixel clock. So this patch makes it configurable > on a per platform basis. This is needed for supporting DM365 for which patches > will be available soon. > > Tested-by: Vaibhav Hiremath , Muralidharan Karicheri > > Acked-by: Vaibhav Hiremath > Signed-off-by: Muralidharan Karicheri Sorry for jumping late into this thread, but I have a couple comments. First, we should *never* pass clock names from platform code to driver code. We have the clkdevs for this. Using clkdev, the right clock is determined from the driver being used and any additional info. Rather than passing the strings along, you should add the driver name to the 'dev_id' field of the clkdev node, and then use the 'con_id' field to distinguish between the two clocks: - CLK(NULL, "vpss_master", &vpss_master_clk), - CLK(NULL, "vpss_slave", &vpss_slave_clk), + CLK("vpfe-capture", "master", &vpss_master_clk), + CLK("vpfe-capture", "slave", &vpss_slave_clk), NOTE: this assumes clks are used from VPFE. When you move to CCDC, this should change accordingly. More on the usage below... > --- > drivers/media/video/davinci/vpfe_capture.c | 98 +-- > include/media/davinci/vpfe_capture.h | 11 ++- > 2 files changed, 70 insertions(+), 39 deletions(-) > > diff --git a/drivers/media/video/davinci/vpfe_capture.c > b/drivers/media/video/davinci/vpfe_capture.c > index 12a1b3d..c3468ee 100644 > --- a/drivers/media/video/davinci/vpfe_capture.c > +++ b/drivers/media/video/davinci/vpfe_capture.c > @@ -1787,61 +1787,87 @@ static struct vpfe_device *vpfe_initialize(void) > return vpfe_dev; > } > > +/** > + * vpfe_disable_clock() - Disable clocks for vpfe capture driver > + * @vpfe_dev - ptr to vpfe capture device > + * > + * Disables clocks defined in vpfe configuration. > + */ > static void vpfe_disable_clock(struct vpfe_device *vpfe_dev) > { > struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; > + int i; > > - clk_disable(vpfe_cfg->vpssclk); > - clk_put(vpfe_cfg->vpssclk); > - clk_disable(vpfe_cfg->slaveclk); > - clk_put(vpfe_cfg->slaveclk); > - v4l2_info(vpfe_dev->pdev->driver, > - "vpfe vpss master & slave clocks disabled\n"); > + for (i = 0; i < vpfe_cfg->num_clocks; i++) { > + clk_disable(vpfe_dev->clks[i]); > + clk_put(vpfe_dev->clks[i]); While cleaning this up, you should move the clk_put() to module disable/unload time. You dont' need to put he clock on every disable. The same for clk_get(). You don't need to get the clock for every enable. Just do a clk_get() at init time. > + } > + kfree(vpfe_dev->clks); > + v4l2_info(vpfe_dev->pdev->driver, "vpfe capture clocks disabled\n"); > } > > +/** > + * vpfe_enable_clock() - Enable clocks for vpfe capture driver > + * @vpfe_dev - ptr to vpfe capture device > + * > + * Enables clocks defined in vpfe configuration. The function > + * assumes that at least one clock is to be defined which is > + * true as of now. re-visit this if this assumption is not true > + */ > static int vpfe_enable_clock(struct vpfe_device *vpfe_dev) > { > struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; > - int ret = -ENOENT; > + int i; > > - vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "vpss_master"); Using my clkdevs from above, you just need to change this to: vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "master"); Now that the device name is in the clkdev node, clk_get() will find the right clock based on the device name. > - if (NULL == vpfe_cfg->vpssclk) { > - v4l2_err(vpfe_dev->pdev->driver, "No clock defined for" > - "vpss_master\n"); > - return ret; > - } > + if (!vpfe_cfg->num_clocks) > + return 0; > > - if (clk_enable(vpfe_cfg->vpssclk)) { > - v4l2_err(vpfe_dev->pdev->driver, > - "vpfe vpss master clock not enabled\n"); > - goto out; > - } > - v4l2_info(vpfe_dev->pdev->driver, > - "vpfe vpss master clock enabled\n"); > + vpfe_dev->clks = kzalloc(vpfe_cfg->num_clocks * > +sizeof(struct clock *), GFP_KERNEL); > > - vpfe_cfg->slaveclk = clk_get(vpfe_dev->pdev, "vpss_slave"); And here: vpfe_cfg->slaveclk = clk_get(vpfe_dev->pdev, "slave"); > - if (NULL == vpfe_cfg->slaveclk) { > - v4l2_err(vpfe_dev->pdev->driver, > - "No clock defined for vpss slave\n"); > - goto out; > + if (NULL == vpfe_dev->clks) { > + v4l2_err(vpfe_dev->pdev->driver, "Memory allocation failed\n"); > + return -ENOMEM; >
RE: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
Hans, Please hold on to this. I will send you an updated patch. Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 phone: 301-407-9583 email: m-kariche...@ti.com >-Original Message- >From: Karicheri, Muralidharan >Sent: Tuesday, December 01, 2009 12:22 PM >To: 'Hans Verkuil' >Cc: Hiremath, Vaibhav; linux-media@vger.kernel.org >Subject: FW: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable > >Hans, > >Could you please add this to your hg tree and send a pull >request to Mauro? This was reviewed by Vaibhav and tested on >DM355, DM6446, AM3517 & DM365. I will request Kevin to pull >the Architecture part of this patch. > >Thanks. > >Murali Karicheri >Software Design Engineer >Texas Instruments Inc. >Germantown, MD 20874 >phone: 301-407-9583 >email: m-kariche...@ti.com > >>-Original Message- >>From: Karicheri, Muralidharan >>Sent: Tuesday, December 01, 2009 12:19 PM >>To: linux-media@vger.kernel.org; hverk...@xs4all.nl; >>khil...@deeprootsystems.com >>Cc: davinci-linux-open-sou...@linux.davincidsp.com; Hiremath, Vaibhav; >>Karicheri, Muralidharan >>Subject: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable >> >>From: Muralidharan Karicheri >> >>v1 - updated based on comments from Vaibhav Hiremath. >> >>On DM365 we use only vpss master clock, where as on DM355 and >>DM6446, we use vpss master and slave clocks for vpfe capture and AM3517 >>we use internal clock and pixel clock. So this patch makes it configurable >>on a per platform basis. This is needed for supporting DM365 for which >>patches >>will be available soon. >> >>Tested-by: Vaibhav Hiremath , Muralidharan Karicheri >kariche...@ti.com> >>Acked-by: Vaibhav Hiremath >>Signed-off-by: Muralidharan Karicheri >>--- >> drivers/media/video/davinci/vpfe_capture.c | 98 +-- >- >>--- >> include/media/davinci/vpfe_capture.h | 11 ++- >> 2 files changed, 70 insertions(+), 39 deletions(-) >> >>diff --git a/drivers/media/video/davinci/vpfe_capture.c >>b/drivers/media/video/davinci/vpfe_capture.c >>index 12a1b3d..c3468ee 100644 >>--- a/drivers/media/video/davinci/vpfe_capture.c >>+++ b/drivers/media/video/davinci/vpfe_capture.c >>@@ -1787,61 +1787,87 @@ static struct vpfe_device *vpfe_initialize(void) >> return vpfe_dev; >> } >> >>+/** >>+ * vpfe_disable_clock() - Disable clocks for vpfe capture driver >>+ * @vpfe_dev - ptr to vpfe capture device >>+ * >>+ * Disables clocks defined in vpfe configuration. >>+ */ >> static void vpfe_disable_clock(struct vpfe_device *vpfe_dev) >> { >> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; >>+ int i; >> >>- clk_disable(vpfe_cfg->vpssclk); >>- clk_put(vpfe_cfg->vpssclk); >>- clk_disable(vpfe_cfg->slaveclk); >>- clk_put(vpfe_cfg->slaveclk); >>- v4l2_info(vpfe_dev->pdev->driver, >>- "vpfe vpss master & slave clocks disabled\n"); >>+ for (i = 0; i < vpfe_cfg->num_clocks; i++) { >>+ clk_disable(vpfe_dev->clks[i]); >>+ clk_put(vpfe_dev->clks[i]); >>+ } >>+ kfree(vpfe_dev->clks); >>+ v4l2_info(vpfe_dev->pdev->driver, "vpfe capture clocks disabled\n"); >> } >> >>+/** >>+ * vpfe_enable_clock() - Enable clocks for vpfe capture driver >>+ * @vpfe_dev - ptr to vpfe capture device >>+ * >>+ * Enables clocks defined in vpfe configuration. The function >>+ * assumes that at least one clock is to be defined which is >>+ * true as of now. re-visit this if this assumption is not true >>+ */ >> static int vpfe_enable_clock(struct vpfe_device *vpfe_dev) >> { >> struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; >>- int ret = -ENOENT; >>+ int i; >> >>- vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "vpss_master"); >>- if (NULL == vpfe_cfg->vpssclk) { >>- v4l2_err(vpfe_dev->pdev->driver, "No clock defined for" >>- "vpss_master\n"); >>- return ret; >>- } >>+ if (!vpfe_cfg->num_clocks) >>+ return 0; >> >>- if (clk_enable(vpfe_cfg->vpssclk)) { >>- v4l2_err(vpfe_dev->pdev->driver, >>- "vpfe vpss master clock not enabled\n"); >>- goto out; >>- } >>- v4l2_
FW: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
Hans, Could you please add this to your hg tree and send a pull request to Mauro? This was reviewed by Vaibhav and tested on DM355, DM6446, AM3517 & DM365. I will request Kevin to pull the Architecture part of this patch. Thanks. Murali Karicheri Software Design Engineer Texas Instruments Inc. Germantown, MD 20874 phone: 301-407-9583 email: m-kariche...@ti.com >-Original Message- >From: Karicheri, Muralidharan >Sent: Tuesday, December 01, 2009 12:19 PM >To: linux-media@vger.kernel.org; hverk...@xs4all.nl; >khil...@deeprootsystems.com >Cc: davinci-linux-open-sou...@linux.davincidsp.com; Hiremath, Vaibhav; >Karicheri, Muralidharan >Subject: [PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable > >From: Muralidharan Karicheri > >v1 - updated based on comments from Vaibhav Hiremath. > >On DM365 we use only vpss master clock, where as on DM355 and >DM6446, we use vpss master and slave clocks for vpfe capture and AM3517 >we use internal clock and pixel clock. So this patch makes it configurable >on a per platform basis. This is needed for supporting DM365 for which >patches >will be available soon. > >Tested-by: Vaibhav Hiremath , Muralidharan Karicheri kariche...@ti.com> >Acked-by: Vaibhav Hiremath >Signed-off-by: Muralidharan Karicheri >--- > drivers/media/video/davinci/vpfe_capture.c | 98 +--- >--- > include/media/davinci/vpfe_capture.h | 11 ++- > 2 files changed, 70 insertions(+), 39 deletions(-) > >diff --git a/drivers/media/video/davinci/vpfe_capture.c >b/drivers/media/video/davinci/vpfe_capture.c >index 12a1b3d..c3468ee 100644 >--- a/drivers/media/video/davinci/vpfe_capture.c >+++ b/drivers/media/video/davinci/vpfe_capture.c >@@ -1787,61 +1787,87 @@ static struct vpfe_device *vpfe_initialize(void) > return vpfe_dev; > } > >+/** >+ * vpfe_disable_clock() - Disable clocks for vpfe capture driver >+ * @vpfe_dev - ptr to vpfe capture device >+ * >+ * Disables clocks defined in vpfe configuration. >+ */ > static void vpfe_disable_clock(struct vpfe_device *vpfe_dev) > { > struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; >+ int i; > >- clk_disable(vpfe_cfg->vpssclk); >- clk_put(vpfe_cfg->vpssclk); >- clk_disable(vpfe_cfg->slaveclk); >- clk_put(vpfe_cfg->slaveclk); >- v4l2_info(vpfe_dev->pdev->driver, >- "vpfe vpss master & slave clocks disabled\n"); >+ for (i = 0; i < vpfe_cfg->num_clocks; i++) { >+ clk_disable(vpfe_dev->clks[i]); >+ clk_put(vpfe_dev->clks[i]); >+ } >+ kfree(vpfe_dev->clks); >+ v4l2_info(vpfe_dev->pdev->driver, "vpfe capture clocks disabled\n"); > } > >+/** >+ * vpfe_enable_clock() - Enable clocks for vpfe capture driver >+ * @vpfe_dev - ptr to vpfe capture device >+ * >+ * Enables clocks defined in vpfe configuration. The function >+ * assumes that at least one clock is to be defined which is >+ * true as of now. re-visit this if this assumption is not true >+ */ > static int vpfe_enable_clock(struct vpfe_device *vpfe_dev) > { > struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; >- int ret = -ENOENT; >+ int i; > >- vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "vpss_master"); >- if (NULL == vpfe_cfg->vpssclk) { >- v4l2_err(vpfe_dev->pdev->driver, "No clock defined for" >- "vpss_master\n"); >- return ret; >- } >+ if (!vpfe_cfg->num_clocks) >+ return 0; > >- if (clk_enable(vpfe_cfg->vpssclk)) { >- v4l2_err(vpfe_dev->pdev->driver, >- "vpfe vpss master clock not enabled\n"); >- goto out; >- } >- v4l2_info(vpfe_dev->pdev->driver, >- "vpfe vpss master clock enabled\n"); >+ vpfe_dev->clks = kzalloc(vpfe_cfg->num_clocks * >+ sizeof(struct clock *), GFP_KERNEL); > >- vpfe_cfg->slaveclk = clk_get(vpfe_dev->pdev, "vpss_slave"); >- if (NULL == vpfe_cfg->slaveclk) { >- v4l2_err(vpfe_dev->pdev->driver, >- "No clock defined for vpss slave\n"); >- goto out; >+ if (NULL == vpfe_dev->clks) { >+ v4l2_err(vpfe_dev->pdev->driver, "Memory allocation failed\n"); >+ return -ENOMEM; > } > >- if (clk_enable(vpfe_cfg->slaveclk)) { >- v4l2_err(vpfe_dev->pdev->driver, >- "vpfe vpss slave
[PATCH - v1 1/2] V4L - vpfe capture - make clocks configurable
From: Muralidharan Karicheri v1 - updated based on comments from Vaibhav Hiremath. On DM365 we use only vpss master clock, where as on DM355 and DM6446, we use vpss master and slave clocks for vpfe capture and AM3517 we use internal clock and pixel clock. So this patch makes it configurable on a per platform basis. This is needed for supporting DM365 for which patches will be available soon. Tested-by: Vaibhav Hiremath , Muralidharan Karicheri Acked-by: Vaibhav Hiremath Signed-off-by: Muralidharan Karicheri --- drivers/media/video/davinci/vpfe_capture.c | 98 +-- include/media/davinci/vpfe_capture.h | 11 ++- 2 files changed, 70 insertions(+), 39 deletions(-) diff --git a/drivers/media/video/davinci/vpfe_capture.c b/drivers/media/video/davinci/vpfe_capture.c index 12a1b3d..c3468ee 100644 --- a/drivers/media/video/davinci/vpfe_capture.c +++ b/drivers/media/video/davinci/vpfe_capture.c @@ -1787,61 +1787,87 @@ static struct vpfe_device *vpfe_initialize(void) return vpfe_dev; } +/** + * vpfe_disable_clock() - Disable clocks for vpfe capture driver + * @vpfe_dev - ptr to vpfe capture device + * + * Disables clocks defined in vpfe configuration. + */ static void vpfe_disable_clock(struct vpfe_device *vpfe_dev) { struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; + int i; - clk_disable(vpfe_cfg->vpssclk); - clk_put(vpfe_cfg->vpssclk); - clk_disable(vpfe_cfg->slaveclk); - clk_put(vpfe_cfg->slaveclk); - v4l2_info(vpfe_dev->pdev->driver, -"vpfe vpss master & slave clocks disabled\n"); + for (i = 0; i < vpfe_cfg->num_clocks; i++) { + clk_disable(vpfe_dev->clks[i]); + clk_put(vpfe_dev->clks[i]); + } + kfree(vpfe_dev->clks); + v4l2_info(vpfe_dev->pdev->driver, "vpfe capture clocks disabled\n"); } +/** + * vpfe_enable_clock() - Enable clocks for vpfe capture driver + * @vpfe_dev - ptr to vpfe capture device + * + * Enables clocks defined in vpfe configuration. The function + * assumes that at least one clock is to be defined which is + * true as of now. re-visit this if this assumption is not true + */ static int vpfe_enable_clock(struct vpfe_device *vpfe_dev) { struct vpfe_config *vpfe_cfg = vpfe_dev->cfg; - int ret = -ENOENT; + int i; - vpfe_cfg->vpssclk = clk_get(vpfe_dev->pdev, "vpss_master"); - if (NULL == vpfe_cfg->vpssclk) { - v4l2_err(vpfe_dev->pdev->driver, "No clock defined for" -"vpss_master\n"); - return ret; - } + if (!vpfe_cfg->num_clocks) + return 0; - if (clk_enable(vpfe_cfg->vpssclk)) { - v4l2_err(vpfe_dev->pdev->driver, - "vpfe vpss master clock not enabled\n"); - goto out; - } - v4l2_info(vpfe_dev->pdev->driver, -"vpfe vpss master clock enabled\n"); + vpfe_dev->clks = kzalloc(vpfe_cfg->num_clocks * + sizeof(struct clock *), GFP_KERNEL); - vpfe_cfg->slaveclk = clk_get(vpfe_dev->pdev, "vpss_slave"); - if (NULL == vpfe_cfg->slaveclk) { - v4l2_err(vpfe_dev->pdev->driver, - "No clock defined for vpss slave\n"); - goto out; + if (NULL == vpfe_dev->clks) { + v4l2_err(vpfe_dev->pdev->driver, "Memory allocation failed\n"); + return -ENOMEM; } - if (clk_enable(vpfe_cfg->slaveclk)) { - v4l2_err(vpfe_dev->pdev->driver, -"vpfe vpss slave clock not enabled\n"); - goto out; + for (i = 0; i < vpfe_cfg->num_clocks; i++) { + if (NULL == vpfe_cfg->clocks[i]) { + v4l2_err(vpfe_dev->pdev->driver, + "clock %s is not defined in vpfe config\n", + vpfe_cfg->clocks[i]); + goto out; + } + + vpfe_dev->clks[i] = clk_get(vpfe_dev->pdev, + vpfe_cfg->clocks[i]); + if (NULL == vpfe_dev->clks[i]) { + v4l2_err(vpfe_dev->pdev->driver, + "Failed to get clock %s\n", + vpfe_cfg->clocks[i]); + goto out; + } + + if (clk_enable(vpfe_dev->clks[i])) { + v4l2_err(vpfe_dev->pdev->driver, + "vpfe clock %s not enabled\n", + vpfe_cfg->clocks[i]); + goto out; + } + + v4l2_info(vpfe_dev->pdev->driver, "vpss clock %s enabled", + vpfe_cfg->clocks[i]); } - v4l2_info(vpfe_dev->pdev->driver, "vpfe vpss slave clock enabled\n"); return 0; out: - if (vpfe_cfg->vpssclk) -