Change in ...coreboot[master]: gale: add dev switch back as physical presence GPIO

[Joel Kitching (Code Review)]

Joel Kitching has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31993

Change subject: gale: add dev switch back as physical presence GPIO
......................................................................

gale: add dev switch back as physical presence GPIO

gale has a button which is essentially used as a
"physical presence" button.  Its only use is to emulate
^D or ^U on boot when the button is pressed.
(See depthcharge src/board/gale/board.c)

Previously (and currently in CrOS firmware branch) this
GPIO was defined as the physical developer switch,
and read as such in depthcharge.  It was removed in
cleanup patch CB:18980.

Add the GPIO back as "physical presence", which will
be read by depthcharge in CL:1532492.

BUG=b:124141368, b:124192753, chromium:942901
TEST=util/lint/checkpatch.pl -g origin/master..HEAD
TEST=util/abuild/abuild -B -e -y -c 50 -p none -x
TEST=make clean && make test-abuild
BRANCH=none

Change-Id: Ic144f839b7f9933d573db8f84c4bf5905eea96f6
Signed-off-by: Joel Kitching 
---
M src/mainboard/google/gale/chromeos.c
1 file changed, 3 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/31993/1

diff --git a/src/mainboard/google/gale/chromeos.c b/src/mainboard/google/gale/chromeos.c
index 939b061..78ca900 100644
--- a/src/mainboard/google/gale/chromeos.c
+++ b/src/mainboard/google/gale/chromeos.c
@@ -25,6 +25,8 @@
 #include <timer.h>
 #include <vendorcode/google/chromeos/chromeos.h>

+#define PP_SW   41
+#define PP_POL  ACTIVE_LOW
 #define REC_POL ACTIVE_LOW
 #define WP_POL  ACTIVE_LOW

@@ -69,6 +71,7 @@
 void fill_lb_gpios(struct lb_gpios *gpios)
 {
    struct lb_gpio chromeos_gpios[] = {
+		{PP_SW, PP_POL, read_gpio(DEV_SW), "physical presence"},
    	{get_rec_sw_gpio_pin(), REC_POL,
    		read_gpio(get_rec_sw_gpio_pin()), "recovery"},
    	{get_wp_status_gpio_pin(), WP_POL,

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31993 )

Change subject: gale: add dev switch back as physical presence GPIO
......................................................................

Patch Set 1: Code-Review+2

(1 comment)

LGTM after fix

https://review.coreboot.org/#/c/31993/1/src/mainboard/google/gale/chromeos.c 
File src/mainboard/google/gale/chromeos.c:

https://review.coreboot.org/#/c/31993/1/src/mainboard/google/gale/chromeos.c... 
PS1, Line 74: DEV_SW
PP_SW?

[Joel Kitching (Code Review)]

Hello Aaron Durbin, Simon Glass, Julius Werner, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31993

to look at the new patch set (#2).

Change subject: gale: add dev switch back as physical presence GPIO
......................................................................

gale: add dev switch back as physical presence GPIO

gale has a button which is essentially used as a
"physical presence" button.  Its only use is to emulate
^D or ^U on boot when the button is pressed.
(See depthcharge src/board/gale/board.c)

Previously (and currently in CrOS firmware branch) this
GPIO was defined as the physical developer switch,
and read as such in depthcharge.  It was removed in
cleanup patch CB:18980.

Add the GPIO back as "physical presence", which will
be read by depthcharge in CL:1532492.

BUG=b:124141368, b:124192753, chromium:942901
TEST=util/lint/checkpatch.pl -g origin/master..HEAD
TEST=util/abuild/abuild -B -e -y -c 50 -p none -x
TEST=make clean && make test-abuild
BRANCH=none

Change-Id: Ic144f839b7f9933d573db8f84c4bf5905eea96f6
Signed-off-by: Joel Kitching 
---
M src/mainboard/google/gale/chromeos.c
1 file changed, 3 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/31993/2

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31993 )

Change subject: gale: add dev switch back as physical presence GPIO
......................................................................

Patch Set 2: Code-Review-1

(1 comment)

Looks like we are one character short... =(

src/mainboard/google/gale/chromeos.c: In function 'fill_lb_gpios':
src/mainboard/google/gale/chromeos.c:74:37: error: initializer-string for array of chars is too long [-Werror]
   {PP_SW, PP_POL, read_gpio(PP_SW), "physical presence"},
                                     ^~~~~~~~~~~~~~~~~~~
src/mainboard/google/gale/chromeos.c:74:37: note: (near initialization for 'chromeos_gpios[0].name')

http://review.coreboot.org/866
100 +struct lb_gpio {
101 + uint32_t port;
102 + uint32_t polarity;
103 + uint32_t value;
104 +#define GPIO_MAX_NAME_LENGTH 16
105 +        uint8_t name[GPIO_MAX_NAME_LENGTH];
106 +};

Given that this data is shared across the boundary from coreboot to the payload, is GPIO_MAX_NAME_LENGTH something that we can change?

Alternatively, any clever abbreviations for "physical presence"?

https://review.coreboot.org/#/c/31993/1/src/mainboard/google/gale/chromeos.c 
File src/mainboard/google/gale/chromeos.c:

https://review.coreboot.org/#/c/31993/1/src/mainboard/google/gale/chromeos.c... 
PS1, Line 74: DEV_SW
> PP_SW?

Done

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31993 )

Change subject: gale: add dev switch back as physical presence GPIO
......................................................................

Patch Set 2:
> Given that this data is shared across the boundary from coreboot to the payload, is GPIO_MAX_NAME_LENGTH something that we can change?
>
> Alternatively, any clever abbreviations for "physical presence"?

Let's avoid changing the record format unless we really have to. How about just "presence"?

[Joel Kitching (Code Review)]

Hello Aaron Durbin, Simon Glass, Julius Werner, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31993

to look at the new patch set (#3).

Change subject: gale: add dev switch back as physical presence GPIO
......................................................................

gale: add dev switch back as physical presence GPIO

gale has a button which is essentially used as a
"physical presence" button.  Its only use is to emulate
^D or ^U on boot when the button is pressed.
(See depthcharge src/board/gale/board.c)

Previously (and currently in CrOS firmware branch) this
GPIO was defined as the physical developer switch,
and read as such in depthcharge.  It was removed in
cleanup patch CB:18980.

Add the GPIO back as physical presence ("presence"),
which will be read by depthcharge in CL:1532492.

BUG=b:124141368, b:124192753, chromium:942901
TEST=util/lint/checkpatch.pl -g origin/master..HEAD
TEST=util/abuild/abuild -B -e -y -c 50 -p none -x
TEST=make clean && make test-abuild
BRANCH=none

Change-Id: Ic144f839b7f9933d573db8f84c4bf5905eea96f6
Signed-off-by: Joel Kitching 
---
M src/mainboard/google/gale/chromeos.c
1 file changed, 3 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/93/31993/3

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31993 )

Change subject: gale: add dev switch back as physical presence GPIO
......................................................................

Patch Set 3: Code-Review+1
> Patch Set 2:
> > Given that this data is shared across the boundary from coreboot to the payload, is GPIO_MAX_NAME_LENGTH something that we can change?
> >
> > Alternatively, any clever abbreviations for "physical presence"?
>
> Let's avoid changing the record format unless we really have to. How about just "presence"?

OK, done.

[Julius Werner (Code Review)]

Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31993 )

Change subject: gale: add dev switch back as physical presence GPIO
......................................................................

Patch Set 3: Code-Review+2

You need to re-fetch and rebase, a bad change made it into the tree that has already been fixed but your local checkout doesn't have the fix yet.

LGTM otherwise.

[Simon Glass (Code Review)]

Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31993 )

