Re: [PATCH v2 1/3] Bluetooth: mt7921s: Support wake on bluetooth

Re: [PATCH v2 1/3] Bluetooth: mt7921s: Support wake on bluetooth

[sean.wang]

From: Sean Wang <sean.wang@mediatek.com>

>Dear Sean,
>
>Am 22.12.21 um 04:11 schrieb sean.wang@mediatek.com:
>> From: Mark Chen <mark-yw.chen@mediatek.com>
>>
>> Enable wake on bluetooth on mt7921s that can be supported since the
>> firmware with version 20211129211059 was added, and the patch would
>> not cause any harm even when the old firmware is applied.
>
>Maybe print a notice level message, if the firmware is too old, and the feature cannot be supported?
>

mt7921 firmware will be updated regularly to add features or fix bugs, so I would try to avoid adding such
message related to firmware to make the driver look messy.

>> The patch was tested by setting up an HID or HOGP profile to connect a
>> Bluetooth keyboard and mouse, then putting the system to suspend, then
>> trying to wake up the system by moving the Bluetooth keyboard or
>> mouse, and then checking if the system can wake up and be brought back
>> to the normal state.
>
>I’d still would like to see the datasheet name, revision, and section in the commit message, even though the datasheet is not public.
>

actually the hardware datasheet doesn't mention the things regarding wake on bluetooth because that is a totally software feature
extended from the previous firmware.

