[PATCH 0/5] iio: adc: xilinx: use even more devres

[PATCH 0/5] iio: adc: xilinx: use even more devres

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This is a follow-up to commit 750628c79bb1 ("iio: adc: xilinx-xadc: use
devm_krealloc()"). I noticed we can use even more devres helpers and entirely
drop the remove() callback.

Bartosz Golaszewski (5):
  iio: adc: xilinx: use helper variable for &pdev->dev
  iio: adc: xilinx: use devm_krealloc() instead of kfree() + kcalloc()
  iio: adc: xilinx: use a devres action to disable and unprepare the
    clock
  iio: adc: xilinx: use devres for irq handling
  iio: adc: xilinx: use iio devres helpers

 drivers/iio/adc/xilinx-xadc-core.c | 146 +++++++++++++----------------
 1 file changed, 65 insertions(+), 81 deletions(-)

-- 
2.29.1

[PATCH 1/5] iio: adc: xilinx: use helper variable for &pdev->dev

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

It's more elegant to use a helper local variable to store the address
of the underlying struct device than to dereference pdev everywhere.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/iio/adc/xilinx-xadc-core.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index f93c34fe5873..8494eb424b33 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1186,6 +1186,7 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 
 static int xadc_probe(struct platform_device *pdev)
 {
+	struct device *dev = &pdev->dev;
 	const struct of_device_id *id;
 	struct iio_dev *indio_dev;
 	unsigned int bipolar_mask;
@@ -1195,10 +1196,10 @@ static int xadc_probe(struct platform_device *pdev)
 	int irq;
 	int i;
 
-	if (!pdev->dev.of_node)
+	if (!dev->of_node)
 		return -ENODEV;
 
-	id = of_match_node(xadc_of_match_table, pdev->dev.of_node);
+	id = of_match_node(xadc_of_match_table, dev->of_node);
 	if (!id)
 		return -EINVAL;
 
@@ -1206,7 +1207,7 @@ static int xadc_probe(struct platform_device *pdev)
 	if (irq <= 0)
 		return -ENXIO;
 
-	indio_dev = devm_iio_device_alloc(&pdev->dev, sizeof(*xadc));
+	indio_dev = devm_iio_device_alloc(dev, sizeof(*xadc));
 	if (!indio_dev)
 		return -ENOMEM;
 
@@ -1226,7 +1227,7 @@ static int xadc_probe(struct platform_device *pdev)
 	indio_dev->modes = INDIO_DIRECT_MODE;
 	indio_dev->info = &xadc_info;
 
-	ret = xadc_parse_dt(indio_dev, pdev->dev.of_node, &conf0);
+	ret = xadc_parse_dt(indio_dev, dev->of_node, &conf0);
 	if (ret)
 		return ret;
 
@@ -1250,7 +1251,7 @@ static int xadc_probe(struct platform_device *pdev)
 		}
 	}
 
-	xadc->clk = devm_clk_get(&pdev->dev, NULL);
+	xadc->clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(xadc->clk)) {
 		ret = PTR_ERR(xadc->clk);
 		goto err_free_samplerate_trigger;
@@ -1276,7 +1277,7 @@ static int xadc_probe(struct platform_device *pdev)
 	}
 
 	ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
-			dev_name(&pdev->dev), indio_dev);
+			  dev_name(dev), indio_dev);
 	if (ret)
 		goto err_clk_disable_unprepare;
 
-- 
2.29.1

[PATCH 2/5] iio: adc: xilinx: use devm_krealloc() instead of kfree() + kcalloc()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We now have devm_krealloc() in the kernel Use it indstead of calling
kfree() and kcalloc() separately.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/iio/adc/xilinx-xadc-core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 8494eb424b33..b516280ccbd4 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -589,8 +589,9 @@ static int xadc_update_scan_mode(struct iio_dev *indio_dev,
 
 	n = bitmap_weight(mask, indio_dev->masklength);
 
-	kfree(xadc->data);
-	xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL);
+	xadc->data = devm_krealloc(indio_dev->dev.parent, xadc->data,
+				   n * sizeof(*xadc->data),
+				   GFP_KERNEL | __GFP_ZERO);
 	if (!xadc->data)
 		return -ENOMEM;
 
