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

[PATCH v2 0/4] 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.

v1 -> v2:
- squash three patches adding more devres calls into one for easier review
- don't insist on the 80 characters limit
- add a new helper: devm_krealloc_array() and use it

Bartosz Golaszewski (4):
  device: provide devm_krealloc_array()
  iio: adc: xilinx: use helper variable for &pdev->dev
  iio: adc: xilinx: use devm_krealloc_array() instead of kfree() +
    kcalloc()
  iio: adc: xilinx: use more devres helpers and remove remove()

 drivers/iio/adc/xilinx-xadc-core.c | 151 +++++++++++++----------------
 include/linux/device.h             |  11 +++
 2 files changed, 80 insertions(+), 82 deletions(-)

-- 
2.29.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/4] device: provide devm_krealloc_array()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

When allocating an array of elements, users should check for
multiplication overflow or preferably use one of the provided helpers
like: devm_kmalloc_array().

This provides devm_krealloc_array() for users who want to reallocate
managed arrays.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/device.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/device.h b/include/linux/device.h
index 5ed101be7b2e..e77203faea55 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -226,6 +226,17 @@ static inline void *devm_kmalloc_array(struct device *dev,
 
 	return devm_kmalloc(dev, bytes, flags);
 }
+static inline void *
+devm_krealloc_array(struct device *dev, void *ptr, size_t new_n,
+		    size_t new_size, gfp_t gfp)
+{
+	size_t bytes;
+
+	if (unlikely(check_mul_overflow(new_n, new_size, &bytes)))
+		return NULL;
+
+	return devm_krealloc(dev, ptr, bytes, gfp);
+}
 static inline void *devm_kcalloc(struct device *dev,
 				 size_t n, size_t size, gfp_t flags)
 {
-- 
2.29.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] device: provide devm_krealloc_array()

[Jonathan Cameron]

On Mon,  2 Nov 2020 15:22:25 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> When allocating an array of elements, users should check for
> multiplication overflow or preferably use one of the provided helpers
> like: devm_kmalloc_array().
> 
> This provides devm_krealloc_array() for users who want to reallocate
> managed arrays.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

+CC Greg KH. 

As this is going into a very generic place I'd like a relevant ack.
That file is a bit of a wild west for acks, but Greg seems most
appropriate person.

So Greg, any comments on this one?

Thanks,

Jonathan

> ---
>  include/linux/device.h | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 5ed101be7b2e..e77203faea55 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -226,6 +226,17 @@ static inline void *devm_kmalloc_array(struct device *dev,
>  
>  	return devm_kmalloc(dev, bytes, flags);
>  }
> +static inline void *
> +devm_krealloc_array(struct device *dev, void *ptr, size_t new_n,
> +		    size_t new_size, gfp_t gfp)
> +{
> +	size_t bytes;
> +
> +	if (unlikely(check_mul_overflow(new_n, new_size, &bytes)))
> +		return NULL;
> +
> +	return devm_krealloc(dev, ptr, bytes, gfp);
> +}
>  static inline void *devm_kcalloc(struct device *dev,
>  				 size_t n, size_t size, gfp_t flags)
>  {


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] device: provide devm_krealloc_array()

[Greg KH]

On Sat, Nov 14, 2020 at 03:46:41PM +0000, Jonathan Cameron wrote:
> On Mon,  2 Nov 2020 15:22:25 +0100
> Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > 
> > When allocating an array of elements, users should check for
> > multiplication overflow or preferably use one of the provided helpers
> > like: devm_kmalloc_array().
> > 
> > This provides devm_krealloc_array() for users who want to reallocate
> > managed arrays.
> > 
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> +CC Greg KH. 
> 
> As this is going into a very generic place I'd like a relevant ack.
> That file is a bit of a wild west for acks, but Greg seems most
> appropriate person.
> 
> So Greg, any comments on this one?

As there is only 1 user of this function in the patch series, you don't
save any extra code space here, I don't think this is worth it.

We are seeing less and less gains from these new devm_* additions, and
only more confusion and problems with them.  So perhaps don't add this?
I don't think it is needed.

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] device: provide devm_krealloc_array()

[Bartosz Golaszewski]

