linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/3] iio: adc: ti_am335x_adc: alloc channels via devm_kcalloc()
@ 2020-04-28 11:14 Alexandru Ardelean
  2020-04-28 11:14 ` [PATCH 2/3] iio: adc: ti_am335x_adc: alloc kfifo & IRQ via devm_ functions Alexandru Ardelean
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Alexandru Ardelean @ 2020-04-28 11:14 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: jic23, rachna, mugunthanvnm, vigneshr, afd, Alexandru Ardelean

This change attaches the life-cycle of the channels array to the parent
device object that is attached to the IIO device.
This way we can remove from the cleanup code, the explicit
tiadc_channels_remove() which simply does a kfree() on the channels array.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---

The reason I'm targetting this driver, is because it's among the few
places where 'indio_dev->buffer' is accessed directly in the driver, and
that makes it difficult to think of a sane-backwards-compatible way to
do multiple IIO buffers.

 drivers/iio/adc/ti_am335x_adc.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 9d984f2a8ba7..d932fe383a24 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -428,7 +428,8 @@ static const char * const chan_name_ain[] = {
 	"AIN7",
 };
 
-static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
+static int tiadc_channel_init(struct device *dev, struct iio_dev *indio_dev,
+			      int channels)
 {
 	struct tiadc_device *adc_dev = iio_priv(indio_dev);
 	struct iio_chan_spec *chan_array;
@@ -436,7 +437,8 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
 	int i;
 
 	indio_dev->num_channels = channels;
-	chan_array = kcalloc(channels, sizeof(*chan_array), GFP_KERNEL);
+	chan_array = devm_kcalloc(dev, channels, sizeof(*chan_array),
+				  GFP_KERNEL);
 	if (chan_array == NULL)
 		return -ENOMEM;
 
@@ -459,11 +461,6 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
 	return 0;
 }
 
-static void tiadc_channels_remove(struct iio_dev *indio_dev)
-{
-	kfree(indio_dev->channels);
-}
-
 static int tiadc_read_raw(struct iio_dev *indio_dev,
 		struct iio_chan_spec const *chan,
 		int *val, int *val2, long mask)
@@ -635,7 +632,7 @@ static int tiadc_probe(struct platform_device *pdev)
 	tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
 	mutex_init(&adc_dev->fifo1_lock);
 
-	err = tiadc_channel_init(indio_dev, adc_dev->channels);
+	err = tiadc_channel_init(&pdev->dev, indio_dev, adc_dev->channels);
 	if (err < 0)
 		return err;
 
@@ -666,7 +663,6 @@ static int tiadc_probe(struct platform_device *pdev)
 err_buffer_unregister:
 	tiadc_iio_buffered_hardware_remove(indio_dev);
 err_free_channels:
-	tiadc_channels_remove(indio_dev);
 	return err;
 }
 
@@ -684,7 +680,6 @@ static int tiadc_remove(struct platform_device *pdev)
 	}
 	iio_device_unregister(indio_dev);
 	tiadc_iio_buffered_hardware_remove(indio_dev);
-	tiadc_channels_remove(indio_dev);
 
 	step_en = get_adc_step_mask(adc_dev);
 	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 2/3] iio: adc: ti_am335x_adc: alloc kfifo & IRQ via devm_ functions
  2020-04-28 11:14 [PATCH 1/3] iio: adc: ti_am335x_adc: alloc channels via devm_kcalloc() Alexandru Ardelean
@ 2020-04-28 11:14 ` Alexandru Ardelean
  2020-07-05 12:04   ` Jonathan Cameron
  2020-04-28 11:14 ` [PATCH 3/3] iio: adc: ti_am335x_adc: convert rest of probe to " Alexandru Ardelean
  2020-07-05 12:04 ` [PATCH 1/3] iio: adc: ti_am335x_adc: alloc channels via devm_kcalloc() Jonathan Cameron
  2 siblings, 1 reply; 6+ messages in thread
From: Alexandru Ardelean @ 2020-04-28 11:14 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: jic23, rachna, mugunthanvnm, vigneshr, afd, Alexandru Ardelean

This change attaches the life-cycle of the kfifo buffer & IRQ to the
parent-device. This in turn cleans up the exit & error paths, since we
don't need to explicitly cleanup these resources.

The main intent here is to remove the explicit cleanup of the
'indio_dev->buffer' via 'iio_kfifo_free(indio_dev->buffer);'.

As we want to add support for multiple buffers per IIO device, having it
exposed like this makes it tricky to consider a safe backwards compatible
approach for it.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/ti_am335x_adc.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index d932fe383a24..03b2ab649cc3 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -377,7 +377,8 @@ static const struct iio_buffer_setup_ops tiadc_buffer_setup_ops = {
 	.postdisable = &tiadc_buffer_postdisable,
 };
 
-static int tiadc_iio_buffered_hardware_setup(struct iio_dev *indio_dev,
+static int tiadc_iio_buffered_hardware_setup(struct device *dev,
+	struct iio_dev *indio_dev,
 	irqreturn_t (*pollfunc_bh)(int irq, void *p),
 	irqreturn_t (*pollfunc_th)(int irq, void *p),
 	int irq,
@@ -387,13 +388,13 @@ static int tiadc_iio_buffered_hardware_setup(struct iio_dev *indio_dev,
 	struct iio_buffer *buffer;
 	int ret;
 
-	buffer = iio_kfifo_allocate();
+	buffer = devm_iio_kfifo_allocate(dev);
 	if (!buffer)
 		return -ENOMEM;
 
 	iio_device_attach_buffer(indio_dev, buffer);
 
-	ret = request_threaded_irq(irq,	pollfunc_th, pollfunc_bh,
+	ret = devm_request_threaded_irq(dev, irq, pollfunc_th, pollfunc_bh,
 				flags, indio_dev->name, indio_dev);
 	if (ret)
 		goto error_kfifo_free;
@@ -408,15 +409,6 @@ static int tiadc_iio_buffered_hardware_setup(struct iio_dev *indio_dev,
 	return ret;
 }
 
-static void tiadc_iio_buffered_hardware_remove(struct iio_dev *indio_dev)
-{
-	struct tiadc_device *adc_dev = iio_priv(indio_dev);
-
-	free_irq(adc_dev->mfd_tscadc->irq, indio_dev);
-	iio_kfifo_free(indio_dev->buffer);
-}
-
-
 static const char * const chan_name_ain[] = {
 	"AIN0",
 	"AIN1",
@@ -636,7 +628,7 @@ static int tiadc_probe(struct platform_device *pdev)
 	if (err < 0)
 		return err;
 
-	err = tiadc_iio_buffered_hardware_setup(indio_dev,
+	err = tiadc_iio_buffered_hardware_setup(&pdev->dev, indio_dev,
 		&tiadc_worker_h,
 		&tiadc_irq_h,
 		adc_dev->mfd_tscadc->irq,
@@ -661,7 +653,6 @@ static int tiadc_probe(struct platform_device *pdev)
 err_dma:
 	iio_device_unregister(indio_dev);
 err_buffer_unregister:
-	tiadc_iio_buffered_hardware_remove(indio_dev);
 err_free_channels:
 	return err;
 }
@@ -679,7 +670,6 @@ static int tiadc_remove(struct platform_device *pdev)
 		dma_release_channel(dma->chan);
 	}
 	iio_device_unregister(indio_dev);
-	tiadc_iio_buffered_hardware_remove(indio_dev);
 
 	step_en = get_adc_step_mask(adc_dev);
 	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH 3/3] iio: adc: ti_am335x_adc: convert rest of probe to devm_ functions
  2020-04-28 11:14 [PATCH 1/3] iio: adc: ti_am335x_adc: alloc channels via devm_kcalloc() Alexandru Ardelean
  2020-04-28 11:14 ` [PATCH 2/3] iio: adc: ti_am335x_adc: alloc kfifo & IRQ via devm_ functions Alexandru Ardelean
