[PATCH v4 1/2] iio: imu: inv_mpu6050: clean set_power_itg and fix usage

[PATCH v4 1/2] iio: imu: inv_mpu6050: clean set_power_itg and fix usage

[Jean-Baptiste Maneyrol]

Rewrite set_power_itg. Failing when turning power off is
no more decreasing the counter now and sleeping only happens
when effectively turning the chip on. Fix also usage in init
function (setting power on one time is sufficient to ensure
chip is effectively on).

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 44 ++++++++++++++++--------------
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
index 20b94d9..f3cf327 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
@@ -191,26 +191,29 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
 
 int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on)
 {
-	int result = 0;
+	int result;
 
 	if (power_on) {
-		if (!st->powerup_count)
+		if (!st->powerup_count) {
 			result = regmap_write(st->map, st->reg->pwr_mgmt_1, 0);
-		if (!result)
-			st->powerup_count++;
+			if (result)
+				return result;
+			usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
+				     INV_MPU6050_REG_UP_TIME_MAX);
+		}
+		st->powerup_count++;
 	} else {
-		st->powerup_count--;
-		if (!st->powerup_count)
+		if (st->powerup_count == 1) {
 			result = regmap_write(st->map, st->reg->pwr_mgmt_1,
 					      INV_MPU6050_BIT_SLEEP);
+			if (result)
+				return result;
+		}
+		st->powerup_count--;
 	}
 
-	if (result)
-		return result;
-
-	if (power_on)
-		usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
-			     INV_MPU6050_REG_UP_TIME_MAX);
+	dev_dbg(regmap_get_device(st->map), "set power %d, count=%u\n",
+		power_on, st->powerup_count);
 
 	return 0;
 }
@@ -856,14 +859,11 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 	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
+	 * Turn power on. After reset, the sleep bit could be on
+	 * or off depending on the OTP settings. Turning power on
 	 * make it in a definite state as well as making the hardware
 	 * state align with the software state
 	 */
-	result = inv_mpu6050_set_power_itg(st, false);
-	if (result)
-		return result;
 	result = inv_mpu6050_set_power_itg(st, true);
 	if (result)
 		return result;
@@ -871,13 +871,17 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
 	result = inv_mpu6050_switch_engine(st, false,
 					   INV_MPU6050_BIT_PWR_ACCL_STBY);
 	if (result)
-		return result;
+		goto error_power_off;
 	result = inv_mpu6050_switch_engine(st, false,
 					   INV_MPU6050_BIT_PWR_GYRO_STBY);
 	if (result)
-		return result;
+		goto error_power_off;
 
-	return 0;
+	return inv_mpu6050_set_power_itg(st, false);
+
+error_power_off:
+	inv_mpu6050_set_power_itg(st, false);
+	return result;
 }
 
 int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,
-- 
2.7.4

[PATCH v4 2/2] iio: imu: inv_mpu6050: use set_power_itg function in i2c mux

[Jean-Baptiste Maneyrol]

Set power function is rewritten manually inside i2c mux select.
Better use the already identical existing function.

Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
---
 drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 33 ++++++++++++-------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
index f70e7b9..eca34b9 100644
--- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
+++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
@@ -29,25 +29,19 @@ static int inv_mpu6050_select_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);
-	int ret = 0;
+	int ret;
 
-	/* Use the same mutex which was used everywhere to protect power-op */
 	mutex_lock(&st->lock);
-	if (!st->powerup_count) {
-		ret = regmap_write(st->map, st->reg->pwr_mgmt_1, 0);
-		if (ret)
-			goto write_error;
 
-		usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
-			     INV_MPU6050_REG_UP_TIME_MAX);
-	}
-	if (!ret) {
-		st->powerup_count++;
-		ret = regmap_write(st->map, st->reg->int_pin_cfg,
-				   INV_MPU6050_INT_PIN_CFG |
-				   INV_MPU6050_BIT_BYPASS_EN);
-	}
-write_error:
+	ret = inv_mpu6050_set_power_itg(st, true);
+	if (ret)
+		goto error_unlock;
+
+	ret = regmap_write(st->map, st->reg->int_pin_cfg,
+			   INV_MPU6050_INT_PIN_CFG |
+			   INV_MPU6050_BIT_BYPASS_EN);
+
+error_unlock:
 	mutex_unlock(&st->lock);
 
 	return ret;
@@ -59,12 +53,11 @@ static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
 	struct inv_mpu6050_state *st = iio_priv(indio_dev);
 
 	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);
+	inv_mpu6050_set_power_itg(st, false);
+
 	mutex_unlock(&st->lock);
 
 	return 0;
-- 
2.7.4

Re: [PATCH v4 1/2] iio: imu: inv_mpu6050: clean set_power_itg and fix usage

[Jonathan Cameron]

On Tue, 17 Apr 2018 09:19:39 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> Rewrite set_power_itg. Failing when turning power off is
> no more decreasing the counter now and sleeping only happens
> when effectively turning the chip on. Fix also usage in init
> function (setting power on one time is sufficient to ensure
> chip is effectively on).
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
This is a bit of a mixture of a fix and some less than minimal (worthwhile)
cleanup, so I'll take this the slow way rather than pushing it as a fix.

