linux-next: build failure after merge of the gpio tree

linux-next: build failure after merge of the gpio tree

[Stephen Rothwell]

Hi Linus,

After merging the gpio tree, today's linux-next build (x86_64
allmodconfig) failed like this:

drivers/regulator/rk808-regulator.c: In function 'rk808_regulator_dt_parse_pdata':
drivers/regulator/rk808-regulator.c:543:24: error: too few arguments to function 'gpiod_get_index'
   pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i);
                        ^
In file included from include/asm-generic/gpio.h:13:0,
                 from include/linux/gpio.h:51,
                 from drivers/regulator/rk808-regulator.c:20:
include/linux/gpio/consumer.h:53:32: note: declared here
 struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
                                ^

Caused by commit

  bad47ad2eef3 ("regulator: rk808: fixed the overshoot when adjust voltage")

from the regulator tree interactings with commit

  b17d1bf16cc7 ("gpio: make flags mandatory for gpiod_get functions")

from the gpio tree.

I added teh following merge fix patch:

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Tue, 21 Jul 2015 13:23:56 +1000
Subject: [PATCH] regulator: rk808: fix up for gpiod_get_index() API change

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 drivers/regulator/rk808-regulator.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c
index ca913fd15598..cfec52c1a4a2 100644
--- a/drivers/regulator/rk808-regulator.c
+++ b/drivers/regulator/rk808-regulator.c
@@ -540,7 +540,7 @@ static int rk808_regulator_dt_parse_pdata(struct device *dev,
 		goto dt_parse_end;
 
 	for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) {
-		pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i);
+		pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i, GPIOD_ASIS);
 		if (IS_ERR(pdata->dvs_gpio[i])) {
 			dev_warn(dev, "there is no dvs%d gpio\n", i);
 			continue;
-- 
2.1.4

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[PATCH] regulator: rk808: make better use of the gpiod API

[Uwe Kleine-König]

The gpiod functions include variants for managed gpiod resources. Use it
to simplify the remove function.

As the driver handles a device node without a specification of dvs gpios
just fine, additionally use the variant of gpiod_get exactly for this
use case. This makes error checking more strict.

As a third benefit this patch makes the driver use the flags parameter
of gpiod_get* which will not be optional any more after 4.2 and so
prevents a build failure when the respective gpiod commit is merged.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

this is the more complete fix of the issue that Stephen found while creating
next-20150721 (see commit f51ec04cf8be9f7ef795f1f39ada17c19f725650).

Best regards
Uwe

 drivers/regulator/rk808-regulator.c | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c
index ca913fd15598..3738e49beb75 100644
--- a/drivers/regulator/rk808-regulator.c
+++ b/drivers/regulator/rk808-regulator.c
@@ -94,7 +94,7 @@ static int rk808_buck1_2_get_voltage_sel_regmap(struct regulator_dev *rdev)
 	unsigned int val;
 	int ret;
 
-	if (IS_ERR(gpio) || gpiod_get_value(gpio) == 0)
+	if (!gpio || gpiod_get_value(gpio) == 0)
 		return regulator_get_voltage_sel_regmap(rdev);
 
 	ret = regmap_read(rdev->regmap,
@@ -168,7 +168,7 @@ static int rk808_buck1_2_set_voltage_sel(struct regulator_dev *rdev,
 	unsigned old_sel;
 	int ret, gpio_level;
 
-	if (IS_ERR(gpio))
+	if (!gpio)
 		return rk808_buck1_2_i2c_set_voltage_sel(rdev, sel);
 
 	gpio_level = gpiod_get_value(gpio);
@@ -205,7 +205,7 @@ static int rk808_buck1_2_set_voltage_time_sel(struct regulator_dev *rdev,
 	struct gpio_desc *gpio = pdata->dvs_gpio[id];
 
 	/* if there is no dvs1/2 pin, we don't need wait extra time here. */
-	if (IS_ERR(gpio))
+	if (!gpio)
 		return 0;
 
 	return regulator_set_voltage_time_sel(rdev, old_selector, new_selector);
@@ -540,14 +540,19 @@ static int rk808_regulator_dt_parse_pdata(struct device *dev,
 		goto dt_parse_end;
 
 	for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) {
-		pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i);
+		pdata->dvs_gpio[i] =
+			devm_gpiod_get_index_optional(client_dev, "dvs", i,
+						      GPIOD_OUT_LOW);
 		if (IS_ERR(pdata->dvs_gpio[i])) {
+			dev_err(dev, "failed to get dvs%d gpio\n", i);
+			return PTR_ERR(pdata->dvs_gpio[i]);
+		}
+
+		if (!pdata->dvs_gpio[i]) {
 			dev_warn(dev, "there is no dvs%d gpio\n", i);
 			continue;
 		}
 
-		gpiod_direction_output(pdata->dvs_gpio[i], 0);
-
 		tmp = i ? RK808_DVS2_POL : RK808_DVS1_POL;
 		ret = regmap_update_bits(map, RK808_IO_POL_REG, tmp,
 				gpiod_is_active_low(pdata->dvs_gpio[i]) ?
@@ -561,14 +566,6 @@ dt_parse_end:
 
 static int rk808_regulator_remove(struct platform_device *pdev)
 {
-	struct rk808_regulator_data *pdata = platform_get_drvdata(pdev);
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) {
-		if (!IS_ERR(pdata->dvs_gpio[i]))
-			gpiod_put(pdata->dvs_gpio[i]);
-	}
-
 	return 0;
 }
 
-- 
2.1.4

Re: [PATCH] regulator: rk808: make better use of the gpiod API

[Linus Walleij]

On Tue, Jul 21, 2015 at 8:59 AM, Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:

> The gpiod functions include variants for managed gpiod resources. Use it
> to simplify the remove function.
>
> As the driver handles a device node without a specification of dvs gpios
> just fine, additionally use the variant of gpiod_get exactly for this
> use case. This makes error checking more strict.
>
> As a third benefit this patch makes the driver use the flags parameter
> of gpiod_get* which will not be optional any more after 4.2 and so
> prevents a build failure when the respective gpiod commit is merged.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
>
> this is the more complete fix of the issue that Stephen found while creating
> next-20150721 (see commit f51ec04cf8be9f7ef795f1f39ada17c19f725650).

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Mark, please apply this.

Yours,
Linus Walleij

Re: [PATCH] regulator: rk808: make better use of the gpiod API

[Mark Brown]

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

On Tue, Jul 21, 2015 at 11:56:49AM +0200, Linus Walleij wrote:
> On Tue, Jul 21, 2015 at 8:59 AM, Uwe Kleine-König

> > The gpiod functions include variants for managed gpiod resources. Use it
> > to simplify the remove function.

> > As the driver handles a device node without a specification of dvs gpios
> > just fine, additionally use the variant of gpiod_get exactly for this
> > use case. This makes error checking more strict.

> > As a third benefit this patch makes the driver use the flags parameter
> > of gpiod_get* which will not be optional any more after 4.2 and so
> > prevents a build failure when the respective gpiod commit is merged.

> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Mark, please apply this.

...and reverted since it doesn't build as it's adding a use of a new API
which doesn't actually exist in Linus' tree yet (hopefully it's in
-next).  I need a tag to pull if I'm going to use this patch.

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

Re: [PATCH] regulator: rk808: make better use of the gpiod API

[Krzysztof Kozlowski]

2015-07-21 15:59 GMT+09:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> The gpiod functions include variants for managed gpiod resources. Use it
> to simplify the remove function.
>
> As the driver handles a device node without a specification of dvs gpios
> just fine, additionally use the variant of gpiod_get exactly for this
> use case. This makes error checking more strict.
>
> As a third benefit this patch makes the driver use the flags parameter
> of gpiod_get* which will not be optional any more after 4.2 and so
> prevents a build failure when the respective gpiod commit is merged.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
>
> this is the more complete fix of the issue that Stephen found while creating
> next-20150721 (see commit f51ec04cf8be9f7ef795f1f39ada17c19f725650).
>
> Best regards
> Uwe
>
>  drivers/regulator/rk808-regulator.c | 25 +++++++++++--------------
>  1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c
> index ca913fd15598..3738e49beb75 100644
> --- a/drivers/regulator/rk808-regulator.c
> +++ b/drivers/regulator/rk808-regulator.c
> @@ -94,7 +94,7 @@ static int rk808_buck1_2_get_voltage_sel_regmap(struct regulator_dev *rdev)
>         unsigned int val;
>         int ret;
>
> -       if (IS_ERR(gpio) || gpiod_get_value(gpio) == 0)
> +       if (!gpio || gpiod_get_value(gpio) == 0)
>                 return regulator_get_voltage_sel_regmap(rdev);
>
>         ret = regmap_read(rdev->regmap,
> @@ -168,7 +168,7 @@ static int rk808_buck1_2_set_voltage_sel(struct regulator_dev *rdev,
>         unsigned old_sel;
>         int ret, gpio_level;
>
> -       if (IS_ERR(gpio))
> +       if (!gpio)
>                 return rk808_buck1_2_i2c_set_voltage_sel(rdev, sel);
>
>         gpio_level = gpiod_get_value(gpio);
> @@ -205,7 +205,7 @@ static int rk808_buck1_2_set_voltage_time_sel(struct regulator_dev *rdev,
>         struct gpio_desc *gpio = pdata->dvs_gpio[id];
>
>         /* if there is no dvs1/2 pin, we don't need wait extra time here. */
> -       if (IS_ERR(gpio))
> +       if (!gpio)
>                 return 0;
>
>         return regulator_set_voltage_time_sel(rdev, old_selector, new_selector);
> @@ -540,14 +540,19 @@ static int rk808_regulator_dt_parse_pdata(struct device *dev,
>                 goto dt_parse_end;
>
>         for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) {
> -               pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i);
> +               pdata->dvs_gpio[i] =
> +                       devm_gpiod_get_index_optional(client_dev, "dvs", i,
> +                                                     GPIOD_OUT_LOW);
>                 if (IS_ERR(pdata->dvs_gpio[i])) {
> +                       dev_err(dev, "failed to get dvs%d gpio\n", i);

Missing of_node_put() from of_get_child_by_name() called before.

> +                       return PTR_ERR(pdata->dvs_gpio[i]);
> +               }
> +
> +               if (!pdata->dvs_gpio[i]) {
>                         dev_warn(dev, "there is no dvs%d gpio\n", i);
>                         continue;
>                 }
>
> -               gpiod_direction_output(pdata->dvs_gpio[i], 0);
> -
>                 tmp = i ? RK808_DVS2_POL : RK808_DVS1_POL;
>                 ret = regmap_update_bits(map, RK808_IO_POL_REG, tmp,
>                                 gpiod_is_active_low(pdata->dvs_gpio[i]) ?
> @@ -561,14 +566,6 @@ dt_parse_end:
>
>  static int rk808_regulator_remove(struct platform_device *pdev)
>  {
> -       struct rk808_regulator_data *pdata = platform_get_drvdata(pdev);
> -       int i;
> -
> -       for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) {
> -               if (!IS_ERR(pdata->dvs_gpio[i]))
> -                       gpiod_put(pdata->dvs_gpio[i]);
> -       }
> -
>         return 0;
>  }

The function looks empty so it can be removed entirely.

Best regards,
Krzysztof

Re: [PATCH] regulator: rk808: make better use of the gpiod API

[Uwe Kleine-König]

On Tue, Jul 21, 2015 at 12:04:15PM +0100, Mark Brown wrote:
> On Tue, Jul 21, 2015 at 11:56:49AM +0200, Linus Walleij wrote:
> > On Tue, Jul 21, 2015 at 8:59 AM, Uwe Kleine-König
> 
> > > The gpiod functions include variants for managed gpiod resources. Use it
> > > to simplify the remove function.
> 
> > > As the driver handles a device node without a specification of dvs gpios
> > > just fine, additionally use the variant of gpiod_get exactly for this
> > > use case. This makes error checking more strict.
> 
> > > As a third benefit this patch makes the driver use the flags parameter
> > > of gpiod_get* which will not be optional any more after 4.2 and so
> > > prevents a build failure when the respective gpiod commit is merged.
> 
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> > 
> > Mark, please apply this.
> 
> ...and reverted since it doesn't build as it's adding a use of a new API
> which doesn't actually exist in Linus' tree yet (hopefully it's in
> -next).  I need a tag to pull if I'm going to use this patch.
If you're refering to the build bot warning I got, too, then the problem
is just a missing #include and the problem existed already before e.g.
for gpiod_get_value with the .config used by the build bot.

In reply to this mail I'll send a fixup patch for the missing include
and a reworked version of my patch that fixes the reference leak pointed
out by Krzysztof Kozlowski.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] regulator: rk808: make better use of the gpiod API

[Uwe Kleine-König]

Hello,

On Tue, Jul 21, 2015 at 10:09:32PM +0900, Krzysztof Kozlowski wrote:
> 2015-07-21 15:59 GMT+09:00 Uwe Kleine-König <u.kleine-koenig@pengutronix.de>:
> > @@ -540,14 +540,19 @@ static int rk808_regulator_dt_parse_pdata(struct device *dev,
> >                 goto dt_parse_end;
> >
> >         for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) {
> > -               pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i);
> > +               pdata->dvs_gpio[i] =
> > +                       devm_gpiod_get_index_optional(client_dev, "dvs", i,
> > +                                                     GPIOD_OUT_LOW);
> >                 if (IS_ERR(pdata->dvs_gpio[i])) {
> > +                       dev_err(dev, "failed to get dvs%d gpio\n", i);
> 
> Missing of_node_put() from of_get_child_by_name() called before.
Good catch, thanks.

> > @@ -561,14 +566,6 @@ dt_parse_end:
> >
> >  static int rk808_regulator_remove(struct platform_device *pdev)
> >  {
> > -       struct rk808_regulator_data *pdata = platform_get_drvdata(pdev);
> > -       int i;
> > -
> > -       for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) {
> > -               if (!IS_ERR(pdata->dvs_gpio[i]))
> > -                       gpiod_put(pdata->dvs_gpio[i]);
> > -       }
> > -
> >         return 0;
> >  }
> 
> The function looks empty so it can be removed entirely.
I assumed that not having a remove function makes the device not
detachable. Not sure about that.
Looking at the code I found that not having a remove function can yield
surprises, though. If your driver has a probe but no remove function the
platform bus glue calls

	dev_pm_domain_attach(_dev, true);

at probe time, but not

	dev_pm_domain_detach(_dev, true);

at remove. I admit I don't know about that dev_pm_domain stuff, but it
looks wrong to only have one but not the other. Greg?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] regulator: rk808: make better use of the gpiod API

[Mark Brown]

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

On Tue, Jul 21, 2015 at 04:35:24PM +0200, Uwe Kleine-König wrote:
> On Tue, Jul 21, 2015 at 10:09:32PM +0900, Krzysztof Kozlowski wrote:

> > The function looks empty so it can be removed entirely.

> I assumed that not having a remove function makes the device not
> detachable. Not sure about that.

No, of course not - the remove function is completely optional.

> Looking at the code I found that not having a remove function can yield
> surprises, though. If your driver has a probe but no remove function the
> platform bus glue calls

> 	dev_pm_domain_attach(_dev, true);

> at probe time, but not

> 	dev_pm_domain_detach(_dev, true);

> at remove. I admit I don't know about that dev_pm_domain stuff, but it
> looks wrong to only have one but not the other. Greg?

That looks like a bug, yes.

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

[PATCH 1/2] regulator: rk808: add #include for gpiod functions

[Uwe Kleine-König]

This fixes a build problem on mips found by the kbuild test robot:

drivers/regulator/rk808-regulator.c: In function 'rk808_buck1_2_get_voltage_sel_regmap':
drivers/regulator/rk808-regulator.c:97:2: error: implicit declaration of function 'gpiod_get_value' [-Werror=implicit-function-declaration]
  if (IS_ERR(gpio) || gpiod_get_value(gpio) == 0)
  ^

Fixes: bad47ad2eef3 ("regulator: rk808: fixed the overshoot when adjust voltage")
Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
 drivers/regulator/rk808-regulator.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c
index ca913fd15598..ac9436d2ea8d 100644
--- a/drivers/regulator/rk808-regulator.c
+++ b/drivers/regulator/rk808-regulator.c
@@ -25,6 +25,7 @@
 #include <linux/mfd/rk808.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/of_regulator.h>
+#include <linux/gpio/consumer.h>
 
 /* Field Definitions */
 #define RK808_BUCK_VSEL_MASK	0x3f
-- 
2.1.4

[PATCH v2 2/2] regulator: rk808: make better use of the gpiod API

[Uwe Kleine-König]

From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

The gpiod functions include variants for managed gpiod resources. Use it
to simplify the remove function.

As the driver handles a device node without a specification of dvs gpios
just fine, additionally use the variant of gpiod_get exactly for this
use case. This makes error checking more strict.

As a third benefit this patch makes the driver use the flags parameter
of gpiod_get* which will not be optional any more after 4.2 and so
prevents a build failure when the respective gpiod commit is merged.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
note the above mentioned gpiod change is already in next, so the driver
fails to build there.

Changes since (implicit) v1, sent with
Message-Id: 1437461993-14860-1-git-send-email-u.kleine-koenig@pengutronix.de:

 - Assert that of_node_put is called in error path to not leak a reference
   and drop now empty remove callback.
   Thanks to Krzysztof Kozlowski for catching.


Best regards
Uwe
 drivers/regulator/rk808-regulator.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/regulator/rk808-regulator.c b/drivers/regulator/rk808-regulator.c
index ac9436d2ea8d..d86a3dcd61e2 100644
--- a/drivers/regulator/rk808-regulator.c
+++ b/drivers/regulator/rk808-regulator.c
@@ -95,7 +95,7 @@ static int rk808_buck1_2_get_voltage_sel_regmap(struct regulator_dev *rdev)
 	unsigned int val;
 	int ret;
 
-	if (IS_ERR(gpio) || gpiod_get_value(gpio) == 0)
+	if (!gpio || gpiod_get_value(gpio) == 0)
 		return regulator_get_voltage_sel_regmap(rdev);
 
 	ret = regmap_read(rdev->regmap,
@@ -169,7 +169,7 @@ static int rk808_buck1_2_set_voltage_sel(struct regulator_dev *rdev,
 	unsigned old_sel;
 	int ret, gpio_level;
 
-	if (IS_ERR(gpio))
+	if (!gpio)
 		return rk808_buck1_2_i2c_set_voltage_sel(rdev, sel);
 
 	gpio_level = gpiod_get_value(gpio);
@@ -206,7 +206,7 @@ static int rk808_buck1_2_set_voltage_time_sel(struct regulator_dev *rdev,
 	struct gpio_desc *gpio = pdata->dvs_gpio[id];
 
 	/* if there is no dvs1/2 pin, we don't need wait extra time here. */
-	if (IS_ERR(gpio))
+	if (!gpio)
 		return 0;
 
 	return regulator_set_voltage_time_sel(rdev, old_selector, new_selector);
@@ -541,14 +541,20 @@ static int rk808_regulator_dt_parse_pdata(struct device *dev,
 		goto dt_parse_end;
 
 	for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) {
-		pdata->dvs_gpio[i] = gpiod_get_index(client_dev, "dvs", i);
+		pdata->dvs_gpio[i] =
+			devm_gpiod_get_index_optional(client_dev, "dvs", i,
+						      GPIOD_OUT_LOW);
 		if (IS_ERR(pdata->dvs_gpio[i])) {
+			ret = PTR_ERR(pdata->dvs_gpio[i]);
+			dev_err(dev, "failed to get dvs%d gpio (%d)\n", i, ret);
+			goto dt_parse_end;
+		}
+
+		if (!pdata->dvs_gpio[i]) {
 			dev_warn(dev, "there is no dvs%d gpio\n", i);
 			continue;
 		}
 
-		gpiod_direction_output(pdata->dvs_gpio[i], 0);
-
 		tmp = i ? RK808_DVS2_POL : RK808_DVS1_POL;
 		ret = regmap_update_bits(map, RK808_IO_POL_REG, tmp,
 				gpiod_is_active_low(pdata->dvs_gpio[i]) ?
@@ -560,19 +566,6 @@ dt_parse_end:
 	return ret;
 }
 
-static int rk808_regulator_remove(struct platform_device *pdev)
-{
-	struct rk808_regulator_data *pdata = platform_get_drvdata(pdev);
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(pdata->dvs_gpio); i++) {
-		if (!IS_ERR(pdata->dvs_gpio[i]))
-			gpiod_put(pdata->dvs_gpio[i]);
-	}
-
-	return 0;
-}
-
 static int rk808_regulator_probe(struct platform_device *pdev)
 {
 	struct rk808 *rk808 = dev_get_drvdata(pdev->dev.parent);
@@ -619,7 +612,6 @@ static int rk808_regulator_probe(struct platform_device *pdev)
 
 static struct platform_driver rk808_regulator_driver = {
 	.probe = rk808_regulator_probe,
-	.remove = rk808_regulator_remove,
 	.driver = {
 		.name = "rk808-regulator",
 		.owner = THIS_MODULE,
-- 
2.1.4

[PATCH] base/platform: assert that dev_pm_domain callbacks are called unconditionally

[Uwe Kleine-König]

When a platform driver doesn't provide a .remove callback the function
platform_drv_remove isn't called and so the call to dev_pm_domain_attach
called at probe time isn't paired by dev_pm_domain_detach at remove
time.

To fix this (and similar issues if different callbacks are missing) hook
up the driver callbacks unconditionally and make them aware that the
platform callbacks might be missing.

Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
---
Hello,

I didn't see any breakage without this patch, but it looks wrong the way
it is.

Best regards
Uwe

 drivers/base/platform.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index 063f0ab15259..62debf4a1348 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -517,7 +517,7 @@ static int platform_drv_probe(struct device *_dev)
 		return ret;
 
 	ret = dev_pm_domain_attach(_dev, true);
-	if (ret != -EPROBE_DEFER) {
+	if (ret != -EPROBE_DEFER && drv->probe) {
 		ret = drv->probe(dev);
 		if (ret)
 			dev_pm_domain_detach(_dev, true);
@@ -542,7 +542,8 @@ static int platform_drv_remove(struct device *_dev)
 	struct platform_device *dev = to_platform_device(_dev);
 	int ret;
 
-	ret = drv->remove(dev);
+	if (drv->remove)
+		ret = drv->remove(dev);
 	dev_pm_domain_detach(_dev, true);
 
 	return ret;
@@ -553,7 +554,8 @@ static void platform_drv_shutdown(struct device *_dev)
 	struct platform_driver *drv = to_platform_driver(_dev->driver);
 	struct platform_device *dev = to_platform_device(_dev);
 
-	drv->shutdown(dev);
+	if (drv->shutdown)
+		drv->shutdown(dev);
 	dev_pm_domain_detach(_dev, true);
 }
 
@@ -567,12 +569,9 @@ int __platform_driver_register(struct platform_driver *drv,
 {
 	drv->driver.owner = owner;
 	drv->driver.bus = &platform_bus_type;
-	if (drv->probe)
-		drv->driver.probe = platform_drv_probe;
-	if (drv->remove)
-		drv->driver.remove = platform_drv_remove;
-	if (drv->shutdown)
-		drv->driver.shutdown = platform_drv_shutdown;
+	drv->driver.probe = platform_drv_probe;
+	drv->driver.remove = platform_drv_remove;
+	drv->driver.shutdown = platform_drv_shutdown;
 
 	return driver_register(&drv->driver);
 }
-- 
2.1.4

Re: [PATCH] regulator: rk808: make better use of the gpiod API

[Krzysztof Kozlowski]

On 21.07.2015 23:41, Mark Brown wrote:
> On Tue, Jul 21, 2015 at 04:35:24PM +0200, Uwe Kleine-König wrote:
>> On Tue, Jul 21, 2015 at 10:09:32PM +0900, Krzysztof Kozlowski wrote:
> 
>>> The function looks empty so it can be removed entirely.
> 
>> I assumed that not having a remove function makes the device not
>> detachable. Not sure about that.
> 
> No, of course not - the remove function is completely optional.
> 
>> Looking at the code I found that not having a remove function can yield
>> surprises, though. If your driver has a probe but no remove function the
>> platform bus glue calls
> 
>> 	dev_pm_domain_attach(_dev, true);
> 
>> at probe time, but not
> 
>> 	dev_pm_domain_detach(_dev, true);
> 
>> at remove. I admit I don't know about that dev_pm_domain stuff, but it
>> looks wrong to only have one but not the other. Greg?
> 
> That looks like a bug, yes.

Cc: linux-pm, Kevin, Rafael, Ulf

I agree, device should be detached from domain regardless of presence of
remove callback.

Documentation (like Documentation/driver-model/driver.txt) does not
mention that remove callback is necessary for unbinding devices. There
is no sense in storing empty removal callbacks.

Best regards,
Krzysztof

Re: [PATCH v2 2/2] regulator: rk808: make better use of the gpiod API

[Krzysztof Kozlowski]

2015-07-21 23:46 GMT+09:00 Uwe Kleine-König <uwe@kleine-koenig.org>:
> From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> The gpiod functions include variants for managed gpiod resources. Use it
> to simplify the remove function.
>
> As the driver handles a device node without a specification of dvs gpios
> just fine, additionally use the variant of gpiod_get exactly for this
> use case. This makes error checking more strict.
>
> As a third benefit this patch makes the driver use the flags parameter
> of gpiod_get* which will not be optional any more after 4.2 and so
> prevents a build failure when the respective gpiod commit is merged.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
> note the above mentioned gpiod change is already in next, so the driver
> fails to build there.
>
> Changes since (implicit) v1, sent with
> Message-Id: 1437461993-14860-1-git-send-email-u.kleine-koenig@pengutronix.de:
>
>  - Assert that of_node_put is called in error path to not leak a reference
>    and drop now empty remove callback.
>    Thanks to Krzysztof Kozlowski for catching.
>
>
> Best regards
> Uwe
>  drivers/regulator/rk808-regulator.c | 32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)

Looks good now:
Reviewed-by: Krzysztof Kozlowski <k.kozlowski@samsung.com>

Best regards,
Krzysztof

Re: [PATCH] regulator: rk808: make better use of the gpiod API

[Uwe Kleine-König]

Hello,

On Wed, Jul 22, 2015 at 09:13:51AM +0900, Krzysztof Kozlowski wrote:
> On 21.07.2015 23:41, Mark Brown wrote:
> > That looks like a bug, yes.
> 
> Cc: linux-pm, Kevin, Rafael, Ulf
Good idea. Note I already sent a patch to Greg that you might want to
look at, too:
	http://article.gmane.org/gmane.linux.kernel/2001345

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] base/platform: assert that dev_pm_domain callbacks are called unconditionally

[Greg Kroah-Hartman]

On Tue, Jul 21, 2015 at 05:08:35PM +0200, Uwe Kleine-König wrote:
> When a platform driver doesn't provide a .remove callback the function
> platform_drv_remove isn't called and so the call to dev_pm_domain_attach
> called at probe time isn't paired by dev_pm_domain_detach at remove
> time.
> 
> To fix this (and similar issues if different callbacks are missing) hook
> up the driver callbacks unconditionally and make them aware that the
> platform callbacks might be missing.
> 
> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
> ---
> Hello,
> 
> I didn't see any breakage without this patch, but it looks wrong the way
> it is.
> 
> Best regards
> Uwe
> 
>  drivers/base/platform.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 063f0ab15259..62debf4a1348 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -517,7 +517,7 @@ static int platform_drv_probe(struct device *_dev)
>  		return ret;
>  
>  	ret = dev_pm_domain_attach(_dev, true);
> -	if (ret != -EPROBE_DEFER) {
> +	if (ret != -EPROBE_DEFER && drv->probe) {
>  		ret = drv->probe(dev);
>  		if (ret)
>  			dev_pm_domain_detach(_dev, true);
> @@ -542,7 +542,8 @@ static int platform_drv_remove(struct device *_dev)
>  	struct platform_device *dev = to_platform_device(_dev);
>  	int ret;
>  
> -	ret = drv->remove(dev);
> +	if (drv->remove)
> +		ret = drv->remove(dev);
>  	dev_pm_domain_detach(_dev, true);

This causes a build warning :(

Please fix.

thanks,

greg k-h