[PATCH] clk: fix function name in devm_clk_put kernel-doc comment

[PATCH] clk: fix function name in devm_clk_put kernel-doc comment

[Uwe Kleine-König]

The paragraph about clk_put already specifies this restriction about
clk_put.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Hello,

I suspect that devm_clk_put is used more often than necessary. After some
grepping around e.g. looking at drivers/tty/serial/clps711x.c it doesn't
seem necessary to call it as the driver core already cares about calling
the devm cleanup callbacks.
The only reason I see being valid to call devm_clk_put is that a device
continues to be bound and still it's sure that the clk in question isn't
needed anymore. Ah, and maybe to enforce an order during cleanup, but
then there is no reason to use devm, the normal functions would just do
fine. I'm replying to this mail with a few patches.

Best regards
Uwe

 include/linux/clk.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/clk.h b/include/linux/clk.h
index 9a6d045..c713ba0 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -214,7 +214,7 @@ void clk_put(struct clk *clk);
  * clock source are balanced by clk_disable calls prior to calling
  * this function.
  *
- * clk_put should not be called from within interrupt context.
+ * devm_clk_put should not be called from within interrupt context.
  */
 void devm_clk_put(struct device *dev, struct clk *clk);
 
-- 
1.8.4.rc3

[PATCH] serial: clps711x: drop needless devm_clk_put

[Uwe Kleine-König]

The nice thing about devm_* is that the driver doesn't need to free the
resources but the driver core takes care about that.

These calls were introduced in commit c08f015 (serial: clps711x: Using
CPU clock subsystem for getting base UART speed).

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Cc: Alexander Shiyan <shc_work@mail.ru>
Cc: Russell King <linux@arm.linux.org.uk>
---
 drivers/tty/serial/clps711x.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/serial/clps711x.c b/drivers/tty/serial/clps711x.c
index 7e4e408..8d0b994 100644
--- a/drivers/tty/serial/clps711x.c
+++ b/drivers/tty/serial/clps711x.c
@@ -459,7 +459,6 @@ static int uart_clps711x_probe(struct platform_device *pdev)
 	ret = uart_register_driver(&s->uart);
 	if (ret) {
 		dev_err(&pdev->dev, "Registering UART driver failed\n");
-		devm_clk_put(&pdev->dev, s->uart_clk);
 		return ret;
 	}
 
@@ -487,7 +486,6 @@ static int uart_clps711x_remove(struct platform_device *pdev)
 	for (i = 0; i < UART_CLPS711X_NR; i++)
 		uart_remove_one_port(&s->uart, &s->port[i]);
 
-	devm_clk_put(&pdev->dev, s->uart_clk);
 	uart_unregister_driver(&s->uart);
 
 	return 0;
-- 
1.8.4.rc3

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] serial: clps711x: drop needless devm_clk_put

[Uwe Kleine-König]

The nice thing about devm_* is that the driver doesn't need to free the
resources but the driver core takes care about that.

These calls were introduced in commit c08f015 (serial: clps711x: Using
CPU clock subsystem for getting base UART speed).

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Cc: Alexander Shiyan <shc_work@mail.ru>
Cc: Russell King <linux@arm.linux.org.uk>
---
 drivers/tty/serial/clps711x.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/tty/serial/clps711x.c b/drivers/tty/serial/clps711x.c
index 7e4e408..8d0b994 100644
--- a/drivers/tty/serial/clps711x.c
+++ b/drivers/tty/serial/clps711x.c
@@ -459,7 +459,6 @@ static int uart_clps711x_probe(struct platform_device *pdev)
 	ret = uart_register_driver(&s->uart);
 	if (ret) {
 		dev_err(&pdev->dev, "Registering UART driver failed\n");
-		devm_clk_put(&pdev->dev, s->uart_clk);
 		return ret;
 	}
 
@@ -487,7 +486,6 @@ static int uart_clps711x_remove(struct platform_device *pdev)
 	for (i = 0; i < UART_CLPS711X_NR; i++)
 		uart_remove_one_port(&s->uart, &s->port[i]);
 
-	devm_clk_put(&pdev->dev, s->uart_clk);
 	uart_unregister_driver(&s->uart);
 
 	return 0;
-- 
1.8.4.rc3

[PATCH] remoteproc/davinci: drop needless devm_clk_put

[Uwe Kleine-König]

The comment above disable_irq says that it is needed to ensure that the
"devm subsystem might end up releasing things before freeing the irq,
thus allowing an interrupt to sneak in while the device is being
removed." disable_irq is enough for this purpose and there is no need to
manually free the reference to the clock.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Cc: Robert Tivy <rtivy@ti.com>
---
 drivers/remoteproc/da8xx_remoteproc.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/remoteproc/da8xx_remoteproc.c b/drivers/remoteproc/da8xx_remoteproc.c
index 129f7b9..b91cd30 100644
--- a/drivers/remoteproc/da8xx_remoteproc.c
+++ b/drivers/remoteproc/da8xx_remoteproc.c
@@ -301,8 +301,6 @@ static int da8xx_rproc_remove(struct platform_device *pdev)
 	 */
 	disable_irq(drproc->irq);
 
-	devm_clk_put(dev, drproc->dsp_clk);
-
 	rproc_del(rproc);
 	rproc_put(rproc);
 
-- 
1.8.4.rc3

[PATCH] video: mmp: drop needless devm cleanup

[Uwe Kleine-König]

The nice thing about devm_* is that the driver doesn't need to free the
resources but the driver core takes care about that. This also
simplifies the error path quite a bit and removes the wrong check for a
clock pointer being NULL.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/video/mmp/hw/mmp_ctrl.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
index 75dca19..6ac7552 100644
--- a/drivers/video/mmp/hw/mmp_ctrl.c
+++ b/drivers/video/mmp/hw/mmp_ctrl.c
@@ -514,7 +514,7 @@ static int mmphw_probe(struct platform_device *pdev)
 	if (IS_ERR(ctrl->clk)) {
 		dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name);
 		ret = -ENOENT;
-		goto failed_get_clk;
+		goto failed;
 	}
 	clk_prepare_enable(ctrl->clk);
 
@@ -551,21 +551,8 @@ failed_path_init:
 		path_deinit(path_plat);
 	}
 
-	if (ctrl->clk) {
-		devm_clk_put(ctrl->dev, ctrl->clk);
-		clk_disable_unprepare(ctrl->clk);
-	}
-failed_get_clk:
-	devm_free_irq(ctrl->dev, ctrl->irq, ctrl);
+	clk_disable_unprepare(ctrl->clk);
 failed:
-	if (ctrl) {
-		if (ctrl->reg_base)
-			devm_iounmap(ctrl->dev, ctrl->reg_base);
-		devm_release_mem_region(ctrl->dev, res->start,
-				resource_size(res));
-		devm_kfree(ctrl->dev, ctrl);
-	}
-
 	dev_err(&pdev->dev, "device init failed\n");
 
 	return ret;
-- 
1.8.4.rc3

[PATCH] video: mmp: drop needless devm cleanup

[Uwe Kleine-König]

The nice thing about devm_* is that the driver doesn't need to free the
resources but the driver core takes care about that. This also
simplifies the error path quite a bit and removes the wrong check for a
clock pointer being NULL.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
 drivers/video/mmp/hw/mmp_ctrl.c | 17 ++---------------
 1 file changed, 2 insertions(+), 15 deletions(-)

diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
index 75dca19..6ac7552 100644
--- a/drivers/video/mmp/hw/mmp_ctrl.c
+++ b/drivers/video/mmp/hw/mmp_ctrl.c
@@ -514,7 +514,7 @@ static int mmphw_probe(struct platform_device *pdev)
 	if (IS_ERR(ctrl->clk)) {
 		dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name);
 		ret = -ENOENT;
-		goto failed_get_clk;
+		goto failed;
 	}
 	clk_prepare_enable(ctrl->clk);
 
@@ -551,21 +551,8 @@ failed_path_init:
 		path_deinit(path_plat);
 	}
 
-	if (ctrl->clk) {
-		devm_clk_put(ctrl->dev, ctrl->clk);
-		clk_disable_unprepare(ctrl->clk);
-	}
-failed_get_clk:
-	devm_free_irq(ctrl->dev, ctrl->irq, ctrl);
+	clk_disable_unprepare(ctrl->clk);
 failed:
-	if (ctrl) {
-		if (ctrl->reg_base)
-			devm_iounmap(ctrl->dev, ctrl->reg_base);
-		devm_release_mem_region(ctrl->dev, res->start,
-				resource_size(res));
-		devm_kfree(ctrl->dev, ctrl);
-	}
-
 	dev_err(&pdev->dev, "device init failed\n");
 
 	return ret;
