common: lightbar: put multiple commands under i2c_lock

If other i2c traffic happens around the time the light bar is
initialized, the i2c bus is wedged.

Instead of waiting 500ms in the motion task loop for the tap sequence to
complete, set under i2c lock the lp power and init sequence.

BRANCH=smaug, samus
BUG=chrome-os-partner:45223
TEST=With msleep(500) removed and With lb_power + lb_init + 100ms, I don't see the wedge at TAP
sequence.

Change-Id: I931eb25bcc5e3237b6c569f2330f72f9fc415964
Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/299543
Reviewed-by: Bill Richardson <wfrichar@google.com>

4 files changed, 63 insertions, 26 deletions

diff --git a/board/samus/power_sequence.c b/board/samus/power_sequence.c
index 83bf1e91f9..a3e40338f4 100644
--- a/board/samus/power_sequence.c
+++ b/board/samus/power_sequence.c
@@ -11,6 +11,8 @@
 #include "common.h"
 #include "console.h"
 #include "extpower.h"
+#include "i2c.h"
+#include "lb_common.h"
 #include "gpio.h"
 #include "hooks.h"
 #include "lid_switch.h"
@@ -317,7 +319,12 @@ enum power_state power_handle_state(enum power_state state)
 		 * available and we won't leak +3VALW through the reset
 		 * line.
 		 */
+		i2c_lock(I2C_PORT_LIGHTBAR, 1);
 		gpio_set_level(GPIO_LIGHTBAR_RESET_L, 1);
+		msleep(1);
+		lb_init(0);
+		msleep(100);
+		i2c_lock(I2C_PORT_LIGHTBAR, 0);
 
 		/*
 		 * Enable touchpad power so it can wake the system from
@@ -544,15 +551,26 @@ int lb_power(int enabled)
 	 * Note, we should delay even if the PP5000 rail was already
 	 * enabled because we can't be sure it's been enabled long
 	 * enough for lightbar IC to respond.
+	 *
+	 * Also, the lightbar do not expect other i2c traffic while
+	 * being power up. Put a lock on the i2c bus.
+	 * see chrome-os-partner:45223.
 	 */
-	if (enabled)
+	if (enabled) {
+		i2c_lock(I2C_PORT_LIGHTBAR, 1);
 		msleep(10);
+	}
 
 	if (enabled != gpio_get_level(GPIO_LIGHTBAR_RESET_L)) {
 		ret = 1;
 		gpio_set_level(GPIO_LIGHTBAR_RESET_L, enabled);
 		msleep(1);
 	}
+	if (enabled) {
+		lb_init(0);
+		msleep(100);
+		i2c_lock(I2C_PORT_LIGHTBAR, 0);
+	}
 
 	return ret;
 }
diff --git a/common/lb_common.c b/common/lb_common.c
index 9a8c4a666e..da6ea95739 100644
--- a/common/lb_common.c
+++ b/common/lb_common.c
@@ -101,6 +101,7 @@
 
 /* Console output macros */
 #define CPUTS(outstr) cputs(CC_LIGHTBAR, outstr)
+#define CPRINTF(format, args...) cprintf(CC_LIGHTBAR, format, ## args)
 #define CPRINTS(format, args...) cprints(CC_LIGHTBAR, format, ## args)
 
 /******************************************************************************/
@@ -114,16 +115,24 @@ static const uint8_t i2c_addr[] = { 0x54, 0x56 };
 
 static inline void controller_write(int ctrl_num, uint8_t reg, uint8_t val)
 {
+	uint8_t buf[2];
+
+	buf[0] = reg;
+	buf[1] = val;
 	ctrl_num = ctrl_num % ARRAY_SIZE(i2c_addr);
-	i2c_write8(I2C_PORT_LIGHTBAR, i2c_addr[ctrl_num], reg, val);
+	i2c_xfer(I2C_PORT_LIGHTBAR, i2c_addr[ctrl_num], buf, 2, 0, 0,
+		 I2C_XFER_SINGLE);
 }
 
 static inline uint8_t controller_read(int ctrl_num, uint8_t reg)
 {
-	int val = 0;
+	uint8_t buf[1];
+	int rv;
+
 	ctrl_num = ctrl_num % ARRAY_SIZE(i2c_addr);
-	i2c_read8(I2C_PORT_LIGHTBAR, i2c_addr[ctrl_num], reg, &val);
-	return val;
+	rv = i2c_xfer(I2C_PORT_LIGHTBAR, i2c_addr[ctrl_num], &reg, 1, buf, 1,
+		      I2C_XFER_SINGLE);
+	return rv ? 0 : buf[0];
 }
 
 /******************************************************************************/
