Received-SPF: pass (google.com: best guess record for domain of linux-bluetooth-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 15 Jan 2020 09:26:33 -0800
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Rocky Liao <rjliao@codeaurora.org>
Cc:     marcel@holtmann.org, johan.hedberg@gmail.com,
        linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, bgodavar@codeaurora.org,
        hemantg@codeaurora.org
Subject: Re: [PATCH v4 1/3] Bluetooth: hci_qca: Add QCA Rome power off
 support to the qca_power_shutdown()
Message-ID: <20200115172633.GM89495@google.com>
References: <20191225060317.5258-1-rjliao@codeaurora.org>
 <20200115085552.11483-1-rjliao@codeaurora.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20200115085552.11483-1-rjliao@codeaurora.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-bluetooth-owner@vger.kernel.org
Precedence: bulk

On Wed, Jan 15, 2020 at 04:55:50PM +0800, Rocky Liao wrote:
> Current qca_power_shutdown() only supports wcn399x, this patch adds Rome
> power off support to it. For Rome it just needs to pull down the bt_en
> GPIO to power off it. This patch also replaces all the power off operation
> in qca_close() with the unified qca_power_shutdown() call.
> 
> Signed-off-by: Rocky Liao <rjliao@codeaurora.org>
> ---
> 
> Changes in v2: None
> Changes in v3: None
> Changes in v4:
>   -rebased the patch with latest code base
>   -moved the change from qca_power_off() to qca_power_shutdown()
>   -replaced all the power off operation in qca_close() with
>    qca_power_shutdown()
>   -updated commit message
> 
>  drivers/bluetooth/hci_qca.c | 28 ++++++++++++++++------------
>  1 file changed, 16 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 992622dc1263..ecb74965be10 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -663,7 +663,6 @@ static int qca_flush(struct hci_uart *hu)
>  /* Close protocol */
>  static int qca_close(struct hci_uart *hu)
>  {
> -	struct qca_serdev *qcadev;
>  	struct qca_data *qca = hu->priv;
>  
>  	BT_DBG("hu %p qca close", hu);
> @@ -679,14 +678,7 @@ static int qca_close(struct hci_uart *hu)
>  	destroy_workqueue(qca->workqueue);
>  	qca->hu = NULL;
>  
> -	if (hu->serdev) {
> -		qcadev = serdev_device_get_drvdata(hu->serdev);
> -		if (qca_is_wcn399x(qcadev->btsoc_type))
> -			qca_power_shutdown(hu);
> -		else
> -			gpiod_set_value_cansleep(qcadev->bt_en, 0);
> -
> -	}
> +	qca_power_shutdown(hu);
>  
>  	kfree_skb(qca->rx_skb);
>  
> @@ -1685,6 +1677,7 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  	struct qca_serdev *qcadev;
>  	struct qca_data *qca = hu->priv;
>  	unsigned long flags;
> +	enum qca_btsoc_type soc_type = qca_soc_type(hu);
>  
>  	qcadev = serdev_device_get_drvdata(hu->serdev);
>  
> @@ -1697,11 +1690,22 @@ static void qca_power_shutdown(struct hci_uart *hu)
>  	qca_flush(hu);
>  	spin_unlock_irqrestore(&qca->hci_ibs_lock, flags);
>  
> -	host_set_baudrate(hu, 2400);
> -	qca_send_power_pulse(hu, false);
> -	qca_regulator_disable(qcadev);
>  	hu->hdev->hw_error = NULL;
>  	hu->hdev->cmd_timeout = NULL;

This is now done before the power off and not after, I suppose it doesn't
make a difference.

> +
> +	/* Non-serdev device usually is powered by external power
> +	 * and don't need additional action in driver for power down
> +	 */
> +	if (!hu->serdev)
> +		return;
> +
> +	if (qca_is_wcn399x(soc_type)) {
> +		host_set_baudrate(hu, 2400);
> +		qca_send_power_pulse(hu, false);
> +		qca_regulator_disable(qcadev);
> +	} else {
> +		gpiod_set_value_cansleep(qcadev->bt_en, 0);
> +	}
>  }

Reviewed-by: Matthias Kaehlcke <mka@chromium.org>