| Message ID | 20210608122455.19583-7-kiran.k@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v9,01/10] Bluetooth: enumerate local supported codec and cache details | expand |
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@intel.com> > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com> > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com> > --- > 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@holtmann.org>"); > 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. Regards Marcel
Hi Marcel, > > 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@intel.com> > > Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com> > > Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com> > > --- > > 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@holtmann.org>"); > > 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. > > Regards > > Marcel Thanks, Kiran
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@intel.com> >>> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com> >>> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com> >>> --- >>> 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@holtmann.org>"); >>> 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. Regards Marcel
Hi Marcel, > -----Original Message----- > From: Marcel Holtmann <marcel@holtmann.org> > Sent: Wednesday, June 16, 2021 10:48 AM > To: K, Kiran <kiran.k@intel.com> > Cc: linux-bluetooth@vger.kernel.org > 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@intel.com> > >>> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com> > >>> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com> > >>> --- > >>> 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@holtmann.org>"); > >>> 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
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@intel.com> >>>>> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com> >>>>> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com> >>>>> --- >>>>> 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@holtmann.org>"); >>>>> 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
Hi Marcel, > -----Original Message----- > From: Marcel Holtmann <marcel@holtmann.org> > Sent: Thursday, June 17, 2021 3:50 PM > To: K, Kiran <kiran.k@intel.com> > Cc: linux-bluetooth@vger.kernel.org > 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@intel.com> > >>>>> Reviewed-by: Chethan T N <chethan.tumkur.narayan@intel.com> > >>>>> Reviewed-by: Srivatsa Ravishankar <ravishankar.srivatsa@intel.com> > >>>>> --- > >>>>> 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@holtmann.org>"); > >>>>> 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. Agree. I will make the suggested changes in the next patchset. > > Regards > > Marcel Thanks, Kiran
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@holtmann.org>"); 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; + #define HCI_OP_READ_LOCAL_VERSION 0x1001 struct hci_rp_read_local_version { __u8 status;