[PATCH 0/6] regulator: add enable GPIO property to pwm-regulator

[PATCH 0/6] regulator: add enable GPIO property to pwm-regulator

[Alexandre Courbot]

This series adds the ability for the pwm-regulator driver to have an enable-gpio
property, a feature that is required for the VDD_GPU regulator of Jetson TX1.

Before doing that though, it goes through a few required changes/improvements
in the regulator framework.

Patch 1 allows the regulator core to use both an enable GPIO and an enable
callback for a given driver. This is required for our use-case since the PWM
also needs to be enabled/disabled in pwm-regulator regardless of whether we are
using an enable GPIO or not.

Patches 2-4 factorize the enable GPIO DT parsing code. With fixed and GPIO
regulators already using that feature, and PWM to do the same, it probably
makes sense to only have one function doing this. The parsing function supports
more properties than individual drivers require, but the DT binding is the
authority on which properties are valid, not the implementation.

Finally patches 5-6 add support for the enable GPIO in pwm-regulator and the
corresponding DT binding. The binding is kept minimal (active-high and
open-drain properties can be specified in the GPIO phandle) on purpose.

Alexandre Courbot (6):
  regulator: core: Allow simultaneous use of enable op and GPIO
  regulator: of: Add enable GPIO configuration function
  regulator: fixed: Use of_get_regulator_gpio_config
  regulator: gpio: Use of_get_regulator_gpio_config
  pwm-regulator: Support for enable GPIO
  dt-bindings: pwm-regulator: Document enable-gpio property

 .../bindings/regulator/pwm-regulator.txt           |  7 ++-
 drivers/regulator/core.c                           | 34 ++++++++------
 drivers/regulator/fixed.c                          | 52 +++++++++++-----------
 drivers/regulator/gpio-regulator.c                 | 47 ++++++++++---------
 drivers/regulator/of_regulator.c                   | 52 ++++++++++++++++++++++
 drivers/regulator/pwm-regulator.c                  |  5 +++
 include/linux/regulator/of_regulator.h             | 14 ++++++
 7 files changed, 149 insertions(+), 62 deletions(-)

-- 
2.8.3

[PATCH 1/6] regulator: core: Allow simultaneous use of enable op and GPIO

[Alexandre Courbot]

The current regulator enable/disable mechanism does not call the driver
enable/disable op if an enable GPIO is set. It may be desirable to use
both mechanisms though, e.g. in the case of a PWM regulator that also
has an enable GPIO.

_regulator_is_enabled() is also updated in order to take both enable
conditions into account.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/regulator/core.c | 34 ++++++++++++++++++++--------------
 1 file changed, 20 insertions(+), 14 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index db320e8fa865..995706070906 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2111,6 +2111,9 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 		}
 	}
 
+	if (!rdev->ena_pin && !rdev->desc->ops->enable)
+		return -EINVAL;
+
 	if (rdev->ena_pin) {
 		if (!rdev->ena_gpio_state) {
 			ret = regulator_ena_gpio_ctrl(rdev, true);
@@ -2118,12 +2121,12 @@ static int _regulator_do_enable(struct regulator_dev *rdev)
 				return ret;
 			rdev->ena_gpio_state = 1;
 		}
-	} else if (rdev->desc->ops->enable) {
+	}
+
+	if (rdev->desc->ops->enable) {
 		ret = rdev->desc->ops->enable(rdev);
 		if (ret < 0)
 			return ret;
-	} else {
-		return -EINVAL;
 	}
 
 	/* Allow the regulator to ramp; it would be useful to extend
@@ -2215,6 +2218,12 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
 
 	trace_regulator_disable(rdev_get_name(rdev));
 
+	if (rdev->desc->ops->disable) {
+		ret = rdev->desc->ops->disable(rdev);
+		if (ret != 0)
+			return ret;
+	}
+
 	if (rdev->ena_pin) {
 		if (rdev->ena_gpio_state) {
 			ret = regulator_ena_gpio_ctrl(rdev, false);
@@ -2222,11 +2231,6 @@ static int _regulator_do_disable(struct regulator_dev *rdev)
 				return ret;
 			rdev->ena_gpio_state = 0;
 		}
-
-	} else if (rdev->desc->ops->disable) {
-		ret = rdev->desc->ops->disable(rdev);
-		if (ret != 0)
-			return ret;
 	}
 
 	/* cares about last_off_jiffy only if off_on_delay is required by
@@ -2436,15 +2440,17 @@ EXPORT_SYMBOL_GPL(regulator_disable_deferred);
 
 static int _regulator_is_enabled(struct regulator_dev *rdev)
 {
-	/* A GPIO control always takes precedence */
+	/* If we don't know then assume that the regulator is always on */
+	bool pin_enb = true;
+	bool ops_enb = true;
+
 	if (rdev->ena_pin)
