Re: [PATCH v3] iio: adc: Modernize single regulartor call

[Jonathan Cameron <jic23@kernel.org>]

On Wed, 21 May 2025 15:54:03 -0300
Gabriel Souza Araujo <gabrielfsouza.araujo@gmail.com> wrote:

> Replace a single instance of legacy regulator with devm_regulator_get_enable().
> This change improves code clarity and aligns with modern kernel APIs.
Typo in patch title (regulator)

> 
> Signed-off-by: Gabriel Souza Araujo <gabrielfsouza.araujo@gmail.com>
> Co-developed-by: Cesar Bispo <cesar.bispo@ime.usp.br>
> Signed-off-by: Cesar Bispo <cesar.bispo@ime.usp.br>

Getting better, but you need to do a little more reading on how device managed
cleanup works.

Jonathan

> ---
> The first version [1] did not compile due to incorrect use of pointers and improper
> parameterization of the devm_regulator_get_enable() function. 
> These issues have been fixed, and the code now compiles successfully.
> Additionally, version two [2] had an incomplete patch, which has also 
> been addressed in this version.
> 
> [1] https://lore.kernel.org/linux-iio/20250429160526.5934-1-cesar.bispo@ime.usp.br/
> [2] https://lore.kernel.org/linux-iio/20250514185239.10078-1-gabrielfsouza.araujo@gmail.com/
> 
>  drivers/iio/adc/qcom-pm8xxx-xoadc.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/qcom-pm8xxx-xoadc.c b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> index 31f88cf7f7f1..a37ba6fad146 100644
> --- a/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> +++ b/drivers/iio/adc/qcom-pm8xxx-xoadc.c
> @@ -911,11 +911,7 @@ static int pm8xxx_xoadc_probe(struct platform_device *pdev)
>  	adc->map = map;
>  
>  	/* Bring up regulator */
> -	adc->vref = devm_regulator_get(dev, "xoadc-ref");
> -	if (IS_ERR(adc->vref))
> -		return dev_err_probe(dev, PTR_ERR(adc->vref),
> -				     "failed to get XOADC VREF regulator\n");
> -	ret = regulator_enable(adc->vref);
> +	ret = devm_regulator_get_enable(dev, "xoadc-ref");

Have a read up on what this is doing.   Hint, it is both about simplifying error
paths and about simplifying remove paths - you only deal with changes needed
for the error path here.

>  	if (ret) {
>  		dev_err(dev, "failed to enable XOADC VREF regulator\n");
>  		return ret;
> @@ -925,7 +921,7 @@ static int pm8xxx_xoadc_probe(struct platform_device *pdev)
>  			pm8xxx_eoc_irq, NULL, 0, variant->name, indio_dev);
>  	if (ret) {
>  		dev_err(dev, "unable to request IRQ\n");

Consider a follow up to use return dev_err_probe() in places like this to
simplify the code.

> -		goto out_disable_vref;
> +		return ret;
>  	}
>  
>  	indio_dev->name = variant->name;
> @@ -936,7 +932,7 @@ static int pm8xxx_xoadc_probe(struct platform_device *pdev)
>  
>  	ret = iio_device_register(indio_dev);
Given this is all that is not manually cleaned up, please add a second patch
to use devm_iio_device_register() for this.

>  	if (ret)
> -		goto out_disable_vref;
> +		return ret;
>  
>  	ret = pm8xxx_calibrate_device(adc);
>  	if (ret)
> @@ -948,9 +944,6 @@ static int pm8xxx_xoadc_probe(struct platform_device *pdev)
>  
>  out_unreg_device:
>  	iio_device_unregister(indio_dev);
> -out_disable_vref:
> -	regulator_disable(adc->vref);
> -
>  	return ret;
>  }
>