Clean up GPIO module

Just code cleanup; no functional changes

BUG=chrome-os-partner:15579
BRANCH=none
TEST=build code; boot link; gpioget still works

Change-Id: If0770c1a5ce0d5c51ba528fbe2944a73fafa949b
Signed-off-by: Randall Spangler <rspangler@chromium.org>
Reviewed-on: https://gerrit.chromium.org/gerrit/36556
Reviewed-by: Simon Glass <sjg@chromium.org>

6 files changed, 138 insertions, 103 deletions

diff --git a/chip/lm4/gpio.c b/chip/lm4/gpio.c
index 23cedd52c3..b5b1105632 100644
--- a/chip/lm4/gpio.c
+++ b/chip/lm4/gpio.c
@@ -5,7 +5,8 @@
 
 /* GPIO module for Chrome EC */
 
-#include "board.h"
+#include "clock.h"
+#include "common.h"
 #include "gpio.h"
 #include "hooks.h"
 #include "power_button.h"
@@ -14,8 +15,7 @@
 #include "timer.h"
 #include "util.h"
 
-
-/* 0-terminated list of GPIO bases */
+/* 0-terminated list of GPIO base addresses */
 static const uint32_t gpio_bases[] = {
 	LM4_GPIO_A, LM4_GPIO_B, LM4_GPIO_C, LM4_GPIO_D,
 	LM4_GPIO_E, LM4_GPIO_F, LM4_GPIO_G, LM4_GPIO_H,
@@ -23,9 +23,15 @@ static const uint32_t gpio_bases[] = {
 	LM4_GPIO_N, LM4_GPIO_P, LM4_GPIO_Q, 0
 };
 
-
-/* Find the index of a GPIO port base address (LM4_GPIO_[A-Q]); this is used by
- * the clock gating registers.  Returns the index, or -1 if no match. */
+/**
+ * Find the index of a GPIO port base address
+ *
+ * This is used by the clock gating registers.
+ *
+ * @param port_base	Base address to find (LM4_GPIO_[A-Q])
+ *
+ * @return The index, or -1 if no match.
+ */
 static int find_gpio_port_index(uint32_t port_base)
 {
 	int i;
@@ -36,8 +42,7 @@ static int find_gpio_port_index(uint32_t port_base)
 	return -1;
 }
 
-
-int gpio_pre_init(void)
+void gpio_pre_init(void)
 {
 	volatile uint32_t scratch  __attribute__((unused));
 	const struct gpio_info *g = gpio_list;
@@ -84,8 +89,6 @@ int gpio_pre_init(void)
 		if ((g->flags & GPIO_OUTPUT) && !is_warm)
 			gpio_set_level(i, g->flags & GPIO_HIGH);
 	}
-
-	return EC_SUCCESS;
 }
 
 static void gpio_init(void)
@@ -104,6 +107,7 @@ static void gpio_init(void)
 	task_enable_irq(LM4_IRQ_GPIOL);
 	task_enable_irq(LM4_IRQ_GPIOM);
 #if defined(KB_SCAN_ROW_IRQ) && (KB_SCAN_ROW_IRQ != LM4_IRQ_GPION)
+	/* Don't enable interrupts for the keyboard input GPIO bank */
 	task_enable_irq(LM4_IRQ_GPION);
 #endif
 	task_enable_irq(LM4_IRQ_GPIOP);
@@ -122,11 +126,9 @@ void gpio_set_alternate_function(int port, int mask, int func)
 	/* Enable the GPIO port if necessary */
 	cgmask = 1 << port_index;
 	if ((LM4_SYSTEM_RCGCGPIO & cgmask) != cgmask) {
-		volatile uint32_t scratch  __attribute__((unused));
 		LM4_SYSTEM_RCGCGPIO |= cgmask;
-		/* Delay a few clocks before accessing GPIO registers on that
-		 * port. */
-		scratch = LM4_SYSTEM_RCGCGPIO;
+		/* Delay a few clocks before accessing registers */
+		clock_wait_cycles(3);
 	}
 
 	if (func) {
@@ -148,40 +150,39 @@ void gpio_set_alternate_function(int port, int mask, int func)
 	LM4_GPIO_DEN(port) |= mask;
 }
 
