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

[Marcel Holtmann]

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?

can we do the quirk setting in btrtl_setup_realtek() instead of creating another exported function.

Regards

Marcel