[PATCH V1] regulator: fixed: dt: add property for gpio open drain flag

[PATCH V1] regulator: fixed: dt: add property for gpio open drain flag

[Laxman Dewangan]

Add property for the gpio flag open drain when registering
fixed regulator.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 .../bindings/regulator/fixed-regulator.txt         |    5 ++++-
 drivers/regulator/fixed.c                          |    3 +++
 2 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
index 9cf57fd..2f5b6b1 100644
--- a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
+++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
@@ -8,6 +8,8 @@ Optional properties:
 - startup-delay-us: startup time in microseconds
 - enable-active-high: Polarity of GPIO is Active high
 If this property is missing, the default assumed is Active low.
+- gpio-open-drain: GPIO is open drain type.
+  If this property is missing then default assumption is false.
 
 Any property defined as part of the core regulator
 binding, defined in regulator.txt, can also be used.
@@ -25,5 +27,6 @@ Example:
 		gpio = <&gpio1 16 0>;
 		startup-delay-us = <70000>;
 		enable-active-high;
-		regulator-boot-on
+		regulator-boot-on;
+		gpio-open-drain;
 	};
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index bd437c1..f09fe7b 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -90,6 +90,9 @@ of_get_fixed_voltage_config(struct device *dev)
 	if (of_find_property(np, "enable-active-high", NULL))
 		config->enable_high = true;
 
+	if (of_find_property(np, "gpio-open-drain", NULL))
+		config->gpio_is_open_drain = true;
+
 	return config;
 }
 
-- 
1.7.1.1

Re: [PATCH V1] regulator: fixed: dt: add property for gpio open drain flag

[Mark Brown]

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

On Mon, May 07, 2012 at 03:58:19PM +0530, Laxman Dewangan wrote:
> Add property for the gpio flag open drain when registering
> fixed regulator.

Applied, thanks.

There's no need to put stuff like "v1" in your patch description, it can
sometimes be useful to do this if it's a revision of a previous patch
but if it's the first version it's not adding anything.

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

Re: [PATCH V1] regulator: fixed: dt: add property for gpio open drain flag

[Stephen Warren]

On 05/07/2012 04:28 AM, Laxman Dewangan wrote:
> Add property for the gpio flag open drain when registering
> fixed regulator.

> diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt ...

> +- gpio-open-drain: GPIO is open drain type.
> +  If this property is missing then default assumption is false.

