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,
