[PATCH] regulator: Add GPIO enable control to fixed voltage regulator driver

[PATCH] regulator: Add GPIO enable control to fixed voltage regulator driver

[Roger Quadros]

From: Roger Quadros <ext-roger.quadros@nokia.com>

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 <ext-roger.quadros@nokia.com>
---
 drivers/regulator/fixed.c       |   60 ++++++++++++++++++++++++++++++++++++++-
 include/linux/regulator/fixed.h |    7 ++++
 2 files changed, 66 insertions(+), 1 deletions(-)

diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index cdc674f..4ba6a50 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -20,20 +20,51 @@
 #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 use_gpio_control;
+	int gpio;
+	int enable_high;
+	int is_enabled;
 };
 
 static int fixed_voltage_is_enabled(struct regulator_dev *dev)
 {
-	return 1;
+	struct fixed_voltage_data *data = rdev_get_drvdata(dev);
+
+	if (data->use_gpio_control)
+		return data->is_enabled;
+	else
+		return 1;
 }
 
 static int fixed_voltage_enable(struct regulator_dev *dev)
 {
+	struct fixed_voltage_data *data = rdev_get_drvdata(dev);
+
+	if (data->use_gpio_control) {
+		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->use_gpio_control) {
+		gpio_set_value(data->gpio,
+				data->enable_high ? 0 : 1);
+		data->is_enabled = 0;
+	}
+
 	return 0;
 }
 
@@ -58,6 +89,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,
 };
@@ -85,6 +117,29 @@ static int regulator_fixed_voltage_probe(struct platform_device *pdev)
 	drvdata->desc.n_voltages = 1;
 
 	drvdata->microvolts = config->microvolts;
+	drvdata->use_gpio_control = config->use_gpio_control;
+	if (drvdata->use_gpio_control) {
+		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\n", config->gpio);
+			goto err_name;
+		} else {
+			ret = gpio_direction_output(config->gpio,
+				config->enable_high ? 0 : 1);
+			if (ret) {
+				dev_err(&pdev->dev, "Could not configure " \
+					"enable GPIO %d direction\n",
+					config->gpio);
+				gpio_free(config->gpio);
+				goto err_name;
+			}
+		}
+	}
 
 	drvdata->dev = regulator_register(&drvdata->desc, &pdev->dev,
 					  config->init_data, drvdata);
@@ -115,6 +170,9 @@ static int regulator_fixed_voltage_remove(struct platform_device *pdev)
 	kfree(drvdata->desc.name);
 	kfree(drvdata);
 
+	if (drvdata->_use_gpio_control)
+		gpio_free(drvdata->gpio);
+
 	return 0;
 }
 
diff --git a/include/linux/regulator/fixed.h b/include/linux/regulator/fixed.h
index 91b4da3..5b963cc 100644
--- a/include/linux/regulator/fixed.h
+++ b/include/linux/regulator/fixed.h
@@ -19,6 +19,13 @@ struct regulator_init_data;
 struct fixed_voltage_config {
 	const char *supply_name;
 	int microvolts;
+	int use_gpio_control;	/* Use GPIO enable control */
+	int gpio;		/* GPIO to use for enable control */
+
+	int enable_high;	/* Polarity of enable GPIO
+				 * 1 = Active High, 0 = Active Low
+				 */
+
 	struct regulator_init_data *init_data;
 };
 
-- 
1.6.0.4

Re: [PATCH] regulator: Add GPIO enable control to fixed voltage regulator driver

[Mark Brown]

On Fri, Jul 31, 2009 at 03:55:18PM +0300, Roger Quadros wrote:

Looks good, some relatively nitpicky issues:

> +	int use_gpio_control;

This isn't needed, just use an invalid GPIO value (zero or less).

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

Nicer to use the _cansleep() variants in case the GPIO is one on an
I2C/SPI device of some kind.  The regulator API doesn't mind if drivers
sleep so long as they don't do so excessively and it's not normally
sufficiently performance critical to make the inlining worth it.

