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

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

[Andi Shyti]

Hi Dmitry,

after our discussion[1], 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

v2 - v3
https://marc.info/?l=linux-kernel&m=151015103707211&w=2
 - fixed a mistake I did while squashing the patches in v2
   (thanks Lars[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

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

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 v3 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..dd59e4a5eb7d 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 (!ts || !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 v3 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 dd59e4a5eb7d..04ab8fbc8184 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 (!ts || !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 v3 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 | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/ad7877.c b/drivers/input/touchscreen/ad7877.c
index 04ab8fbc8184..98deffde3fe2 100644
--- a/drivers/input/touchscreen/ad7877.c
+++ b/drivers/input/touchscreen/ad7877.c
@@ -710,8 +710,11 @@ 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 (!ts || !input_dev)
+	if (!input_dev)
 		return -ENOMEM;
 
 	err = devm_add_action_or_reset(&spi->dev, ad7877_disable, ts);
-- 
2.15.0

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

[Dmitry Torokhov]

Hi Andi,

On Wed, Nov 08, 2017 at 05:50:19PM +0200, Andi Shyti wrote:
> 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.
> 

There is not reason why this change needs to be split from the first
one; I folded it back in and applied, thank you.

> 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 dd59e4a5eb7d..04ab8fbc8184 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 (!ts || !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
> 

-- 
Dmitry