[PATCH RFC 0/3] ARM: regulator: add Freescale MXS regulator driver

[PATCH RFC 0/3] ARM: regulator: add Freescale MXS regulator driver

[Stefan Wahren]

This patch series draft should adds support for Freescale i.MX23, i.MX28 
on-chip regulators. I'm new to regulator drivers and need early feedback for 
the driver's stage before i go in the wrong direction.

The information about the i.MX28 regulators are from chapter 11 of the 
reference manual [1].

This driver based on the Freescale high level [2] and low level driver [3], 
but contains the following changes:

* devictree support
* code cleanup

Please bear in mind that the code is not tested against real hardware and 
it's not expected to work.

I'm not sure if it's correct, but these series based on the for-next branch
of Mark Brown's regulator repository.

Any comments about the code are welcome.

[1] - http://cache.freescale.com/files/dsp/doc/ref_manual/MCIMX28RM.pdf
[2] - http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/drivers/regulator/mxs-regulator.c?h=imx_2.6.35_maintain
[3] - http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/tree/arch/arm/mach-mx28/power.c?h=imx_2.6.35_maintain

Stefan Wahren (3):
  DT: add binding for MXS regulator
  ARM: regulator: add Freescale MXS regulator driver
  DT: ARM: mxs: enable regulator support for i.MX28

 .../bindings/regulator/mxs-regulator.txt           |  38 ++
 arch/arm/boot/dts/imx28.dtsi                       |  50 ++-
 drivers/regulator/Kconfig                          |   9 +
 drivers/regulator/Makefile                         |   1 +
 drivers/regulator/mxs-regulator.c                  | 411 +++++++++++++++++++++
 5 files changed, 508 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/mxs-regulator.txt
 create mode 100644 drivers/regulator/mxs-regulator.c

-- 
1.8.1.4

[PATCH RFC 1/3] DT: add binding for MXS regulator

[Stefan Wahren]

This patch adds the Device tree bindings for the Freescale MXS on-chip
regulators.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 .../bindings/regulator/mxs-regulator.txt           | 38 ++++++++++++++++++++++
 1 file changed, 38 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/regulator/mxs-regulator.txt

diff --git a/Documentation/devicetree/bindings/regulator/mxs-regulator.txt b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
new file mode 100644
index 0000000..c3226cc
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
@@ -0,0 +1,38 @@
+MXS regulators
+
+Required node properties:
+- compatible: Should be "fsl,<soc>-power", where soc is "imx23" or "imx28"
+- reg: Offset and length of the register set for the device
+
+Required regulator properties:
+- compatible: Must be "fsl,mxs-regulator"
+- reg: Offset of the register set for the regulator
+- mxs-max-reg-val: Maximum value of this register
+
+Optional regulator properties:
+- mxs-default-microvolt: initial voltage of the regulator
+
+Any regulator property defined as part of the core regulator
+binding, defined in regulator.txt, can also be used.
+
+Example:
+
+	power: power@80044000 {
+		compatible = "fsl,imx28-power";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0x80044000 0x2000>;
+
+		reg_vddio: regulator-vddio@80044060 {
+			reg = <0x80044060>;
+			compatible = "fsl,mxs-regulator";
+			regulator-name = "vddio";
+			regulator-min-microvolt = <2800000>;
+			regulator-max-microvolt = <3600000>;
+			regulator-microvolt-offset = <80000>;
+			regulator-always-on;
+			mxs-max-reg-val = <0x10>;
+			mxs-default-microvolt = <3300000>;
+		};
+	};
+
-- 
1.8.1.4

Re: [PATCH RFC 1/3] DT: add binding for MXS regulator

[Sergei Shtylyov]

Hello.

On 9/7/2014 3:37 PM, Stefan Wahren wrote:

> This patch adds the Device tree bindings for the Freescale MXS on-chip
> regulators.

> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>   .../bindings/regulator/mxs-regulator.txt           | 38 ++++++++++++++++++++++
>   1 file changed, 38 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/regulator/mxs-regulator.txt

> diff --git a/Documentation/devicetree/bindings/regulator/mxs-regulator.txt b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
> new file mode 100644
> index 0000000..c3226cc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
> @@ -0,0 +1,38 @@
> +MXS regulators
> +
> +Required node properties:
> +- compatible: Should be "fsl,<soc>-power", where soc is "imx23" or "imx28"
> +- reg: Offset and length of the register set for the device
> +
> +Required regulator properties:
> +- compatible: Must be "fsl,mxs-regulator"
> +- reg: Offset of the register set for the regulator
> +- mxs-max-reg-val: Maximum value of this register
> +
> +Optional regulator properties:
> +- mxs-default-microvolt: initial voltage of the regulator

    Vendor specific property names should be prefixed with the vendor name and 
comma.

WBR, Sergei

Re: [PATCH RFC 1/3] DT: add binding for MXS regulator

[Mark Rutland]

On Sun, Sep 07, 2014 at 12:37:47PM +0100, Stefan Wahren wrote:
> This patch adds the Device tree bindings for the Freescale MXS on-chip
> regulators.
> 
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
> ---
>  .../bindings/regulator/mxs-regulator.txt           | 38 ++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/regulator/mxs-regulator.txt
> 
> diff --git a/Documentation/devicetree/bindings/regulator/mxs-regulator.txt b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
> new file mode 100644
> index 0000000..c3226cc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
> @@ -0,0 +1,38 @@
> +MXS regulators
> +
> +Required node properties:
> +- compatible: Should be "fsl,<soc>-power", where soc is "imx23" or "imx28"
> +- reg: Offset and length of the register set for the device

We require #address-cells and #size-cells if the child nodes have reg
entries.

> +
> +Required regulator properties:
> +- compatible: Must be "fsl,mxs-regulator"
> +- reg: Offset of the register set for the regulator

Is this the offset or the absolute physical address?

The example seems to be absolute.

> +- mxs-max-reg-val: Maximum value of this register

What does this even mean? What's the format?

Is this not implied by standard properties like regulator-max-microvolt?

> +Optional regulator properties:
> +- mxs-default-microvolt: initial voltage of the regulator

