[PATCH v3 0/9] Bluetooth: Add RTL8723BS support

[PATCH v3 0/9] Bluetooth: Add RTL8723BS support

[Hans de Goede]

Hi All,

Here is v2 of my series of patches adding support for RTL8723BS'
bluetooth component through serdev enumeration.

This builds on the great work done on this by Martin Blumenstingl
which makes the btrtl.c code also support firmware+config loading
for the RTL8723BS, as well as Jeremy's Cline work to add ACPI
enumeration support on top of this.

Marcel, I've not seen any feedback on v2 of this series and I believe
this series is ready for merging now, can we get this merged please?

New in v3:
-Fix build error when CONFIG_RTL_BT is not set, reported by Paul Cercueil

New in v2:
-Dropped devicetree specific patches
 (these can be reposted later by someone who can actually test them)
-Add the option to pass a postfix for the config firmware filename,
 to btrtl_initialize() (new patch). For ACPI serdev enumerated
 devies this will be set to the ACPI HID, so the filename becomes e.g.
 rtl_bt/rtl8732bs_config-OBDA8723.bin
 This is done because the config is board specific and we may need a
 different file for e.g. ARM boards

I've tested this series on several Intel boards with a RTL8723BS chip.

Regards,

Hans

[PATCH v3 1/9] Bluetooth: btrtl: add MODULE_FIRMWARE declarations

[Hans de Goede]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

This makes the firmware names show up in modinfo.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/btrtl.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 437f080deaab..c08f63e3bc14 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -528,3 +528,12 @@ MODULE_AUTHOR("Daniel Drake <drake@endlessm.com>");
 MODULE_DESCRIPTION("Bluetooth support for Realtek devices ver " VERSION);
 MODULE_VERSION(VERSION);
 MODULE_LICENSE("GPL");
+MODULE_FIRMWARE("rtl_bt/rtl8723a_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8723b_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8723b_config.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8761a_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8761a_config.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8821a_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8821a_config.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8822b_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8822b_config.bin");
-- 
2.17.1

[PATCH v3 2/9] Bluetooth: btrtl: split the device initialization into smaller parts

[Hans de Goede]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

This prepares the btrtl code so it can be used to initialize Bluetooth
modules connected via UART (these are found for example on the RTL8723BS
and RTL8723DS SDIO chips, which come with an embedded UART Bluetooth
module).

The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART
initialization tools (rtk_hciattach) use the following sequence:
1) send H5 sync pattern (already supported by hci_h5)
2) get LMP version (already supported by btrtl)
3) get ROM version (already supported by btrtl)
4) load the firmware and config for the current chipset (already
   supported by btrtl)
5) read UART settings from the config blob (currently not supported)
6) send UART settings via a vendor command to the device (which changes
   the baudrate of the device and enables or disables flow control
   depending on the config)
7) change the baudrate and flow control settings on the host
8) send the firmware and config blob to the device (already supported by
   btrtl)

The main reason why the initialization has to be split is step #7. This
requires changes to the underlying "bus", which should be kept outside
of the "generic" btrtl driver.
The idea for this split is borrowed from the btbcm driver but adjusted
where needed (the btrtl driver for example needs two blobs: firmware and
config, while the btbcm only needs one).

