[PATCH 00/15 v3] Regulator ena_gpiod fixups

[PATCH 00/15 v3] Regulator ena_gpiod fixups

[Linus Walleij]

Here is a third iteration of these fixups after thinking
over Marek's remarks on the v2 version.

Also available in git branch
gpio-descriptors-regulator-fixup in the GPIO git tree:
https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-gpio.git/log/?h=gpio-descriptors-regulator-fixup

I added two fixup item to the list: making the gpiod
from of node respect nonexlusive flags and fixing the
shared regulators in devm_gpiod_get*.

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. Make gpiod_get_from_of_node() respect the
   GPIOD_FLAGS_BIT_NONEXCLUSIVE flag.

5. Fix up devm_gpiod_get_* to respect the
   GPIOD_FLAGS_BIT_NONEXCLUSIVE flag.

6. 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 (15):
  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: Enable nonexclusive gpiods from DT nodes
  gpio: devres: Handle nonexclusive GPIOs
  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          | 80 ++++++++++++++++++++++----
 drivers/gpio/gpiolib.c                 |  2 +
 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 ++++++++
 16 files changed, 214 insertions(+), 50 deletions(-)

-- 
2.19.2

[PATCH 01/15 v3] 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->v3:
- Resending.
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.2

[PATCH 02/15 v3] 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 v2->v3:
- Resending.
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.2

[PATCH 03/15 v3] 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 v2->v3:
- Resending.
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.2

[PATCH 04/15 v3] 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 v2->v3:
- Resending.
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.2

[PATCH 05/15 v3] 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 v2->v3:
- Resending.
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.2

[PATCH 06/15 v3] 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 v2->v3:
- Resending.
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.2

[PATCH 07/15 v3] 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 v2->v3:
- Resending.
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.2

[PATCH 08/15 v3] 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 v2->v3:
- Resending.
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.2

[PATCH 09/15 v3] gpio: Enable nonexclusive gpiods from DT nodes

[Linus Walleij]

This makes gpiod_get_from_of_node() respect the
GPIOD_FLAGS_BIT_NONEXCLUSIVE flag which is especially
nice when getting regulator GPIOs right out of device
tree nodes.

Cc: Mark Brown <broonie@kernel.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Suggested-by: Marek Szyprowski <m.szyprowski@samsung.com>
Fixes: b0ce7b29bfcd ("regulator/gpio: Allow nonexclusive GPIO access")
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v3:
- New patch after Marek's review.
---
 drivers/gpio/gpiolib.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 230e41562462..a7e3fd512e2d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4204,6 +4204,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 && (flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE))
+		return desc;
 	if (ret)
 		return ERR_PTR(ret);
 
-- 
2.19.2

[PATCH 10/15 v3] gpio: devres: Handle nonexclusive GPIOs

[Linus Walleij]

When we get a nonexeclusive GPIO descriptor using managed
resources, we should only add it to the list of managed
resources once: on the first user. Augment the
devm_gpiod_get_index() and devm_gpiod_get_from_of_node()
calls to account for this by checking if the descriptor
is already resource managed before we proceed to allocate
a new resource management struct.

Cc: Mark Brown <broonie@kernel.org>
Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Cc: Marek Szyprowski <m.szyprowski@samsung.com>
Fixes: b0ce7b29bfcd ("regulator/gpio: Allow nonexclusive GPIO access")
Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v3:
- New patch.
- This fix is in response to an issue Marek saw in the fixups
  for resource-managed GPIO descriptors used with ena_gpiod
  in the regulator subsystem.
- I first thought to apply it to the GPIO tree directly, but
  since it is not a regression it is better to put it into
  the regulator tree with the rest of the patches.
---
 drivers/gpio/gpiolib-devres.c | 50 ++++++++++++++++++++++++++---------
 1 file changed, 38 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
index 01959369360b..a57e968025a8 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -98,15 +98,28 @@ struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev,
 	struct gpio_desc **dr;
 	struct gpio_desc *desc;
 
+	desc = gpiod_get_index(dev, con_id, idx, flags);
+	if (IS_ERR(desc))
+		return desc;
+
+	/*
+	 * For non-exclusive GPIO descriptors, check if this descriptor is
+	 * already under resource management by this device.
+	 */
+	if (flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) {
+		struct devres *dres;
+
+		dres = devres_find(dev, devm_gpiod_release,
+				   devm_gpiod_match, desc);
+		if (dres)
+			return desc;
+	}
+
 	dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *),
 			  GFP_KERNEL);
