* [PATCH] can: c_can: Move pm_runtime_enable/disable calls to common code
@ 2012-09-13 7:28 AnilKumar Ch
2012-09-13 7:48 ` Marc Kleine-Budde
2012-09-13 14:14 ` Kevin Hilman
0 siblings, 2 replies; 6+ messages in thread
From: AnilKumar Ch @ 2012-09-13 7:28 UTC (permalink / raw)
To: wg, mkl, khilman; +Cc: linux-can, linux-omap, anantgole, nsekhar, AnilKumar Ch
Move pm_runtime_enable/disable calls to c_can.c driver. Current
implementation is such that platform driver is doing pm_runtime
enable/disable and core driver is doing put_sync/get_sync.
PM runtime calls should be invoked if there is a valid device
pointer from platform driver so moving enable/disable calls
to core driver.
Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
---
Incorporated Kevin's comments on "can: c_can: Add runtime PM
support to Bosch C_CAN/D_CAN controller" patch.
This patch is tested on AM335x-EVM and cleanly applies on
linux-can master
drivers/net/can/c_can/c_can.c | 18 +++++++++++++++++-
drivers/net/can/c_can/c_can_platform.c | 5 -----
2 files changed, 17 insertions(+), 6 deletions(-)
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index aa6c5eb..e472975 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -1155,10 +1155,23 @@ static const struct net_device_ops c_can_netdev_ops = {
int register_c_can_dev(struct net_device *dev)
{
+ int ret;
+ struct c_can_priv *priv = netdev_priv(dev);
+
+ if (priv->device)
+ pm_runtime_enable(priv->device);
+
dev->flags |= IFF_ECHO; /* we support local echo */
dev->netdev_ops = &c_can_netdev_ops;
- return register_candev(dev);
+ ret = register_candev(dev);
+ if (ret) {
+ if (priv->device)
+ pm_runtime_disable(priv->device);
+ return ret;
+ }
+
+ return 0;
}
EXPORT_SYMBOL_GPL(register_c_can_dev);
@@ -1170,6 +1183,9 @@ void unregister_c_can_dev(struct net_device *dev)
c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
unregister_candev(dev);
+
+ if (priv->device)
+ pm_runtime_disable(priv->device);
}
EXPORT_SYMBOL_GPL(unregister_c_can_dev);
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index c351975..491101a 100644
--- a/drivers/net/can/c_can/c_can_platform.c
+++ b/drivers/net/can/c_can/c_can_platform.c
@@ -32,7 +32,6 @@
#include <linux/clk.h>
#include <linux/of.h>
#include <linux/of_device.h>
-#include <linux/pm_runtime.h>
#include <linux/pinctrl/consumer.h>
#include <linux/can/dev.h>
@@ -185,8 +184,6 @@ 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;
@@ -209,7 +206,6 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
exit_clear_drvdata:
platform_set_drvdata(pdev, NULL);
- pm_runtime_disable(&pdev->dev);
exit_free_device:
free_c_can_dev(dev);
exit_iounmap:
@@ -239,7 +235,6 @@ static int __devexit c_can_plat_remove(struct platform_device *pdev)
mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
release_mem_region(mem->start, resource_size(mem));
- pm_runtime_disable(&pdev->dev);
clk_put(priv->priv);
return 0;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] can: c_can: Move pm_runtime_enable/disable calls to common code
2012-09-13 7:28 [PATCH] can: c_can: Move pm_runtime_enable/disable calls to common code AnilKumar Ch
@ 2012-09-13 7:48 ` Marc Kleine-Budde
2012-09-13 8:25 ` AnilKumar, Chimata
2012-09-13 14:14 ` Kevin Hilman
1 sibling, 1 reply; 6+ messages in thread
From: Marc Kleine-Budde @ 2012-09-13 7:48 UTC (permalink / raw)
To: AnilKumar Ch; +Cc: wg, khilman, linux-can, linux-omap, anantgole, nsekhar
[-- Attachment #1: Type: text/plain, Size: 3742 bytes --]
On 09/13/2012 09:28 AM, AnilKumar Ch wrote:
> Move pm_runtime_enable/disable calls to c_can.c driver. Current
> implementation is such that platform driver is doing pm_runtime
> enable/disable and core driver is doing put_sync/get_sync.
>
> PM runtime calls should be invoked if there is a valid device
> pointer from platform driver so moving enable/disable calls
> to core driver.
>
> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> ---
> Incorporated Kevin's comments on "can: c_can: Add runtime PM
> support to Bosch C_CAN/D_CAN controller" patch.
>
> This patch is tested on AM335x-EVM and cleanly applies on
> linux-can master
I'll squash this into "can: c_can: Add runtime PM support to Bosch
C_CAN/D_CAN controller".
>
> drivers/net/can/c_can/c_can.c | 18 +++++++++++++++++-
> drivers/net/can/c_can/c_can_platform.c | 5 -----
> 2 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index aa6c5eb..e472975 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -1155,10 +1155,23 @@ static const struct net_device_ops c_can_netdev_ops = {
>
> int register_c_can_dev(struct net_device *dev)
> {
> + int ret;
> + struct c_can_priv *priv = netdev_priv(dev);
> +
> + if (priv->device)
> + pm_runtime_enable(priv->device);
I'll add a static inline for these two new function, too.
> +
> dev->flags |= IFF_ECHO; /* we support local echo */
> dev->netdev_ops = &c_can_netdev_ops;
>
> - return register_candev(dev);
> + ret = register_candev(dev);
> + if (ret) {
> + if (priv->device)
> + pm_runtime_disable(priv->device);
> + return ret;
> + }
> +
> + return 0;
> }
> EXPORT_SYMBOL_GPL(register_c_can_dev);
>
> @@ -1170,6 +1183,9 @@ void unregister_c_can_dev(struct net_device *dev)
> c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
>
> unregister_candev(dev);
> +
> + if (priv->device)
> + pm_runtime_disable(priv->device);
> }
> EXPORT_SYMBOL_GPL(unregister_c_can_dev);
>
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index c351975..491101a 100644
> --- a/drivers/net/can/c_can/c_can_platform.c
> +++ b/drivers/net/can/c_can/c_can_platform.c
> @@ -32,7 +32,6 @@
> #include <linux/clk.h>
> #include <linux/of.h>
> #include <linux/of_device.h>
> -#include <linux/pm_runtime.h>
> #include <linux/pinctrl/consumer.h>
>
> #include <linux/can/dev.h>
> @@ -185,8 +184,6 @@ 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;
> @@ -209,7 +206,6 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
>
> exit_clear_drvdata:
> platform_set_drvdata(pdev, NULL);
> - pm_runtime_disable(&pdev->dev);
When squaahsing both patches we see still some changes here, I'll fix
that, too.
> exit_free_device:
> free_c_can_dev(dev);
> exit_iounmap:
> @@ -239,7 +235,6 @@ static int __devexit c_can_plat_remove(struct platform_device *pdev)
> mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> release_mem_region(mem->start, resource_size(mem));
>
> - pm_runtime_disable(&pdev->dev);
> clk_put(priv->priv);
>
> return 0;
>
Marc
--
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: 259 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* RE: [PATCH] can: c_can: Move pm_runtime_enable/disable calls to common code
2012-09-13 7:48 ` Marc Kleine-Budde
@ 2012-09-13 8:25 ` AnilKumar, Chimata
0 siblings, 0 replies; 6+ messages in thread
From: AnilKumar, Chimata @ 2012-09-13 8:25 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: wg@grandegger.com, khilman@deeprootsystems.com,
linux-can@vger.kernel.org, linux-omap@vger.kernel.org,
Gole, Anant, Nori, Sekhar
Marc,
On Thu, Sep 13, 2012 at 13:18:07, Marc Kleine-Budde wrote:
> On 09/13/2012 09:28 AM, AnilKumar Ch wrote:
> > Move pm_runtime_enable/disable calls to c_can.c driver. Current
> > implementation is such that platform driver is doing pm_runtime
> > enable/disable and core driver is doing put_sync/get_sync.
> >
> > PM runtime calls should be invoked if there is a valid device
> > pointer from platform driver so moving enable/disable calls
> > to core driver.
> >
> > Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> > ---
> > Incorporated Kevin's comments on "can: c_can: Add runtime PM
> > support to Bosch C_CAN/D_CAN controller" patch.
> >
> > This patch is tested on AM335x-EVM and cleanly applies on
> > linux-can master
>
> I'll squash this into "can: c_can: Add runtime PM support to Bosch
> C_CAN/D_CAN controller".
> >
> > drivers/net/can/c_can/c_can.c | 18 +++++++++++++++++-
> > drivers/net/can/c_can/c_can_platform.c | 5 -----
> > 2 files changed, 17 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> > index aa6c5eb..e472975 100644
> > --- a/drivers/net/can/c_can/c_can.c
> > +++ b/drivers/net/can/c_can/c_can.c
> > @@ -1155,10 +1155,23 @@ static const struct net_device_ops c_can_netdev_ops = {
> >
> > int register_c_can_dev(struct net_device *dev)
> > {
> > + int ret;
> > + struct c_can_priv *priv = netdev_priv(dev);
> > +
> > + if (priv->device)
> > + pm_runtime_enable(priv->device);
>
> I'll add a static inline for these two new function, too.
My bad, this should be my first change.
>
> > +
> > dev->flags |= IFF_ECHO; /* we support local echo */
> > dev->netdev_ops = &c_can_netdev_ops;
> >
> > - return register_candev(dev);
> > + ret = register_candev(dev);
> > + if (ret) {
> > + if (priv->device)
> > + pm_runtime_disable(priv->device);
> > + return ret;
> > + }
> > +
> > + return 0;
> > }
> > EXPORT_SYMBOL_GPL(register_c_can_dev);
> >
> > @@ -1170,6 +1183,9 @@ void unregister_c_can_dev(struct net_device *dev)
> > c_can_enable_all_interrupts(priv, DISABLE_ALL_INTERRUPTS);
> >
> > unregister_candev(dev);
> > +
> > + if (priv->device)
> > + pm_runtime_disable(priv->device);
> > }
> > EXPORT_SYMBOL_GPL(unregister_c_can_dev);
> >
> > diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> > index c351975..491101a 100644
> > --- a/drivers/net/can/c_can/c_can_platform.c
> > +++ b/drivers/net/can/c_can/c_can_platform.c
> > @@ -32,7 +32,6 @@
> > #include <linux/clk.h>
> > #include <linux/of.h>
> > #include <linux/of_device.h>
> > -#include <linux/pm_runtime.h>
> > #include <linux/pinctrl/consumer.h>
> >
> > #include <linux/can/dev.h>
> > @@ -185,8 +184,6 @@ 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;
> > @@ -209,7 +206,6 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
> >
> > exit_clear_drvdata:
> > platform_set_drvdata(pdev, NULL);
> > - pm_runtime_disable(&pdev->dev);
>
> When squaahsing both patches we see still some changes here, I'll fix
> that, too.
Thanks Marc
Thanks
AnilKumar
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] can: c_can: Move pm_runtime_enable/disable calls to common code
2012-09-13 7:28 [PATCH] can: c_can: Move pm_runtime_enable/disable calls to common code AnilKumar Ch
2012-09-13 7:48 ` Marc Kleine-Budde
@ 2012-09-13 14:14 ` Kevin Hilman
2012-09-13 14:19 ` Marc Kleine-Budde
1 sibling, 1 reply; 6+ messages in thread
From: Kevin Hilman @ 2012-09-13 14:14 UTC (permalink / raw)
To: AnilKumar Ch; +Cc: wg, mkl, linux-can, linux-omap, anantgole, nsekhar
AnilKumar Ch <anilkumar@ti.com> writes:
> Move pm_runtime_enable/disable calls to c_can.c driver. Current
> implementation is such that platform driver is doing pm_runtime
> enable/disable and core driver is doing put_sync/get_sync.
>
> PM runtime calls should be invoked if there is a valid device
> pointer from platform driver so moving enable/disable calls
> to core driver.
>
> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
> ---
> Incorporated Kevin's comments on "can: c_can: Add runtime PM
> support to Bosch C_CAN/D_CAN controller" patch.
This looks better, but in addition, you can get rid of the
runtime PM helper functions you added (the ones that check for
priv->device) and call the pm_runtime_get/put APIs directly.
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] can: c_can: Move pm_runtime_enable/disable calls to common code
2012-09-13 14:14 ` Kevin Hilman
@ 2012-09-13 14:19 ` Marc Kleine-Budde
2012-09-13 21:35 ` Kevin Hilman
0 siblings, 1 reply; 6+ messages in thread
From: Marc Kleine-Budde @ 2012-09-13 14:19 UTC (permalink / raw)
To: Kevin Hilman; +Cc: AnilKumar Ch, wg, linux-can, linux-omap, anantgole, nsekhar
[-- Attachment #1: Type: text/plain, Size: 1212 bytes --]
On 09/13/2012 04:14 PM, Kevin Hilman wrote:
> AnilKumar Ch <anilkumar@ti.com> writes:
>
>> Move pm_runtime_enable/disable calls to c_can.c driver. Current
>> implementation is such that platform driver is doing pm_runtime
>> enable/disable and core driver is doing put_sync/get_sync.
>>
>> PM runtime calls should be invoked if there is a valid device
>> pointer from platform driver so moving enable/disable calls
>> to core driver.
>>
>> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
>> ---
>> Incorporated Kevin's comments on "can: c_can: Add runtime PM
>> support to Bosch C_CAN/D_CAN controller" patch.
>
> This looks better, but in addition, you can get rid of the
> runtime PM helper functions you added (the ones that check for
> priv->device) and call the pm_runtime_get/put APIs directly.
But priv->device might be NULL. AFAICS pm_runtime_get() is not safe to
be called with a NULL pointer.
Marc
--
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: 259 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] can: c_can: Move pm_runtime_enable/disable calls to common code
2012-09-13 14:19 ` Marc Kleine-Budde
@ 2012-09-13 21:35 ` Kevin Hilman
0 siblings, 0 replies; 6+ messages in thread
From: Kevin Hilman @ 2012-09-13 21:35 UTC (permalink / raw)
To: Marc Kleine-Budde
Cc: AnilKumar Ch, wg, linux-can, linux-omap, anantgole, nsekhar
Marc Kleine-Budde <mkl@pengutronix.de> writes:
> On 09/13/2012 04:14 PM, Kevin Hilman wrote:
>> AnilKumar Ch <anilkumar@ti.com> writes:
>>
>>> Move pm_runtime_enable/disable calls to c_can.c driver. Current
>>> implementation is such that platform driver is doing pm_runtime
>>> enable/disable and core driver is doing put_sync/get_sync.
>>>
>>> PM runtime calls should be invoked if there is a valid device
>>> pointer from platform driver so moving enable/disable calls
>>> to core driver.
>>>
>>> Signed-off-by: AnilKumar Ch <anilkumar@ti.com>
>>> ---
>>> Incorporated Kevin's comments on "can: c_can: Add runtime PM
>>> support to Bosch C_CAN/D_CAN controller" patch.
>>
>> This looks better, but in addition, you can get rid of the
>> runtime PM helper functions you added (the ones that check for
>> priv->device) and call the pm_runtime_get/put APIs directly.
>
> But priv->device might be NULL. AFAICS pm_runtime_get() is not safe to
> be called with a NULL pointer.
Yes, you're right. Guess there's not a clean way to get rid of those
helpers.
Sorry for the noise,
Kevin
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-13 21:35 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-09-13 7:28 [PATCH] can: c_can: Move pm_runtime_enable/disable calls to common code AnilKumar Ch
2012-09-13 7:48 ` Marc Kleine-Budde
2012-09-13 8:25 ` AnilKumar, Chimata
2012-09-13 14:14 ` Kevin Hilman
2012-09-13 14:19 ` Marc Kleine-Budde
2012-09-13 21:35 ` Kevin Hilman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox