Hi Rajat, On Fri, Dec 16, 2016 at 11:30:03AM -0800, Rajat Jain wrote: > Some onboard BT chips (e.g. Marvell 8997) contain a wakeup pin that > can be connected to a gpio on the CPU side, and can be used to wakeup > the host out-of-band. This can be useful in situations where the > in-band wakeup is not possible or not preferable (e.g. the in-band > wakeup may require the USB host controller to remain active, and > hence consuming more system power during system sleep). > > The oob gpio interrupt to be used for wakeup on the CPU side, is > read from the device tree node, (using standard interrupt descriptors). > A devcie tree binding document is also added for the driver. The > compatible string is in compliance with > Documentation/devicetree/bindings/usb/usb-device.txt > > Signed-off-by: Rajat Jain <rajatja@xxxxxxxxxx> A few small comments below, but other than those, for the whole series: Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx> > --- > v2: * Use interrupt-names ("wakeup") instead of assuming first interrupt. > * Leave it on device tree to specify IRQ flags (level /edge triggered) > * Mark the device as non wakeable on exit. > > Documentation/devicetree/bindings/net/btusb.txt | 40 ++++++++++++ > drivers/bluetooth/btusb.c | 84 +++++++++++++++++++++++++ > 2 files changed, 124 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/btusb.txt > > diff --git a/Documentation/devicetree/bindings/net/btusb.txt b/Documentation/devicetree/bindings/net/btusb.txt > new file mode 100644 > index 0000000..2c0355c > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/btusb.txt > @@ -0,0 +1,40 @@ > +Generic Bluetooth controller over USB (btusb driver) > +--------------------------------------------------- > + > +Required properties: > + > + - compatible : should comply with the format "usbVID,PID" specified in > + Documentation/devicetree/bindings/usb/usb-device.txt > + At the time of writing, the only OF supported devices > + (more may be added later) are: > + > + "usb1286,204e" (Marvell 8997) I don't know if anyone cares about these sort of organizational aspects, but in patch 3 you're extending this binding with device-specific Marvell bindings. Seems like it'd be good to have some clear way to correlate those 2. e.g., add a reference to marvell-bt-sd8xxx.txt (or marvell-bt-8xxx.txt, once you rename it) in patch 3. > + > +Optional properties: > + > + - interrupt-parent: phandle of the parent interrupt controller > + - interrupt-names: (see below) > + - interrupts : The interrupt specified by the name "wakeup" is the interrupt > + that shall be used for out-of-band wake-on-bt. Driver will > + request this interrupt for wakeup. During system suspend, the > + irq will be enabled so that the bluetooth chip can wakeup host > + platform out of band. During system resume, the irq will be > + disabled to make sure unnecessary interrupt is not received. > + > +Example: > + > +Following example uses irq pin number 3 of gpio0 for out of band wake-on-bt: > + > +&usb_host1_ehci { > + status = "okay"; > + #address-cells = <1>; > + #size-cells = <0>; > + > + mvl_bt1: bt@1 { > + compatible = "usb1286,204e"; > + reg = <1>; > + interrupt-parent = <&gpio0>; > + interrupt-name = "wakeup"; > + interrupts = <3 IRQ_TYPE_LEVEL_LOW>; > + }; > +}; > diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > index ce22cef..beca4e9 100644 > --- a/drivers/bluetooth/btusb.c > +++ b/drivers/bluetooth/btusb.c > @@ -24,6 +24,8 @@ > #include <linux/module.h> > #include <linux/usb.h> > #include <linux/firmware.h> > +#include <linux/of_device.h> > +#include <linux/of_irq.h> > #include <asm/unaligned.h> > > #include <net/bluetooth/bluetooth.h> > @@ -369,6 +371,7 @@ static const struct usb_device_id blacklist_table[] = { > #define BTUSB_BOOTING 9 > #define BTUSB_RESET_RESUME 10 > #define BTUSB_DIAG_RUNNING 11 > +#define BTUSB_OOB_WAKE_DISABLED 12 > > struct btusb_data { > struct hci_dev *hdev; > @@ -416,6 +419,8 @@ struct btusb_data { > int (*recv_bulk)(struct btusb_data *data, void *buffer, int count); > > int (*setup_on_usb)(struct hci_dev *hdev); > + > + int oob_wake_irq; /* irq for out-of-band wake-on-bt */ > }; > > static inline void btusb_free_frags(struct btusb_data *data) > @@ -2728,6 +2733,65 @@ static int btusb_bcm_set_diag(struct hci_dev *hdev, bool enable) > } > #endif > > +#ifdef CONFIG_PM > +static irqreturn_t btusb_oob_wake_handler(int irq, void *priv) > +{ > + struct btusb_data *data = priv; > + > + /* Disable only if not already disabled (keep it balanced) */ > + if (!test_and_set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags)) { > + disable_irq_nosync(irq); > + disable_irq_wake(irq); > + } > + pm_wakeup_event(&data->udev->dev, 0); > + return IRQ_HANDLED; > +} > + > +static const struct of_device_id btusb_match_table[] = { > + { .compatible = "usb1286,204e" }, > + { } > +}; > +MODULE_DEVICE_TABLE(of, btusb_match_table); You define a match table here, but you also define essentially same table for Marvell-specific additions in patch 3. It looks like maybe it's legal to have more than one OF table in a module? But it seems like it would get confusing, besides being somewhat strange to maintain. It might also produce duplicate 'modinfo' output. If you really want to independently opt into device-tree-specified interrupts vs. Marvell-specific interrrupt configuration, then you should probably just merge the latter into the former table, and implement a Marvell/GPIO flag to stick in the .data field of this table. Or it might be fine to drop one or both "match" checks. Particularly for the Marvell-specific stuff, it's probably fair just to check if it has an ->of_node and if 'id->driver_info & BTUSB_MARVELL'. Any other Marvell device-specific quirks could probably be keyed off of the (weirdly-named?) blacklist_table[], which already matches PID/VID. > + > +/* Use an oob wakeup pin? */ > +static int btusb_config_oob_wake(struct hci_dev *hdev) > +{ > + struct btusb_data *data = hci_get_drvdata(hdev); > + struct device *dev = &data->udev->dev; > + int irq, ret; > + > + if (!of_match_device(btusb_match_table, dev)) > + return 0; > + > + /* Move on if no IRQ specified */ > + irq = of_irq_get_byname(dev->of_node, "wakeup"); > + if (irq <= 0) { > + bt_dev_dbg(hdev, "%s: no OOB Wakeup IRQ in DT", __func__); > + return 0; > + } > + > + set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags); > + > + ret = devm_request_irq(&hdev->dev, irq, btusb_oob_wake_handler, > + 0, "OOB Wake-on-BT", data); > + if (ret) { > + bt_dev_err(hdev, "%s: IRQ request failed", __func__); > + return ret; > + } > + > + ret = device_init_wakeup(dev, true); > + if (ret) { > + bt_dev_err(hdev, "%s: failed to init_wakeup\n", __func__); > + return ret; > + } > + > + data->oob_wake_irq = irq; > + disable_irq(irq); > + bt_dev_info(hdev, "OOB Wake-on-BT configured at IRQ %u\n", irq); > + return 0; > +} > +#endif > + > static int btusb_probe(struct usb_interface *intf, > const struct usb_device_id *id) > { > @@ -2849,6 +2913,11 @@ static int btusb_probe(struct usb_interface *intf, > hdev->send = btusb_send_frame; > hdev->notify = btusb_notify; > > +#ifdef CONFIG_PM > + err = btusb_config_oob_wake(hdev); > + if (err) > + goto out_free_dev; > +#endif > if (id->driver_info & BTUSB_CW6622) > set_bit(HCI_QUIRK_BROKEN_STORED_LINK_KEY, &hdev->quirks); > > @@ -3061,6 +3130,9 @@ static void btusb_disconnect(struct usb_interface *intf) > usb_driver_release_interface(&btusb_driver, data->isoc); > } > > + if (data->oob_wake_irq) > + device_init_wakeup(&data->udev->dev, false); > + > hci_free_dev(hdev); > } > > @@ -3089,6 +3161,12 @@ static int btusb_suspend(struct usb_interface *intf, pm_message_t message) > btusb_stop_traffic(data); > usb_kill_anchored_urbs(&data->tx_anchor); > > + if (data->oob_wake_irq && device_may_wakeup(&data->udev->dev)) { > + clear_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags); > + enable_irq_wake(data->oob_wake_irq); > + enable_irq(data->oob_wake_irq); > + } > + > /* Optionally request a device reset on resume, but only when > * wakeups are disabled. If wakeups are enabled we assume the > * device will stay powered up throughout suspend. > @@ -3126,6 +3204,12 @@ static int btusb_resume(struct usb_interface *intf) > if (--data->suspend_count) Not related to your patch exactly, but isn't this 'suspend_count' stuff useless? I'd send a patch, but it might conflict with yours. Just wanted to note it and see if I'm crazy... Brian > return 0; > > + /* Disable only if not already disabled (keep it balanced) */ > + if (!test_and_set_bit(BTUSB_OOB_WAKE_DISABLED, &data->flags)) { > + disable_irq(data->oob_wake_irq); > + disable_irq_wake(data->oob_wake_irq); > + } > + > if (!test_bit(HCI_RUNNING, &hdev->flags)) > goto done; > > -- > 2.8.0.rc3.226.g39d4020 > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html