Use devm_kasprintf instead of kasprintf. Also, remove various
gotos by direct returns and drop unneeded label err_free_name.
Signed-off-by: Vaishali Thakkar <[email protected]>
---
drivers/power/bq24735-charger.c | 25 ++++++++++---------------
1 file changed, 10 insertions(+), 15 deletions(-)
diff --git a/drivers/power/bq24735-charger.c b/drivers/power/bq24735-charger.c
index b017437..b2bb67e 100644
--- a/drivers/power/bq24735-charger.c
+++ b/drivers/power/bq24735-charger.c
@@ -267,8 +267,9 @@ static int bq24735_charger_probe(struct i2c_client *client,
name = (char *)charger->pdata->name;
if (!name) {
- name = kasprintf(GFP_KERNEL, "bq24735@%s",
- dev_name(&client->dev));
+ name = devm_kasprintf(&client->dev, GFP_KERNEL,
+ "bq24735@%s",
+ dev_name(&client->dev));
if (!name) {
dev_err(&client->dev, "Failed to alloc device name\n");
return -ENOMEM;
@@ -296,23 +297,21 @@ static int bq24735_charger_probe(struct i2c_client *client,
if (ret < 0) {
dev_err(&client->dev, "Failed to read manufacturer id : %d\n",
ret);
- goto err_free_name;
+ return ret;
} else if (ret != 0x0040) {
dev_err(&client->dev,
"manufacturer id mismatch. 0x0040 != 0x%04x\n", ret);
- ret = -ENODEV;
- goto err_free_name;
+ return -ENODEV;
}
ret = bq24735_read_word(client, BQ24735_DEVICE_ID);
if (ret < 0) {
dev_err(&client->dev, "Failed to read device id : %d\n", ret);
- goto err_free_name;
+ return ret;
} else if (ret != 0x000B) {
dev_err(&client->dev,
"device id mismatch. 0x000b != 0x%04x\n", ret);
- ret = -ENODEV;
- goto err_free_name;
+ return -ENODEV;
}
if (gpio_is_valid(charger->pdata->status_gpio)) {
@@ -331,7 +330,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
ret = bq24735_config_charger(charger);
if (ret < 0) {
dev_err(&client->dev, "failed in configuring charger");
- goto err_free_name;
+ return ret;
}
/* check for AC adapter presence */
@@ -339,7 +338,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
ret = bq24735_enable_charging(charger);
if (ret < 0) {
dev_err(&client->dev, "Failed to enable charging\n");
- goto err_free_name;
+ return ret;
}
}
@@ -349,7 +348,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
ret = PTR_ERR(charger->charger);
dev_err(&client->dev, "Failed to register power supply: %d\n",
ret);
- goto err_free_name;
+ return ret;
}
if (client->irq) {
@@ -371,10 +370,6 @@ static int bq24735_charger_probe(struct i2c_client *client,
return 0;
err_unregister_supply:
power_supply_unregister(charger->charger);
-err_free_name:
- if (name != charger->pdata->name)
- kfree(name);
-
return ret;
}
--
1.9.1
Hi,
On Fri, Jul 24, 2015 at 1:58 PM, Vaishali Thakkar
<[email protected]> wrote:
> Use devm_kasprintf instead of kasprintf. Also, remove various
> gotos by direct returns and drop unneeded label err_free_name.
If there's to be a respin, reword this so that it becomes clearer that
removing the various gotos and the label is an effect of using
devm_kasprintf here. I started out thinking that this patch did two
things.
Frans
Hi,
Thanks for the cleanup patch.
I have a couple of comments inlined.
> Subject: Re: [PATCH] power_supply: Adjust devm usage
Please make this "power_supply: bq24735: ...".
On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
> Use devm_kasprintf instead of kasprintf. Also, remove various
> gotos by direct returns and drop unneeded label err_free_name.
Please also use devm_power_supply_unregister() instead
of power_supply_unregister() to further simplify the driver.
> @@ -267,8 +267,9 @@ static int bq24735_charger_probe() {}
> [...]
Your patch is missing removal of the
kfree(charger->charger_desc.name) in bq24735_charger_remove().
-- Sebastian
2015-07-24 21:17 GMT+09:00 Frans Klaver <[email protected]>:
> Hi,
>
> On Fri, Jul 24, 2015 at 1:58 PM, Vaishali Thakkar
> <[email protected]> wrote:
>> Use devm_kasprintf instead of kasprintf. Also, remove various
>> gotos by direct returns and drop unneeded label err_free_name.
>
> If there's to be a respin, reword this so that it becomes clearer that
> removing the various gotos and the label is an effect of using
> devm_kasprintf here. I started out thinking that this patch did two
> things.
And please put a driver prefix in the subject (like in rest of
commits). Beside of that:
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On Fri, Jul 24, 2015 at 5:56 PM, Sebastian Reichel <[email protected]> wrote:
> Hi,
Hi
> Thanks for the cleanup patch.
> I have a couple of comments inlined.
>
>> Subject: Re: [PATCH] power_supply: Adjust devm usage
>
> Please make this "power_supply: bq24735: ...".
Ok. Sure.
> On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
>> Use devm_kasprintf instead of kasprintf. Also, remove various
>> gotos by direct returns and drop unneeded label err_free_name.
>
> Please also use devm_power_supply_unregister() instead
> of power_supply_unregister() to further simplify the driver.
Ok.
>> @@ -267,8 +267,9 @@ static int bq24735_charger_probe() {}
>> [...]
>
> Your patch is missing removal of the
> kfree(charger->charger_desc.name) in bq24735_charger_remove().
Yes. Because it seems that this kfree is freeing some other data which
is not related to devm_kzalloc. I was not sure about removing it.
So, I was about to discuss it in a separate thread. Also, in the remove function
we have devm_free_irq. I am unsure it too. Because normally remove functions
do not use devm counterparts.
> -- Sebastian
--
Vaishali
On Fri, Jul 24, 2015 at 5:59 PM, Krzysztof Kozlowski
<[email protected]> wrote:
> 2015-07-24 21:17 GMT+09:00 Frans Klaver <[email protected]>:
>> Hi,
Hi
>> On Fri, Jul 24, 2015 at 1:58 PM, Vaishali Thakkar
>> <[email protected]> wrote:
>>> Use devm_kasprintf instead of kasprintf. Also, remove various
>>> gotos by direct returns and drop unneeded label err_free_name.
>>
>> If there's to be a respin, reword this so that it becomes clearer that
>> removing the various gotos and the label is an effect of using
>> devm_kasprintf here. I started out thinking that this patch did two
>> things.
Ok. Sure. I will change the commit log.
> And please put a driver prefix in the subject (like in rest of
> commits). Beside of that:
> Reviewed-by: Krzysztof Kozlowski <[email protected]>
Ok.
> Best regards,
> Krzysztof
--
Vaishali
2015-07-24 21:26 GMT+09:00 Sebastian Reichel <[email protected]>:
> Hi,
>
> Thanks for the cleanup patch.
> I have a couple of comments inlined.
>
>> Subject: Re: [PATCH] power_supply: Adjust devm usage
>
> Please make this "power_supply: bq24735: ...".
>
> On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
>> Use devm_kasprintf instead of kasprintf. Also, remove various
>> gotos by direct returns and drop unneeded label err_free_name.
>
> Please also use devm_power_supply_unregister() instead
> of power_supply_unregister() to further simplify the driver.
>
>> @@ -267,8 +267,9 @@ static int bq24735_charger_probe() {}
>> [...]
>
> Your patch is missing removal of the
> kfree(charger->charger_desc.name) in bq24735_charger_remove().
Right, I missed that... My review was not sufficient.
Best regards,
Krzysztof
On Fri, Jul 24, 2015 at 2:26 PM, Sebastian Reichel <[email protected]> wrote:
> Hi,
>
> Thanks for the cleanup patch.
> I have a couple of comments inlined.
>
>> Subject: Re: [PATCH] power_supply: Adjust devm usage
>
> Please make this "power_supply: bq24735: ...".
>
> On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
>> Use devm_kasprintf instead of kasprintf. Also, remove various
>> gotos by direct returns and drop unneeded label err_free_name.
>
> Please also use devm_power_supply_unregister() instead
> of power_supply_unregister() to further simplify the driver.
Sounds like a separate patch.
Frans
Hi,
On Fri, Jul 24, 2015 at 06:03:38PM +0530, Vaishali Thakkar wrote:
> On Fri, Jul 24, 2015 at 5:56 PM, Sebastian Reichel <[email protected]> wrote:
> > On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
> >> @@ -267,8 +267,9 @@ static int bq24735_charger_probe() {}
> >> [...]
> >
> > Your patch is missing removal of the
> > kfree(charger->charger_desc.name) in bq24735_charger_remove().
>
> Yes. Because it seems that this kfree is freeing some other data which
> is not related to devm_kzalloc. I was not sure about removing it.
> So, I was about to discuss it in a separate thread.s
it's assigned in the probe function:
name = kasprintf(...);
...
supply_desc->name = name;
...
power_supply_register(..., supply_desc, ...);
> Also, in the remove function we have devm_free_irq. I am unsure it
> too. Because normally remove functions do not use devm
> counterparts.
It's required to free the irq before removing the power supply
device.
If the power supply is registered with devm, that should happen
automatically, since it is requested before the irq. Thus the
remove function can be removed completely at that point :)
-- Sebastian
Hi,
On Fri, Jul 24, 2015 at 02:39:12PM +0200, Frans Klaver wrote:
> On Fri, Jul 24, 2015 at 2:26 PM, Sebastian Reichel <[email protected]> wrote:
> > On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
> >> Use devm_kasprintf instead of kasprintf. Also, remove various
> >> gotos by direct returns and drop unneeded label err_free_name.
> >
> > Please also use devm_power_supply_unregister() instead
> > of power_supply_unregister() to further simplify the driver.
>
> Sounds like a separate patch.
I would be fine with either one or two patches. It's common, that
devm conversion happens in one commit instead of one patch per
function change.
-- Sebastian
On Fri, Jul 24, 2015 at 6:29 PM, Sebastian Reichel <[email protected]> wrote:
> Hi,
>
> On Fri, Jul 24, 2015 at 06:03:38PM +0530, Vaishali Thakkar wrote:
>> On Fri, Jul 24, 2015 at 5:56 PM, Sebastian Reichel <[email protected]> wrote:
>> > On Fri, Jul 24, 2015 at 05:28:13PM +0530, Vaishali Thakkar wrote:
>> >> @@ -267,8 +267,9 @@ static int bq24735_charger_probe() {}
>> >> [...]
>> >
>> > Your patch is missing removal of the
>> > kfree(charger->charger_desc.name) in bq24735_charger_remove().
>>
>> Yes. Because it seems that this kfree is freeing some other data which
>> is not related to devm_kzalloc. I was not sure about removing it.
>> So, I was about to discuss it in a separate thread.s
>
> it's assigned in the probe function:
>
> name = kasprintf(...);
> ...
> supply_desc->name = name;
> ...
> power_supply_register(..., supply_desc, ...);
>
Oh. Yes. I missed that.
>> Also, in the remove function we have devm_free_irq. I am unsure it
>> too. Because normally remove functions do not use devm
>> counterparts.
>
> It's required to free the irq before removing the power supply
> device.
>
> If the power supply is registered with devm, that should happen
> automatically, since it is requested before the irq. Thus the
> remove function can be removed completely at that point :)
This makes sense. Thanks for explanation and review :)
So, can I send all changes along with getting rid of remove
function here in a single patch?
> -- Sebastian
--
Vaishali
Hi,
On Fri, Jul 24, 2015 at 06:44:41PM +0530, Vaishali Thakkar wrote:
> [...]
>
> This makes sense. Thanks for explanation and review :)
> So, can I send all changes along with getting rid of remove
> function here in a single patch?
A single patch is fine for me. Use something like the
following patch subject then:
"power_supply: bq24735: Convert to using managed resources"
-- Sebastian
On Fri, Jul 24, 2015 at 7:08 PM, Sebastian Reichel <[email protected]> wrote:
> Hi,
Hi,
> On Fri, Jul 24, 2015 at 06:44:41PM +0530, Vaishali Thakkar wrote:
>> [...]
>>
>> This makes sense. Thanks for explanation and review :)
>> So, can I send all changes along with getting rid of remove
>> function here in a single patch?
>
> A single patch is fine for me. Use something like the
> following patch subject then:
>
> "power_supply: bq24735: Convert to using managed resources"
Ok. Sure. I'll send version 2 of a patch with this subject line and detailed
commit log.
Thank You.
> -- Sebastian
--
Vaishali
On Fri 2015-07-24 17:28:13, Vaishali Thakkar wrote:
> Use devm_kasprintf instead of kasprintf. Also, remove various
> gotos by direct returns and drop unneeded label err_free_name.
What happens if some /sys file is still open when the device is
removed?
> Signed-off-by: Vaishali Thakkar <[email protected]>
> ---
> drivers/power/bq24735-charger.c | 25 ++++++++++---------------
> 1 file changed, 10 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/power/bq24735-charger.c b/drivers/power/bq24735-charger.c
> index b017437..b2bb67e 100644
> --- a/drivers/power/bq24735-charger.c
> +++ b/drivers/power/bq24735-charger.c
> @@ -267,8 +267,9 @@ static int bq24735_charger_probe(struct i2c_client *client,
>
> name = (char *)charger->pdata->name;
> if (!name) {
> - name = kasprintf(GFP_KERNEL, "bq24735@%s",
> - dev_name(&client->dev));
> + name = devm_kasprintf(&client->dev, GFP_KERNEL,
> + "bq24735@%s",
> + dev_name(&client->dev));
> if (!name) {
> dev_err(&client->dev, "Failed to alloc device name\n");
> return -ENOMEM;
> @@ -296,23 +297,21 @@ static int bq24735_charger_probe(struct i2c_client *client,
> if (ret < 0) {
> dev_err(&client->dev, "Failed to read manufacturer id : %d\n",
> ret);
> - goto err_free_name;
> + return ret;
> } else if (ret != 0x0040) {
> dev_err(&client->dev,
> "manufacturer id mismatch. 0x0040 != 0x%04x\n", ret);
> - ret = -ENODEV;
> - goto err_free_name;
> + return -ENODEV;
> }
>
> ret = bq24735_read_word(client, BQ24735_DEVICE_ID);
> if (ret < 0) {
> dev_err(&client->dev, "Failed to read device id : %d\n", ret);
> - goto err_free_name;
> + return ret;
> } else if (ret != 0x000B) {
> dev_err(&client->dev,
> "device id mismatch. 0x000b != 0x%04x\n", ret);
> - ret = -ENODEV;
> - goto err_free_name;
> + return -ENODEV;
> }
>
> if (gpio_is_valid(charger->pdata->status_gpio)) {
> @@ -331,7 +330,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
> ret = bq24735_config_charger(charger);
> if (ret < 0) {
> dev_err(&client->dev, "failed in configuring charger");
> - goto err_free_name;
> + return ret;
> }
>
> /* check for AC adapter presence */
> @@ -339,7 +338,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
> ret = bq24735_enable_charging(charger);
> if (ret < 0) {
> dev_err(&client->dev, "Failed to enable charging\n");
> - goto err_free_name;
> + return ret;
> }
> }
>
> @@ -349,7 +348,7 @@ static int bq24735_charger_probe(struct i2c_client *client,
> ret = PTR_ERR(charger->charger);
> dev_err(&client->dev, "Failed to register power supply: %d\n",
> ret);
> - goto err_free_name;
> + return ret;
> }
>
> if (client->irq) {
> @@ -371,10 +370,6 @@ static int bq24735_charger_probe(struct i2c_client *client,
> return 0;
> err_unregister_supply:
> power_supply_unregister(charger->charger);
> -err_free_name:
> - if (name != charger->pdata->name)
> - kfree(name);
> -
> return ret;
> }
>
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi,
On Sun, Aug 02, 2015 at 08:53:43AM +0200, Pavel Machek wrote:
> On Fri 2015-07-24 17:28:13, Vaishali Thakkar wrote:
> > Use devm_kasprintf instead of kasprintf. Also, remove various
> > gotos by direct returns and drop unneeded label err_free_name.
>
> What happens if some /sys file is still open when the device is
> removed?
There is currently discussion about this on LKML:
https://lkml.org/lkml/2015/7/15/731
-- Sebastian