Change ec_command() to return negative values for errors

This is more compatible with kernel and u-boot, and will make it
easier to share code between the ec project and those.

BUG=none
TEST=manual:

  ectool version -> should work normally

on ec, do 'hostevent set 0x40000'.  Then at root shell
  ectool queryec
EC returned error result code 19

  ectool flashread 0x100000 16 foo
Reading 16 bytes at offset 1048576...
EC returned error result code 2
Read error at offset 0

Signed-off-by: Randall Spangler <rspangler@chromium.org>

Change-Id: I5e2a85f96c874d0730c14e1438a533649cd594f8
Reviewed-on: https://gerrit.chromium.org/gerrit/26359
Commit-Ready: Randall Spangler <rspangler@chromium.org>
Reviewed-by: Randall Spangler <rspangler@chromium.org>
Tested-by: Randall Spangler <rspangler@chromium.org>

6 files changed, 129 insertions, 101 deletions

diff --git a/util/burn_my_ec.c b/util/burn_my_ec.c
index 56c15af1b9..f7b8273428 100644
--- a/util/burn_my_ec.c
+++ b/util/burn_my_ec.c
@@ -20,18 +20,17 @@
 
 static const char * const part_name[] = {"unknown", "RO", "A", "B"};
 
-enum ec_current_image get_version(void)
+enum ec_current_image get_version(enum ec_current_image *version_ptr)
 {
 	struct ec_response_get_version r;
 	struct ec_response_get_build_info r2;
 	int res;
 
 	res = ec_command(EC_CMD_GET_VERSION, NULL, 0, &r, sizeof(r));
-	if (res)
+	if (res < 0)
 		return res;
-	res = ec_command(EC_CMD_GET_BUILD_INFO,
-			NULL, 0, &r2, sizeof(r2));
-	if (res)
+	res = ec_command(EC_CMD_GET_BUILD_INFO, NULL, 0, &r2, sizeof(r2));
+	if (res < 0)
 		return res;
 
 	/* Ensure versions are null-terminated before we print them */
@@ -49,7 +48,10 @@ enum ec_current_image get_version(void)
 		part_name[r.current_image] : "?"));
 	printf("Build info:    %s\n", r2.build_string);
 
