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.
-Original Message-
From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
Sent: Wednesday, January 02, 2013 12:06 AM
To: Albert Wang
Cc: cor...@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 lby...@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 lby...@marvell.com
Signed-off-by: Albert Wang twan...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo
RE: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
Hi Guennadi,
Please see my comments below.
-Original Message-
From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
Sent: Friday, January 04, 2013 6:25 PM
To: Libin Yang
Cc: Albert Wang; cor...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
[...]--
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 V3 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
Hi, Nicolas
Thank you for your reminder. :)
Happy New Year!
-Original Message-
From: Nicolas THERY [mailto:nicolas.th...@st.com]
Sent: Friday, 04 January, 2013 01:38
To: Albert Wang
Cc: Jonathan Corbet; g.liakhovet...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
Hi Guennadi,
Thanks for your review. Please see my comments below.
-Original Message-
From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
Sent: Wednesday, January 02, 2013 12:06 AM
To: Albert Wang
Cc: cor...@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 lby...@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 lby...@marvell.com
Signed-off-by: Albert Wang twan...@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) / 100 / 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 majord
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 lby...@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 lby...@marvell.com
Signed-off-by: Albert Wang twan...@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_CRCR0x50/* 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) / 100 / 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
RE: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
Hi, Guennadi
-Original Message-
From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
Sent: Wednesday, 02 January, 2013 00:06
To: Albert Wang
Cc: cor...@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 lby...@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 lby...@marvell.com
Signed-off-by: Albert Wang twan...@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) / 100 / 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
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 twan...@marvell.com wrote:
From: Libin Yang lby...@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 cor...@lwn.net
jon
--
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 V3 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
Hi, Jonathan
-Original Message-
From: Jonathan Corbet [mailto:cor...@lwn.net]
Sent: Monday, 17 December, 2012 00:03
To: Albert Wang
Cc: g.liakhovet...@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 twan...@marvell.com wrote:
From: Libin Yang lby...@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 cor...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH V3 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
From: Libin Yang lby...@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 lby...@marvell.com
Signed-off-by: Albert Wang twan...@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)
+{
+ 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) / 100 / 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
