can: flexcan: add 2nd clock to support imx53 and newer

Commit Message

Marc Kleine-Budde July 17, 2012, 10:05 p.m. UTC

From: Steffen Trumtrar <[e-mail]>

This patch add support for a second clock to the flexcan driver. On modern
freescale ARM cores like the imx53 and imx6q two clocks ("ipg" and "per")
must be enabled in order to access the CAN core.

In the original driver, the clock was requested without specifying the
connection id, further all mainline ARM archs with flexcan support
(imx28, imx25, imx35) register their flexcan clock without a connection id,
too.

This patch first renames the existing clk variable to clk_ipg and adds the
connection id "ipg" to the clk_get() call. Then a second clock "per" is
requested. As all archs don't specify a connection id, both clk_get return
the same clock. This ensures compatibility to existing flexcan support
and adds support for imx53 at the same time.

After this patch hits mainline, the archs may give their existing flexcan
clock the "ipg" connection id and implement a dummy "per" clock.

This patch has been tested on imx28 (unmodified clk tree) and on imx53
with a seperate "ipg" and "per" clock.

Cc: Sascha Hauer <[e-mail]>
Cc: Shawn Guo <[e-mail]>
Cc: Hui Wang <[e-mail]>
Signed-off-by: Steffen Trumtrar <[e-mail]>
Signed-off-by: Marc Kleine-Budde <[e-mail]>
---
 drivers/net/can/flexcan.c |   52 ++++++++++++++++++++++++++++++---------------
 1 file changed, 35 insertions(+), 17 deletions(-)

Comments

Jason Wang July 18, 2012, 2:12 a.m. UTC | #1

