[PATCH 0/7] Fix regulators and PM for AK8975 v3

[PATCH 0/7] Fix regulators and PM for AK8975 v3

[Linus Walleij]

This series finally has been rebased and tested on top of
Jonathan's development tree, also rebasing on top of Leonards
patch for AK8975 which was in there, and test-compiled the
result.

Linus Walleij (6):
  iio: magn: ak8975: fix regulator usage
  iio: magn: ak8975: add Vid regulator
  iio: magn: ak8975: refactor regulator handlers
  iio: magn: ak8975: allow a delay after enabling regulators
  iio: magn: ak8975: make sure to power down at remove()
  iio: magn: ak8975: deploy runtime and system PM

 drivers/iio/magnetometer/ak8975.c | 151 ++++++++++++++++++++++++++++++--------
 1 file changed, 120 insertions(+), 31 deletions(-)

-- 
2.4.11

[PATCH 1/6 v3] iio: magn: ak8975: fix regulator usage

[Linus Walleij]

IS_ERR_OR_NULL() should never be used with regulators because
a NULL pointer may be a perfectly valid dummy regulator

We should always succeed to fetch and enable a regulator, but
it may be a dummy. That is fine, so bail out for any real
errors or probe deferrals

Include the error code in the warning print so we know what
kind of problem we're dealing with (for example it is nice to
see if it is a probe deferral).

As we will bail out of probe if the regulator is erroneous,
just issue regulator_disable() on the poweroff path: it will
succeed.

Cc: Mark Brown <broonie@kernel.org>
Cc: Lars-Peter Clausen Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Fix the unrelated chunk removing the Vid regulator that
  the next patch adds. Was basing development on a bad tree
  and not looking close enough.
ChangeLog v1->v2:
- No changes
---
 drivers/iio/magnetometer/ak8975.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 57d3654d7caf..7f51adba7bad 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -389,17 +389,16 @@ static int ak8975_power_on(struct i2c_client *client)
 	int ret;
 
 	data->vdd = devm_regulator_get(&client->dev, "vdd");
-	if (IS_ERR_OR_NULL(data->vdd)) {
+	if (IS_ERR(data->vdd)) {
 		ret = PTR_ERR(data->vdd);
-		if (ret == -ENODEV)
-			ret = 0;
 	} else {
 		ret = regulator_enable(data->vdd);
 	}
-
-	if (ret)
-		dev_err(&client->dev, "failed to enable Vdd supply: %d\n", ret);
-	return ret;
+	if (ret) {
+		dev_warn(&client->dev,
+			 "Failed to enable specified Vdd supply\n");
+		return ret;
+	}
 }
 
 /* Disable attached power regulator if any. */
@@ -408,8 +407,7 @@ static void ak8975_power_off(const struct i2c_client *client)
 	const struct iio_dev *indio_dev = i2c_get_clientdata(client);
 	const struct ak8975_data *data = iio_priv(indio_dev);
 