Why is this necessary?

Mark.

> +
> +Any regulator property defined as part of the core regulator
> +binding, defined in regulator.txt, can also be used.
> +
> +Example:
> +
> +	power: power@80044000 {
> +		compatible = "fsl,imx28-power";
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		reg = <0x80044000 0x2000>;
> +
> +		reg_vddio: regulator-vddio@80044060 {
> +			reg = <0x80044060>;
> +			compatible = "fsl,mxs-regulator";
> +			regulator-name = "vddio";
> +			regulator-min-microvolt = <2800000>;
> +			regulator-max-microvolt = <3600000>;
> +			regulator-microvolt-offset = <80000>;
> +			regulator-always-on;
> +			mxs-max-reg-val = <0x10>;
> +			mxs-default-microvolt = <3300000>;
> +		};
> +	};
> +
> -- 
> 1.8.1.4
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

Re: [PATCH RFC 1/3] DT: add binding for MXS regulator

[Stefan Wahren]

Hi Mark,

Am 09.09.2014 19:59, schrieb Mark Rutland:
> On Sun, Sep 07, 2014 at 12:37:47PM +0100, Stefan Wahren wrote:
>> This patch adds the Device tree bindings for the Freescale MXS on-chip
>> regulators.
>>
>> Signed-off-by: Stefan Wahren <stefan.wahren-eS4NqCHxEME@public.gmane.org>
>> ---
>>   .../bindings/regulator/mxs-regulator.txt           | 38 ++++++++++++++++++++++
>>   1 file changed, 38 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/regulator/mxs-regulator.txt
>>
>> diff --git a/Documentation/devicetree/bindings/regulator/mxs-regulator.txt b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
>> new file mode 100644
>> index 0000000..c3226cc
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/regulator/mxs-regulator.txt
>> @@ -0,0 +1,38 @@
>> +MXS regulators
>> +
>> +Required node properties:
>> +- compatible: Should be "fsl,<soc>-power", where soc is "imx23" or "imx28"
>> +- reg: Offset and length of the register set for the device
>
> We require #address-cells and #size-cells if the child nodes have reg
> entries.

okay.

>
>> +
>> +Required regulator properties:
>> +- compatible: Must be "fsl,mxs-regulator"
>> +- reg: Offset of the register set for the regulator
>
> Is this the offset or the absolute physical address?
>
> The example seems to be absolute.

You are right, the description is wrong.

>
>> +- mxs-max-reg-val: Maximum value of this register
>
> What does this even mean? What's the format?
>
> Is this not implied by standard properties like regulator-max-microvolt?

Unfortunately not as long the step size in microvolt isn't defined. The 
parameter defines the register value in hex corresponding to 
regulator-max-microvolt.

step size = (regulator-max-microvolt - regulator-min-microvolt) / 
mxs-max-reg-val

May be i missed or understand something wrong?

>
>> +Optional regulator properties:
>> +- mxs-default-microvolt: initial voltage of the regulator
>
> Why is this necessary?

The original driver from Freescale set the MX28 regulator vddio to 3.38 
V during registering. I think that is ugly to implement in the driver 
part, so i add a more general property in the devicetree binding.

>
> Mark.

Stefan
--
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

[PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator driver

[Stefan Wahren]

This patch adds driver support for Freescale i.MX23, i.MX28 on-chip regulators.
There are 4 voltage regulators: vddd, vdda, vddio, vddmem and 1 overall
current regulator.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 drivers/regulator/Kconfig         |   9 +
 drivers/regulator/Makefile        |   1 +
 drivers/regulator/mxs-regulator.c | 411 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 421 insertions(+)
 create mode 100644 drivers/regulator/mxs-regulator.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7a3a061..e90e587 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -432,6 +432,15 @@ config REGULATOR_MC13892
 	  Say y here to support the regulators found on the Freescale MC13892
 	  PMIC.
 
+config REGULATOR_MXS
+	tristate "Freescale MXS on-chip regulators"
+	depends on ARCH_MXS
+	default y if ARCH_MXS
+	help
+	  Say y here to support Freescale MXS on-chip regulators.
+	  It is recommended that this option be enabled on i.MX23,
+	  i.MX28 platform.
+
 config REGULATOR_PALMAS
 	tristate "TI Palmas PMIC Regulators"
 	depends on MFD_PALMAS
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 25271f8..0f5b66c 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -58,6 +58,7 @@ obj-$(CONFIG_REGULATOR_MAX77802) += max77802.o
 obj-$(CONFIG_REGULATOR_MC13783) += mc13783-regulator.o
 obj-$(CONFIG_REGULATOR_MC13892) += mc13892-regulator.o
 obj-$(CONFIG_REGULATOR_MC13XXX_CORE) +=  mc13xxx-regulator-core.o
+obj-$(CONFIG_REGULATOR_MXS) += mxs-regulator.o
 obj-$(CONFIG_REGULATOR_PALMAS) += palmas-regulator.o
 obj-$(CONFIG_REGULATOR_PFUZE100) += pfuze100-regulator.o
 obj-$(CONFIG_REGULATOR_TPS51632) += tps51632-regulator.o
diff --git a/drivers/regulator/mxs-regulator.c b/drivers/regulator/mxs-regulator.c
new file mode 100644
index 0000000..7d74518
--- /dev/null
+++ b/drivers/regulator/mxs-regulator.c
@@ -0,0 +1,411 @@
+/*
+ * Freescale STMP378X voltage regulators
+ *
+ * Embedded Alley Solutions, Inc <source@embeddedalley.com>
+ *
+ * Copyright (C) 2014 Stefan Wahren
+ * Copyright (C) 2010 Freescale Semiconductor, Inc.
+ * Copyright 2008 Embedded Alley Solutions, Inc All Rights Reserved.
+ */
+
+/*
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/of_regulator.h>
+#include <linux/slab.h>
+
+#define HW_POWER_VDDDCTRL	(0x00000040)
+#define HW_POWER_VDDACTRL	(0x00000050)
+#define HW_POWER_VDDIOCTRL	(0x00000060)
+#define HW_POWER_STS	        (0x000000c0)
+
+#define BM_POWER_STS_DC_OK	(1 << 9)
+#define BM_POWER_REG_MODE       (1 << 17)
+#define BM_POWER_LEVEL_TRG      0x1f
+
+#define MXS_REG5V_NOT_USB 0
+#define MXS_REG5V_IS_USB 1
+
+
+struct mxs_regulator {
+	struct regulator_desc rdesc;
+	struct regulator_init_data *initdata;
+	struct mxs_regulator *parent;
+
+	spinlock_t         lock;
+	wait_queue_head_t  wait_q;
+	struct notifier_block nb;
+
+	const char *name;
+	void __iomem *base_addr;
+	void __iomem *power_addr;
+	int mode;
+	int cur_uV;
+	int cur_uA;
+	int max_uA;
+	u32 max_reg_val;
+};
+
+static int mxs_set_voltage(struct regulator_dev *reg, int min_uV, int max_uV,
+			   unsigned *selector)
+{
+	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
+	struct regulation_constraints *con = &sreg->initdata->constraints;
+	void __iomem *power_sts = sreg->power_addr + HW_POWER_STS;
+	u32 val, regs, i;
+
+	pr_debug("%s: uv %d, min %d, max %d\n", __func__,
+		max_uV, con->min_uV, con->max_uV);
+
+	if (max_uV < con->min_uV || max_uV > con->max_uV)
+		return -EINVAL;
+
+	val = (max_uV - con->min_uV) * sreg->max_reg_val /
+			(con->max_uV - con->min_uV);
+
+	regs = (__raw_readl(sreg->base_addr) & ~BM_POWER_LEVEL_TRG);
+	pr_debug("%s: calculated val %d\n", __func__, val);
+	__raw_writel(val | regs, sreg->base_addr);
+	for (i = 20; i; i--) {
+		if (__raw_readl(power_sts) & BM_POWER_STS_DC_OK)
+			break;
+		udelay(1);
+	}
+
+	if (i)
+		goto out;
+
+	__raw_writel(val | regs, sreg->base_addr);
+	for (i = 40000; i; i--) {
+		if (__raw_readl(power_sts) & BM_POWER_STS_DC_OK)
+			break;
+		udelay(1);
+	}
+
+	if (i)
+		goto out;
+
+	for (i = 40000; i; i--) {
+		if (__raw_readl(power_sts) & BM_POWER_STS_DC_OK)
+			break;
+		udelay(1);
+	}
+
+out:
+	return !i;
+}
+
+
+static int mxs_get_voltage(struct regulator_dev *reg)
+{
+	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
+	struct regulation_constraints *con = &sreg->initdata->constraints;
+	int uv;
+	u32 val = __raw_readl(sreg->base_addr) & BM_POWER_LEVEL_TRG;
+
+	if (val > sreg->max_reg_val)
+		val = sreg->max_reg_val;
+
+	uv = con->min_uV + val *
+		(con->max_uV - con->min_uV) / sreg->max_reg_val;
+
+	return uv;
+}
+
+static int main_add_current(struct mxs_regulator *sreg,
+			    int uA)
+{
+	pr_debug("%s: enter reg %s, uA=%d\n", __func__, sreg->name, uA);
+
+	if (uA > 0 && (sreg->cur_uA + uA > sreg->max_uA))
+		return -EINVAL;
+
+	sreg->cur_uA += uA;
+
+	return 0;
+}
+
+static int mxs_set_current_limit(struct regulator_dev *reg, int min_uA,
+				 int max_uA)
+{
+	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
+	struct mxs_regulator *parent = sreg->parent;
+	int ret = 0;
+	unsigned long flags;
+
+	pr_debug("%s: enter reg %s, uA=%d\n",
+		 __func__, sreg->name, max_uA);
+
+	if (parent) {
+		spin_lock_irqsave(&parent->lock, flags);
+		ret = main_add_current(parent, max_uA - sreg->cur_uA);
+		spin_unlock_irqrestore(&parent->lock, flags);
+	}
+
+	if ((!ret) || (!parent))
+		goto out;
+
+	if (sreg->mode == REGULATOR_MODE_FAST)
+		return ret;
+
+	while (ret) {
+		wait_event(parent->wait_q ,
+			   (max_uA - sreg->cur_uA <
+			    parent->max_uA -
+			    parent->cur_uA));
+		spin_lock_irqsave(&parent->lock, flags);
+		ret = main_add_current(parent, max_uA - sreg->cur_uA);
+		spin_unlock_irqrestore(&parent->lock, flags);
+	}
+out:
+	if (parent && (max_uA - sreg->cur_uA < 0))
+		wake_up_all(&parent->wait_q);
+	sreg->cur_uA = max_uA;
+	return 0;
+}
+
+static int mxs_get_current_limit(struct regulator_dev *reg)
+{
+	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
+
+	return sreg->cur_uA;
+}
+
+static int mxs_set_mode(struct regulator_dev *reg, unsigned int mode)
+{
+	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
+	int ret = 0;
+	u32 val;
+
+	switch (mode) {
+	case REGULATOR_MODE_FAST:
+		val = __raw_readl(sreg->base_addr);
+		__raw_writel(val | BM_POWER_REG_MODE, sreg->base_addr);
+		break;
+
+	case REGULATOR_MODE_NORMAL:
+		val = __raw_readl(sreg->base_addr);
+		__raw_writel(val & ~BM_POWER_REG_MODE, sreg->base_addr);
+		break;
+
+	default:
+		ret = -EINVAL;
+		break;
+	}
+	return ret;
+}
+
+static unsigned int mxs_get_mode(struct regulator_dev *reg)
+{
+	struct mxs_regulator *sreg = rdev_get_drvdata(reg);
+	u32 val = __raw_readl(sreg->base_addr) & BM_POWER_REG_MODE;
+
+	return val ? REGULATOR_MODE_FAST : REGULATOR_MODE_NORMAL;
+}
+
+static struct regulator_ops mxs_rops = {
+	.set_voltage	= mxs_set_voltage,
+	.get_voltage	= mxs_get_voltage,
+	.set_current_limit	= mxs_set_current_limit,
+	.get_current_limit	= mxs_get_current_limit,
+	.set_mode	= mxs_set_mode,
+	.get_mode	= mxs_get_mode,
+};
+
+static int reg_callback(struct notifier_block *self,
+			unsigned long event, void *data)
+{
+	unsigned long flags;
+	struct mxs_regulator *sreg =
+		container_of(self, struct mxs_regulator , nb);
+
+	switch (event) {
+	case MXS_REG5V_IS_USB:
+		spin_lock_irqsave(&sreg->lock, flags);
+		sreg->max_uA = 500000;
+		spin_unlock_irqrestore(&sreg->lock, flags);
+		break;
+	case MXS_REG5V_NOT_USB:
+		spin_lock_irqsave(&sreg->lock, flags);
+		sreg->max_uA = 0x7fffffff;
+		spin_unlock_irqrestore(&sreg->lock, flags);
+		break;
+	}
+
+	return 0;
+}
+
+static int mxs_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *parent;
+	struct regulator_desc *rdesc;
+	struct regulator_dev *rdev;
+	struct mxs_regulator *sreg;
+	struct regulator_init_data *initdata;
+	struct regulation_constraints *con;
+	struct regulator_config config = { };
+	void __iomem *base_addr = NULL;
+	void __iomem *power_addr = NULL;
+	u64 regaddr64 = 0;
+	const u32 *regaddr_p;
+	u32 val = 0;
+	int ret;
+
+	if (!np) {
+		dev_err(dev, "missing device tree\n");
+		return -EINVAL;
+	}
+
+	/* get device base address */
+	base_addr = of_iomap(np, 0);
+	if (!base_addr)
+		return -ENXIO;
+
+	parent = of_get_parent(np);
+	if (!parent)
+		return -ENXIO;
+
+	power_addr = of_iomap(parent, 0);
+	if (!power_addr)
+		return -ENXIO;
+
+	regaddr_p = of_get_address(np, 0, NULL, NULL);
+	if (regaddr_p)
+		regaddr64 = of_translate_address(np, regaddr_p);
+
+	if (!regaddr64) {
+		dev_err(dev, "no or invalid reg property set\n");
+		return -EINVAL;
+	}
+
+	initdata = of_get_regulator_init_data(dev, np);
+	if (!initdata)
+		return -EINVAL;
+
+	ret = of_property_read_u32(np, "mxs-max-reg-val",
+				   &val);
+	if (!val) {
+		dev_err(dev, "no or invalid mxs-max-reg-val property set\n");
+		return ret;
+	}
+
+	dev_info(dev, "regulator found\n");
+
+	sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
+	if (!sreg)
+		return -ENOMEM;
+	sreg->initdata = initdata;
+	sreg->name = of_get_property(np, "regulator-name", NULL);
+	sreg->cur_uA = 0;
+	sreg->cur_uV = 0;
+	sreg->base_addr = base_addr;
+	sreg->power_addr = power_addr;
+	init_waitqueue_head(&sreg->wait_q);
+	spin_lock_init(&sreg->lock);
+	sreg->max_reg_val = val;
+
+	rdesc = &sreg->rdesc;
+	rdesc->name = sreg->name;
+	rdesc->owner = THIS_MODULE;
+	rdesc->ops = &mxs_rops;
+
+	if (strcmp(rdesc->name, "overall_current") == 0)
+		rdesc->type = REGULATOR_CURRENT;
+	else
+		rdesc->type = REGULATOR_VOLTAGE;
+
+	con = &initdata->constraints;
+	rdesc->n_voltages = sreg->max_reg_val;
+	rdesc->min_uV = con->min_uV;
+	rdesc->uV_step = (con->max_uV - con->min_uV) / sreg->max_reg_val;
+	rdesc->linear_min_sel = 0;
+	rdesc->vsel_reg = regaddr64;
+	rdesc->vsel_mask = BM_POWER_LEVEL_TRG;
+
+	config.dev = &pdev->dev;
+	config.init_data = initdata;
+	config.driver_data = sreg;
+	config.of_node = np;
+
+	pr_debug("probing regulator %s %s %d\n",
+			sreg->name,
+			rdesc->name,
+			pdev->id);
+
+	/* register regulator */
+	rdev = devm_regulator_register(dev, rdesc, &config);
+
+	if (IS_ERR(rdev)) {
+		dev_err(&pdev->dev, "failed to register %s\n",
+			rdesc->name);
+		return PTR_ERR(rdev);
+	}
+
+	if (sreg->max_uA) {
+		struct regulator *regu;
+
+		regu = regulator_get(NULL, sreg->name);
+		sreg->nb.notifier_call = reg_callback;
+		regulator_register_notifier(regu, &sreg->nb);
+	}
+
+	platform_set_drvdata(pdev, rdev);
+
+	of_property_read_u32(np, "mxs-default-microvolt",
+				   &val);
+
+	if (val)
+		mxs_set_voltage(rdev, val, val, NULL);
+
+	return 0;
+}
+
+static struct of_device_id of_mxs_regulator_match_tbl[] = {
+	{ .compatible = "fsl,mxs-regulator", },
+	{ /* end */ }
+};
+
+static struct platform_driver mxs_regulator_driver = {
+	.driver = {
+		.name	= "mxs-regulator",
+		.owner  = THIS_MODULE,
+		.of_match_table = of_mxs_regulator_match_tbl,
+	},
+	.probe	= mxs_regulator_probe,
+};
+
+static int __init mxs_regulator_init(void)
+{
+	return platform_driver_register(&mxs_regulator_driver);
+}
+postcore_initcall(mxs_regulator_init);
+
+static void __exit mxs_regulator_exit(void)
+{
+	platform_driver_unregister(&mxs_regulator_driver);
+}
+module_exit(mxs_regulator_exit);
+
+MODULE_AUTHOR("Embedded Alley Solutions <source@embeddedalley.com>");
+MODULE_AUTHOR("Stefan Wahren <stefan.wahren@i2se.com>");
+MODULE_DESCRIPTION("Freescale STMP378X voltage regulators");
+MODULE_LICENSE("GPL v2");
-- 
1.8.1.4

