Re: [PATCH 7/7] regulator: ti-abb: Use devm_regulator_register

[Nishanth Menon <nm@ti.com>]

On 09/04/2013 01:31 AM, Sachin Kamat wrote:
> devm_* simplifies the code.
> 
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>

Seems to do a little more than devm_* alone, there is pruning of error
handling done here as well.

I am not comfortable with loosing print message information that help
debug issues on the field - which we usually have to based on just
kernel logs.

Please reduce the change to just using devm_regulator_register and
removal of .remove function and i am ok with that change.
> ---
>  drivers/regulator/ti-abb-regulator.c |   82 ++++++++++------------------------
>  1 file changed, 23 insertions(+), 59 deletions(-)
> 
> diff --git a/drivers/regulator/ti-abb-regulator.c b/drivers/regulator/ti-abb-regulator.c
> index 3753ed0..02392f8 100644
> --- a/drivers/regulator/ti-abb-regulator.c
> +++ b/drivers/regulator/ti-abb-regulator.c
> @@ -696,22 +696,17 @@ static int ti_abb_probe(struct platform_device *pdev)
>  	match = of_match_device(ti_abb_of_match, dev);
>  	if (!match) {
>  		/* We do not expect this to happen */
> -		ret = -ENODEV;
>  		dev_err(dev, "%s: Unable to match device\n", __func__);
> -		goto err;
> +		return -ENODEV;
>  	}
>  	if (!match->data) {
> -		ret = -EINVAL;
>  		dev_err(dev, "%s: Bad data in match\n", __func__);
> -		goto err;
> +		return -EINVAL;
>  	}
>  
>  	abb = devm_kzalloc(dev, sizeof(struct ti_abb), GFP_KERNEL);
> -	if (!abb) {
> -		dev_err(dev, "%s: Unable to allocate ABB struct\n", __func__);
> -		ret = -ENOMEM;
> -		goto err;
> -	}
> +	if (!abb)
> +		return -ENOMEM;
here.