-	if (!IS_ERR_OR_NULL(data->vdd))
-		regulator_disable(data->vdd);
+	regulator_disable(data->vdd);
 }
 
 /*
-- 
2.4.11

Re: [PATCH 1/6 v3] iio: magn: ak8975: fix regulator usage

[Jonathan Cameron]

On 29/06/16 13:08, Linus Walleij wrote:
> IS_ERR_OR_NULL() should never be used with regulators because
> a NULL pointer may be a perfectly valid dummy regulator
> 
> We should always succeed to fetch and enable a regulator, but
> it may be a dummy. That is fine, so bail out for any real
> errors or probe deferrals
> 
> Include the error code in the warning print so we know what
> kind of problem we're dealing with (for example it is nice to
> see if it is a probe deferral).
> 
> As we will bail out of probe if the regulator is erroneous,
> just issue regulator_disable() on the poweroff path: it will
> succeed.
> 
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Lars-Peter Clausen Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Applied to the togreg branch of iio.git - initially pushed out
as testing for the autobuilders to play with it.

Thanks,

Jonathan
> ---
> ChangeLog v2->v3:
> - Fix the unrelated chunk removing the Vid regulator that
>   the next patch adds. Was basing development on a bad tree
>   and not looking close enough.
> ChangeLog v1->v2:
> - No changes
> ---
>  drivers/iio/magnetometer/ak8975.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 57d3654d7caf..7f51adba7bad 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -389,17 +389,16 @@ static int ak8975_power_on(struct i2c_client *client)
>  	int ret;
>  
>  	data->vdd = devm_regulator_get(&client->dev, "vdd");
> -	if (IS_ERR_OR_NULL(data->vdd)) {
> +	if (IS_ERR(data->vdd)) {
>  		ret = PTR_ERR(data->vdd);
> -		if (ret == -ENODEV)
> -			ret = 0;
>  	} else {
>  		ret = regulator_enable(data->vdd);
>  	}
> -
> -	if (ret)
> -		dev_err(&client->dev, "failed to enable Vdd supply: %d\n", ret);
> -	return ret;
> +	if (ret) {
> +		dev_warn(&client->dev,
> +			 "Failed to enable specified Vdd supply\n");
> +		return ret;
> +	}
>  }
>  
>  /* Disable attached power regulator if any. */
> @@ -408,8 +407,7 @@ static void ak8975_power_off(const struct i2c_client *client)
>  	const struct iio_dev *indio_dev = i2c_get_clientdata(client);
>  	const struct ak8975_data *data = iio_priv(indio_dev);
>  
> -	if (!IS_ERR_OR_NULL(data->vdd))
> -		regulator_disable(data->vdd);
> +	regulator_disable(data->vdd);
>  }
>  
>  /*
> 

[PATCH 2/6 v3] iio: magn: ak8975: add Vid regulator

[Linus Walleij]

The AK8975 has two power sources: Vdd (analog voltage supply)
and Vid (digital voltage supply). Optionally also obtain the Vid
supply regulator and enable it.

If an error occurs when enabling one of the regulators: bail out.

Cc: Gregor Boirie <gregor.boirie@parrot.com>
Cc: Richard Leitner <dev@g0hl1n.net>
Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- No changes. Numbered v2 to be part of the patch set.
- Rebase.
ChangeLog v1->v2:
- Just obtain the other regulator exactly like the first without
  trying to be fancy.
- Rebase and rewrite on top of the patch fixing up the regulator
  usage.
---
 drivers/iio/magnetometer/ak8975.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 7f51adba7bad..af3395a0fb16 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -379,6 +379,7 @@ struct ak8975_data {
 	u8			cntl_cache;
 	struct iio_mount_matrix orientation;
 	struct regulator	*vdd;
+	struct regulator	*vid;
 };
 
 /* Enable attached power regulator if any. */
@@ -399,6 +400,19 @@ static int ak8975_power_on(struct i2c_client *client)
 			 "Failed to enable specified Vdd supply\n");
 		return ret;
 	}
+
+	data->vid = devm_regulator_get(&client->dev, "vid");
+	if (IS_ERR(data->vid)) {
+		ret = PTR_ERR(data->vid);
+	} else {
+		ret = regulator_enable(data->vid);
+	}
+	if (ret) {
+		dev_warn(&client->dev,
+			 "Failed to enable specified Vid supply\n");
+		return ret;
+	}
+	return 0;
 }
 
 /* Disable attached power regulator if any. */
@@ -407,6 +421,7 @@ static void ak8975_power_off(const struct i2c_client *client)
 	const struct iio_dev *indio_dev = i2c_get_clientdata(client);
 	const struct ak8975_data *data = iio_priv(indio_dev);
 
+	regulator_disable(data->vid);
 	regulator_disable(data->vdd);
 }
 
-- 
2.4.11

Re: [PATCH 2/6 v3] iio: magn: ak8975: add Vid regulator

[Jonathan Cameron]

