| Message ID | 1355565484-15791-4-git-send-email-twang13@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 15 Dec 2012 17:57:52 +0800 Albert Wang <twang13@marvell.com> wrote: > From: Libin Yang <lbyang@marvell.com> > > This patch adds the clock tree support for marvell-ccic. > > Each board may require different clk enabling sequence. > Developer need add the clk_name in correct sequence in board driver > to use this feature. > > +static void mcam_clk_set(struct mcam_camera *mcam, int on) > +{ > + unsigned int i; > + > + if (on) { > + for (i = 0; i < mcam->clk_num; i++) { > + if (mcam->clk[i]) > + clk_enable(mcam->clk[i]); > + } > + } else { > + for (i = mcam->clk_num; i > 0; i--) { > + if (mcam->clk[i - 1]) > + clk_disable(mcam->clk[i - 1]); > + } > + } > +} A couple of minor comments: - This function is always called with a constant value for "on". It would be easier to read (and less prone to unfortunate brace errors) if it were just two functions: mcam_clk_enable() and mcam_clk_disable(). - I'd write the second for loop as: for (i = mcal->clk_num - 1; i >= 0; i==) { just to match the values used in the other direction and avoid the subscript arithmetic. > +static void mcam_init_clk(struct mcam_camera *mcam, > + struct mmp_camera_platform_data *pdata, int init) So why does an "init" function have an "init" parameter? Again, I think this would be far better split into two functions. Among other things, that would help to reduce the deep nesting below. > +{ > + unsigned int i; > + > + if (NR_MCAM_CLK < pdata->clk_num) { > + dev_err(mcam->dev, "Too many mcam clocks defined\n"); > + mcam->clk_num = 0; > + return; > + } > + > + if (init) { > + for (i = 0; i < pdata->clk_num; i++) { > + if (pdata->clk_name[i] != NULL) { > + mcam->clk[i] = devm_clk_get(mcam->dev, > + pdata->clk_name[i]); > + if (IS_ERR(mcam->clk[i])) { > + dev_err(mcam->dev, > + "Could not get clk: %s\n", > + pdata->clk_name[i]); > + mcam->clk_num = 0; > + return; > + } > + } > + } > + mcam->clk_num = pdata->clk_num; > + } else > + mcam->clk_num = 0; > +} Again, minor comments, but I do think the code would be improved by splitting those functions. Meanwhile: Acked-by: Jonathan Corbet <corbet@lwn.net> jon -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Jonathan >-----Original Message----- >From: Jonathan Corbet [mailto:corbet@lwn.net] >Sent: Monday, 17 December, 2012 00:03 >To: Albert Wang >Cc: g.liakhovetski@gmx.de; linux-media@vger.kernel.org; Libin Yang >Subject: Re: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for marvell- >ccic driver > >On Sat, 15 Dec 2012 17:57:52 +0800 >Albert Wang <twang13@marvell.com> wrote: > >> From: Libin Yang <lbyang@marvell.com> >> >> This patch adds the clock tree support for marvell-ccic. >> >> Each board may require different clk enabling sequence. >> Developer need add the clk_name in correct sequence in board driver >> to use this feature. >> >> +static void mcam_clk_set(struct mcam_camera *mcam, int on) >> +{ >> + unsigned int i; >> + >> + if (on) { >> + for (i = 0; i < mcam->clk_num; i++) { >> + if (mcam->clk[i]) >> + clk_enable(mcam->clk[i]); >> + } >> + } else { >> + for (i = mcam->clk_num; i > 0; i--) { >> + if (mcam->clk[i - 1]) >> + clk_disable(mcam->clk[i - 1]); >> + } >> + } >> +} > >A couple of minor comments: > > - This function is always called with a constant value for "on". It would > be easier to read (and less prone to unfortunate brace errors) if it > were just two functions: mcam_clk_enable() and mcam_clk_disable(). > [Albert Wang] OK, that's fine to split it to 2 functions. :) > - I'd write the second for loop as: > > for (i = mcal->clk_num - 1; i >= 0; i==) { > > just to match the values used in the other direction and avoid the > subscript arithmetic. > [Albert Wang] Yes, we can improve it. :) >> +static void mcam_init_clk(struct mcam_camera *mcam, >> + struct mmp_camera_platform_data *pdata, int init) > >So why does an "init" function have an "init" parameter? Again, I think >this would be far better split into two functions. Among other things, >that would help to reduce the deep nesting below. > [Albert Wang] Yes, the parameter name is confused. And we will split this function too. :) >> +{ >> + unsigned int i; >> + >> + if (NR_MCAM_CLK < pdata->clk_num) { >> + dev_err(mcam->dev, "Too many mcam clocks defined\n"); >> + mcam->clk_num = 0; >> + return; >> + } >> + >> + if (init) { >> + for (i = 0; i < pdata->clk_num; i++) { >> + if (pdata->clk_name[i] != NULL) { >> + mcam->clk[i] = devm_clk_get(mcam->dev, >> + pdata->clk_name[i]); >> + if (IS_ERR(mcam->clk[i])) { >> + dev_err(mcam->dev, >> + "Could not get clk: %s\n", >> + pdata->clk_name[i]); >> + mcam->clk_num = 0; >> + return; >> + } >> + } >> + } >> + mcam->clk_num = pdata->clk_num; >> + } else >> + mcam->clk_num = 0; >> +} > >Again, minor comments, but I do think the code would be improved by >splitting those functions. Meanwhile: > >Acked-by: Jonathan Corbet <corbet@lwn.net> > >jon Thanks Albert Wang 86-21-61092656 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 15 Dec 2012, Albert Wang wrote: > From: Libin Yang <lbyang@marvell.com> > > This patch adds the clock tree support for marvell-ccic. > > Each board may require different clk enabling sequence. > Developer need add the clk_name in correct sequence in board driver > to use this feature. > > Signed-off-by: Libin Yang <lbyang@marvell.com> > Signed-off-by: Albert Wang <twang13@marvell.com> > --- > drivers/media/platform/marvell-ccic/mcam-core.h | 4 ++ > drivers/media/platform/marvell-ccic/mmp-driver.c | 57 +++++++++++++++++++++- > include/media/mmp-camera.h | 5 ++ > 3 files changed, 65 insertions(+), 1 deletion(-) > > diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h > index ca63010..86e634e 100755 > --- a/drivers/media/platform/marvell-ccic/mcam-core.h > +++ b/drivers/media/platform/marvell-ccic/mcam-core.h > @@ -88,6 +88,7 @@ struct mcam_frame_state { > * the dev_lock spinlock; they are marked as such by comments. > * dev_lock is also required for access to device registers. > */ > +#define NR_MCAM_CLK 4 > struct mcam_camera { > /* > * These fields should be set by the platform code prior to > @@ -109,6 +110,9 @@ struct mcam_camera { > int lane; /* lane number */ > > struct clk *pll1; > + /* clock tree support */ > + struct clk *clk[NR_MCAM_CLK]; > + int clk_num; > > /* > * Callbacks from the core to the platform code. > diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c > index 603fa0a..2c4dce3 100755 > --- a/drivers/media/platform/marvell-ccic/mmp-driver.c > +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c > @@ -104,6 +104,23 @@ static struct mmp_camera *mmpcam_find_device(struct platform_device *pdev) > #define REG_CCIC_DCGCR 0x28 /* CCIC dyn clock gate ctrl reg */ > #define REG_CCIC_CRCR 0x50 /* CCIC clk reset ctrl reg */ > > +static void mcam_clk_set(struct mcam_camera *mcam, int on) Personally, I would also make two functions out of this - "on" and "off," but that's a matter of taste, perhaps. > +{ > + unsigned int i; > + > + if (on) { > + for (i = 0; i < mcam->clk_num; i++) { > + if (mcam->clk[i]) > + clk_enable(mcam->clk[i]); > + } > + } else { > + for (i = mcam->clk_num; i > 0; i--) { > + if (mcam->clk[i - 1]) > + clk_disable(mcam->clk[i - 1]); > + } > + } > +} > + > /* > * Power control. > */ > @@ -134,6 +151,8 @@ static void mmpcam_power_up(struct mcam_camera *mcam) > mdelay(5); > gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */ > mdelay(5); > + > + mcam_clk_set(mcam, 1); > } > > static void mmpcam_power_down(struct mcam_camera *mcam) > @@ -151,6 +170,8 @@ static void mmpcam_power_down(struct mcam_camera *mcam) > pdata = cam->pdev->dev.platform_data; > gpio_set_value(pdata->sensor_power_gpio, 0); > gpio_set_value(pdata->sensor_reset_gpio, 0); > + > + mcam_clk_set(mcam, 0); > } > > /* > @@ -202,7 +223,7 @@ void mmpcam_calc_dphy(struct mcam_camera *mcam) > * pll1 will never be changed, it is a fixed value > */ > > - if (IS_ERR(mcam->pll1)) > + if (IS_ERR_OR_NULL(mcam->pll1)) Why are you changing this? If this really were needed, you should do this already in the previous patch, where you add these lines. But I don't think this is a good idea, don't think Russell would like this :-) NULL is a valid clock. Only a negative error is a failure. In fact, if you like, you could initialise .pll1 to ERR_PTR(-EINVAL) in your previous patch in mmpcam_probe(). > return; > > tx_clk_esc = clk_get_rate(mcam->pll1) / 1000000 / 12; > @@ -229,6 +250,35 @@ static irqreturn_t mmpcam_irq(int irq, void *data) > return IRQ_RETVAL(handled); > } > > +static void mcam_init_clk(struct mcam_camera *mcam, > + struct mmp_camera_platform_data *pdata, int init) I don't think this "int init" makes sense. Please, remove the parameter and you actually don't need the de-initialisation, no reason to set num_clk = 0 before freeing memory. > +{ > + unsigned int i; > + > + if (NR_MCAM_CLK < pdata->clk_num) { > + dev_err(mcam->dev, "Too many mcam clocks defined\n"); > + mcam->clk_num = 0; > + return; > + } > + > + if (init) { > + for (i = 0; i < pdata->clk_num; i++) { > + if (pdata->clk_name[i] != NULL) { > + mcam->clk[i] = devm_clk_get(mcam->dev, > + pdata->clk_name[i]); Sorry, no. Passing clock names in platform data doesn't look right to me. Clock names are a property of the consumer device, not of clock supplier. Also, your platform tells you to get clk_num clocks, you fail to get one of them, you don't continue trying the rest and just return with no error. This seems strange, usually a failure to get clocks, that the platform tells you to get, is fatal. > + if (IS_ERR(mcam->clk[i])) { > + dev_err(mcam->dev, > + "Could not get clk: %s\n", > + pdata->clk_name[i]); > + mcam->clk_num = 0; > + return; > + } > + } > + } > + mcam->clk_num = pdata->clk_num; > + } else > + mcam->clk_num = 0; > +} > > static int mmpcam_probe(struct platform_device *pdev) > { Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Guennadi >-----Original Message----- >From: Guennadi Liakhovetski [mailto:g.liakhovetski@gmx.de] >Sent: Wednesday, 02 January, 2013 00:06 >To: Albert Wang >Cc: corbet@lwn.net; linux-media@vger.kernel.org; Libin Yang >Subject: Re: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for marvell- >ccic driver > >On Sat, 15 Dec 2012, Albert Wang wrote: > >> From: Libin Yang <lbyang@marvell.com> >> >> This patch adds the clock tree support for marvell-ccic. >> >> Each board may require different clk enabling sequence. >> Developer need add the clk_name in correct sequence in board driver >> to use this feature. >> >> Signed-off-by: Libin Yang <lbyang@marvell.com> >> Signed-off-by: Albert Wang <twang13@marvell.com> >> --- >> drivers/media/platform/marvell-ccic/mcam-core.h | 4 ++ >> drivers/media/platform/marvell-ccic/mmp-driver.c | 57 +++++++++++++++++++++- >> include/media/mmp-camera.h | 5 ++ >> 3 files changed, 65 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h >b/drivers/media/platform/marvell-ccic/mcam-core.h >> index ca63010..86e634e 100755 >> --- a/drivers/media/platform/marvell-ccic/mcam-core.h >> +++ b/drivers/media/platform/marvell-ccic/mcam-core.h >> @@ -88,6 +88,7 @@ struct mcam_frame_state { >> * the dev_lock spinlock; they are marked as such by comments. >> * dev_lock is also required for access to device registers. >> */ >> +#define NR_MCAM_CLK 4 >> struct mcam_camera { >> /* >> * These fields should be set by the platform code prior to >> @@ -109,6 +110,9 @@ struct mcam_camera { >> int lane; /* lane number */ >> >> struct clk *pll1; >> + /* clock tree support */ >> + struct clk *clk[NR_MCAM_CLK]; >> + int clk_num; >> >> /* >> * Callbacks from the core to the platform code. >> diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c >b/drivers/media/platform/marvell-ccic/mmp-driver.c >> index 603fa0a..2c4dce3 100755 >> --- a/drivers/media/platform/marvell-ccic/mmp-driver.c >> +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c >> @@ -104,6 +104,23 @@ static struct mmp_camera *mmpcam_find_device(struct >platform_device *pdev) >> #define REG_CCIC_DCGCR 0x28 /* CCIC dyn clock gate ctrl reg */ >> #define REG_CCIC_CRCR 0x50 /* CCIC clk reset ctrl reg */ >> >> +static void mcam_clk_set(struct mcam_camera *mcam, int on) > >Personally, I would also make two functions out of this - "on" and "off," >but that's a matter of taste, perhaps. > [Albert Wang] That's we have planned to do in next version. >> +{ >> + unsigned int i; >> + >> + if (on) { >> + for (i = 0; i < mcam->clk_num; i++) { >> + if (mcam->clk[i]) >> + clk_enable(mcam->clk[i]); >> + } >> + } else { >> + for (i = mcam->clk_num; i > 0; i--) { >> + if (mcam->clk[i - 1]) >> + clk_disable(mcam->clk[i - 1]); >> + } >> + } >> +} >> + >> /* >> * Power control. >> */ >> @@ -134,6 +151,8 @@ static void mmpcam_power_up(struct mcam_camera *mcam) >> mdelay(5); >> gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */ >> mdelay(5); >> + >> + mcam_clk_set(mcam, 1); >> } >> >> static void mmpcam_power_down(struct mcam_camera *mcam) >> @@ -151,6 +170,8 @@ static void mmpcam_power_down(struct mcam_camera >*mcam) >> pdata = cam->pdev->dev.platform_data; >> gpio_set_value(pdata->sensor_power_gpio, 0); >> gpio_set_value(pdata->sensor_reset_gpio, 0); >> + >> + mcam_clk_set(mcam, 0); >> } >> >> /* >> @@ -202,7 +223,7 @@ void mmpcam_calc_dphy(struct mcam_camera *mcam) >> * pll1 will never be changed, it is a fixed value >> */ >> >> - if (IS_ERR(mcam->pll1)) >> + if (IS_ERR_OR_NULL(mcam->pll1)) > >Why are you changing this? If this really were needed, you should do this >already in the previous patch, where you add these lines. But I don't >think this is a good idea, don't think Russell would like this :-) NULL is >a valid clock. Only a negative error is a failure. In fact, if you like, >you could initialise .pll1 to ERR_PTR(-EINVAL) in your previous patch in >mmpcam_probe(). > [Albert Wang] We will double check it if it's our mistake. Thanks. >> return; >> >> tx_clk_esc = clk_get_rate(mcam->pll1) / 1000000 / 12; >> @@ -229,6 +250,35 @@ static irqreturn_t mmpcam_irq(int irq, void *data) >> return IRQ_RETVAL(handled); >> } >> >> +static void mcam_init_clk(struct mcam_camera *mcam, >> + struct mmp_camera_platform_data *pdata, int init) > >I don't think this "int init" makes sense. Please, remove the parameter >and you actually don't need the de-initialisation, no reason to set >num_clk = 0 before freeing memory. > [Albert Wang] OK, we will double check it. >> +{ >> + unsigned int i; >> + >> + if (NR_MCAM_CLK < pdata->clk_num) { >> + dev_err(mcam->dev, "Too many mcam clocks defined\n"); >> + mcam->clk_num = 0; >> + return; >> + } >> + >> + if (init) { >> + for (i = 0; i < pdata->clk_num; i++) { >> + if (pdata->clk_name[i] != NULL) { >> + mcam->clk[i] = devm_clk_get(mcam->dev, >> + pdata->clk_name[i]); > >Sorry, no. Passing clock names in platform data doesn't look right to me. >Clock names are a property of the consumer device, not of clock supplier. >Also, your platform tells you to get clk_num clocks, you fail to get one >of them, you don't continue trying the rest and just return with no error. >This seems strange, usually a failure to get clocks, that the platform >tells you to get, is fatal. > [Albert Wang] We will update it. >> + if (IS_ERR(mcam->clk[i])) { >> + dev_err(mcam->dev, >> + "Could not get clk: %s\n", >> + pdata->clk_name[i]); >> + mcam->clk_num = 0; >> + return; >> + } >> + } >> + } >> + mcam->clk_num = pdata->clk_num; >> + } else >> + mcam->clk_num = 0; >> +} >> >> static int mmpcam_probe(struct platform_device *pdev) >> { > >Thanks >Guennadi >--- >Guennadi Liakhovetski, Ph.D. >Freelance Open-Source Software Developer >http://www.open-technology.de/ Thanks Albert Wang 86-21-61092656 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2012-12-16 22:51, Albert Wang wrote: [...] >>> >>> +static void mcam_clk_set(struct mcam_camera *mcam, int on) >>> +{ >>> + unsigned int i; >>> + >>> + if (on) { >>> + for (i = 0; i < mcam->clk_num; i++) { >>> + if (mcam->clk[i]) >>> + clk_enable(mcam->clk[i]); >>> + } >>> + } else { >>> + for (i = mcam->clk_num; i > 0; i--) { >>> + if (mcam->clk[i - 1]) >>> + clk_disable(mcam->clk[i - 1]); >>> + } >>> + } >>> +} >> >> A couple of minor comments: >> >> - This function is always called with a constant value for "on". It would >> be easier to read (and less prone to unfortunate brace errors) if it >> were just two functions: mcam_clk_enable() and mcam_clk_disable(). >> > [Albert Wang] OK, that's fine to split it to 2 functions. :) > >> - I'd write the second for loop as: >> >> for (i = mcal->clk_num - 1; i >= 0; i==) { >> >> just to match the values used in the other direction and avoid the >> subscript arithmetic. >> > [Albert Wang] Yes, we can improve it. :) Bewar: i is unsigned so testing i >= 0 will loop forever. [...]-- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Nicolas Thank you for your reminder. :) Happy New Year! >-----Original Message----- >From: Nicolas THERY [mailto:nicolas.thery@st.com] >Sent: Friday, 04 January, 2013 01:38 >To: Albert Wang >Cc: Jonathan Corbet; g.liakhovetski@gmx.de; linux-media@vger.kernel.org; Libin Yang >Subject: Re: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for marvell- >ccic driver > > > >On 2012-12-16 22:51, Albert Wang wrote: >[...] >>>> >>>> +static void mcam_clk_set(struct mcam_camera *mcam, int on) >>>> +{ >>>> + unsigned int i; >>>> + >>>> + if (on) { >>>> + for (i = 0; i < mcam->clk_num; i++) { >>>> + if (mcam->clk[i]) >>>> + clk_enable(mcam->clk[i]); >>>> + } >>>> + } else { >>>> + for (i = mcam->clk_num; i > 0; i--) { >>>> + if (mcam->clk[i - 1]) >>>> + clk_disable(mcam->clk[i - 1]); >>>> + } >>>> + } >>>> +} >>> >>> A couple of minor comments: >>> >>> - This function is always called with a constant value for "on". It would >>> be easier to read (and less prone to unfortunate brace errors) if it >>> were just two functions: mcam_clk_enable() and mcam_clk_disable(). >>> >> [Albert Wang] OK, that's fine to split it to 2 functions. :) >> >>> - I'd write the second for loop as: >>> >>> for (i = mcal->clk_num - 1; i >= 0; i==) { >>> >>> just to match the values used in the other direction and avoid the >>> subscript arithmetic. >>> >> [Albert Wang] Yes, we can improve it. :) > >Bewar: i is unsigned so testing i >= 0 will loop forever. > [Albert Wang] Yes, it looks my original code can work. :) >[...] Thanks Albert Wang 86-21-61092656 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, Thanks for your review. Please see my comments below. >-----Original Message----- >From: Guennadi Liakhovetski [mailto:g.liakhovetski@gmx.de] >Sent: Wednesday, January 02, 2013 12:06 AM >To: Albert Wang >Cc: corbet@lwn.net; linux-media@vger.kernel.org; Libin Yang >Subject: Re: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for >marvell-ccic driver > >On Sat, 15 Dec 2012, Albert Wang wrote: > >> From: Libin Yang <lbyang@marvell.com> >> >> This patch adds the clock tree support for marvell-ccic. >> >> Each board may require different clk enabling sequence. >> Developer need add the clk_name in correct sequence in board driver >> to use this feature. >> >> Signed-off-by: Libin Yang <lbyang@marvell.com> >> Signed-off-by: Albert Wang <twang13@marvell.com> >> --- >> drivers/media/platform/marvell-ccic/mcam-core.h | 4 ++ >> drivers/media/platform/marvell-ccic/mmp-driver.c | 57 +++++++++++++++++++++- >> include/media/mmp-camera.h | 5 ++ >> 3 files changed, 65 insertions(+), 1 deletion(-) >> [snip] >> diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c >b/drivers/media/platform/marvell-ccic/mmp-driver.c >> index 603fa0a..2c4dce3 100755 >> --- a/drivers/media/platform/marvell-ccic/mmp-driver.c >> +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c [snip] >> + >> + mcam_clk_set(mcam, 0); >> } >> >> /* >> @@ -202,7 +223,7 @@ void mmpcam_calc_dphy(struct mcam_camera *mcam) >> * pll1 will never be changed, it is a fixed value >> */ >> >> - if (IS_ERR(mcam->pll1)) >> + if (IS_ERR_OR_NULL(mcam->pll1)) > >Why are you changing this? If this really were needed, you should do this >already in the previous patch, where you add these lines. But I don't >think this is a good idea, don't think Russell would like this :-) NULL is >a valid clock. Only a negative error is a failure. In fact, if you like, >you could initialise .pll1 to ERR_PTR(-EINVAL) in your previous patch in >mmpcam_probe(). In the below code, we will use platform related clk_get_rate() to get the rate. In the function we do not judge the clk is NULL or not. If we do not judge here, we need judge for NULL in the later, otherwise, error may happen. Or do you think it is better that we should judge the pointer in the function clk_get_rate()? > >> return; >> >> tx_clk_esc = clk_get_rate(mcam->pll1) / 1000000 / 12; >> @@ -229,6 +250,35 @@ static irqreturn_t mmpcam_irq(int irq, void *data) >> return IRQ_RETVAL(handled); >> } >> >> +static void mcam_init_clk(struct mcam_camera *mcam, >> + struct mmp_camera_platform_data *pdata, int init) > >I don't think this "int init" makes sense. Please, remove the parameter >and you actually don't need the de-initialisation, no reason to set >num_clk = 0 before freeing memory. Yes, the init parameter is not good here, which make confusion. The driver de-initiatives the clks because I want to make sure the driver will never enable the clks after de-initialization. After second consideration based on your suggestion, I will remove de-initialization, because this scenario will never happen. > >> +{ >> + unsigned int i; >> + >> + if (NR_MCAM_CLK < pdata->clk_num) { >> + dev_err(mcam->dev, "Too many mcam clocks defined\n"); >> + mcam->clk_num = 0; >> + return; >> + } >> + >> + if (init) { >> + for (i = 0; i < pdata->clk_num; i++) { >> + if (pdata->clk_name[i] != NULL) { >> + mcam->clk[i] = devm_clk_get(mcam->dev, >> + pdata->clk_name[i]); > >Sorry, no. Passing clock names in platform data doesn't look right to me. >Clock names are a property of the consumer device, not of clock supplier. >Also, your platform tells you to get clk_num clocks, you fail to get one >of them, you don't continue trying the rest and just return with no error. >This seems strange, usually a failure to get clocks, that the platform >tells you to get, is fatal. I agree that after failing to get the clk, we should return error instead of just returning. For the clock names, the clock names are different on different platforms. So we need platform data passing the clock names. Do you have any suggestions? > >> + if (IS_ERR(mcam->clk[i])) { >> + dev_err(mcam->dev, >> + "Could not get clk: %s\n", >> + pdata->clk_name[i]); >> + mcam->clk_num = 0; >> + return; >> + } >> + } >> + } >> + mcam->clk_num = pdata->clk_num; >> + } else >> + mcam->clk_num = 0; >> +} >> >> static int mmpcam_probe(struct platform_device *pdev) >> { > >Thanks >Guennadi >--- >Guennadi Liakhovetski, Ph.D. >Freelance Open-Source Software Developer >http://www.open-technology.de/ Regards, Libin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Libin On Thu, 3 Jan 2013, Libin Yang wrote: > Hi Guennadi, > > Thanks for your review. Please see my comments below. > > >-----Original Message----- > >From: Guennadi Liakhovetski [mailto:g.liakhovetski@gmx.de] > >Sent: Wednesday, January 02, 2013 12:06 AM > >To: Albert Wang > >Cc: corbet@lwn.net; linux-media@vger.kernel.org; Libin Yang > >Subject: Re: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for > >marvell-ccic driver > > > >On Sat, 15 Dec 2012, Albert Wang wrote: > > > >> From: Libin Yang <lbyang@marvell.com> > >> > >> This patch adds the clock tree support for marvell-ccic. > >> > >> Each board may require different clk enabling sequence. > >> Developer need add the clk_name in correct sequence in board driver > >> to use this feature. > >> > >> Signed-off-by: Libin Yang <lbyang@marvell.com> > >> Signed-off-by: Albert Wang <twang13@marvell.com> > >> --- > >> drivers/media/platform/marvell-ccic/mcam-core.h | 4 ++ > >> drivers/media/platform/marvell-ccic/mmp-driver.c | 57 +++++++++++++++++++++- > >> include/media/mmp-camera.h | 5 ++ > >> 3 files changed, 65 insertions(+), 1 deletion(-) > >> > [snip] > > >> diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c > >b/drivers/media/platform/marvell-ccic/mmp-driver.c > >> index 603fa0a..2c4dce3 100755 > >> --- a/drivers/media/platform/marvell-ccic/mmp-driver.c > >> +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c > [snip] > > >> + > >> + mcam_clk_set(mcam, 0); > >> } > >> > >> /* > >> @@ -202,7 +223,7 @@ void mmpcam_calc_dphy(struct mcam_camera *mcam) > >> * pll1 will never be changed, it is a fixed value > >> */ > >> > >> - if (IS_ERR(mcam->pll1)) > >> + if (IS_ERR_OR_NULL(mcam->pll1)) > > > >Why are you changing this? If this really were needed, you should do this > >already in the previous patch, where you add these lines. But I don't > >think this is a good idea, don't think Russell would like this :-) NULL is > >a valid clock. Only a negative error is a failure. In fact, if you like, > >you could initialise .pll1 to ERR_PTR(-EINVAL) in your previous patch in > >mmpcam_probe(). > > In the below code, we will use platform related clk_get_rate() to get the rate. > In the function we do not judge the clk is NULL or not. If we do not judge here, > we need judge for NULL in the later, otherwise, error may happen. Or do you > think it is better that we should judge the pointer in the function clk_get_rate()? I think, there is a problem here. Firstly, if you really want to check for "clock API not supported" or a similar type of condition by checking get_clk() return value for NULL, you should do this immediately in the patch, where you add this code: in "[PATCH V3 02/15] [media] marvell-ccic: add MIPI support for marvell-ccic driver." Secondly, it's probably ok to check this to say - no clock, co reason to try to use it, in which case you skip calculating your ->dphy[2] value, and it remains == 0, presumably, is this what you want to have? But, I think, there's a bigger problem in your patch #02/15: you don't check for mcam->dphy != NULL. So, I think, this has to be fixed in that patch, not here. [snip] > >> +{ > >> + unsigned int i; > >> + > >> + if (NR_MCAM_CLK < pdata->clk_num) { > >> + dev_err(mcam->dev, "Too many mcam clocks defined\n"); > >> + mcam->clk_num = 0; > >> + return; > >> + } > >> + > >> + if (init) { > >> + for (i = 0; i < pdata->clk_num; i++) { > >> + if (pdata->clk_name[i] != NULL) { > >> + mcam->clk[i] = devm_clk_get(mcam->dev, > >> + pdata->clk_name[i]); > > > >Sorry, no. Passing clock names in platform data doesn't look right to me. > >Clock names are a property of the consumer device, not of clock supplier. > >Also, your platform tells you to get clk_num clocks, you fail to get one > >of them, you don't continue trying the rest and just return with no error. > >This seems strange, usually a failure to get clocks, that the platform > >tells you to get, is fatal. > > I agree that after failing to get the clk, we should return error > instead of just returning. > > For the clock names, the clock names are different on different platforms. > So we need platform data passing the clock names. Do you have any suggestions? I think you should use the same names on all platforms. As I said, those are names of _consumer_ clocks, not of supplier. And the consumer on all platforms is the same - your camera unit. If you cannot remove existing clock entries for compatibility reasons you, probably, can just add clock lookup entries for them. In the _very_ worst case, maybe make a table of clock names and, depending on the SoC type use one of them, but that's really also not very apropriate, not sure, whether that would be accepted. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer http://www.open-technology.de/ -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Guennadi, Please see my comments below. >-----Original Message----- >From: Guennadi Liakhovetski [mailto:g.liakhovetski@gmx.de] >Sent: Friday, January 04, 2013 6:25 PM >To: Libin Yang >Cc: Albert Wang; corbet@lwn.net; linux-media@vger.kernel.org >Subject: RE: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for >marvell-ccic driver > >Hi Libin > >On Thu, 3 Jan 2013, Libin Yang wrote: > >> Hi Guennadi, >> >> Thanks for your review. Please see my comments below. >> [snip] >> >> /* >> >> @@ -202,7 +223,7 @@ void mmpcam_calc_dphy(struct mcam_camera *mcam) >> >> * pll1 will never be changed, it is a fixed value >> >> */ >> >> >> >> - if (IS_ERR(mcam->pll1)) >> >> + if (IS_ERR_OR_NULL(mcam->pll1)) >> > >> >Why are you changing this? If this really were needed, you should do this >> >already in the previous patch, where you add these lines. But I don't >> >think this is a good idea, don't think Russell would like this :-) NULL is >> >a valid clock. Only a negative error is a failure. In fact, if you like, >> >you could initialise .pll1 to ERR_PTR(-EINVAL) in your previous patch in >> >mmpcam_probe(). >> >> In the below code, we will use platform related clk_get_rate() to get the rate. >> In the function we do not judge the clk is NULL or not. If we do not judge here, >> we need judge for NULL in the later, otherwise, error may happen. Or do you >> think it is better that we should judge the pointer in the function clk_get_rate()? > >I think, there is a problem here. Firstly, if you really want to check for >"clock API not supported" or a similar type of condition by checking >get_clk() return value for NULL, you should do this immediately in the >patch, where you add this code: in "[PATCH V3 02/15] [media] marvell-ccic: >add MIPI support for marvell-ccic driver." Secondly, it's probably ok to >check this to say - no clock, co reason to try to use it, in which case >you skip calculating your ->dphy[2] value, and it remains == 0, >presumably, is this what you want to have? But, I think, there's a bigger >problem in your patch #02/15: you don't check for mcam->dphy != NULL. So, >I think, this has to be fixed in that patch, not here. Your understanding is right. We will try to use the default value if the pll1 is not available. So we just return if pll1 is error or NULL. And mostly the default value should work. And this reminds me that there is a little issue: we should not return fail in the mcam_v4l_open() function when failing to get the pll1 clk because we can also use the default values. What I plan to code in the next version is: 1. Remove the judgement of (IS_ERR(cam->pll1)) in the open function when get the clk. 2. Still use IS_ERR_OR_NULL(mcam->pll1), so that we can use the default vaule if pll1 is not available. What do you think of it? mcam->dphy != NULL may be a critical issue. Thanks for digging out this bug. We will fix it in the next version. > >[snip] > >> >> +{ >> >> + unsigned int i; >> >> + >> >> + if (NR_MCAM_CLK < pdata->clk_num) { >> >> + dev_err(mcam->dev, "Too many mcam clocks defined\n"); >> >> + mcam->clk_num = 0; >> >> + return; >> >> + } >> >> + >> >> + if (init) { >> >> + for (i = 0; i < pdata->clk_num; i++) { >> >> + if (pdata->clk_name[i] != NULL) { >> >> + mcam->clk[i] = devm_clk_get(mcam->dev, >> >> + pdata->clk_name[i]); >> > >> >Sorry, no. Passing clock names in platform data doesn't look right to me. >> >Clock names are a property of the consumer device, not of clock supplier. >> >Also, your platform tells you to get clk_num clocks, you fail to get one >> >of them, you don't continue trying the rest and just return with no error. >> >This seems strange, usually a failure to get clocks, that the platform >> >tells you to get, is fatal. >> >> I agree that after failing to get the clk, we should return error >> instead of just returning. >> >> For the clock names, the clock names are different on different platforms. >> So we need platform data passing the clock names. Do you have any suggestions? > >I think you should use the same names on all platforms. As I said, those >are names of _consumer_ clocks, not of supplier. And the consumer on all >platforms is the same - your camera unit. If you cannot remove existing >clock entries for compatibility reasons you, probably, can just add clock >lookup entries for them. In the _very_ worst case, maybe make a table of >clock names and, depending on the SoC type use one of them, but that's >really also not very apropriate, not sure, whether that would be accepted. OK, I see. I will use the names directly in the camera driver without getting them from the platform data. > >Thanks >Guennadi >--- >Guennadi Liakhovetski, Ph.D. >Freelance Open-Source Software Developer >http://www.open-technology.de/ Regards, Libin -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h index ca63010..86e634e 100755 --- a/drivers/media/platform/marvell-ccic/mcam-core.h +++ b/drivers/media/platform/marvell-ccic/mcam-core.h @@ -88,6 +88,7 @@ struct mcam_frame_state { * the dev_lock spinlock; they are marked as such by comments. * dev_lock is also required for access to device registers. */ +#define NR_MCAM_CLK 4 struct mcam_camera { /* * These fields should be set by the platform code prior to @@ -109,6 +110,9 @@ struct mcam_camera { int lane; /* lane number */ struct clk *pll1; + /* clock tree support */ + struct clk *clk[NR_MCAM_CLK]; + int clk_num; /* * Callbacks from the core to the platform code. diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c index 603fa0a..2c4dce3 100755 --- a/drivers/media/platform/marvell-ccic/mmp-driver.c +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c @@ -104,6 +104,23 @@ static struct mmp_camera *mmpcam_find_device(struct platform_device *pdev) #define REG_CCIC_DCGCR 0x28 /* CCIC dyn clock gate ctrl reg */ #define REG_CCIC_CRCR 0x50 /* CCIC clk reset ctrl reg */ +static void mcam_clk_set(struct mcam_camera *mcam, int on) +{ + unsigned int i; + + if (on) { + for (i = 0; i < mcam->clk_num; i++) { + if (mcam->clk[i]) + clk_enable(mcam->clk[i]); + } + } else { + for (i = mcam->clk_num; i > 0; i--) { + if (mcam->clk[i - 1]) + clk_disable(mcam->clk[i - 1]); + } + } +} + /* * Power control. */ @@ -134,6 +151,8 @@ static void mmpcam_power_up(struct mcam_camera *mcam) mdelay(5); gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */ mdelay(5); + + mcam_clk_set(mcam, 1); } static void mmpcam_power_down(struct mcam_camera *mcam) @@ -151,6 +170,8 @@ static void mmpcam_power_down(struct mcam_camera *mcam) pdata = cam->pdev->dev.platform_data; gpio_set_value(pdata->sensor_power_gpio, 0); gpio_set_value(pdata->sensor_reset_gpio, 0); + + mcam_clk_set(mcam, 0); } /* @@ -202,7 +223,7 @@ void mmpcam_calc_dphy(struct mcam_camera *mcam) * pll1 will never be changed, it is a fixed value */ - if (IS_ERR(mcam->pll1)) + if (IS_ERR_OR_NULL(mcam->pll1)) return; tx_clk_esc = clk_get_rate(mcam->pll1) / 1000000 / 12; @@ -229,6 +250,35 @@ static irqreturn_t mmpcam_irq(int irq, void *data) return IRQ_RETVAL(handled); } +static void mcam_init_clk(struct mcam_camera *mcam, + struct mmp_camera_platform_data *pdata, int init) +{ + unsigned int i; + + if (NR_MCAM_CLK < pdata->clk_num) { + dev_err(mcam->dev, "Too many mcam clocks defined\n"); + mcam->clk_num = 0; + return; + } + + if (init) { + for (i = 0; i < pdata->clk_num; i++) { + if (pdata->clk_name[i] != NULL) { + mcam->clk[i] = devm_clk_get(mcam->dev, + pdata->clk_name[i]); + if (IS_ERR(mcam->clk[i])) { + dev_err(mcam->dev, + "Could not get clk: %s\n", + pdata->clk_name[i]); + mcam->clk_num = 0; + return; + } + } + } + mcam->clk_num = pdata->clk_num; + } else + mcam->clk_num = 0; +} static int mmpcam_probe(struct platform_device *pdev) { @@ -293,6 +343,8 @@ static int mmpcam_probe(struct platform_device *pdev) ret = -ENODEV; goto out_unmap1; } + + mcam_init_clk(mcam, pdata, 1); /* * Find the i2c adapter. This assumes, of course, that the * i2c bus is already up and functioning. @@ -320,6 +372,7 @@ static int mmpcam_probe(struct platform_device *pdev) goto out_gpio; } gpio_direction_output(pdata->sensor_reset_gpio, 0); + /* * Power the device up and hand it off to the core. */ @@ -352,6 +405,7 @@ out_gpio2: out_gpio: gpio_free(pdata->sensor_power_gpio); out_unmap2: + mcam_init_clk(mcam, pdata, 0); iounmap(cam->power_regs); out_unmap1: iounmap(mcam->regs); @@ -375,6 +429,7 @@ static int mmpcam_remove(struct mmp_camera *cam) gpio_free(pdata->sensor_power_gpio); iounmap(cam->power_regs); iounmap(mcam->regs); + mcam_init_clk(mcam, pdata, 0); kfree(cam); return 0; } diff --git a/include/media/mmp-camera.h b/include/media/mmp-camera.h index 813efe2..c339d43 100755 --- a/include/media/mmp-camera.h +++ b/include/media/mmp-camera.h @@ -16,4 +16,9 @@ struct mmp_camera_platform_data { int mipi_enabled; /* MIPI enabled flag */ int lane; /* ccic used lane number; 0 means DVP mode */ int lane_clk; + /* + * clock tree support + */ + char *clk_name[4]; + int clk_num; };