-		return rdev->ena_gpio_state;
+		pin_enb = rdev->ena_gpio_state;
 
-	/* If we don't know then assume that the regulator is always on */
-	if (!rdev->desc->ops->is_enabled)
-		return 1;
+	if (rdev->desc->ops->is_enabled)
+		ops_enb = rdev->desc->ops->is_enabled(rdev);
 
-	return rdev->desc->ops->is_enabled(rdev);
+	return pin_enb && ops_enb;
 }
 
 static int _regulator_list_voltage(struct regulator *regulator,
-- 
2.8.3

[PATCH 2/6] regulator: of: Add enable GPIO configuration function

[Alexandre Courbot]

Several regulator drivers are using an enable GPIO with similar DT
properties, yet each driver is parsing these properties its own way. Add
the of_get_regulator_gpio_config() function which is able to parse all
known properties and update the regulator_config accordingly.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/regulator/of_regulator.c       | 52 ++++++++++++++++++++++++++++++++++
 include/linux/regulator/of_regulator.h | 14 +++++++++
 2 files changed, 66 insertions(+)

diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c
index cd828dbf9d52..f1cff511320b 100644
--- a/drivers/regulator/of_regulator.c
+++ b/drivers/regulator/of_regulator.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/of.h>
+#include <linux/of_gpio.h>
 #include <linux/regulator/machine.h>
 #include <linux/regulator/driver.h>
 #include <linux/regulator/of_regulator.h>
@@ -350,3 +351,54 @@ struct regulator_init_data *regulator_of_get_init_data(struct device *dev,
 
 	return init_data;
 }
+
+int of_get_regulator_gpio_config(struct device *dev, struct device_node *np,
+				 const char *prop,
+				 struct regulator_config *cfg)
+{
+	enum of_gpio_flags flags;
+	bool active_high;
+	int ena_gpio;
+
+	/* Do we have an enable GPIO property? */
+	ena_gpio = of_get_named_gpio_flags(np, prop, 0, &flags);
+	if (!gpio_is_valid(ena_gpio)) {
+		/* No enable GPIO defined, nothing to do */
+		if (ena_gpio == -ENOENT)
+			return 0;
+
+		if (ena_gpio != -EPROBE_DEFER)
+			dev_err(dev, "error getting enable GPIO: %d\n",
+				ena_gpio);
+		return ena_gpio;
+	}
+
+	cfg->ena_gpio_initialized = true;
+	cfg->ena_gpio = ena_gpio;
+
+	/* Is GPIO active-low? */
+	active_high = of_property_read_bool(np, "enable-active-high");
+	cfg->ena_gpio_invert = of_property_read_bool(np, "enable-active-high") ?
+				false : !!(flags & OF_GPIO_ACTIVE_LOW);
+
+	/* Should GPIO be set? */
+	if (of_property_read_bool(np, "regulator-boot-on") ||
+	    of_property_read_bool(np, "regulator-always-on") ||
+	    of_property_read_bool(np, "enable-at-boot")) {
+		if (cfg->ena_gpio_invert)
+			cfg->ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
+		else
+			cfg->ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
+	} else {
+		if (cfg->ena_gpio_invert)
+			cfg->ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
+		else
+			cfg->ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
+	}
+
+	if (of_property_read_bool(np, "gpio_open_drain"))
+		cfg->ena_gpio_flags |= GPIOF_OPEN_DRAIN;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_regulator_gpio_config);
diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h
index 763953f7e3b8..eb4f1b6a26ba 100644
--- a/include/linux/regulator/of_regulator.h
+++ b/include/linux/regulator/of_regulator.h
@@ -7,6 +7,7 @@
 #define __LINUX_OF_REG_H
 
 struct regulator_desc;
+struct regulator_config;
 
 struct of_regulator_match {
 	const char *name;
@@ -24,6 +25,10 @@ extern struct regulator_init_data
 extern int of_regulator_match(struct device *dev, struct device_node *node,
 			      struct of_regulator_match *matches,
 			      unsigned int num_matches);
+extern int of_get_regulator_gpio_config(struct device *dev,
+					struct device_node *node,
+					const char *prop,
+					struct regulator_config *config);
 #else
 static inline struct regulator_init_data
 	*of_get_regulator_init_data(struct device *dev,
@@ -40,6 +45,15 @@ static inline int of_regulator_match(struct device *dev,
 {
 	return 0;
 }
+
+static inline int of_get_regulator_gpio_config(struct device *dev,
+					       struct device_node *node,
+					       const char *prop,
+					       struct regulator_config *config)
+{
+	return 0;
+}
+
 #endif /* CONFIG_OF */
 
 #endif /* __LINUX_OF_REG_H */
-- 
2.8.3

[PATCH 3/6] regulator: fixed: Use of_get_regulator_gpio_config

[Alexandre Courbot]

If instanciated from the DT, use of_get_regulator_gpio_config to obtain
the enable GPIO configuration.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/regulator/fixed.c | 52 ++++++++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 25 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 988a7472c2ab..76a8dea763cf 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -75,22 +75,14 @@ of_get_fixed_voltage_config(struct device *dev,
 		return ERR_PTR(-EINVAL);
 	}
 
-	if (init_data->constraints.boot_on)
-		config->enabled_at_boot = true;
-
-	config->gpio = of_get_named_gpio(np, "gpio", 0);
-	if ((config->gpio < 0) && (config->gpio != -ENOENT))
-		return ERR_PTR(config->gpio);
-
 	of_property_read_u32(np, "startup-delay-us", &config->startup_delay);
 
-	config->enable_high = of_property_read_bool(np, "enable-active-high");
-	config->gpio_is_open_drain = of_property_read_bool(np,
-							   "gpio-open-drain");
-
 	if (of_find_property(np, "vin-supply", NULL))
 		config->input_supply = "vin";
 
+	/* GPIO info will be obtained via regulator_of_get_gpio_config */
+	config->gpio = -ENOENT;
+
 	return config;
 }
 
@@ -114,6 +106,12 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 						     &drvdata->desc);
 		if (IS_ERR(config))
 			return PTR_ERR(config);
+
+		ret = of_get_regulator_gpio_config(&pdev->dev,
+						   pdev->dev.of_node, "gpio",
+						   &cfg);
+		if (ret)
+			return ret;
 	} else {
 		config = dev_get_platdata(&pdev->dev);
 	}
@@ -150,25 +148,29 @@ static int reg_fixed_voltage_probe(struct platform_device *pdev)
 
 	drvdata->desc.fixed_uV = config->microvolts;
 
+	/*
+	 * For platform-defined regulators - DT is already handled by
+	 * of_get_regulator_gpio_config
+	 */
 	if (gpio_is_valid(config->gpio)) {
 		cfg.ena_gpio = config->gpio;
 		if (pdev->dev.of_node)
 			cfg.ena_gpio_initialized = true;
+		cfg.ena_gpio_invert = !config->enable_high;
+		if (config->enabled_at_boot) {
+			if (config->enable_high)
+				cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
+			else
+				cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
+		} else {
+			if (config->enable_high)
+				cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
+			else
+				cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
+		}
+		if (config->gpio_is_open_drain)
+			cfg.ena_gpio_flags |= GPIOF_OPEN_DRAIN;
 	}
-	cfg.ena_gpio_invert = !config->enable_high;
-	if (config->enabled_at_boot) {
-		if (config->enable_high)
-			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
-		else
-			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
-	} else {
-		if (config->enable_high)
-			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
-		else
-			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
-	}
-	if (config->gpio_is_open_drain)
-		cfg.ena_gpio_flags |= GPIOF_OPEN_DRAIN;
 
 	cfg.dev = &pdev->dev;
 	cfg.init_data = config->init_data;
-- 
2.8.3

[PATCH 4/6] regulator: gpio: Use of_get_regulator_gpio_config

[Alexandre Courbot]

If instanciated from the DT, use of_get_regulator_gpio_config to obtain
the enable GPIO configuration.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/regulator/gpio-regulator.c | 47 ++++++++++++++++++++------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index 83e89e5d4752..5c079f854e77 100644
--- a/drivers/regulator/gpio-regulator.c
+++ b/drivers/regulator/gpio-regulator.c
@@ -153,18 +153,8 @@ of_get_gpio_regulator_config(struct device *dev, struct device_node *np,
 
 	config->supply_name = config->init_data->constraints.name;
 
-	if (of_property_read_bool(np, "enable-active-high"))
-		config->enable_high = true;
-
-	if (of_property_read_bool(np, "enable-at-boot"))
-		config->enabled_at_boot = true;
-
 	of_property_read_u32(np, "startup-delay-us", &config->startup_delay);
 
-	config->enable_gpio = of_get_named_gpio(np, "enable-gpio", 0);
-	if (config->enable_gpio == -EPROBE_DEFER)
-		return ERR_PTR(-EPROBE_DEFER);
-
 	/* Fetch GPIOs. - optional property*/
 	ret = of_gpio_count(np);
 	if ((ret < 0) && (ret != -ENOENT))
@@ -237,6 +227,9 @@ of_get_gpio_regulator_config(struct device *dev, struct device_node *np,
 				 regtype);
 	}
 
+	/* GPIO info will be obtained via regulator_of_get_gpio_config */
+	config->enable_gpio = -ENOENT;
+
 	return config;
 }
 
