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> |