> +		if (ret) {
> +			dev_err(&pdev->dev, "Could not obtain regulator " \
> +					"enable GPIO %d\n", config->gpio);

Please do something like:

	dev_err(&pdev->dev,
		"Could not obtain enable GPIO %d: %d\n",
		config->gpio, ret);

so that the error message is all in one in the source (so it's easier to
find when grepping the kernel log.  You also don't need the \.

> +			goto err_name;
> +		} else {

No need for the else clause; you've got the goto above.

> +			ret = gpio_direction_output(config->gpio,
> +				config->enable_high ? 0 : 1);
> +			if (ret) {
> +				dev_err(&pdev->dev, "Could not configure " \
> +					"enable GPIO %d direction\n",
> +					config->gpio);
> +				gpio_free(config->gpio);
> +				goto err_name;
> +			}

Same comment as above with regard to the error message.  It would be
nice to have the default state passed in as platform data if you can't
read it back to help avoid bouncing supplies at startup.  IIRC
gpio_get_value() will generally take a good stab at giving the current
state no matter if the GPIO is input our output but I'd need to check.

> +       int use_gpio_control;   /* Use GPIO enable control */
> +       int gpio;               /* GPIO to use for enable control */
> +
> +       int enable_high;        /* Polarity of enable GPIO
> +                                * 1 = Active High, 0 = Active Low

If you mark the comments with /** they'll get picked up by kerneldoc.

Re: [PATCH] regulator: Add GPIO enable control to fixed voltage regulator driver

[pHilipp Zabel]

On Fri, Jul 31, 2009 at 3:10 PM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Jul 31, 2009 at 03:55:18PM +0300, Roger Quadros wrote:
>
> Looks good, some relatively nitpicky issues:
>
>> +     int use_gpio_control;
>
> This isn't needed, just use an invalid GPIO value (zero or less).

Negative only, actually. Zero itself is a valid GPIO number (which is
a bit unfortunate because if you forget to initialize .gpio, it will
default to GPIO #0).

If you drop .use_gpio_control, use gpio_is_valid(data->gpio) for this check:

>> +       if (data->use_gpio_control) {
>> +               gpio_set_value(data->gpio,
>> +                               data->enable_high ? 1 : 0);
>
> Nicer to use the _cansleep() variants in case the GPIO is one on an
> I2C/SPI device of some kind.  The regulator API doesn't mind if drivers
> sleep so long as they don't do so excessively and it's not normally
> sufficiently performance critical to make the inlining worth it.
>
>> +             if (ret) {
>> +                     dev_err(&pdev->dev, "Could not obtain regulator " \
>> +                                     "enable GPIO %d\n", config->gpio);
>
> Please do something like:
>
>        dev_err(&pdev->dev,
>                "Could not obtain enable GPIO %d: %d\n",
>                config->gpio, ret);
>
> so that the error message is all in one in the source (so it's easier to
> find when grepping the kernel log.  You also don't need the \.
>
>> +                     goto err_name;
>> +             } else {
>
> No need for the else clause; you've got the goto above.
>
>> +                     ret = gpio_direction_output(config->gpio,
>> +                             config->enable_high ? 0 : 1);
>> +                     if (ret) {
>> +                             dev_err(&pdev->dev, "Could not configure " \
>> +                                     "enable GPIO %d direction\n",
>> +                                     config->gpio);
>> +                             gpio_free(config->gpio);
>> +                             goto err_name;
>> +                     }
>
> Same comment as above with regard to the error message.  It would be
> nice to have the default state passed in as platform data if you can't
> read it back to help avoid bouncing supplies at startup.  IIRC
> gpio_get_value() will generally take a good stab at giving the current
> state no matter if the GPIO is input our output but I'd need to check.

I think this is not clearly defined in the GPIO API document, so it
could be architecture dependent.

>> +       int use_gpio_control;   /* Use GPIO enable control */
>> +       int gpio;               /* GPIO to use for enable control */
>> +
>> +       int enable_high;        /* Polarity of enable GPIO
>> +                                * 1 = Active High, 0 = Active Low
>
> If you mark the comments with /** they'll get picked up by kerneldoc.

Maybe following the style in Documentation/kernel-doc-nano-HOWTO.txt
would be worthwhile, then.

regards
Philipp

Re: [PATCH] regulator: Add GPIO enable control to fixed voltage regulator driver

[Mark Brown]

On Fri, Jul 31, 2009 at 03:25:36PM +0200, pHilipp Zabel wrote:
> On Fri, Jul 31, 2009 at 3:10 PM, Mark
> Brown<broonie@opensource.wolfsonmicro.com> wrote:

> > This isn't needed, just use an invalid GPIO value (zero or less).

> Negative only, actually. Zero itself is a valid GPIO number (which is
> a bit unfortunate because if you forget to initialize .gpio, it will
> default to GPIO #0).

> If you drop .use_gpio_control, use gpio_is_valid(data->gpio) for this check:

Feh, better print a warning for GPIO 0 for at least a kernel release.
Fortunately we've got no mainline users of fixed voltage regulators.

> > Same comment as above with regard to the error message. ?It would be
> > nice to have the default state passed in as platform data if you can't
> > read it back to help avoid bouncing supplies at startup. ?IIRC
> > gpio_get_value() will generally take a good stab at giving the current
> > state no matter if the GPIO is input our output but I'd need to check.

> I think this is not clearly defined in the GPIO API document, so it
> could be architecture dependent.

It's not particularly; having checked gpiolib just passes this straight
through to the underlying driver.  Since they'd have to go out of their
way to do something unconstructive it should be safe to do the read and
we can worry about problem cases if they crop up.

> > If you mark the comments with /** they'll get picked up by kerneldoc.

> Maybe following the style in Documentation/kernel-doc-nano-HOWTO.txt
> would be worthwhile, then.

Indeed.

Re: [PATCH] regulator: Add GPIO enable control to fixed voltage regulator driver

[Roger Quadros]

On Fri, Jul 31, 2009 at 4:34 PM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Jul 31, 2009 at 03:25:36PM +0200, pHilipp Zabel wrote:
>> On Fri, Jul 31, 2009 at 3:10 PM, Mark
>> Brown<broonie@opensource.wolfsonmicro.com> wrote:
>
>> > This isn't needed, just use an invalid GPIO value (zero or less).
>
>> Negative only, actually. Zero itself is a valid GPIO number (which is
>> a bit unfortunate because if you forget to initialize .gpio, it will
>> default to GPIO #0).
>
>> If you drop .use_gpio_control, use gpio_is_valid(data->gpio) for this check:
>
> Feh, better print a warning for GPIO 0 for at least a kernel release.
> Fortunately we've got no mainline users of fixed voltage regulators.
>

I think most architectures start their GPIO numbering from 0.
By 'a release', do you mean next Kernel release will be indexing GPIOs from 1?

>> > Same comment as above with regard to the error message. ?It would be
>> > nice to have the default state passed in as platform data if you can't
>> > read it back to help avoid bouncing supplies at startup. ?IIRC
>> > gpio_get_value() will generally take a good stab at giving the current
>> > state no matter if the GPIO is input our output but I'd need to check.
>
>> I think this is not clearly defined in the GPIO API document, so it
>> could be architecture dependent.
>
> It's not particularly; having checked gpiolib just passes this straight
> through to the underlying driver.  Since they'd have to go out of their
> way to do something unconstructive it should be safe to do the read and
> we can worry about problem cases if they crop up.
>
>> > If you mark the comments with /** they'll get picked up by kerneldoc.
>
>> Maybe following the style in Documentation/kernel-doc-nano-HOWTO.txt
>> would be worthwhile, then.
>
> Indeed.
>

Taken other suggestions for v2. Thanks Mark and Philipp.

Re: [PATCH] regulator: Add GPIO enable control to fixed voltage regulator driver

[Mark Brown]

On Fri, Jul 31, 2009 at 04:43:40PM +0300, Roger Quadros wrote:
> On Fri, Jul 31, 2009 at 4:34 PM, Mark
> Brown<broonie@opensource.wolfsonmicro.com> wrote:

> > Feh, better print a warning for GPIO 0 for at least a kernel release.
> > Fortunately we've got no mainline users of fixed voltage regulators.

> I think most architectures start their GPIO numbering from 0.
> By 'a release', do you mean next Kernel release will be indexing GPIOs from 1?

No; I mean the driver should print a warning when using GPIO 0 as a way
of managing the transition.  Otherwise any existing users will find the
driver suddenly starts using a GPIO.

Re: [PATCH] regulator: Add GPIO enable control to fixed voltage regulator driver

[Roger Quadros]

On Fri, Jul 31, 2009 at 5:15 PM, Mark
Brown<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Jul 31, 2009 at 05:13:23PM +0300, Roger Quadros wrote:
>
>> OK I got what you meant now. If we use the use_gpio_enable flag then there would
>> not be any problem even if GPIO is set to 0 ignorantly. Because in
>> that case use_gpio_enable will
>> also be set to zero.
>> So, I feel we should keep use_gpio_enable as it is in the original patch.
>
> There's no in-tree users to care about and the use_gpio_enable thing is
> just ugly - it's not worth carrying it around for ever just for this.
>

OK. I will remove use_gpio_enable then.

Re: [PATCH] regulator: Add GPIO enable control to fixed voltage regulator driver

[Roger Quadros]

On Fri, Jul 31, 2009 at 5:22 PM, Roger Quadros<quadros.roger@gmail.com> wrote:
> On Fri, Jul 31, 2009 at 5:15 PM, Mark
> Brown<broonie@opensource.wolfsonmicro.com> wrote:
>> On Fri, Jul 31, 2009 at 05:13:23PM +0300, Roger Quadros wrote:
>>
>>> OK I got what you meant now. If we use the use_gpio_enable flag then there would
>>> not be any problem even if GPIO is set to 0 ignorantly. Because in
>>> that case use_gpio_enable will
>>> also be set to zero.
>>> So, I feel we should keep use_gpio_enable as it is in the original patch.
>>
>> There's no in-tree users to care about and the use_gpio_enable thing is
>> just ugly - it's not worth carrying it around for ever just for this.
>>
>
> OK. I will remove use_gpio_enable then.
>

Also, if regulator register fails then we should free the GPIO
before goto err_name