-	if (!dr)
+	if (!dr) {
+		gpiod_put(desc);
 		return ERR_PTR(-ENOMEM);
-
-	desc = gpiod_get_index(dev, con_id, idx, flags);
-	if (IS_ERR(desc)) {
-		devres_free(dr);
-		return desc;
 	}
 
 	*dr = desc;
@@ -140,15 +153,28 @@ struct gpio_desc *devm_gpiod_get_from_of_node(struct device *dev,
 	struct gpio_desc **dr;
 	struct gpio_desc *desc;
 
+	desc = gpiod_get_from_of_node(node, propname, index, dflags, label);
+	if (IS_ERR(desc))
+		return desc;
+
+	/*
+	 * For non-exclusive GPIO descriptors, check if this descriptor is
+	 * already under resource management by this device.
+	 */
+	if (dflags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) {
+		struct devres *dres;
+
+		dres = devres_find(dev, devm_gpiod_release,
+				   devm_gpiod_match, desc);
+		if (dres)
+			return desc;
+	}
+
 	dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *),
 			  GFP_KERNEL);
-	if (!dr)
+	if (!dr) {
+		gpiod_put(desc);
 		return ERR_PTR(-ENOMEM);
-
-	desc = gpiod_get_from_of_node(node, propname, index, dflags, label);
-	if (IS_ERR(desc)) {
-		devres_free(dr);
-		return desc;
 	}
 
 	*dr = desc;
-- 
2.19.2

[PATCH 11/15 v3] 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 v2->v3:
- Make sure to bail out of gpiod_unhinge() if the descritor
  passed in is NULL or an error pointer.
- Accept of devres_destroy() returns -ENOENT: if we have
  nonexclusive GPIOs, we may attempt to remove resource
  management from the same descriptor multiple times.
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         | 30 +++++++++++++++++++++++++++
 include/linux/gpio/consumer.h         | 10 +++++++++
 3 files changed, 41 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 a57e968025a8..e9a17ec4de02 100644
--- a/drivers/gpio/gpiolib-devres.c
+++ b/drivers/gpio/gpiolib-devres.c
@@ -346,6 +346,36 @@ 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)
+{
+	int ret;
+
+	if (IS_ERR_OR_NULL(desc))
+		return;
+	ret = devres_destroy(dev, devm_gpiod_release,
+			     devm_gpiod_match, desc);
+	/*
+	 * If the GPIO descriptor is requested as nonexclusive, we
+	 * may call this function several times on the same descriptor
+	 * so it is OK if devres_destroy() returns -ENOENT.
+	 */
+	if (ret == -ENOENT)
+		return;
+	/* Anything else we should warn about */
+	WARN_ON(ret);
+}
+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.2

[PATCH 12/15 v3] 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 v2->v3:
- Resending.
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.2

[PATCH 13/15 v3] 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 v2->v3:
- Resending.
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.2

[PATCH 14/15 v3] 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 v2->v3:
- Resending.
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.2

[PATCH 15/15 v3] 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 v2->v3:
- Resending.
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.2

Re: [PATCH 06/15 v3] regulator: max8973: Let core handle GPIO descriptor

[Charles Keepax]

On Wed, Dec 05, 2018 at 01:47:12PM +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 v2->v3:
> - Resending.
> 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*)
> ---
> @@ -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);

My comment on v2 still stands here, the GPIO is not passed to
the regulator core on this path. Very sorry it took me so long
to review v2 (been one of those wonderful weeks at this end)
and then I managed to perfectly time reviewing it to the exact
second you sent v3.

I think apart from this the series looks good to me though.

Thanks,
Charles

Re: [PATCH 11/15 v3] gpio: Add devm_gpiod_unhinge()

[Marek Szyprowski]

Hi Linus,

