[PATCH 00/10] Regulator ena_gpiod fixups

[PATCH 00/10] Regulator ena_gpiod fixups

[Linus Walleij]

We noticed a refcounting fight between the kernel device
core devm_* and the regulator core refcounting, pertaining
to enable GPIOd:s that may be shared between multiple
regulators.

Fix this with a series that hand it all over to the
regulator core and remove any devm_* stuff pertaining
to these GPIOs.

This includes a patch to gpiolib to make gpiod_get_from_node()
available. Just go ahead and apply this to the regulator
tree.

If these need to go for fixes or not is up to the
regulator maintainer, but all commits have a proper
Fixes: tag in case they would. I have noted in the
four last commits that the gpiolib patch is a
prerequisite.

Linus Walleij (10):
  regulator: fixed: Let core handle GPIO descriptor
  regulator: lm363x: Let core handle GPIO descriptor
  regulator: lp8788-ldo: Let core handle GPIO descriptor
  regulator: max8952: Let core handle GPIO descriptor
  regulator: max8973: Let core handle GPIO descriptor
  gpio: Export gpiod_get_from_of_node()
  regulator: da9211: Let core handle GPIO descriptors
  regulator: max77686: Let core handle GPIO descriptor
  regulator: s5m8767: Let core handle GPIO descriptors
  regulator: tps65090: Let core handle GPIO descriptors

 drivers/gpio/gpiolib.h                 |  6 -----
 drivers/regulator/da9211-regulator.c   |  4 +--
 drivers/regulator/fixed.c              |  4 ++-
 drivers/regulator/lm363x-regulator.c   |  8 ++++--
 drivers/regulator/lp8788-ldo.c         |  4 ++-
 drivers/regulator/max77686-regulator.c |  3 +--
 drivers/regulator/max8952.c            |  8 +++---
 drivers/regulator/max8973-regulator.c  | 12 ++++++---
 drivers/regulator/s5m8767.c            | 37 ++++++++++++++++++--------
 drivers/regulator/tps65090-regulator.c | 10 +++----
 include/linux/gpio/consumer.h          | 13 +++++++++
 11 files changed, 72 insertions(+), 37 deletions(-)

-- 
2.19.1

[PATCH 01/10] regulator: fixed: Let core handle GPIO descriptor

[Linus Walleij]

Use the gpiod_get() rather than the devm_* version so that the
regulator core can handle the lifecycle of these descriptors.

Fixes: efdfeb079cc3 ("regulator: fixed: Convert to use GPIO descriptor only")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/regulator/fixed.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index ccc29038f19a..52091ebf7164 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -183,7 +183,7 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 	 */
 	gflags |= GPIOD_FLAGS_BIT_NONEXCLUSIVE;
 
-	cfg.ena_gpiod = devm_gpiod_get_optional(&pdev->dev, NULL, gflags);
+	cfg.ena_gpiod = gpiod_get_optional(&pdev->dev, NULL, gflags);
 	if (IS_ERR(cfg.ena_gpiod))
 		return PTR_ERR(cfg.ena_gpiod);
 
