On 12/25/2012 05:43 PM, Ben Hutchings wrote:
On Fri, 2012-12-21 at 02:52 +0000, Larry Finger wrote:
[...]
--- /dev/null
+++ b/drivers/bluetooth/rtk_btusb.c
[...]
+#include <linux/version.h>
Not needed.
+#include <linux/pm_runtime.h>
Move this up to the first group of #includes.
+#define HDEV_BUS (hdev->bus)
This is just obfuscation.
+#define USB_RPM 1
+
+#define GET_DRV_DATA(x) hci_get_drvdata(x)
+
+
+#define BTUSB_RPM 0 /*1 SS enable; 0 SS disable */
Run-time power management should be enabled if it works and not included
yet if it doesn't. This shouldn't be a compile-time option in a
production driver.
+#define LOAD_CONFIG 0
Seems to be a rather pointless development option, but if it still has
some value then it deserves a comment.
+#define URB_CANCELING_DELAY_MS 10 /* Added by Realtek */
/* BWH 2012-12-25: Doesn't Realtek have version control to record this? */
[...]
+/*******************************
+** Reasil patch code
+********************************/
Another weird little bit of history which no-one cares about.
+#include <linux/firmware.h>
+#include <linux/suspend.h>
+#include <net/bluetooth/hci.h>
Belong at the top of the file.
[...]
+static int rtkbt_pm_notify(struct notifier_block *notifier, ulong pm_event,
+ void *unused)
+{
+ struct dev_data *dev_entry;
+ struct patch_info *patch_entry;
+ struct usb_device *udev;
+
+ dev_entry = container_of(notifier, struct dev_data, pm_notifier);
+ patch_entry = dev_entry->patch_entry;
+ udev = dev_entry->udev;
+ RTKBT_DBG("rtkbt_pm_notify pm_event =%ld", pm_event);
+ switch (pm_event) {
+ case PM_SUSPEND_PREPARE:
+ case PM_HIBERNATION_PREPARE:
+ patch_entry->fw_len = load_firmware(dev_entry,
+ &patch_entry->fw_cache);
But this is done once for each device, not for each device type. So you
potentially load the firmware multiple times here and leak all but one.
Anyway I'm not sure this caching is needed any more due to the firmware
management improvements in 3.7.
I will modify this to make sure that the firmware is loaded only once.
[...]
+static int load_firmware(struct dev_data *dev_entry, uint8_t **buff)
+{
+#if LOAD_CONFIG
+ const struct firmware *fw;
+#endif
+ struct usb_device *udev;
+ struct patch_info *patch_entry;
+ char *fw_name;
+ int fw_len = 0, ret_val;
+
+ udev = dev_entry->udev;
+ init_completion(&dev_entry->firmware_loading_complete);
+ patch_entry = dev_entry->patch_entry;
+ fw_name = patch_entry->patch_name;
+ RTKBT_DBG("Reading firmware file %s", fw_name);
+ ret_val = request_firmware_nowait(THIS_MODULE, 1, fw_name, &udev->dev,
+ GFP_KERNEL, dev_entry, bt_fw_cb);
+ if (ret_val < 0)
+ goto fw_fail;
+
+ wait_for_completion(&dev_entry->firmware_loading_complete);
What was the point of using request_firmware_nowait() then?
None. Obviously a synchronous firmware load will work as long as load_firmware()
is not called from the probe routine, and this one will fail if it is.
+ if (!dev_entry->fw)
+ goto fw_fail;
+ *buff = kzalloc(dev_entry->fw->size, GFP_KERNEL);
+ if (NULL == *buff)
+ goto alloc_fail;
+ memcpy(*buff, dev_entry->fw->data, dev_entry->fw->size);
+ fw_len = dev_entry->fw->size;
+
+#if LOAD_CONFIG
+ release_firmware(dev_entry->fw);
+ fw_name = patch_entry->config_name;
+ ret_val = request_firmware(&fw, fw_name, &udev->dev);
+ if (ret_val < 0) {
+ fw_len = 0;
+ kfree(*buff);
+ *buff = NULL;
+ goto fw_fail;
+ }
+
+ *buff = krealloc(*buff, fw_len + fw->size, GFP_KERNEL);
+ if (NULL == *buff) {
+ fw_len = 0;
+ release_firmware(fw);
+ goto fw_fail;
+ }
+ memcpy(*buff + fw_len, fw->data, fw->size);
+ fw_len += fw->size;
+#endif
It's easy to concatenate files in userland; why do it in the driver?
+alloc_fail:
+ release_firmware(dev_entry->fw);
+fw_fail:
+ return fw_len;
+}
[...]
Thanks for the review. Following the previous suggestions, I am trying to use
btusb.c for this device with the mini-driver proposed by Tedd Ho-Jeong An.
Larry
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
