Re: [PATCH RESEND 1/3] Bluetooth: btmtksdio: add the support of wake on bluetooth

[sean.wang at mediatek.com]

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


>Dear Sean, dear Mark,
>
>
>Am 18.12.21 um 02:08 schrieb sean.wang at mediatek.com:
>> From: Mark Chen <mark-yw.chen at mediatek.com>
>
>Shorter summary: Bluetooth: btmtksdio: Support wake on bluetooth

seemed nice to me, will do in the next version

>
>> Add the support to enable wake on bluetooth
>
>Can you please list the datasheet name and revision used for the implementation.

the datasheet is not released in public so Mark and I cannot provide the detail here, but we will make the patch easy to review and understand.

>Please document how you tested this.

It 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 be waken up and be brought back to the normal state.

>
>> Co-developed-by: Sean Wang <sean.wang at mediatek.com>
>> Signed-off-by: Sean Wang <sean.wang at mediatek.com>
>> Signed-off-by: Mark Chen <mark-yw.chen at mediatek.com>
>> ---
>>   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) {
>
>Why the check for 0x7921? Is that device only supported? The commit message does not say anything about it.

currently, wake on bt is only supported by mt7921, we will change the subject to "Bluetooth: mt7921s: Support wake on bluetooth"
and refine the message body to reflect the fact.

>
>> +		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__);
>
>Could the error message be extended, so normal users know what to do?
>Currently it’s only useful for developers.

That is done by referring to btusb.c. I have willing to enhance the part along with btusb.c if Marcel has any idea about that.

>
>> +
>> +	return err;
>>   }
>>
>>   static void btmtksdio_remove(struct sdio_func *func)
>
>
>Kind regards,
>
>Paul
>
>