Re: [PATCH] Bluetooth: hci_h5: Add RTL8822CS capabilities

[Archie Pusaka]

Hi Marcel,

On Fri, 21 May 2021 at 00:02, Archie Pusaka <apusaka@xxxxxxxxxx> wrote:
>
> Hi Marcel,
>
> On Thu, 20 May 2021 at 23:18, Marcel Holtmann <marcel@xxxxxxxxxxxx> wrote:
> >
> > Hi Archie,
> >
> > >>>>>> RTL8822 chipset supports WBS, and this information is conveyed in
> > >>>>>> btusb.c. However, the UART driver doesn't have this information just
> > >>>>>> yet.
> > >>>>>>
> > >>>>>> Signed-off-by: Archie Pusaka <apusaka@xxxxxxxxxxxx>
> > >>>>>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@xxxxxxxxxxxx>
> > >>>>>> ---
> > >>>>>>
> > >>>>>> drivers/bluetooth/btrtl.c  | 26 ++++++++++++++++----------
> > >>>>>> drivers/bluetooth/btrtl.h  |  2 ++
> > >>>>>> drivers/bluetooth/hci_h5.c |  5 +----
> > >>>>>> 3 files changed, 19 insertions(+), 14 deletions(-)
> > >>>>>>
> > >>>>>> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> > >>>>>> index e7fe5fb22753..988a09860c6b 100644
> > >>>>>> --- a/drivers/bluetooth/btrtl.c
> > >>>>>> +++ b/drivers/bluetooth/btrtl.c
> > >>>>>> @@ -719,17 +719,8 @@ int btrtl_download_firmware(struct hci_dev *hdev,
> > >>>>>> }
> > >>>>>> EXPORT_SYMBOL_GPL(btrtl_download_firmware);
> > >>>>>>
> > >>>>>> -int btrtl_setup_realtek(struct hci_dev *hdev)
> > >>>>>> +void btrtl_set_quirks(struct hci_dev *hdev, struct btrtl_device_info *btrtl_dev)
> > >>>>>> {
> > >>>>>> -     struct btrtl_device_info *btrtl_dev;
> > >>>>>> -     int ret;
> > >>>>>> -
> > >>>>>> -     btrtl_dev = btrtl_initialize(hdev, NULL);
> > >>>>>> -     if (IS_ERR(btrtl_dev))
> > >>>>>> -             return PTR_ERR(btrtl_dev);
> > >>>>>> -
> > >>>>>> -     ret = btrtl_download_firmware(hdev, btrtl_dev);
> > >>>>>> -
> > >>>>>>     /* Enable controller to do both LE scan and BR/EDR inquiry
> > >>>>>>      * simultaneously.
> > >>>>>>      */
> > >>>>>> @@ -750,6 +741,21 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
> > >>>>>>             rtl_dev_dbg(hdev, "WBS supported not enabled.");
> > >>>>>>             break;
> > >>>>>>     }
> > >>>>>> +}
> > >>>>>> +EXPORT_SYMBOL_GPL(btrtl_set_quirks);
> > >>>>>> +
> > >>>>>> +int btrtl_setup_realtek(struct hci_dev *hdev)
> > >>>>>> +{
> > >>>>>> +     struct btrtl_device_info *btrtl_dev;
> > >>>>>> +     int ret;
> > >>>>>> +
> > >>>>>> +     btrtl_dev = btrtl_initialize(hdev, NULL);
> > >>>>>> +     if (IS_ERR(btrtl_dev))
> > >>>>>> +             return PTR_ERR(btrtl_dev);
> > >>>>>> +
> > >>>>>> +     ret = btrtl_download_firmware(hdev, btrtl_dev);
> > >>>>>> +
> > >>>>>> +     btrtl_set_quirks(hdev, btrtl_dev);
> > >>>>>>
> > >>>>>>     btrtl_free(btrtl_dev);
> > >>>>>>     return ret;
> > >>>>>> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
> > >>>>>> index 2a582682136d..260167f01b08 100644
> > >>>>>> --- a/drivers/bluetooth/btrtl.h
> > >>>>>> +++ b/drivers/bluetooth/btrtl.h
> > >>>>>> @@ -54,6 +54,8 @@ struct btrtl_device_info *btrtl_initialize(struct hci_dev *hdev,
> > >>>>>> void btrtl_free(struct btrtl_device_info *btrtl_dev);
> > >>>>>> int btrtl_download_firmware(struct hci_dev *hdev,
> > >>>>>>                         struct btrtl_device_info *btrtl_dev);
> > >>>>>> +void btrtl_set_quirks(struct hci_dev *hdev,
> > >>>>>> +                   struct btrtl_device_info *btrtl_dev);
> > >>>>>> int btrtl_setup_realtek(struct hci_dev *hdev);
> > >>>>>> int btrtl_shutdown_realtek(struct hci_dev *hdev);
> > >>>>>> int btrtl_get_uart_settings(struct hci_dev *hdev,
> > >>>>>> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> > >>>>>> index 27e96681d583..e0520639f4ba 100644
> > >>>>>> --- a/drivers/bluetooth/hci_h5.c
> > >>>>>> +++ b/drivers/bluetooth/hci_h5.c
> > >>>>>> @@ -906,10 +906,7 @@ static int h5_btrtl_setup(struct h5 *h5)
> > >>>>>>     /* Give the device some time before the hci-core sends it a reset */
> > >>>>>>     usleep_range(10000, 20000);
> > >>>>>>
> > >>>>>> -     /* Enable controller to do both LE scan and BR/EDR inquiry
> > >>>>>> -      * simultaneously.
> > >>>>>> -      */
> > >>>>>> -     set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks);
> > >>>>>> +     btrtl_set_quirks(h5->hu->hdev, btrtl_dev);
> > >>>>>
> > >>>>> any reason why not just setting WBS quirk here?
> > >>>>
> > >>>> Hmm, I think WBS is the feature of the chipset and not the transport.
> > >>>> Therefore isn't it better to just have it set in one place?
> > >>>> Setting the quirks here means we need to copy paste the settings from btrtl.c.
> > >>>
> > >>> but since you are already setting HCI_QUIRK_SIMULTANEOUS_DISCOVERY right now, I don’t see the difference.
> > >>
> > >> Sorry, I don't get what you mean.
> > >> With this patch I also moved HCI_QUIRK_SIMULTANEOUS_DISCOVERY into
> > >> btrtl.c, so it's together with the WBS quirk.
> > >>
> > >>> Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling.
> > >>
> > >> To be honest, I am not aware about the story of the broken erroneous
> > >> packet flag.
> > >> Last time I checked I still needed the quirk to have RTL8822 on UART
> > >> properly run WBS, but that was months ago...
> > >> Let me verify whether this quirk is still needed.
> > >
> > > It looks like we still need the WBS quirk because otherwise the host
> > > wouldn't know whether the controller supports WBS or not. It's used in
> > > get_supported_settings() in mgmt.c.
> >
> > and why not set it unconditionally for all Realtek chips?
>
> Not all Realtek chips supports WBS, therefore
> HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED is only set on some of them.

Are there any other concerns you might have?

Cheers,
Archie