[PATCH v2 0/3] use managed functions for ad7897 driver

[PATCH v2 0/3] use managed functions for ad7897 driver

[Andi Shyti]

Hi Dmitry,

after our discussion[*], I decided to do it a more properly by
replacing in the touchscreen drivers the initialization functions
with their related managed functions.

I will slowly send patches for other drivers.

The last patch is trivial, but somehow it bothers me, please feel
free to reject it.

Thanks,
Andi

[*] https://marc.info/?l=linux-input&m=150671805312148&w=2

v1 - v2
https://marc.info/?l=linux-kernel&m=150963732620135&w=2

 - squashed all the patches that were switching to the managed
   resource allocation
 - removed the remove() function and used
   devm_add_action_or_reset for cleaning when exiting

Andi Shyti (3):
  Input: ad7897 - use managed allocated resources
  Input: ad7897 - use devm_add_action_or_reset to disable the device
  Input: ad7897 - use separate error handling for different allocators

 drivers/input/touchscreen/ad7877.c | 65 ++++++++++++--------------------------
 1 file changed, 20 insertions(+), 45 deletions(-)

-- 
2.15.0

[PATCH v2 1/3] Input: ad7897 - use managed allocated resources

[Andi Shyti]

Use managed allocated resources to simplify error handling during
probing failure and module exiting.

With this all the goto labels in the probe function together with
the cleanups in the remove function are unnecessary, therefore
removed.

CC: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Andi Shyti <andi@etezian.org>
---
 drivers/input/touchscreen/ad7877.c | 42 +++++++++-----------------------------
 1 file changed, 10 insertions(+), 32 deletions(-)

diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index 0381c7809d1b..c8a143db8681 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -707,12 +707,10 @@ static int ad7877_probe(struct spi_device *spi)
 		return err;
 	}
 
-	ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
-	input_dev = input_allocate_device();
-	if (!ts || !input_dev) {
-		err = -ENOMEM;
-		goto err_free_mem;
-	}
+	ts = devm_kzalloc(&spi->dev, sizeof(struct ad7877), GFP_KERNEL);
+	input_dev = devm_input_allocate_device(&spi->dev);
+	if (!input_dev)
+		return -ENOMEM;
 
 	spi_set_drvdata(spi, ts);
 	ts->spi = spi;
@@ -764,8 +762,7 @@ static int ad7877_probe(struct spi_device *spi)
 	if (verify != AD7877_MM_SEQUENCE){
 		dev_err(&spi->dev, "%s: Failed to probe %s\n",
 			dev_name(&spi->dev), input_dev->name);
-		err = -ENODEV;
-		goto err_free_mem;
+		return -ENODEV;
 	}
 
 	if (gpio3)
@@ -775,45 +772,26 @@ static int ad7877_probe(struct spi_device *spi)
 
 	/* Request AD7877 /DAV GPIO interrupt */
 
-	err = request_threaded_irq(spi->irq, NULL, ad7877_irq,
+	err = devm_request_threaded_irq(&spi->dev, spi->irq, NULL, ad7877_irq,
 				   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 				   spi->dev.driver->name, ts);
 	if (err) {
 		dev_dbg(&spi->dev, "irq %d busy?\n", spi->irq);
-		goto err_free_mem;
+		return err;
 	}
 
-	err = sysfs_create_group(&spi->dev.kobj, &ad7877_attr_group);
-	if (err)
-		goto err_free_irq;
-
-	err = input_register_device(input_dev);
+	err = devm_device_add_group(&spi->dev, &ad7877_attr_group);
 	if (err)
-		goto err_remove_attr_group;
-
-	return 0;
+		return err;
 
-err_remove_attr_group:
-	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
-err_free_irq:
-	free_irq(spi->irq, ts);
-err_free_mem:
-	input_free_device(input_dev);
-	kfree(ts);
-	return err;
+	return input_register_device(input_dev);
 }
 
 static int ad7877_remove(struct spi_device *spi)
 {
 	struct ad7877 *ts = spi_get_drvdata(spi);
 
-	sysfs_remove_group(&spi->dev.kobj, &ad7877_attr_group);
-
 	ad7877_disable(ts);
-	free_irq(ts->spi->irq, ts);
-
-	input_unregister_device(ts->input);
-	kfree(ts);
 
 	dev_dbg(&spi->dev, "unregistered touchscreen\n");
 
-- 
2.15.0

[PATCH v2 2/3] Input: ad7897 - use devm_add_action_or_reset to disable the device

[Andi Shyti]

Use the ad7877_disable() as a custom action when the driver gets
removed instead of calling it from the remove function.

Because ad7877_remove() was just calling the disable function,
get rid of it.

CC: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Andi Shyti <andi@etezian.org>
---
 drivers/input/touchscreen/ad7877.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index c8a143db8681..cf59e569d890 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -417,8 +417,10 @@ static irqreturn_t ad7877_irq(int irq, void *handle)
 	return IRQ_HANDLED;
 }
 
