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. Thanks, Neeraj
