[PATCH RFT] regulator: lp8788-ldo: Use ldo->en_pin to check if regulator is enabled by external pin

[PATCH RFT] regulator: lp8788-ldo: Use ldo->en_pin to check if regulator is enabled by external pin

[Axel Lin]

ldo->en_pin is set iff the regulator is enabled by external pin.

This patch sets ldo->en_pin to NULL if lp8788_gpio_request_ldo_en() fails, then
we can use it to determinate if the regulator is controlled by external pin or
register.

lp8788_get_ldo_enable_mode(), lp8788_ldo_ctrl_by_extern_pin() and
lp8788_ldo_is_enabled_by_extern_pin() functions are not used now, remove them.

Signed-off-by: Axel Lin <axel.lin@ingics.com>
---
 drivers/regulator/lp8788-ldo.c |  107 ++++++----------------------------------
 1 file changed, 14 insertions(+), 93 deletions(-)

diff --git a/drivers/regulator/lp8788-ldo.c b/drivers/regulator/lp8788-ldo.c
index 416bb60..cd5a14a 100644
--- a/drivers/regulator/lp8788-ldo.c
+++ b/drivers/regulator/lp8788-ldo.c
@@ -88,11 +88,6 @@
 #define ENABLE				GPIOF_OUT_INIT_HIGH
 #define DISABLE				GPIOF_OUT_INIT_LOW
 
