| Message ID | 1318263578-7407-4-git-send-email-rnayak@ti.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote: > +- regulator-change-voltage: boolean, Output voltage can be changed by software > +- regulator-change-current: boolean, Output current can be chnaged by software > +- regulator-change-mode: boolean, Operating mode can be changed by software > +- regulator-change-status: boolean, Enable/Disable control for regulator exists > +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled > +- regulator-mode-fast: boolean, allow/set fast mode for the regulator > +- regulator-mode-normal: boolean, allow/set normal mode for the regulator > +- regulator-mode-idle: boolean, allow/set idle mode for the regulator > +- regulator-mode-standby: boolean, allow/set standby mode for the regulator > +- regulator-input-uV: Input voltage for regulator when supplied by another regulator > +- regulator-always-on: boolean, regulator should never be disabled Thinking about this I'm not sure that these should go in the device tree, they're as much talking about what Linux is able to cope with as they are talking about what the hardware is able to do. Sometimes they'll be fixed by the design but they are sometimes also restricted by what the software is currently capable of. DRMS is a prime example here, it depends on how good we are at telling the API about how much current we're using.
On Monday 10 October 2011 10:52 PM, Mark Brown wrote: > On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote: > >> +- regulator-change-voltage: boolean, Output voltage can be changed by software >> +- regulator-change-current: boolean, Output current can be chnaged by software >> +- regulator-change-mode: boolean, Operating mode can be changed by software >> +- regulator-change-status: boolean, Enable/Disable control for regulator exists >> +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled >> +- regulator-mode-fast: boolean, allow/set fast mode for the regulator >> +- regulator-mode-normal: boolean, allow/set normal mode for the regulator >> +- regulator-mode-idle: boolean, allow/set idle mode for the regulator >> +- regulator-mode-standby: boolean, allow/set standby mode for the regulator >> +- regulator-input-uV: Input voltage for regulator when supplied by another regulator >> +- regulator-always-on: boolean, regulator should never be disabled > > Thinking about this I'm not sure that these should go in the device > tree, they're as much talking about what Linux is able to cope with as > they are talking about what the hardware is able to do. Sometimes > they'll be fixed by the design but they are sometimes also restricted by > what the software is currently capable of. DRMS is a prime example > here, it depends on how good we are at telling the API about how much > current we're using. So is there another way of passing these, if not through device tree? There are other linux specific things that need to be passed to the framework as well, like the state of the regulator in the various linux specific suspend states, like standby/mem/disk. So if these can't be passed through device tree, should they still be passed in some way through platform_data? Or is it best to identify the bindings as being linux specific and not generic by appending a "linux," to the bindings. Grant, need some help and advice.
On Tue, Oct 11, 2011 at 11:29:29AM +0530, Rajendra Nayak wrote: > On Monday 10 October 2011 10:52 PM, Mark Brown wrote: > >On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote: > > > >>+- regulator-change-voltage: boolean, Output voltage can be changed by software > >>+- regulator-change-current: boolean, Output current can be chnaged by software > >>+- regulator-change-mode: boolean, Operating mode can be changed by software > >>+- regulator-change-status: boolean, Enable/Disable control for regulator exists > >>+- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled > >>+- regulator-mode-fast: boolean, allow/set fast mode for the regulator > >>+- regulator-mode-normal: boolean, allow/set normal mode for the regulator > >>+- regulator-mode-idle: boolean, allow/set idle mode for the regulator > >>+- regulator-mode-standby: boolean, allow/set standby mode for the regulator > >>+- regulator-input-uV: Input voltage for regulator when supplied by another regulator > >>+- regulator-always-on: boolean, regulator should never be disabled > > > >Thinking about this I'm not sure that these should go in the device > >tree, they're as much talking about what Linux is able to cope with as > >they are talking about what the hardware is able to do. Sometimes > >they'll be fixed by the design but they are sometimes also restricted by > >what the software is currently capable of. DRMS is a prime example > >here, it depends on how good we are at telling the API about how much > >current we're using. > > So is there another way of passing these, if not through device tree? > There are other linux specific things that need to be passed to the > framework as well, like the state of the regulator in the various > linux specific suspend states, like standby/mem/disk. > So if these can't be passed through device tree, should they still > be passed in some way through platform_data? Or is it best to identify > the bindings as being linux specific and not generic by appending a > "linux," to the bindings. > > Grant, need some help and advice. > I can't really help much here. If it is a property of the hardware, then it absolutely needs to be in the device tree. If it is a limitation of Linux, or the Linux driver, then I would say that it should be encoded in the driver. I don't understand enough of the problem space of regulators to give a good answer about what you should do. These *sound* like flags that the driver would set based on its own capabilities; in which case it is something that would be encoded directly into the driver. g.
On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote: > The helper routine is meant to be used by regulator drivers > to extract the regulator_init_data structure from the data > that is passed from device tree. > 'consumer_supplies' which is part of regulator_init_data is not extracted > as the regulator consumer mappings are passed through DT differently, > implemented in subsequent patches. > Similarly the regulator<-->parent/supply mapping is handled in > subsequent patches. > > Also add documentation for regulator bindings to be used to pass > regulator_init_data struct information from device tree. > > Signed-off-by: Rajendra Nayak <rnayak@ti.com> > --- > .../devicetree/bindings/regulator/regulator.txt | 44 +++++++++ > drivers/regulator/Kconfig | 8 ++ > drivers/regulator/Makefile | 1 + > drivers/regulator/of_regulator.c | 93 ++++++++++++++++++++ > include/linux/regulator/of_regulator.h | 21 +++++ > 5 files changed, 167 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt > create mode 100644 drivers/regulator/of_regulator.c > create mode 100644 include/linux/regulator/of_regulator.h > > diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt > new file mode 100644 > index 0000000..a623fdd > --- /dev/null > +++ b/Documentation/devicetree/bindings/regulator/regulator.txt > @@ -0,0 +1,44 @@ > +Voltage/Current Regulators > + > +Optional properties: > +- regulator-min-uV: smallest voltage consumers may set > +- regulator-max-uV: largest voltage consumers may set > +- regulator-uV-offset: Offset applied to voltages to compensate for voltage drops > +- regulator-min-uA: smallest current consumers may set > +- regulator-max-uA: largest current consumers may set > +- regulator-change-voltage: boolean, Output voltage can be changed by software > +- regulator-change-current: boolean, Output current can be chnaged by software > +- regulator-change-mode: boolean, Operating mode can be changed by software > +- regulator-change-status: boolean, Enable/Disable control for regulator exists > +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled > +- regulator-mode-fast: boolean, allow/set fast mode for the regulator > +- regulator-mode-normal: boolean, allow/set normal mode for the regulator > +- regulator-mode-idle: boolean, allow/set idle mode for the regulator > +- regulator-mode-standby: boolean, allow/set standby mode for the regulator > +- regulator-input-uV: Input voltage for regulator when supplied by another regulator > +- regulator-always-on: boolean, regulator should never be disabled > +- regulator-boot-on: bootloader/firmware enabled regulator > +- <name>-supply: phandle to the parent supply/regulator node > + > +Example: > + > + xyzreg: regulator@0 { > + regulator-min-uV = <1000000>; > + regulator-max-uV = <2500000>; > + regulator-mode-fast; > + regulator-change-voltage; > + regulator-always-on; > + vin-supply = <&vin>; > + }; > + > +The same binding used by a regulator to reference its > +supply can be used by any consumer to reference its > +regulator/supply > + > +Example of a device node referencing a regulator node, > + > + devicenode: node@0x0 { > + ... > + ... > + <name>-supply = <&xyzreg>; > + }; > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > index c7fd2c0..981c92e 100644 > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -64,6 +64,14 @@ config REGULATOR_USERSPACE_CONSUMER > > If unsure, say no. > > +config OF_REGULATOR > + tristate "OF regulator helpers" > + depends on OF > + default y if OF drop the default y line. This probably shouldn't even be a user-visible option. Make drivers select OF_REGULATOR if it needs it. > + help > + OpenFirmware regulator framework helper routines that can > + used by regulator drivers to extract data from device tree. > + > config REGULATOR_BQ24022 > tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC" > help > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > index 040d5aa..e6bc009 100644 > --- a/drivers/regulator/Makefile > +++ b/drivers/regulator/Makefile > @@ -7,6 +7,7 @@ obj-$(CONFIG_REGULATOR) += core.o dummy.o > obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o > obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o > obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o > +obj-$(CONFIG_OF_REGULATOR) += of_regulator.o > > obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o > obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o > diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c > new file mode 100644 > index 0000000..9262d06 > --- /dev/null > +++ b/drivers/regulator/of_regulator.c > @@ -0,0 +1,93 @@ > +/* > + * OF helpers for regulator framework > + * > + * Copyright (C) 2011 Texas Instruments, Inc. > + * Rajendra Nayak <rnayak@ti.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include <linux/slab.h> > +#include <linux/of.h> > +#include <linux/regulator/machine.h> > + > +static void of_get_regulation_constraints(struct device_node *np, > + struct regulator_init_data **init_data) > +{ > + const __be32 *min_uV, *max_uV, *uV_offset; > + const __be32 *min_uA, *max_uA, *input_uV; > + unsigned int valid_modes_mask = 0, valid_ops_mask = 0; > + > + min_uV = of_get_property(np, "regulator-min-uV", NULL); > + if (min_uV) > + (*init_data)->constraints.min_uV = be32_to_cpu(*min_uV); > + max_uV = of_get_property(np, "regulator-max-uV", NULL); > + if (max_uV) > + (*init_data)->constraints.max_uV = be32_to_cpu(*max_uV); > + uV_offset = of_get_property(np, "regulator-uV-offset", NULL); > + if (uV_offset) > + (*init_data)->constraints.uV_offset = be32_to_cpu(*uV_offset); > + min_uA = of_get_property(np, "regulator-min-uA", NULL); > + if (min_uA) > + (*init_data)->constraints.min_uA = be32_to_cpu(*min_uA); > + max_uA = of_get_property(np, "regulator-max-uA", NULL); > + if (max_uA) > + (*init_data)->constraints.max_uA = be32_to_cpu(*max_uA); > + input_uV = of_get_property(np, "regulator-input-uV", NULL); > + if (input_uV) > + (*init_data)->constraints.input_uV = be32_to_cpu(*input_uV); > + > + /* valid modes mask */ > + if (of_find_property(np, "regulator-mode-fast", NULL)) > + valid_modes_mask |= REGULATOR_MODE_FAST; > + if (of_find_property(np, "regulator-mode-normal", NULL)) > + valid_modes_mask |= REGULATOR_MODE_NORMAL; > + if (of_find_property(np, "regulator-mode-idle", NULL)) > + valid_modes_mask |= REGULATOR_MODE_IDLE; > + if (of_find_property(np, "regulator-mode-standby", NULL)) > + valid_modes_mask |= REGULATOR_MODE_STANDBY; > + > + /* valid ops mask */ > + if (of_find_property(np, "regulator-change-voltage", NULL)) > + valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE; > + if (of_find_property(np, "regulator-change-current", NULL)) > + valid_ops_mask |= REGULATOR_CHANGE_CURRENT; > + if (of_find_property(np, "regulator-change-mode", NULL)) > + valid_ops_mask |= REGULATOR_CHANGE_MODE; > + if (of_find_property(np, "regulator-change-status", NULL)) > + valid_ops_mask |= REGULATOR_CHANGE_STATUS; > + > + (*init_data)->constraints.valid_modes_mask = valid_modes_mask; > + (*init_data)->constraints.valid_ops_mask = valid_ops_mask; > + > + if (of_find_property(np, "regulator-always-on", NULL)) > + (*init_data)->constraints.always_on = true; > + if (of_find_property(np, "regulator-boot-on", NULL)) > + (*init_data)->constraints.boot_on = true; > +} > + > +/** > + * of_get_regulator_init_data - extract regulator_init_data structure info > + * @dev: device requesting for regulator_init_data > + * > + * Populates regulator_init_data structure by extracting data from device > + * tree node, returns a pointer to the populated struture or NULL if memory > + * alloc fails. > + */ > +struct regulator_init_data *of_get_regulator_init_data(struct device *dev) > +{ > + struct regulator_init_data *init_data; > + > + if (!dev->of_node) > + return NULL; > + > + init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL); > + if (!init_data) > + return NULL; /* Out of memory? */ > + > + of_get_regulation_constraints(dev->of_node, &init_data); > + return init_data; > +} > diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h > new file mode 100644 > index 0000000..c5a1ad6 > --- /dev/null > +++ b/include/linux/regulator/of_regulator.h > @@ -0,0 +1,21 @@ > +/* > + * OpenFirmware regulator support routines > + * > + */ > + > +#ifndef __LINUX_OF_REG_H > +#define __LINUX_OF_REG_H > + > +#if defined(CONFIG_OF_REGULATOR) > +extern struct regulator_init_data > + *of_get_regulator_init_data(struct device *dev); > +#else > +static inline struct regulator_init_data > + *of_get_regulator_init_data(struct device *dev) > +{ > + return NULL; > +} > +#endif /* CONFIG_OF_REGULATOR */ > + > +#endif /* __LINUX_OF_REG_H */ > + > -- > 1.7.1 >
On Thu, Oct 13, 2011 at 12:38:21PM -0600, Grant Likely wrote: > These *sound* like flags that the driver would set based on its own > capabilities; in which case it is something that would be encoded > directly into the driver. They're almost entirely a combination of system specifics and software capabilities - it's not even reliably one or the other. For example the ability to switch regulator modes is going to depend on a combination of the modes a specific regulator offers, the set of things connected to the regulator in the current system (not all of which will be software visible), how good all the connected consumer drivers are at providing accurate information about what they're going to need from the system in terms of quality of regulation and current draw and how good the board design is in making these things reality. For robustness (for some of these things if you get it wrong we're talking catastrophic hardware failure) the Linux regulator API is extremely conservative and won't touch the hardware unless explicitly told that a specific thing is OK on a given board.
On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote: > The helper routine is meant to be used by regulator drivers > to extract the regulator_init_data structure from the data > that is passed from device tree. > 'consumer_supplies' which is part of regulator_init_data is not extracted > as the regulator consumer mappings are passed through DT differently, > implemented in subsequent patches. > Similarly the regulator<-->parent/supply mapping is handled in > subsequent patches. > > Also add documentation for regulator bindings to be used to pass > regulator_init_data struct information from device tree. > > Signed-off-by: Rajendra Nayak <rnayak@ti.com> > --- > .../devicetree/bindings/regulator/regulator.txt | 44 +++++++++ > drivers/regulator/Kconfig | 8 ++ > drivers/regulator/Makefile | 1 + > drivers/regulator/of_regulator.c | 93 ++++++++++++++++++++ > include/linux/regulator/of_regulator.h | 21 +++++ > 5 files changed, 167 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt > create mode 100644 drivers/regulator/of_regulator.c > create mode 100644 include/linux/regulator/of_regulator.h > > diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt > new file mode 100644 > index 0000000..a623fdd > --- /dev/null > +++ b/Documentation/devicetree/bindings/regulator/regulator.txt > @@ -0,0 +1,44 @@ > +Voltage/Current Regulators > + > +Optional properties: > +- regulator-min-uV: smallest voltage consumers may set > +- regulator-max-uV: largest voltage consumers may set > +- regulator-uV-offset: Offset applied to voltages to compensate for voltage drops > +- regulator-min-uA: smallest current consumers may set > +- regulator-max-uA: largest current consumers may set > +- regulator-change-voltage: boolean, Output voltage can be changed by software > +- regulator-change-current: boolean, Output current can be chnaged by software > +- regulator-change-mode: boolean, Operating mode can be changed by software > +- regulator-change-status: boolean, Enable/Disable control for regulator exists > +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled > +- regulator-mode-fast: boolean, allow/set fast mode for the regulator > +- regulator-mode-normal: boolean, allow/set normal mode for the regulator > +- regulator-mode-idle: boolean, allow/set idle mode for the regulator > +- regulator-mode-standby: boolean, allow/set standby mode for the regulator > +- regulator-input-uV: Input voltage for regulator when supplied by another regulator > +- regulator-always-on: boolean, regulator should never be disabled > +- regulator-boot-on: bootloader/firmware enabled regulator > +- <name>-supply: phandle to the parent supply/regulator node > + > +Example: > + > + xyzreg: regulator@0 { > + regulator-min-uV = <1000000>; > + regulator-max-uV = <2500000>; > + regulator-mode-fast; > + regulator-change-voltage; > + regulator-always-on; > + vin-supply = <&vin>; > + }; > + > +The same binding used by a regulator to reference its > +supply can be used by any consumer to reference its > +regulator/supply > + > +Example of a device node referencing a regulator node, > + > + devicenode: node@0x0 { > + ... > + ... > + <name>-supply = <&xyzreg>; > + }; > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > index c7fd2c0..981c92e 100644 > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -64,6 +64,14 @@ config REGULATOR_USERSPACE_CONSUMER > > If unsure, say no. > > +config OF_REGULATOR > + tristate "OF regulator helpers" > + depends on OF > + default y if OF > + help > + OpenFirmware regulator framework helper routines that can > + used by regulator drivers to extract data from device tree. > + > config REGULATOR_BQ24022 > tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC" > help > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > index 040d5aa..e6bc009 100644 > --- a/drivers/regulator/Makefile > +++ b/drivers/regulator/Makefile > @@ -7,6 +7,7 @@ obj-$(CONFIG_REGULATOR) += core.o dummy.o > obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o > obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o > obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o > +obj-$(CONFIG_OF_REGULATOR) += of_regulator.o > > obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o > obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o > diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c > new file mode 100644 > index 0000000..9262d06 > --- /dev/null > +++ b/drivers/regulator/of_regulator.c > @@ -0,0 +1,93 @@ > +/* > + * OF helpers for regulator framework > + * > + * Copyright (C) 2011 Texas Instruments, Inc. > + * Rajendra Nayak <rnayak@ti.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include <linux/slab.h> > +#include <linux/of.h> > +#include <linux/regulator/machine.h> > + > +static void of_get_regulation_constraints(struct device_node *np, > + struct regulator_init_data **init_data) > +{ > + const __be32 *min_uV, *max_uV, *uV_offset; > + const __be32 *min_uA, *max_uA, *input_uV; > + unsigned int valid_modes_mask = 0, valid_ops_mask = 0; > + > + min_uV = of_get_property(np, "regulator-min-uV", NULL); > + if (min_uV) > + (*init_data)->constraints.min_uV = be32_to_cpu(*min_uV); > + max_uV = of_get_property(np, "regulator-max-uV", NULL); > + if (max_uV) > + (*init_data)->constraints.max_uV = be32_to_cpu(*max_uV); > + uV_offset = of_get_property(np, "regulator-uV-offset", NULL); > + if (uV_offset) > + (*init_data)->constraints.uV_offset = be32_to_cpu(*uV_offset); > + min_uA = of_get_property(np, "regulator-min-uA", NULL); > + if (min_uA) > + (*init_data)->constraints.min_uA = be32_to_cpu(*min_uA); > + max_uA = of_get_property(np, "regulator-max-uA", NULL); > + if (max_uA) > + (*init_data)->constraints.max_uA = be32_to_cpu(*max_uA); > + input_uV = of_get_property(np, "regulator-input-uV", NULL); > + if (input_uV) > + (*init_data)->constraints.input_uV = be32_to_cpu(*input_uV); > + > + /* valid modes mask */ > + if (of_find_property(np, "regulator-mode-fast", NULL)) > + valid_modes_mask |= REGULATOR_MODE_FAST; > + if (of_find_property(np, "regulator-mode-normal", NULL)) > + valid_modes_mask |= REGULATOR_MODE_NORMAL; > + if (of_find_property(np, "regulator-mode-idle", NULL)) > + valid_modes_mask |= REGULATOR_MODE_IDLE; > + if (of_find_property(np, "regulator-mode-standby", NULL)) > + valid_modes_mask |= REGULATOR_MODE_STANDBY; > + > + /* valid ops mask */ > + if (of_find_property(np, "regulator-change-voltage", NULL)) > + valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE; > + if (of_find_property(np, "regulator-change-current", NULL)) > + valid_ops_mask |= REGULATOR_CHANGE_CURRENT; > + if (of_find_property(np, "regulator-change-mode", NULL)) > + valid_ops_mask |= REGULATOR_CHANGE_MODE; > + if (of_find_property(np, "regulator-change-status", NULL)) > + valid_ops_mask |= REGULATOR_CHANGE_STATUS; > + > + (*init_data)->constraints.valid_modes_mask = valid_modes_mask; > + (*init_data)->constraints.valid_ops_mask = valid_ops_mask; > + > + if (of_find_property(np, "regulator-always-on", NULL)) > + (*init_data)->constraints.always_on = true; > + if (of_find_property(np, "regulator-boot-on", NULL)) > + (*init_data)->constraints.boot_on = true; > +} > + I do not see that of_get_regulation_constraints() covers the following fields. We do not support them for DT probe? struct regulation_constraints { char *name; ... /* regulator suspend states for global PMIC STANDBY/HIBERNATE */ struct regulator_state state_disk; struct regulator_state state_mem; struct regulator_state state_standby; suspend_state_t initial_state; /* suspend state to set at init */ /* mode to set on startup */ unsigned int initial_mode; /* constraint flags */ ... unsigned apply_uV:1; /* apply uV constraint if min == max */ };
> I do not see that of_get_regulation_constraints() covers the following > fields. We do not support them for DT probe? I left these out as I wasn't sure how such (if at all) Linux specific params should be passed through dt, and I am still not quite sure :( > > struct regulation_constraints { > > char *name; > > ... > > /* regulator suspend states for global PMIC STANDBY/HIBERNATE */ > struct regulator_state state_disk; > struct regulator_state state_mem; > struct regulator_state state_standby; > suspend_state_t initial_state; /* suspend state to set at init */ > > /* mode to set on startup */ > unsigned int initial_mode; > > /* constraint flags */ > ... > unsigned apply_uV:1; /* apply uV constraint if min == max */ > }; >
On Mon, Oct 17, 2011 at 09:47:17AM +0530, Rajendra Nayak wrote: > > >I do not see that of_get_regulation_constraints() covers the following > >fields. We do not support them for DT probe? > > I left these out as I wasn't sure how such (if at all) Linux > specific params should be passed through dt, and I am still not > quite sure :( > I'm seeing some linux specific parameters encoded in the device tree below. Documentation/devicetree/bindings/gpio/gpio_keys.txt Documentation/devicetree/bindings/gpio/led.txt Mark, I understand that ideally device tree is supposed to describe pure hardware configurations. But practically, when migrating a driver to device tree probe, we are trying to move the configurations described by platform_data into device tree to save the use of platform_data for device tree probe. Then some of the configuration may not be so purely hardware related. But I do not see this is a critical problem. What do you think? Is it what you want that we classify the configurations clearly and put only strict hardware configurations into device tree and keep others still in platform_data?
On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote: > The helper routine is meant to be used by regulator drivers > to extract the regulator_init_data structure from the data > that is passed from device tree. > 'consumer_supplies' which is part of regulator_init_data is not extracted > as the regulator consumer mappings are passed through DT differently, > implemented in subsequent patches. > Similarly the regulator<-->parent/supply mapping is handled in > subsequent patches. > > Also add documentation for regulator bindings to be used to pass > regulator_init_data struct information from device tree. > > Signed-off-by: Rajendra Nayak <rnayak@ti.com> > --- > .../devicetree/bindings/regulator/regulator.txt | 44 +++++++++ > drivers/regulator/Kconfig | 8 ++ > drivers/regulator/Makefile | 1 + > drivers/regulator/of_regulator.c | 93 ++++++++++++++++++++ > include/linux/regulator/of_regulator.h | 21 +++++ > 5 files changed, 167 insertions(+), 0 deletions(-) > create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt > create mode 100644 drivers/regulator/of_regulator.c > create mode 100644 include/linux/regulator/of_regulator.h > > diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt > new file mode 100644 > index 0000000..a623fdd > --- /dev/null > +++ b/Documentation/devicetree/bindings/regulator/regulator.txt > @@ -0,0 +1,44 @@ > +Voltage/Current Regulators > + > +Optional properties: > +- regulator-min-uV: smallest voltage consumers may set > +- regulator-max-uV: largest voltage consumers may set > +- regulator-uV-offset: Offset applied to voltages to compensate for voltage drops > +- regulator-min-uA: smallest current consumers may set > +- regulator-max-uA: largest current consumers may set > +- regulator-change-voltage: boolean, Output voltage can be changed by software > +- regulator-change-current: boolean, Output current can be chnaged by software > +- regulator-change-mode: boolean, Operating mode can be changed by software > +- regulator-change-status: boolean, Enable/Disable control for regulator exists > +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled > +- regulator-mode-fast: boolean, allow/set fast mode for the regulator > +- regulator-mode-normal: boolean, allow/set normal mode for the regulator > +- regulator-mode-idle: boolean, allow/set idle mode for the regulator > +- regulator-mode-standby: boolean, allow/set standby mode for the regulator > +- regulator-input-uV: Input voltage for regulator when supplied by another regulator > +- regulator-always-on: boolean, regulator should never be disabled > +- regulator-boot-on: bootloader/firmware enabled regulator > +- <name>-supply: phandle to the parent supply/regulator node > + > +Example: > + > + xyzreg: regulator@0 { Does this node have a compatible string? With looking at the code, I guess it has a compatible string in your dts file. > + regulator-min-uV = <1000000>; > + regulator-max-uV = <2500000>; > + regulator-mode-fast; > + regulator-change-voltage; > + regulator-always-on; > + vin-supply = <&vin>; > + }; > + > +The same binding used by a regulator to reference its > +supply can be used by any consumer to reference its > +regulator/supply > + > +Example of a device node referencing a regulator node, > + > + devicenode: node@0x0 { > + ... > + ... > + <name>-supply = <&xyzreg>; > + }; > diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > index c7fd2c0..981c92e 100644 > --- a/drivers/regulator/Kconfig > +++ b/drivers/regulator/Kconfig > @@ -64,6 +64,14 @@ config REGULATOR_USERSPACE_CONSUMER > > If unsure, say no. > > +config OF_REGULATOR > + tristate "OF regulator helpers" > + depends on OF > + default y if OF > + help > + OpenFirmware regulator framework helper routines that can > + used by regulator drivers to extract data from device tree. > + > config REGULATOR_BQ24022 > tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC" > help > diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > index 040d5aa..e6bc009 100644 > --- a/drivers/regulator/Makefile > +++ b/drivers/regulator/Makefile > @@ -7,6 +7,7 @@ obj-$(CONFIG_REGULATOR) += core.o dummy.o > obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o > obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o > obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o > +obj-$(CONFIG_OF_REGULATOR) += of_regulator.o > > obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o > obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o > diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c > new file mode 100644 > index 0000000..9262d06 > --- /dev/null > +++ b/drivers/regulator/of_regulator.c > @@ -0,0 +1,93 @@ > +/* > + * OF helpers for regulator framework > + * > + * Copyright (C) 2011 Texas Instruments, Inc. > + * Rajendra Nayak <rnayak@ti.com> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + */ > + > +#include <linux/slab.h> > +#include <linux/of.h> > +#include <linux/regulator/machine.h> > + > +static void of_get_regulation_constraints(struct device_node *np, > + struct regulator_init_data **init_data) > +{ > + const __be32 *min_uV, *max_uV, *uV_offset; > + const __be32 *min_uA, *max_uA, *input_uV; > + unsigned int valid_modes_mask = 0, valid_ops_mask = 0; > + > + min_uV = of_get_property(np, "regulator-min-uV", NULL); > + if (min_uV) > + (*init_data)->constraints.min_uV = be32_to_cpu(*min_uV); > + max_uV = of_get_property(np, "regulator-max-uV", NULL); > + if (max_uV) > + (*init_data)->constraints.max_uV = be32_to_cpu(*max_uV); > + uV_offset = of_get_property(np, "regulator-uV-offset", NULL); > + if (uV_offset) > + (*init_data)->constraints.uV_offset = be32_to_cpu(*uV_offset); > + min_uA = of_get_property(np, "regulator-min-uA", NULL); > + if (min_uA) > + (*init_data)->constraints.min_uA = be32_to_cpu(*min_uA); > + max_uA = of_get_property(np, "regulator-max-uA", NULL); > + if (max_uA) > + (*init_data)->constraints.max_uA = be32_to_cpu(*max_uA); > + input_uV = of_get_property(np, "regulator-input-uV", NULL); > + if (input_uV) > + (*init_data)->constraints.input_uV = be32_to_cpu(*input_uV); > + > + /* valid modes mask */ > + if (of_find_property(np, "regulator-mode-fast", NULL)) > + valid_modes_mask |= REGULATOR_MODE_FAST; > + if (of_find_property(np, "regulator-mode-normal", NULL)) > + valid_modes_mask |= REGULATOR_MODE_NORMAL; > + if (of_find_property(np, "regulator-mode-idle", NULL)) > + valid_modes_mask |= REGULATOR_MODE_IDLE; > + if (of_find_property(np, "regulator-mode-standby", NULL)) > + valid_modes_mask |= REGULATOR_MODE_STANDBY; > + > + /* valid ops mask */ > + if (of_find_property(np, "regulator-change-voltage", NULL)) > + valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE; > + if (of_find_property(np, "regulator-change-current", NULL)) > + valid_ops_mask |= REGULATOR_CHANGE_CURRENT; > + if (of_find_property(np, "regulator-change-mode", NULL)) > + valid_ops_mask |= REGULATOR_CHANGE_MODE; > + if (of_find_property(np, "regulator-change-status", NULL)) > + valid_ops_mask |= REGULATOR_CHANGE_STATUS; > + > + (*init_data)->constraints.valid_modes_mask = valid_modes_mask; > + (*init_data)->constraints.valid_ops_mask = valid_ops_mask; > + > + if (of_find_property(np, "regulator-always-on", NULL)) > + (*init_data)->constraints.always_on = true; > + if (of_find_property(np, "regulator-boot-on", NULL)) > + (*init_data)->constraints.boot_on = true; > +} > + > +/** > + * of_get_regulator_init_data - extract regulator_init_data structure info > + * @dev: device requesting for regulator_init_data > + * > + * Populates regulator_init_data structure by extracting data from device > + * tree node, returns a pointer to the populated struture or NULL if memory > + * alloc fails. > + */ > +struct regulator_init_data *of_get_regulator_init_data(struct device *dev) I remember that Mark ever had a comment on a previous regulator DT series from Haojian Zhuang, saying this DT parsing procedure can probably just be private to regulator core and invisible to regulator drivers. That said whenever regulator_register() gets called with parameter init_data as NULL, it should try to retrieve regulator_init_data from device tree. It will ease regulator drivers a little. They will only need to call regulator_register() with NULL init_data for DT case. More importantly, doing so will help solve the dual 'dev' problem I see below. > +{ > + struct regulator_init_data *init_data; > + > + if (!dev->of_node) > + return NULL; > + > + init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL); > + if (!init_data) > + return NULL; /* Out of memory? */ > + > + of_get_regulation_constraints(dev->of_node, &init_data); Beside the 'dev' here with of_node attached, there will be another 'dev' created by regulator core function regulator_register(), which is wrapped by 'regulator_dev'. So we have two 'dev'. One is created by DT core with of_node attached, and used to retrieve regulator_init_data from device tree. Another one is created in regulator_register() and used by regulator core. IMO, this is not what we want. Instead of having two 'dev' for given regulator, we should use the same 'dev' created in regulator_register() to retrieve regulator_init_data from device tree, so that we do not need the 'dev' created by DT core. That said, with DT parsing procedure moved into regulator core, we can get of_node for given regulator and attach it to the 'dev' created by regulator core. Will elaborate it a little more in patch #5. Regards, Shawn > + return init_data; > +} > diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h > new file mode 100644 > index 0000000..c5a1ad6 > --- /dev/null > +++ b/include/linux/regulator/of_regulator.h > @@ -0,0 +1,21 @@ > +/* > + * OpenFirmware regulator support routines > + * > + */ > + > +#ifndef __LINUX_OF_REG_H > +#define __LINUX_OF_REG_H > + > +#if defined(CONFIG_OF_REGULATOR) > +extern struct regulator_init_data > + *of_get_regulator_init_data(struct device *dev); > +#else > +static inline struct regulator_init_data > + *of_get_regulator_init_data(struct device *dev) > +{ > + return NULL; > +} > +#endif /* CONFIG_OF_REGULATOR */ > + > +#endif /* __LINUX_OF_REG_H */ > + > -- > 1.7.1 > > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss >
On Tue, Oct 18, 2011 at 07:58:37PM +0800, Shawn Guo wrote: > I understand that ideally device tree is supposed to describe pure > hardware configurations. But practically, when migrating a driver > to device tree probe, we are trying to move the configurations > described by platform_data into device tree to save the use of > platform_data for device tree probe. Then some of the configuration > may not be so purely hardware related. But I do not see this is a > critical problem. It's not just Linux-specific stuff, some of this is even specific to what current Linux drivers can do - updating the kernel could mean a different set of constraints.
On Tue, Oct 18, 2011 at 05:00:46PM +0100, Mark Brown wrote: > On Tue, Oct 18, 2011 at 07:58:37PM +0800, Shawn Guo wrote: > > > I understand that ideally device tree is supposed to describe pure > > hardware configurations. But practically, when migrating a driver > > to device tree probe, we are trying to move the configurations > > described by platform_data into device tree to save the use of > > platform_data for device tree probe. Then some of the configuration > > may not be so purely hardware related. But I do not see this is a > > critical problem. > > It's not just Linux-specific stuff, some of this is even specific to > what current Linux drivers can do - updating the kernel could mean a > different set of constraints. > Well, from what I see, the 'struct regulation_constraints' is defined in machine.h and meant to be the regulator machine/board interface. With the example I'm looking at, mc13892, the regulation_constraints configuration is fully passed from machine/board file. If there is something specific to what drivers can do, it probably should be encoded in regulator driver rather than staying in regulation_constraints. So how do you think we should proceed here?
On Tuesday 18 October 2011 06:50 PM, Shawn Guo wrote: > On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote: >> The helper routine is meant to be used by regulator drivers >> to extract the regulator_init_data structure from the data >> that is passed from device tree. >> 'consumer_supplies' which is part of regulator_init_data is not extracted >> as the regulator consumer mappings are passed through DT differently, >> implemented in subsequent patches. >> Similarly the regulator<-->parent/supply mapping is handled in >> subsequent patches. >> >> Also add documentation for regulator bindings to be used to pass >> regulator_init_data struct information from device tree. >> >> Signed-off-by: Rajendra Nayak<rnayak@ti.com> >> --- >> .../devicetree/bindings/regulator/regulator.txt | 44 +++++++++ >> drivers/regulator/Kconfig | 8 ++ >> drivers/regulator/Makefile | 1 + >> drivers/regulator/of_regulator.c | 93 ++++++++++++++++++++ >> include/linux/regulator/of_regulator.h | 21 +++++ >> 5 files changed, 167 insertions(+), 0 deletions(-) >> create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt >> create mode 100644 drivers/regulator/of_regulator.c >> create mode 100644 include/linux/regulator/of_regulator.h >> >> diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt >> new file mode 100644 >> index 0000000..a623fdd >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/regulator/regulator.txt >> @@ -0,0 +1,44 @@ >> +Voltage/Current Regulators >> + >> +Optional properties: >> +- regulator-min-uV: smallest voltage consumers may set >> +- regulator-max-uV: largest voltage consumers may set >> +- regulator-uV-offset: Offset applied to voltages to compensate for voltage drops >> +- regulator-min-uA: smallest current consumers may set >> +- regulator-max-uA: largest current consumers may set >> +- regulator-change-voltage: boolean, Output voltage can be changed by software >> +- regulator-change-current: boolean, Output current can be chnaged by software >> +- regulator-change-mode: boolean, Operating mode can be changed by software >> +- regulator-change-status: boolean, Enable/Disable control for regulator exists >> +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled >> +- regulator-mode-fast: boolean, allow/set fast mode for the regulator >> +- regulator-mode-normal: boolean, allow/set normal mode for the regulator >> +- regulator-mode-idle: boolean, allow/set idle mode for the regulator >> +- regulator-mode-standby: boolean, allow/set standby mode for the regulator >> +- regulator-input-uV: Input voltage for regulator when supplied by another regulator >> +- regulator-always-on: boolean, regulator should never be disabled >> +- regulator-boot-on: bootloader/firmware enabled regulator >> +-<name>-supply: phandle to the parent supply/regulator node >> + >> +Example: >> + >> + xyzreg: regulator@0 { > > Does this node have a compatible string? With looking at the code, > I guess it has a compatible string in your dts file. The compatible string depends on the regulator driver used. I added one for the twl regulators when I added support for them. > >> + regulator-min-uV =<1000000>; >> + regulator-max-uV =<2500000>; >> + regulator-mode-fast; >> + regulator-change-voltage; >> + regulator-always-on; >> + vin-supply =<&vin>; >> + }; >> + >> +The same binding used by a regulator to reference its >> +supply can be used by any consumer to reference its >> +regulator/supply >> + >> +Example of a device node referencing a regulator node, >> + >> + devicenode: node@0x0 { >> + ... >> + ... >> + <name>-supply =<&xyzreg>; >> + }; >> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig >> index c7fd2c0..981c92e 100644 >> --- a/drivers/regulator/Kconfig >> +++ b/drivers/regulator/Kconfig >> @@ -64,6 +64,14 @@ config REGULATOR_USERSPACE_CONSUMER >> >> If unsure, say no. >> >> +config OF_REGULATOR >> + tristate "OF regulator helpers" >> + depends on OF >> + default y if OF >> + help >> + OpenFirmware regulator framework helper routines that can >> + used by regulator drivers to extract data from device tree. >> + >> config REGULATOR_BQ24022 >> tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC" >> help >> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile >> index 040d5aa..e6bc009 100644 >> --- a/drivers/regulator/Makefile >> +++ b/drivers/regulator/Makefile >> @@ -7,6 +7,7 @@ obj-$(CONFIG_REGULATOR) += core.o dummy.o >> obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o >> obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o >> obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o >> +obj-$(CONFIG_OF_REGULATOR) += of_regulator.o >> >> obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o >> obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o >> diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c >> new file mode 100644 >> index 0000000..9262d06 >> --- /dev/null >> +++ b/drivers/regulator/of_regulator.c >> @@ -0,0 +1,93 @@ >> +/* >> + * OF helpers for regulator framework >> + * >> + * Copyright (C) 2011 Texas Instruments, Inc. >> + * Rajendra Nayak<rnayak@ti.com> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License as published by >> + * the Free Software Foundation; either version 2 of the License, or >> + * (at your option) any later version. >> + */ >> + >> +#include<linux/slab.h> >> +#include<linux/of.h> >> +#include<linux/regulator/machine.h> >> + >> +static void of_get_regulation_constraints(struct device_node *np, >> + struct regulator_init_data **init_data) >> +{ >> + const __be32 *min_uV, *max_uV, *uV_offset; >> + const __be32 *min_uA, *max_uA, *input_uV; >> + unsigned int valid_modes_mask = 0, valid_ops_mask = 0; >> + >> + min_uV = of_get_property(np, "regulator-min-uV", NULL); >> + if (min_uV) >> + (*init_data)->constraints.min_uV = be32_to_cpu(*min_uV); >> + max_uV = of_get_property(np, "regulator-max-uV", NULL); >> + if (max_uV) >> + (*init_data)->constraints.max_uV = be32_to_cpu(*max_uV); >> + uV_offset = of_get_property(np, "regulator-uV-offset", NULL); >> + if (uV_offset) >> + (*init_data)->constraints.uV_offset = be32_to_cpu(*uV_offset); >> + min_uA = of_get_property(np, "regulator-min-uA", NULL); >> + if (min_uA) >> + (*init_data)->constraints.min_uA = be32_to_cpu(*min_uA); >> + max_uA = of_get_property(np, "regulator-max-uA", NULL); >> + if (max_uA) >> + (*init_data)->constraints.max_uA = be32_to_cpu(*max_uA); >> + input_uV = of_get_property(np, "regulator-input-uV", NULL); >> + if (input_uV) >> + (*init_data)->constraints.input_uV = be32_to_cpu(*input_uV); >> + >> + /* valid modes mask */ >> + if (of_find_property(np, "regulator-mode-fast", NULL)) >> + valid_modes_mask |= REGULATOR_MODE_FAST; >> + if (of_find_property(np, "regulator-mode-normal", NULL)) >> + valid_modes_mask |= REGULATOR_MODE_NORMAL; >> + if (of_find_property(np, "regulator-mode-idle", NULL)) >> + valid_modes_mask |= REGULATOR_MODE_IDLE; >> + if (of_find_property(np, "regulator-mode-standby", NULL)) >> + valid_modes_mask |= REGULATOR_MODE_STANDBY; >> + >> + /* valid ops mask */ >> + if (of_find_property(np, "regulator-change-voltage", NULL)) >> + valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE; >> + if (of_find_property(np, "regulator-change-current", NULL)) >> + valid_ops_mask |= REGULATOR_CHANGE_CURRENT; >> + if (of_find_property(np, "regulator-change-mode", NULL)) >> + valid_ops_mask |= REGULATOR_CHANGE_MODE; >> + if (of_find_property(np, "regulator-change-status", NULL)) >> + valid_ops_mask |= REGULATOR_CHANGE_STATUS; >> + >> + (*init_data)->constraints.valid_modes_mask = valid_modes_mask; >> + (*init_data)->constraints.valid_ops_mask = valid_ops_mask; >> + >> + if (of_find_property(np, "regulator-always-on", NULL)) >> + (*init_data)->constraints.always_on = true; >> + if (of_find_property(np, "regulator-boot-on", NULL)) >> + (*init_data)->constraints.boot_on = true; >> +} >> + >> +/** >> + * of_get_regulator_init_data - extract regulator_init_data structure info >> + * @dev: device requesting for regulator_init_data >> + * >> + * Populates regulator_init_data structure by extracting data from device >> + * tree node, returns a pointer to the populated struture or NULL if memory >> + * alloc fails. >> + */ >> +struct regulator_init_data *of_get_regulator_init_data(struct device *dev) > > I remember that Mark ever had a comment on a previous regulator DT > series from Haojian Zhuang, saying this DT parsing procedure can > probably just be private to regulator core and invisible to regulator > drivers. That said whenever regulator_register() gets called with > parameter init_data as NULL, it should try to retrieve > regulator_init_data from device tree. It will ease regulator drivers > a little. They will only need to call regulator_register() with NULL > init_data for DT case. More importantly, doing so will help solve the > dual 'dev' problem I see below. Yes, it seems like a good idea given that most drivers seem to blindly pass the regulator_init_data onto regulator_register, however there are cases like the twl regulator driver which seems to peek into the constraints passed from the board to make sure it drops anything that the hardware does not support, or cases like the db8500 where regulator_init_data for all regulators are bundled together and the driver extracts and registers them as separate regulators. Exporting an api instead to extract regulator_init_data to the driver might help in those cases. > >> +{ >> + struct regulator_init_data *init_data; >> + >> + if (!dev->of_node) >> + return NULL; >> + >> + init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL); >> + if (!init_data) >> + return NULL; /* Out of memory? */ >> + >> + of_get_regulation_constraints(dev->of_node,&init_data); > > Beside the 'dev' here with of_node attached, there will be another > 'dev' created by regulator core function regulator_register(), which > is wrapped by 'regulator_dev'. > > So we have two 'dev'. One is created by DT core with of_node attached, > and used to retrieve regulator_init_data from device tree. Another one > is created in regulator_register() and used by regulator core. But thats not something newly done now with DT. Thats how it was even in the non-DT world. There were always two devices with the regulator_dev device as the child. > > IMO, this is not what we want. Instead of having two 'dev' for given > regulator, we should use the same 'dev' created in regulator_register() > to retrieve regulator_init_data from device tree, so that we do not need > the 'dev' created by DT core. That said, with DT parsing procedure > moved into regulator core, we can get of_node for given regulator and > attach it to the 'dev' created by regulator core. > > Will elaborate it a little more in patch #5. > > Regards, > Shawn > >> + return init_data; >> +} >> diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h >> new file mode 100644 >> index 0000000..c5a1ad6 >> --- /dev/null >> +++ b/include/linux/regulator/of_regulator.h >> @@ -0,0 +1,21 @@ >> +/* >> + * OpenFirmware regulator support routines >> + * >> + */ >> + >> +#ifndef __LINUX_OF_REG_H >> +#define __LINUX_OF_REG_H >> + >> +#if defined(CONFIG_OF_REGULATOR) >> +extern struct regulator_init_data >> + *of_get_regulator_init_data(struct device *dev); >> +#else >> +static inline struct regulator_init_data >> + *of_get_regulator_init_data(struct device *dev) >> +{ >> + return NULL; >> +} >> +#endif /* CONFIG_OF_REGULATOR */ >> + >> +#endif /* __LINUX_OF_REG_H */ >> + >> -- >> 1.7.1 >> >> _______________________________________________ >> devicetree-discuss mailing list >> devicetree-discuss@lists.ozlabs.org >> https://lists.ozlabs.org/listinfo/devicetree-discuss >> >
On Wed, Oct 19, 2011 at 05:05:56PM +0530, Rajendra Nayak wrote: > On Tuesday 18 October 2011 06:50 PM, Shawn Guo wrote: > >On Mon, Oct 10, 2011 at 09:49:36PM +0530, Rajendra Nayak wrote: > >>The helper routine is meant to be used by regulator drivers > >>to extract the regulator_init_data structure from the data > >>that is passed from device tree. > >>'consumer_supplies' which is part of regulator_init_data is not extracted > >>as the regulator consumer mappings are passed through DT differently, > >>implemented in subsequent patches. > >>Similarly the regulator<-->parent/supply mapping is handled in > >>subsequent patches. > >> > >>Also add documentation for regulator bindings to be used to pass > >>regulator_init_data struct information from device tree. > >> > >>Signed-off-by: Rajendra Nayak<rnayak@ti.com> > >>--- > >> .../devicetree/bindings/regulator/regulator.txt | 44 +++++++++ > >> drivers/regulator/Kconfig | 8 ++ > >> drivers/regulator/Makefile | 1 + > >> drivers/regulator/of_regulator.c | 93 ++++++++++++++++++++ > >> include/linux/regulator/of_regulator.h | 21 +++++ > >> 5 files changed, 167 insertions(+), 0 deletions(-) > >> create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt > >> create mode 100644 drivers/regulator/of_regulator.c > >> create mode 100644 include/linux/regulator/of_regulator.h > >> > >>diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt > >>new file mode 100644 > >>index 0000000..a623fdd > >>--- /dev/null > >>+++ b/Documentation/devicetree/bindings/regulator/regulator.txt > >>@@ -0,0 +1,44 @@ > >>+Voltage/Current Regulators > >>+ > >>+Optional properties: > >>+- regulator-min-uV: smallest voltage consumers may set > >>+- regulator-max-uV: largest voltage consumers may set > >>+- regulator-uV-offset: Offset applied to voltages to compensate for voltage drops > >>+- regulator-min-uA: smallest current consumers may set > >>+- regulator-max-uA: largest current consumers may set > >>+- regulator-change-voltage: boolean, Output voltage can be changed by software > >>+- regulator-change-current: boolean, Output current can be chnaged by software > >>+- regulator-change-mode: boolean, Operating mode can be changed by software > >>+- regulator-change-status: boolean, Enable/Disable control for regulator exists > >>+- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled > >>+- regulator-mode-fast: boolean, allow/set fast mode for the regulator > >>+- regulator-mode-normal: boolean, allow/set normal mode for the regulator > >>+- regulator-mode-idle: boolean, allow/set idle mode for the regulator > >>+- regulator-mode-standby: boolean, allow/set standby mode for the regulator > >>+- regulator-input-uV: Input voltage for regulator when supplied by another regulator > >>+- regulator-always-on: boolean, regulator should never be disabled > >>+- regulator-boot-on: bootloader/firmware enabled regulator > >>+-<name>-supply: phandle to the parent supply/regulator node > >>+ > >>+Example: > >>+ > >>+ xyzreg: regulator@0 { > > > >Does this node have a compatible string? With looking at the code, > >I guess it has a compatible string in your dts file. > > The compatible string depends on the regulator driver used. > I added one for the twl regulators when I added support for > them. > > > > >>+ regulator-min-uV =<1000000>; > >>+ regulator-max-uV =<2500000>; > >>+ regulator-mode-fast; > >>+ regulator-change-voltage; > >>+ regulator-always-on; > >>+ vin-supply =<&vin>; > >>+ }; > >>+ > >>+The same binding used by a regulator to reference its > >>+supply can be used by any consumer to reference its > >>+regulator/supply > >>+ > >>+Example of a device node referencing a regulator node, > >>+ > >>+ devicenode: node@0x0 { > >>+ ... > >>+ ... > >>+ <name>-supply =<&xyzreg>; > >>+ }; > >>diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig > >>index c7fd2c0..981c92e 100644 > >>--- a/drivers/regulator/Kconfig > >>+++ b/drivers/regulator/Kconfig > >>@@ -64,6 +64,14 @@ config REGULATOR_USERSPACE_CONSUMER > >> > >> If unsure, say no. > >> > >>+config OF_REGULATOR > >>+ tristate "OF regulator helpers" > >>+ depends on OF > >>+ default y if OF > >>+ help > >>+ OpenFirmware regulator framework helper routines that can > >>+ used by regulator drivers to extract data from device tree. > >>+ > >> config REGULATOR_BQ24022 > >> tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC" > >> help > >>diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile > >>index 040d5aa..e6bc009 100644 > >>--- a/drivers/regulator/Makefile > >>+++ b/drivers/regulator/Makefile > >>@@ -7,6 +7,7 @@ obj-$(CONFIG_REGULATOR) += core.o dummy.o > >> obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o > >> obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o > >> obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o > >>+obj-$(CONFIG_OF_REGULATOR) += of_regulator.o > >> > >> obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o > >> obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o > >>diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c > >>new file mode 100644 > >>index 0000000..9262d06 > >>--- /dev/null > >>+++ b/drivers/regulator/of_regulator.c > >>@@ -0,0 +1,93 @@ > >>+/* > >>+ * OF helpers for regulator framework > >>+ * > >>+ * Copyright (C) 2011 Texas Instruments, Inc. > >>+ * Rajendra Nayak<rnayak@ti.com> > >>+ * > >>+ * This program is free software; you can redistribute it and/or modify > >>+ * it under the terms of the GNU General Public License as published by > >>+ * the Free Software Foundation; either version 2 of the License, or > >>+ * (at your option) any later version. > >>+ */ > >>+ > >>+#include<linux/slab.h> > >>+#include<linux/of.h> > >>+#include<linux/regulator/machine.h> > >>+ > >>+static void of_get_regulation_constraints(struct device_node *np, > >>+ struct regulator_init_data **init_data) > >>+{ > >>+ const __be32 *min_uV, *max_uV, *uV_offset; > >>+ const __be32 *min_uA, *max_uA, *input_uV; > >>+ unsigned int valid_modes_mask = 0, valid_ops_mask = 0; > >>+ > >>+ min_uV = of_get_property(np, "regulator-min-uV", NULL); > >>+ if (min_uV) > >>+ (*init_data)->constraints.min_uV = be32_to_cpu(*min_uV); > >>+ max_uV = of_get_property(np, "regulator-max-uV", NULL); > >>+ if (max_uV) > >>+ (*init_data)->constraints.max_uV = be32_to_cpu(*max_uV); > >>+ uV_offset = of_get_property(np, "regulator-uV-offset", NULL); > >>+ if (uV_offset) > >>+ (*init_data)->constraints.uV_offset = be32_to_cpu(*uV_offset); > >>+ min_uA = of_get_property(np, "regulator-min-uA", NULL); > >>+ if (min_uA) > >>+ (*init_data)->constraints.min_uA = be32_to_cpu(*min_uA); > >>+ max_uA = of_get_property(np, "regulator-max-uA", NULL); > >>+ if (max_uA) > >>+ (*init_data)->constraints.max_uA = be32_to_cpu(*max_uA); > >>+ input_uV = of_get_property(np, "regulator-input-uV", NULL); > >>+ if (input_uV) > >>+ (*init_data)->constraints.input_uV = be32_to_cpu(*input_uV); > >>+ > >>+ /* valid modes mask */ > >>+ if (of_find_property(np, "regulator-mode-fast", NULL)) > >>+ valid_modes_mask |= REGULATOR_MODE_FAST; > >>+ if (of_find_property(np, "regulator-mode-normal", NULL)) > >>+ valid_modes_mask |= REGULATOR_MODE_NORMAL; > >>+ if (of_find_property(np, "regulator-mode-idle", NULL)) > >>+ valid_modes_mask |= REGULATOR_MODE_IDLE; > >>+ if (of_find_property(np, "regulator-mode-standby", NULL)) > >>+ valid_modes_mask |= REGULATOR_MODE_STANDBY; > >>+ > >>+ /* valid ops mask */ > >>+ if (of_find_property(np, "regulator-change-voltage", NULL)) > >>+ valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE; > >>+ if (of_find_property(np, "regulator-change-current", NULL)) > >>+ valid_ops_mask |= REGULATOR_CHANGE_CURRENT; > >>+ if (of_find_property(np, "regulator-change-mode", NULL)) > >>+ valid_ops_mask |= REGULATOR_CHANGE_MODE; > >>+ if (of_find_property(np, "regulator-change-status", NULL)) > >>+ valid_ops_mask |= REGULATOR_CHANGE_STATUS; > >>+ > >>+ (*init_data)->constraints.valid_modes_mask = valid_modes_mask; > >>+ (*init_data)->constraints.valid_ops_mask = valid_ops_mask; > >>+ > >>+ if (of_find_property(np, "regulator-always-on", NULL)) > >>+ (*init_data)->constraints.always_on = true; > >>+ if (of_find_property(np, "regulator-boot-on", NULL)) > >>+ (*init_data)->constraints.boot_on = true; > >>+} > >>+ > >>+/** > >>+ * of_get_regulator_init_data - extract regulator_init_data structure info > >>+ * @dev: device requesting for regulator_init_data > >>+ * > >>+ * Populates regulator_init_data structure by extracting data from device > >>+ * tree node, returns a pointer to the populated struture or NULL if memory > >>+ * alloc fails. > >>+ */ > >>+struct regulator_init_data *of_get_regulator_init_data(struct device *dev) > > > >I remember that Mark ever had a comment on a previous regulator DT > >series from Haojian Zhuang, saying this DT parsing procedure can > >probably just be private to regulator core and invisible to regulator > >drivers. That said whenever regulator_register() gets called with > >parameter init_data as NULL, it should try to retrieve > >regulator_init_data from device tree. It will ease regulator drivers > >a little. They will only need to call regulator_register() with NULL > >init_data for DT case. More importantly, doing so will help solve the > >dual 'dev' problem I see below. > > Yes, it seems like a good idea given that most drivers seem to blindly > pass the regulator_init_data onto regulator_register, however there > are cases like the twl regulator driver which seems to peek into the > constraints passed from the board to make sure it drops anything that > the hardware does not support, I'm not sure why twl works in that way. Is it a sign that those configuration peeked by twl regulator driver should be encoded in twl regulator driver itself instead of being passed from the board? Or why the board does not pass something matching driver/hardware capability to save that peek? > or cases like the db8500 where > regulator_init_data for all regulators are bundled together and the > driver extracts and registers them as separate regulators. For this particular case, we just end up with having some duplicated constraints description in the dts file. To me, it's not a problem. > Exporting an > api instead to extract regulator_init_data to the driver might help > in those cases. > IMO, these cases should be generalized to fit the common pattern of regulator drivers. > > > >>+{ > >>+ struct regulator_init_data *init_data; > >>+ > >>+ if (!dev->of_node) > >>+ return NULL; > >>+ > >>+ init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL); > >>+ if (!init_data) > >>+ return NULL; /* Out of memory? */ > >>+ > >>+ of_get_regulation_constraints(dev->of_node,&init_data); > > > >Beside the 'dev' here with of_node attached, there will be another > >'dev' created by regulator core function regulator_register(), which > >is wrapped by 'regulator_dev'. > > > >So we have two 'dev'. One is created by DT core with of_node attached, > >and used to retrieve regulator_init_data from device tree. Another one > >is created in regulator_register() and used by regulator core. > > But thats not something newly done now with DT. Thats how it was even > in the non-DT world. There were always two devices with the > regulator_dev device as the child. > Let's look at mc13892-regulator driver. There are 23 regulators defined in array mc13892_regulators. Needless to say, there is a dev behind mc13892-regulator driver. And when getting probed, this driver will call regulator_register() to register those 23 regulators individually. That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the parent of all other 23 'dev' (wrapped by regulator_dev). But with the current DT implementation, we will have at least 1 + 23 * 2 'dev'. These extra 23 'dev' is totally new with DT. Regards, Shawn > > > >IMO, this is not what we want. Instead of having two 'dev' for given > >regulator, we should use the same 'dev' created in regulator_register() > >to retrieve regulator_init_data from device tree, so that we do not need > >the 'dev' created by DT core. That said, with DT parsing procedure > >moved into regulator core, we can get of_node for given regulator and > >attach it to the 'dev' created by regulator core. > > > >Will elaborate it a little more in patch #5. > > > >Regards, > >Shawn > > > >>+ return init_data; > >>+} > >>diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h > >>new file mode 100644 > >>index 0000000..c5a1ad6 > >>--- /dev/null > >>+++ b/include/linux/regulator/of_regulator.h > >>@@ -0,0 +1,21 @@ > >>+/* > >>+ * OpenFirmware regulator support routines > >>+ * > >>+ */ > >>+ > >>+#ifndef __LINUX_OF_REG_H > >>+#define __LINUX_OF_REG_H > >>+ > >>+#if defined(CONFIG_OF_REGULATOR) > >>+extern struct regulator_init_data > >>+ *of_get_regulator_init_data(struct device *dev); > >>+#else > >>+static inline struct regulator_init_data > >>+ *of_get_regulator_init_data(struct device *dev) > >>+{ > >>+ return NULL; > >>+} > >>+#endif /* CONFIG_OF_REGULATOR */ > >>+ > >>+#endif /* __LINUX_OF_REG_H */ > >>+ > >>-- > >>1.7.1 > >> > >>_______________________________________________ > >>devicetree-discuss mailing list > >>devicetree-discuss@lists.ozlabs.org > >>https://lists.ozlabs.org/listinfo/devicetree-discuss > >> > > > >
On Wed, Oct 19, 2011 at 01:33:55PM +0800, Shawn Guo wrote: > On Tue, Oct 18, 2011 at 05:00:46PM +0100, Mark Brown wrote: > > It's not just Linux-specific stuff, some of this is even specific to > > what current Linux drivers can do - updating the kernel could mean a > > different set of constraints. > Well, from what I see, the 'struct regulation_constraints' is defined > in machine.h and meant to be the regulator machine/board interface. ...which will depend on the system integrator's understanding of what their system is capable of right now. > With the example I'm looking at, mc13892, the regulation_constraints > configuration is fully passed from machine/board file. If there is > something specific to what drivers can do, it probably should be encoded > in regulator driver rather than staying in regulation_constraints. I don't think you're quite understanding the issue - it's an integration problem with three different variables. It's a combination of what the chips can do, what the drivers can do and if the board design affects any of this stuff. Only the board can come to a final decision.
On Wed, Oct 19, 2011 at 10:42:16PM +0800, Shawn Guo wrote: *Please* delete irrelevant quotes from mails. > On Wed, Oct 19, 2011 at 05:05:56PM +0530, Rajendra Nayak wrote: > > Yes, it seems like a good idea given that most drivers seem to blindly > > pass the regulator_init_data onto regulator_register, however there > > are cases like the twl regulator driver which seems to peek into the > > constraints passed from the board to make sure it drops anything that > > the hardware does not support, > I'm not sure why twl works in that way. Is it a sign that those > configuration peeked by twl regulator driver should be encoded in twl > regulator driver itself instead of being passed from the board? Or > why the board does not pass something matching driver/hardware > capability to save that peek? It's completely unneeded but harmless, it's there because there were a large number of discussions going on with the original author and it was easier to merge the code.
On Wed, Oct 19, 2011 at 03:47:34PM +0100, Mark Brown wrote: > On Wed, Oct 19, 2011 at 01:33:55PM +0800, Shawn Guo wrote: > > On Tue, Oct 18, 2011 at 05:00:46PM +0100, Mark Brown wrote: > > > > It's not just Linux-specific stuff, some of this is even specific to > > > what current Linux drivers can do - updating the kernel could mean a > > > different set of constraints. > > > Well, from what I see, the 'struct regulation_constraints' is defined > > in machine.h and meant to be the regulator machine/board interface. > > ...which will depend on the system integrator's understanding of what > their system is capable of right now. > > > With the example I'm looking at, mc13892, the regulation_constraints > > configuration is fully passed from machine/board file. If there is > > something specific to what drivers can do, it probably should be encoded > > in regulator driver rather than staying in regulation_constraints. > > I don't think you're quite understanding the issue - it's an integration > problem with three different variables. It's a combination of what the > chips can do, what the drivers can do and if the board design affects > any of this stuff. > Honestly, I'm still pretty new to regulator subsystem, so really need your help to understand the situation. > Only the board can come to a final decision. > The dts is very capable and suitable to describe the board's decision. But you disagree that we put all constraints description into DT. I'm a pretty confused here. So again, what is your suggestion to this 'problem'?
On Wed, Oct 19, 2011 at 11:04:49PM +0800, Shawn Guo wrote: > On Wed, Oct 19, 2011 at 03:47:34PM +0100, Mark Brown wrote: > > Only the board can come to a final decision. > The dts is very capable and suitable to describe the board's decision. > But you disagree that we put all constraints description into DT. I'm > a pretty confused here. So again, what is your suggestion to this > 'problem'? The problem is that the DT is supposed to be separate to the kernel and the decisions can depend on what the kernel is currently capable of. When the data is embedded in the kernel it's not an issue as the data is attached to the rest of the code, when the data becomes detatched from the kernel it becomes an issue. I don't see any issue with leaving some things out of the DT bindings; you were the one raising that as a concern.
On Wednesday 19 October 2011 08:40 PM, Mark Brown wrote: > The problem is that the DT is supposed to be separate to the kernel and > the decisions can depend on what the kernel is currently capable of. > When the data is embedded in the kernel it's not an issue as the data is > attached to the rest of the code, when the data becomes detatched from > the kernel it becomes an issue. completely agreed. > > I don't see any issue with leaving some things out of the DT bindings; > you were the one raising that as a concern. The problem is, that there doesn't seem to be a clean way to embed *board data* into the kernel with DT, if left out of the DT bindings. There is the auxdata way of still attaching platform_data, but that I thought was a stopgap for just handling function pointers.
> > I'm not sure why twl works in that way. Is it a sign that those > configuration peeked by twl regulator driver should be encoded in twl > regulator driver itself instead of being passed from the board? Or No, the driver is just trying to make sure that nothing invalid (not supported by hardware) gets passed from the boards. > why the board does not pass something matching driver/hardware > capability to save that peek? > >> or cases like the db8500 where >> regulator_init_data for all regulators are bundled together and the >> driver extracts and registers them as separate regulators. > > For this particular case, we just end up with having some duplicated > constraints description in the dts file. To me, it's not a problem. > >> Exporting an >> api instead to extract regulator_init_data to the driver might help >> in those cases. >> > IMO, these cases should be generalized to fit the common pattern of > regulator drivers. > >>> >>>> +{ >>>> + struct regulator_init_data *init_data; >>>> + >>>> + if (!dev->of_node) >>>> + return NULL; >>>> + >>>> + init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL); >>>> + if (!init_data) >>>> + return NULL; /* Out of memory? */ >>>> + >>>> + of_get_regulation_constraints(dev->of_node,&init_data); >>> >>> Beside the 'dev' here with of_node attached, there will be another >>> 'dev' created by regulator core function regulator_register(), which >>> is wrapped by 'regulator_dev'. >>> >>> So we have two 'dev'. One is created by DT core with of_node attached, >>> and used to retrieve regulator_init_data from device tree. Another one >>> is created in regulator_register() and used by regulator core. >> >> But thats not something newly done now with DT. Thats how it was even >> in the non-DT world. There were always two devices with the >> regulator_dev device as the child. >> > Let's look at mc13892-regulator driver. There are 23 regulators defined > in array mc13892_regulators. Needless to say, there is a dev behind > mc13892-regulator driver. And when getting probed, this driver will > call regulator_register() to register those 23 regulators individually. > That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the > parent of all other 23 'dev' (wrapped by regulator_dev). But with the > current DT implementation, we will have at least 1 + 23 * 2 'dev'. > These extra 23 'dev' is totally new with DT. > but thats only because the mc13892-regulator driver is implemeted in such a way that all the regulators on the platform are bundled in as *one* device. It would again depend on how you would pass these from the DT, if you indeed stick to the same way of bundling all regulators as one device from DT, the mc13892-regulator probe would just get called once and there would be one device associated, no?
On Thu, Oct 20, 2011 at 10:48:58AM +0530, Rajendra Nayak wrote: > >Let's look at mc13892-regulator driver. There are 23 regulators defined > >in array mc13892_regulators. Needless to say, there is a dev behind > >mc13892-regulator driver. And when getting probed, this driver will > >call regulator_register() to register those 23 regulators individually. > >That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the > >parent of all other 23 'dev' (wrapped by regulator_dev). But with the > >current DT implementation, we will have at least 1 + 23 * 2 'dev'. > >These extra 23 'dev' is totally new with DT. > > > > but thats only because the mc13892-regulator driver is implemeted in > such a way that all the regulators on the platform are bundled in as > *one* device. I did not look into too many regulator drivers, but I expect this is way that most regulator drivers are implemented in. Having mc13892-regulator being probed 23 times to register these 23 regulators just makes less sense to me. > It would again depend on how you would pass these from > the DT, if you indeed stick to the same way of bundling all regulators > as one device from DT, the mc13892-regulator probe would just get called > once and there would be one device associated, no? > Yes, I indeed would stick to the same way of bundling the registration of all regulators with mc13892-regulator being probed once. The problem I have with the current regulator core DT implementation is that it assumes the device_node of rdev->dev (dev wrapped in regulator_dev) is being attached to rdev->dev.parent rather than itself. Back to mc13892-regulator example, that said, it requires the dev of mc13892-regulator have the device_node of individual regulator attached to. IOW, the current implementation forces mc13892-regulator to be probed 23 times to register those 23 regulators. This is wrong to me.
On Thu, Oct 20, 2011 at 09:12:10AM +0530, Rajendra Nayak wrote: > On Wednesday 19 October 2011 08:40 PM, Mark Brown wrote: > >I don't see any issue with leaving some things out of the DT bindings; > >you were the one raising that as a concern. > The problem is, that there doesn't seem to be a clean way to embed > *board data* into the kernel with DT, if left out of the DT bindings. > There is the auxdata way of still attaching platform_data, but that I > thought was a stopgap for just handling function pointers. We can always start off just completely omitting the data and then see how we go from there. If we only cover 50% of users that's still 50% more than are currently covered with device tree right now and it means we can then spin round and look at the bits that are hard again without review fatigue on the bits that are easy.
On Thursday 20 October 2011 11:44 AM, Shawn Guo wrote: > On Thu, Oct 20, 2011 at 10:48:58AM +0530, Rajendra Nayak wrote: >>> Let's look at mc13892-regulator driver. There are 23 regulators defined >>> in array mc13892_regulators. Needless to say, there is a dev behind >>> mc13892-regulator driver. And when getting probed, this driver will >>> call regulator_register() to register those 23 regulators individually. >>> That said, for non-dt world, we have 1 + 23 'dev' with that 1 as the >>> parent of all other 23 'dev' (wrapped by regulator_dev). But with the >>> current DT implementation, we will have at least 1 + 23 * 2 'dev'. >>> These extra 23 'dev' is totally new with DT. >>> >> >> but thats only because the mc13892-regulator driver is implemeted in >> such a way that all the regulators on the platform are bundled in as >> *one* device. > > I did not look into too many regulator drivers, but I expect this is > way that most regulator drivers are implemented in. Having > mc13892-regulator being probed 23 times to register these 23 regulators > just makes less sense to me. > >> It would again depend on how you would pass these from >> the DT, if you indeed stick to the same way of bundling all regulators >> as one device from DT, the mc13892-regulator probe would just get called >> once and there would be one device associated, no? >> > Yes, I indeed would stick to the same way of bundling the registration > of all regulators with mc13892-regulator being probed once. The problem > I have with the current regulator core DT implementation is that it > assumes the device_node of rdev->dev (dev wrapped in regulator_dev) is > being attached to rdev->dev.parent rather than itself. Back to > mc13892-regulator example, that said, it requires the dev of > mc13892-regulator have the device_node of individual regulator attached > to. IOW, the current implementation forces mc13892-regulator to be > probed 23 times to register those 23 regulators. This is wrong to me. I think I now understand to some extent the problem that you seem to be reporting. It is mainly with drivers which bundle all regulators and pass them as one device and would want to do so with DT too. however I am still not clear on how what you seem to suggest would solve this problem. Note that not all drivers do it this way, and there are drivers where each regulator is considered as one device and I suspect they would remain that way with DT too. And hence we need to support both. Do you have any RFC patch/code which could explain better what you are suggesting we do here? >
> > We can always start off just completely omitting the data and then see > how we go from there. If we only cover 50% of users that's still 50% > more than are currently covered with device tree right now and it means > we can then spin round and look at the bits that are hard again without > review fatigue on the bits that are easy. Ok, will drop the problematic bindings for now and repost.
* Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 02:07]: > On Thu, Oct 20, 2011 at 09:12:10AM +0530, Rajendra Nayak wrote: > > On Wednesday 19 October 2011 08:40 PM, Mark Brown wrote: > > > >I don't see any issue with leaving some things out of the DT bindings; > > >you were the one raising that as a concern. > > > The problem is, that there doesn't seem to be a clean way to embed > > *board data* into the kernel with DT, if left out of the DT bindings. > > There is the auxdata way of still attaching platform_data, but that I > > thought was a stopgap for just handling function pointers. > > We can always start off just completely omitting the data and then see > how we go from there. If we only cover 50% of users that's still 50% > more than are currently covered with device tree right now and it means > we can then spin round and look at the bits that are hard again without > review fatigue on the bits that are easy. We still need to pass the board configuration somehow, otherwise we can never remove all the platform data glue layers. And if we can't do that, we'll forever have all the nasty merge conflicts when adding new drivers. And there's an unnecessary dependency between adding drivers and the core SoC code. So we really need to remove all the platform data glue layers so driver probe can initialize things automagically. Kernel cmdline is of course one way to pass options, but probably won't scale for all the board wirings.. And the kernel cmdline should really be for user configurable options. So should we just have a cmdline string for each DT entry to pass the board specific options from DT to the driver? Something along the lines module options? Regards, Tony
On Thu, Oct 20, 2011 at 09:27:43AM -0700, Tony Lindgren wrote: > * Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 02:07]: > > We can always start off just completely omitting the data and then see > > how we go from there. If we only cover 50% of users that's still 50% > > more than are currently covered with device tree right now and it means > > we can then spin round and look at the bits that are hard again without > > review fatigue on the bits that are easy. > We still need to pass the board configuration somehow, otherwise we can > never remove all the platform data glue layers. And if we can't do that, > we'll forever have all the nasty merge conflicts when adding new drivers. > And there's an unnecessary dependency between adding drivers and the > core SoC code. The current patches cover the overwhelming majority of the existing board configuration - the stuff that's Linux specific is also relatively rarely used in actual systems. For example all the board I work with regularly would be perfectly happy with the generic stuff - the Linux specific stuff is relatively rarely used.
* Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 09:05]: > On Thu, Oct 20, 2011 at 09:27:43AM -0700, Tony Lindgren wrote: > > * Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 02:07]: > > > > We can always start off just completely omitting the data and then see > > > how we go from there. If we only cover 50% of users that's still 50% > > > more than are currently covered with device tree right now and it means > > > we can then spin round and look at the bits that are hard again without > > > review fatigue on the bits that are easy. > > > We still need to pass the board configuration somehow, otherwise we can > > never remove all the platform data glue layers. And if we can't do that, > > we'll forever have all the nasty merge conflicts when adding new drivers. > > And there's an unnecessary dependency between adding drivers and the > > core SoC code. > > The current patches cover the overwhelming majority of the existing > board configuration - the stuff that's Linux specific is also relatively > rarely used in actual systems. For example all the board I work with > regularly would be perfectly happy with the generic stuff - the Linux > specific stuff is relatively rarely used. Right, but in addition the board specific integration variables still need to be passed somehow to the driver. That's where a DT entry specific configuration string might be the best option as it still allows describing the hardware using DT standards, while also allowing board specific configuration too. The issue there is then how do we keep these options from getting out of control.. BTW, of course this issue is generic to all drivers, not specific to this patchset. With this patchset leaving out the non-standard entries is the right way to go to so we can merge it. Cheers, Tony
* Tony Lindgren <tony@atomide.com> [111020 09:31]: > > That's where a DT entry specific configuration string might be the best > option as it still allows describing the hardware using DT standards, > while also allowing board specific configuration too. Hmm, actually, can't we just pass the board specific configuration in the board specific .dts file and then it gets merged in with the arch independent DT node? Regards, Tony
On Thu, Oct 20, 2011 at 10:05:34AM -0700, Tony Lindgren wrote: > Right, but in addition the board specific integration variables still > need to be passed somehow to the driver. I'm not sure you're aware of what the regulator bindings that are being discussed are - they're entirely board specific. The discussion was about how to handle certain parts of the feature set Linux currently has.
On Thu, Oct 20, 2011 at 10:22:19AM -0700, Tony Lindgren wrote: > Hmm, actually, can't we just pass the board specific configuration in > the board specific .dts file and then it gets merged in with the > arch independent DT node? I don't understand what an "arch independent DT node" would be?
* Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 12:23]: > On Thu, Oct 20, 2011 at 10:22:19AM -0700, Tony Lindgren wrote: > > > Hmm, actually, can't we just pass the board specific configuration in > > the board specific .dts file and then it gets merged in with the > > arch independent DT node? > > I don't understand what an "arch independent DT node" would be? Well that's the standard non-Linux specific parts. Basically what's in the $Subject patch minus what you commented on earlier: > +- regulator-change-voltage: boolean, Output voltage can be changed by software > +- regulator-change-current: boolean, Output current can be chnaged by software > +- regulator-change-mode: boolean, Operating mode can be changed by software > +- regulator-change-status: boolean, Enable/Disable control for regulator exists > +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled > +- regulator-mode-fast: boolean, allow/set fast mode for the regulator > +- regulator-mode-normal: boolean, allow/set normal mode for the regulator > +- regulator-mode-idle: boolean, allow/set idle mode for the regulator > +- regulator-mode-standby: boolean, allow/set standby mode for the regulator > +- regulator-input-uV: Input voltage for regulator when supplied by another regulator > +- regulator-always-on: boolean, regulator should never be disabled We still need to figure out how to get the above board specific data to the device driver probe in a way where we can avoid having platform glue code. Any thoughts on that? Tony
On Thu, Oct 20, 2011 at 01:10:55PM -0700, Tony Lindgren wrote: > * Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 12:23]: > > I don't understand what an "arch independent DT node" would be? > Well that's the standard non-Linux specific parts. Basically what's > in the $Subject patch minus what you commented on earlier: Oh, you don't mean arch specific at all then. > > +- regulator-change-voltage: boolean, Output voltage can be changed by software > > +- regulator-change-current: boolean, Output current can be chnaged by software These aren't really Linux specific, and you could probably just infer them from having a voltage/current range anyway. > > +- regulator-change-mode: boolean, Operating mode can be changed by software > > +- regulator-change-status: boolean, Enable/Disable control for regulator exists > > +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled > > +- regulator-mode-fast: boolean, allow/set fast mode for the regulator > > +- regulator-mode-normal: boolean, allow/set normal mode for the regulator > > +- regulator-mode-idle: boolean, allow/set idle mode for the regulator > > +- regulator-mode-standby: boolean, allow/set standby mode for the regulator > > +- regulator-input-uV: Input voltage for regulator when supplied by another regulator > > +- regulator-always-on: boolean, regulator should never be disabled > We still need to figure out how to get the above board specific > data to the device driver probe in a way where we can avoid having > platform glue code. Any thoughts on that? I think we should just not bother with most of the above and see if anyone notices, with the exception of always on and status changes it's vanishingly rare for anyone to actually do anything constructive with them. Neither of the two I mentioned are Linux specific either, they mean somehing useful for any OS it's just that the decision is policy which is going to depend on the OS version. Status changes we can probably allow people to enable, it'll just mean that older device trees won't get good power use out of newer kernels and if you move to an older kernel things will start exploding as drivers loose regulator support. Always on we can probably live without, it's a combination of overspecification and interaction with _has_full_constraints() which isn't represented in this binding anyway.
* Mark Brown <broonie@opensource.wolfsonmicro.com> [111020 14:08]: > On Thu, Oct 20, 2011 at 01:10:55PM -0700, Tony Lindgren wrote: > > > We still need to figure out how to get the above board specific > > data to the device driver probe in a way where we can avoid having > > platform glue code. Any thoughts on that? > > I think we should just not bother with most of the above and see if > anyone notices, with the exception of always on and status changes it's > vanishingly rare for anyone to actually do anything constructive with > them. OK > Neither of the two I mentioned are Linux specific either, they mean > somehing useful for any OS it's just that the decision is policy which > is going to depend on the OS version. Status changes we can probably > allow people to enable, it'll just mean that older device trees won't > get good power use out of newer kernels and if you move to an older > kernel things will start exploding as drivers loose regulator support. > Always on we can probably live without, it's a combination of > overspecification and interaction with _has_full_constraints() which > isn't represented in this binding anyway. Yeah, lot of these are "how the regulator using driver is using the regulator" or "how the operating system is using the regulator" type policies. Regards, Tony
On Thu, Oct 20, 2011 at 10:22:19AM -0700, Tony Lindgren wrote: > * Tony Lindgren <tony@atomide.com> [111020 09:31]: > > > > That's where a DT entry specific configuration string might be the best > > option as it still allows describing the hardware using DT standards, > > while also allowing board specific configuration too. > > Hmm, actually, can't we just pass the board specific configuration in > the board specific .dts file and then it gets merged in with the > arch independent DT node? It would be fine to, say, have a omap-specific board configuration node for collecting things that really don't fit anywhere else. That would be in some ways analogous to the way we've decided to use top a top level node for describing a machines audio complex. g.
diff --git a/Documentation/devicetree/bindings/regulator/regulator.txt b/Documentation/devicetree/bindings/regulator/regulator.txt new file mode 100644 index 0000000..a623fdd --- /dev/null +++ b/Documentation/devicetree/bindings/regulator/regulator.txt @@ -0,0 +1,44 @@ +Voltage/Current Regulators + +Optional properties: +- regulator-min-uV: smallest voltage consumers may set +- regulator-max-uV: largest voltage consumers may set +- regulator-uV-offset: Offset applied to voltages to compensate for voltage drops +- regulator-min-uA: smallest current consumers may set +- regulator-max-uA: largest current consumers may set +- regulator-change-voltage: boolean, Output voltage can be changed by software +- regulator-change-current: boolean, Output current can be chnaged by software +- regulator-change-mode: boolean, Operating mode can be changed by software +- regulator-change-status: boolean, Enable/Disable control for regulator exists +- regulator-change-drms: boolean, Dynamic regulator mode switching is enabled +- regulator-mode-fast: boolean, allow/set fast mode for the regulator +- regulator-mode-normal: boolean, allow/set normal mode for the regulator +- regulator-mode-idle: boolean, allow/set idle mode for the regulator +- regulator-mode-standby: boolean, allow/set standby mode for the regulator +- regulator-input-uV: Input voltage for regulator when supplied by another regulator +- regulator-always-on: boolean, regulator should never be disabled +- regulator-boot-on: bootloader/firmware enabled regulator +- <name>-supply: phandle to the parent supply/regulator node + +Example: + + xyzreg: regulator@0 { + regulator-min-uV = <1000000>; + regulator-max-uV = <2500000>; + regulator-mode-fast; + regulator-change-voltage; + regulator-always-on; + vin-supply = <&vin>; + }; + +The same binding used by a regulator to reference its +supply can be used by any consumer to reference its +regulator/supply + +Example of a device node referencing a regulator node, + + devicenode: node@0x0 { + ... + ... + <name>-supply = <&xyzreg>; + }; diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig index c7fd2c0..981c92e 100644 --- a/drivers/regulator/Kconfig +++ b/drivers/regulator/Kconfig @@ -64,6 +64,14 @@ config REGULATOR_USERSPACE_CONSUMER If unsure, say no. +config OF_REGULATOR + tristate "OF regulator helpers" + depends on OF + default y if OF + help + OpenFirmware regulator framework helper routines that can + used by regulator drivers to extract data from device tree. + config REGULATOR_BQ24022 tristate "TI bq24022 Dual Input 1-Cell Li-Ion Charger IC" help diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile index 040d5aa..e6bc009 100644 --- a/drivers/regulator/Makefile +++ b/drivers/regulator/Makefile @@ -7,6 +7,7 @@ obj-$(CONFIG_REGULATOR) += core.o dummy.o obj-$(CONFIG_REGULATOR_FIXED_VOLTAGE) += fixed.o obj-$(CONFIG_REGULATOR_VIRTUAL_CONSUMER) += virtual.o obj-$(CONFIG_REGULATOR_USERSPACE_CONSUMER) += userspace-consumer.o +obj-$(CONFIG_OF_REGULATOR) += of_regulator.o obj-$(CONFIG_REGULATOR_AD5398) += ad5398.o obj-$(CONFIG_REGULATOR_BQ24022) += bq24022.o diff --git a/drivers/regulator/of_regulator.c b/drivers/regulator/of_regulator.c new file mode 100644 index 0000000..9262d06 --- /dev/null +++ b/drivers/regulator/of_regulator.c @@ -0,0 +1,93 @@ +/* + * OF helpers for regulator framework + * + * Copyright (C) 2011 Texas Instruments, Inc. + * Rajendra Nayak <rnayak@ti.com> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/slab.h> +#include <linux/of.h> +#include <linux/regulator/machine.h> + +static void of_get_regulation_constraints(struct device_node *np, + struct regulator_init_data **init_data) +{ + const __be32 *min_uV, *max_uV, *uV_offset; + const __be32 *min_uA, *max_uA, *input_uV; + unsigned int valid_modes_mask = 0, valid_ops_mask = 0; + + min_uV = of_get_property(np, "regulator-min-uV", NULL); + if (min_uV) + (*init_data)->constraints.min_uV = be32_to_cpu(*min_uV); + max_uV = of_get_property(np, "regulator-max-uV", NULL); + if (max_uV) + (*init_data)->constraints.max_uV = be32_to_cpu(*max_uV); + uV_offset = of_get_property(np, "regulator-uV-offset", NULL); + if (uV_offset) + (*init_data)->constraints.uV_offset = be32_to_cpu(*uV_offset); + min_uA = of_get_property(np, "regulator-min-uA", NULL); + if (min_uA) + (*init_data)->constraints.min_uA = be32_to_cpu(*min_uA); + max_uA = of_get_property(np, "regulator-max-uA", NULL); + if (max_uA) + (*init_data)->constraints.max_uA = be32_to_cpu(*max_uA); + input_uV = of_get_property(np, "regulator-input-uV", NULL); + if (input_uV) + (*init_data)->constraints.input_uV = be32_to_cpu(*input_uV); + + /* valid modes mask */ + if (of_find_property(np, "regulator-mode-fast", NULL)) + valid_modes_mask |= REGULATOR_MODE_FAST; + if (of_find_property(np, "regulator-mode-normal", NULL)) + valid_modes_mask |= REGULATOR_MODE_NORMAL; + if (of_find_property(np, "regulator-mode-idle", NULL)) + valid_modes_mask |= REGULATOR_MODE_IDLE; + if (of_find_property(np, "regulator-mode-standby", NULL)) + valid_modes_mask |= REGULATOR_MODE_STANDBY; + + /* valid ops mask */ + if (of_find_property(np, "regulator-change-voltage", NULL)) + valid_ops_mask |= REGULATOR_CHANGE_VOLTAGE; + if (of_find_property(np, "regulator-change-current", NULL)) + valid_ops_mask |= REGULATOR_CHANGE_CURRENT; + if (of_find_property(np, "regulator-change-mode", NULL)) + valid_ops_mask |= REGULATOR_CHANGE_MODE; + if (of_find_property(np, "regulator-change-status", NULL)) + valid_ops_mask |= REGULATOR_CHANGE_STATUS; + + (*init_data)->constraints.valid_modes_mask = valid_modes_mask; + (*init_data)->constraints.valid_ops_mask = valid_ops_mask; + + if (of_find_property(np, "regulator-always-on", NULL)) + (*init_data)->constraints.always_on = true; + if (of_find_property(np, "regulator-boot-on", NULL)) + (*init_data)->constraints.boot_on = true; +} + +/** + * of_get_regulator_init_data - extract regulator_init_data structure info + * @dev: device requesting for regulator_init_data + * + * Populates regulator_init_data structure by extracting data from device + * tree node, returns a pointer to the populated struture or NULL if memory + * alloc fails. + */ +struct regulator_init_data *of_get_regulator_init_data(struct device *dev) +{ + struct regulator_init_data *init_data; + + if (!dev->of_node) + return NULL; + + init_data = devm_kzalloc(dev, sizeof(*init_data), GFP_KERNEL); + if (!init_data) + return NULL; /* Out of memory? */ + + of_get_regulation_constraints(dev->of_node, &init_data); + return init_data; +} diff --git a/include/linux/regulator/of_regulator.h b/include/linux/regulator/of_regulator.h new file mode 100644 index 0000000..c5a1ad6 --- /dev/null +++ b/include/linux/regulator/of_regulator.h @@ -0,0 +1,21 @@ +/* + * OpenFirmware regulator support routines + * + */ + +#ifndef __LINUX_OF_REG_H +#define __LINUX_OF_REG_H + +#if defined(CONFIG_OF_REGULATOR) +extern struct regulator_init_data + *of_get_regulator_init_data(struct device *dev); +#else +static inline struct regulator_init_data + *of_get_regulator_init_data(struct device *dev) +{ + return NULL; +} +#endif /* CONFIG_OF_REGULATOR */ + +#endif /* __LINUX_OF_REG_H */ +
The helper routine is meant to be used by regulator drivers to extract the regulator_init_data structure from the data that is passed from device tree. 'consumer_supplies' which is part of regulator_init_data is not extracted as the regulator consumer mappings are passed through DT differently, implemented in subsequent patches. Similarly the regulator<-->parent/supply mapping is handled in subsequent patches. Also add documentation for regulator bindings to be used to pass regulator_init_data struct information from device tree. Signed-off-by: Rajendra Nayak <rnayak@ti.com> --- .../devicetree/bindings/regulator/regulator.txt | 44 +++++++++ drivers/regulator/Kconfig | 8 ++ drivers/regulator/Makefile | 1 + drivers/regulator/of_regulator.c | 93 ++++++++++++++++++++ include/linux/regulator/of_regulator.h | 21 +++++ 5 files changed, 167 insertions(+), 0 deletions(-) create mode 100644 Documentation/devicetree/bindings/regulator/regulator.txt create mode 100644 drivers/regulator/of_regulator.c create mode 100644 include/linux/regulator/of_regulator.h