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.
>
> It cannot be done easily since the first part of btrtl_setup_realtek()
> is used exclusively for btusb, which is done differently in hci_h5.
>
> We can have it another way: define btrtl_setup_realtek_h5() to do the
> setup for h5 part in btrtl.c. This would effectively move all of
> h5_btrtl_setup() inside hci_h5.c, most notably the serdev setup. In
> turn, we don't have to expose btrtl_set_quirks(), and we can even hide
> btrtl_initialize(), btrtl_free(), and btrtl_download_firmware() inside
> btrtl.c.
> I'm not sure though why would one want that? We still need to export
> the new btrtl_setup_realtek_h5().
I am a bit disappointed that nobody took up the work on bt3wire.c so that we can have a clean serdev based driver for 3-Wire / H:5 support. That would make supporting USB and UART vendor setups from the same manufacturer a lot easier.
The consistent hci_h5.c hacking is not doing anybody any favor in the long run. It will get more and more complicated especially since the underlying core design is a line discipline. This is a hint with a massively large hammer.
Regards
Marcel