-enum lp8788_enable_mode {
-	REGISTER,
-	EXTPIN,
-};
-
 enum lp8788_ldo_id {
 	DLDO1,
 	DLDO2,
@@ -189,114 +184,38 @@ static enum lp8788_ldo_id lp8788_aldo_id[] = {
 	ALDO10,
 };
 
-/* DLDO 7, 9 and 11, ALDO 1 ~ 5 and 7
-   : can be enabled either by external pin or by i2c register */
-static enum lp8788_enable_mode
-lp8788_get_ldo_enable_mode(struct lp8788_ldo *ldo, enum lp8788_ldo_id id)
-{
-	int ret;
-	u8 val, mask;
-
-	ret = lp8788_read_byte(ldo->lp, LP8788_EN_SEL, &val);
-	if (ret)
-		return ret;
-
-	switch (id) {
-	case DLDO7:
-		mask =  LP8788_EN_SEL_DLDO7_M;
-		break;
-	case DLDO9:
-	case DLDO11:
-		mask =  LP8788_EN_SEL_DLDO911_M;
-		break;
-	case ALDO1:
-		mask =  LP8788_EN_SEL_ALDO1_M;
-		break;
-	case ALDO2 ... ALDO4:
-		mask =  LP8788_EN_SEL_ALDO234_M;
-		break;
-	case ALDO5:
-		mask =  LP8788_EN_SEL_ALDO5_M;
-		break;
-	case ALDO7:
-		mask =  LP8788_EN_SEL_ALDO7_M;
-		break;
-	default:
-		return REGISTER;
-	}
-
-	return val & mask ? EXTPIN : REGISTER;
-}
-
-static int lp8788_ldo_ctrl_by_extern_pin(struct lp8788_ldo *ldo, int pinstate)
-{
-	struct lp8788_ldo_enable_pin *pin = ldo->en_pin;
-
-	if (!pin)
-		return -EINVAL;
-
-	if (gpio_is_valid(pin->gpio))
-		gpio_set_value(pin->gpio, pinstate);
-
-	return 0;
-}
-
-static int lp8788_ldo_is_enabled_by_extern_pin(struct lp8788_ldo *ldo)
-{
-	struct lp8788_ldo_enable_pin *pin = ldo->en_pin;
-
-	if (!pin)
-		return -EINVAL;
-
-	return gpio_get_value(pin->gpio) ? 1 : 0;
-}
-
 static int lp8788_ldo_enable(struct regulator_dev *rdev)
 {
 	struct lp8788_ldo *ldo = rdev_get_drvdata(rdev);
-	enum lp8788_ldo_id id = rdev_get_id(rdev);
-	enum lp8788_enable_mode mode = lp8788_get_ldo_enable_mode(ldo, id);
 
-	switch (mode) {
-	case EXTPIN:
-		return lp8788_ldo_ctrl_by_extern_pin(ldo, ENABLE);
-	case REGISTER:
+	if (ldo->en_pin) {
+		gpio_set_value(ldo->en_pin->gpio, ENABLE);
+		return 0;
+	} else {
 		return regulator_enable_regmap(rdev);
-	default:
-		return -EINVAL;
 	}
 }
 
 static int lp8788_ldo_disable(struct regulator_dev *rdev)
 {
 	struct lp8788_ldo *ldo = rdev_get_drvdata(rdev);
-	enum lp8788_ldo_id id = rdev_get_id(rdev);
-	enum lp8788_enable_mode mode = lp8788_get_ldo_enable_mode(ldo, id);
 
-	switch (mode) {
-	case EXTPIN:
-		return lp8788_ldo_ctrl_by_extern_pin(ldo, DISABLE);
-	case REGISTER:
+	if (ldo->en_pin) {
+		gpio_set_value(ldo->en_pin->gpio, DISABLE);
+		return 0;
+	} else {
 		return regulator_disable_regmap(rdev);
-	default:
-		return -EINVAL;
 	}
 }
 
 static int lp8788_ldo_is_enabled(struct regulator_dev *rdev)
 {
 	struct lp8788_ldo *ldo = rdev_get_drvdata(rdev);
-	enum lp8788_ldo_id id = rdev_get_id(rdev);
-	enum lp8788_enable_mode mode = lp8788_get_ldo_enable_mode(ldo, id);
 
-	switch (mode) {
-	case EXTPIN:
-		return lp8788_ldo_is_enabled_by_extern_pin(ldo);
-	case REGISTER:
+	if (ldo->en_pin)
+		return gpio_get_value(ldo->en_pin->gpio) ? 1 : 0;
+	else
 		return regulator_is_enabled_regmap(rdev);
-	default:
-		return -EINVAL;
-	}
 }
 
 static int lp8788_ldo_enable_time(struct regulator_dev *rdev)
@@ -696,8 +615,10 @@ static int lp8788_config_ldo_enable_mode(struct platform_device *pdev,
 	ldo->en_pin = pdata->ldo_pin[enable_id];
 
 	ret = lp8788_gpio_request_ldo_en(pdev, ldo, enable_id);
-	if (ret)
+	if (ret) {
+		ldo->en_pin = NULL;
 		goto set_default_ldo_enable_mode;
+	}
 
 	return ret;
 
-- 
1.7.9.5

RE: [PATCH RFT] regulator: lp8788-ldo: Use ldo->en_pin to check if regulator is enabled by external pin

[Kim, Milo]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1280 bytes --]

Hi Axel,

> -----Original Message-----
> From: Axel Lin [mailto:axel.lin@ingics.com]
> Sent: Saturday, January 05, 2013 4:34 PM
> To: Mark Brown
> Cc: Kim, Milo; Girdwood, Liam; linux-kernel@vger.kernel.org
> Subject: [PATCH RFT] regulator: lp8788-ldo: Use ldo->en_pin to check if
> regulator is enabled by external pin
> 
> ldo->en_pin is set iff the regulator is enabled by external pin.
> 
> This patch sets ldo->en_pin to NULL if lp8788_gpio_request_ldo_en()
> fails, then
> we can use it to determinate if the regulator is controlled by external
> pin or
> register.
> 
> lp8788_get_ldo_enable_mode(), lp8788_ldo_ctrl_by_extern_pin() and
> lp8788_ldo_is_enabled_by_extern_pin() functions are not used now,
> remove them.

I love this patch and it works in a real board.

Initial driver code controls LDOs by reading the regulator control mode.
It guarantees working even if the control mode is changed by mistake.
But this case can be fixed using LP8788 platform data.
This patch makes the driver simpler, so I do ack your patch.
Thanks!

Acked-by: Milo Kim <milo.kim@ti.com>
Tested-by: Milo Kim <milo.kim@ti.com>

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH RFT] regulator: lp8788-ldo: Use ldo->en_pin to check if regulator is enabled by external pin

[Mark Brown]

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

On Sat, Jan 05, 2013 at 03:33:43PM +0800, Axel Lin wrote:
> ldo->en_pin is set iff the regulator is enabled by external pin.
> 
> This patch sets ldo->en_pin to NULL if lp8788_gpio_request_ldo_en() fails, then
> we can use it to determinate if the regulator is controlled by external pin or
> register.

Applied, thanks.  However we should be converting the driver to use the
core support for this - the GPIO can just be given to the core in the
regulator config which should handle everything transparently.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH RFT] regulator: lp8788-ldo: Use ldo->en_pin to check if regulator is enabled by external pin

[Kim, Milo]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Monday, January 07, 2013 8:08 PM
> To: Axel Lin
> Cc: Kim, Milo; Girdwood, Liam; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH RFT] regulator: lp8788-ldo: Use ldo->en_pin to
> check if regulator is enabled by external pin
> 
> On Sat, Jan 05, 2013 at 03:33:43PM +0800, Axel Lin wrote:
> > ldo->en_pin is set iff the regulator is enabled by external pin.
> >
> > This patch sets ldo->en_pin to NULL if lp8788_gpio_request_ldo_en()
> fails, then
> > we can use it to determinate if the regulator is controlled by
> external pin or
> > register.
> 
> Applied, thanks.  However we should be converting the driver to use the
> core support for this - the GPIO can just be given to the core in the
> regulator config which should handle everything transparently.

