Re: [PATCH v3 2/3] Bluetooth: hci_bcm: Prepare PM runtime support

[Frederic Danis <frederic.danis@linux.intel.com>]

Hello Marcel,

On 09/09/2015 18:18, Marcel Holtmann wrote:
> Hi Fred,
>
>> Change some CONFIG_PM_SLEEP to CONFIG_PM as hu and is_suspended parameters
>> will be used during PM runtime callbacks.
>>
>> Add __bcm_suspend() and __bcm_resume() which performs link management for
>> PM callbacks.
>>
>> Signed-off-by: Frederic Danis <frederic.danis@linux.intel.com>
>> ---
>> drivers/bluetooth/hci_bcm.c | 70 ++++++++++++++++++++++++++-------------------
>> 1 file changed, 41 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/bluetooth/hci_bcm.c b/drivers/bluetooth/hci_bcm.c
>> index 6551251..29ed069 100644
>> --- a/drivers/bluetooth/hci_bcm.c
>> +++ b/drivers/bluetooth/hci_bcm.c
>> @@ -56,7 +56,7 @@ struct bcm_device {
>> 	int			irq;
>> 	u8			irq_polarity;
>>
>> -#ifdef CONFIG_PM_SLEEP
>> +#ifdef CONFIG_PM
>> 	struct hci_uart		*hu;
>> 	bool			is_suspended; /* suspend/resume flag */
>> #endif
>> @@ -153,7 +153,7 @@ static int bcm_gpio_set_power(struct bcm_device *dev, bool powered)
>> 	return 0;
>> }
>>
>> -#ifdef CONFIG_PM_SLEEP
>> +#ifdef CONFIG_PM
>> static irqreturn_t bcm_host_wake(int irq, void *data)
>> {
>> 	struct bcm_device *bdev = data;
>> @@ -259,7 +259,7 @@ static int bcm_open(struct hci_uart *hu)
>> 		if (hu->tty->dev->parent == dev->pdev->dev.parent) {
>> 			bcm->dev = dev;
>> 			hu->init_speed = dev->init_speed;
>> -#ifdef CONFIG_PM_SLEEP
>> +#ifdef CONFIG_PM
>> 			dev->hu = hu;
>> #endif
>> 			bcm_gpio_set_power(bcm->dev, true);
>> @@ -283,7 +283,7 @@ static int bcm_close(struct hci_uart *hu)
>> 	mutex_lock(&bcm_device_lock);
>> 	if (bcm_device_exists(bdev)) {
>> 		bcm_gpio_set_power(bdev, false);
>> -#ifdef CONFIG_PM_SLEEP
>> +#ifdef CONFIG_PM
>> 		if (device_can_wakeup(&bdev->pdev->dev)) {
>> 			devm_free_irq(&bdev->pdev->dev, bdev->irq, bdev);
>> 			device_init_wakeup(&bdev->pdev->dev, false);
>> @@ -425,6 +425,41 @@ static struct sk_buff *bcm_dequeue(struct hci_uart *hu)
>> 	return skb_dequeue(&bcm->txq);
>> }
>>
>> +#ifdef CONFIG_PM
>> +static void __bcm_suspend(struct bcm_device *bdev)
>> +{
>> +	if (!bdev->is_suspended && bdev->hu) {
>> +		hci_uart_set_flow_control(bdev->hu, true);
>> +
>> +		/* Once this returns, driver suspends BT via GPIO */
>> +		bdev->is_suspended = true;
>> +	}
>> +
>> +	/* Suspend the device */
>> +	if (bdev->device_wakeup) {
>> +		gpiod_set_value(bdev->device_wakeup, false);
>> +		bt_dev_dbg(bdev, "suspend, delaying 15 ms");
>> +		mdelay(15);
>> +	}
>> +}
>> +
>> +static void __bcm_resume(struct bcm_device *bdev)
>> +{
>> +	if (bdev->device_wakeup) {
>> +		gpiod_set_value(bdev->device_wakeup, true);
>> +		bt_dev_dbg(bdev, "resume, delaying 15 ms");
>> +		mdelay(15);
>> +	}
>> +
>> +	/* When this executes, the device has woken up already */
>> +	if (bdev->is_suspended && bdev->hu) {
>> +		bdev->is_suspended = false;
>> +
>> +		hci_uart_set_flow_control(bdev->hu, false);
>> +	}
>> +}
>
> I wonder if naming these bcm_suspend_device and bcm_resume_device are not better names actually. Since even the comments in these section say that is what you are doing.
>
> And as a general note, I think for devices with complicated power management, it is a good idea to add more comments in various place to explain why things are done a certain way. Nobody is going to remember this in 6 month when we have to look at this driver again for adding support for another model.

I will do this

Regards

Fred