[PATCH V1 0/3] Support for open drain gpios in gpilib/fixed regulators.

[PATCH V1 0/3] Support for open drain gpios in gpilib/fixed regulators.

[Laxman Dewangan]

This patch is as per discussion on the patch
[PATCH V1] regulator: fixed: Support for open-drain gpio 
where it was suggested to push the handling of open drain gpios
into the gpio libray.
Also gpio documentation says the possibility of open-drain gpio support.

This is series of patches  to add support of the open drain gpios
in standard gpio library and using this in fixed regulators.
- Added GPIOF_OD flag so that client can request the gpio with this flag.
- Added support for the sysfs interface for open-drain.
- Handle open drain gpio configuration properly.
- Use this flag in fixed regualtor to tell that gpio is open drain.
- Gpio documentation changes to have support for open-drain.

Laxman Dewangan (3):
  gpio: gpiolib: Support for open drain gpios
  Documentation: gpio: Add details of open-drain configuration
  regulator: fixed: Support for open-drain gpio

 Documentation/gpio.txt          |   21 +++++-
 drivers/gpio/gpiolib.c          |  144 ++++++++++++++++++++++++++++++++++++++-
 drivers/regulator/fixed.c       |   29 ++++----
 include/linux/gpio.h            |    3 +
 include/linux/regulator/fixed.h |    7 ++
 5 files changed, 184 insertions(+), 20 deletions(-)

[PATCH V1 1/3] gpio: gpiolib: Support for open drain gpios

[Laxman Dewangan]

Adding support for the open drain gpio on which client
can specify the open drain property through GPIO flag
at the time of gpio request.
The open drain pins cannot be driven to output with
value of 1 and so when client request for setting the
pin to HIGH, the gpio will be set to input direction
and hence PULL-UP on pins will make the state to HIGH.
The open drain pin can be driven to 0 by setting output
with value of 0.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 drivers/gpio/gpiolib.c |  144 +++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/gpio.h   |    3 +
 2 files changed, 145 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 17fdf4b..2347324 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -58,6 +58,7 @@ struct gpio_desc {
 #define FLAG_TRIG_FALL	5	/* trigger on falling edge */
 #define FLAG_TRIG_RISE	6	/* trigger on rising edge */
 #define FLAG_ACTIVE_LOW	7	/* sysfs value has active low */
+#define FLAG_OPEN_DRAIN 8	/* Gpio is open drain type */
 
 #define ID_SHIFT	16	/* add new flags before this one */
 
@@ -543,9 +544,68 @@ static ssize_t gpio_active_low_store(struct device *dev,
 static const DEVICE_ATTR(active_low, 0644,
 		gpio_active_low_show, gpio_active_low_store);
 
+static int sysfs_set_open_drain(struct gpio_desc *desc, struct device *dev,
+				int value)
+{
+	if (!!test_bit(FLAG_OPEN_DRAIN, &desc->flags) == !!value)
+		return 0;
+
+	if (value)
+		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
+	else
+		clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
+	return 0;
+}
+
+static ssize_t gpio_open_drain_show(struct device *dev,
+		struct device_attribute *attr, char *buf)
+{
+	const struct gpio_desc	*desc = dev_get_drvdata(dev);
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags))
+		status = -EIO;
+	else
+		status = sprintf(buf, "%d\n",
+				!!test_bit(FLAG_OPEN_DRAIN, &desc->flags));
+
+	mutex_unlock(&sysfs_lock);
+
+	return status;
+}
+
+static ssize_t gpio_open_drain_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t size)
+{
+	struct gpio_desc	*desc = dev_get_drvdata(dev);
+	ssize_t			status;
+
+	mutex_lock(&sysfs_lock);
+
+	if (!test_bit(FLAG_EXPORT, &desc->flags)) {
+		status = -EIO;
+	} else {
+		long		value;
+
+		status = strict_strtol(buf, 0, &value);
+		if (status == 0)
+			status = sysfs_set_open_drain(desc, dev, value != 0);
+	}
+
+	mutex_unlock(&sysfs_lock);
+
+	return status ? : size;
+}
+
+static const DEVICE_ATTR(open_drain, 0644,
+		gpio_open_drain_show, gpio_open_drain_store);
+
 static const struct attribute *gpio_attrs[] = {
 	&dev_attr_value.attr,
 	&dev_attr_active_low.attr,
+	&dev_attr_open_drain.attr,
 	NULL,
 };
 
