RE: [PATCH v1 2/2] Bluetooth: btnxpuart: Add GPIO support to power save feature

[Neeraj Sanjay Kale]

Hi Shenwei,

Thank you for the review.

> >
> > 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

The expected behavior is level trigger.
The piece of code you are referring to is from power save init, where we are setting the initial value of GPIO as HIGH.
However, if the FW is already present and running, with unknown power save state, a GPIO toggle ensures the chip wakes up, and FW and driver are in sync.

Thanks,
Neeraj