This also prepares the code for step #5 (parsing the config blob) by
centralizing the code which loads the firmware and config blobs and
storing the result in the new struct btrtl_device_info.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2 (Hans de Goede)
-Fix some minor style issues / checkpatch warnings
---
 drivers/bluetooth/btrtl.c | 280 +++++++++++++++++++++++---------------
 drivers/bluetooth/btrtl.h |  21 +++
 2 files changed, 195 insertions(+), 106 deletions(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index c08f63e3bc14..03b80383e936 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -47,48 +47,96 @@ struct id_table {
 	__u16 lmp_subver;
 	__u16 hci_rev;
 	bool config_needed;
+	bool has_rom_version;
 	char *fw_name;
 	char *cfg_name;
 };
 
+struct btrtl_device_info {
+	const struct id_table *ic_info;
+	u8 rom_version;
+	u8 *fw_data;
+	int fw_len;
+	u8 *cfg_data;
+	int cfg_len;
+};
+
 static const struct id_table ic_id_table[] = {
+	{ IC_MATCH_FL_LMPSUBV, RTL_ROM_LMP_8723A, 0x0,
+	  .config_needed = false,
+	  .has_rom_version = false,
+	  .fw_name = "rtl_bt/rtl8723a_fw.bin",
+	  .cfg_name = NULL },
+
+	{ IC_MATCH_FL_LMPSUBV, RTL_ROM_LMP_3499, 0x0,
+	  .config_needed = false,
+	  .has_rom_version = false,
+	  .fw_name = "rtl_bt/rtl8723a_fw.bin",
+	  .cfg_name = NULL },
+
 	/* 8723B */
 	{ IC_INFO(RTL_ROM_LMP_8723B, 0xb),
 	  .config_needed = false,
+	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8723b_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8723b_config.bin" },
 
 	/* 8723D */
 	{ IC_INFO(RTL_ROM_LMP_8723B, 0xd),
 	  .config_needed = true,
+	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8723d_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8723d_config.bin" },
 
 	/* 8821A */
 	{ IC_INFO(RTL_ROM_LMP_8821A, 0xa),
 	  .config_needed = false,
+	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8821a_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8821a_config.bin" },
 
 	/* 8821C */
 	{ IC_INFO(RTL_ROM_LMP_8821A, 0xc),
 	  .config_needed = false,
+	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8821c_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8821c_config.bin" },
 
 	/* 8761A */
 	{ IC_MATCH_FL_LMPSUBV, RTL_ROM_LMP_8761A, 0x0,
 	  .config_needed = false,
+	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8761a_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8761a_config.bin" },
 
 	/* 8822B */
 	{ IC_INFO(RTL_ROM_LMP_8822B, 0xb),
 	  .config_needed = true,
+	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8822b_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8822b_config.bin" },
 	};
 
+static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ic_id_table); i++) {
+		if ((ic_id_table[i].match_flags & IC_MATCH_FL_LMPSUBV) &&
+		    (ic_id_table[i].lmp_subver != lmp_subver))
+			continue;
+		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIREV) &&
+		    (ic_id_table[i].hci_rev != hci_rev))
+			continue;
+
+		break;
+	}
+	if (i >= ARRAY_SIZE(ic_id_table))
+		return NULL;
+
+	return &ic_id_table[i];
+}
+
 static int rtl_read_rom_version(struct hci_dev *hdev, u8 *version)
 {
 	struct rtl_rom_version_evt *rom_version;
@@ -118,16 +166,16 @@ static int rtl_read_rom_version(struct hci_dev *hdev, u8 *version)
 	return 0;
 }
 
-static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
-				const struct firmware *fw,
+static int rtlbt_parse_firmware(struct hci_dev *hdev,
+				struct btrtl_device_info *btrtl_dev,
 				unsigned char **_buf)
 {
 	const u8 extension_sig[] = { 0x51, 0x04, 0xfd, 0x77 };
 	struct rtl_epatch_header *epatch_info;
 	unsigned char *buf;
-	int i, ret, len;
+	int i, len;
 	size_t min_size;
-	u8 opcode, length, data, rom_version = 0;
+	u8 opcode, length, data;
 	int project_id = -1;
 	const unsigned char *fwptr, *chip_id_base;
 	const unsigned char *patch_length_base, *patch_offset_base;
@@ -146,15 +194,11 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
 		{ RTL_ROM_LMP_8821A, 10 },	/* 8821C */
 	};
 
-	ret = rtl_read_rom_version(hdev, &rom_version);
-	if (ret)
-		return ret;
-
 	min_size = sizeof(struct rtl_epatch_header) + sizeof(extension_sig) + 3;
-	if (fw->size < min_size)
+	if (btrtl_dev->fw_len < min_size)
 		return -EINVAL;
 
-	fwptr = fw->data + fw->size - sizeof(extension_sig);
+	fwptr = btrtl_dev->fw_data + btrtl_dev->fw_len - sizeof(extension_sig);
 	if (memcmp(fwptr, extension_sig, sizeof(extension_sig)) != 0) {
 		BT_ERR("%s: extension section signature mismatch", hdev->name);
 		return -EINVAL;
@@ -166,7 +210,7 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
 	 * Once we have that, we double-check that that project_id is suitable
 	 * for the hardware we are working with.
 	 */
-	while (fwptr >= fw->data + (sizeof(struct rtl_epatch_header) + 3)) {
+	while (fwptr >= btrtl_dev->fw_data + (sizeof(*epatch_info) + 3)) {
 		opcode = *--fwptr;
 		length = *--fwptr;
 		data = *--fwptr;
@@ -206,13 +250,15 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
 		return -EINVAL;
 	}
 
-	if (lmp_subver != project_id_to_lmp_subver[i].lmp_subver) {
+	if (btrtl_dev->ic_info->lmp_subver !=
+				project_id_to_lmp_subver[i].lmp_subver) {
 		BT_ERR("%s: firmware is for %x but this is a %x", hdev->name,
-		       project_id_to_lmp_subver[i].lmp_subver, lmp_subver);
+		       project_id_to_lmp_subver[i].lmp_subver,
+		       btrtl_dev->ic_info->lmp_subver);
 		return -EINVAL;
 	}
 
-	epatch_info = (struct rtl_epatch_header *)fw->data;
+	epatch_info = (struct rtl_epatch_header *)btrtl_dev->fw_data;
 	if (memcmp(epatch_info->signature, RTL_EPATCH_SIGNATURE, 8) != 0) {
 		BT_ERR("%s: bad EPATCH signature", hdev->name);
 		return -EINVAL;
@@ -229,16 +275,16 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
 	 * Find the right patch for this chip.
 	 */
 	min_size += 8 * num_patches;
-	if (fw->size < min_size)
+	if (btrtl_dev->fw_len < min_size)
 		return -EINVAL;
 
-	chip_id_base = fw->data + sizeof(struct rtl_epatch_header);
+	chip_id_base = btrtl_dev->fw_data + sizeof(struct rtl_epatch_header);
 	patch_length_base = chip_id_base + (sizeof(u16) * num_patches);
 	patch_offset_base = patch_length_base + (sizeof(u16) * num_patches);
 	for (i = 0; i < num_patches; i++) {
 		u16 chip_id = get_unaligned_le16(chip_id_base +
 						 (i * sizeof(u16)));
-		if (chip_id == rom_version + 1) {
+		if (chip_id == btrtl_dev->rom_version + 1) {
 			patch_length = get_unaligned_le16(patch_length_base +
 							  (i * sizeof(u16)));
 			patch_offset = get_unaligned_le32(patch_offset_base +
@@ -249,20 +295,21 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
 
 	if (!patch_offset) {
 		BT_ERR("%s: didn't find patch for chip id %d",
-		       hdev->name, rom_version);
+		       hdev->name, btrtl_dev->rom_version);
 		return -EINVAL;
 	}
 
 	BT_DBG("length=%x offset=%x index %d", patch_length, patch_offset, i);
 	min_size = patch_offset + patch_length;
-	if (fw->size < min_size)
+	if (btrtl_dev->fw_len < min_size)
 		return -EINVAL;
 
 	/* Copy the firmware into a new buffer and write the version at
 	 * the end.
 	 */
 	len = patch_length;
-	buf = kmemdup(fw->data + patch_offset, patch_length, GFP_KERNEL);
+	buf = kmemdup(btrtl_dev->fw_data + patch_offset, patch_length,
+		      GFP_KERNEL);
 	if (!buf)
 		return -ENOMEM;
 
@@ -324,7 +371,7 @@ static int rtl_download_firmware(struct hci_dev *hdev,
 	return ret;
 }
 
-static int rtl_load_config(struct hci_dev *hdev, const char *name, u8 **buff)
+static int rtl_load_file(struct hci_dev *hdev, const char *name, u8 **buff)
 {
 	const struct firmware *fw;
 	int ret;
@@ -343,96 +390,37 @@ static int rtl_load_config(struct hci_dev *hdev, const char *name, u8 **buff)
 	return ret;
 }
 
-static int btrtl_setup_rtl8723a(struct hci_dev *hdev)
+static int btrtl_setup_rtl8723a(struct hci_dev *hdev,
+				struct btrtl_device_info *btrtl_dev)
 {
-	const struct firmware *fw;
-	int ret;
-
-	bt_dev_info(hdev, "rtl: loading rtl_bt/rtl8723a_fw.bin");
-	ret = request_firmware(&fw, "rtl_bt/rtl8723a_fw.bin", &hdev->dev);
-	if (ret < 0) {
-		BT_ERR("%s: Failed to load rtl_bt/rtl8723a_fw.bin", hdev->name);
-		return ret;
-	}
-
-	if (fw->size < 8) {
-		ret = -EINVAL;
-		goto out;
-	}
+	if (btrtl_dev->fw_len < 8)
+		return -EINVAL;
 
 	/* Check that the firmware doesn't have the epatch signature
 	 * (which is only for RTL8723B and newer).
 	 */
-	if (!memcmp(fw->data, RTL_EPATCH_SIGNATURE, 8)) {
+	if (!memcmp(btrtl_dev->fw_data, RTL_EPATCH_SIGNATURE, 8)) {
 		BT_ERR("%s: unexpected EPATCH signature!", hdev->name);
-		ret = -EINVAL;
-		goto out;
+		return -EINVAL;
 	}
 
-	ret = rtl_download_firmware(hdev, fw->data, fw->size);
-
-out:
-	release_firmware(fw);
-	return ret;
+	return rtl_download_firmware(hdev, btrtl_dev->fw_data,
+				     btrtl_dev->fw_len);
 }
 
-static int btrtl_setup_rtl8723b(struct hci_dev *hdev, u16 hci_rev,
-				u16 lmp_subver)
+static int btrtl_setup_rtl8723b(struct hci_dev *hdev,
+				struct btrtl_device_info *btrtl_dev)
 {
 	unsigned char *fw_data = NULL;
-	const struct firmware *fw;
 	int ret;
-	int cfg_sz;
-	u8 *cfg_buff = NULL;
 	u8 *tbuff;
-	char *cfg_name = NULL;
-	char *fw_name = NULL;
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(ic_id_table); i++) {
-		if ((ic_id_table[i].match_flags & IC_MATCH_FL_LMPSUBV) &&
-		    (ic_id_table[i].lmp_subver != lmp_subver))
-			continue;
-		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIREV) &&
-		    (ic_id_table[i].hci_rev != hci_rev))
-			continue;
-
-		break;
-	}
-
-	if (i >= ARRAY_SIZE(ic_id_table)) {
-		BT_ERR("%s: unknown IC info, lmp subver %04x, hci rev %04x",
-		       hdev->name, lmp_subver, hci_rev);
-		return -EINVAL;
-	}
-
-	cfg_name = ic_id_table[i].cfg_name;
-
-	if (cfg_name) {
-		cfg_sz = rtl_load_config(hdev, cfg_name, &cfg_buff);
-		if (cfg_sz < 0) {
-			cfg_sz = 0;
-			if (ic_id_table[i].config_needed)
-				BT_ERR("Necessary config file %s not found\n",
-				       cfg_name);
-		}
-	} else
-		cfg_sz = 0;
-
-	fw_name = ic_id_table[i].fw_name;
-	bt_dev_info(hdev, "rtl: loading %s", fw_name);
-	ret = request_firmware(&fw, fw_name, &hdev->dev);
-	if (ret < 0) {
-		BT_ERR("%s: Failed to load %s", hdev->name, fw_name);
-		goto err_req_fw;
-	}
 
-	ret = rtlbt_parse_firmware(hdev, lmp_subver, fw, &fw_data);
+	ret = rtlbt_parse_firmware(hdev, btrtl_dev, &fw_data);
 	if (ret < 0)
 		goto out;
 
-	if (cfg_sz) {
-		tbuff = kzalloc(ret + cfg_sz, GFP_KERNEL);
+	if (btrtl_dev->cfg_len > 0) {
+		tbuff = kzalloc(ret + btrtl_dev->cfg_len, GFP_KERNEL);
 		if (!tbuff) {
 			ret = -ENOMEM;
 			goto out;
@@ -441,22 +429,18 @@ static int btrtl_setup_rtl8723b(struct hci_dev *hdev, u16 hci_rev,
 		memcpy(tbuff, fw_data, ret);
 		kfree(fw_data);
 
-		memcpy(tbuff + ret, cfg_buff, cfg_sz);
-		ret += cfg_sz;
+		memcpy(tbuff + ret, btrtl_dev->cfg_data, btrtl_dev->cfg_len);
+		ret += btrtl_dev->cfg_len;
 
 		fw_data = tbuff;
 	}
 
-	bt_dev_info(hdev, "cfg_sz %d, total size %d", cfg_sz, ret);
+	bt_dev_info(hdev, "cfg_sz %d, total size %d", btrtl_dev->cfg_len, ret);
 
 	ret = rtl_download_firmware(hdev, fw_data, ret);
 
 out:
-	release_firmware(fw);
 	kfree(fw_data);
-err_req_fw:
-	if (cfg_sz)
-		kfree(cfg_buff);
 	return ret;
 }
 
@@ -482,15 +466,33 @@ static struct sk_buff *btrtl_read_local_version(struct hci_dev *hdev)
 	return skb;
 }
 
-int btrtl_setup_realtek(struct hci_dev *hdev)
+void btrtl_free(struct btrtl_device_info *btrtl_dev)
+{
+	kfree(btrtl_dev->fw_data);
+	kfree(btrtl_dev->cfg_data);
+	kfree(btrtl_dev);
+}
+EXPORT_SYMBOL_GPL(btrtl_free);
+
+struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
 {
+	struct btrtl_device_info *btrtl_dev;
 	struct sk_buff *skb;
 	struct hci_rp_read_local_version *resp;
 	u16 hci_rev, lmp_subver;
+	int ret;
+
+	btrtl_dev = kzalloc(sizeof(*btrtl_dev), GFP_KERNEL);
+	if (!btrtl_dev) {
+		ret = -ENOMEM;
+		goto err_alloc;
+	}
 
 	skb = btrtl_read_local_version(hdev);
-	if (IS_ERR(skb))
-		return -PTR_ERR(skb);
+	if (IS_ERR(skb)) {
+		ret = PTR_ERR(skb);
+		goto err_free;
+	}
 
 	resp = (struct hci_rp_read_local_version *)skb->data;
 	bt_dev_info(hdev, "rtl: examining hci_ver=%02x hci_rev=%04x "
@@ -502,26 +504,92 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
 	lmp_subver = le16_to_cpu(resp->lmp_subver);
 	kfree_skb(skb);
 
+	btrtl_dev->ic_info = btrtl_match_ic(lmp_subver, hci_rev);
+	if (!btrtl_dev->ic_info) {
+		bt_dev_err(hdev, "rtl: unknown IC info, lmp subver %04x, hci rev %04x",
+			   lmp_subver, hci_rev);
+		ret = -EINVAL;
+		goto err_free;
+	}
+
+	if (btrtl_dev->ic_info->has_rom_version) {
+		ret = rtl_read_rom_version(hdev, &btrtl_dev->rom_version);
+		if (ret)
+			goto err_free;
+	}
+
+	btrtl_dev->fw_len = rtl_load_file(hdev, btrtl_dev->ic_info->fw_name,
+					  &btrtl_dev->fw_data);
+	if (btrtl_dev->fw_len < 0) {
+		bt_dev_err(hdev, "firmware file %s not found\n",
+			   btrtl_dev->ic_info->fw_name);
+		ret = btrtl_dev->fw_len;
+		goto err_free;
+	}
+
+	if (btrtl_dev->ic_info->cfg_name) {
+		btrtl_dev->cfg_len = rtl_load_file(hdev,
+						   btrtl_dev->ic_info->cfg_name,
+						   &btrtl_dev->cfg_data);
+		if (btrtl_dev->ic_info->config_needed &&
+		    btrtl_dev->cfg_len <= 0) {
+			bt_dev_err(hdev,
+				   "mandatory config file %s not found\n",
+				   btrtl_dev->ic_info->cfg_name);
+			ret = btrtl_dev->cfg_len;
+			goto err_free;
+		}
+	}
+
+	return btrtl_dev;
+
+err_free:
+	btrtl_free(btrtl_dev);
+err_alloc:
+	return ERR_PTR(ret);
+}
+EXPORT_SYMBOL_GPL(btrtl_initialize);
+
+int btrtl_download_firmware(struct hci_dev *hdev,
+			    struct btrtl_device_info *btrtl_dev)
+{
 	/* Match a set of subver values that correspond to stock firmware,
 	 * which is not compatible with standard btusb.
 	 * If matched, upload an alternative firmware that does conform to
 	 * standard btusb. Once that firmware is uploaded, the subver changes
 	 * to a different value.
 	 */
-	switch (lmp_subver) {
+	switch (btrtl_dev->ic_info->lmp_subver) {
 	case RTL_ROM_LMP_8723A:
 	case RTL_ROM_LMP_3499:
-		return btrtl_setup_rtl8723a(hdev);
+		return btrtl_setup_rtl8723a(hdev, btrtl_dev);
 	case RTL_ROM_LMP_8723B:
 	case RTL_ROM_LMP_8821A:
 	case RTL_ROM_LMP_8761A:
 	case RTL_ROM_LMP_8822B:
-		return btrtl_setup_rtl8723b(hdev, hci_rev, lmp_subver);
+		return btrtl_setup_rtl8723b(hdev, btrtl_dev);
 	default:
 		bt_dev_info(hdev, "rtl: assuming no firmware upload needed");
 		return 0;
 	}
 }
+EXPORT_SYMBOL_GPL(btrtl_download_firmware);
+
+int btrtl_setup_realtek(struct hci_dev *hdev)
+{
+	struct btrtl_device_info *btrtl_dev;
+	int ret;
+
+	btrtl_dev = btrtl_initialize(hdev);
+	if (IS_ERR(btrtl_dev))
+		return PTR_ERR(btrtl_dev);
+
+	ret = btrtl_download_firmware(hdev, btrtl_dev);
+
+	btrtl_free(btrtl_dev);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(btrtl_setup_realtek);
 
 MODULE_AUTHOR("Daniel Drake <drake@endlessm.com>");
diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
index 38ffe4890cd1..21c28dcbe5e0 100644
--- a/drivers/bluetooth/btrtl.h
+++ b/drivers/bluetooth/btrtl.h
@@ -17,6 +17,8 @@
 
 #define RTL_FRAG_LEN 252
 
+struct btrtl_device_info;
+
 struct rtl_download_cmd {
 	__u8 index;
 	__u8 data[RTL_FRAG_LEN];
@@ -40,10 +42,29 @@ struct rtl_epatch_header {
 
 #if IS_ENABLED(CONFIG_BT_RTL)
 
+struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev);
+void btrtl_free(struct btrtl_device_info *btrtl_dev);
+int btrtl_download_firmware(struct hci_dev *hdev,
+			    struct btrtl_device_info *btrtl_dev);
 int btrtl_setup_realtek(struct hci_dev *hdev);
 
 #else
 
+static inline struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
+static inline void btrtl_free(struct btrtl_device_info *btrtl_dev)
+{
+}
+
+static inline int btrtl_download_firmware(struct hci_dev *hdev,
+					  struct btrtl_device_info *btrtl_dev)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int btrtl_setup_realtek(struct hci_dev *hdev)
 {
 	return -EOPNOTSUPP;
-- 
2.17.1

Re: [PATCH v3 2/9] Bluetooth: btrtl: split the device initialization into smaller parts

[Marcel Holtmann]

Hi Hans,

> This prepares the btrtl code so it can be used to initialize Bluetooth
> modules connected via UART (these are found for example on the RTL8723BS
> and RTL8723DS SDIO chips, which come with an embedded UART Bluetooth
> module).
> 
> The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART
> initialization tools (rtk_hciattach) use the following sequence:
> 1) send H5 sync pattern (already supported by hci_h5)
> 2) get LMP version (already supported by btrtl)
> 3) get ROM version (already supported by btrtl)
> 4) load the firmware and config for the current chipset (already
>   supported by btrtl)
> 5) read UART settings from the config blob (currently not supported)
> 6) send UART settings via a vendor command to the device (which changes
>   the baudrate of the device and enables or disables flow control
>   depending on the config)
> 7) change the baudrate and flow control settings on the host
> 8) send the firmware and config blob to the device (already supported by
>   btrtl)
> 
> The main reason why the initialization has to be split is step #7. This
> requires changes to the underlying "bus", which should be kept outside
> of the "generic" btrtl driver.
> The idea for this split is borrowed from the btbcm driver but adjusted
> where needed (the btrtl driver for example needs two blobs: firmware and
> config, while the btbcm only needs one).
> 
> This also prepares the code for step #5 (parsing the config blob) by
> centralizing the code which loads the firmware and config blobs and
> storing the result in the new struct btrtl_device_info.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Jeremy Cline <jeremy@jcline.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2 (Hans de Goede)
> -Fix some minor style issues / checkpatch warnings
> ---
> drivers/bluetooth/btrtl.c | 280 +++++++++++++++++++++++---------------
> drivers/bluetooth/btrtl.h |  21 +++
> 2 files changed, 195 insertions(+), 106 deletions(-)
> 
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index c08f63e3bc14..03b80383e936 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -47,48 +47,96 @@ struct id_table {
> 	__u16 lmp_subver;
> 	__u16 hci_rev;
> 	bool config_needed;
> +	bool has_rom_version;
> 	char *fw_name;
> 	char *cfg_name;
> };
> 
> +struct btrtl_device_info {
> +	const struct id_table *ic_info;
> +	u8 rom_version;
> +	u8 *fw_data;
> +	int fw_len;
> +	u8 *cfg_data;
> +	int cfg_len;
> +};
> +
> static const struct id_table ic_id_table[] = {
> +	{ IC_MATCH_FL_LMPSUBV, RTL_ROM_LMP_8723A, 0x0,
> +	  .config_needed = false,
> +	  .has_rom_version = false,
> +	  .fw_name = "rtl_bt/rtl8723a_fw.bin",
> +	  .cfg_name = NULL },
> +
> +	{ IC_MATCH_FL_LMPSUBV, RTL_ROM_LMP_3499, 0x0,
> +	  .config_needed = false,
> +	  .has_rom_version = false,
> +	  .fw_name = "rtl_bt/rtl8723a_fw.bin",
> +	  .cfg_name = NULL },
> +
> 	/* 8723B */
> 	{ IC_INFO(RTL_ROM_LMP_8723B, 0xb),
> 	  .config_needed = false,
> +	  .has_rom_version = true,
> 	  .fw_name  = "rtl_bt/rtl8723b_fw.bin",
> 	  .cfg_name = "rtl_bt/rtl8723b_config.bin" },
> 
> 	/* 8723D */
> 	{ IC_INFO(RTL_ROM_LMP_8723B, 0xd),
> 	  .config_needed = true,
> +	  .has_rom_version = true,
> 	  .fw_name  = "rtl_bt/rtl8723d_fw.bin",
> 	  .cfg_name = "rtl_bt/rtl8723d_config.bin" },
> 
> 	/* 8821A */
> 	{ IC_INFO(RTL_ROM_LMP_8821A, 0xa),
> 	  .config_needed = false,
> +	  .has_rom_version = true,
> 	  .fw_name  = "rtl_bt/rtl8821a_fw.bin",
> 	  .cfg_name = "rtl_bt/rtl8821a_config.bin" },
> 
> 	/* 8821C */
> 	{ IC_INFO(RTL_ROM_LMP_8821A, 0xc),
> 	  .config_needed = false,
> +	  .has_rom_version = true,
> 	  .fw_name  = "rtl_bt/rtl8821c_fw.bin",
> 	  .cfg_name = "rtl_bt/rtl8821c_config.bin" },
> 
> 	/* 8761A */
> 	{ IC_MATCH_FL_LMPSUBV, RTL_ROM_LMP_8761A, 0x0,
> 	  .config_needed = false,
> +	  .has_rom_version = true,
> 	  .fw_name  = "rtl_bt/rtl8761a_fw.bin",
> 	  .cfg_name = "rtl_bt/rtl8761a_config.bin" },
> 
> 	/* 8822B */
> 	{ IC_INFO(RTL_ROM_LMP_8822B, 0xb),
> 	  .config_needed = true,
> +	  .has_rom_version = true,
> 	  .fw_name  = "rtl_bt/rtl8822b_fw.bin",
> 	  .cfg_name = "rtl_bt/rtl8822b_config.bin" },
> 	};
> 
> +static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(ic_id_table); i++) {
> +		if ((ic_id_table[i].match_flags & IC_MATCH_FL_LMPSUBV) &&
> +		    (ic_id_table[i].lmp_subver != lmp_subver))
> +			continue;
> +		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIREV) &&
> +		    (ic_id_table[i].hci_rev != hci_rev))
> +			continue;
> +
> +		break;
> +	}
> +	if (i >= ARRAY_SIZE(ic_id_table))
> +		return NULL;
> +
> +	return &ic_id_table[i];
> +}
> +
> static int rtl_read_rom_version(struct hci_dev *hdev, u8 *version)
> {
> 	struct rtl_rom_version_evt *rom_version;
> @@ -118,16 +166,16 @@ static int rtl_read_rom_version(struct hci_dev *hdev, u8 *version)
> 	return 0;
> }
> 
> -static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
> -				const struct firmware *fw,
> +static int rtlbt_parse_firmware(struct hci_dev *hdev,
> +				struct btrtl_device_info *btrtl_dev,
> 				unsigned char **_buf)
> {
> 	const u8 extension_sig[] = { 0x51, 0x04, 0xfd, 0x77 };
> 	struct rtl_epatch_header *epatch_info;
> 	unsigned char *buf;
> -	int i, ret, len;
> +	int i, len;
> 	size_t min_size;
> -	u8 opcode, length, data, rom_version = 0;
> +	u8 opcode, length, data;
> 	int project_id = -1;
> 	const unsigned char *fwptr, *chip_id_base;
> 	const unsigned char *patch_length_base, *patch_offset_base;
> @@ -146,15 +194,11 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
> 		{ RTL_ROM_LMP_8821A, 10 },	/* 8821C */
> 	};
> 
> -	ret = rtl_read_rom_version(hdev, &rom_version);
> -	if (ret)
> -		return ret;
> -
> 	min_size = sizeof(struct rtl_epatch_header) + sizeof(extension_sig) + 3;
> -	if (fw->size < min_size)
> +	if (btrtl_dev->fw_len < min_size)
> 		return -EINVAL;
> 
> -	fwptr = fw->data + fw->size - sizeof(extension_sig);
> +	fwptr = btrtl_dev->fw_data + btrtl_dev->fw_len - sizeof(extension_sig);
> 	if (memcmp(fwptr, extension_sig, sizeof(extension_sig)) != 0) {
> 		BT_ERR("%s: extension section signature mismatch", hdev->name);
> 		return -EINVAL;

we really need to move towards bt_dev_err here and stop duplicating the hdev->name parameter. I have fixed some of the driver, but it seems that I sill missed a few. So please get this fixed as well.

> @@ -166,7 +210,7 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
> 	 * Once we have that, we double-check that that project_id is suitable
> 	 * for the hardware we are working with.
> 	 */
> -	while (fwptr >= fw->data + (sizeof(struct rtl_epatch_header) + 3)) {
> +	while (fwptr >= btrtl_dev->fw_data + (sizeof(*epatch_info) + 3)) {
> 		opcode = *--fwptr;
> 		length = *--fwptr;
> 		data = *--fwptr;
> @@ -206,13 +250,15 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
> 		return -EINVAL;
> 	}
> 
> -	if (lmp_subver != project_id_to_lmp_subver[i].lmp_subver) {
> +	if (btrtl_dev->ic_info->lmp_subver !=
> +				project_id_to_lmp_subver[i].lmp_subver) {
> 		BT_ERR("%s: firmware is for %x but this is a %x", hdev->name,
> -		       project_id_to_lmp_subver[i].lmp_subver, lmp_subver);
> +		       project_id_to_lmp_subver[i].lmp_subver,
> +		       btrtl_dev->ic_info->lmp_subver);
> 		return -EINVAL;
> 	}
> 
> -	epatch_info = (struct rtl_epatch_header *)fw->data;
> +	epatch_info = (struct rtl_epatch_header *)btrtl_dev->fw_data;
> 	if (memcmp(epatch_info->signature, RTL_EPATCH_SIGNATURE, 8) != 0) {
> 		BT_ERR("%s: bad EPATCH signature", hdev->name);
> 		return -EINVAL;
> @@ -229,16 +275,16 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
> 	 * Find the right patch for this chip.
> 	 */
> 	min_size += 8 * num_patches;
> -	if (fw->size < min_size)
> +	if (btrtl_dev->fw_len < min_size)
> 		return -EINVAL;
> 
> -	chip_id_base = fw->data + sizeof(struct rtl_epatch_header);
> +	chip_id_base = btrtl_dev->fw_data + sizeof(struct rtl_epatch_header);
> 	patch_length_base = chip_id_base + (sizeof(u16) * num_patches);
> 	patch_offset_base = patch_length_base + (sizeof(u16) * num_patches);
> 	for (i = 0; i < num_patches; i++) {
> 		u16 chip_id = get_unaligned_le16(chip_id_base +
> 						 (i * sizeof(u16)));
> -		if (chip_id == rom_version + 1) {
> +		if (chip_id == btrtl_dev->rom_version + 1) {
> 			patch_length = get_unaligned_le16(patch_length_base +
> 							  (i * sizeof(u16)));
> 			patch_offset = get_unaligned_le32(patch_offset_base +
> @@ -249,20 +295,21 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev, u16 lmp_subver,
> 
> 	if (!patch_offset) {
> 		BT_ERR("%s: didn't find patch for chip id %d",
> -		       hdev->name, rom_version);
> +		       hdev->name, btrtl_dev->rom_version);
> 		return -EINVAL;
> 	}
> 
> 	BT_DBG("length=%x offset=%x index %d", patch_length, patch_offset, i);
> 	min_size = patch_offset + patch_length;
> -	if (fw->size < min_size)
> +	if (btrtl_dev->fw_len < min_size)
> 		return -EINVAL;
> 
> 	/* Copy the firmware into a new buffer and write the version at
> 	 * the end.
> 	 */
> 	len = patch_length;
> -	buf = kmemdup(fw->data + patch_offset, patch_length, GFP_KERNEL);
> +	buf = kmemdup(btrtl_dev->fw_data + patch_offset, patch_length,
> +		      GFP_KERNEL);
> 	if (!buf)
> 		return -ENOMEM;
> 
> @@ -324,7 +371,7 @@ static int rtl_download_firmware(struct hci_dev *hdev,
> 	return ret;
> }
> 
> -static int rtl_load_config(struct hci_dev *hdev, const char *name, u8 **buff)
> +static int rtl_load_file(struct hci_dev *hdev, const char *name, u8 **buff)
> {
> 	const struct firmware *fw;
> 	int ret;
> @@ -343,96 +390,37 @@ static int rtl_load_config(struct hci_dev *hdev, const char *name, u8 **buff)
> 	return ret;
> }
> 
> -static int btrtl_setup_rtl8723a(struct hci_dev *hdev)
> +static int btrtl_setup_rtl8723a(struct hci_dev *hdev,
> +				struct btrtl_device_info *btrtl_dev)
> {
> -	const struct firmware *fw;
> -	int ret;
> -
> -	bt_dev_info(hdev, "rtl: loading rtl_bt/rtl8723a_fw.bin");
> -	ret = request_firmware(&fw, "rtl_bt/rtl8723a_fw.bin", &hdev->dev);
> -	if (ret < 0) {
> -		BT_ERR("%s: Failed to load rtl_bt/rtl8723a_fw.bin", hdev->name);
> -		return ret;
> -	}
> -
> -	if (fw->size < 8) {
> -		ret = -EINVAL;
> -		goto out;
> -	}
> +	if (btrtl_dev->fw_len < 8)
> +		return -EINVAL;
> 
> 	/* Check that the firmware doesn't have the epatch signature
> 	 * (which is only for RTL8723B and newer).
> 	 */
> -	if (!memcmp(fw->data, RTL_EPATCH_SIGNATURE, 8)) {
> +	if (!memcmp(btrtl_dev->fw_data, RTL_EPATCH_SIGNATURE, 8)) {
> 		BT_ERR("%s: unexpected EPATCH signature!", hdev->name);
> -		ret = -EINVAL;
> -		goto out;
> +		return -EINVAL;
> 	}
> 
> -	ret = rtl_download_firmware(hdev, fw->data, fw->size);
> -
> -out:
> -	release_firmware(fw);
> -	return ret;
> +	return rtl_download_firmware(hdev, btrtl_dev->fw_data,
> +				     btrtl_dev->fw_len);
> }
> 
> -static int btrtl_setup_rtl8723b(struct hci_dev *hdev, u16 hci_rev,
> -				u16 lmp_subver)
> +static int btrtl_setup_rtl8723b(struct hci_dev *hdev,
> +				struct btrtl_device_info *btrtl_dev)
> {
> 	unsigned char *fw_data = NULL;
> -	const struct firmware *fw;
> 	int ret;
> -	int cfg_sz;
> -	u8 *cfg_buff = NULL;
> 	u8 *tbuff;
> -	char *cfg_name = NULL;
> -	char *fw_name = NULL;
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(ic_id_table); i++) {
> -		if ((ic_id_table[i].match_flags & IC_MATCH_FL_LMPSUBV) &&
> -		    (ic_id_table[i].lmp_subver != lmp_subver))
> -			continue;
> -		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIREV) &&
> -		    (ic_id_table[i].hci_rev != hci_rev))
> -			continue;
> -
> -		break;
> -	}
> -
> -	if (i >= ARRAY_SIZE(ic_id_table)) {
> -		BT_ERR("%s: unknown IC info, lmp subver %04x, hci rev %04x",
> -		       hdev->name, lmp_subver, hci_rev);
> -		return -EINVAL;
> -	}
> -
> -	cfg_name = ic_id_table[i].cfg_name;
> -
> -	if (cfg_name) {
> -		cfg_sz = rtl_load_config(hdev, cfg_name, &cfg_buff);
> -		if (cfg_sz < 0) {
> -			cfg_sz = 0;
> -			if (ic_id_table[i].config_needed)
> -				BT_ERR("Necessary config file %s not found\n",
> -				       cfg_name);
> -		}
> -	} else
> -		cfg_sz = 0;
> -
> -	fw_name = ic_id_table[i].fw_name;
> -	bt_dev_info(hdev, "rtl: loading %s", fw_name);
> -	ret = request_firmware(&fw, fw_name, &hdev->dev);
> -	if (ret < 0) {
> -		BT_ERR("%s: Failed to load %s", hdev->name, fw_name);
> -		goto err_req_fw;
> -	}
> 
> -	ret = rtlbt_parse_firmware(hdev, lmp_subver, fw, &fw_data);
> +	ret = rtlbt_parse_firmware(hdev, btrtl_dev, &fw_data);
> 	if (ret < 0)
> 		goto out;
> 
> -	if (cfg_sz) {
> -		tbuff = kzalloc(ret + cfg_sz, GFP_KERNEL);
> +	if (btrtl_dev->cfg_len > 0) {
> +		tbuff = kzalloc(ret + btrtl_dev->cfg_len, GFP_KERNEL);
> 		if (!tbuff) {
> 			ret = -ENOMEM;
> 			goto out;
> @@ -441,22 +429,18 @@ static int btrtl_setup_rtl8723b(struct hci_dev *hdev, u16 hci_rev,
> 		memcpy(tbuff, fw_data, ret);
> 		kfree(fw_data);
> 
> -		memcpy(tbuff + ret, cfg_buff, cfg_sz);
> -		ret += cfg_sz;
> +		memcpy(tbuff + ret, btrtl_dev->cfg_data, btrtl_dev->cfg_len);
> +		ret += btrtl_dev->cfg_len;
> 
> 		fw_data = tbuff;
> 	}
> 
> -	bt_dev_info(hdev, "cfg_sz %d, total size %d", cfg_sz, ret);
> +	bt_dev_info(hdev, "cfg_sz %d, total size %d", btrtl_dev->cfg_len, ret);
> 
> 	ret = rtl_download_firmware(hdev, fw_data, ret);
> 
> out:
> -	release_firmware(fw);
> 	kfree(fw_data);
> -err_req_fw:
> -	if (cfg_sz)
> -		kfree(cfg_buff);
> 	return ret;
> }
> 
> @@ -482,15 +466,33 @@ static struct sk_buff *btrtl_read_local_version(struct hci_dev *hdev)
> 	return skb;
> }
> 
> -int btrtl_setup_realtek(struct hci_dev *hdev)
> +void btrtl_free(struct btrtl_device_info *btrtl_dev)
> +{
> +	kfree(btrtl_dev->fw_data);
> +	kfree(btrtl_dev->cfg_data);
> +	kfree(btrtl_dev);
> +}
> +EXPORT_SYMBOL_GPL(btrtl_free);
> +
> +struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
> {
> +	struct btrtl_device_info *btrtl_dev;
> 	struct sk_buff *skb;
> 	struct hci_rp_read_local_version *resp;
> 	u16 hci_rev, lmp_subver;
> +	int ret;
> +
> +	btrtl_dev = kzalloc(sizeof(*btrtl_dev), GFP_KERNEL);
> +	if (!btrtl_dev) {
> +		ret = -ENOMEM;
> +		goto err_alloc;
> +	}
> 
> 	skb = btrtl_read_local_version(hdev);
> -	if (IS_ERR(skb))
> -		return -PTR_ERR(skb);
> +	if (IS_ERR(skb)) {
> +		ret = PTR_ERR(skb);
> +		goto err_free;
> +	}
> 
> 	resp = (struct hci_rp_read_local_version *)skb->data;
> 	bt_dev_info(hdev, "rtl: examining hci_ver=%02x hci_rev=%04x "
> @@ -502,26 +504,92 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
> 	lmp_subver = le16_to_cpu(resp->lmp_subver);
> 	kfree_skb(skb);
> 
> +	btrtl_dev->ic_info = btrtl_match_ic(lmp_subver, hci_rev);
> +	if (!btrtl_dev->ic_info) {
> +		bt_dev_err(hdev, "rtl: unknown IC info, lmp subver %04x, hci rev %04x",
> +			   lmp_subver, hci_rev);

So I know that I have doing the BCM: prefix for Broadcom and generally I am fine having this since it helps when people report errors to spot what is vendor specific error and what is not. However I think it is time to unify this a bit. So actually I would use RTL: as prefix here instead of all lower-case, but we can also change BCM: to bcm: if people are more like it. Then it would also become intel: for the other ones. Might be more kernel stylish to keep it all lower-case.

However the most important part is that we create a rtl_dev_err macro for this. I don't want this rtl: string splattered all over the code. And yes, the Broadcom code needs a bcm_dev_err etc.

As a bit of background, I want these all feed through common bt_dev_* and streamlined via proper macros so that in the long run, I can also feed these btmon. If they can go additionally through the Bluetooth monitor socket, then a trace file will contain the HCI traces and the kernel logging information so we can easily figure out what went wrong where. We can already do this via bluetoothd which can feed errors etc. back into the Bluetooth monitor, but we also need that for driver specific code.

> +		ret = -EINVAL;
> +		goto err_free;
> +	}
> +
> +	if (btrtl_dev->ic_info->has_rom_version) {
> +		ret = rtl_read_rom_version(hdev, &btrtl_dev->rom_version);
> +		if (ret)
> +			goto err_free;
> +	}
> +
> +	btrtl_dev->fw_len = rtl_load_file(hdev, btrtl_dev->ic_info->fw_name,
> +					  &btrtl_dev->fw_data);
> +	if (btrtl_dev->fw_len < 0) {
> +		bt_dev_err(hdev, "firmware file %s not found\n",
> +			   btrtl_dev->ic_info->fw_name);
> +		ret = btrtl_dev->fw_len;
> +		goto err_free;
> +	}
> +
> +	if (btrtl_dev->ic_info->cfg_name) {
> +		btrtl_dev->cfg_len = rtl_load_file(hdev,
> +						   btrtl_dev->ic_info->cfg_name,
> +						   &btrtl_dev->cfg_data);
> +		if (btrtl_dev->ic_info->config_needed &&
> +		    btrtl_dev->cfg_len <= 0) {
> +			bt_dev_err(hdev,
> +				   "mandatory config file %s not found\n",
> +				   btrtl_dev->ic_info->cfg_name);
> +			ret = btrtl_dev->cfg_len;
> +			goto err_free;
> +		}
> +	}
> +
> +	return btrtl_dev;
> +
> +err_free:
> +	btrtl_free(btrtl_dev);
> +err_alloc:
> +	return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(btrtl_initialize);
> +
> +int btrtl_download_firmware(struct hci_dev *hdev,
> +			    struct btrtl_device_info *btrtl_dev)
> +{
> 	/* Match a set of subver values that correspond to stock firmware,
> 	 * which is not compatible with standard btusb.
> 	 * If matched, upload an alternative firmware that does conform to
> 	 * standard btusb. Once that firmware is uploaded, the subver changes
> 	 * to a different value.
> 	 */
> -	switch (lmp_subver) {
> +	switch (btrtl_dev->ic_info->lmp_subver) {
> 	case RTL_ROM_LMP_8723A:
> 	case RTL_ROM_LMP_3499:
> -		return btrtl_setup_rtl8723a(hdev);
> +		return btrtl_setup_rtl8723a(hdev, btrtl_dev);
> 	case RTL_ROM_LMP_8723B:
> 	case RTL_ROM_LMP_8821A:
> 	case RTL_ROM_LMP_8761A:
> 	case RTL_ROM_LMP_8822B:
> -		return btrtl_setup_rtl8723b(hdev, hci_rev, lmp_subver);
> +		return btrtl_setup_rtl8723b(hdev, btrtl_dev);
> 	default:
> 		bt_dev_info(hdev, "rtl: assuming no firmware upload needed");
> 		return 0;
> 	}
> }
> +EXPORT_SYMBOL_GPL(btrtl_download_firmware);
> +
> +int btrtl_setup_realtek(struct hci_dev *hdev)
> +{
> +	struct btrtl_device_info *btrtl_dev;
> +	int ret;
> +
> +	btrtl_dev = btrtl_initialize(hdev);
> +	if (IS_ERR(btrtl_dev))
> +		return PTR_ERR(btrtl_dev);
> +
> +	ret = btrtl_download_firmware(hdev, btrtl_dev);
> +
> +	btrtl_free(btrtl_dev);
> +
> +	return ret;
> +}
> EXPORT_SYMBOL_GPL(btrtl_setup_realtek);
> 
> MODULE_AUTHOR("Daniel Drake <drake@endlessm.com>");
> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
> index 38ffe4890cd1..21c28dcbe5e0 100644
> --- a/drivers/bluetooth/btrtl.h
> +++ b/drivers/bluetooth/btrtl.h
> @@ -17,6 +17,8 @@
> 
> #define RTL_FRAG_LEN 252
> 
> +struct btrtl_device_info;
> +
> struct rtl_download_cmd {
> 	__u8 index;
> 	__u8 data[RTL_FRAG_LEN];
> @@ -40,10 +42,29 @@ struct rtl_epatch_header {
> 
> #if IS_ENABLED(CONFIG_BT_RTL)
> 
> +struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev);
> +void btrtl_free(struct btrtl_device_info *btrtl_dev);
> +int btrtl_download_firmware(struct hci_dev *hdev,
> +			    struct btrtl_device_info *btrtl_dev);
> int btrtl_setup_realtek(struct hci_dev *hdev);
> 
> #else
> 
> +static inline struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
> +{
> +	return ERR_PTR(-EOPNOTSUPP);
> +}
> +
> +static inline void btrtl_free(struct btrtl_device_info *btrtl_dev)
> +{
> +}
> +
> +static inline int btrtl_download_firmware(struct hci_dev *hdev,
> +					  struct btrtl_device_info *btrtl_dev)
> +{
> +	return -EOPNOTSUPP;
> +}
> +
> static inline int btrtl_setup_realtek(struct hci_dev *hdev)
> {
> 	return -EOPNOTSUPP;

Regards

Marcel

[PATCH v3 3/9] Bluetooth: btrtl: add support for retrieving the UART settings

[Hans de Goede]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

The UART settings are embedded in the config blob. This has to be parsed
to successfully initialize the Bluetooth part of the RTL8723BS (which is
an SDIO chip, but the Bluetooth part is connected via UART).

The Realtek "rtl8723bs_bt" and "rtl8723ds_bt" userspace Bluetooth UART
initialization tools (rtk_hciattach) use the following sequence:
- send H5 sync pattern (already supported by hci_h5)
- get LMP version (already supported by btrtl)
- get ROM version (already supported by btrtl)
- load the firmware and config for the current chipset (already
  supported by btrtl)
- read UART settings from the config blob (part of this patch)
- send UART settings via a vendor command to the device (which changes
  the baudrate of the device and enables or disables flow control
  depending on the config)
- change the baudrate and flow control settings on the host
- send the firmware and config blob to the device (already supported by
  btrtl)

Sending the last firmware and config blob download command
(rtl_download_cmd) fails if the UART settings are not updated
beforehand. This is presumably because the device applies the config
right after the firmware and config blob download - which means that at
this point the host is using different UART settings than the device
(which will obviously result in non-working communication).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Make btrtl_convert_baudrate() static
---
 drivers/bluetooth/btrtl.c | 112 ++++++++++++++++++++++++++++++++++++++
 drivers/bluetooth/btrtl.h |  25 +++++++++
 2 files changed, 137 insertions(+)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 03b80383e936..6c0d88d23d6d 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -34,6 +34,7 @@
 #define RTL_ROM_LMP_8821A	0x8821
 #define RTL_ROM_LMP_8761A	0x8761
 #define RTL_ROM_LMP_8822B	0x8822
+#define RTL_CONFIG_MAGIC	0x8723ab55
 
 #define IC_MATCH_FL_LMPSUBV	(1 << 0)
 #define IC_MATCH_FL_HCIREV	(1 << 1)
@@ -592,6 +593,117 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
 }
 EXPORT_SYMBOL_GPL(btrtl_setup_realtek);
 
+static unsigned int btrtl_convert_baudrate(u32 device_baudrate)
+{
+	switch (device_baudrate) {
+	case 0x0252a00a:
+		return 230400;
+
+	case 0x05f75004:
+		return 921600;
+
+	case 0x00005004:
+		return 1000000;
+
+	case 0x04928002:
+	case 0x01128002:
+		return 1500000;
+
+	case 0x00005002:
+		return 2000000;
+
+	case 0x0000b001:
+		return 2500000;
+
+	case 0x04928001:
+		return 3000000;
+
+	case 0x052a6001:
+		return 3500000;
+
+	case 0x00005001:
+		return 4000000;
+
+	case 0x0252c014:
+	default:
+		return 115200;
+	}
+}
+
+int btrtl_get_uart_settings(struct hci_dev *hdev,
+			    struct btrtl_device_info *btrtl_dev,
+			    unsigned int *controller_baudrate,
+			    u32 *device_baudrate, bool *flow_control)
+{
+	struct rtl_vendor_config *config;
+	struct rtl_vendor_config_entry *entry;
+	int i, total_data_len;
+	bool found = false;
+
+	total_data_len = btrtl_dev->cfg_len - sizeof(*config);
+	if (total_data_len <= 0) {
+		bt_dev_warn(hdev, "rtl: no config loaded");
+		return -EINVAL;
+	}
+
+	config = (struct rtl_vendor_config *)btrtl_dev->cfg_data;
+	if (le32_to_cpu(config->signature) != RTL_CONFIG_MAGIC) {
+		bt_dev_err(hdev, "rtl: invalid config magic");
+		return -EINVAL;
+	}
+
+	if (total_data_len < le16_to_cpu(config->total_len)) {
+		bt_dev_err(hdev, "rtl: config is too short");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < total_data_len; ) {
+		entry = ((void *)config->entry) + i;
+
+		switch (le16_to_cpu(entry->offset)) {
+		case 0xc:
+			if (entry->len < sizeof(*device_baudrate)) {
+				bt_dev_err(hdev,
+					   "rtl: invalid UART config entry");
+				return -EINVAL;
+			}
+
+			*device_baudrate = get_unaligned_le32(entry->data);
+			*controller_baudrate = btrtl_convert_baudrate(
+							*device_baudrate);
+
+			if (entry->len >= 13)
+				*flow_control = !!(entry->data[12] & BIT(2));
+			else
+				*flow_control = false;
+
+			found = true;
+			break;
+
+		default:
+			bt_dev_dbg(hdev,
+				   "rtl: skipping config entry 0x%x (len %u)",
+				   le16_to_cpu(entry->offset), entry->len);
+			break;
+		};
+
+		i += sizeof(*entry) + entry->len;
+	}
+
+	if (!found) {
+		bt_dev_err(hdev, "rtl: no UART config entry found");
+		return -ENOENT;
+	}
+
+	bt_dev_dbg(hdev, "rtl: device baudrate = 0x%08x", *device_baudrate);
+	bt_dev_dbg(hdev, "rtl: controller baudrate = %u",
+		   *controller_baudrate);
+	bt_dev_dbg(hdev, "rtl: flow control %d", *flow_control);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(btrtl_get_uart_settings);
+
 MODULE_AUTHOR("Daniel Drake <drake@endlessm.com>");
 MODULE_DESCRIPTION("Bluetooth support for Realtek devices ver " VERSION);
 MODULE_VERSION(VERSION);
diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
index 21c28dcbe5e0..2e7856fa5ac7 100644
--- a/drivers/bluetooth/btrtl.h
+++ b/drivers/bluetooth/btrtl.h
@@ -40,6 +40,18 @@ struct rtl_epatch_header {
 	__le16 num_patches;
 } __packed;
 
+struct rtl_vendor_config_entry {
+	__le16 offset;
+	__u8 len;
+	__u8 data[0];
+} __packed;
+
+struct rtl_vendor_config {
+	__le32 signature;
+	__le16 total_len;
+	struct rtl_vendor_config_entry entry[0];
+} __packed;
+
 #if IS_ENABLED(CONFIG_BT_RTL)
 
 struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev);
@@ -47,6 +59,10 @@ void btrtl_free(struct btrtl_device_info *btrtl_dev);
 int btrtl_download_firmware(struct hci_dev *hdev,
 			    struct btrtl_device_info *btrtl_dev);
 int btrtl_setup_realtek(struct hci_dev *hdev);
+int btrtl_get_uart_settings(struct hci_dev *hdev,
+			    struct btrtl_device_info *btrtl_dev,
+			    unsigned int *controller_baudrate,
+			    u32 *device_baudrate, bool *flow_control);
 
 #else
 
@@ -70,4 +86,13 @@ static inline int btrtl_setup_realtek(struct hci_dev *hdev)
 	return -EOPNOTSUPP;
 }
 
+static inline int btrtl_get_uart_settings(struct hci_dev *hdev,
+					  struct btrtl_device_info *btrtl_dev,
+					  unsigned int *controller_baudrate,
+					  u32 *device_baudrate,
+					  bool *flow_control)
+{
+	return -ENOENT;
+}
+
 #endif
-- 
2.17.1

[PATCH v3 4/9] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips

[Hans de Goede]

From: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

The Realtek RTL8723BS and RTL8723DS chipsets are SDIO wifi chips. They
also contain a Bluetooth module which is connected via UART to the host.

Realtek's userspace initialization tool (rtk_hciattach) differentiates
these two via the HCI version and revision returned by the
HCI_OP_READ_LOCAL_VERSION command.
Additionally we apply these checks only the for UART devices. Everything
else is assumed to be a "RTL8723B" which was originally supported by the
driver (communicating via USB).

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Signed-off-by: Jeremy Cline <jeremy@jcline.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2 (Hans de Goede)
-Fix some minor style issues / checkpatch warnings
---
 drivers/bluetooth/btrtl.c | 51 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 47 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 6c0d88d23d6d..8dacca64fe41 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -38,6 +38,8 @@
 
 #define IC_MATCH_FL_LMPSUBV	(1 << 0)
 #define IC_MATCH_FL_HCIREV	(1 << 1)
+#define IC_MATCH_FL_HCIVER	(1 << 2)
+#define IC_MATCH_FL_HCIBUS	(1 << 3)
 #define IC_INFO(lmps, hcir) \
 	.match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV, \
 	.lmp_subver = (lmps), \
@@ -47,6 +49,8 @@ struct id_table {
 	__u16 match_flags;
 	__u16 lmp_subver;
 	__u16 hci_rev;
+	__u8 hci_ver;
+	__u8 hci_bus;
 	bool config_needed;
 	bool has_rom_version;
 	char *fw_name;
@@ -75,6 +79,18 @@ static const struct id_table ic_id_table[] = {
 	  .fw_name = "rtl_bt/rtl8723a_fw.bin",
 	  .cfg_name = NULL },
 
+	/* 8723BS */
+	{ .match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV |
+			 IC_MATCH_FL_HCIVER | IC_MATCH_FL_HCIBUS,
+	  .lmp_subver = RTL_ROM_LMP_8723B,
+	  .hci_rev = 0xb,
+	  .hci_ver = 6,
+	  .hci_bus = HCI_UART,
+	  .config_needed = true,
+	  .has_rom_version = true,
+	  .fw_name  = "rtl_bt/rtl8723bs_fw.bin",
+	  .cfg_name = "rtl_bt/rtl8723bs_config.bin" },
+
 	/* 8723B */
 	{ IC_INFO(RTL_ROM_LMP_8723B, 0xb),
 	  .config_needed = false,
@@ -89,6 +105,18 @@ static const struct id_table ic_id_table[] = {
 	  .fw_name  = "rtl_bt/rtl8723d_fw.bin",
 	  .cfg_name = "rtl_bt/rtl8723d_config.bin" },
 
+	/* 8723DS */
+	{ .match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV |
+			 IC_MATCH_FL_HCIVER | IC_MATCH_FL_HCIBUS,
+	  .lmp_subver = RTL_ROM_LMP_8723B,
+	  .hci_rev = 0xd,
+	  .hci_ver = 8,
+	  .hci_bus = HCI_UART,
+	  .config_needed = true,
+	  .has_rom_version = true,
+	  .fw_name  = "rtl_bt/rtl8723ds_fw.bin",
+	  .cfg_name = "rtl_bt/rtl8723ds_config.bin" },
+
 	/* 8821A */
 	{ IC_INFO(RTL_ROM_LMP_8821A, 0xa),
 	  .config_needed = false,
@@ -118,7 +146,8 @@ static const struct id_table ic_id_table[] = {
 	  .cfg_name = "rtl_bt/rtl8822b_config.bin" },
 	};
 
-static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev)
+static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev,
+					     u8 hci_ver, u8 hci_bus)
 {
 	int i;
 
@@ -129,6 +158,12 @@ static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev)
 		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIREV) &&
 		    (ic_id_table[i].hci_rev != hci_rev))
 			continue;
+		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIVER) &&
+		    (ic_id_table[i].hci_ver != hci_ver))
+			continue;
+		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIBUS) &&
+		    (ic_id_table[i].hci_bus != hci_bus))
+			continue;
 
 		break;
 	}
@@ -481,6 +516,7 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
 	struct sk_buff *skb;
 	struct hci_rp_read_local_version *resp;
 	u16 hci_rev, lmp_subver;
+	u8 hci_ver;
 	int ret;
 
 	btrtl_dev = kzalloc(sizeof(*btrtl_dev), GFP_KERNEL);
@@ -501,14 +537,17 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
 		    resp->hci_ver, resp->hci_rev,
 		    resp->lmp_ver, resp->lmp_subver);
 
+	hci_ver = resp->hci_ver;
 	hci_rev = le16_to_cpu(resp->hci_rev);
 	lmp_subver = le16_to_cpu(resp->lmp_subver);
 	kfree_skb(skb);
 
-	btrtl_dev->ic_info = btrtl_match_ic(lmp_subver, hci_rev);
+	btrtl_dev->ic_info = btrtl_match_ic(lmp_subver, hci_rev, hci_ver,
+					    hdev->bus);
+
 	if (!btrtl_dev->ic_info) {
-		bt_dev_err(hdev, "rtl: unknown IC info, lmp subver %04x, hci rev %04x",
-			   lmp_subver, hci_rev);
+		bt_dev_err(hdev, "rtl: unknown IC info, lmp subver %04x, hci rev %04x, hci ver %04x",
+			   lmp_subver, hci_rev, hci_ver);
 		ret = -EINVAL;
 		goto err_free;
 	}
@@ -711,6 +750,10 @@ MODULE_LICENSE("GPL");
 MODULE_FIRMWARE("rtl_bt/rtl8723a_fw.bin");
 MODULE_FIRMWARE("rtl_bt/rtl8723b_fw.bin");
 MODULE_FIRMWARE("rtl_bt/rtl8723b_config.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8723bs_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8723bs_config.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8723ds_fw.bin");
+MODULE_FIRMWARE("rtl_bt/rtl8723ds_config.bin");
 MODULE_FIRMWARE("rtl_bt/rtl8761a_fw.bin");
 MODULE_FIRMWARE("rtl_bt/rtl8761a_config.bin");
 MODULE_FIRMWARE("rtl_bt/rtl8821a_fw.bin");
-- 
2.17.1

Re: [PATCH v3 4/9] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips

[Marcel Holtmann]

Hi Hans,

> The Realtek RTL8723BS and RTL8723DS chipsets are SDIO wifi chips. They
> also contain a Bluetooth module which is connected via UART to the host.
> 
> Realtek's userspace initialization tool (rtk_hciattach) differentiates
> these two via the HCI version and revision returned by the
> HCI_OP_READ_LOCAL_VERSION command.
> Additionally we apply these checks only the for UART devices. Everything
> else is assumed to be a "RTL8723B" which was originally supported by the
> driver (communicating via USB).
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> Signed-off-by: Jeremy Cline <jeremy@jcline.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2 (Hans de Goede)
> -Fix some minor style issues / checkpatch warnings
> ---
> drivers/bluetooth/btrtl.c | 51 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 47 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index 6c0d88d23d6d..8dacca64fe41 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -38,6 +38,8 @@
> 
> #define IC_MATCH_FL_LMPSUBV	(1 << 0)
> #define IC_MATCH_FL_HCIREV	(1 << 1)
> +#define IC_MATCH_FL_HCIVER	(1 << 2)
> +#define IC_MATCH_FL_HCIBUS	(1 << 3)
> #define IC_INFO(lmps, hcir) \
> 	.match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV, \
> 	.lmp_subver = (lmps), \
> @@ -47,6 +49,8 @@ struct id_table {
> 	__u16 match_flags;
> 	__u16 lmp_subver;
> 	__u16 hci_rev;
> +	__u8 hci_ver;
> +	__u8 hci_bus;
> 	bool config_needed;
> 	bool has_rom_version;
> 	char *fw_name;
> @@ -75,6 +79,18 @@ static const struct id_table ic_id_table[] = {
> 	  .fw_name = "rtl_bt/rtl8723a_fw.bin",
> 	  .cfg_name = NULL },
> 
> +	/* 8723BS */
> +	{ .match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV |
> +			 IC_MATCH_FL_HCIVER | IC_MATCH_FL_HCIBUS,
> +	  .lmp_subver = RTL_ROM_LMP_8723B,
> +	  .hci_rev = 0xb,
> +	  .hci_ver = 6,
> +	  .hci_bus = HCI_UART,
> +	  .config_needed = true,
> +	  .has_rom_version = true,
> +	  .fw_name  = "rtl_bt/rtl8723bs_fw.bin",
> +	  .cfg_name = "rtl_bt/rtl8723bs_config.bin" },
> +
> 	/* 8723B */
> 	{ IC_INFO(RTL_ROM_LMP_8723B, 0xb),
> 	  .config_needed = false,
> @@ -89,6 +105,18 @@ static const struct id_table ic_id_table[] = {
> 	  .fw_name  = "rtl_bt/rtl8723d_fw.bin",
> 	  .cfg_name = "rtl_bt/rtl8723d_config.bin" },
> 
> +	/* 8723DS */
> +	{ .match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV |
> +			 IC_MATCH_FL_HCIVER | IC_MATCH_FL_HCIBUS,
> +	  .lmp_subver = RTL_ROM_LMP_8723B,
> +	  .hci_rev = 0xd,
> +	  .hci_ver = 8,
> +	  .hci_bus = HCI_UART,
> +	  .config_needed = true,
> +	  .has_rom_version = true,
> +	  .fw_name  = "rtl_bt/rtl8723ds_fw.bin",
> +	  .cfg_name = "rtl_bt/rtl8723ds_config.bin" },
> +
> 	/* 8821A */
> 	{ IC_INFO(RTL_ROM_LMP_8821A, 0xa),
> 	  .config_needed = false,
> @@ -118,7 +146,8 @@ static const struct id_table ic_id_table[] = {
> 	  .cfg_name = "rtl_bt/rtl8822b_config.bin" },
> 	};
> 
> -static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev)
> +static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev,
> +					     u8 hci_ver, u8 hci_bus)
> {
> 	int i;
> 
> @@ -129,6 +158,12 @@ static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev)
> 		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIREV) &&
> 		    (ic_id_table[i].hci_rev != hci_rev))
> 			continue;
> +		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIVER) &&
> +		    (ic_id_table[i].hci_ver != hci_ver))
> +			continue;
> +		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIBUS) &&
> +		    (ic_id_table[i].hci_bus != hci_bus))
> +			continue;
> 
> 		break;
> 	}
> @@ -481,6 +516,7 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
> 	struct sk_buff *skb;
> 	struct hci_rp_read_local_version *resp;
> 	u16 hci_rev, lmp_subver;
> +	u8 hci_ver;
> 	int ret;
> 
> 	btrtl_dev = kzalloc(sizeof(*btrtl_dev), GFP_KERNEL);
> @@ -501,14 +537,17 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
> 		    resp->hci_ver, resp->hci_rev,
> 		    resp->lmp_ver, resp->lmp_subver);
> 
> +	hci_ver = resp->hci_ver;
> 	hci_rev = le16_to_cpu(resp->hci_rev);
> 	lmp_subver = le16_to_cpu(resp->lmp_subver);
> 	kfree_skb(skb);
> 
> -	btrtl_dev->ic_info = btrtl_match_ic(lmp_subver, hci_rev);
> +	btrtl_dev->ic_info = btrtl_match_ic(lmp_subver, hci_rev, hci_ver,
> +					    hdev->bus);
> +