@@ -864,6 +924,48 @@ done:
 EXPORT_SYMBOL_GPL(gpio_sysfs_set_active_low);
 
 /**
+ * gpio_sysfs_set_open_drain - set the open drain status of gpio sysfs value
+ * @gpio: gpio to change
+ * @value: non-zero to use open drain enable else open drain disable.
+ *
+ * Returns zero on success, else an error.
+ */
+int gpio_sysfs_set_open_drain(unsigned gpio, int value)
+{
+	struct gpio_desc	*desc;
+	struct device		*dev = NULL;
+	int			status = -EINVAL;
+
+	if (!gpio_is_valid(gpio))
+		goto done;
+
+	mutex_lock(&sysfs_lock);
+
+	desc = &gpio_desc[gpio];
+
+	if (test_bit(FLAG_EXPORT, &desc->flags)) {
+		dev = class_find_device(&gpio_class, NULL, desc, match_export);
+		if (dev == NULL) {
+			status = -ENODEV;
+			goto unlock;
+		}
+
+	}
+
+	status = sysfs_set_open_drain(desc, dev, value);
+
+unlock:
+	mutex_unlock(&sysfs_lock);
+
+done:
+	if (status)
+		pr_debug("%s: gpio%d status %d\n", __func__, gpio, status);
+
+	return status;
+}
+EXPORT_SYMBOL_GPL(gpio_sysfs_set_open_drain);
+
+/**
  * gpio_unexport - reverse effect of gpio_export()
  * @gpio: gpio to make unavailable
  *
@@ -1261,6 +1363,7 @@ void gpio_free(unsigned gpio)
 		module_put(desc->chip->owner);
 		clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
 		clear_bit(FLAG_REQUESTED, &desc->flags);
+		clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
 	} else
 		WARN_ON(extra_checks);
 
@@ -1282,6 +1385,9 @@ int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
 	if (err)
 		return err;
 
+	if (flags & GPIOF_OD)
+		set_bit(FLAG_OPEN_DRAIN, &gpio_desc[gpio].flags);
+
 	if (flags & GPIOF_DIR_IN)
 		err = gpio_direction_input(gpio);
 	else
@@ -1431,6 +1537,9 @@ int gpio_direction_output(unsigned gpio, int value)
 	struct gpio_desc	*desc = &gpio_desc[gpio];
 	int			status = -EINVAL;
 
+	if (value && test_bit(FLAG_OPEN_DRAIN,  &desc->flags))
+		return gpio_direction_input(gpio);
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	if (!gpio_is_valid(gpio))
@@ -1567,6 +1676,31 @@ int __gpio_get_value(unsigned gpio)
 }
 EXPORT_SYMBOL_GPL(__gpio_get_value);
 
+/*
+ *  _gpio_set_open_drain_value() - Set the open drain gpio's value.
+ * @gpio: Gpio whose state need to be set.
+ * @chip: Gpio chip.
+ * @value: Non-zero for setting it HIGH otherise it will set to LOW.
+ */
+static void _gpio_set_open_drain_value(unsigned gpio,
+			struct gpio_chip *chip, int value)
+{
+	int err = 0;
+	if (value) {
+		err = chip->direction_input(chip, gpio - chip->base);
+		if (!err)
+			clear_bit(FLAG_IS_OUT, &gpio_desc[gpio].flags);
+	} else {
+		err = chip->direction_output(chip, gpio - chip->base, 0);
+		if (!err)
+			set_bit(FLAG_IS_OUT, &gpio_desc[gpio].flags);
+	}
+	trace_gpio_direction(gpio, value, err);
+	if (err < 0)
+		pr_err("%s: Error in set_value for open drain gpio%d err %d\n",
+					__func__, gpio, err);
+}
+
 /**
  * __gpio_set_value() - assign a gpio's value
  * @gpio: gpio whose value will be assigned
@@ -1583,7 +1717,10 @@ void __gpio_set_value(unsigned gpio, int value)
 	chip = gpio_to_chip(gpio);
 	WARN_ON(chip->can_sleep);
 	trace_gpio_value(gpio, 0, value);
-	chip->set(chip, gpio - chip->base, value);
+	if (test_bit(FLAG_OPEN_DRAIN,  &gpio_desc[gpio].flags))
+		_gpio_set_open_drain_value(gpio, chip, value);
+	else
+		chip->set(chip, gpio - chip->base, value);
 }
 EXPORT_SYMBOL_GPL(__gpio_set_value);
 
@@ -1650,7 +1787,10 @@ void gpio_set_value_cansleep(unsigned gpio, int value)
 	might_sleep_if(extra_checks);
 	chip = gpio_to_chip(gpio);
 	trace_gpio_value(gpio, 0, value);
-	chip->set(chip, gpio - chip->base, value);
+	if (test_bit(FLAG_OPEN_DRAIN,  &gpio_desc[gpio].flags))
+		_gpio_set_open_drain_value(gpio, chip, value);
+	else
+		chip->set(chip, gpio - chip->base, value);
 }
 EXPORT_SYMBOL_GPL(gpio_set_value_cansleep);
 
diff --git a/include/linux/gpio.h b/include/linux/gpio.h
index 38ac48b..13b32dd 100644
--- a/include/linux/gpio.h
+++ b/include/linux/gpio.h
@@ -14,6 +14,9 @@
 #define GPIOF_OUT_INIT_LOW	(GPIOF_DIR_OUT | GPIOF_INIT_LOW)
 #define GPIOF_OUT_INIT_HIGH	(GPIOF_DIR_OUT | GPIOF_INIT_HIGH)
 
+/* Gpio pin is open drain */
+#define GPIOF_OD	(1 << 2)
+
 /**
  * struct gpio - a structure describing a GPIO with configuration
  * @gpio:	the GPIO number
-- 
1.7.1.1

[PATCH V1 2/3] Documentation: gpio: Add details of open-drain configuration

[Laxman Dewangan]

Adding details of open drain configuration of the gpio so that
client can set the pin as open drain at the time of gpio request.

Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>
---
 Documentation/gpio.txt |   21 +++++++++++++++++++--
 1 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
index 792faa3..b08933c 100644
--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -302,6 +302,7 @@ where 'flags' is currently defined to specify the following properties:
 
 	* GPIOF_INIT_LOW	- as output, set initial level to LOW
 	* GPIOF_INIT_HIGH	- as output, set initial level to HIGH
+	* GPIOF_OD		- gpio pin is open drain type.
 
 since GPIOF_INIT_* are only valid when configured as output, so group valid
 combinations as:
@@ -310,8 +311,7 @@ combinations as:
 	* GPIOF_OUT_INIT_LOW	- configured as output, initial level LOW
 	* GPIOF_OUT_INIT_HIGH	- configured as output, initial level HIGH
 
-In the future, these flags can be extended to support more properties such
-as open-drain status.
+In the future, these flags can be extended to support more properties.
 
 Further more, to ease the claim/release of multiple GPIOs, 'struct gpio' is
 introduced to encapsulate all three fields as:
@@ -641,6 +641,13 @@ and have the following read/write attributes:
 		for "rising" and "falling" edges will follow this
 		setting.
 
+	"open_drain" ... reads as either 0 (false) or 1 (true).  Write
+		any nonzero value to make the pin in open drain.
+		By setting open drain to true, the output can be set
+		to HIGH by external PULL UP and setting direction to input.
+		The output will be set to LOW by setting direction to
+		output with value is 0.
+
 GPIO controllers have paths like /sys/class/gpio/gpiochip42/ (for the
 controller implementing GPIOs starting at #42) and have the following
 read-only attributes:
@@ -679,6 +686,9 @@ requested using gpio_request():
 	/* change the polarity of a GPIO node in sysfs */
 	int gpio_sysfs_set_active_low(unsigned gpio, int value);
 
