[PATCH v2 0/5] Input: adp5589: cleanup and use device-managed functions

[PATCH v2 0/5] Input: adp5589: cleanup and use device-managed functions

[Alexandru Ardelean]

This change-set does a cleanup of the driver to use device-managed
functions. The error & exit paths are simplified, and some potential
leaks should be removed.

Changelog v1 -> v2:
* https://lore.kernel.org/linux-input/20201111084833.40995-1-alexandru.ardelean@analog.com/T/#t
* dropped patch: 'Input: adp5589: use a single variable for error in probe'
* for patch 'Input: adp5589: use devm_add_action_or_reset() for register clear'
  moved devm_add_action_or_reset() right after switch statement;
  using 'error' variable to check for error condition

Alexandru Ardelean (5):
  Input: adp5589: use devm_kzalloc() to allocate the kpad object
  Input: adp5589: use device-managed function in adp5589_keypad_add()
  Input: adp5589: remove setup/teardown hooks for gpios
  Input: adp5589: use devm_gpiochip_add_data() for gpios
  Input: adp5589: use devm_add_action_or_reset() for register clear

 drivers/input/keyboard/adp5589-keys.c | 120 ++++++--------------------
 include/linux/input/adp5589.h         |   7 --
 2 files changed, 27 insertions(+), 100 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] Input: adp5589: use devm_kzalloc() to allocate the kpad object

[Alexandru Ardelean]

This removes the need to manually free the kpad object and cleans up some
exit/error paths.
The error path cleanup should reduce the risk of any memory leaks with this
object.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/input/keyboard/adp5589-keys.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
index eb0e9cd66bcb..922497b65ab0 100644
--- a/drivers/input/keyboard/adp5589-keys.c
+++ b/drivers/input/keyboard/adp5589-keys.c
@@ -1007,7 +1007,7 @@ static int adp5589_probe(struct i2c_client *client,
 		return -EINVAL;
 	}
 
-	kpad = kzalloc(sizeof(*kpad), GFP_KERNEL);
+	kpad = devm_kzalloc(&client->dev, sizeof(*kpad), GFP_KERNEL);
 	if (!kpad)
 		return -ENOMEM;
 
@@ -1028,17 +1028,15 @@ static int adp5589_probe(struct i2c_client *client,
 	}
 
 	ret = adp5589_read(client, ADP5589_5_ID);
-	if (ret < 0) {
-		error = ret;
-		goto err_free_mem;
-	}
+	if (ret < 0)
+		return ret;
 
 	revid = (u8) ret & ADP5589_5_DEVICE_ID_MASK;
 
 	if (pdata->keymapsize) {
 		error = adp5589_keypad_add(kpad, revid);
 		if (error)
-			goto err_free_mem;
+			return error;
 	}
 
 	error = adp5589_setup(kpad);
@@ -1059,8 +1057,6 @@ static int adp5589_probe(struct i2c_client *client,
 
 err_keypad_remove:
 	adp5589_keypad_remove(kpad);
-err_free_mem:
-	kfree(kpad);
 
 	return error;
 }
@@ -1072,7 +1068,6 @@ static int adp5589_remove(struct i2c_client *client)
 	adp5589_write(client, kpad->var->reg(ADP5589_GENERAL_CFG), 0);
 	adp5589_keypad_remove(kpad);
 	adp5589_gpio_remove(kpad);
-	kfree(kpad);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v2 2/5] Input: adp5589: use device-managed function in adp5589_keypad_add()

[Alexandru Ardelean]

This change makes use of the devm_input_allocate_device() function, which
gets rid of the input_free_device() and input_unregister_device() calls.

When a device is allocated via input_allocate_device(), the
input_register_device() call will also be device-managed, so there is no
longer need to manually call unregister.

Also, the irq is allocated with the devm_request_threaded_irq(), so with
these two changes, the adp5589_keypad_remove() function is no longer
needed.

