drivers/staging/nokia_h4p/TODO

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