Peichao Li has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
mb/google/wheelie: add new variant for wheelie
Add initial support for wheelie variant board.
BUG=b:154664137 BRANCH=None TEST=build
Signed-off-by: peichao.wang peichao.wang@bitland.corp-partner.google.com Change-Id: Id638e987f45c247dae824f221a38ccf32626572f --- A src/mainboard/google/dedede/variants/wheelie/Makefile.inc A src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h A src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h A src/mainboard/google/dedede/variants/wheelie/overridetree.cb 4 files changed, 35 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/40587/1
diff --git a/src/mainboard/google/dedede/variants/wheelie/Makefile.inc b/src/mainboard/google/dedede/variants/wheelie/Makefile.inc new file mode 100644 index 0000000..13afb36 --- /dev/null +++ b/src/mainboard/google/dedede/variants/wheelie/Makefile.inc @@ -0,0 +1,8 @@ +## +## This file is part of the coreboot project. +## +## +## SPDX-License-Identifier: GPL-2.0-or-later +## + +SPD_SOURCES = Micron_MT53E512M32D2NP_2GB #0b0000 diff --git a/src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h b/src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h new file mode 100644 index 0000000..70bd8e7 --- /dev/null +++ b/src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h @@ -0,0 +1,13 @@ +/* + * This file is part of the coreboot project. + * + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef MAINBOARD_EC_H +#define MAINBOARD_EC_H + +#include <baseboard/ec.h> + +#endif diff --git a/src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h b/src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h new file mode 100644 index 0000000..fd92743 --- /dev/null +++ b/src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h @@ -0,0 +1,13 @@ +/* + * This file is part of the coreboot project. + * + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef MAINBOARD_GPIO_H +#define MAINBOARD_GPIO_H + +#include <baseboard/gpio.h> + +#endif /* MAINBOARD_GPIO_H */ diff --git a/src/mainboard/google/dedede/variants/wheelie/overridetree.cb b/src/mainboard/google/dedede/variants/wheelie/overridetree.cb new file mode 100644 index 0000000..75ef7f7 --- /dev/null +++ b/src/mainboard/google/dedede/variants/wheelie/overridetree.cb @@ -0,0 +1 @@ +chip soc/intel/jasperlake
Peichao Li has removed Patrick Georgi from this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Removed reviewer Patrick Georgi.
Peichao Li has removed Martin Roth from this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Removed reviewer Martin Roth.
Peichao Li has removed Furquan Shaikh from this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Removed reviewer Furquan Shaikh.
Peichao Li has removed Justin TerAvest from this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Removed reviewer Justin TerAvest.
Peichao Li has removed Marco Chen from this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Removed reviewer Marco Chen.
Peichao Li has removed Magf - from this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Removed reviewer Magf -.
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Patch Set 1:
How about Kconfig.name and Kconfig?
Hello Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40587
to look at the new patch set (#2).
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
mb/google/wheelie: add new variant for wheelie
Add initial support for wheelie variant board.
BUG=b:154664137 BRANCH=None TEST=build
Signed-off-by: peichao.wang peichao.wang@bitland.corp-partner.google.com Change-Id: Id638e987f45c247dae824f221a38ccf32626572f --- M src/mainboard/google/dedede/Kconfig M src/mainboard/google/dedede/Kconfig.name A src/mainboard/google/dedede/variants/wheelie/Makefile.inc A src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h A src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h A src/mainboard/google/dedede/variants/wheelie/overridetree.cb 6 files changed, 43 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/40587/2
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40587/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40587/1//COMMIT_MSG@7 PS1, Line 7: wheelie dedede
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40587/2/src/mainboard/google/dedede... File src/mainboard/google/dedede/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/40587/2/src/mainboard/google/dedede... PS2, Line 25: select BOARD_ROMSIZE_KB_32768 nit: tab instead of spaces
Hello Furquan Shaikh, Patrick Georgi, Martin Roth, Justin TerAvest, Marco Chen, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40587
to look at the new patch set (#3).
Change subject: mb/google/dedede: add new variant for wheelie ......................................................................
mb/google/dedede: add new variant for wheelie
Add initial support for wheelie variant board.
BUG=b:154664137 BRANCH=None TEST=build
Signed-off-by: peichao.wang peichao.wang@bitland.corp-partner.google.com Change-Id: Id638e987f45c247dae824f221a38ccf32626572f --- M src/mainboard/google/dedede/Kconfig M src/mainboard/google/dedede/Kconfig.name A src/mainboard/google/dedede/variants/wheelie/Makefile.inc A src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h A src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h A src/mainboard/google/dedede/variants/wheelie/overridetree.cb 6 files changed, 43 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/40587/3
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Justin TerAvest, Marco Chen, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40587
to look at the new patch set (#4).
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
mb/google/wheelie: add new variant for wheelie
Add initial support for wheelie variant board.
BUG=b:154664137 BRANCH=None TEST=build
Signed-off-by: peichao.wang peichao.wang@bitland.corp-partner.google.com Change-Id: Id638e987f45c247dae824f221a38ccf32626572f --- M src/mainboard/google/dedede/Kconfig M src/mainboard/google/dedede/Kconfig.name A src/mainboard/google/dedede/variants/wheelie/Makefile.inc A src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h A src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h A src/mainboard/google/dedede/variants/wheelie/overridetree.cb 6 files changed, 43 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/40587/4
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40587/2/src/mainboard/google/dedede... File src/mainboard/google/dedede/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/40587/2/src/mainboard/google/dedede... PS2, Line 25: select BOARD_ROMSIZE_KB_32768
nit: tab instead of spaces
Dear Marco, done.
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Justin TerAvest, Marco Chen, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40587
to look at the new patch set (#5).
Change subject: mb/google/dedede: add new variant for wheelie ......................................................................
mb/google/dedede: add new variant for wheelie
Add initial support for wheelie variant board.
BUG=b:154664137 BRANCH=None TEST=build
Signed-off-by: peichao.wang peichao.wang@bitland.corp-partner.google.com Change-Id: Id638e987f45c247dae824f221a38ccf32626572f --- M src/mainboard/google/dedede/Kconfig M src/mainboard/google/dedede/Kconfig.name A src/mainboard/google/dedede/variants/wheelie/Makefile.inc A src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h A src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h A src/mainboard/google/dedede/variants/wheelie/overridetree.cb 6 files changed, 43 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/40587/5
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Justin TerAvest, Marco Chen, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40587
to look at the new patch set (#6).
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
mb/google/wheelie: add new variant for wheelie
Add initial support for wheelie variant board.
BUG=b:154664137 BRANCH=None TEST=build
Signed-off-by: peichao.wang peichao.wang@bitland.corp-partner.google.com Change-Id: Id638e987f45c247dae824f221a38ccf32626572f --- M src/mainboard/google/dedede/Kconfig M src/mainboard/google/dedede/Kconfig.name A src/mainboard/google/dedede/variants/wheelie/Makefile.inc A src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h A src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h A src/mainboard/google/dedede/variants/wheelie/overridetree.cb 6 files changed, 45 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/40587/6
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Patch Set 6:
(2 comments)
https://review.coreboot.org/c/coreboot/+/40587/1//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40587/1//COMMIT_MSG@7 PS1, Line 7: wheelie
dedede
Done
https://review.coreboot.org/c/coreboot/+/40587/2/src/mainboard/google/dedede... File src/mainboard/google/dedede/Kconfig.name:
https://review.coreboot.org/c/coreboot/+/40587/2/src/mainboard/google/dedede... PS2, Line 25: select BOARD_ROMSIZE_KB_32768
Dear Marco, done.
Done
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/40587/6/src/mainboard/google/dedede... File src/mainboard/google/dedede/Kconfig:
https://review.coreboot.org/c/coreboot/+/40587/6/src/mainboard/google/dedede... PS6, Line 81: Wheelie wheelie
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/wheelie: add new variant for wheelie ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/40587/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40587/6//COMMIT_MSG@7 PS6, Line 7: wheelie dedede
https://review.coreboot.org/c/coreboot/+/40587/6//COMMIT_MSG@15 PS6, Line 15: Signed-off-by: peichao.wang peichao.wang@bitland.corp-partner.google.com Nit: Move it after Change-Id line.
https://review.coreboot.org/c/coreboot/+/40587/6/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/wheelie/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/40587/6/src/mainboard/google/dedede... PS6, Line 1: chip soc/intel/jasperlake Define this as well, so that the I2C rise, hold and fall times can be tuned later.
# Intel Common SoC Config #+-------------------+---------------------------+ #| Field | Value | #+-------------------+---------------------------+ #| GSPI0 | cr50 TPM. Early init is | #| | required to set up a BAR | #| | for TPM communication | #| | before memory is up | #| I2C0 | Trackpad | #| I2C1 | Digitizer | #| I2C2 | Touchscreen | #| I2C3 | Camera | #| I2C4 | Audio | #+-------------------+---------------------------+ register "common_soc_config" = "{ .gspi[0] = { .speed_mhz = 1, .early_init = 1, }, .i2c[0] = { .speed = I2C_SPEED_FAST, }, .i2c[1] = { .speed = I2C_SPEED_FAST, }, .i2c[2] = { .speed = I2C_SPEED_FAST, }, .i2c[3] = { .speed = I2C_SPEED_FAST, }, .i2c[4] = { .speed = I2C_SPEED_FAST, }, }"
Hello build bot (Jenkins), Furquan Shaikh, Patrick Georgi, Martin Roth, Justin TerAvest, Marco Chen, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/40587
to look at the new patch set (#7).
Change subject: mb/google/dedede: add new variant for wheelie ......................................................................
mb/google/dedede: add new variant for wheelie
Add initial support for wheelie variant board.
BUG=b:154664137 BRANCH=None TEST=build
Change-Id: Id638e987f45c247dae824f221a38ccf32626572f Signed-off-by: peichao.wang peichao.wang@bitland.corp-partner.google.com --- M src/mainboard/google/dedede/Kconfig M src/mainboard/google/dedede/Kconfig.name A src/mainboard/google/dedede/variants/wheelie/Makefile.inc A src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h A src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h A src/mainboard/google/dedede/variants/wheelie/overridetree.cb 6 files changed, 81 insertions(+), 0 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/87/40587/7
Peichao Li has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/dedede: add new variant for wheelie ......................................................................
Patch Set 7:
(4 comments)
https://review.coreboot.org/c/coreboot/+/40587/6//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/40587/6//COMMIT_MSG@7 PS6, Line 7: wheelie
dedede
Done
https://review.coreboot.org/c/coreboot/+/40587/6//COMMIT_MSG@15 PS6, Line 15: Signed-off-by: peichao.wang peichao.wang@bitland.corp-partner.google.com
Nit: Move it after Change-Id line.
Done
https://review.coreboot.org/c/coreboot/+/40587/6/src/mainboard/google/dedede... File src/mainboard/google/dedede/Kconfig:
https://review.coreboot.org/c/coreboot/+/40587/6/src/mainboard/google/dedede... PS6, Line 81: Wheelie
wheelie
Done
https://review.coreboot.org/c/coreboot/+/40587/6/src/mainboard/google/dedede... File src/mainboard/google/dedede/variants/wheelie/overridetree.cb:
https://review.coreboot.org/c/coreboot/+/40587/6/src/mainboard/google/dedede... PS6, Line 1: chip soc/intel/jasperlake
Define this as well, so that the I2C rise, hold and fall times can be tuned later. […]
Done
Marco Chen has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/dedede: add new variant for wheelie ......................................................................
Patch Set 7: Code-Review+2
Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/dedede: add new variant for wheelie ......................................................................
Patch Set 7: Code-Review+2
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/40587 )
Change subject: mb/google/dedede: add new variant for wheelie ......................................................................
mb/google/dedede: add new variant for wheelie
Add initial support for wheelie variant board.
BUG=b:154664137 BRANCH=None TEST=build
Change-Id: Id638e987f45c247dae824f221a38ccf32626572f Signed-off-by: peichao.wang peichao.wang@bitland.corp-partner.google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/40587 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Marco Chen marcochen@google.com Reviewed-by: Karthik Ramasubramanian kramasub@google.com --- M src/mainboard/google/dedede/Kconfig M src/mainboard/google/dedede/Kconfig.name A src/mainboard/google/dedede/variants/wheelie/Makefile.inc A src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h A src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h A src/mainboard/google/dedede/variants/wheelie/overridetree.cb 6 files changed, 81 insertions(+), 0 deletions(-)
Approvals: build bot (Jenkins): Verified Marco Chen: Looks good to me, approved Karthik Ramasubramanian: Looks good to me, approved
diff --git a/src/mainboard/google/dedede/Kconfig b/src/mainboard/google/dedede/Kconfig index 1aabd05..3743908 100644 --- a/src/mainboard/google/dedede/Kconfig +++ b/src/mainboard/google/dedede/Kconfig @@ -55,6 +55,7 @@ default "Dedede" if BOARD_GOOGLE_DEDEDE default "Waddledoo" if BOARD_GOOGLE_WADDLEDOO default "Waddledee" if BOARD_GOOGLE_WADDLEDEE + default "Wheelie" if BOARD_GOOGLE_WHEELIE
config MAX_CPUS int @@ -77,5 +78,6 @@ default "dedede" if BOARD_GOOGLE_DEDEDE default "waddledoo" if BOARD_GOOGLE_WADDLEDOO default "waddledee" if BOARD_GOOGLE_WADDLEDEE + default "wheelie" if BOARD_GOOGLE_WHEELIE
endif #BOARD_GOOGLE_BASEBOARD_DEDEDE diff --git a/src/mainboard/google/dedede/Kconfig.name b/src/mainboard/google/dedede/Kconfig.name index 36c2467..cf9298c 100644 --- a/src/mainboard/google/dedede/Kconfig.name +++ b/src/mainboard/google/dedede/Kconfig.name @@ -17,3 +17,9 @@ select BOARD_GOOGLE_BASEBOARD_DEDEDE select BASEBOARD_DEDEDE_LAPTOP select BOARD_ROMSIZE_KB_32768 + +config BOARD_GOOGLE_WHEELIE + bool "Wheelie" + select BOARD_GOOGLE_BASEBOARD_DEDEDE + select BASEBOARD_DEDEDE_LAPTOP + select BOARD_ROMSIZE_KB_32768 diff --git a/src/mainboard/google/dedede/variants/wheelie/Makefile.inc b/src/mainboard/google/dedede/variants/wheelie/Makefile.inc new file mode 100644 index 0000000..13afb36 --- /dev/null +++ b/src/mainboard/google/dedede/variants/wheelie/Makefile.inc @@ -0,0 +1,8 @@ +## +## This file is part of the coreboot project. +## +## +## SPDX-License-Identifier: GPL-2.0-or-later +## + +SPD_SOURCES = Micron_MT53E512M32D2NP_2GB #0b0000 diff --git a/src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h b/src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h new file mode 100644 index 0000000..70bd8e7 --- /dev/null +++ b/src/mainboard/google/dedede/variants/wheelie/include/variant/ec.h @@ -0,0 +1,13 @@ +/* + * This file is part of the coreboot project. + * + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef MAINBOARD_EC_H +#define MAINBOARD_EC_H + +#include <baseboard/ec.h> + +#endif diff --git a/src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h b/src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h new file mode 100644 index 0000000..fd92743 --- /dev/null +++ b/src/mainboard/google/dedede/variants/wheelie/include/variant/gpio.h @@ -0,0 +1,13 @@ +/* + * This file is part of the coreboot project. + * + * + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef MAINBOARD_GPIO_H +#define MAINBOARD_GPIO_H + +#include <baseboard/gpio.h> + +#endif /* MAINBOARD_GPIO_H */ diff --git a/src/mainboard/google/dedede/variants/wheelie/overridetree.cb b/src/mainboard/google/dedede/variants/wheelie/overridetree.cb new file mode 100644 index 0000000..1e75864 --- /dev/null +++ b/src/mainboard/google/dedede/variants/wheelie/overridetree.cb @@ -0,0 +1,39 @@ +chip soc/intel/jasperlake + + # Intel Common SoC Config + #+-------------------+---------------------------+ + #| Field | Value | + #+-------------------+---------------------------+ + #| GSPI0 | cr50 TPM. Early init is | + #| | required to set up a BAR | + #| | for TPM communication | + #| | before memory is up | + #| I2C0 | Trackpad | + #| I2C1 | Digitizer | + #| I2C2 | Touchscreen | + #| I2C3 | Camera | + #| I2C4 | Audio | + #+-------------------+---------------------------+ + register "common_soc_config" = "{ + .gspi[0] = { + .speed_mhz = 1, + .early_init = 1, + }, + .i2c[0] = { + .speed = I2C_SPEED_FAST, + }, + .i2c[1] = { + .speed = I2C_SPEED_FAST, + }, + .i2c[2] = { + .speed = I2C_SPEED_FAST, + }, + .i2c[3] = { + .speed = I2C_SPEED_FAST, + }, + .i2c[4] = { + .speed = I2C_SPEED_FAST, + }, + }" + device domain 0 on end +end
-
Karthik Ramasubramanian (Code Review) -
Marco Chen (Code Review)
-
Patrick Georgi (Code Review)
-
Peichao Li (Code Review)