diff options
Diffstat (limited to 'drivers/staging/nokia_h4p/TODO')
| -rw-r--r-- | drivers/staging/nokia_h4p/TODO | 132 |
1 files changed, 0 insertions, 132 deletions
diff --git a/drivers/staging/nokia_h4p/TODO b/drivers/staging/nokia_h4p/TODO deleted file mode 100644 index 0ec5823e0ca8..000000000000 --- a/drivers/staging/nokia_h4p/TODO +++ /dev/null @@ -1,132 +0,0 @@ -Few attempts to submission have been made, last review comments were received in - -Date: Wed, 15 Jan 2014 19:01:51 -0800 -From: Marcel Holtmann <marcel@holtmann.org> -Subject: Re: [PATCH v6] Bluetooth: Add hci_h4p driver - -Some code refactoring is still needed. - -TODO: - -> +++ b/drivers/bluetooth/hci_h4p.h - -can we please get the naming straight. File names do not start with -hci_ anymore. We moved away from it since that term is too generic. - -> +struct hci_h4p_info { - -Can we please get rid of the hci_ prefix for everything. Copying from -drivers that are over 10 years old is not a good idea. Please look at -recent ones. - -> + struct timer_list lazy_release; - -Timer? Not delayed work? - -> +void hci_h4p_outb(struct hci_h4p_info *info, unsigned int offset, u8 val); -> +u8 hci_h4p_inb(struct hci_h4p_info *info, unsigned int offset); -> +void hci_h4p_set_rts(struct hci_h4p_info *info, int active); -> +int hci_h4p_wait_for_cts(struct hci_h4p_info *info, int active, int timeout_ms); -> +void __hci_h4p_set_auto_ctsrts(struct hci_h4p_info *info, int on, u8 which); -> +void hci_h4p_set_auto_ctsrts(struct hci_h4p_info *info, int on, u8 which); -> +void hci_h4p_change_speed(struct hci_h4p_info *info, unsigned long speed); -> +int hci_h4p_reset_uart(struct hci_h4p_info *info); -> +void hci_h4p_init_uart(struct hci_h4p_info *info); -> +void hci_h4p_enable_tx(struct hci_h4p_info *info); -> +void hci_h4p_store_regs(struct hci_h4p_info *info); -> +void hci_h4p_restore_regs(struct hci_h4p_info *info); -> +void hci_h4p_smart_idle(struct hci_h4p_info *info, bool enable); - -These are a lot of public functions. Are they all really needed or can -the code be done smart. - -> +static ssize_t hci_h4p_store_bdaddr(struct device *dev, -> + struct device_attribute *attr, -> + const char *buf, size_t count) -> +{ -> + struct hci_h4p_info *info = dev_get_drvdata(dev); - -Since none of these devices can function without having a valid -address, the way this should work is that we should not register the -HCI device when probing the platform device. - -The HCI device should be registered once a valid address has been -written into the sysfs file. I do not want to play the tricks with -bringing up the device without a valid address. - -> + hdev->close = hci_h4p_hci_close; -> + hdev->flush = hci_h4p_hci_flush; -> + hdev->send = hci_h4p_hci_send_frame; - -It needs to use hdev->setup to load the firmware. I assume the -firmware only needs to be loaded once. That is exactly what -hdev->setup does. It gets executed once. - -> + set_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks); - -Is this quirk really needed? Normally only Bluetooth 1.1 and early -devices qualify for it. - -> +static int hci_h4p_bcm_set_bdaddr(struct hci_h4p_info *info, struct sk_buff *skb) -> +{ -> + int i; -> + static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf}; -> + int not_valid; - -Has this actually been confirmed that we can just randomly set an -address out of the Nokia range. I do not think so. This is a pretty -bad idea. - -I have no interest in merging a driver with such a hack. - -> + not_valid = 1; -> + for (i = 0; i < 6; i++) { -> + if (info->bd_addr[i] != 0x00) { -> + not_valid = 0; -> + break; -> + } -> + } - -Anybody every heard of memcmp or bacmp and BDADDR_ANY? - -> + if (not_valid) { -> + dev_info(info->dev, "Valid bluetooth address not found," -> + " setting some random\n"); -> + /* When address is not valid, use some random */ -> + memcpy(info->bd_addr, nokia_oui, 3); -> + get_random_bytes(info->bd_addr + 3, 3); -> + } - - -And why does every single chip firmware does this differently. Seriously, this is a mess. - -> +void hci_h4p_parse_fw_event(struct hci_h4p_info *info, struct sk_buff *skb) -> +{ -> + switch (info->man_id) { -> + case H4P_ID_CSR: -> + hci_h4p_bc4_parse_fw_event(info, skb); -> + break; -... -> +} - -We have proper HCI sync command handling in recent kernels. I really -do not know why this is hand coded these days. Check how the Intel -firmware loading inside btusb.c does it. - -> +inline u8 hci_h4p_inb(struct hci_h4p_info *info, unsigned int offset) -> +{ -> + return __raw_readb(info->uart_base + (offset << 2)); -> +} - -Inline in a *.c file for a non-static function. Makes no sense to me. - -> +/** -> + * struct hci_h4p_platform data - hci_h4p Platform data structure -> + */ -> +struct hci_h4p_platform_data { - -please have a proper name here. For example -btnokia_h4p_platform_data. - -Please send patches to Greg Kroah-Hartman <greg@kroah.com> and Cc: -Pavel Machek <pavel@ucw.cz> |