Re: [PATCH v5 1/3] iio: adc: rockchip_saradc: move all of probe to devm-functions

[Jonathan Cameron]

On Sun, 19 Apr 2020 12:02:05 +0200
Heiko Stuebner <heiko@xxxxxxxxx> wrote:

> From: Heiko Stuebner <heiko.stuebner@xxxxxxxxxxxxxxxxxxxxx>
> 
> Parts of the saradc probe rely on devm functions and later parts do not.
> This makes it more difficult to for example enable triggers via their
> devm-functions and would need more undo-work in remove.
> 
> So to make life easier for the driver, move the rest of probe calls
> also to their devm-equivalents.
> 
> This includes moving the clk- and regulator-disabling to a devm_action
> so that they gets disabled both during remove and in the error case
> in probe, after the action is registered.
> 
> Signed-off-by: Heiko Stuebner <heiko.stuebner@xxxxxxxxxxxxxxxxxxxxx>
> ---
> changes in v5:
> - none
> changes in v4:
> - new patch as suggested by Jonathan
> 
>  drivers/iio/adc/rockchip_saradc.c | 37 ++++++++++++++++---------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/iio/adc/rockchip_saradc.c b/drivers/iio/adc/rockchip_saradc.c
> index 582ba047c4a6..270eb7e83823 100644
> --- a/drivers/iio/adc/rockchip_saradc.c
> +++ b/drivers/iio/adc/rockchip_saradc.c
> @@ -193,6 +193,15 @@ static void rockchip_saradc_reset_controller(struct reset_control *reset)
>  	reset_control_deassert(reset);
>  }
>  
> +static void rockchip_saradc_disable(void *data)
> +{
> +	struct rockchip_saradc *info = data;
> +
> +	clk_disable_unprepare(info->clk);
> +	clk_disable_unprepare(info->pclk);
> +	regulator_disable(info->vref);

You should do these independently.  If you use
a separate devm_add_action_or_reset you can drop the error handling
in probe because that will all be cleaned up automatically as well.

Right now you have a nasty hybrid of managed and unmanaged needing
manual cleanup in some paths.

It will take a few more lines of code, but it will be a lot easier
to review / maintain.

Jonathan


> +}
> +
>  static int rockchip_saradc_probe(struct platform_device *pdev)
>  {
>  	struct rockchip_saradc *info = NULL;
> @@ -304,6 +313,14 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>  		goto err_pclk;
>  	}
>  
> +	ret = devm_add_action_or_reset(&pdev->dev,
> +				       rockchip_saradc_disable, info);
> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to register devm action, %d\n",
> +			ret);
> +		return ret;
> +	}
> +
>  	platform_set_drvdata(pdev, indio_dev);
>  
>  	indio_dev->name = dev_name(&pdev->dev);
> @@ -315,14 +332,12 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>  	indio_dev->channels = info->data->channels;
>  	indio_dev->num_channels = info->data->num_channels;
>  
> -	ret = iio_device_register(indio_dev);
> +	ret = devm_iio_device_register(&pdev->dev, indio_dev);
>  	if (ret)
> -		goto err_clk;
> +		return ret;
>  
>  	return 0;
>  
> -err_clk:
> -	clk_disable_unprepare(info->clk);
>  err_pclk:
>  	clk_disable_unprepare(info->pclk);

>  err_reg_voltage:
> @@ -330,19 +345,6 @@ static int rockchip_saradc_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> -static int rockchip_saradc_remove(struct platform_device *pdev)
> -{
> -	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> -	struct rockchip_saradc *info = iio_priv(indio_dev);
> -
> -	iio_device_unregister(indio_dev);
> -	clk_disable_unprepare(info->clk);
> -	clk_disable_unprepare(info->pclk);
> -	regulator_disable(info->vref);
> -
> -	return 0;
> -}
> -
>  #ifdef CONFIG_PM_SLEEP
>  static int rockchip_saradc_suspend(struct device *dev)
>  {
> @@ -383,7 +385,6 @@ static SIMPLE_DEV_PM_OPS(rockchip_saradc_pm_ops,
>  
>  static struct platform_driver rockchip_saradc_driver = {
>  	.probe		= rockchip_saradc_probe,
> -	.remove		= rockchip_saradc_remove,
>  	.driver		= {
>  		.name	= "rockchip-saradc",
>  		.of_match_table = rockchip_saradc_match,