[PATCH 0/5] fix regulators and PM for AK8975

[PATCH 0/5] fix regulators and PM for AK8975

[Linus Walleij]

This fixes up the AK8975 so that it performs nicely on my APQ8060
Dragonboard which controls the power to it with a simple regulator
which actually goes off and on and cuts the power between uses
by using runtime PM's autosuspend feature.

Works like a charm for me, but would be nice to see more people try
it.

Linus Walleij (5):
  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: deploy runtime PM

 drivers/iio/magnetometer/ak8975.c | 133 +++++++++++++++++++++++++++++++-------
 1 file changed, 110 insertions(+), 23 deletions(-)

-- 
2.4.11

[PATCH 1/5] 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>
---
 drivers/iio/magnetometer/ak8975.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 609a2c401b5d..bc0770205b2d 100644
--- a/drivers/iio/magnetometer/ak8975.c
+++ b/drivers/iio/magnetometer/ak8975.c
@@ -389,17 +389,17 @@ 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_warn(&client->dev,
+			 "Failed to enable specified Vdd supply\n");
+		return ret;
+	}
 
-	if (ret)
-		dev_err(&client->dev, "failed to enable Vdd supply: %d\n", ret);
-	return ret;
 }
 
 /* Disable attached power regulator if any. */
@@ -408,8 +408,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

[PATCH 2/5] 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:
- Rebase and rewrite on top of the patch fixing up the regulator
  usage.
ChangeLog v1->v2:
- Just obtain the other regulator exactly like the first without
  trying to be fancy.
---
 drivers/iio/magnetometer/ak8975.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index bc0770205b2d..62f73d9d38b4 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. */
@@ -400,6 +401,18 @@ static int ak8975_power_on(struct i2c_client *client)
 		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. */
@@ -408,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

[PATCH 3/5] 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>
---
 drivers/iio/magnetometer/ak8975.c | 38 +++++++++++++++-----------------------
 1 file changed, 15 insertions(+), 23 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 62f73d9d38b4..8844e6f45c8e 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);
 }
@@ -932,7 +916,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;
 
-- 
2.4.11

[PATCH 4/5] 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>
---
 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 8844e6f45c8e..1c6e4157a91f 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

[PATCH 5/5] iio: magn: ak8975: deploy runtime 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.

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>
---
 drivers/iio/magnetometer/ak8975.c | 80 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 78 insertions(+), 2 deletions(-)

diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
index 1c6e4157a91f..71a8e3348b8a 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>
@@ -691,6 +692,8 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
 	const struct ak_def *def = data->def;
 	int ret;
 
+	pm_runtime_get_sync(&data->client->dev);
+
 	mutex_lock(&data->lock);
 
 	ret = ak8975_start_read_axis(data, client);
@@ -703,6 +706,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);
+
 	/* Clamp to valid range. */
 	*val = clamp_t(s16, ret, -def->range, def->range);
 	return IIO_VAL_INT;
@@ -970,26 +976,95 @@ 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:
 	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);
 
+	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_power_off(client);
+	ak8975_set_mode(data, POWER_DOWN);
+	ak8975_power_off(data);
+
+	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;
+
+	dev_info(&client->dev, "%s\n", __func__);
+	/* 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;
+
+	dev_info(&client->dev, "%s\n", __func__);
+	/* 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},
@@ -1017,6 +1092,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 5/5] iio: magn: ak8975: deploy runtime PM

[Ulf Hansson]

On 22 June 2016 at 00:18, 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.
>
> 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>
> ---
>  drivers/iio/magnetometer/ak8975.c | 80 ++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 78 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 1c6e4157a91f..71a8e3348b8a 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>
> @@ -691,6 +692,8 @@ static int ak8975_read_axis(struct iio_dev *indio_dev, int index, int *val)
>         const struct ak_def *def = data->def;
>         int ret;
>
> +       pm_runtime_get_sync(&data->client->dev);
> +
>         mutex_lock(&data->lock);
>
>         ret = ak8975_start_read_axis(data, client);
> @@ -703,6 +706,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);
> +
>         /* Clamp to valid range. */
>         *val = clamp_t(s16, ret, -def->range, def->range);
>         return IIO_VAL_INT;
> @@ -970,26 +976,95 @@ 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:
>         iio_triggered_buffer_cleanup(indio_dev);
>  power_off:
> -       ak8975_power_off(client);
> +       ak8975_power_off(data);

Is this really related to this change?

>         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);
>
> +       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_power_off(client);
> +       ak8975_set_mode(data, POWER_DOWN);
> +       ak8975_power_off(data);

