Skip to contentSkip to conversation

Bluetooth: btusb: Add routine for applying Intel DDC parameters

PATCH Superseded Tedd Ho-Jeong An 2 participants 3 messages Jun 10, 2015 – Jun 12, 2015 Bluetooth drivers

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Tedd Ho-Jeong An

author
#0 lore raw

PATCH v4 1/1 Bluetooth: btusb: Add routine for applying Intel DDC parameters

From: Tedd Ho-Jeong An <[email protected]>

This patch adds the routine to apply the DDC parameter from device specific ddc file.

Once the device is rest to operational mode, optionally, it can download the device specific configration (DDC) parameters before the BlueZ starts the stack initialization.

It opens the DDC file based on HW_VARIANT and DEV_REVISION and send ID/Value with HCI_Intel_Write_DDC command.

Format of DDC file
DDC file consists of one or more number of HCI_Intel_Write_DDC command that contains the DDC structure in parameter.

DDC Structure
It has 'Length' field of one octet, DDC 'ID' field of two octets followed by the array of DDC 'Value' that gives the value of parameters itself.
'Length' contains the length of DDC 'ID' and DDC 'Value'.

+------------+----------+
| Size(byte) |    Name  |
+------------+----------+
|      1     | Length   |
+------------+----------+
|      2     | ID       |
+------------+----------+
| Length - 2 | Value    |
+------------+----------+

In case of command failure, it returns the command complete with parameter that contains the ID that failed to apply.

Signed-off-by Tedd Ho-Jeong An <[email protected]>
---
 drivers/bluetooth/btintel.h |  5 ++++
 drivers/bluetooth/btusb.c   | 63 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 68 insertions(+)
drivers/bluetooth/btintel.h +5 -0
index 4bda6ab..affb216 100644
@@ -69,6 +69,11 @@ struct intel_secure_send_result {
 	__u8     status;
 } __packed;
 
+struct intel_write_ddc {
+	__u8	status;
+	__le16	failed_id;
+} __packed;
+
 #if IS_ENABLED(CONFIG_BT_INTEL)
 
 int btintel_check_bdaddr(struct hci_dev *hdev);
drivers/bluetooth/btusb.c +63 -0

82 lines not on this page · raw message

Signature · 1 line
2.1.0
maintainer
#1 lore raw

Hi Tedd,

Quoted 78 lines

This patch adds the routine to apply the DDC parameter from device specific ddc file.

Once the device is rest to operational mode, optionally, it can download the device specific configration (DDC) parameters before the BlueZ starts the stack initialization.

It opens the DDC file based on HW_VARIANT and DEV_REVISION and send ID/Value with HCI_Intel_Write_DDC command.

Format of DDC file
DDC file consists of one or more number of HCI_Intel_Write_DDC command that contains the DDC structure in parameter.

DDC Structure
It has 'Length' field of one octet, DDC 'ID' field of two octets followed by the array of DDC 'Value' that gives the value of parameters itself.
'Length' contains the length of DDC 'ID' and DDC 'Value'.

+------------+----------+
| Size(byte) |    Name  |
+------------+----------+
|      1     | Length   |
+------------+----------+
|      2     | ID       |
+------------+----------+
| Length - 2 | Value    |
+------------+----------+

In case of command failure, it returns the command complete with parameter that contains the ID that failed to apply.

Signed-off-by Tedd Ho-Jeong An <[email protected]>

---
drivers/bluetooth/btintel.h | 5 ++++
drivers/bluetooth/btusb.c | 63 +++++++++++++++++++++++++++++++++++++++++++++

2 files changed, 68 insertions(+)
drivers/bluetooth/btintel.h +5 -0
index 4bda6ab..affb216 100644
@@ -69,6 +69,11 @@ struct intel_secure_send_result {
 	__u8     status;
 } __packed;
 
+struct intel_write_ddc {
+	__u8	status;
+	__le16	failed_id;
+} __packed;
+
 #if IS_ENABLED(CONFIG_BT_INTEL)
 
 int btintel_check_bdaddr(struct hci_dev *hdev);
drivers/bluetooth/btusb.c +4 -0
index e97d036..66002d7d 100644
@@ -1936,6 +1936,7 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 	char fwname[64];
 	ktime_t calltime, delta, rettime;
 	unsigned long long duration;
+	u8 dev_revid;
 	int err;
 
 	BT_DBG("%s", hdev->name);

@@ -2083,6 +2084,9 @@ static int btusb_setup_intel_new(struct hci_dev *hdev)
 
 	BT_INFO("%s: Found device firmware: %s", hdev->name, fwname);
 