I am bit reluctant to let you use hdev->bus here since that should be essentially local to the core. I know you set it before calling hci_register_dev, but it was never really designed to be accessed again by the driver? Do you need access to it or can this be done internally in the driver?

Regards

Marcel

Re: [PATCH v3 4/9] Bluetooth: btrtl: add support for the RTL8723BS and RTL8723DS chips

[Hans de Goede]

HI,

On 14-07-18 18:09, Marcel Holtmann wrote:
> Hi Hans,
> 
>> The Realtek RTL8723BS and RTL8723DS chipsets are SDIO wifi chips. They
>> also contain a Bluetooth module which is connected via UART to the host.
>>
>> Realtek's userspace initialization tool (rtk_hciattach) differentiates
>> these two via the HCI version and revision returned by the
>> HCI_OP_READ_LOCAL_VERSION command.
>> Additionally we apply these checks only the for UART devices. Everything
>> else is assumed to be a "RTL8723B" which was originally supported by the
>> driver (communicating via USB).
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> Signed-off-by: Jeremy Cline <jeremy@jcline.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2 (Hans de Goede)
>> -Fix some minor style issues / checkpatch warnings
>> ---
>> drivers/bluetooth/btrtl.c | 51 ++++++++++++++++++++++++++++++++++++---
>> 1 file changed, 47 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
>> index 6c0d88d23d6d..8dacca64fe41 100644
>> --- a/drivers/bluetooth/btrtl.c
>> +++ b/drivers/bluetooth/btrtl.c
>> @@ -38,6 +38,8 @@
>>
>> #define IC_MATCH_FL_LMPSUBV	(1 << 0)
>> #define IC_MATCH_FL_HCIREV	(1 << 1)
>> +#define IC_MATCH_FL_HCIVER	(1 << 2)
>> +#define IC_MATCH_FL_HCIBUS	(1 << 3)
>> #define IC_INFO(lmps, hcir) \
>> 	.match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV, \
>> 	.lmp_subver = (lmps), \
>> @@ -47,6 +49,8 @@ struct id_table {
>> 	__u16 match_flags;
>> 	__u16 lmp_subver;
>> 	__u16 hci_rev;
>> +	__u8 hci_ver;
>> +	__u8 hci_bus;
>> 	bool config_needed;
>> 	bool has_rom_version;
>> 	char *fw_name;
>> @@ -75,6 +79,18 @@ static const struct id_table ic_id_table[] = {
>> 	  .fw_name = "rtl_bt/rtl8723a_fw.bin",
>> 	  .cfg_name = NULL },
>>
>> +	/* 8723BS */
>> +	{ .match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV |
>> +			 IC_MATCH_FL_HCIVER | IC_MATCH_FL_HCIBUS,
>> +	  .lmp_subver = RTL_ROM_LMP_8723B,
>> +	  .hci_rev = 0xb,
>> +	  .hci_ver = 6,
>> +	  .hci_bus = HCI_UART,
>> +	  .config_needed = true,
>> +	  .has_rom_version = true,
>> +	  .fw_name  = "rtl_bt/rtl8723bs_fw.bin",
>> +	  .cfg_name = "rtl_bt/rtl8723bs_config.bin" },
>> +
>> 	/* 8723B */
>> 	{ IC_INFO(RTL_ROM_LMP_8723B, 0xb),
>> 	  .config_needed = false,
>> @@ -89,6 +105,18 @@ static const struct id_table ic_id_table[] = {
>> 	  .fw_name  = "rtl_bt/rtl8723d_fw.bin",
>> 	  .cfg_name = "rtl_bt/rtl8723d_config.bin" },
>>
>> +	/* 8723DS */
>> +	{ .match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV |
>> +			 IC_MATCH_FL_HCIVER | IC_MATCH_FL_HCIBUS,
>> +	  .lmp_subver = RTL_ROM_LMP_8723B,
>> +	  .hci_rev = 0xd,
>> +	  .hci_ver = 8,
>> +	  .hci_bus = HCI_UART,
>> +	  .config_needed = true,
>> +	  .has_rom_version = true,
>> +	  .fw_name  = "rtl_bt/rtl8723ds_fw.bin",
>> +	  .cfg_name = "rtl_bt/rtl8723ds_config.bin" },
>> +
>> 	/* 8821A */
>> 	{ IC_INFO(RTL_ROM_LMP_8821A, 0xa),
>> 	  .config_needed = false,
>> @@ -118,7 +146,8 @@ static const struct id_table ic_id_table[] = {
>> 	  .cfg_name = "rtl_bt/rtl8822b_config.bin" },
>> 	};
>>
>> -static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev)
>> +static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev,
>> +					     u8 hci_ver, u8 hci_bus)
>> {
>> 	int i;
>>
>> @@ -129,6 +158,12 @@ static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev)
>> 		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIREV) &&
>> 		    (ic_id_table[i].hci_rev != hci_rev))
>> 			continue;
>> +		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIVER) &&
>> +		    (ic_id_table[i].hci_ver != hci_ver))
>> +			continue;
>> +		if ((ic_id_table[i].match_flags & IC_MATCH_FL_HCIBUS) &&
>> +		    (ic_id_table[i].hci_bus != hci_bus))
>> +			continue;
>>
>> 		break;
>> 	}
>> @@ -481,6 +516,7 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
>> 	struct sk_buff *skb;
>> 	struct hci_rp_read_local_version *resp;
>> 	u16 hci_rev, lmp_subver;
>> +	u8 hci_ver;
>> 	int ret;
>>
>> 	btrtl_dev = kzalloc(sizeof(*btrtl_dev), GFP_KERNEL);
>> @@ -501,14 +537,17 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
>> 		    resp->hci_ver, resp->hci_rev,
>> 		    resp->lmp_ver, resp->lmp_subver);
>>
>> +	hci_ver = resp->hci_ver;
>> 	hci_rev = le16_to_cpu(resp->hci_rev);
>> 	lmp_subver = le16_to_cpu(resp->lmp_subver);
>> 	kfree_skb(skb);
>>
>> -	btrtl_dev->ic_info = btrtl_match_ic(lmp_subver, hci_rev);
>> +	btrtl_dev->ic_info = btrtl_match_ic(lmp_subver, hci_rev, hci_ver,
>> +					    hdev->bus);
>> +
> 
> I am bit reluctant to let you use hdev->bus here since that should be essentially local to the core. I know you set it before calling hci_register_dev, but it was never really designed to be accessed again by the driver? Do you need access to it or can this be done internally in the driver?