@ 2020-04-28 11:14 ` Alexandru Ardelean
  2020-05-03 10:38   ` Jonathan Cameron
  2020-07-05 12:04 ` [PATCH 1/3] iio: adc: ti_am335x_adc: alloc channels via devm_kcalloc() Jonathan Cameron
  2 siblings, 1 reply; 6+ messages in thread
From: Alexandru Ardelean @ 2020-04-28 11:14 UTC (permalink / raw)
  To: linux-iio, linux-kernel
  Cc: jic23, rachna, mugunthanvnm, vigneshr, afd, Alexandru Ardelean

This change converts the rest of the probe to use devm_ functions.
Consequently this allows us to remove the remove hook.

It tries to preserve the initial order or probe & remove.
The devm_add_action() call hooks the cleanup routine (what's needed still
for the remove part).
If that doesn't work the DMA channel is cleaned up manually inside the
probe hook. This done (like this) because the remove hook has a peculiar
cleanup that tries to restore a step-mask, and that only seems to happen on
the remove hook, and not in any probe error-cleanup paths.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/iio/adc/ti_am335x_adc.c | 63 +++++++++++++++++----------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
index 03b2ab649cc3..9fac83e036c1 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -562,6 +562,18 @@ static int tiadc_request_dma(struct platform_device *pdev,
 	return -ENOMEM;
 }
 
+static void tiadc_cleanup_dma(struct tiadc_device *adc_dev)
+{
+	struct tiadc_dma *dma = &adc_dev->dma;
+
+	if (!dma->chan)
+		return;
+
+	dma_free_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
+			  dma->buf, dma->addr);
+	dma_release_channel(dma->chan);
+}
+
 static int tiadc_parse_dt(struct platform_device *pdev,
 			  struct tiadc_device *adc_dev)
 {
@@ -593,6 +605,17 @@ static int tiadc_parse_dt(struct platform_device *pdev,
 	return 0;
 }
 
+static void tiadc_cleanup(void *data)
+{
+	struct tiadc_device *adc_dev = data;
+	u32 step_en;
+
+	tiadc_cleanup_dma(adc_dev);
+
+	step_en = get_adc_step_mask(adc_dev);
+	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
+}
+
 static int tiadc_probe(struct platform_device *pdev)
 {
 	struct iio_dev		*indio_dev;
@@ -635,48 +658,27 @@ static int tiadc_probe(struct platform_device *pdev)
 		IRQF_SHARED,
 		&tiadc_buffer_setup_ops);
 
+	err = devm_iio_device_register(&pdev->dev, indio_dev);
 	if (err)
-		goto err_free_channels;
-
-	err = iio_device_register(indio_dev);
-	if (err)
-		goto err_buffer_unregister;
+		return err;
 
 	platform_set_drvdata(pdev, indio_dev);
 
 	err = tiadc_request_dma(pdev, adc_dev);
 	if (err && err == -EPROBE_DEFER)
-		goto err_dma;
+		return err;
+
+	err = devm_add_action(&pdev->dev, tiadc_cleanup, adc_dev);
+	if (err)
+		goto err_free_dma;
 
 	return 0;
 
-err_dma:
-	iio_device_unregister(indio_dev);
-err_buffer_unregister:
-err_free_channels:
+err_free_dma:
+	tiadc_cleanup_dma(adc_dev);
 	return err;
 }
 
-static int tiadc_remove(struct platform_device *pdev)
-{
-	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
-	struct tiadc_device *adc_dev = iio_priv(indio_dev);
-	struct tiadc_dma *dma = &adc_dev->dma;
-	u32 step_en;
-
-	if (dma->chan) {
-		dma_free_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
-				  dma->buf, dma->addr);
-		dma_release_channel(dma->chan);
-	}
-	iio_device_unregister(indio_dev);
-
-	step_en = get_adc_step_mask(adc_dev);
-	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
-
-	return 0;
-}
-
 static int __maybe_unused tiadc_suspend(struct device *dev)
 {
 	struct iio_dev *indio_dev = dev_get_drvdata(dev);
@@ -723,7 +725,6 @@ static struct platform_driver tiadc_driver = {
 		.of_match_table = ti_adc_dt_ids,
 	},
 	.probe	= tiadc_probe,
-	.remove	= tiadc_remove,
 };
 module_platform_driver(tiadc_driver);
 
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH 3/3] iio: adc: ti_am335x_adc: convert rest of probe to devm_ functions
  2020-04-28 11:14 ` [PATCH 3/3] iio: adc: ti_am335x_adc: convert rest of probe to " Alexandru Ardelean