I think these two last lines deserves as separate patch.

> +
> +       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;
> +
> +       dev_info(&client->dev, "%s\n", __func__);

Perhaps remove this print, seems like a leftover for a debug exercise. :-)

> +       /* 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;
> +
> +       dev_info(&client->dev, "%s\n", __func__);

Ditto.

> +       /* 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)

The system PM support isn't mentioned in the change log.

I don't mind that you add it within $subject patch, just add some
words about it in the changelog. Perhaps you can mention that you
"deploy system PM support via the runtime PM centric approach".

> +       SET_RUNTIME_PM_OPS(ak8975_runtime_suspend,
> +                          ak8975_runtime_resume, NULL)
> +};
> +
>  static const struct i2c_device_id ak8975_id[] = {
>         {"ak8975", AK8975},
>         {"ak8963", AK8963},
> @@ -1017,6 +1092,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
>

Nice work!

Kind regards
Uffe

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

[Gregor Boirie]

On 06/22/2016 12:18 AM, 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>
> ---
>   drivers/iio/magnetometer/ak8975.c | 15 +++++++--------
>   1 file changed, 7 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iio/magnetometer/ak8975.c b/drivers/iio/magnetometer/ak8975.c
> index 609a2c401b5d..bc0770205b2d 100644
> --- a/drivers/iio/magnetometer/ak8975.c
> +++ b/drivers/iio/magnetometer/ak8975.c
> @@ -389,17 +389,17 @@ 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;
what if dummy supplies are not allowed and regulator is not declared / 
found into device tree ?

I have no such regulator on some boards, just a main power supply that 
is not driven by soft.
How should this be handled in this case ?
>   	} else {
>   		ret = regulator_enable(data->vdd);
>   	}
> +	if (ret) {
> +		dev_warn(&client->dev,
> +			 "Failed to enable specified Vdd supply\n");
> +		return ret;
> +	}
>   
> -	if (ret)
> -		dev_err(&client->dev, "failed to enable Vdd supply: %d\n", ret);
> -	return ret;
>   }
>   
>   /* Disable attached power regulator if any. */
> @@ -408,8 +408,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);
>   }
>   
>   /*

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

[Linus Walleij]

Disclaimer: I am not the regulator maintainer but I think I
understand how these things are supposed to work.
Mark will correct me if I say something stupid, I hope.

On Wed, Jun 22, 2016 at 1:53 PM, Gregor Boirie <gregor.boirie@parrot.com> wrote:
> On 06/22/2016 12:18 AM, Linus Walleij wrote:

>>         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;
>
> what if dummy supplies are not allowed and regulator is not declared / found
> into device tree ?

What do you mean with "dummy supplies not allowed"? It is an
integral part of the regulator framework. It's not like you can choose
not to have it.

The only way a regulator doesn't resolve to a dummy
is with regulator_get_exclusive() which I'm not using or if your board
is lacking full constraints. (All DT boards have full constraints.)

In that case I think you should make sure to call
regulator_has_full_constraints() in your boardfile, and fix your
regulators because they're likely just broken.

> I have no such regulator on some boards, just a main power supply that is
> not driven by soft.

That should be defined in the DT as a fixed regulator
something like this:

+               /* Main power of the board: 3.7V */
+               vph: regulator-fixed {
+                       compatible = "regulator-fixed";
+                       regulator-min-microvolt = <3700000>;
+                       regulator-max-microvolt = <3700000>;
+                       regulator-name = "VPH";
+                       regulator-type = "voltage";
+                       regulator-always-on;
+                       regulator-boot-on;
+               };

This can also be done with a boardfile.

Of course also select REGULATOR_FIXED_VOLTAGE for
your board in KConfig.

> How should this be handled in this case ?

If the component has a (non-optional) external power
supply, its driver should issue regulator_get/enable for it.

If a board does not have all the proper regulators defined (which
is easy, in worst case just use fixed-regulator), it should
resort to use dummy regulators. It needs to call
regulator_has_full_constraints() for the dummies to kick in.

It is not proper to try to quirk around in the driver and
avoid getting a regulator just to stay compatible. Not
properly supplying either proper regulators nor dummies is
considered a bug.

Yours,
Linus Walleij

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 310 bytes --]

On Wed, Jun 22, 2016 at 05:41:51PM +0200, Linus Walleij wrote:

> The only way a regulator doesn't resolve to a dummy
> is with regulator_get_exclusive() which I'm not using or if your board
> is lacking full constraints. (All DT boards have full constraints.)

You mean regulator_get_optional() here I think.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]