+	/* change the pin to open drain in sysfs */
+	int gpio_sysfs_set_open_drain(unsigned gpio, int value);
+
 After a kernel driver requests a GPIO, it may only be made available in
 the sysfs interface by gpio_export().  The driver can control whether the
 signal direction may change.  This helps drivers prevent userspace code
@@ -698,3 +708,10 @@ differences between boards from user space.  This only affects the
 sysfs interface.  Polarity change can be done both before and after
 gpio_export(), and previously enabled poll(2) support for either
 rising or falling edge will be reconfigured to follow this setting.
+
+Drivers can use gpio_sysfs_set_open_drain() to enable/disable the open
+drain property of that pins. This only affect the sysfs interface.
+The flag will be set as open drain if thsi function is called with value
+of 1. It is recommended to set the open drain property before setting
+the value in output mode so that pin state cn be set properly based
+on the value.
-- 
1.7.1.1

Re: [PATCH V1 2/3] Documentation: gpio: Add details of open-drain configuration

[Grant Likely]

On Mon, Feb 13, 2012 at 11:59:47AM +0530, Laxman Dewangan wrote:
> Adding details of open drain configuration of the gpio so that
> client can set the pin as open drain at the time of gpio request.
> 
> Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com>

Implicitly, the gpio api already supports open-drain and several drivers
make use of it.  Instead of being a separate flag; users who need open
drain will set the pin to input.  For example, see the i2c-gpio driver.