Marc Kleine-Budde wrote:
> From: Steffen Trumtrar <[e-mail]>
>
> This patch add support for a second clock to the flexcan driver. On modern
> freescale ARM cores like the imx53 and imx6q two clocks ("ipg" and "per")
> must be enabled in order to access the CAN core.
>
> In the original driver, the clock was requested without specifying the
> connection id, further all mainline ARM archs with flexcan support
> (imx28, imx25, imx35) register their flexcan clock without a connection id,
> too.
>
> This patch first renames the existing clk variable to clk_ipg and adds the
> connection id "ipg" to the clk_get() call. Then a second clock "per" is
> requested. As all archs don't specify a connection id, both clk_get return
> the same clock. This ensures compatibility to existing flexcan support
> and adds support for imx53 at the same time.
>
> After this patch hits mainline, the archs may give their existing flexcan
> clock the "ipg" connection id and implement a dummy "per" clock.
>
> This patch has been tested on imx28 (unmodified clk tree) and on imx53
> with a seperate "ipg" and "per" clock.
>
> Cc: Sascha Hauer <[e-mail]>
> Cc: Shawn Guo <[e-mail]>
> Cc: Hui Wang <[e-mail]>
> Signed-off-by: Steffen Trumtrar <[e-mail]>
> Signed-off-by: Marc Kleine-Budde <[e-mail]>
> ---
>  drivers/net/can/flexcan.c |   52 ++++++++++++++++++++++++++++++---------------
>  1 file changed, 35 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
> index c8a6fc7..281d51f 100644
> --- a/drivers/net/can/flexcan.c
> +++ b/drivers/net/can/flexcan.c
> @@ -190,7 +190,8 @@ struct flexcan_priv {
>
>   
[...]
>  	if (!clock_freq) {
> -		clk = clk_get(&pdev->dev, NULL);
> -		if (IS_ERR(clk)) {
> -			dev_err(&pdev->dev, "no clock defined\n");
> -			err = PTR_ERR(clk);
> +		clk_ipg = clk_get(&pdev->dev, "ipg");
> +		if (IS_ERR(clk_ipg)) {
> +			dev_err(&pdev->dev, "no ipg clock defined\n");
> +			err = PTR_ERR(clk_ipg);
> +			goto failed_clock;
> +		}
> +		clock_freq = clk_get_rate(clk_ipg);
> +
> +		clk_per = clk_get(&pdev->dev, "per");
> +		if (IS_ERR(clk_per)) {
> +			dev_err(&pdev->dev, "no per clock defined\n");
> +			err = PTR_ERR(clk_per);
>  			goto failed_clock;
>  		}
>   
For those only register one clk and without con_id (mx35), clk_per will 
equal to clk_ipg, how to handle this situation, modify mx35 clk tree?
> -		clock_freq = clk_get_rate(clk);
>  	}
>  
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -1039,7 +1052,8 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
>  		CAN_CTRLMODE_BERR_REPORTING;
>  	priv->base = base;
>  	priv->dev = dev;
> -	priv->clk = clk;
> +	priv->clk_ipg = clk_ipg;
> +	priv->clk_per = clk_per;
>  	priv->pdata = pdev->dev.platform_data;
>  	priv->devtype_data = devtype_data;
>  
> @@ -1067,9 +1081,11 @@ static int __devinit flexcan_probe(struct platform_device *pdev)
>   failed_map:
>  	release_mem_region(mem->start, mem_size);
>   failed_get:
> -	if (clk)
> -		clk_put(clk);
>   failed_clock:
> +	if (clk_per)
>   
Use if (!IS_ERR(clk_per))
> +		clk_put(clk_per);
> +	if (clk_ipg)
>   
Ditto.

If we use devm_clk_get(), we can save to call clk_put().
> +		clk_put(clk_ipg);
>  	return err;
>  }
>  
> @@ -1086,8 +1102,10 @@ static int __devexit flexcan_remove(struct platform_device *pdev)
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	release_mem_region(mem->start, resource_size(mem));
>  
> -	if (priv->clk)
> -		clk_put(priv->clk);
> +	if (priv->clk_per)
> +		clk_put(priv->clk_per);
> +	if (priv->clk_ipg)
> +		clk_put(priv->clk_ipg);
>  
>  	free_candev(dev);
>  
>

Marc Kleine-Budde July 18, 2012, 8:37 a.m. UTC | #2

On 07/18/2012 04:12 AM, Hui Wang wrote:
> Marc Kleine-Budde wrote:
>> From: Steffen Trumtrar <[e-mail]>
>>
>> This patch add support for a second clock to the flexcan driver. On
>> modern
>> freescale ARM cores like the imx53 and imx6q two clocks ("ipg" and "per")
>> must be enabled in order to access the CAN core.
>>
>> In the original driver, the clock was requested without specifying the
>> connection id, further all mainline ARM archs with flexcan support
>> (imx28, imx25, imx35) register their flexcan clock without a
>> connection id,
>> too.
>>
>> This patch first renames the existing clk variable to clk_ipg and adds
>> the
>> connection id "ipg" to the clk_get() call. Then a second clock "per" is
>> requested. As all archs don't specify a connection id, both clk_get
>> return
>> the same clock. This ensures compatibility to existing flexcan support
>> and adds support for imx53 at the same time.
>>
>> After this patch hits mainline, the archs may give their existing flexcan
>> clock the "ipg" connection id and implement a dummy "per" clock.
>>
>> This patch has been tested on imx28 (unmodified clk tree) and on imx53
>> with a seperate "ipg" and "per" clock.
>>
>> Cc: Sascha Hauer <[e-mail]>
>> Cc: Shawn Guo <[e-mail]>
>> Cc: Hui Wang <[e-mail]>
>> Signed-off-by: Steffen Trumtrar <[e-mail]>
>> Signed-off-by: Marc Kleine-Budde <[e-mail]>
>> ---
>>  drivers/net/can/flexcan.c |   52
>> ++++++++++++++++++++++++++++++---------------
>>  1 file changed, 35 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>> index c8a6fc7..281d51f 100644
>> --- a/drivers/net/can/flexcan.c
>> +++ b/drivers/net/can/flexcan.c
>> @@ -190,7 +190,8 @@ struct flexcan_priv {
>>
>>   
> [...]
>>      if (!clock_freq) {
>> -        clk = clk_get(&pdev->dev, NULL);
>> -        if (IS_ERR(clk)) {
>> -            dev_err(&pdev->dev, "no clock defined\n");
>> -            err = PTR_ERR(clk);
>> +        clk_ipg = clk_get(&pdev->dev, "ipg");
>> +        if (IS_ERR(clk_ipg)) {
>> +            dev_err(&pdev->dev, "no ipg clock defined\n");
>> +            err = PTR_ERR(clk_ipg);
>> +            goto failed_clock;
>> +        }
>> +        clock_freq = clk_get_rate(clk_ipg);
>> +
>> +        clk_per = clk_get(&pdev->dev, "per");
>> +        if (IS_ERR(clk_per)) {
>> +            dev_err(&pdev->dev, "no per clock defined\n");
>> +            err = PTR_ERR(clk_per);
>>              goto failed_clock;
>>          }
>>   
> For those only register one clk and without con_id (mx35), clk_per will
> equal to clk_ipg, how to handle this situation, modify mx35 clk tree?

This isn't a problem, the clock is just enabled twice. As soon as this
patch is mainline the clock tree on the one-clock-for-flexcan archs can
modify their clock tree. I've commented on this in the commit message,
have you read it?

>> -        clock_freq = clk_get_rate(clk);
>>      }
>>  
>>      mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> @@ -1039,7 +1052,8 @@ static int __devinit flexcan_probe(struct
>> platform_device *pdev)
>>          CAN_CTRLMODE_BERR_REPORTING;
>>      priv->base = base;
>>      priv->dev = dev;
>> -    priv->clk = clk;
>> +    priv->clk_ipg = clk_ipg;
>> +    priv->clk_per = clk_per;
>>      priv->pdata = pdev->dev.platform_data;
>>      priv->devtype_data = devtype_data;
>>  
>> @@ -1067,9 +1081,11 @@ static int __devinit flexcan_probe(struct
>> platform_device *pdev)
>>   failed_map:
>>      release_mem_region(mem->start, mem_size);
>>   failed_get:
>> -    if (clk)
>> -        clk_put(clk);
>>   failed_clock:
>> +    if (clk_per)
>>   
> Use if (!IS_ERR(clk_per))

Yes, good catch.

Is it allowed to call clk_put with a NULL pointer? Both clocks can be
NULL, if the frequency is defined via the device tree, this is case for
powerpc.

>> +        clk_put(clk_per);
>> +    if (clk_ipg)
>>   
> Ditto.
> 
> If we use devm_clk_get(), we can save to call clk_put().

Even better. I think I'll send a separate patch to convert the whole
driver to devm.

>> +        clk_put(clk_ipg);
>>      return err;
>>  }
>>  
>> @@ -1086,8 +1102,10 @@ static int __devexit flexcan_remove(struct
>> platform_device *pdev)
>>      mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>      release_mem_region(mem->start, resource_size(mem));
>>  
>> -    if (priv->clk)
>> -        clk_put(priv->clk);
>> +    if (priv->clk_per)
>> +        clk_put(priv->clk_per);
>> +    if (priv->clk_ipg)
>> +        clk_put(priv->clk_ipg);

This will go away with devm then :)
>>  
>>      free_candev(dev);
>>  
>>   
> 
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-can" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marc

Jason Wang July 18, 2012, 8:48 a.m. UTC | #3

Marc Kleine-Budde wrote:
> On 07/18/2012 04:12 AM, Hui Wang wrote:
>   
>> Marc Kleine-Budde wrote:
>>     
>>> From: Steffen Trumtrar <[e-mail]>
>>>
>>> This patch add support for a second clock to the flexcan driver. On
>>> modern
>>> freescale ARM cores like the imx53 and imx6q two clocks ("ipg" and "per")
>>> must be enabled in order to access the CAN core.
>>>
>>> In the original driver, the clock was requested without specifying the
>>> connection id, further all mainline ARM archs with flexcan support
>>> (imx28, imx25, imx35) register their flexcan clock without a
>>> connection id,
>>> too.
>>>
>>> This patch first renames the existing clk variable to clk_ipg and adds
>>> the
>>> connection id "ipg" to the clk_get() call. Then a second clock "per" is
>>> requested. As all archs don't specify a connection id, both clk_get
>>> return
>>> the same clock. This ensures compatibility to existing flexcan support
>>> and adds support for imx53 at the same time.
>>>
>>> After this patch hits mainline, the archs may give their existing flexcan
>>> clock the "ipg" connection id and implement a dummy "per" clock.
>>>
>>> This patch has been tested on imx28 (unmodified clk tree) and on imx53
>>> with a seperate "ipg" and "per" clock.
>>>
>>> Cc: Sascha Hauer <[e-mail]>
>>> Cc: Shawn Guo <[e-mail]>
>>> Cc: Hui Wang <[e-mail]>
>>> Signed-off-by: Steffen Trumtrar <[e-mail]>
>>> Signed-off-by: Marc Kleine-Budde <[e-mail]>
>>> ---
>>>  drivers/net/can/flexcan.c |   52
>>> ++++++++++++++++++++++++++++++---------------
>>>  1 file changed, 35 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
>>> index c8a6fc7..281d51f 100644
>>> --- a/drivers/net/can/flexcan.c
>>> +++ b/drivers/net/can/flexcan.c
>>> @@ -190,7 +190,8 @@ struct flexcan_priv {
>>>
>>>   
>>>       
>> [...]
>>     
>>>      if (!clock_freq) {
>>> -        clk = clk_get(&pdev->dev, NULL);
>>> -        if (IS_ERR(clk)) {
>>> -            dev_err(&pdev->dev, "no clock defined\n");
>>> -            err = PTR_ERR(clk);
>>> +        clk_ipg = clk_get(&pdev->dev, "ipg");
>>> +        if (IS_ERR(clk_ipg)) {
>>> +            dev_err(&pdev->dev, "no ipg clock defined\n");
>>> +            err = PTR_ERR(clk_ipg);
>>> +            goto failed_clock;
>>> +        }
>>> +        clock_freq = clk_get_rate(clk_ipg);
>>> +
>>> +        clk_per = clk_get(&pdev->dev, "per");
>>> +        if (IS_ERR(clk_per)) {
>>> +            dev_err(&pdev->dev, "no per clock defined\n");
>>> +            err = PTR_ERR(clk_per);
>>>              goto failed_clock;
>>>          }
>>>   
>>>       
>> For those only register one clk and without con_id (mx35), clk_per will
>> equal to clk_ipg, how to handle this situation, modify mx35 clk tree?
>>     
>
> This isn't a problem, the clock is just enabled twice. As soon as this
> patch is mainline the clock tree on the one-clock-for-flexcan archs can
> modify their clock tree. I've commented on this in the commit message,
> have you read it?
>   
Got it now. :-)

Sascha Hauer July 18, 2012, 8:49 a.m. UTC | #4

On Wed, Jul 18, 2012 at 10:37:22AM +0200, Marc Kleine-Budde wrote:
> 
> Yes, good catch.
> 
> Is it allowed to call clk_put with a NULL pointer?

As NULL is a valid clock by definition it is allowed to do this.
However, calling clk_put with a NULL pointer when a corresponding
call to clk_get never returned NULL is probably not a good idea.

Sascha

Lothar Waßmann July 18, 2012, 9:05 a.m. UTC | #5

Hi,

Marc Kleine-Budde writes:
> On 07/18/2012 04:12 AM, Hui Wang wrote:
> > Marc Kleine-Budde wrote:
> >> From: Steffen Trumtrar <[e-mail]>
> >>
> >> This patch add support for a second clock to the flexcan driver. On
> >> modern
> >> freescale ARM cores like the imx53 and imx6q two clocks ("ipg" and "per")
> >> must be enabled in order to access the CAN core.
> >> -        clock_freq = clk_get_rate(clk);
> >>      }
> >>  
> >>      mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> @@ -1039,7 +1052,8 @@ static int __devinit flexcan_probe(struct
> >> platform_device *pdev)
> >>          CAN_CTRLMODE_BERR_REPORTING;
> >>      priv->base = base;
> >>      priv->dev = dev;
> >> -    priv->clk = clk;
> >> +    priv->clk_ipg = clk_ipg;
> >> +    priv->clk_per = clk_per;
> >>      priv->pdata = pdev->dev.platform_data;
> >>      priv->devtype_data = devtype_data;
> >>  
> >> @@ -1067,9 +1081,11 @@ static int __devinit flexcan_probe(struct
> >> platform_device *pdev)
> >>   failed_map:
> >>      release_mem_region(mem->start, mem_size);
> >>   failed_get:
> >> -    if (clk)
> >> -        clk_put(clk);
> >>   failed_clock:
> >> +    if (clk_per)
> >>   
> > Use if (!IS_ERR(clk_per))
> 
> Yes, good catch.
> 
> Is it allowed to call clk_put with a NULL pointer? Both clocks can be
> NULL, if the frequency is defined via the device tree, this is case for
> powerpc.
> 
This has been discussed here already several times. Everything
returned by clk_get() that does not satisfy the IS_ERR() check, is a
valid clk cookie and can be used for the clk API calls.


Lothar Waßmann

Marc Kleine-Budde July 18, 2012, 9:21 a.m. UTC | #6

On 07/18/2012 11:05 AM, Lothar Waßmann wrote:
> Hi,
> 
> Marc Kleine-Budde writes:
>> On 07/18/2012 04:12 AM, Hui Wang wrote:
>>> Marc Kleine-Budde wrote:
>>>> From: Steffen Trumtrar <[e-mail]>
>>>>
>>>> This patch add support for a second clock to the flexcan driver. On
>>>> modern
>>>> freescale ARM cores like the imx53 and imx6q two clocks ("ipg" and "per")
>>>> must be enabled in order to access the CAN core.
>>>> -        clock_freq = clk_get_rate(clk);
>>>>      }
>>>>  
>>>>      mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> @@ -1039,7 +1052,8 @@ static int __devinit flexcan_probe(struct
>>>> platform_device *pdev)
>>>>          CAN_CTRLMODE_BERR_REPORTING;
>>>>      priv->base = base;
>>>>      priv->dev = dev;
>>>> -    priv->clk = clk;
>>>> +    priv->clk_ipg = clk_ipg;
>>>> +    priv->clk_per = clk_per;
>>>>      priv->pdata = pdev->dev.platform_data;
>>>>      priv->devtype_data = devtype_data;
>>>>  
>>>> @@ -1067,9 +1081,11 @@ static int __devinit flexcan_probe(struct
>>>> platform_device *pdev)
>>>>   failed_map:
>>>>      release_mem_region(mem->start, mem_size);
>>>>   failed_get:
>>>> -    if (clk)
>>>> -        clk_put(clk);
>>>>   failed_clock:
>>>> +    if (clk_per)
>>>>   
>>> Use if (!IS_ERR(clk_per))
>>
>> Yes, good catch.
>>
>> Is it allowed to call clk_put with a NULL pointer? Both clocks can be
>> NULL, if the frequency is defined via the device tree, this is case for
>> powerpc.
>>
> This has been discussed here already several times. Everything
> returned by clk_get() that does not satisfy the IS_ERR() check, is a
> valid clk cookie and can be used for the clk API calls.

I've switched to devm completely (see Patch v2), so the clk_put is obsolete.

In this particular driver the NULL pointer in question is not returned
via the clk_get(). On powerpc we don't use clk_get(), because it's not
working (AFIAK) on the p1010 (the powerpc arch that has a flexcan).

Marc

Patch

diff --git a/drivers/net/can/flexcan.c b/drivers/net/can/flexcan.c
index c8a6fc7..281d51f 100644
--- a/drivers/net/can/flexcan.c
+++ b/drivers/net/can/flexcan.c
@@ -190,7 +190,8 @@  struct flexcan_priv {
 	u32 reg_esr;
 	u32 reg_ctrl_default;
 
-	struct clk *clk;
+	struct clk *clk_ipg;
+	struct clk *clk_per;
 	struct flexcan_platform_data *pdata;
 	const struct flexcan_devtype_data *devtype_data;
 };
@@ -828,7 +829,8 @@  static int flexcan_open(struct net_device *dev)
 	struct flexcan_priv *priv = netdev_priv(dev);
 	int err;
 
-	clk_prepare_enable(priv->clk);
+	clk_prepare_enable(priv->clk_ipg);
+	clk_prepare_enable(priv->clk_per);
 
 	err = open_candev(dev);
 	if (err)
@@ -850,7 +852,8 @@  static int flexcan_open(struct net_device *dev)
  out_close:
 	close_candev(dev);
  out:
-	clk_disable_unprepare(priv->clk);
+	clk_disable_unprepare(priv->clk_per);
+	clk_disable_unprepare(priv->clk_ipg);
 
 	return err;
 }
