[PATCH 00/13 v2] Regulator ena_gpiod fixups

[PATCH 00/13 v2] Regulator ena_gpiod fixups

[Linus Walleij]

Here is a second iteration of these fixups after thinking
over Charles Keepax excellent comments on the first series
of fixes.

To make sure GPIO descriptors are never left dangling
(as far as I can tell!) I use this stepwise approach:

1. Fix the regulator_register() in the core to guarantee
   that after calling this with a valid GPIO descriptor
   in ena_gpiod it will be gpiod_put() if there is any
   problem.

2. Fix up simple descriptor consumers to just gpiod_get()
   and let the core take over the descriptor. Only handle
   the errorpath up to this point.

3. Export gpiod_get_from_of_node() and let max77686
   obtain a GPIO descriptor from the device tree without
   any devres make-up in the DT parsing callback.

4. Invent devm_gpiod_unhinge() that will remove the devres
   monitoring of a devm_* allocated GPIO descriptor
   right before handling it over to the regulator core,
   and use this in the otherwise hairy da9211,
   s5m8767, tps65090 and s2mps11 drivers.

Linus Walleij (13):
  regulator: core: Track dangling GPIO descriptors
  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: max77686: Let core handle GPIO descriptor
  gpio: Add devm_gpiod_unhinge()
  regulator: da9211: Hand over GPIO to regulator core
  regulator: s5m8767: Hand over GPIO to regulator core
  regulator: tps65090: Hand over GPIO to regulator core
  regulator: s2mps11: Hand over GPIO to regulator core

 Documentation/driver-model/devres.txt  |  1 +
 drivers/gpio/gpiolib-devres.c          | 17 ++++++++
 drivers/gpio/gpiolib.h                 |  6 ---
 drivers/regulator/core.c               | 55 +++++++++++++++++++++-----
 drivers/regulator/da9211-regulator.c   |  6 +++
 drivers/regulator/fixed.c              |  6 ++-
 drivers/regulator/lm363x-regulator.c   |  8 +++-
 drivers/regulator/lp8788-ldo.c         |  8 +++-
 drivers/regulator/max77686-regulator.c | 14 ++++---
 drivers/regulator/max8952.c            | 10 +++--
 drivers/regulator/max8973-regulator.c  | 23 +++++++----
 drivers/regulator/s2mps11.c            |  7 +++-
 drivers/regulator/s5m8767.c            |  9 ++++-
 drivers/regulator/tps65090-regulator.c |  6 +++
 include/linux/gpio/consumer.h          | 23 +++++++++++
 15 files changed, 161 insertions(+), 38 deletions(-)

-- 
2.19.1

[PATCH 01/13 v2] regulator: core: Track dangling GPIO descriptors

[Linus Walleij]

If a GPIO descriptor is passed to the regulator_register()
function inside the config->ena_gpiod callers must be
sure that once they call this API the regulator core
owns that descriptor and will make sure to issue
gpiod_put() on it, no matter whether the call is
successful or not.

For device tree regulators, the regulator core will
automatically set up regulator init data from the device
tree when registering a regulator by calling
regulator_of_get_init_data() which in turn calls down to
the regulator driver's .of_parse_cb() callback.
This callback (in drivers such as for max77686) may also
choose to fill in the config->ena_gpiod field with a GPIO
descriptor.

Harden the errorpath of regulator_register() to
properly gpiod_put() any passed in cfg->ena_gpiod
or any gpiod coming from the device tree on any type
of error.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- New patch, just numbering v2 to keep it in line with
  the patch set it came out of.
---
 drivers/regulator/core.c | 55 ++++++++++++++++++++++++++++++++--------
 1 file changed, 45 insertions(+), 10 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 757619878068..79cb090ff22f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4868,21 +4868,33 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	struct regulator_config *config = NULL;
 	static atomic_t regulator_no = ATOMIC_INIT(-1);
 	struct regulator_dev *rdev;
+	bool dangling_cfg_gpiod = false;
+	bool dangling_of_gpiod = false;
 	struct device *dev;
 	int ret, i;
 
-	if (regulator_desc == NULL || cfg == NULL)
+	if (cfg == NULL)
 		return ERR_PTR(-EINVAL);
+	if (cfg->ena_gpiod)
+		dangling_cfg_gpiod = true;
+	if (regulator_desc == NULL) {
+		ret = -EINVAL;
+		goto rinse;
+	}
 
 	dev = cfg->dev;
 	WARN_ON(!dev);
 
-	if (regulator_desc->name == NULL || regulator_desc->ops == NULL)
-		return ERR_PTR(-EINVAL);
+	if (regulator_desc->name == NULL || regulator_desc->ops == NULL) {
+		ret = -EINVAL;
+		goto rinse;
+	}
 
 	if (regulator_desc->type != REGULATOR_VOLTAGE &&
-	    regulator_desc->type != REGULATOR_CURRENT)
-		return ERR_PTR(-EINVAL);
+	    regulator_desc->type != REGULATOR_CURRENT) {
+		ret = -EINVAL;
+		goto rinse;
+	}
 
 	/* Only one of each should be implemented */
 	WARN_ON(regulator_desc->ops->get_voltage &&
@@ -4893,16 +4905,20 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	/* If we're using selectors we must implement list_voltage. */
 	if (regulator_desc->ops->get_voltage_sel &&
 	    !regulator_desc->ops->list_voltage) {
-		return ERR_PTR(-EINVAL);
+		ret = -EINVAL;
+		goto rinse;
 	}
 	if (regulator_desc->ops->set_voltage_sel &&
 	    !regulator_desc->ops->list_voltage) {
-		return ERR_PTR(-EINVAL);
+		ret = -EINVAL;
+		goto rinse;
 	}
 
 	rdev = kzalloc(sizeof(struct regulator_dev), GFP_KERNEL);
-	if (rdev == NULL)
-		return ERR_PTR(-ENOMEM);
+	if (rdev == NULL) {
+		ret = -ENOMEM;
+		goto rinse;
+	}
 
 	/*
 	 * Duplicate the config so the driver could override it after
@@ -4911,11 +4927,22 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	config = kmemdup(cfg, sizeof(*cfg), GFP_KERNEL);
 	if (config == NULL) {
 		kfree(rdev);
-		return ERR_PTR(-ENOMEM);
+		ret = -ENOMEM;
+		goto rinse;
 	}
 
 	init_data = regulator_of_get_init_data(dev, regulator_desc, config,
 					       &rdev->dev.of_node);
+	/*
+	 * We need to keep track of any GPIO descriptor coming from the
+	 * device tree until we have handled it over to the core. If the
+	 * config that was passed in to this function DOES NOT contain
+	 * a descriptor, and the config after this call DOES contain
+	 * a descriptor, we definately got one from parsing the device
+	 * tree.
+	 */
+	if (!cfg->ena_gpiod && config->ena_gpiod)
+		dangling_of_gpiod = true;
 	if (!init_data) {
 		init_data = config->init_data;
 		rdev->dev.of_node = of_node_get(config->of_node);
@@ -4954,6 +4981,9 @@ regulator_register(const struct regulator_desc *regulator_desc,
 				 config->ena_gpio, ret);
 			goto clean;
 		}
+		/* The regulator core took over the GPIO descriptor */
+		dangling_cfg_gpiod = false;
+		dangling_of_gpiod = false;
 	}
 
 	/* register with sysfs */