@@ -263,6 +256,11 @@ static int gpio_regulator_probe(struct platform_device *pdev)
 						      &drvdata->desc);
 		if (IS_ERR(config))
 			return PTR_ERR(config);
+
+		ret = of_get_regulator_gpio_config(&pdev->dev, np,
+						   "enable-gpio", &cfg);
+		if (ret)
+			return ret;
 	}
 
 	drvdata->desc.name = kstrdup(config->supply_name, GFP_KERNEL);
@@ -337,21 +335,26 @@ static int gpio_regulator_probe(struct platform_device *pdev)
 	cfg.driver_data = drvdata;
 	cfg.of_node = np;
 
+	/*
+	 * For platform-defined regulators - DT is already handled by
+	 * of_get_regulator_gpio_config
+	 */
 	if (gpio_is_valid(config->enable_gpio)) {
 		cfg.ena_gpio = config->enable_gpio;
 		cfg.ena_gpio_initialized = true;
-	}
-	cfg.ena_gpio_invert = !config->enable_high;
-	if (config->enabled_at_boot) {
-		if (config->enable_high)
-			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
-		else
-			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
-	} else {
-		if (config->enable_high)
-			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
-		else
-			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
+
+		cfg.ena_gpio_invert = !config->enable_high;
+		if (config->enabled_at_boot) {
+			if (config->enable_high)
+				cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
+			else
+				cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
+		} else {
+			if (config->enable_high)
+				cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
+			else
+				cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
+		}
 	}
 
 	drvdata->dev = regulator_register(&drvdata->desc, &cfg);