@@ -864,7 +867,8 @@  static int flexcan_close(struct net_device *dev)
 	flexcan_chip_stop(dev);
 
 	free_irq(dev->irq, dev);
-	clk_disable_unprepare(priv->clk);
+	clk_disable_unprepare(priv->clk_per);
+	clk_disable_unprepare(priv->clk_ipg);
 
 	close_candev(dev);
 
@@ -903,7 +907,8 @@  static int __devinit register_flexcandev(struct net_device *dev)
 	struct flexcan_regs __iomem *regs = priv->base;
 	u32 reg, err;
 
-	clk_prepare_enable(priv->clk);
+	clk_prepare_enable(priv->clk_ipg);
+	clk_prepare_enable(priv->clk_per);
 
 	/* select "bus clock", chip must be disabled */
 	flexcan_chip_disable(priv);
@@ -936,7 +941,8 @@  static int __devinit register_flexcandev(struct net_device *dev)
  out:
 	/* disable core and turn off clocks */
 	flexcan_chip_disable(priv);
-	clk_disable_unprepare(priv->clk);
+	clk_disable_unprepare(priv->clk_per);
+	clk_disable_unprepare(priv->clk_ipg);
 
 	return err;
 }
@@ -964,7 +970,7 @@  static int __devinit flexcan_probe(struct platform_device *pdev)
 	struct net_device *dev;
 	struct flexcan_priv *priv;
 	struct resource *mem;