Change subject: gale: add dev switch back as physical presence GPIO
......................................................................

Patch Set 3: Code-Review+1

[Joel Kitching (Code Review)]

Joel Kitching has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31993 )

Change subject: gale: add dev switch back as physical presence GPIO
......................................................................

Patch Set 4:
> Patch Set 3: Code-Review+2
>
> You need to re-fetch and rebase, a bad change made it into the tree that has already been fixed but your local checkout doesn't have the fix yet.
>
> LGTM otherwise.

Done, thanks!

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31993 )

Change subject: gale: add dev switch back as physical presence GPIO
......................................................................

gale: add dev switch back as physical presence GPIO

gale has a button which is essentially used as a
"physical presence" button.  Its only use is to emulate
^D or ^U on boot when the button is pressed.
(See depthcharge src/board/gale/board.c)

Previously (and currently in CrOS firmware branch) this
GPIO was defined as the physical developer switch,
and read as such in depthcharge.  It was removed in
cleanup patch CB:18980.

Add the GPIO back as physical presence ("presence"),
which will be read by depthcharge in CL:1532492.

BUG=b:124141368, b:124192753, chromium:942901
TEST=util/lint/checkpatch.pl -g origin/master..HEAD
TEST=util/abuild/abuild -B -e -y -c 50 -p none -x
TEST=make clean && make test-abuild
BRANCH=none

Change-Id: Ic144f839b7f9933d573db8f84c4bf5905eea96f6
Signed-off-by: Joel Kitching 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31993
Tested-by: build bot (Jenkins) 
Reviewed-by: Julius Werner 
Reviewed-by: Simon Glass 
---
M src/mainboard/google/gale/chromeos.c
1 file changed, 3 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Julius Werner: Looks good to me, approved
  Simon Glass: Looks good to me, but someone else must approve
  Joel Kitching: Looks good to me, but someone else must approve

diff --git a/src/mainboard/google/gale/chromeos.c b/src/mainboard/google/gale/chromeos.c
index 09fdc2f..66b1a62 100644
--- a/src/mainboard/google/gale/chromeos.c
+++ b/src/mainboard/google/gale/chromeos.c
@@ -24,6 +24,8 @@
 #include <timer.h>
 #include <vendorcode/google/chromeos/chromeos.h>

+#define PP_SW   41
+#define PP_POL  ACTIVE_LOW
 #define REC_POL ACTIVE_LOW
 #define WP_POL  ACTIVE_LOW

@@ -68,6 +70,7 @@
 void fill_lb_gpios(struct lb_gpios *gpios)
 {
    struct lb_gpio chromeos_gpios[] = {
+		{PP_SW, PP_POL, read_gpio(PP_SW), "presence"},
    	{get_rec_sw_gpio_pin(), REC_POL,
    		read_gpio(get_rec_sw_gpio_pin()), "recovery"},
    	{get_wp_status_gpio_pin(), WP_POL,