Hi Marcel,
> -----Original Message-----
> From: Marcel Holtmann <marcel@xxxxxxxxxxxx>
> Sent: Wednesday, June 16, 2021 10:48 AM
> To: K, Kiran <kiran.k@xxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v9 07/10] Bluetooth: btintel: define callback to set data
> path
>
> 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.
>
> Regards
>
> Marcel
Thanks,
Kiran
