2021-04-09 14:23:46

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [PATCH v2] Bluetooth: hci_h5: btrtl: Add quirk for keep power in suspend/resume

Hi Hilda,

> RTL8822C devices support BT wakeup Host. Add a quirk for these specific
> devices did not power off during suspend and resume.
> By this change, if the Host support that received BT device signal then
> it can be wakeup.
>
> Signed-off-by: hildawu <[email protected]>
> ---
> Changes in v2:
> - Add missing struct member
> - Modify title for fit length
> ---
> ---
> drivers/bluetooth/btrtl.c | 36 ------------------------------------
> drivers/bluetooth/btrtl.h | 36 ++++++++++++++++++++++++++++++++++++
> drivers/bluetooth/hci_h5.c | 35 ++++++++++++++++++++++++-----------
> include/net/bluetooth/hci.h | 9 +++++++++
> 4 files changed, 69 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index e7fe5fb22753..94d1e7885aee 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -38,42 +38,6 @@
> .hci_ver = (hciv), \
> .hci_bus = (bus)
>
> -enum btrtl_chip_id {
> - CHIP_ID_8723A,
> - CHIP_ID_8723B,
> - CHIP_ID_8821A,
> - CHIP_ID_8761A,
> - CHIP_ID_8822B = 8,
> - CHIP_ID_8723D,
> - CHIP_ID_8821C,
> - CHIP_ID_8822C = 13,
> - CHIP_ID_8761B,
> - CHIP_ID_8852A = 18,
> -};
> -
> -struct id_table {
> - __u16 match_flags;
> - __u16 lmp_subver;
> - __u16 hci_rev;
> - __u8 hci_ver;
> - __u8 hci_bus;
> - bool config_needed;
> - bool has_rom_version;
> - char *fw_name;
> - char *cfg_name;
> -};
> -
> -struct btrtl_device_info {
> - const struct id_table *ic_info;
> - u8 rom_version;
> - u8 *fw_data;
> - int fw_len;
> - u8 *cfg_data;
> - int cfg_len;
> - bool drop_fw;
> - int project_id;
> -};
> -
> static const struct id_table ic_id_table[] = {
> /* 8723A */
> { IC_INFO(RTL_ROM_LMP_8723A, 0xb, 0x6, HCI_USB),
> diff --git a/drivers/bluetooth/btrtl.h b/drivers/bluetooth/btrtl.h
> index 2a582682136d..713768b38e21 100644
> --- a/drivers/bluetooth/btrtl.h
> +++ b/drivers/bluetooth/btrtl.h
> @@ -12,6 +12,42 @@
> #define rtl_dev_info(dev, fmt, ...) bt_dev_info(dev, "RTL: " fmt, ##__VA_ARGS__)
> #define rtl_dev_dbg(dev, fmt, ...) bt_dev_dbg(dev, "RTL: " fmt, ##__VA_ARGS__)
>
> +enum btrtl_chip_id {
> + CHIP_ID_8723A,
> + CHIP_ID_8723B,
> + CHIP_ID_8821A,
> + CHIP_ID_8761A,
> + CHIP_ID_8822B = 8,
> + CHIP_ID_8723D,
> + CHIP_ID_8821C,
> + CHIP_ID_8822C = 13,
> + CHIP_ID_8761B,
> + CHIP_ID_8852A = 18,
> +};
> +
> +struct id_table {
> + __u16 match_flags;
> + __u16 lmp_subver;
> + __u16 hci_rev;
> + __u8 hci_ver;
> + __u8 hci_bus;
> + bool config_needed;
> + bool has_rom_version;
> + char *fw_name;
> + char *cfg_name;
> +};
> +
> +struct btrtl_device_info {
> + const struct id_table *ic_info;
> + u8 rom_version;
> + u8 *fw_data;
> + int fw_len;
> + u8 *cfg_data;
> + int cfg_len;
> + bool drop_fw;
> + int project_id;
> +};
> +
> struct btrtl_device_info;

I rather not move around these things.

> struct rtl_download_cmd {
> diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
> index 27e96681d583..1ca4ff89ea14 100644
> --- a/drivers/bluetooth/hci_h5.c
> +++ b/drivers/bluetooth/hci_h5.c
> @@ -909,7 +909,15 @@ static int h5_btrtl_setup(struct h5 *h5)
> /* Enable controller to do both LE scan and BR/EDR inquiry
> * simultaneously.
> */
> - set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks);
> + switch (btrtl_dev->project_id) {
> + case CHIP_ID_8822C:
> + case CHIP_ID_8852A:
> + set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &h5->hu->hdev->quirks);
> + set_bit(HCI_QUIRK_DEVICES_WAKEUP_SUPPORTED, &h5->hu->hdev->quirks);
> + break;
> + default:
> + break;
> + }
>
> out_free:
> btrtl_free(btrtl_dev);
> @@ -945,8 +953,11 @@ static void h5_btrtl_close(struct h5 *h5)
> static int h5_btrtl_suspend(struct h5 *h5)
> {
> serdev_device_set_flow_control(h5->hu->serdev, false);
> - gpiod_set_value_cansleep(h5->device_wake_gpio, 0);
> - gpiod_set_value_cansleep(h5->enable_gpio, 0);
> +
> + if (!test_bit(HCI_QUIRK_DEVICES_WAKEUP_SUPPORTED, &h5->hu->hdev->quirks)) {
> + gpiod_set_value_cansleep(h5->device_wake_gpio, 0);
> + gpiod_set_value_cansleep(h5->enable_gpio, 0);
> + }
> return 0;
> }
>
> @@ -972,17 +983,19 @@ static void h5_btrtl_reprobe_worker(struct work_struct *work)
>
> static int h5_btrtl_resume(struct h5 *h5)
> {
> - struct h5_btrtl_reprobe *reprobe;
> + if (!test_bit(HCI_QUIRK_DEVICES_WAKEUP_SUPPORTED, &h5->hu->hdev->quirks)) {
> + struct h5_btrtl_reprobe *reprobe;
>
> - reprobe = kzalloc(sizeof(*reprobe), GFP_KERNEL);
> - if (!reprobe)
> - return -ENOMEM;
> + reprobe = kzalloc(sizeof(*reprobe), GFP_KERNEL);
> + if (!reprobe)
> + return -ENOMEM;
>
> - __module_get(THIS_MODULE);
> + __module_get(THIS_MODULE);
>
> - INIT_WORK(&reprobe->work, h5_btrtl_reprobe_worker);
> - reprobe->dev = get_device(&h5->hu->serdev->dev);
> - queue_work(system_long_wq, &reprobe->work);
> + INIT_WORK(&reprobe->work, h5_btrtl_reprobe_worker);
> + reprobe->dev = get_device(&h5->hu->serdev->dev);
> + queue_work(system_long_wq, &reprobe->work);
> + }
> return 0;
> }
>
> diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
> index ea4ae551c426..1e4c2a97ab8d 100644
> --- a/include/net/bluetooth/hci.h
> +++ b/include/net/bluetooth/hci.h
> @@ -246,6 +246,15 @@ enum {
> * HCI after resume.
> */
> HCI_QUIRK_NO_SUSPEND_NOTIFIER,
> +
> + /* When this quirk is set, the controller does not power off
> + * during suspend and resume. This mechanism lets BT devices wake
> + * the Host up if the Host and chips support.
> + *
> + * This quirk can be set before hci_register_dev is called or
> + * during the hdev->setup vendor callback.
> + */
> + HCI_QUIRK_DEVICES_WAKEUP_SUPPORTED,
> };

Since this hci_uart specific, please don’t introduce another quirk and keep this internal.

Regards

Marcel