Hi Kiran,
>>>>> Adds callback function which is called to set the data path for HFP
>>>>> offload case before opening SCO connection
>>>>>
>>>>> Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
>>>>> Reviewed-by: Chethan T N <chethan.tumkur.narayan@xxxxxxxxx>
>>>>> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@xxxxxxxxx>
>>>>> ---
>>>>> drivers/bluetooth/btintel.c | 50
>>>> +++++++++++++++++++++++++++++++++++++
>>>>> drivers/bluetooth/btintel.h | 8 ++++++
>>>>> drivers/bluetooth/btusb.c | 4 ++-
>>>>> include/net/bluetooth/hci.h | 8 ++++++
>>>>> 4 files changed, 69 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/bluetooth/btintel.c
>>>>> b/drivers/bluetooth/btintel.c index 95c6a1bef35e..3eba5c587ef6
>>>>> 100644
>>>>> --- a/drivers/bluetooth/btintel.c
>>>>> +++ b/drivers/bluetooth/btintel.c
>>>>> @@ -1308,6 +1308,56 @@ int btintel_read_offload_usecases(struct
>>>>> hci_dev *hdev, } EXPORT_SYMBOL_GPL(btintel_read_offload_usecases);
>>>>>
>>>>> +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
>>>>> + struct bt_codec *codec)
>>>>> +{
>>>>> + __u8 preset;
>>>>> + struct hci_op_configure_data_path *cmd;
>>>>> + __u8 buffer[255];
>>>>> + struct sk_buff *skb;
>>>>> +
>>>>> + if (type != SCO_LINK && type != ESCO_LINK)
>>>>> + return -EINVAL;
>>>>> +
>>>>> + switch (codec->id) {
>>>>> + case 0x02:
>>>>> + preset = 0x00;
>>>>> + break;
>>>>> + case 0x05:
>>>>> + preset = 0x01;
>>>>> + break;
>>>>> + default:
>>>>> + return -EINVAL;
>>>>> + }
>>>>> +
>>>>> + cmd = (void *)buffer;
>>>>> + cmd->data_path_id = 0x01;
>>>>> + cmd->len = 1;
>>>>> + cmd->data[0] = preset;
>>>>> +
>>>>> + cmd->direction = 0x00;
>>>>> + skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
>>>> sizeof(*cmd) + 1,
>>>>> + cmd, HCI_INIT_TIMEOUT);
>>>>> + if (IS_ERR(skb)) {
>>>>> + bt_dev_err(hdev, "configure input data path failed (%ld)",
>>>>> + PTR_ERR(skb));
>>>>> + return PTR_ERR(skb);
>>>>> + }
>>>>> + kfree_skb(skb);
>>>>> +
>>>>> + cmd->direction = 0x01;
>>>>> + skb = __hci_cmd_sync(hdev, HCI_CONFIGURE_DATA_PATH,
>>>> sizeof(*cmd) + 1,
>>>>> + cmd, HCI_INIT_TIMEOUT);
>>>>> + if (IS_ERR(skb)) {
>>>>> + bt_dev_err(hdev, "configure output data path failed (%ld)",
>>>>> + PTR_ERR(skb));
>>>>> + return PTR_ERR(skb);
>>>>> + }
>>>>> + kfree_skb(skb);
>>>>> + return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(btintel_set_data_path);
>>>>> +
>>>>> MODULE_AUTHOR("Marcel Holtmann <marcel@xxxxxxxxxxxx>");
>>>>> MODULE_DESCRIPTION("Bluetooth support for Intel devices ver "
>>>>> VERSION); MODULE_VERSION(VERSION); diff --git
>>>>> a/drivers/bluetooth/btintel.h b/drivers/bluetooth/btintel.h index
>>>>> 9bcc489680db..9806970c9871 100644
>>>>> --- a/drivers/bluetooth/btintel.h
>>>>> +++ b/drivers/bluetooth/btintel.h
>>>>> @@ -183,6 +183,8 @@ int btintel_set_debug_features(struct hci_dev
>>>>> *hdev, int btintel_read_offload_usecases(struct hci_dev *hdev,
>>>>> struct intel_offload_usecases *usecases); int
>>>>> btintel_get_data_path(struct hci_dev *hdev);
>>>>> +int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
>>>>> + struct bt_codec *codec);
>>>>> #else
>>>>>
>>>>> static inline int btintel_check_bdaddr(struct hci_dev *hdev) @@
>>>>> -325,4
>>>>> +327,10 @@ static int btintel_get_data_path(struct hci_dev *hdev) {
>>>>> return -EOPNOTSUPP;
>>>>> }
>>>>> +
>>>>> +static int btintel_set_data_path(struct hci_dev *hdev, __u8 type,
>>>>> + struct bt_codec *codec)
>>>>> +{
>>>>> + return -EOPNOTSUPP;
>>>>> +}
>>>>> #endif
>>>>> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
>>>>> index 1e51beec5776..afafa44752a1 100644
>>>>> --- a/drivers/bluetooth/btusb.c
>>>>> +++ b/drivers/bluetooth/btusb.c
>>>>> @@ -3012,8 +3012,10 @@ static int btusb_setup_intel_newgen(struct
>>>> hci_dev *hdev)
>>>>> err = btintel_read_offload_usecases(hdev, &usecases);
>>>>> if (!err) {
>>>>> /* set get_data_path callback if offload is supported */
>>>>> - if (usecases.preset[0] & 0x03)
>>>>> + if (usecases.preset[0] & 0x03) {
>>>>> hdev->get_data_path = btintel_get_data_path;
>>>>> + hdev->set_data_path = btintel_set_data_path;
>>>>> + }
>>>>> }
>>>>
>>>>> /* Read the Intel version information after loading the FW */ diff
>>>>> --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
>>>>> index 31a5ac8918fc..42963188dcea 100644
>>>>> --- a/include/net/bluetooth/hci.h
>>>>> +++ b/include/net/bluetooth/hci.h
>>>>> @@ -1250,6 +1250,14 @@ struct hci_rp_read_local_oob_ext_data {
>>>>> __u8 rand256[16];
>>>>> } __packed;
>>>>>
>>>>> +#define HCI_CONFIGURE_DATA_PATH 0x0c83
>>>>> +struct hci_op_configure_data_path {
>>>>> + __u8 direction;
>>>>> + __u8 data_path_id;
>>>>> + __u8 len;
>>>>> + __u8 data[];
>>>>> +} __packed;
>>>>> +
>>>>
>>>> if this is a standard HCI command, why is this done as hdev-
>>> set_data_path?
>>>> This makes no sense too me.
>>> Intel uses HCI_CONFIGURE_DATA_PATH to command to set the preset ID
>> (NBS, WBS, ...). Here len and data[] are vendor specific. I should have
>> prefixed these fields with vnd_. I will address this in next patchset.
>>
>> if the command is defined by the Bluetooth SIG, it is handle in the core.
>> However if it needs vendor specific input that we need a callback for just that
>> data.
>
> The current design uses HCI_CONFIGURE_DATA_PATH inside set_data_path callback and its not used at core. I have leveraged SIG command here to minimize defining of new vendor command as vnd_data[] gives flexibility to pass in non-standard values. Other vendors may not have same command/flow to configure data path.
>
> If we are not supposed to use Bluetooth SIG command at driver level, then I need to come up with a new vendor specific command. Please help with your input.
I don’t understand this argumentation. The Bluetooth standard defined HCI_Configure_Data_Path with Vendor_Specific_Config for exactly this reason. So just use it especially if our controllers already support it.
Now I am starting to wonder if this design is making things complicated for no reason. Isn’t it enough to have a hdev->get_data_path_config callback that allows to retrieve such data from the driver.
Frankly, the only thing you need from a driver is that it tells you the values of data_path_id and the vendor_config so that you can feed it back into the controller. Or am I missing anything here?
Let me be clear, if there is a SIG defined command, we implement support for in the core and not the driver. I do not want that other vendors have to define everything over and over again. That is what a standard is for. Only the vendor specific portions are handed off to the driver or userspace to provide.
Regards
Marcel
