staging: remove nokia_hp4p driver

source "drivers/staging/gs_fpgaboot/Kconfig"

source "drivers/staging/nokia_h4p/Kconfig"

source "drivers/staging/skein/Kconfig"

source "drivers/staging/unisys/Kconfig"

obj-$(CONFIG_DGAP)			+= dgap/

obj-$(CONFIG_MTD_SPINAND_MT29F)	+= mt29f_spinand/

obj-$(CONFIG_GS_FPGABOOT)	+= gs_fpgaboot/

obj-$(CONFIG_BT_NOKIA_H4P)	+= nokia_h4p/

obj-$(CONFIG_CRYPTO_SKEIN)	+= skein/

obj-$(CONFIG_UNISYSSPAR)	+= unisys/

config BT_NOKIA_H4P

	tristate "HCI driver with H4 Nokia extensions"

	depends on BT && ARCH_OMAP

	help

	  Bluetooth HCI driver with H4 extensions.  This driver provides

	  support for H4+ Bluetooth chip with vendor-specific H4 extensions.

	  Say Y here to compile support for h4 extended devices into the kernel

	  or say M to compile it as module (btnokia_h4p).

obj-$(CONFIG_BT_NOKIA_H4P)		+= btnokia_h4p.o

btnokia_h4p-objs := nokia_core.o nokia_fw.o nokia_uart.o nokia_fw-csr.o \

		nokia_fw-bcm.o nokia_fw-ti1273.o

ccflags-y += -D__CHECK_ENDIAN__

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>