Return-Path: Content-Type: text/plain; charset=us-ascii Mime-Version: 1.0 (Mac OS X Mail 11.2 \(3445.5.20\)) Subject: Re: [PATCH v4 3/3] Bluetooth: hci_qca: Add serdev support From: Marcel Holtmann In-Reply-To: <20180314155514.3374-4-thierry.escande@linaro.org> Date: Wed, 14 Mar 2018 21:14:41 +0100 Cc: Rob Herring , Andy Gross , Johan Hedberg , David Brown , Mark Rutland , Andy Shevchenko , Loic Poulain , Bjorn Andersson , Srinivas Kandagatla , linux-bluetooth@vger.kernel.org, linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org Message-Id: References: <20180314155514.3374-1-thierry.escande@linaro.org> <20180314155514.3374-4-thierry.escande@linaro.org> To: Thierry Escande Sender: linux-kernel-owner@vger.kernel.org List-ID: Hi Thierry, > Add support for Qualcomm serial slave devices. Probe the serial device, > retrieve its maximum speed and register a new hci uart device. > > Signed-off-by: Thierry Escande > --- > > v4: > - Rename divclk4 as susclk (its name in the bt chip) > - Use gpiod_set_value_cansleep() > - Replace #include with > - Restore dependency on BT_HCIUART > > v3: > - Remove redundant call to gpiod_set_value() after devm_gpiod_get() > - Check returned values for clk_set_rate() and clk_prepare_enable() > - Use clk_disable_unprepare() > > v2: > - Fix author email > > drivers/bluetooth/Kconfig | 1 + > drivers/bluetooth/hci_qca.c | 109 +++++++++++++++++++++++++++++++++++++++++++- > 2 files changed, 108 insertions(+), 2 deletions(-) > > diff --git a/drivers/bluetooth/Kconfig b/drivers/bluetooth/Kconfig > index 07e55cd8f8c8..e0f1a6609b68 100644 > --- a/drivers/bluetooth/Kconfig > +++ b/drivers/bluetooth/Kconfig > @@ -196,6 +196,7 @@ config BT_HCIUART_BCM > config BT_HCIUART_QCA > bool "Qualcomm Atheros protocol support" > depends on BT_HCIUART > + depends on BT_HCIUART_SERDEV > select BT_HCIUART_H4 > select BT_QCA > help > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c > index 05ec530b8a3a..6cf0d1d4595a 100644 > --- a/drivers/bluetooth/hci_qca.c > +++ b/drivers/bluetooth/hci_qca.c > @@ -29,7 +29,11 @@ > */ > > #include > +#include > #include > +#include > +#include > +#include > > #include > #include > @@ -50,6 +54,9 @@ > #define IBS_TX_IDLE_TIMEOUT_MS 2000 > #define BAUDRATE_SETTLE_TIMEOUT_MS 300 > > +/* susclk rate */ > +#define SUSCLK_RATE_32KHZ 32768 > + > /* HCI_IBS transmit side sleep protocol states */ > enum tx_ibs_states { > HCI_IBS_TX_ASLEEP, > @@ -111,6 +118,12 @@ struct qca_data { > u64 votes_off; > }; > > +struct qca_serdev { > + struct hci_uart serdev_hu; > + struct gpio_desc *bt_en; > + struct clk *susclk; > +}; > + > static void __serial_clock_on(struct tty_struct *tty) > { > /* TODO: Some chipset requires to enable UART clock on client > @@ -386,6 +399,7 @@ static void hci_ibs_wake_retrans_timeout(struct timer_list *t) > /* Initialize protocol */ > static int qca_open(struct hci_uart *hu) > { > + struct qca_serdev *qcadev; > struct qca_data *qca; > > BT_DBG("hu %p qca_open", hu); > @@ -444,6 +458,13 @@ static int qca_open(struct hci_uart *hu) > timer_setup(&qca->tx_idle_timer, hci_ibs_tx_idle_timeout, 0); > qca->tx_idle_delay = IBS_TX_IDLE_TIMEOUT_MS; > > + if (hu->serdev) { > + serdev_device_open(hu->serdev); > + > + qcadev = serdev_device_get_drvdata(hu->serdev); > + gpiod_set_value_cansleep(qcadev->bt_en, 1); > + } > + > BT_DBG("HCI_UART_QCA open, tx_idle_delay=%u, wake_retrans=%u", > qca->tx_idle_delay, qca->wake_retrans); > > @@ -512,6 +533,7 @@ 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); > @@ -525,6 +547,13 @@ static int qca_close(struct hci_uart *hu) > destroy_workqueue(qca->workqueue); > qca->hu = NULL; > > + if (hu->serdev) { > + serdev_device_close(hu->serdev); > + > + qcadev = serdev_device_get_drvdata(hu->serdev); > + gpiod_set_value_cansleep(qcadev->bt_en, 0); > + } > + > kfree_skb(qca->rx_skb); > > hu->priv = NULL; > @@ -885,6 +914,14 @@ static int qca_set_baudrate(struct hci_dev *hdev, uint8_t baudrate) > return 0; > } > > +static inline void host_set_baudrate(struct hci_uart *hu, unsigned int speed) > +{ > + if (hu->serdev) > + serdev_device_set_baudrate(hu->serdev, speed); > + else > + hci_uart_set_baudrate(hu, speed); > +} > + > static int qca_setup(struct hci_uart *hu) > { > struct hci_dev *hdev = hu->hdev; > @@ -905,7 +942,7 @@ static int qca_setup(struct hci_uart *hu) > speed = hu->proto->init_speed; > > if (speed) > - hci_uart_set_baudrate(hu, speed); > + host_set_baudrate(hu, speed); > > /* Setup user speed if needed */ > speed = 0; > @@ -924,7 +961,7 @@ static int qca_setup(struct hci_uart *hu) > ret); > return ret; > } > - hci_uart_set_baudrate(hu, speed); > + host_set_baudrate(hu, speed); > } > > /* Setup patch / NVM configurations */ > @@ -958,12 +995,80 @@ static struct hci_uart_proto qca_proto = { > .dequeue = qca_dequeue, > }; > > +static int qca_serdev_probe(struct serdev_device *serdev) > +{ > + struct qca_serdev *qcadev; > + int err; > + > + qcadev = devm_kzalloc(&serdev->dev, sizeof(*qcadev), GFP_KERNEL); > + if (!qcadev) > + return -ENOMEM; > + > + qcadev->serdev_hu.serdev = serdev; > + serdev_device_set_drvdata(serdev, qcadev); > + > + qcadev->bt_en = devm_gpiod_get(&serdev->dev, "bt-disable-n", > + GPIOD_OUT_LOW); > + if (IS_ERR(qcadev->bt_en)) { > + dev_err(&serdev->dev, "failed to acquire bt-disable-n gpio\n"); > + return PTR_ERR(qcadev->bt_en); > + } > + > + qcadev->susclk = devm_clk_get(&serdev->dev, NULL); > + if (IS_ERR(qcadev->susclk)) { > + dev_err(&serdev->dev, "failed to acquire clk\n"); > + return PTR_ERR(qcadev->susclk); > + } > + > + err = clk_set_rate(qcadev->susclk, SUSCLK_RATE_32KHZ); > + if (err) > + return err; > + > + err = clk_prepare_enable(qcadev->susclk); > + if (err) > + return err; > + > + err = hci_uart_register_device(&qcadev->serdev_hu, &qca_proto); > + if (err) > + clk_disable_unprepare(qcadev->susclk); > + > + return err; > +} > + > +static void qca_serdev_remove(struct serdev_device *serdev) > +{ > + struct qca_serdev *qcadev = serdev_device_get_drvdata(serdev); > + > + hci_uart_unregister_device(&qcadev->serdev_hu); > + > + clk_disable_unprepare(qcadev->susclk); > +} > + > +static const struct of_device_id qca_bluetooth_of_match[] = { > + { .compatible = "qcom,qca6174-bt" }, > + { /* sentinel */ } > +}; so I am fine taking this patch after we agree on the GPIO naming in the DT file, however .. While looking into the 3-Wire serdev patches to support Realtek devices, I realized that hacking serdev support into hci_uart.ko is actually pretty much a huge mess. It works, but it is also not pretty and makes the code really complicated and spaghetti like. The nice thing about hci_uart.ko is that all vendor specific stuff is in a separate file (in comparison to btusb.ko), but the not so nice part is that it is largely messy and complicated. So I have a basic btuart.ko driver ready that speaks only H:4 and serdev. It is clean and tiny and I added already basic Broadcom support to it. So maybe going the path like btusb.ko to select different setup routines is good enough. I need to work out a few kinks in it, but it is a lot cleaner from a code perspective. In addition, I have the basics for a bt3wire.ko driver that will then take H:5 instead of H:4. I will send them around shortly and maybe it makes sense to go down this path and integrate QCA support there. I currently only have RPi3 on my desk with serdev attached Broadcom chip. So it would needs others testing if this can work. Regards Marcel