[PATCH 0/2] Input: spear-keyboard - Fix an issue and clean-up code

[PATCH 0/2] Input: spear-keyboard - Fix an issue and clean-up code

[Christophe JAILLET]

Patch 1 fixes an old double put issue in the remove function.

Patch 2 is just a clean up to remove some lines of code. It changes the
order of some initialization so should be reviewed with care.
This driver looks old, without changes other than clean-up for more than
10 years, so maybe this patch should just be dropped.
I let you decide if safe and useful or not.


Both patch are compile tested only.

Christophe JAILLET (2):
  Input: spear-keyboard - Fix a double put in spear_kbd_remove()
  Input: spear-keyboard - Switch to devm_clk_get_prepared()

 drivers/input/keyboard/spear-keyboard.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

-- 
2.45.2

[PATCH 1/2] Input: spear-keyboard - Fix a double put in spear_kbd_remove()

[Christophe JAILLET]

The 'input_dev' is a managed resource allocated with
devm_input_allocate_device(), so there is no need to call
input_unregister_device() in the remove function.

In fact, this call was correctly removed in commit 6102752eb354 ("Input:
spear-keyboard - switch to using managed resources"), but silently
re-introduced later in the commit in Fixes.

Fixes: 9336648978c2 ("Input: spear-keyboard - add clk_{un}prepare() support")
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested-only
---
 drivers/input/keyboard/spear-keyboard.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 557d00a667ce..5d9fc8dc9433 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c
@@ -276,7 +276,6 @@ static void spear_kbd_remove(struct platform_device *pdev)
 {
 	struct spear_kbd *kbd = platform_get_drvdata(pdev);
 
-	input_unregister_device(kbd->input);
 	clk_unprepare(kbd->clk);
 }
 
-- 
2.45.2

[PATCH 2/2] Input: spear-keyboard - Switch to devm_clk_get_prepared()

[Christophe JAILLET]

Use devm_clk_get_prepared() in order to remove a clk_unprepare() in an
error handling path of the probe and completely remove the .remove()
function.

Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
Compile tested only.
---
 drivers/input/keyboard/spear-keyboard.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
index 5d9fc8dc9433..1df4feb8ba01 100644
--- a/drivers/input/keyboard/spear-keyboard.c
+++ b/drivers/input/keyboard/spear-keyboard.c
@@ -222,7 +222,7 @@ static int spear_kbd_probe(struct platform_device *pdev)
 	if (IS_ERR(kbd->io_base))
 		return PTR_ERR(kbd->io_base);
 
-	kbd->clk = devm_clk_get(&pdev->dev, NULL);
+	kbd->clk = devm_clk_get_prepared(&pdev->dev, NULL);
 	if (IS_ERR(kbd->clk))
 		return PTR_ERR(kbd->clk);
 
@@ -255,14 +255,9 @@ static int spear_kbd_probe(struct platform_device *pdev)
 		return error;
 	}
 
-	error = clk_prepare(kbd->clk);
-	if (error)
-		return error;
-
 	error = input_register_device(input_dev);
 	if (error) {
 		dev_err(&pdev->dev, "Unable to register keyboard device\n");
-		clk_unprepare(kbd->clk);
 		return error;
 	}
 
@@ -272,13 +267,6 @@ static int spear_kbd_probe(struct platform_device *pdev)
 	return 0;
 }
 
