staging/bluetooth: Add hci_h4p driver (91eef3e2) · Commits · e / devices / android_kernel_fairphone_FP5

drivers/staging/Kconfig

+2 −0

Original line number	Original line	Diff line number	Diff line
	@@ -148,4 +148,6 @@ source "drivers/staging/dgap/Kconfig"

	source "drivers/staging/gs_fpgaboot/Kconfig"		source "drivers/staging/gs_fpgaboot/Kconfig"

			source "drivers/staging/nokia_h4p/Kconfig"

	endif # STAGING		endif # STAGING

drivers/staging/Makefile

+1 −0

Original line number	Original line	Diff line number	Diff line
	@@ -66,3 +66,4 @@ obj-$(CONFIG_DGNC) += dgnc/
	obj-$(CONFIG_DGAP) += dgap/		obj-$(CONFIG_DGAP) += dgap/
	obj-$(CONFIG_MTD_SPINAND_MT29F) += mt29f_spinand/		obj-$(CONFIG_MTD_SPINAND_MT29F) += mt29f_spinand/
	obj-$(CONFIG_GS_FPGABOOT) += gs_fpgaboot/		obj-$(CONFIG_GS_FPGABOOT) += gs_fpgaboot/
			obj-$(CONFIG_BT_NOKIA_H4P) += nokia_h4p/

drivers/staging/nokia_h4p/Kconfig

0 → 100644

+9 −0

Original line number	Original line	Diff line number	Diff line
			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).

drivers/staging/nokia_h4p/Makefile

0 → 100644

+6 −0

Original line number	Original line	Diff line number	Diff line

			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__

drivers/staging/nokia_h4p/TODO

0 → 100644

+140 −0

Original line number	Original line	Diff line number	Diff line
			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.

			> +#define FW_NAME_TI1271_LE "ti1273_le.bin"
			> +#define FW_NAME_TI1271 "ti1273.bin"
			> +#define FW_NAME_BCM2048 "bcmfw.bin"
			> +#define FW_NAME_CSR "bc4fw.bin"

			We do these have to be global in a header file. This should be
			confined to the specific firmware part.

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