From 2dc1418ccdad1b06f686cddc717e02bcb80ff648 Mon Sep 17 00:00:00 2001 From: Randall Spangler Date: Fri, 25 Oct 2013 10:26:16 -0700 Subject: cleanup: Assorted TODO comments Remove comments if no longer applicable, or assign bug numbers if they still are. Tidy some debug output. No code changes other than the debug output. BUG=chrome-os-partner:18343 BRANCH=none TEST=build all platforms, pass unit tests Change-Id: I2277e73fbf8cc93f3b1b35ee115e0f2f52eb8cf9 Signed-off-by: Randall Spangler Reviewed-on: https://chromium-review.googlesource.com/175215 Reviewed-by: Bill Richardson --- chip/lm4/gpio.c | 3 ++- chip/lm4/peci.c | 17 +++++++++++++---- common/extpower_spring.c | 4 ++-- common/host_command.c | 46 +++++++++++++++++++++++---------------------- common/keyboard_scan.c | 7 +------ common/system.c | 7 +++++-- common/util.c | 15 ++++++++++++--- driver/usb_switch_tsu6721.c | 6 +++--- power/baytrail.c | 1 - 9 files changed, 62 insertions(+), 44 deletions(-) diff --git a/chip/lm4/gpio.c b/chip/lm4/gpio.c index 38b5e7ed46..7bed71bbc6 100644 --- a/chip/lm4/gpio.c +++ b/chip/lm4/gpio.c @@ -47,8 +47,9 @@ void gpio_set_alternate_function(uint32_t port, uint32_t mask, int func) int port_index = find_gpio_port_index(port); int cgmask; + /* Ignore (do nothing for) invalid port values */ if (port_index < 0) - return; /* TODO: assert */ + return; /* Enable the GPIO port in run and sleep. */ cgmask = 1 << port_index; diff --git a/chip/lm4/peci.c b/chip/lm4/peci.c index 7f0d692439..74f081d6c4 100644 --- a/chip/lm4/peci.c +++ b/chip/lm4/peci.c @@ -16,8 +16,13 @@ #include "temp_sensor.h" #include "util.h" -/* Max junction temperature for processor in degrees C */ -/* TODO: read TjMax from processor via PECI */ +/* + * Max junction temperature for processor in degrees C. This is correct for + * Ivy Bridge and Haswell; future chips don't have PECI. + * + * In theory we could read TjMax from the processor via PECI, but that requires + * closed-source Intel PECI commands. + */ #define PECI_TJMAX 105 /* Initial PECI baud rate */ @@ -26,8 +31,12 @@ /* Polling interval for PECI, in ms */ #define PECI_POLL_INTERVAL_MS 250 -/* Internal and external path delays, in ns */ -#define PECI_TD_FET_NS 60 /* Guess; TODO: what is real delay */ +/* + * Internal and external path delays, in ns. The external delay is a + * best-guess measurement, but we're fairly tolerant of a bad guess because + * PECI_BAUD_RATE is slow compared to PECI's actual maximum baud rate. + */ +#define PECI_TD_FET_NS 60 #define PECI_TD_INT_NS 80 /* Number of controller retries. Should be between 0 and 7. */ diff --git a/common/extpower_spring.c b/common/extpower_spring.c index 022788704b..de294d4e9f 100644 --- a/common/extpower_spring.c +++ b/common/extpower_spring.c @@ -600,8 +600,8 @@ static void usb_device_change(int dev_type) /* External API */ /* - * TODO: Init here until we can do with HOOK_INIT. Just need to set prio so we - * init before the charger task does. + * TODO(crosbug.com/p/23741): Init here until we can do with HOOK_INIT. Just + * need to set prio so we init before the charger task does. */ void extpower_charge_init(void) { diff --git a/common/host_command.c b/common/host_command.c index 55f27d9199..34c274001a 100644 --- a/common/host_command.c +++ b/common/host_command.c @@ -72,44 +72,46 @@ test_mockable void host_send_response(struct host_cmd_handler_args *args) { #ifdef CONFIG_HOST_COMMAND_STATUS /* - * TODO(sjg@chromium.org): - * If we got an 'in progress' previously, then this - * must be the completion of that command, so stash the result - * code. We can't send it back to the host now since we already sent - * the in-progress response and the host is on to other things now. * - * If we are in interrupt context, then we are handling a - * get_status response or an immediate error which prevented us - * from processing the command. Note we can't check for the - * GET_COMMS_STATUS command in args->command because the original - * command value has now been overwritten. + * If we are in interrupt context, then we are handling a get_status + * response or an immediate error which prevented us from processing + * the command. Note we can't check for the GET_COMMS_STATUS command in + * args->command because the original command value has now been + * overwritten. * * When a EC_CMD_RESEND_RESPONSE arrives we will supply this response * to that command. - * - * We don't support stashing response data, so mark the response as - * unavailable in that case. - * - * TODO(sjg@chromium.org): If we stashed the command in host_command - * before processing it, then it would not get overwritten by a - * subsequent command and we could simplify the logic here by adding - * a flag to host_cmd_handler_args to indicate that the command had - * an interim response. We would have to make this stashing dependent - * on CONFIG_HOST_COMMAND_STATUS also. */ if (!in_interrupt_context()) { if (command_pending) { - CPRINTF("pending complete, size=%d, result=%d\n", + /* + * We previously got EC_RES_IN_PROGRESS. This must be + * the completion of that command, so stash the result + * code. + */ + CPRINTF("[%T HC pending done, size=%d, result=%d]\n", args->response_size, args->result); + + /* + * We don't support stashing response data, so mark the + * response as unavailable in that case. + */ if (args->response_size != 0) saved_result = EC_RES_UNAVAILABLE; else saved_result = args->result; + + /* + * We can't send the response back to the host now + * since we already sent the in-progress response and + * the host is on to other things now. + */ command_pending = 0; return; + } else if (args->result == EC_RES_IN_PROGRESS) { command_pending = 1; - CPRINTF("Command pending\n"); + CPRINTF("[HC pending]\n"); } } #endif diff --git a/common/keyboard_scan.c b/common/keyboard_scan.c index 9641ff6ca9..d1e90c72c7 100644 --- a/common/keyboard_scan.c +++ b/common/keyboard_scan.c @@ -301,12 +301,7 @@ static int check_keys_changed(uint8_t *state) /* Read the raw key state */ any_pressed = read_matrix(new_state); - /* Ignore if so many keys are pressed that we're ghosting */ - /* - * TODO: maybe in this case we should reset all the debounce times, - * because in the ghosting case we're not paying attention to any of - * the keys which aren't ghosting. - */ + /* Ignore if so many keys are pressed that we're ghosting. */ if (has_ghosting(new_state)) return any_pressed; diff --git a/common/system.c b/common/system.c index 4af045a641..dc965aea1e 100644 --- a/common/system.c +++ b/common/system.c @@ -220,8 +220,11 @@ void system_disable_jump(void) #ifdef CONFIG_MPU /* - * Lock down memory - * TODO: Lock down other images (RO or RW) not running. + * Lock down memory to prevent code execution from data areas. + * + * TODO(crosbug.com/p/16904): Also lock down the image which isn't + * running (RO if RW, or vice versa), so a bad or malicious jump can't + * execute code from that image. */ { int mpu_error = mpu_protect_ram(); diff --git a/common/util.c b/common/util.c index f120cdad24..46e11372ed 100644 --- a/common/util.c +++ b/common/util.c @@ -171,7 +171,10 @@ int memcmp(const void *s1, const void *s2, int len) void *memcpy(void *dest, const void *src, int len) { - /* TODO: optimized version using LDM/STM would be much faster */ + /* + * TODO(crosbug.com/p/23720): if src/dest are aligned, copy a word at a + * time instead. + */ char *d = (char *)dest; const char *s = (const char *)src; while (len > 0) { @@ -184,7 +187,10 @@ void *memcpy(void *dest, const void *src, int len) void *memset(void *dest, int c, int len) { - /* TODO: optimized version using STM would be much faster */ + /* + * TODO(crosbug.com/p/23720): if dest is aligned, copy a word at a time + * instead. + */ char *d = (char *)dest; while (len > 0) { *(d++) = c; @@ -205,7 +211,10 @@ void *memmove(void *dest, const void *src, int len) /* Copy from end, so we don't overwrite the source */ char *d = (char *)dest + len; const char *s = (const char *)src + len; - /* TODO: optimized version using LDM/STM would be much faster */ + /* + * TODO(crosbug.com/p/23720): if src/dest are aligned, copy a + * word at a time instead. + */ while (len > 0) { *(--d) = *(--s); len--; diff --git a/driver/usb_switch_tsu6721.c b/driver/usb_switch_tsu6721.c index dc089561be..f31641aeb4 100644 --- a/driver/usb_switch_tsu6721.c +++ b/driver/usb_switch_tsu6721.c @@ -168,9 +168,9 @@ int tsu6721_init(void) return res ? EC_ERROR_UNKNOWN : EC_SUCCESS; } /* - * TODO(vpalatin): using the I2C early in the HOOK_INIT - * currently triggers all sort of badness, I need to debug - * this before re-activatin this initialization. + * TODO(crosbug.com/p/23741): Using I2C early in the HOOK_INIT currently + * triggers all sort of badness. Debug this before re-activating + * initialization in HOOK_INIT. */ #if 0 DECLARE_HOOK(HOOK_INIT, tsu6721_init, HOOK_PRIO_DEFAULT); diff --git a/power/baytrail.c b/power/baytrail.c index e76cff93e9..9ca1633a2f 100644 --- a/power/baytrail.c +++ b/power/baytrail.c @@ -60,7 +60,6 @@ void chipset_force_shutdown(void) * Force x86 off. This condition will reset once the state machine * transitions to G3. */ - /* TODO(rspangler): verify this works */ gpio_set_level(GPIO_PCH_SYS_PWROK, 0); gpio_set_level(GPIO_PCH_RSMRST_L, 0); } -- cgit v1.2.1