Hi Marcel,
On Wed, 26 May 2021 at 22:59, 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?
>
> 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().
BTW I just noticed I missed a declaration in btrtl.h, so I will send a
v2 patch to fix it.
Cheers,
Archie