I'm not convinced this is needed; but my opinion could be swayed.

Linus mentioned that this should be part of pinctrl instead of the gpio API,
but I think there is an argument for making it part of the gpio API,
particularly since open-drain is pretty much a universal concept that all
gpio controllers can support (unlike driver strength) and as I said above, it
is already implicitly supported by gpiolib.  The difference with this
method is it would allow drivers like the gpio-i2c.c driver to set the
flag at gpio request time and then be able to always use gpio_set_value()
regardless of the pin mode.

However, I'm not thrilled about adding things to the already-horrible sysfs
abi.  Please drop that hunk entirely or put it into a separate patch so it
doesn't block the core functionality.

Also, you should include a patch to make i2c-gpio.c use this new
functionality as a proof-of-concept.  You may not be able to test it,
but it will make it a lot easier to justify merging by showing how it
cleans up a gpio user.

Have you though about support for lines that are pulled low instead of
high?  Those aren't as common, but it is conceivable that some
hardware would need it.

g.

> ---
>  Documentation/gpio.txt |   21 +++++++++++++++++++--
>  1 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/gpio.txt b/Documentation/gpio.txt
> index 792faa3..b08933c 100644
> --- a/Documentation/gpio.txt
> +++ b/Documentation/gpio.txt
> @@ -302,6 +302,7 @@ where 'flags' is currently defined to specify the following properties:
>  
>  	* GPIOF_INIT_LOW	- as output, set initial level to LOW
>  	* GPIOF_INIT_HIGH	- as output, set initial level to HIGH
> +	* GPIOF_OD		- gpio pin is open drain type.
>  
>  since GPIOF_INIT_* are only valid when configured as output, so group valid
>  combinations as:
> @@ -310,8 +311,7 @@ combinations as:
>  	* GPIOF_OUT_INIT_LOW	- configured as output, initial level LOW
>  	* GPIOF_OUT_INIT_HIGH	- configured as output, initial level HIGH
>  
> -In the future, these flags can be extended to support more properties such
> -as open-drain status.
> +In the future, these flags can be extended to support more properties.
>  
>  Further more, to ease the claim/release of multiple GPIOs, 'struct gpio' is
>  introduced to encapsulate all three fields as:
> @@ -641,6 +641,13 @@ and have the following read/write attributes:
>  		for "rising" and "falling" edges will follow this
>  		setting.
>  
> +	"open_drain" ... reads as either 0 (false) or 1 (true).  Write
> +		any nonzero value to make the pin in open drain.
> +		By setting open drain to true, the output can be set
> +		to HIGH by external PULL UP and setting direction to input.
> +		The output will be set to LOW by setting direction to
> +		output with value is 0.
> +
>  GPIO controllers have paths like /sys/class/gpio/gpiochip42/ (for the
>  controller implementing GPIOs starting at #42) and have the following
>  read-only attributes:
> @@ -679,6 +686,9 @@ requested using gpio_request():
>  	/* change the polarity of a GPIO node in sysfs */
>  	int gpio_sysfs_set_active_low(unsigned gpio, int value);
>  
> +	/* change the pin to open drain in sysfs */
> +	int gpio_sysfs_set_open_drain(unsigned gpio, int value);
> +
>  After a kernel driver requests a GPIO, it may only be made available in
>  the sysfs interface by gpio_export().  The driver can control whether the
>  signal direction may change.  This helps drivers prevent userspace code
> @@ -698,3 +708,10 @@ differences between boards from user space.  This only affects the
>  sysfs interface.  Polarity change can be done both before and after
>  gpio_export(), and previously enabled poll(2) support for either
>  rising or falling edge will be reconfigured to follow this setting.
> +
> +Drivers can use gpio_sysfs_set_open_drain() to enable/disable the open
> +drain property of that pins. This only affect the sysfs interface.
> +The flag will be set as open drain if thsi function is called with value
> +of 1. It is recommended to set the open drain property before setting
> +the value in output mode so that pin state cn be set properly based
> +on the value.
> -- 
> 1.7.1.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V1 2/3] Documentation: gpio: Add details of open-drain configuration

