RE: [PATCH v3] can: Convert to runtime_pm

[Appana Durga Kedareswara Rao]

Hi Soren,

-----Original Message-----
From: SÃren Brinkmann [mailto:soren.brinkmann@xxxxxxxxxx]
Sent: Thursday, November 27, 2014 11:54 PM
To: Appana Durga Kedareswara Rao
Cc: wg@xxxxxxxxxxxxxx; mkl@xxxxxxxxxxxxxx; Michal Simek; grant.likely@xxxxxxxxxx; robh+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-can@xxxxxxxxxxxxxxx; Appana Durga Kedareswara Rao; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
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@xxxxxxxxxx>
> Signed-off-by: Kedareswara rao Appana <appanad@xxxxxxxxxx>
> ---
> 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.