-	return r.current_image;
+	if (version_ptr)
+		*version_ptr = r.current_image;
+
+	return 0;
 }
 
 int flash_partition(enum ec_current_image part, const uint8_t *payload,
@@ -62,9 +64,14 @@ int flash_partition(enum ec_current_image part, const uint8_t *payload,
 	struct ec_response_flash_read rd_resp;
 	int res;
 	uint32_t i;
-	enum ec_current_image current;
+	enum ec_current_image current = EC_IMAGE_UNKNOWN;
+
+	res = get_version(&current);
+	if (res < 0) {
+		fprintf(stderr, "Get version failed : %d\n", res);
+		return -1;
+	}
 
-	current = get_version();
 	if (current == part) {
 		rst_req.cmd = part == EC_IMAGE_RO ?
 			      EC_REBOOT_JUMP_RW_A : EC_REBOOT_JUMP_RO;
@@ -79,7 +86,7 @@ int flash_partition(enum ec_current_image part, const uint8_t *payload,
 	er_req.size = size;
 	er_req.offset = offset;
 	res = ec_command(EC_CMD_FLASH_ERASE, &er_req, sizeof(er_req), NULL, 0);
-	if (res) {
+	if (res < 0) {
 		fprintf(stderr, "Erase failed : %d\n", res);
 		return -1;
 	}
@@ -93,7 +100,7 @@ int flash_partition(enum ec_current_image part, const uint8_t *payload,
 		memcpy(wr_req.data, payload + i, wr_req.size);
 		res = ec_command(EC_CMD_FLASH_WRITE, &wr_req, sizeof(wr_req),
 				 NULL, 0);
-		if (res) {
+		if (res < 0) {
 			fprintf(stderr, "Write error at 0x%08x : %d\n", i, res);
 			return -1;
 		}
@@ -107,7 +114,7 @@ int flash_partition(enum ec_current_image part, const uint8_t *payload,
 		rd_req.size = MIN(size - i, sizeof(rd_resp.data));
 		res = ec_command(EC_CMD_FLASH_READ, &rd_req, sizeof(rd_req),
 				 &rd_resp, sizeof(rd_resp));
-		if (res) {
+		if (res < 0) {
 			fprintf(stderr, "Read error at 0x%08x : %d\n", i, res);
 			return -1;
 		}
@@ -116,9 +123,8 @@ int flash_partition(enum ec_current_image part, const uint8_t *payload,
 				offset + i, offset + i + size);
 	}
 	printf("Done.\n");
-	get_version();
 
-	return 0;
+	return get_version(NULL);
 }
 
 /* black magic to include the EC firmware binary as a payload inside
diff --git a/util/comm-host.h b/util/comm-host.h
index 9647a06218..969e71f4a6 100644
--- a/util/comm-host.h
+++ b/util/comm-host.h
@@ -13,14 +13,18 @@
  * returns 0 in case of success or error code. */
 int comm_init(void);
 
-/* Sends a command to the EC.  Returns the command status code, or
- * -1 if other error. */
+/*
+ * Send a command to the EC.  Returns the length of output data returned (0 if
+ * none), or a negative number if error; errors are -EC_RES_* constants from
+ * ec_commands.h.
+ */
 int ec_command(int command, const void *indata, int insize,
 	       void *outdata, int outsize);
 
-/* Returns the content of the EC information area mapped as "memory".
- *
- * the offsets are defined by the EC_MEMMAP_ constants. */
+/*
+ * Return the content of the EC information area mapped as "memory".
+ * The offsets are defined by the EC_MEMMAP_ constants.
+ */
 uint8_t read_mapped_mem8(uint8_t offset);
 uint16_t read_mapped_mem16(uint8_t offset);
 uint32_t read_mapped_mem32(uint8_t offset);
diff --git a/util/comm-i2c.c b/util/comm-i2c.c
index 63af28b8ce..7e968f035a 100644
--- a/util/comm-i2c.c
+++ b/util/comm-i2c.c
@@ -91,11 +91,11 @@ int ec_command(int command, const void *indata, int insize,
 	struct i2c_msg i2c_msg[2];
 
 	if (i2c_fd < 0)
-		return -1;
+		return -EC_RES_ERROR;
 
 	if (ioctl(i2c_fd, I2C_SLAVE, EC_I2C_ADDR) < 0) {
 		fprintf(stderr, "Cannot set I2C slave address\n");
-		return -1;
+		return -EC_RES_ERROR;
 	}
 
 	i2c_msg[0].addr = EC_I2C_ADDR;
@@ -149,6 +149,7 @@ int ec_command(int command, const void *indata, int insize,
 	if (ret < 0) {
 		fprintf(stderr, "i2c transfer failed: %d (err: %d)\n",
 			ret, errno);
+		ret = -EC_RES_ERROR;
 		goto done;
 	}
 
@@ -157,6 +158,8 @@ int ec_command(int command, const void *indata, int insize,
 	if (ret) {
 		debug("command 0x%02x returned an error %d\n",
 			 command, i2c_msg[1].buf[0]);
+		/* Translate ERROR to -ERROR */
+		ret = -ret;
 	} else if (outsize) {
 		debug("i2c resp  :");
 		/* copy response packet payload and compute checksum */
@@ -169,9 +172,12 @@ int ec_command(int command, const void *indata, int insize,
 
 		if (sum != resp_buf[outsize + 1]) {
 			debug("bad packet checksum\n");
-			ret = -1;
+			ret = -EC_RES_ERROR;
 			goto done;
 		}
+
+		/* Return output buffer size */
+		ret = outsize;
 	}
 done:
 	if (resp_buf)
diff --git a/util/comm-lpc.c b/util/comm-lpc.c
index 4f3aae3912..9e296ba866 100644
--- a/util/comm-lpc.c
+++ b/util/comm-lpc.c
@@ -12,7 +12,6 @@
 #include "comm-host.h"
 #include "ec_commands.h"
 
-
 #define INITIAL_UDELAY 5     /* 5 us */
 #define MAXIMUM_UDELAY 10000 /* 10 ms */
 
@@ -27,8 +26,15 @@ int comm_init(void)
 		return -3;
 	}
 
-	/* Test if the I/O port has been configured for GEC.
-	 * If they all are 0xff, then very possible you cannot access GEC. */
+	/*
+	 * Test if the I/O port has been configured for Chromium EC LPC
+	 * interface.  If all the bytes are 0xff, very likely that Chromium EC
+	 * is not present.
+	 *
+	 * TODO: (crosbug.com/p/10963) Should only need to look at the command
+	 * byte, since we don't support ACPI burst mode and thus bit 4 should
+	 * be 0.
+	 */
 	byte &= inb(EC_LPC_ADDR_USER_CMD);
 	byte &= inb(EC_LPC_ADDR_USER_DATA);
 	for (i = 0; i < EC_PARAM_SIZE && byte == 0xff; ++i)
@@ -38,27 +44,32 @@ int comm_init(void)
 			EC_LPC_ADDR_USER_CMD, EC_LPC_ADDR_USER_DATA,
 			EC_LPC_ADDR_USER_PARAM,
 			EC_LPC_ADDR_USER_PARAM + EC_PARAM_SIZE - 1);
-		fprintf(stderr, "Very likely this board doesn't have GEC.\n");
+		fprintf(stderr,
+			"Very likely this board doesn't have a Chromium EC.\n");
 		return -4;
 	}
 
 	return 0;
 }
 
-
-/* Waits for the EC to be unbusy.  Returns 0 if unbusy, non-zero if
- * timeout. */
+/*
+ * Wait for the EC to be unbusy.  Returns 0 if unbusy, non-zero if
+ * timeout.
+ */
 static int wait_for_ec(int status_addr, int timeout_usec)
 {
 	int i;
 	int delay = INITIAL_UDELAY;
 
 	for (i = 0; i < timeout_usec; i += delay) {
-		/* Delay first, in case we just sent out a command but
-		 * the EC hasn't raise the busy flag. However, I think
-		 * this doesn't happen since the LPC commands are executed
-		 * in order and the busy flag is set by hardware.
-		 * TODO: move this delay after inb(status). */
+		/*
+		 * Delay first, in case we just sent out a command but the EC
+		 * hasn't raise the busy flag. However, I think this doesn't
+		 * happen since the LPC commands are executed in order and the
+		 * busy flag is set by hardware.
+		 *
+		 * TODO: move this delay after inb(status).
+		 */
 		usleep(MIN(delay, timeout_usec - i));
 
 		if (!(inb(status_addr) & EC_LPC_STATUS_BUSY_MASK))
@@ -71,8 +82,6 @@ static int wait_for_ec(int status_addr, int timeout_usec)
 	return -1;  /* Timeout */
 }
 
-/* Sends a command to the EC.  Returns the command status code, or
- * -1 if other error. */
 int ec_command(int command, const void *indata, int insize,
 	       void *outdata, int outsize) {
 	uint8_t *d;
@@ -85,12 +94,12 @@ int ec_command(int command, const void *indata, int insize,
 
 	if (insize > EC_PARAM_SIZE || outsize > EC_PARAM_SIZE) {
 		fprintf(stderr, "Data size too big\n");
-		return -1;
+		return -EC_RES_ERROR;
 	}
 
 	if (wait_for_ec(cmd_addr, 1000000)) {
 		fprintf(stderr, "Timeout waiting for EC ready\n");
-		return -1;
+		return -EC_RES_ERROR;
 	}
 
 	/* Write data, if any */
@@ -102,14 +111,14 @@ int ec_command(int command, const void *indata, int insize,
 
 	if (wait_for_ec(cmd_addr, 1000000)) {
 		fprintf(stderr, "Timeout waiting for EC response\n");
-		return -1;
+		return -EC_RES_ERROR;
 	}
 
 	/* Check result */
 	i = inb(data_addr);
 	if (i) {
 		fprintf(stderr, "EC returned error result code %d\n", i);
-		return i;
+		return -i;
 	}
 
 	/* Read data, if any */
@@ -117,28 +126,28 @@ int ec_command(int command, const void *indata, int insize,
 	for (i = 0, d = (uint8_t *)outdata; i < outsize; i++, d++)
 		*d = inb(param_addr + i);
 
-	return 0;
+	/*
+	 * LPC protocol doesn't have a way to communicate the true output
+	 * size, so assume we got everything we asked for.
+	 */
+	return outsize;
 }
 
-
 uint8_t read_mapped_mem8(uint8_t offset)
 {
 	return inb(EC_LPC_ADDR_MEMMAP + offset);
 }
 
-
 uint16_t read_mapped_mem16(uint8_t offset)
 {
 	return inw(EC_LPC_ADDR_MEMMAP + offset);
 }
 
-
 uint32_t read_mapped_mem32(uint8_t offset)
 {
 	return inl(EC_LPC_ADDR_MEMMAP + offset);
 }
 
-
 int read_mapped_string(uint8_t offset, char *buf)
 {
 	int c;
diff --git a/util/ectool.c b/util/ectool.c
index 4fb953b1fe..c0a9a20857 100644
--- a/util/ectool.c
+++ b/util/ectool.c
@@ -220,7 +220,7 @@ int cmd_hello(int argc, char *argv[])
 	p.in_data = 0xa0b0c0d0;
 
 	rv = ec_command(EC_CMD_HELLO, &p, sizeof(p), &r, sizeof(r));
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	if (r.out_data != 0xa1b2c3d4) {
@@ -242,11 +242,11 @@ int cmd_version(int argc, char *argv[])
 	int rv;
 
 	rv = ec_command(EC_CMD_GET_VERSION, NULL, 0, &r, sizeof(r));
-	if (rv)
+	if (rv < 0)
 		return rv;
 	rv = ec_command(EC_CMD_GET_BUILD_INFO,
 			NULL, 0, &r2, sizeof(r2));
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	/* Ensure versions are null-terminated before we print them */
@@ -304,10 +304,10 @@ int cmd_read_test(int argc, char *argv[])
 		p.size = MIN(size - i, sizeof(r.data));
 		rv = ec_command(EC_CMD_READ_TEST, &p, sizeof(p),
 				&r, sizeof(r));
-		if (rv) {
+		if (rv < 0) {
 			fprintf(stderr, "Read error at offset %d\n", i);
 			free(buf);
-			return -1;
+			return rv;
 		}
 		memcpy(buf + i, r.data, p.size);
 	}
@@ -384,7 +384,7 @@ int cmd_flash_info(int argc, char *argv[])
 	int rv;
 
 	rv = ec_command(EC_CMD_FLASH_INFO, NULL, 0, &r, sizeof(r));
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("FlashSize %d\nWriteSize %d\nEraseSize %d\nProtectSize %d\n",
@@ -434,10 +434,10 @@ int cmd_flash_read(int argc, char *argv[])
 		p.size = MIN(size - i, sizeof(r.data));
 		rv = ec_command(EC_CMD_FLASH_READ,
 				&p, sizeof(p), &r, sizeof(r));
-		if (rv) {
+		if (rv < 0) {
 			fprintf(stderr, "Read error at offset %d\n", i);
 			free(buf);
-			return -1;
+			return rv;
 		}
 		memcpy(buf + i, r.data, p.size);
 	}
@@ -445,7 +445,7 @@ int cmd_flash_read(int argc, char *argv[])
 	rv = write_file(argv[3], buf, size);
 	free(buf);
 	if (rv)
-		return -1;
+		return rv;
 
 	printf("done.\n");
 	return 0;
@@ -484,10 +484,10 @@ int cmd_flash_write(int argc, char *argv[])
 		p.size = MIN(size - i, sizeof(p.data));
 		memcpy(p.data, buf + i, p.size);
 		rv = ec_command(EC_CMD_FLASH_WRITE, &p, sizeof(p), NULL, 0);
-		if (rv) {
+		if (rv < 0) {
 			fprintf(stderr, "Write error at offset %d\n", i);
 			free(buf);
-			return -1;
+			return rv;
 		}
 	}
 
@@ -518,7 +518,7 @@ int cmd_flash_erase(int argc, char *argv[])
 	}
 
 	printf("Erasing %d bytes at offset %d...\n", p.size, p.offset);
-	if (ec_command(EC_CMD_FLASH_ERASE, &p, sizeof(p), NULL, 0))
+	if (ec_command(EC_CMD_FLASH_ERASE, &p, sizeof(p), NULL, 0) < 0)
 		return -1;
 
 	printf("done.\n");
@@ -607,7 +607,7 @@ int cmd_temp_sensor_info(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_TEMP_SENSOR_GET_INFO,
 			&p, sizeof(p), &r, sizeof(r));
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("Sensor name: %s\n", r.sensor_name);
@@ -644,7 +644,7 @@ int cmd_thermal_get_threshold(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_THERMAL_GET_THRESHOLD,
 			&p, sizeof(p), &r, sizeof(r));
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	if (r.value < 0)
@@ -688,9 +688,8 @@ int cmd_thermal_set_threshold(int argc, char *argv[])
 		return -1;
 	}
 
-	rv = ec_command(EC_CMD_THERMAL_SET_THRESHOLD,
-			&p, sizeof(p), NULL, 0);
-	if (rv)
+	rv = ec_command(EC_CMD_THERMAL_SET_THRESHOLD, &p, sizeof(p), NULL, 0);
+	if (rv < 0)
 		return rv;
 
 	printf("Threshold %d for sensor type %d set to %d.\n",
@@ -704,9 +703,8 @@ int cmd_thermal_auto_fan_ctrl(int argc, char *argv[])
 {
 	int rv;
 
-	rv = ec_command(EC_CMD_THERMAL_AUTO_FAN_CTRL,
-			NULL, 0, NULL, 0);
-	if (rv)
+	rv = ec_command(EC_CMD_THERMAL_AUTO_FAN_CTRL, NULL, 0, NULL, 0);
+	if (rv < 0)
 		return rv;
 
 	printf("Automatic fan control is now on.\n");
@@ -748,9 +746,8 @@ int cmd_pwm_set_fan_rpm(int argc, char *argv[])
 		return -1;
 	}
 
-	rv = ec_command(EC_CMD_PWM_SET_FAN_TARGET_RPM,
-			&p, sizeof(p), NULL, 0);
-	if (rv)
+	rv = ec_command(EC_CMD_PWM_SET_FAN_TARGET_RPM, &p, sizeof(p), NULL, 0);
+	if (rv < 0)
 		return rv;
 
 	printf("Fan target RPM set.\n");
@@ -765,7 +762,7 @@ int cmd_pwm_get_keyboard_backlight(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_PWM_GET_KEYBOARD_BACKLIGHT,
 			NULL, 0, &r, sizeof(r));
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	if (r.enabled == 1)
@@ -795,7 +792,7 @@ int cmd_pwm_set_keyboard_backlight(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_PWM_SET_KEYBOARD_BACKLIGHT,
 			&p, sizeof(p), NULL, 0);
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("Keyboard backlight set.\n");
@@ -819,9 +816,8 @@ int cmd_fanduty(int argc, char *argv[])
 		return -1;
 	}
 
-	rv = ec_command(EC_CMD_PWM_SET_FAN_DUTY,
-			&p, sizeof(p), NULL, 0);
-	if (rv)
+	rv = ec_command(EC_CMD_PWM_SET_FAN_DUTY, &p, sizeof(p), NULL, 0);
+	if (rv < 0)
 		return rv;
 
 	printf("Fan duty cycle set.\n");
@@ -1018,7 +1014,7 @@ static int cmd_vboot(int argc, char **argv)
 			       vb_command_paramcount[param.in.cmd].insize,
 			       &param,
 			       vb_command_paramcount[param.in.cmd].outsize);
-		if (r)
+		if (r < 0)
 			return r;
 
 		v = param.out.get_flags.val;
@@ -1072,7 +1068,7 @@ int cmd_usb_charge_set_mode(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_USB_CHARGE_SET_MODE,
 			&p, sizeof(p), NULL, 0);
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("USB charging mode set.\n");
@@ -1113,7 +1109,7 @@ int cmd_kbpress(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_MKBP_SIMULATE_KEY,
 			&p, sizeof(p), NULL, 0);
-	if (rv)
+	if (rv < 0)
 		return rv;
 	printf("Done.\n");
 	return 0;
@@ -1126,7 +1122,7 @@ int cmd_pstore_info(int argc, char *argv[])
 	int rv;
 
 	rv = ec_command(EC_CMD_PSTORE_INFO, NULL, 0, &r, sizeof(r));
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("PstoreSize %d\nAccessSize %d\n", r.pstore_size, r.access_size);
@@ -1173,10 +1169,10 @@ int cmd_pstore_read(int argc, char *argv[])
 		p.size = MIN(size - i, EC_PSTORE_SIZE_MAX);
 		rv = ec_command(EC_CMD_PSTORE_READ,
 				&p, sizeof(p), &r, sizeof(r));
-		if (rv) {
+		if (rv < 0) {
 			fprintf(stderr, "Read error at offset %d\n", i);
 			free(buf);
-			return -1;
+			return rv;
 		}
 		memcpy(buf + i, r.data, p.size);
 	}
@@ -1184,7 +1180,7 @@ int cmd_pstore_read(int argc, char *argv[])
 	rv = write_file(argv[3], buf, size);
 	free(buf);
 	if (rv)
-		return -1;
+		return rv;
 
 	printf("done.\n");
 	return 0;
@@ -1222,12 +1218,11 @@ int cmd_pstore_write(int argc, char *argv[])
 		p.offset = offset + i;
 		p.size = MIN(size - i, EC_PSTORE_SIZE_MAX);
 		memcpy(p.data, buf + i, p.size);
-		rv = ec_command(EC_CMD_PSTORE_WRITE,
-				&p, sizeof(p), NULL, 0);
-		if (rv) {
+		rv = ec_command(EC_CMD_PSTORE_WRITE, &p, sizeof(p), NULL, 0);
+		if (rv < 0) {
 			fprintf(stderr, "Write error at offset %d\n", i);
 			free(buf);
-			return -1;
+			return rv;
 		}
 	}
 
@@ -1242,10 +1237,17 @@ int cmd_acpi_query_ec(int argc, char *argv[])
 	int rv;
 
 	rv = ec_command(EC_CMD_ACPI_QUERY_EVENT, NULL, 0, NULL, 0);
-	if (rv)
+	if (rv < 0) {
+		/*
+		 * ACPI query-event follows different rules for its return
+		 * code; it returns the next pending event instead of an error
+		 * code.  So turn the return code back into a positive number.
+		 */
+		rv = -rv;
 		printf("Got host event %d (mask 0x%08x)\n", rv, 1 << (rv - 1));
-	else
+	} else {
 		printf("No host event pending.\n");
+	}
 	return 0;
 }
 
@@ -1265,7 +1267,7 @@ int cmd_host_event_get_smi_mask(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_HOST_EVENT_GET_SMI_MASK,
 			NULL, 0, &r, sizeof(r));
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("Current host event SMI mask: 0x%08x\n", r.mask);
@@ -1280,7 +1282,7 @@ int cmd_host_event_get_sci_mask(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_HOST_EVENT_GET_SCI_MASK,
 			NULL, 0, &r, sizeof(r));
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("Current host event SCI mask: 0x%08x\n", r.mask);
@@ -1295,7 +1297,7 @@ int cmd_host_event_get_wake_mask(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_HOST_EVENT_GET_WAKE_MASK,
 			NULL, 0, &r, sizeof(r));
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("Current host event wake mask: 0x%08x\n", r.mask);
@@ -1321,7 +1323,7 @@ int cmd_host_event_set_smi_mask(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_HOST_EVENT_SET_SMI_MASK,
 			&p, sizeof(p), NULL, 0);
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("Mask set.\n");
@@ -1347,7 +1349,7 @@ int cmd_host_event_set_sci_mask(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_HOST_EVENT_SET_SCI_MASK,
 			&p, sizeof(p), NULL, 0);
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("Mask set.\n");
@@ -1373,7 +1375,7 @@ int cmd_host_event_set_wake_mask(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_HOST_EVENT_SET_WAKE_MASK,
 			&p, sizeof(p), NULL, 0);
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("Mask set.\n");
@@ -1399,7 +1401,7 @@ int cmd_host_event_clear(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_HOST_EVENT_CLEAR,
 			&p, sizeof(p), NULL, 0);
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("Host events cleared.\n");
@@ -1446,7 +1448,7 @@ int cmd_wireless(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_SWITCH_ENABLE_WIRELESS,
 			&p, sizeof(p), NULL, 0);
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("Success.\n");
@@ -1472,7 +1474,7 @@ int cmd_lcd_backlight(int argc, char *argv[])
 
 	rv = ec_command(EC_CMD_SWITCH_ENABLE_BKLIGHT,
 			&p, sizeof(p), NULL, 0);
-	if (rv)
+	if (rv < 0)
 		return rv;
 
 	printf("Success.\n");
@@ -1556,9 +1558,8 @@ int cmd_chipinfo(int argc, char *argv[])
 
 	printf("Chip info:\n");
 
-	rv = ec_command(EC_CMD_GET_CHIP_INFO,
-			NULL, 0, &info, sizeof(info));
-	if (rv)
+	rv = ec_command(EC_CMD_GET_CHIP_INFO, NULL, 0, &info, sizeof(info));
+	if (rv < 0)
 		return rv;
 	printf("  vendor:    %s\n", info.vendor);
 	printf("  name:      %s\n", info.name);
@@ -1620,7 +1621,8 @@ int cmd_ec_hash(int argc, char *argv[])
 	if (argc < 2) {
 		/* Get hash status */
 		p.cmd = EC_VBOOT_HASH_GET;
-		if (ec_command(EC_CMD_VBOOT_HASH, &p, sizeof(p), &r, sizeof(r)))
+		if (ec_command(EC_CMD_VBOOT_HASH,
+			       &p, sizeof(p), &r, sizeof(r)) < 0)
 			return -1;
 
 		return ec_hash_print(&r);
@@ -1629,7 +1631,8 @@ int cmd_ec_hash(int argc, char *argv[])
 	if (argc == 2 && !strcasecmp(argv[1], "abort")) {
 		/* Abort hash calculation */
 		p.cmd = EC_VBOOT_HASH_ABORT;
-		if (ec_command(EC_CMD_VBOOT_HASH, &p, sizeof(p), &r, sizeof(r)))
+		if (ec_command(EC_CMD_VBOOT_HASH,
+			       &p, sizeof(p), &r, sizeof(r)) < 0)
 			return -1;
 		return 0;
 	}
@@ -1675,7 +1678,7 @@ int cmd_ec_hash(int argc, char *argv[])
 		p.nonce_size = 0;
 
 	printf("Hashing %d bytes at offset %d...\n", p.size, p.offset);
-	if (ec_command(EC_CMD_VBOOT_HASH, &p, sizeof(p), &r, sizeof(r)))
+	if (ec_command(EC_CMD_VBOOT_HASH, &p, sizeof(p), &r, sizeof(r)) < 0)
 		return -1;
 
 	/* Start command doesn't wait for hashing to finish */
diff --git a/util/lbplay.c b/util/lbplay.c
index 71511c581c..8b0c014cfe 100644
--- a/util/lbplay.c
+++ b/util/lbplay.c
@@ -131,7 +131,7 @@ void wait_for_ec_to_stop(void)
 			fprintf(stderr, "EC isn't responding\n");
 			exit(1);
 		}
-	} while (r != 0 && param.out.get_seq.num != LIGHTBAR_STOP);
+	} while (r < 0 && param.out.get_seq.num != LIGHTBAR_STOP);
 }
 
 int main(int argc, char **argv)