RE: [PATCH 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
Hi Libin
On Tue, 27 Nov 2012, Libin Yang wrote:
> Hello Guennadi,
>
> Thanks for your suggestion, please see my comments below.
>
> Best Regards,
> Libin
>
> >>-Original Message-
> >>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
> >>Sent: Tuesday, 27 November, 2012 18:50
> >>To: Albert Wang
> >>Cc: cor...@lwn.net; linux-media@vger.kernel.org; Libin Yang
> >>Subject: Re: [PATCH 03/15] [media] marvell-ccic: add clock tree support for
> >>marvell-ccic
> >>driver
> >>
> >>> + mcam->clk_num = pdata->clk_num;
> >>> + } else {
> >>> + for (i = 0; i < pdata->clk_num; i++) {
> >>> + if (mcam->clk[i]) {
> >>> + clk_put(mcam->clk[i]);
> >>> + mcam->clk[i] = NULL;
> >>> + }
> >>> + }
> >>> + mcam->clk_num = 0;
> >>> + }
> >>> +}
> >>
> >>Don't think I like this. IIUC, your driver should only try to use clocks,
> >>that it knows about,
> >>not some random clocks, passed from the platform data. So, you should be
> >>using explicit
> >>clock names. In your platform data you can set whether a specific clock
> >>should be used or
> >>not, but not pass clock names down. Also you might want to consider using
> >>devm_clk_get()
> >>and be more careful with error handling.
> >>
> >OK, we will try to enhance it.
>
> [Libin] Because there are some boards using mmp chip, and the clock
> names on different board may be totally different. And also this is why
> the clock number is not definite. To support more boards, the dynamic
> names are used instead of the static names.
No, I don't think it's right. The clock connection ID is the ID of the
clock _consumer_, not the clock provider. So, your camera IP block has
several clock inputs, and your platforms should provide clock lookup
entries with names of those clock _inputs_, not of their clock sources.
BTW, I really doubt it your camera block has 4 clock inputs? If some of
them are parents of the clocks, that really supply the block (which would
also explain why you call it a tree), then you don't have to clk_get()
them explicitly. The clock framework will refcount and enable those parent
clocks for you. So, I think, you really should fix your platforms.
This has been discussed multiple times on the mailing lists, feel free to
do some research, here one link:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/131302/focus=37730
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-info.html
RE: [PATCH 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
Hello Guennadi,
Thanks for your suggestion, please see my comments below.
Best Regards,
Libin
>>-Original Message-
>>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
>>Sent: Tuesday, 27 November, 2012 18:50
>>To: Albert Wang
>>Cc: cor...@lwn.net; linux-media@vger.kernel.org; Libin Yang
>>Subject: Re: [PATCH 03/15] [media] marvell-ccic: add clock tree support for
>>marvell-ccic
>>driver
>>
>>> + mcam->clk_num = pdata->clk_num;
>>> + } else {
>>> + for (i = 0; i < pdata->clk_num; i++) {
>>> + if (mcam->clk[i]) {
>>> + clk_put(mcam->clk[i]);
>>> + mcam->clk[i] = NULL;
>>> + }
>>> + }
>>> + mcam->clk_num = 0;
>>> + }
>>> +}
>>
>>Don't think I like this. IIUC, your driver should only try to use clocks,
>>that it knows about,
>>not some random clocks, passed from the platform data. So, you should be
>>using explicit
>>clock names. In your platform data you can set whether a specific clock
>>should be used or
>>not, but not pass clock names down. Also you might want to consider using
>>devm_clk_get()
>>and be more careful with error handling.
>>
>OK, we will try to enhance it.
[Libin] Because there are some boards using mmp chip, and the clock names on
different board may be totally different. And also this is why the clock number
is not definite. To support more boards, the dynamic names are used instead of
the static names.
>
>>>
>>> static int mmpcam_probe(struct platform_device *pdev) { @@ -290,6
--
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 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
Hi, Guennadi
We will update it soon.
>-Original Message-
>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
>Sent: Tuesday, 27 November, 2012 18:50
>To: Albert Wang
>Cc: cor...@lwn.net; linux-media@vger.kernel.org; Libin Yang
>Subject: Re: [PATCH 03/15] [media] marvell-ccic: add clock tree support for
>marvell-ccic
>driver
>
>On Fri, 23 Nov 2012, Albert Wang wrote:
>
>> From: Libin Yang
>>
>> 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
>> Signed-off-by: Albert Wang
>> ---
>> drivers/media/platform/marvell-ccic/mcam-core.h |6 +++
>> drivers/media/platform/marvell-ccic/mmp-driver.c | 57
>> ++
>> include/media/mmp-camera.h |5 ++
>> 3 files changed, 68 insertions(+)
>>
>> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h
>> b/drivers/media/platform/marvell-ccic/mcam-core.h
>> index 2d444a1..0df6b1c 100755
>> --- a/drivers/media/platform/marvell-ccic/mcam-core.h
>> +++ b/drivers/media/platform/marvell-ccic/mcam-core.h
>> @@ -88,6 +88,7 @@ struct mmp_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 @@
>> -107,6 +108,11 @@ struct mcam_camera {
>> int (*dphy)[3];
>> int mipi_enabled;
>> int lane; /* lane number */
>> +
>> +/* 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 9d7aa79..80977b0 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])
>
>From your init below, mcam->clk[i] can be a negative error code.
>
Yes. We will fix it.
>> +clk_enable(mcam->clk[i]);
>> +}
>> +} else {
>> +for (i = 0; i < mcam->clk_num; i++) {
>> +if (mcam->clk[i])
>> +clk_disable(mcam->clk[i]);
>> +}
>> +}
>> +}
>> +
>> /*
>> * 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);
>> }
>>
>> /*
>> @@ -229,6 +250,37 @@ 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_warn(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]
Re: [PATCH 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
On Fri, 23 Nov 2012, Albert Wang wrote:
> From: Libin Yang
>
> 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
> Signed-off-by: Albert Wang
> ---
> drivers/media/platform/marvell-ccic/mcam-core.h |6 +++
> drivers/media/platform/marvell-ccic/mmp-driver.c | 57
> ++
> include/media/mmp-camera.h |5 ++
> 3 files changed, 68 insertions(+)
>
> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h
> b/drivers/media/platform/marvell-ccic/mcam-core.h
> index 2d444a1..0df6b1c 100755
> --- a/drivers/media/platform/marvell-ccic/mcam-core.h
> +++ b/drivers/media/platform/marvell-ccic/mcam-core.h
> @@ -88,6 +88,7 @@ struct mmp_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
> @@ -107,6 +108,11 @@ struct mcam_camera {
> int (*dphy)[3];
> int mipi_enabled;
> int lane; /* lane number */
> +
> + /* 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 9d7aa79..80977b0 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)
> +{
> + unsigned int i;
> +
> + if (on) {
> + for (i = 0; i < mcam->clk_num; i++) {
> + if (mcam->clk[i])
>From your init below, mcam->clk[i] can be a negative error code.
> + clk_enable(mcam->clk[i]);
> + }
> + } else {
> + for (i = 0; i < mcam->clk_num; i++) {
> + if (mcam->clk[i])
> + clk_disable(mcam->clk[i]);
> + }
> + }
> +}
> +
> /*
> * 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);
> }
>
> /*
> @@ -229,6 +250,37 @@ 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_warn(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] = clk_get(mcam->dev,
> + pdata->clk_name[i]);
> + if (IS_ERR(mcam->clk[i]))
> + dev_warn(mcam->dev, "Could not get clk: %s\n",
> + pdata->clk_name[i]);
You issue a warning but continue initialisation, leaving mcam->clk[i] set
to an error value.
> + }
> + mcam->clk_num = pdata->clk_num;
> + } else {
> + for (i = 0; i < pdata->clk_num; i++) {
> + if (mcam->clk[i]) {
> + clk_put(mcam->clk[i]);
> + mcam->clk[i] = NULL;
> + }
> + }
> + mcam->clk_num = 0;
> + }
> +}
Don't think I like this. IIUC, your driver should only try to use clocks,
that it knows about, not some random clocks, passed from the platform
data. So, you should be using explicit clock names. In your platform data
you can set whether a specific clock should be used or not, but not pass
clock names down. Also you might want to consider using devm_
[PATCH 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
From: Libin Yang
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
Signed-off-by: Albert Wang
---
drivers/media/platform/marvell-ccic/mcam-core.h |6 +++
drivers/media/platform/marvell-ccic/mmp-driver.c | 57 ++
include/media/mmp-camera.h |5 ++
3 files changed, 68 insertions(+)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h
b/drivers/media/platform/marvell-ccic/mcam-core.h
index 2d444a1..0df6b1c 100755
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -88,6 +88,7 @@ struct mmp_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
@@ -107,6 +108,11 @@ struct mcam_camera {
int (*dphy)[3];
int mipi_enabled;
int lane; /* lane number */
+
+ /* 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 9d7aa79..80977b0 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 = 0; i < mcam->clk_num; i++) {
+ if (mcam->clk[i])
+ clk_disable(mcam->clk[i]);
+ }
+ }
+}
+
/*
* 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);
}
/*
@@ -229,6 +250,37 @@ 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_warn(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] = clk_get(mcam->dev,
+ pdata->clk_name[i]);
+ if (IS_ERR(mcam->clk[i]))
+ dev_warn(mcam->dev, "Could not get clk: %s\n",
+pdata->clk_name[i]);
+ }
+ mcam->clk_num = pdata->clk_num;
+ } else {
+ for (i = 0; i < pdata->clk_num; i++) {
+ if (mcam->clk[i]) {
+ clk_put(mcam->clk[i]);
+ mcam->clk[i] = NULL;
+ }
+ }
+ mcam->clk_num = 0;
+ }
+}
static int mmpcam_probe(struct platform_device *pdev)
{
@@ -290,6 +342,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.
@@ -317,6 +371,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.
*/
@@ -349,6 +404,7 @@ out_gpio2:
out_gpio:
gpio_free(pdata->sensor_pow
