New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 783371 link

Starred by 1 user

Issue metadata

Status: Untriaged
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Task



Sign in to add a comment

ec: Deduplicate code across boards

Project Member Reported by rspangler@chromium.org, Nov 9 2017

Issue description

In ToT and firmware branches, we often copy/paste/modify entire board directories.

Do an analysis of ToT and the last couple of branches (e.g. reef) to determine how we can move more code to common/ or drivers/.

We've done that in the past with LED and battery/charging configuration.  We're likely due for another round.
 
Cc: chingcodes@chromium.org
There has been some accidental progress on this front in just the past few weeks (eg. https://chromium-review.googlesource.com/#/c/chromiumos/platform/ec/+/733954/, https://chromium-review.googlesource.com/#/c/chromiumos/platform/ec/+/702681/, https://chromium-review.googlesource.com/#/c/chromiumos/platform/ec/+/701412/,
https://chromium-review.googlesource.com/#/c/chromiumos/platform/ec/+/748919/) but we can make more improvements, if we spend time on it.

Also see the arrangement for kevin / gru.

- Both boards share a common board/ folder.
- LED code is very divergent, so we have separate source files for the two boards.
- #ifdefs are otherwise scattered (20 #ifdefs total, not too awful).

Comment 3 by sha...@chromium.org, Nov 10 2017

Here are some more low-hanging fruits:

tcpc_get_alert_status() - Move to common, need to standardize some GPIO naming.

pd_is_valid_input_voltage() - Only used by samus_pd, don't require definition.

board_is_vbus_too_low() - Can be defined in common, with exception of samus_pd.

pd_custom_vdm() - Move to common, with various VDO_CMD_* support behind #ifdefs.

svdm_dp_* - Move to common, two main options (w/ HPD GPIO and w/o).

svdm_gfu_* - I don't think we need support for this VDM on most of our devices.

'struct battery_info' - start_charging_min_c / start_charging_max_c fields are unused, seems like a bug.

board_cut_off_battery() - Almost without exception, involves writing some custom value to some custom manuf. access register twice. Register / value sometimes aren't known at compile time. Consider moving reg info into struct battery_info, then moving board_cut_off_battery() to common.

charger_profile_override_get_param() - Usually empty, don't require definition.

Comment 4 by sjg@chromium.org, Nov 14 2017

Shawn if people took these on could you help with reviews? What hardware would they need to test?

Comment 5 by sha...@chromium.org, Nov 14 2017

Sure, I can always do code reviews. Any recent Chromebook with TOT EC FW support would be fine for testing (I recommend kevin).

The difficulty of implementing the items in #3 ranges from simple to not so simple.
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 18 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/34a97f50d5275f1fa2f1cb377683c96286b2421d

commit 34a97f50d5275f1fa2f1cb377683c96286b2421d
Author: Aseda Aboagye <aaboagye@google.com>
Date: Sat Nov 18 04:18:38 2017

buttons: Make buttons[] common.

Nearly every board had a buttons array defined in which its contents had
the standard volume buttons.  This commit creates a single common
buttons array that can contain the standard volume buttons and recovery
buttons.  If a board has volume up and down buttons, they can simply
define CONFIG_VOLUME_BUTTONS and it will populate the buttons array with
the standard definition.  The buttons are active low and have a 30 ms
debounce period.  Similiarly, if a board has a dedicated recovery
button, defining CONFIG_DEDICATED_RECOVERY_BUTTON will also populate the
buttons array with a recovery button.

BUG=chromium:783371
BRANCH=None
TEST=make -j buildall.
TEST=Flash a device with CONFIG_VOLUME_BUTTONS, verify pressing volume
buttons still work.

Change-Id: Ie5d63670ca4c6b146ec8ffb64d40ea9ce437b913
Signed-off-by: Aseda Aboagye <aaboagye@google.com>
Reviewed-on: https://chromium-review.googlesource.com/773794
Commit-Ready: Aseda Aboagye <aaboagye@chromium.org>
Tested-by: Aseda Aboagye <aaboagye@chromium.org>
Reviewed-by: Daisuke Nojiri <dnojiri@chromium.org>
Reviewed-by: Shawn N <shawnn@chromium.org>

[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/glkrvp/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/rainier/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/eve/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/common/keyboard_mkbp.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/common/build.mk
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/eve/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/rainier/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/glkrvp/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/test/test_config.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/reef_it8320/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/reef/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/reef_it8320/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/reef/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/nautilus/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/rowan/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/poppy/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/nautilus/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/poppy/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/rowan/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/wheatley/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/test/button.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/strago/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/nefario/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/coral/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/grunt/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/include/config.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/coral/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/nefario/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/strago/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/scarlet/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/grunt/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/fizz/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/glados/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/fizz/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/kahlee/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/glados/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/common/button.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/host/gpio.inc
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/common/main.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/kevin/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/host/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/scarlet/board.h
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/wheatley/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/board/kevin/board.c
[modify] https://crrev.com/34a97f50d5275f1fa2f1cb377683c96286b2421d/include/button.h

Project Member

Comment 7 by bugdroid1@chromium.org, Dec 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/ec/+/a6509d28ab33aedce55c12a081582fbe38b79235

commit a6509d28ab33aedce55c12a081582fbe38b79235
Author: Aseda Aboagye <aaboagye@google.com>
Date: Tue Dec 19 06:55:02 2017

config: Add CONFIG_BUTTON_TRIGGERED_RECOVERY.

The CONFIG_BUTTON_RECOVERY option was a little confusing especially when
we have the CONFIG_DEDICATED_RECOVERY_BUTTON option.  This commit
renames CONFIG_BUTTON_RECOVERY to CONFIG_BUTTON_TRIGGERED_RECOVERY to
help make things a little clearer.

Additionally, to avoid copy paste, defining
CONFIG_BUTTON_TRIGGERED_RECOVERY will populate the recovery_buttons
table with either the volume buttons or a dedicated recovery button
depending what the board is configured for.

Lastly, if CONFIG_DEDICATED_RECOVERY_BUTTON is defined,
CONFIG_BUTTON_TRIGGERED_RECOVERY is defined as well since it's implicit.

BUG=chromium:783371
BRANCH=None
TEST=make -j buildall

Change-Id: Idccaa4d049ace0df3b98b35bdd38ac9dbd843200
Signed-off-by: Aseda Aboagye <aaboagye@google.com>
Reviewed-on: https://chromium-review.googlesource.com/830917
Commit-Ready: Aseda Aboagye <aaboagye@chromium.org>
Tested-by: Aseda Aboagye <aaboagye@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Daisuke Nojiri <dnojiri@chromium.org>

[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/board/nami/board.c
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/board/rainier/board.h
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/board/scarlet/board.c
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/board/rowan/board.h
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/board/poppy/board.c
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/board/scarlet/board.h
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/board/rainier/board.c
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/board/nami/board.h
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/board/poppy/board.h
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/board/rowan/board.c
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/common/button.c
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/board/fizz/board.c
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/board/fizz/board.h
[modify] https://crrev.com/a6509d28ab33aedce55c12a081582fbe38b79235/include/config.h

Sign in to add a comment