On Sat, Nov 14, 2020 at 5:16 PM Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Sat, Nov 14, 2020 at 03:46:41PM +0000, Jonathan Cameron wrote:
> > On Mon,  2 Nov 2020 15:22:25 +0100
> > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > When allocating an array of elements, users should check for
> > > multiplication overflow or preferably use one of the provided helpers
> > > like: devm_kmalloc_array().
> > >
> > > This provides devm_krealloc_array() for users who want to reallocate
> > > managed arrays.
> > >
> > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > +CC Greg KH.
> >
> > As this is going into a very generic place I'd like a relevant ack.
> > That file is a bit of a wild west for acks, but Greg seems most
> > appropriate person.
> >
> > So Greg, any comments on this one?
>
> As there is only 1 user of this function in the patch series, you don't
> save any extra code space here, I don't think this is worth it.
>

It's worth it in that the overflow check before allocation doesn't
seem to belong in a driver IMO but is a general check that should live
in common code.

> We are seeing less and less gains from these new devm_* additions, and
> only more confusion and problems with them.  So perhaps don't add this?
> I don't think it is needed.
>

I think you're referring to the discussion on
devm_platform_ioremap_resource()? I would argue that consolidation of
common operations in helpers is rarely a bad thing but it's a
discussion for another thread.

I'm not too attached to this patch - if you think this should be
dropped then fine, but I don't see how the name devm_krealloc_array()
can confuse anyone.

Bartosz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] device: provide devm_krealloc_array()

[Bartosz Golaszewski]

On Mon, Nov 16, 2020 at 11:18 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> On Sat, Nov 14, 2020 at 5:16 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> >
> > On Sat, Nov 14, 2020 at 03:46:41PM +0000, Jonathan Cameron wrote:
> > > On Mon,  2 Nov 2020 15:22:25 +0100
> > > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > >
> > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > >
> > > > When allocating an array of elements, users should check for
> > > > multiplication overflow or preferably use one of the provided helpers
> > > > like: devm_kmalloc_array().
> > > >
> > > > This provides devm_krealloc_array() for users who want to reallocate
> > > > managed arrays.
> > > >
> > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > >
> > > +CC Greg KH.
> > >
> > > As this is going into a very generic place I'd like a relevant ack.
> > > That file is a bit of a wild west for acks, but Greg seems most
> > > appropriate person.
> > >
> > > So Greg, any comments on this one?
> >
> > As there is only 1 user of this function in the patch series, you don't
> > save any extra code space here, I don't think this is worth it.
> >
>
> It's worth it in that the overflow check before allocation doesn't
> seem to belong in a driver IMO but is a general check that should live
> in common code.
>
> > We are seeing less and less gains from these new devm_* additions, and
> > only more confusion and problems with them.  So perhaps don't add this?
> > I don't think it is needed.
> >
>
> I think you're referring to the discussion on
> devm_platform_ioremap_resource()? I would argue that consolidation of
> common operations in helpers is rarely a bad thing but it's a
> discussion for another thread.
>
> I'm not too attached to this patch - if you think this should be
> dropped then fine, but I don't see how the name devm_krealloc_array()
> can confuse anyone.
>

Greg: what's the final call on this?

Bartosz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] device: provide devm_krealloc_array()

[Jonathan Cameron]

On Mon, 23 Nov 2020 12:38:26 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> On Mon, Nov 16, 2020 at 11:18 AM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> >
> > On Sat, Nov 14, 2020 at 5:16 PM Greg KH <gregkh@linuxfoundation.org> wrote:  
> > >
> > > On Sat, Nov 14, 2020 at 03:46:41PM +0000, Jonathan Cameron wrote:  
> > > > On Mon,  2 Nov 2020 15:22:25 +0100
> > > > Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > >  
> > > > > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> > > > >
> > > > > When allocating an array of elements, users should check for
> > > > > multiplication overflow or preferably use one of the provided helpers
> > > > > like: devm_kmalloc_array().
> > > > >
> > > > > This provides devm_krealloc_array() for users who want to reallocate
> > > > > managed arrays.
> > > > >
> > > > > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>  
> > > >
> > > > +CC Greg KH.
> > > >
> > > > As this is going into a very generic place I'd like a relevant ack.
> > > > That file is a bit of a wild west for acks, but Greg seems most
> > > > appropriate person.
> > > >
> > > > So Greg, any comments on this one?  
> > >
> > > As there is only 1 user of this function in the patch series, you don't
> > > save any extra code space here, I don't think this is worth it.
> > >  
> >
> > It's worth it in that the overflow check before allocation doesn't
> > seem to belong in a driver IMO but is a general check that should live
> > in common code.
> >  
> > > We are seeing less and less gains from these new devm_* additions, and
> > > only more confusion and problems with them.  So perhaps don't add this?
> > > I don't think it is needed.
> > >  
> >
> > I think you're referring to the discussion on
> > devm_platform_ioremap_resource()? I would argue that consolidation of
> > common operations in helpers is rarely a bad thing but it's a
> > discussion for another thread.
> >
> > I'm not too attached to this patch - if you think this should be
> > dropped then fine, but I don't see how the name devm_krealloc_array()
> > can confuse anyone.
> >  
> 
> Greg: what's the final call on this?