[Mark Brown]

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

On Mon, Feb 13, 2012 at 02:18:09PM -0700, Grant Likely wrote:
> On Mon, Feb 13, 2012 at 11:59:47AM +0530, Laxman Dewangan wrote:
> > Adding details of open drain configuration of the gpio so that
> > client can set the pin as open drain at the time of gpio request.

> Implicitly, the gpio api already supports open-drain and several drivers
> make use of it.  Instead of being a separate flag; users who need open
> drain will set the pin to input.  For example, see the i2c-gpio driver.

> I'm not convinced this is needed; but my opinion could be swayed.

The actual idea is to provide support for doing the switch to input to
drivers that just want to set a logic level and don't (at their level)
care one way or another if it's a CMOS or open drain output but instead
leaves it up to board configuration which mode is used.  Laxman posted a
patch for doing this to a regulator driver but looking at it the code
for emulating open drain while also maintaining support for regular CMOS
is fiddly enough that it seemed like it should be factored out of the
individual drivers.

> Also, you should include a patch to make i2c-gpio.c use this new
> functionality as a proof-of-concept.  You may not be able to test it,
> but it will make it a lot easier to justify merging by showing how it
> cleans up a gpio user.

The regulator patch is one example - I think things that could be CMOS
are probably more interesting here than drivers that always want open
drain as they have more of a complexity hit from needing to decide if
they'll use the emulation or not.

We could also at some later point add support for hardware which can
implement open drain mode itself though I'm not sure if there's enough
problem with emulating to make it worth the effort.

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

Re: [PATCH V1 2/3] Documentation: gpio: Add details of open-drain configuration

[Laxman Dewangan]

On Tuesday 14 February 2012 03:36 AM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
>> Implicitly, the gpio api already supports open-drain and several drivers
>> make use of it.  Instead of being a separate flag; users who need open
>> drain will set the pin to input.  For example, see the i2c-gpio driver.
>> I'm not convinced this is needed; but my opinion could be swayed.
> The actual idea is to provide support for doing the switch to input to
> drivers that just want to set a logic level and don't (at their level)
> care one way or another if it's a CMOS or open drain output but instead
> leaves it up to board configuration which mode is used.  Laxman posted a
> patch for doing this to a regulator driver but looking at it the code
> for emulating open drain while also maintaining support for regular CMOS
> is fiddly enough that it seemed like it should be factored out of the
> individual drivers.
>

Implementing open-drain handling in every gpio client (like 
i2c-gpio/regulator/fixed.c) is just duplicating the code everywhere. 
Also it leaves to have such implementation buggy which is in following 
piece of code (taken from i2c-gpio.c)

         if (pdata->sda_is_open_drain) {
                 gpio_direction_output(pdata->sda_pin, 1);
                 bit_data->setsda = i2c_gpio_setsda_val;
         } else {
                 gpio_direction_input(pdata->sda_pin);
                 bit_data->setsda = i2c_gpio_setsda_dir;
         }

Header says
  * @sda_is_open_drain: SDA is configured as open drain, i.e. the pin
  *      isn't actively driven high when setting the output value high.
  *      gpio_get_value() must return the actual pin state even if the
  *      pin is configured as an output.

And hence the check should be
if (!pdata->sda_is_open_drain) {
:::::
}