Thanks, Mark!

I have a question.
Do you mean the dependency of CONFIG_GENERIC_GPIO in this driver?

If yes, lp8788-ldo driver still works.
If the gpio_request_one() gets failed on loading the driver, then the lp8788-ldo
 driver refers to register based by default.
So LDOs can be enabled/disabled by the I2C, not external GPIO pins.
In that case, GPIO functions are not invoked any more.

Milo

Re: [PATCH RFT] regulator: lp8788-ldo: Use ldo->en_pin to check if regulator is enabled by external pin

[Mark Brown]

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

On Mon, Jan 07, 2013 at 11:23:26PM +0000, Kim, Milo wrote:

> > Applied, thanks.  However we should be converting the driver to use the
> > core support for this - the GPIO can just be given to the core in the
> > regulator config which should handle everything transparently.

> I have a question.
> Do you mean the dependency of CONFIG_GENERIC_GPIO in this driver?

No.  I mean you should use the regulator API core functionality for
managing GPIO enables instead of open coding it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH RFT] regulator: lp8788-ldo: Use ldo->en_pin to check if regulator is enabled by external pin

[Mark Brown]

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

On Tue, Jan 08, 2013 at 12:06:44PM +0800, Axel Lin wrote:

> In this driver,
> There is a case that one gpio controls more than one regulator.
> e.g. ALDO2, ALDO3, ALDO4 are controlled by the same pin.

> It looks like current regulator core does not support this case.

Then the code to cope with this should be ported over to the core
instead of being open coded in the driver.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

RE: [PATCH RFT] regulator: lp8788-ldo: Use ldo->en_pin to check if regulator is enabled by external pin

[Kim, Milo]

> -----Original Message-----
> From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com]
> Sent: Tuesday, January 08, 2013 7:44 PM
> To: Axel Lin
> Cc: Kim, Milo; Girdwood, Liam; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH RFT] regulator: lp8788-ldo: Use ldo->en_pin to
> check if regulator is enabled by external pin
> 
> On Tue, Jan 08, 2013 at 12:06:44PM +0800, Axel Lin wrote:
> 
> > In this driver,
> > There is a case that one gpio controls more than one regulator.
> > e.g. ALDO2, ALDO3, ALDO4 are controlled by the same pin.
> 
> > It looks like current regulator core does not support this case.
> 
> Then the code to cope with this should be ported over to the core
> instead of being open coded in the driver.

