| Message ID | 23745449.572421389854083634.JavaMail.weblogic@epml20 |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi, On Thu, Jan 16, 2014 at 06:34:46AM +0000, Saurabh Singh wrote: > This patch adds support for parsing following regulator contraints from device tree blob. > 1. valid modes mask (valid_modes_mask) > 2. input microvolt(input_uV) > 3. initial mode (initial_mode) > 4. initial state (initial_state) > 5. state mem (state_mem) > 6. state disk (state_disk) > 7. state standby (state_standby) > > This patch is currently against a linux 3.12.6 kernel. > > diffstat for this patch is: > of_regulator.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 63 insertions(+) > > To apply the patch, in the root of a kernel tree use: > patch -p1 < of_regulator.patch > > Please let me know any feedback you have on this patch or the approach used. There are several issues with this patch: * None of these properties are documented. Documentation is required so that the contract is defined. That allows people to learn how to use the properties, and makes clear what we can and cannot change kernel-side. * It leaks Linux internal details (e.g. suspend_state_t values, valid_mode_mask) without any attempt at abstraction, in violation of dt principles. * Accessors are used poorly. Endianness conversion is done manually rather than being left to accessors, and property lengths aren't checked. > > Regards, > ===================== > Saurabh Singh Sengar > Lead Engineer > Samsung R&D Institute > India > Samsung > ===================== > Signed-off-by: Saurabh Singh Sengar <saurabh1.s@samsung.com> > > -------------------------------------------------------------------------------- > --- linux-3.12.6/drivers/regulator/of_regulator.c.orig 2014-01-08 17:19:43.085903573 +0530 > +++ linux-3.12.6/drivers/regulator/of_regulator.c 2014-01-15 20:12:22.146543128 +0530 > @@ -16,11 +16,40 @@ > #include <linux/regulator/machine.h> > #include <linux/regulator/of_regulator.h> > > +/** > + * set_regulator_state_constraints - set regulator state for low power system states > + * @np: device node for the low power regulator state > + * @regulator_state: regulator_state structure need to be filled > + */ > +static void set_regulator_state_constraints(struct device_node *np, > + struct regulator_state *regulator_state) > +{ > + const __be32 *uV, *mode; > + > + uV = of_get_property(np, "regulator-state-uV", NULL); Typically properties are all lower-case. > + if (uV) > + regulator_state->uV = be32_to_cpu(*uV); As an example, to use accessors correctly, here you should have: u32 uv; of_property_read_u32(np, "regulator-state-uv", &uv); However, as far as I can see this value should come from an input supply anyway. Thanks, Mark. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
===================== Saurabh Singh Sengar Lead Engineer Samsung R&D Institute India Samsung ===================== Signed-off-by: Saurabh Singh Sengar <saurabh1.s@samsung.com> -------------------------------------------------------------------------------- --- linux-3.12.6/drivers/regulator/of_regulator.c.orig 2014-01-08 17:19:43.085903573 +0530 +++ linux-3.12.6/drivers/regulator/of_regulator.c 2014-01-15 20:12:22.146543128 +0530 @@ -16,11 +16,40 @@ #include <linux/regulator/machine.h> #include <linux/regulator/of_regulator.h> +/** + * set_regulator_state_constraints - set regulator state for low power system states + * @np: device node for the low power regulator state + * @regulator_state: regulator_state structure need to be filled + */ +static void set_regulator_state_constraints(struct device_node *np, + struct regulator_state *regulator_state) +{ + const __be32 *uV, *mode; + + uV = of_get_property(np, "regulator-state-uV", NULL); + if (uV) + regulator_state->uV = be32_to_cpu(*uV); + + mode = of_get_property(np, "regulator-state-mode", NULL); + if (mode) + regulator_state->mode = be32_to_cpu(*mode); + + if (of_find_property(np, "regulator-state-enabled", NULL)) + regulator_state->enabled = true; + + if (of_find_property(np, "regulator-state-disabled", NULL)) + regulator_state->disabled = true; +} + + 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, *ramp_delay; + const __be32 *initial_mode, *initial_state; + const __be32 *input_uV, *valid_modes_mask; + struct device_node *state; struct property *prop; struct regulation_constraints *constraints = &(*init_data)->constraints; @@ -73,6 +102,40 @@ static void of_get_regulation_constraint else constraints->ramp_disable = true; } + + valid_modes_mask = of_get_property(np, + "regulator-valid-modes-mask", NULL); + if (valid_modes_mask) + constraints->valid_modes_mask = be32_to_cpu(*valid_modes_mask); + + input_uV = of_get_property(np, "regulator-input-microvolt", NULL); + if (input_uV) + constraints->input_uV = be32_to_cpu(*input_uV); + + initial_mode = of_get_property(np, "regulator-initial-mode", NULL); + if (initial_mode) + constraints->initial_mode = be32_to_cpu(*initial_mode); + + initial_state = of_get_property(np, "regulator-initial-state", NULL); + if (initial_state) + constraints->initial_state = be32_to_cpu(*initial_state); + + /* regulator state during low power system states */ + state = of_find_node_by_name(np, "regulator-state-mem"); + if (state) + set_regulator_state_constraints(state, + &constraints->state_mem); + + state = of_find_node_by_name(np, "regulator-state-disk"); + if (state) + set_regulator_state_constraints(state, + &constraints->state_disk); + + state = of_find_node_by_name(np, "regulator-state-standby"); + if (state) + set_regulator_state_constraints(state, + &constraints->state_standby); + } /**