[v3,2/4] regulator: adapt fixed regulator driver to dt

Message ID 1319702185-16108-3-git-send-email-rnayak@ti.com
State New
Headers show

Commit Message

Rajendra Nayak Oct. 27, 2011, 7:56 a.m.
The fixed regulator driver uses of_get_fixed_voltage_config()
to extract fixed_voltage_config structure contents from device tree.

Also add documenation for additional bindings for fixed
regulators that can be passed through dt.

Signed-off-by: Rajendra Nayak <rnayak@ti.com>
---
 .../bindings/regulator/fixed-regulator.txt         |   25 +++++++++
 drivers/regulator/fixed.c                          |   57 ++++++++++++++++++++
 2 files changed, 82 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/fixed-regulator.txt

Comments

Mark Brown Oct. 27, 2011, 12:18 p.m. | #1
On Thu, Oct 27, 2011 at 01:26:23PM +0530, Rajendra Nayak wrote:
> The fixed regulator driver uses of_get_fixed_voltage_config()
> to extract fixed_voltage_config structure contents from device tree.
> 
> Also add documenation for additional bindings for fixed
> regulators that can be passed through dt.

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
Olof Johansson Nov. 4, 2011, 8:34 p.m. | #2
On Thu, Oct 27, 2011 at 01:26:23PM +0530, Rajendra Nayak wrote:
> The fixed regulator driver uses of_get_fixed_voltage_config()
> to extract fixed_voltage_config structure contents from device tree.
> 
> Also add documenation for additional bindings for fixed
> regulators that can be passed through dt.
> 
> Signed-off-by: Rajendra Nayak <rnayak@ti.com>

Shouldn't a fixed regulator just be a subset of a fixed one? If so, should the
binding be merged with that one?

> ---
>  .../bindings/regulator/fixed-regulator.txt         |   25 +++++++++
>  drivers/regulator/fixed.c                          |   57 ++++++++++++++++++++
>  2 files changed, 82 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/regulator/fixed-regulator.txt
> 
> diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
> new file mode 100644
> index 0000000..049df3d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
> @@ -0,0 +1,25 @@
> +Fixed Voltage regulators
> +
> +Required properties:
> +- compatible: Must be "regulator-fixed";
> +
> +Optional properties:
> +- regulator-fixed-supply: Name of the regulator supply
> +- regulator-fixed-microvolts: Output voltage of regulator

Other regulator binding usese uV, here it's microvolts. Pick one, microvolts
has the benefit of not needing caps. :)

> +- regulator-fixed-gpio: gpio to use for enable control
> +- regulator-fixed-startup-delay: startup time in microseconds

startup-delay-ms ?

> +- regulator-fixed-enable-high: Polarity of enable GPIO,
> +  1 = Active High, 0 = Active low

Some gpio specifiers allow you to specify active high or low flags, but either
way something like "enable-active-low" as a property (with active high as
default if property is missing) is a more devicetreey convention.

> +- regulator-fixed-enabled-at-boot: 1 = yes, 0 = no

Same here, you can drop the prefix. Also, the regular regulators use
"regulator-name" for the supply name, it would make sense to reuse the same
naming here, right?