-
 const char *gpio_get_name(enum gpio_signal signal)
 {
 	return gpio_list[signal].name;
 }
 
-
 int gpio_get_level(enum gpio_signal signal)
 {
 	return LM4_GPIO_DATA(gpio_list[signal].port,
 			     gpio_list[signal].mask) ? 1 : 0;
 }
 
-
-int gpio_set_level(enum gpio_signal signal, int value)
+void gpio_set_level(enum gpio_signal signal, int value)
 {
-	/* Ok to write 0xff becuase LM4_GPIO_DATA bit-masks only the bit
-	 * we care about. */
+	/*
+	 * Ok to write 0xff becuase LM4_GPIO_DATA bit-masks only the bit
+	 * we care about.
+	 */
 	LM4_GPIO_DATA(gpio_list[signal].port,
 		      gpio_list[signal].mask) = (value ? 0xff : 0);
-	return EC_SUCCESS;
 }
 
-
-int gpio_set_flags(enum gpio_signal signal, int flags)
+void gpio_set_flags(enum gpio_signal signal, int flags)
 {
 	const struct gpio_info *g = gpio_list + signal;
 
 	if (flags & GPIO_DEFAULT)
-		return EC_SUCCESS;
+		return;
+
 	if (flags & GPIO_OUTPUT) {
-		/* Output */
-		/* Select open drain first, so that we don't glitch the signal
-		 * when changing the line to an output. */
+		/*
+		 * Select open drain first, so that we don't glitch the signal
+		 * when changing the line to an output.
+		 */
 		if (g->flags & GPIO_OPEN_DRAIN)
 			LM4_GPIO_ODR(g->port) |= g->mask;
 		else
@@ -220,11 +221,8 @@ int gpio_set_flags(enum gpio_signal signal, int flags)
 		LM4_GPIO_IBE(g->port) |= g->mask;
 	else
 		LM4_GPIO_IBE(g->port) &= ~g->mask;
-
-	return EC_SUCCESS;
 }
 
-
 int gpio_enable_interrupt(enum gpio_signal signal)
 {
 	const struct gpio_info *g = gpio_list + signal;
@@ -251,8 +249,10 @@ static void gpio_interrupt(int port, uint32_t mis)
 	}
 }
 
