Re: [PATCH v4 11/31] net: can: mscan: improve clock API use

[Marc Kleine-Budde <mkl@pengutronix.de>]

[-- Attachment #1: Type: text/plain, Size: 5490 bytes --]

On 08/06/2013 10:43 PM, Gerhard Sittig wrote:
> the .get_clock() callback is run from probe() and might allocate
> resources, introduce a .put_clock() callback that is run from remove()
> to undo any allocation activities
> 
> prepare and enable the clocks in open(), disable and unprepare the
> clocks in close() if clocks were acquired during probe(), to not assume
> knowledge about which activities are done in probe() and remove()
> 
> use devm_get_clk() to lookup the SYS and REF clocks, to have the clocks
> put upon device shutdown
> 
> store pointers to data structures upon successful allocation already
> instead of deferral until complete setup, such that subroutines in the
> setup sequence may access those data structures as well to track their
> resource acquisition
> 
> since clock allocation remains optional, the release callback as well as
> the enable/disable calls in open/close are optional as well
> 
> Signed-off-by: Gerhard Sittig <gsi@denx.de>
> ---
>  drivers/net/can/mscan/mpc5xxx_can.c |   18 ++++++++++++------
>  drivers/net/can/mscan/mscan.c       |   27 ++++++++++++++++++++++++++-
>  drivers/net/can/mscan/mscan.h       |    3 +++
>  3 files changed, 41 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/can/mscan/mpc5xxx_can.c b/drivers/net/can/mscan/mpc5xxx_can.c
> index bc422ba..e59b3a3 100644
> --- a/drivers/net/can/mscan/mpc5xxx_can.c
> +++ b/drivers/net/can/mscan/mpc5xxx_can.c
> @@ -40,6 +40,7 @@ struct mpc5xxx_can_data {
>  	unsigned int type;
>  	u32 (*get_clock)(struct platform_device *ofdev, const char *clock_name,
>  			 int *mscan_clksrc);
> +	void (*put_clock)(struct platform_device *ofdev);
>  };
>  
>  #ifdef CONFIG_PPC_MPC52xx
> @@ -180,7 +181,7 @@ static u32 mpc512x_can_get_clock(struct platform_device *ofdev,
>  			clockdiv = 1;
>  
>  		if (!clock_name || !strcmp(clock_name, "sys")) {
> -			sys_clk = clk_get(&ofdev->dev, "sys_clk");
> +			sys_clk = devm_clk_get(&ofdev->dev, "sys_clk");
>  			if (IS_ERR(sys_clk)) {
>  				dev_err(&ofdev->dev, "couldn't get sys_clk\n");
>  				goto exit_unmap;
> @@ -203,7 +204,7 @@ static u32 mpc512x_can_get_clock(struct platform_device *ofdev,
>  		}
>  
>  		if (clocksrc < 0) {
> -			ref_clk = clk_get(&ofdev->dev, "ref_clk");
> +			ref_clk = devm_clk_get(&ofdev->dev, "ref_clk");
>  			if (IS_ERR(ref_clk)) {
>  				dev_err(&ofdev->dev, "couldn't get ref_clk\n");
>  				goto exit_unmap;
> @@ -280,6 +281,8 @@ static int mpc5xxx_can_probe(struct platform_device *ofdev)
>  	dev = alloc_mscandev();
>  	if (!dev)
>  		goto exit_dispose_irq;
> +	platform_set_drvdata(ofdev, dev);
> +	SET_NETDEV_DEV(dev, &ofdev->dev);
>  
>  	priv = netdev_priv(dev);
>  	priv->reg_base = base;
> @@ -296,8 +299,6 @@ static int mpc5xxx_can_probe(struct platform_device *ofdev)
>  		goto exit_free_mscan;
>  	}
>  
> -	SET_NETDEV_DEV(dev, &ofdev->dev);
> -
>  	err = register_mscandev(dev, mscan_clksrc);
>  	if (err) {
>  		dev_err(&ofdev->dev, "registering %s failed (err=%d)\n",
> @@ -305,8 +306,6 @@ static int mpc5xxx_can_probe(struct platform_device *ofdev)
>  		goto exit_free_mscan;
>  	}
>  
> -	platform_set_drvdata(ofdev, dev);
> -
>  	dev_info(&ofdev->dev, "MSCAN at 0x%p, irq %d, clock %d Hz\n",
>  		 priv->reg_base, dev->irq, priv->can.clock.freq);
>  
> @@ -324,10 +323,17 @@ exit_unmap_mem:
>  
>  static int mpc5xxx_can_remove(struct platform_device *ofdev)
>  {
> +	const struct of_device_id *match;
> +	const struct mpc5xxx_can_data *data;
>  	struct net_device *dev = platform_get_drvdata(ofdev);
>  	struct mscan_priv *priv = netdev_priv(dev);
>  
> +	match = of_match_device(mpc5xxx_can_table, &ofdev->dev);
> +	data = match ? match->data : NULL;
> +
>  	unregister_mscandev(dev);
> +	if (data && data->put_clock)
> +		data->put_clock(ofdev);
>  	iounmap(priv->reg_base);
>  	irq_dispose_mapping(dev->irq);
>  	free_candev(dev);
> diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c
> index e6b4095..4f998f5 100644
> --- a/drivers/net/can/mscan/mscan.c
> +++ b/drivers/net/can/mscan/mscan.c
> @@ -573,10 +573,24 @@ static int mscan_open(struct net_device *dev)
>  	struct mscan_priv *priv = netdev_priv(dev);
>  	struct mscan_regs __iomem *regs = priv->reg_base;
>  
> +	if (priv->clk_ipg) {
> +		ret = clk_prepare_enable(priv->clk_ipg);
> +		if (ret)
> +			goto exit_retcode;
> +	}
> +	if (priv->clk_can) {
> +		ret = clk_prepare_enable(priv->clk_can);
> +		if (ret) {
> +			if (priv->clk_ipg)
> +				clk_disable_unprepare(priv->clk_ipg);
> +			goto exit_retcode;

Why don't you add another jump label and jump to that to disable the
ipkg clock?


> +		}
> +	}
> +
>  	/* common open */
>  	ret = open_candev(dev);
>  	if (ret)
> -		return ret;
> +		goto exit_dis_clock;
>  
>  	napi_enable(&priv->napi);
>  
> @@ -604,6 +618,12 @@ exit_free_irq:
>  exit_napi_disable:
>  	napi_disable(&priv->napi);
>  	close_candev(dev);
> +exit_dis_clock:
> +	if (priv->clk_can)
> +		clk_disable_unprepare(priv->clk_can);
> +	if (priv->clk_ipg)
> +		clk_disable_unprepare(priv->clk_ipg);
> +exit_retcode:
>  	return ret;
>  }

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 --]