hdev->bus is only used to differentiate between the USB 8723B and the SDIO 8723BS
devices when looking up the info in the ic_id_table[]. As such all that is done
is a "hdev->bus == HCI_UART" comparison, which seems innocent enough and not
prone to breakage if the core changes its use of bus a bit.

The alternative would be to pass the bus-type from the USB / UART specific
probe() functions as parameter to the various intermediate functions until
we end up in btrtl_initialize() but that seems a but silly since we have
it available in hdev->bus already.

Regards,

Hans

[PATCH v3 5/9] Bluetooth: btrtl: Add support for a config filename postfix

[Hans de Goede]

The contents of the rtl_bt/rtlXXXX_config.bin file may be board specific
allow the caller of btrtl_initialize to specify a postfix identifying
the board, which if specified will make btrtl_initialize look for
rtl_bt/rtlXXXX_config-<postfix>.bin instead.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/btrtl.c | 32 ++++++++++++++++++++------------
 drivers/bluetooth/btrtl.h |  6 ++++--
 2 files changed, 24 insertions(+), 14 deletions(-)

diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 8dacca64fe41..67ff5b7c2116 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -89,21 +89,21 @@ static const struct id_table ic_id_table[] = {
 	  .config_needed = true,
 	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8723bs_fw.bin",
-	  .cfg_name = "rtl_bt/rtl8723bs_config.bin" },
+	  .cfg_name = "rtl_bt/rtl8723bs_config" },
 
 	/* 8723B */
 	{ IC_INFO(RTL_ROM_LMP_8723B, 0xb),
 	  .config_needed = false,
 	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8723b_fw.bin",
-	  .cfg_name = "rtl_bt/rtl8723b_config.bin" },
+	  .cfg_name = "rtl_bt/rtl8723b_config" },
 
 	/* 8723D */
 	{ IC_INFO(RTL_ROM_LMP_8723B, 0xd),
 	  .config_needed = true,
 	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8723d_fw.bin",
-	  .cfg_name = "rtl_bt/rtl8723d_config.bin" },
+	  .cfg_name = "rtl_bt/rtl8723d_config" },
 
 	/* 8723DS */
 	{ .match_flags = IC_MATCH_FL_LMPSUBV | IC_MATCH_FL_HCIREV |
@@ -115,35 +115,35 @@ static const struct id_table ic_id_table[] = {
 	  .config_needed = true,
 	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8723ds_fw.bin",
-	  .cfg_name = "rtl_bt/rtl8723ds_config.bin" },
+	  .cfg_name = "rtl_bt/rtl8723ds_config" },
 
 	/* 8821A */
 	{ IC_INFO(RTL_ROM_LMP_8821A, 0xa),
 	  .config_needed = false,
 	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8821a_fw.bin",
-	  .cfg_name = "rtl_bt/rtl8821a_config.bin" },
+	  .cfg_name = "rtl_bt/rtl8821a_config" },
 
 	/* 8821C */
 	{ IC_INFO(RTL_ROM_LMP_8821A, 0xc),
 	  .config_needed = false,
 	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8821c_fw.bin",
-	  .cfg_name = "rtl_bt/rtl8821c_config.bin" },
+	  .cfg_name = "rtl_bt/rtl8821c_config" },
 
 	/* 8761A */
 	{ IC_MATCH_FL_LMPSUBV, RTL_ROM_LMP_8761A, 0x0,
 	  .config_needed = false,
 	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8761a_fw.bin",
-	  .cfg_name = "rtl_bt/rtl8761a_config.bin" },
+	  .cfg_name = "rtl_bt/rtl8761a_config" },
 
 	/* 8822B */
 	{ IC_INFO(RTL_ROM_LMP_8822B, 0xb),
 	  .config_needed = true,
 	  .has_rom_version = true,
 	  .fw_name  = "rtl_bt/rtl8822b_fw.bin",
-	  .cfg_name = "rtl_bt/rtl8822b_config.bin" },
+	  .cfg_name = "rtl_bt/rtl8822b_config" },
 	};
 
 static const struct id_table *btrtl_match_ic(u16 lmp_subver, u16 hci_rev,
@@ -510,11 +510,13 @@ void btrtl_free(struct btrtl_device_info *btrtl_dev)
 }
 EXPORT_SYMBOL_GPL(btrtl_free);
 
-struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
+struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
+					   const char *postfix)
 {
 	struct btrtl_device_info *btrtl_dev;
 	struct sk_buff *skb;
 	struct hci_rp_read_local_version *resp;
+	char cfg_name[40];
 	u16 hci_rev, lmp_subver;
 	u8 hci_ver;
 	int ret;
@@ -568,8 +570,14 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
 	}
 
 	if (btrtl_dev->ic_info->cfg_name) {
-		btrtl_dev->cfg_len = rtl_load_file(hdev,
-						   btrtl_dev->ic_info->cfg_name,
+		if (postfix) {
+			snprintf(cfg_name, sizeof(cfg_name), "%s-%s.bin",
+				 btrtl_dev->ic_info->cfg_name, postfix);
+		} else {
+			snprintf(cfg_name, sizeof(cfg_name), "%s.bin",
+				 btrtl_dev->ic_info->cfg_name);
+		}
+		btrtl_dev->cfg_len = rtl_load_file(hdev, cfg_name,
 						   &btrtl_dev->cfg_data);
 		if (btrtl_dev->ic_info->config_needed &&
 		    btrtl_dev->cfg_len <= 0) {
@@ -620,7 +628,7 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
 	struct btrtl_device_info *btrtl_dev;
 	int ret;
 
-	btrtl_dev = btrtl_initialize(hdev);
+	btrtl_dev = btrtl_initialize(hdev, NULL);
 	if (IS_ERR(btrtl_dev))
 		return PTR_ERR(btrtl_dev);
 
diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
index 2e7856fa5ac7..1e82ee256d02 100644
--- a/drivers/bluetooth/btrtl.h
+++ b/drivers/bluetooth/btrtl.h
@@ -54,7 +54,8 @@ struct rtl_vendor_config {
 
 #if IS_ENABLED(CONFIG_BT_RTL)
 
-struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev);
+struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
+					   const char *postfix);
 void btrtl_free(struct btrtl_device_info *btrtl_dev);
 int btrtl_download_firmware(struct hci_dev *hdev,
 			    struct btrtl_device_info *btrtl_dev);
@@ -66,7 +67,8 @@ int btrtl_get_uart_settings(struct hci_dev *hdev,
 
 #else
 
-static inline struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev)
+static inline struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
+							 const char *postfix)
 {
 	return ERR_PTR(-EOPNOTSUPP);
 }
-- 
2.17.1

Re: [PATCH v3 5/9] Bluetooth: btrtl: Add support for a config filename postfix

[Marcel Holtmann]

Hi Hans,

> The contents of the rtl_bt/rtlXXXX_config.bin file may be board specific
> allow the caller of btrtl_initialize to specify a postfix identifying
> the board, which if specified will make btrtl_initialize look for
> rtl_bt/rtlXXXX_config-<postfix>.bin instead.

frankly I do not get this patch. Where would the <postfix> come from? Is that something we would put into DT or where? I think you need a bit more explanation here.

Regards

Marcel

Re: [PATCH v3 5/9] Bluetooth: btrtl: Add support for a config filename postfix

[Hans de Goede]

Hi,

Thank you for the review of the other patches, I will prepare
a new version addressing your remarks on the other patches.

On 14-07-18 18:06, Marcel Holtmann wrote:
> Hi Hans,
> 
>> The contents of the rtl_bt/rtlXXXX_config.bin file may be board specific
>> allow the caller of btrtl_initialize to specify a postfix identifying
>> the board, which if specified will make btrtl_initialize look for
>> rtl_bt/rtlXXXX_config-<postfix>.bin instead.
> 
> frankly I do not get this patch. Where would the <postfix> come from? Is that something we would put into DT or where? I think you need a bit more explanation here.

As mentioned before this patch-set only adds support for ACPI
enumeration for now as I don't have hw to test the DT enumeration.

For ACPI enumeration I use the ACPI HID, as can be seen in the
"Bluetooth: hci_h5: Add vendor setup, open, and close callbacks"
patch which does:

+	if (has_acpi_companion(dev)) {
+		match = acpi_match_device(dev->driver->acpi_match_table, dev);
+		if (!match)
+			return -ENODEV;
+
+		h5->vnd = (const struct h5_vnd *)match->driver_data;
+		h5->id  = (char *)match->id;
+	}
+

And then the "Bluetooth: hci_h5: Add support for the RTL8723BS"
passes h5->id as postfix:

+static int h5_btrtl_setup(struct h5 *h5)
+{
+	struct btrtl_device_info *btrtl_dev;
+	struct sk_buff *skb;
+	__le32 baudrate_data;
+	u32 device_baudrate;
+	unsigned int controller_baudrate;
+	bool flow_control;
+	int err;
+
+	btrtl_dev = btrtl_initialize(h5->hu->hdev, h5->id);
+

For DT we could either use a property in the node
describing the BT device as postfix, or since we want
to have a unique per board name use the machine "compatible"
string as postfix.

Regards,

Hans

[PATCH v3 6/9] Bluetooth: hci_h5: Add support for serdev enumerated devices

[Hans de Goede]

Add basic support for serdev enumerated devices, note sine this does
not (yet) declare any of / ACPI ids to bind to atm this is a nop.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_h5.c | 52 +++++++++++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index 6a8d0d06aba7..672f63623bf7 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -23,6 +23,7 @@
 
 #include <linux/kernel.h>
 #include <linux/errno.h>
+#include <linux/serdev.h>
 #include <linux/skbuff.h>
 
 #include <net/bluetooth/bluetooth.h>
@@ -65,6 +66,9 @@ enum {
 };
 
 struct h5 {
+	/* Must be the first member, hci_serdev.c expects this. */
+	struct hci_uart		serdev_hu;
+
 	struct sk_buff_head	unack;		/* Unack'ed packets queue */
 	struct sk_buff_head	rel;		/* Reliable packets queue */
 	struct sk_buff_head	unrel;		/* Unreliable packets queue */
@@ -193,9 +197,13 @@ static int h5_open(struct hci_uart *hu)
 
 	BT_DBG("hu %p", hu);
 
-	h5 = kzalloc(sizeof(*h5), GFP_KERNEL);
-	if (!h5)
-		return -ENOMEM;
+	if (hu->serdev) {
+		h5 = serdev_device_get_drvdata(hu->serdev);
+	} else {
+		h5 = kzalloc(sizeof(*h5), GFP_KERNEL);
+		if (!h5)
+			return -ENOMEM;
+	}
 
 	hu->priv = h5;
 	h5->hu = hu;
@@ -229,7 +237,8 @@ static int h5_close(struct hci_uart *hu)
 	skb_queue_purge(&h5->rel);
 	skb_queue_purge(&h5->unrel);
 
-	kfree(h5);
+	if (!hu->serdev)
+		kfree(h5);
 
 	return 0;
 }
@@ -750,12 +759,47 @@ static const struct hci_uart_proto h5p = {
 	.flush		= h5_flush,
 };
 
+static int h5_serdev_probe(struct serdev_device *serdev)
+{
+	struct device *dev = &serdev->dev;
+	struct h5 *h5;
+
+	h5 = devm_kzalloc(dev, sizeof(*h5), GFP_KERNEL);
+	if (!h5)
+		return -ENOMEM;
+
+	set_bit(HCI_UART_RESET_ON_INIT, &h5->serdev_hu.flags);
+
+	h5->hu = &h5->serdev_hu;
+	h5->serdev_hu.serdev = serdev;
+	serdev_device_set_drvdata(serdev, h5);
+
+	return hci_uart_register_device(&h5->serdev_hu, &h5p);
+}
+
+static void h5_serdev_remove(struct serdev_device *serdev)
+{
+	struct h5 *h5 = serdev_device_get_drvdata(serdev);
+
+	hci_uart_unregister_device(&h5->serdev_hu);
+}
+
+static struct serdev_device_driver h5_serdev_driver = {
+	.probe = h5_serdev_probe,
+	.remove = h5_serdev_remove,
+	.driver = {
+		.name = "hci_uart_h5",
+	},
+};
+
 int __init h5_init(void)
 {
+	serdev_device_driver_register(&h5_serdev_driver);
 	return hci_uart_register_proto(&h5p);
 }
 
 int __exit h5_deinit(void)
 {
+	serdev_device_driver_unregister(&h5_serdev_driver);
 	return hci_uart_unregister_proto(&h5p);
 }
-- 
2.17.1

[PATCH v3 7/9] Bluetooth: hci_h5: Add vendor setup, open, and close callbacks

[Hans de Goede]

From: Jeremy Cline <jeremy@jcline.org>

Allow vendor-specific setup, open, and close functions to be defined.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
[hdegoede@redhat.com: Port from bt3wire.c to hci_h5.c, drop dt support]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_h5.c | 39 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index 672f63623bf7..0cf4f1e9df0c 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -21,8 +21,9 @@
  *
  */
 
-#include <linux/kernel.h>
+#include <linux/acpi.h>
 #include <linux/errno.h>
+#include <linux/kernel.h>
 #include <linux/serdev.h>
 #include <linux/skbuff.h>
 
@@ -99,6 +100,15 @@ struct h5 {
 		H5_SLEEPING,
 		H5_WAKING_UP,
 	} sleep;
+
+	const struct h5_vnd *vnd;
+	const char *id;
+};
+
+struct h5_vnd {
+	int (*setup)(struct h5 *h5);
+	void (*open)(struct h5 *h5);
+	void (*close)(struct h5 *h5);
 };
 
 static void h5_reset_rx(struct h5 *h5);
@@ -218,6 +228,9 @@ static int h5_open(struct hci_uart *hu)
 
 	h5->tx_win = H5_TX_WIN_MAX;
 
+	if (h5->vnd && h5->vnd->open)
+		h5->vnd->open(h5);
+
 	set_bit(HCI_UART_INIT_PENDING, &hu->hdev_flags);
 
 	/* Send initial sync request */
@@ -237,12 +250,25 @@ static int h5_close(struct hci_uart *hu)
 	skb_queue_purge(&h5->rel);
 	skb_queue_purge(&h5->unrel);
 
+	if (h5->vnd && h5->vnd->close)
+		h5->vnd->close(h5);
+
 	if (!hu->serdev)
 		kfree(h5);
 
 	return 0;
 }
 
