Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1428541 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/6] regulator: core: Allow simultaneous use of enable op and GPIO Alexandre Courbot <acourbot@nvidia.com> - 2016-06-22 10:30 +0200
    Re: [PATCH 1/6] regulator: core: Allow simultaneous use of enable op  and GPIO Mark Brown <broonie@kernel.org> - 2016-06-22 12:40 +0200
      Re: [PATCH 1/6] regulator: core: Allow simultaneous use of enable op  and GPIO Alexandre Courbot <gnurou@gmail.com> - 2016-06-23 03:20 +0200
        Re: [PATCH 1/6] regulator: core: Allow simultaneous use of enable op  and GPIO Alexandre Courbot <gnurou@gmail.com> - 2016-06-23 07:40 +0200
          Re: [PATCH 1/6] regulator: core: Allow simultaneous use of enable op  and GPIO Mark Brown <broonie@kernel.org> - 2016-06-23 12:10 +0200

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

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

[toc] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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!

[toc] | [prev] | [next] | [standalone]

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

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).

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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...

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web