@@ -1372,7 +1373,6 @@ static int xadc_remove(struct platform_device *pdev)
 	free_irq(xadc->irq, indio_dev);
 	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
 	clk_disable_unprepare(xadc->clk);
-	kfree(xadc->data);
 
 	return 0;
 }
-- 
2.29.1

Re: [PATCH 2/5] iio: adc: xilinx: use devm_krealloc() instead of kfree() + kcalloc()

[Andy Shevchenko]

On Mon, Oct 26, 2020 at 4:03 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
>
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> We now have devm_krealloc() in the kernel Use it indstead of calling
> kfree() and kcalloc() separately.

Which is completely lawful when size > previous_size (I mean, the
additional patch you sent previously seems not related to this).

> -       kfree(xadc->data);
> -       xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL);
> +       xadc->data = devm_krealloc(indio_dev->dev.parent, xadc->data,
> +                                  n * sizeof(*xadc->data),

I think you need to use something from overflow.h instead of explicit
multiplication here.

> +                                  GFP_KERNEL | __GFP_ZERO);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] iio: adc: xilinx: use devm_krealloc() instead of kfree() + kcalloc()

[Bartosz Golaszewski]

On Tue, Oct 27, 2020 at 10:33 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Mon, Oct 26, 2020 at 4:03 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > We now have devm_krealloc() in the kernel Use it indstead of calling
> > kfree() and kcalloc() separately.
>
> Which is completely lawful when size > previous_size (I mean, the
> additional patch you sent previously seems not related to this).
>

Sure but devm_krealloc() is cleaner and adds the benefit of resource management.

> > -       kfree(xadc->data);
> > -       xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL);
> > +       xadc->data = devm_krealloc(indio_dev->dev.parent, xadc->data,
> > +                                  n * sizeof(*xadc->data),
>
> I think you need to use something from overflow.h instead of explicit
> multiplication here.
>

Or maybe add devm_krealloc_array() which would perform the checks
behind the scenes?

> > +                                  GFP_KERNEL | __GFP_ZERO);
>
> --
> With Best Regards,
> Andy Shevchenko

Bartosz

Re: [PATCH 2/5] iio: adc: xilinx: use devm_krealloc() instead of kfree() + kcalloc()

[Andy Shevchenko]

On Tue, Oct 27, 2020 at 12:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Tue, Oct 27, 2020 at 10:33 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Mon, Oct 26, 2020 at 4:03 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > We now have devm_krealloc() in the kernel Use it indstead of calling
> > > kfree() and kcalloc() separately.
> >
> > Which is completely lawful when size > previous_size (I mean, the
> > additional patch you sent previously seems not related to this).
> >
>
> Sure but devm_krealloc() is cleaner and adds the benefit of resource management.

I meant devm_krealloc(). It should work in this case without your
additional "fix" patch.

> > > -       kfree(xadc->data);
> > > -       xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL);
> > > +       xadc->data = devm_krealloc(indio_dev->dev.parent, xadc->data,
> > > +                                  n * sizeof(*xadc->data),
> >
> > I think you need to use something from overflow.h instead of explicit
> > multiplication here.
> >
>
> Or maybe add devm_krealloc_array() which would perform the checks
> behind the scenes?

Maybe. But what to do in the cases when you have struct with flexible
arrays, like
struct foo {
...
 type bar[];
};

?

And you do kzalloc(sizeof(foo)) followed by krealloc(). The above name
(krealloc_array) may be a bit ambiguous.

> > > +                                  GFP_KERNEL | __GFP_ZERO);

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/5] iio: adc: xilinx: use devm_krealloc() instead of kfree() + kcalloc()