-- 
1.8.4.rc3

Re: [PATCH] video: mmp: drop needless devm cleanup

[Russell King - ARM Linux]

On Mon, Sep 23, 2013 at 06:13:10PM +0200, Uwe Kleine-König wrote:
> The nice thing about devm_* is that the driver doesn't need to free the
> resources but the driver core takes care about that. This also
> simplifies the error path quite a bit and removes the wrong check for a
> clock pointer being NULL.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/mmp/hw/mmp_ctrl.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
> index 75dca19..6ac7552 100644
> --- a/drivers/video/mmp/hw/mmp_ctrl.c
> +++ b/drivers/video/mmp/hw/mmp_ctrl.c
> @@ -514,7 +514,7 @@ static int mmphw_probe(struct platform_device *pdev)
>  	if (IS_ERR(ctrl->clk)) {
>  		dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name);
>  		ret = -ENOENT;
> -		goto failed_get_clk;
> +		goto failed;
>  	}
>  	clk_prepare_enable(ctrl->clk);
>  
> @@ -551,21 +551,8 @@ failed_path_init:
>  		path_deinit(path_plat);
>  	}
>  
> -	if (ctrl->clk) {
> -		devm_clk_put(ctrl->dev, ctrl->clk);
> -		clk_disable_unprepare(ctrl->clk);

And this patch also fixes the above: disabling/unpreparing _after_ putting
the thing - which was quite silly... :)

[PATCH] video: mmp: drop needless devm cleanup

[Russell King - ARM Linux]

On Mon, Sep 23, 2013 at 06:13:10PM +0200, Uwe Kleine-K?nig wrote:
> The nice thing about devm_* is that the driver doesn't need to free the
> resources but the driver core takes care about that. This also
> simplifies the error path quite a bit and removes the wrong check for a
> clock pointer being NULL.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/mmp/hw/mmp_ctrl.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
> index 75dca19..6ac7552 100644
> --- a/drivers/video/mmp/hw/mmp_ctrl.c
> +++ b/drivers/video/mmp/hw/mmp_ctrl.c
> @@ -514,7 +514,7 @@ static int mmphw_probe(struct platform_device *pdev)
>  	if (IS_ERR(ctrl->clk)) {
>  		dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name);
>  		ret = -ENOENT;
> -		goto failed_get_clk;
> +		goto failed;
>  	}
>  	clk_prepare_enable(ctrl->clk);
>  
> @@ -551,21 +551,8 @@ failed_path_init:
>  		path_deinit(path_plat);
>  	}
>  
> -	if (ctrl->clk) {
> -		devm_clk_put(ctrl->dev, ctrl->clk);
> -		clk_disable_unprepare(ctrl->clk);

And this patch also fixes the above: disabling/unpreparing _after_ putting
the thing - which was quite silly... :)

[PATCH] clk: fix function name in devm_clk_put kernel-doc comment

[Russell King - ARM Linux]

On Mon, Sep 23, 2013 at 05:45:59PM +0200, Uwe Kleine-K?nig wrote:
> The paragraph about clk_put already specifies this restriction about
> clk_put.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> I suspect that devm_clk_put is used more often than necessary. After some
> grepping around e.g. looking at drivers/tty/serial/clps711x.c it doesn't
> seem necessary to call it as the driver core already cares about calling
> the devm cleanup callbacks.

The one(s) you've left are in drivers/media/platform/marvell-ccic/mmp-driver.c:

static void mcam_deinit_clk(struct mcam_camera *mcam)
{
        unsigned int i;

        for (i = 0; i < NR_MCAM_CLK; i++) {
                if (!IS_ERR(mcam->clk[i])) {
                        if (mcam->clk[i])
                                devm_clk_put(mcam->dev, mcam->clk[i]);
                }
                mcam->clk[i] = NULL;
        }
}

static int mmpcam_probe(struct platform_device *pdev)
{
        cam = devm_kzalloc(&pdev->dev, sizeof(*cam), GFP_KERNEL);
...
        mcam = &cam->mcam;
...
        mcam->regs = devm_ioremap_resource(&pdev->dev, res);
...
        cam->power_regs = devm_ioremap_resource(&pdev->dev, res);
...
out_unregister:
        mccic_shutdown(mcam);
out_power_down:
        mmpcam_power_down(mcam);
out_deinit_clk:
        mcam_deinit_clk(mcam);
        return ret;
}

static int mmpcam_remove(struct mmp_camera *cam)
{
...
        mcam_deinit_clk(mcam);
        iounmap(cam->power_regs);
        iounmap(mcam->regs);
        kfree(cam);
        return 0;
}

So, mcam_deinit_clk() provides nothing useful and should be killed.

Second thing to spot from the above is that kfree() is broken.  As
are those iounmap()s.  The more I look at this, the more I find wrong.
gpio_free() too.

Quite honestly, I think this driver is quite broken as it stands -
anything you do to it (even patching it without build testing) is
IMHO likely to improve this driver!

Re: [PATCH] video: mmp: drop needless devm cleanup

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1499 bytes --]

On 23/09/13 19:19, Russell King - ARM Linux wrote:
> On Mon, Sep 23, 2013 at 06:13:10PM +0200, Uwe Kleine-König wrote:
>> The nice thing about devm_* is that the driver doesn't need to free the
>> resources but the driver core takes care about that. This also
>> simplifies the error path quite a bit and removes the wrong check for a
>> clock pointer being NULL.
>>
>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>> ---
>>  drivers/video/mmp/hw/mmp_ctrl.c | 17 ++---------------
>>  1 file changed, 2 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
>> index 75dca19..6ac7552 100644
>> --- a/drivers/video/mmp/hw/mmp_ctrl.c
>> +++ b/drivers/video/mmp/hw/mmp_ctrl.c
>> @@ -514,7 +514,7 @@ static int mmphw_probe(struct platform_device *pdev)
>>  	if (IS_ERR(ctrl->clk)) {
>>  		dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name);
>>  		ret = -ENOENT;
>> -		goto failed_get_clk;
>> +		goto failed;
>>  	}
>>  	clk_prepare_enable(ctrl->clk);
>>  
>> @@ -551,21 +551,8 @@ failed_path_init:
>>  		path_deinit(path_plat);
>>  	}
>>  
>> -	if (ctrl->clk) {
>> -		devm_clk_put(ctrl->dev, ctrl->clk);
>> -		clk_disable_unprepare(ctrl->clk);
> 
> And this patch also fixes the above: disabling/unpreparing _after_ putting
> the thing - which was quite silly... :)

Hmm, I wonder if that causes any issues... I.e. should this patch go for
3.12, or is 3.13 fine?

 Tomi



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[PATCH] video: mmp: drop needless devm cleanup

[Tomi Valkeinen]

On 23/09/13 19:19, Russell King - ARM Linux wrote:
> On Mon, Sep 23, 2013 at 06:13:10PM +0200, Uwe Kleine-K?nig wrote:
>> The nice thing about devm_* is that the driver doesn't need to free the
>> resources but the driver core takes care about that. This also
>> simplifies the error path quite a bit and removes the wrong check for a
>> clock pointer being NULL.
>>
>> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
>> ---
>>  drivers/video/mmp/hw/mmp_ctrl.c | 17 ++---------------
>>  1 file changed, 2 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
>> index 75dca19..6ac7552 100644
>> --- a/drivers/video/mmp/hw/mmp_ctrl.c
>> +++ b/drivers/video/mmp/hw/mmp_ctrl.c
>> @@ -514,7 +514,7 @@ static int mmphw_probe(struct platform_device *pdev)
>>  	if (IS_ERR(ctrl->clk)) {
>>  		dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name);
>>  		ret = -ENOENT;
>> -		goto failed_get_clk;
>> +		goto failed;
>>  	}
>>  	clk_prepare_enable(ctrl->clk);
>>  
>> @@ -551,21 +551,8 @@ failed_path_init:
>>  		path_deinit(path_plat);
>>  	}
>>  
>> -	if (ctrl->clk) {
>> -		devm_clk_put(ctrl->dev, ctrl->clk);
>> -		clk_disable_unprepare(ctrl->clk);
> 
> And this patch also fixes the above: disabling/unpreparing _after_ putting
> the thing - which was quite silly... :)

Hmm, I wonder if that causes any issues... I.e. should this patch go for
3.12, or is 3.13 fine?

 Tomi


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130924/c25c927b/attachment-0001.sig>

Re: [PATCH] video: mmp: drop needless devm cleanup

[Zhou Zhu]