All these can be avoided by supporting open-drain flag and handling in 
gpio library.

>> Also, you should include a patch to make i2c-gpio.c use this new
>> functionality as a proof-of-concept.  You may not be able to test it,
>> but it will make it a lot easier to justify merging by showing how it
>> cleans up a gpio user.

I can do the cleanup in i2c-gpio also which will reduce lots of code if 
this change in gpiolib is acceptable and fine with everyone.
Let me know if patch regulator/fixed.c V1 is sufficient or not to 
justify. I will create the patch for i2c-gpio  also.

Re: [PATCH V1 2/3] Documentation: gpio: Add details of open-drain configuration

[Laxman Dewangan]

On Tuesday 14 February 2012 02:48 AM, Grant Likely wrote:
> On Mon, Feb 13, 2012 at 11:59:47AM +0530, Laxman Dewangan wrote:
>> Adding details of open drain configuration of the gpio so that
>> client can set the pin as open drain at the time of gpio request.
>>
>> Signed-off-by: Laxman Dewangan<ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>
> Linus mentioned that this should be part of pinctrl instead of the gpio API,
> but I think there is an argument for making it part of the gpio API,
> particularly since open-drain is pretty much a universal concept that all
> gpio controllers can support (unlike driver strength) and as I said above, it
> is already implicitly supported by gpiolib.

I am not sure about other soc but taking Tegra as example, the gpio 
controller is almost same for same series of soc. It does not very much 
about different version of the socs. The pins, number of pins. pin 
controls, pin is OD or not etc vary from variant of socs and so it is 
much more depends on the particular soc rather than  the major family.
Bringing pincontrol information in gpio driver will make the gpio driver 
complex which need to take care of every variant of chips.

>    The difference with this
> method is it would allow drivers like the gpio-i2c.c driver to set the
> flag at gpio request time and then be able to always use gpio_set_value()
> regardless of the pin mode.
>
> However, I'm not thrilled about adding things to the already-horrible sysfs
> abi.  Please drop that hunk entirely or put it into a separate patch so it
> doesn't block the core functionality.
>

I will split the change in two parts one for core driver and other for 
the sysfs interface if all changes are fine here.

> Have you though about support for lines that are pulled low instead of
> high?  Those aren't as common, but it is conceivable that some
> hardware would need it.
I think open drain pin should not be pulled low otherwise it will not be 
possible to make the pin as HIGH with the assumption that the OD pin 
should never be driven to HIGH

But if it is there in any case then it should be handle differently at  
client level without letting the gpio driver that it is open-drain.

Re: [PATCH V1 2/3] Documentation: gpio: Add details of open-drain configuration

[Linus Walleij]

On Mon, Feb 13, 2012 at 10:18 PM, Grant Likely
<grant.likely@secretlab.ca> wrote:

> Linus mentioned that this should be part of pinctrl instead of the gpio API,
> but I think there is an argument for making it part of the gpio API,
> particularly since open-drain is pretty much a universal concept that all
> gpio controllers can support (unlike driver strength)

Actually pinctrl is engineered to be used as back-end for gpiolib
so thinking about it I'm pretty happy with this arrangement, the gpiolib
driver can very well call down to pinctrl to have the actual setting
done if needed. So it's no big deal.

It is also a case for making some of the pin config business
generic, which I have failed at in the past.

> Have you though about support for lines that are pulled low instead of
> high?  Those aren't as common, but it is conceivable that some
> hardware would need it.

So if the idea is (if I get it correctly) that this thing is an input
sometimes and open drain/collector output sometimes, then
open source/emitter for the inverse situation is an equally valid
case right? In that case I think it'd be best to add both.

The COH901 driver for U300 supports open source/emitter
BTW.

Yours,
Linus Walleij

Re: [PATCH V1 2/3] Documentation: gpio: Add details of open-drain configuration

[Laxman Dewangan]

On Thursday 16 February 2012 03:55 AM, Linus Walleij wrote:
>> Have you though about support for lines that are pulled low instead of
>> high?  Those aren't as common, but it is conceivable that some
>> hardware would need it.
> So if the idea is (if I get it correctly) that this thing is an input
> sometimes and open drain/collector output sometimes, then
> open source/emitter for the inverse situation is an equally valid
> case right? In that case I think it'd be best to add both.
>
> The COH901 driver for U300 supports open source/emitter
> BTW.
>

