* [PATCH v2 1/5] Input: adp5589: use devm_kzalloc() to allocate the kpad object
2020-11-12 7:43 [PATCH v2 0/5] Input: adp5589: cleanup and use device-managed functions Alexandru Ardelean
@ 2020-11-12 7:43 ` Alexandru Ardelean
2020-11-19 7:06 ` Dmitry Torokhov
2020-11-12 7:43 ` [PATCH v2 2/5] Input: adp5589: use device-managed function in adp5589_keypad_add() Alexandru Ardelean
` (3 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Alexandru Ardelean @ 2020-11-12 7:43 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: dmitry.torokhov, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH v2 2/5] Input: adp5589: use device-managed function in adp5589_keypad_add()
2020-11-12 7:43 [PATCH v2 0/5] Input: adp5589: cleanup and use device-managed functions Alexandru Ardelean
2020-11-12 7:43 ` [PATCH v2 1/5] Input: adp5589: use devm_kzalloc() to allocate the kpad object Alexandru Ardelean
@ 2020-11-12 7:43 ` Alexandru Ardelean
2020-11-19 7:09 ` Dmitry Torokhov
2020-11-12 7:43 ` [PATCH v2 3/5] Input: adp5589: remove setup/teardown hooks for gpios Alexandru Ardelean
` (2 subsequent siblings)
4 siblings, 1 reply; 11+ messages in thread
From: Alexandru Ardelean @ 2020-11-12 7:43 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: dmitry.torokhov, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 2/5] Input: adp5589: use device-managed function in adp5589_keypad_add()
2020-11-12 7:43 ` [PATCH v2 2/5] Input: adp5589: use device-managed function in adp5589_keypad_add() Alexandru Ardelean
@ 2020-11-19 7:09 ` Dmitry Torokhov
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2020-11-19 7:09 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 3/5] Input: adp5589: remove setup/teardown hooks for gpios
2020-11-12 7:43 [PATCH v2 0/5] Input: adp5589: cleanup and use device-managed functions Alexandru Ardelean
2020-11-12 7:43 ` [PATCH v2 1/5] Input: adp5589: use devm_kzalloc() to allocate the kpad object Alexandru Ardelean
2020-11-12 7:43 ` [PATCH v2 2/5] Input: adp5589: use device-managed function in adp5589_keypad_add() Alexandru Ardelean
@ 2020-11-12 7:43 ` Alexandru Ardelean
2020-11-19 7:10 ` Dmitry Torokhov
2020-11-12 7:43 ` [PATCH v2 4/5] Input: adp5589: use devm_gpiochip_add_data() " Alexandru Ardelean
2020-11-12 7:43 ` [PATCH v2 5/5] Input: adp5589: use devm_add_action_or_reset() for register clear Alexandru Ardelean
4 siblings, 1 reply; 11+ messages in thread
From: Alexandru Ardelean @ 2020-11-12 7:43 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: dmitry.torokhov, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 3/5] Input: adp5589: remove setup/teardown hooks for gpios
2020-11-12 7:43 ` [PATCH v2 3/5] Input: adp5589: remove setup/teardown hooks for gpios Alexandru Ardelean
@ 2020-11-19 7:10 ` Dmitry Torokhov
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2020-11-19 7:10 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 4/5] Input: adp5589: use devm_gpiochip_add_data() for gpios
2020-11-12 7:43 [PATCH v2 0/5] Input: adp5589: cleanup and use device-managed functions Alexandru Ardelean
` (2 preceding siblings ...)
2020-11-12 7:43 ` [PATCH v2 3/5] Input: adp5589: remove setup/teardown hooks for gpios Alexandru Ardelean
@ 2020-11-12 7:43 ` Alexandru Ardelean
2020-11-19 7:10 ` Dmitry Torokhov
2020-11-12 7:43 ` [PATCH v2 5/5] Input: adp5589: use devm_add_action_or_reset() for register clear Alexandru Ardelean
4 siblings, 1 reply; 11+ messages in thread
From: Alexandru Ardelean @ 2020-11-12 7:43 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: dmitry.torokhov, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH v2 4/5] Input: adp5589: use devm_gpiochip_add_data() for gpios
2020-11-12 7:43 ` [PATCH v2 4/5] Input: adp5589: use devm_gpiochip_add_data() " Alexandru Ardelean
@ 2020-11-19 7:10 ` Dmitry Torokhov
0 siblings, 0 replies; 11+ messages in thread
From: Dmitry Torokhov @ 2020-11-19 7:10 UTC (permalink / raw)
To: Alexandru Ardelean; +Cc: linux-input, linux-kernel
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 5/5] Input: adp5589: use devm_add_action_or_reset() for register clear
2020-11-12 7:43 [PATCH v2 0/5] Input: adp5589: cleanup and use device-managed functions Alexandru Ardelean
` (3 preceding siblings ...)
2020-11-12 7:43 ` [PATCH v2 4/5] Input: adp5589: use devm_gpiochip_add_data() " Alexandru Ardelean
@ 2020-11-12 7:43 ` Alexandru Ardelean
2020-11-19 7:10 ` Dmitry Torokhov
4 siblings, 1 reply; 11+ messages in thread
From: Alexandru Ardelean @ 2020-11-12 7:43 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: dmitry.torokhov, 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
^ permalink raw reply related [flat|nested] 11+ messages in thread