On 09/24/2013 03:34 PM, Tomi Valkeinen wrote:
> On 23/09/13 19:19, Russell King - ARM Linux wrote:
>> On Mon, Sep 23, 2013 at 06:13:10PM +0200, Uwe Kleine-König wrote:
>>> The nice thing about devm_* is that the driver doesn't need to free the
>>> resources but the driver core takes care about that. This also
>>> simplifies the error path quite a bit and removes the wrong check for a
>>> clock pointer being NULL.
>>>
>>> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>>> ---
>>>   drivers/video/mmp/hw/mmp_ctrl.c | 17 ++---------------
>>>   1 file changed, 2 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
>>> index 75dca19..6ac7552 100644
>>> --- a/drivers/video/mmp/hw/mmp_ctrl.c
>>> +++ b/drivers/video/mmp/hw/mmp_ctrl.c
>>> @@ -514,7 +514,7 @@ static int mmphw_probe(struct platform_device *pdev)
>>>   	if (IS_ERR(ctrl->clk)) {
>>>   		dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name);
>>>   		ret = -ENOENT;
>>> -		goto failed_get_clk;
>>> +		goto failed;
>>>   	}
>>>   	clk_prepare_enable(ctrl->clk);
>>>
>>> @@ -551,21 +551,8 @@ failed_path_init:
>>>   		path_deinit(path_plat);
>>>   	}
>>>
>>> -	if (ctrl->clk) {
>>> -		devm_clk_put(ctrl->dev, ctrl->clk);
>>> -		clk_disable_unprepare(ctrl->clk);
>>
>> And this patch also fixes the above: disabling/unpreparing _after_ putting
>> the thing - which was quite silly... :)
>
> Hmm, I wonder if that causes any issues... I.e. should this patch go for
> 3.12, or is 3.13 fine?
>
>   Tomi
>
It would cause oops if probe failed due to some reason - although it 
would almost never happen so we missed it.
Thank you for finding it out.

-- 
Thanks, -Zhou

[PATCH] video: mmp: drop needless devm cleanup

[Zhou Zhu]

On 09/24/2013 03:34 PM, Tomi Valkeinen wrote:
> On 23/09/13 19:19, Russell King - ARM Linux wrote:
>> On Mon, Sep 23, 2013 at 06:13:10PM +0200, Uwe Kleine-K?nig wrote:
>>> The nice thing about devm_* is that the driver doesn't need to free the
>>> resources but the driver core takes care about that. This also
>>> simplifies the error path quite a bit and removes the wrong check for a
>>> clock pointer being NULL.
>>>
>>> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
>>> ---
>>>   drivers/video/mmp/hw/mmp_ctrl.c | 17 ++---------------
>>>   1 file changed, 2 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/video/mmp/hw/mmp_ctrl.c b/drivers/video/mmp/hw/mmp_ctrl.c
>>> index 75dca19..6ac7552 100644
>>> --- a/drivers/video/mmp/hw/mmp_ctrl.c
>>> +++ b/drivers/video/mmp/hw/mmp_ctrl.c
>>> @@ -514,7 +514,7 @@ static int mmphw_probe(struct platform_device *pdev)
>>>   	if (IS_ERR(ctrl->clk)) {
>>>   		dev_err(ctrl->dev, "unable to get clk %s\n", mi->clk_name);
>>>   		ret = -ENOENT;
>>> -		goto failed_get_clk;
>>> +		goto failed;
>>>   	}
>>>   	clk_prepare_enable(ctrl->clk);
>>>
>>> @@ -551,21 +551,8 @@ failed_path_init:
>>>   		path_deinit(path_plat);
>>>   	}
>>>
>>> -	if (ctrl->clk) {
>>> -		devm_clk_put(ctrl->dev, ctrl->clk);
>>> -		clk_disable_unprepare(ctrl->clk);
>>
>> And this patch also fixes the above: disabling/unpreparing _after_ putting
>> the thing - which was quite silly... :)
>
> Hmm, I wonder if that causes any issues... I.e. should this patch go for
> 3.12, or is 3.13 fine?
>
>   Tomi
>
It would cause oops if probe failed due to some reason - although it 
would almost never happen so we missed it.
Thank you for finding it out.

-- 
Thanks, -Zhou

[PATCH] ASoC: kirkwood: simplify clock handling

[Uwe Kleine-König]

There is no need to not use extclk if it is identical to the main clk.
The main motivation for this patch is dropping devm_clk_put which is
used in a wrong way by all other users.

While at it also extend the comments.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

there might be some further optimisations possible. I only note them
here because I don't have the hardware to test:

 - only enable extclk if it a clock rate used that makes use of the
   external clock. Not sure if that works; hardware docs reading
   necessary.
 - only provide extclk if it's != the internal clock.
 - The code uses:

	priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL);

   i.e. provides a con_id in the dt-case. I think that using NULL
   unconditionally should also work, i.e. return the first clk
   associated to the device. OTOH the current code might make things
   clearer because it's more explicit.

Best regards
Uwe
---
 sound/soc/kirkwood/kirkwood-i2s.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 0f3d73d..8224f93 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -104,16 +104,25 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
 	uint32_t clks_ctrl;
 
 	if (rate == 44100 || rate == 48000 || rate == 96000) {
-		/* use internal dco for the supported rates
-		 * defined in kirkwood_i2s_dai */
+		/*
+		 * use internal dco for the supported rates
+		 * defined in kirkwood_i2s_dai
+		 * Note: For these specific rates the dco is also used if
+		 * kirkwood_i2s_dai_extclk is in use.
+		 */
 		dev_dbg(dai->dev, "%s: dco set rate = %lu\n",
 			__func__, rate);
 		kirkwood_set_dco(priv->io, rate);
 
 		clks_ctrl = KIRKWOOD_MCLK_SOURCE_DCO;
 	} else {
-		/* use the external clock for the other rates
-		 * defined in kirkwood_i2s_dai_extclk */
+		/*
+		 * use the external clock for the other rates
+		 * defined in kirkwood_i2s_dai_extclk
+		 * This case isn't reached if kirkwood_i2s_dai is in use (and so
+		 * extclk isn't valid) because it only supports the three rates
+		 * above.
+		 */
 		dev_dbg(dai->dev, "%s: extclk set rate = %lu -> %lu\n",
 			__func__, rate, 256 * rate);
 		clk_set_rate(priv->extclk, 256 * rate);
@@ -497,12 +506,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 
 	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
 	if (!IS_ERR(priv->extclk)) {
-		if (priv->extclk == priv->clk) {
-			devm_clk_put(&pdev->dev, priv->extclk);
-			priv->extclk = ERR_PTR(-EINVAL);
-		} else {
+		clk_prepare_enable(priv->extclk);
+		if (priv->extclk != priv->clk) {
 			dev_info(&pdev->dev, "found external clock\n");
-			clk_prepare_enable(priv->extclk);
 			soc_dai = &kirkwood_i2s_dai_extclk;
 		}
 	}
-- 
1.8.4.rc3

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH] ASoC: kirkwood: simplify clock handling

[Uwe Kleine-König]

There is no need to not use extclk if it is identical to the main clk.
The main motivation for this patch is dropping devm_clk_put which is
used in a wrong way by all other users.

While at it also extend the comments.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Hello,

there might be some further optimisations possible. I only note them
here because I don't have the hardware to test:

 - only enable extclk if it a clock rate used that makes use of the
   external clock. Not sure if that works; hardware docs reading
   necessary.
 - only provide extclk if it's != the internal clock.
 - The code uses:

	priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL);

   i.e. provides a con_id in the dt-case. I think that using NULL
   unconditionally should also work, i.e. return the first clk
   associated to the device. OTOH the current code might make things
   clearer because it's more explicit.

Best regards
Uwe
---
 sound/soc/kirkwood/kirkwood-i2s.c | 24 +++++++++++++++---------
 1 file changed, 15 insertions(+), 9 deletions(-)