@@ -5039,8 +5069,13 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	regulator_ena_gpio_free(rdev);
 	mutex_unlock(&regulator_list_mutex);
 clean:
+	if (dangling_of_gpiod)
+		gpiod_put(config->ena_gpiod);
 	kfree(rdev);
 	kfree(config);
+rinse:
+	if (dangling_cfg_gpiod)
+		gpiod_put(cfg->ena_gpiod);
 	return ERR_PTR(ret);
 }
 EXPORT_SYMBOL_GPL(regulator_register);
-- 
2.19.1

[PATCH 02/13 v2] 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>
---
ChangeLog v1->v2:
- Drop the conditional gpiod_put() on the errorpath: the
  regulator core will take care of that once
  devm_regulator_register() gets called.
- Put a comment in the code so maintainers knows not to
  use managed resources (devm*)
---
 drivers/regulator/fixed.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index ccc29038f19a..9abdb9130766 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -183,7 +183,11 @@ 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);
+	/*
+	 * Do not use devm* here: the regulator core takes over the
+	 * lifecycle management of the GPIO descriptor.
+	 */
+	cfg.ena_gpiod = gpiod_get_optional(&pdev->dev, NULL, gflags);
 	if (IS_ERR(cfg.ena_gpiod))
 		return PTR_ERR(cfg.ena_gpiod);
 
-- 
2.19.1

[PATCH 03/13 v2] 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>
---
ChangeLog v1->v2:
- Drop the gpiod_put() on the errorpath after devm_regulator_register()
  as this will be handled by the regulator core.
- Put a comment in the code so maintainers knows not to
  use managed resources (devm*)
---
 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..8c0e8419c43f 100644
--- a/drivers/regulator/lm363x-regulator.c
+++ b/drivers/regulator/lm363x-regulator.c
@@ -224,13 +224,15 @@ static struct gpio_desc *lm363x_regulator_of_get_enable_gpio(struct device *dev,
 	/*
 	 * Check LCM_EN1/2_GPIO is configured.
 	 * Those pins are used for enabling VPOS/VNEG LDOs.
+	 * Do not use devm* here: the regulator core takes over the
+	 * lifecycle management of the GPIO descriptor.
 	 */
 	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 +265,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;
 		}
-- 
2.19.1

[PATCH 04/13 v2] 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>
---
ChangeLog v1->v2:
- Drop the gpiod_put() on the errorpath after devm_regulator_register()
  as this will be handled by the regulator core.
- Put a comment in the code so maintainers knows not to
  use managed resources (devm*)
---
 drivers/regulator/lp8788-ldo.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/lp8788-ldo.c b/drivers/regulator/lp8788-ldo.c
index 553b4790050f..2ee22e7ea675 100644
--- a/drivers/regulator/lp8788-ldo.c
+++ b/drivers/regulator/lp8788-ldo.c
@@ -501,8 +501,12 @@ static int lp8788_config_ldo_enable_mode(struct platform_device *pdev,
 		return 0;
 	}
 
