Re: [PATCH v1] Bluetooth: hci_qca: Make bt_en and susclk not mandatory for QCA Rome

[Rocky Liao]

å 2020-03-04 16:53ïJohan Hovold åéï
> On Wed, Mar 04, 2020 at 09:54:29AM +0800, Rocky Liao wrote:
> > On some platforms the bt_en pin and susclk are default on and there
> > is no exposed resource to control them. This patch makes the bt_en
> > and susclk not mandatory to have BT work. It also will not set the
> > HCI_QUIRK_NON_PERSISTENT_SETUP and shutdown() callback if bt_en is
> > not available.
> >
> > Signed-off-by: Rocky Liao <rjliao@xxxxxxxxxxxxxx>
> > ---
> >  drivers/bluetooth/hci_qca.c | 47 
> > ++++++++++++++++++++-----------------
> >  1 file changed, 26 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> > index bf436d6e638e..325baa046c3a 100644
> > --- a/drivers/bluetooth/hci_qca.c
> > +++ b/drivers/bluetooth/hci_qca.c
> > @@ -1562,9 +1562,11 @@ static int qca_power_on(struct hci_dev *hdev)
> >  		ret = qca_wcn3990_init(hu);
> >  	} else {
> >  		qcadev = serdev_device_get_drvdata(hu->serdev);
> > -		gpiod_set_value_cansleep(qcadev->bt_en, 1);
> > -		/* Controller needs time to bootup. */
> > -		msleep(150);
> > +		if (!IS_ERR(qcadev->bt_en)) {
> > +			gpiod_set_value_cansleep(qcadev->bt_en, 1);
> > +			/* Controller needs time to bootup. */
> > +			msleep(150);
> > +		}
> >  	}
> >
> >  	return ret;
> > @@ -1750,7 +1752,7 @@ static void qca_power_shutdown(struct hci_uart 
> > *hu)
> >  		host_set_baudrate(hu, 2400);
> >  		qca_send_power_pulse(hu, false);
> >  		qca_regulator_disable(qcadev);
> > -	} else {
> > +	} else if (!IS_ERR(qcadev->bt_en)) {
> >  		gpiod_set_value_cansleep(qcadev->bt_en, 0);
> >  	}
> >  }
> > @@ -1852,6 +1854,7 @@ static int qca_serdev_probe(struct serdev_device 
> > *serdev)
> >  	struct hci_dev *hdev;
> >  	const struct qca_vreg_data *data;
> >  	int err;
> > +	bool power_ctrl_enabled = true;
> >
> >  	qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL);
> >  	if (!qcadev)
> > @@ -1901,35 +1904,37 @@ static int qca_serdev_probe(struct 
> > serdev_device *serdev)
> >  		qcadev->bt_en = devm_gpiod_get(&serdev->dev, "enable",
> >  					       GPIOD_OUT_LOW);
>
> Shouldn't you use devm_gpiod_get_optional()?
>
> >  		if (IS_ERR(qcadev->bt_en)) {
> > -			dev_err(&serdev->dev, "failed to acquire enable gpio\n");
> > -			return PTR_ERR(qcadev->bt_en);
> > +			dev_warn(&serdev->dev, "failed to acquire enable gpio\n");
> > +			power_ctrl_enabled = false;
>
> And bail out on errors, but treat NULL as !port_ctrl_enabled?
>
> >  		}
> >
> >  		qcadev->susclk = devm_clk_get(&serdev->dev, NULL);
>
> And devm_clk_get_optional() here.
>
> Etc.
>
> Also does the devicetree binding need to be updated to reflect this
> change?
OK, I will update to use the suggested API.
The devicetress binding doesn't need to be updated as they are already 
optional there.

> Johan