From 083721e0170867a80f357608ffe7be04fc36efe2 Mon Sep 17 00:00:00 2001 From: Alec Berg Date: Tue, 18 Mar 2014 10:16:51 -0700 Subject: accel: Add disable and re-enable accel around critical register writes According to the kionix datasheet, you are supposed to disable the accel before writing to various critical registers. We haven't had a problem thus far, but better to match the datasheet to avoid future problems. BUG=none BRANCH=rambi TEST=ran on glimmer. made sure accels still work using lidangle command. Change-Id: If2d86c39c24c6ba13e118850c5e3f6c0174f4eb7 Signed-off-by: Alec Berg Reviewed-on: https://chromium-review.googlesource.com/190486 Reviewed-by: Randall Spangler --- driver/accel_kxcj9.c | 162 +++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 138 insertions(+), 24 deletions(-) diff --git a/driver/accel_kxcj9.c b/driver/accel_kxcj9.c index a95d2c46b7..fcc7df35b9 100644 --- a/driver/accel_kxcj9.c +++ b/driver/accel_kxcj9.c @@ -17,6 +17,9 @@ #define CPUTS(outstr) cputs(CC_ACCEL, outstr) #define CPRINTF(format, args...) cprintf(CC_ACCEL, format, ## args) +/* Number of times to attempt to enable sensor before giving up. */ +#define SENSOR_ENABLE_ATTEMPTS 3 + /* Range of the accelerometers: 2G, 4G, or 8G. */ static int sensor_range[ACCEL_COUNT] = {KXCJ9_GSEL_2G, KXCJ9_GSEL_2G}; @@ -43,10 +46,72 @@ static int raw_write8(const int addr, const int reg, int data) return i2c_write8(I2C_PORT_ACCEL, addr, reg, data); } +/** + * Disable sensor by taking it out of operating mode. When disabled, the + * acceleration data does not change. + * + * Note: This is intended to be called in a pair with enable_sensor(). + * + * @id Sensor index + * @ctrl1 Pointer to location to store KXCJ9_CTRL1 register after disabling + * + * @return EC_SUCCESS if successful, EC_ERROR_* otherwise + */ +static int disable_sensor(const enum accel_id id, int *ctrl1) +{ + int ret; + + /* + * Read the current state of the ctrl1 register so that we can restore + * it later. + */ + ret = raw_read8(accel_addr[id], KXCJ9_CTRL1, ctrl1); + if (ret != EC_SUCCESS) + return ret; + + /* Disable sensor. */ + *ctrl1 &= ~KXCJ9_CTRL1_PC1; + ret = raw_write8(accel_addr[id], KXCJ9_CTRL1, *ctrl1); + if (ret != EC_SUCCESS) + return ret; + + return EC_SUCCESS; +} + +/** + * Enable sensor by placing it in operating mode. + * + * Note: This is intended to be called in a pair with disable_sensor(). + * + * @id Sensor index + * @ctrl1 Value of KXCJ9_CTRL1 register to write to sensor + * + * @return EC_SUCCESS if successful, EC_ERROR_* otherwise + */ +static int enable_sensor(const enum accel_id id, const int ctrl1) +{ + int i, ret; + + for (i = 0; i < SENSOR_ENABLE_ATTEMPTS; i++) { + /* Enable accelerometer based on ctrl1 value. */ + ret = raw_write8(accel_addr[id], KXCJ9_CTRL1, + ctrl1 | KXCJ9_CTRL1_PC1); + + /* On first success, we are done. */ + if (ret == EC_SUCCESS) + return EC_SUCCESS; + } + + /* Cannot enable accel, print warning and return an error. */ + CPRINTF("[%T Error trying to enable accelerometer %d]\n", id); + + return ret; +} + int accel_write_range(const enum accel_id id, const int range) { - int ret; + int ret, ctrl1; /* Check for valid id. */ if (id < 0 || id >= ACCEL_COUNT) @@ -60,6 +125,17 @@ int accel_write_range(const enum accel_id id, const int range) range != KXCJ9_GSEL_8G) return EC_ERROR_INVAL; + /* + * TODO(crosbug.com/p/26884): This driver currently assumes only one + * task can call this function. If this isn't true anymore, need to + * protect with a mutex. + */ + + /* Disable the sensor to allow for changing of critical parameters. */ + ret = disable_sensor(id, &ctrl1); + if (ret != EC_SUCCESS) + return ret; + ret = raw_write8(accel_addr[id], KXCJ9_CTRL1, KXCJ9_CTRL1_PC1 | sensor_resolution[id] | range); @@ -67,12 +143,16 @@ int accel_write_range(const enum accel_id id, const int range) if (ret == EC_SUCCESS) sensor_range[id] = range; + /* Re-enable the sensor. */ + if (enable_sensor(id, ctrl1) != EC_SUCCESS) + return EC_ERROR_UNKNOWN; + return ret; } int accel_write_resolution(const enum accel_id id, const int res) { - int ret; + int ret, ctrl1; /* Check for valid id. */ if (id < 0 || id >= ACCEL_COUNT) @@ -82,6 +162,17 @@ int accel_write_resolution(const enum accel_id id, const int res) if (res != KXCJ9_RES_12BIT && res != KXCJ9_RES_8BIT) return EC_ERROR_INVAL; + /* + * TODO(crosbug.com/p/26884): This driver currently assumes only one + * task can call this function. If this isn't true anymore, need to + * protect with a mutex. + */ + + /* Disable the sensor to allow for changing of critical parameters. */ + ret = disable_sensor(id, &ctrl1); + if (ret != EC_SUCCESS) + return ret; + ret = raw_write8(accel_addr[id], KXCJ9_CTRL1, KXCJ9_CTRL1_PC1 | res | sensor_range[id]); @@ -89,12 +180,16 @@ int accel_write_resolution(const enum accel_id id, const int res) if (ret == EC_SUCCESS) sensor_resolution[id] = res; + /* Re-enable the sensor. */ + if (enable_sensor(id, ctrl1) != EC_SUCCESS) + return EC_ERROR_UNKNOWN; + return ret; } int accel_write_datarate(const enum accel_id id, const int rate) { - int ret; + int ret, ctrl1; /* Check for valid id. */ if (id < 0 || id >= ACCEL_COUNT) @@ -104,6 +199,17 @@ int accel_write_datarate(const enum accel_id id, const int rate) if (rate < KXCJ9_OSA_12_50HZ || rate > KXCJ9_OSA_6_250HZ) return EC_ERROR_INVAL; + /* + * TODO(crosbug.com/p/26884): This driver currently assumes only one + * task can call this function. If this isn't true anymore, need to + * protect with a mutex. + */ + + /* Disable the sensor to allow for changing of critical parameters. */ + ret = disable_sensor(id, &ctrl1); + if (ret != EC_SUCCESS) + return ret; + /* Set output data rate. */ ret = raw_write8(accel_addr[id], KXCJ9_DATA_CTRL, rate); @@ -111,6 +217,10 @@ int accel_write_datarate(const enum accel_id id, const int rate) if (ret == EC_SUCCESS) sensor_datarate[id] = rate; + /* Re-enable the sensor. */ + if (enable_sensor(id, ctrl1) != EC_SUCCESS) + return EC_ERROR_UNKNOWN; + return ret; } @@ -125,14 +235,8 @@ int accel_set_interrupt(const enum accel_id id, unsigned int threshold) * protect with a mutex. */ - /* - * Read the current status of the control register and disable the - * sensor to allow for changing of critical parameters. - */ - ret = raw_read8(accel_addr[id], KXCJ9_CTRL1, &ctrl1); - if (ret != EC_SUCCESS) - return ret; - ret = raw_write8(accel_addr[id], KXCJ9_CTRL1, ctrl1 & ~KXCJ9_CTRL1_PC1); + /* Disable the sensor to allow for changing of critical parameters. */ + ret = disable_sensor(id, &ctrl1); if (ret != EC_SUCCESS) return ret; @@ -173,13 +277,9 @@ int accel_set_interrupt(const enum accel_id id, unsigned int threshold) ret = raw_read8(accel_addr[id], KXCJ9_INT_REL, &tmp); error_enable_sensor: - /* Re-enable accelerometer. */ - if (raw_write8(accel_addr[id], KXCJ9_CTRL1, ctrl1 | KXCJ9_CTRL1_PC1) != - EC_SUCCESS) { - /* Cannot re-enable accel, print warning and return an error. */ - CPRINTF("[%T Error trying to enable accelerometer %d]\n", id); + /* Re-enable the sensor. */ + if (enable_sensor(id, ctrl1) != EC_SUCCESS) return EC_ERROR_UNKNOWN; - } return ret; } @@ -246,6 +346,17 @@ int accel_init(enum accel_id id) if (id < 0 || id >= ACCEL_COUNT) return EC_ERROR_INVAL; + /* + * TODO(crosbug.com/p/26884): This driver currently assumes only one + * task can call this function. If this isn't true anymore, need to + * protect with a mutex. + */ + + /* Disable the sensor to allow for changing of critical parameters. */ + ret = disable_sensor(id, &ctrl1); + if (ret != EC_SUCCESS) + return ret; + /* * This sensor can be powered through an EC reboot, so the state of * the sensor is unknown here. Initiate software reset to restore @@ -271,6 +382,14 @@ int accel_init(enum accel_id id) msleep(10); } + /* Set resolution and range. */ + ctrl1 = sensor_resolution[id] | sensor_range[id]; +#ifdef CONFIG_ACCEL_INTERRUPTS + /* Enable wake up (motion detect) functionality. */ + ctrl1 |= KXCJ9_CTRL1_WUFE; +#endif + ret = raw_write8(accel_addr[id], KXCJ9_CTRL1, ctrl1); + #ifdef CONFIG_ACCEL_INTERRUPTS /* Set interrupt polarity to rising edge and keep interrupt disabled. */ ret |= raw_write8(accel_addr[id], KXCJ9_INT_CTRL1, KXCJ9_INT_CTRL1_IEA); @@ -295,13 +414,8 @@ int accel_init(enum accel_id id) /* Set output data rate. */ ret |= raw_write8(accel_addr[id], KXCJ9_DATA_CTRL, sensor_datarate[id]); - /* Set resolution and range and enable sensor. */ - ctrl1 = sensor_resolution[id] | sensor_range[id] | KXCJ9_CTRL1_PC1; -#ifdef CONFIG_ACCEL_INTERRUPTS - /* Enable wake up (motion detect) functionality. */ - ctrl1 |= KXCJ9_CTRL1_WUFE; -#endif - ret = raw_write8(accel_addr[id], KXCJ9_CTRL1, ctrl1); + /* Enable the sensor. */ + ret |= enable_sensor(id, ctrl1); return ret; } -- cgit v1.2.1