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

Message ID	20201026133609.24262-5-brgl@bgdev.pl (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=QOf4=EB=vger.kernel.org=linux-iio-owner@kernel.org> From: Bartosz Golaszewski <brgl@bgdev.pl> To: Jonathan Cameron <jic23@kernel.org>, Lars-Peter Clausen <lars@metafoo.de>, Peter Meerwald-Stadler <pmeerw@pmeerw.net>, Michal Simek <michal.simek@xilinx.com> Cc: linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Bartosz Golaszewski <bgolaszewski@baylibre.com> Subject: [PATCH 4/5] iio: adc: xilinx: use devres for irq handling Date: Mon, 26 Oct 2020 14:36:08 +0100 Message-Id: <20201026133609.24262-5-brgl@bgdev.pl> In-Reply-To: <20201026133609.24262-1-brgl@bgdev.pl> References: <20201026133609.24262-1-brgl@bgdev.pl> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Precedence: bulk
Series	iio: adc: xilinx: use even more devres \| expand [0/5] iio: adc: xilinx: use even more devres [1/5] iio: adc: xilinx: use helper variable for &pdev->dev [2/5] iio: adc: xilinx: use devm_krealloc() instead of kfree() + kcalloc() [3/5] iio: adc: xilinx: use a devres action to disable and unprepare the clock [4/5] iio: adc: xilinx: use devres for irq handling [5/5] iio: adc: xilinx: use iio devres helpers

Message ID

20201026133609.24262-5-brgl@bgdev.pl (mailing list archive)

State

New, archived

Headers

From: Bartosz Golaszewski <brgl@bgdev.pl>
To: Jonathan Cameron <jic23@kernel.org>,
        Lars-Peter Clausen <lars@metafoo.de>,
        Peter Meerwald-Stadler <pmeerw@pmeerw.net>,
        Michal Simek <michal.simek@xilinx.com>
Cc: linux-iio@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-kernel@vger.kernel.org,
        Bartosz Golaszewski <bgolaszewski@baylibre.com>
Subject: [PATCH 4/5] iio: adc: xilinx: use devres for irq handling
Date: Mon, 26 Oct 2020 14:36:08 +0100
Message-Id: <20201026133609.24262-5-brgl@bgdev.pl>
In-Reply-To: <20201026133609.24262-1-brgl@bgdev.pl>
References: <20201026133609.24262-1-brgl@bgdev.pl>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

Series

iio: adc: xilinx: use even more devres | expand

Commit Message

Bartosz Golaszewski Oct. 26, 2020, 1:36 p.m. UTC

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>
---
 drivers/iio/adc/xilinx-xadc-core.c | 35 ++++++++++++++++++------------
 1 file changed, 21 insertions(+), 14 deletions(-)

Comments

Jonathan Cameron Oct. 29, 2020, 3:41 p.m. UTC | #1

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;
>  }

Bartosz Golaszewski Oct. 30, 2020, 10:52 a.m. UTC | #2

On Thu, Oct 29, 2020 at 4:41 PM Jonathan Cameron <jic23@kernel.org> wrote:
>
> 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
>

Hi Jonathan,

My two priorities for the ordering of this series were: correct
end-result and not breaking anything on the way. The latter
unfortunately gets in the way of cleaner looking intermediate patches.

I tried to not alter the ordering in which the resources are freed at
any step. As devres release callbacks are called *after* remove() and
in a reverse order to how they were registered, I needed to start from
the bottom of the remove() callback and convert the last operation,
then go upwards from there.

If I tried to do it from the top - I probably could remove labels
earlier and in a cleaner manner but it wouldn't guarantee
bisectability.

Bartosz

Jonathan Cameron Oct. 31, 2020, 11:10 a.m. UTC | #3

On Fri, 30 Oct 2020 11:52:00 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> On Thu, Oct 29, 2020 at 4:41 PM Jonathan Cameron <jic23@kernel.org> wrote:
> >
> > 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
> >  
> 
> Hi Jonathan,
> 
> My two priorities for the ordering of this series were: correct
> end-result and not breaking anything on the way. The latter
> unfortunately gets in the way of cleaner looking intermediate patches.
> 
> I tried to not alter the ordering in which the resources are freed at
> any step. As devres release callbacks are called *after* remove() and
> in a reverse order to how they were registered, I needed to start from
> the bottom of the remove() callback and convert the last operation,
> then go upwards from there.
> 
> If I tried to do it from the top - I probably could remove labels
> earlier and in a cleaner manner but it wouldn't guarantee
> bisectability.
> 

Maybe best plan is to squash last 3 patches into one?

I suspect that's going to be easier to review.

Jonathan


> Bartosz

Bartosz Golaszewski Nov. 2, 2020, 8:39 a.m. UTC | #4

On Sat, Oct 31, 2020 at 12:10 PM Jonathan Cameron <jic23@kernel.org> wrote:
>

[snip]

> >
> > Hi Jonathan,
> >
> > My two priorities for the ordering of this series were: correct
> > end-result and not breaking anything on the way. The latter
> > unfortunately gets in the way of cleaner looking intermediate patches.
> >
> > I tried to not alter the ordering in which the resources are freed at
> > any step. As devres release callbacks are called *after* remove() and
> > in a reverse order to how they were registered, I needed to start from
> > the bottom of the remove() callback and convert the last operation,
> > then go upwards from there.
> >
> > If I tried to do it from the top - I probably could remove labels
> > earlier and in a cleaner manner but it wouldn't guarantee
> > bisectability.
> >
>
> Maybe best plan is to squash last 3 patches into one?
>
> I suspect that's going to be easier to review.
>
> Jonathan
>

Sure I can do this.

Bartosz

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;
 }

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

Commit Message

Comments

Patch