Reroll the series without this patch.  If it turns out to be a good idea
in the long run we can always bring it back, but for now it's blocking
the rest of the series.

Thanks,

Jonathan

> 
> Bartosz


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/4] 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/4] iio: adc: xilinx: use devm_krealloc_array() instead of kfree() + kcalloc()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

We now have devm_krealloc_array() 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 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 8494eb424b33..7792aa4cf9cb 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -586,14 +586,18 @@ static int xadc_update_scan_mode(struct iio_dev *indio_dev,
 {
 	struct xadc *xadc = iio_priv(indio_dev);
 	unsigned int n;
+	void *data;
 
 	n = bitmap_weight(mask, indio_dev->masklength);
 
-	kfree(xadc->data);
-	xadc->data = kcalloc(n, sizeof(*xadc->data), GFP_KERNEL);
-	if (!xadc->data)
+	data = devm_krealloc_array(indio_dev->dev.parent, xadc->data, n,
+				   sizeof(*xadc->data), GFP_KERNEL);
+	if (!data)
 		return -ENOMEM;
 
+	memset(data, 0, n * sizeof(*xadc->data));
+	xadc->data = data;
+
 	return 0;
 }
 
@@ -1372,7 +1376,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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/4] iio: adc: xilinx: use more devres helpers and remove remove()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

In order to simplify resource management and error paths in probe() and
entirely drop the remove() callback - use devres helpers wherever
possible. Define devm actions for cancelling the delayed work and
disabling the clock.

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

diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
index 7792aa4cf9cb..6ce1745b4995 100644
--- a/drivers/iio/adc/xilinx-xadc-core.c
+++ b/drivers/iio/adc/xilinx-xadc-core.c
@@ -709,11 +709,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);
 
@@ -721,15 +722,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)
@@ -1188,6 +1185,20 @@ 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 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;
@@ -1236,34 +1247,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)
+		return ret;
 
 	/*
 	 * Make sure not to exceed the maximum samplerate since otherwise the
@@ -1272,22 +1284,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 = 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)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
+				       &xadc->zynq_unmask_work);
 	if (ret)
-		goto err_clk_disable_unprepare;
+		return ret;
 
 	ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
 	if (ret)
-		goto err_free_irq;
+		return ret;
 
 	for (i = 0; i < 16; i++)
 		xadc_read_adc_reg(xadc, XADC_REG_THRESHOLD(i),
@@ -1295,7 +1313,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;
+		return ret;
 
 	bipolar_mask = 0;
 	for (i = 0; i < indio_dev->num_channels; i++) {
@@ -1305,17 +1323,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_irq;
+		return ret;
+
 	ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1),
 		bipolar_mask >> 16);
 	if (ret)
-		goto err_free_irq;
+		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_irq;
+		return ret;
 
 	/* Set thresholds to min/max */
 	for (i = 0; i < 16; i++) {
@@ -1330,59 +1349,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_irq;
+			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_irq;
+		return ret;
 
 	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_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);
-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);
-	}
-	free_irq(xadc->irq, indio_dev);
-	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
-	clk_disable_unprepare(xadc->clk);
-
 	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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] iio: adc: xilinx: use more devres helpers and remove remove()

[Jonathan Cameron]

On Mon,  2 Nov 2020 15:22:28 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> In order to simplify resource management and error paths in probe() and
> entirely drop the remove() callback - use devres helpers wherever
> possible. Define devm actions for cancelling the delayed work and
> disabling the clock.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
One addition inline.

thanks,

Jonathan