-static void spear_kbd_remove(struct platform_device *pdev)
-{
-	struct spear_kbd *kbd = platform_get_drvdata(pdev);
-
-	clk_unprepare(kbd->clk);
-}
-
 static int spear_kbd_suspend(struct device *dev)
 {
 	struct platform_device *pdev = to_platform_device(dev);
@@ -372,7 +360,6 @@ MODULE_DEVICE_TABLE(of, spear_kbd_id_table);
 
 static struct platform_driver spear_kbd_driver = {
 	.probe		= spear_kbd_probe,
-	.remove_new	= spear_kbd_remove,
 	.driver		= {
 		.name	= "keyboard",
 		.pm	= pm_sleep_ptr(&spear_kbd_pm_ops),
-- 
2.45.2

Re: [PATCH 1/2] Input: spear-keyboard - Fix a double put in spear_kbd_remove()

[Dmitry Torokhov]

Hi Christophe,

On Thu, Jul 25, 2024 at 10:46:49PM +0200, Christophe JAILLET wrote:
> The 'input_dev' is a managed resource allocated with
> devm_input_allocate_device(), so there is no need to call
> input_unregister_device() in the remove function.
> 
> In fact, this call was correctly removed in commit 6102752eb354 ("Input:
> spear-keyboard - switch to using managed resources"), but silently
> re-introduced later in the commit in Fixes.

This change is incorrect as it leads to an active and enabled clock
being unprepared to early. We need to unregister input device which in
turn will call spear_kbd_close() if needed which will disable the clock
in question. Only after that we can unprepare it.

There is also no double put as input core will recognize that input
device was unregistered explicitly and will not attempt to unregister it
2nd time through devm:

void input_unregister_device(struct input_dev *dev)
{
	if (dev->devres_managed) {
		WARN_ON(devres_destroy(dev->dev.parent,
					devm_input_device_unregister,
					devm_input_device_match,
					dev));
		__input_unregister_device(dev);
		/*
		 * We do not do input_put_device() here because it will be done
		 * when 2nd devres fires up.
		 */
	} else {
		__input_unregister_device(dev);
		input_put_device(dev);
	}
}

> 
> Fixes: 9336648978c2 ("Input: spear-keyboard - add clk_{un}prepare() support")
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
> ---
> Compile tested-only
> ---
>  drivers/input/keyboard/spear-keyboard.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
> index 557d00a667ce..5d9fc8dc9433 100644
> --- a/drivers/input/keyboard/spear-keyboard.c
> +++ b/drivers/input/keyboard/spear-keyboard.c
> @@ -276,7 +276,6 @@ static void spear_kbd_remove(struct platform_device *pdev)
>  {
>  	struct spear_kbd *kbd = platform_get_drvdata(pdev);
>  
> -	input_unregister_device(kbd->input);
>  	clk_unprepare(kbd->clk);
>  }
>  
> -- 
> 2.45.2
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] Input: spear-keyboard - Switch to devm_clk_get_prepared()

[Dmitry Torokhov]

On Thu, Jul 25, 2024 at 10:46:50PM +0200, Christophe JAILLET wrote:
> Use devm_clk_get_prepared() in order to remove a clk_unprepare() in an
> error handling path of the probe and completely remove the .remove()
> function.
> 
> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>

Please refresh after dropping the 1st patch and resend. You may remove
the call to input_unregister_device() in the context of this change as
without explicit call to clk_unprepare() on removal we can fully rely on
devm for cleaning up.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] Input: spear-keyboard - Fix a double put in spear_kbd_remove()

[Christophe JAILLET]

Le 25/07/2024 à 22:52, Dmitry Torokhov a écrit :
> Hi Christophe,
> 
> On Thu, Jul 25, 2024 at 10:46:49PM +0200, Christophe JAILLET wrote:
>> The 'input_dev' is a managed resource allocated with
>> devm_input_allocate_device(), so there is no need to call
>> input_unregister_device() in the remove function.
>>
>> In fact, this call was correctly removed in commit 6102752eb354 ("Input:
>> spear-keyboard - switch to using managed resources"), but silently
>> re-introduced later in the commit in Fixes.
> 
> This change is incorrect as it leads to an active and enabled clock
> being unprepared to early. We need to unregister input device which in
> turn will call spear_kbd_close() if needed which will disable the clock
> in question. Only after that we can unprepare it.
> 
> There is also no double put as input core will recognize that input
> device was unregistered explicitly and will not attempt to unregister it
> 2nd time through devm:

Got it.

Thanks for the review and the detailed explanation.
Sorry for the noise.

I'll resend as asked in patch 2/2, if saving some lines of code makes 
enough sense for you.
But as said in the cover letter, if there is no issue, I'm not sure it 
worth the time for an old driver.

CJ

> 
> void input_unregister_device(struct input_dev *dev)
> {
> 	if (dev->devres_managed) {
> 		WARN_ON(devres_destroy(dev->dev.parent,
> 					devm_input_device_unregister,
> 					devm_input_device_match,
> 					dev));
> 		__input_unregister_device(dev);
> 		/*
> 		 * We do not do input_put_device() here because it will be done
> 		 * when 2nd devres fires up.
> 		 */
> 	} else {
> 		__input_unregister_device(dev);
> 		input_put_device(dev);
> 	}
> }
> 
>>
>> Fixes: 9336648978c2 ("Input: spear-keyboard - add clk_{un}prepare() support")
>> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
>> ---
>> Compile tested-only
>> ---
>>   drivers/input/keyboard/spear-keyboard.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/input/keyboard/spear-keyboard.c b/drivers/input/keyboard/spear-keyboard.c
>> index 557d00a667ce..5d9fc8dc9433 100644
>> --- a/drivers/input/keyboard/spear-keyboard.c
>> +++ b/drivers/input/keyboard/spear-keyboard.c
>> @@ -276,7 +276,6 @@ static void spear_kbd_remove(struct platform_device *pdev)
>>   {
>>   	struct spear_kbd *kbd = platform_get_drvdata(pdev);
>>   
>> -	input_unregister_device(kbd->input);
>>   	clk_unprepare(kbd->clk);
>>   }
>>   
>> -- 
>> 2.45.2
>>
> 
> Thanks.
> 

Re: [PATCH 1/2] Input: spear-keyboard - Fix a double put in spear_kbd_remove()

[Dmitry Torokhov]

On Thu, Jul 25, 2024 at 11:34:14PM +0200, Christophe JAILLET wrote:
> Le 25/07/2024 à 22:52, Dmitry Torokhov a écrit :
> > Hi Christophe,
> > 
> > On Thu, Jul 25, 2024 at 10:46:49PM +0200, Christophe JAILLET wrote:
> > > The 'input_dev' is a managed resource allocated with
> > > devm_input_allocate_device(), so there is no need to call
> > > input_unregister_device() in the remove function.
> > > 
> > > In fact, this call was correctly removed in commit 6102752eb354 ("Input:
> > > spear-keyboard - switch to using managed resources"), but silently
> > > re-introduced later in the commit in Fixes.
> > 
> > This change is incorrect as it leads to an active and enabled clock
> > being unprepared to early. We need to unregister input device which in
> > turn will call spear_kbd_close() if needed which will disable the clock
> > in question. Only after that we can unprepare it.
> > 
> > There is also no double put as input core will recognize that input
> > device was unregistered explicitly and will not attempt to unregister it
> > 2nd time through devm:
> 
> Got it.
> 
> Thanks for the review and the detailed explanation.
> Sorry for the noise.
> 
> I'll resend as asked in patch 2/2, if saving some lines of code makes enough
> sense for you.
> But as said in the cover letter, if there is no issue, I'm not sure it worth
> the time for an old driver.

I generally like infrastructure cleanups, unless it is too much trouble.

Thanks.

-- 
Dmitry