>> Co-developed-by: Sean Wang <sean.wang@mediatek.com>
>> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
>> Signed-off-by: Mark Chen <mark-yw.chen@mediatek.com>
>> ---
>> v2: refine the git message
>> ---
>>   drivers/bluetooth/btmtk.h     |  8 ++++++++
>>   drivers/bluetooth/btmtksdio.c | 31 ++++++++++++++++++++++++++++++-
>>   2 files changed, 38 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bluetooth/btmtk.h b/drivers/bluetooth/btmtk.h
>> index 6e7b0c7567c0..2be1d2680ad8 100644
>> --- a/drivers/bluetooth/btmtk.h
>> +++ b/drivers/bluetooth/btmtk.h
>> @@ -68,6 +68,14 @@ struct btmtk_tci_sleep {
>>	u8 time_compensation;
>>   } __packed;
>>
>> +struct btmtk_wakeon {
>> +	u8 mode;
>> +	u8 gpo;
>> +	u8 active_high;
>> +	__le16 enable_delay;
>> +	__le16 wakeup_delay;
>> +} __packed;
>> +
>>   struct btmtk_hci_wmt_params {
>>	u8 op;
>>	u8 flag;
>> diff --git a/drivers/bluetooth/btmtksdio.c
>> b/drivers/bluetooth/btmtksdio.c index b5ea8d3bffaa..771733ce362b
>> 100644
>> --- a/drivers/bluetooth/btmtksdio.c
>> +++ b/drivers/bluetooth/btmtksdio.c
>> @@ -958,6 +958,30 @@ static int btmtksdio_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
>>	return 0;
>>   }
>>
>> +static bool btmtk_sdio_wakeup(struct hci_dev *hdev) {
>> +	struct btmtksdio_dev *bdev = hci_get_drvdata(hdev);
>> +	bool may_wakeup = device_may_wakeup(bdev->dev);
>> +	struct btmtk_wakeon bt_awake = {
>> +		.mode = 0x1,
>> +		.gpo = 0,
>> +		.active_high = 0x1,
>> +		.enable_delay = cpu_to_le16(0xc80),
>> +		.wakeup_delay = cpu_to_le16(0x20)
>> +	};
>> +	struct sk_buff *skb;
>> +
>> +	if (may_wakeup &&
>> +	    bdev->data->chipid == 0x7921) {
>> +		skb =  __hci_cmd_sync(hdev, 0xfc27, sizeof(bt_awake),
>> +				      &bt_awake, HCI_CMD_TIMEOUT);
>> +		if (IS_ERR(skb))
>> +			may_wakeup = false;
>> +	}
>> +
>> +	return may_wakeup;
>> +}
>> +
>>   static int btmtksdio_probe(struct sdio_func *func,
>>			   const struct sdio_device_id *id)
>>   {
>> @@ -998,6 +1022,7 @@ static int btmtksdio_probe(struct sdio_func *func,
>>	hdev->shutdown = btmtksdio_shutdown;
>>	hdev->send     = btmtksdio_send_frame;
>>	hdev->set_bdaddr = btmtk_set_bdaddr;
>> +	hdev->wakeup = btmtk_sdio_wakeup;
>
>Just a nit, that two and three lines above, the equal signs are aligned.

thanks, I will do it with another patch

>
>>	SET_HCIDEV_DEV(hdev, &func->dev);
>>
>> @@ -1032,7 +1057,11 @@ static int btmtksdio_probe(struct sdio_func *func,
>>	 */
>>	pm_runtime_put_noidle(bdev->dev);
>>
>> -	return 0;
>> +	err = device_init_wakeup(bdev->dev, true);
>> +	if (err)
>> +		bt_dev_err(hdev, "%s: failed to init_wakeup", __func__);
>> +
>> +	return err;
>>   }
>>
>>   static void btmtksdio_remove(struct sdio_func *func)
>
>
>Kind regards,
>
>Paul

[PATCH v2 1/3] Bluetooth: mt7921s: Support wake on bluetooth

[sean.wang]

From: Mark Chen <mark-yw.chen@mediatek.com>

Enable wake on bluetooth on mt7921s that can be supported since the
firmware with version 20211129211059 was added, and the patch would
not cause any harm even when the old firmware is applied.

The patch was tested by setting up an HID or HOGP profile to connect a
Bluetooth keyboard and mouse, then putting the system to suspend, then
trying to wake up the system by moving the Bluetooth keyboard or mouse,
and then checking if the system can wake up and be brought back to
the normal state.

Co-developed-by: Sean Wang <sean.wang@mediatek.com>
Signed-off-by: Sean Wang <sean.wang@mediatek.com>
Signed-off-by: Mark Chen <mark-yw.chen@mediatek.com>
---
v2: refine the git message
---
 drivers/bluetooth/btmtk.h     |  8 ++++++++
 drivers/bluetooth/btmtksdio.c | 31 ++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/bluetooth/btmtk.h b/drivers/bluetooth/btmtk.h
index 6e7b0c7567c0..2be1d2680ad8 100644
--- a/drivers/bluetooth/btmtk.h
+++ b/drivers/bluetooth/btmtk.h
@@ -68,6 +68,14 @@ struct btmtk_tci_sleep {
 	u8 time_compensation;
 } __packed;
 
+struct btmtk_wakeon {
+	u8 mode;
+	u8 gpo;
+	u8 active_high;
+	__le16 enable_delay;
+	__le16 wakeup_delay;
+} __packed;
+
 struct btmtk_hci_wmt_params {
 	u8 op;
 	u8 flag;
diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
index b5ea8d3bffaa..771733ce362b 100644
--- a/drivers/bluetooth/btmtksdio.c
+++ b/drivers/bluetooth/btmtksdio.c
@@ -958,6 +958,30 @@ static int btmtksdio_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
 	return 0;
 }
 
+static bool btmtk_sdio_wakeup(struct hci_dev *hdev)
+{
+	struct btmtksdio_dev *bdev = hci_get_drvdata(hdev);
+	bool may_wakeup = device_may_wakeup(bdev->dev);
+	struct btmtk_wakeon bt_awake = {
+		.mode = 0x1,
+		.gpo = 0,
+		.active_high = 0x1,
+		.enable_delay = cpu_to_le16(0xc80),
+		.wakeup_delay = cpu_to_le16(0x20)
+	};
+	struct sk_buff *skb;
+
+	if (may_wakeup &&
+	    bdev->data->chipid == 0x7921) {
+		skb =  __hci_cmd_sync(hdev, 0xfc27, sizeof(bt_awake),
+				      &bt_awake, HCI_CMD_TIMEOUT);
+		if (IS_ERR(skb))
+			may_wakeup = false;
+	}
+
+	return may_wakeup;
+}
+
 static int btmtksdio_probe(struct sdio_func *func,
 			   const struct sdio_device_id *id)
 {
@@ -998,6 +1022,7 @@ static int btmtksdio_probe(struct sdio_func *func,
 	hdev->shutdown = btmtksdio_shutdown;
 	hdev->send     = btmtksdio_send_frame;
 	hdev->set_bdaddr = btmtk_set_bdaddr;
+	hdev->wakeup = btmtk_sdio_wakeup;
 
 	SET_HCIDEV_DEV(hdev, &func->dev);
 
@@ -1032,7 +1057,11 @@ static int btmtksdio_probe(struct sdio_func *func,
 	 */
 	pm_runtime_put_noidle(bdev->dev);
 
-	return 0;
+	err = device_init_wakeup(bdev->dev, true);
+	if (err)
+		bt_dev_err(hdev, "%s: failed to init_wakeup", __func__);
+
+	return err;
 }
 
 static void btmtksdio_remove(struct sdio_func *func)
-- 
2.25.1

Re: [PATCH v2 1/3] Bluetooth: mt7921s: Support wake on bluetooth

[Marcel Holtmann]

Hi Sean,

> Enable wake on bluetooth on mt7921s that can be supported since the
> firmware with version 20211129211059 was added, and the patch would
> not cause any harm even when the old firmware is applied.
> 
> The patch was tested by setting up an HID or HOGP profile to connect a
> Bluetooth keyboard and mouse, then putting the system to suspend, then
> trying to wake up the system by moving the Bluetooth keyboard or mouse,
> and then checking if the system can wake up and be brought back to
> the normal state.
> 
> Co-developed-by: Sean Wang <sean.wang@mediatek.com>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> Signed-off-by: Mark Chen <mark-yw.chen@mediatek.com>
> ---
> v2: refine the git message
> ---
> drivers/bluetooth/btmtk.h     |  8 ++++++++
> drivers/bluetooth/btmtksdio.c | 31 ++++++++++++++++++++++++++++++-
> 2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btmtk.h b/drivers/bluetooth/btmtk.h
> index 6e7b0c7567c0..2be1d2680ad8 100644
> --- a/drivers/bluetooth/btmtk.h
> +++ b/drivers/bluetooth/btmtk.h
> @@ -68,6 +68,14 @@ struct btmtk_tci_sleep {
> 	u8 time_compensation;
> } __packed;
> 
> +struct btmtk_wakeon {
> +	u8 mode;
> +	u8 gpo;
> +	u8 active_high;
> +	__le16 enable_delay;
> +	__le16 wakeup_delay;
> +} __packed;
> +
> struct btmtk_hci_wmt_params {
> 	u8 op;
> 	u8 flag;
> diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
> index b5ea8d3bffaa..771733ce362b 100644
> --- a/drivers/bluetooth/btmtksdio.c
> +++ b/drivers/bluetooth/btmtksdio.c
> @@ -958,6 +958,30 @@ static int btmtksdio_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
> 	return 0;
> }
> 
> +static bool btmtk_sdio_wakeup(struct hci_dev *hdev)
> +{
> +	struct btmtksdio_dev *bdev = hci_get_drvdata(hdev);
> +	bool may_wakeup = device_may_wakeup(bdev->dev);
> +	struct btmtk_wakeon bt_awake = {
> +		.mode = 0x1,
> +		.gpo = 0,
> +		.active_high = 0x1,
> +		.enable_delay = cpu_to_le16(0xc80),
> +		.wakeup_delay = cpu_to_le16(0x20)
> +	};
> +	struct sk_buff *skb;
> +
> +	if (may_wakeup &&
> +	    bdev->data->chipid == 0x7921) {

this doesn’t fit in a single line?

> +		skb =  __hci_cmd_sync(hdev, 0xfc27, sizeof(bt_awake),
> +				      &bt_awake, HCI_CMD_TIMEOUT);
> +		if (IS_ERR(skb))
> +			may_wakeup = false;

Memory leak?

Any why not move the skb variable into the scope where it is used.

> +	}
> +
> +	return may_wakeup;
> +}
> +
> static int btmtksdio_probe(struct sdio_func *func,
> 			   const struct sdio_device_id *id)
> {
> @@ -998,6 +1022,7 @@ static int btmtksdio_probe(struct sdio_func *func,
> 	hdev->shutdown = btmtksdio_shutdown;
> 	hdev->send     = btmtksdio_send_frame;
> 	hdev->set_bdaddr = btmtk_set_bdaddr;
> +	hdev->wakeup = btmtk_sdio_wakeup;
> 
> 	SET_HCIDEV_DEV(hdev, &func->dev);
> 
> @@ -1032,7 +1057,11 @@ static int btmtksdio_probe(struct sdio_func *func,
> 	 */
> 	pm_runtime_put_noidle(bdev->dev);
> 
> -	return 0;
> +	err = device_init_wakeup(bdev->dev, true);
> +	if (err)
> +		bt_dev_err(hdev, "%s: failed to init_wakeup", __func__);
> +
> +	return err;
> }
> 
> static void btmtksdio_remove(struct sdio_func *func)

Regards

Marcel

Re: [PATCH v2 1/3] Bluetooth: mt7921s: Support wake on bluetooth

[Paul Menzel]

Dear Sean,


Am 22.12.21 um 04:11 schrieb sean.wang@mediatek.com:
> From: Mark Chen <mark-yw.chen@mediatek.com>
> 
> Enable wake on bluetooth on mt7921s that can be supported since the
> firmware with version 20211129211059 was added, and the patch would
> not cause any harm even when the old firmware is applied.

Maybe print a notice level message, if the firmware is too old, and the 
feature cannot be supported?

> The patch was tested by setting up an HID or HOGP profile to connect a
> Bluetooth keyboard and mouse, then putting the system to suspend, then
> trying to wake up the system by moving the Bluetooth keyboard or mouse,
> and then checking if the system can wake up and be brought back to
> the normal state.

I’d still would like to see the datasheet name, revision, and section in 
the commit message, even though the datasheet is not public.

> Co-developed-by: Sean Wang <sean.wang@mediatek.com>
> Signed-off-by: Sean Wang <sean.wang@mediatek.com>
> Signed-off-by: Mark Chen <mark-yw.chen@mediatek.com>
> ---
> v2: refine the git message
> ---
>   drivers/bluetooth/btmtk.h     |  8 ++++++++
>   drivers/bluetooth/btmtksdio.c | 31 ++++++++++++++++++++++++++++++-
>   2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bluetooth/btmtk.h b/drivers/bluetooth/btmtk.h
> index 6e7b0c7567c0..2be1d2680ad8 100644
> --- a/drivers/bluetooth/btmtk.h
> +++ b/drivers/bluetooth/btmtk.h
> @@ -68,6 +68,14 @@ struct btmtk_tci_sleep {
>   	u8 time_compensation;
>   } __packed;
>   
> +struct btmtk_wakeon {
> +	u8 mode;
> +	u8 gpo;
> +	u8 active_high;
> +	__le16 enable_delay;
> +	__le16 wakeup_delay;
> +} __packed;
> +
>   struct btmtk_hci_wmt_params {
>   	u8 op;
>   	u8 flag;
> diff --git a/drivers/bluetooth/btmtksdio.c b/drivers/bluetooth/btmtksdio.c
> index b5ea8d3bffaa..771733ce362b 100644
> --- a/drivers/bluetooth/btmtksdio.c
> +++ b/drivers/bluetooth/btmtksdio.c
> @@ -958,6 +958,30 @@ static int btmtksdio_send_frame(struct hci_dev *hdev, struct sk_buff *skb)
>   	return 0;
>   }
>   
> +static bool btmtk_sdio_wakeup(struct hci_dev *hdev)
> +{
> +	struct btmtksdio_dev *bdev = hci_get_drvdata(hdev);
> +	bool may_wakeup = device_may_wakeup(bdev->dev);
> +	struct btmtk_wakeon bt_awake = {
> +		.mode = 0x1,
> +		.gpo = 0,
> +		.active_high = 0x1,
> +		.enable_delay = cpu_to_le16(0xc80),
> +		.wakeup_delay = cpu_to_le16(0x20)
> +	};
> +	struct sk_buff *skb;
> +
> +	if (may_wakeup &&
> +	    bdev->data->chipid == 0x7921) {
> +		skb =  __hci_cmd_sync(hdev, 0xfc27, sizeof(bt_awake),
> +				      &bt_awake, HCI_CMD_TIMEOUT);
> +		if (IS_ERR(skb))
> +			may_wakeup = false;
> +	}
> +
> +	return may_wakeup;
> +}
> +
>   static int btmtksdio_probe(struct sdio_func *func,
>   			   const struct sdio_device_id *id)
>   {
> @@ -998,6 +1022,7 @@ static int btmtksdio_probe(struct sdio_func *func,
>   	hdev->shutdown = btmtksdio_shutdown;
>   	hdev->send     = btmtksdio_send_frame;
>   	hdev->set_bdaddr = btmtk_set_bdaddr;
> +	hdev->wakeup = btmtk_sdio_wakeup;

Just a nit, that two and three lines above, the equal signs are aligned.

>   	SET_HCIDEV_DEV(hdev, &func->dev);
>   
> @@ -1032,7 +1057,11 @@ static int btmtksdio_probe(struct sdio_func *func,
>   	 */
>   	pm_runtime_put_noidle(bdev->dev);
>   
> -	return 0;
> +	err = device_init_wakeup(bdev->dev, true);
> +	if (err)
> +		bt_dev_err(hdev, "%s: failed to init_wakeup", __func__);
> +
> +	return err;
>   }
>   
>   static void btmtksdio_remove(struct sdio_func *func)


Kind regards,

Paul