[4/6] regulator: lp872x: Add enable GPIO pin support

Commit Message

Paul Kocialkowski Dec. 24, 2015, 6:12 p.m. UTC

Le mercredi 23 décembre 2015 à 11:56 +0000, Mark Brown a écrit :
> On Wed, Dec 23, 2015 at 11:58:37AM +0100, Paul Kocialkowski wrote:
> 
> > +	gpio = lp->pdata->enable_gpio;
> > +	if (!gpio_is_valid(gpio))
> > +		return 0;
> > +
> > +	/* Always set enable GPIO high. */
> > +	ret = devm_gpio_request_one(lp->dev, gpio, GPIOF_OUT_INIT_HIGH, "LP872X EN");
> > +	if (ret) {
> > +		dev_err(lp->dev, "gpio request err: %d\n", ret);
> > +		return ret;
> > +	}
> 
> This isn't really adding support for the enable GPIO as the changelog
> suggests, it's requesting but not managing the GPIO.  Since there is
> core support for manging enable GPIOs this seems especially silly,
> please tell the core about the GPIO and then it will work at runtime
> too.

It looks like the core bindings for GPIO can only be used instead of the
rdev->desc->ops->enable callback, not jointly, which doesn't fit my use
case, where both the GPIO and register write have to be used to enable
regulators.

I think it would be worth making it possible to use both in core, since
that situation is probably shared with other regulators. I suggest the
following diff (that would be split into a separate patch in v2 of this
series):

 		if (pin->enable_count > 1) {
@@ -2063,6 +2062,14 @@ static int _regulator_do_enable(struct
regulator_dev *rdev)
 		}
 	}
 
+	if (rdev->desc->ops->enable) {
+		ret = rdev->desc->ops->enable(rdev);
+		if (ret < 0)
+			return ret;
+	} else if (!rdev->ena_pin) {
+		return -EINVAL;
+	}
+
 	if (rdev->ena_pin) {
 		if (!rdev->ena_gpio_state) {
 			ret = regulator_ena_gpio_ctrl(rdev, true);
@@ -2070,10 +2077,6 @@ static int _regulator_do_enable(struct
regulator_dev *rdev)
 				return ret;
 			rdev->ena_gpio_state = 1;
 		}
-	} else if (rdev->desc->ops->enable) {
-		ret = rdev->desc->ops->enable(rdev);
-		if (ret < 0)
-			return ret;
 	} else {
 		return -EINVAL;
 	}

Comments

Mark Brown Dec. 24, 2015, 7:35 p.m. UTC | #1

On Thu, Dec 24, 2015 at 07:12:53PM +0100, Paul Kocialkowski wrote:
> Le mercredi 23 décembre 2015 à 11:56 +0000, Mark Brown a écrit :

> > This isn't really adding support for the enable GPIO as the changelog
> > suggests, it's requesting but not managing the GPIO.  Since there is
> > core support for manging enable GPIOs this seems especially silly,
> > please tell the core about the GPIO and then it will work at runtime
> > too.

> It looks like the core bindings for GPIO can only be used instead of the
> rdev->desc->ops->enable callback, not jointly, which doesn't fit my use
> case, where both the GPIO and register write have to be used to enable
> regulators.

> I think it would be worth making it possible to use both in core, since
> that situation is probably shared with other regulators. I suggest the
> following diff (that would be split into a separate patch in v2 of this
> series):

No, that's broken - the whole point with using a GPIO for enable control
on a lot of devices is that it is much faster than doing a register
write.  What I would expect to happen in this case is that when
initialsing the GPIO we set the register to enabled and then only manage
the GPIO at runtime.

Paul Kocialkowski Dec. 24, 2015, 8:05 p.m. UTC | #2

Le jeudi 24 décembre 2015 à 19:35 +0000, Mark Brown a écrit :
> On Thu, Dec 24, 2015 at 07:12:53PM +0100, Paul Kocialkowski wrote:
> > Le mercredi 23 décembre 2015 à 11:56 +0000, Mark Brown a écrit :
> 
> > > This isn't really adding support for the enable GPIO as the changelog
> > > suggests, it's requesting but not managing the GPIO.  Since there is
> > > core support for manging enable GPIOs this seems especially silly,
> > > please tell the core about the GPIO and then it will work at runtime
> > > too.
> 
> > It looks like the core bindings for GPIO can only be used instead of the
> > rdev->desc->ops->enable callback, not jointly, which doesn't fit my use
> > case, where both the GPIO and register write have to be used to enable
> > regulators.
> 
> > I think it would be worth making it possible to use both in core, since
> > that situation is probably shared with other regulators. I suggest the
> > following diff (that would be split into a separate patch in v2 of this
> > series):
> 
> No, that's broken - the whole point with using a GPIO for enable control
> on a lot of devices is that it is much faster than doing a register
> write.  What I would expect to happen in this case is that when
> initialsing the GPIO we set the register to enabled and then only manage
> the GPIO at runtime.

That GPIO is shared with all regulators that the chip provides, so doing
things this way certainly won't work if we don't want all the regulators
powered at all times: in that case, the GPIO has precedence over the
regulator-specific registers.

Doing things the other way round would work and I suppose that chips
that can use a GPIO over registers just wouldn't register any
ops->enable callback in their description.

Patch

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e92f344..dd0674f 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -1963,7 +1963,6 @@  static int regulator_ena_gpio_ctrl(struct
regulator_dev *rdev, bool enable)
 		if (pin->enable_count == 0)
 			gpiod_set_value_cansleep(pin->gpiod,
 						 !pin->ena_gpio_invert);
-
 		pin->enable_count++;
 	} else {