@@ -189,15 +198,6 @@ static const struct initdata_s init_vals[] = {
 	{0x1a, 0x00},				/* current for LED 1 (green) */
 };
 
-static void set_from_array(const struct initdata_s *data, int count)
-{
-	int i;
-	for (i = 0; i < count; i++) {
-		controller_write(0, data[i].reg, data[i].val);
-		controller_write(1, data[i].reg, data[i].val);
-	}
-}
-
 /* Controller register lookup tables. */
 static const uint8_t led_to_ctrl[] = { 1, 1, 0, 0 };
 #ifdef BOARD_BDS
@@ -242,9 +242,11 @@ static void setrgb(int led, int red, int green, int blue)
 	current[led][2] = blue;
 	ctrl = led_to_ctrl[led];
 	bank = led_to_isc[led];
+	i2c_lock(I2C_PORT_LIGHTBAR, 1);
 	controller_write(ctrl, bank, scale(blue, MAX_BLUE));
 	controller_write(ctrl, bank+1, scale(red, MAX_RED));
 	controller_write(ctrl, bank+2, scale(green, MAX_GREEN));
+	i2c_lock(I2C_PORT_LIGHTBAR, 0);
 }
 
 /* LEDs are numbered 0-3, RGB values should be in 0-255.
@@ -289,10 +291,21 @@ uint8_t lb_get_brightness(void)
 }
 
 /* Initialize the controller ICs after reset */
-void lb_init(void)
+void lb_init(int use_lock)
 {
-	CPRINTS("LB_init_vals");
-	set_from_array(init_vals, ARRAY_SIZE(init_vals));
+	int i;
+
+	CPRINTF("[%T LB_init_vals ");
+	for (i = 0; i < ARRAY_SIZE(init_vals); i++) {
+		CPRINTF("%c", '0' + i % 10);
+		if (use_lock)
+			i2c_lock(I2C_PORT_LIGHTBAR, 1);
+		controller_write(0, init_vals[i].reg, init_vals[i].val);
+		controller_write(1, init_vals[i].reg, init_vals[i].val);
+		if (use_lock)
+			i2c_lock(I2C_PORT_LIGHTBAR, 0);
+	}
+	CPRINTF("]\n");
 	memset(current, 0, sizeof(current));
 }
 
@@ -300,16 +313,20 @@ void lb_init(void)
 void lb_off(void)
 {
 	CPRINTS("LB_off");
+	i2c_lock(I2C_PORT_LIGHTBAR, 1);
 	controller_write(0, 0x01, 0x00);
 	controller_write(1, 0x01, 0x00);
+	i2c_lock(I2C_PORT_LIGHTBAR, 0);
 }
 
 /* Come out of standby mode. */
 void lb_on(void)
 {
 	CPRINTS("LB_on");
+	i2c_lock(I2C_PORT_LIGHTBAR, 1);
 	controller_write(0, 0x01, 0x20);
 	controller_write(1, 0x01, 0x20);
+	i2c_lock(I2C_PORT_LIGHTBAR, 0);
 }
 
 static const uint8_t dump_reglist[] = {
@@ -331,13 +348,17 @@ void lb_hc_cmd_dump(struct ec_response_lightbar *out)
 	for (i = 0; i < ARRAY_SIZE(dump_reglist); i++) {
 		reg = dump_reglist[i];
 		out->dump.vals[i].reg = reg;
+		i2c_lock(I2C_PORT_LIGHTBAR, 1);
 		out->dump.vals[i].ic0 = controller_read(0, reg);
 		out->dump.vals[i].ic1 = controller_read(1, reg);
+		i2c_lock(I2C_PORT_LIGHTBAR, 0);
 	}
 }
 
 /* Helper for host command to write controller registers directly */
 void lb_hc_cmd_reg(const struct ec_params_lightbar *in)
 {
+	i2c_lock(I2C_PORT_LIGHTBAR, 1);
 	controller_write(in->reg.ctrl, in->reg.reg, in->reg.value);
+	i2c_lock(I2C_PORT_LIGHTBAR, 0);
 }
