About gpio-regulator setting on DT

Hi Lee, Mark, Liam

I would like to use ${LINUX}/drivers/gpio-regulator.c via DT.
My board needs "GPIOF_OUT_INIT_HIGH" on struct gpio :: flags

But, current of_get_gpio_regulator_config() seems doesn't have such setting method. This means it will be "GPIOF_OUT_INIT_LOW"

	for (i = 0; i < config->nr_gpios; i++) {
		gpio = of_get_named_gpio(np, "gpios", i);
		if (gpio < 0)
			break;
		config->gpios[i].gpio = gpio;
	}

How to set GPIOF_OUT_INIT_HIGH via DT ? Or, am I misunderstanding ?

Best regards
---
Kuninori Morimoto

The combination of the enable-active-high and enable-at-boot properties ought be able to cause the driver to do the right thing, the flags do this:

	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;
	}

of_get_named_gpio() just looks up the GPIO number, it doesn't request the GPIO.

I think you've just run in to the same problem that we've found with the GPIO regulator code for the vmmcq on the lager where the DT probed version is getting 1800mV for MMC whereas the platform probed version gets 3300mV for MMC (and thus works better).

My view is that we should really add an initialisation voltage setting to the regulators so that if there is >2 states we can select the state it starts in.

Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

The drivers using the regulator should be doing that if the regulator has variable supplies, the expectation is that trying to provide something outside of a driver that's actively managing this is just going to give more opportunity for the system to become fragile. A write only driver like the GPIO regulator may want to provide something but it's not clear to me that this would help for generic regulators.

Hi Mark

Hmm...
I'm not sure detail,
but, I need config->gpios[ptr].flags instead of cfg.ena_gpio_flags. Because it is used for drvdata->state.

static int gpio_regulator_probe(struct platform_device *pdev)

{
    if (np) {
        config = of_get_gpio_regulator_config(&pdev->dev, np);
        if (IS_ERR(config))
            return PTR_ERR(config);
    }
    ...

    /* build initial state from gpio init data. */
    state = 0;
    for (ptr = 0; ptr < drvdata->nr_gpios; ptr++) {
        if (config->gpios[ptr].flags & GPIOF_OUT_INIT_HIGH)     <= we need this
               state |= (1 << ptr);
    }
    drvdata->state = state;
    ...

    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;
    }

)

Best regards
---
Kuninori Morimoto

So this just looks like a bug in the code - the DT has all the data needed but the parsing code isn't setting everything up that the rest of the code needs and/or we should drop one of the ways of configuring things in the main probe function. Avoiding having two ways of doing the same thing in the main probe would definitely be good.

From: Kuninori Morimoto <[email protected]>

config->gpios[x].flags indicates initial pin status, and it will be used for drvdata->state
on gpio_regulator_probe().
But, current of_get_gpio_regulator_config() doesn't care about this flags.
This patch adds new gpios-status property in order to care about initial pin status.

---
 .../bindings/regulator/gpio-regulator.txt          |    1 +
 drivers/regulator/gpio-regulator.c                 |   11 +++++++++++
 2 files changed, 12 insertions(+)

index 63c6598..3ecb585 100644
@@ -8,6 +8,7 @@ Required properties:
 Optional properties:
 - enable-gpio		: GPIO to use to enable/disable the regulator.
 - gpios			: GPIO group used to control voltage.
+- gpios-states		: gpios pin's initial states. 1 means HIGH
 - startup-delay-us	: Startup time in microseconds.
 - enable-active-high	: Polarity of GPIO is active high (default is low).
 

index c0a1d00..7c8e37a 100644
@@ -172,11 +172,22 @@ of_get_gpio_regulator_config(struct device *dev, struct device_node *np)
 	if (!config->gpios)
 		return ERR_PTR(-ENOMEM);
 
+	prop = of_find_property(np, "gpios-states", NULL);
+	if (prop) {
+		proplen = prop->length / sizeof(int);
+		if (proplen != config->nr_gpios) {
+			/* gpios <-> gpios-states mismatch */
+			prop = NULL;
+		}
+	}
+
 	for (i = 0; i < config->nr_gpios; i++) {
 		gpio = of_get_named_gpio(np, "gpios", i);
 		if (gpio < 0)
 			break;
 		config->gpios[i].gpio = gpio;
+		if (prop && be32_to_cpup((int *)prop->value + i))
+			config->gpios[i].flags = GPIOF_OUT_INIT_HIGH;
 	}
 
 	/* Fetch states. */

1.7.9.5

I've applied this, thanks. However, a couple of things that it'd be nice to improve with followup patches:

A mention that this is an array of values would be good (it's clear from the code but not from the document), as would saying that the defualt is low if nothing is specified.

From: Kuninori Morimoto <[email protected]>

---
 drivers/regulator/gpio-regulator.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ac3a8c7..5491cee 100644
@@ -176,7 +176,7 @@ of_get_gpio_regulator_config(struct device *dev, struct device_node *np)
 	if (prop) {
 		proplen = prop->length / sizeof(int);
 		if (proplen != config->nr_gpios) {
-			/* gpios <-> gpios-states mismatch */
+			dev_warn(dev, "gpios <-> gpios-states mismatch\n");
 			prop = NULL;
 		}
 	}

1.7.9.5

Applied both, thanks.

From: Kuninori Morimoto <[email protected]>

gpios-states is array, and default is 0

---
 .../bindings/regulator/gpio-regulator.txt          |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

index 3ecb585..356e8bb 100644
@@ -8,7 +8,8 @@ Required properties:
 Optional properties:
 - enable-gpio		: GPIO to use to enable/disable the regulator.
 - gpios			: GPIO group used to control voltage.
-- gpios-states		: gpios pin's initial states. 1 means HIGH
+- gpios-states		: gpios pin's initial states array. 0: LOW, 1: HIGH.
+			  defualt is LOW if nothing is specified.
 - startup-delay-us	: Startup time in microseconds.
 - enable-active-high	: Polarity of GPIO is active high (default is low).
 

1.7.9.5