Hi Luiz,
> -----Original Message-----
> From: Luiz Augusto von Dentz <luiz.dentz@xxxxxxxxx>
> Sent: Thursday, September 2, 2021 5:24 AM
> To: K, Kiran <kiran.k@xxxxxxxxx>
> Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Tumkur Narayan, Chethan
> <chethan.tumkur.narayan@xxxxxxxxx>
> Subject: Re: [PATCH v13 12/12] Bluetooth: Allow usb to auto-suspend when SCO
> use non-HCI transport
>
> Hi Kiran,
>
> On Tue, Aug 31, 2021 at 4:54 AM Kiran K <kiran.k@xxxxxxxxx> wrote:
> >
> > From: Chethan T N <chethan.tumkur.narayan@xxxxxxxxx>
> >
> > Currently usb tranport is not allowed to suspend when SCO over HCI
> > tranport is active.
> >
> > This patch shall enable the usb tranport to suspend when SCO link use
> > non-HCI transport
> >
> > Signed-off-by: Chethan T N <chethan.tumkur.narayan@xxxxxxxxx>
> > Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
> > ---
> >
> > Notes:
> > changes in v13:
> > - suspend usb in HFP offload use case
> >
> > drivers/bluetooth/btintel.c | 2 +-
> > include/net/bluetooth/bluetooth.h | 4 ++++
> > net/bluetooth/hci_event.c | 20 +++++++++++---------
> > net/bluetooth/sco.c | 2 +-
> > 4 files changed, 17 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> > index 6091b691ddc2..2d64e289cf6e 100644
> > --- a/drivers/bluetooth/btintel.c
> > +++ b/drivers/bluetooth/btintel.c
> > @@ -2215,7 +2215,7 @@ static int btintel_get_codec_config_data(struct
> > hci_dev *hdev, static int btintel_get_data_path_id(struct hci_dev
> > *hdev, __u8 *data_path_id) {
> > /* Intel uses 1 as data path id for all the usecases */
> > - *data_path_id = 1;
> > + *data_path_id = BT_SCO_PCM_PATH;
> > return 0;
> > }
> >
> > diff --git a/include/net/bluetooth/bluetooth.h
> > b/include/net/bluetooth/bluetooth.h
> > index c1fa90fb7502..9e2745863b33 100644
> > --- a/include/net/bluetooth/bluetooth.h
> > +++ b/include/net/bluetooth/bluetooth.h
> > @@ -177,6 +177,10 @@ struct bt_codecs {
> > #define CODING_FORMAT_TRANSPARENT 0x03
> > #define CODING_FORMAT_MSBC 0x05
> >
> > +/* Audio data transport path used for SCO */ #define BT_SCO_HCI_PATH
> > +0x00 #define BT_SCO_PCM_PATH 0x01
>
> If this is in fact vendor specific perhaps you should be declared in btintel.h not
> here.
This is defined the Host Controller Interface assigned numbers, page no.3 "Transport Layer"- https://btprodspecificationrefs.blob.core.windows.net/assigned-numbers/Assigned%20Number%20Types/Host%20Controller%20Interface.pdf. So defined in bluetooth.h, let me know if you think otherwise.
>
> > +
> > __printf(1, 2)
> > void bt_info(const char *fmt, ...);
> > __printf(1, 2)
> > diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
> > index b48e24629fa4..7ff11cba89cf 100644
> > --- a/net/bluetooth/hci_event.c
> > +++ b/net/bluetooth/hci_event.c
> > @@ -4516,15 +4516,17 @@ static void hci_sync_conn_complete_evt(struct
> > hci_dev *hdev,
> >
> > bt_dev_dbg(hdev, "SCO connected with air mode: %02x",
> > ev->air_mode);
> >
> > - switch (ev->air_mode) {
> > - case 0x02:
> > - if (hdev->notify)
> > - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> > - break;
> > - case 0x03:
> > - if (hdev->notify)
> > - hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> > - break;
> > + if (conn->codec.data_path == BT_SCO_HCI_PATH) {
> > + switch (ev->air_mode) {
> > + case 0x02:
> > + if (hdev->notify)
> > + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_CVSD);
> > + break;
> > + case 0x03:
> > + if (hdev->notify)
> > + hdev->notify(hdev, HCI_NOTIFY_ENABLE_SCO_TRANSP);
> > + break;
> > + }
>
> Hmm I think we might need to notify the driver to enable PCM routing so the
> likes of btusb can call usb_disable_endpoint/usb_enable_endpoint for example
> since in theory userspace may choose to switch from
> offload and vice-versa, note without calling usb_disable_endpoint there might
> not be much power saving after all since the endpoint will remain active or do
> we actually have a good explanation why it shall not be called when using PCM
> routing? Note that usb_set_interface will call usb_enable_interface that will
> then call usb_enable_endpoint so perhaps we need to call
> usb_disable_interface, either way we can't assume the platform will be
> restricted to only use one or the other.
Ack, Does it make sense to define and notify events "HCI_NOTIFY_DISABLE_SCO_USB_INTF ", "HCI_NOTIFY_ENABLE_SCO_USB_INTF " accordingly and handle this in btusb driver by disabling/enabling the ISOC endpoint respectively. That will serve the purpose or switch between software to hardware.
>
> > }
> >
> > hci_connect_cfm(conn, ev->status); diff --git
> > a/net/bluetooth/sco.c b/net/bluetooth/sco.c index
> > 004bce2b5eca..f35c12ca6aa5 100644
> > --- a/net/bluetooth/sco.c
> > +++ b/net/bluetooth/sco.c
> > @@ -506,7 +506,7 @@ static struct sock *sco_sock_alloc(struct net *net,
> struct socket *sock,
> > sco_pi(sk)->codec.id = CODING_FORMAT_CVSD;
> > sco_pi(sk)->codec.cid = 0xffff;
> > sco_pi(sk)->codec.vid = 0xffff;
> > - sco_pi(sk)->codec.data_path = 0x00;
> > + sco_pi(sk)->codec.data_path = BT_SCO_HCI_PATH;
> >
> > bt_sock_link(&sco_sk_list, sk);
> > return sk;
> > --
> > 2.17.1
> >
>
>
> --
> Luiz Augusto von Dentz