+	/* Keep Device Revision to use later for DDC */
+	dev_revid = le16_to_cpu(params->dev_revid);
+
 	kfree_skb(skb);
 
 	if (fw->size < 644) {

drivers/bluetooth/btusb.c done: · line 2254

Quoted 7 lines
drivers/bluetooth/btusb.c +4 -0
@@ -2244,7 +2248,66 @@ done:
 
 	clear_bit(BTUSB_BOOTLOADER, &data->flags);
 
+	/* Once the device is running in operational mode, it needs to apply
+	 * the device configuration(DDC) parameters to the device.
+	 */
+	snprintf(fwname, sizeof(fwname), "intel/ibt-11-%u.ddc", dev_revid);

I would actually just move creating the DDC filename above instead of bothering to introduce a new dev_revid variable.

drivers/bluetooth/btusb.c done: · line 2259

Quoted 5 lines
drivers/bluetooth/btusb.c +5 -0
+
+	/* Device can work without DDC parameters so even if it fails to load
+	 * the file, no need to fail the setup.
+	 */
+	err = request_firmware_direct(&fw, fwname, &hdev->dev);

Why _direct compared to the other one?

drivers/bluetooth/btusb.c done: · line 2269

Quoted 10 lines
drivers/bluetooth/btusb.c +10 -0
+	if (err < 0) {
+		BT_ERR("failed to open the ddc file: %s", fwname);
+		return 0;
+	}
+
+	BT_INFO("%s: Intel DDC file opened: %s", hdev->name, fwname);
+
+	fw_ptr = fw->data;
+
+	/* DDC file contains DDC parameters in HCI Command format. */

Did we agree on that? I think we did not. We just want the plain DDC parameters. We might have crossed our messages here. I prefer plain DDC key value pairs in the file.

Quoted 11 lines
drivers/bluetooth/btusb.c +11 -0
+	while (fw->size > fw_ptr - fw->data) {
+		struct hci_command_hdr *cmd;
+		struct intel_write_ddc *wr_ddc;
+
+		cmd = (struct hci_command_hdr *)fw_ptr;
+		fw_ptr += sizeof(*cmd);
+
+		skb = __hci_cmd_sync(hdev, le16_to_cpu(cmd->opcode), cmd->plen,
+				     fw_ptr, HCI_INIT_TIMEOUT);
+		if (IS_ERR(skb)) {
+			BT_ERR("%s: Failed to send Intel_Write_DDC (%ld)",

drivers/bluetooth/btusb.c done: · line 2290

Quoted 10 lines
drivers/bluetooth/btusb.c +10 -0
+			       hdev->name, PTR_ERR(skb));
+			err = PTR_ERR(skb);
+			goto exit_error_ddc;
+		}
+
+		/* If the command fails, it needs to display the event param
+		 * that contains the last failed DDC ID
+		 */
+		wr_ddc = (struct intel_write_ddc *)skb->data;
+		if (wr_ddc->status) {

This is not going to work. __hci_cmd_sync already evaluated the status. So you need to do that in the IS_ERR case above.

Quoted 12 lines
drivers/bluetooth/btusb.c +12 -0
+			BT_ERR("%s: Last failed DDC ID: 0x%04x",
+			       hdev->name, wr_ddc->failed_id);
+			err = -bt_to_errno(wr_ddc->status);
+			kfree_skb(skb);
+			goto exit_error_ddc;
+		}
+
+		fw_ptr += cmd->plen;
+		kfree_skb(skb);
+	}
+	release_firmware(fw);
+

drivers/bluetooth/btusb.c done: · line 2312

Quoted 10 lines
drivers/bluetooth/btusb.c +7 -0
+	BT_INFO("%s: Completed to apply Intel DDC parameters", hdev->name);
+
 	return 0;
+
+exit_error_ddc:
+	release_firmware(fw);
+
+	return err;
 }

Regards

Marcel

author
#2 lore raw

Hi Marcel,

Quoted 13 lines · On Fri, 12 Jun 2015 11:36:16 +0200 Marcel Holtmann <[email protected]> wrote:

Hi Tedd,

drivers/bluetooth/btusb.c done: · line 2254

Quoted 8 lines
drivers/bluetooth/btusb.c +4 -0
@@ -2244,7 +2248,66 @@ done:
 
 	clear_bit(BTUSB_BOOTLOADER, &data->flags);
 
+	/* Once the device is running in operational mode, it needs to apply
+	 * the device configuration(DDC) parameters to the device.
+	 */
+	snprintf(fwname, sizeof(fwname), "intel/ibt-11-%u.ddc", dev_revid);

I would actually just move creating the DDC filename above instead of bothering to introduce a new dev_revid variable.

Quoted 7 lines

drivers/bluetooth/btusb.c done: · line 2259

Quoted 5 lines
drivers/bluetooth/btusb.c +5 -0
+
+	/* Device can work without DDC parameters so even if it fails to load
+	 * the file, no need to fail the setup.
+	 */
+	err = request_firmware_direct(&fw, fwname, &hdev->dev);

Why _direct compared to the other one?

If the DDC file doesn't exist, then request_firmware() displays the following extra message:

[ 2193.141256] bluetooth hci0: Direct firmware load for intel/ibt-11-5.ddc failed with error -2

I thought its not necessary to display and give a negative impression since the device can work without DDC file.

The request_firmware_direct() suppress the error message and this is the comment from the function says it is useful for optional firmware.

* This function works pretty much like request_firmware(), but this doesn't
* fall back to usermode helper even if the firmware couldn't be loaded
* directly from fs. Hence it's useful for loading optional firmwares, which
* aren't always present, without extra long timeouts of udev.

Functionally, both works fine.

Quoted 14 lines

drivers/bluetooth/btusb.c done: · line 2269

Quoted 11 lines
drivers/bluetooth/btusb.c +10 -0
+	if (err < 0) {
+		BT_ERR("failed to open the ddc file: %s", fwname);
+		return 0;
+	}
+
+	BT_INFO("%s: Intel DDC file opened: %s", hdev->name, fwname);
+
+	fw_ptr = fw->data;
+
+	/* DDC file contains DDC parameters in HCI Command format. */

Did we agree on that? I think we did not. We just want the plain DDC parameters. We might have crossed our messages here. I prefer plain DDC key value pairs in the file.

Thanks for clarification. :)

Quoted 6 lines
Quoted 1 line

Regards

Marcel

Regards,
Tedd