regulator: Add GPIO enable control to fixed voltage regulator driver

PATCH Superseded Roger Quadros 3 participants 8 messages Jul 31, 2009 2 messages not in the archive Voltage and current regulator framework

lore

Share & tools

lore

Roger Quadros

author

2009-07-31 12:55 UTC #0 lore raw

PATCH 1/1 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]>

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

drivers/regulator/fixed.c +59 -1

index cdc674f..4ba6a50 100644
@@ -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;
 }

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

index 91b4da3..5b963cc 100644
@@ -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;
 };

Signature · 1 line

1.6.0.4

Mark Brown

2009-07-31 13:10 UTC #1 lore raw

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

Looks good, some relatively nitpicky issues:

drivers/regulator/fixed.c line 29

Quoted 1 line

drivers/regulator/fixed.c +1 -0
+	int use_gpio_control;

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

drivers/regulator/fixed.c line 51

Quoted 3 lines

drivers/regulator/fixed.c +3 -0

+       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.

drivers/regulator/fixed.c static int regulator_fixed_voltage_probe(struct platform_device *pdev) · line 129

Quoted 3 lines

drivers/regulator/fixed.c +3 -0

+		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 \.

drivers/regulator/fixed.c static int regulator_fixed_voltage_probe(struct platform_device *pdev) · line 131

Quoted 2 lines

drivers/regulator/fixed.c +2 -0
+			goto err_name;
+		} else {

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

drivers/regulator/fixed.c static int regulator_fixed_voltage_probe(struct platform_device *pdev) · line 140

Quoted 9 lines

drivers/regulator/fixed.c +9 -0

+			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.

include/linux/regulator/fixed.h struct regulator_init_data; · line 26

Quoted 5 lines

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

+       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.

pHilipp Zabel

2009-07-31 13:25 UTC #2 lore raw

Quoted 7 lines · Mark Brown

On Fri, Jul 31, 2009 at 03:55:18PM +0300, Roger Quadros wrote:
Looks good, some relatively nitpicky issues:
Quoted 1 line
+ 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:

Quoted 37 lines

Quoted 3 lines
+ 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.
Quoted 3 lines
+ 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 \.
Quoted 2 lines
+ goto err_name;
+ } else {
No need for the else clause; you've got the goto above.
Quoted 9 lines
+ 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;
+ }

Quoted 5 lines

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.

Quoted 7 lines

Quoted 5 lines
+ 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

Mark Brown

2009-07-31 13:34 UTC #3 lore raw

Quoted 10 lines · pHilipp Zabel

Quoted 1 line · Mark Brown
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.

Quoted 8 lines

Quoted 5 lines
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.

Quoted 4 lines

Quoted 1 line
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.

Roger Quadros

author

2009-07-31 13:43 UTC #4 lore raw

Quoted 6 lines · Mark Brown

Quoted 4 lines · pHilipp Zabel
Quoted 1 line · Mark Brown
This isn't needed, just use an invalid GPIO value (zero or less).

Quoted 9 lines

Quoted 5 lines
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?

Quoted 14 lines

Quoted 8 lines
Quoted 5 lines
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.

Quoted 7 lines

Quoted 4 lines
Quoted 1 line
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.

Mark Brown

2009-07-31 13:50 UTC #5 lore raw

Quoted 8 lines · Roger Quadros

Quoted 2 lines · Mark Brown
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.

A message that is not in the archive (sent off-list or not yet received): [email protected]

Roger Quadros

author

2009-07-31 14:22 UTC #8 lore raw

Quoted 11 lines · Mark Brown

Quoted 5 lines · Roger Quadros
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.

Roger Quadros

author

2009-07-31 14:34 UTC #9 lore raw

Quoted 10 lines · Roger Quadros

Quoted 8 lines · Mark Brown
Quoted 5 lines · Roger Quadros
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.

Quoted 6 lines

Quoted 3 lines
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