-/* Handlers for each GPIO port.  These read and clear the interrupt bits for
- * the port, then call the master handler above. */
+/**
+ * Handlers for each GPIO port.  These read and clear the interrupt bits for
+ * the port, then call the master handler above.
+ */
 #define GPIO_IRQ_FUNC(irqfunc, gpiobase)		\
 	static void irqfunc(void)			\
 	{						\
@@ -281,9 +281,10 @@ GPIO_IRQ_FUNC(__gpio_q_interrupt, LM4_GPIO_Q);
 
 #undef GPIO_IRQ_FUNC
 
-/* Declare IRQs */
-/* TODO: nesting this macro inside the GPIO_IRQ_FUNC macro works poorly because
- * DECLARE_IRQ() stringizes its inputs. */
+/*
+ * Declare IRQs.  Nesting this macro inside the GPIO_IRQ_FUNC macro works
+ * poorly because DECLARE_IRQ() stringizes its inputs.
+ */
 DECLARE_IRQ(LM4_IRQ_GPIOA, __gpio_a_interrupt, 1);
 DECLARE_IRQ(LM4_IRQ_GPIOB, __gpio_b_interrupt, 1);
 DECLARE_IRQ(LM4_IRQ_GPIOC, __gpio_c_interrupt, 1);
diff --git a/chip/stm32/gpio-stm32f100.c b/chip/stm32/gpio-stm32f100.c
index 5d91ff343e..b308138e93 100644
--- a/chip/stm32/gpio-stm32f100.c
+++ b/chip/stm32/gpio-stm32f100.c
@@ -5,7 +5,7 @@
 
 /* GPIO module for Chrome EC */
 
-#include "config.h"
+#include "common.h"
 #include "console.h"
 #include "gpio.h"
 #include "hooks.h"
@@ -32,7 +32,9 @@ struct port_config {
 	uint32_t cnf;
 };
 
-/* helper function for generating bitmasks for STM32 GPIO config registers */
+/**
+ * Helper function for generating bitmasks for STM32 GPIO config registers
+ */
 static void gpio_config_info(const struct gpio_info *g, uint32_t *addr,
 		uint32_t *mode, uint32_t *cnf) {
 	/*
@@ -52,13 +54,14 @@ static void gpio_config_info(const struct gpio_info *g, uint32_t *addr,
 	*cnf = *mode << 2;
 }
 
-int gpio_set_flags(enum gpio_signal signal, int flags)
+void gpio_set_flags(enum gpio_signal signal, int flags)
 {
 	const struct gpio_info *g = gpio_list + signal;
 	uint32_t addr, cnf, mode, mask;
 
 	if (flags & GPIO_DEFAULT)
-		return EC_SUCCESS;
+		return;
+
 	gpio_config_info(g, &addr, &mode, &cnf);
 	mask = REG32(addr) & ~(cnf | mode);
 
@@ -68,13 +71,15 @@ int gpio_set_flags(enum gpio_signal signal, int flags)
 	 * output, or alternate function.
 	 */
 	if (flags & GPIO_OUTPUT) {
-		/* FIXME: This assumes output max speed of 10MHz */
+		/* TODO: This assumes output max speed of 10MHz */
 		mask |= 0x11111111 & mode;
 		if (flags & GPIO_OPEN_DRAIN)
 			mask |= 0x44444444 & cnf;
 	} else {
-		/* GPIOx_ODR determines which resistor to activate in
-		 * input mode, see Table 16 (datasheet rm0041) */
+		/*
+		 * GPIOx_ODR determines which resistor to activate in
+		 * input mode, see Table 16 (datasheet rm0041)
+		 */
 		if ((flags & GPIO_PULL_UP) == GPIO_PULL_UP) {
 			mask |= 0x88888888 & cnf;
 			STM32_GPIO_BSRR_OFF(g->port) |= g->mask;
@@ -95,9 +100,9 @@ int gpio_set_flags(enum gpio_signal signal, int flags)
 	if ((flags & GPIO_OUTPUT) && !is_warm_boot)
 		/* General purpose, MODE = 01
 		 *
-		 * If this is a cold boot, set the level.
-		 * On a warm reboot, leave things where they were
-		 * or we'll shut off the AP. */
+		 * If this is a cold boot, set the level.  On a warm reboot,
+		 * leave things where they were or we'll shut off the AP.
+		 */
 		gpio_set_level(signal, flags & GPIO_HIGH);
 
 	REG32(addr) = mask;
@@ -109,12 +114,9 @@ int gpio_set_flags(enum gpio_signal signal, int flags)
 	if (flags & (GPIO_INT_FALLING | GPIO_INT_BOTH))
 		STM32_EXTI_FTSR |= g->mask;
 	/* Interrupt is enabled by gpio_enable_interrupt() */
-
-	return EC_SUCCESS;
 }
 
-
-int gpio_pre_init(void)
+void gpio_pre_init(void)
 {
 	const struct gpio_info *g = gpio_list;
 	int i;
@@ -123,7 +125,9 @@ int gpio_pre_init(void)
 		/* This is a warm reboot : TIM2 is already active */
 		is_warm_boot = 1;
 	} else {
-		/* Enable all GPIOs clocks
+		/*
+		 * Enable all GPIOs clocks
+		 *
 		 * TODO: more fine-grained enabling for power saving
 		 */
 		STM32_RCC_APB2ENR |= 0x1fd;
@@ -132,8 +136,6 @@ int gpio_pre_init(void)
 	/* Set all GPIOs to defaults */
 	for (i = 0; i < GPIO_COUNT; i++, g++)
 		gpio_set_flags(i, g->flags);
-
-	return EC_SUCCESS;
 }
 
 void gpio_init(void)
@@ -163,12 +165,10 @@ int gpio_get_level(enum gpio_signal signal)
 }
 
 
-int gpio_set_level(enum gpio_signal signal, int value)
+void gpio_set_level(enum gpio_signal signal, int value)
 {
 	STM32_GPIO_BSRR_OFF(gpio_list[signal].port) =
 			gpio_list[signal].mask << (value ? 0 : 16);
-
-	return EC_SUCCESS;
 }
 
 int gpio_enable_interrupt(enum gpio_signal signal)
diff --git a/chip/stm32/gpio-stm32l15x.c b/chip/stm32/gpio-stm32l15x.c
index 3984eccaf6..659e05df78 100644
--- a/chip/stm32/gpio-stm32l15x.c
+++ b/chip/stm32/gpio-stm32l15x.c
@@ -20,16 +20,15 @@
 /* For each EXTI bit, record which GPIO entry is using it */
 static const struct gpio_info *exti_events[16];
 
-int gpio_set_flags(enum gpio_signal signal, int flags)
+void gpio_set_flags(enum gpio_signal signal, int flags)
 {
 	/*
 	 * TODO(dhendrix): Move GPIO setup code from gpio_pre_init
 	 * into here like we did for STM32F
 	 */
-	return EC_ERROR_UNIMPLEMENTED;
 }
 
-int gpio_pre_init(void)
+void gpio_pre_init(void)
 {
 	const struct gpio_info *g = gpio_list;
 	int is_warm = 0;
@@ -73,9 +72,11 @@ int gpio_pre_init(void)
 		if (g->flags & GPIO_OUTPUT) { /* General purpose, MODE = 01 */
 			val |= 0x55555555 & mask2;
 			STM32_GPIO_MODER_OFF(g->port) = val;
-			/* If this is a cold boot, set the level.
-			 * On a warm reboot, leave things where they were
-			 * or we'll shut off the AP. */
+			/*
+			 * If this is a cold boot, set the level.  On a warm
+			 * reboot, leave things where they were or we'll shut
+			 * off the AP.
+			 */
 			if (!is_warm)
 				gpio_set_level(i, g->flags & GPIO_HIGH);
 		} else if (g->flags & GPIO_INPUT) { /* Input, MODE=00 */
@@ -90,8 +91,6 @@ int gpio_pre_init(void)
 			STM32_EXTI_FTSR |= g->mask;
 		/* Interrupt is enabled by gpio_enable_interrupt() */
 	}
-
-	return EC_SUCCESS;
 }
 
 static void gpio_init(void)
@@ -150,12 +149,10 @@ int gpio_get_level(enum gpio_signal signal)
 }
 
 
-int gpio_set_level(enum gpio_signal signal, int value)
+void gpio_set_level(enum gpio_signal signal, int value)
 {
 	STM32_GPIO_BSRR_OFF(gpio_list[signal].port) =
 			gpio_list[signal].mask << (value ? 0 : 16);
-
-	return EC_SUCCESS;
 }
 
 int gpio_enable_interrupt(enum gpio_signal signal)
diff --git a/chip/stm32/gpio.c b/chip/stm32/gpio.c
index a2d5b4daf7..d488c9c619 100644
--- a/chip/stm32/gpio.c
+++ b/chip/stm32/gpio.c
@@ -5,7 +5,7 @@
 
 /* Common GPIO module for Chrome EC */
 
-#include "board.h"
+#include "common.h"
 #include "gpio.h"
 
 
diff --git a/common/gpio_commands.c b/common/gpio_commands.c
index 2768540115..e429bc09a9 100644
--- a/common/gpio_commands.c
+++ b/common/gpio_commands.c
@@ -12,12 +12,15 @@
 #include "system.h"
 #include "util.h"
 
-
 static uint8_t last_val[(GPIO_COUNT + 7) / 8];
 
-
-/* Find a GPIO signal by name.  Returns the signal index, or GPIO_COUNT if
- * no match. */
+/**
+ * Find a GPIO signal by name.
+ *
+ * @param name		Signal name to find
+ *
+ * @return the signal index, or GPIO_COUNT if no match.
+ */
 static enum gpio_signal find_signal_by_name(const char *name)
 {
 	const struct gpio_info *g = gpio_list;
@@ -34,9 +37,14 @@ static enum gpio_signal find_signal_by_name(const char *name)
 	return GPIO_COUNT;
 }
 
-
-/* If v is different from the last value for index i, updates the last value
- * and returns 1; else returns 0. */
+/**
+ * Update last_val
+ *
+ * @param i		Index of last_val[] to update
+ * @param v		New value for last_val[i]
+ *
+ * @return 1 if last_val[i] was updated, 0 if last_val[i]==v.
+ */
 static int last_val_changed(int i, int v)
 {
 	if (v && !(last_val[i / 8] & (1 << (i % 8)))) {
@@ -80,8 +88,7 @@ static int command_gpio_get(int argc, char **argv)
 		changed = last_val_changed(i, v);
 		ccprintf("  %d%c %s\n", v, (changed ? '*' : ' '), g->name);
 
-		/* We have enough GPIOs that we'll overflow the output buffer
-		 * without flushing */
+		/* Flush console to avoid truncating output */
 		cflush();
 	}
 	return EC_SUCCESS;
@@ -91,7 +98,6 @@ DECLARE_CONSOLE_COMMAND(gpioget, command_gpio_get,
 			"Read GPIO value(s)",
 			NULL);
 
-
 static int command_gpio_set(int argc, char **argv)
 {
 	const struct gpio_info *g;
@@ -116,7 +122,9 @@ static int command_gpio_set(int argc, char **argv)
 	if (*e)
 		return EC_ERROR_PARAM2;
 
-	return gpio_set_level(i, v);
+	gpio_set_level(i, v);
+
+	return EC_SUCCESS;
 }
 DECLARE_CONSOLE_COMMAND(gpioset, command_gpio_set,
 			"name <0 | 1>",
@@ -145,7 +153,6 @@ static int gpio_command_get(struct host_cmd_handler_args *args)
 }
 DECLARE_HOST_COMMAND(EC_CMD_GPIO_GET, gpio_command_get, EC_VER_MASK(0));
 
-
 static int gpio_command_set(struct host_cmd_handler_args *args)
 {
 	const struct ec_params_gpio_set *p = args->params;
@@ -166,8 +173,8 @@ static int gpio_command_set(struct host_cmd_handler_args *args)
 	if (!(g->flags & GPIO_OUTPUT))
 		return EC_RES_ERROR;
 
-	if (gpio_set_level(i, p->val) != EC_SUCCESS)
-		return EC_RES_ERROR;
+	gpio_set_level(i, p->val);
+
 	return EC_RES_SUCCESS;
 }
 DECLARE_HOST_COMMAND(EC_CMD_GPIO_SET, gpio_command_set, EC_VER_MASK(0));
diff --git a/include/gpio.h b/include/gpio.h
index 0007e07cfa..77923ef4cb 100644
--- a/include/gpio.h
+++ b/include/gpio.h
@@ -8,10 +8,8 @@
 #ifndef __CROS_EC_GPIO_H
 #define __CROS_EC_GPIO_H
 
-#include "board.h"  /* For board-dependent enum gpio_signal list */
 #include "common.h"
 
-
 /* Flag definitions for gpio_info. */
 #define GPIO_INPUT       0x0000  /* Input */
 #define GPIO_OUTPUT      0x0001  /* Output */
@@ -45,9 +43,11 @@ struct gpio_info {
 	int mask;         /* Bitmask on that port (0x01 - 0x80; 0x00 =
 			   * signal not implemented) */
 	uint32_t flags;   /* Flags (GPIO_*) */
-	/* Interrupt handler.  If non-NULL, and the signal's interrupt is
+	/*
+	 * Interrupt handler.  If non-NULL, and the signal's interrupt is
 	 * enabled, this will be called in the context of the GPIO interrupt
-	 * handler. */
+	 * handler.
+	 */
 	void (*irq_handler)(enum gpio_signal signal);
 };
 
@@ -57,12 +57,19 @@ extern const struct gpio_info gpio_list[GPIO_COUNT];
 /* Macro for signals which don't exist */
 #define GPIO_SIGNAL_NOT_IMPLEMENTED(name) {name, LM4_GPIO_A, 0, 0, NULL}
 
+/**
+ * Pre-initializes the module.
+ *
+ * This occurs before clocks or tasks are set up.
+ */
+void gpio_pre_init(void);
 
-/* Pre-initializes the module.  This occurs before clocks or tasks are
- * set up. */
-int gpio_pre_init(void);
-
-/* Get the current value of a signal (0=low, 1=hi). */
+/**
+ * Get the current value of a signal
+ *
+ * @param signal	Signal to get
+ * @return 0 if low, 1 if high.
+ */
 int gpio_get_level(enum gpio_signal signal);
 
 /**
@@ -78,30 +85,53 @@ int gpio_get_level(enum gpio_signal signal);
 uint16_t *gpio_get_level_reg(enum gpio_signal signal, uint32_t *mask);
 
 /**
- * Returns the name of a given GPIO signal.
+ * Return the name of a given GPIO signal.
  *
- * @param signal	Signal to return.
+ * @param signal	Signal to name
  * @returns name of the given signal
  */
 const char *gpio_get_name(enum gpio_signal signal);
 
-/* Set the flags for a signal.  Note that this does not set the signal level
- * based on the presence/absence of GPIO_HIGH; call gpio_set_level() afterwards
- * to do that if needed. */
-int gpio_set_flags(enum gpio_signal signal, int flags);
+/**
+ * Set the flags for a signal.
+ *
+ * Note that this does not set the signal level based on the presence/absence
+ * of GPIO_HIGH; call gpio_set_level() afterwards to do that if needed.
+ *
+ * @param signal	Signal to set flags for
+ * @param flags		New flags for the signal
+ */
+void gpio_set_flags(enum gpio_signal signal, int flags);
 
-/* Set the current value of a signal. */
-int gpio_set_level(enum gpio_signal signal, int value);
+/**
+ * Set the value of a signal.
+ *
+ * @param signal	Signal to set
+ * @param value		New value for signal (0 = low, != high */
+void gpio_set_level(enum gpio_signal signal, int value);
 
-/* Enable interrupts for the signal.  The signal must have been defined with
+/**
+ * Enable interrupts for the signal.
+ *
+ * The signal must have been defined with
  * an interrupt handler.  Normally called by the module which handles the
- * interrupt, once it's ready to start processing interrupts. */
+ * interrupt, once it's ready to start processing interrupts.
+ *
+ * @param signal	Signal to enable interrrupts for
+ * @return EC_SUCCESS, or non-zero if error.
+ */
 int gpio_enable_interrupt(enum gpio_signal signal);
 
-/* Set alternate function <func> for GPIO <port> (LM4_GPIO_*) and <mask>.  If
- * func==0, configures the specified GPIOs for normal GPIO operation.
+/**
+ * Set alternate function for GPIO(s).
  *
- * This is intended for use by other modules' configure_gpio() functions. */
+ * This is intended for use by other modules' configure_gpio() functions.
+ *
+ * @param port		GPIO port to set (LM4_GPIO_*)
+ * @param mask		Bitmask of pins on that port to affect
+ * @param func		Alternate function; if 0, configures the specified
+ *			GPIOs for normal GPIO operation.
+ */
 void gpio_set_alternate_function(int port, int mask, int func);
 
 #endif  /* __CROS_EC_GPIO_H */