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

[Marcel Holtmann <marcel@holtmann.org>]

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@chromium.org>
>>> Reviewed-by: Abhishek Pandit-Subedi <abhishekpandit@chromium.org>
>>> ---
>>> 
>>> 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.

Can we actually verify that we still need the WBS quirk. I think we fixed the broken errerrnous packet flag handling.

Regards

Marcel