Re: [PATCH v8] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller

[Marc Kleine-Budde <mkl@pengutronix.de>]

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

On 08/20/2012 01:06 PM, AnilKumar, Chimata wrote:
> On Mon, Aug 20, 2012 at 16:33:43, AnilKumar, Chimata wrote:
>> On Mon, Aug 20, 2012 at 16:27:53, Marc Kleine-Budde wrote:
>>> On 08/20/2012 12:41 PM, Marc Kleine-Budde wrote:
>>>> On 08/20/2012 10:45 AM, AnilKumar Ch wrote:
>>>>> Add Runtime PM support to C_CAN/D_CAN controller. The runtime PM
>>>>> APIs control clocks for C_CAN/D_CAN IP and prevent access to the
>>>>> register of C_CAN/D_CAN IP when clock is turned off.
>>>>>
>>>>> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
>>>>> ---
>>>>> This patch has been tested on AM335X EVM. Due to lack of hardware
>>>>> I am not able to test c_can functionality. I appreciate if anyone
>>>>> can test c_can functionality with this patch.
>>>>>
>>>>> This patch is based on "can-next/master" 
>>>>>
>>>>> Changes from v7:
>>>>> 	- Incorporated Marc's commets on v7
>>>>> 	  * changed device pointer to c_can_priv pointer
>>>>
>>>> looks good.
>>>>
>>>> [...]
>>>>
>>>>> --- a/drivers/net/can/c_can/c_can_platform.c
>>>>> +++ b/drivers/net/can/c_can/c_can_platform.c
>>>>> @@ -32,6 +32,7 @@
>>>>>  #include <linux/clk.h>
>>>>>  #include <linux/of.h>
>>>>>  #include <linux/of_device.h>
>>>>> +#include <linux/pm_runtime.h>
>>>>>  
>>>>>  #include <linux/can/dev.h>
>>>>>  
>>>>> @@ -177,8 +178,11 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>>>>>  		goto exit_free_device;
>>>>                      ^^^^^^^^^^^^^^^^
>>>>>  	}
>>>>>  
>>>>> +	pm_runtime_enable(&pdev->dev);
>>>>> +
>>>>>  	dev->irq = irq;
>>>>>  	priv->base = addr;
>>>>> +	priv->device = &pdev->dev;
>>>>>  	priv->can.clock.freq = clk_get_rate(clk);
>>>>>  	priv->priv = clk;
>>>>>  
>>>>> @@ -198,6 +202,7 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>>>>>  
>>>>>  exit_free_device:
>>>>>  	platform_set_drvdata(pdev, NULL);
>>>>> +	pm_runtime_disable(&pdev->dev);
>>>>
>>>> I think this will end up in a NULL pointer deferf.
>>>
>>> Should be no problem, as you're using pdev->dev not priv->device.
>>>
>>> Sorry for the noise :)
>>>
>>
>> One more catch here, return path is not proper.
>>
>> exit_free_device: should contain only free_c_can_dev(dev);
> 
> And second goto exit_free_device; call should modify to
> goto exit_clear_drvdata;
> 
> return path looks like this
> 
> exit_clear_drvdata:
> 	platform_set_drvdata(pdev, NULL);
> 	pm_runtime_disable(&pdev->dev);
> exit_free_device:
> 	free_c_can_dev(dev);
> ...

yes, please send a V9


-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


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