@@ -195,6 +195,8 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 	drvdata->dev = devm_regulator_register(&pdev->dev, &drvdata->desc,
 					       &cfg);
 	if (IS_ERR(drvdata->dev)) {
+		if (cfg.ena_gpiod)
+			gpiod_put(cfg.ena_gpiod);
 		ret = PTR_ERR(drvdata->dev);
 		dev_err(&pdev->dev, "Failed to register regulator: %d\n", ret);
 		return ret;
-- 
2.19.1

[PATCH 02/10] regulator: lm363x: Let core handle GPIO descriptor

[Linus Walleij]

Use the gpiod_get() rather than the devm_* version so that the
regulator core can handle the lifecycle of these descriptors.

Fixes: b2d751b7f69b ("regulator: lm363x: Pass descriptor instead of GPIO number")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/regulator/lm363x-regulator.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/lm363x-regulator.c b/drivers/regulator/lm363x-regulator.c
index bbedb08d257b..06cfba441c46 100644
--- a/drivers/regulator/lm363x-regulator.c
+++ b/drivers/regulator/lm363x-regulator.c
@@ -227,10 +227,10 @@ static struct gpio_desc *lm363x_regulator_of_get_enable_gpio(struct device *dev,
 	 */
 	switch (id) {
 	case LM3632_LDO_POS:
-		return devm_gpiod_get_index_optional(dev, "enable", 0,
+		return gpiod_get_index_optional(dev, "enable", 0,
 				GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
 	case LM3632_LDO_NEG:
-		return devm_gpiod_get_index_optional(dev, "enable", 1,
+		return gpiod_get_index_optional(dev, "enable", 1,
 				GPIOD_OUT_LOW | GPIOD_FLAGS_BIT_NONEXCLUSIVE);
 	default:
 		return NULL;
@@ -263,6 +263,8 @@ static int lm363x_regulator_probe(struct platform_device *pdev)
 					 LM3632_EXT_EN_MASK,
 					 LM3632_EXT_EN_MASK);
 		if (ret) {
+			if (gpiod)
+				gpiod_put(gpiod);
 			dev_err(dev, "External pin err: %d\n", ret);
 			return ret;
 		}
@@ -270,6 +272,8 @@ static int lm363x_regulator_probe(struct platform_device *pdev)
 
 	rdev = devm_regulator_register(dev, &lm363x_regulator_desc[id], &cfg);
 	if (IS_ERR(rdev)) {
+		if (gpiod)
+			gpiod_put(gpiod);
 		ret = PTR_ERR(rdev);
 		dev_err(dev, "[%d] regulator register err: %d\n", id, ret);
 		return ret;
-- 
2.19.1

[PATCH 03/10] regulator: lp8788-ldo: Let core handle GPIO descriptor

[Linus Walleij]

Use the gpiod_get() rather than the devm_* version so that the
regulator core can handle the lifecycle of these descriptors.

Fixes: 2468f0d51548 ("regulator: lp8788-ldo: Pass descriptor instead of GPIO number")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/regulator/lp8788-ldo.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/lp8788-ldo.c b/drivers/regulator/lp8788-ldo.c
index 553b4790050f..416d073ec62c 100644
--- a/drivers/regulator/lp8788-ldo.c
+++ b/drivers/regulator/lp8788-ldo.c
@@ -502,7 +502,7 @@ static int lp8788_config_ldo_enable_mode(struct platform_device *pdev,
 	}
 
 	/* FIXME: check default mode for GPIO here: high or low? */
-	ldo->ena_gpiod = devm_gpiod_get_index_optional(&pdev->dev,
+	ldo->ena_gpiod = gpiod_get_index_optional(&pdev->dev,
 					       "enable",
 					       enable_id,
 					       GPIOD_OUT_HIGH |
@@ -548,6 +548,8 @@ static int lp8788_dldo_probe(struct platform_device *pdev)
 
 	rdev = devm_regulator_register(&pdev->dev, &lp8788_dldo_desc[id], &cfg);
 	if (IS_ERR(rdev)) {
+		if (ldo->ena_gpiod)
+			gpiod_put(ldo->ena_gpiod);
 		ret = PTR_ERR(rdev);
 		dev_err(&pdev->dev, "DLDO%d regulator register err = %d\n",
 				id + 1, ret);
-- 
2.19.1

[PATCH 04/10] regulator: max8952: Let core handle GPIO descriptor

[Linus Walleij]

Use the gpiod_get() rather than the devm_* version so that the
regulator core can handle the lifecycle of these descriptors.

Fixes: d7a261c2d1f2 ("regulator: max8952: Pass descriptor instead of GPIO number")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/regulator/max8952.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/max8952.c b/drivers/regulator/max8952.c
index 6c39fff73b8a..baa8b771a6e4 100644
--- a/drivers/regulator/max8952.c
+++ b/drivers/regulator/max8952.c
@@ -231,9 +231,9 @@ static int max8952_pmic_probe(struct i2c_client *client,
 	else
 		gflags = GPIOD_OUT_LOW;
 	gflags |= GPIOD_FLAGS_BIT_NONEXCLUSIVE;
-	gpiod = devm_gpiod_get_optional(&client->dev,
-					"max8952,en",
-					gflags);
+	gpiod = gpiod_get_optional(&client->dev,
+				   "max8952,en",
+				   gflags);
 	if (IS_ERR(gpiod))
 		return PTR_ERR(gpiod);
 	if (gpiod)
@@ -241,6 +241,8 @@ static int max8952_pmic_probe(struct i2c_client *client,
 
 	rdev = devm_regulator_register(&client->dev, &regulator, &config);
 	if (IS_ERR(rdev)) {
+		if (gpiod)
+			gpiod_put(gpiod);
 		ret = PTR_ERR(rdev);
 		dev_err(&client->dev, "regulator init failed (%d)\n", ret);
 		return ret;
-- 
2.19.1

[PATCH 05/10] regulator: max8973: Let core handle GPIO descriptor

[Linus Walleij]

Use the gpiod_get() rather than the devm_* version so that the
regulator core can handle the lifecycle of these descriptors.

Fixes: e7d2be696faa ("regulator: max8973: Pass descriptor instead of GPIO number")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/regulator/max8973-regulator.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c
index e7a58b509032..6e11324cb9cf 100644
--- a/drivers/regulator/max8973-regulator.c
+++ b/drivers/regulator/max8973-regulator.c
@@ -632,7 +632,7 @@ static int max8973_probe(struct i2c_client *client,
 	struct max8973_chip *max;
 	bool pdata_from_dt = false;
 	unsigned int chip_id;
-	struct gpio_desc *gpiod;
+	struct gpio_desc *gpiod = NULL;
 	enum gpiod_flags gflags;
 	int ret;
 
@@ -759,9 +759,9 @@ static int max8973_probe(struct i2c_client *client,
 		else
 			gflags = GPIOD_OUT_LOW;
 		gflags |= GPIOD_FLAGS_BIT_NONEXCLUSIVE;
-		gpiod = devm_gpiod_get_optional(&client->dev,
-						"maxim,enable",
-						gflags);
+		gpiod = gpiod_get_optional(&client->dev,
+					   "maxim,enable",
+					   gflags);
 		if (IS_ERR(gpiod))
 			return PTR_ERR(gpiod);
 		if (gpiod) {
@@ -798,6 +798,8 @@ static int max8973_probe(struct i2c_client *client,
 
 	ret = max8973_init_dcdc(max, pdata);
 	if (ret < 0) {
+		if (gpiod)
+			gpiod_put(gpiod);
 		dev_err(max->dev, "Max8973 Init failed, err = %d\n", ret);
 		return ret;
 	}
@@ -811,6 +813,8 @@ static int max8973_probe(struct i2c_client *client,
 	/* Register the regulators */
 	rdev = devm_regulator_register(&client->dev, &max->desc, &config);
 	if (IS_ERR(rdev)) {
+		if (gpiod)
+			gpiod_put(gpiod);
 		ret = PTR_ERR(rdev);
 		dev_err(max->dev, "regulator register failed, err %d\n", ret);
 		return ret;
-- 
2.19.1

[PATCH 06/10] gpio: Export gpiod_get_from_of_node()

[Linus Walleij]

This function already exist inside gpiolib, we were just
reluctant to make it available to the kernel at large as
the devm_* seemed to be enough for anyone.

However we found out that regulators need to do their own
lifecycle/refcounting on GPIO descriptors and explicitly
call gpiod_put() when done with a descriptor, so export
this function so we can hand the refcounting over to the
regulator core for these descriptors after retrieveal.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpiolib.h        |  6 ------
 include/linux/gpio/consumer.h | 13 +++++++++++++
 2 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 087d865286a0..bc57f0dc5953 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -201,12 +201,6 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 				  struct gpio_array *array_info,
 				  unsigned long *value_bitmap);
 
-/* This is just passed between gpiolib and devres */
-struct gpio_desc *gpiod_get_from_of_node(struct device_node *node,
-					 const char *propname, int index,
-					 enum gpiod_flags dflags,
-					 const char *label);
-
 extern struct spinlock gpio_lock;
 extern struct list_head gpio_devices;
 
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index f2f887795d43..348885f2f3d3 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -172,6 +172,10 @@ int desc_to_gpio(const struct gpio_desc *desc);
 struct device_node;
 struct fwnode_handle;
 
+struct gpio_desc *gpiod_get_from_of_node(struct device_node *node,
+					 const char *propname, int index,
+					 enum gpiod_flags dflags,
+					 const char *label);
 struct gpio_desc *devm_gpiod_get_from_of_node(struct device *dev,
 					      struct device_node *node,
 					      const char *propname, int index,
@@ -517,6 +521,15 @@ static inline int desc_to_gpio(const struct gpio_desc *desc)
 struct device_node;
 struct fwnode_handle;
 
+static inline
+struct gpio_desc *gpiod_get_from_of_node(struct device_node *node,
+					 const char *propname, int index,
+					 enum gpiod_flags dflags,
+					 const char *label)
+{
+	return ERR_PTR(-ENOSYS);
+}
+
 static inline
 struct gpio_desc *devm_gpiod_get_from_of_node(struct device *dev,
 					      struct device_node *node,
-- 
2.19.1

[PATCH 07/10] regulator: da9211: Let core handle GPIO descriptors

[Linus Walleij]

Use the gpiod_get_from_of_node() rather than the devm_*
version so that the regulator core can handle the lifecycle
of these descriptors.

This patch requires "gpio: Export gpiod_get_from_of_node()"
to be applied first.

Fixes: 11da04af0d3b ("regulator: da9211: Pass descriptors instead of GPIO numbers")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/regulator/da9211-regulator.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/da9211-regulator.c b/drivers/regulator/da9211-regulator.c
index 8f68c7a05d27..bfdead356526 100644
--- a/drivers/regulator/da9211-regulator.c
+++ b/drivers/regulator/da9211-regulator.c
@@ -293,8 +293,8 @@ static struct da9211_pdata *da9211_parse_regulators_dt(
 
 		pdata->init_data[n] = da9211_matches[i].init_data;
 		pdata->reg_node[n] = da9211_matches[i].of_node;
-		pdata->gpiod_ren[n] = devm_gpiod_get_from_of_node(dev,
-				  da9211_matches[i].of_node,
+		pdata->gpiod_ren[n] =
+			gpiod_get_from_of_node(da9211_matches[i].of_node,
 				  "enable",
 				  0,
 				  GPIOD_OUT_HIGH | GPIOD_FLAGS_BIT_NONEXCLUSIVE,
-- 
2.19.1

Re: [PATCH 07/10] regulator: da9211: Let core handle GPIO descriptors

[Charles Keepax]

On Wed, Nov 28, 2018 at 11:43:47AM +0100, Linus Walleij wrote:
> Use the gpiod_get_from_of_node() rather than the devm_*
> version so that the regulator core can handle the lifecycle
> of these descriptors.
> 
> This patch requires "gpio: Export gpiod_get_from_of_node()"
> to be applied first.
> 
> Fixes: 11da04af0d3b ("regulator: da9211: Pass descriptors instead of GPIO numbers")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/regulator/da9211-regulator.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/da9211-regulator.c b/drivers/regulator/da9211-regulator.c
> index 8f68c7a05d27..bfdead356526 100644
> --- a/drivers/regulator/da9211-regulator.c
> +++ b/drivers/regulator/da9211-regulator.c
> @@ -293,8 +293,8 @@ static struct da9211_pdata *da9211_parse_regulators_dt(
>  
>  		pdata->init_data[n] = da9211_matches[i].init_data;
>  		pdata->reg_node[n] = da9211_matches[i].of_node;
> -		pdata->gpiod_ren[n] = devm_gpiod_get_from_of_node(dev,
> -				  da9211_matches[i].of_node,
> +		pdata->gpiod_ren[n] =
> +			gpiod_get_from_of_node(da9211_matches[i].of_node,

This driver has a lot of error paths that will leak the GPIO with
this change.

Thanks,
Charles

>  				  "enable",
>  				  0,
>  				  GPIOD_OUT_HIGH | GPIOD_FLAGS_BIT_NONEXCLUSIVE,
> -- 
> 2.19.1

[PATCH 08/10] regulator: max77686: Let core handle GPIO descriptor

[Linus Walleij]

Use the gpiod_get_from_of_node() rather than the devm_*
version so that the regulator core can handle the lifecycle
of these descriptors.

Fixes: 96392c3d8ca4 ("regulator: max77686: Pass descriptor instead of GPIO number")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/regulator/max77686-regulator.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/regulator/max77686-regulator.c b/drivers/regulator/max77686-regulator.c
index f5cee1775905..236cd42002f0 100644
--- a/drivers/regulator/max77686-regulator.c
+++ b/drivers/regulator/max77686-regulator.c
@@ -255,8 +255,7 @@ static int max77686_of_parse_cb(struct device_node *np,
 	case MAX77686_BUCK8:
 	case MAX77686_BUCK9:
 	case MAX77686_LDO20 ... MAX77686_LDO22:
-		config->ena_gpiod = devm_gpiod_get_from_of_node(max77686->dev,
-				np,
+		config->ena_gpiod = gpiod_get_from_of_node(np,
 				"maxim,ena",
 				0,
 				GPIOD_OUT_HIGH | GPIOD_FLAGS_BIT_NONEXCLUSIVE,
-- 
2.19.1

Re: [PATCH 08/10] regulator: max77686: Let core handle GPIO descriptor

[Charles Keepax]

On Wed, Nov 28, 2018 at 11:43:48AM +0100, Linus Walleij wrote:
> Use the gpiod_get_from_of_node() rather than the devm_*
> version so that the regulator core can handle the lifecycle
> of these descriptors.
> 
> Fixes: 96392c3d8ca4 ("regulator: max77686: Pass descriptor instead of GPIO number")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/regulator/max77686-regulator.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/regulator/max77686-regulator.c b/drivers/regulator/max77686-regulator.c
> index f5cee1775905..236cd42002f0 100644
> --- a/drivers/regulator/max77686-regulator.c
> +++ b/drivers/regulator/max77686-regulator.c
> @@ -255,8 +255,7 @@ static int max77686_of_parse_cb(struct device_node *np,
>  	case MAX77686_BUCK8:
>  	case MAX77686_BUCK9:
>  	case MAX77686_LDO20 ... MAX77686_LDO22:
> -		config->ena_gpiod = devm_gpiod_get_from_of_node(max77686->dev,
> -				np,
> +		config->ena_gpiod = gpiod_get_from_of_node(np,

As this is inside the of_parse_cb, it probably needs some thought
on where the GPIO would need to be freed on which error paths, I
am not sure it is immediately obvious to me but I suspect it will
need to be freed in some cases.

Thanks,
Charles

Re: [PATCH 08/10] regulator: max77686: Let core handle GPIO descriptor

[Linus Walleij]

On Wed, Nov 28, 2018 at 4:24 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Wed, Nov 28, 2018 at 11:43:48AM +0100, Linus Walleij wrote:

> > @@ -255,8 +255,7 @@ static int max77686_of_parse_cb(struct device_node *np,
(...)
> > +             config->ena_gpiod = gpiod_get_from_of_node(np,
>
> As this is inside the of_parse_cb, it probably needs some thought
> on where the GPIO would need to be freed on which error paths, I
> am not sure it is immediately obvious to me but I suspect it will
> need to be freed in some cases.

I looked it over and came up with a patch making sure that if
the regulator_of_get_init_data() assigns config->ena_gpiod
it gets freed unless handled over to the regulator core.

Thanks for pointing this out, it was a tricky corner case!

Yours,
Linus Walleij

[PATCH 09/10] regulator: s5m8767: Let core handle GPIO descriptors

[Linus Walleij]

Use the gpiod_get_from_of_node() rather than the devm_*
version so that the regulator core can handle the lifecycle
of these descriptors.

Introduce an errorpath so we free any retrieved descriptors
properly.

This patch requires "gpio: Export gpiod_get_from_of_node()"
to be applied first.

Fixes: 9ae5cc75ceaa ("regulator: s5m8767: Pass descriptor instead of GPIO number")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/regulator/s5m8767.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index 219b9afda0cb..2d8f6cd4f142 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -528,7 +528,7 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 	struct device_node *pmic_np, *regulators_np, *reg_np;
 	struct sec_regulator_data *rdata;
 	struct sec_opmode_data *rmode;
-	unsigned int i, dvs_voltage_nr = 8, ret;
+	unsigned int i, j, dvs_voltage_nr = 8, ret;
 
 	pmic_np = iodev->dev->of_node;
 	if (!pmic_np) {
@@ -559,6 +559,7 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 
 	pdata->regulators = rdata;
 	pdata->opmode = rmode;
+	j = 0; /* Keeps track of populated elements for errorpath */
 	for_each_child_of_node(regulators_np, reg_np) {
 		for (i = 0; i < ARRAY_SIZE(regulators); i++)
 			if (!of_node_cmp(reg_np->name, regulators[i].name))
@@ -571,9 +572,7 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 			continue;
 		}
 
-		rdata->ext_control_gpiod = devm_gpiod_get_from_of_node(
-			&pdev->dev,
-			reg_np,
+		rdata->ext_control_gpiod = gpiod_get_from_of_node(reg_np,
 			"s5m8767,pmic-ext-control-gpios",
 			0,
 			GPIOD_OUT_HIGH | GPIOD_FLAGS_BIT_NONEXCLUSIVE,
@@ -597,6 +596,7 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 			rmode->mode = S5M8767_OPMODE_NORMAL_MODE;
 		}
 		rmode++;
+		j++;
 	}
 
 	of_node_put(regulators_np);
@@ -608,7 +608,8 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 				"s5m8767,pmic-buck2-dvs-voltage",
 				pdata->buck2_voltage, dvs_voltage_nr)) {
 			dev_err(iodev->dev, "buck2 voltages not specified\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto err_gpiod_put;
 		}
 	}
 
@@ -619,7 +620,8 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 				"s5m8767,pmic-buck3-dvs-voltage",
 				pdata->buck3_voltage, dvs_voltage_nr)) {
 			dev_err(iodev->dev, "buck3 voltages not specified\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto err_gpiod_put;
 		}
 	}
 
@@ -630,15 +632,18 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 				"s5m8767,pmic-buck4-dvs-voltage",
 				pdata->buck4_voltage, dvs_voltage_nr)) {
 			dev_err(iodev->dev, "buck4 voltages not specified\n");
-			return -EINVAL;
+			ret = -EINVAL;
+			goto err_gpiod_put;
 		}
 	}
 
 	if (pdata->buck2_gpiodvs || pdata->buck3_gpiodvs ||
 						pdata->buck4_gpiodvs) {
 		ret = s5m8767_pmic_dt_parse_dvs_gpio(iodev, pdata, pmic_np);
-		if (ret)
-			return -EINVAL;
+		if (ret) {
+			ret = -EINVAL;
+			goto err_gpiod_put;
+		}
 
 		if (of_property_read_u32(pmic_np,
 				"s5m8767,pmic-buck-default-dvs-idx",
@@ -654,8 +659,10 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 	}
 
 	ret = s5m8767_pmic_dt_parse_ds_gpio(iodev, pdata, pmic_np);
-	if (ret)
-		return -EINVAL;
+	if (ret) {
+		ret = -EINVAL;
+		goto err_gpiod_put;
+	}
 
 	if (of_get_property(pmic_np, "s5m8767,pmic-buck2-ramp-enable", NULL))
 		pdata->buck2_ramp_enable = true;
@@ -674,6 +681,14 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
 	}
 
 	return 0;
+
+err_gpiod_put:
+	while (j) {
+		gpiod_put(rdata->ext_control_gpiod);
+		rdata--;
+		j--;
+	}
+	return ret;
 }
 #else
 static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
-- 
2.19.1

Re: [PATCH 09/10] regulator: s5m8767: Let core handle GPIO descriptors

[Charles Keepax]

On Wed, Nov 28, 2018 at 11:43:49AM +0100, Linus Walleij wrote:
> Use the gpiod_get_from_of_node() rather than the devm_*
> version so that the regulator core can handle the lifecycle
> of these descriptors.
> 
> Introduce an errorpath so we free any retrieved descriptors
> properly.
> 
> This patch requires "gpio: Export gpiod_get_from_of_node()"
> to be applied first.
> 
> Fixes: 9ae5cc75ceaa ("regulator: s5m8767: Pass descriptor instead of GPIO number")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/regulator/s5m8767.c | 37 ++++++++++++++++++++++++++-----------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> @@ -674,6 +681,14 @@ static int s5m8767_pmic_dt_parse_pdata(struct platform_device *pdev,
>  	}
>  
>  	return 0;
> +
> +err_gpiod_put:
> +	while (j) {
> +		gpiod_put(rdata->ext_control_gpiod);
> +		rdata--;
> +		j--;
> +	}
> +	return ret;
>  }

These looks like it handles the error paths in
s5m8767_pmic_dt_parse_pdata, however there are still all the
error paths between the call to that function and the call to
regulator_register that need to be handled as well.

Thanks,
Charles

[PATCH 10/10] regulator: tps65090: Let core handle GPIO descriptors

[Linus Walleij]

Use the gpiod_get_from_of_node() rather than the devm_*
version so that the regulator core can handle the lifecycle
of these descriptors.

This patch requires "gpio: Export gpiod_get_from_of_node()"
to be applied first.

Fixes: 3012e81446d0 ("regulator: tps65090: Pass descriptor instead of GPIO number")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/regulator/tps65090-regulator.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
index db714d5edafc..223f6974a9f3 100644
--- a/drivers/regulator/tps65090-regulator.c
+++ b/drivers/regulator/tps65090-regulator.c
@@ -376,11 +376,11 @@ static struct tps65090_platform_data *tps65090_parse_dt_reg_data(
 				gflags = GPIOD_OUT_LOW;
 			gflags |= GPIOD_FLAGS_BIT_NONEXCLUSIVE;
 
-			rpdata->gpiod = devm_gpiod_get_from_of_node(&pdev->dev,
-								    tps65090_matches[idx].of_node,
-								    "dcdc-ext-control-gpios", 0,
-								    gflags,
-								    "tps65090");
+			rpdata->gpiod = gpiod_get_from_of_node(
+						tps65090_matches[idx].of_node,
+						"dcdc-ext-control-gpios", 0,
+						gflags,
+						"tps65090");
 			if (IS_ERR(rpdata->gpiod))
 				return ERR_CAST(rpdata->gpiod);
 			if (!rpdata->gpiod)
-- 
2.19.1

Re: [PATCH 10/10] regulator: tps65090: Let core handle GPIO descriptors

[Charles Keepax]

On Wed, Nov 28, 2018 at 11:43:50AM +0100, Linus Walleij wrote:
> Use the gpiod_get_from_of_node() rather than the devm_*
> version so that the regulator core can handle the lifecycle
> of these descriptors.
> 
> This patch requires "gpio: Export gpiod_get_from_of_node()"
> to be applied first.
> 
> Fixes: 3012e81446d0 ("regulator: tps65090: Pass descriptor instead of GPIO number")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/regulator/tps65090-regulator.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
> index db714d5edafc..223f6974a9f3 100644
> --- a/drivers/regulator/tps65090-regulator.c
> +++ b/drivers/regulator/tps65090-regulator.c
> @@ -376,11 +376,11 @@ static struct tps65090_platform_data *tps65090_parse_dt_reg_data(
>  				gflags = GPIOD_OUT_LOW;
>  			gflags |= GPIOD_FLAGS_BIT_NONEXCLUSIVE;
>  
> -			rpdata->gpiod = devm_gpiod_get_from_of_node(&pdev->dev,
> -								    tps65090_matches[idx].of_node,
> -								    "dcdc-ext-control-gpios", 0,
> -								    gflags,
> -								    "tps65090");
> +			rpdata->gpiod = gpiod_get_from_of_node(
> +						tps65090_matches[idx].of_node,
> +						"dcdc-ext-control-gpios", 0,
> +						gflags,
> +						"tps65090");
>  			if (IS_ERR(rpdata->gpiod))
>  				return ERR_CAST(rpdata->gpiod);
>  			if (!rpdata->gpiod)

This one needs some handling to avoid leaking the gpio too.

Thanks,
Charles

Re: [PATCH 00/10] Regulator ena_gpiod fixups

[Charles Keepax]

On Wed, Nov 28, 2018 at 11:43:40AM +0100, Linus Walleij wrote:
>  drivers/gpio/gpiolib.h                 |  6 -----
>  drivers/regulator/da9211-regulator.c   |  4 +--
>  drivers/regulator/fixed.c              |  4 ++-
>  drivers/regulator/lm363x-regulator.c   |  8 ++++--
>  drivers/regulator/lp8788-ldo.c         |  4 ++-
>  drivers/regulator/max77686-regulator.c |  3 +--
>  drivers/regulator/max8952.c            |  8 +++---
>  drivers/regulator/max8973-regulator.c  | 12 ++++++---
>  drivers/regulator/s5m8767.c            | 37 ++++++++++++++++++--------
>  drivers/regulator/tps65090-regulator.c | 10 +++----
>  include/linux/gpio/consumer.h          | 13 +++++++++
>  11 files changed, 72 insertions(+), 37 deletions(-)

It looks like the patches are assuming the regulator core,
doesn't free the GPIO on an error, however that is not true in
all cases. If only a single regulator has requested the GPIO then
all the error paths after the call to regulator_ena_gpio_request
in regulator_register will free the GPIO. Although this is not the
case if more than one regulator has requested the GPIO.

Thanks,
charles

Re: [PATCH 00/10] Regulator ena_gpiod fixups

[Linus Walleij]

On Wed, Nov 28, 2018 at 4:22 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:

> It looks like the patches are assuming the regulator core,
> doesn't free the GPIO on an error, however that is not true in
> all cases. If only a single regulator has requested the GPIO then
> all the error paths after the call to regulator_ena_gpio_request
> in regulator_register will free the GPIO.

I guess part of it is that I should make sure not to gpiod_put()
if the [devm_]regulator_register() fails, I will go over the
series with that in mind!

Essentially the semantic is that the  [devm_]regulator_register()
call will immediately take ownership of the descriptor
and place it in the regulator core.

I'll check!

> Although this is not the
> case if more than one regulator has requested the GPIO.

This should be fine since the regulator core refcounts it,
when all the other regulators drops it, gpiod_put() will be
called as the refcount goes down to 0.

Yours,
Linus Walleij

Re: [PATCH 00/10] Regulator ena_gpiod fixups

[Mark Brown]

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

On Thu, Nov 29, 2018 at 04:52:04PM +0100, Linus Walleij wrote:

> Essentially the semantic is that the  [devm_]regulator_register()
> call will immediately take ownership of the descriptor
> and place it in the regulator core.

I think that's going to be the easiest thing to work with, it's more
pain in the regulator core but simpler for all the users which is
probably a good balance.

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

Re: [PATCH 00/10] Regulator ena_gpiod fixups

[Bartosz Golaszewski]

śr., 28 lis 2018 o 11:43 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> We noticed a refcounting fight between the kernel device
> core devm_* and the regulator core refcounting, pertaining
> to enable GPIOd:s that may be shared between multiple
> regulators.
>
> Fix this with a series that hand it all over to the
> regulator core and remove any devm_* stuff pertaining
> to these GPIOs.
>
> This includes a patch to gpiolib to make gpiod_get_from_node()
> available. Just go ahead and apply this to the regulator
> tree.
>
> If these need to go for fixes or not is up to the
> regulator maintainer, but all commits have a proper
> Fixes: tag in case they would. I have noted in the
> four last commits that the gpiolib patch is a
> prerequisite.
>
> Linus Walleij (10):
>   regulator: fixed: Let core handle GPIO descriptor
>   regulator: lm363x: Let core handle GPIO descriptor
>   regulator: lp8788-ldo: Let core handle GPIO descriptor
>   regulator: max8952: Let core handle GPIO descriptor
>   regulator: max8973: Let core handle GPIO descriptor
>   gpio: Export gpiod_get_from_of_node()
>   regulator: da9211: Let core handle GPIO descriptors
>   regulator: max77686: Let core handle GPIO descriptor
>   regulator: s5m8767: Let core handle GPIO descriptors
>   regulator: tps65090: Let core handle GPIO descriptors
>
>  drivers/gpio/gpiolib.h                 |  6 -----
>  drivers/regulator/da9211-regulator.c   |  4 +--
>  drivers/regulator/fixed.c              |  4 ++-
>  drivers/regulator/lm363x-regulator.c   |  8 ++++--
>  drivers/regulator/lp8788-ldo.c         |  4 ++-
>  drivers/regulator/max77686-regulator.c |  3 +--
>  drivers/regulator/max8952.c            |  8 +++---
>  drivers/regulator/max8973-regulator.c  | 12 ++++++---
>  drivers/regulator/s5m8767.c            | 37 ++++++++++++++++++--------
>  drivers/regulator/tps65090-regulator.c | 10 +++----
>  include/linux/gpio/consumer.h          | 13 +++++++++
>  11 files changed, 72 insertions(+), 37 deletions(-)
>
> --
> 2.19.1
>

I'm wondering if instead of using the non-devm variants of
gpiod_get_*() routines, we shouldn't provide helpers in the regulator
framework that would be named accordingly, for example:
regulator_gpiod_get_optional() etc. even if all they do is call the
relevant gpiolib function. Those helpers could then be documented as
passing the control over GPIO lines over to the regulator subsystem.

The reason for that is that most driver developers will automatically
use devm functions whenever available and having a single non-devm
function without any comment used in a driver normally using devres
looks like a bug. Expect people sending "fixes" in a couple months.

Bart

Re: [PATCH 00/10] Regulator ena_gpiod fixups

[Mark Brown]

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

On Thu, Nov 29, 2018 at 07:38:20PM +0100, Bartosz Golaszewski wrote:

> I'm wondering if instead of using the non-devm variants of
> gpiod_get_*() routines, we shouldn't provide helpers in the regulator
> framework that would be named accordingly, for example:
> regulator_gpiod_get_optional() etc. even if all they do is call the
> relevant gpiolib function. Those helpers could then be documented as
> passing the control over GPIO lines over to the regulator subsystem.

> The reason for that is that most driver developers will automatically
> use devm functions whenever available and having a single non-devm
> function without any comment used in a driver normally using devres
> looks like a bug. Expect people sending "fixes" in a couple months.

I predict that people would then immediately start demanding devm_
variants of that function...

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

Re: [PATCH 00/10] Regulator ena_gpiod fixups

[Bartosz Golaszewski]

czw., 29 lis 2018 o 20:01 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Thu, Nov 29, 2018 at 07:38:20PM +0100, Bartosz Golaszewski wrote:
>
> > I'm wondering if instead of using the non-devm variants of
> > gpiod_get_*() routines, we shouldn't provide helpers in the regulator
> > framework that would be named accordingly, for example:
> > regulator_gpiod_get_optional() etc. even if all they do is call the
> > relevant gpiolib function. Those helpers could then be documented as
> > passing the control over GPIO lines over to the regulator subsystem.
>
> > The reason for that is that most driver developers will automatically
> > use devm functions whenever available and having a single non-devm
> > function without any comment used in a driver normally using devres
> > looks like a bug. Expect people sending "fixes" in a couple months.
>
> I predict that people would then immediately start demanding devm_
> variants of that function...

At least we could document it in the code.

If I wouldn't know about the reason for not using devm and saw a stray
gpiod_get() without a corresponding put() I'd probably send a patch to
fix it, but if I saw something like regulator_gpiod_get(), I'd look at
what this routine does.

Matter of taste I guess, but I'd prefer the latter. At the very least
we could add a comment to each call saying that the regulator
framework will take care of that resource.

Bart

Re: [PATCH 00/10] Regulator ena_gpiod fixups

[Linus Walleij]

On Fri, Nov 30, 2018 at 9:35 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:

> At least we could document it in the code.

I've put some comments in my patch set, I will check if I can add some
more in the less-than-obvious spots.

Yours,
Linus Walleij