This cleans up the error & exit paths.
It also looks like the input_free_device() should have been called on the
remove() hook, but doesn't look like it is.

This change may also also fix some potential leaks that were probably
omitted earlier.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/input/keyboard/adp5589-keys.c | 47 +++++++--------------------
 1 file changed, 11 insertions(+), 36 deletions(-)

diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
index 922497b65ab0..e96ffd5ed69e 100644
--- a/drivers/input/keyboard/adp5589-keys.c
+++ b/drivers/input/keyboard/adp5589-keys.c
@@ -909,7 +909,7 @@ static int adp5589_keypad_add(struct adp5589_kpad *kpad, unsigned int revid)
 		return -EINVAL;
 	}
 
-	input = input_allocate_device();
+	input = devm_input_allocate_device(&client->dev);
 	if (!input)
 		return -ENOMEM;
 
@@ -953,38 +953,19 @@ static int adp5589_keypad_add(struct adp5589_kpad *kpad, unsigned int revid)
 		__set_bit(kpad->gpimap[i].sw_evt, input->swbit);
 
 	error = input_register_device(input);
-	if (error) {
-		dev_err(&client->dev, "unable to register input device\n");
-		goto err_free_input;
-	}
+	if (error)
+		return error;
 
-	error = request_threaded_irq(client->irq, NULL, adp5589_irq,
-				     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
-				     client->dev.driver->name, kpad);
-	if (error) {
-		dev_err(&client->dev, "irq %d busy?\n", client->irq);
-		goto err_unreg_dev;
-	}
+	error = devm_request_threaded_irq(&client->dev, client->irq, NULL,
+					  adp5589_irq,
+					  IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					  client->dev.driver->name, kpad);
+	if (error)
+		return error;
 
 	device_init_wakeup(&client->dev, 1);
 
 	return 0;
-
-err_unreg_dev:
-	input_unregister_device(input);
-	input = NULL;
-err_free_input:
-	input_free_device(input);
-
-	return error;
-}
-
-static void adp5589_keypad_remove(struct adp5589_kpad *kpad)
-{
-	if (kpad->input) {
-		free_irq(kpad->client->irq, kpad);
-		input_unregister_device(kpad->input);
-	}
 }
 
 static int adp5589_probe(struct i2c_client *client,
@@ -1041,24 +1022,19 @@ static int adp5589_probe(struct i2c_client *client,
 
 	error = adp5589_setup(kpad);
 	if (error)
-		goto err_keypad_remove;
+		return error;
 
 	if (kpad->gpimapsize)
 		adp5589_report_switch_state(kpad);
 
 	error = adp5589_gpio_add(kpad);
 	if (error)
-		goto err_keypad_remove;
+		return error;
 
 	i2c_set_clientdata(client, kpad);
 
 	dev_info(&client->dev, "Rev.%d keypad, irq %d\n", revid, client->irq);
 	return 0;
-
-err_keypad_remove:
-	adp5589_keypad_remove(kpad);
-
-	return error;
 }
 
 static int adp5589_remove(struct i2c_client *client)
@@ -1066,7 +1042,6 @@ static int adp5589_remove(struct i2c_client *client)
 	struct adp5589_kpad *kpad = i2c_get_clientdata(client);
 
 	adp5589_write(client, kpad->var->reg(ADP5589_GENERAL_CFG), 0);
-	adp5589_keypad_remove(kpad);
 	adp5589_gpio_remove(kpad);
 
 	return 0;
-- 
2.17.1

[PATCH v2 3/5] Input: adp5589: remove setup/teardown hooks for gpios

[Alexandru Ardelean]

This is currently just dead code. It's from around a time when
platform-data was used, and a board could hook it's own special callback
for setup/teardown, and a private object (via 'context').

This change removes it, as there are no more users in mainline for this.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/input/keyboard/adp5589-keys.c | 21 ---------------------
 include/linux/input/adp5589.h         |  7 -------
 2 files changed, 28 deletions(-)

diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
index e96ffd5ed69e..cc91a47931bd 100644
--- a/drivers/input/keyboard/adp5589-keys.c
+++ b/drivers/input/keyboard/adp5589-keys.c
@@ -539,35 +539,14 @@ static int adp5589_gpio_add(struct adp5589_kpad *kpad)
 					    ADP5589_GPIO_DIRECTION_A) + i);
 	}
 