-	struct clk *clk = NULL;
+	struct clk *clk_ipg = NULL, *clk_per = NULL;
 	struct pinctrl *pinctrl;
 	void __iomem *base;
 	resource_size_t mem_size;
@@ -980,13 +986,20 @@  static int __devinit flexcan_probe(struct platform_device *pdev)
 						"clock-frequency", &clock_freq);
 
 	if (!clock_freq) {
-		clk = clk_get(&pdev->dev, NULL);
-		if (IS_ERR(clk)) {
-			dev_err(&pdev->dev, "no clock defined\n");
-			err = PTR_ERR(clk);
+		clk_ipg = clk_get(&pdev->dev, "ipg");
+		if (IS_ERR(clk_ipg)) {
+			dev_err(&pdev->dev, "no ipg clock defined\n");
+			err = PTR_ERR(clk_ipg);
+			goto failed_clock;
+		}
+		clock_freq = clk_get_rate(clk_ipg);
+
+		clk_per = clk_get(&pdev->dev, "per");
+		if (IS_ERR(clk_per)) {
+			dev_err(&pdev->dev, "no per clock defined\n");
+			err = PTR_ERR(clk_per);
 			goto failed_clock;
 		}
-		clock_freq = clk_get_rate(clk);
 	}
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1039,7 +1052,8 @@  static int __devinit flexcan_probe(struct platform_device *pdev)
 		CAN_CTRLMODE_BERR_REPORTING;
 	priv->base = base;
 	priv->dev = dev;
-	priv->clk = clk;
+	priv->clk_ipg = clk_ipg;
+	priv->clk_per = clk_per;
 	priv->pdata = pdev->dev.platform_data;
 	priv->devtype_data = devtype_data;
 
@@ -1067,9 +1081,11 @@  static int __devinit flexcan_probe(struct platform_device *pdev)
  failed_map:
 	release_mem_region(mem->start, mem_size);
  failed_get:
-	if (clk)
-		clk_put(clk);
  failed_clock:
+	if (clk_per)
+		clk_put(clk_per);
+	if (clk_ipg)
+		clk_put(clk_ipg);
 	return err;
 }
 
@@ -1086,8 +1102,10 @@  static int __devexit flexcan_remove(struct platform_device *pdev)
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	release_mem_region(mem->start, resource_size(mem));
 
-	if (priv->clk)
-		clk_put(priv->clk);
+	if (priv->clk_per)
+		clk_put(priv->clk_per);
+	if (priv->clk_ipg)
+		clk_put(priv->clk_ipg);
 
 	free_candev(dev);