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

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

Alexandru Ardelean (6):
  Input: adp5589: use a single variable for error in probe
  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 | 134 +++++++-------------------
 include/linux/input/adp5589.h         |   7 --
 2 files changed, 34 insertions(+), 107 deletions(-)

-- 
2.17.1

[PATCH 1/6] Input: adp5589: use a single variable for error in probe

[Alexandru Ardelean]

The 'error' & 'ret' variables are used. This is a bit of duplication.
This change replaces the use of error with the 'ret' variable since the
name is a bit more generic.

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

diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
index eb0e9cd66bcb..4cfb26e3d79c 100644
--- a/drivers/input/keyboard/adp5589-keys.c
+++ b/drivers/input/keyboard/adp5589-keys.c
@@ -994,7 +994,7 @@ static int adp5589_probe(struct i2c_client *client,
 	const struct adp5589_kpad_platform_data *pdata =
 		dev_get_platdata(&client->dev);
 	unsigned int revid;
-	int error, ret;
+	int ret;
 
 	if (!i2c_check_functionality(client->adapter,
 				     I2C_FUNC_SMBUS_BYTE_DATA)) {
@@ -1028,28 +1028,26 @@ static int adp5589_probe(struct i2c_client *client,
 	}
 
 	ret = adp5589_read(client, ADP5589_5_ID);
-	if (ret < 0) {
-		error = ret;
+	if (ret < 0)
 		goto err_free_mem;
-	}
 
 	revid = (u8) ret & ADP5589_5_DEVICE_ID_MASK;
 
 	if (pdata->keymapsize) {
-		error = adp5589_keypad_add(kpad, revid);
-		if (error)
+		ret = adp5589_keypad_add(kpad, revid);
+		if (ret)
 			goto err_free_mem;
 	}
 
-	error = adp5589_setup(kpad);
-	if (error)
+	ret = adp5589_setup(kpad);
+	if (ret)
 		goto err_keypad_remove;
 
 	if (kpad->gpimapsize)
 		adp5589_report_switch_state(kpad);
 
-	error = adp5589_gpio_add(kpad);
-	if (error)
+	ret = adp5589_gpio_add(kpad);
+	if (ret)
 		goto err_keypad_remove;
 
 	i2c_set_clientdata(client, kpad);
@@ -1062,7 +1060,7 @@ static int adp5589_probe(struct i2c_client *client,
 err_free_mem:
 	kfree(kpad);
 
-	return error;
+	return ret;
 }
 
 static int adp5589_remove(struct i2c_client *client)
-- 
2.17.1

Re: [PATCH 1/6] Input: adp5589: use a single variable for error in probe

[Dmitry Torokhov]

Hi Alexandru,

On Wed, Nov 11, 2020 at 10:48:28AM +0200, Alexandru Ardelean wrote:
> The 'error' & 'ret' variables are used. This is a bit of duplication.
> This change replaces the use of error with the 'ret' variable since the
> name is a bit more generic.

I really prefer variables that carry error codes/success and are used in
error paths to be called "error", and "ret" or "retval" to be used in
cases where we may return actual data.

Thanks.

-- 
Dmitry

RE: [PATCH 1/6] Input: adp5589: use a single variable for error in probe

[Ardelean, Alexandru]

> -----Original Message-----
> From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Sent: Thursday, November 12, 2020 2:38 AM
> To: Ardelean, Alexandru <alexandru.Ardelean@analog.com>
> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/6] Input: adp5589: use a single variable for error in probe
> 
> [External]
> 
> Hi Alexandru,
> 
> On Wed, Nov 11, 2020 at 10:48:28AM +0200, Alexandru Ardelean wrote:
> > The 'error' & 'ret' variables are used. This is a bit of duplication.
> > This change replaces the use of error with the 'ret' variable since
> > the name is a bit more generic.
> 
> I really prefer variables that carry error codes/success and are used in error
> paths to be called "error", and "ret" or "retval" to be used in cases where we
> may return actual data.
> 

Ack.
Will do it the other way around for v2.

> Thanks.
> 
> --
> Dmitry

RE: [PATCH 1/6] Input: adp5589: use a single variable for error in probe

[Ardelean, Alexandru]

> -----Original Message-----
> From: Ardelean, Alexandru
> Sent: Thursday, November 12, 2020 8:40 AM
> To: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH 1/6] Input: adp5589: use a single variable for error in probe
> 
> 
> > -----Original Message-----
> > From: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Sent: Thursday, November 12, 2020 2:38 AM
> > To: Ardelean, Alexandru <alexandru.Ardelean@analog.com>
> > Cc: linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
> > Subject: Re: [PATCH 1/6] Input: adp5589: use a single variable for
> > error in probe
> >
> > [External]
> >
> > Hi Alexandru,
> >
> > On Wed, Nov 11, 2020 at 10:48:28AM +0200, Alexandru Ardelean wrote:
> > > The 'error' & 'ret' variables are used. This is a bit of duplication.
> > > This change replaces the use of error with the 'ret' variable since
> > > the name is a bit more generic.
> >
> > I really prefer variables that carry error codes/success and are used
> > in error paths to be called "error", and "ret" or "retval" to be used
> > in cases where we may return actual data.
> >
> 
> Ack.
> Will do it the other way around for v2.
> 

Wait.
I just had some coffee.
I think you're saying to drop this patch.
Also fine from my side.

> > Thanks.
> >
> > --
> > Dmitry

[PATCH 2/6] 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 | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/input/keyboard/adp5589-keys.c b/drivers/input/keyboard/adp5589-keys.c
index 4cfb26e3d79c..9f41118b4321 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;
 
@@ -1029,14 +1029,14 @@ static int adp5589_probe(struct i2c_client *client,
 
 	ret = adp5589_read(client, ADP5589_5_ID);
 	if (ret < 0)
-		goto err_free_mem;
+		return ret;
 
 	revid = (u8) ret & ADP5589_5_DEVICE_ID_MASK;
 
 	if (pdata->keymapsize) {
 		ret = adp5589_keypad_add(kpad, revid);
 		if (ret)
-			goto err_free_mem;
+			return ret;
 	}
 
 	ret = adp5589_setup(kpad);
@@ -1057,8 +1057,6 @@ static int adp5589_probe(struct i2c_client *client,
 
 err_keypad_remove:
 	adp5589_keypad_remove(kpad);
-err_free_mem:
-	kfree(kpad);
 
 	return ret;
 }
@@ -1070,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 3/6] 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 9f41118b4321..a774f0848a34 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,
 
 	ret = adp5589_setup(kpad);
 	if (ret)
-		goto err_keypad_remove;
+		return ret;
 
 	if (kpad->gpimapsize)
 		adp5589_report_switch_state(kpad);
 
 	ret = adp5589_gpio_add(kpad);
 	if (ret)
-		goto err_keypad_remove;
+		return ret;
 
 	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 ret;
 }
 
 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 4/6] 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 a774f0848a34..2bf57f08af24 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 5/6] 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 2bf57f08af24..0bd7170041e0 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 6/6] 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 0bd7170041e0..7d1ab0aa6b14 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)
 {
@@ -976,6 +984,11 @@ static int adp5589_probe(struct i2c_client *client,
 
 	revid = (u8) ret & ADP5589_5_DEVICE_ID_MASK;
 
+	ret = devm_add_action_or_reset(&client->dev, adp5589_clear_config,
+				       client);
+	if (ret < 0)
+		return ret;
+
 	if (pdata->keymapsize) {
 		ret = adp5589_keypad_add(kpad, revid);
 		if (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