projects / deliverable / linux.git / blame
| Commit | Line | Data |
|---|---|---|
| 91eef3e2 PM |
1 | Few attempts to submission have been made, last review comments were received in |
| 2 | ||
| 3 | Date: Wed, 15 Jan 2014 19:01:51 -0800 | |
| 4 | From: Marcel Holtmann <marcel@holtmann.org> | |
| 5 | Subject: Re: [PATCH v6] Bluetooth: Add hci_h4p driver | |
| 6 | ||
| 7 | Some code refactoring is still needed. | |
| 8 | ||
| 9 | TODO: | |
| 10 | ||
| 11 | > +++ b/drivers/bluetooth/hci_h4p.h | |
| 12 | ||
| 13 | can we please get the naming straight. File names do not start with | |
| 14 | hci_ anymore. We moved away from it since that term is too generic. | |
| 15 | ||
| 91eef3e2 PM |
16 | > +struct hci_h4p_info { |
| 17 | ||
| 18 | Can we please get rid of the hci_ prefix for everything. Copying from | |
| 19 | drivers that are over 10 years old is not a good idea. Please look at | |
| 20 | recent ones. | |
| 21 | ||
| 22 | > + struct timer_list lazy_release; | |
| 23 | ||
| 24 | Timer? Not delayed work? | |
| 25 | ||
| 26 | > +void hci_h4p_outb(struct hci_h4p_info *info, unsigned int offset, u8 val); | |
| 27 | > +u8 hci_h4p_inb(struct hci_h4p_info *info, unsigned int offset); | |
| 28 | > +void hci_h4p_set_rts(struct hci_h4p_info *info, int active); | |
| 29 | > +int hci_h4p_wait_for_cts(struct hci_h4p_info *info, int active, int timeout_ms); | |
| 30 | > +void __hci_h4p_set_auto_ctsrts(struct hci_h4p_info *info, int on, u8 which); | |
| 31 | > +void hci_h4p_set_auto_ctsrts(struct hci_h4p_info *info, int on, u8 which); | |
| 32 | > +void hci_h4p_change_speed(struct hci_h4p_info *info, unsigned long speed); | |
| 33 | > +int hci_h4p_reset_uart(struct hci_h4p_info *info); | |
| 34 | > +void hci_h4p_init_uart(struct hci_h4p_info *info); | |
| 35 | > +void hci_h4p_enable_tx(struct hci_h4p_info *info); | |
| 36 | > +void hci_h4p_store_regs(struct hci_h4p_info *info); | |
| 37 | > +void hci_h4p_restore_regs(struct hci_h4p_info *info); | |
| 38 | > +void hci_h4p_smart_idle(struct hci_h4p_info *info, bool enable); | |
| 39 | ||
| 40 | These are a lot of public functions. Are they all really needed or can | |
| 41 | the code be done smart. | |
| 42 | ||
| 43 | > +static ssize_t hci_h4p_store_bdaddr(struct device *dev, | |
| 44 | > + struct device_attribute *attr, | |
| 45 | > + const char *buf, size_t count) | |
| 46 | > +{ | |
| 47 | > + struct hci_h4p_info *info = dev_get_drvdata(dev); | |
| 48 | ||
| 49 | Since none of these devices can function without having a valid | |
| 50 | address, the way this should work is that we should not register the | |
| 51 | HCI device when probing the platform device. | |
| 52 | ||
| 53 | The HCI device should be registered once a valid address has been | |
| 54 | written into the sysfs file. I do not want to play the tricks with | |
| 55 | bringing up the device without a valid address. | |
| 56 | ||
| 57 | > + hdev->close = hci_h4p_hci_close; | |
| 58 | > + hdev->flush = hci_h4p_hci_flush; | |
| 59 | > + hdev->send = hci_h4p_hci_send_frame; | |
| 60 | ||
| 61 | It needs to use hdev->setup to load the firmware. I assume the | |
| 62 | firmware only needs to be loaded once. That is exactly what | |
| 63 | hdev->setup does. It gets executed once. | |
| 64 | ||
| 65 | > + set_bit(HCI_QUIRK_RESET_ON_CLOSE, &hdev->quirks); | |
| 66 | ||
| 67 | Is this quirk really needed? Normally only Bluetooth 1.1 and early | |
| 68 | devices qualify for it. | |
| 69 | ||
| 70 | > +static int hci_h4p_bcm_set_bdaddr(struct hci_h4p_info *info, struct sk_buff *skb) | |
| 71 | > +{ | |
| 72 | > + int i; | |
| 73 | > + static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf}; | |
| 74 | > + int not_valid; | |
| 75 | ||
| 76 | Has this actually been confirmed that we can just randomly set an | |
| 77 | address out of the Nokia range. I do not think so. This is a pretty | |
| 78 | bad idea. | |
| 79 | ||
| 80 | I have no interest in merging a driver with such a hack. | |
| 81 | ||
| 82 | > + not_valid = 1; | |
| 83 | > + for (i = 0; i < 6; i++) { | |
| 84 | > + if (info->bd_addr[i] != 0x00) { | |
| 85 | > + not_valid = 0; | |
| 86 | > + break; | |
| 87 | > + } | |
| 88 | > + } | |
| 89 | ||
| 90 | Anybody every heard of memcmp or bacmp and BDADDR_ANY? | |
| 91 | ||
| 92 | > + if (not_valid) { | |
| 93 | > + dev_info(info->dev, "Valid bluetooth address not found," | |
| 94 | > + " setting some random\n"); | |
| 95 | > + /* When address is not valid, use some random */ | |
| 96 | > + memcpy(info->bd_addr, nokia_oui, 3); | |
| 97 | > + get_random_bytes(info->bd_addr + 3, 3); | |
| 98 | > + } | |
| 99 | ||
| 100 | ||
| 101 | And why does every single chip firmware does this differently. Seriously, this is a mess. | |
| 102 | ||
| 103 | > +void hci_h4p_parse_fw_event(struct hci_h4p_info *info, struct sk_buff *skb) | |
| 104 | > +{ | |
| 105 | > + switch (info->man_id) { | |
| 106 | > + case H4P_ID_CSR: | |
| 107 | > + hci_h4p_bc4_parse_fw_event(info, skb); | |
| 108 | > + break; | |
| 109 | ... | |
| 110 | > +} | |
| 111 | ||
| 112 | We have proper HCI sync command handling in recent kernels. I really | |
| 113 | do not know why this is hand coded these days. Check how the Intel | |
| 114 | firmware loading inside btusb.c does it. | |
| 115 | ||
| 116 | > +inline u8 hci_h4p_inb(struct hci_h4p_info *info, unsigned int offset) | |
| 117 | > +{ | |
| 118 | > + return __raw_readb(info->uart_base + (offset << 2)); | |
| 119 | > +} | |
| 120 | ||
| 121 | Inline in a *.c file for a non-static function. Makes no sense to me. | |
| 122 | ||
| 123 | > +/** | |
| 124 | > + * struct hci_h4p_platform data - hci_h4p Platform data structure | |
| 125 | > + */ | |
| 126 | > +struct hci_h4p_platform_data { | |
| 127 | ||
| 128 | please have a proper name here. For example | |
| 129 | btnokia_h4p_platform_data. | |
| 130 | ||
| 131 | Please send patches to Greg Kroah-Hartman <greg@kroah.com> and Cc: | |
| 132 | Pavel Machek <pavel@ucw.cz> |