Yes, I can add the open source also but like to be in incremental 
change. Not together with open drain.
We can go with:
- open drain core driver support.
- open drain sysfs interface support
- open source core driver support
- open source sysfs interface.

I have already changes for the first 2 which we can review/apply,
Meanwhile I will work on open source support.

Does it look good?

> Yours,
> Linus Walleij

Re: [PATCH V1 2/3] Documentation: gpio: Add details of open-drain configuration

[Linus Walleij]

On Thu, Feb 16, 2012 at 9:28 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:

> Yes, I can add the open source also but like to be in incremental change.
> Not together with open drain.
> We can go with:
> - open drain core driver support.
> - open drain sysfs interface support
> - open source core driver support
> - open source sysfs interface.
>
> I have already changes for the first 2 which we can review/apply,
> Meanwhile I will work on open source support.
>
> Does it look good?

Well as a patch concept it is OK but I have real bad feelings about
patch  [2/4] and [4/4] adding sysfs.

What the GPIO subsystem really needs is to get rid of sysfs
interfaces, have them deprecated and replaced with a
per-gpiochip /dev/gpioN with ioctl()s or similar instead. The sysfs
interface is just ... it won't scale.

Yours,
Linus Walleij

Re: [PATCH V1 2/3] Documentation: gpio: Add details of open-drain configuration

[Laxman Dewangan]

On Friday 17 February 2012 01:30 AM, Linus Walleij wrote:
> On Thu, Feb 16, 2012 at 9:28 AM, Laxman Dewangan<ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>  wrote:
>
>> Yes, I can add the open source also but like to be in incremental change.
>> Not together with open drain.
>> We can go with:
>> - open drain core driver support.
>> - open drain sysfs interface support
>> - open source core driver support
>> - open source sysfs interface.
>>
>> I have already changes for the first 2 which we can review/apply,
>> Meanwhile I will work on open source support.
>>
>> Does it look good?
> Well as a patch concept it is OK but I have real bad feelings about
> patch  [2/4] and [4/4] adding sysfs.
>
> What the GPIO subsystem really needs is to get rid of sysfs
> interfaces, have them deprecated and replaced with a
> per-gpiochip /dev/gpioN with ioctl()s or similar instead. The sysfs
> interface is just ... it won't scale.
>
Wow, then I will not add 2/4 and 4/4.

> Yours,
> Linus Walleij

[PATCH V1 3/3] regulator: fixed: Support for open-drain gpio

[Laxman Dewangan]

