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 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? > 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.spinics.net/lists/linux-serial/msg66060.html 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; ``` Regards, Loic
