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
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.
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
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.
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
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.
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.
Signed-off-by: Kuninori Morimoto <[email protected]>
---
.../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
On Thu, Jan 30, 2014 at 09:25:14PM -0800, Kuninori Morimoto wrote:
> 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().
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.
From: Kuninori Morimoto <[email protected]>
Signed-off-by: Kuninori Morimoto <[email protected]>
---
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
From: Kuninori Morimoto <[email protected]>
gpios-states is array, and default is 0
Signed-off-by: Kuninori Morimoto <[email protected]>
---
.../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
On Tue, Feb 11, 2014 at 05:27:08PM -0800, Kuninori Morimoto wrote:
> From: Kuninori Morimoto <[email protected]>
>
> Signed-off-by: Kuninori Morimoto <[email protected]>
Applied both, thanks.