Given that there's a GPIOF_OPEN_DRAIN, isn't the GPIO specifier in the
gpio property meant to encode this somehow (the format is up to the GPIO
controller's binding) - the flags are returned by of_get_named_gpio_flags().

(especially given you asked me off-list about updating the GPIO binding
documentation with information about the flags that Tegra's binding
documented for this)

Re: [PATCH V1] regulator: fixed: dt: add property for gpio open drain flag

[Laxman Dewangan]

On Monday 07 May 2012 08:35 PM, Stephen Warren wrote:
> On 05/07/2012 04:28 AM, Laxman Dewangan wrote:
>> Add property for the gpio flag open drain when registering
>> fixed regulator.
>> diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt ...
>> +- gpio-open-drain: GPIO is open drain type.
>> +  If this property is missing then default assumption is false.
> Given that there's a GPIOF_OPEN_DRAIN, isn't the GPIO specifier in the
> gpio property meant to encode this somehow (the format is up to the GPIO
> controller's binding) - the flags are returned by of_get_named_gpio_flags().
>
> (especially given you asked me off-list about updating the GPIO binding
> documentation with information about the flags that Tegra's binding
> documented for this)

gpio of binding does not explain that there is always gpio flag  
(GPIOF_*) in gpio cell argument. It is up to the chip specific gpio 
driver how they implement the argumnets.
For the example of gpio-samsung have the 4 argument list and all are for 
different purpose. And if they want to set pin as open drain, it will 
not be possible  on their argument list as none of the cell argument 
reflect to GPIOF_* flag.
For the case of tegra, there is no of_xlate() implementation and hence 
it is by default return as GPIOF_.

And the client driver which is indpendent of chip specific gpio driver 
should able to handle this.

Re: [PATCH V1] regulator: fixed: dt: add property for gpio open drain flag

[Stephen Warren]

On 05/07/2012 09:08 AM, Laxman Dewangan wrote:
> On Monday 07 May 2012 08:35 PM, Stephen Warren wrote:
>> On 05/07/2012 04:28 AM, Laxman Dewangan wrote:
>>> Add property for the gpio flag open drain when registering
>>> fixed regulator.
>>> diff --git
>>> a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt ...
>>> +- gpio-open-drain: GPIO is open drain type.
>>> +  If this property is missing then default assumption is false.
>> Given that there's a GPIOF_OPEN_DRAIN, isn't the GPIO specifier in the
>> gpio property meant to encode this somehow (the format is up to the GPIO
>> controller's binding) - the flags are returned by
>> of_get_named_gpio_flags().
>>
>> (especially given you asked me off-list about updating the GPIO binding
>> documentation with information about the flags that Tegra's binding
>> documented for this)
> 
> gpio of binding does not explain that there is always gpio flag 
> (GPIOF_*) in gpio cell argument. It is up to the chip specific gpio
> driver how they implement the argumnets.
> For the example of gpio-samsung have the 4 argument list and all are for
> different purpose. And if they want to set pin as open drain, it will
> not be possible  on their argument list as none of the cell argument
> reflect to GPIOF_* flag.
> For the case of tegra, there is no of_xlate() implementation and hence
> it is by default return as GPIOF_.
> 
> And the client driver which is indpendent of chip specific gpio driver
> should able to handle this.

Yes, there are some issues to be resolved here.

My point is that there's clear intent for of_gpio_simple_xlate() to
return the GPIO flags from a field in the GPIO specifier in device tree,
irrespective of whether some GPIO bindings don't allow this, or whether
this is fully documented.

However, this patch introduces an alternative mechanism to specify some
of the flags.

We shouldn't have two different mechanisms to represent this
information. How do we resolve the situation where a particular SoC's
GPIO binding allows the flags to be specified, and this patch allows the
open-drain flag to be specified, and the two values are different?

Either we should force all bindings to have some way of specifying the
flags, or we should remove the flags from all bindings and rely on
separate properties.

IIRC, a similar thing came up with GPIO inversion, perhaps in Arnd's
recent unified SDHCI bindings. However, I don't think we really resolved
the issue during the discussion there.

Re: [PATCH V1] regulator: fixed: dt: add property for gpio open drain flag

[Mark Brown]

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

On Mon, May 07, 2012 at 09:20:33AM -0600, Stephen Warren wrote:

> My point is that there's clear intent for of_gpio_simple_xlate() to
> return the GPIO flags from a field in the GPIO specifier in device tree,
> irrespective of whether some GPIO bindings don't allow this, or whether
> this is fully documented.

It's really not idiomatic for drivers using GPIOs to allow configuration
of their flags in the first place.  Or, quite frankly, to use flags.
Honestly I'd not expect any bindings that the GPIO driver provides to
have any attention at all paid to them most of the time, looking at the
code seems to support that (though there's surprisingly few bindings
using GPIOs at all it seems).

Before we start doing stuff to improve this it feels like we should do
things like get devm_ versions of gpiolib APIs in there (since most
conversions would really benefit from that too) - I've been sending a
patch for that but my backlog of unmerged patches for it keeps getting
bigger rather than smaller.

> We shouldn't have two different mechanisms to represent this
> information. How do we resolve the situation where a particular SoC's
> GPIO binding allows the flags to be specified, and this patch allows the
> open-drain flag to be specified, and the two values are different?

> Either we should force all bindings to have some way of specifying the
> flags, or we should remove the flags from all bindings and rely on
> separate properties.

There's also the case where the bindings probably should be ignored
because the device driver knows what the connection has to be anyway and
there's no point in forcing people to encode it in the binding.

It's a real shame all these bindings are driver specific, it's just like
interrupts :(

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

Re: [PATCH V1] regulator: fixed: dt: add property for gpio open drain flag

[Stephen Warren]

On 05/07/2012 10:24 AM, Mark Brown wrote:
> On Mon, May 07, 2012 at 09:20:33AM -0600, Stephen Warren wrote:
> 
>> My point is that there's clear intent for of_gpio_simple_xlate()
>> to return the GPIO flags from a field in the GPIO specifier in
>> device tree, irrespective of whether some GPIO bindings don't
>> allow this, or whether this is fully documented.
> 
> It's really not idiomatic for drivers using GPIOs to allow
> configuration of their flags in the first place.  Or, quite
> frankly, to use flags. Honestly I'd not expect any bindings that
> the GPIO driver provides to have any attention at all paid to them
> most of the time, looking at the code seems to support that (though
> there's surprisingly few bindings using GPIOs at all it seems).

So, are you saying we should remove the flags from the GPIO bindings,
and do something custom in each consumer?

Right now, we can't rely on of_get_gpio_flags(), since not all GPIO
drivers actually allow flags to be represented in bindings.

However, having each client have a unique binding for things like
open-drain, inverted, ... seems bad; there are probably fewer
providers than consumers, although a single standardized
representation might admittedly have been ever better.

> Before we start doing stuff to improve this it feels like we should
> do things like get devm_ versions of gpiolib APIs in there (since
> most conversions would really benefit from that too) - I've been
> sending a patch for that but my backlog of unmerged patches for it
> keeps getting bigger rather than smaller.
> 
>> We shouldn't have two different mechanisms to represent this 
>> information. How do we resolve the situation where a particular
>> SoC's GPIO binding allows the flags to be specified, and this
>> patch allows the open-drain flag to be specified, and the two
>> values are different?
> 
>> Either we should force all bindings to have some way of
>> specifying the flags, or we should remove the flags from all
>> bindings and rely on separate properties.
> 
> There's also the case where the bindings probably should be
> ignored because the device driver knows what the connection has to
> be anyway and there's no point in forcing people to encode it in
> the binding.
> 
> It's a real shame all these bindings are driver specific, it's just
> like interrupts :(

Re: [PATCH V1] regulator: fixed: dt: add property for gpio open drain flag

[Mark Brown]

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

On Tue, May 08, 2012 at 03:26:16PM -0600, Stephen Warren wrote:
> On 05/07/2012 10:24 AM, Mark Brown wrote:

> > It's really not idiomatic for drivers using GPIOs to allow
> > configuration of their flags in the first place.  Or, quite
> > frankly, to use flags. Honestly I'd not expect any bindings that
> > the GPIO driver provides to have any attention at all paid to them
> > most of the time, looking at the code seems to support that (though
> > there's surprisingly few bindings using GPIOs at all it seems).

> So, are you saying we should remove the flags from the GPIO bindings,
> and do something custom in each consumer?

Not really, it's more internal Linux stuff with the gpio API being a bit
stuck.

> Right now, we can't rely on of_get_gpio_flags(), since not all GPIO
> drivers actually allow flags to be represented in bindings.

> However, having each client have a unique binding for things like
> open-drain, inverted, ... seems bad; there are probably fewer
> providers than consumers, although a single standardized

It really depends what the device is doing with the GPIO, we would at
least need to have a way for the user to get the flags back out in case
the configuration is important for what it does.  Most of this stuff is
going to be nonsensical for most uses anyway, though, and obviously
there's the whole middleware emulation thing as well.

> representation might admittedly have been ever better.

Well, that's legacy device tree stuff for you :/

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