Change in ...coreboot[master]: soc/amd/stoneyridge/gpio: Allow specifying 0 value for debounce timeout
Hello Daniel Kurtz,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/30998
to review the following change.
Change subject: soc/amd/stoneyridge/gpio: Allow specifying 0 value for debounce timeout ......................................................................
soc/amd/stoneyridge/gpio: Allow specifying 0 value for debounce timeout
It is possible to configure debounce, but leave it disabled by specifying a 0 value for the timeout. Add a define for allowing to do so via the PAD_DEBOUNCE() macro.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
BUG=b:113880780 BRANCH=none TEST=compile
Change-Id: I9de61297b0677cc904535a51c16970eecb52021d --- M src/soc/amd/stoneyridge/include/soc/gpio.h 1 file changed, 1 insertion(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/98/30998/1
diff --git a/src/soc/amd/stoneyridge/include/soc/gpio.h b/src/soc/amd/stoneyridge/include/soc/gpio.h index 04eda49..47eae84 100644 --- a/src/soc/amd/stoneyridge/include/soc/gpio.h +++ b/src/soc/amd/stoneyridge/include/soc/gpio.h @@ -462,6 +462,7 @@ #define GPIO_TIMEBASE_15560uS (1 << 7) #define GPIO_TIMEBASE_62440uS (GPIO_TIMEBASE_183uS | \ GPIO_TIMEBASE_15560uS) +#define GPIO_IN_DEBOUNCE_DISABLED (0 | GPIO_TIMEBASE_61uS) #define GPIO_IN_60uS (1 | GPIO_TIMEBASE_61uS) #define GPIO_IN_120uS (2 | GPIO_TIMEBASE_61uS) #define GPIO_IN_200uS (3 | GPIO_TIMEBASE_61uS)
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30998 )
Change subject: soc/amd/stoneyridge/gpio: Allow specifying 0 value for debounce timeout ......................................................................
Patch Set 1: Code-Review+1
(1 comment)
https://review.coreboot.org/#/c/30998/1/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/gpio.h:
https://review.coreboot.org/#/c/30998/1/src/soc/amd/stoneyridge/include/soc/... PS1, Line 465: #define GPIO_IN_DEBOUNCE_DISABLED (0 | GPIO_TIMEBASE_61uS) I think it would be clearer if it was just 0.
Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30998 )
Change subject: soc/amd/stoneyridge/gpio: Allow specifying 0 value for debounce timeout ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/30998/1/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/gpio.h:
https://review.coreboot.org/#/c/30998/1/src/soc/amd/stoneyridge/include/soc/... PS1, Line 465: #define GPIO_IN_DEBOUNCE_DISABLED (0 | GPIO_TIMEBASE_61uS)
I think it would be clearer if it was just 0.
We do it both ways in plenty of places in the coreboot code. It's like (1 << 0) to match other shifts. I'm going to give it a +2 and let Dan decide whether to change it or not.
Daniel Kurtz has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30998 )
Change subject: soc/amd/stoneyridge/gpio: Allow specifying 0 value for debounce timeout ......................................................................
Patch Set 1:
(1 comment)
This change is ready for review.
https://review.coreboot.org/#/c/30998/1/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/gpio.h:
https://review.coreboot.org/#/c/30998/1/src/soc/amd/stoneyridge/include/soc/... PS1, Line 465: #define GPIO_IN_DEBOUNCE_DISABLED (0 | GPIO_TIMEBASE_61uS)
We do it both ways in plenty of places in the coreboot code. […]
"just 0" would (silently) imply GPIO_TIMEBASE_61uS, which is just one of 4 possible "zeros".
To make the "have coreboot preset the exact same values as kernel" work, we are relying on it choosing exactly the GPIO_TIMEBASE_61uS 0.
Thus, IMHO it is better to be explicit.
Raul Rangel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/30998 )
Change subject: soc/amd/stoneyridge/gpio: Allow specifying 0 value for debounce timeout ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/30998/1/src/soc/amd/stoneyridge/include/soc/... File src/soc/amd/stoneyridge/include/soc/gpio.h:
https://review.coreboot.org/#/c/30998/1/src/soc/amd/stoneyridge/include/soc/... PS1, Line 465: #define GPIO_IN_DEBOUNCE_DISABLED (0 | GPIO_TIMEBASE_61uS)
"just 0" would (silently) imply GPIO_TIMEBASE_61uS, which is just one of 4 possible "zeros". […]
sgtm
Patrick Georgi has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/30998 )
Change subject: soc/amd/stoneyridge/gpio: Allow specifying 0 value for debounce timeout ......................................................................
soc/amd/stoneyridge/gpio: Allow specifying 0 value for debounce timeout
It is possible to configure debounce, but leave it disabled by specifying a 0 value for the timeout. Add a define for allowing to do so via the PAD_DEBOUNCE() macro.
Signed-off-by: Daniel Kurtz djkurtz@chromium.org
BUG=b:113880780 BRANCH=none TEST=compile
Change-Id: I9de61297b0677cc904535a51c16970eecb52021d Reviewed-on: https://review.coreboot.org/c/30998 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Martin Roth martinroth@google.com Reviewed-by: Raul Rangel rrangel@chromium.org --- M src/soc/amd/stoneyridge/include/soc/gpio.h 1 file changed, 1 insertion(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Martin Roth: Looks good to me, approved Raul Rangel: Looks good to me, approved
diff --git a/src/soc/amd/stoneyridge/include/soc/gpio.h b/src/soc/amd/stoneyridge/include/soc/gpio.h index 04eda49..47eae84 100644 --- a/src/soc/amd/stoneyridge/include/soc/gpio.h +++ b/src/soc/amd/stoneyridge/include/soc/gpio.h @@ -462,6 +462,7 @@ #define GPIO_TIMEBASE_15560uS (1 << 7) #define GPIO_TIMEBASE_62440uS (GPIO_TIMEBASE_183uS | \ GPIO_TIMEBASE_15560uS) +#define GPIO_IN_DEBOUNCE_DISABLED (0 | GPIO_TIMEBASE_61uS) #define GPIO_IN_60uS (1 | GPIO_TIMEBASE_61uS) #define GPIO_IN_120uS (2 | GPIO_TIMEBASE_61uS) #define GPIO_IN_200uS (3 | GPIO_TIMEBASE_61uS)
-
Daniel Kurtz (Code Review) -
Martin Roth (Code Review)
-
Patrick Georgi (Code Review)
-
Raul Rangel (Code Review)