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

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

[Laxman Dewangan]

The open drain pins act as bidirectional and it should
not be driven to high as it can cause power to ground
shorting in some cases. And hence gpio which is controlling
this pin should not drive output to high.
To make the pin to high/low for enabling/disabling the switches,
the pins will have the pull-up connected and the high/low can be
set using following methods:
LOW: gpio_direction_output(gpio, 0) ... this drives the signal
	and overrides the pullup.
HIGH: gpio_direction_input(gpio) ... this turns off the output,
	so the pullup (or some other device) controls the signal.

Implementing a mechanism to properly handle the open-drain gpio
to control the regulator switches.

Signed-off-by: Laxman Dewangan <ldewangan-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
-Adding the flag in platform data to tell whether gpio is
 open-drain or not.
- Based on flag, setting the pins state. If it is non-open-drain
  then driver output to high/low. if it is open drain then only
  driver low or set to input for making the pin to high through
  pullups.


 drivers/regulator/fixed.c       |   42 ++++++++++++++++++++++++++++++++++++--
 include/linux/regulator/fixed.h |    7 ++++++
 2 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index e24e3a1..d47d684 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -40,6 +40,7 @@ struct fixed_voltage_data {
 	unsigned startup_delay;
 	bool enable_high;
 	bool is_enabled;
+	bool gpio_is_open_drain;
 };
 
 
@@ -94,6 +95,20 @@ of_get_fixed_voltage_config(struct device *dev)
 	return config;
 }
 
+static int set_open_drain_gpio(struct device *dev, int gpio, bool is_high)
+{
+	int ret;
+	if (is_high)
+		ret = gpio_direction_input(gpio);
+	else
+		ret = gpio_direction_output(gpio, 0);
+	if (ret < 0)
+		dev_err(dev,
+		    "Could not configure open drain GPIO %d direction: %d\n",
+				gpio, ret);
+	return ret;
+}
+
 static int fixed_voltage_is_enabled(struct regulator_dev *dev)
 {
 	struct fixed_voltage_data *data = rdev_get_drvdata(dev);
@@ -106,7 +121,15 @@ static int fixed_voltage_enable(struct regulator_dev *dev)
 	struct fixed_voltage_data *data = rdev_get_drvdata(dev);
 
 	if (gpio_is_valid(data->gpio)) {
-		gpio_set_value_cansleep(data->gpio, data->enable_high);
+		if (data->gpio_is_open_drain) {
+			struct device *parent_dev = rdev_get_dev(dev);
+			int ret;
+			ret = set_open_drain_gpio(parent_dev, data->gpio,
+					data->enable_high);
+			if (ret < 0)
+				return ret;
+		} else
+			gpio_set_value_cansleep(data->gpio, data->enable_high);
 		data->is_enabled = true;
 	}
 
@@ -118,7 +141,15 @@ static int fixed_voltage_disable(struct regulator_dev *dev)
 	struct fixed_voltage_data *data = rdev_get_drvdata(dev);
 
 	if (gpio_is_valid(data->gpio)) {
-		gpio_set_value_cansleep(data->gpio, !data->enable_high);
+		if (data->gpio_is_open_drain) {
+			struct device *parent_dev = rdev_get_dev(dev);
+			int ret;
+			ret = set_open_drain_gpio(parent_dev, data->gpio,
+					!data->enable_high);
+			if (ret < 0)
+				return ret;
+		} else
+			gpio_set_value_cansleep(data->gpio, !data->enable_high);
 		data->is_enabled = false;
 	}
 
@@ -200,6 +231,7 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 
 	if (gpio_is_valid(config->gpio)) {
 		drvdata->enable_high = config->enable_high;
+		drvdata->gpio_is_open_drain = config->gpio_is_open_drain;
 
 		/* FIXME: Remove below print warning
 		 *
@@ -231,7 +263,11 @@ static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 		ret = drvdata->is_enabled ?
 				config->enable_high : !config->enable_high;
 
-		ret = gpio_direction_output(config->gpio, ret);
+		if (drvdata->gpio_is_open_drain)
+			ret = set_open_drain_gpio(&pdev->dev, config->gpio,
+							ret);
+		else
+			ret = gpio_direction_output(config->gpio, ret);
 		if (ret) {
 			dev_err(&pdev->dev,
 			   "Could not configure regulator enable GPIO %d direction: %d\n",
diff --git a/include/linux/regulator/fixed.h b/include/linux/regulator/fixed.h
index ffd7d50..9dcd135 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 is normal or open-drain.
+ *			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] regulator: fixed: Support for open-drain gpio

[Mark Brown]

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

On Tue, Feb 07, 2012 at 04:16:36PM +0530, Laxman Dewangan wrote:

> To make the pin to high/low for enabling/disabling the switches,
> the pins will have the pull-up connected and the high/low can be
> set using following methods:
> LOW: gpio_direction_output(gpio, 0) ... this drives the signal
> 	and overrides the pullup.
> HIGH: gpio_direction_input(gpio) ... this turns off the output,
> 	so the pullup (or some other device) controls the signal.

Thinking about this further this is potentially going to apply to any
GPIO output - perhaps we should push the implementation down into
gpiolib so we just specify a flag when requesting the GPIO and then
gpiolib does the pull low/high Z thing for us.  That would be much less
effort in individual drivers, we'd just need to be able to set the flag
and could potentially directly use any hardware open drain output
support (some GPIO controllers will do all this in hardware) if the
driver API were extended.  Grant, does that sound reasonable?

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

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

[Laxman Dewangan]

On Tuesday 07 February 2012 05:13 PM, Mark Brown wrote:
> * PGP Signed by an unknown key
>
> On Tue, Feb 07, 2012 at 04:16:36PM +0530, Laxman Dewangan wrote:
>
>> To make the pin to high/low for enabling/disabling the switches,
>> the pins will have the pull-up connected and the high/low can be
>> set using following methods:
>> LOW: gpio_direction_output(gpio, 0) ... this drives the signal
>> 	and overrides the pullup.
>> HIGH: gpio_direction_input(gpio) ... this turns off the output,
>> 	so the pullup (or some other device) controls the signal.
> Thinking about this further this is potentially going to apply to any
> GPIO output - perhaps we should push the implementation down into
> gpiolib so we just specify a flag when requesting the GPIO and then
> gpiolib does the pull low/high Z thing for us.  That would be much less
> effort in individual drivers, we'd just need to be able to set the flag
> and could potentially directly use any hardware open drain output
> support (some GPIO controllers will do all this in hardware) if the
> driver API were extended.  Grant, does that sound reasonable?
>
If Grant agree on this then we can have following thing:
- we have already apis like gpio_request_one() and 
gpio_request_multiple() which take the flags.
- We need to add the flag GPIOF_OD in current list of flags for requester.
- In the gpio_request_xx(), just store this flag in gpio_desc for 
corresponding gpios.
- When client set direction_output() with val 1 then do 
direction_input(). So check will be in function direction_output()
- When client  call set_value(), again check for value ==1 and GPIO_OD 
and if it is there, call direction_input.

On client side change, call proper request function with proper flag 
(with GPIOF_OD if it is require).

Does it sound good?

> * Unknown Key
> * 0x6E30FDDD