On 2018-12-05 13:47, Linus Walleij wrote:
> 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 v2->v3:
> - Make sure to bail out of gpiod_unhinge() if the descritor
>   passed in is NULL or an error pointer.
> - Accept of devres_destroy() returns -ENOENT: if we have
>   nonexclusive GPIOs, we may attempt to remove resource
>   management from the same descriptor multiple times.
> 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         | 30 +++++++++++++++++++++++++++
>  include/linux/gpio/consumer.h         | 10 +++++++++
>  3 files changed, 41 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 a57e968025a8..e9a17ec4de02 100644
> --- a/drivers/gpio/gpiolib-devres.c
> +++ b/drivers/gpio/gpiolib-devres.c
> @@ -346,6 +346,36 @@ 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)
> +{
> +	int ret;
> +
> +	if (IS_ERR_OR_NULL(desc))
> +		return;
> +	ret = devres_destroy(dev, devm_gpiod_release,
> +			     devm_gpiod_match, desc);

    devres_destroy(dev, devm_gpiod_release, devm_gpiod_match, &desc);

> +	/*
> +	 * If the GPIO descriptor is requested as nonexclusive, we
> +	 * may call this function several times on the same descriptor
> +	 * so it is OK if devres_destroy() returns -ENOENT.
> +	 */
> +	if (ret == -ENOENT)
> +		return;
> +	/* Anything else we should warn about */
> +	WARN_ON(ret);
> +}
> +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();

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

Re: [PATCH 10/15 v3] gpio: devres: Handle nonexclusive GPIOs

[Marek Szyprowski]

Hi Linus,

On 2018-12-05 13:47, Linus Walleij wrote:
> When we get a nonexeclusive GPIO descriptor using managed
> resources, we should only add it to the list of managed
> resources once: on the first user. Augment the
> devm_gpiod_get_index() and devm_gpiod_get_from_of_node()
> calls to account for this by checking if the descriptor
> is already resource managed before we proceed to allocate
> a new resource management struct.
>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Cc: Marek Szyprowski <m.szyprowski@samsung.com>
> Fixes: b0ce7b29bfcd ("regulator/gpio: Allow nonexclusive GPIO access")
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v3:
> - New patch.
> - This fix is in response to an issue Marek saw in the fixups
>   for resource-managed GPIO descriptors used with ena_gpiod
>   in the regulator subsystem.
> - I first thought to apply it to the GPIO tree directly, but
>   since it is not a regression it is better to put it into
>   the regulator tree with the rest of the patches.
> ---
>  drivers/gpio/gpiolib-devres.c | 50 ++++++++++++++++++++++++++---------
>  1 file changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-devres.c b/drivers/gpio/gpiolib-devres.c
> index 01959369360b..a57e968025a8 100644
> --- a/drivers/gpio/gpiolib-devres.c
> +++ b/drivers/gpio/gpiolib-devres.c
> @@ -98,15 +98,28 @@ struct gpio_desc *__must_check devm_gpiod_get_index(struct device *dev,
>  	struct gpio_desc **dr;
>  	struct gpio_desc *desc;
>  
> +	desc = gpiod_get_index(dev, con_id, idx, flags);
> +	if (IS_ERR(desc))
> +		return desc;
> +
> +	/*
> +	 * For non-exclusive GPIO descriptors, check if this descriptor is
> +	 * already under resource management by this device.
> +	 */
> +	if (flags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) {
> +		struct devres *dres;
> +
> +		dres = devres_find(dev, devm_gpiod_release,
> +				   devm_gpiod_match, desc);

    devres_find(dev, devm_gpiod_release, devm_gpiod_match, &desc);

> +		if (dres)
> +			return desc;
> +	}
> +
>  	dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *),
>  			  GFP_KERNEL);
> -	if (!dr)
> +	if (!dr) {
> +		gpiod_put(desc);
>  		return ERR_PTR(-ENOMEM);
> -
> -	desc = gpiod_get_index(dev, con_id, idx, flags);
> -	if (IS_ERR(desc)) {
> -		devres_free(dr);
> -		return desc;
>  	}
>  
>  	*dr = desc;
> @@ -140,15 +153,28 @@ struct gpio_desc *devm_gpiod_get_from_of_node(struct device *dev,
>  	struct gpio_desc **dr;
>  	struct gpio_desc *desc;
>  
> +	desc = gpiod_get_from_of_node(node, propname, index, dflags, label);
> +	if (IS_ERR(desc))
> +		return desc;
> +
> +	/*
> +	 * For non-exclusive GPIO descriptors, check if this descriptor is
> +	 * already under resource management by this device.
> +	 */
> +	if (dflags & GPIOD_FLAGS_BIT_NONEXCLUSIVE) {
> +		struct devres *dres;
> +
> +		dres = devres_find(dev, devm_gpiod_release,
> +				   devm_gpiod_match, desc);

 	devres_find(dev, devm_gpiod_release, devm_gpiod_match, &desc);

> +		if (dres)
> +			return desc;
> +	}
> +
>  	dr = devres_alloc(devm_gpiod_release, sizeof(struct gpio_desc *),
>  			  GFP_KERNEL);
> -	if (!dr)
> +	if (!dr) {
> +		gpiod_put(desc);
>  		return ERR_PTR(-ENOMEM);
> -
> -	desc = gpiod_get_from_of_node(node, propname, index, dflags, label);
> -	if (IS_ERR(desc)) {
> -		devres_free(dr);
> -		return desc;
>  	}
>  
>  	*dr = desc;

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

Re: [PATCH 06/15 v3] regulator: max8973: Let core handle GPIO descriptor

[Linus Walleij]

On Wed, Dec 5, 2018 at 2:43 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Wed, Dec 05, 2018 at 01:47:12PM +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 v2->v3:
> > - Resending.
> > 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*)
> > ---
> > @@ -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);
>
> My comment on v2 still stands here, the GPIO is not passed to
> the regulator core on this path.

Patch 01 should take care of that, did you check it?

Yours,
Linus Walleij

Re: [PATCH 06/15 v3] regulator: max8973: Let core handle GPIO descriptor

[Charles Keepax]

On Wed, Dec 05, 2018 at 03:42:06PM +0100, Linus Walleij wrote:
> On Wed, Dec 5, 2018 at 2:43 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > On Wed, Dec 05, 2018 at 01:47:12PM +0100, Linus Walleij wrote:
> > > @@ -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);
> >
> > My comment on v2 still stands here, the GPIO is not passed to
> > the regulator core on this path.
> 
> Patch 01 should take care of that, did you check it?
> 