Applied to the togreg branch of iio.git and pushed out as testing
for the autobuilders to play with it.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_core.c | 44 ++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> index 20b94d9..f3cf327 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_core.c
> @@ -191,26 +191,29 @@ int inv_mpu6050_switch_engine(struct inv_mpu6050_state *st, bool en, u32 mask)
>  
>  int inv_mpu6050_set_power_itg(struct inv_mpu6050_state *st, bool power_on)
>  {
> -	int result = 0;
> +	int result;
>  
>  	if (power_on) {
> -		if (!st->powerup_count)
> +		if (!st->powerup_count) {
>  			result = regmap_write(st->map, st->reg->pwr_mgmt_1, 0);
> -		if (!result)
> -			st->powerup_count++;
> +			if (result)
> +				return result;
> +			usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
> +				     INV_MPU6050_REG_UP_TIME_MAX);
> +		}
> +		st->powerup_count++;
>  	} else {
> -		st->powerup_count--;
> -		if (!st->powerup_count)
> +		if (st->powerup_count == 1) {
>  			result = regmap_write(st->map, st->reg->pwr_mgmt_1,
>  					      INV_MPU6050_BIT_SLEEP);
> +			if (result)
> +				return result;
> +		}
> +		st->powerup_count--;
>  	}
>  
> -	if (result)
> -		return result;
> -
> -	if (power_on)
> -		usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
> -			     INV_MPU6050_REG_UP_TIME_MAX);
> +	dev_dbg(regmap_get_device(st->map), "set power %d, count=%u\n",
> +		power_on, st->powerup_count);
>  
>  	return 0;
>  }
> @@ -856,14 +859,11 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
>  	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
> +	 * Turn power on. After reset, the sleep bit could be on
> +	 * or off depending on the OTP settings. Turning power on
>  	 * make it in a definite state as well as making the hardware
>  	 * state align with the software state
>  	 */
> -	result = inv_mpu6050_set_power_itg(st, false);
> -	if (result)
> -		return result;
>  	result = inv_mpu6050_set_power_itg(st, true);
>  	if (result)
>  		return result;
> @@ -871,13 +871,17 @@ static int inv_check_and_setup_chip(struct inv_mpu6050_state *st)
>  	result = inv_mpu6050_switch_engine(st, false,
>  					   INV_MPU6050_BIT_PWR_ACCL_STBY);
>  	if (result)
> -		return result;
> +		goto error_power_off;
>  	result = inv_mpu6050_switch_engine(st, false,
>  					   INV_MPU6050_BIT_PWR_GYRO_STBY);
>  	if (result)
> -		return result;
> +		goto error_power_off;
>  
> -	return 0;
> +	return inv_mpu6050_set_power_itg(st, false);
> +
> +error_power_off:
> +	inv_mpu6050_set_power_itg(st, false);
> +	return result;
>  }
>  
>  int inv_mpu_core_probe(struct regmap *regmap, int irq, const char *name,

Re: [PATCH v4 2/2] iio: imu: inv_mpu6050: use set_power_itg function in i2c mux

[Jonathan Cameron]

On Tue, 17 Apr 2018 09:19:40 +0200
Jean-Baptiste Maneyrol <jmaneyrol@invensense.com> wrote:

> Set power function is rewritten manually inside i2c mux select.
> Better use the already identical existing function.
> 
> Signed-off-by: Jean-Baptiste Maneyrol <jmaneyrol@invensense.com>
Applied to the togreg branch of iio.git and pushed out as testing for
the autobuilders to paly with it.

Thanks,

Jonathan

> ---
>  drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c | 33 ++++++++++++-------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> index f70e7b9..eca34b9 100644
> --- a/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> +++ b/drivers/iio/imu/inv_mpu6050/inv_mpu_i2c.c
> @@ -29,25 +29,19 @@ static int inv_mpu6050_select_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);
> -	int ret = 0;
> +	int ret;
>  
> -	/* Use the same mutex which was used everywhere to protect power-op */
>  	mutex_lock(&st->lock);
> -	if (!st->powerup_count) {
> -		ret = regmap_write(st->map, st->reg->pwr_mgmt_1, 0);
> -		if (ret)
> -			goto write_error;
>  
> -		usleep_range(INV_MPU6050_REG_UP_TIME_MIN,
> -			     INV_MPU6050_REG_UP_TIME_MAX);
> -	}
> -	if (!ret) {
> -		st->powerup_count++;
> -		ret = regmap_write(st->map, st->reg->int_pin_cfg,
> -				   INV_MPU6050_INT_PIN_CFG |
> -				   INV_MPU6050_BIT_BYPASS_EN);
> -	}
> -write_error:
> +	ret = inv_mpu6050_set_power_itg(st, true);
> +	if (ret)
> +		goto error_unlock;
> +
> +	ret = regmap_write(st->map, st->reg->int_pin_cfg,
> +			   INV_MPU6050_INT_PIN_CFG |
> +			   INV_MPU6050_BIT_BYPASS_EN);
> +
> +error_unlock:
>  	mutex_unlock(&st->lock);
>  
>  	return ret;
> @@ -59,12 +53,11 @@ static int inv_mpu6050_deselect_bypass(struct i2c_mux_core *muxc, u32 chan_id)
>  	struct inv_mpu6050_state *st = iio_priv(indio_dev);
>  
>  	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);
> +	inv_mpu6050_set_power_itg(st, false);
> +
>  	mutex_unlock(&st->lock);
>  
>  	return 0;