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