> +
> +Example:
> +
> +	abc: fixedregulator@0 {
> +		compatible = "regulator-fixed";
> +		regulator-fixed-supply = "fixed-supply";
> +		regulator-fixed-microvolts = <1800000>;
> +		regulator-fixed-gpio = <43>;

This is not a valid gpio specifier.

> +		regulator-fixed-startup-delay = <70000>;
> +		regulator-fixed-enable-high;
> +		regulator-fixed-enabled-at-boot;
> +	};
> diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
> index 2fe9d99..9851b42 100644
> --- a/drivers/regulator/fixed.c
> +++ b/drivers/regulator/fixed.c
> @@ -26,6 +26,9 @@
>  #include <linux/gpio.h>
>  #include <linux/delay.h>
>  #include <linux/slab.h>
> +#include <linux/of.h>
> +#include <linux/regulator/of_regulator.h>
> +#include <linux/regulator/machine.h>
>  
>  struct fixed_voltage_data {
>  	struct regulator_desc desc;
> @@ -37,6 +40,46 @@ struct fixed_voltage_data {
>  	bool is_enabled;
>  };
>  
> +
> +/**
> + * of_get_fixed_voltage_config - extract fixed_voltage_config structure info
> + * @dev: device requesting for fixed_voltage_config
> + *
> + * Populates fixed_voltage_config structure by extracting data from device
> + * tree node, returns a pointer to the populated structure of NULL if memory
> + * alloc fails.
> + */
> +struct fixed_voltage_config *of_get_fixed_voltage_config(struct device *dev)
> +{
> +	struct fixed_voltage_config *config;
> +	struct device_node *np = dev->of_node;
> +	const __be32 *microvolts, *gpio, *delay;
> +
> +	config = devm_kzalloc(dev, sizeof(struct fixed_voltage_config), GFP_KERNEL);
> +	if (!config)
> +		return NULL;
> +
> +	config->supply_name = of_get_property(np, "regulator-fixed-supply", NULL);
> +	microvolts = of_get_property(np, "regulator-fixed-microvolts", NULL);
> +	if (microvolts)
> +		config->microvolts = be32_to_cpu(*microvolts);
> +	gpio = of_get_property(np, "regulator-fixed-gpio", NULL);
> +	if (gpio)
> +		config->gpio = be32_to_cpu(*gpio);

This needs to be fixed to parse a gpio properly instead.



-Olof
Mark Brown Nov. 4, 2011, 9:01 p.m. | #3
On Fri, Nov 04, 2011 at 01:34:22PM -0700, Olof Johansson wrote:

> Shouldn't a fixed regulator just be a subset of a fixed one? If so, should the
> binding be merged with that one?

No, the fixed voltage regultor is a superset of a general regulator - it
has additional information like the voltage it supplies and the optional
enable GPIO.

> > +- regulator-fixed-enabled-at-boot: 1 = yes, 0 = no

> Same here, you can drop the prefix. Also, the regular regulators use
> "regulator-name" for the supply name, it would make sense to reuse the same
> naming here, right?

I'm having a hard time associating your second comment with the property
being discussed - could you clarify please?
Olof Johansson Nov. 4, 2011, 9:18 p.m. | #4
On Fri, Nov 04, 2011 at 09:01:52PM +0000, Mark Brown wrote:
> On Fri, Nov 04, 2011 at 01:34:22PM -0700, Olof Johansson wrote:
> 
> > Shouldn't a fixed regulator just be a subset of a fixed one? If so, should the
> > binding be merged with that one?
> 
> No, the fixed voltage regultor is a superset of a general regulator - it
> has additional information like the voltage it supplies and the optional
> enable GPIO.

Still, seems like it could be merged into one regulator binding.

> > > +- regulator-fixed-enabled-at-boot: 1 = yes, 0 = no
> 
> > Same here, you can drop the prefix. Also, the regular regulators use
> > "regulator-name" for the supply name, it would make sense to reuse the same
> > naming here, right?
> 
> I'm having a hard time associating your second comment with the property
> being discussed - could you clarify please?

It was the first comment I wrote on the whole block and later interspersed
other comments above, so I don't blame you for not understanding. The
second comment was related to the "regulator-fixed-supply" property at
the top of the block.


-Olof
Mark Brown Nov. 4, 2011, 9:25 p.m. | #5
On Fri, Nov 04, 2011 at 02:18:24PM -0700, Olof Johansson wrote:
> On Fri, Nov 04, 2011 at 09:01:52PM +0000, Mark Brown wrote:

> > No, the fixed voltage regultor is a superset of a general regulator - it
> > has additional information like the voltage it supplies and the optional
> > enable GPIO.

> Still, seems like it could be merged into one regulator binding.

I don't see how you can usefully do that, the task of plumbing a
regulator into a board is largely orthogonal to the specific feature set
of a given regulator.  The specific bindings for a fixed voltage
regulator would be useful or unhelpful for most regultors controlled via
I2C.
Olof Johansson Nov. 4, 2011, 9:47 p.m. | #6
On Fri, Nov 4, 2011 at 2:25 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Nov 04, 2011 at 02:18:24PM -0700, Olof Johansson wrote:
>> On Fri, Nov 04, 2011 at 09:01:52PM +0000, Mark Brown wrote:
>
>> > No, the fixed voltage regultor is a superset of a general regulator - it
>> > has additional information like the voltage it supplies and the optional
>> > enable GPIO.
>
>> Still, seems like it could be merged into one regulator binding.
>
> I don't see how you can usefully do that, the task of plumbing a
> regulator into a board is largely orthogonal to the specific feature set
> of a given regulator.  The specific bindings for a fixed voltage
> regulator would be useful or unhelpful for most regultors controlled via
> I2C.

I meant more that the fixed regulators should reuse as much as
possible from the generic regulator bindings, instead of completely
forking them.

Then, depending on how they are controlled, there will be more
specific bindings. So the case of a gpio-controlled fixed regulator
would have a binding where the format of the properties to find the
gpio, etc, would be described. But things like voltage (without a
range, obviously) would be using the same bindings as the other
regulators.

Does that make more sense?


-Olof
Mark Brown Nov. 4, 2011, 9:57 p.m. | #7
On Fri, Nov 04, 2011 at 02:47:05PM -0700, Olof Johansson wrote:
> On Fri, Nov 4, 2011 at 2:25 PM, Mark Brown

> > I don't see how you can usefully do that, the task of plumbing a
> > regulator into a board is largely orthogonal to the specific feature set
> > of a given regulator.  The specific bindings for a fixed voltage
> > regulator would be useful or unhelpful for most regultors controlled via
> > I2C.

> I meant more that the fixed regulators should reuse as much as
> possible from the generic regulator bindings, instead of completely
> forking them.

That appears to be what's going on?  The fixed voltage regulator
includes by reference the core regulator binding, all of the properties
it defines with the possible exception of the supply name are not
covered in the core binding.

> Then, depending on how they are controlled, there will be more
> specific bindings. So the case of a gpio-controlled fixed regulator
> would have a binding where the format of the properties to find the
> gpio, etc, would be described. But things like voltage (without a
> range, obviously) would be using the same bindings as the other
> regulators.

The only overlap I'm seeing is the voltage?  

The intended semantic for the voltage is rather different.  The core
binding for the voltage specifies the range of voltages it is possible
to set a regulator to on a given board and is used to give permission to
the system to reconfigure the regulator.  The binding here tells the
system what voltage a fixed voltage regulator is running at.  We could
have the fixed voltage regulator read the same binding - though there's
some risk of mild confusion it shouldn't be too bad.
Olof Johansson Nov. 4, 2011, 10:09 p.m. | #8
On Fri, Nov 4, 2011 at 2:57 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, Nov 04, 2011 at 02:47:05PM -0700, Olof Johansson wrote:
>> On Fri, Nov 4, 2011 at 2:25 PM, Mark Brown
>
>> > I don't see how you can usefully do that, the task of plumbing a
>> > regulator into a board is largely orthogonal to the specific feature set
>> > of a given regulator.  The specific bindings for a fixed voltage
>> > regulator would be useful or unhelpful for most regultors controlled via
>> > I2C.
>
>> I meant more that the fixed regulators should reuse as much as
>> possible from the generic regulator bindings, instead of completely
>> forking them.
>
> That appears to be what's going on?  The fixed voltage regulator
> includes by reference the core regulator binding, all of the properties
> it defines with the possible exception of the supply name are not
> covered in the core binding.

The fixed-regulator binding makes no reference to the generic binding,
and the example includes no properties that are defined in that one.
So I definitely did not make that conclusion based on what I saw.

>> Then, depending on how they are controlled, there will be more
>> specific bindings. So the case of a gpio-controlled fixed regulator
>> would have a binding where the format of the properties to find the
>> gpio, etc, would be described. But things like voltage (without a
>> range, obviously) would be using the same bindings as the other
>> regulators.
>
> The only overlap I'm seeing is the voltage?
>
> The intended semantic for the voltage is rather different.  The core
> binding for the voltage specifies the range of voltages it is possible
> to set a regulator to on a given board and is used to give permission to
> the system to reconfigure the regulator.  The binding here tells the
> system what voltage a fixed voltage regulator is running at.  We could
> have the fixed voltage regulator read the same binding - though there's
> some risk of mild confusion it shouldn't be too bad.

Yeah, voltage is the obvious one where fixed would have a max and min
that is the same when you have a fixed regulator.

But even things like allowing (optional) attributes such as
startup-delay on non-fixed regulators could make sense. Keep in mind
that the device tree should focus on describing the hardware, not just
what the linux driver needs from it. So maybe instead of
startup-delay, specifying ramp-up speed instead of time needed until
power is good could be the way to go there.


-Olof
Mark Brown Nov. 4, 2011, 10:23 p.m. | #9
On Fri, Nov 04, 2011 at 03:09:32PM -0700, Olof Johansson wrote:

> But even things like allowing (optional) attributes such as
> startup-delay on non-fixed regulators could make sense. Keep in mind
> that the device tree should focus on describing the hardware, not just
> what the linux driver needs from it. So maybe instead of
> startup-delay, specifying ramp-up speed instead of time needed until
> power is good could be the way to go there.

This is in general something that a driver should know as a result of
knowing which regulator it's dealing with - in many cases these are
properties which can be varied at runtime on hardware which has register
control.  Replicating this into the device tree would make things more
error prone.
Rajendra Nayak Nov. 7, 2011, 6:27 a.m. | #10
>
>> +- regulator-fixed-gpio: gpio to use for enable control
>> +- regulator-fixed-startup-delay: startup time in microseconds
>
> startup-delay-ms ?

ok.

>
>> +- regulator-fixed-enable-high: Polarity of enable GPIO,
>> +  1 = Active High, 0 = Active low
>
> Some gpio specifiers allow you to specify active high or low flags, but either
> way something like "enable-active-low" as a property (with active high as
> default if property is missing) is a more devicetreey convention.
>
>> +- regulator-fixed-enabled-at-boot: 1 = yes, 0 = no
>
> Same here, you can drop the prefix. Also, the regular regulators use
> "regulator-name" for the supply name, it would make sense to reuse the same
> naming here, right?

yes, will do these changes.

>
>> +
>> +Example:
>> +
>> +	abc: fixedregulator@0 {
>> +		compatible = "regulator-fixed";
>> +		regulator-fixed-supply = "fixed-supply";
>> +		regulator-fixed-microvolts =<1800000>;
>> +		regulator-fixed-gpio =<43>;
>
> This is not a valid gpio specifier.

right. will fix.
>
>> +		regulator-fixed-startup-delay =<70000>;
>> +		regulator-fixed-enable-high;
>> +		regulator-fixed-enabled-at-boot;
>> +	};
>> diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
>> index 2fe9d99..9851b42 100644
>> --- a/drivers/regulator/fixed.c
>> +++ b/drivers/regulator/fixed.c
>> @@ -26,6 +26,9 @@
>>   #include<linux/gpio.h>
>>   #include<linux/delay.h>
>>   #include<linux/slab.h>
>> +#include<linux/of.h>
>> +#include<linux/regulator/of_regulator.h>
>> +#include<linux/regulator/machine.h>
>>
>>   struct fixed_voltage_data {
>>   	struct regulator_desc desc;
>> @@ -37,6 +40,46 @@ struct fixed_voltage_data {
>>   	bool is_enabled;
>>   };
>>
>> +
>> +/**
>> + * of_get_fixed_voltage_config - extract fixed_voltage_config structure info
>> + * @dev: device requesting for fixed_voltage_config
>> + *
>> + * Populates fixed_voltage_config structure by extracting data from device
>> + * tree node, returns a pointer to the populated structure of NULL if memory
>> + * alloc fails.
>> + */
>> +struct fixed_voltage_config *of_get_fixed_voltage_config(struct device *dev)
>> +{
>> +	struct fixed_voltage_config *config;
>> +	struct device_node *np = dev->of_node;
>> +	const __be32 *microvolts, *gpio, *delay;
>> +
>> +	config = devm_kzalloc(dev, sizeof(struct fixed_voltage_config), GFP_KERNEL);
>> +	if (!config)
>> +		return NULL;
>> +
>> +	config->supply_name = of_get_property(np, "regulator-fixed-supply", NULL);
>> +	microvolts = of_get_property(np, "regulator-fixed-microvolts", NULL);
>> +	if (microvolts)
>> +		config->microvolts = be32_to_cpu(*microvolts);
>> +	gpio = of_get_property(np, "regulator-fixed-gpio", NULL);
>> +	if (gpio)
>> +		config->gpio = be32_to_cpu(*gpio);
>
> This needs to be fixed to parse a gpio properly instead.

yes, will use of_get_gpio() here.

>
>
>
> -Olof

Patch hide | download patch | download mbox

diff --git a/Documentation/devicetree/bindings/regulator/fixed-regulator.txt b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
new file mode 100644
index 0000000..049df3d
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/fixed-regulator.txt
@@ -0,0 +1,25 @@ 
+Fixed Voltage regulators
+
+Required properties:
+- compatible: Must be "regulator-fixed";
+
+Optional properties:
+- regulator-fixed-supply: Name of the regulator supply
+- regulator-fixed-microvolts: Output voltage of regulator
+- regulator-fixed-gpio: gpio to use for enable control
+- regulator-fixed-startup-delay: startup time in microseconds
+- regulator-fixed-enable-high: Polarity of enable GPIO,
+  1 = Active High, 0 = Active low
+- regulator-fixed-enabled-at-boot: 1 = yes, 0 = no
+
+Example:
+
+	abc: fixedregulator@0 {
+		compatible = "regulator-fixed";
+		regulator-fixed-supply = "fixed-supply";
+		regulator-fixed-microvolts = <1800000>;
+		regulator-fixed-gpio = <43>;
+		regulator-fixed-startup-delay = <70000>;
+		regulator-fixed-enable-high;
+		regulator-fixed-enabled-at-boot;
+	};
diff --git a/drivers/regulator/fixed.c b/drivers/regulator/fixed.c
index 2fe9d99..9851b42 100644
--- a/drivers/regulator/fixed.c
+++ b/drivers/regulator/fixed.c
@@ -26,6 +26,9 @@ 
 #include <linux/gpio.h>
 #include <linux/delay.h>
 #include <linux/slab.h>
+#include <linux/of.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/regulator/machine.h>
 
 struct fixed_voltage_data {
 	struct regulator_desc desc;
@@ -37,6 +40,46 @@  struct fixed_voltage_data {
 	bool is_enabled;
 };
 
+
+/**
+ * of_get_fixed_voltage_config - extract fixed_voltage_config structure info
+ * @dev: device requesting for fixed_voltage_config
+ *
+ * Populates fixed_voltage_config structure by extracting data from device
+ * tree node, returns a pointer to the populated structure of NULL if memory
+ * alloc fails.
+ */
+struct fixed_voltage_config *of_get_fixed_voltage_config(struct device *dev)
+{
+	struct fixed_voltage_config *config;
+	struct device_node *np = dev->of_node;
+	const __be32 *microvolts, *gpio, *delay;
+
+	config = devm_kzalloc(dev, sizeof(struct fixed_voltage_config), GFP_KERNEL);
+	if (!config)
+		return NULL;
+
+	config->supply_name = of_get_property(np, "regulator-fixed-supply", NULL);
+	microvolts = of_get_property(np, "regulator-fixed-microvolts", NULL);
+	if (microvolts)
+		config->microvolts = be32_to_cpu(*microvolts);
+	gpio = of_get_property(np, "regulator-fixed-gpio", NULL);
+	if (gpio)
+		config->gpio = be32_to_cpu(*gpio);
+	delay = of_get_property(np, "regulator-fixed-startup-delay", NULL);
+	if (delay)
+		config->startup_delay = be32_to_cpu(*delay);
+
+	if (of_find_property(np, "regulator-fixed-enable-high", NULL))
+		config->enable_high = true;
+	if (of_find_property(np, "regulator-fixed-enabled-at-boot", NULL))
+		config->enabled_at_boot = true;
+
+	config->init_data = of_get_regulator_init_data(dev);
+
+	return config;
+}
+
 static int fixed_voltage_is_enabled(struct regulator_dev *dev)
 {
 	struct fixed_voltage_data *data = rdev_get_drvdata(dev);
@@ -108,6 +151,9 @@  static int __devinit reg_fixed_voltage_probe(struct platform_device *pdev)
 	struct fixed_voltage_data *drvdata;
 	int ret;
 
+	if (pdev->dev.of_node)
+		config = of_get_fixed_voltage_config(&pdev->dev);
+
 	drvdata = kzalloc(sizeof(struct fixed_voltage_data), GFP_KERNEL);
 	if (drvdata == NULL) {
 		dev_err(&pdev->dev, "Failed to allocate device data\n");
@@ -216,12 +262,23 @@  static int __devexit reg_fixed_voltage_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#if defined(CONFIG_OF)
+static const struct of_device_id fixed_of_match[] __devinitconst = {
+	{ .compatible = "regulator-fixed", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, fixed_of_match);
+#else
+#define fixed_of_match NULL
+#endif
+
 static struct platform_driver regulator_fixed_voltage_driver = {
 	.probe		= reg_fixed_voltage_probe,
 	.remove		= __devexit_p(reg_fixed_voltage_remove),
 	.driver		= {
 		.name		= "reg-fixed-voltage",
 		.owner		= THIS_MODULE,
+		.of_match_table = fixed_of_match,
 	},
 };