diff --git a/sound/soc/kirkwood/kirkwood-i2s.c b/sound/soc/kirkwood/kirkwood-i2s.c
index 0f3d73d..8224f93 100644
--- a/sound/soc/kirkwood/kirkwood-i2s.c
+++ b/sound/soc/kirkwood/kirkwood-i2s.c
@@ -104,16 +104,25 @@ static void kirkwood_set_rate(struct snd_soc_dai *dai,
 	uint32_t clks_ctrl;
 
 	if (rate == 44100 || rate == 48000 || rate == 96000) {
-		/* use internal dco for the supported rates
-		 * defined in kirkwood_i2s_dai */
+		/*
+		 * use internal dco for the supported rates
+		 * defined in kirkwood_i2s_dai
+		 * Note: For these specific rates the dco is also used if
+		 * kirkwood_i2s_dai_extclk is in use.
+		 */
 		dev_dbg(dai->dev, "%s: dco set rate = %lu\n",
 			__func__, rate);
 		kirkwood_set_dco(priv->io, rate);
 
 		clks_ctrl = KIRKWOOD_MCLK_SOURCE_DCO;
 	} else {
-		/* use the external clock for the other rates
-		 * defined in kirkwood_i2s_dai_extclk */
+		/*
+		 * use the external clock for the other rates
+		 * defined in kirkwood_i2s_dai_extclk
+		 * This case isn't reached if kirkwood_i2s_dai is in use (and so
+		 * extclk isn't valid) because it only supports the three rates
+		 * above.
+		 */
 		dev_dbg(dai->dev, "%s: extclk set rate = %lu -> %lu\n",
 			__func__, rate, 256 * rate);
 		clk_set_rate(priv->extclk, 256 * rate);
@@ -497,12 +506,9 @@ static int kirkwood_i2s_dev_probe(struct platform_device *pdev)
 
 	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
 	if (!IS_ERR(priv->extclk)) {
-		if (priv->extclk == priv->clk) {
-			devm_clk_put(&pdev->dev, priv->extclk);
-			priv->extclk = ERR_PTR(-EINVAL);
-		} else {
+		clk_prepare_enable(priv->extclk);
+		if (priv->extclk != priv->clk) {
 			dev_info(&pdev->dev, "found external clock\n");
-			clk_prepare_enable(priv->extclk);
 			soc_dai = &kirkwood_i2s_dai_extclk;
 		}
 	}
-- 
1.8.4.rc3

Re: [PATCH] ASoC: kirkwood: simplify clock handling

[Russell King - ARM Linux]

On Tue, Sep 24, 2013 at 08:12:35PM +0200, Uwe Kleine-König wrote:
> There is no need to not use extclk if it is identical to the main clk.
> The main motivation for this patch is dropping devm_clk_put which is
> used in a wrong way by all other users.

NAK.  There's patches around which switch this driver to always use
the external clock when it's available.  This patch prevents that
from happening because we no longer know whether it is the external
clock or not.

What we could do is just lose the reference to the second clock if
it turns out to be idential to the first - it'll get cleaned up if
the driver is unloaded anyway.  In other words, the call to
devm_clk_put() here can be viewed as merely an optimisation.

[PATCH] ASoC: kirkwood: simplify clock handling

[Russell King - ARM Linux]

On Tue, Sep 24, 2013 at 08:12:35PM +0200, Uwe Kleine-K?nig wrote:
> There is no need to not use extclk if it is identical to the main clk.
> The main motivation for this patch is dropping devm_clk_put which is
> used in a wrong way by all other users.

NAK.  There's patches around which switch this driver to always use
the external clock when it's available.  This patch prevents that
from happening because we no longer know whether it is the external
clock or not.

What we could do is just lose the reference to the second clock if
it turns out to be idential to the first - it'll get cleaned up if
the driver is unloaded anyway.  In other words, the call to
devm_clk_put() here can be viewed as merely an optimisation.

[PATCH] [media] marvell-ccic: simplify and fix clk handling (a bit)

[Uwe Kleine-König]

The marvell-ccic does several things wrong or ineffectively in the clock
handling and it's usage of the devm_* stuff

 - it assumes clk_get doesn't return NULL
 - it explicitly calls devm_clk_put instead just keeping the reference
   during it's lifetime and let the driver core call it
 - it calls kfree, gpio_free and free_irq for resources it requested
   using devm_kzalloc, devm_gpio_request and devm_request_irq
   respectively.
 - it mixes devm_ and unmanaged resources which probably results in a
   race condition during remove

This patch fixes all but the last issue in this list. This patch doesn't
introduce new reasons for not building, but there are already several
bulid problems.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Cc: Libin Yang <lbyang@marvell.com>
---
 drivers/media/platform/marvell-ccic/mmp-driver.c | 26 ++----------------------
 1 file changed, 2 insertions(+), 24 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c
index b5a19af..49ff52e 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -143,7 +143,6 @@ static int mmpcam_power_up(struct mcam_camera *mcam)
 	struct mmp_camera_platform_data *pdata;
 
 	if (mcam->bus_type == V4L2_MBUS_CSI2) {
-		cam->mipi_clk = devm_clk_get(mcam->dev, "mipi");
 		if ((IS_ERR(cam->mipi_clk) && mcam->dphy[2] == 0))
 			return PTR_ERR(cam->mipi_clk);
 	}
@@ -186,12 +185,6 @@ static void mmpcam_power_down(struct mcam_camera *mcam)
 	gpio_set_value(pdata->sensor_power_gpio, 0);
 	gpio_set_value(pdata->sensor_reset_gpio, 0);
 
-	if (mcam->bus_type == V4L2_MBUS_CSI2 && !IS_ERR(cam->mipi_clk)) {
-		if (cam->mipi_clk)
-			devm_clk_put(mcam->dev, cam->mipi_clk);
-		cam->mipi_clk = NULL;
-	}
-
 	mcam_clk_disable(mcam);
 }
 
@@ -325,19 +318,6 @@ static irqreturn_t mmpcam_irq(int irq, void *data)
 	return IRQ_RETVAL(handled);
 }
 
-static void mcam_deinit_clk(struct mcam_camera *mcam)
-{
-	unsigned int i;
-
-	for (i = 0; i < NR_MCAM_CLK; i++) {
-		if (!IS_ERR(mcam->clk[i])) {
-			if (mcam->clk[i])
-				devm_clk_put(mcam->dev, mcam->clk[i]);
-		}
-		mcam->clk[i] = NULL;
-	}
-}
-
 static void mcam_init_clk(struct mcam_camera *mcam)
 {
 	unsigned int i;
@@ -371,7 +351,7 @@ static int mmpcam_probe(struct platform_device *pdev)
 	if (cam == NULL)
 		return -ENOMEM;
 	cam->pdev = pdev;
-	cam->mipi_clk = NULL;
+	cam->mipi_clk = devm_clk_get(&pdev->dev, "mipi");
 	INIT_LIST_HEAD(&cam->devlist);
 
 	mcam = &cam->mcam;
@@ -442,6 +422,7 @@ static int mmpcam_probe(struct platform_device *pdev)
 	/*
 	 * Power the device up and hand it off to the core.
 	 */
+
 	ret = mmpcam_power_up(mcam);
 	if (ret)
 		goto out_deinit_clk;
@@ -470,7 +451,6 @@ out_unregister:
 out_power_down:
 	mmpcam_power_down(mcam);
 out_deinit_clk:
-	mcam_deinit_clk(mcam);
 	return ret;
 }
 
@@ -487,10 +467,8 @@ static int mmpcam_remove(struct mmp_camera *cam)
 	pdata = cam->pdev->dev.platform_data;
 	gpio_free(pdata->sensor_reset_gpio);
 	gpio_free(pdata->sensor_power_gpio);
-	mcam_deinit_clk(mcam);
 	iounmap(cam->power_regs);
 	iounmap(mcam->regs);
-	kfree(cam);
 	return 0;
 }
 
-- 
1.8.4.rc3

[PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

[Uwe Kleine-König]

The marvell-ccic does several things wrong or ineffectively in the clock
handling and it's usage of the devm_* stuff

 - it assumes clk_get doesn't return NULL
 - it explicitly calls devm_clk_put instead just keeping the reference
   during it's lifetime and let the driver core call it
 - it calls kfree, gpio_free and free_irq for resources it requested
   using devm_kzalloc, devm_gpio_request and devm_request_irq
   respectively.
 - it mixes devm_ and unmanaged resources which probably results in a
   race condition during remove

This patch fixes all but the last issue in this list. This patch doesn't
introduce new reasons for not building, but there are already several
bulid problems.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Cc: Libin Yang <lbyang@marvell.com>

Changes since (implicit) v1:
 - really fix the third issue in the list.
 - drop whitespace noise hunk

---
 drivers/media/platform/marvell-ccic/mmp-driver.c | 29 ++----------------------
 1 file changed, 2 insertions(+), 27 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c b/drivers/media/platform/marvell-ccic/mmp-driver.c
index b5a19af..ed16f81e 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -143,7 +143,6 @@ static int mmpcam_power_up(struct mcam_camera *mcam)
 	struct mmp_camera_platform_data *pdata;
 
 	if (mcam->bus_type == V4L2_MBUS_CSI2) {
-		cam->mipi_clk = devm_clk_get(mcam->dev, "mipi");
 		if ((IS_ERR(cam->mipi_clk) && mcam->dphy[2] == 0))
 			return PTR_ERR(cam->mipi_clk);
 	}
@@ -186,12 +185,6 @@ static void mmpcam_power_down(struct mcam_camera *mcam)
 	gpio_set_value(pdata->sensor_power_gpio, 0);
 	gpio_set_value(pdata->sensor_reset_gpio, 0);
 
-	if (mcam->bus_type == V4L2_MBUS_CSI2 && !IS_ERR(cam->mipi_clk)) {
-		if (cam->mipi_clk)
-			devm_clk_put(mcam->dev, cam->mipi_clk);
-		cam->mipi_clk = NULL;
-	}
-
 	mcam_clk_disable(mcam);
 }
 