-	/* FIXME: check default mode for GPIO here: high or low? */
-	ldo->ena_gpiod = devm_gpiod_get_index_optional(&pdev->dev,
+	/*
+	 * Do not use devm* here: the regulator core takes over the
+	 * lifecycle management of the GPIO descriptor.
+	 * FIXME: check default mode for GPIO here: high or low?
+	 */
+	ldo->ena_gpiod = gpiod_get_index_optional(&pdev->dev,
 					       "enable",
 					       enable_id,
 					       GPIOD_OUT_HIGH |
-- 
2.19.1

[PATCH 05/13 v2] 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>
---
ChangeLog v1->v2:
- Drop the gpiod_put() on the errorpath after devm_regulator_register()
  as this will be handled by the regulator core.
- Put a comment in the code so maintainers knows not to
  use managed resources (devm*)
---
 drivers/regulator/max8952.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/max8952.c b/drivers/regulator/max8952.c
index 6c39fff73b8a..cf2a2912cb1b 100644
--- a/drivers/regulator/max8952.c
+++ b/drivers/regulator/max8952.c
@@ -231,9 +231,13 @@ 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);
+	/*
+	 * Do not use devm* here: the regulator core takes over the
+	 * lifecycle management of the GPIO descriptor.
+	 */
+	gpiod = gpiod_get_optional(&client->dev,
+				   "max8952,en",
+				   gflags);
 	if (IS_ERR(gpiod))
 		return PTR_ERR(gpiod);
 	if (gpiod)
-- 
2.19.1

[PATCH 06/13 v2] 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>
---
ChangeLog v1->v2:
- Drop the gpiod_put() on the errorpath after devm_regulator_register()
  as this will be handled by the regulator core.
- Fix the second case of devm_gpiod_get()
- Put a comment in the code so maintainers knows not to
  use managed resources (devm*)
---
 drivers/regulator/max8973-regulator.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/drivers/regulator/max8973-regulator.c b/drivers/regulator/max8973-regulator.c
index e7a58b509032..ef8f4789a517 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,13 @@ 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);
+		/*
+		 * Do not use devm* here: the regulator core takes over the
+		 * lifecycle management of the GPIO descriptor.
+		 */
+		gpiod = gpiod_get_optional(&client->dev,
+					   "maxim,enable",
+					   gflags);
 		if (IS_ERR(gpiod))
 			return PTR_ERR(gpiod);
 		if (gpiod) {
@@ -775,10 +779,13 @@ static int max8973_probe(struct i2c_client *client,
 		/*
 		 * We do not let the core switch this regulator on/off,
 		 * we just leave it on.
+		 *
+		 * Do not use devm* here: the regulator core takes over the
+		 * lifecycle management of the GPIO descriptor.
 		 */
-		gpiod = devm_gpiod_get_optional(&client->dev,
-						"maxim,enable",
-						GPIOD_OUT_HIGH);
+		gpiod = gpiod_get_optional(&client->dev,
+					   "maxim,enable",
+					   GPIOD_OUT_HIGH);
 		if (IS_ERR(gpiod))
 			return PTR_ERR(gpiod);
 		if (gpiod)
@@ -798,6 +805,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;
 	}
-- 
2.19.1

Re: [PATCH 06/13 v2] regulator: max8973: Let core handle GPIO descriptor

[Charles Keepax]

On Sat, Dec 01, 2018 at 04:41:44PM +0100, Linus Walleij wrote:
> 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>
> ---
> ChangeLog v1->v2:
> - Drop the gpiod_put() on the errorpath after devm_regulator_register()
>   as this will be handled by the regulator core.
> - Fix the second case of devm_gpiod_get()
> - Put a comment in the code so maintainers knows not to
>   use managed resources (devm*)
> ---
> @@ -759,9 +759,13 @@ 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);
> +		/*
> +		 * Do not use devm* here: the regulator core takes over the
> +		 * lifecycle management of the GPIO descriptor.
> +		 */
> +		gpiod = gpiod_get_optional(&client->dev,
> +					   "maxim,enable",
> +					   gflags);
>  		if (IS_ERR(gpiod))
>  			return PTR_ERR(gpiod);
>  		if (gpiod) {
> @@ -775,10 +779,13 @@ static int max8973_probe(struct i2c_client *client,
>  		/*
>  		 * We do not let the core switch this regulator on/off,
>  		 * we just leave it on.
> +		 *
> +		 * Do not use devm* here: the regulator core takes over the
> +		 * lifecycle management of the GPIO descriptor.
>  		 */
> -		gpiod = devm_gpiod_get_optional(&client->dev,
> -						"maxim,enable",
> -						GPIOD_OUT_HIGH);
> +		gpiod = gpiod_get_optional(&client->dev,
> +					   "maxim,enable",
> +					   GPIOD_OUT_HIGH);

Need to be careful here this path actually never passes the GPIO
to the regulator core. I suspect you want to leave this one as a
devm_

>  		if (IS_ERR(gpiod))
>  			return PTR_ERR(gpiod);
>  		if (gpiod)
> @@ -798,6 +805,8 @@ static int max8973_probe(struct i2c_client *client,
>  
>  	ret = max8973_init_dcdc(max, pdata);
>  	if (ret < 0) {
> +		if (gpiod)
> +			gpiod_put(gpiod);

And make this use config.ena_gpiod instead.

Thanks,
Charles

[PATCH 07/13 v2] 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>
---
ChangeLog v1->v2:
- Resending
---
 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 08/13 v2] 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.

Fix up the errorpath so that we free this descriptor if
an error occurs in the callback. Rely on the regulator
core to deal with it after this point: a previous patch
fixed up the regulator core to properly dispose any
GPIO descriptors once you call regulator_register().

Fixes: 96392c3d8ca4 ("regulator: max77686: Pass descriptor instead of GPIO number")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Handle the errorpath (hopefully) correct, back out of
  regulator registration by freeing the descriptors of all
  not yet registered regulators.
---
 drivers/regulator/max77686-regulator.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/regulator/max77686-regulator.c b/drivers/regulator/max77686-regulator.c
index f5cee1775905..8020eb57374a 100644
--- a/drivers/regulator/max77686-regulator.c
+++ b/drivers/regulator/max77686-regulator.c
@@ -250,13 +250,13 @@ static int max77686_of_parse_cb(struct device_node *np,
 		struct regulator_config *config)
 {
 	struct max77686_data *max77686 = config->driver_data;
+	int ret;
 
 	switch (desc->id) {
 	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,
@@ -271,9 +271,13 @@ static int max77686_of_parse_cb(struct device_node *np,
 	if (config->ena_gpiod) {
 		set_bit(desc->id, max77686->gpio_enabled);
 
-		return regmap_update_bits(config->regmap, desc->enable_reg,
-					  desc->enable_mask,
-					  MAX77686_GPIO_CONTROL);
+		ret = regmap_update_bits(config->regmap, desc->enable_reg,
+					 desc->enable_mask,
+					 MAX77686_GPIO_CONTROL);
+		if (ret) {
+			gpiod_put(config->ena_gpiod);
+			config->ena_gpiod = NULL;
+		}
 	}
 
 	return 0;
-- 
2.19.1

[PATCH 09/13 v2] gpio: Add devm_gpiod_unhinge()

[Linus Walleij]

This adds a function named devm_gpiod_unhinge() that removes
the resource management from a GPIO descriptor.

I am not sure if this is the best anglosaxon name for the
function, no other managed resources have an equivalent
currently, but I chose "unhinge" as the closest intuitive
thing I could imagine that fits Rusty Russell's API design
criterions "the obvious use is the correct one" and
"the name tells you how to use it".

The idea came out of a remark from Mark Brown that it should
be possible to handle over management of a resource from
devres to the regulator core, and indeed we can do that.

Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- New patch to be able to hand over GPIO descriptors to
  the regulator core.
- Mark: feel free to apply this to the regulator tree with
  the regulator tree with the rest.
---
 Documentation/driver-model/devres.txt |  1 +
 drivers/gpio/gpiolib-devres.c         | 17 +++++++++++++++++
 include/linux/gpio/consumer.h         | 10 ++++++++++
 3 files changed, 28 insertions(+)

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 43681ca0837f..fc4cc24dfb97 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -254,6 +254,7 @@ GPIO
   devm_gpiod_get_index_optional()
   devm_gpiod_get_optional()
   devm_gpiod_put()
+  devm_gpiod_unhinge()
   devm_gpiochip_add_data()
   devm_gpiochip_remove()
   devm_gpio_request()
diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
index 01959369360b..5864e758d7f2 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -320,6 +320,23 @@ void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
 }
 EXPORT_SYMBOL(devm_gpiod_put);
 
+/**
+ * devm_gpiod_unhinge - Remove resource management from a gpio descriptor
+ * @dev:	GPIO consumer
+ * @desc:	GPIO descriptor to remove resource management from
+ *
+ * Remove resource management from a GPIO descriptor. This is needed when
+ * you want to hand over lifecycle management of a descriptor to another
+ * mechanism.
+ */
+
+void devm_gpiod_unhinge(struct device *dev, struct gpio_desc *desc)
+{
+	WARN_ON(devres_destroy(dev, devm_gpiod_release,
+			       devm_gpiod_match, desc));
+}
+EXPORT_SYMBOL(devm_gpiod_unhinge);
+
 /**
  * devm_gpiod_put_array - Resource-managed gpiod_put_array()
  * @dev:	GPIO consumer
diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
index 348885f2f3d3..8aebcf822082 100644
--- a/include/linux/gpio/consumer.h
+++ b/include/linux/gpio/consumer.h
@@ -104,6 +104,7 @@ struct gpio_descs *__must_check
 devm_gpiod_get_array_optional(struct device *dev, const char *con_id,
 			      enum gpiod_flags flags);
 void devm_gpiod_put(struct device *dev, struct gpio_desc *desc);
+void devm_gpiod_unhinge(struct device *dev, struct gpio_desc *desc);
 void devm_gpiod_put_array(struct device *dev, struct gpio_descs *descs);
 
 int gpiod_get_direction(struct gpio_desc *desc);
@@ -249,6 +250,15 @@ static inline void gpiod_put(struct gpio_desc *desc)
 	WARN_ON(1);
 }
 
+static inline void devm_gpiod_unhinge(struct device *dev,
+				      struct gpio_desc *desc)
+{
+	might_sleep();
+
+	/* GPIO can never have been requested */
+	WARN_ON(1);
+}
+
 static inline void gpiod_put_array(struct gpio_descs *descs)
 {
 	might_sleep();
-- 
2.19.1

Re: [PATCH 09/13 v2] gpio: Add devm_gpiod_unhinge()

[Bartosz Golaszewski]

sob., 1 gru 2018 o 16:53 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> This adds a function named devm_gpiod_unhinge() that removes
> the resource management from a GPIO descriptor.
>
> I am not sure if this is the best anglosaxon name for the
> function, no other managed resources have an equivalent
> currently, but I chose "unhinge" as the closest intuitive
> thing I could imagine that fits Rusty Russell's API design
> criterions "the obvious use is the correct one" and
> "the name tells you how to use it".
>
> The idea came out of a remark from Mark Brown that it should
> be possible to handle over management of a resource from
> devres to the regulator core, and indeed we can do that.
>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - New patch to be able to hand over GPIO descriptors to
>   the regulator core.
> - Mark: feel free to apply this to the regulator tree with
>   the regulator tree with the rest.
> ---
>  Documentation/driver-model/devres.txt |  1 +
>  drivers/gpio/gpiolib-devres.c         | 17 +++++++++++++++++
>  include/linux/gpio/consumer.h         | 10 ++++++++++
>  3 files changed, 28 insertions(+)
>
> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
> index 43681ca0837f..fc4cc24dfb97 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -254,6 +254,7 @@ GPIO
>    devm_gpiod_get_index_optional()
>    devm_gpiod_get_optional()
>    devm_gpiod_put()
> +  devm_gpiod_unhinge()
>    devm_gpiochip_add_data()
>    devm_gpiochip_remove()
>    devm_gpio_request()
> diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
> index 01959369360b..5864e758d7f2 100644
> --- a/drivers/gpio/gpiolib-devres.c
> +++ b/drivers/gpio/gpiolib-devres.c
> @@ -320,6 +320,23 @@ void devm_gpiod_put(struct device *dev, struct gpio_desc *desc)
>  }
>  EXPORT_SYMBOL(devm_gpiod_put);
>
> +/**
> + * devm_gpiod_unhinge - Remove resource management from a gpio descriptor
> + * @dev:       GPIO consumer
> + * @desc:      GPIO descriptor to remove resource management from
> + *
> + * Remove resource management from a GPIO descriptor. This is needed when
> + * you want to hand over lifecycle management of a descriptor to another
> + * mechanism.
> + */
> +
> +void devm_gpiod_unhinge(struct device *dev, struct gpio_desc *desc)
> +{
> +       WARN_ON(devres_destroy(dev, devm_gpiod_release,
> +                              devm_gpiod_match, desc));
> +}
> +EXPORT_SYMBOL(devm_gpiod_unhinge);
> +
>  /**
>   * devm_gpiod_put_array - Resource-managed gpiod_put_array()
>   * @dev:       GPIO consumer
> diff --git a/include/linux/gpio/consumer.h b/include/linux/gpio/consumer.h
> index 348885f2f3d3..8aebcf822082 100644
> --- a/include/linux/gpio/consumer.h
> +++ b/include/linux/gpio/consumer.h
> @@ -104,6 +104,7 @@ struct gpio_descs *__must_check
>  devm_gpiod_get_array_optional(struct device *dev, const char *con_id,
>                               enum gpiod_flags flags);
>  void devm_gpiod_put(struct device *dev, struct gpio_desc *desc);
> +void devm_gpiod_unhinge(struct device *dev, struct gpio_desc *desc);
>  void devm_gpiod_put_array(struct device *dev, struct gpio_descs *descs);
>
>  int gpiod_get_direction(struct gpio_desc *desc);
> @@ -249,6 +250,15 @@ static inline void gpiod_put(struct gpio_desc *desc)
>         WARN_ON(1);
>  }
>
> +static inline void devm_gpiod_unhinge(struct device *dev,
> +                                     struct gpio_desc *desc)
> +{
> +       might_sleep();
> +
> +       /* GPIO can never have been requested */
> +       WARN_ON(1);
> +}
> +
>  static inline void gpiod_put_array(struct gpio_descs *descs)
>  {
>         might_sleep();
> --
> 2.19.1
>

Yes! This is much better than simply using non-devm variants of gpiod_get().

Reviewed-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

[PATCH 10/13 v2] regulator: da9211: Hand over GPIO to regulator core

[Linus Walleij]

The GPIO descriptors used by the DA9211 driver are retrieved
during probe() and it is really helpful to have those under
devres management because of all the errorpaths in the
intialization.

Using the new dev_gpiod_unhinge() call we can remove the
devres management of the descriptor right before handing
it over to the regulators core.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- New patch handling this drivers GPIO descriptors properly.
---
 drivers/regulator/da9211-regulator.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/regulator/da9211-regulator.c b/drivers/regulator/da9211-regulator.c
index 8f68c7a05d27..109ee12d4362 100644
--- a/drivers/regulator/da9211-regulator.c
+++ b/drivers/regulator/da9211-regulator.c
@@ -389,6 +389,12 @@ static int da9211_regulator_init(struct da9211 *chip)
 		else
 			config.ena_gpiod = NULL;
 
+		/*
+		 * Hand the GPIO descriptor management over to the regulator
+		 * core, remove it from GPIO devres management.
+		 */
+		if (config.ena_gpiod)
+			devm_gpiod_unhinge(chip->dev, config.ena_gpiod);
 		chip->rdev[i] = devm_regulator_register(chip->dev,
 			&da9211_regulators[i], &config);
 		if (IS_ERR(chip->rdev[i])) {
-- 
2.19.1

[PATCH 11/13 v2] regulator: s5m8767: Hand over GPIO to regulator core

[Linus Walleij]

The GPIO descriptors used by the S5M8767 driver are retrieved
during probe() and it is really helpful to have those under
devres management because of all the errorpaths in the
intialization.

Using the new dev_gpiod_unhinge() call we can remove the
devres management of the descriptor right before handing
it over to the regulators core.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- New patch handling this drivers GPIO descriptors properly.
---
 drivers/regulator/s5m8767.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/s5m8767.c b/drivers/regulator/s5m8767.c
index 219b9afda0cb..d1edd8c2bcec 100644
--- a/drivers/regulator/s5m8767.c
+++ b/drivers/regulator/s5m8767.c
@@ -956,10 +956,17 @@ static int s5m8767_pmic_probe(struct platform_device *pdev)
 		config.regmap = iodev->regmap_pmic;
 		config.of_node = pdata->regulators[i].reg_node;
 		config.ena_gpiod = NULL;
-		if (pdata->regulators[i].ext_control_gpiod)
+		if (pdata->regulators[i].ext_control_gpiod) {
+			/* Assigns config.ena_gpiod */
 			s5m8767_regulator_config_ext_control(s5m8767,
 					&pdata->regulators[i], &config);
 
+			/*
+			 * Hand the GPIO descriptor management over to the
+			 * regulator core, remove it from devres management.
+			 */
+			devm_gpiod_unhinge(s5m8767->dev, config.ena_gpiod);
+		}
 		rdev = devm_regulator_register(&pdev->dev, &regulators[id],
 						  &config);
 		if (IS_ERR(rdev)) {
-- 
2.19.1

[PATCH 12/13 v2] regulator: tps65090: Hand over GPIO to regulator core

[Linus Walleij]

The GPIO descriptors used by the TPS65090 driver are retrieved
during probe() and it is really helpful to have those under
devres management because of all the errorpaths in the
intialization.

Using the new dev_gpiod_unhinge() call we can remove the
devres management of the descriptor right before handing
it over to the regulators core.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- New patch handling this drivers GPIO descriptors properly.
---
 drivers/regulator/tps65090-regulator.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/regulator/tps65090-regulator.c b/drivers/regulator/tps65090-regulator.c
index db714d5edafc..0614551796a1 100644
--- a/drivers/regulator/tps65090-regulator.c
+++ b/drivers/regulator/tps65090-regulator.c
@@ -480,6 +480,12 @@ static int tps65090_regulator_probe(struct platform_device *pdev)
 		else
 			config.of_node = NULL;
 
+		/*
+		 * Hand the GPIO descriptor management over to the regulator
+		 * core, remove it from devres management.
+		 */
+		if (config.ena_gpiod)
+			devm_gpiod_unhinge(&pdev->dev, config.ena_gpiod);
 		rdev = devm_regulator_register(&pdev->dev, ri->desc, &config);
 		if (IS_ERR(rdev)) {
 			dev_err(&pdev->dev, "failed to register regulator %s\n",
-- 
2.19.1

[PATCH 13/13 v2] regulator: s2mps11: Hand over GPIO to regulator core

[Linus Walleij]

The GPIO descriptors used by the S2MPS11 driver are retrieved
during probe() and it is really helpful to have those under
devres management because of all the errorpaths in the
intialization.

Using the new dev_gpiod_unhinge() call we can remove the
devres management of the descriptor right before handing
it over to the regulators core.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- New patch handling this drivers GPIO descriptors properly.
---
 drivers/regulator/s2mps11.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/regulator/s2mps11.c b/drivers/regulator/s2mps11.c
index 63e66f485cc0..ee4a23ab0663 100644
--- a/drivers/regulator/s2mps11.c
+++ b/drivers/regulator/s2mps11.c
@@ -1178,7 +1178,12 @@ static int s2mps11_pmic_probe(struct platform_device *pdev)
 			config.of_node = rdata[i].of_node;
 		}
 		config.ena_gpiod = s2mps11->ext_control_gpiod[i];
-
+		/*
+		 * Hand the GPIO descriptor management over to the regulator
+		 * core, remove it from devres management.
+		 */
+		if (config.ena_gpiod)
+			devm_gpiod_unhinge(&pdev->dev, config.ena_gpiod);
 		regulator = devm_regulator_register(&pdev->dev,
 						&regulators[i], &config);
 		if (IS_ERR(regulator)) {
-- 
2.19.1

Re: [PATCH 00/13 v2] Regulator ena_gpiod fixups

[Marek Szyprowski]

Hi Linus,

On 2018-12-01 16:41, Linus Walleij wrote:
> Here is a second iteration of these fixups after thinking
> over Charles Keepax excellent comments on the first series
> of fixes.
>
> To make sure GPIO descriptors are never left dangling
> (as far as I can tell!) I use this stepwise approach:
>
> 1. Fix the regulator_register() in the core to guarantee
>    that after calling this with a valid GPIO descriptor
>    in ena_gpiod it will be gpiod_put() if there is any
>    problem.
>
> 2. Fix up simple descriptor consumers to just gpiod_get()
>    and let the core take over the descriptor. Only handle
>    the errorpath up to this point.
>
> 3. Export gpiod_get_from_of_node() and let max77686
>    obtain a GPIO descriptor from the device tree without
>    any devres make-up in the DT parsing callback.
>
> 4. Invent devm_gpiod_unhinge() that will remove the devres
>    monitoring of a devm_* allocated GPIO descriptor
>    right before handling it over to the regulator core,
>    and use this in the otherwise hairy da9211,
>    s5m8767, tps65090 and s2mps11 drivers.


The idea is good imho, but it looks that there are some missing cases in
the code. Here are some logs from the boards I have access to:


Artik5 evaluation board (arch/arm/boot/dts/exynos3250-artik5-eval.dtb):

s2mps11-pmic s2mps14-regulator: Failed to get control GPIO for 11/LDO12
------------[ cut here ]------------
WARNING: CPU: 1 PID: 1 at drivers/gpio/gpiolib-devres.c:336
s2mps11_pmic_probe+0x1c4/0x49c
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Not tainted
4.20.0-rc5-next-20181203-00020-gf6d64d46ca8c #1156
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[<c01125fc>] (unwind_backtrace) from [<c010e140>] (show_stack+0x10/0x14)
[<c010e140>] (show_stack) from [<c0a510bc>] (dump_stack+0x98/0xc4)
[<c0a510bc>] (dump_stack) from [<c0127078>] (__warn+0x10c/0x124)
[<c0127078>] (__warn) from [<c01271a4>] (warn_slowpath_null+0x40/0x48)
[<c01271a4>] (warn_slowpath_null) from [<c04e5c94>]
(s2mps11_pmic_probe+0x1c4/0x49c)
[<c04e5c94>] (s2mps11_pmic_probe) from [<c0583e60>]
(platform_drv_probe+0x48/0x9c)
[<c0583e60>] (platform_drv_probe) from [<c0581858>]
(really_probe+0x274/0x400)
[<c0581858>] (really_probe) from [<c0581ba0>]
(driver_probe_device+0x78/0x1b8)
[<c0581ba0>] (driver_probe_device) from [<c0581e08>]
(__driver_attach+0x128/0x144)
[<c0581e08>] (__driver_attach) from [<c057f71c>]
(bus_for_each_dev+0x68/0xb4)
[<c057f71c>] (bus_for_each_dev) from [<c0580a1c>]
(bus_add_driver+0x1a8/0x268)
[<c0580a1c>] (bus_add_driver) from [<c0582e44>] (driver_register+0x78/0x10c)
[<c0582e44>] (driver_register) from [<c0103154>]
(do_one_initcall+0x8c/0x410)
[<c0103154>] (do_one_initcall) from [<c0f0124c>]
(kernel_init_freeable+0x3c8/0x4d0)
[<c0f0124c>] (kernel_init_freeable) from [<c0a6afd4>]
(kernel_init+0x8/0x10c)
random: fast init done
[<c0a6afd4>] (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xd8cc1fb0 to 0xd8cc1ff8)
1fa0:                                     00000000 00000000 00000000
00000000
1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000
00000000
1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
irq event stamp: 98209
hardirqs last  enabled at (98217): [<c0192e14>] console_unlock+0x4ac/0x698
hardirqs last disabled at (98256): [<c0192a34>] console_unlock+0xcc/0x698
softirqs last  enabled at (98276): [<c0102618>] __do_softirq+0x4f0/0x5e0
softirqs last disabled at (98289): [<c012f2f4>] irq_exit+0x160/0x16c
---[ end trace fed5502b2c7cd57b ]---
Unable to handle kernel paging request at virtual address fffffff0
pgd = (ptrval)
[fffffff0] *pgd=597ce841, *pte=00000000, *ppte=00000000
Internal error: Oops: 27 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 1 PID: 1 Comm: swapper/0 Tainted: G        W        
4.20.0-rc5-next-20181203-00020-gf6d64d46ca8c #1156
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
PC is at devm_gpiod_match+0x4/0x18
LR is at devres_remove+0x68/0xb8
...
Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 4000406a  DAC: 00000051
Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
Stack: (0xd8cc1d58 to 0xd8cc2000)
...
[<c04a6f1c>] (devm_gpiod_match) from [<c05854b8>] (devres_remove+0x68/0xb8)
[<c05854b8>] (devres_remove) from [<c0585cdc>] (devres_destroy+0x8/0x24)
[<c0585cdc>] (devres_destroy) from [<c04a74a4>]
(devm_gpiod_unhinge+0x1c/0x38)
[<c04a74a4>] (devm_gpiod_unhinge) from [<c04e5c94>]
(s2mps11_pmic_probe+0x1c4/0x49c)
[<c04e5c94>] (s2mps11_pmic_probe) from [<c0583e60>]
(platform_drv_probe+0x48/0x9c)
[<c0583e60>] (platform_drv_probe) from [<c0581858>]
(really_probe+0x274/0x400)
[<c0581858>] (really_probe) from [<c0581ba0>]
(driver_probe_device+0x78/0x1b8)
[<c0581ba0>] (driver_probe_device) from [<c0581e08>]
(__driver_attach+0x128/0x144)
[<c0581e08>] (__driver_attach) from [<c057f71c>]
(bus_for_each_dev+0x68/0xb4)
[<c057f71c>] (bus_for_each_dev) from [<c0580a1c>]
(bus_add_driver+0x1a8/0x268)
[<c0580a1c>] (bus_add_driver) from [<c0582e44>] (driver_register+0x78/0x10c)
[<c0582e44>] (driver_register) from [<c0103154>]
(do_one_initcall+0x8c/0x410)
[<c0103154>] (do_one_initcall) from [<c0f0124c>]
(kernel_init_freeable+0x3c8/0x4d0)
[<c0f0124c>] (kernel_init_freeable) from [<c0a6afd4>]
(kernel_init+0x8/0x10c)
[<c0a6afd4>] (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xd8cc1fb0 to 0xd8cc1ff8)
...
---[ end trace fed5502b2c7cd57c ]---


Rinato (arch/arm/boot/dts/exynos3250-rinato.dtb):

s2mps11-pmic s2mps14-regulator: Failed to get control GPIO for 11/LDO12
random: fast init done
------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at drivers/gpio/gpiolib-devres.c:336
s2mps11_pmic_probe+0x1c4/0x49c
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Not tainted
4.20.0-rc5-next-20181203-00020-gf6d64d46ca8c #1156
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
[<c01125fc>] (unwind_backtrace) from [<c010e140>] (show_stack+0x10/0x14)
[<c010e140>] (show_stack) from [<c0a510bc>] (dump_stack+0x98/0xc4)
[<c0a510bc>] (dump_stack) from [<c0127078>] (__warn+0x10c/0x124)
[<c0127078>] (__warn) from [<c01271a4>] (warn_slowpath_null+0x40/0x48)
[<c01271a4>] (warn_slowpath_null) from [<c04e5c94>]
(s2mps11_pmic_probe+0x1c4/0x49c)
[<c04e5c94>] (s2mps11_pmic_probe) from [<c0583e60>]
(platform_drv_probe+0x48/0x9c)
[<c0583e60>] (platform_drv_probe) from [<c0581858>]
(really_probe+0x274/0x400)
[<c0581858>] (really_probe) from [<c0581ba0>]
(driver_probe_device+0x78/0x1b8)
[<c0581ba0>] (driver_probe_device) from [<c0581e08>]
(__driver_attach+0x128/0x144)
[<c0581e08>] (__driver_attach) from [<c057f71c>]
(bus_for_each_dev+0x68/0xb4)
[<c057f71c>] (bus_for_each_dev) from [<c0580a1c>]
(bus_add_driver+0x1a8/0x268)
[<c0580a1c>] (bus_add_driver) from [<c0582e44>] (driver_register+0x78/0x10c)
[<c0582e44>] (driver_register) from [<c0103154>]
(do_one_initcall+0x8c/0x410)
[<c0103154>] (do_one_initcall) from [<c0f0124c>]
(kernel_init_freeable+0x3c8/0x4d0)
[<c0f0124c>] (kernel_init_freeable) from [<c0a6afd4>]
(kernel_init+0x8/0x10c)
[<c0a6afd4>] (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xd94c1fb0 to 0xd94c1ff8)
...
irq event stamp: 114729
hardirqs last  enabled at (114747): [<c0192e14>] console_unlock+0x4ac/0x698
hardirqs last disabled at (114764): [<c0192a34>] console_unlock+0xcc/0x698
softirqs last  enabled at (114762): [<c0102618>] __do_softirq+0x4f0/0x5e0
softirqs last disabled at (114781): [<c012f2f4>] irq_exit+0x160/0x16c
---[ end trace 5961f9fa5249019f ]---
Unable to handle kernel paging request at virtual address fffffff0
pgd = (ptrval)
[fffffff0] *pgd=5fece841, *pte=00000000, *ppte=00000000
Internal error: Oops: 27 [#1] PREEMPT SMP ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper/0 Tainted: G        W        
4.20.0-rc5-next-20181203-00020-gf6d64d46ca8c #1156
Hardware name: SAMSUNG EXYNOS (Flattened Device Tree)
PC is at devm_gpiod_match+0x4/0x18
LR is at devres_remove+0x68/0xb8
...
Flags: NzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment none
Control: 10c5387d  Table: 4000406a  DAC: 00000051
Process swapper/0 (pid: 1, stack limit = 0x(ptrval))
Stack: (0xd94c1d58 to 0xd94c2000)
...
[<c04a6f1c>] (devm_gpiod_match) from [<c05854b8>] (devres_remove+0x68/0xb8)
[<c05854b8>] (devres_remove) from [<c0585cdc>] (devres_destroy+0x8/0x24)
[<c0585cdc>] (devres_destroy) from [<c04a74a4>]
(devm_gpiod_unhinge+0x1c/0x38)
[<c04a74a4>] (devm_gpiod_unhinge) from [<c04e5c94>]
(s2mps11_pmic_probe+0x1c4/0x49c)
[<c04e5c94>] (s2mps11_pmic_probe) from [<c0583e60>]
(platform_drv_probe+0x48/0x9c)
[<c0583e60>] (platform_drv_probe) from [<c0581858>]
(really_probe+0x274/0x400)
[<c0581858>] (really_probe) from [<c0581ba0>]
(driver_probe_device+0x78/0x1b8)
[<c0581ba0>] (driver_probe_device) from [<c0581e08>]
(__driver_attach+0x128/0x144)
[<c0581e08>] (__driver_attach) from [<c057f71c>]
(bus_for_each_dev+0x68/0xb4)
[<c057f71c>] (bus_for_each_dev) from [<c0580a1c>]
(bus_add_driver+0x1a8/0x268)
[<c0580a1c>] (bus_add_driver) from [<c0582e44>] (driver_register+0x78/0x10c)
[<c0582e44>] (driver_register) from [<c0103154>]
(do_one_initcall+0x8c/0x410)
[<c0103154>] (do_one_initcall) from [<c0f0124c>]
(kernel_init_freeable+0x3c8/0x4d0)
[<c0f0124c>] (kernel_init_freeable) from [<c0a6afd4>]
(kernel_init+0x8/0x10c)
[<c0a6afd4>] (kernel_init) from [<c01010b4>] (ret_from_fork+0x14/0x20)
Exception stack(0xd94c1fb0 to 0xd94c1ff8)
...
---[ end trace 5961f9fa524901a0 ]---


Other boards with other variants of s2mps11 or other PMICs works fine.




>
> Linus Walleij (13):
>   regulator: core: Track dangling GPIO descriptors
>   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: max77686: Let core handle GPIO descriptor
>   gpio: Add devm_gpiod_unhinge()
>   regulator: da9211: Hand over GPIO to regulator core
>   regulator: s5m8767: Hand over GPIO to regulator core
>   regulator: tps65090: Hand over GPIO to regulator core
>   regulator: s2mps11: Hand over GPIO to regulator core
>
>  Documentation/driver-model/devres.txt  |  1 +
>  drivers/gpio/gpiolib-devres.c          | 17 ++++++++
>  drivers/gpio/gpiolib.h                 |  6 ---
>  drivers/regulator/core.c               | 55 +++++++++++++++++++++-----
>  drivers/regulator/da9211-regulator.c   |  6 +++
>  drivers/regulator/fixed.c              |  6 ++-
>  drivers/regulator/lm363x-regulator.c   |  8 +++-
>  drivers/regulator/lp8788-ldo.c         |  8 +++-
>  drivers/regulator/max77686-regulator.c | 14 ++++---
>  drivers/regulator/max8952.c            | 10 +++--
>  drivers/regulator/max8973-regulator.c  | 23 +++++++----
>  drivers/regulator/s2mps11.c            |  7 +++-
>  drivers/regulator/s5m8767.c            |  9 ++++-
>  drivers/regulator/tps65090-regulator.c |  6 +++
>  include/linux/gpio/consumer.h          | 23 +++++++++++
>  15 files changed, 161 insertions(+), 38 deletions(-)
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 00/13 v2] Regulator ena_gpiod fixups

[Linus Walleij]

Hi Marek,

first, thanks a *lot* for testing this, it is is much, much appreciated!

On Mon, Dec 3, 2018 at 3:35 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:

> The idea is good imho, but it looks that there are some missing cases in
> the code. Here are some logs from the boards I have access to:

OK let's fix this!

> Artik5 evaluation board (arch/arm/boot/dts/exynos3250-artik5-eval.dtb):
> s2mps11-pmic s2mps14-regulator: Failed to get control GPIO for 11/LDO12

Question: this is supposed to fail, right? It is something
like a probe deferral or nonexisting GPIO controller?

I look in the upstream tree:
arch/arm/boot/dts/exynos3250-artik5.dtsi
where s2mps14 is defined:

ldo12_reg: LDO12 {
    /* VDD72 ~ VDD73 */
    regulator-name = "VLDO12_2.8V";
    regulator-min-microvolt = <2800000>;
    regulator-max-microvolt = <2800000>;
    samsung,ext-control-gpios = <&gpk0 2 GPIO_ACTIVE_HIGH>;
};

I didn't really change anything about this, so this missing
GPIO descriptor looks worrysome.

Anyways what happens is this:

gpio[reg] = devm_gpiod_get_from_of_node(...)
if (IS_ERR(gpio[reg]))
(...)
            continue;

So this IS_ERR descriptor is left around. So we should
probably handle erronoeus or NULL descriptors in
gpiod_unhinge().

If you add this on top, does it start working?

diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
index 5864e758d7f2..e35751bf0ea8 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -332,6 +332,8 @@ EXPORT_SYMBOL(devm_gpiod_put);

 void devm_gpiod_unhinge(struct device *dev, struct gpio_desc *desc)
 {
+       if (IS_ERR_OR_NULL(desc))
+               return;
        WARN_ON(devres_destroy(dev, devm_gpiod_release,
                               devm_gpiod_match, desc));
 }

Yours,
Linus Walleij

Re: [PATCH 00/13 v2] Regulator ena_gpiod fixups

[Marek Szyprowski]

Hi Linus,

On 2018-12-04 10:31, Linus Walleij wrote:
> Hi Marek,
>
> first, thanks a *lot* for testing this, it is is much, much appreciated!
>
> On Mon, Dec 3, 2018 at 3:35 PM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>
>> The idea is good imho, but it looks that there are some missing cases in
>> the code. Here are some logs from the boards I have access to:
> OK let's fix this!
>
>> Artik5 evaluation board (arch/arm/boot/dts/exynos3250-artik5-eval.dtb):
>> s2mps11-pmic s2mps14-regulator: Failed to get control GPIO for 11/LDO12
> Question: this is supposed to fail, right? It is something
> like a probe deferral or nonexisting GPIO controller?

It looks that the issue has been introduced earlier, but I didn't notice it.

gpiod_get_from_of_node() doesn't handle GPIOD_FLAGS_BIT_NONEXCLUSIVE
flag, the rest is just a result of it.


Here we have a case, where 2 regulators provided by s2mps11 driver have
a common gpio enable line (by PMIC design), so s2mps11 calls
devm_gpiod_get_from_of_node() 2 times for exactly the same gpio descriptor.


Fixing gpiod_get_from_of_node() for GPIOD_FLAGS_BIT_NONEXCLUSIVE is trivial:

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cd84315ad586..ace194665b19 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4192,6 +4192,8 @@ struct gpio_desc *gpiod_get_from_of_node(struct
device_node *node,
        transitory = flags & OF_GPIO_TRANSITORY;

        ret = gpiod_request(desc, label);
+       if (ret == -EBUSY && (dflags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
+               return desc;
        if (ret)
                return ERR_PTR(ret);


With the above fix I still however get 2 warnings from devres functions,
but this is probably caused by adding the same entry 2 times to the list
without proper refcounting... I will check that later.


> I look in the upstream tree:
> arch/arm/boot/dts/exynos3250-artik5.dtsi
> where s2mps14 is defined:
>
> ldo12_reg: LDO12 {
>     /* VDD72 ~ VDD73 */
>     regulator-name = "VLDO12_2.8V";
>     regulator-min-microvolt = <2800000>;
>     regulator-max-microvolt = <2800000>;
>     samsung,ext-control-gpios = <&gpk0 2 GPIO_ACTIVE_HIGH>;
> };
>
> I didn't really change anything about this, so this missing
> GPIO descriptor looks worrysome.
>
> Anyways what happens is this:
>
> gpio[reg] = devm_gpiod_get_from_of_node(...)
> if (IS_ERR(gpio[reg]))
> (...)
>             continue;
>
> So this IS_ERR descriptor is left around. So we should
> probably handle erronoeus or NULL descriptors in
> gpiod_unhinge().
>
> If you add this on top, does it start working?
>
> diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
> index 5864e758d7f2..e35751bf0ea8 100644
> --- a/drivers/gpio/gpiolib-devres.c
> +++ b/drivers/gpio/gpiolib-devres.c
> @@ -332,6 +332,8 @@ EXPORT_SYMBOL(devm_gpiod_put);
>
>  void devm_gpiod_unhinge(struct device *dev, struct gpio_desc *desc)
>  {
> +       if (IS_ERR_OR_NULL(desc))
> +               return;
>         WARN_ON(devres_destroy(dev, devm_gpiod_release,
>                                devm_gpiod_match, desc));
>  }
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 00/13 v2] Regulator ena_gpiod fixups

[Linus Walleij]

On Tue, Dec 4, 2018 at 11:33 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:

> >> Artik5 evaluation board (arch/arm/boot/dts/exynos3250-artik5-eval.dtb):
> >> s2mps11-pmic s2mps14-regulator: Failed to get control GPIO for 11/LDO12
> > Question: this is supposed to fail, right? It is something
> > like a probe deferral or nonexisting GPIO controller?
>
> It looks that the issue has been introduced earlier, but I didn't notice it.

Sorry :(

> gpiod_get_from_of_node() doesn't handle GPIOD_FLAGS_BIT_NONEXCLUSIVE
> flag, the rest is just a result of it.

OK I see.

> Here we have a case, where 2 regulators provided by s2mps11 driver have
> a common gpio enable line (by PMIC design), so s2mps11 calls
> devm_gpiod_get_from_of_node() 2 times for exactly the same gpio descriptor.
>
> Fixing gpiod_get_from_of_node() for GPIOD_FLAGS_BIT_NONEXCLUSIVE is trivial:

I will add a patch like this to the series!

> With the above fix I still however get 2 warnings from devres functions,
> but this is probably caused by adding the same entry 2 times to the list
> without proper refcounting... I will check that later.

Ah I see this regulator driver really excercise all corner cases of
these nonexclusive GPIO lines. (Which is good!)

Indeed devres is not going to like adding the same thing twice.

I just sent a fix for that, subject:
"gpio: devres: Handle nonexclusive GPIOs"
you could perhaps try it on top of this
series? I intend to merge that separately as a fix for current,
as it is a bug.

Yours,
Linus Walleij

Re: [PATCH 00/13 v2] Regulator ena_gpiod fixups

[Marek Szyprowski]

Hi Linus,

On 2018-12-04 13:44, Linus Walleij wrote:
> On Tue, Dec 4, 2018 at 11:33 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>
>>>> Artik5 evaluation board (arch/arm/boot/dts/exynos3250-artik5-eval.dtb):
>>>> s2mps11-pmic s2mps14-regulator: Failed to get control GPIO for 11/LDO12
>>> Question: this is supposed to fail, right? It is something
>>> like a probe deferral or nonexisting GPIO controller?
>> It looks that the issue has been introduced earlier, but I didn't notice it.
> Sorry :(
>
>> gpiod_get_from_of_node() doesn't handle GPIOD_FLAGS_BIT_NONEXCLUSIVE
>> flag, the rest is just a result of it.
> OK I see.
>
>> Here we have a case, where 2 regulators provided by s2mps11 driver have
>> a common gpio enable line (by PMIC design), so s2mps11 calls
>> devm_gpiod_get_from_of_node() 2 times for exactly the same gpio descriptor.
>>
>> Fixing gpiod_get_from_of_node() for GPIOD_FLAGS_BIT_NONEXCLUSIVE is trivial:
> I will add a patch like this to the series!
>
>> With the above fix I still however get 2 warnings from devres functions,
>> but this is probably caused by adding the same entry 2 times to the list
>> without proper refcounting... I will check that later.
> Ah I see this regulator driver really excercise all corner cases of
> these nonexclusive GPIO lines. (Which is good!)
>
> Indeed devres is not going to like adding the same thing twice.
>
> I just sent a fix for that, subject:
> "gpio: devres: Handle nonexclusive GPIOs"
> you could perhaps try it on top of this
> series? I intend to merge that separately as a fix for current,
> as it is a bug.

Those 2 warnings were worrying me and I finally found! devres_*
functions require to pass a pointer to the pointer as match_data, so
desc must be passed as &desc to devres_find() and devres_destroy()
functions, otherwise they always return -ENOENT. I will comment
respective lines in your patches then.


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 00/13 v2] Regulator ena_gpiod fixups

[Linus Walleij]

On Wed, Dec 5, 2018 at 3:30 PM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:

> > I just sent a fix for that, subject:
> > "gpio: devres: Handle nonexclusive GPIOs"
> > you could perhaps try it on top of this
> > series? I intend to merge that separately as a fix for current,
> > as it is a bug.
>
> Those 2 warnings were worrying me and I finally found! devres_*
> functions require to pass a pointer to the pointer as match_data, so
> desc must be passed as &desc to devres_find() and devres_destroy()
> functions, otherwise they always return -ENOENT. I will comment
> respective lines in your patches then.

Ah thanks so much Marek!

I will follow up with a v4 patch set immediately once I have it nailed
down. Devres is tricky, and I'm not the smartest...

Yours,
Linus Walleij