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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <AS4PR04MB969234817F3568F6DB877C33E7712@AS4PR04MB9692.eurprd04.prod.outlook.com>
Date: Thu, 3 Oct 2024 15:38:05 +0000
From: Neeraj Sanjay Kale <neeraj.sanjaykale@....com>
To: Shenwei Wang <shenwei.wang@....com>, "marcel@...tmann.org"
	<marcel@...tmann.org>, "luiz.dentz@...il.com" <luiz.dentz@...il.com>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>
CC: "linux-bluetooth@...r.kernel.org" <linux-bluetooth@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>, Amitkumar Karwar
	<amitkumar.karwar@....com>, Rohit Fule <rohit.fule@....com>, Sherry Sun
	<sherry.sun@....com>, Luke Wang <ziniu.wang_1@....com>, Bough Chen
	<haibo.chen@....com>, LnxRevLi <LnxRevLi@....com>
Subject: RE: [PATCH v1 2/2] Bluetooth: btnxpuart: Add GPIO support to power
 save feature

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@....com>
> > ---
> >  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