+static int h5_setup(struct hci_uart *hu)
+{
+	struct h5 *h5 = hu->priv;
+
+	if (h5->vnd && h5->vnd->setup)
+		return h5->vnd->setup(h5);
+
+	return 0;
+}
+
 static void h5_pkt_cull(struct h5 *h5)
 {
 	struct sk_buff *skb, *tmp;
@@ -753,6 +779,7 @@ static const struct hci_uart_proto h5p = {
 	.name		= "Three-wire (H5)",
 	.open		= h5_open,
 	.close		= h5_close,
+	.setup		= h5_setup,
 	.recv		= h5_recv,
 	.enqueue	= h5_enqueue,
 	.dequeue	= h5_dequeue,
@@ -761,6 +788,7 @@ static const struct hci_uart_proto h5p = {
 
 static int h5_serdev_probe(struct serdev_device *serdev)
 {
+	const struct acpi_device_id *match;
 	struct device *dev = &serdev->dev;
 	struct h5 *h5;
 
@@ -774,6 +802,15 @@ static int h5_serdev_probe(struct serdev_device *serdev)
 	h5->serdev_hu.serdev = serdev;
 	serdev_device_set_drvdata(serdev, h5);
 
+	if (has_acpi_companion(dev)) {
+		match = acpi_match_device(dev->driver->acpi_match_table, dev);
+		if (!match)
+			return -ENODEV;
+
+		h5->vnd = (const struct h5_vnd *)match->driver_data;
+		h5->id  = (char *)match->id;
+	}
+
 	return hci_uart_register_device(&h5->serdev_hu, &h5p);
 }
 
-- 
2.17.1

Re: [PATCH v3 7/9] Bluetooth: hci_h5: Add vendor setup, open, and close callbacks

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 2207 bytes --]

Hi Jeremy,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on bluetooth-next/master]
[also build test ERROR on v4.18-rc4 next-20180709]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/Bluetooth-Add-RTL8723BS-support/20180710-165735
base:   https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git master
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 8.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        GCC_VERSION=8.1.0 make.cross ARCH=xtensa 

All errors (new ones prefixed by >>):

   drivers//bluetooth/hci_h5.c: In function 'h5_serdev_probe':
>> drivers//bluetooth/hci_h5.c:810:41: error: dereferencing pointer to incomplete type 'const struct acpi_device_id'
      h5->vnd = (const struct h5_vnd *)match->driver_data;
                                            ^~

vim +810 drivers//bluetooth/hci_h5.c

   788	
   789	static int h5_serdev_probe(struct serdev_device *serdev)
   790	{
   791		const struct acpi_device_id *match;
   792		struct device *dev = &serdev->dev;
   793		struct h5 *h5;
   794	
   795		h5 = devm_kzalloc(dev, sizeof(*h5), GFP_KERNEL);
   796		if (!h5)
   797			return -ENOMEM;
   798	
   799		set_bit(HCI_UART_RESET_ON_INIT, &h5->serdev_hu.flags);
   800	
   801		h5->hu = &h5->serdev_hu;
   802		h5->serdev_hu.serdev = serdev;
   803		serdev_device_set_drvdata(serdev, h5);
   804	
   805		if (has_acpi_companion(dev)) {
   806			match = acpi_match_device(dev->driver->acpi_match_table, dev);
   807			if (!match)
   808				return -ENODEV;
   809	
 > 810			h5->vnd = (const struct h5_vnd *)match->driver_data;
   811			h5->id  = (char *)match->id;
   812		}
   813	
   814		return hci_uart_register_device(&h5->serdev_hu, &h5p);
   815	}
   816	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 54179 bytes --]

