| Message ID | 1347521292-28751-1-git-send-email-anilkumar@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)