Skip to contentSkip to conversation

ath3k: Avoid duplication of code

PATCH In review Rogério Brito 2 participants 2 messages Jan 27, 2011 – Jan 28, 2011 Bluetooth drivers

lore
Share & tools
lore

Rogério Brito

author
#0 lore raw

PATCH 1/2 ath3k: Avoid duplication of code

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 <[email protected]>

---

I am sending here two proposals for rewriting the code. This is the first one. The second uses the ternary operator and seems to have more usage in the kernel (or, at least, under mm).

---

drivers/bluetooth/ath3k.c +5 -5
index a126e61..0043781 100644
@@ -116,13 +116,13 @@ 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;
+	if (ret)
+		return -EIO;
+	else
+		return 0;
 }
 
 static void ath3k_disconnect(struct usb_interface *intf)
#1 lore raw

Hi,

2011/1/28 Rogério Brito <[email protected]>:

Quoted 45 lines

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 <[email protected]>

---

I am sending here two proposals for rewriting the code.  This is the first one. The second uses the ternary operator and seems to have more usage in the kernel (or, at least, under mm).

---

drivers/bluetooth/ath3k.c +0 -2
index a126e61..0043781 100644
@@ -116,13 +116,13 @@ static int ath3k_probe(struct usb_interface *intf,
                return -EIO;
        }
 
-       if (ath3k_load_firmware(udev, firmware)) {
-               release_firmware(firmware);

drivers/bluetooth/ath3k.c static int ath3k_probe(struct usb_interface *intf, · line 125

Quoted 10 lines
drivers/bluetooth/ath3k.c +5 -3
-               return -EIO;
-       }
+       ret = ath3k_load_firmware(udev, firmware);
        release_firmware(firmware);
 
-       return 0;
+       if (ret)
+               return -EIO;
+       else
+               return 0;

We don't need "else" after a return statement, in order to avoid a indentation level.

Quoted 9 lines
drivers/bluetooth/ath3k.c +0 -1
  }
 
  static void ath3k_disconnect(struct usb_interface *intf)
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/