-	if (gpio_data->setup) {
-		error = gpio_data->setup(kpad->client,
-					 kpad->gc.base, kpad->gc.ngpio,
-					 gpio_data->context);
-		if (error)
-			dev_warn(dev, "setup failed, %d\n", error);
-	}
-
 	return 0;
 }
 
 static void adp5589_gpio_remove(struct adp5589_kpad *kpad)
 {
-	struct device *dev = &kpad->client->dev;
-	const struct adp5589_kpad_platform_data *pdata = dev_get_platdata(dev);
-	const struct adp5589_gpio_platform_data *gpio_data = pdata->gpio_data;
-	int error;
-
 	if (!kpad->export_gpio)
 		return;
 
-	if (gpio_data->teardown) {
-		error = gpio_data->teardown(kpad->client,
-					    kpad->gc.base, kpad->gc.ngpio,
-					    gpio_data->context);
-		if (error)
-			dev_warn(dev, "teardown failed %d\n", error);
-	}
-
 	gpiochip_remove(&kpad->gc);
 }
 #else
diff --git a/include/linux/input/adp5589.h b/include/linux/input/adp5589.h
index c0523af96893..0e4742c8c81e 100644
--- a/include/linux/input/adp5589.h
+++ b/include/linux/input/adp5589.h
@@ -175,13 +175,6 @@ struct i2c_client; /* forward declaration */
 
 struct adp5589_gpio_platform_data {
 	int	gpio_start;	/* GPIO Chip base # */
-	int	(*setup)(struct i2c_client *client,
-				int gpio, unsigned ngpio,
-				void *context);
-	int	(*teardown)(struct i2c_client *client,
-				int gpio, unsigned ngpio,
-				void *context);
-	void	*context;
 };
 
 #endif
-- 
2.17.1

[PATCH v2 4/5] Input: adp5589: use devm_gpiochip_add_data() for gpios

[Alexandru Ardelean]

This change makes use of the devm_gpiochip_add_data() function. With this
the gpiochip_remove() function can be removed, and the
adp5589_gpio_remove() function as well.

The kpad->export_gpio variable is also redundant now, and has been removed.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/input/keyboard/adp5589-keys.c | 22 ++--------------------
 1 file changed, 2 insertions(+), 20 deletions(-)

diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
index cc91a47931bd..ab390b01db08 100644
--- a/drivers/input/keyboard/adp5589-keys.c
+++ b/drivers/input/keyboard/adp5589-keys.c
@@ -238,7 +238,6 @@ struct adp5589_kpad {
 	bool support_row5;
 #ifdef CONFIG_GPIOLIB
 	unsigned char gpiomap[ADP5589_MAXGPIO];
-	bool export_gpio;
 	struct gpio_chip gc;
 	struct mutex gpio_lock;	/* Protect cached dir, dat_out */
 	u8 dat_out[3];
@@ -512,8 +511,6 @@ static int adp5589_gpio_add(struct adp5589_kpad *kpad)
 		return 0;
 	}
 
-	kpad->export_gpio = true;
-
 	kpad->gc.direction_input = adp5589_gpio_direction_input;
 	kpad->gc.direction_output = adp5589_gpio_direction_output;
 	kpad->gc.get = adp5589_gpio_get_value;
@@ -526,11 +523,9 @@ static int adp5589_gpio_add(struct adp5589_kpad *kpad)
 
 	mutex_init(&kpad->gpio_lock);
 
-	error = gpiochip_add_data(&kpad->gc, kpad);
-	if (error) {
-		dev_err(dev, "gpiochip_add_data() failed, err: %d\n", error);
+	error = devm_gpiochip_add_data(dev, &kpad->gc, kpad);
+	if (error)
 		return error;
-	}
 
 	for (i = 0; i <= kpad->var->bank(kpad->var->maxgpio); i++) {
 		kpad->dat_out[i] = adp5589_read(kpad->client, kpad->var->reg(
@@ -541,23 +536,11 @@ static int adp5589_gpio_add(struct adp5589_kpad *kpad)
 
 	return 0;
 }
-
-static void adp5589_gpio_remove(struct adp5589_kpad *kpad)
-{
-	if (!kpad->export_gpio)
-		return;
-
-	gpiochip_remove(&kpad->gc);
-}
 #else
 static inline int adp5589_gpio_add(struct adp5589_kpad *kpad)
 {
 	return 0;
 }
-
-static inline void adp5589_gpio_remove(struct adp5589_kpad *kpad)
-{
-}
 #endif
 
 static void adp5589_report_switches(struct adp5589_kpad *kpad,
@@ -1021,7 +1004,6 @@ static int adp5589_remove(struct i2c_client *client)
 	struct adp5589_kpad *kpad = i2c_get_clientdata(client);
 
 	adp5589_write(client, kpad->var->reg(ADP5589_GENERAL_CFG), 0);
-	adp5589_gpio_remove(kpad);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v2 5/5] Input: adp5589: use devm_add_action_or_reset() for register clear

[Alexandru Ardelean]

The driver clears the general configuration register during the remove()
hook. This should also be done in case the driver exits on error.

This change move the clear of that register to the
devm_add_action_or_reset() hook.

Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
---
 drivers/input/keyboard/adp5589-keys.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
index ab390b01db08..9d5a39187b77 100644
--- a/drivers/input/keyboard/adp5589-keys.c
+++ b/drivers/input/keyboard/adp5589-keys.c
@@ -930,6 +930,14 @@ static int adp5589_keypad_add(struct adp5589_kpad *kpad, unsigned int revid)
 	return 0;
 }
 
+static void adp5589_clear_config(void *data)
+{
+	struct i2c_client *client = data;
+	struct adp5589_kpad *kpad = i2c_get_clientdata(client);
+
+	adp5589_write(client, kpad->var->reg(ADP5589_GENERAL_CFG), 0);
+}
+
 static int adp5589_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -970,6 +978,11 @@ static int adp5589_probe(struct i2c_client *client,
 		break;
 	}
 
+	error = devm_add_action_or_reset(&client->dev, adp5589_clear_config,
+					 client);
+	if (error)
+		return error;
+
 	ret = adp5589_read(client, ADP5589_5_ID);
 	if (ret < 0)
 		return ret;
@@ -999,15 +1012,6 @@ static int adp5589_probe(struct i2c_client *client,
 	return 0;
 }
 
-static int adp5589_remove(struct i2c_client *client)
-{
-	struct adp5589_kpad *kpad = i2c_get_clientdata(client);
-
-	adp5589_write(client, kpad->var->reg(ADP5589_GENERAL_CFG), 0);
-
-	return 0;
-}
-
 #ifdef CONFIG_PM_SLEEP
 static int adp5589_suspend(struct device *dev)
 {
@@ -1059,7 +1063,6 @@ static struct i2c_driver adp5589_driver = {
 		.pm = &adp5589_dev_pm_ops,
 	},
 	.probe = adp5589_probe,
-	.remove = adp5589_remove,
 	.id_table = adp5589_id,
 };
 
-- 
2.17.1

Re: [PATCH v2 1/5] Input: adp5589: use devm_kzalloc() to allocate the kpad object

[Dmitry Torokhov]

On Thu, Nov 12, 2020 at 09:43:04AM +0200, Alexandru Ardelean wrote:
> This removes the need to manually free the kpad object and cleans up some
> exit/error paths.
> The error path cleanup should reduce the risk of any memory leaks with this
> object.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Applied, thank you.

-- 
Dmitry

Re: [PATCH v2 2/5] Input: adp5589: use device-managed function in adp5589_keypad_add()

[Dmitry Torokhov]

Hi Alexandru,

On Thu, Nov 12, 2020 at 09:43:05AM +0200, Alexandru Ardelean wrote:
> This change makes use of the devm_input_allocate_device() function, which
> gets rid of the input_free_device() and input_unregister_device() calls.
> 
> When a device is allocated via input_allocate_device(), the
> input_register_device() call will also be device-managed, so there is no
> longer need to manually call unregister.
> 
> Also, the irq is allocated with the devm_request_threaded_irq(), so with
> these two changes, the adp5589_keypad_remove() function is no longer
> needed.
> 
> This cleans up the error & exit paths.
> It also looks like the input_free_device() should have been called on the
> remove() hook, but doesn't look like it is.

Actually it should not be called once input_unregister_device() is
called. If you check, you will see that in probe() we set the pointer to
input device to NULL after calling unregister, which makes subsequent
call to input_free_device() a noop. We did that so that we have single
error handling flow.

I dropped this paragraph.

> 
> This change may also also fix some potential leaks that were probably
> omitted earlier.

This one too.

> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
>  drivers/input/keyboard/adp5589-keys.c | 47 +++++++--------------------
>  1 file changed, 11 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
> index 922497b65ab0..e96ffd5ed69e 100644
> --- a/drivers/input/keyboard/adp5589-keys.c
> +++ b/drivers/input/keyboard/adp5589-keys.c
> @@ -909,7 +909,7 @@ static int adp5589_keypad_add(struct adp5589_kpad *kpad, unsigned int revid)
>  		return -EINVAL;
>  	}
>  
> -	input = input_allocate_device();
> +	input = devm_input_allocate_device(&client->dev);
>  	if (!input)
>  		return -ENOMEM;
>  
> @@ -953,38 +953,19 @@ static int adp5589_keypad_add(struct adp5589_kpad *kpad, unsigned int revid)
>  		__set_bit(kpad->gpimap[i].sw_evt, input->swbit);
>  
>  	error = input_register_device(input);
> -	if (error) {
> -		dev_err(&client->dev, "unable to register input device\n");

If you want to drop error messages, please send a separate patch with
justification.

I restored error messgaes, and applied.

Thank you.

-- 
Dmitry

Re: [PATCH v2 3/5] Input: adp5589: remove setup/teardown hooks for gpios

[Dmitry Torokhov]

On Thu, Nov 12, 2020 at 09:43:06AM +0200, Alexandru Ardelean wrote:
> This is currently just dead code. It's from around a time when
> platform-data was used, and a board could hook it's own special callback
> for setup/teardown, and a private object (via 'context').
> 
> This change removes it, as there are no more users in mainline for this.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Applied, thank you.

-- 
Dmitry

Re: [PATCH v2 4/5] Input: adp5589: use devm_gpiochip_add_data() for gpios

[Dmitry Torokhov]

On Thu, Nov 12, 2020 at 09:43:07AM +0200, Alexandru Ardelean wrote:
> This change makes use of the devm_gpiochip_add_data() function. With this
> the gpiochip_remove() function can be removed, and the
> adp5589_gpio_remove() function as well.
> 
> The kpad->export_gpio variable is also redundant now, and has been removed.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Applied, thank you.

-- 
Dmitry

Re: [PATCH v2 5/5] Input: adp5589: use devm_add_action_or_reset() for register clear

[Dmitry Torokhov]

On Thu, Nov 12, 2020 at 09:43:08AM +0200, Alexandru Ardelean wrote:
> The driver clears the general configuration register during the remove()
> hook. This should also be done in case the driver exits on error.
> 
> This change move the clear of that register to the
> devm_add_action_or_reset() hook.
> 
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>

Applied, thank you.

-- 
Dmitry