Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <[email protected]>
> Sent: Thursday, June 17, 2021 3:50 PM
> To: K, Kiran <[email protected]>
> Cc: [email protected]
> 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 <[email protected]>
> >>>>> Reviewed-by: Chethan T N <[email protected]>
> >>>>> Reviewed-by: Srivatsa Ravishankar <[email protected]>
> >>>>> ---
> >>>>> 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 <[email protected]>");
> >>>>> 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

2021-06-23 03:25:26

by Kiran K

[permalink] [raw]

Subject: RE: [PATCH v9 10/10] Bluetooth: Add support for msbc coding format

Hi Marcel,

> -----Original Message-----
> From: Marcel Holtmann <[email protected]>
> Sent: Wednesday, June 16, 2021 1:14 AM
> To: K, Kiran <[email protected]>
> Cc: [email protected]
> Subject: Re: [PATCH v9 10/10] Bluetooth: Add support for msbc coding
> format
>
> Hi Kiran,
>
> > In Enhanced_Setup_Synchronous_Command, add support for msbc coding
> > format
> >
> > Signed-off-by: Kiran K <[email protected]>
> > Reviewed-by: Chethan T N <[email protected]>
> > Reviewed-by: Srivatsa Ravishankar <[email protected]>
> > ---
> > include/net/bluetooth/bluetooth.h | 1 +
> > net/bluetooth/hci_conn.c | 27 ++++++++++++++++++++++++++-
> > 2 files changed, 27 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/bluetooth/bluetooth.h
> > b/include/net/bluetooth/bluetooth.h
> > index af2809121571..056699224da7 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -175,6 +175,7 @@ struct bt_codecs {
> >
> > #define CODING_FORMAT_CVSD 0x02
> > #define CODING_FORMAT_TRANSPARENT 0x03
> > +#define CODING_FORMAT_MSBC 0x05
> >
> > __printf(1, 2)
> > void bt_info(const char *fmt, ...);
> > diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c index
> > 9569b21adb88..73c134459361 100644
> > --- a/net/bluetooth/hci_conn.c
> > +++ b/net/bluetooth/hci_conn.c
> > @@ -328,6 +328,32 @@ bool hci_enhanced_setup_sync(struct hci_conn
> *conn, __u16 handle)
> > cp.rx_bandwidth = cpu_to_le32(0x00001f40);
> >
> > switch (conn->codec.id) {
> > + case CODING_FORMAT_MSBC:
> > + if (!find_next_esco_param(conn, esco_param_msbc,
> > + ARRAY_SIZE(esco_param_msbc)))
> > + return false;
> > +
> > + param = &esco_param_msbc[conn->attempt - 1];
> > + cp.tx_coding_format.id = 0x05;
> > + cp.rx_coding_format.id = 0x05;
> > + cp.tx_codec_frame_size = __cpu_to_le16(60);
> > + cp.rx_codec_frame_size = __cpu_to_le16(60);
> > + cp.in_bandwidth = __cpu_to_le32(32000);
> > + cp.out_bandwidth = __cpu_to_le32(32000);
> > + cp.in_coding_format.id = 0x04;
> > + cp.out_coding_format.id = 0x04;
> > + cp.in_coded_data_size = __cpu_to_le16(16);
> > + cp.out_coded_data_size = __cpu_to_le16(16);
> > + cp.in_pcm_data_format = 2;
> > + cp.out_pcm_data_format = 2;
> > + cp.in_pcm_sample_payload_msb_pos = 0;
> > + cp.out_pcm_sample_payload_msb_pos = 0;
> > + cp.in_data_path = conn->codec.data_path;
> > + cp.out_data_path = conn->codec.data_path;
> > + cp.in_trasnport_unit_size = 1;
> > + cp.out_trasnport_unit_size = 1;
>
> so s/trasnport/transport/
>
> Please spellcheck your structs.

Ack.

>
> > + break;
> > +
> > case CODING_FORMAT_TRANSPARENT:
> > if (!find_next_esco_param(conn, esco_param_msbc,
> > ARRAY_SIZE(esco_param_msbc)))
> > @@ -383,7 +409,6 @@ bool hci_enhanced_setup_sync(struct hci_conn
> *conn, __u16 handle)
> > cp.in_trasnport_unit_size = 16;
> > cp.out_trasnport_unit_size = 16;
> > break;
> > -
>
> We can not have these random hunks in patches. You need to review your
> final set before sending it out.

Ack.

>
> Regards
>
> Marcel

Thanks,
Kiran