regulator: Add GPIO enable control to fixed voltage regulator driver

From: Roger Quadros <[email protected]>

Now fixed regulators that have their enable pin connected to a GPIO line can use the fixed regulator driver. The GPIO number and polarity information is passed through platform data. GPIO enable control is acheived using gpiolib.

Signed-off-by Roger Quadros <[email protected]>

Reviewed-by Mark Brown <[email protected]>

Reviewed-by Philipp Zabel <[email protected]>

---
 drivers/regulator/fixed.c       |   66 +++++++++++++++++++++++++++++++++++++-
 include/linux/regulator/fixed.h |   17 ++++++++++
 2 files changed, 81 insertions(+), 2 deletions(-)

drivers/regulator/fixed.c +0 -0

index cdc674f..bcebb0c 100644
@@ -20,20 +20,50 @@
 #include <linux/platform_device.h>
 #include <linux/regulator/driver.h>

drivers/regulator/fixed.c +5 -1

 };
 
 static int fixed_voltage_is_enabled(struct regulator_dev *dev)
 {
-	return 1;
+	struct fixed_voltage_data *data = rdev_get_drvdata(dev);
+
+	if (data->gpio)
+		return data->is_enabled;
+	else

drivers/regulator/fixed.c +5 -0

+		drvdata->gpio = config->gpio;
+		drvdata->enable_high = config->enable_high;
+
+		ret = gpio_request(config->gpio,
+					config->supply_name);

drivers/regulator/fixed.c +2 -1

+		dev_warn(&pdev->dev, "Not using GPIO 0 for enable control\n");
 
 	drvdata->dev = regulator_register(&drvdata->desc, &pdev->dev,
 					  config->init_data, drvdata);
 	if (IS_ERR(drvdata->dev)) {
 		ret = PTR_ERR(drvdata->dev);
-		goto err_name;
+		goto err_gpio;
 	}
 
 	platform_set_drvdata(pdev, drvdata);

drivers/regulator/fixed.c +2 -0

+		gpio_free(drvdata->gpio);
+
 	return 0;
 }
 

include/linux/regulator/fixed.h +9 -0

index 91b4da3..b40e06b 100644
@@ -16,9 +16,26 @@
 
 struct regulator_init_data;
 
+/**
+ * struct fixed_voltage_config - fixed_voltage_config structure
+ * @supply_name:	Name of the regulator supply
+ * @microvolts:		Output voltage of regulator
+ * @use_gpio_control:	Use GPIO enable control
+ * @gpio:		GPIO to use for enable control
+ * @enable_high:	Polarity of enable GPIO
+ *			1 = Active high, 0 = Active low
+ * @init_data:		regulator_init_data

From: Roger Quadros <[email protected]>

Now fixed regulators that have their enable pin connected to a GPIO line can use the fixed regulator driver. The GPIO number and polarity information is passed through platform data. GPIO enable control is acheived using gpiolib.

Signed-off-by Roger Quadros <[email protected]>

Reviewed-by Mark Brown <[email protected]>

Reviewed-by Philipp Zabel <[email protected]>

---
 drivers/regulator/fixed.c       |   66 +++++++++++++++++++++++++++++++++++++- include/linux/regulator/fixed.h |   17 ++++++++++

 2 files changed, 81 insertions(+), 2 deletions(-)

drivers/regulator/fixed.c +2 -0

index cdc674f..bcebb0c 100644
@@ -20,20 +20,50 @@
  #include <linux/platform_device.h>
  #include <linux/regulator/driver.h>
  #include <linux/regulator/fixed.h>
+#include <linux/gpio.h>
 
  struct fixed_voltage_data {
        struct regulator_desc desc;
        struct regulator_dev *dev;
        int microvolts;
+       int gpio;

drivers/regulator/fixed.c +19 -0

+               gpio_set_value(data->gpio,
+                               data->enable_high ? 1 : 0);
+               data->is_enabled = 1;
+       }
+
+       return 0;
+}
+
+static int fixed_voltage_disable(struct regulator_dev *dev)
+{
+       struct fixed_voltage_data *data = rdev_get_drvdata(dev);
+
+       if (data->gpio) {
+               gpio_set_value(data->gpio,
+                               data->enable_high ? 0 : 1);
+               data->is_enabled = 0;
+       }
+
        return 0;
  }
 

@@ -58,6 +88,7 @@ static int fixed_voltage_list_voltage(struct regulator_dev *dev,
  static struct regulator_ops fixed_voltage_ops = {
        .is_enabled = fixed_voltage_is_enabled,
        .enable = fixed_voltage_enable,
+       .disable = fixed_voltage_disable,
        .get_voltage = fixed_voltage_get_voltage,
        .list_voltage = fixed_voltage_list_voltage,
  };

drivers/regulator/fixed.c +25 -1

+               drvdata->gpio = config->gpio;
+               drvdata->enable_high = config->enable_high;
+
+               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
+                * to prevent glitch
+                */
+               ret = gpio_get_value(config->gpio);
+               ret = gpio_direction_output(config->gpio, ret);
+               if (ret) {
+                       dev_err(&pdev->dev, "Could not configure "
+                               "enable GPIO %d direction: %d\n",
+                                               config->gpio, ret);
+                       goto err_gpio;
+               }
+       } else
+               dev_warn(&pdev->dev, "Not using GPIO 0 for enable control\n");
 
        drvdata->dev = regulator_register(&drvdata->desc, &pdev->dev,
                                          config->init_data, drvdata);
        if (IS_ERR(drvdata->dev)) {
                ret = PTR_ERR(drvdata->dev);
-               goto err_name;
+               goto err_gpio;
        }
 
        platform_set_drvdata(pdev, drvdata);

