MIME-Version: 1.0
References: <20211119082027.12809-1-kiran.k@intel.com> <20211119082027.12809-5-kiran.k@intel.com>
In-Reply-To: <20211119082027.12809-5-kiran.k@intel.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Tue, 23 Nov 2021 15:49:36 -0800
Message-ID: <CABBYNZ+M_LixdS9p=BBiuSR1NvbUpp8HGTxEX_Npgf7UsjeB8A@mail.gmail.com>
Subject: Re: [PATCH v4 05/14] Bluetooth: btintel: Add support to fetch data
 path id for a2dp offload
To:     Kiran K <kiran.k@intel.com>
Cc:     "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>,
        "Srivatsa, Ravishankar" <ravishankar.srivatsa@intel.com>,
        Chethan T N <chethan.tumkur.narayan@intel.com>,
        Luiz Augusto Von Dentz <luiz.von.dentz@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Kiran,

On Fri, Nov 19, 2021 at 12:16 AM Kiran K <kiran.k@intel.com> wrote:
>
> During *setup*, when configuring offload, set get_data_path_id callback
> function and support fetching of data path id for a2dp offload use case.
>
> 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 | 28 +++++++++++++++++++++++-----
>  1 file changed, 23 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bluetooth/btintel.c b/drivers/bluetooth/btintel.c
> index 4b6d7ea08425..1501376ccf72 100644
> --- a/drivers/bluetooth/btintel.c
> +++ b/drivers/bluetooth/btintel.c
> @@ -2214,16 +2214,30 @@ static int btintel_get_data_path_id(struct hci_dev *hdev, __u8 transport,
>  {
>         struct btintel_data *intel_data;
>
> -       if (transport != HCI_TRANSPORT_SCO_ESCO)
> +       if (transport != HCI_TRANSPORT_SCO_ESCO &&
> +           transport != HCI_TRANSPORT_ACL) {
> +               bt_dev_err(hdev, "Invalid transport type %u", transport);
>                 return -EINVAL;
> +       }
>
>         intel_data = hci_get_priv((hdev));
>
> -       if (intel_data->use_cases.preset[0] & 0x03) {
> -               /* Intel uses 1 as data path id for all the usecases */
> -               *data_path_id = 1;
> -               return 0;
> +       switch (transport) {
> +       case HCI_TRANSPORT_SCO_ESCO:
> +               if (intel_data->use_cases.preset[0] & 0x03) {
> +                       *data_path_id = 1;
> +                       return 0;
> +               }
> +               break;
> +       case HCI_TRANSPORT_ACL:
> +               if (intel_data->use_cases.preset[0] & 0x08) {
> +                       *data_path_id = 1;
> +                       return 0;
> +               }

I would suggest adding the bits of use_cases as defines using BIT
macro, it may also be a good idea to check if the preset should
actually be a bitmap and then use the likes of test_bit, etc.

> +               break;
>         }
> +       bt_dev_err(hdev, "Required preset is not supported 0x%02x",
> +                  intel_data->use_cases.preset[0]);
>         return  -EOPNOTSUPP;
>  }
>
> @@ -2262,6 +2276,10 @@ static int btintel_configure_offload(struct hci_dev *hdev)
>                 hdev->get_codec_config_data = btintel_get_codec_config_data;
>         }
>
> +       /* supports SBC codec for a2dp offload */
> +       if (use_cases->preset[0] & 0x08)
> +               hdev->get_data_path_id = btintel_get_data_path_id;

This seems to be checking only for one of the use cases. How about
SCO/ESCO, doesn't that use get_data_path_id as well?

>  error:
>         kfree_skb(skb);
>         return err;
> --
> 2.17.1
>


-- 
Luiz Augusto von Dentz