About gpio-regulator setting on DT

About gpio-regulator setting on DT

[Kuninori Morimoto]

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

About gpio-regulator setting on DT

[Kuninori Morimoto]

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

Re: About gpio-regulator setting on DT

[Mark Brown]

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

On Wed, Jan 29, 2014 at 12:38:19AM -0800, Kuninori Morimoto wrote:

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

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.

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

Re: About gpio-regulator setting on DT

[Ben Dooks]

On 29/01/14 12:45, Mark Brown wrote:
> On Wed, Jan 29, 2014 at 12:38:19AM -0800, Kuninori Morimoto wrote:
>
>> How to set GPIOF_OUT_INIT_HIGH via DT ?
>> Or, am I misunderstanding ?
>
> 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

Re: About gpio-regulator setting on DT

[Mark Brown]

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

On Wed, Jan 29, 2014 at 04:16:03PM +0000, Ben Dooks wrote:
> On 29/01/14 12:45, Mark Brown wrote:

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

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.

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

Re: About gpio-regulator setting on DT

[Kuninori Morimoto]

Hi Mark

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

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

Re: About gpio-regulator setting on DT

[Kuninori Morimoto]

Hi Mark

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

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

Re: About gpio-regulator setting on DT

[Mark Brown]

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

On Wed, Jan 29, 2014 at 04:08:30PM -0800, Kuninori Morimoto wrote:

>     /* 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;

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.

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

[PATCH] regulator: gpio: bugfix: add gpios-status for DT

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

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.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 .../bindings/regulator/gpio-regulator.txt          |    1 +
 drivers/regulator/gpio-regulator.c                 |   11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
index 63c6598..3ecb585 100644
--- a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
@@ -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).
 
diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index c0a1d00..7c8e37a 100644
--- a/drivers/regulator/gpio-regulator.c
+++ b/drivers/regulator/gpio-regulator.c
@@ -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

[PATCH] regulator: gpio: bugfix: add gpios-status for DT

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

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.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 .../bindings/regulator/gpio-regulator.txt          |    1 +
 drivers/regulator/gpio-regulator.c                 |   11 +++++++++++
 2 files changed, 12 insertions(+)

diff --git a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
index 63c6598..3ecb585 100644
--- a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
@@ -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).
 
diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index c0a1d00..7c8e37a 100644
--- a/drivers/regulator/gpio-regulator.c
+++ b/drivers/regulator/gpio-regulator.c
@@ -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

Re: [PATCH] regulator: gpio: bugfix: add gpios-status for DT

[Mark Brown]

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

On Thu, Jan 30, 2014 at 09:25:14PM -0800, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> config->gpios[x].flags indicates initial pin status,
> and it will be used for drvdata->state
> on gpio_regulator_probe().

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

> +- gpios-states		: gpios pin's initial states. 1 means HIGH

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.

> +		if (proplen != config->nr_gpios) {
> +			/* gpios <-> gpios-states mismatch */
> +			prop = NULL;
> +		}
> +	}

It's probably worth printing a warning here.

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

Re: [PATCH] regulator: gpio: bugfix: add gpios-status for DT

[Mark Brown]

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

On Thu, Jan 30, 2014 at 09:25:14PM -0800, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> config->gpios[x].flags indicates initial pin status,
> and it will be used for drvdata->state
> on gpio_regulator_probe().

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

> +- gpios-states		: gpios pin's initial states. 1 means HIGH

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.

> +		if (proplen != config->nr_gpios) {
> +			/* gpios <-> gpios-states mismatch */
> +			prop = NULL;
> +		}
> +	}

It's probably worth printing a warning here.

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

[PATCH 1/2] regulator: gpio: print warning if gpios <-> gpios-states mismatch on DT

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/regulator/gpio-regulator.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index ac3a8c7..5491cee 100644
--- a/drivers/regulator/gpio-regulator.c
+++ b/drivers/regulator/gpio-regulator.c
@@ -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

[PATCH 1/2] regulator: gpio: print warning if gpios <-> gpios-states mismatch on DT

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 drivers/regulator/gpio-regulator.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/regulator/gpio-regulator.c b/drivers/regulator/gpio-regulator.c
index ac3a8c7..5491cee 100644
--- a/drivers/regulator/gpio-regulator.c
+++ b/drivers/regulator/gpio-regulator.c
@@ -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

Re: [PATCH 1/2] regulator: gpio: print warning if gpios <-> gpios-states mismatch on DT

[Mark Brown]

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

On Tue, Feb 11, 2014 at 05:27:08PM -0800, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

Applied both, thanks.

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

[PATCH 2/2] regulator: gpio: explain detail of gpios-states

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

gpios-states is array, and default is 0

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 .../bindings/regulator/gpio-regulator.txt          |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
index 3ecb585..356e8bb 100644
--- a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
@@ -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

[PATCH 2/2] regulator: gpio: explain detail of gpios-states

[Kuninori Morimoto]

From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>

gpios-states is array, and default is 0

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
---
 .../bindings/regulator/gpio-regulator.txt          |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
index 3ecb585..356e8bb 100644
--- a/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/gpio-regulator.txt
@@ -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