[PATCH] Input: gpio-keys - update to devm_* API

[PATCH] Input: gpio-keys - update to devm_* API

[Manish Badarkhe]

Update the code to use devm_* API so that driver core will manage
resources.

Signed-off-by: Manish Badarkhe <badarkhe.manish-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
:100644 100644 440ce32... b4db721... M	drivers/input/keyboard/gpio_keys.c
 drivers/input/keyboard/gpio_keys.c |   18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 440ce32..b4db721 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -588,7 +588,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 		goto err_out;
 	}
 
-	pdata = kzalloc(sizeof(*pdata) + nbuttons * (sizeof *button),
+	pdata = devm_kzalloc(dev, sizeof(*pdata) + nbuttons * (sizeof(*button)),
 			GFP_KERNEL);
 	if (!pdata) {
 		error = -ENOMEM;
@@ -618,7 +618,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 				dev_err(dev,
 					"Failed to get gpio flags, error: %d\n",
 					error);
-			goto err_free_pdata;
+			goto err_out;
 		}
 
 		button = &pdata->buttons[i++];
@@ -630,7 +630,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 			dev_err(dev, "Button without keycode: 0x%x\n",
 				button->gpio);
 			error = -EINVAL;
-			goto err_free_pdata;
+			goto err_out;
 		}
 
 		button->desc = of_get_property(pp, "label", NULL);
@@ -647,13 +647,11 @@ gpio_keys_get_devtree_pdata(struct device *dev)
 
 	if (pdata->nbuttons == 0) {
 		error = -EINVAL;
-		goto err_free_pdata;
+		goto err_out;
 	}
 
 	return pdata;
 
-err_free_pdata:
-	kfree(pdata);
 err_out:
 	return ERR_PTR(error);
 }
@@ -699,10 +697,10 @@ static int gpio_keys_probe(struct platform_device *pdev)
 			return PTR_ERR(pdata);
 	}
 
-	ddata = kzalloc(sizeof(struct gpio_keys_drvdata) +
+	ddata = devm_kzalloc(&pdev->dev, sizeof(struct gpio_keys_drvdata) +
 			pdata->nbuttons * sizeof(struct gpio_button_data),
 			GFP_KERNEL);
-	input = input_allocate_device();
+	input = devm_input_allocate_device(&pdev->dev);
 	if (!ddata || !input) {
 		dev_err(dev, "failed to allocate state\n");
 		error = -ENOMEM;
@@ -768,8 +766,6 @@ static int gpio_keys_probe(struct platform_device *pdev)
 		gpio_remove_key(&ddata->data[i]);
 
  fail1:
-	input_free_device(input);
-	kfree(ddata);
 	/* If we have no platform data, we allocated pdata dynamically. */
 	if (!dev_get_platdata(&pdev->dev))
 		kfree(pdata);
@@ -796,8 +792,6 @@ static int gpio_keys_remove(struct platform_device *pdev)
 	if (!dev_get_platdata(&pdev->dev))
 		kfree(ddata->pdata);
 
-	kfree(ddata);
-
 	return 0;
 }
 
-- 
1.7.10.4

Re: [PATCH] Input: gpio-keys - update to devm_* API

[Tomasz Figa]

Hi Manish,

Thanks for the patch. I have few comments inline, though.

On Sunday 15 of September 2013 01:22:23 Manish Badarkhe wrote:
> Update the code to use devm_* API so that driver core will manage
> resources.
> 
> Signed-off-by: Manish Badarkhe <badarkhe.manish@gmail.com>
> ---
> 
> :100644 100644 440ce32... b4db721... M	drivers/input/keyboard/gpio_keys.c
> 
>  drivers/input/keyboard/gpio_keys.c |   18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/input/keyboard/gpio_keys.c
> b/drivers/input/keyboard/gpio_keys.c index 440ce32..b4db721 100644
> --- a/drivers/input/keyboard/gpio_keys.c
> +++ b/drivers/input/keyboard/gpio_keys.c
> @@ -588,7 +588,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
>  		goto err_out;
>  	}
> 
> -	pdata = kzalloc(sizeof(*pdata) + nbuttons * (sizeof *button),
> +	pdata = devm_kzalloc(dev, sizeof(*pdata) + nbuttons *
> (sizeof(*button)), GFP_KERNEL);
>  	if (!pdata) {
>  		error = -ENOMEM;
> @@ -618,7 +618,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
>  				dev_err(dev,
>  					"Failed to get gpio flags, error: %d\n",
>  					error);
> -			goto err_free_pdata;
> +			goto err_out;

Since the only thing done after err_out label is returning the error, you 
can simply return the error here.

>  		}
> 
>  		button = &pdata->buttons[i++];
> @@ -630,7 +630,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
>  			dev_err(dev, "Button without keycode: 0x%x\n",
>  				button->gpio);
>  			error = -EINVAL;
> -			goto err_free_pdata;
> +			goto err_out;

Ditto.

>  		}
> 
>  		button->desc = of_get_property(pp, "label", NULL);
> @@ -647,13 +647,11 @@ gpio_keys_get_devtree_pdata(struct device *dev)
> 
>  	if (pdata->nbuttons == 0) {
>  		error = -EINVAL;
> -		goto err_free_pdata;
> +		goto err_out;

Ditto.

>  	}
> 
>  	return pdata;
> 
> -err_free_pdata:
> -	kfree(pdata);
>  err_out:
>  	return ERR_PTR(error);