>  	abb->regs = match->data;
>  
>  	/* Map ABB resources */
> @@ -719,21 +714,17 @@ static int ti_abb_probe(struct platform_device *pdev)
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
>  	if (!res) {
>  		dev_err(dev, "Missing '%s' IO resource\n", pname);
> -		ret = -ENODEV;
> -		goto err;
> +		return -ENODEV;
>  	}
>  	abb->base = devm_ioremap_resource(dev, res);
> -	if (IS_ERR(abb->base)) {
> -		ret = PTR_ERR(abb->base);
> -		goto err;
> -	}
> +	if (IS_ERR(abb->base))
> +		return PTR_ERR(abb->base);
>  
>  	pname = "int-address";
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, pname);
>  	if (!res) {
>  		dev_err(dev, "Missing '%s' IO resource\n", pname);
> -		ret = -ENODEV;
> -		goto err;
> +		return -ENODEV;
>  	}
>  	/*
>  	 * We may have shared interrupt register offsets which are
> @@ -743,8 +734,7 @@ static int ti_abb_probe(struct platform_device *pdev)
>  					     resource_size(res));
>  	if (!abb->int_base) {
>  		dev_err(dev, "Unable to map '%s'\n", pname);
> -		ret = -ENOMEM;
> -		goto err;
> +		return -ENOMEM;
>  	}
>  
>  	/* Map Optional resources */
> @@ -764,8 +754,7 @@ static int ti_abb_probe(struct platform_device *pdev)
>  					       resource_size(res));
>  	if (!abb->efuse_base) {
>  		dev_err(dev, "Unable to map '%s'\n", pname);
> -		ret = -ENOMEM;
> -		goto err;
> +		return -ENOMEM;
>  	}
>  
>  	pname = "ldo-address";
> @@ -776,10 +765,8 @@ static int ti_abb_probe(struct platform_device *pdev)
>  		goto skip_opt;
>  	}
>  	abb->ldo_base = devm_ioremap_resource(dev, res);
> -	if (IS_ERR(abb->ldo_base)) {
> -		ret = PTR_ERR(abb->ldo_base);
> -		goto err;
> -	}
> +	if (IS_ERR(abb->ldo_base))
> +		return PTR_ERR(abb->ldo_base);
>  
>  	/* IF ldo_base is set, the following are mandatory */
>  	pname = "ti,ldovbb-override-mask";
> @@ -788,12 +775,11 @@ static int ti_abb_probe(struct platform_device *pdev)
>  				 &abb->ldovbb_override_mask);
>  	if (ret) {
>  		dev_err(dev, "Missing '%s' (%d)\n", pname, ret);
> -		goto err;
> +		return ret;
>  	}
>  	if (!abb->ldovbb_override_mask) {
>  		dev_err(dev, "Invalid property:'%s' set as 0!\n", pname);
> -		ret = -EINVAL;
> -		goto err;
> +		return -EINVAL;
>  	}
>  
>  	pname = "ti,ldovbb-vset-mask";
> @@ -802,12 +788,11 @@ static int ti_abb_probe(struct platform_device *pdev)
>  				 &abb->ldovbb_vset_mask);
>  	if (ret) {
>  		dev_err(dev, "Missing '%s' (%d)\n", pname, ret);
> -		goto err;
> +		return ret;
>  	}
>  	if (!abb->ldovbb_vset_mask) {
>  		dev_err(dev, "Invalid property:'%s' set as 0!\n", pname);
> -		ret = -EINVAL;
> -		goto err;
> +		return -EINVAL;
>  	}
>  
>  skip_opt:
> @@ -817,31 +802,29 @@ skip_opt:
>  				 &abb->txdone_mask);
>  	if (ret) {
>  		dev_err(dev, "Missing '%s' (%d)\n", pname, ret);
> -		goto err;
> +		return ret;
>  	}
>  	if (!abb->txdone_mask) {
>  		dev_err(dev, "Invalid property:'%s' set as 0!\n", pname);
> -		ret = -EINVAL;
> -		goto err;
> +		return -EINVAL;
>  	}
>  
>  	initdata = of_get_regulator_init_data(dev, pdev->dev.of_node);
>  	if (!initdata) {
> -		ret = -ENOMEM;
>  		dev_err(dev, "%s: Unable to alloc regulator init data\n",
>  			__func__);
> -		goto err;
> +		return -ENOMEM;
>  	}
>  
>  	/* init ABB opp_sel table */
>  	ret = ti_abb_init_table(dev, abb, initdata);
>  	if (ret)
> -		goto err;
> +		return ret;
>  
>  	/* init ABB timing */
>  	ret = ti_abb_init_timings(dev, abb);
>  	if (ret)
> -		goto err;
> +		return ret;
>  
>  	desc = &abb->rdesc;
>  	desc->name = dev_name(dev);
> @@ -859,12 +842,12 @@ skip_opt:
>  	config.driver_data = abb;
>  	config.of_node = pdev->dev.of_node;
>  
> -	rdev = regulator_register(desc, &config);
> +	rdev = devm_regulator_register(dev, desc, &config);

ok with this.

>  	if (IS_ERR(rdev)) {
>  		ret = PTR_ERR(rdev);
>  		dev_err(dev, "%s: failed to register regulator(%d)\n",
>  			__func__, ret);
> -		goto err;
> +		return ret;
>  	}
>  	platform_set_drvdata(pdev, rdev);
>  
> @@ -872,31 +855,12 @@ skip_opt:
>  	ti_abb_rmw(abb->regs->sr2_en_mask, 1, abb->regs->setup_reg, abb->base);
>  
>  	return 0;
> -
> -err:
> -	dev_err(dev, "%s: Failed to initialize(%d)\n", __func__, ret);
> -	return ret;
here -> with this, the lazy non detailed prints end with a generic
fail case.

I can cleanup the error handling if you like.

> -}
> -
> -/**
> - * ti_abb_remove() - cleanups
> - * @pdev: ABB platform device
> - *
> - * Return: 0
> - */
> -static int ti_abb_remove(struct platform_device *pdev)
> -{
> -	struct regulator_dev *rdev = platform_get_drvdata(pdev);
> -
> -	regulator_unregister(rdev);
> -	return 0;
ok with this
>  }
>  
>  MODULE_ALIAS("platform:ti_abb");
>  
>  static struct platform_driver ti_abb_driver = {
>  	.probe = ti_abb_probe,
> -	.remove = ti_abb_remove,
ok with this
>  	.driver = {
>  		   .name = "ti_abb",
>  		   .owner = THIS_MODULE,
> 


-- 
Regards,
Nishanth Menon