Some of regulator switches like bidirectional load switches
connected through the open drain gpios. As open drain gpios
should not be driven to output with value of 1, it needs to
handle differently to set the pin state to HIGH.
Adding the flag to find out if gpio is open drain and pass
this information to gpio library for handling open-drain gpio
configuration accordingly.

Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/regulator/fixed.c       |   29 +++++++++++++----------------
 include/linux/regulator/fixed.h |    7 +++++++
 2 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index e24e3a1..d6c8d66 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -199,7 +199,9 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 	drvdata->startup_delay = config->startup_delay;
 
 	if (gpio_is_valid(config->gpio)) {
+		int gpio_flag;
 		drvdata->enable_high = config->enable_high;
+		drvdata->is_enabled = config->enabled_at_boot;
 
 		/* FIXME: Remove below print warning
 		 *
@@ -216,29 +218,24 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 			dev_warn(&pdev->dev,
 				"using GPIO 0 for regulator enable control\n");
 
-		ret = gpio_request(config->gpio, config->supply_name);
-		if (ret) {
-			dev_err(&pdev->dev,
-			   "Could not obtain regulator enable GPIO %d: %d\n",
-							config->gpio, ret);
-			goto err_name;
-		}
-
-		/* set output direction without changing state
+		/*
+		 * set output direction without changing state
 		 * to prevent glitch
 		 */
-		drvdata->is_enabled = config->enabled_at_boot;
-		ret = drvdata->is_enabled ?
-				config->enable_high : !config->enable_high;
+		gpio_flag = drvdata->is_enabled ?
+				GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW;
+
+		if (config->gpio_is_open_drain)
+			gpio_flag |= GPIOF_OD;
 
-		ret = gpio_direction_output(config->gpio, ret);
+		ret = gpio_request_one(config->gpio, gpio_flag,
+						config->supply_name);
 		if (ret) {
 			dev_err(&pdev->dev,
-			   "Could not configure regulator enable GPIO %d direction: %d\n",
+			   "Could not obtain regulator enable GPIO %d: %d\n",
 							config->gpio, ret);
-			goto err_gpio;
+			goto err_name;
 		}
-
 	} else {
 		/* Regulator without GPIO control is considered
 		 * always enabled
diff --git a/include/linux/regulator/fixed.h b/include/linux/regulator/fixed.h
index ffd7d50..350edb5 100644
--- a/include/linux/regulator/fixed.h
+++ b/include/linux/regulator/fixed.h
@@ -26,6 +26,12 @@ struct regulator_init_data;
  * @gpio:		GPIO to use for enable control
  * 			set to -EINVAL if not used
  * @startup_delay:	Start-up time in microseconds
+ * @gpio_is_open_drain: Gpio pin is open drain or normal type.
+ *			If it is open drain then HIGH will be set
+ *			through PULL-UP and gpio will be set as input
+ *			and low will be set as gpio-output with driven
+ *			to low. For non-open-drain case, the gpio will
+ *			will be in output and drive to low/high accordingly.
  * @enable_high:	Polarity of enable GPIO
  *			1 = Active high, 0 = Active low
  * @enabled_at_boot:	Whether regulator has been enabled at
@@ -43,6 +49,7 @@ struct fixed_voltage_config {
 	int microvolts;
 	int gpio;
 	unsigned startup_delay;
+	unsigned gpio_is_open_drain:1;
 	unsigned enable_high:1;
 	unsigned enabled_at_boot:1;
 	struct regulator_init_data *init_data;
-- 
1.7.1.1

Re: [PATCH V1 0/3] Support for open drain gpios in gpilib/fixed regulators.

[Linus Walleij]

On Mon, Feb 13, 2012 at 7:29 AM, Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:

> This is series of patches  to add support of the open drain gpios
> in standard gpio library and using this in fixed regulators.
> - Added GPIOF_OD flag so that client can request the gpio with this flag.
> - Added support for the sysfs interface for open-drain.
> - Handle open drain gpio configuration properly.
> - Use this flag in fixed regualtor to tell that gpio is open drain.
> - Gpio documentation changes to have support for open-drain.

My gut feeling is that this stuff needs to go into pinctrl.

The reason, which has been discussed when similar patches (from me,
for example) were NACK:ed in the past, is that if we put in OD pin
properties, we put in all pin properties (open source, drive strength,
biasing pull-up/pull-down etc etc) and that is the reason to why the
orthogonal pinctrl subsystem was created in the first place.

So I'd opt for moving the driver you need to drivers/pinctrl and
expose the gpiolib interface from there, while adding pinctrl
on the side.

Also, we had a flamefest a while back as to whether we should
enumerate all config options (like OD, OS...) or have it all-custom,
and I was sort of shot down on that. But I have a generic control patch
set boiling so if you like that kind of stuff I'll respin it.

Yours,
Linus Walleij

Re: [PATCH V1 0/3] Support for open drain gpios in gpilib/fixed regulators.

[Mark Brown]

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

On Mon, Feb 13, 2012 at 09:02:03PM +0100, Linus Walleij wrote:
> On Mon, Feb 13, 2012 at 7:29 AM, Laxman Dewangan <ldewangan@nvidia.com> wrote:

> > This is series of patches  to add support of the open drain gpios
> > in standard gpio library and using this in fixed regulators.
> > - Added GPIOF_OD flag so that client can request the gpio with this flag.
> > - Added support for the sysfs interface for open-drain.
> > - Handle open drain gpio configuration properly.
> > - Use this flag in fixed regualtor to tell that gpio is open drain.
> > - Gpio documentation changes to have support for open-drain.

> My gut feeling is that this stuff needs to go into pinctrl.

Does that really make sense for cases like this where we're emulating
the open drain support rather than actually configuring the hardware
into an open drain mode?  That emulation is a pretty GPIO specific
thing.

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