@ 2020-05-03 10:38   ` Jonathan Cameron
  0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2020-05-03 10:38 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, rachna, mugunthanvnm, vigneshr, afd

On Tue, 28 Apr 2020 14:14:30 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change converts the rest of the probe to use devm_ functions.
> Consequently this allows us to remove the remove hook.
> 
> It tries to preserve the initial order or probe & remove.
> The devm_add_action() call hooks the cleanup routine (what's needed still
> for the remove part).
> If that doesn't work the DMA channel is cleaned up manually inside the
> probe hook. This done (like this) because the remove hook has a peculiar
> cleanup that tries to restore a step-mask, and that only seems to happen on
> the remove hook, and not in any probe error-cleanup paths.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

First two patches are fine, but this last one is (as you've noted) more
complex.  I'd like to cleanup the complexity rather than papering over
it.

So the real question is why we need to restore the step-mask on exit, but
not in other paths in the code.

From what I recall (and it's been quite a lot of years) the step mask
is controlling the 'scan' so that we capture the set of enabled channels
and no others (there is a mux that is being controlled).

The current optimization is to not bother resetting that to empty when
we read individual channels, or come out of buffered mode because we
will set it anyway when moving to some new mode.

What I can't understand is why we need to set it in the exit path?
This is a complex corner given the involvement of the touchscreen
driver and mfd.  My first inclination is we may be better off leaving
it alone unless we have a test setup to make sure we fully understand
what is going on.

Given your stated reason for tidying this up was to deal with the
buffer stuff and this has no impact on that, I'll take patches 1 and 2 for
now and leave this one out.

However, I'd like to leave more time for comments on those two as well
(though they seem 'obviously' correct to me).

Thanks,

Jonathan


> ---
>  drivers/iio/adc/ti_am335x_adc.c | 63 +++++++++++++++++----------------
>  1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 03b2ab649cc3..9fac83e036c1 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -562,6 +562,18 @@ static int tiadc_request_dma(struct platform_device *pdev,
>  	return -ENOMEM;
>  }
>  
> +static void tiadc_cleanup_dma(struct tiadc_device *adc_dev)
> +{
> +	struct tiadc_dma *dma = &adc_dev->dma;
> +
> +	if (!dma->chan)
> +		return;
> +
> +	dma_free_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
> +			  dma->buf, dma->addr);
> +	dma_release_channel(dma->chan);
> +}
> +
>  static int tiadc_parse_dt(struct platform_device *pdev,
>  			  struct tiadc_device *adc_dev)
>  {
> @@ -593,6 +605,17 @@ static int tiadc_parse_dt(struct platform_device *pdev,
>  	return 0;
>  }
>  
> +static void tiadc_cleanup(void *data)
> +{
> +	struct tiadc_device *adc_dev = data;
> +	u32 step_en;
> +
> +	tiadc_cleanup_dma(adc_dev);
> +
> +	step_en = get_adc_step_mask(adc_dev);
> +	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
> +}
> +
>  static int tiadc_probe(struct platform_device *pdev)
>  {
>  	struct iio_dev		*indio_dev;
> @@ -635,48 +658,27 @@ static int tiadc_probe(struct platform_device *pdev)
>  		IRQF_SHARED,
>  		&tiadc_buffer_setup_ops);
>  
> +	err = devm_iio_device_register(&pdev->dev, indio_dev);
>  	if (err)
> -		goto err_free_channels;
> -
> -	err = iio_device_register(indio_dev);
> -	if (err)
> -		goto err_buffer_unregister;
> +		return err;
>  
>  	platform_set_drvdata(pdev, indio_dev);
>  
>  	err = tiadc_request_dma(pdev, adc_dev);
>  	if (err && err == -EPROBE_DEFER)
> -		goto err_dma;
> +		return err;
> +
> +	err = devm_add_action(&pdev->dev, tiadc_cleanup, adc_dev);
> +	if (err)
> +		goto err_free_dma;
>  
>  	return 0;
>  
> -err_dma:
> -	iio_device_unregister(indio_dev);
> -err_buffer_unregister:
> -err_free_channels:
> +err_free_dma:
> +	tiadc_cleanup_dma(adc_dev);
>  	return err;
>  }
>  
> -static int tiadc_remove(struct platform_device *pdev)
> -{
> -	struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> -	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> -	struct tiadc_dma *dma = &adc_dev->dma;
> -	u32 step_en;
> -
> -	if (dma->chan) {
> -		dma_free_coherent(dma->chan->device->dev, DMA_BUFFER_SIZE,
> -				  dma->buf, dma->addr);
> -		dma_release_channel(dma->chan);
> -	}
> -	iio_device_unregister(indio_dev);
> -
> -	step_en = get_adc_step_mask(adc_dev);
> -	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
> -
> -	return 0;
> -}
> -
>  static int __maybe_unused tiadc_suspend(struct device *dev)
>  {
>  	struct iio_dev *indio_dev = dev_get_drvdata(dev);
> @@ -723,7 +725,6 @@ static struct platform_driver tiadc_driver = {
>  		.of_match_table = ti_adc_dt_ids,
>  	},
>  	.probe	= tiadc_probe,
> -	.remove	= tiadc_remove,
>  };
>  module_platform_driver(tiadc_driver);
>  


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 1/3] iio: adc: ti_am335x_adc: alloc channels via devm_kcalloc()
  2020-04-28 11:14 [PATCH 1/3] iio: adc: ti_am335x_adc: alloc channels via devm_kcalloc() Alexandru Ardelean
  2020-04-28 11:14 ` [PATCH 2/3] iio: adc: ti_am335x_adc: alloc kfifo & IRQ via devm_ functions Alexandru Ardelean
  2020-04-28 11:14 ` [PATCH 3/3] iio: adc: ti_am335x_adc: convert rest of probe to " Alexandru Ardelean
@ 2020-07-05 12:04 ` Jonathan Cameron
  2 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2020-07-05 12:04 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, rachna, mugunthanvnm, vigneshr, afd

