On Mon, 17 Feb 2025 at 16:41, Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx> wrote: > > Hi Loic, > > > On Mon, 17 Feb 2025 at 14:53, Neeraj Sanjay Kale > > <neeraj.sanjaykale@xxxxxxx> wrote: > > > Hi Loic, > > > > > > Thank you for your patch. Just a few suggestions below: > > > > > > > @@ -616,6 +617,13 @@ static void ps_init(struct hci_dev *hdev) > > > > break; > > > > } > > > > > > > > + if (!device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein- > > pin", > > > > + &psdata->h2c_wakeup_gpio)) > > > > + psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO; > > > > + if (!device_property_read_u8(&nxpdev->serdev->dev, > > > > + "nxp,wakeout- > > > > pin", > > > > + &psdata->c2h_wakeup_gpio)) > > > > + psdata->c2h_wakeupmode = > > BT_HOST_WAKEUP_METHOD_GPIO; > > > > + > > > > psdata->cur_psmode = PS_MODE_DISABLE; > > > > psdata->target_ps_mode = DEFAULT_PS_MODE; > > > > > > > Please move device_property_read for "nxp,wakein-pin" to ps_setup(), after > > "device-wakeup" is read. > > > > Ok, but then I'll need to move all the default value handling from > > ps_setup() into ps_init() as well. > I don't think that would be needed. Simply using the example code I mentioned below should suffice. > > To re-iterate, if "device-wakeup-gpios" is defined, we use WAKEUP_METHOD_GPIO, and if "nxp,wakein-pin" is present, use it, else simply use 0xff. > > But if "device-wakeup-gpios" is absent, use default WAKEUP_METHOD_BREAK. > > > > > > > > > I think we should not set h2c_wakeupmode as WAKEUP_METHOD_GPIO > > based on "nxp,wakein-pin" alone. > > > > > > In existing code, we are setting default_h2c_wakeup_mode to > > WAKEUP_METHOD_GPIO if "device-wakeup" is defined in DT, and psdata- > > >h2c_wakeup_gpio = 0xff. WAKE_IN pin is not read. > > > In this case the FW considers default GPIO as WAKE_IN pin (as per > > datasheet), which is a valid scenario. > > > > > > But this logic will fail if we specify only "nxp,wakein-pin", without "device- > > wakeup" in DT. > > > Hence, I recommend something as follows in ps_setup(): > > > - if (!psdata->h2c_ps_gpio) > > > + if (!psdata->h2c_ps_gpio || > > > + device_property_read_u8(&nxpdev->serdev->dev, "nxp,wakein-pin", > > > + &psdata->h2c_wakeup_gpio)) > > > psdata->h2c_wakeup_gpio = 0xff; > > > > Ok, will do, look like we should print an explicit error then, as it would be a > > clear devicetree misconfiguration? > Yes, an error print for "nxp,wakein-pin", without "device-wakeup-gpios" would be helpful. Thanks! > > > > > > For "nxp,wakeout-pin", I have yet to submit patch for "host-wakeup-gpios". > > I can move "nxp,wakeout-pin" later if required. > > > > It may not be necessary, I've submitted an other PR for handling simple > > dedicated wakeup interrupts at serdev core level instead of having to re- > > implement it in each driver: > > https://www.s/ > > pinics.net%2Flists%2Flinux- > > serial%2Fmsg66060.html&data=05%7C02%7Cneeraj.sanjaykale%40nxp.com% > > 7C979e9f5f906b438d707e08dd4f61b6e9%7C686ea1d3bc2b4c6fa92cd99c5c30 > > 1635%7C0%7C0%7C638754003307634680%7CUnknown%7CTWFpbGZsb3d8e > > yJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoi > > TWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=h%2B9yWxHbWkimN > > 7oHOM0ZU9Cgi1OK86NLGKP7Hw%2B6vRs%3D&reserved=0 > > > > With that, all we would need is adding the wakeup interrupt in the devicetree: > > ``` > > interrupt-parent = <&gpio4>; > > interrupts = <8 IRQ_TYPE_EDGE_FALLING>; > > interrupt-names = "wakeup"; > > wakeup-source; > > ``` > Yes, this was my initial approach, which works fine. But I think using "host-wakeup-gpios" would be a cleaner approach. > Driver will simply use the gpiod_to_irq() API to get an IRQ handler. > ``` > compatible = "nxp,88w8987-bt"; > host-wakeup-gpios = <&gpio3 24 GPIO_ACTIVE_HIGH>; > ``` > Please do let me know if this method has any drawbacks. Two points: - Why bother with a GPIO if we can directly pass an interrupt (and if actually don't care about gpio framework usage) - Why re-implementing it in the driver if the dedicated interrupt can be handled at the generic layer (serdev bus/core) Would be good if we can stick with `Documentation/devicetree/bindings/power/wakeup-source.txt`. Regards, Loic