Re: [PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator driver

[Mark Rutland]

[...]

> +       regs = (__raw_readl(sreg->base_addr) & ~BM_POWER_LEVEL_TRG);

I suspect you should be using the *_relaxed accessors rather than the
__raw_* accessors.

[...]

> +static int mxs_regulator_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       struct device_node *parent;
> +       struct regulator_desc *rdesc;
> +       struct regulator_dev *rdev;
> +       struct mxs_regulator *sreg;
> +       struct regulator_init_data *initdata;
> +       struct regulation_constraints *con;
> +       struct regulator_config config = { };
> +       void __iomem *base_addr = NULL;
> +       void __iomem *power_addr = NULL;
> +       u64 regaddr64 = 0;
> +       const u32 *regaddr_p;
> +       u32 val = 0;
> +       int ret;
> +
> +       if (!np) {
> +               dev_err(dev, "missing device tree\n");
> +               return -EINVAL;
> +       }
> +
> +       /* get device base address */
> +       base_addr = of_iomap(np, 0);
> +       if (!base_addr)
> +               return -ENXIO;
> +
> +       parent = of_get_parent(np);
> +       if (!parent)
> +               return -ENXIO;

Leak of the (successfully mapped) base_addr.

> +
> +       power_addr = of_iomap(parent, 0);
> +       if (!power_addr)
> +               return -ENXIO;

Leak of base_addr and dangling refcount on parent. These apply to all
subsequent returns.

> +
> +       regaddr_p = of_get_address(np, 0, NULL, NULL);

of_get_address returns a __be32*, not a u32*, so sparse will be very
unhappy here...

> +       if (regaddr_p)
> +               regaddr64 = of_translate_address(np, regaddr_p);

...and as of_translate_address returns a u64 you'll need a separate
variable for the input and output.

> +
> +       if (!regaddr64) {
> +               dev_err(dev, "no or invalid reg property set\n");
> +               return -EINVAL;
> +       }
> +
> +       initdata = of_get_regulator_init_data(dev, np);
> +       if (!initdata)
> +               return -EINVAL;
> +
> +       ret = of_property_read_u32(np, "mxs-max-reg-val",
> +                                  &val);
> +       if (!val) {
> +               dev_err(dev, "no or invalid mxs-max-reg-val property set\n");
> +               return ret;
> +       }
> +
> +       dev_info(dev, "regulator found\n");
> +
> +       sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
> +       if (!sreg)
> +               return -ENOMEM;
> +       sreg->initdata = initdata;
> +       sreg->name = of_get_property(np, "regulator-name", NULL);

I'm not keen on using of_get_property here. We have no idea if
regulator-name is even a string (it should be, but we have no
guarantee).

> +       sreg->cur_uA = 0;
> +       sreg->cur_uV = 0;
> +       sreg->base_addr = base_addr;
> +       sreg->power_addr = power_addr;
> +       init_waitqueue_head(&sreg->wait_q);
> +       spin_lock_init(&sreg->lock);
> +       sreg->max_reg_val = val;
> +
> +       rdesc = &sreg->rdesc;
> +       rdesc->name = sreg->name;
> +       rdesc->owner = THIS_MODULE;
> +       rdesc->ops = &mxs_rops;
> +
> +       if (strcmp(rdesc->name, "overall_current") == 0)
> +               rdesc->type = REGULATOR_CURRENT;
> +       else
> +               rdesc->type = REGULATOR_VOLTAGE;

Wouldn't it make more sense to explicitly match the names you expect?

> +       con = &initdata->constraints;
> +       rdesc->n_voltages = sreg->max_reg_val;
> +       rdesc->min_uV = con->min_uV;
> +       rdesc->uV_step = (con->max_uV - con->min_uV) / sreg->max_reg_val;
> +       rdesc->linear_min_sel = 0;
> +       rdesc->vsel_reg = regaddr64;
> +       rdesc->vsel_mask = BM_POWER_LEVEL_TRG;
> +
> +       config.dev = &pdev->dev;
> +       config.init_data = initdata;
> +       config.driver_data = sreg;
> +       config.of_node = np;
> +
> +       pr_debug("probing regulator %s %s %d\n",
> +                       sreg->name,
> +                       rdesc->name,
> +                       pdev->id);

Aren't those two names always the same per the code above?

> +
> +       /* register regulator */
> +       rdev = devm_regulator_register(dev, rdesc, &config);
> +
> +       if (IS_ERR(rdev)) {
> +               dev_err(&pdev->dev, "failed to register %s\n",
> +                       rdesc->name);
> +               return PTR_ERR(rdev);
> +       }
> +
> +       if (sreg->max_uA) {
> +               struct regulator *regu;
> +
> +               regu = regulator_get(NULL, sreg->name);
> +               sreg->nb.notifier_call = reg_callback;
> +               regulator_register_notifier(regu, &sreg->nb);
> +       }
> +
> +       platform_set_drvdata(pdev, rdev);
> +
> +       of_property_read_u32(np, "mxs-default-microvolt",
> +                                  &val);
> +
> +       if (val)
> +               mxs_set_voltage(rdev, val, val, NULL);

As I mentioned in my comments on the binding, I'd like to know why this
is necessary and if it is why it shouldn't be a standardised property.

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

Re: [PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator driver

[Stefan Wahren]

Hi,

Am 09.09.2014 20:22, schrieb Mark Rutland:
> [...]
>
>> +       regs = (__raw_readl(sreg->base_addr) & ~BM_POWER_LEVEL_TRG);
>
> I suspect you should be using the *_relaxed accessors rather than the
> __raw_* accessors.
>
> [...]
>
>> +static int mxs_regulator_probe(struct platform_device *pdev)
>> +{
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *np = dev->of_node;
>> +       struct device_node *parent;
>> +       struct regulator_desc *rdesc;
>> +       struct regulator_dev *rdev;
>> +       struct mxs_regulator *sreg;
>> +       struct regulator_init_data *initdata;
>> +       struct regulation_constraints *con;
>> +       struct regulator_config config = { };
>> +       void __iomem *base_addr = NULL;
>> +       void __iomem *power_addr = NULL;
>> +       u64 regaddr64 = 0;
>> +       const u32 *regaddr_p;
>> +       u32 val = 0;
>> +       int ret;
>> +
>> +       if (!np) {
>> +               dev_err(dev, "missing device tree\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       /* get device base address */
>> +       base_addr = of_iomap(np, 0);
>> +       if (!base_addr)
>> +               return -ENXIO;
>> +
>> +       parent = of_get_parent(np);
>> +       if (!parent)
>> +               return -ENXIO;
>
> Leak of the (successfully mapped) base_addr.
>
>> +
>> +       power_addr = of_iomap(parent, 0);
>> +       if (!power_addr)
>> +               return -ENXIO;
>
> Leak of base_addr and dangling refcount on parent. These apply to all
> subsequent returns.
>
>> +
>> +       regaddr_p = of_get_address(np, 0, NULL, NULL);
>
> of_get_address returns a __be32*, not a u32*, so sparse will be very
> unhappy here...
>
>> +       if (regaddr_p)
>> +               regaddr64 = of_translate_address(np, regaddr_p);
>
> ...and as of_translate_address returns a u64 you'll need a separate
> variable for the input and output.
>
>> +
>> +       if (!regaddr64) {
>> +               dev_err(dev, "no or invalid reg property set\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       initdata = of_get_regulator_init_data(dev, np);
>> +       if (!initdata)
>> +               return -EINVAL;
>> +
>> +       ret = of_property_read_u32(np, "mxs-max-reg-val",
>> +                                  &val);
>> +       if (!val) {
>> +               dev_err(dev, "no or invalid mxs-max-reg-val property set\n");
>> +               return ret;
>> +       }
>> +
>> +       dev_info(dev, "regulator found\n");
>> +
>> +       sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
>> +       if (!sreg)
>> +               return -ENOMEM;
>> +       sreg->initdata = initdata;
>> +       sreg->name = of_get_property(np, "regulator-name", NULL);
>
> I'm not keen on using of_get_property here. We have no idea if
> regulator-name is even a string (it should be, but we have no
> guarantee).

Better using of_property_read_string?

>
>> +       sreg->cur_uA = 0;
>> +       sreg->cur_uV = 0;
>> +       sreg->base_addr = base_addr;
>> +       sreg->power_addr = power_addr;
>> +       init_waitqueue_head(&sreg->wait_q);
>> +       spin_lock_init(&sreg->lock);
>> +       sreg->max_reg_val = val;
>> +
>> +       rdesc = &sreg->rdesc;
>> +       rdesc->name = sreg->name;
>> +       rdesc->owner = THIS_MODULE;
>> +       rdesc->ops = &mxs_rops;
>> +
>> +       if (strcmp(rdesc->name, "overall_current") == 0)
>> +               rdesc->type = REGULATOR_CURRENT;
>> +       else
>> +               rdesc->type = REGULATOR_VOLTAGE;
>
> Wouldn't it make more sense to explicitly match the names you expect?
>

Okay, i make "regulator-name" a required property and use a white list 
of all possible regulators.

>> +       con = &initdata->constraints;
>> +       rdesc->n_voltages = sreg->max_reg_val;
>> +       rdesc->min_uV = con->min_uV;
>> +       rdesc->uV_step = (con->max_uV - con->min_uV) / sreg->max_reg_val;
>> +       rdesc->linear_min_sel = 0;
>> +       rdesc->vsel_reg = regaddr64;
>> +       rdesc->vsel_mask = BM_POWER_LEVEL_TRG;
>> +
>> +       config.dev = &pdev->dev;
>> +       config.init_data = initdata;
>> +       config.driver_data = sreg;
>> +       config.of_node = np;
>> +
>> +       pr_debug("probing regulator %s %s %d\n",
>> +                       sreg->name,
>> +                       rdesc->name,
>> +                       pdev->id);
>
> Aren't those two names always the same per the code above?
>

Sure, i will fix that.

>> +
>> +       /* register regulator */
>> +       rdev = devm_regulator_register(dev, rdesc, &config);
>> +
>> +       if (IS_ERR(rdev)) {
>> +               dev_err(&pdev->dev, "failed to register %s\n",
>> +                       rdesc->name);
>> +               return PTR_ERR(rdev);
>> +       }
>> +
>> +       if (sreg->max_uA) {
>> +               struct regulator *regu;
>> +
>> +               regu = regulator_get(NULL, sreg->name);
>> +               sreg->nb.notifier_call = reg_callback;
>> +               regulator_register_notifier(regu, &sreg->nb);
>> +       }
>> +
>> +       platform_set_drvdata(pdev, rdev);
>> +
>> +       of_property_read_u32(np, "mxs-default-microvolt",
>> +                                  &val);
>> +
>> +       if (val)
>> +               mxs_set_voltage(rdev, val, val, NULL);
>
> As I mentioned in my comments on the binding, I'd like to know why this
> is necessary and if it is why it shouldn't be a standardised property.

 From my understanding the standardised properties only defines a range, 
but no default state of the regulators. If the initialization from the 
bootloader or a hardcoded initialization in the driver is okay then the 
property is not necessary.

> Mark.
>

Thanks for your feedback.

Stefan

Re: [PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator driver

[Mark Rutland]

On Tue, Sep 09, 2014 at 08:17:17PM +0100, Stefan Wahren wrote:
> Hi,

[...]

> >> +       sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
> >> +       if (!sreg)
> >> +               return -ENOMEM;
> >> +       sreg->initdata = initdata;
> >> +       sreg->name = of_get_property(np, "regulator-name", NULL);
> >
> > I'm not keen on using of_get_property here. We have no idea if
> > regulator-name is even a string (it should be, but we have no
> > guarantee).
> 
> Better using of_property_read_string?

Yes. That will check the value is NUL-terminated, at least.

> >> +       sreg->cur_uA = 0;
> >> +       sreg->cur_uV = 0;
> >> +       sreg->base_addr = base_addr;
> >> +       sreg->power_addr = power_addr;
> >> +       init_waitqueue_head(&sreg->wait_q);
> >> +       spin_lock_init(&sreg->lock);
> >> +       sreg->max_reg_val = val;
> >> +
> >> +       rdesc = &sreg->rdesc;
> >> +       rdesc->name = sreg->name;
> >> +       rdesc->owner = THIS_MODULE;
> >> +       rdesc->ops = &mxs_rops;
> >> +
> >> +       if (strcmp(rdesc->name, "overall_current") == 0)
> >> +               rdesc->type = REGULATOR_CURRENT;
> >> +       else
> >> +               rdesc->type = REGULATOR_VOLTAGE;
> >
> > Wouldn't it make more sense to explicitly match the names you expect?
> >
> 
> Okay, i make "regulator-name" a required property and use a white list 
> of all possible regulators.

I'm not sure if regulator-name is the right way to go. I believe the
thing to do is match on the node name of the subnodes (as you were doing
here).

The only thing I didn't like was assuming the nodes were voltage
regulators if they weren't called "overall_current". I'd prefer explicit
matching, or something more general.

[...]

> >> +       of_property_read_u32(np, "mxs-default-microvolt",
> >> +                                  &val);
> >> +
> >> +       if (val)
> >> +               mxs_set_voltage(rdev, val, val, NULL);
> >
> > As I mentioned in my comments on the binding, I'd like to know why this
> > is necessary and if it is why it shouldn't be a standardised property.
> 
>  From my understanding the standardised properties only defines a range, 
> but no default state of the regulators. If the initialization from the 
> bootloader or a hardcoded initialization in the driver is okay then the 
> property is not necessary.

Sure. My questions was why it is necessary to preconfigure the
regulators at all rather than why it is necessary to do so in this
manner.

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

Re: [PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1020 bytes --]

On Wed, Sep 10, 2014 at 03:18:53PM +0100, Mark Rutland wrote:
> On Tue, Sep 09, 2014 at 08:17:17PM +0100, Stefan Wahren wrote:

Ugh, this looks like it might be a regulator driver but since the
subject line was "ARM: " I deleted it unread - if your changelog looks
different to all the other changelogs in the subsystem it probably needs
changing.

> > >> +       sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
> > >> +       if (!sreg)
> > >> +               return -ENOMEM;
> > >> +       sreg->initdata = initdata;
> > >> +       sreg->name = of_get_property(np, "regulator-name", NULL);

> > > I'm not keen on using of_get_property here. We have no idea if
> > > regulator-name is even a string (it should be, but we have no
> > > guarantee).

> > Better using of_property_read_string?

> Yes. That will check the value is NUL-terminated, at least.

Or just remove the property entirely...  without having seen the
bindings if we're specifying the name of the device via the device tree
something seems wrong.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator driver

[Stefan Wahren]

Hi Mark,

Am 10.09.2014 17:13, schrieb Mark Brown:
> On Wed, Sep 10, 2014 at 03:18:53PM +0100, Mark Rutland wrote:
>> On Tue, Sep 09, 2014 at 08:17:17PM +0100, Stefan Wahren wrote:
>
> Ugh, this looks like it might be a regulator driver but since the
> subject line was "ARM: " I deleted it unread - if your changelog looks
> different to all the other changelogs in the subsystem it probably needs
> changing.

sorry about the confusion, i will remove ARM in the next version.

Changelog? I didn't send a changelog because it was my first version.

Should i resend this version only to you?

>
>>>>> +       sreg = devm_kzalloc(dev, sizeof(*sreg), GFP_KERNEL);
>>>>> +       if (!sreg)
>>>>> +               return -ENOMEM;
>>>>> +       sreg->initdata = initdata;
>>>>> +       sreg->name = of_get_property(np, "regulator-name", NULL);
>
>>>> I'm not keen on using of_get_property here. We have no idea if
>>>> regulator-name is even a string (it should be, but we have no
>>>> guarantee).
>
>>> Better using of_property_read_string?
>
>> Yes. That will check the value is NUL-terminated, at least.
>
> Or just remove the property entirely...  without having seen the
> bindings if we're specifying the name of the device via the device tree
> something seems wrong.
>

BR Stefan

Re: [PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator driver

[Fabio Estevam]

Hi Stefan,

On Wed, Sep 10, 2014 at 2:32 PM, Stefan Wahren <info@lategoodbye.de> wrote:
> Hi Mark,
>
> Am 10.09.2014 17:13, schrieb Mark Brown:
>>
>> On Wed, Sep 10, 2014 at 03:18:53PM +0100, Mark Rutland wrote:
>>>
>>> On Tue, Sep 09, 2014 at 08:17:17PM +0100, Stefan Wahren wrote:
>>
>>
>> Ugh, this looks like it might be a regulator driver but since the
>> subject line was "ARM: " I deleted it unread - if your changelog looks
>> different to all the other changelogs in the subsystem it probably needs
>> changing.
>
>
> sorry about the confusion, i will remove ARM in the next version.
>
> Changelog? I didn't send a changelog because it was my first version.
>
> Should i resend this version only to you?

In the cover letter of this RFC series you mentioned that this has not
been tested on real hardware.

What about sending a new version of this series (with the RFC prefix
removed and with Mark Rutland's suggestion implemented) tested on a
mx28 board and also with the Subject of the regulator patch changed to
'regulator: add support for mxs regulator" with Mark Brown on Cc?

Re: [PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator driver

[Stefan Wahren]

Hi Fabio,

Am 10.09.2014 20:54, schrieb Fabio Estevam:
> Hi Stefan,
>
> On Wed, Sep 10, 2014 at 2:32 PM, Stefan Wahren <info-saaNCTdWVBT7BZbvpMY5sg@public.gmane.org> wrote:
>> Hi Mark,
>>
>> Am 10.09.2014 17:13, schrieb Mark Brown:
>>>
>>> On Wed, Sep 10, 2014 at 03:18:53PM +0100, Mark Rutland wrote:
>>>>
>>>> On Tue, Sep 09, 2014 at 08:17:17PM +0100, Stefan Wahren wrote:
>>>
>>>
>>> Ugh, this looks like it might be a regulator driver but since the
>>> subject line was "ARM: " I deleted it unread - if your changelog looks
>>> different to all the other changelogs in the subsystem it probably needs
>>> changing.
>>
>>
>> sorry about the confusion, i will remove ARM in the next version.
>>
>> Changelog? I didn't send a changelog because it was my first version.
>>
>> Should i resend this version only to you?
>
> In the cover letter of this RFC series you mentioned that this has not
> been tested on real hardware.
>
> What about sending a new version of this series (with the RFC prefix
> removed and with Mark Rutland's suggestion implemented) tested on a
> mx28 board and also with the Subject of the regulator patch changed to
> 'regulator: add support for mxs regulator" with Mark Brown on Cc?
>

that's the same idea i had about the first real version of my patch. 
Unfortunately i can do the porting only in my spare time. So i try to 
avoid with the RFC series the situation, that i spend many days in 
development and testing, but after it the regulator guys says it went in 
the complete wrong direction. The advice about the anatop regulator was 
helpful, but there is still some unsureness.

I will try to get a Duckbill, so i can do testing at home.

Thanks

Stefan
--
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

Re: [PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator driver

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 747 bytes --]

On Wed, Sep 10, 2014 at 07:32:36PM +0200, Stefan Wahren wrote:
> Am 10.09.2014 17:13, schrieb Mark Brown:

> >Ugh, this looks like it might be a regulator driver but since the
> >subject line was "ARM: " I deleted it unread - if your changelog looks
> >different to all the other changelogs in the subsystem it probably needs
> >changing.

> sorry about the confusion, i will remove ARM in the next version.

> Changelog? I didn't send a changelog because it was my first version.

The changelog is the text describing the change you are making, not
something describing differences between revisions of the patch set.

> Should i resend this version only to you?

It sounds like this needs a new version anyway so may as well just wait
for that.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator driver

[Stefan Wahren]

Hi,

Am 10.09.2014 16:18, schrieb Mark Rutland:
>[...]
>
>>>> +       of_property_read_u32(np, "mxs-default-microvolt",
>>>> +                                  &val);
>>>> +
>>>> +       if (val)
>>>> +               mxs_set_voltage(rdev, val, val, NULL);
>>>
>>> As I mentioned in my comments on the binding, I'd like to know why this
>>> is necessary and if it is why it shouldn't be a standardised property.
>>
>>   From my understanding the standardised properties only defines a range,
>> but no default state of the regulators. If the initialization from the
>> bootloader or a hardcoded initialization in the driver is okay then the
>> property is not necessary.
>
> Sure. My questions was why it is necessary to preconfigure the
> regulators at all rather than why it is necessary to do so in this
> manner.
>
> Mark.
>

sorry i don't have a clue. In the original code there isn't a comment 
about the reason. Currently there is no init of the vddio regulator by 
the kernel and everything works fine.

@Fabio: Do you have any doubts?

Stefan
--
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

Re: [PATCH RFC 2/3] ARM: regulator: add Freescale MXS regulator driver

[Fabio Estevam]

On Wed, Sep 10, 2014 at 2:24 PM, Stefan Wahren <wahrenst-hi6Y0CQ0nG0@public.gmane.org> wrote:

> sorry i don't have a clue. In the original code there isn't a comment about
> the reason. Currently there is no init of the vddio regulator by the kernel
> and everything works fine.
>
> @Fabio: Do you have any doubts?

I would say, just use and report whatever value comes from the
bootloader. No need to force a particular regulator  output.
--
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

[PATCH RFC 3/3] DT: ARM: mxs: enable regulator support for i.MX28

[Stefan Wahren]

This patch enables the regulator support for i.MX28.

Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
---
 arch/arm/boot/dts/imx28.dtsi | 50 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 49 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx28.dtsi b/arch/arm/boot/dts/imx28.dtsi
index a95cc53..10c7cdf 100644
--- a/arch/arm/boot/dts/imx28.dtsi
+++ b/arch/arm/boot/dts/imx28.dtsi
@@ -979,8 +979,56 @@
 			};
 
 			power: power@80044000 {
+				compatible = "fsl,imx28-power";
+				#address-cells = <1>;
+				#size-cells = <0>;
 				reg = <0x80044000 0x2000>;
-				status = "disabled";
+
+				/*
+				reg_curr: regulator-overall_current@80044010 {
+					reg = <0x80044010>;
+					compatible = "fsl,mxs-regulator";
+					regulator-name = "overall_current";
+					regulator-min-microvolt = <>;
+					regulator-max-microvolt = <>;
+					regulator-min-microamp = <>;
+					regulator-max-microamp = <>;
+					regulator-always-on;
+					regulator-boot-on;
+				};
+				*/
+
+				reg_vddd: regulator-vddd@80044040 {
+					reg = <0x80044040>;
+					compatible = "fsl,mxs-regulator";
+					regulator-name = "vddd";
+					regulator-min-microvolt = <800000>;
+					regulator-max-microvolt = <1575000>;
+					regulator-always-on;
+					mxs-max-reg-val = <0x1f>;
+				};
+
+				reg_vdda: regulator-vdda@80044050 {
+					reg = <0x80044050>;
+					compatible = "fsl,mxs-regulator";
+					regulator-name = "vdda";
+					regulator-min-microvolt = <1500000>;
+					regulator-max-microvolt = <2275000>;
+					regulator-always-on;
+					mxs-max-reg-val = <0x1f>;
+				};
+
+				reg_vddio: regulator-vddio@80044060 {
+					reg = <0x80044060>;
+					compatible = "fsl,mxs-regulator";
+					regulator-name = "vddio";
+					regulator-min-microvolt = <2800000>;
+					regulator-max-microvolt = <3600000>;
+					regulator-microvolt-offset = <80000>;
+					regulator-always-on;
+					mxs-max-reg-val = <0x10>;
+					mxs-default-microvolt = <3300000>;
+				};
 			};
 
 			saif1: saif@80046000 {
-- 
1.8.1.4