| Message ID | 1319702185-16108-3-git-send-email-rnayak@ti.com |
|---|---|
| State | New |
| Headers | show |
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>
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
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?
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
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.
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
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.
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
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.
> >> +- 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
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, }, };
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