@@ -325,19 +318,6 @@ static irqreturn_t mmpcam_irq(int irq, void *data)
 	return IRQ_RETVAL(handled);
 }
 
-static void mcam_deinit_clk(struct mcam_camera *mcam)
-{
-	unsigned int i;
-
-	for (i = 0; i < NR_MCAM_CLK; i++) {
-		if (!IS_ERR(mcam->clk[i])) {
-			if (mcam->clk[i])
-				devm_clk_put(mcam->dev, mcam->clk[i]);
-		}
-		mcam->clk[i] = NULL;
-	}
-}
-
 static void mcam_init_clk(struct mcam_camera *mcam)
 {
 	unsigned int i;
@@ -371,7 +351,7 @@ static int mmpcam_probe(struct platform_device *pdev)
 	if (cam == NULL)
 		return -ENOMEM;
 	cam->pdev = pdev;
-	cam->mipi_clk = NULL;
+	cam->mipi_clk = devm_clk_get(&pdev->dev, "mipi");
 	INIT_LIST_HEAD(&cam->devlist);
 
 	mcam = &cam->mcam;
@@ -442,6 +422,7 @@ static int mmpcam_probe(struct platform_device *pdev)
 	/*
 	 * Power the device up and hand it off to the core.
 	 */
+
 	ret = mmpcam_power_up(mcam);
 	if (ret)
 		goto out_deinit_clk;
@@ -470,7 +451,6 @@ out_unregister:
 out_power_down:
 	mmpcam_power_down(mcam);
 out_deinit_clk:
-	mcam_deinit_clk(mcam);
 	return ret;
 }
 
@@ -481,16 +461,11 @@ static int mmpcam_remove(struct mmp_camera *cam)
 	struct mmp_camera_platform_data *pdata;
 
 	mmpcam_remove_device(cam);
-	free_irq(cam->irq, mcam);
 	mccic_shutdown(mcam);
 	mmpcam_power_down(mcam);
 	pdata = cam->pdev->dev.platform_data;
-	gpio_free(pdata->sensor_reset_gpio);
-	gpio_free(pdata->sensor_power_gpio);
-	mcam_deinit_clk(mcam);
 	iounmap(cam->power_regs);
 	iounmap(mcam->regs);
-	kfree(cam);
 	return 0;
 }
 
-- 
1.8.4.rc3

Re: [PATCH] ASoC: kirkwood: simplify clock handling

[Jean-Francois Moine]

On Tue, 24 Sep 2013 20:12:35 +0200
Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote:

> There is no need to not use extclk if it is identical to the main clk.
> The main motivation for this patch is dropping devm_clk_put which is
> used in a wrong way by all other users.
> 
> While at it also extend the comments.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> there might be some further optimisations possible. I only note them
> here because I don't have the hardware to test:
> 
>  - only enable extclk if it a clock rate used that makes use of the
>    external clock. Not sure if that works; hardware docs reading
>    necessary.
>  - only provide extclk if it's != the internal clock.
>  - The code uses:
> 
> 	priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL);
> 
>    i.e. provides a con_id in the dt-case. I think that using NULL
>    unconditionally should also work, i.e. return the first clk
>    associated to the device. OTOH the current code might make things
>    clearer because it's more explicit.
	[snip]

Uwe,

