1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
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>
|
