Hi Loic, > > > > > @@ -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)... > In send_wakeup_method_cmd(), we always set pcmd.h2c_wakeup_gpio = 0xff, no matter if the wakeup method is BREAK or GPIO. This was done on-purpose to allow FW to use default (hardcoded)WAKE_IN pin, specific to the chip, for GPIO wake-up method. User can get the WAKE_IN pin info from the chip's datasheet. Pretty straightforward right? With your patch, send_wakeup_method_cmd() sets pcmd.h2c_wakeup_gpio = psdata->h2c_wakeup_gpio. The GPIO based device wakeup functionality works with very limited number of pins, which again varies from one chip to another. So not adding wakein-pin was intentional (based on internal discussion) to avoid any sort of confusion and maintain consistency between datasheet and FW default pins. We would want this behaviour to be continued in this patch, such that if "device-wakeup-gpios" is defined **without** "wakein-pin", the pcmd. h2c_wakeup_gpio parameter = psdata->h2c_wakeup_gpio would still be 0xff in send_wakeup_method_cmd(). Hope this clarifies everything. Thanks, Neeraj