Mark, could you review the code below?

This covers multiple regulators are enabled by shared one GPIO pin.
It doesn't look nice yet, but I'd like to get your feedback on this idea first.

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 0f65b24..f63bdf1 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -3328,6 +3328,7 @@ regulator_register(const struct regulator_desc *regulator_desc,
 	struct device *dev;
 	int ret, i;
 	const char *supply = NULL;
+	bool shared_ena_pin = false;
 
 	if (regulator_desc == NULL || config == NULL)
 		return ERR_PTR(-EINVAL);
@@ -3409,17 +3410,24 @@ regulator_register(const struct regulator_desc *regulator_desc,
 		if (ret != 0) {
 			rdev_err(rdev, "Failed to request enable GPIO%d: %d\n",
 				 config->ena_gpio, ret);
-			goto wash;
+
+			if (ret == -EBUSY && config->is_gpio_shared)
+				shared_ena_pin = true;
+
+			if (!shared_ena_pin)
+				goto wash;
 		}
 
-		rdev->ena_gpio = config->ena_gpio;
-		rdev->ena_gpio_invert = config->ena_gpio_invert;
+		if (!shared_ena_pin) {
+			rdev->ena_gpio = config->ena_gpio;
+			rdev->ena_gpio_invert = config->ena_gpio_invert;
 
-		if (config->ena_gpio_flags & GPIOF_OUT_INIT_HIGH)
-			rdev->ena_gpio_state = 1;
+			if (config->ena_gpio_flags & GPIOF_OUT_INIT_HIGH)
+				rdev->ena_gpio_state = 1;
 
-		if (rdev->ena_gpio_invert)
-			rdev->ena_gpio_state = !rdev->ena_gpio_state;
+			if (rdev->ena_gpio_invert)
+				rdev->ena_gpio_state = !rdev->ena_gpio_state;
+		}
 	}
 
 	/* set regulator constraints */
diff --git a/include/linux/regulator/driver.h b/include/linux/regulator/driver.h
index d10bb0f..fa0e4e5 100644
--- a/include/linux/regulator/driver.h
+++ b/include/linux/regulator/driver.h
@@ -241,6 +241,8 @@ struct regulator_desc {
  * @regmap: regmap to use for core regmap helpers if dev_get_regulator() is
  *          insufficient.
  * @ena_gpio: GPIO controlling regulator enable.
+ * @is_gpio_shared: Set true if enable GPIO is shared (multiple regulators are
+ *                 enabled by one GPIO pin).
  * @ena_gpio_invert: Sense for GPIO enable control.
  * @ena_gpio_flags: Flags to use when calling gpio_request_one()
  */
@@ -252,6 +254,7 @@ struct regulator_config {
 	struct regmap *regmap;
 
 	int ena_gpio;
+	bool is_gpio_shared;
 	unsigned int ena_gpio_invert:1;
 	unsigned int ena_gpio_flags;
 };

Thanks,
Milo

Re: [PATCH RFT] regulator: lp8788-ldo: Use ldo->en_pin to check if regulator is enabled by external pin

[Mark Brown]

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

On Wed, Jan 09, 2013 at 02:34:40AM +0000, Kim, Milo wrote:

> -			goto wash;
> +
> +			if (ret == -EBUSY && config->is_gpio_shared)
> +				shared_ena_pin = true;
> +
> +			if (!shared_ena_pin)
> +				goto wash;

It'd be much nicer if we were able to just keep a list of GPIOs
controlling regulators as we see them and automatically work out if
they're shared.  Probably we should just refactor the code so that the
GPIO object is a separate thing rather than just storing the state
directly in the regulator struct.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]