* [PATCH v9] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
@ 2012-08-20 11:20 AnilKumar Ch
2012-08-27 5:06 ` AnilKumar, Chimata
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: AnilKumar Ch @ 2012-08-20 11:20 UTC (permalink / raw)
To: wg, mkl, linux-can; +Cc: linux-omap, anantgole, nsekhar, AnilKumar Ch
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 v8:
- corrected the return path sequence in c_can_probe()
Changes from v7:
- Incorporated Marc's comments on v7
* changed device pointer to c_can_priv pointer
Changes from v6:
- Incorporated Marc's comments on v6
* changed dev pointer to priv
* removed platform_device.h include from c_can.c
Changes from v5:
- Incorporated Marc's comments on v5
* changed runtime pm calls in c_can driver to handle
the drivers which are not using platform drivers.
* added device pointer protection in c_can driver if
not passed from platform/pci driver.
Changes from v4:
- Incorporated Vaibhav H review comments on v4.
* Moved pm_runtime put/get_sync calls to appropriate positions.
- This patch is from "Add DT support to C_CAN/D_CAN controller"
patch series. Rest of the patches in this series were applied
so this v5 contains only this patch.
drivers/net/can/c_can/c_can.c | 24 +++++++++++++++++++++++-
drivers/net/can/c_can/c_can.h | 1 +
drivers/net/can/c_can/c_can_platform.c | 11 +++++++++--
3 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
index 4c538e3..966d318 100644
--- a/drivers/net/can/c_can/c_can.c
+++ b/drivers/net/can/c_can/c_can.c
@@ -34,6 +34,7 @@
#include <linux/if_ether.h>
#include <linux/list.h>
#include <linux/io.h>
+#include <linux/pm_runtime.h>
#include <linux/can.h>
#include <linux/can/dev.h>
@@ -201,6 +202,18 @@ static const struct can_bittiming_const c_can_bittiming_const = {
.brp_inc = 1,
};
+static inline void c_can_pm_runtime_get_sync(struct c_can_priv *priv)
+{
+ if (priv->device)
+ pm_runtime_get_sync(priv->device);
+}
+
+static inline void c_can_pm_runtime_put_sync(struct c_can_priv *priv)
+{
+ if (priv->device)
+ pm_runtime_put_sync(priv->device);
+}
+
static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
{
return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) +
@@ -673,11 +686,15 @@ static int c_can_get_berr_counter(const struct net_device *dev,
unsigned int reg_err_counter;
struct c_can_priv *priv = netdev_priv(dev);
+ c_can_pm_runtime_get_sync(priv);
+
reg_err_counter = priv->read_reg(priv, C_CAN_ERR_CNT_REG);
bec->rxerr = (reg_err_counter & ERR_CNT_REC_MASK) >>
ERR_CNT_REC_SHIFT;
bec->txerr = reg_err_counter & ERR_CNT_TEC_MASK;
+ c_can_pm_runtime_put_sync(priv);
+
return 0;
}
@@ -1053,11 +1070,13 @@ static int c_can_open(struct net_device *dev)
int err;
struct c_can_priv *priv = netdev_priv(dev);
+ c_can_pm_runtime_get_sync(priv);
+
/* open the can device */
err = open_candev(dev);
if (err) {
netdev_err(dev, "failed to open can device\n");
- return err;
+ goto exit_open_fail;
}
/* register interrupt handler */
@@ -1079,6 +1098,8 @@ static int c_can_open(struct net_device *dev)
exit_irq_fail:
close_candev(dev);
+exit_open_fail:
+ c_can_pm_runtime_put_sync(priv);
return err;
}
@@ -1091,6 +1112,7 @@ static int c_can_close(struct net_device *dev)
c_can_stop(dev);
free_irq(dev->irq, dev);
close_candev(dev);
+ c_can_pm_runtime_put_sync(priv);
return 0;
}
diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
index 4e56baa..1437a6d 100644
--- a/drivers/net/can/c_can/c_can.h
+++ b/drivers/net/can/c_can/c_can.h
@@ -153,6 +153,7 @@ struct c_can_priv {
struct can_priv can; /* must be the first member */
struct napi_struct napi;
struct net_device *dev;
+ struct device *device;
int tx_object;
int current_status;
int last_status;
diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
index d0a66cf..90801c4 100644
--- 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;
@@ -189,15 +193,17 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
if (ret) {
dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
KBUILD_MODNAME, ret);
- goto exit_free_device;
+ goto exit_clear_drvdata;
}
dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
KBUILD_MODNAME, priv->base, dev->irq);
return 0;
-exit_free_device:
+exit_clear_drvdata:
platform_set_drvdata(pdev, NULL);
+ pm_runtime_disable(&pdev->dev);
+exit_free_device:
free_c_can_dev(dev);
exit_iounmap:
iounmap(addr);
@@ -226,6 +232,7 @@ 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.9.5
^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH v9] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
2012-08-20 11:20 [PATCH v9] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller AnilKumar Ch
@ 2012-08-27 5:06 ` AnilKumar, Chimata
2012-09-03 8:52 ` Marc Kleine-Budde
2012-09-06 23:37 ` Kevin Hilman
2 siblings, 0 replies; 8+ messages in thread
From: AnilKumar, Chimata @ 2012-08-27 5:06 UTC (permalink / raw)
To: AnilKumar, Chimata, wg, mkl, linux-can
Cc: linux-omap, Gole, Anant, Nori, Sekhar
Hi Marc,
On Mon, Aug 20, 2012 at 16:50:54, AnilKumar, Chimata 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 v8:
> - corrected the return path sequence in c_can_probe()
>
> Changes from v7:
> - Incorporated Marc's comments on v7
> * changed device pointer to c_can_priv pointer
>
> Changes from v6:
> - Incorporated Marc's comments on v6
> * changed dev pointer to priv
> * removed platform_device.h include from c_can.c
>
> Changes from v5:
> - Incorporated Marc's comments on v5
> * changed runtime pm calls in c_can driver to handle
> the drivers which are not using platform drivers.
> * added device pointer protection in c_can driver if
> not passed from platform/pci driver.
>
> Changes from v4:
> - Incorporated Vaibhav H review comments on v4.
> * Moved pm_runtime put/get_sync calls to appropriate positions.
> - This patch is from "Add DT support to C_CAN/D_CAN controller"
> patch series. Rest of the patches in this series were applied
> so this v5 contains only this patch.
>
> drivers/net/can/c_can/c_can.c | 24 +++++++++++++++++++++++-
> drivers/net/can/c_can/c_can.h | 1 +
> drivers/net/can/c_can/c_can_platform.c | 11 +++++++++--
> 3 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 4c538e3..966d318 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -34,6 +34,7 @@
> #include <linux/if_ether.h>
> #include <linux/list.h>
> #include <linux/io.h>
> +#include <linux/pm_runtime.h>
>
> #include <linux/can.h>
> #include <linux/can/dev.h>
> @@ -201,6 +202,18 @@ static const struct can_bittiming_const c_can_bittiming_const = {
> .brp_inc = 1,
> };
>
> +static inline void c_can_pm_runtime_get_sync(struct c_can_priv *priv)
> +{
> + if (priv->device)
> + pm_runtime_get_sync(priv->device);
> +}
> +
> +static inline void c_can_pm_runtime_put_sync(struct c_can_priv *priv)
> +{
> + if (priv->device)
> + pm_runtime_put_sync(priv->device);
> +}
> +
> static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
> {
> return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) +
> @@ -673,11 +686,15 @@ static int c_can_get_berr_counter(const struct net_device *dev,
> unsigned int reg_err_counter;
> struct c_can_priv *priv = netdev_priv(dev);
>
> + c_can_pm_runtime_get_sync(priv);
> +
> reg_err_counter = priv->read_reg(priv, C_CAN_ERR_CNT_REG);
> bec->rxerr = (reg_err_counter & ERR_CNT_REC_MASK) >>
> ERR_CNT_REC_SHIFT;
> bec->txerr = reg_err_counter & ERR_CNT_TEC_MASK;
>
> + c_can_pm_runtime_put_sync(priv);
> +
> return 0;
> }
>
> @@ -1053,11 +1070,13 @@ static int c_can_open(struct net_device *dev)
> int err;
> struct c_can_priv *priv = netdev_priv(dev);
>
> + c_can_pm_runtime_get_sync(priv);
> +
> /* open the can device */
> err = open_candev(dev);
> if (err) {
> netdev_err(dev, "failed to open can device\n");
> - return err;
> + goto exit_open_fail;
> }
>
> /* register interrupt handler */
> @@ -1079,6 +1098,8 @@ static int c_can_open(struct net_device *dev)
>
> exit_irq_fail:
> close_candev(dev);
> +exit_open_fail:
> + c_can_pm_runtime_put_sync(priv);
> return err;
> }
>
> @@ -1091,6 +1112,7 @@ static int c_can_close(struct net_device *dev)
> c_can_stop(dev);
> free_irq(dev->irq, dev);
> close_candev(dev);
> + c_can_pm_runtime_put_sync(priv);
>
> return 0;
> }
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 4e56baa..1437a6d 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -153,6 +153,7 @@ struct c_can_priv {
> struct can_priv can; /* must be the first member */
> struct napi_struct napi;
> struct net_device *dev;
> + struct device *device;
> int tx_object;
> int current_status;
> int last_status;
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index d0a66cf..90801c4 100644
> --- 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;
>
> @@ -189,15 +193,17 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
> if (ret) {
> dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> KBUILD_MODNAME, ret);
> - goto exit_free_device;
> + goto exit_clear_drvdata;
> }
>
> dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> KBUILD_MODNAME, priv->base, dev->irq);
> return 0;
>
> -exit_free_device:
> +exit_clear_drvdata:
> platform_set_drvdata(pdev, NULL);
> + pm_runtime_disable(&pdev->dev);
> +exit_free_device:
> free_c_can_dev(dev);
> exit_iounmap:
> iounmap(addr);
> @@ -226,6 +232,7 @@ 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;
> --
If there are no comments can you please push this patch
Thanks
AnilKumar
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v9] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
2012-08-20 11:20 [PATCH v9] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller AnilKumar Ch
2012-08-27 5:06 ` AnilKumar, Chimata
@ 2012-09-03 8:52 ` Marc Kleine-Budde
2012-09-03 10:52 ` AnilKumar, Chimata
2012-09-06 23:37 ` Kevin Hilman
2 siblings, 1 reply; 8+ messages in thread
From: Marc Kleine-Budde @ 2012-09-03 8:52 UTC (permalink / raw)
To: AnilKumar Ch; +Cc: wg, linux-can, linux-omap, anantgole, nsekhar
[-- Attachment #1: Type: text/plain, Size: 6581 bytes --]
On 08/20/2012 01:20 PM, 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.
Looks good. I've marked the c_can_priv *priv pointer as const in
c_can_pm_runtime_{get,put}_sync.
thanks, Marc
>
> This patch is based on "can-next/master"
>
> Changes from v8:
> - corrected the return path sequence in c_can_probe()
>
> Changes from v7:
> - Incorporated Marc's comments on v7
> * changed device pointer to c_can_priv pointer
>
> Changes from v6:
> - Incorporated Marc's comments on v6
> * changed dev pointer to priv
> * removed platform_device.h include from c_can.c
>
> Changes from v5:
> - Incorporated Marc's comments on v5
> * changed runtime pm calls in c_can driver to handle
> the drivers which are not using platform drivers.
> * added device pointer protection in c_can driver if
> not passed from platform/pci driver.
>
> Changes from v4:
> - Incorporated Vaibhav H review comments on v4.
> * Moved pm_runtime put/get_sync calls to appropriate positions.
> - This patch is from "Add DT support to C_CAN/D_CAN controller"
> patch series. Rest of the patches in this series were applied
> so this v5 contains only this patch.
>
> drivers/net/can/c_can/c_can.c | 24 +++++++++++++++++++++++-
> drivers/net/can/c_can/c_can.h | 1 +
> drivers/net/can/c_can/c_can_platform.c | 11 +++++++++--
> 3 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 4c538e3..966d318 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -34,6 +34,7 @@
> #include <linux/if_ether.h>
> #include <linux/list.h>
> #include <linux/io.h>
> +#include <linux/pm_runtime.h>
>
> #include <linux/can.h>
> #include <linux/can/dev.h>
> @@ -201,6 +202,18 @@ static const struct can_bittiming_const c_can_bittiming_const = {
> .brp_inc = 1,
> };
>
> +static inline void c_can_pm_runtime_get_sync(struct c_can_priv *priv)
I've marked the priv pointer as const.
> +{
> + if (priv->device)
> + pm_runtime_get_sync(priv->device);
> +}
> +
> +static inline void c_can_pm_runtime_put_sync(struct c_can_priv *priv)
dito.
> +{
> + if (priv->device)
> + pm_runtime_put_sync(priv->device);
> +}
> +
> static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
> {
> return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) +
> @@ -673,11 +686,15 @@ static int c_can_get_berr_counter(const struct net_device *dev,
> unsigned int reg_err_counter;
> struct c_can_priv *priv = netdev_priv(dev);
>
> + c_can_pm_runtime_get_sync(priv);
> +
> reg_err_counter = priv->read_reg(priv, C_CAN_ERR_CNT_REG);
> bec->rxerr = (reg_err_counter & ERR_CNT_REC_MASK) >>
> ERR_CNT_REC_SHIFT;
> bec->txerr = reg_err_counter & ERR_CNT_TEC_MASK;
>
> + c_can_pm_runtime_put_sync(priv);
> +
> return 0;
> }
>
> @@ -1053,11 +1070,13 @@ static int c_can_open(struct net_device *dev)
> int err;
> struct c_can_priv *priv = netdev_priv(dev);
>
> + c_can_pm_runtime_get_sync(priv);
> +
> /* open the can device */
> err = open_candev(dev);
> if (err) {
> netdev_err(dev, "failed to open can device\n");
> - return err;
> + goto exit_open_fail;
> }
>
> /* register interrupt handler */
> @@ -1079,6 +1098,8 @@ static int c_can_open(struct net_device *dev)
>
> exit_irq_fail:
> close_candev(dev);
> +exit_open_fail:
> + c_can_pm_runtime_put_sync(priv);
> return err;
> }
>
> @@ -1091,6 +1112,7 @@ static int c_can_close(struct net_device *dev)
> c_can_stop(dev);
> free_irq(dev->irq, dev);
> close_candev(dev);
> + c_can_pm_runtime_put_sync(priv);
>
> return 0;
> }
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 4e56baa..1437a6d 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -153,6 +153,7 @@ struct c_can_priv {
> struct can_priv can; /* must be the first member */
> struct napi_struct napi;
> struct net_device *dev;
> + struct device *device;
> int tx_object;
> int current_status;
> int last_status;
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index d0a66cf..90801c4 100644
> --- 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;
>
> @@ -189,15 +193,17 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
> if (ret) {
> dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> KBUILD_MODNAME, ret);
> - goto exit_free_device;
> + goto exit_clear_drvdata;
> }
>
> dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> KBUILD_MODNAME, priv->base, dev->irq);
> return 0;
>
> -exit_free_device:
> +exit_clear_drvdata:
> platform_set_drvdata(pdev, NULL);
> + pm_runtime_disable(&pdev->dev);
> +exit_free_device:
> free_c_can_dev(dev);
> exit_iounmap:
> iounmap(addr);
> @@ -226,6 +232,7 @@ 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;
>
--
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] 8+ messages in thread
* RE: [PATCH v9] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
2012-09-03 8:52 ` Marc Kleine-Budde
@ 2012-09-03 10:52 ` AnilKumar, Chimata
0 siblings, 0 replies; 8+ messages in thread
From: AnilKumar, Chimata @ 2012-09-03 10:52 UTC (permalink / raw)
To: Marc Kleine-Budde; +Cc: wg, linux-can, linux-omap, Gole, Anant, Nori, Sekhar
Marc,
On Mon, Sep 03, 2012 at 14:22:22, Marc Kleine-Budde wrote:
> On 08/20/2012 01:20 PM, 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.
>
> Looks good. I've marked the c_can_priv *priv pointer as const in
> c_can_pm_runtime_{get,put}_sync.
>
Yes, that is better.
Thanks marc.
Regards
AnilKumar
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v9] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
2012-08-20 11:20 [PATCH v9] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller AnilKumar Ch
2012-08-27 5:06 ` AnilKumar, Chimata
2012-09-03 8:52 ` Marc Kleine-Budde
@ 2012-09-06 23:37 ` Kevin Hilman
2012-09-07 3:28 ` AnilKumar, Chimata
2 siblings, 1 reply; 8+ messages in thread
From: Kevin Hilman @ 2012-09-06 23:37 UTC (permalink / raw)
To: AnilKumar Ch; +Cc: wg, mkl, linux-can, linux-omap, anantgole, nsekhar
AnilKumar Ch <anilkumar@ti.com> writes:
> 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>
I'm not familar with the CAN specifics here, but some comments on the
runtime PM implementation.
> ---
> 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 v8:
> - corrected the return path sequence in c_can_probe()
>
> Changes from v7:
> - Incorporated Marc's comments on v7
> * changed device pointer to c_can_priv pointer
>
> Changes from v6:
> - Incorporated Marc's comments on v6
> * changed dev pointer to priv
> * removed platform_device.h include from c_can.c
>
> Changes from v5:
> - Incorporated Marc's comments on v5
> * changed runtime pm calls in c_can driver to handle
> the drivers which are not using platform drivers.
> * added device pointer protection in c_can driver if
> not passed from platform/pci driver.
>
> Changes from v4:
> - Incorporated Vaibhav H review comments on v4.
> * Moved pm_runtime put/get_sync calls to appropriate positions.
> - This patch is from "Add DT support to C_CAN/D_CAN controller"
> patch series. Rest of the patches in this series were applied
> so this v5 contains only this patch.
>
> drivers/net/can/c_can/c_can.c | 24 +++++++++++++++++++++++-
> drivers/net/can/c_can/c_can.h | 1 +
> drivers/net/can/c_can/c_can_platform.c | 11 +++++++++--
> 3 files changed, 33 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> index 4c538e3..966d318 100644
> --- a/drivers/net/can/c_can/c_can.c
> +++ b/drivers/net/can/c_can/c_can.c
> @@ -34,6 +34,7 @@
> #include <linux/if_ether.h>
> #include <linux/list.h>
> #include <linux/io.h>
> +#include <linux/pm_runtime.h>
>
> #include <linux/can.h>
> #include <linux/can/dev.h>
> @@ -201,6 +202,18 @@ static const struct can_bittiming_const c_can_bittiming_const = {
> .brp_inc = 1,
> };
>
> +static inline void c_can_pm_runtime_get_sync(struct c_can_priv *priv)
> +{
> + if (priv->device)
> + pm_runtime_get_sync(priv->device);
> +}
> +
> +static inline void c_can_pm_runtime_put_sync(struct c_can_priv *priv)
> +{
> + if (priv->device)
> + pm_runtime_put_sync(priv->device);
> +}
IMO, these extra helpers are rather unsightly, and should not be needed.
The driver should just be directly doing get_sync/put_sync. If
priv->device isn't presnt, then runtime PM should just never be enabled.
Kevin
> static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
> {
> return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) +
> @@ -673,11 +686,15 @@ static int c_can_get_berr_counter(const struct net_device *dev,
> unsigned int reg_err_counter;
> struct c_can_priv *priv = netdev_priv(dev);
>
> + c_can_pm_runtime_get_sync(priv);
> +
> reg_err_counter = priv->read_reg(priv, C_CAN_ERR_CNT_REG);
> bec->rxerr = (reg_err_counter & ERR_CNT_REC_MASK) >>
> ERR_CNT_REC_SHIFT;
> bec->txerr = reg_err_counter & ERR_CNT_TEC_MASK;
>
> + c_can_pm_runtime_put_sync(priv);
> +
> return 0;
> }
>
> @@ -1053,11 +1070,13 @@ static int c_can_open(struct net_device *dev)
> int err;
> struct c_can_priv *priv = netdev_priv(dev);
>
> + c_can_pm_runtime_get_sync(priv);
> +
> /* open the can device */
> err = open_candev(dev);
> if (err) {
> netdev_err(dev, "failed to open can device\n");
> - return err;
> + goto exit_open_fail;
> }
>
> /* register interrupt handler */
> @@ -1079,6 +1098,8 @@ static int c_can_open(struct net_device *dev)
>
> exit_irq_fail:
> close_candev(dev);
> +exit_open_fail:
> + c_can_pm_runtime_put_sync(priv);
> return err;
> }
>
> @@ -1091,6 +1112,7 @@ static int c_can_close(struct net_device *dev)
> c_can_stop(dev);
> free_irq(dev->irq, dev);
> close_candev(dev);
> + c_can_pm_runtime_put_sync(priv);
>
> return 0;
> }
> diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> index 4e56baa..1437a6d 100644
> --- a/drivers/net/can/c_can/c_can.h
> +++ b/drivers/net/can/c_can/c_can.h
> @@ -153,6 +153,7 @@ struct c_can_priv {
> struct can_priv can; /* must be the first member */
> struct napi_struct napi;
> struct net_device *dev;
> + struct device *device;
> int tx_object;
> int current_status;
> int last_status;
> diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> index d0a66cf..90801c4 100644
> --- 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;
>
> @@ -189,15 +193,17 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
> if (ret) {
> dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> KBUILD_MODNAME, ret);
> - goto exit_free_device;
> + goto exit_clear_drvdata;
> }
>
> dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> KBUILD_MODNAME, priv->base, dev->irq);
> return 0;
>
> -exit_free_device:
> +exit_clear_drvdata:
> platform_set_drvdata(pdev, NULL);
> + pm_runtime_disable(&pdev->dev);
> +exit_free_device:
> free_c_can_dev(dev);
> exit_iounmap:
> iounmap(addr);
> @@ -226,6 +232,7 @@ 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;
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v9] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
2012-09-06 23:37 ` Kevin Hilman
@ 2012-09-07 3:28 ` AnilKumar, Chimata
2012-09-07 21:01 ` Kevin Hilman
0 siblings, 1 reply; 8+ messages in thread
From: AnilKumar, Chimata @ 2012-09-07 3:28 UTC (permalink / raw)
To: Kevin Hilman; +Cc: wg, mkl, linux-can, linux-omap, Gole, Anant, Nori, Sekhar
Hi Kevin,
On Fri, Sep 07, 2012 at 05:07:56, Kevin Hilman wrote:
> AnilKumar Ch <anilkumar@ti.com> writes:
>
> > 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>
>
> I'm not familar with the CAN specifics here, but some comments on the
> runtime PM implementation.
Thanks for the comments.
>
> > ---
> > 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 v8:
> > - corrected the return path sequence in c_can_probe()
> >
> > Changes from v7:
> > - Incorporated Marc's comments on v7
> > * changed device pointer to c_can_priv pointer
> >
> > Changes from v6:
> > - Incorporated Marc's comments on v6
> > * changed dev pointer to priv
> > * removed platform_device.h include from c_can.c
> >
> > Changes from v5:
> > - Incorporated Marc's comments on v5
> > * changed runtime pm calls in c_can driver to handle
> > the drivers which are not using platform drivers.
> > * added device pointer protection in c_can driver if
> > not passed from platform/pci driver.
> >
> > Changes from v4:
> > - Incorporated Vaibhav H review comments on v4.
> > * Moved pm_runtime put/get_sync calls to appropriate positions.
> > - This patch is from "Add DT support to C_CAN/D_CAN controller"
> > patch series. Rest of the patches in this series were applied
> > so this v5 contains only this patch.
> >
> > drivers/net/can/c_can/c_can.c | 24 +++++++++++++++++++++++-
> > drivers/net/can/c_can/c_can.h | 1 +
> > drivers/net/can/c_can/c_can_platform.c | 11 +++++++++--
> > 3 files changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> > index 4c538e3..966d318 100644
> > --- a/drivers/net/can/c_can/c_can.c
> > +++ b/drivers/net/can/c_can/c_can.c
> > @@ -34,6 +34,7 @@
> > #include <linux/if_ether.h>
> > #include <linux/list.h>
> > #include <linux/io.h>
> > +#include <linux/pm_runtime.h>
> >
> > #include <linux/can.h>
> > #include <linux/can/dev.h>
> > @@ -201,6 +202,18 @@ static const struct can_bittiming_const c_can_bittiming_const = {
> > .brp_inc = 1,
> > };
> >
> > +static inline void c_can_pm_runtime_get_sync(struct c_can_priv *priv)
> > +{
> > + if (priv->device)
> > + pm_runtime_get_sync(priv->device);
> > +}
> > +
> > +static inline void c_can_pm_runtime_put_sync(struct c_can_priv *priv)
> > +{
> > + if (priv->device)
> > + pm_runtime_put_sync(priv->device);
> > +}
>
> IMO, these extra helpers are rather unsightly, and should not be needed.
> The driver should just be directly doing get_sync/put_sync. If
> priv->device isn't presnt, then runtime PM should just never be enabled.
>
In case of c_can driver we have two drivers one is generic c_can.c driver,
provides the basic functionality of CAN. Another two drivers c_can_platform.c
and c_can_pci.c, which uses core c_can.c driver by exporting some platform
specific ops like read/write.
priv->device pointer is passed from c_can_platform.c by this means
"priv->device = &pdev->dev;" (see below) but not for c_can_pci.c
The purpose of check here is for *_pci.c driver which do not have runtime pm
implemented yet so we should do and get_sync/put_sync. In case of *_pci.c
driver there is no pm_runtime_enable/disable once that is implemented then
this check will be removed.
Thanks
AnilKumar
>
> > static inline int get_tx_next_msg_obj(const struct c_can_priv *priv)
> > {
> > return (priv->tx_next & C_CAN_NEXT_MSG_OBJ_MASK) +
> > @@ -673,11 +686,15 @@ static int c_can_get_berr_counter(const struct net_device *dev,
> > unsigned int reg_err_counter;
> > struct c_can_priv *priv = netdev_priv(dev);
> >
> > + c_can_pm_runtime_get_sync(priv);
> > +
> > reg_err_counter = priv->read_reg(priv, C_CAN_ERR_CNT_REG);
> > bec->rxerr = (reg_err_counter & ERR_CNT_REC_MASK) >>
> > ERR_CNT_REC_SHIFT;
> > bec->txerr = reg_err_counter & ERR_CNT_TEC_MASK;
> >
> > + c_can_pm_runtime_put_sync(priv);
> > +
> > return 0;
> > }
> >
> > @@ -1053,11 +1070,13 @@ static int c_can_open(struct net_device *dev)
> > int err;
> > struct c_can_priv *priv = netdev_priv(dev);
> >
> > + c_can_pm_runtime_get_sync(priv);
> > +
> > /* open the can device */
> > err = open_candev(dev);
> > if (err) {
> > netdev_err(dev, "failed to open can device\n");
> > - return err;
> > + goto exit_open_fail;
> > }
> >
> > /* register interrupt handler */
> > @@ -1079,6 +1098,8 @@ static int c_can_open(struct net_device *dev)
> >
> > exit_irq_fail:
> > close_candev(dev);
> > +exit_open_fail:
> > + c_can_pm_runtime_put_sync(priv);
> > return err;
> > }
> >
> > @@ -1091,6 +1112,7 @@ static int c_can_close(struct net_device *dev)
> > c_can_stop(dev);
> > free_irq(dev->irq, dev);
> > close_candev(dev);
> > + c_can_pm_runtime_put_sync(priv);
> >
> > return 0;
> > }
> > diff --git a/drivers/net/can/c_can/c_can.h b/drivers/net/can/c_can/c_can.h
> > index 4e56baa..1437a6d 100644
> > --- a/drivers/net/can/c_can/c_can.h
> > +++ b/drivers/net/can/c_can/c_can.h
> > @@ -153,6 +153,7 @@ struct c_can_priv {
> > struct can_priv can; /* must be the first member */
> > struct napi_struct napi;
> > struct net_device *dev;
> > + struct device *device;
> > int tx_object;
> > int current_status;
> > int last_status;
> > diff --git a/drivers/net/can/c_can/c_can_platform.c b/drivers/net/can/c_can/c_can_platform.c
> > index d0a66cf..90801c4 100644
> > --- 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;
> >
> > @@ -189,15 +193,17 @@ static int __devinit c_can_plat_probe(struct platform_device *pdev)
> > if (ret) {
> > dev_err(&pdev->dev, "registering %s failed (err=%d)\n",
> > KBUILD_MODNAME, ret);
> > - goto exit_free_device;
> > + goto exit_clear_drvdata;
> > }
> >
> > dev_info(&pdev->dev, "%s device registered (regs=%p, irq=%d)\n",
> > KBUILD_MODNAME, priv->base, dev->irq);
> > return 0;
> >
> > -exit_free_device:
> > +exit_clear_drvdata:
> > platform_set_drvdata(pdev, NULL);
> > + pm_runtime_disable(&pdev->dev);
> > +exit_free_device:
> > free_c_can_dev(dev);
> > exit_iounmap:
> > iounmap(addr);
> > @@ -226,6 +232,7 @@ 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;
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v9] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
2012-09-07 3:28 ` AnilKumar, Chimata
@ 2012-09-07 21:01 ` Kevin Hilman
2012-09-10 10:44 ` AnilKumar, Chimata
0 siblings, 1 reply; 8+ messages in thread
From: Kevin Hilman @ 2012-09-07 21:01 UTC (permalink / raw)
To: AnilKumar, Chimata
Cc: wg, mkl, linux-can, linux-omap, Gole, Anant, Nori, Sekhar
"AnilKumar, Chimata" <anilkumar@ti.com> writes:
> Hi Kevin,
>
> On Fri, Sep 07, 2012 at 05:07:56, Kevin Hilman wrote:
>> AnilKumar Ch <anilkumar@ti.com> writes:
>>
>> > 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>
>>
>> I'm not familar with the CAN specifics here, but some comments on the
>> runtime PM implementation.
>
> Thanks for the comments.
>
>>
>> > ---
>> > 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 v8:
>> > - corrected the return path sequence in c_can_probe()
>> >
>> > Changes from v7:
>> > - Incorporated Marc's comments on v7
>> > * changed device pointer to c_can_priv pointer
>> >
>> > Changes from v6:
>> > - Incorporated Marc's comments on v6
>> > * changed dev pointer to priv
>> > * removed platform_device.h include from c_can.c
>> >
>> > Changes from v5:
>> > - Incorporated Marc's comments on v5
>> > * changed runtime pm calls in c_can driver to handle
>> > the drivers which are not using platform drivers.
>> > * added device pointer protection in c_can driver if
>> > not passed from platform/pci driver.
>> >
>> > Changes from v4:
>> > - Incorporated Vaibhav H review comments on v4.
>> > * Moved pm_runtime put/get_sync calls to appropriate positions.
>> > - This patch is from "Add DT support to C_CAN/D_CAN controller"
>> > patch series. Rest of the patches in this series were applied
>> > so this v5 contains only this patch.
>> >
>> > drivers/net/can/c_can/c_can.c | 24 +++++++++++++++++++++++-
>> > drivers/net/can/c_can/c_can.h | 1 +
>> > drivers/net/can/c_can/c_can_platform.c | 11 +++++++++--
>> > 3 files changed, 33 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
>> > index 4c538e3..966d318 100644
>> > --- a/drivers/net/can/c_can/c_can.c
>> > +++ b/drivers/net/can/c_can/c_can.c
>> > @@ -34,6 +34,7 @@
>> > #include <linux/if_ether.h>
>> > #include <linux/list.h>
>> > #include <linux/io.h>
>> > +#include <linux/pm_runtime.h>
>> >
>> > #include <linux/can.h>
>> > #include <linux/can/dev.h>
>> > @@ -201,6 +202,18 @@ static const struct can_bittiming_const c_can_bittiming_const = {
>> > .brp_inc = 1,
>> > };
>> >
>> > +static inline void c_can_pm_runtime_get_sync(struct c_can_priv *priv)
>> > +{
>> > + if (priv->device)
>> > + pm_runtime_get_sync(priv->device);
>> > +}
>> > +
>> > +static inline void c_can_pm_runtime_put_sync(struct c_can_priv *priv)
>> > +{
>> > + if (priv->device)
>> > + pm_runtime_put_sync(priv->device);
>> > +}
>>
>> IMO, these extra helpers are rather unsightly, and should not be needed.
>> The driver should just be directly doing get_sync/put_sync. If
>> priv->device isn't presnt, then runtime PM should just never be enabled.
>>
>
> In case of c_can driver we have two drivers one is generic c_can.c driver,
> provides the basic functionality of CAN. Another two drivers c_can_platform.c
> and c_can_pci.c, which uses core c_can.c driver by exporting some platform
> specific ops like read/write.
>
> priv->device pointer is passed from c_can_platform.c by this means
> "priv->device = &pdev->dev;" (see below) but not for c_can_pci.c
>
> The purpose of check here is for *_pci.c driver which do not have runtime pm
> implemented yet so we should do and get_sync/put_sync. In case of *_pci.c
> driver there is no pm_runtime_enable/disable once that is implemented then
> this check will be removed.
Then you should probably move the pm_runtime_enable/disable into the
common code (where the get_sync/put_sync) are. Then you could simply
avoid the pm_runtime_enable() if there is no priv->device.
Kevin
^ permalink raw reply [flat|nested] 8+ messages in thread
* RE: [PATCH v9] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller
2012-09-07 21:01 ` Kevin Hilman
@ 2012-09-10 10:44 ` AnilKumar, Chimata
0 siblings, 0 replies; 8+ messages in thread
From: AnilKumar, Chimata @ 2012-09-10 10:44 UTC (permalink / raw)
To: Kevin Hilman; +Cc: wg, mkl, linux-can, linux-omap, Gole, Anant, Nori, Sekhar
Hi Kevin,
On Sat, Sep 08, 2012 at 02:31:22, Kevin Hilman wrote:
> "AnilKumar, Chimata" <anilkumar@ti.com> writes:
>
> > Hi Kevin,
> >
> > On Fri, Sep 07, 2012 at 05:07:56, Kevin Hilman wrote:
> >> AnilKumar Ch <anilkumar@ti.com> writes:
> >>
> >> > 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>
> >>
> >> I'm not familar with the CAN specifics here, but some comments on the
> >> runtime PM implementation.
> >
> > Thanks for the comments.
> >
> >>
> >> > ---
> >> > 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 v8:
> >> > - corrected the return path sequence in c_can_probe()
> >> >
> >> > Changes from v7:
> >> > - Incorporated Marc's comments on v7
> >> > * changed device pointer to c_can_priv pointer
> >> >
> >> > Changes from v6:
> >> > - Incorporated Marc's comments on v6
> >> > * changed dev pointer to priv
> >> > * removed platform_device.h include from c_can.c
> >> >
> >> > Changes from v5:
> >> > - Incorporated Marc's comments on v5
> >> > * changed runtime pm calls in c_can driver to handle
> >> > the drivers which are not using platform drivers.
> >> > * added device pointer protection in c_can driver if
> >> > not passed from platform/pci driver.
> >> >
> >> > Changes from v4:
> >> > - Incorporated Vaibhav H review comments on v4.
> >> > * Moved pm_runtime put/get_sync calls to appropriate positions.
> >> > - This patch is from "Add DT support to C_CAN/D_CAN controller"
> >> > patch series. Rest of the patches in this series were applied
> >> > so this v5 contains only this patch.
> >> >
> >> > drivers/net/can/c_can/c_can.c | 24 +++++++++++++++++++++++-
> >> > drivers/net/can/c_can/c_can.h | 1 +
> >> > drivers/net/can/c_can/c_can_platform.c | 11 +++++++++--
> >> > 3 files changed, 33 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/net/can/c_can/c_can.c b/drivers/net/can/c_can/c_can.c
> >> > index 4c538e3..966d318 100644
> >> > --- a/drivers/net/can/c_can/c_can.c
> >> > +++ b/drivers/net/can/c_can/c_can.c
> >> > @@ -34,6 +34,7 @@
> >> > #include <linux/if_ether.h>
> >> > #include <linux/list.h>
> >> > #include <linux/io.h>
> >> > +#include <linux/pm_runtime.h>
> >> >
> >> > #include <linux/can.h>
> >> > #include <linux/can/dev.h>
> >> > @@ -201,6 +202,18 @@ static const struct can_bittiming_const c_can_bittiming_const = {
> >> > .brp_inc = 1,
> >> > };
> >> >
> >> > +static inline void c_can_pm_runtime_get_sync(struct c_can_priv *priv)
> >> > +{
> >> > + if (priv->device)
> >> > + pm_runtime_get_sync(priv->device);
> >> > +}
> >> > +
> >> > +static inline void c_can_pm_runtime_put_sync(struct c_can_priv *priv)
> >> > +{
> >> > + if (priv->device)
> >> > + pm_runtime_put_sync(priv->device);
> >> > +}
> >>
> >> IMO, these extra helpers are rather unsightly, and should not be needed.
> >> The driver should just be directly doing get_sync/put_sync. If
> >> priv->device isn't presnt, then runtime PM should just never be enabled.
> >>
> >
> > In case of c_can driver we have two drivers one is generic c_can.c driver,
> > provides the basic functionality of CAN. Another two drivers c_can_platform.c
> > and c_can_pci.c, which uses core c_can.c driver by exporting some platform
> > specific ops like read/write.
> >
> > priv->device pointer is passed from c_can_platform.c by this means
> > "priv->device = &pdev->dev;" (see below) but not for c_can_pci.c
> >
> > The purpose of check here is for *_pci.c driver which do not have runtime pm
> > implemented yet so we should do and get_sync/put_sync. In case of *_pci.c
> > driver there is no pm_runtime_enable/disable once that is implemented then
> > this check will be removed.
>
> Then you should probably move the pm_runtime_enable/disable into the
> common code (where the get_sync/put_sync) are. Then you could simply
> avoid the pm_runtime_enable() if there is no priv->device.
>
Thanks for the comments
I got your point, will move pm_runtime_enable/disable to common code.
Thanks
AnilKumar
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-09-10 10:44 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-08-20 11:20 [PATCH v9] can: c_can: Add runtime PM support to Bosch C_CAN/D_CAN controller AnilKumar Ch
2012-08-27 5:06 ` AnilKumar, Chimata
2012-09-03 8:52 ` Marc Kleine-Budde
2012-09-03 10:52 ` AnilKumar, Chimata
2012-09-06 23:37 ` Kevin Hilman
2012-09-07 3:28 ` AnilKumar, Chimata
2012-09-07 21:01 ` Kevin Hilman
2012-09-10 10:44 ` AnilKumar, Chimata
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.