drivers/regulator/fixed.c +0 -0

        return 0;
  }
 

include/linux/regulator/fixed.h +8 -0

index 91b4da3..b40e06b 100644
@@ -16,9 +16,26 @@
 
  struct regulator_init_data;
 
+/**
+ * struct fixed_voltage_config - fixed_voltage_config structure
+ * @supply_name:       Name of the regulator supply
+ * @microvolts:                Output voltage of regulator
+ * @use_gpio_control:  Use GPIO enable control
+ * @gpio:              GPIO to use for enable control
+ * @enable_high:       Polarity of enable GPIO
+ *                     1 = Active high, 0 = Active low

include/linux/regulator/fixed.h +2 -1

+       int gpio;
+       int enable_high;
        struct regulator_init_data *init_data;
  };
 
--

1.6.0.4

Signed-off-by Roger Quadros <[email protected]>

Reviewed-by Mark Brown <[email protected]>

Quoted 2 lines · Roger Quadros

Signed-off-by Roger Quadros <[email protected]>

Reviewed-by Mark Brown <[email protected]>

Err...  no, it isn't.  I reviewed a previous version of the patch but there were some issues that needed fixing.  I've not reviewed this version.

Quoted 7 lines

 static int fixed_voltage_is_enabled(struct regulator_dev *dev) {
-     return 1;
+     struct fixed_voltage_data *data = rdev_get_drvdata(dev); +
+     if (data->gpio)
+             return data->is_enabled;

Please use gpio_is_valid() as Phillip previously suggested.  For this particular use you could simplify the code marginally by just setting is_enabled at probe() time if there's no GPIO, not that it really matters.

Quoted 3 lines

+       if (data->gpio) {
+               gpio_set_value(data->gpio,
+                               data->enable_high ? 1 : 0);

_cansleep(), please, as previously requested.

Quoted 1 line

+             ret = gpio_get_value(config->gpio);

Can this fail?  It returns an int...

Quoted 6 lines

+             ret = gpio_direction_output(config->gpio, ret);
+             if (ret) {
+                     dev_err(&pdev->dev, "Could not configure "
+                             "enable GPIO %d direction: %d\n",
+                                             config->gpio, ret);
+                     goto err_gpio;

As previously requested please don't split for log messages text over lines unless you have to.  It makes it easier to find the message from a kernel log - grep for 'configure enable GPIO' won't match this source file the way you've written it above.

Quoted 3 lines

+             }
+     } else
+             dev_warn(&pdev->dev, "Not using GPIO 0 for enable control\n");

Probably better to soldier on and use the GPIO here but either way is fine; the warning is for the benefit of existing out of tree users to help them transition gracefully.

Quoted 6 lines · Mark Brown

Quoted 1 line · Roger Quadros

+             ret = gpio_get_value(config->gpio);

Can this fail?  It returns an int...

No, gpio_get_value(_cansleep) return either zero or non-zero. I think some I2C gpio chips just return zero when the read fails. Also that's the return value if a gpiochip doesn't define the .get method.

Only problem: drivers/gpio/gpiolib.c contains this piece of wisdom:

/* Drivers MUST set GPIO direction before making get/set calls.  In
 * some cases this is done in early boot, before IRQs are enabled. [...]
 */

Not having gpio_get_direction() in gpiolib is a limitation I think. Can we just do the following for now ? (i.e. disable regulator by default) Will there be any glitch? If yes, then how?

Quoted 3 lines · Roger Quadros

Not having gpio_get_direction() in gpiolib is a limitation I think. Can we just do the following for now ? (i.e. disable regulator by default) Will there be any glitch? If yes, then how?

If you're going to do this then like I say you should make the default state controllable by platform data.  If we unconditionally disable the regulator then if, for example, the regulator is controlling the display backlight and it's on when Linux get controls then the user will see the backlight blink off then on during startup.

Do we need to check whether CONFIG_GPIOLIB is available? or do we need to explicitly
select it in Kconfig?