[Bartosz Golaszewski]

On Tue, Oct 27, 2020 at 11:29 AM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Tue, Oct 27, 2020 at 12:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Tue, Oct 27, 2020 at 10:33 AM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > >
> > > On Mon, Oct 26, 2020 at 4:03 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > We now have devm_krealloc() in the kernel Use it indstead of calling
> > > > kfree() and kcalloc() separately.
> > >
> > > Which is completely lawful when size > previous_size (I mean, the
> > > additional patch you sent previously seems not related to this).
> > >
> >
> > Sure but devm_krealloc() is cleaner and adds the benefit of resource management.
>
> I meant devm_krealloc(). It should work in this case without your
> additional "fix" patch.
>

I know, this is why I sent the fix separately. The fix is still
correct on its own.

> > > > -       kfree(xadc->data);
> > > > -       xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL);
> > > > +       xadc->data = devm_krealloc(indio_dev->dev.parent, xadc->data,
> > > > +                                  n * sizeof(*xadc->data),
> > >
> > > I think you need to use something from overflow.h instead of explicit
> > > multiplication here.
> > >
> >
> > Or maybe add devm_krealloc_array() which would perform the checks
> > behind the scenes?
>
> Maybe. But what to do in the cases when you have struct with flexible
> arrays, like
> struct foo {
> ...
>  type bar[];
> };
>
> ?

Just use regular devm_krealloc() with struct_size()?

>
> And you do kzalloc(sizeof(foo)) followed by krealloc(). The above name
> (krealloc_array) may be a bit ambiguous.

But devm_krealloc_array() would only be useful for memory allocated by
kmalloc_array() or kcalloc(). I don't see what's your point.

Bartosz

Re: [PATCH 2/5] iio: adc: xilinx: use devm_krealloc() instead of kfree() + kcalloc()

[Andy Shevchenko]

On Tue, Oct 27, 2020 at 12:40 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
> On Tue, Oct 27, 2020 at 11:29 AM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Tue, Oct 27, 2020 at 12:04 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > On Tue, Oct 27, 2020 at 10:33 AM Andy Shevchenko
> > > <andy.shevchenko@gmail.com> wrote:

...

> > I meant devm_krealloc(). It should work in this case without your
> > additional "fix" patch.

> I know, this is why I sent the fix separately. The fix is still
> correct on its own.

My point is it's not needed. At all.
It will actually make a regression. But this is for discussion in that thread.

...

> > > Or maybe add devm_krealloc_array() which would perform the checks
> > > behind the scenes?
> >
> > Maybe. But what to do in the cases when you have struct with flexible
> > arrays, like
> > struct foo {
> > ...
> >  type bar[];
> > };
> >
> > ?
>
> Just use regular devm_krealloc() with struct_size()?
>
> >
> > And you do kzalloc(sizeof(foo)) followed by krealloc(). The above name
> > (krealloc_array) may be a bit ambiguous.
>
> But devm_krealloc_array() would only be useful for memory allocated by
> kmalloc_array() or kcalloc(). I don't see what's your point.

Naming ambiguity.
Here I'm not against it. If you think it's a good idea, go for it!

-- 
With Best Regards,
Andy Shevchenko

[PATCH 3/5] iio: adc: xilinx: use a devres action to disable and unprepare the clock

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

In order to simplify resource management and error paths in probe(), add
a devm action that calls clk_disable_unprepare() at driver detach.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/iio/adc/xilinx-xadc-core.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index b516280ccbd4..e0da6258092c 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -1185,6 +1185,13 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
 	return 0;
 }
 
+static void xadc_clk_disable_unprepare(void *data)
+{
+	struct clk *clk = data;
+
+	clk_disable_unprepare(clk);
+}
+
 static int xadc_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -1262,6 +1269,11 @@ static int xadc_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_free_samplerate_trigger;
 
