Re: [PATCH v1 2/3] iio: adc: bcm_iproc_adc: use devm_add_action_or_reset()

On 08/25, [email protected] wrote:
> From: Mohammad Shahid <[email protected]>
> 
> Replace the manual ADC and clock cleanup in probe error paths
> and remove() with devm_add_action_or_reset().
> 
> Register cleanup actions immediately after enabling the clock and
> ADC so that the resources are automatically released on probe
> failure and device removal.
> 
> This also allows the cleanup labels to be removed and the
> iio_device_register() failure path to return directly.
> 
> Signed-off-by: Mohammad Shahid <[email protected]>
> ---
...
> +static void iproc_adc_clk_disable(void *data)
> +{
> +	struct clk *clk = data;
> +
> +	clk_disable_unprepare(clk);
> +}
> +
The above callback is not needed with devm_clk_get_enabled(). See comment below.

>  static int iproc_adc_read_raw(struct iio_dev *indio_dev,
>  			  struct iio_chan_spec const *chan,
>  			  int *val,
> @@ -551,9 +565,17 @@ static int iproc_adc_probe(struct platform_device *pdev)
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to enable clock\n");
>  
> +	ret = devm_add_action_or_reset(dev, iproc_adc_clk_disable, adc_priv->adc_clk);
> +	if (ret)
> +		return ret;
> +
Hmm, the whole clock get/add_action/enable sequence can be replaced by
devm_clk_get_enabled(). Unless the ADC needs to clear IPROC_ADC_AUXIN_SCAN_ENA
before tsc_clk gets enabled. Otherwise, the update can be done with fewer
LOC by calling devm_clk_get_enabled(). Also, since adc_clk is only used during
device probe, it can be declared as a local variable rather than a field of
struct iproc_adc_priv. So, in addition to updating to devm interfaces, patch 2
can reduce struct iproc_adc_priv size by keeping adc_clk as a local variable of
iproc_adc_probe().

>  	ret = iproc_adc_enable(indio_dev);
>  	if (ret)
> -		goto err_adc_enable;
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, iproc_adc_disable_action, indio_dev);
> +	if (ret)
> +		return ret;
>  
>  	indio_dev->name = "iproc-static-adc";
>  	indio_dev->info = &iproc_adc_iio_info;
> @@ -562,29 +584,18 @@ static int iproc_adc_probe(struct platform_device *pdev)
>  	indio_dev->num_channels = ARRAY_SIZE(iproc_adc_iio_channels);
>  
>  	ret = iio_device_register(indio_dev);
> -	if (ret) {
> -		dev_err(&pdev->dev, "iio_device_register failed:err %d\n", ret);
> -		goto err_clk;
> -	}
> +	if (ret)
> +		return dev_err_probe(dev, ret, "iio_device_register failed\n");
This looks good, and can become even more concise with iio_device_register().
In addition to patch 2 (clk and adc) and patch 3 (mutex) device managed patches,
add a patch 4 updating from iio_device_register() to devm_iio_device_register().
With that, all device resources shall be released on device detach and
iproc_adc_remove() won't be needed anymore.

>  
>  	return 0;
>  
> -err_clk:
> -	iproc_adc_disable(indio_dev);
> -err_adc_enable:
> -	clk_disable_unprepare(adc_priv->adc_clk);
> -
> -	return ret;
>  }
>  
>  static void iproc_adc_remove(struct platform_device *pdev)
>  {
>  	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> -	struct iproc_adc_priv *adc_priv = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
> -	iproc_adc_disable(indio_dev);
> -	clk_disable_unprepare(adc_priv->adc_clk);
>  }