Re: [PATCH] Input: gpio-keys - update to devm_* API
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.torok...@gmail.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: gpio-keys - update to devm_* API
Hi Dmitry, On Fri, Sep 20, 2013 at 2:52 AM, Dmitry Torokhov dmitry.torok...@gmail.com 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.torok...@gmail.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: gpio-keys - update to devm_* API
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.man...@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: gpio-keys - update to devm_* API
Hi Tomasz
On Tue, Sep 17, 2013 at 4:49 PM, Tomasz Figa t.f...@samsung.com 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.man...@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.
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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: gpio-keys - update to devm_* API
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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: gpio-keys - update to devm_* API
Hi Dmitry, On Wed, Sep 18, 2013 at 12:22 AM, Dmitry Torokhov dmitry.torok...@gmail.com 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 -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
