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 
> ---
>  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(>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(>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

[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 
---
 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(>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(>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(>dev, "irq %d busy?\n", client->irq);
-   goto err_unreg_dev;
-   }
+   error = devm_request_threaded_irq(>dev, client->irq, NULL,
+ adp5589_irq,
+ IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+ client->dev.driver->name, kpad);
+   if (error)
+   return error;
 
device_init_wakeup(>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(>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