[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-can
Subject:    RE: [PATCH v3] can: Convert to runtime_pm
From:       Appana Durga Kedareswara Rao <appana.durga.rao () xilinx ! com>
Date:       2014-11-28 1:49:12
Message-ID: d306383c84b9479d933b15c4b38da532 () BN1AFFO11FD025 ! protection ! gbl
[Download RAW message or body]

Hi Soren,

-----Original Message-----
From: Sören Brinkmann [mailto:soren.brinkmann@xilinx.com]
Sent: Thursday, November 27, 2014 11:54 PM
To: Appana Durga Kedareswara Rao
Cc: wg@grandegger.com; mkl@pengutronix.de; Michal Simek; grant.likely@linaro.org; \
robh+dt@kernel.org; devicetree@vger.kernel.org; netdev@vger.kernel.org; \
linux-kernel@vger.kernel.org; linux-can@vger.kernel.org; Appana Durga Kedareswara \
                Rao; linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v3] can: Convert to runtime_pm

Hi Kedar,

On Thu, 2014-11-27 at 06:38PM +0530, Kedareswara rao Appana wrote:
> Instead of enabling/disabling clocks at several locations in the
> driver, use the runtime_pm framework. This consolidates the actions
> for runtime PM in the appropriate callbacks and makes the driver more
> readable and mantainable.
> 
> Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com>
> Signed-off-by: Kedareswara rao Appana <appanad@xilinx.com>
> ---
> Changes for v3:
> - Converted the driver to use runtime_pm.
> Changes for v2:
> - Removed the struct platform_device* from suspend/resume
> as suggest by Lothar.
> 
> drivers/net/can/xilinx_can.c |  119
> +++++++++++++++++++++++++----------------
> 1 files changed, 72 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/net/can/xilinx_can.c
> b/drivers/net/can/xilinx_can.c index 8a998e3..1be28ed 100644
> --- a/drivers/net/can/xilinx_can.c
> +++ b/drivers/net/can/xilinx_can.c
> @@ -32,6 +32,7 @@
> #include <linux/can/dev.h>
> #include <linux/can/error.h>
> #include <linux/can/led.h>
> +#include <linux/pm_runtime.h>
> 
> #define DRIVER_NAME  "xilinx_can"
> 
> @@ -138,7 +139,7 @@ struct xcan_priv {
> u32 (*read_reg)(const struct xcan_priv *priv, enum xcan_reg reg);
> void (*write_reg)(const struct xcan_priv *priv, enum xcan_reg reg,
> u32 val);
> -     struct net_device *dev;
> +     struct device *dev;
> void __iomem *reg_base;
> unsigned long irq_flags;
> struct clk *bus_clk;
> @@ -842,6 +843,13 @@ static int xcan_open(struct net_device *ndev)
> struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
> 
> +     ret = pm_runtime_get_sync(priv->dev);
> +     if (ret < 0) {
> +             netdev_err(ndev, "%s: runtime CAN resume failed(%d)\n\r",

There might be other issues than the resume that make this fail. It should probably \
just say 'pm_runtime_get failed'. The CAN in the string should not be needed, the \
netdev_err macro makes sure the device name is printed. Can we have a space between \
'failed' and the error code? There should not be a '\r'

K sure will modify the error message as you explained above.

> +                             __func__, ret);
> +             return ret;
> +     }
> +
> ret = request_irq(ndev->irq, xcan_interrupt, priv->irq_flags,
> ndev->name, ndev);
> if (ret < 0) {
> @@ -849,29 +857,17 @@ static int xcan_open(struct net_device *ndev)
> goto err;
> }
> 
> -     ret = clk_prepare_enable(priv->can_clk);
> -     if (ret) {
> -             netdev_err(ndev, "unable to enable device clock\n");
> -             goto err_irq;
> -     }
> -
> -     ret = clk_prepare_enable(priv->bus_clk);
> -     if (ret) {
> -             netdev_err(ndev, "unable to enable bus clock\n");
> -             goto err_can_clk;
> -     }
> -
> /* Set chip into reset mode */
> ret = set_reset_mode(ndev);
> if (ret < 0) {
> netdev_err(ndev, "mode resetting failed!\n");
> -             goto err_bus_clk;
> +             goto err_irq;
> }
> 
> /* Common open */
> ret = open_candev(ndev);
> if (ret)
> -             goto err_bus_clk;
> +             goto err_irq;
> 
> ret = xcan_chip_start(ndev);
> if (ret < 0) {
> @@ -887,13 +883,11 @@ static int xcan_open(struct net_device *ndev)
> 
> err_candev:
> close_candev(ndev);
> -err_bus_clk:
> -     clk_disable_unprepare(priv->bus_clk);
> -err_can_clk:
> -     clk_disable_unprepare(priv->can_clk);
> err_irq:
> free_irq(ndev->irq, ndev);
> err:
> +     pm_runtime_put(priv->dev);
> +
> return ret;
> }
> 
> @@ -910,12 +904,11 @@ static int xcan_close(struct net_device *ndev)
> netif_stop_queue(ndev);
> napi_disable(&priv->napi);
> xcan_chip_stop(ndev);
> -     clk_disable_unprepare(priv->bus_clk);
> -     clk_disable_unprepare(priv->can_clk);
> free_irq(ndev->irq, ndev);
> close_candev(ndev);
> 
> can_led_event(ndev, CAN_LED_EVENT_STOP);
> +     pm_runtime_put(priv->dev);
> 
> return 0;
> }
> @@ -934,27 +927,21 @@ static int xcan_get_berr_counter(const struct net_device \
> *ndev, struct xcan_priv *priv = netdev_priv(ndev);
> int ret;
> 
> -     ret = clk_prepare_enable(priv->can_clk);
> -     if (ret)
> -             goto err;
> +     ret = pm_runtime_get_sync(priv->dev);
> +     if (ret < 0) {
> +             netdev_err(ndev, "%s: runtime resume failed(%d)\n\r",
> +                             __func__, ret);

As above.

K will modify this too.

Regards,
Kedar.
        Sören


This email and any attachments are intended for the sole use of the named \
recipient(s) and contain(s) confidential information that may be proprietary, \
privileged or copyrighted under applicable law. If you are not the intended \
recipient, do not read, copy, or forward this email message or any attachments. \
Delete this email message and any attachments immediately.

--
To unsubscribe from this list: send the line "unsubscribe linux-can" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[prev in list] [next in list] [prev in thread] [next in thread]