On Tue, 28 Apr 2020 14:14:28 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change attaches the life-cycle of the channels array to the parent
> device object that is attached to the IIO device.
> This way we can remove from the cleanup code, the explicit
> tiadc_channels_remove() which simply does a kfree() on the channels array.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Still not sure on patch 3 but I'll pick up the first 2 so we can forget
about those.

Applied to the togreg branch of iio.git and pushed out as testing for
all the usual reasons.

Thanks,

Jonathan

> ---
> 
> The reason I'm targetting this driver, is because it's among the few
> places where 'indio_dev->buffer' is accessed directly in the driver, and
> that makes it difficult to think of a sane-backwards-compatible way to
> do multiple IIO buffers.
> 
>  drivers/iio/adc/ti_am335x_adc.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index 9d984f2a8ba7..d932fe383a24 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -428,7 +428,8 @@ static const char * const chan_name_ain[] = {
>  	"AIN7",
>  };
>  
> -static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
> +static int tiadc_channel_init(struct device *dev, struct iio_dev *indio_dev,
> +			      int channels)
>  {
>  	struct tiadc_device *adc_dev = iio_priv(indio_dev);
>  	struct iio_chan_spec *chan_array;
> @@ -436,7 +437,8 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
>  	int i;
>  
>  	indio_dev->num_channels = channels;
> -	chan_array = kcalloc(channels, sizeof(*chan_array), GFP_KERNEL);
> +	chan_array = devm_kcalloc(dev, channels, sizeof(*chan_array),
> +				  GFP_KERNEL);
>  	if (chan_array == NULL)
>  		return -ENOMEM;
>  
> @@ -459,11 +461,6 @@ static int tiadc_channel_init(struct iio_dev *indio_dev, int channels)
>  	return 0;
>  }
>  
> -static void tiadc_channels_remove(struct iio_dev *indio_dev)
> -{
> -	kfree(indio_dev->channels);
> -}
> -
>  static int tiadc_read_raw(struct iio_dev *indio_dev,
>  		struct iio_chan_spec const *chan,
>  		int *val, int *val2, long mask)
> @@ -635,7 +632,7 @@ static int tiadc_probe(struct platform_device *pdev)
>  	tiadc_writel(adc_dev, REG_FIFO1THR, FIFO1_THRESHOLD);
>  	mutex_init(&adc_dev->fifo1_lock);
>  
> -	err = tiadc_channel_init(indio_dev, adc_dev->channels);
> +	err = tiadc_channel_init(&pdev->dev, indio_dev, adc_dev->channels);
>  	if (err < 0)
>  		return err;
>  
> @@ -666,7 +663,6 @@ static int tiadc_probe(struct platform_device *pdev)
>  err_buffer_unregister:
>  	tiadc_iio_buffered_hardware_remove(indio_dev);
>  err_free_channels:
> -	tiadc_channels_remove(indio_dev);
>  	return err;
>  }
>  
> @@ -684,7 +680,6 @@ static int tiadc_remove(struct platform_device *pdev)
>  	}
>  	iio_device_unregister(indio_dev);
>  	tiadc_iio_buffered_hardware_remove(indio_dev);
> -	tiadc_channels_remove(indio_dev);
>  
>  	step_en = get_adc_step_mask(adc_dev);
>  	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH 2/3] iio: adc: ti_am335x_adc: alloc kfifo & IRQ via devm_ functions
  2020-04-28 11:14 ` [PATCH 2/3] iio: adc: ti_am335x_adc: alloc kfifo & IRQ via devm_ functions Alexandru Ardelean
@ 2020-07-05 12:04   ` Jonathan Cameron
  0 siblings, 0 replies; 6+ messages in thread