+	ret = devm_add_action_or_reset(dev,
+				       xadc_clk_disable_unprepare, xadc->clk);
+	if (ret)
+		goto err_free_samplerate_trigger;
+
 	/*
 	 * Make sure not to exceed the maximum samplerate since otherwise the
 	 * resulting interrupt storm will soft-lock the system.
@@ -1280,7 +1292,7 @@ static int xadc_probe(struct platform_device *pdev)
 	ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
 			  dev_name(dev), indio_dev);
 	if (ret)
-		goto err_clk_disable_unprepare;
+		goto err_free_samplerate_trigger;
 
 	ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
 	if (ret)
@@ -1344,8 +1356,6 @@ static int xadc_probe(struct platform_device *pdev)
 err_free_irq:
 	free_irq(xadc->irq, indio_dev);
 	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
-err_clk_disable_unprepare:
-	clk_disable_unprepare(xadc->clk);
 err_free_samplerate_trigger:
 	if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
 		iio_trigger_free(xadc->samplerate_trigger);
@@ -1372,7 +1382,6 @@ static int xadc_remove(struct platform_device *pdev)
 	}
 	free_irq(xadc->irq, indio_dev);
 	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
-	clk_disable_unprepare(xadc->clk);
 
 	return 0;
 }
-- 
2.29.1

Re: [PATCH 3/5] iio: adc: xilinx: use a devres action to disable and unprepare the clock

[Jonathan Cameron]

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

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> In order to simplify resource management and error paths in probe(), add
> a devm action that calls clk_disable_unprepare() at driver detach.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index b516280ccbd4..e0da6258092c 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -1185,6 +1185,13 @@ static int xadc_parse_dt(struct iio_dev *indio_dev, struct device_node *np,
>  	return 0;
>  }
>  
> +static void xadc_clk_disable_unprepare(void *data)
> +{
> +	struct clk *clk = data;
> +
> +	clk_disable_unprepare(clk);
> +}
> +
>  static int xadc_probe(struct platform_device *pdev)
>  {
>  	struct device *dev = &pdev->dev;
> @@ -1262,6 +1269,11 @@ static int xadc_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_free_samplerate_trigger;
>  

Hi Bartosz,

> +	ret = devm_add_action_or_reset(dev,
> +				       xadc_clk_disable_unprepare, xadc->clk);

Take advantage of flexibility in line length limit and put that on line line.

The moment I see a devm call with a goto in the error path I get suspicious about
ordering.  

So why do we have actions in the error paths of probe that don't turn up
in the remove function?  Fix that before you do this change.

There are some cases in here already that push the limits on this
such as clock allocation but they are easily argued as safe.  This is getting
less obvious.


Jonathan


> +	if (ret)
> +		goto err_free_samplerate_trigger;
> +
>  	/*
>  	 * Make sure not to exceed the maximum samplerate since otherwise the
>  	 * resulting interrupt storm will soft-lock the system.
> @@ -1280,7 +1292,7 @@ static int xadc_probe(struct platform_device *pdev)
>  	ret = request_irq(xadc->irq, xadc->ops->interrupt_handler, 0,
>  			  dev_name(dev), indio_dev);
>  	if (ret)
> -		goto err_clk_disable_unprepare;
> +		goto err_free_samplerate_trigger;
>  
>  	ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
>  	if (ret)
> @@ -1344,8 +1356,6 @@ static int xadc_probe(struct platform_device *pdev)
>  err_free_irq:
>  	free_irq(xadc->irq, indio_dev);
>  	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
> -err_clk_disable_unprepare:
> -	clk_disable_unprepare(xadc->clk);
>  err_free_samplerate_trigger:
>  	if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
>  		iio_trigger_free(xadc->samplerate_trigger);
> @@ -1372,7 +1382,6 @@ static int xadc_remove(struct platform_device *pdev)
>  	}
>  	free_irq(xadc->irq, indio_dev);
>  	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
> -	clk_disable_unprepare(xadc->clk);
>  
>  	return 0;
>  }

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

[Bartosz Golaszewski]

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(-)

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

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

[Jonathan Cameron]

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

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

[Bartosz Golaszewski]

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

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

[Jonathan Cameron]

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

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

[Bartosz Golaszewski]

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

[PATCH 5/5] iio: adc: xilinx: use iio devres helpers

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Shrink the code by using devres variants of all iio routines. This allows
us to entirely drop the remove() callback as well as significantly
simplifies error paths in probe().

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/iio/adc/xilinx-xadc-core.c | 97 ++++++++++--------------------
 1 file changed, 32 insertions(+), 65 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 4440b7a9bd36..c17705205456 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -706,11 +706,12 @@ static const struct iio_trigger_ops xadc_trigger_ops = {
 static struct iio_trigger *xadc_alloc_trigger(struct iio_dev *indio_dev,
 	const char *name)
 {
+	struct device *dev = indio_dev->dev.parent;
 	struct iio_trigger *trig;
 	int ret;
 
-	trig = iio_trigger_alloc("%s%d-%s", indio_dev->name,
-				indio_dev->id, name);
+	trig = devm_iio_trigger_alloc(dev, "%s%d-%s", indio_dev->name,
+				      indio_dev->id, name);
 	if (trig == NULL)
 		return ERR_PTR(-ENOMEM);
 
@@ -718,15 +719,11 @@ static struct iio_trigger *xadc_alloc_trigger(struct iio_dev *indio_dev,
 	trig->ops = &xadc_trigger_ops;
 	iio_trigger_set_drvdata(trig, iio_priv(indio_dev));
 
-	ret = iio_trigger_register(trig);
+	ret = devm_iio_trigger_register(dev, trig);
 	if (ret)
-		goto error_free_trig;
+		return ERR_PTR(ret);
 
 	return trig;
-
-error_free_trig:
-	iio_trigger_free(trig);
-	return ERR_PTR(ret);
 }
 
 static int xadc_power_adc_b(struct xadc *xadc, unsigned int seq_mode)
@@ -1247,39 +1244,35 @@ static int xadc_probe(struct platform_device *pdev)
 		return ret;
 
 	if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
-		ret = iio_triggered_buffer_setup(indio_dev,
-			&iio_pollfunc_store_time, &xadc_trigger_handler,
-			&xadc_buffer_ops);
+		ret = devm_iio_triggered_buffer_setup(dev, indio_dev,
+						      &iio_pollfunc_store_time,
+						      &xadc_trigger_handler,
+						      &xadc_buffer_ops);
 		if (ret)
 			return ret;
 
 		xadc->convst_trigger = xadc_alloc_trigger(indio_dev, "convst");
-		if (IS_ERR(xadc->convst_trigger)) {
-			ret = PTR_ERR(xadc->convst_trigger);
-			goto err_triggered_buffer_cleanup;
-		}
+		if (IS_ERR(xadc->convst_trigger))
+			return PTR_ERR(xadc->convst_trigger);
+
 		xadc->samplerate_trigger = xadc_alloc_trigger(indio_dev,
 			"samplerate");
-		if (IS_ERR(xadc->samplerate_trigger)) {
-			ret = PTR_ERR(xadc->samplerate_trigger);
-			goto err_free_convst_trigger;
-		}
+		if (IS_ERR(xadc->samplerate_trigger))
+			return PTR_ERR(xadc->samplerate_trigger);
 	}
 
 	xadc->clk = devm_clk_get(dev, NULL);
-	if (IS_ERR(xadc->clk)) {
-		ret = PTR_ERR(xadc->clk);
-		goto err_free_samplerate_trigger;
-	}
+	if (IS_ERR(xadc->clk))
+		return PTR_ERR(xadc->clk);
 
 	ret = clk_prepare_enable(xadc->clk);
 	if (ret)
-		goto err_free_samplerate_trigger;
+		return ret;
 
 	ret = devm_add_action_or_reset(dev,
 				       xadc_clk_disable_unprepare, xadc->clk);
 	if (ret)
-		goto err_free_samplerate_trigger;
+		return ret;
 
 	/*
 	 * Make sure not to exceed the maximum samplerate since otherwise the
@@ -1288,27 +1281,28 @@ static int xadc_probe(struct platform_device *pdev)
 	if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
 		ret = xadc_read_samplerate(xadc);
 		if (ret < 0)
-			goto err_free_samplerate_trigger;
+			return ret;
+
 		if (ret > XADC_MAX_SAMPLERATE) {
 			ret = xadc_write_samplerate(xadc, XADC_MAX_SAMPLERATE);
 			if (ret < 0)
-				goto err_free_samplerate_trigger;
+				return ret;
 		}
 	}
 
 	ret = devm_request_irq(dev, xadc->irq, xadc->ops->interrupt_handler, 0,
 			       dev_name(dev), indio_dev);
 	if (ret)
-		goto err_free_samplerate_trigger;
+		return ret;
 
 	ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
 				       &xadc->zynq_unmask_work);
 	if (ret)
-		goto err_free_samplerate_trigger;
+		return ret;
 
 	ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
 	if (ret)
-		goto err_free_samplerate_trigger;
+		return ret;
 
 	for (i = 0; i < 16; i++)
 		xadc_read_adc_reg(xadc, XADC_REG_THRESHOLD(i),
@@ -1316,7 +1310,7 @@ static int xadc_probe(struct platform_device *pdev)
 
 	ret = xadc_write_adc_reg(xadc, XADC_REG_CONF0, conf0);
 	if (ret)
-		goto err_free_samplerate_trigger;
+		return ret;
 
 	bipolar_mask = 0;
 	for (i = 0; i < indio_dev->num_channels; i++) {
@@ -1326,17 +1320,18 @@ 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_samplerate_trigger;
+		return ret;
+
 	ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1),
 		bipolar_mask >> 16);
 	if (ret)
-		goto err_free_samplerate_trigger;
+		return ret;
 
 	/* 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_samplerate_trigger;
+		return ret;
 
 	/* Set thresholds to min/max */
 	for (i = 0; i < 16; i++) {
@@ -1351,51 +1346,23 @@ 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_samplerate_trigger;
+			return ret;
 	}
 
 	/* Go to non-buffered mode */
 	xadc_postdisable(indio_dev);
 