The code around line 104 in kirkwood-i2s.c is not what it should be
(the patch from Russell is lost somewhere in the mailing-list).
Instead of:

	if (rate == 44100 || rate == 48000 || rate == 96000) {
		/* use internal dco for the supported rates
		 * defined in kirkwood_i2s_dai */

it should be:

	if (IS_ERR(priv->extclk)) {	/* no external clock */
		/* use internal dco - the supported rates are
		 * defined in kirkwood_i2s_dai */

That is: if there is an external clock, use it.

In fact, the internal dco is used for two audio devices. When both
devices are used at the same time, at least one of them must always use
an external clock, otherwise, there is a clock rate conflict.

As only one clock is used, there is no need to declare 2 clocks in the
DT, but the driver must know if it uses the internal or external clock
(to set the right clock input and also because their rates are not set
the same way)

So, the probe code should be:

	/* check first if an external clock is declared */
	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
	if (!IS_ERR(priv->extclk)) {
		... use the external clock ...
	} else {

		/* get the first clock which must be the dco */
		priv->clk = devm_clk_get(&pdev->dev, NULL);
		if (IS_ERR(priv->clk))
			.. error, no clock ..
		.. use the internal dco ...
	}

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

[PATCH] ASoC: kirkwood: simplify clock handling

[Jean-Francois Moine]

On Tue, 24 Sep 2013 20:12:35 +0200
Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> wrote:

> There is no need to not use extclk if it is identical to the main clk.
> The main motivation for this patch is dropping devm_clk_put which is
> used in a wrong way by all other users.
> 
> While at it also extend the comments.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> there might be some further optimisations possible. I only note them
> here because I don't have the hardware to test:
> 
>  - only enable extclk if it a clock rate used that makes use of the
>    external clock. Not sure if that works; hardware docs reading
>    necessary.
>  - only provide extclk if it's != the internal clock.
>  - The code uses:
> 
> 	priv->clk = devm_clk_get(&pdev->dev, np ? "internal" : NULL);
> 
>    i.e. provides a con_id in the dt-case. I think that using NULL
>    unconditionally should also work, i.e. return the first clk
>    associated to the device. OTOH the current code might make things
>    clearer because it's more explicit.
	[snip]

Uwe,

The code around line 104 in kirkwood-i2s.c is not what it should be
(the patch from Russell is lost somewhere in the mailing-list).
Instead of:

	if (rate == 44100 || rate == 48000 || rate == 96000) {
		/* use internal dco for the supported rates
		 * defined in kirkwood_i2s_dai */

it should be:

	if (IS_ERR(priv->extclk)) {	/* no external clock */
		/* use internal dco - the supported rates are
		 * defined in kirkwood_i2s_dai */

That is: if there is an external clock, use it.

In fact, the internal dco is used for two audio devices. When both
devices are used at the same time, at least one of them must always use
an external clock, otherwise, there is a clock rate conflict.

As only one clock is used, there is no need to declare 2 clocks in the
DT, but the driver must know if it uses the internal or external clock
(to set the right clock input and also because their rates are not set
the same way)

So, the probe code should be:

	/* check first if an external clock is declared */
	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
	if (!IS_ERR(priv->extclk)) {
		... use the external clock ...
	} else {

		/* get the first clock which must be the dco */
		priv->clk = devm_clk_get(&pdev->dev, NULL);
		if (IS_ERR(priv->clk))
			.. error, no clock ..
		.. use the internal dco ...
	}

-- 
Ken ar c'henta?	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH] ASoC: kirkwood: simplify clock handling

[Russell King - ARM Linux]

On Tue, Sep 24, 2013 at 09:04:42PM +0200, Jean-Francois Moine wrote:
> So, the probe code should be:
> 
> 	/* check first if an external clock is declared */
> 	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
> 	if (!IS_ERR(priv->extclk)) {
> 		... use the external clock ...
> 	} else {
> 
> 		/* get the first clock which must be the dco */
> 		priv->clk = devm_clk_get(&pdev->dev, NULL);
> 		if (IS_ERR(priv->clk))
> 			.. error, no clock ..
> 		.. use the internal dco ...
> 	}

Actually no - we need to get and enable the internal clock so that we can
access the registers - the Dove locks solid if you access the audio block
registers without its internal clock to the audio block enabled.

[PATCH] ASoC: kirkwood: simplify clock handling

[Russell King - ARM Linux]

On Tue, Sep 24, 2013 at 09:04:42PM +0200, Jean-Francois Moine wrote:
> So, the probe code should be:
> 
> 	/* check first if an external clock is declared */
> 	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
> 	if (!IS_ERR(priv->extclk)) {
> 		... use the external clock ...
> 	} else {
> 
> 		/* get the first clock which must be the dco */
> 		priv->clk = devm_clk_get(&pdev->dev, NULL);
> 		if (IS_ERR(priv->clk))
> 			.. error, no clock ..
> 		.. use the internal dco ...
> 	}

Actually no - we need to get and enable the internal clock so that we can
access the registers - the Dove locks solid if you access the audio block
registers without its internal clock to the audio block enabled.

[PATCH] [RFC] devm: drop devm_clk_put

[Uwe Kleine-König]

devm_clk_put isn't really necessary as a driver usually keeps the
reference during its lifetime and there are no in-tree users. So
drop it.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
The "no in-tree users" part of the commit log isn't true yet, as there
is no agreed-on patch for sound/soc/kirkwood/kirkwood-i2s.c yet and the
other patches in this thread are not applied/acked yet.
---
 Documentation/driver-model/devres.txt |  1 -
 drivers/clk/clk-devres.c              | 10 ----------
 include/linux/clk.h                   | 19 +++----------------
 3 files changed, 3 insertions(+), 27 deletions(-)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 5bdc8cb..9d04836 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -287,7 +287,6 @@ REGULATOR
 
 CLOCK
   devm_clk_get()
-  devm_clk_put()
 
 PINCTRL
   devm_pinctrl_get()
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 8f57154..0943d8f 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -43,13 +43,3 @@ static int devm_clk_match(struct device *dev, void *res, void *data)
 	}
 	return *c == data;
 }
-
-void devm_clk_put(struct device *dev, struct clk *clk)
-{
-	int ret;
-
-	ret = devres_release(dev, devm_clk_release, devm_clk_match, clk);
-
-	WARN_ON(ret);
-}
-EXPORT_SYMBOL(devm_clk_put);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 9a6d045..5115bc6 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -154,7 +154,9 @@ struct clk *clk_get(struct device *dev, const char *id);
  * devm_clk_get should not be called from within interrupt context.
  *
  * The clock will automatically be freed when the device is unbound
- * from the bus.
+ * from the bus. Note there is deliberately no devm_clk_put function as a
+ * driver usally keeps the reference to its clocks during the complete driver
+ * live time.
  */
 struct clk *devm_clk_get(struct device *dev, const char *id);
 
@@ -205,19 +207,6 @@ unsigned long clk_get_rate(struct clk *clk);
  */
 void clk_put(struct clk *clk);
 
-/**
- * devm_clk_put	- "free" a managed clock source
- * @dev: device used to acuqire the clock
- * @clk: clock source acquired with devm_clk_get()
- *
- * Note: drivers must ensure that all clk_enable calls made on this
- * clock source are balanced by clk_disable calls prior to calling
- * this function.
- *
- * clk_put should not be called from within interrupt context.
- */
-void devm_clk_put(struct device *dev, struct clk *clk);
-
 /*
  * The remaining APIs are optional for machine class support.
  */
@@ -290,8 +279,6 @@ static inline struct clk *devm_clk_get(struct device *dev, const char *id)
 
 static inline void clk_put(struct clk *clk) {}
 
-static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
-
 static inline int clk_enable(struct clk *clk)
 {
 	return 0;
-- 
1.8.4.rc3

[PATCH] [RFC] devm: drop devm_clk_put

[Uwe Kleine-König]

devm_clk_put isn't really necessary as a driver usually keeps the
reference during its lifetime and there are no in-tree users. So
drop it.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
The "no in-tree users" part of the commit log isn't true yet, as there
is no agreed-on patch for sound/soc/kirkwood/kirkwood-i2s.c yet and the
other patches in this thread are not applied/acked yet.
---
 Documentation/driver-model/devres.txt |  1 -
 drivers/clk/clk-devres.c              | 10 ----------
 include/linux/clk.h                   | 19 +++----------------
 3 files changed, 3 insertions(+), 27 deletions(-)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 5bdc8cb..9d04836 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -287,7 +287,6 @@ REGULATOR
 
 CLOCK
   devm_clk_get()
-  devm_clk_put()
 
 PINCTRL
   devm_pinctrl_get()
diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
index 8f57154..0943d8f 100644
--- a/drivers/clk/clk-devres.c
+++ b/drivers/clk/clk-devres.c
@@ -43,13 +43,3 @@ static int devm_clk_match(struct device *dev, void *res, void *data)
 	}
 	return *c == data;
 }
-
-void devm_clk_put(struct device *dev, struct clk *clk)
-{
-	int ret;
-
-	ret = devres_release(dev, devm_clk_release, devm_clk_match, clk);
-
-	WARN_ON(ret);
-}
-EXPORT_SYMBOL(devm_clk_put);
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 9a6d045..5115bc6 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -154,7 +154,9 @@ struct clk *clk_get(struct device *dev, const char *id);
  * devm_clk_get should not be called from within interrupt context.
  *
  * The clock will automatically be freed when the device is unbound
- * from the bus.
+ * from the bus. Note there is deliberately no devm_clk_put function as a
+ * driver usally keeps the reference to its clocks during the complete driver
+ * live time.
  */
 struct clk *devm_clk_get(struct device *dev, const char *id);
 
@@ -205,19 +207,6 @@ unsigned long clk_get_rate(struct clk *clk);
  */
 void clk_put(struct clk *clk);
 
-/**
- * devm_clk_put	- "free" a managed clock source
- * @dev: device used to acuqire the clock
- * @clk: clock source acquired with devm_clk_get()
- *
- * Note: drivers must ensure that all clk_enable calls made on this
- * clock source are balanced by clk_disable calls prior to calling
- * this function.
- *
- * clk_put should not be called from within interrupt context.
- */
-void devm_clk_put(struct device *dev, struct clk *clk);
-
 /*
  * The remaining APIs are optional for machine class support.
  */
@@ -290,8 +279,6 @@ static inline struct clk *devm_clk_get(struct device *dev, const char *id)
 
 static inline void clk_put(struct clk *clk) {}
 
-static inline void devm_clk_put(struct device *dev, struct clk *clk) {}
-
 static inline int clk_enable(struct clk *clk)
 {
 	return 0;
-- 
1.8.4.rc3

Re: [PATCH] ASoC: kirkwood: simplify clock handling

[Uwe Kleine-König]

On Tue, Sep 24, 2013 at 08:05:34PM +0100, Russell King - ARM Linux wrote:
> On Tue, Sep 24, 2013 at 09:04:42PM +0200, Jean-Francois Moine wrote:
> > So, the probe code should be:
> > 
> > 	/* check first if an external clock is declared */
> > 	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
> > 	if (!IS_ERR(priv->extclk)) {
> > 		... use the external clock ...
> > 	} else {
> > 
> > 		/* get the first clock which must be the dco */
> > 		priv->clk = devm_clk_get(&pdev->dev, NULL);
> > 		if (IS_ERR(priv->clk))
> > 			.. error, no clock ..
> > 		.. use the internal dco ...
> > 	}
> 
> Actually no - we need to get and enable the internal clock so that we can
> access the registers - the Dove locks solid if you access the audio block
> registers without its internal clock to the audio block enabled.
So what is the plan here? Apply Russell's patch and then just drop the
devm_clk_put? Do you have a pointer to that patch? Then I'd follow up
with a patch for devm_clk_put.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] ASoC: kirkwood: simplify clock handling

[Uwe Kleine-König]

On Tue, Sep 24, 2013 at 08:05:34PM +0100, Russell King - ARM Linux wrote:
> On Tue, Sep 24, 2013 at 09:04:42PM +0200, Jean-Francois Moine wrote:
> > So, the probe code should be:
> > 
> > 	/* check first if an external clock is declared */
> > 	priv->extclk = devm_clk_get(&pdev->dev, "extclk");
> > 	if (!IS_ERR(priv->extclk)) {
> > 		... use the external clock ...
> > 	} else {
> > 
> > 		/* get the first clock which must be the dco */
> > 		priv->clk = devm_clk_get(&pdev->dev, NULL);
> > 		if (IS_ERR(priv->clk))
> > 			.. error, no clock ..
> > 		.. use the internal dco ...
> > 	}
> 
> Actually no - we need to get and enable the internal clock so that we can
> access the registers - the Dove locks solid if you access the audio block
> registers without its internal clock to the audio block enabled.
So what is the plan here? Apply Russell's patch and then just drop the
devm_clk_put? Do you have a pointer to that patch? Then I'd follow up
with a patch for devm_clk_put.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

[Jonathan Corbet]

On Tue, 24 Sep 2013 20:59:47 +0200
Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> wrote:

> The marvell-ccic does several things wrong or ineffectively in the clock
> handling and it's usage of the devm_* stuff
> 
>  - it assumes clk_get doesn't return NULL
>  - it explicitly calls devm_clk_put instead just keeping the reference
>    during it's lifetime and let the driver core call it
>  - it calls kfree, gpio_free and free_irq for resources it requested
>    using devm_kzalloc, devm_gpio_request and devm_request_irq
>    respectively.
>  - it mixes devm_ and unmanaged resources which probably results in a
>    race condition during remove

OK, all of that stuff was added this time around by Libin; my
understanding of that particular hardware is ... minimal.  The basic
idea of the patch seems sound.  I do note, though, that you've changed
the behavior of the driver somewhat.  The MIPI clock is current
obtained at power-up time and released on power-down; you've moved it
to probe time instead, and it's held for the lifetime of the driver.
Perhaps that's even better, I don't know...Libin, what do you say on
that?

The free_irq() call is also removed by a patch previously submitted by
Wei Yongjun.

> This patch fixes all but the last issue in this list. This patch doesn't
> introduce new reasons for not building, but there are already several
> bulid problems.

Care to report those?

Thanks,

jon

[PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

[Libin Yang]

Hi Jonathan & Uwe,

In the clk enable and prepare function, we will check the NULL pointer. So it should be no problem.

For the mipi_clk, it is shared between other components, so we put the clk it we don't use it.

For the free_irq, it's my fault. Out before patch really removed this code together with gpio free .... It missed the last part of the original patch.


Regards,
Libin 

>-----Original Message-----
>From: Jonathan Corbet [mailto:corbet at lwn.net]
>Sent: Wednesday, September 25, 2013 3:15 PM
>To: Uwe Kleine-K?nig
>Cc: Mauro Carvalho Chehab; linux-media at vger.kernel.org; linux-arm-
>kernel at lists.infradead.org; Russell King; kernel at pengutronix.de; Libin Yang
>Subject: Re: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)
>
>On Tue, 24 Sep 2013 20:59:47 +0200
>Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> wrote:
>
>> The marvell-ccic does several things wrong or ineffectively in the
>> clock handling and it's usage of the devm_* stuff
>>
>>  - it assumes clk_get doesn't return NULL
>>  - it explicitly calls devm_clk_put instead just keeping the reference
>>    during it's lifetime and let the driver core call it
>>  - it calls kfree, gpio_free and free_irq for resources it requested
>>    using devm_kzalloc, devm_gpio_request and devm_request_irq
>>    respectively.
>>  - it mixes devm_ and unmanaged resources which probably results in a
>>    race condition during remove
>
>OK, all of that stuff was added this time around by Libin; my understanding of that particular
>hardware is ... minimal.  The basic idea of the patch seems sound.  I do note, though, that
>you've changed the behavior of the driver somewhat.  The MIPI clock is current obtained at
>power-up time and released on power-down; you've moved it to probe time instead, and it's
>held for the lifetime of the driver.
>Perhaps that's even better, I don't know...Libin, what do you say on that?
>
>The free_irq() call is also removed by a patch previously submitted by Wei Yongjun.
>
>> This patch fixes all but the last issue in this list. This patch
>> doesn't introduce new reasons for not building, but there are already
>> several bulid problems.
>
>Care to report those?
>
>Thanks,
>
>jon

[PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

[Uwe Kleine-König]

Hi Libin,

On Wed, Sep 25, 2013 at 07:47:19PM -0700, Libin Yang wrote:
> In the clk enable and prepare function, we will check the NULL
> pointer. So it should be no problem.
I'm not sure what you mean here and unfortunately your quoting style
makes your statement appear without context.

	if (... && !IS_ERR(cam->mipi_clk)) {
		if (cam->mipi_clk)
			devm_clk_put(mcam->dev, cam->mipi_clk);
		cam->mipi_clk = NULL;
	}

might work in your setup, but it's wrong usage of the clk API. There is
no reason NULL couldn't be a valid clk pointer. Moreover I cannot find a
place in that driver that calls prepare and/or enable for the mipi_clk.
(BTW, calling clk_get_rate on a disabled clk is another thing you
shouldn't do.)

> For the mipi_clk, it is shared between other components, so we put the clk it we don't use it.
There should be no problem if >1 driver holds a reference to the
clock. It's clk_disable (+ maybe clk_unprepare) you should call when you
don't need it any more. (But even having >1 driver enabling a clk isn't
a problem ...)

> >> This patch fixes all but the last issue in this list. This patch
> >> doesn't introduce new reasons for not building, but there are already
> >> several bulid problems.
> >
> >Care to report those?
Sure:

	  CC      drivers/media/platform/marvell-ccic/mmp-driver.o
	drivers/media/platform/marvell-ccic/mmp-driver.c: In function 'mmpcam_calc_dphy':
	drivers/media/platform/marvell-ccic/mmp-driver.c:264:15: error: 'struct mmp_camera_platform_data' has no member named 'dphy3_algo'
	drivers/media/platform/marvell-ccic/mmp-driver.c:265:7: error: 'DPHY3_ALGO_PXA910' undeclared (first use in this function)
	drivers/media/platform/marvell-ccic/mmp-driver.c:265:7: note: each undeclared identifier is reported only once for each function it appears in
	drivers/media/platform/marvell-ccic/mmp-driver.c:269:8: error: 'struct mmp_camera_platform_data' has no member named 'dphy'
	drivers/media/platform/marvell-ccic/mmp-driver.c:270:17: error: 'struct mmp_camera_platform_data' has no member named 'lane_clk'
	drivers/media/platform/marvell-ccic/mmp-driver.c:271:16: error: 'struct mmp_camera_platform_data' has no member named 'lane_clk'
	drivers/media/platform/marvell-ccic/mmp-driver.c:273:7: error: 'DPHY3_ALGO_PXA2128' undeclared (first use in this function)
	drivers/media/platform/marvell-ccic/mmp-driver.c:277:8: error: 'struct mmp_camera_platform_data' has no member named 'dphy'
	drivers/media/platform/marvell-ccic/mmp-driver.c:278:17: error: 'struct mmp_camera_platform_data' has no member named 'lane_clk'
	drivers/media/platform/marvell-ccic/mmp-driver.c:279:16: error: 'struct mmp_camera_platform_data' has no member named 'lane_clk'
	drivers/media/platform/marvell-ccic/mmp-driver.c:308:7: error: 'struct mmp_camera_platform_data' has no member named 'dphy'
	drivers/media/platform/marvell-ccic/mmp-driver.c:312:104: error: 'struct mmp_camera_platform_data' has no member named 'dphy'
	drivers/media/platform/marvell-ccic/mmp-driver.c:312:120: error: 'struct mmp_camera_platform_data' has no member named 'dphy'
	drivers/media/platform/marvell-ccic/mmp-driver.c:312:136: error: 'struct mmp_camera_platform_data' has no member named 'dphy'
	drivers/media/platform/marvell-ccic/mmp-driver.c: In function 'mmpcam_probe':
	drivers/media/platform/marvell-ccic/mmp-driver.c:385:24: error: 'struct mmp_camera_platform_data' has no member named 'mclk_min'
	drivers/media/platform/marvell-ccic/mmp-driver.c:386:24: error: 'struct mmp_camera_platform_data' has no member named 'mclk_src'
	drivers/media/platform/marvell-ccic/mmp-driver.c:387:24: error: 'struct mmp_camera_platform_data' has no member named 'mclk_div'
	drivers/media/platform/marvell-ccic/mmp-driver.c:388:24: error: 'struct mmp_camera_platform_data' has no member named 'bus_type'
	drivers/media/platform/marvell-ccic/mmp-driver.c:389:20: error: 'struct mmp_camera_platform_data' has no member named 'dphy'
	drivers/media/platform/marvell-ccic/mmp-driver.c:391:20: error: 'struct mmp_camera_platform_data' has no member named 'lane'

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

[Russell King - ARM Linux]

On Thu, Sep 26, 2013 at 10:13:56AM +0200, Uwe Kleine-K?nig wrote:
> Hi Libin,
> 
> On Wed, Sep 25, 2013 at 07:47:19PM -0700, Libin Yang wrote:
> > In the clk enable and prepare function, we will check the NULL
> > pointer. So it should be no problem.
> I'm not sure what you mean here and unfortunately your quoting style
> makes your statement appear without context.
> 
> 	if (... && !IS_ERR(cam->mipi_clk)) {
> 		if (cam->mipi_clk)
> 			devm_clk_put(mcam->dev, cam->mipi_clk);
> 		cam->mipi_clk = NULL;
> 	}
> 
> might work in your setup, but it's wrong usage of the clk API. There is
> no reason NULL couldn't be a valid clk pointer. Moreover I cannot find a
> place in that driver that calls prepare and/or enable for the mipi_clk.
> (BTW, calling clk_get_rate on a disabled clk is another thing you
> shouldn't do.)

It's a bug for another reason.  Consider this:

	clk = devm_clk_get(...);

Now, as the CLK API defines only IS_ERR(clk) to be errors, if clk is NULL
then the devm API will allocate a tracking structure for the "allocated"
clock.  If you then do:

	if (!IS_ERR(clk)) {
		if (clk)
			devm_clk_put(clk);
		clk = NULL;
	}

Then this structure won't get freed.  Next time you call devm_clk_get(),
you'll allocate another tracking structure.  If the driver does this a
lot, it will spawn lots of these tracking structures which will only get
cleaned up when the device is unbound (possibly never.)

So, what this driver is doing with its NULL checks against clocks is
buggy, no two ways about it.

Re: [PATCH] video: mmp: drop needless devm cleanup

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

On 23/09/13 19:13, Uwe Kleine-König wrote:
> The nice thing about devm_* is that the driver doesn't need to free the
> resources but the driver core takes care about that. This also
> simplifies the error path quite a bit and removes the wrong check for a
> clock pointer being NULL.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/mmp/hw/mmp_ctrl.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 

Thanks, queuing for 3.12 fixes.

 Tomi




[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

[PATCH] video: mmp: drop needless devm cleanup

[Tomi Valkeinen]

On 23/09/13 19:13, Uwe Kleine-K?nig wrote:
> The nice thing about devm_* is that the driver doesn't need to free the
> resources but the driver core takes care about that. This also
> simplifies the error path quite a bit and removes the wrong check for a
> clock pointer being NULL.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
>  drivers/video/mmp/hw/mmp_ctrl.c | 17 ++---------------
>  1 file changed, 2 insertions(+), 15 deletions(-)
> 

Thanks, queuing for 3.12 fixes.

 Tomi



-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 901 bytes
Desc: OpenPGP digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130926/a1a59c36/attachment.sig>

[PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

[Libin Yang]

Hi Uwe,

Thanks for your reviewing. Please see the comments below.

>On Wed, Sep 25, 2013 at 07:47:19PM -0700, Libin Yang wrote:
>> In the clk enable and prepare function, we will check the NULL
>> pointer. So it should be no problem.
>I'm not sure what you mean here and unfortunately your quoting style makes your statement
>appear without context.
>
>	if (... && !IS_ERR(cam->mipi_clk)) {
>		if (cam->mipi_clk)
>			devm_clk_put(mcam->dev, cam->mipi_clk);
>		cam->mipi_clk = NULL;
>	}
>
>might work in your setup, but it's wrong usage of the clk API. There is no reason NULL
>couldn't be a valid clk pointer. Moreover I cannot find a place in that driver that calls prepare
>and/or enable for the mipi_clk.

[Libin] Right. NULL could be a valid clk pointer. In the code, the clk will not be released if mipi_clk is NULL.
Is below OK?
+	if (... && !IS_ERR(cam->mipi_clk)) {
+		devm_clk_put(mcam->dev, cam->mipi_clk);
+		cam->mipi_clk = NULL;
+	}
Set cam->mipi_clk = NULL is let cam->mipi_clk to be the initial state just like after cam is allocated.

>(BTW, calling clk_get_rate on a disabled clk is another thing you shouldn't do.)

[Libin] Thanks for pointing it out. We enable the clk in other components. 
Yes, you are right. We should enable the clk explicitly here.

>
>> For the mipi_clk, it is shared between other components, so we put the clk it we don't use it.
>There should be no problem if >1 driver holds a reference to the clock. It's clk_disable (+
>maybe clk_unprepare) you should call when you don't need it any more. (But even having >1
>driver enabling a clk isn't a problem ...)

[Libin] So you mean we need not release the clk here and wait for devm to release it later? I will check it with my colleagues to see whether they are OK with this.

>
>> >> This patch fixes all but the last issue in this list. This patch
>> >> doesn't introduce new reasons for not building, but there are
>> >> already several bulid problems.
>> >
>> >Care to report those?
>Sure:
>
>	  CC      drivers/media/platform/marvell-ccic/mmp-driver.o
>	drivers/media/platform/marvell-ccic/mmp-driver.c: In function 'mmpcam_calc_dphy':
>	drivers/media/platform/marvell-ccic/mmp-driver.c:264:15: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy3_algo'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:265:7: error:
>'DPHY3_ALGO_PXA910' undeclared (first use in this function)
>	drivers/media/platform/marvell-ccic/mmp-driver.c:265:7: note: each undeclared
>identifier is reported only once for each function it appears in
>	drivers/media/platform/marvell-ccic/mmp-driver.c:269:8: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:270:17: error: 'struct
>mmp_camera_platform_data' has no member named 'lane_clk'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:271:16: error: 'struct
>mmp_camera_platform_data' has no member named 'lane_clk'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:273:7: error:
>'DPHY3_ALGO_PXA2128' undeclared (first use in this function)
>	drivers/media/platform/marvell-ccic/mmp-driver.c:277:8: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:278:17: error: 'struct
>mmp_camera_platform_data' has no member named 'lane_clk'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:279:16: error: 'struct
>mmp_camera_platform_data' has no member named 'lane_clk'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:308:7: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:312:104: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:312:120: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:312:136: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
>	drivers/media/platform/marvell-ccic/mmp-driver.c: In function 'mmpcam_probe':
>	drivers/media/platform/marvell-ccic/mmp-driver.c:385:24: error: 'struct
>mmp_camera_platform_data' has no member named 'mclk_min'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:386:24: error: 'struct
>mmp_camera_platform_data' has no member named 'mclk_src'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:387:24: error: 'struct
>mmp_camera_platform_data' has no member named 'mclk_div'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:388:24: error: 'struct
>mmp_camera_platform_data' has no member named 'bus_type'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:389:20: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
>	drivers/media/platform/marvell-ccic/mmp-driver.c:391:20: error: 'struct
>mmp_camera_platform_data' has no member named 'lane'
>
>Best regards
>Uwe
>
>--
>Pengutronix e.K.                           | Uwe Kleine-K?nig            |
>Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)

[Libin Yang]

Hi Russell,


>-----Original Message-----
>From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
>Sent: Thursday, September 26, 2013 4:24 PM
>To: Uwe Kleine-K?nig
>Cc: Libin Yang; Jonathan Corbet; Mauro Carvalho Chehab; linux-media at vger.kernel.org;
>linux-arm-kernel at lists.infradead.org; kernel at pengutronix.de
>Subject: Re: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)
>
>On Thu, Sep 26, 2013 at 10:13:56AM +0200, Uwe Kleine-K?nig wrote:
>> Hi Libin,
>>
>> On Wed, Sep 25, 2013 at 07:47:19PM -0700, Libin Yang wrote:
>> > In the clk enable and prepare function, we will check the NULL
>> > pointer. So it should be no problem.
>> I'm not sure what you mean here and unfortunately your quoting style
>> makes your statement appear without context.
>>
>> 	if (... && !IS_ERR(cam->mipi_clk)) {
>> 		if (cam->mipi_clk)
>> 			devm_clk_put(mcam->dev, cam->mipi_clk);
>> 		cam->mipi_clk = NULL;
>> 	}
>>
>> might work in your setup, but it's wrong usage of the clk API. There
>> is no reason NULL couldn't be a valid clk pointer. Moreover I cannot
>> find a place in that driver that calls prepare and/or enable for the mipi_clk.
>> (BTW, calling clk_get_rate on a disabled clk is another thing you
>> shouldn't do.)
>
>It's a bug for another reason.  Consider this:
>
>	clk = devm_clk_get(...);
>
>Now, as the CLK API defines only IS_ERR(clk) to be errors, if clk is NULL then the devm
>API will allocate a tracking structure for the "allocated"
>clock.  If you then do:
>
>	if (!IS_ERR(clk)) {
>		if (clk)
>			devm_clk_put(clk);
>		clk = NULL;
>	}
>
>Then this structure won't get freed.  Next time you call devm_clk_get(), you'll allocate another
>tracking structure.  If the driver does this a lot, it will spawn lots of these tracking structures
>which will only get cleaned up when the device is unbound (possibly never.)
>
>So, what this driver is doing with its NULL checks against clocks is buggy, no two ways
>about it. 

[Libin] Yes, you are right. it will not release the clk tracking structure if it is NULL and may allocate again later. It is a bug.

Regards,
Libin

[PATCH] remoteproc/davinci: drop needless devm_clk_put

[Ohad Ben-Cohen]

On Mon, Sep 23, 2013 at 7:00 PM, Uwe Kleine-K?nig
<u.kleine-koenig@pengutronix.de> wrote:
> The comment above disable_irq says that it is needed to ensure that the
> "devm subsystem might end up releasing things before freeing the irq,
> thus allowing an interrupt to sneak in while the device is being
> removed." disable_irq is enough for this purpose and there is no need to
> manually free the reference to the clock.
>
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>

Applied and pushed to remoteproc-next, thanks!