[PATCH v3 8/9] Bluetooth: hci_h5: Add support for the RTL8723BS

[Hans de Goede]

From: Jeremy Cline <jeremy@jcline.org>

Implement support for the RTL8723BS chip.

Signed-off-by: Jeremy Cline <jeremy@jcline.org>
[hdegoede@redhat.com: Port from bt3wire.c to hci_h5.c, drop broken GPIO code]
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_h5.c | 69 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 69 insertions(+)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index 0cf4f1e9df0c..fd5f50e30a79 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -30,6 +30,7 @@
 #include <net/bluetooth/bluetooth.h>
 #include <net/bluetooth/hci_core.h>
 
+#include "btrtl.h"
 #include "hci_uart.h"
 
 #define HCI_3WIRE_ACK_PKT	0
@@ -821,11 +822,79 @@ static void h5_serdev_remove(struct serdev_device *serdev)
 	hci_uart_unregister_device(&h5->serdev_hu);
 }
 
+static int h5_btrtl_setup(struct h5 *h5)
+{
+	struct btrtl_device_info *btrtl_dev;
+	struct sk_buff *skb;
+	__le32 baudrate_data;
+	u32 device_baudrate;
+	unsigned int controller_baudrate;
+	bool flow_control;
+	int err;
+
+	btrtl_dev = btrtl_initialize(h5->hu->hdev, h5->id);
+	if (IS_ERR(btrtl_dev))
+		return PTR_ERR(btrtl_dev);
+
+	err = btrtl_get_uart_settings(h5->hu->hdev, btrtl_dev,
+				      &controller_baudrate, &device_baudrate,
+				      &flow_control);
+	if (err)
+		goto out_free;
+
+	baudrate_data = cpu_to_le32(device_baudrate);
+	skb = __hci_cmd_sync(h5->hu->hdev, 0xfc17, sizeof(baudrate_data),
+			     &baudrate_data, HCI_INIT_TIMEOUT);
+	if (IS_ERR(skb)) {
+		bt_dev_err(h5->hu->hdev, "set baud rate command failed");
+		err = PTR_ERR(skb);
+		goto out_free;
+	} else {
+		kfree_skb(skb);
+	}
+	/* Give the device some time to set up the new baudrate. */
+	usleep_range(10000, 20000);
+
+	serdev_device_set_baudrate(h5->hu->serdev, controller_baudrate);
+	serdev_device_set_flow_control(h5->hu->serdev, flow_control);
+
+	err = btrtl_download_firmware(h5->hu->hdev, btrtl_dev);
+	/* Give the device some time before the hci-core sends it a reset */
+	usleep_range(10000, 20000);
+
+out_free:
+	btrtl_free(btrtl_dev);
+
+	return err;
+}
+
+static void h5_btrtl_open(struct h5 *h5)
+{
+	/* Devices always start with these fixed parameters */
+	serdev_device_set_flow_control(h5->hu->serdev, false);
+	serdev_device_set_parity(h5->hu->serdev, SERDEV_PARITY_EVEN);
+	serdev_device_set_baudrate(h5->hu->serdev, 115200);
+}
+
+static struct h5_vnd rtl_vnd = {
+	.setup		= h5_btrtl_setup,
+	.open		= h5_btrtl_open,
+};
+
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id h5_acpi_match[] = {
+	{ "OBDA8723", (kernel_ulong_t)&rtl_vnd },
+	{ },
+};
+MODULE_DEVICE_TABLE(acpi, h5_acpi_match);
+#endif
+
 static struct serdev_device_driver h5_serdev_driver = {
 	.probe = h5_serdev_probe,
 	.remove = h5_serdev_remove,
 	.driver = {
 		.name = "hci_uart_h5",
+		.acpi_match_table = ACPI_PTR(h5_acpi_match),
 	},
 };
 
