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>