On 29/06/16 13:08, Linus Walleij wrote:
> The AK8975 has two power sources: Vdd (analog voltage supply)
> and Vid (digital voltage supply). Optionally also obtain the Vid
> supply regulator and enable it.
> 
> If an error occurs when enabling one of the regulators: bail out.
> 
> Cc: Gregor Boirie <gregor.boirie@parrot.com>
> Cc: Richard Leitner <dev@g0hl1n.net>
> Cc: Krzysztof Kozlowski <k.kozlowski@samsung.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Applied. Thanks
> ---
> ChangeLog v2->v3:
> - No changes. Numbered v2 to be part of the patch set.
> - Rebase.
> ChangeLog v1->v2:
> - Just obtain the other regulator exactly like the first without
>   trying to be fancy.
> - Rebase and rewrite on top of the patch fixing up the regulator
>   usage.
> ---
>  drivers/iio/magnetometer/ak8975.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 7f51adba7bad..af3395a0fb16 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -379,6 +379,7 @@ struct ak8975_data {
>  	u8			cntl_cache;
>  	struct iio_mount_matrix orientation;
>  	struct regulator	*vdd;
> +	struct regulator	*vid;
>  };
>  
>  /* Enable attached power regulator if any. */
> @@ -399,6 +400,19 @@ static int ak8975_power_on(struct i2c_client *client)
>  			 "Failed to enable specified Vdd supply\n");
>  		return ret;
>  	}
> +
> +	data->vid = devm_regulator_get(&client->dev, "vid");
> +	if (IS_ERR(data->vid)) {
> +		ret = PTR_ERR(data->vid);
> +	} else {
> +		ret = regulator_enable(data->vid);
> +	}
> +	if (ret) {
> +		dev_warn(&client->dev,
> +			 "Failed to enable specified Vid supply\n");
> +		return ret;
> +	}
> +	return 0;
>  }
>  
>  /* Disable attached power regulator if any. */
> @@ -407,6 +421,7 @@ static void ak8975_power_off(const struct i2c_client *client)
>  	const struct iio_dev *indio_dev = i2c_get_clientdata(client);
>  	const struct ak8975_data *data = iio_priv(indio_dev);
>  
> +	regulator_disable(data->vid);
>  	regulator_disable(data->vdd);
>  }
>  
> 

[PATCH 3/6 v3] iio: magn: ak8975: refactor regulator handlers

[Linus Walleij]

Move the regulator_get() calls directly into the probe() function,
keep only the power_on()/power_off() functions to flick the
regulators on/off.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Rebase
ChangeLog v1->v2:
- Fix missed conversions that happened to end up in a later
  patch in the series, mea culpa.
---
 drivers/iio/magnetometer/ak8975.c | 43 ++++++++++++++++-----------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index af3395a0fb16..f72f51d1f3f5 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -383,32 +383,19 @@ struct ak8975_data {
 };
 
 /* Enable attached power regulator if any. */