Yeah, patch 1 makes the regulator core consistently handle GPIOs
that are passed into it. However, on the MAX77621 path in this
switch statement the GPIO is never passed into the regulator
core, so the core never takes ownership of it, so the driver still
needs to manage the lifetime.

It should be a pretty easy fix though as commented on v2, again
apologies for the slow review.

Thanks,
Charles

Re: [PATCH 06/15 v3] regulator: max8973: Let core handle GPIO descriptor

[Linus Walleij]

On Wed, Dec 5, 2018 at 4:33 PM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Wed, Dec 05, 2018 at 03:42:06PM +0100, Linus Walleij wrote:
> > On Wed, Dec 5, 2018 at 2:43 PM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > > On Wed, Dec 05, 2018 at 01:47:12PM +0100, Linus Walleij wrote:
> > > > @@ -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);
> > >
> > > My comment on v2 still stands here, the GPIO is not passed to
> > > the regulator core on this path.
> >
> > Patch 01 should take care of that, did you check it?
>
> Yeah, patch 1 makes the regulator core consistently handle GPIOs
> that are passed into it.

Sorry. I confused this patch for the max77686 patch. That
one was fixed with patch 01...

> However, on the MAX77621 path in this
> switch statement the GPIO is never passed into the regulator
> core, so the core never takes ownership of it, so the driver still
> needs to manage the lifetime.
>
> It should be a pretty easy fix though as commented on v2, again
> apologies for the slow review.

OK I switch it to devm_ as you suggested, as we implemented
gpiod_unhinge it's a piece of cake nowadays.

Thanks a lot!
Linus Walleij

Re: [PATCH 06/15 v3] regulator: max8973: Let core handle GPIO descriptor

[Charles Keepax]

On Thu, Dec 06, 2018 at 09:58:30AM +0100, Linus Walleij wrote:
> On Wed, Dec 5, 2018 at 4:33 PM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > On Wed, Dec 05, 2018 at 03:42:06PM +0100, Linus Walleij wrote:
> > > On Wed, Dec 5, 2018 at 2:43 PM Charles Keepax
> > > <ckeepax@opensource.cirrus.com> wrote:
> > > > On Wed, Dec 05, 2018 at 01:47:12PM +0100, Linus Walleij wrote:
> > > > > @@ -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);
> > > >
> > > > My comment on v2 still stands here, the GPIO is not passed to
> > > > the regulator core on this path.
> > >
> > > Patch 01 should take care of that, did you check it?
> >
> > Yeah, patch 1 makes the regulator core consistently handle GPIOs
> > that are passed into it.
> 
> Sorry. I confused this patch for the max77686 patch. That
> one was fixed with patch 01...
> 
> > However, on the MAX77621 path in this
> > switch statement the GPIO is never passed into the regulator
> > core, so the core never takes ownership of it, so the driver still
> > needs to manage the lifetime.
> >
> > It should be a pretty easy fix though as commented on v2, again
> > apologies for the slow review.
> 
> OK I switch it to devm_ as you suggested, as we implemented
> gpiod_unhinge it's a piece of cake nowadays.
> 

