Hi Paul,
>-----Original Message-----
>From: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
>Sent: Friday, July 25, 2025 2:35 PM
>To: K, Kiran <kiran.k@xxxxxxxxx>; Devegowda, Chandrashekar
><chandrashekar.devegowda@xxxxxxxxx>
>Cc: linux-bluetooth@xxxxxxxxxxxxxxx; Srivatsa, Ravishankar
><ravishankar.srivatsa@xxxxxxxxx>; Tumkur Narayan, Chethan
><chethan.tumkur.narayan@xxxxxxxxx>; bhelgaas@xxxxxxxxxx; linux-
>pci@xxxxxxxxxxxxxxx
>Subject: Re: [PATCH v6] Bluetooth: btintel_pcie: Add support for _suspend() /
>_resume()
>
>Dear Kiran,
>
>
>Thank you for sending the improved version 6.
>
>Am 25.07.25 um 11:01 schrieb Kiran K:
>> From: Chandrashekar Devegowda <chandrashekar.devegowda@xxxxxxxxx>
>>
>> This patch implements _suspend() and _resume() functions for the
>> Bluetooth controller. When the system enters a suspended state, the
>> driver notifies the controller to perform necessary housekeeping tasks
>> by writing to the sleep control register and waits for an alive
>> interrupt. The firmware raises the alive interrupt when it has
>> transitioned to the D3 state. The same flow occurs when the system
>> resumes.
>>
>> Command to test host initiated wakeup after 60 seconds sudo rtcwake -m
>> mem -s 60
>>
>> dmesg log (tested on Whale Peak2 on Panther Lake platform) On system
>> suspend:
>> [Fri Jul 25 11:05:37 2025] Bluetooth: hci0: device entered into d3
>> state from d0 in 80 us
>>
>> On system resume:
>> [Fri Jul 25 11:06:36 2025] Bluetooth: hci0: device entered into d0
>> state from d3 in 7117 us
>>
>> Signed-off-by: Chandrashekar Devegowda
>> <chandrashekar.devegowda@xxxxxxxxx>
>> Signed-off-by: Kiran K <kiran.k@xxxxxxxxx>
>> ---
>> changes in v6:
>> - s/delta/delta_us/g
>> - s/CONFIG_PM/CONFIG_PM_SLEEP/g
>> - use pm_sleep_pr()/pm_str() to avoid #ifdefs
>> - remove the code to set persistance mode as its not relevant to
>> this patch
>>
>> changes in v5:
>> - refactor _suspend() / _resume() to set the D3HOT/D3COLD based on
>power
>> event
>> - remove SIMPLE_DEV_PM_OPS and define the required pm_ops callback
>> functions
>>
>> changes in v4:
>> - Moved document and section details from the commit message as
>comment in code.
>>
>> changes in v3:
>> - Corrected the typo's
>> - Updated the CC list as suggested.
>> - Corrected the format specifiers in the logs.
>>
>> changes in v2:
>> - Updated the commit message with test steps and logs.
>> - Added logs to include the timeout message.
>> - Fixed a potential race condition during suspend and resume.
>>
>> drivers/bluetooth/btintel_pcie.c | 90 ++++++++++++++++++++++++++++++++
>> 1 file changed, 90 insertions(+)
>>
>> diff --git a/drivers/bluetooth/btintel_pcie.c
>> b/drivers/bluetooth/btintel_pcie.c
>> index 6e7bbbd35279..c419521493fe 100644
>> --- a/drivers/bluetooth/btintel_pcie.c
>> +++ b/drivers/bluetooth/btintel_pcie.c
>> @@ -2573,11 +2573,101 @@ static void btintel_pcie_coredump(struct device
>*dev)
>> }
>> #endif
>>
>> +#ifdef CONFIG_PM_SLEEP
>> +static int btintel_pcie_suspend_late(struct device *dev, pm_message_t
>> +mesg) {
>> + struct pci_dev *pdev = to_pci_dev(dev);
>> + struct btintel_pcie_data *data;
>> + ktime_t start;
>> + u32 dxstate;
>> + s64 delta_us;
>> + int err;
>> +
>> + data = pci_get_drvdata(pdev);
>> +
>> + dxstate = (mesg.event == PM_EVENT_SUSPEND ?
>> + BTINTEL_PCIE_STATE_D3_HOT :
>BTINTEL_PCIE_STATE_D3_COLD);
>
>I believe the brackets are uncommon. You actually can leave it out, or, if
>brackets need to be used, only surround the condition.
>
>> +
>> + data->gp0_received = false;
>> +
>> + start = ktime_get();
>> +
>> + /* Refer: 6.4.11.7 -> Platform power management */
>> + btintel_pcie_wr_sleep_cntrl(data, dxstate);
>> + err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
>> +
>msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>> + if (err == 0) {
>> + bt_dev_err(data->hdev, "Timeout (%u ms) on alive interrupt
>for D3 entry",
>> + BTINTEL_DEFAULT_INTR_TIMEOUT_MS);
>> + return -EBUSY;
>> + }
>> +
>> + delta_us = ktime_to_ns(ktime_get() - start) / 1000;
>> + bt_dev_info(data->hdev, "device entered into d3 state from d0 in %lld
>us",
>> + delta_us);
>> + return 0;
>> +}
>> +
>> +static int btintel_pcie_suspend(struct device *dev) {
>> + return btintel_pcie_suspend_late(dev, PMSG_SUSPEND); }
>> +
>> +static int btintel_pcie_hibernate(struct device *dev) {
>> + return btintel_pcie_suspend_late(dev, PMSG_HIBERNATE); }
>> +
>> +static int btintel_pcie_freeze(struct device *dev) {
>> + return btintel_pcie_suspend_late(dev, PMSG_FREEZE); }
>> +
>> +static int btintel_pcie_resume(struct device *dev) {
>> + struct pci_dev *pdev = to_pci_dev(dev);
>> + struct btintel_pcie_data *data;
>> + ktime_t start;
>> + int err;
>> + s64 delta_us;
>> +
>> + data = pci_get_drvdata(pdev);
>> + data->gp0_received = false;
>> +
>> + start = ktime_get();
>> +
>> + /* Refer: 6.4.11.7 -> Platform power management */
>> + btintel_pcie_wr_sleep_cntrl(data, BTINTEL_PCIE_STATE_D0);
>> + err = wait_event_timeout(data->gp0_wait_q, data->gp0_received,
>> +
>msecs_to_jiffies(BTINTEL_DEFAULT_INTR_TIMEOUT_MS));
>> + if (err == 0) {
>> + bt_dev_err(data->hdev, "Timeout (%u ms) on alive interrupt
>for D0 entry",
>> + BTINTEL_DEFAULT_INTR_TIMEOUT_MS);
>> + return -EBUSY;
>> + }
>> +
>> + delta_us = ktime_to_ns(ktime_get() - start) / 1000;
>> + bt_dev_info(data->hdev, "device entered into d0 state from d3 in %lld
>us",
>> + delta_us);
>> + return 0;
>> +}
>> +
>> +const struct dev_pm_ops btintel_pcie_pm_ops = {
>> + .suspend = pm_sleep_ptr(btintel_pcie_suspend),
>> + .resume = pm_sleep_ptr(btintel_pcie_resume),
>> + .freeze = pm_sleep_ptr(btintel_pcie_freeze),
>> + .thaw = pm_sleep_ptr(btintel_pcie_resume),
>> + .poweroff = pm_sleep_ptr(btintel_pcie_hibernate),
>> + .restore = pm_sleep_ptr(btintel_pcie_resume),
>> +};
>> +#endif
>> +
>> static struct pci_driver btintel_pcie_driver = {
>> .name = KBUILD_MODNAME,
>> .id_table = btintel_pcie_table,
>> .probe = btintel_pcie_probe,
>> .remove = btintel_pcie_remove,
>> + .driver.pm = pm_ptr(&btintel_pcie_pm_ops),
>> #ifdef CONFIG_DEV_COREDUMP
>> .driver.coredump = btintel_pcie_coredump
>> #endif
>
>With the above comment fixed, please feel free to add:
>
>Reviewed-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>
Ack.
>
>
>Kind regards,
>
>Paul
Thanks,
Kiran
