Re: [PATCH 4/5] iio: adc: xilinx: use devres for irq handling

From: Jonathan Cameron <jic23@kernel.org>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Lars-Peter Clausen <lars@metafoo.de>,
	Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
	Michal Simek <michal.simek@xilinx.com>,
	linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Bartosz Golaszewski <bgolaszewski@baylibre.com>
Subject: Re: [PATCH 4/5] iio: adc: xilinx: use devres for irq handling
Date: Thu, 29 Oct 2020 15:41:18 +0000	[thread overview]
Message-ID: <20201029154118.12fd6c23@archlinux> (raw)
In-Reply-To: <20201026133609.24262-5-brgl@bgdev.pl>

On Mon, 26 Oct 2020 14:36:08 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Further simplify the remove() callback and error paths in probe() by
> using the managed variant of request_irq() as well as using a devm action
> for cancelling the delayed work at driver detach.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Again, this is potentially fine but I'd rather you cleaned up the ordering first
rather than doing things in this order.

The end result of the whole series looks like it will be correct, but that isn't
so obvious for the intermediate patches on their own.

Also, you end up with a lot of noise renaming gotos that then go away at the
end.

Jonathan

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 35 ++++++++++++++++++------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index e0da6258092c..4440b7a9bd36 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -1192,6 +1192,13 @@ static void xadc_clk_disable_unprepare(void *data)
>  	clk_disable_unprepare(clk);
>  }
>  
> +static void xadc_cancel_delayed_work(void *data)
> +{
> +	struct delayed_work *work = data;
> +
> +	cancel_delayed_work_sync(work);
> +}
> +
>  static int xadc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -1289,14 +1296,19 @@ static int xadc_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
> -			  dev_name(dev), indio_dev);
> +	ret = devm_request_irq(dev, xadc->irq, xadc->ops->interrupt_handler, 0,
> +			       dev_name(dev), indio_dev);
> +	if (ret)
> +		goto err_free_samplerate_trigger;
> +
> +	ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
> +				       &xadc->zynq_unmask_work);
>  	if (ret)
>  		goto err_free_samplerate_trigger;
>  
>  	ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
>  	if (ret)
> -		goto err_free_irq;
> +		goto err_free_samplerate_trigger;
>  
>  	for (i = 0; i < 16; i++)
>  		xadc_read_adc_reg(xadc, XADC_REG_THRESHOLD(i),
> @@ -1304,7 +1316,7 @@ static int xadc_probe(struct platform_device *pdev)
>  
>  	ret = xadc_write_adc_reg(xadc, XADC_REG_CONF0, conf0);
>  	if (ret)
> -		goto err_free_irq;
> +		goto err_free_samplerate_trigger;
>  
>  	bipolar_mask = 0;
>  	for (i = 0; i < indio_dev->num_channels; i++) {
> @@ -1314,17 +1326,17 @@ static int xadc_probe(struct platform_device *pdev)
>  
>  	ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(0), bipolar_mask);
>  	if (ret)
> -		goto err_free_irq;
> +		goto err_free_samplerate_trigger;
>  	ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1),
>  		bipolar_mask >> 16);
>  	if (ret)
> -		goto err_free_irq;
> +		goto err_free_samplerate_trigger;
>  
>  	/* Disable all alarms */
>  	ret = xadc_update_adc_reg(xadc, XADC_REG_CONF1, XADC_CONF1_ALARM_MASK,
>  				  XADC_CONF1_ALARM_MASK);
>  	if (ret)
> -		goto err_free_irq;
> +		goto err_free_samplerate_trigger;
>  
>  	/* Set thresholds to min/max */
>  	for (i = 0; i < 16; i++) {
> @@ -1339,7 +1351,7 @@ static int xadc_probe(struct platform_device *pdev)
>  		ret = xadc_write_adc_reg(xadc, XADC_REG_THRESHOLD(i),
>  			xadc->threshold[i]);
>  		if (ret)
> -			goto err_free_irq;
> +			goto err_free_samplerate_trigger;
>  	}
>  
>  	/* Go to non-buffered mode */
> @@ -1347,15 +1359,12 @@ static int xadc_probe(struct platform_device *pdev)
>  
>  	ret = iio_device_register(indio_dev);
>  	if (ret)
> -		goto err_free_irq;
> +		goto err_free_samplerate_trigger;
>  
>  	platform_set_drvdata(pdev, indio_dev);
>  
>  	return 0;
>  
> -err_free_irq:
> -	free_irq(xadc->irq, indio_dev);
> -	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
>  err_free_samplerate_trigger:
>  	if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
>  		iio_trigger_free(xadc->samplerate_trigger);
> @@ -1380,8 +1389,6 @@ static int xadc_remove(struct platform_device *pdev)
>  		iio_trigger_free(xadc->convst_trigger);
>  		iio_triggered_buffer_cleanup(indio_dev);
>  	}
> -	free_irq(xadc->irq, indio_dev);
> -	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
>  
>  	return 0;
>  }