diff options
Diffstat (limited to 'drivers/staging/nokia_h4p/TODO')
| -rw-r--r-- | drivers/staging/nokia_h4p/TODO | 132 |
1 files changed, 132 insertions, 0 deletions
diff --git a/drivers/staging/nokia_h4p/TODO b/drivers/staging/nokia_h4p/TODO new file mode 100644 index 000000000000..0ec5823e0ca8 --- /dev/null +++ b/drivers/staging/nokia_h4p/TODO @@ -0,0 +1,132 @@ +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> |