-static int ak8975_power_on(struct i2c_client *client)
+static int ak8975_power_on(const struct ak8975_data *data)
 {
-	const struct iio_dev *indio_dev = i2c_get_clientdata(client);
-	struct ak8975_data *data = iio_priv(indio_dev);
 	int ret;
 
-	data->vdd = devm_regulator_get(&client->dev, "vdd");
-	if (IS_ERR(data->vdd)) {
-		ret = PTR_ERR(data->vdd);
-	} else {
-		ret = regulator_enable(data->vdd);
-	}
+	ret = regulator_enable(data->vdd);
 	if (ret) {
-		dev_warn(&client->dev,
+		dev_warn(&data->client->dev,
 			 "Failed to enable specified Vdd supply\n");
 		return ret;
 	}
-
-	data->vid = devm_regulator_get(&client->dev, "vid");
-	if (IS_ERR(data->vid)) {
-		ret = PTR_ERR(data->vid);
-	} else {
-		ret = regulator_enable(data->vid);
-	}
+	ret = regulator_enable(data->vid);
 	if (ret) {
-		dev_warn(&client->dev,
+		dev_warn(&data->client->dev,
 			 "Failed to enable specified Vid supply\n");
 		return ret;
 	}
@@ -416,11 +403,8 @@ static int ak8975_power_on(struct i2c_client *client)
 }
 
 /* Disable attached power regulator if any. */
-static void ak8975_power_off(const struct i2c_client *client)
+static void ak8975_power_off(const struct ak8975_data *data)
 {
-	const struct iio_dev *indio_dev = i2c_get_clientdata(client);
-	const struct ak8975_data *data = iio_priv(indio_dev);
-
 	regulator_disable(data->vid);
 	regulator_disable(data->vdd);
 }
@@ -936,7 +920,15 @@ static int ak8975_probe(struct i2c_client *client,
 
 	data->def = &ak_def_array[chipset];
 
-	err = ak8975_power_on(client);
+	/* Fetch the regulators */
+	data->vdd = devm_regulator_get(&client->dev, "vdd");
+	if (IS_ERR(data->vdd))
+		return PTR_ERR(data->vdd);
+	data->vid = devm_regulator_get(&client->dev, "vid");
+	if (IS_ERR(data->vid))
+		return PTR_ERR(data->vid);
+
+	err = ak8975_power_on(data);
 	if (err)
 		return err;
 
@@ -981,17 +973,18 @@ static int ak8975_probe(struct i2c_client *client,
 cleanup_buffer:
 	iio_triggered_buffer_cleanup(indio_dev);
 power_off:
-	ak8975_power_off(client);
+	ak8975_power_off(data);
 	return err;
 }
 
 static int ak8975_remove(struct i2c_client *client)
 {
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct ak8975_data *data = iio_priv(indio_dev);
 
 	iio_device_unregister(indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
-	ak8975_power_off(client);
+	ak8975_power_off(data);
 
 	return 0;
 }
-- 
2.4.11

Re: [PATCH 3/6 v3] iio: magn: ak8975: refactor regulator handlers

[Jonathan Cameron]

On 29/06/16 13:08, Linus Walleij wrote:
> Move the regulator_get() calls directly into the probe() function,
> keep only the power_on()/power_off() functions to flick the
> regulators on/off.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Applied. Thanks
> ---
> ChangeLog v2->v3:
> - Rebase
> ChangeLog v1->v2:
> - Fix missed conversions that happened to end up in a later
>   patch in the series, mea culpa.
> ---
>  drivers/iio/magnetometer/ak8975.c | 43 ++++++++++++++++-----------------------
>  1 file changed, 18 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index af3395a0fb16..f72f51d1f3f5 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -383,32 +383,19 @@ struct ak8975_data {
>  };
>  
>  /* Enable attached power regulator if any. */
> -static int ak8975_power_on(struct i2c_client *client)
> +static int ak8975_power_on(const struct ak8975_data *data)
>  {
> -	const struct iio_dev *indio_dev = i2c_get_clientdata(client);
> -	struct ak8975_data *data = iio_priv(indio_dev);
>  	int ret;
>  
> -	data->vdd = devm_regulator_get(&client->dev, "vdd");
> -	if (IS_ERR(data->vdd)) {
> -		ret = PTR_ERR(data->vdd);
> -	} else {
> -		ret = regulator_enable(data->vdd);
> -	}
> +	ret = regulator_enable(data->vdd);
>  	if (ret) {
> -		dev_warn(&client->dev,
> +		dev_warn(&data->client->dev,
>  			 "Failed to enable specified Vdd supply\n");
>  		return ret;
>  	}
> -
> -	data->vid = devm_regulator_get(&client->dev, "vid");
> -	if (IS_ERR(data->vid)) {
> -		ret = PTR_ERR(data->vid);
> -	} else {
> -		ret = regulator_enable(data->vid);
> -	}
> +	ret = regulator_enable(data->vid);
>  	if (ret) {
> -		dev_warn(&client->dev,
> +		dev_warn(&data->client->dev,
>  			 "Failed to enable specified Vid supply\n");
>  		return ret;
>  	}
> @@ -416,11 +403,8 @@ static int ak8975_power_on(struct i2c_client *client)
>  }
>  
>  /* Disable attached power regulator if any. */
> -static void ak8975_power_off(const struct i2c_client *client)
> +static void ak8975_power_off(const struct ak8975_data *data)
>  {
> -	const struct iio_dev *indio_dev = i2c_get_clientdata(client);
> -	const struct ak8975_data *data = iio_priv(indio_dev);
> -
>  	regulator_disable(data->vid);
>  	regulator_disable(data->vdd);
>  }
> @@ -936,7 +920,15 @@ static int ak8975_probe(struct i2c_client *client,
>  
>  	data->def = &ak_def_array[chipset];
>  
> -	err = ak8975_power_on(client);
> +	/* Fetch the regulators */
> +	data->vdd = devm_regulator_get(&client->dev, "vdd");
> +	if (IS_ERR(data->vdd))
> +		return PTR_ERR(data->vdd);
> +	data->vid = devm_regulator_get(&client->dev, "vid");
> +	if (IS_ERR(data->vid))
> +		return PTR_ERR(data->vid);
> +
> +	err = ak8975_power_on(data);
>  	if (err)
>  		return err;
>  
> @@ -981,17 +973,18 @@ static int ak8975_probe(struct i2c_client *client,
>  cleanup_buffer:
>  	iio_triggered_buffer_cleanup(indio_dev);
>  power_off:
> -	ak8975_power_off(client);
> +	ak8975_power_off(data);
>  	return err;
>  }
>  
>  static int ak8975_remove(struct i2c_client *client)
>  {
>  	struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +	struct ak8975_data *data = iio_priv(indio_dev);
>  
>  	iio_device_unregister(indio_dev);
>  	iio_triggered_buffer_cleanup(indio_dev);
> -	ak8975_power_off(client);
> +	ak8975_power_off(data);
>  
>  	return 0;
>  }
> 

[PATCH 4/6 v3] iio: magn: ak8975: allow a delay after enabling regulators

[Linus Walleij]

The datasheet actually specifies that we need to wait atleast
500us after powering on the device before trying to set mode.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Rebase
ChangeLog v1->v2:
- No changes.
---
 drivers/iio/magnetometer/ak8975.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index f72f51d1f3f5..c67cf3a6128d 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -399,6 +399,12 @@ static int ak8975_power_on(const struct ak8975_data *data)
 			 "Failed to enable specified Vid supply\n");
 		return ret;
 	}
+	/*
+	 * According to the datasheet the power supply rise time i 200us
+	 * and the minimum wait time before mode setting is 100us, in
+	 * total 300 us. Add some margin and say minimum 500us here.
+	 */
+	usleep_range(500, 1000);
 	return 0;
 }
 
-- 
2.4.11

Re: [PATCH 4/6 v3] iio: magn: ak8975: allow a delay after enabling regulators

[Jonathan Cameron]

On 29/06/16 13:08, Linus Walleij wrote:
> The datasheet actually specifies that we need to wait atleast
> 500us after powering on the device before trying to set mode.
> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Applied. thanks
> ---
> ChangeLog v2->v3:
> - Rebase
> ChangeLog v1->v2:
> - No changes.
> ---
>  drivers/iio/magnetometer/ak8975.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index f72f51d1f3f5..c67cf3a6128d 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -399,6 +399,12 @@ static int ak8975_power_on(const struct ak8975_data *data)
>  			 "Failed to enable specified Vid supply\n");
>  		return ret;
>  	}
> +	/*
> +	 * According to the datasheet the power supply rise time i 200us
> +	 * and the minimum wait time before mode setting is 100us, in
> +	 * total 300 us. Add some margin and say minimum 500us here.
> +	 */
> +	usleep_range(500, 1000);
>  	return 0;
>  }
>  
> 

[PATCH 5/6 v3] iio: magn: ak8975: make sure to power down at remove()

[Linus Walleij]

The code was not powering the magnetometer down properly at
remove(): just cutting the regulators without first setting the
device in power off mode. Fix this.

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Rebase
ChangeLog v1->v2:
- New patch in the series after Ulf's review comment that this
  should be separate.
---
 drivers/iio/magnetometer/ak8975.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index c67cf3a6128d..43dc1c74be9e 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -990,6 +990,7 @@ static int ak8975_remove(struct i2c_client *client)
 
 	iio_device_unregister(indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
+	ak8975_set_mode(data, POWER_DOWN);
 	ak8975_power_off(data);
 
 	return 0;
-- 
2.4.11

Re: [PATCH 5/6 v3] iio: magn: ak8975: make sure to power down at remove()

[Ulf Hansson]

On 29 June 2016 at 14:08, Linus Walleij <linus.walleij@linaro.org> wrote:
> The code was not powering the magnetometer down properly at
> remove(): just cutting the regulators without first setting the
> device in power off mode. Fix this.
>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
> ChangeLog v2->v3:
> - Rebase
> ChangeLog v1->v2:
> - New patch in the series after Ulf's review comment that this
>   should be separate.
> ---
>  drivers/iio/magnetometer/ak8975.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index c67cf3a6128d..43dc1c74be9e 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -990,6 +990,7 @@ static int ak8975_remove(struct i2c_client *client)
>
>         iio_device_unregister(indio_dev);
>         iio_triggered_buffer_cleanup(indio_dev);
> +       ak8975_set_mode(data, POWER_DOWN);
>         ak8975_power_off(data);
>
>         return 0;
> --
> 2.4.11
>

Re: [PATCH 5/6 v3] iio: magn: ak8975: make sure to power down at remove()

[Jonathan Cameron]

On 29/06/16 17:33, Ulf Hansson wrote:
> On 29 June 2016 at 14:08, Linus Walleij <linus.walleij@linaro.org> wrote:
>> The code was not powering the magnetometer down properly at
>> remove(): just cutting the regulators without first setting the
>> device in power off mode. Fix this.
>>
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Applied to the togreg branch of iio.git. Initially pushed out
as testing for the autobuilders to play hide and seek.
> 
> Kind regards
> Uffe
> 
>> ---
>> ChangeLog v2->v3:
>> - Rebase
>> ChangeLog v1->v2:
>> - New patch in the series after Ulf's review comment that this
>>   should be separate.
>> ---
>>  drivers/iio/magnetometer/ak8975.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
>> index c67cf3a6128d..43dc1c74be9e 100644
>> --- a/drivers/iio/magnetometer/ak8975.c
>> +++ b/drivers/iio/magnetometer/ak8975.c
>> @@ -990,6 +990,7 @@ static int ak8975_remove(struct i2c_client *client)
>>
>>         iio_device_unregister(indio_dev);
>>         iio_triggered_buffer_cleanup(indio_dev);
>> +       ak8975_set_mode(data, POWER_DOWN);
>>         ak8975_power_off(data);
>>
>>         return 0;
>> --
>> 2.4.11
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 6/6 v3] iio: magn: ak8975: deploy runtime and system PM

[Linus Walleij]

This adds runtime PM support to the AK8975 driver. It solves two
problems:

- After reading the first value the chip was left in MODE_ONCE,
  meaning (presumably) it may be consuming more power. Now the
  runtime PM hooks kick in and set it to POWER_DOWN.

- Regulators were simply enabled and left on, making it
  impossible to turn the power consuming regulators off because
  of the increased refcount. We now disable the regulators at
  autosuspend.

- We also handle system suspend: by using pm_runtime_force_suspend()
  and pm_runtime_force_resume() from the system PM sleep hooks,
  the runtime PM code is managing the power also for this case.
  It is currently not completely optimal: when the system resumes
  the AK8975 goes into active mode even if noone is going to use
  it: currently the force calls need to be paired, but the runtime
  PM people are working on making it possible to leave devices
  runtime suspended when coming back from sleep.

Inspired by my work on the BH1780 light sensor driver.

Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v2->v3:
- Rebase, fix commit message.
ChangeLog v1->v2:
- Split off an unrelated change to a separate patch.
- Remove silly debug prints
- Update commit description to describe system PM handling.
---
 drivers/iio/magnetometer/ak8975.c | 72 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 43dc1c74be9e..d26796eb0b1b 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -33,6 +33,7 @@
 #include <linux/of_gpio.h>
 #include <linux/acpi.h>
 #include <linux/regulator/consumer.h>
+#include <linux/pm_runtime.h>
 
 #include <linux/iio/iio.h>
 #include <linux/iio/sysfs.h>
@@ -692,6 +693,8 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
 	u16 buff;
 	int ret;
 
+	pm_runtime_get_sync(&data->client->dev);
+
 	mutex_lock(&data->lock);
 
 	ret = ak8975_start_read_axis(data, client);
@@ -706,6 +709,9 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
 
 	mutex_unlock(&data->lock);
 
+	pm_runtime_mark_last_busy(&data->client->dev);
+	pm_runtime_put_autosuspend(&data->client->dev);
+
 	/* Swap bytes and convert to valid range. */
 	buff = le16_to_cpu(buff);
 	*val = clamp_t(s16, buff, -def->range, def->range);
@@ -974,6 +980,18 @@ static int ak8975_probe(struct i2c_client *client,
 		goto cleanup_buffer;
 	}
 
+	/* Enable runtime PM */
+	pm_runtime_get_noresume(&client->dev);
+	pm_runtime_set_active(&client->dev);
+	pm_runtime_enable(&client->dev);
+	/*
+	 * The device comes online in 500us, so add two orders of magnitude
+	 * of delay before autosuspending: 50 ms.
+	 */
+	pm_runtime_set_autosuspend_delay(&client->dev, 50);
+	pm_runtime_use_autosuspend(&client->dev);
+	pm_runtime_put(&client->dev);
+
 	return 0;
 
 cleanup_buffer:
@@ -988,6 +1006,9 @@ static int ak8975_remove(struct i2c_client *client)
 	struct iio_dev *indio_dev = i2c_get_clientdata(client);
 	struct ak8975_data *data = iio_priv(indio_dev);
 
+	pm_runtime_get_sync(&client->dev);
+	pm_runtime_put_noidle(&client->dev);
+	pm_runtime_disable(&client->dev);
 	iio_device_unregister(indio_dev);
 	iio_triggered_buffer_cleanup(indio_dev);
 	ak8975_set_mode(data, POWER_DOWN);
@@ -996,6 +1017,56 @@ static int ak8975_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int ak8975_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct ak8975_data *data = iio_priv(indio_dev);
+	int ret;
+
+	/* Set the device in power down if it wasn't already */
+	ret = ak8975_set_mode(data, POWER_DOWN);
+	if (ret < 0) {
+		dev_err(&client->dev, "Error in setting power-down mode\n");
+		return ret;
+	}
+	/* Next cut the regulators */
+	ak8975_power_off(data);
+
+	return 0;
+}
+
+static int ak8975_runtime_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct iio_dev *indio_dev = i2c_get_clientdata(client);
+	struct ak8975_data *data = iio_priv(indio_dev);
+	int ret;
+
+	/* Take up the regulators */
+	ak8975_power_on(data);
+	/*
+	 * We come up in powered down mode, the reading routines will
+	 * put us in the mode to read values later.
+	 */
+	ret = ak8975_set_mode(data, POWER_DOWN);
+	if (ret < 0) {
+		dev_err(&client->dev, "Error in setting power-down mode\n");
+		return ret;
+	}
+
+	return 0;
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops ak8975_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(ak8975_runtime_suspend,
+			   ak8975_runtime_resume, NULL)
+};
+
 static const struct i2c_device_id ak8975_id[] = {
 	{"ak8975", AK8975},
 	{"ak8963", AK8963},
@@ -1023,6 +1094,7 @@ MODULE_DEVICE_TABLE(of, ak8975_of_match);
 static struct i2c_driver ak8975_driver = {
 	.driver = {
 		.name	= "ak8975",
+		.pm = &ak8975_dev_pm_ops,
 		.of_match_table = of_match_ptr(ak8975_of_match),
 		.acpi_match_table = ACPI_PTR(ak_acpi_match),
 	},
-- 
2.4.11

Re: [PATCH 6/6 v3] iio: magn: ak8975: deploy runtime and system PM

[Ulf Hansson]

On 29 June 2016 at 14:08, Linus Walleij <linus.walleij@linaro.org> wrote:
> This adds runtime PM support to the AK8975 driver. It solves two
> problems:
>
> - After reading the first value the chip was left in MODE_ONCE,
>   meaning (presumably) it may be consuming more power. Now the
>   runtime PM hooks kick in and set it to POWER_DOWN.
>
> - Regulators were simply enabled and left on, making it
>   impossible to turn the power consuming regulators off because
>   of the increased refcount. We now disable the regulators at
>   autosuspend.
>
> - We also handle system suspend: by using pm_runtime_force_suspend()
>   and pm_runtime_force_resume() from the system PM sleep hooks,
>   the runtime PM code is managing the power also for this case.
>   It is currently not completely optimal: when the system resumes
>   the AK8975 goes into active mode even if noone is going to use
>   it: currently the force calls need to be paired, but the runtime
>   PM people are working on making it possible to leave devices
>   runtime suspended when coming back from sleep.
>
> Inspired by my work on the BH1780 light sensor driver.
>
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
> ChangeLog v2->v3:
> - Rebase, fix commit message.
> ChangeLog v1->v2:
> - Split off an unrelated change to a separate patch.
> - Remove silly debug prints
> - Update commit description to describe system PM handling.
> ---
>  drivers/iio/magnetometer/ak8975.c | 72 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 72 insertions(+)
>
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 43dc1c74be9e..d26796eb0b1b 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -33,6 +33,7 @@
>  #include <linux/of_gpio.h>
>  #include <linux/acpi.h>
>  #include <linux/regulator/consumer.h>
> +#include <linux/pm_runtime.h>
>
>  #include <linux/iio/iio.h>
>  #include <linux/iio/sysfs.h>
> @@ -692,6 +693,8 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>         u16 buff;
>         int ret;
>
> +       pm_runtime_get_sync(&data->client->dev);
> +
>         mutex_lock(&data->lock);
>
>         ret = ak8975_start_read_axis(data, client);
> @@ -706,6 +709,9 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>
>         mutex_unlock(&data->lock);
>
> +       pm_runtime_mark_last_busy(&data->client->dev);
> +       pm_runtime_put_autosuspend(&data->client->dev);
> +
>         /* Swap bytes and convert to valid range. */
>         buff = le16_to_cpu(buff);
>         *val = clamp_t(s16, buff, -def->range, def->range);
> @@ -974,6 +980,18 @@ static int ak8975_probe(struct i2c_client *client,
>                 goto cleanup_buffer;
>         }
>
> +       /* Enable runtime PM */
> +       pm_runtime_get_noresume(&client->dev);
> +       pm_runtime_set_active(&client->dev);
> +       pm_runtime_enable(&client->dev);
> +       /*
> +        * The device comes online in 500us, so add two orders of magnitude
> +        * of delay before autosuspending: 50 ms.
> +        */
> +       pm_runtime_set_autosuspend_delay(&client->dev, 50);
> +       pm_runtime_use_autosuspend(&client->dev);
> +       pm_runtime_put(&client->dev);
> +
>         return 0;
>
>  cleanup_buffer:
> @@ -988,6 +1006,9 @@ static int ak8975_remove(struct i2c_client *client)
>         struct iio_dev *indio_dev = i2c_get_clientdata(client);
>         struct ak8975_data *data = iio_priv(indio_dev);
>
> +       pm_runtime_get_sync(&client->dev);
> +       pm_runtime_put_noidle(&client->dev);
> +       pm_runtime_disable(&client->dev);
>         iio_device_unregister(indio_dev);
>         iio_triggered_buffer_cleanup(indio_dev);
>         ak8975_set_mode(data, POWER_DOWN);
> @@ -996,6 +1017,56 @@ static int ak8975_remove(struct i2c_client *client)
>         return 0;
>  }
>
> +#ifdef CONFIG_PM
> +static int ak8975_runtime_suspend(struct device *dev)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +       struct ak8975_data *data = iio_priv(indio_dev);
> +       int ret;
> +
> +       /* Set the device in power down if it wasn't already */
> +       ret = ak8975_set_mode(data, POWER_DOWN);
> +       if (ret < 0) {
> +               dev_err(&client->dev, "Error in setting power-down mode\n");
> +               return ret;
> +       }
> +       /* Next cut the regulators */
> +       ak8975_power_off(data);
> +
> +       return 0;
> +}
> +
> +static int ak8975_runtime_resume(struct device *dev)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +       struct ak8975_data *data = iio_priv(indio_dev);
> +       int ret;
> +
> +       /* Take up the regulators */
> +       ak8975_power_on(data);
> +       /*
> +        * We come up in powered down mode, the reading routines will
> +        * put us in the mode to read values later.
> +        */
> +       ret = ak8975_set_mode(data, POWER_DOWN);
> +       if (ret < 0) {
> +               dev_err(&client->dev, "Error in setting power-down mode\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops ak8975_dev_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                               pm_runtime_force_resume)
> +       SET_RUNTIME_PM_OPS(ak8975_runtime_suspend,
> +                          ak8975_runtime_resume, NULL)
> +};
> +
>  static const struct i2c_device_id ak8975_id[] = {
>         {"ak8975", AK8975},
>         {"ak8963", AK8963},
> @@ -1023,6 +1094,7 @@ MODULE_DEVICE_TABLE(of, ak8975_of_match);
>  static struct i2c_driver ak8975_driver = {
>         .driver = {
>                 .name   = "ak8975",
> +               .pm = &ak8975_dev_pm_ops,
>                 .of_match_table = of_match_ptr(ak8975_of_match),
>                 .acpi_match_table = ACPI_PTR(ak_acpi_match),
>         },
> --
> 2.4.11
>