Re: [PATCH v3 4/6] iio: adc: bcm_iproc_adc: use devm-managed ADC cleanup

[Jonathan Cameron]

> Use devm_add_action_or_reset() to disable the ADC automatically when
> the device is detached or probe fails.
> 
> This removes the manual ADC cleanup from the remove and error paths
> and ties the ADC cleanup to the device lifetime.

Sashiko raised a bunch of pre existing issues whilst looking at this
one.  As far as I'm concerned whether correct or not they are out of
scope for this series.  Feel free to take a look if you like though
(for a potential follow up)  Note I haven't analysed any of them -
just noted there were a few comments - so they may or may not be correct.

https://sashiko.dev/#/patchset/20260905093631.48667-1-mdshahid03%40gmail.com

> 
> Signed-off-by: Mohammad Shahid <mdshahid03@xxxxxxxxx>
>
> diff --git a/drivers/iio/adc/bcm_iproc_adc.c b/drivers/iio/adc/bcm_iproc_adc.c
> index 3bbf814d4c8f..24fb41751e93 100644
> --- a/drivers/iio/adc/bcm_iproc_adc.c
> +++ b/drivers/iio/adc/bcm_iproc_adc.c
> @@ -450,6 +450,13 @@ static void iproc_adc_disable(struct iio_dev *indio_dev)
>  	}
>  }
>  
> +static void iproc_adc_disable_action(void *data)
> +{
> +	struct iio_dev *indio_dev = data;
> +
> +	iproc_adc_disable(indio_dev);

For this it is fine and more compact to do

static void iproc_adc_disable_action(void *indio_dev)
{
	iproc_adc_disable(indio_dev);
}

Given we are going via a void * anyway, there is no additional
type safety or particular readability advantages to having the
intermediate given we have well known naming fo the struct iio_dev *


> +}
> +
>  static int iproc_adc_read_raw(struct iio_dev *indio_dev,
>  			  struct iio_chan_spec const *chan,
>  			  int *val,
> @@ -551,7 +558,11 @@ static int iproc_adc_probe(struct platform_device *pdev)
>  
>  	ret = iproc_adc_enable(indio_dev);
>  	if (ret)
> -		goto err_adc_enable;
> +		return ret;

This change belongs in the earlier patch as none of the other changes
in this patch have anything to do with enabling it.

> +
> +	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;
> @@ -560,17 +571,11 @@ 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");
>  
>  	return 0;
>  
> -err_clk:
> -	iproc_adc_disable(indio_dev);
> -err_adc_enable:

This label should have gone in the earlier patch.

-- 
Jonathan Cameron <jonathan.cameron@xxxxxxxxxxxxxxxx>