-- 
2.8.3

[PATCH 5/6] pwm-regulator: Support for enable GPIO

[Alexandre Courbot]

Enable the pwm-regulator driver to make use of an enable GPIO.

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/regulator/pwm-regulator.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/regulator/pwm-regulator.c b/drivers/regulator/pwm-regulator.c
index ab3cc0235843..8e5e4e744c51 100644
--- a/drivers/regulator/pwm-regulator.c
+++ b/drivers/regulator/pwm-regulator.c
@@ -273,6 +273,11 @@ static int pwm_regulator_probe(struct platform_device *pdev)
 	if (!init_data)
 		return -ENOMEM;
 
+	ret = of_get_regulator_gpio_config(&pdev->dev, np, "enable-gpio",
+					   &config);
+	if (ret)
+		return ret;
+
 	config.of_node = np;
 	config.dev = &pdev->dev;
 	config.driver_data = drvdata;
-- 
2.8.3

[PATCH 6/6] dt-bindings: pwm-regulator: Document enable-gpio property

[Alexandre Courbot]

Add and document the enable-gpio property. Its behavior is similar to
the property of the same name found in GPIO regulator and fixed
regulator.

Cc: devicetree@vger.kernel.org
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 Documentation/devicetree/bindings/regulator/pwm-regulator.txt | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
index ed936f0f34f2..4f9dd27f52f5 100644
--- a/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/pwm-regulator.txt
@@ -38,13 +38,18 @@ NB: To be clear, if voltage-table is provided, then the device will be used
 in Voltage Table Mode.  If no voltage-table is provided, then the device will
 be used in Continuous Voltage Mode.
 
