* [PATCH 02/02] ath3k: Avoid duplication of code
@ 2011-01-27 23:24 Rogério Brito
2011-01-28 8:56 ` Alexander Holler
0 siblings, 1 reply; 6+ messages in thread
From: Rogério Brito @ 2011-01-27 23:24 UTC (permalink / raw)
To: linux-kernel; +Cc: Alexander Holler, Gustavo F. Padovan, rbrito
Hi.
In commit 86e09287e4f8c81831b4d4118a48597565f0d21b, to reduce memory
usage, the functions of the ath3k module were rewritten to release the
firmware blob after it has been loaded (successfully or not).
The resuting code has some redundancy and the compiler can potentially
produce better code if we omit a function call that is unconditionally
executed in
,----
| if (ath3k_load_firmware(udev, firmware)) {
| release_firmware(firmware);
| return -EIO;
| }
| release_firmware(firmware);
|
| return 0;
| }
`----
It may also be argued that the code becomes easier to read, and also to
see the code coverage of the snippet in question.
Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
---
This is the second (preferred?) version of the patch in question.
---
diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c
index a126e61..d51c5a3 100644
--- a/drivers/bluetooth/ath3k.c
+++ b/drivers/bluetooth/ath3k.c
@@ -116,13 +116,10 @@ static int ath3k_probe(struct usb_interface *intf,
return -EIO;
}
- if (ath3k_load_firmware(udev, firmware)) {
- release_firmware(firmware);
- return -EIO;
- }
+ ret = ath3k_load_firmware(udev, firmware);
release_firmware(firmware);
- return 0;
+ return ret ? -EIO : 0;
}
static void ath3k_disconnect(struct usb_interface *intf)
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH 02/02] ath3k: Avoid duplication of code
2011-01-27 23:24 [PATCH 02/02] ath3k: Avoid duplication of code Rogério Brito
@ 2011-01-28 8:56 ` Alexander Holler
2011-01-28 22:18 ` [PATCH] " Rogério Brito
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Holler @ 2011-01-28 8:56 UTC (permalink / raw)
To: Rogério Brito; +Cc: linux-kernel, Gustavo F. Padovan
Hello Rogério,
Am 28.01.2011 00:24, schrieb Rogério Brito:
> The resuting code has some redundancy and the compiler can potentially
> produce better code if we omit a function call that is unconditionally
> executed in
> diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c
> index a126e61..d51c5a3 100644
> --- a/drivers/bluetooth/ath3k.c
> +++ b/drivers/bluetooth/ath3k.c
> @@ -116,13 +116,10 @@ static int ath3k_probe(struct usb_interface *intf,
> return -EIO;
> }
>
> - if (ath3k_load_firmware(udev, firmware)) {
> - release_firmware(firmware);
> - return -EIO;
> - }
> + ret = ath3k_load_firmware(udev, firmware);
> release_firmware(firmware);
>
> - return 0;
> + return ret ? -EIO : 0;
> }
Looks nice, but doesn't compile. I assume you should at least try to
compile the stuff you want to change. ;)
Regards,
Alexander
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH] ath3k: Avoid duplication of code
2011-01-28 8:56 ` Alexander Holler
@ 2011-01-28 22:18 ` Rogério Brito
2011-01-29 11:59 ` Alexander Holler
2011-02-02 16:37 ` Gustavo F. Padovan
0 siblings, 2 replies; 6+ messages in thread
From: Rogério Brito @ 2011-01-28 22:18 UTC (permalink / raw)
To: linux-kernel; +Cc: Miguel Ojeda, Alexander Holler, Gustavo F. Padovan, rbrito
Hi.
In commit 86e09287e4f8c81831b4d4118a48597565f0d21b, to reduce memory
usage, the functions of the ath3k module were rewritten to release the
firmware blob after it has been loaded (successfully or not).
The resuting code has some redundancy and the compiler can potentially
produce better code if we omit a function call that is unconditionally
executed in
,----
| if (ath3k_load_firmware(udev, firmware)) {
| release_firmware(firmware);
| return -EIO;
| }
| release_firmware(firmware);
|
| return 0;
| }
`----
It may also be argued that the rewritten code becomes easier to read,
and also to see the code coverage of the snippet in question.
Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
Cc: Alexander Holler <holler@ahsoftware.de>
Cc: "Gustavo F. Padovan" <padovan@profusion.mobi>
Cc: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
---
Thanks for all the feedback on the previous patches. The one that didn't
compile before was sent without a hunk. This time everything all the
basics are verified.
diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c
index a126e61..42fa531 100644
--- a/drivers/bluetooth/ath3k.c
+++ b/drivers/bluetooth/ath3k.c
@@ -106,6 +106,7 @@ static int ath3k_probe(struct usb_interface *intf,
{
const struct firmware *firmware;
struct usb_device *udev = interface_to_usbdev(intf);
+ int ret;
BT_DBG("intf %p id %p", intf, id);
@@ -116,13 +117,10 @@ static int ath3k_probe(struct usb_interface *intf,
return -EIO;
}
- if (ath3k_load_firmware(udev, firmware)) {
- release_firmware(firmware);
- return -EIO;
- }
+ ret = ath3k_load_firmware(udev, firmware);
release_firmware(firmware);
- return 0;
+ return ret ? -EIO : 0;
}
static void ath3k_disconnect(struct usb_interface *intf)
--
Rogério Brito : rbrito@{mackenzie,ime.usp}.br : GPG key 1024D/7C2CAEB8
http://www.ime.usp.br/~rbrito : http://meusite.mackenzie.com.br/rbrito
Projects: algorithms.berlios.de : lame.sf.net : vrms.alioth.debian.org
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] ath3k: Avoid duplication of code
2011-01-28 22:18 ` [PATCH] " Rogério Brito
@ 2011-01-29 11:59 ` Alexander Holler
2011-01-30 21:38 ` Rogério Brito
2011-02-02 16:37 ` Gustavo F. Padovan
1 sibling, 1 reply; 6+ messages in thread
From: Alexander Holler @ 2011-01-29 11:59 UTC (permalink / raw)
To: Rogério Brito; +Cc: linux-kernel, Miguel Ojeda, Gustavo F. Padovan
Am 28.01.2011 23:18, schrieb Rogério Brito:
> Thanks for all the feedback on the previous patches. The one that
> didn't compile before was sent without a hunk. This time everything
> all the basics are verified.
They both had the same error, so I assume none of them compiled.
Besides that I don't have an opinion on your patch.
That stuff is only executed once when a device will be attached. The
only reason I had a look at the code, was that I once got an out of
memory error after I've attached such an device to a memory constrained
(embedded) system.
Regards,
Alexander
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ath3k: Avoid duplication of code
2011-01-29 11:59 ` Alexander Holler
@ 2011-01-30 21:38 ` Rogério Brito
0 siblings, 0 replies; 6+ messages in thread
From: Rogério Brito @ 2011-01-30 21:38 UTC (permalink / raw)
To: Alexander Holler; +Cc: linux-kernel, Miguel Ojeda, Gustavo F. Padovan
Hi, Alexander.
On Jan 29 2011, Alexander Holler wrote:
> Am 28.01.2011 23:18, schrieb Rogério Brito:
> >Thanks for all the feedback on the previous patches. The one that
> >didn't compile before was sent without a hunk. This time everything
> >all the basics are verified.
>
> They both had the same error, so I assume none of them compiled.
No, they both compiled. It was just that I messed up when generating my
patches (with git add --interactive). My git-fu is currently low, but
improving. :)
Anyway, the new version that I sent (when applied) compiles, and makes it
shorter to read the code (some could say that it more explicitly exposes
what happens in the function).
Regards,
--
Rogério Brito : rbrito@{ime.usp.br,gmail.com} : GPG key 4096R/BCFCAAAA
http://rb.doesntexist.org : Packages for LaTeX : algorithms.berlios.de
DebianQA: http://qa.debian.org/developer.php?login=rbrito%40ime.usp.br
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ath3k: Avoid duplication of code
2011-01-28 22:18 ` [PATCH] " Rogério Brito
2011-01-29 11:59 ` Alexander Holler
@ 2011-02-02 16:37 ` Gustavo F. Padovan
1 sibling, 0 replies; 6+ messages in thread
From: Gustavo F. Padovan @ 2011-02-02 16:37 UTC (permalink / raw)
To: Rogério Brito; +Cc: linux-kernel, Miguel Ojeda, Alexander Holler
Hi Rogério,
* Rogério Brito <rbrito@ime.usp.br> [2011-01-28 20:18:10 -0200]:
>
> Hi.
>
> In commit 86e09287e4f8c81831b4d4118a48597565f0d21b, to reduce memory
> usage, the functions of the ath3k module were rewritten to release the
> firmware blob after it has been loaded (successfully or not).
>
> The resuting code has some redundancy and the compiler can potentially
> produce better code if we omit a function call that is unconditionally
> executed in
>
> ,----
> | if (ath3k_load_firmware(udev, firmware)) {
> | release_firmware(firmware);
> | return -EIO;
> | }
> | release_firmware(firmware);
> |
> | return 0;
> | }
> `----
>
> It may also be argued that the rewritten code becomes easier to read,
> and also to see the code coverage of the snippet in question.
>
>
> Signed-off-by: Rogério Brito <rbrito@ime.usp.br>
> Cc: Alexander Holler <holler@ahsoftware.de>
> Cc: "Gustavo F. Padovan" <padovan@profusion.mobi>
> Cc: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com>
Please send me your patch in a 'git format-patch' way, so I can apply it
without having to edit the commit message. And add "Bluetooth:" to the
beginning of the commit title.
>
> ---
>
> Thanks for all the feedback on the previous patches. The one that didn't
> compile before was sent without a hunk. This time everything all the
> basics are verified.
>
>
> diff --git a/drivers/bluetooth/ath3k.c b/drivers/bluetooth/ath3k.c
> index a126e61..42fa531 100644
> --- a/drivers/bluetooth/ath3k.c
> +++ b/drivers/bluetooth/ath3k.c
> @@ -106,6 +106,7 @@ static int ath3k_probe(struct usb_interface *intf,
> {
> const struct firmware *firmware;
> struct usb_device *udev = interface_to_usbdev(intf);
> + int ret;
>
> BT_DBG("intf %p id %p", intf, id);
>
> @@ -116,13 +117,10 @@ static int ath3k_probe(struct usb_interface *intf,
> return -EIO;
> }
>
> - if (ath3k_load_firmware(udev, firmware)) {
> - release_firmware(firmware);
> - return -EIO;
> - }
> + ret = ath3k_load_firmware(udev, firmware);
> release_firmware(firmware);
>
> - return 0;
> + return ret ? -EIO : 0;
just return ret;
It is enough.
Regards,
--
Gustavo F. Padovan
http://profusion.mobi
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-02-02 16:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-01-27 23:24 [PATCH 02/02] ath3k: Avoid duplication of code Rogério Brito
2011-01-28 8:56 ` Alexander Holler
2011-01-28 22:18 ` [PATCH] " Rogério Brito
2011-01-29 11:59 ` Alexander Holler
2011-01-30 21:38 ` Rogério Brito
2011-02-02 16:37 ` Gustavo F. Padovan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox