> -----Original Message----- > From: Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx> > Sent: Tuesday, October 1, 2024 12:40 PM > To: marcel@xxxxxxxxxxxx; luiz.dentz@xxxxxxxxx; robh@xxxxxxxxxx; > krzk+dt@xxxxxxxxxx; conor+dt@xxxxxxxxxx > Cc: linux-bluetooth@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; Amitkumar Karwar <amitkumar.karwar@xxxxxxx>; > Rohit Fule <rohit.fule@xxxxxxx>; Neeraj Sanjay Kale > <neeraj.sanjaykale@xxxxxxx>; Sherry Sun <sherry.sun@xxxxxxx>; Luke Wang > <ziniu.wang_1@xxxxxxx>; Bough Chen <haibo.chen@xxxxxxx>; LnxRevLi > <LnxRevLi@xxxxxxx> > Subject: [PATCH v1 2/2] Bluetooth: btnxpuart: Add GPIO support to power save > feature > > This adds support for driving the chip into sleep or wakeup with a GPIO. > > If the device tree property h2c-ps-gpio is defined, the driver utilizes this GPIO for > controlling the chip's power save state, else it uses the default UART-break > method. > > Signed-off-by: Neeraj Sanjay Kale <neeraj.sanjaykale@xxxxxxx> > --- > drivers/bluetooth/btnxpuart.c | 36 +++++++++++++++++++++++++++++++++-- > 1 file changed, 34 insertions(+), 2 deletions(-) > > switch (psdata->h2c_wakeupmode) { > + case WAKEUP_METHOD_GPIO: > + pcmd.h2c_wakeupmode = BT_CTRL_WAKEUP_METHOD_GPIO; > + break; > case WAKEUP_METHOD_DTR: > pcmd.h2c_wakeupmode = BT_CTRL_WAKEUP_METHOD_DSR; > break; > psdata->h2c_ps_interval = PS_DEFAULT_TIMEOUT_PERIOD_MS; > - switch (DEFAULT_H2C_WAKEUP_MODE) { > + > + switch (default_h2c_wakeup_mode) { > + case WAKEUP_METHOD_GPIO: > + psdata->h2c_wakeupmode = WAKEUP_METHOD_GPIO; > + gpiod_set_value_cansleep(psdata->h2c_ps_gpio, 1); > + usleep_range(5000, 10000); > + gpiod_set_value_cansleep(psdata->h2c_ps_gpio, 0); > + usleep_range(5000, 10000); > + break; Based on the above GPIO operation sequences, it indicates that the target device likely responds to a falling edge (transition from high to low) as its wakeup trigger, is it? In the cover letter, you mentioned " the driver puts the chip into power save state by driving the GPIO high, and wakes it up by driving the GPIO low". Seems the expected behavior is a level trigger. This appears to be a discrepancy between the code implementation and the description in the cover letter regarding the wakeup mechanism. Can you please clarify it? Thanks, Shenwei > case WAKEUP_METHOD_DTR: > psdata->h2c_wakeupmode = WAKEUP_METHOD_DTR; > serdev_device_set_tiocm(nxpdev->serdev, 0, TIOCM_DTR); @@ > -1279,6 +1308,9 @@ static int nxp_enqueue(struct hci_dev *hdev, struct sk_buff > *skb) > psdata->c2h_wakeup_gpio = > wakeup_parm.c2h_wakeup_gpio; > psdata->h2c_wakeup_gpio = > wakeup_parm.h2c_wakeup_gpio; > switch (wakeup_parm.h2c_wakeupmode) { > + case BT_CTRL_WAKEUP_METHOD_GPIO: > + psdata->h2c_wakeupmode = > WAKEUP_METHOD_GPIO; > + break; > case BT_CTRL_WAKEUP_METHOD_DSR: > psdata->h2c_wakeupmode = > WAKEUP_METHOD_DTR; > break; > -- > 2.25.1