Re: [PATCH v1 1/1] Bluetooth: hci_qca: Add poweroff support during hci down for wcn3990

[Balakrishna Godavarthi]

Hi Marcel,

On 2018-08-25 00:20, Marcel Holtmann wrote:
> Hi Balakrishna,
>
> > This patch enables power off support for hci down and power on support
> > for hci up. As wcn3990 power sources are ignited by regulators, we 
> > will
> > turn off them during hci down, i.e. an complete power off of wcn3990.
> > So while hci up, we will call vendor specific open/close and setup 
> > which
> > will turn on the regulators, requests BT chip version and download the
> > firmware.
> >
> > Signed-off-by: Balakrishna Godavarthi <bgodavar@xxxxxxxxxxxxxx>
> > ---
> > drivers/bluetooth/hci_qca.c | 34 ++++++++++++++++++++++++++++++++++
> > 1 file changed, 34 insertions(+)
> >
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index e182f6019f68..98d33c6b8909 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -595,6 +595,9 @@ static int qca_close(struct hci_uart *hu)
> > 	struct qca_serdev *qcadev;
> > 	struct qca_data *qca = hu->priv;
> >
> > +	if (!qca)
> > +		return 0;
> > +
> > 	BT_DBG("hu %p qca close", hu);
> >
> > 	serial_clock_vote(HCI_IBS_VOTE_STATS_UPDATE, hu);
> > @@ -1098,6 +1101,32 @@ static int qca_set_speed(struct hci_uart *hu, 
> > enum qca_speed_type speed_type)
> > 	return 0;
> > }
> >
> > +static int hci_uart_open(struct hci_dev *hdev)
> > +{
> > +	struct hci_uart *hu = hci_get_drvdata(hdev);
> > +
> > +	BT_DBG("%s %p", hdev->name, hdev);
> > +	set_bit(HCI_UART_PROTO_READY, &hu->flags);
> > +
> > +	return qca_open(hu);
> > +}
> > +
> > +static int hci_uart_close(struct hci_dev *hdev)
> > +{
> > +	struct hci_uart *hu = hci_get_drvdata(hdev);
> > +
> > +	BT_DBG("%s %p", hdev->name, hdev);
> > +
> > +	/* After this step, we should not allow Tx and Rx work
> > +	 * function to execute. As we are freeing qca tx and rx
> > +	 * buffers which may cause kernel crash.
> > +	 */
> > +	clear_bit(HCI_UART_PROTO_READY, &hu->flags);
> > +	qca_close(hu);
> > +
> > +	return 0;
> > +}
> > +
> > static int qca_wcn3990_init(struct hci_uart *hu)
> > {
> > 	struct hci_dev *hdev = hu->hdev;
> > @@ -1154,6 +1183,11 @@ static int qca_setup(struct hci_uart *hu)
> >
> > 	if (qcadev->btsoc_type == QCA_WCN3990) {
> > 		bt_dev_info(hdev, "setting up wcn3990");
> > +
> > +		hdev->open  = hci_uart_open;
> > +		hdev->close = hci_uart_close;
> > +		set_bit(HCI_QUIRK_NON_PERSISTENT_SETUP, &hdev->quirks);
> > +
>
> I do not get this change and I am also against it. Once hdev->open and
> hdev->close are assigned, donât re-assign them. You called
> hci_register_dev and you are not changing the basic transport
> open/close functions after that. If you need some vendor specific
> things to open and close the transport, then do it properly.

[Bala]: the reason to re-assign hdev->close & hdev->open is, when we do 
an hci0 down we are
        shutting down the regulators to turn off wcn3990 and closing the 
vendor proto i.e.clearing the memory too.,
        So we are re-assigning the hdev->close to proto close. Here is 
the use case of the above change wrt mobile handset.
        we will setup BT during bootup, i.e. we enable the clocks or 
regulators to turn on BT chip connected.
        let us assume that we turn off the BT chip i.e. hci0 down. so to 
current implementation we will down the HCI0,
        but still we have our uart clocks or BT clocks are regulators 
are enabled. This is causing power and memory consumption.

> When I posted the btuart.c serdev driver, it would be a lot easier to
> add vendor specific things then to hci_serdev.c which is still kinda
> hacked onto a line discipline driver.

[Bala]: when can we expect btuart.c serdev driver.

> Regards
>
> Marcel

--
Regards
Balakrishna.