diff --git a/common/lightbar.c b/common/lightbar.c
index 27749c5faf..b7f7bcf680 100644
--- a/common/lightbar.c
+++ b/common/lightbar.c
@@ -429,7 +429,7 @@ static uint32_t sequence_S3S0(void)
 	int ci;
 	uint32_t res;
 
-	lb_init();
+	lb_init(1);
 	lb_on();
 	get_battery_level();
 
@@ -623,7 +623,7 @@ static uint32_t sequence_S3(void)
 	int ci;
 
 	lb_off();
-	lb_init();
+	lb_init(1);
 	lb_set_rgb(NUM_LEDS, 0, 0, 0);
 	while (1) {
 		WAIT_OR_RET(st.p.s3_sleep_for);
@@ -669,7 +669,7 @@ static uint32_t sequence_S5S3(void)
 	 * respond. Don't return early, because we still want to initialize the
 	 * lightbar even if another message comes along while we're waiting. */
 	usleep(100);
-	lb_init();
+	lb_init(1);
 	lb_set_rgb(NUM_LEDS, 0, 0, 0);
 	lb_on();
 	/* next sequence */
@@ -732,7 +732,6 @@ static uint32_t sequence_S5(void)
 #ifdef CONFIG_LIGHTBAR_POWER_RAILS
 			/* Request that lightbar power rails be turned on. */
 			if (lb_power(1)) {
-				lb_init();
 				lb_set_rgb(NUM_LEDS, 0, 0, 0);
 			}
 #endif
@@ -788,7 +787,7 @@ static uint32_t sequence_RUN(void)
  */
 static uint32_t sequence_ERROR(void)
 {
-	lb_init();
+	lb_init(1);
 	lb_on();
 
 	lb_set_rgb(0, 255, 255, 255);
@@ -1062,7 +1061,6 @@ static uint32_t sequence_TAP(void)
 #ifdef CONFIG_LIGHTBAR_POWER_RAILS
 	/* Request that the lightbar power rails be turned on. */
 	if (lb_power(1)) {
-		lb_init();
 		lb_set_rgb(NUM_LEDS, 0, 0, 0);
 	}
 #endif
@@ -1667,7 +1665,7 @@ static int lpc_cmd_lightbar(struct host_cmd_handler_args *args)
 		lb_on();
 		break;
 	case LIGHTBAR_CMD_INIT:
-		lb_init();
+		lb_init(1);
 		break;
 	case LIGHTBAR_CMD_SET_BRIGHTNESS:
 		lb_set_brightness(in->set_brightness.num);
@@ -1905,7 +1903,7 @@ static int command_lightbar(int argc, char **argv)
 	}
 
 	if (!strcasecmp(argv[1], "init")) {
-		lb_init();
+		lb_init(1);
 		return EC_SUCCESS;
 	}
 
diff --git a/include/lb_common.h b/include/lb_common.h
index b68540176b..ef357e98ea 100644
--- a/include/lb_common.h
+++ b/include/lb_common.h
@@ -23,7 +23,7 @@ void lb_set_brightness(unsigned int newval);
 /* Get the overall brighness level. */
 uint8_t lb_get_brightness(void);
 /* Initialize the IC controller registers to sane values. */
-void lb_init(void);
+void lb_init(int use_lock);
 /* Disable the LED current off (the IC stays on). */
 void lb_off(void);
 /* Enable the LED current. */