You shouldn't really need to use unhinge, you can just use devm
on the path that doesn't pass it to the core and not on the
one that does. You just need to update the error case below it to
use config->ena_gpiod rather than gpiod.

Thanks,
Charles

Re: [PATCH 06/15 v3] regulator: max8973: Let core handle GPIO descriptor

[Linus Walleij]

On Thu, Dec 6, 2018 at 11:34 AM Charles Keepax
<ckeepax@opensource.cirrus.com> wrote:
> On Thu, Dec 06, 2018 at 09:58:30AM +0100, Linus Walleij wrote:
> > On Wed, Dec 5, 2018 at 4:33 PM Charles Keepax
> > <ckeepax@opensource.cirrus.com> wrote:
> > > On Wed, Dec 05, 2018 at 03:42:06PM +0100, Linus Walleij wrote:
> > > > On Wed, Dec 5, 2018 at 2:43 PM Charles Keepax
> > > > <ckeepax@opensource.cirrus.com> wrote:
> > > > > On Wed, Dec 05, 2018 at 01:47:12PM +0100, Linus Walleij wrote:
> > > > > > @@ -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);
> > > > >
> > > > > My comment on v2 still stands here, the GPIO is not passed to
> > > > > the regulator core on this path.
> > > >
> > > > Patch 01 should take care of that, did you check it?
> > >
> > > Yeah, patch 1 makes the regulator core consistently handle GPIOs
> > > that are passed into it.
> >
> > Sorry. I confused this patch for the max77686 patch. That
> > one was fixed with patch 01...
> >
> > > However, on the MAX77621 path in this
> > > switch statement the GPIO is never passed into the regulator
> > > core, so the core never takes ownership of it, so the driver still
> > > needs to manage the lifetime.
> > >
> > > It should be a pretty easy fix though as commented on v2, again
> > > apologies for the slow review.
> >
> > OK I switch it to devm_ as you suggested, as we implemented
> > gpiod_unhinge it's a piece of cake nowadays.
> >
>
> You shouldn't really need to use unhinge, you can just use devm
> on the path that doesn't pass it to the core and not on the
> one that does. You just need to update the error case below it to
> use config->ena_gpiod rather than gpiod.

Indeed I just think it will be confusing when people read the code.

It's better consistency if its just devm_* and the  we unhinge the
one we pass to the regulator core IMO.

Yours,
Linus Walleij

Re: [PATCH 06/15 v3] regulator: max8973: Let core handle GPIO descriptor

[Charles Keepax]

On Thu, Dec 06, 2018 at 12:47:46PM +0100, Linus Walleij wrote:
> On Thu, Dec 6, 2018 at 11:34 AM Charles Keepax
> <ckeepax@opensource.cirrus.com> wrote:
> > On Thu, Dec 06, 2018 at 09:58:30AM +0100, Linus Walleij wrote:
> > > On Wed, Dec 5, 2018 at 4:33 PM Charles Keepax
> > > <ckeepax@opensource.cirrus.com> wrote:
> > > > On Wed, Dec 05, 2018 at 03:42:06PM +0100, Linus Walleij wrote:
> > > > > On Wed, Dec 5, 2018 at 2:43 PM Charles Keepax
> > > > > <ckeepax@opensource.cirrus.com> wrote:
> > > > > > On Wed, Dec 05, 2018 at 01:47:12PM +0100, Linus Walleij wrote:
> > You shouldn't really need to use unhinge, you can just use devm
> > on the path that doesn't pass it to the core and not on the
> > one that does. You just need to update the error case below it to
> > use config->ena_gpiod rather than gpiod.
> 
> Indeed I just think it will be confusing when people read the code.
> 
> It's better consistency if its just devm_* and the  we unhinge the
> one we pass to the regulator core IMO.
> 

That's ok no objections from my if you prefer it that way.

Thanks,
Charles