Re: [PATCH] Parse missing regulator constraints from device tree blob

[Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> 
> --------------------------------------------------------------------------------
> --- 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html