+Optional properties:
+--------------------
+- enable-gpio:		GPIO to use to enable/disable the regulator
+
 Any property defined as part of the core regulator binding can also be used.
 (See: ../regulator/regulator.txt)
 
-Continuous Voltage Example:
+Continuous Voltage With Enable GPIO Example:
 	pwm_regulator {
 		compatible = "pwm-regulator;
 		pwms = <&pwm1 0 8448 0>;
+		enable-gpio = <&gpio0 23 GPIO_ACTIVE_HIGH>;
 		regulator-min-microvolt = <1016000>;
 		regulator-max-microvolt = <1114000>;
 		regulator-name = "vdd_logic";
-- 
2.8.3

Re: [PATCH 1/6] regulator: core: Allow simultaneous use of enable op and GPIO

[Mark Brown]

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

On Wed, Jun 22, 2016 at 05:25:53PM +0900, Alexandre Courbot wrote:
> The current regulator enable/disable mechanism does not call the driver
> enable/disable op if an enable GPIO is set. It may be desirable to use
> both mechanisms though, e.g. in the case of a PWM regulator that also
> has an enable GPIO.
> 
> _regulator_is_enabled() is also updated in order to take both enable
> conditions into account.

This is going to break or at least reduce the performance of a lot of
users - it is very common for regulators to have configurable support
for a GPIO enable in addition to a register enable with the GPIO enable
replacing a register enable for improved performance.  If you have some
strange device that requires GPIO and other operations the driver should
handle that, if nothing else it's likely that there are sequencing
requirements between the two which we are probably not going to get
right for everyone in the core.

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

Re: [PATCH 2/6] regulator: of: Add enable GPIO configuration function

[Mark Brown]

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

On Wed, Jun 22, 2016 at 05:25:54PM +0900, Alexandre Courbot wrote:
> Several regulator drivers are using an enable GPIO with similar DT
> properties, yet each driver is parsing these properties its own way. Add
> the of_get_regulator_gpio_config() function which is able to parse all
> known properties and update the regulator_config accordingly.

Why is this a function and not something data driven in the core?

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

Re: [PATCH 6/6] dt-bindings: pwm-regulator: Document enable-gpio property

[Mark Brown]

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

On Wed, Jun 22, 2016 at 05:25:58PM +0900, Alexandre Courbot wrote:
> Add and document the enable-gpio property. Its behavior is similar to
> the property of the same name found in GPIO regulator and fixed
> regulator.

Please submit patches using subject lines reflecting the style for the
subsystem.  This makes it easier for people to identify relevant
patches.

> +Optional properties:
> +--------------------
> +- enable-gpio:		GPIO to use to enable/disable the regulator
> +

The DT people are going to want this to be -gpios.

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

Re: [PATCH 1/6] regulator: core: Allow simultaneous use of enable op and GPIO

[Alexandre Courbot]

On Wed, Jun 22, 2016 at 7:34 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jun 22, 2016 at 05:25:53PM +0900, Alexandre Courbot wrote:
>> The current regulator enable/disable mechanism does not call the driver
>> enable/disable op if an enable GPIO is set. It may be desirable to use
>> both mechanisms though, e.g. in the case of a PWM regulator that also
>> has an enable GPIO.
>>
>> _regulator_is_enabled() is also updated in order to take both enable
>> conditions into account.
>
> This is going to break or at least reduce the performance of a lot of
> users - it is very common for regulators to have configurable support
> for a GPIO enable in addition to a register enable with the GPIO enable
> replacing a register enable for improved performance.

Ah, I wasn't aware of this.

> If you have some
> strange device that requires GPIO and other operations the driver should
> handle that, if nothing else it's likely that there are sequencing
> requirements between the two which we are probably not going to get
> right for everyone in the core.

Having dedicated enable GPIO code in the PWM driver sounded redundant
since we already have the same in the core, which is why I went for
this approach. But with your above point it seems like I have no
choice.

Will rework this and send a simpler change to just pwm-regulator, thanks!

Re: [PATCH 1/6] regulator: core: Allow simultaneous use of enable op and GPIO

[Alexandre Courbot]

On Thu, Jun 23, 2016 at 10:10 AM, Alexandre Courbot <gnurou@gmail.com> wrote:
> On Wed, Jun 22, 2016 at 7:34 PM, Mark Brown <broonie@kernel.org> wrote:
>> On Wed, Jun 22, 2016 at 05:25:53PM +0900, Alexandre Courbot wrote:
>>> The current regulator enable/disable mechanism does not call the driver
>>> enable/disable op if an enable GPIO is set. It may be desirable to use
>>> both mechanisms though, e.g. in the case of a PWM regulator that also
>>> has an enable GPIO.
>>>
>>> _regulator_is_enabled() is also updated in order to take both enable
>>> conditions into account.
>>
>> This is going to break or at least reduce the performance of a lot of
>> users - it is very common for regulators to have configurable support
>> for a GPIO enable in addition to a register enable with the GPIO enable
>> replacing a register enable for improved performance.
>
> Ah, I wasn't aware of this.
>
>> If you have some
>> strange device that requires GPIO and other operations the driver should
>> handle that, if nothing else it's likely that there are sequencing
>> requirements between the two which we are probably not going to get
>> right for everyone in the core.
>
> Having dedicated enable GPIO code in the PWM driver sounded redundant
> since we already have the same in the core, which is why I went for
> this approach. But with your above point it seems like I have no
> choice.

There is also another potential problem with not using the enable GPIO
in the regulator core: said GPIO can not be shared anymore between
several regulators, as this was handled by the core.

That's not a big issue for our use-case but I just wanted to point it
out. Maybe we need a more global solution for shared GPIOs, but I can
see a few challenges on the way (e.g. which policy to adopt and how to
handle conflicts).

Re: [PATCH 1/6] regulator: core: Allow simultaneous use of enable op and GPIO

[Mark Brown]

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

On Thu, Jun 23, 2016 at 02:29:39PM +0900, Alexandre Courbot wrote:

> There is also another potential problem with not using the enable GPIO
> in the regulator core: said GPIO can not be shared anymore between
> several regulators, as this was handled by the core.

Yeah, but there's more fun there since you'd need to sync up with the
device specific enable as well - it's not clear that such hardware would
make sense TBH.

> That's not a big issue for our use-case but I just wanted to point it
> out. Maybe we need a more global solution for shared GPIOs, but I can
> see a few challenges on the way (e.g. which policy to adopt and how to
> handle conflicts).

Indeed.  I've never seen such hardware though so...

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