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. > > 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; I don't get it, can you clarify when we should default to 0xff value for h2c_wakeup_gpio? and what it means for the firmware. Before the above change, we apply 0xff if there is **no** device-wakeup gpio, so if wakeup mode is not GPIO. After the above change, we apply 0xff if there is no device-wakeup gpio but also if there is no wakein-pin (whether there is a device-wakeup gpio or not)... Regards, Loic