Then the whole error path here can be dropped.

>  }
> @@ -699,10 +697,10 @@ static int gpio_keys_probe(struct platform_device
> *pdev) return PTR_ERR(pdata);
>  	}
> 
> -	ddata = kzalloc(sizeof(struct gpio_keys_drvdata) +
> +	ddata = devm_kzalloc(&pdev->dev, sizeof(struct gpio_keys_drvdata) +
>  			pdata->nbuttons * sizeof(struct gpio_button_data),
>  			GFP_KERNEL);
> -	input = input_allocate_device();
> +	input = devm_input_allocate_device(&pdev->dev);
>  	if (!ddata || !input) {
>  		dev_err(dev, "failed to allocate state\n");
>  		error = -ENOMEM;
> @@ -768,8 +766,6 @@ static int gpio_keys_probe(struct platform_device
> *pdev) gpio_remove_key(&ddata->data[i]);
> 
>   fail1:
> -	input_free_device(input);
> -	kfree(ddata);
>  	/* If we have no platform data, we allocated pdata dynamically. */
>  	if (!dev_get_platdata(&pdev->dev))
>  		kfree(pdata);

This is incorrect and unnecessary. Since pdata was allocated using devm_ 
helper it will be freed automatically. As a side note, if you want to 
explicitly free memory allocated using devm_ helpers, you need to do it 
using their devm_ free counterparts, not directly.

> @@ -796,8 +792,6 @@ static int gpio_keys_remove(struct platform_device
> *pdev) if (!dev_get_platdata(&pdev->dev))
>  		kfree(ddata->pdata);

Same here.

Best regards,
Tomasz

Re: [PATCH] Input: gpio-keys - update to devm_* API

[Manish Badarkhe]

Hi Tomasz

On Tue, Sep 17, 2013 at 4:49 PM, Tomasz Figa <t.figa-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org> wrote:
> Hi Manish,
>
> Thanks for the patch. I have few comments inline, though.
>
> On Sunday 15 of September 2013 01:22:23 Manish Badarkhe wrote:
>> Update the code to use devm_* API so that driver core will manage
>> resources.
>>
>> Signed-off-by: Manish Badarkhe <badarkhe.manish-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>
>> :100644 100644 440ce32... b4db721... M        drivers/input/keyboard/gpio_keys.c
>>
>>  drivers/input/keyboard/gpio_keys.c |   18 ++++++------------
>>  1 file changed, 6 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/input/keyboard/gpio_keys.c
>> b/drivers/input/keyboard/gpio_keys.c index 440ce32..b4db721 100644
>> --- a/drivers/input/keyboard/gpio_keys.c
>> +++ b/drivers/input/keyboard/gpio_keys.c
>> @@ -588,7 +588,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
>>               goto err_out;
>>       }
>>
>> -     pdata = kzalloc(sizeof(*pdata) + nbuttons * (sizeof *button),
>> +     pdata = devm_kzalloc(dev, sizeof(*pdata) + nbuttons *
>> (sizeof(*button)), GFP_KERNEL);
>>       if (!pdata) {
>>               error = -ENOMEM;
>> @@ -618,7 +618,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
>>                               dev_err(dev,
>>                                       "Failed to get gpio flags, error: %d\n",
>>                                       error);
>> -                     goto err_free_pdata;
>> +                     goto err_out;
>
> Since the only thing done after err_out label is returning the error, you
> can simply return the error here.

Agreed, will do that.

>
>>               }
>>
>>               button = &pdata->buttons[i++];
>> @@ -630,7 +630,7 @@ gpio_keys_get_devtree_pdata(struct device *dev)
>>                       dev_err(dev, "Button without keycode: 0x%x\n",
>>                               button->gpio);
>>                       error = -EINVAL;
>> -                     goto err_free_pdata;
>> +                     goto err_out;
>
> Ditto.

Ok

>
>>               }
>>
>>               button->desc = of_get_property(pp, "label", NULL);
>> @@ -647,13 +647,11 @@ gpio_keys_get_devtree_pdata(struct device *dev)
>>
>>       if (pdata->nbuttons == 0) {
>>               error = -EINVAL;
>> -             goto err_free_pdata;
>> +             goto err_out;
>
> Ditto.

Ok

>
>>       }
>>
>>       return pdata;
>>
>> -err_free_pdata:
>> -     kfree(pdata);
>>  err_out:
>>       return ERR_PTR(error);
>
> Then the whole error path here can be dropped.

Ok, I will drop this error path.

>
>>  }
>> @@ -699,10 +697,10 @@ static int gpio_keys_probe(struct platform_device
>> *pdev) return PTR_ERR(pdata);
>>       }
>>
>> -     ddata = kzalloc(sizeof(struct gpio_keys_drvdata) +
>> +     ddata = devm_kzalloc(&pdev->dev, sizeof(struct gpio_keys_drvdata) +
>>                       pdata->nbuttons * sizeof(struct gpio_button_data),
>>                       GFP_KERNEL);
>> -     input = input_allocate_device();
>> +     input = devm_input_allocate_device(&pdev->dev);
>>       if (!ddata || !input) {
>>               dev_err(dev, "failed to allocate state\n");
>>               error = -ENOMEM;
>> @@ -768,8 +766,6 @@ static int gpio_keys_probe(struct platform_device
>> *pdev) gpio_remove_key(&ddata->data[i]);
>>
>>   fail1:
>> -     input_free_device(input);
>> -     kfree(ddata);
>>       /* If we have no platform data, we allocated pdata dynamically. */
>>       if (!dev_get_platdata(&pdev->dev))
>>               kfree(pdata);
>
> This is incorrect and unnecessary. Since pdata was allocated using devm_
> helper it will be freed automatically. As a side note, if you want to
> explicitly free memory allocated using devm_ helpers, you need to do it
> using their devm_ free counterparts, not directly.

Ok, I will remove this as "pdata" is getting freed automatically.

>> @@ -796,8 +792,6 @@ static int gpio_keys_remove(struct platform_device
>> *pdev) if (!dev_get_platdata(&pdev->dev))
>>               kfree(ddata->pdata);
>
> Same here.

Ok, I will remove this as "pdata" is getting freed automatically.

Regards
Manish Badarkhe

Re: [PATCH] Input: gpio-keys - update to devm_* API

[Dmitry Torokhov]

Hi Manish,

On Sun, Sep 15, 2013 at 01:22:23AM +0530, Manish Badarkhe wrote:
> Update the code to use devm_* API so that driver core will manage
> resources.
> 

And the benefit of this would be...?

There are still resources that are managed in traditional way and I
really dislike mixing the 2 styles. I can see applying patch that
converts the driver to use devm_ for all its resources and gets rid of
the remove() method altogether, but I am not sure how beneficial this
kind of changes are for existing drivers.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: gpio-keys - update to devm_* API

[Manish Badarkhe]

Hi Dmitry,

On Wed, Sep 18, 2013 at 12:22 AM, Dmitry Torokhov
<dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hi Manish,
>
> On Sun, Sep 15, 2013 at 01:22:23AM +0530, Manish Badarkhe wrote:
>> Update the code to use devm_* API so that driver core will manage
>> resources.
>>
>
> And the benefit of this would be...?
>
> There are still resources that are managed in traditional way and I
> really dislike mixing the 2 styles. I can see applying patch that
> converts the driver to use devm_ for all its resources and gets rid of
> the remove() method altogether, but I am not sure how beneficial this
> kind of changes are for existing drivers.

IMO devm_ makes us to manage resources properly without much care about freeing
it ( as devm_handles freeing automatically). But, stiil  there are use
cases which makes
us to mix this devm_  with traditionally managed resources.
Not much sure about any other benefits.

Regards
Manish Badarkhe

Re: [PATCH] Input: gpio-keys - update to devm_* API

[Dmitry Torokhov]

On Wed, Sep 18, 2013 at 12:41:11AM +0530, Manish Badarkhe wrote:
> Hi Dmitry,
> 
> On Wed, Sep 18, 2013 at 12:22 AM, Dmitry Torokhov
> <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Hi Manish,
> >
> > On Sun, Sep 15, 2013 at 01:22:23AM +0530, Manish Badarkhe wrote:
> >> Update the code to use devm_* API so that driver core will manage
> >> resources.
> >>
> >
> > And the benefit of this would be...?
> >
> > There are still resources that are managed in traditional way and I
> > really dislike mixing the 2 styles. I can see applying patch that
> > converts the driver to use devm_ for all its resources and gets rid of
> > the remove() method altogether, but I am not sure how beneficial this
> > kind of changes are for existing drivers.
> 
> IMO devm_ makes us to manage resources properly without much care about freeing
> it ( as devm_handles freeing automatically).

Are the resources released improperly in the current version of the
driver? IOW I do not see the point in applying this patch as it does not
make the driver materially better.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: gpio-keys - update to devm_* API

[Manish Badarkhe]

Hi Dmitry,

On Fri, Sep 20, 2013 at 2:52 AM, Dmitry Torokhov
<dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Sep 18, 2013 at 12:41:11AM +0530, Manish Badarkhe wrote:
>> Hi Dmitry,
>>
>> On Wed, Sep 18, 2013 at 12:22 AM, Dmitry Torokhov
>> <dmitry.torokhov-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> > Hi Manish,
>> >
>> > On Sun, Sep 15, 2013 at 01:22:23AM +0530, Manish Badarkhe wrote:
>> >> Update the code to use devm_* API so that driver core will manage
>> >> resources.
>> >>
>> >
>> > And the benefit of this would be...?
>> >
>> > There are still resources that are managed in traditional way and I
>> > really dislike mixing the 2 styles. I can see applying patch that
>> > converts the driver to use devm_ for all its resources and gets rid of
>> > the remove() method altogether, but I am not sure how beneficial this
>> > kind of changes are for existing drivers.
>>
>> IMO devm_ makes us to manage resources properly without much care about freeing
>> it ( as devm_handles freeing automatically).
>
> Are the resources released improperly in the current version of the
> driver? IOW I do not see the point in applying this patch as it does not
> make the driver materially better.

Resources are released properly in current version of driver but using
a traditional
way. This patch is just a clean up and  to use new devm_ variant. Yes,
This patch does
not make a driver materially better but make use of "devm_" variant as
like any other
drivers.

Thanks
Manish Badarkhe