-	ret = iio_device_register(indio_dev);
+	ret = devm_iio_device_register(dev, indio_dev);
 	if (ret)
-		goto err_free_samplerate_trigger;
+		return ret;
 
 	platform_set_drvdata(pdev, indio_dev);
 
-	return 0;
-
-err_free_samplerate_trigger:
-	if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
-		iio_trigger_free(xadc->samplerate_trigger);
-err_free_convst_trigger:
-	if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
-		iio_trigger_free(xadc->convst_trigger);
-err_triggered_buffer_cleanup:
-	if (xadc->ops->flags & XADC_FLAGS_BUFFERED)
-		iio_triggered_buffer_cleanup(indio_dev);
-
-	return ret;
-}
-
-static int xadc_remove(struct platform_device *pdev)
-{
-	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
-	struct xadc *xadc = iio_priv(indio_dev);
-
-	iio_device_unregister(indio_dev);
-	if (xadc->ops->flags & XADC_FLAGS_BUFFERED) {
-		iio_trigger_free(xadc->samplerate_trigger);
-		iio_trigger_free(xadc->convst_trigger);
-		iio_triggered_buffer_cleanup(indio_dev);
-	}
-
 	return 0;
 }
 
 static struct platform_driver xadc_driver = {
 	.probe = xadc_probe,
-	.remove = xadc_remove,
 	.driver = {
 		.name = "xadc",
 		.of_match_table = xadc_of_match_table,
-- 
2.29.1