-static void ad7877_disable(struct ad7877 *ts)
+static void ad7877_disable(void *data)
 {
+	struct ad7877 *ts = data;
+
 	mutex_lock(&ts->mutex);
 
 	if (!ts->disabled) {
@@ -712,6 +714,10 @@ static int ad7877_probe(struct spi_device *spi)
 	if (!input_dev)
 		return -ENOMEM;
 
+	err = devm_add_action_or_reset(&spi->dev, ad7877_disable, ts);
+	if (err)
+		return err;
+
 	spi_set_drvdata(spi, ts);
 	ts->spi = spi;
 	ts->input = input_dev;
@@ -787,17 +793,6 @@ static int ad7877_probe(struct spi_device *spi)
 	return input_register_device(input_dev);
 }
 
-static int ad7877_remove(struct spi_device *spi)
-{
-	struct ad7877 *ts = spi_get_drvdata(spi);
-
-	ad7877_disable(ts);
-
-	dev_dbg(&spi->dev, "unregistered touchscreen\n");
-
-	return 0;
-}
-
 static int __maybe_unused ad7877_suspend(struct device *dev)
 {
 	struct ad7877 *ts = dev_get_drvdata(dev);
@@ -824,7 +819,6 @@ static struct spi_driver ad7877_driver = {
 		.pm	= &ad7877_pm,
 	},
 	.probe		= ad7877_probe,
-	.remove		= ad7877_remove,
 };
 
 module_spi_driver(ad7877_driver);
-- 
2.15.0

[PATCH v2 3/3] Input: ad7897 - use separate error handling for different allocators

[Andi Shyti]

Split the error between devm_kzalloc and
devm_input_allocate_device, there is no need to call the second
allocator if the first has failed. Besides this doesn't provide
practical advantages.

CC: Michael Hennerich <michael.hennerich@analog.com>
Signed-off-by: Andi Shyti <andi@etezian.org>
---
 drivers/input/touchscreen/ad7877.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index cf59e569d890..98deffde3fe2 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -710,6 +710,9 @@ static int ad7877_probe(struct spi_device *spi)
 	}
 
 	ts = devm_kzalloc(&spi->dev, sizeof(struct ad7877), GFP_KERNEL);
+	if (!ts)
+		return -ENOMEM;
+
 	input_dev = devm_input_allocate_device(&spi->dev);
 	if (!input_dev)
 		return -ENOMEM;
-- 
2.15.0

Re: [PATCH v2 1/3] Input: ad7897 - use managed allocated resources

[Lars-Peter Clausen]

On 11/08/2017 03:04 PM, Andi Shyti wrote:
> Use managed allocated resources to simplify error handling during
> probing failure and module exiting.
> 
> With this all the goto labels in the probe function together with
> the cleanups in the remove function are unnecessary, therefore
> removed.
> 
> CC: Michael Hennerich <michael.hennerich@analog.com>
> Signed-off-by: Andi Shyti <andi@etezian.org>
> ---
>  drivers/input/touchscreen/ad7877.c | 42 +++++++++-----------------------------
>  1 file changed, 10 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
> index 0381c7809d1b..c8a143db8681 100644
> --- a/drivers/input/touchscreen/ad7877.c
> +++ b/drivers/input/touchscreen/ad7877.c
> @@ -707,12 +707,10 @@ static int ad7877_probe(struct spi_device *spi)
>  		return err;
>  	}
>  
> -	ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
> -	input_dev = input_allocate_device();
> -	if (!ts || !input_dev) {
> -		err = -ENOMEM;
> -		goto err_free_mem;
> -	}
> +	ts = devm_kzalloc(&spi->dev, sizeof(struct ad7877), GFP_KERNEL);
> +	input_dev = devm_input_allocate_device(&spi->dev);
> +	if (!input_dev)

You removed the check for 'ts' here and only added it back in patch 3.

> +		return -ENOMEM;
>  
>  	spi_set_drvdata(spi, ts);
>  	ts->spi = spi;
[...]

Re: [PATCH v2 1/3] Input: ad7897 - use managed allocated resources

[Andi Shyti]

> > -	ts = kzalloc(sizeof(struct ad7877), GFP_KERNEL);
> > -	input_dev = input_allocate_device();
> > -	if (!ts || !input_dev) {
> > -		err = -ENOMEM;
> > -		goto err_free_mem;
> > -	}
> > +	ts = devm_kzalloc(&spi->dev, sizeof(struct ad7877), GFP_KERNEL);
> > +	input_dev = devm_input_allocate_device(&spi->dev);
> > +	if (!input_dev)
> 
> You removed the check for 'ts' here and only added it back in patch 3.

Oh yes, because I squashed the 5 patches I sent earlier and I
forgot about this.

Thanks!

Andi