From: Jonathan Cameron @ 2020-07-05 12:04 UTC (permalink / raw)
  To: Alexandru Ardelean
  Cc: linux-iio, linux-kernel, rachna, mugunthanvnm, vigneshr, afd

On Tue, 28 Apr 2020 14:14:29 +0300
Alexandru Ardelean <alexandru.ardelean@analog.com> wrote:

> This change attaches the life-cycle of the kfifo buffer & IRQ to the
> parent-device. This in turn cleans up the exit & error paths, since we
> don't need to explicitly cleanup these resources.
> 
> The main intent here is to remove the explicit cleanup of the
> 'indio_dev->buffer' via 'iio_kfifo_free(indio_dev->buffer);'.
> 
> As we want to add support for multiple buffers per IIO device, having it
> exposed like this makes it tricky to consider a safe backwards compatible
> approach for it.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
Applied.

> ---
>  drivers/iio/adc/ti_am335x_adc.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> index d932fe383a24..03b2ab649cc3 100644
> --- a/drivers/iio/adc/ti_am335x_adc.c
> +++ b/drivers/iio/adc/ti_am335x_adc.c
> @@ -377,7 +377,8 @@ static const struct iio_buffer_setup_ops tiadc_buffer_setup_ops = {
>  	.postdisable = &tiadc_buffer_postdisable,
>  };
>  
> -static int tiadc_iio_buffered_hardware_setup(struct iio_dev *indio_dev,
> +static int tiadc_iio_buffered_hardware_setup(struct device *dev,
> +	struct iio_dev *indio_dev,
>  	irqreturn_t (*pollfunc_bh)(int irq, void *p),
>  	irqreturn_t (*pollfunc_th)(int irq, void *p),
>  	int irq,
> @@ -387,13 +388,13 @@ static int tiadc_iio_buffered_hardware_setup(struct iio_dev *indio_dev,
>  	struct iio_buffer *buffer;
>  	int ret;
>  
> -	buffer = iio_kfifo_allocate();
> +	buffer = devm_iio_kfifo_allocate(dev);
>  	if (!buffer)
>  		return -ENOMEM;
>  
>  	iio_device_attach_buffer(indio_dev, buffer);
>  
> -	ret = request_threaded_irq(irq,	pollfunc_th, pollfunc_bh,
> +	ret = devm_request_threaded_irq(dev, irq, pollfunc_th, pollfunc_bh,
>  				flags, indio_dev->name, indio_dev);
>  	if (ret)
>  		goto error_kfifo_free;
> @@ -408,15 +409,6 @@ static int tiadc_iio_buffered_hardware_setup(struct iio_dev *indio_dev,
>  	return ret;
>  }
>  
> -static void tiadc_iio_buffered_hardware_remove(struct iio_dev *indio_dev)
> -{
> -	struct tiadc_device *adc_dev = iio_priv(indio_dev);
> -
> -	free_irq(adc_dev->mfd_tscadc->irq, indio_dev);
> -	iio_kfifo_free(indio_dev->buffer);
> -}
> -
> -
>  static const char * const chan_name_ain[] = {
>  	"AIN0",
>  	"AIN1",
> @@ -636,7 +628,7 @@ static int tiadc_probe(struct platform_device *pdev)
>  	if (err < 0)
>  		return err;
>  
> -	err = tiadc_iio_buffered_hardware_setup(indio_dev,
> +	err = tiadc_iio_buffered_hardware_setup(&pdev->dev, indio_dev,
>  		&tiadc_worker_h,
>  		&tiadc_irq_h,
>  		adc_dev->mfd_tscadc->irq,
> @@ -661,7 +653,6 @@ static int tiadc_probe(struct platform_device *pdev)
>  err_dma:
>  	iio_device_unregister(indio_dev);
>  err_buffer_unregister:
> -	tiadc_iio_buffered_hardware_remove(indio_dev);
>  err_free_channels:
>  	return err;
>  }
> @@ -679,7 +670,6 @@ static int tiadc_remove(struct platform_device *pdev)
>  		dma_release_channel(dma->chan);
>  	}
>  	iio_device_unregister(indio_dev);
> -	tiadc_iio_buffered_hardware_remove(indio_dev);
>  
>  	step_en = get_adc_step_mask(adc_dev);
>  	am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2020-07-05 12:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-28 11:14 [PATCH 1/3] iio: adc: ti_am335x_adc: alloc channels via devm_kcalloc() Alexandru Ardelean
2020-04-28 11:14 ` [PATCH 2/3] iio: adc: ti_am335x_adc: alloc kfifo & IRQ via devm_ functions Alexandru Ardelean
2020-07-05 12:04   ` Jonathan Cameron
2020-04-28 11:14 ` [PATCH 3/3] iio: adc: ti_am335x_adc: convert rest of probe to " Alexandru Ardelean
2020-05-03 10:38   ` Jonathan Cameron
2020-07-05 12:04 ` [PATCH 1/3] iio: adc: ti_am335x_adc: alloc channels via devm_kcalloc() Jonathan Cameron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).