From 68cd6e5b206b21ff5226c806c55fd8c718d5bde5 Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Maneyrol Date: Wed, 7 Jun 2017 13:41:42 +0000 Subject: iio: imu: inv_mpu6050: fix lock issues by using our own mutex There are several locks issues when using buffer and direct polling data at the same time. Use our own mutex for managing locking and block simultaneous use of buffer and direct polling by using iio_device_{claim/release}_direct_mode. This makes chip_config enable bit obsolete, so delete it. Signed-off-by: Jean-Baptiste Maneyrol Signed-off-by: Jonathan Cameron --- drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 147 +++++++++++++++----------- drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 8 +- drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h | 5 +- drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c | 6 +- drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c | 11 +- 5 files changed, 103 insertions(+), 74 deletions(-) (limited to 'drivers/iio/imu/inv_mpu6050') diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c index 96dabbd2f004..9b4bbeea3d4f 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c @@ -187,7 +187,6 @@ int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on) int result = 0; if (power_on) { - /* Already under indio-dev->mlock mutex */ if (!st->powerup_count) result = regmap_write(st->map, st->reg->pwr_mgmt_1, 0); if (!result) @@ -298,50 +297,37 @@ inv_mpu6050_read_raw(struct iio_dev *indio_dev, int result; ret = IIO_VAL_INT; - result = 0; - mutex_lock(&indio_dev->mlock); - if (!st->chip_config.enable) { - result = inv_mpu6050_set_power_itg(st, true); - if (result) - goto error_read_raw; - } - /* when enable is on, power is already on */ + mutex_lock(&st->lock); + result = iio_device_claim_direct_mode(indio_dev); + if (result) + goto error_read_raw_unlock; + result = inv_mpu6050_set_power_itg(st, true); + if (result) + goto error_read_raw_release; switch (chan->type) { case IIO_ANGL_VEL: - if (!st->chip_config.gyro_fifo_enable || - !st->chip_config.enable) { - result = inv_mpu6050_switch_engine(st, true, - INV_MPU6050_BIT_PWR_GYRO_STBY); - if (result) - goto error_read_raw; - } + result = inv_mpu6050_switch_engine(st, true, + INV_MPU6050_BIT_PWR_GYRO_STBY); + if (result) + goto error_read_raw_power_off; ret = inv_mpu6050_sensor_show(st, st->reg->raw_gyro, chan->channel2, val); - if (!st->chip_config.gyro_fifo_enable || - !st->chip_config.enable) { - result = inv_mpu6050_switch_engine(st, false, - INV_MPU6050_BIT_PWR_GYRO_STBY); - if (result) - goto error_read_raw; - } + result = inv_mpu6050_switch_engine(st, false, + INV_MPU6050_BIT_PWR_GYRO_STBY); + if (result) + goto error_read_raw_power_off; break; case IIO_ACCEL: - if (!st->chip_config.accl_fifo_enable || - !st->chip_config.enable) { - result = inv_mpu6050_switch_engine(st, true, - INV_MPU6050_BIT_PWR_ACCL_STBY); - if (result) - goto error_read_raw; - } + result = inv_mpu6050_switch_engine(st, true, + INV_MPU6050_BIT_PWR_ACCL_STBY); + if (result) + goto error_read_raw_power_off; ret = inv_mpu6050_sensor_show(st, st->reg->raw_accl, chan->channel2, val); - if (!st->chip_config.accl_fifo_enable || - !st->chip_config.enable) { - result = inv_mpu6050_switch_engine(st, false, - INV_MPU6050_BIT_PWR_ACCL_STBY); - if (result) - goto error_read_raw; - } + result = inv_mpu6050_switch_engine(st, false, + INV_MPU6050_BIT_PWR_ACCL_STBY); + if (result) + goto error_read_raw_power_off; break; case IIO_TEMP: /* wait for stablization */ @@ -353,10 +339,12 @@ inv_mpu6050_read_raw(struct iio_dev *indio_dev, ret = -EINVAL; break; } -error_read_raw: - if (!st->chip_config.enable) - result |= inv_mpu6050_set_power_itg(st, false); - mutex_unlock(&indio_dev->mlock); +error_read_raw_power_off: + result |= inv_mpu6050_set_power_itg(st, false); +error_read_raw_release: + iio_device_release_direct_mode(indio_dev); +error_read_raw_unlock: + mutex_unlock(&st->lock); if (result) return result; @@ -365,13 +353,17 @@ error_read_raw: case IIO_CHAN_INFO_SCALE: switch (chan->type) { case IIO_ANGL_VEL: + mutex_lock(&st->lock); *val = 0; *val2 = gyro_scale_6050[st->chip_config.fsr]; + mutex_unlock(&st->lock); return IIO_VAL_INT_PLUS_NANO; case IIO_ACCEL: + mutex_lock(&st->lock); *val = 0; *val2 = accel_scale[st->chip_config.accl_fs]; + mutex_unlock(&st->lock); return IIO_VAL_INT_PLUS_MICRO; case IIO_TEMP: @@ -394,12 +386,16 @@ error_read_raw: case IIO_CHAN_INFO_CALIBBIAS: switch (chan->type) { case IIO_ANGL_VEL: + mutex_lock(&st->lock); ret = inv_mpu6050_sensor_show(st, st->reg->gyro_offset, chan->channel2, val); + mutex_unlock(&st->lock); return IIO_VAL_INT; case IIO_ACCEL: + mutex_lock(&st->lock); ret = inv_mpu6050_sensor_show(st, st->reg->accl_offset, chan->channel2, val); + mutex_unlock(&st->lock); return IIO_VAL_INT; default: @@ -475,18 +471,17 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev, struct inv_mpu6050_state *st = iio_priv(indio_dev); int result; - mutex_lock(&indio_dev->mlock); + mutex_lock(&st->lock); /* * we should only update scale when the chip is disabled, i.e. * not running */ - if (st->chip_config.enable) { - result = -EBUSY; - goto error_write_raw; - } + result = iio_device_claim_direct_mode(indio_dev); + if (result) + goto error_write_raw_unlock; result = inv_mpu6050_set_power_itg(st, true); if (result) - goto error_write_raw; + goto error_write_raw_release; switch (mask) { case IIO_CHAN_INFO_SCALE: @@ -522,9 +517,11 @@ static int inv_mpu6050_write_raw(struct iio_dev *indio_dev, break; } -error_write_raw: result |= inv_mpu6050_set_power_itg(st, false); - mutex_unlock(&indio_dev->mlock); +error_write_raw_release: + iio_device_release_direct_mode(indio_dev); +error_write_raw_unlock: + mutex_unlock(&st->lock); return result; } @@ -578,31 +575,35 @@ inv_mpu6050_fifo_rate_store(struct device *dev, struct device_attribute *attr, if (fifo_rate < INV_MPU6050_MIN_FIFO_RATE || fifo_rate > INV_MPU6050_MAX_FIFO_RATE) return -EINVAL; - if (fifo_rate == st->chip_config.fifo_rate) - return count; - mutex_lock(&indio_dev->mlock); - if (st->chip_config.enable) { - result = -EBUSY; - goto fifo_rate_fail; + mutex_lock(&st->lock); + if (fifo_rate == st->chip_config.fifo_rate) { + result = 0; + goto fifo_rate_fail_unlock; } + result = iio_device_claim_direct_mode(indio_dev); + if (result) + goto fifo_rate_fail_unlock; result = inv_mpu6050_set_power_itg(st, true); if (result) - goto fifo_rate_fail; + goto fifo_rate_fail_release; d = INV_MPU6050_ONE_K_HZ / fifo_rate - 1; result = regmap_write(st->map, st->reg->sample_rate_div, d); if (result) - goto fifo_rate_fail; + goto fifo_rate_fail_power_off; st->chip_config.fifo_rate = fifo_rate; result = inv_mpu6050_set_lpf(st, fifo_rate); if (result) - goto fifo_rate_fail; + goto fifo_rate_fail_power_off; -fifo_rate_fail: +fifo_rate_fail_power_off: result |= inv_mpu6050_set_power_itg(st, false); - mutex_unlock(&indio_dev->mlock); +fifo_rate_fail_release: + iio_device_release_direct_mode(indio_dev); +fifo_rate_fail_unlock: + mutex_unlock(&st->lock); if (result) return result; @@ -617,8 +618,13 @@ inv_fifo_rate_show(struct device *dev, struct device_attribute *attr, char *buf) { struct inv_mpu6050_state *st = iio_priv(dev_to_iio_dev(dev)); + unsigned fifo_rate; + + mutex_lock(&st->lock); + fifo_rate = st->chip_config.fifo_rate; + mutex_unlock(&st->lock); - return sprintf(buf, "%d\n", st->chip_config.fifo_rate); + return scnprintf(buf, PAGE_SIZE, "%u\n", fifo_rate); } /** @@ -836,6 +842,7 @@ int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name, return -ENODEV; } st = iio_priv(indio_dev); + mutex_init(&st->lock); st->chip_type = chip_type; st->powerup_count = 0; st->irq = irq; @@ -929,12 +936,26 @@ EXPORT_SYMBOL_GPL(inv_mpu_core_remove); static int inv_mpu_resume(struct device *dev) { - return inv_mpu6050_set_power_itg(iio_priv(dev_get_drvdata(dev)), true); + struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev)); + int result; + + mutex_lock(&st->lock); + result = inv_mpu6050_set_power_itg(st, true); + mutex_unlock(&st->lock); + + return result; } static int inv_mpu_suspend(struct device *dev) { - return inv_mpu6050_set_power_itg(iio_priv(dev_get_drvdata(dev)), false); + struct inv_mpu6050_state *st = iio_priv(dev_get_drvdata(dev)); + int result; + + mutex_lock(&st->lock); + result = inv_mpu6050_set_power_itg(st, false); + mutex_unlock(&st->lock); + + return result; } #endif /* CONFIG_PM_SLEEP */ diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c index 64b5f5b92200..fcd7a92b6cf8 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c @@ -32,7 +32,7 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id) int ret = 0; /* Use the same mutex which was used everywhere to protect power-op */ - mutex_lock(&indio_dev->mlock); + mutex_lock(&st->lock); if (!st->powerup_count) { ret = regmap_write(st->map, st->reg->pwr_mgmt_1, 0); if (ret) @@ -48,7 +48,7 @@ static int inv_mpu6050_select_bypass(struct i2c_mux_core *muxc, u32 chan_id) INV_MPU6050_BIT_BYPASS_EN); } write_error: - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&st->lock); return ret; } @@ -58,14 +58,14 @@ static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id) struct iio_dev *indio_dev = i2c_mux_priv(muxc); struct inv_mpu6050_state *st = iio_priv(indio_dev); - mutex_lock(&indio_dev->mlock); + mutex_lock(&st->lock); /* It doesn't really mattter, if any of the calls fails */ regmap_write(st->map, st->reg->int_pin_cfg, INV_MPU6050_INT_PIN_CFG); st->powerup_count--; if (!st->powerup_count) regmap_write(st->map, st->reg->pwr_mgmt_1, INV_MPU6050_BIT_SLEEP); - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&st->lock); return 0; } diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h index ef13de7a2c20..713fa7b06e08 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_iio.h @@ -14,6 +14,7 @@ #include #include #include +#include #include #include #include @@ -80,7 +81,6 @@ enum inv_devices { * @fsr: Full scale range. * @lpf: Digital low pass filter frequency. * @accl_fs: accel full scale range. - * @enable: master enable state. * @accl_fifo_enable: enable accel data output * @gyro_fifo_enable: enable gyro data output * @fifo_rate: FIFO update rate. @@ -89,7 +89,6 @@ struct inv_mpu6050_chip_config { unsigned int fsr:2; unsigned int lpf:3; unsigned int accl_fs:2; - unsigned int enable:1; unsigned int accl_fifo_enable:1; unsigned int gyro_fifo_enable:1; u16 fifo_rate; @@ -112,6 +111,7 @@ struct inv_mpu6050_hw { /* * struct inv_mpu6050_state - Driver state variables. * @TIMESTAMP_FIFO_SIZE: fifo size for timestamp. + * @lock: Chip access lock. * @trig: IIO trigger. * @chip_config: Cached attribute information. * @reg: Map of important registers. @@ -126,6 +126,7 @@ struct inv_mpu6050_hw { */ struct inv_mpu6050_state { #define TIMESTAMP_FIFO_SIZE 16 + struct mutex lock; struct iio_trigger *trig; struct inv_mpu6050_chip_config chip_config; const struct inv_mpu6050_reg_map *reg; diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c index 3a9f3eac91ab..ff81c6aa009d 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_ring.c @@ -128,7 +128,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) u16 fifo_count; s64 timestamp; - mutex_lock(&indio_dev->mlock); + mutex_lock(&st->lock); if (!(st->chip_config.accl_fifo_enable | st->chip_config.gyro_fifo_enable)) goto end_session; @@ -178,7 +178,7 @@ irqreturn_t inv_mpu6050_read_fifo(int irq, void *p) } end_session: - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&st->lock); iio_trigger_notify_done(indio_dev->trig); return IRQ_HANDLED; @@ -186,7 +186,7 @@ end_session: flush_fifo: /* Flush HW and SW FIFOs. */ inv_reset_fifo(indio_dev); - mutex_unlock(&indio_dev->mlock); + mutex_unlock(&st->lock); iio_trigger_notify_done(indio_dev->trig); return IRQ_HANDLED; diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c index e8818d4dd4b8..540070f0a230 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_trigger.c @@ -90,7 +90,6 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable) if (result) return result; } - st->chip_config.enable = enable; return 0; } @@ -103,7 +102,15 @@ static int inv_mpu6050_set_enable(struct iio_dev *indio_dev, bool enable) static int inv_mpu_data_rdy_trigger_set_state(struct iio_trigger *trig, bool state) { - return inv_mpu6050_set_enable(iio_trigger_get_drvdata(trig), state); + struct iio_dev *indio_dev = iio_trigger_get_drvdata(trig); + struct inv_mpu6050_state *st = iio_priv(indio_dev); + int result; + + mutex_lock(&st->lock); + result = inv_mpu6050_set_enable(indio_dev, state); + mutex_unlock(&st->lock); + + return result; } static const struct iio_trigger_ops inv_mpu_trigger_ops = { -- cgit 1.4.1 From 8a12ed7df7750778808c6feb0a5fd5725d2a8aeb Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Maneyrol Date: Wed, 7 Jun 2017 13:42:26 +0000 Subject: iio: imu: inv_mpu6050: replace sprintf with scnprintf Signed-off-by: Jean-Baptiste Maneyrol Signed-off-by: Jonathan Cameron --- drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'drivers/iio/imu/inv_mpu6050') diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c index 9b4bbeea3d4f..60732e90ccfa 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c @@ -651,7 +651,8 @@ static ssize_t inv_attr_show(struct device *dev, struct device_attribute *attr, case ATTR_ACCL_MATRIX: m = st->plat_data.orientation; - return sprintf(buf, "%d, %d, %d; %d, %d, %d; %d, %d, %d\n", + return scnprintf(buf, PAGE_SIZE, + "%d, %d, %d; %d, %d, %d; %d, %d, %d\n", m[0], m[1], m[2], m[3], m[4], m[5], m[6], m[7], m[8]); default: return -EINVAL; -- cgit 1.4.1 From 881b556f6cf22d37be9340f293f2db10ce7ae8bf Mon Sep 17 00:00:00 2001 From: Jean-Baptiste Maneyrol Date: Tue, 6 Jun 2017 10:29:52 +0000 Subject: iio: imu: inv_mpu6050: test whoami first and against all known values SPI bus is never generating error during transfer, so to check if a chip is correctly connected on a SPI bus we enforce whoami check to be correct. In this way we can assure SPI probe is failing if there is no chip connected. Note that this is really papering over boards that claim the device is there when it isn't. Not a bad thing to cope with, but not necessarily stable material. Fixes: cec0154556f8 ("iio: inv_mpu6050: Check WHO_AM_I register on probe") Signed-off-by: Jean-Baptiste Maneyrol Signed-off-by: Jonathan Cameron --- drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 33 ++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 9 deletions(-) (limited to 'drivers/iio/imu/inv_mpu6050') diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c index 60732e90ccfa..eea4576c2d10 100644 --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c @@ -777,27 +777,42 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st) { int result; unsigned int regval; + int i; st->hw = &hw_info[st->chip_type]; st->reg = hw_info[st->chip_type].reg; - /* reset to make sure previous state are not there */ - result = regmap_write(st->map, st->reg->pwr_mgmt_1, - INV_MPU6050_BIT_H_RESET); - if (result) - return result; - msleep(INV_MPU6050_POWER_UP_TIME); - /* check chip self-identification */ result = regmap_read(st->map, INV_MPU6050_REG_WHOAMI, ®val); if (result) return result; if (regval != st->hw->whoami) { - dev_warn(regmap_get_device(st->map), - "whoami mismatch got %#02x expected %#02hhx for %s\n", + /* check whoami against all possible values */ + for (i = 0; i < INV_NUM_PARTS; ++i) { + if (regval == hw_info[i].whoami) { + dev_warn(regmap_get_device(st->map), + "whoami mismatch got %#02x (%s)" + "expected %#02hhx (%s)\n", + regval, hw_info[i].name, + st->hw->whoami, st->hw->name); + break; + } + } + if (i >= INV_NUM_PARTS) { + dev_err(regmap_get_device(st->map), + "invalid whoami %#02x expected %#02hhx (%s)\n", regval, st->hw->whoami, st->hw->name); + return -ENODEV; + } } + /* reset to make sure previous state are not there */ + result = regmap_write(st->map, st->reg->pwr_mgmt_1, + INV_MPU6050_BIT_H_RESET); + if (result) + return result; + msleep(INV_MPU6050_POWER_UP_TIME); + /* * toggle power state. After reset, the sleep bit could be on * or off depending on the OTP settings. Toggling power would -- cgit 1.4.1