-- 
2.17.1

[PATCH v3 9/9] Bluetooth: hci_h5: Add support for enable and device-wake GPIOs

[Hans de Goede]

Add support for the enable and device-wake GPIOs used on ACPI enumerated
RTL8723BS devices.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/bluetooth/hci_h5.c | 41 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index fd5f50e30a79..8417d745a593 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -23,6 +23,7 @@
 
 #include <linux/acpi.h>
 #include <linux/errno.h>
+#include <linux/gpio/consumer.h>
 #include <linux/kernel.h>
 #include <linux/serdev.h>
 #include <linux/skbuff.h>
@@ -104,12 +105,16 @@ struct h5 {
 
 	const struct h5_vnd *vnd;
 	const char *id;
+
+	struct gpio_desc *enable_gpio;
+	struct gpio_desc *device_wake_gpio;
 };
 
 struct h5_vnd {
 	int (*setup)(struct h5 *h5);
 	void (*open)(struct h5 *h5);
 	void (*close)(struct h5 *h5);
+	const struct acpi_gpio_mapping *acpi_gpio_map;
 };
 
 static void h5_reset_rx(struct h5 *h5);
@@ -810,8 +815,21 @@ static int h5_serdev_probe(struct serdev_device *serdev)
 
 		h5->vnd = (const struct h5_vnd *)match->driver_data;
 		h5->id  = (char *)match->id;
+
+		if (h5->vnd->acpi_gpio_map)
+			devm_acpi_dev_add_driver_gpios(dev,
+						       h5->vnd->acpi_gpio_map);
 	}
 
+	h5->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);
+	if (IS_ERR(h5->enable_gpio))
+		return PTR_ERR(h5->enable_gpio);
+
+	h5->device_wake_gpio = devm_gpiod_get_optional(dev, "device-wake",
+						       GPIOD_OUT_LOW);
+	if (IS_ERR(h5->device_wake_gpio))
+		return PTR_ERR(h5->device_wake_gpio);
+
 	return hci_uart_register_device(&h5->serdev_hu, &h5p);
 }
 
@@ -874,11 +892,34 @@ static void h5_btrtl_open(struct h5 *h5)
 	serdev_device_set_flow_control(h5->hu->serdev, false);
 	serdev_device_set_parity(h5->hu->serdev, SERDEV_PARITY_EVEN);
 	serdev_device_set_baudrate(h5->hu->serdev, 115200);
+
+	/* The controller needs up to 500ms to wakeup */
+	gpiod_set_value_cansleep(h5->enable_gpio, 1);
+	gpiod_set_value_cansleep(h5->device_wake_gpio, 1);
+	msleep(500);
 }
 
+static void h5_btrtl_close(struct h5 *h5)
+{
+	gpiod_set_value_cansleep(h5->device_wake_gpio, 0);
+	gpiod_set_value_cansleep(h5->enable_gpio, 0);
+}
+
+static const struct acpi_gpio_params btrtl_device_wake_gpios = { 0, 0, false };
+static const struct acpi_gpio_params btrtl_enable_gpios = { 1, 0, false };
+static const struct acpi_gpio_params btrtl_host_wake_gpios = { 2, 0, false };
+static const struct acpi_gpio_mapping acpi_btrtl_gpios[] = {
+	{ "device-wake-gpios", &btrtl_device_wake_gpios, 1 },
+	{ "enable-gpios", &btrtl_enable_gpios, 1 },
+	{ "host-wake-gpios", &btrtl_host_wake_gpios, 1 },
+	{},
+};
+
 static struct h5_vnd rtl_vnd = {
 	.setup		= h5_btrtl_setup,
 	.open		= h5_btrtl_open,
+	.close		= h5_btrtl_close,
+	.acpi_gpio_map	= acpi_btrtl_gpios,
 };
 
 #ifdef CONFIG_ACPI
-- 
2.17.1

Re: [PATCH v3 0/9] Bluetooth: Add RTL8723BS support

[Ian W MORRISON]

On Tue, 10 Jul 2018 at 18:12, Hans de Goede <hdegoede@redhat.com> wrote:
>
>
> New in v3:
> -Fix build error when CONFIG_RTL_BT is not set, reported by Paul Cercueil
>
Hi Hans,

Minor point but v3 fix is for CONFIG_BT_RTL.

Regards,
Ian