> ---
>  drivers/iio/adc/xilinx-xadc-core.c | 129 +++++++++++++----------------
>  1 file changed, 56 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/iio/adc/xilinx-xadc-core.c b/drivers/iio/adc/xilinx-xadc-core.c
> index 7792aa4cf9cb..6ce1745b4995 100644
> --- a/drivers/iio/adc/xilinx-xadc-core.c
> +++ b/drivers/iio/adc/xilinx-xadc-core.c
> @@ -709,11 +709,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);
>  
> @@ -721,15 +722,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)
> @@ -1188,6 +1185,20 @@ 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 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;
> @@ -1236,34 +1247,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)
> +		return ret;
>  
>  	/*
>  	 * Make sure not to exceed the maximum samplerate since otherwise the
> @@ -1272,22 +1284,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 = 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)
> +		return ret;
> +
> +	ret = devm_add_action_or_reset(dev, xadc_cancel_delayed_work,
> +				       &xadc->zynq_unmask_work);
>  	if (ret)
> -		goto err_clk_disable_unprepare;
> +		return ret;
>  
>  	ret = xadc->ops->setup(pdev, indio_dev, xadc->irq);
>  	if (ret)
> -		goto err_free_irq;
> +		return ret;
>  
>  	for (i = 0; i < 16; i++)
>  		xadc_read_adc_reg(xadc, XADC_REG_THRESHOLD(i),
> @@ -1295,7 +1313,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;
> +		return ret;
>  
>  	bipolar_mask = 0;
>  	for (i = 0; i < indio_dev->num_channels; i++) {
> @@ -1305,17 +1323,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_irq;
> +		return ret;
> +
>  	ret = xadc_write_adc_reg(xadc, XADC_REG_INPUT_MODE(1),
>  		bipolar_mask >> 16);
>  	if (ret)
> -		goto err_free_irq;
> +		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_irq;
> +		return ret;
>  
>  	/* Set thresholds to min/max */
>  	for (i = 0; i < 16; i++) {
> @@ -1330,59 +1349,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_irq;
> +			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_irq;
> +		return ret;
>  
>  	platform_set_drvdata(pdev, indio_dev);

I haven't checked carefully, but it looks like you don't need
this any more as there are no remaining calls to
platform_get_drvdata().


I can clean that up whilst applying as long as you confirm
I'm right!

Thanks,

Jonathan

>  
> -	return 0;
> -
> -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);
> -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);
> -	}
> -	free_irq(xadc->irq, indio_dev);
> -	cancel_delayed_work_sync(&xadc->zynq_unmask_work);
> -	clk_disable_unprepare(xadc->clk);
> -
>  	return 0;
>  }
>  
>  static struct platform_driver xadc_driver = {
>  	.probe = xadc_probe,
> -	.remove = xadc_remove,
>  	.driver = {
>  		.name = "xadc",
>  		.of_match_table = xadc_of_match_table,


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] iio: adc: xilinx: use more devres helpers and remove remove()

[Bartosz Golaszewski]

On Sun, Nov 8, 2020 at 6:08 PM Jonathan Cameron <jic23@kernel.org> wrote:
>

[snip]

>
> I haven't checked carefully, but it looks like you don't need
> this any more as there are no remaining calls to
> platform_get_drvdata().
>

This looks to be correct.

There's a call to iio_trigger_get_drvdata() and it accesses the same
drvdata but it's called on the trigger device and there's a
corresponding call to iio_trigger_set_drvdata() for it.

>
> I can clean that up whilst applying as long as you confirm
> I'm right!
>

Yes please do, thanks!

Bartosz

> Thanks,
>
> Jonathan
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/4] iio: adc: xilinx: use even more devres

[Jonathan Cameron]

On Mon,  2 Nov 2020 15:22:24 +0100
Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> 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.
Given the generic nature of the additions to device.h I'm going to
let this one sit a little longer for review.

Remind me in a few weeks time if I seem to have lost it.
Also, do a quick sanity check on whether I am fine to drop
the platform_set_drvdata in the final patch.

Thanks,

Jonathan

> 
> v1 -> v2:
> - squash three patches adding more devres calls into one for easier review
> - don't insist on the 80 characters limit
> - add a new helper: devm_krealloc_array() and use it
> 
> Bartosz Golaszewski (4):
>   device: provide devm_krealloc_array()
>   iio: adc: xilinx: use helper variable for &pdev->dev
>   iio: adc: xilinx: use devm_krealloc_array() instead of kfree() +
>     kcalloc()
>   iio: adc: xilinx: use more devres helpers and remove remove()
> 
>  drivers/iio/adc/xilinx-xadc-core.c | 151 +++++++++++++----------------
>  include/linux/device.h             |  11 +++
>  2 files changed, 80 insertions(+), 82 deletions(-)
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel