[PATCH] Regulator: Add Anatop regulator driver

[PATCH] Regulator: Add Anatop regulator driver

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Hi all,

This is the Anatop regulator used by Freescale i.MX6 SoC.
Please take a look and give me some comments.

Many Thanks,
Paul

Ying-Chun Liu (PaulLiu) (1):
  Regulator: Add Anatop regulator driver

 drivers/regulator/Kconfig                  |    7 +
 drivers/regulator/Makefile                 |    1 +
 drivers/regulator/anatop-regulator.c       |  236 ++++++++++++++++++++++++++++
 include/linux/regulator/anatop-regulator.h |   82 ++++++++++
 4 files changed, 326 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regulator/anatop-regulator.c
 create mode 100644 include/linux/regulator/anatop-regulator.h

-- 
1.7.7.3

[PATCH] Regulator: Add Anatop regulator driver

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Anatop regulator driver is used by i.MX6 SoC. This patch adds the
Anatop regulator driver.

Signed-off-by: Nancy Chen <Nancy.Chen@freescale.com>
Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
---
 drivers/regulator/Kconfig                  |    7 +
 drivers/regulator/Makefile                 |    1 +
 drivers/regulator/anatop-regulator.c       |  236 ++++++++++++++++++++++++++++
 include/linux/regulator/anatop-regulator.h |   82 ++++++++++
 4 files changed, 326 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regulator/anatop-regulator.c
 create mode 100644 include/linux/regulator/anatop-regulator.h

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 9713b1b..790eca5 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -327,5 +327,12 @@ config REGULATOR_AAT2870
 	  If you have a AnalogicTech AAT2870 say Y to enable the
 	  regulator driver.
 
+config REGULATOR_ANATOP
+	tristate "ANATOP LDO regulators"
+	depends on ARCH_MX6
+	default y if ARCH_MX6
+	help
+	  Say y here to support ANATOP LDOs regulators.
+
 endif
 
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 93a6318..990c332 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -46,5 +46,6 @@ obj-$(CONFIG_REGULATOR_AB8500)	+= ab8500.o
 obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
 obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
 obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
+obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
 
 ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
new file mode 100644
index 0000000..61ad1c2
--- /dev/null
+++ b/drivers/regulator/anatop-regulator.c
@@ -0,0 +1,236 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * 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.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/anatop-regulator.h>
+
+static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
+				  int max_uV, unsigned *selector)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+
+	if (anatop_reg->rdata->set_voltage)
+		return anatop_reg->rdata->set_voltage(anatop_reg, max_uV);
+	else
+		return -ENOTSUPP;
+}
+
+static int anatop_get_voltage(struct regulator_dev *reg)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+
+	if (anatop_reg->rdata->get_voltage)
+		return anatop_reg->rdata->get_voltage(anatop_reg);
+	else
+		return -ENOTSUPP;
+}
+
+static int anatop_enable(struct regulator_dev *reg)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+
+	return anatop_reg->rdata->enable(anatop_reg);
+}
+
+static int anatop_disable(struct regulator_dev *reg)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+
+	return anatop_reg->rdata->disable(anatop_reg);
+}
+
+static int anatop_is_enabled(struct regulator_dev *reg)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+
+	return anatop_reg->rdata->is_enabled(anatop_reg);
+}
+
+static struct regulator_ops anatop_rops = {
+	.set_voltage	= anatop_set_voltage,
+	.get_voltage	= anatop_get_voltage,
+	.enable		= anatop_enable,
+	.disable	= anatop_disable,
+	.is_enabled	= anatop_is_enabled,
+};
+
+static struct regulator_desc anatop_reg_desc[] = {
+	{
+		.name = "vddpu",
+		.id = ANATOP_VDDPU,
+		.ops = &anatop_rops,
+		.irq = 0,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vddcore",
+		.id = ANATOP_VDDCORE,
+		.ops = &anatop_rops,
+		.irq = 0,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vddsoc",
+		.id = ANATOP_VDDSOC,
+		.ops = &anatop_rops,
+		.irq = 0,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vdd2p5",
+		.id = ANATOP_VDD2P5,
+		.ops = &anatop_rops,
+		.irq = 0,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vdd1p1",
+		.id = ANATOP_VDD1P1,
+		.ops = &anatop_rops,
+		.irq = 0,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vdd3p0",
+		.id = ANATOP_VDD3P0,
+		.ops = &anatop_rops,
+		.irq = 0,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+};
+
+int anatop_regulator_probe(struct platform_device *pdev)
+{
+	struct regulator_desc *rdesc;
+	struct regulator_dev *rdev;
+	struct anatop_regulator *sreg;
+	struct regulator_init_data *initdata;
+
+	sreg = platform_get_drvdata(pdev);
+	initdata = pdev->dev.platform_data;
+	sreg->cur_current = 0;
+	sreg->next_current = 0;
+	sreg->cur_voltage = 0;
+
+	init_waitqueue_head(&sreg->wait_q);
+	spin_lock_init(&sreg->lock);
+
+	if (pdev->id > ANATOP_SUPPLY_NUM) {
+		rdesc = kzalloc(sizeof(struct regulator_desc), GFP_KERNEL);
+		memcpy(rdesc, &anatop_reg_desc[ANATOP_SUPPLY_NUM],
+			sizeof(struct regulator_desc));
+		rdesc->name = kstrdup(sreg->rdata->name, GFP_KERNEL);
+	} else
+		rdesc = &anatop_reg_desc[pdev->id];
+
+	pr_debug("probing regulator %s %s %d\n",
+			sreg->rdata->name,
+			rdesc->name,
+			pdev->id);
+
+	/* register regulator */
+	rdev = regulator_register(rdesc, &pdev->dev,
+				  initdata, sreg);
+
+	if (IS_ERR(rdev)) {
+		dev_err(&pdev->dev, "failed to register %s\n",
+			rdesc->name);
+		return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+
+
+int anatop_regulator_remove(struct platform_device *pdev)
+{
+	struct regulator_dev *rdev = platform_get_drvdata(pdev);
+
+	regulator_unregister(rdev);
+
+	return 0;
+
+}
+
+int anatop_register_regulator(
+		struct anatop_regulator *reg_data, int reg,
+			      struct regulator_init_data *initdata)
+{
+	struct platform_device *pdev;
+	int ret;
+
+	pdev = platform_device_alloc("anatop_reg", reg);
+	if (!pdev)
+		return -ENOMEM;
+
+	pdev->dev.platform_data = initdata;
+
+	platform_set_drvdata(pdev, reg_data);
+	ret = platform_device_add(pdev);
+
+	if (ret != 0) {
+		pr_debug("Failed to register regulator %d: %d\n",
+			reg, ret);
+		platform_device_del(pdev);
+	}
+	pr_debug("register regulator %s, %d: %d\n",
+			reg_data->rdata->name, reg, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(anatop_register_regulator);
+
+struct platform_driver anatop_reg = {
+	.driver = {
+		.name	= "anatop_reg",
+	},
+	.probe	= anatop_regulator_probe,
+	.remove	= anatop_regulator_remove,
+};
+
+int anatop_regulator_init(void)
+{
+	return platform_driver_register(&anatop_reg);
+}
+
+void anatop_regulator_exit(void)
+{
+	platform_driver_unregister(&anatop_reg);
+}
+
+postcore_initcall(anatop_regulator_init);
+module_exit(anatop_regulator_exit);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("ANATOP Regulator driver");
+MODULE_LICENSE("GPL");
+
diff --git a/include/linux/regulator/anatop-regulator.h b/include/linux/regulator/anatop-regulator.h
new file mode 100644
index 0000000..a1d98ed
--- /dev/null
+++ b/include/linux/regulator/anatop-regulator.h
@@ -0,0 +1,82 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * 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.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifndef __ANATOP_REGULATOR_H
+#define __ANATOP_REGULATOR_H
+#include <linux/regulator/driver.h>
+
+/* regulator supplies for Anatop */
+enum anatop_regulator_supplies {
+	ANATOP_VDDPU,
+	ANATOP_VDDCORE,
+	ANATOP_VDDSOC,
+	ANATOP_VDD2P5,
+	ANATOP_VDD1P1,
+	ANATOP_VDD3P0,
+	ANATOP_SUPPLY_NUM
+};
+
+struct anatop_regulator {
+	struct regulator_desc regulator;
+	struct anatop_regulator *parent;
+	struct anatop_regulator_data *rdata;
+	struct completion done;
+
+	spinlock_t         lock;
+	wait_queue_head_t  wait_q;
+	struct notifier_block nb;
+
+	int mode;
+	int cur_voltage;
+	int cur_current;
+	int next_current;
+};
+
+
+struct anatop_regulator_data {
+	char name[80];
+	char *parent_name;
+	int (*reg_register)(struct anatop_regulator *sreg);
+	int (*set_voltage)(struct anatop_regulator *sreg, int uv);
+	int (*get_voltage)(struct anatop_regulator *sreg);
+	int (*set_current)(struct anatop_regulator *sreg, int uA);
+	int (*get_current)(struct anatop_regulator *sreg);
+	int (*enable)(struct anatop_regulator *sreg);
+	int (*disable)(struct anatop_regulator *sreg);
+	int (*is_enabled)(struct anatop_regulator *sreg);
+	int (*set_mode)(struct anatop_regulator *sreg, int mode);
+	int (*get_mode)(struct anatop_regulator *sreg);
+	int (*get_optimum_mode)(struct anatop_regulator *sreg,
+			int input_uV, int output_uV, int load_uA);
+	u32 control_reg;
+	int vol_bit_shift;
+	int vol_bit_mask;
+	int min_bit_val;
+	int min_voltage;
+	int max_voltage;
+	int max_current;
+	struct regulation_constraints *cnstraints;
+};
+
+int anatop_register_regulator(
+		struct anatop_regulator *reg_data, int reg,
+		      struct regulator_init_data *initdata);
+
+#endif /* __ANATOP_REGULATOR_H */
-- 
1.7.7.3

Re: [PATCH] Regulator: Add Anatop regulator driver

[Mark Brown]

On Wed, Dec 07, 2011 at 09:53:18PM +0800, Ying-Chun Liu (PaulLiu) wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> 
> Anatop regulator driver is used by i.MX6 SoC. This patch adds the
> Anatop regulator driver.

This changelog isn't terribly verbose but looking at the code what
you've actually got here is a driver that is simply an indirection
layer for the regulator API and no explanation as to why you're doing
this.  My first thought is that anything using this driver should just
be a regulator driver directly.

Re: [PATCH] Regulator: Add Anatop regulator driver

[Ying-Chun Liu (PaulLiu)]

(2011年12月07日 23:54), Mark Brown wrote:
> On Wed, Dec 07, 2011 at 09:53:18PM +0800, Ying-Chun Liu (PaulLiu) wrote:
>> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
>>
>> Anatop regulator driver is used by i.MX6 SoC. This patch adds the
>> Anatop regulator driver.
> 
> This changelog isn't terribly verbose but looking at the code what
> you've actually got here is a driver that is simply an indirection
> layer for the regulator API and no explanation as to why you're doing
> this.  My first thought is that anything using this driver should just
> be a regulator driver directly.

Hi Mark,

Thanks. I think you are correct. I just search the LKML and found that
this has been discussed already.
https://lkml.org/lkml/2011/10/28/325

So what we should do is forget this patch and use fixed regulators
rather than create an indirection layer.

Many Thanks,
Paul

[PATCHv2] Regulator: Add Anatop regulator driver

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Anatop regulator driver is used by i.MX6 SoC. The regulator provides
controlling the voltage of PU, CORE, SOC, and some devices. This patch
adds the Anatop regulator driver.

Signed-off-by: Nancy Chen <Nancy.Chen@freescale.com>
Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
---
 drivers/regulator/Kconfig                  |    7 +
 drivers/regulator/Makefile                 |    1 +
 drivers/regulator/anatop-regulator.c       |  260 ++++++++++++++++++++++++++++
 include/linux/regulator/anatop-regulator.h |   67 +++++++
 4 files changed, 335 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regulator/anatop-regulator.c
 create mode 100644 include/linux/regulator/anatop-regulator.h

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 9713b1b..6aebd6d 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -327,5 +327,12 @@ config REGULATOR_AAT2870
 	  If you have a AnalogicTech AAT2870 say Y to enable the
 	  regulator driver.
 
+config REGULATOR_ANATOP
+	tristate "ANATOP LDO regulators"
+	depends on SOC_IMX6
+	default y if SOC_IMX6
+	help
+	  Say y here to support ANATOP LDOs regulators.
+
 endif
 
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 93a6318..990c332 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -46,5 +46,6 @@ obj-$(CONFIG_REGULATOR_AB8500)	+= ab8500.o
 obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
 obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
 obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
+obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
 
 ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
new file mode 100644
index 0000000..31156d5
--- /dev/null
+++ b/drivers/regulator/anatop-regulator.c
@@ -0,0 +1,260 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * 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.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/anatop-regulator.h>
+
+static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
+				  int max_uV, unsigned *selector)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	u32 val, rega;
+	int uv;
+
+	uv = max_uV;
+	pr_debug("%s: uv %d, min %d, max %d\n", __func__,
+		 uv, reg->constraints->min_uV,
+		 reg->constraints->max_uV);
+
+	if (uv < reg->constraints->min_uV || uv > reg->constraints->max_uV)
+		return -EINVAL;
+
+	if (anatop_reg->rdata->control_reg) {
+		val = anatop_reg->rdata->min_bit_val +
+			(uv - reg->constraints->min_uV) / 25000;
+		rega = (__raw_readl(anatop_reg->rdata->control_reg) &
+		       ~(anatop_reg->rdata->vol_bit_mask <<
+			 anatop_reg->rdata->vol_bit_shift));
+		pr_debug("%s: calculated val %d\n", __func__, val);
+		__raw_writel((val << anatop_reg->rdata->vol_bit_shift) | rega,
+			     anatop_reg->rdata->control_reg);
+		return 0;
+	} else {
+		pr_debug("Regulator not supported.\n");
+		return -ENOTSUPP;
+	}
+}
+
+static int anatop_get_voltage(struct regulator_dev *reg)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	int uv;
+	struct anatop_regulator_data *rdata = anatop_reg->rdata;
+
+	if (rdata->control_reg) {
+		u32 val = (__raw_readl(rdata->control_reg) <<
+			   rdata->vol_bit_shift) & rdata->vol_bit_mask;
+		uv = reg->constraints->min_uV
+		  + (val - rdata->min_bit_val) * 25000;
+		pr_debug("vddio = %d, val=%u\n", uv, val);
+		return uv;
+	} else {
+		pr_debug("Regulator not supported.\n");
+		return -ENOTSUPP;
+	}
+}
+
+static int anatop_enable(struct regulator_dev *reg)
+{
+	return 0;
+}
+
+static int anatop_disable(struct regulator_dev *reg)
+{
+	return 0;
+}
+
+static int anatop_is_enabled(struct regulator_dev *reg)
+{
+	return 1;
+}
+
+static struct regulator_ops anatop_rops = {
+	.set_voltage	= anatop_set_voltage,
+	.get_voltage	= anatop_get_voltage,
+	.enable		= anatop_enable,
+	.disable	= anatop_disable,
+	.is_enabled	= anatop_is_enabled,
+};
+
+static struct regulator_desc anatop_reg_desc[] = {
+	{
+		.name = "vddpu",
+		.id = ANATOP_VDDPU,
+		.ops = &anatop_rops,
+		.irq = 0,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vddcore",
+		.id = ANATOP_VDDCORE,
+		.ops = &anatop_rops,
+		.irq = 0,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vddsoc",
+		.id = ANATOP_VDDSOC,
+		.ops = &anatop_rops,
+		.irq = 0,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vdd2p5",
+		.id = ANATOP_VDD2P5,
+		.ops = &anatop_rops,
+		.irq = 0,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vdd1p1",
+		.id = ANATOP_VDD1P1,
+		.ops = &anatop_rops,
+		.irq = 0,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vdd3p0",
+		.id = ANATOP_VDD3P0,
+		.ops = &anatop_rops,
+		.irq = 0,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+};
+
+int anatop_regulator_probe(struct platform_device *pdev)
+{
+	struct regulator_desc *rdesc;
+	struct regulator_dev *rdev;
+	struct anatop_regulator *sreg;
+	struct regulator_init_data *initdata;
+
+	sreg = platform_get_drvdata(pdev);
+	initdata = pdev->dev.platform_data;
+	sreg->cur_current = 0;
+	sreg->next_current = 0;
+	sreg->cur_voltage = 0;
+
+	init_waitqueue_head(&sreg->wait_q);
+	spin_lock_init(&sreg->lock);
+
+	if (pdev->id > ANATOP_SUPPLY_NUM) {
+		rdesc = kzalloc(sizeof(struct regulator_desc), GFP_KERNEL);
+		memcpy(rdesc, &anatop_reg_desc[ANATOP_SUPPLY_NUM],
+			sizeof(struct regulator_desc));
+		rdesc->name = kstrdup(sreg->rdata->name, GFP_KERNEL);
+	} else
+		rdesc = &anatop_reg_desc[pdev->id];
+
+	pr_debug("probing regulator %s %s %d\n",
+			sreg->rdata->name,
+			rdesc->name,
+			pdev->id);
+
+	/* register regulator */
+	rdev = regulator_register(rdesc, &pdev->dev,
+				  initdata, sreg);
+
+	if (IS_ERR(rdev)) {
+		dev_err(&pdev->dev, "failed to register %s\n",
+			rdesc->name);
+		return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+
+
+int anatop_regulator_remove(struct platform_device *pdev)
+{
+	struct regulator_dev *rdev = platform_get_drvdata(pdev);
+
+	regulator_unregister(rdev);
+
+	return 0;
+
+}
+
+int anatop_register_regulator(
+		struct anatop_regulator *reg_data, int reg,
+			      struct regulator_init_data *initdata)
+{
+	struct platform_device *pdev;
+	int ret;
+
+	pdev = platform_device_alloc("anatop_reg", reg);
+	if (!pdev)
+		return -ENOMEM;
+
+	pdev->dev.platform_data = initdata;
+
+	platform_set_drvdata(pdev, reg_data);
+	ret = platform_device_add(pdev);
+
+	if (ret != 0) {
+		pr_debug("Failed to register regulator %d: %d\n",
+			reg, ret);
+		platform_device_del(pdev);
+	}
+	pr_debug("register regulator %s, %d: %d\n",
+			reg_data->rdata->name, reg, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(anatop_register_regulator);
+
+struct platform_driver anatop_reg = {
+	.driver = {
+		.name	= "anatop_reg",
+	},
+	.probe	= anatop_regulator_probe,
+	.remove	= anatop_regulator_remove,
+};
+
+int anatop_regulator_init(void)
+{
+	return platform_driver_register(&anatop_reg);
+}
+
+void anatop_regulator_exit(void)
+{
+	platform_driver_unregister(&anatop_reg);
+}
+
+postcore_initcall(anatop_regulator_init);
+module_exit(anatop_regulator_exit);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("ANATOP Regulator driver");
+MODULE_LICENSE("GPL");
+
diff --git a/include/linux/regulator/anatop-regulator.h b/include/linux/regulator/anatop-regulator.h
new file mode 100644
index 0000000..70f096b
--- /dev/null
+++ b/include/linux/regulator/anatop-regulator.h
@@ -0,0 +1,67 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * 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.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifndef __ANATOP_REGULATOR_H
+#define __ANATOP_REGULATOR_H
+#include <linux/regulator/driver.h>
+
+/* regulator supplies for Anatop */
+enum anatop_regulator_supplies {
+	ANATOP_VDDPU,
+	ANATOP_VDDCORE,
+	ANATOP_VDDSOC,
+	ANATOP_VDD2P5,
+	ANATOP_VDD1P1,
+	ANATOP_VDD3P0,
+	ANATOP_SUPPLY_NUM
+};
+
+struct anatop_regulator {
+	struct regulator_desc regulator;
+	struct anatop_regulator *parent;
+	struct anatop_regulator_data *rdata;
+	struct completion done;
+
+	spinlock_t         lock;
+	wait_queue_head_t  wait_q;
+	struct notifier_block nb;
+
+	int mode;
+	int cur_voltage;
+	int cur_current;
+	int next_current;
+};
+
+
+struct anatop_regulator_data {
+	char name[80];
+	char *parent_name;
+
+	u32 control_reg;
+	int vol_bit_shift;
+	int vol_bit_mask;
+	int min_bit_val;
+};
+
+int anatop_register_regulator(
+		struct anatop_regulator *reg_data, int reg,
+		      struct regulator_init_data *initdata);
+
+#endif /* __ANATOP_REGULATOR_H */
-- 
1.7.7.3

Re: [PATCHv2] Regulator: Add Anatop regulator driver

[Mark Brown]

On Wed, Dec 21, 2011 at 05:03:31PM +0800, Ying-Chun Liu (PaulLiu) wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> 
> Anatop regulator driver is used by i.MX6 SoC. The regulator provides
> controlling the voltage of PU, CORE, SOC, and some devices. This patch
> adds the Anatop regulator driver.

It's still not at all clear to me what an "Anatop" regulator is or why
everything is being done as platform data.

> +config REGULATOR_ANATOP
> +	tristate "ANATOP LDO regulators"
> +	depends on SOC_IMX6
> +	default y if SOC_IMX6

This isn't great, it might be on your reference design but people are
going to change that.

> +#include <linux/platform_device.h>
> +#include <linux/regulator/machine.h>

Why does your regulator driver need this?  That suggests a layering
violation.

> +static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
> +				  int max_uV, unsigned *selector)
> +{
> +	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
> +	u32 val, rega;
> +	int uv;
> +
> +	uv = max_uV;

This looks wrong, you should be aiming to get as close as possible to
the minimum not the maximum.

> +	if (anatop_reg->rdata->control_reg) {
> +		val = anatop_reg->rdata->min_bit_val +
> +			(uv - reg->constraints->min_uV) / 25000;

You're not checking that the resulting voltage matches the constraints
or updating selector.

> +	} else {
> +		pr_debug("Regulator not supported.\n");

Why are you logging an error as a debug message?  You should also use
dev_ logging.

> +static int anatop_get_voltage(struct regulator_dev *reg)
> +{

Implement this as get_voltage_sel()

> +static int anatop_enable(struct regulator_dev *reg)
> +{
> +	return 0;
> +}
> +
> +static int anatop_disable(struct regulator_dev *reg)
> +{
> +	return 0;
> +}
> +
> +static int anatop_is_enabled(struct regulator_dev *reg)
> +{
> +	return 1;
> +}

The regulator clearly doesn't have enable or disable support at all, it
shouldn't have these operations.

> +static struct regulator_ops anatop_rops = {
> +	.set_voltage	= anatop_set_voltage,
> +	.get_voltage	= anatop_get_voltage,

You should implement list_voltage() as well.

> +static struct regulator_desc anatop_reg_desc[] = {
> +	{
> +		.name = "vddpu",
> +		.id = ANATOP_VDDPU,
> +		.ops = &anatop_rops,
> +		.irq = 0,

No need to set zero fields.  It's also *very* odd to see a table
explicitly listing regulators by name in a driver that doesn't know
which registers it's working with.

> +int anatop_regulator_probe(struct platform_device *pdev)
> +{
> +	struct regulator_desc *rdesc;
> +	struct regulator_dev *rdev;
> +	struct anatop_regulator *sreg;
> +	struct regulator_init_data *initdata;
> +
> +	sreg = platform_get_drvdata(pdev);
> +	initdata = pdev->dev.platform_data;
> +	sreg->cur_current = 0;
> +	sreg->next_current = 0;
> +	sreg->cur_voltage = 0;

You're trying to read the driver data but you haven't set any.  This is
going to crash.

> +	init_waitqueue_head(&sreg->wait_q);

This waitqueue appears to never be referenced.

> +	if (pdev->id > ANATOP_SUPPLY_NUM) {
> +		rdesc = kzalloc(sizeof(struct regulator_desc), GFP_KERNEL);

devm_kzalloc() and check the return value.

> +		memcpy(rdesc, &anatop_reg_desc[ANATOP_SUPPLY_NUM],
> +			sizeof(struct regulator_desc));
> +		rdesc->name = kstrdup(sreg->rdata->name, GFP_KERNEL);

This looks really confused, you've got some regulators embedded into the
driver and some with things passed in as platform data.  Either approach
should be fine but mixing both complicates things needlessly.

> +	} else
> +		rdesc = &anatop_reg_desc[pdev->id];

Use braces on both sides of the if.

> +	pr_debug("probing regulator %s %s %d\n",
> +			sreg->rdata->name,
> +			rdesc->name,
> +			pdev->id);

A lot of this logging looks like it's just replicating logging from the
core.

> +	/* register regulator */
> +	rdev = regulator_register(rdesc, &pdev->dev,
> +				  initdata, sreg);

The driver isn't doing anything to for example map the register bank
it's using.

> +int anatop_register_regulator(
> +		struct anatop_regulator *reg_data, int reg,
> +			      struct regulator_init_data *initdata)
> +{

Eew, no.  Just register the platform device normally.

> +	int mode;
> +	int cur_voltage;
> +	int cur_current;
> +	int next_current;

These appear to be unused and are rather obscure.

> +struct anatop_regulator_data {
> +	char name[80];

const char *.

> +	char *parent_name;

What's this?

Re: [PATCHv2] Regulator: Add Anatop regulator driver

[Mark Brown]

On Thu, Dec 22, 2011 at 11:33:38AM +0000, Mark Brown wrote:
> On Wed, Dec 21, 2011 at 05:03:31PM +0800, Ying-Chun Liu (PaulLiu) wrote:

> > +	if (anatop_reg->rdata->control_reg) {
> > +		val = anatop_reg->rdata->min_bit_val +
> > +			(uv - reg->constraints->min_uV) / 25000;

> You're not checking that the resulting voltage matches the constraints
> or updating selector.

Also on re-reading this looks *very* broken - you're using the
constraints value in your set_voltage() operation.  The runtime
constraints a system has should have *no* impact on the way you ask for
a specific voltage from the regulator.

Re: [PATCHv2] Regulator: Add Anatop regulator driver

[Ying-Chun Liu (PaulLiu)]

(2011年12月22日 19:33), Mark Brown wrote:
> 
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/machine.h>
> 
> Why does your regulator driver need this?  That suggests a layering
> violation.
> 

Sorry, I'm not sure what does this mean.
But if I want to access regulator_constraints, shouldn't I include this
header file?

Yours Sincerely,
Paul

[PATCH v3] Regulator: Add Anatop regulator driver

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Anatop is an integrated regulator inside i.MX6 SoC.
There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
This patch adds the Anatop regulator driver.

Signed-off-by: Nancy Chen <Nancy.Chen@freescale.com>
Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
---
 drivers/regulator/Kconfig                  |    6 +
 drivers/regulator/Makefile                 |    1 +
 drivers/regulator/anatop-regulator.c       |  214 ++++++++++++++++++++++++++++
 include/linux/regulator/anatop-regulator.h |   63 ++++++++
 4 files changed, 284 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regulator/anatop-regulator.c
 create mode 100644 include/linux/regulator/anatop-regulator.h

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 9713b1b..fc22b8d 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -327,5 +327,11 @@ config REGULATOR_AAT2870
 	  If you have a AnalogicTech AAT2870 say Y to enable the
 	  regulator driver.
 
+config REGULATOR_ANATOP
+	tristate "ANATOP LDO regulators"
+	depends on SOC_IMX6
+	help
+	  Say y here to support ANATOP LDOs regulators.
+
 endif
 
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 93a6318..990c332 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -46,5 +46,6 @@ obj-$(CONFIG_REGULATOR_AB8500)	+= ab8500.o
 obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
 obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
 obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
+obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
 
 ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
new file mode 100644
index 0000000..a925bcc
--- /dev/null
+++ b/drivers/regulator/anatop-regulator.c
@@ -0,0 +1,214 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * 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.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/machine.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/anatop-regulator.h>
+
+static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
+				  int max_uV, unsigned *selector)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	u32 val, rega, sel;
+	int uv;
+
+	uv = min_uV;
+	pr_debug("%s: uv %d, min %d, max %d\n", __func__,
+		 uv, anatop_reg->rdata->min_voltage,
+		 anatop_reg->rdata->max_voltage);
+
+	if (uv < anatop_reg->rdata->min_voltage
+	    || uv > anatop_reg->rdata->max_voltage) {
+		if (max_uV > anatop_reg->rdata->min_voltage)
+			uv = anatop_reg->rdata->min_voltage;
+		else
+			return -EINVAL;
+	}
+
+	if (uv < reg->constraints->min_uV || uv > reg->constraints->max_uV)
+		return -EINVAL;
+
+	if (anatop_reg->rdata->control_reg) {
+		sel = (uv - anatop_reg->rdata->min_voltage) / 25000;
+		val = anatop_reg->rdata->min_bit_val + sel;
+		rega = (__raw_readl(anatop_reg->rdata->control_reg) &
+		       ~(anatop_reg->rdata->vol_bit_mask <<
+			 anatop_reg->rdata->vol_bit_shift));
+		*selector = sel;
+		pr_debug("%s: calculated val %d\n", __func__, val);
+		__raw_writel((val << anatop_reg->rdata->vol_bit_shift) | rega,
+			     anatop_reg->rdata->control_reg);
+		return 0;
+	} else {
+		return -ENOTSUPP;
+	}
+}
+
+static int anatop_get_voltage_sel(struct regulator_dev *reg)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	int selector;
+	struct anatop_regulator_data *rdata = anatop_reg->rdata;
+
+	if (rdata->control_reg) {
+		u32 val = (__raw_readl(rdata->control_reg) <<
+			   rdata->vol_bit_shift) & rdata->vol_bit_mask;
+		selector = val - rdata->min_bit_val;
+		return selector;
+	} else {
+		return -ENOTSUPP;
+	}
+}
+
+static int anatop_list_voltage(struct regulator_dev *dev, unsigned selector)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(dev);
+	int uv;
+	struct anatop_regulator_data *rdata = anatop_reg->rdata;
+
+	uv = rdata->min_voltage
+		+ selector * 25000;
+	pr_debug("vddio = %d, selector = %u\n", uv, selector);
+	return uv;
+}
+
+static struct regulator_ops anatop_rops = {
+	.set_voltage     = anatop_set_voltage,
+	.get_voltage_sel = anatop_get_voltage_sel,
+	.list_voltage    = anatop_list_voltage,
+};
+
+static struct regulator_desc anatop_reg_desc[] = {
+	{
+		.name = "vddpu",
+		.id = ANATOP_VDDPU,
+		.ops = &anatop_rops,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vddcore",
+		.id = ANATOP_VDDCORE,
+		.ops = &anatop_rops,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vddsoc",
+		.id = ANATOP_VDDSOC,
+		.ops = &anatop_rops,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vdd2p5",
+		.id = ANATOP_VDD2P5,
+		.ops = &anatop_rops,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vdd1p1",
+		.id = ANATOP_VDD1P1,
+		.ops = &anatop_rops,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+	{
+		.name = "vdd3p0",
+		.id = ANATOP_VDD3P0,
+		.ops = &anatop_rops,
+		.type = REGULATOR_VOLTAGE,
+		.owner = THIS_MODULE
+	},
+};
+
+int anatop_regulator_probe(struct platform_device *pdev)
+{
+	struct regulator_desc *rdesc;
+	struct regulator_dev *rdev;
+	struct anatop_regulator *sreg;
+	struct regulator_init_data *initdata;
+
+	initdata = pdev->dev.platform_data;
+	sreg = initdata->driver_data;
+
+	spin_lock_init(&sreg->lock);
+
+	if (pdev->id > ANATOP_SUPPLY_NUM) {
+		dev_err(&pdev->dev, "failed to register regulator id %d\n",
+			pdev->id);
+		return -EINVAL;
+	} else {
+		rdesc = &anatop_reg_desc[pdev->id];
+	}
+
+	/* register regulator */
+	rdev = regulator_register(rdesc, &pdev->dev,
+				  pdev->dev.platform_data, sreg);
+	platform_set_drvdata(pdev, rdev);
+
+	if (IS_ERR(rdev)) {
+		dev_err(&pdev->dev, "failed to register %s\n",
+			rdesc->name);
+		return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+
+int anatop_regulator_remove(struct platform_device *pdev)
+{
+	struct regulator_dev *rdev = platform_get_drvdata(pdev);
+	regulator_unregister(rdev);
+	return 0;
+}
+
+struct platform_driver anatop_reg = {
+	.driver = {
+		.name	= "anatop_reg",
+	},
+	.probe	= anatop_regulator_probe,
+	.remove	= anatop_regulator_remove,
+};
+
+int anatop_regulator_init(void)
+{
+	return platform_driver_register(&anatop_reg);
+}
+
+void anatop_regulator_exit(void)
+{
+	platform_driver_unregister(&anatop_reg);
+}
+
+postcore_initcall(anatop_regulator_init);
+module_exit(anatop_regulator_exit);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("ANATOP Regulator driver");
+MODULE_LICENSE("GPL");
+
diff --git a/include/linux/regulator/anatop-regulator.h b/include/linux/regulator/anatop-regulator.h
new file mode 100644
index 0000000..b29b009
--- /dev/null
+++ b/include/linux/regulator/anatop-regulator.h
@@ -0,0 +1,63 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * 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.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifndef __ANATOP_REGULATOR_H
+#define __ANATOP_REGULATOR_H
+#include <linux/regulator/driver.h>
+
+/* regulator supplies for Anatop */
+enum anatop_regulator_supplies {
+	ANATOP_VDDPU,
+	ANATOP_VDDCORE,
+	ANATOP_VDDSOC,
+	ANATOP_VDD2P5,
+	ANATOP_VDD1P1,
+	ANATOP_VDD3P0,
+	ANATOP_SUPPLY_NUM
+};
+
+struct anatop_regulator {
+	struct regulator_desc regulator;
+	struct anatop_regulator *parent;
+	struct anatop_regulator_data *rdata;
+	struct completion done;
+
+	spinlock_t         lock;
+	struct notifier_block nb;
+
+};
+
+
+struct anatop_regulator_data {
+	const char *name;
+
+	u32 control_reg;
+	int vol_bit_shift;
+	int vol_bit_mask;
+	int min_bit_val;
+	int min_voltage;
+	int max_voltage;
+};
+
+int anatop_register_regulator(
+		struct anatop_regulator *reg_data, int reg,
+		      struct regulator_init_data *initdata);
+
+#endif /* __ANATOP_REGULATOR_H */
-- 
1.7.7.3

Re: [PATCHv2] Regulator: Add Anatop regulator driver

[Mark Brown]

On Tue, Dec 27, 2011 at 06:06:27PM +0800, Ying-Chun Liu (PaulLiu) wrote:
> (2011年12月22日 19:33), Mark Brown wrote:

> >> +#include <linux/platform_device.h>
> >> +#include <linux/regulator/machine.h>

> > Why does your regulator driver need this?  That suggests a layering
> > violation.

> Sorry, I'm not sure what does this mean.
> But if I want to access regulator_constraints, shouldn't I include this
> header file?

If your regulator driver wants to access regulator_constraints it is
doing something wrong, that should never be required.

Re: [PATCH v3] Regulator: Add Anatop regulator driver

[Mark Brown]

On Tue, Dec 27, 2011 at 06:16:34PM +0800, Ying-Chun Liu (PaulLiu) wrote:

> +	initdata = pdev->dev.platform_data;
> +	sreg = initdata->driver_data;
> +
> +	spin_lock_init(&sreg->lock);

You don't actually appear to use this, though it looks like you need to
do something to protect against simultaneous access to the registers.

> +struct anatop_regulator {
> +       struct regulator_desc regulator;
> +       struct anatop_regulator *parent;
> +       struct anatop_regulator_data *rdata;
> +       struct completion done;
> +
> +       spinlock_t         lock;
> +       struct notifier_block nb;

You aren't using half the things in this structure.

Re: [PATCH v3] Regulator: Add Anatop regulator driver

[Shawn Guo]

On Tue, Dec 27, 2011 at 06:16:34PM +0800, Ying-Chun Liu (PaulLiu) wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> 
> Anatop is an integrated regulator inside i.MX6 SoC.
> There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
> And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
> This patch adds the Anatop regulator driver.
> 
> Signed-off-by: Nancy Chen <Nancy.Chen@freescale.com>
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Liam Girdwood <lrg@ti.com>
> ---
>  drivers/regulator/Kconfig                  |    6 +
>  drivers/regulator/Makefile                 |    1 +
>  drivers/regulator/anatop-regulator.c       |  214 ++++++++++++++++++++++++++++
>  include/linux/regulator/anatop-regulator.h |   63 ++++++++
>  4 files changed, 284 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/regulator/anatop-regulator.c
>  create mode 100644 include/linux/regulator/anatop-regulator.h
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 9713b1b..fc22b8d 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -327,5 +327,11 @@ config REGULATOR_AAT2870
>  	  If you have a AnalogicTech AAT2870 say Y to enable the
>  	  regulator driver.
>  
> +config REGULATOR_ANATOP
> +	tristate "ANATOP LDO regulators"
> +	depends on SOC_IMX6

There is no symbol 'SOC_IMX6'.  Instead, it's 'SOC_IMX6Q'.

[...]

> +int anatop_regulator_probe(struct platform_device *pdev)
> +{
> +	struct regulator_desc *rdesc;
> +	struct regulator_dev *rdev;
> +	struct anatop_regulator *sreg;
> +	struct regulator_init_data *initdata;
> +
> +	initdata = pdev->dev.platform_data;

It seems that the driver only gets probed in non-dt way.  But imx6q
only supports DT.  How does this driver work on imx6q?

PS. The regulator DT binding has been available on -next, so I do not
understand why you are still getting regulator_init_data from
platform_data rather than device tree.

-- 
Regards,
Shawn

[PATCH v4 1/2] mfd: Add anatop mfd driver

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
Anatop provides regulators and thermal.
This driver handles the address space and the operation of the mfd device.

Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
---
 drivers/mfd/Kconfig        |    6 ++
 drivers/mfd/Makefile       |    1 +
 drivers/mfd/anatop-mfd.c   |  152 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/anatop.h |   39 +++++++++++
 4 files changed, 198 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mfd/anatop-mfd.c
 create mode 100644 include/linux/mfd/anatop.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index cd13e9f..4f71627 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -830,6 +830,12 @@ config MFD_INTEL_MSIC
 	  Passage) chip. This chip embeds audio, battery, GPIO, etc.
 	  devices used in Intel Medfield platforms.
 
+config MFD_ANATOP
+        bool "Support for Anatop"
+	depends on SOC_IMX6Q
+	help
+	  Select this option to enable Anatop MFD driver.
+
 endmenu
 endif
 
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b953bab..8e11060 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -112,3 +112,4 @@ obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
 obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
 obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
 obj-$(CONFIG_MFD_S5M_CORE)	+= s5m-core.o s5m-irq.o
+obj-$(CONFIG_MFD_ANATOP)	+= anatop-mfd.o
diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
new file mode 100644
index 0000000..58c6054
--- /dev/null
+++ b/drivers/mfd/anatop-mfd.c
@@ -0,0 +1,152 @@
+/*
+ * Anatop MFD driver
+ *
+ * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
+ * Copyright (C) 2012 Linaro
+ *
+ *  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.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *  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.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/mfd/anatop.h>
+
+static u32 anatop_read(struct anatop *adata, u32 addr, int bit_shift, int bits)
+{
+	u32 val;
+	int mask;
+	if (bits == 32)
+		mask = 0xff;
+	else
+		mask = (1 << bits) - 1;
+
+	val = ioread32(adata->ioreg+addr);
+	val = (val >> bit_shift) & mask;
+	return val;
+}
+
+static void anatop_write(struct anatop *adata, u32 addr, int bit_shift,
+			 int bits, u32 data)
+{
+	u32 val;
+	int mask;
+	if (bits == 32)
+		mask = 0xff;
+	else
+		mask = (1 << bits) - 1;
+
+	val = ioread32(adata->ioreg+addr) & ~(mask << bit_shift);
+	iowrite32((data << bit_shift) | val, adata->ioreg);
+}
+
+static const struct of_device_id of_anatop_regulator_match[] = {
+	{
+		.compatible = "fsl,anatop-regulator",
+	},
+	{
+		.compatible = "fsl,anatop-thermal",
+	},
+	{ },
+};
+
+static int of_anatop_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	u64 ofaddr;
+	u64 ofsize;
+	void *ioreg;
+	struct anatop *drvdata;
+	int ret = 0;
+	const __be32 *rval;
+
+	rval = of_get_address(np, 0, &ofsize, NULL);
+	if (rval)
+		ofaddr = be32_to_cpu(*rval);
+	else
+		return -EINVAL;
+
+	ioreg = ioremap(ofaddr, ofsize);
+	if (!ioreg)
+		return -EINVAL;
+	drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);
+	if (!drvdata)
+		return -EINVAL;
+	drvdata->ioreg = ioreg;
+	drvdata->read = anatop_read;
+	drvdata->write = anatop_write;
+	platform_set_drvdata(pdev, drvdata);
+	of_platform_bus_probe(np, of_anatop_regulator_match, dev);
+	return ret;
+}
+
+static int of_anatop_remove(struct platform_device *pdev)
+{
+	return 0;
+}
+
+static const struct of_device_id of_anatop_match[] = {
+	{
+		.compatible = "fsl,imx6q-anatop",
+	},
+	{ },
+};
+
+static struct platform_driver anatop_of_driver = {
+	.driver = {
+		.name = "anatop-mfd",
+		.owner = THIS_MODULE,
+		.of_match_table = of_anatop_match,
+	},
+	.probe		= of_anatop_probe,
+	.remove		= of_anatop_remove,
+};
+
+static int __init anatop_init(void)
+{
+	int ret;
+	ret = platform_driver_register(&anatop_of_driver);
+	return ret;
+}
+
+static void __exit anatop_exit(void)
+{
+	platform_driver_unregister(&anatop_of_driver);
+}
+
+postcore_initcall(anatop_init);
+module_exit(anatop_exit);
+
+MODULE_AUTHOR("Ying-Chun Liu (PaulLiu)");
+MODULE_DESCRIPTION("ANATOP MFD driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/anatop.h b/include/linux/mfd/anatop.h
new file mode 100644
index 0000000..4425538
--- /dev/null
+++ b/include/linux/mfd/anatop.h
@@ -0,0 +1,39 @@
+/*
+ * anatop.h - Anatop MFD driver
+ *
+ *  Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paulliu@debian.org>
+ *  Copyright (C) 2012 Linaro
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef __LINUX_MFD_ANATOP_H
+#define __LINUX_MFD_ANATOP_H
+
+/**
+ * anatop - MFD data
+ * @ioreg: ioremap register
+ * @read: function to read bits from the device
+ * @write: function to write bits to the device
+ */
+struct anatop {
+	void *ioreg;
+	u32 (*read) (struct anatop *adata, u32 addr, int bit_shift, int bits);
+	void (*write) (struct anatop *adata, u32 addr, int bit_shift,
+		       int bits, u32 data);
+
+};
+
+#endif /*  __LINUX_MFD_ANATOP_H */
-- 
1.7.8.3

[PATCH v4 2/2] Regulator: Add Anatop regulator driver

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Anatop is an integrated regulator inside i.MX6 SoC.
There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
This patch adds the Anatop regulator driver.

Signed-off-by: Nancy Chen <Nancy.Chen@freescale.com>
Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
---
 drivers/regulator/Kconfig                  |    6 +
 drivers/regulator/Makefile                 |    1 +
 drivers/regulator/anatop-regulator.c       |  204 ++++++++++++++++++++++++++++
 include/linux/regulator/anatop-regulator.h |   49 +++++++
 4 files changed, 260 insertions(+), 0 deletions(-)
 create mode 100644 drivers/regulator/anatop-regulator.c
 create mode 100644 include/linux/regulator/anatop-regulator.h

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7a61b17..5fbcda2 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -335,5 +335,11 @@ config REGULATOR_AAT2870
 	  If you have a AnalogicTech AAT2870 say Y to enable the
 	  regulator driver.
 
+config REGULATOR_ANATOP
+	tristate "ANATOP LDO regulators"
+	depends on MFD_ANATOP
+	help
+	  Say y here to support ANATOP LDOs regulators.
+
 endif
 
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 503bac8..8440871 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -48,5 +48,6 @@ obj-$(CONFIG_REGULATOR_AB8500)	+= ab8500.o
 obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
 obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
 obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
+obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
 
 ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
new file mode 100644
index 0000000..d800c88
--- /dev/null
+++ b/drivers/regulator/anatop-regulator.c
@@ -0,0 +1,204 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * 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.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/anatop-regulator.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/regulator/of_regulator.h>
+
+static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
+				  int max_uV, unsigned *selector)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	u32 val, sel;
+	int uv;
+
+	uv = min_uV;
+	pr_debug("%s: uv %d, min %d, max %d\n", __func__,
+		 uv, anatop_reg->rdata->min_voltage,
+		 anatop_reg->rdata->max_voltage);
+
+	if (uv < anatop_reg->rdata->min_voltage
+	    || uv > anatop_reg->rdata->max_voltage) {
+		if (max_uV > anatop_reg->rdata->min_voltage)
+			uv = anatop_reg->rdata->min_voltage;
+		else
+			return -EINVAL;
+	}
+
+	if (anatop_reg->rdata->control_reg) {
+		sel = (uv - anatop_reg->rdata->min_voltage) / 25000;
+		val = anatop_reg->rdata->min_bit_val + sel;
+		*selector = sel;
+		pr_debug("%s: calculated val %d\n", __func__, val);
+		anatop_reg->rdata->mfd->write(anatop_reg->rdata->mfd,
+					      anatop_reg->rdata->control_reg,
+					      anatop_reg->rdata->vol_bit_shift,
+					      anatop_reg->rdata->vol_bit_size,
+					      val);
+		return 0;
+	} else {
+		return -ENOTSUPP;
+	}
+}
+
+static int anatop_get_voltage_sel(struct regulator_dev *reg)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	int selector;
+	struct anatop_regulator_data *rdata = anatop_reg->rdata;
+
+	if (rdata->control_reg) {
+		u32 val = rdata->mfd->read(rdata->mfd,
+				     rdata->control_reg,
+				     rdata->vol_bit_shift,
+				     rdata->vol_bit_size);
+		selector = val - rdata->min_bit_val;
+		return selector;
+	} else {
+		return -ENOTSUPP;
+	}
+}
+
+static int anatop_list_voltage(struct regulator_dev *dev, unsigned selector)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(dev);
+	int uv;
+	struct anatop_regulator_data *rdata = anatop_reg->rdata;
+
+	uv = rdata->min_voltage + selector * 25000;
+	pr_debug("vddio = %d, selector = %u\n", uv, selector);
+	return uv;
+}
+
+static struct regulator_ops anatop_rops = {
+	.set_voltage     = anatop_set_voltage,
+	.get_voltage_sel = anatop_get_voltage_sel,
+	.list_voltage    = anatop_list_voltage,
+};
+
+int anatop_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct regulator_desc *rdesc;
+	struct regulator_dev *rdev;
+	struct anatop_regulator *sreg;
+	struct regulator_init_data *initdata;
+	struct anatop *anatopmfd = dev_get_drvdata(pdev->dev.parent);
+	const __be32 *rval;
+	u64 ofsize;
+
+	initdata = of_get_regulator_init_data(dev, dev->of_node);
+	sreg = devm_kzalloc(dev, sizeof(struct anatop_regulator), GFP_KERNEL);
+	if (!sreg)
+		return -EINVAL;
+	rdesc = devm_kzalloc(dev, sizeof(struct regulator_desc), GFP_KERNEL);
+	if (!rdesc)
+		return -EINVAL;
+	sreg->initdata = initdata;
+	sreg->regulator = rdesc;
+	memset(rdesc, 0, sizeof(struct regulator_desc));
+	rdesc->name = kstrdup(of_get_property(np, "regulator-name", NULL),
+			      GFP_KERNEL);
+	rdesc->ops = &anatop_rops;
+	rdesc->type = REGULATOR_VOLTAGE;
+	rdesc->owner = THIS_MODULE;
+	sreg->rdata = devm_kzalloc(dev,
+				   sizeof(struct anatop_regulator_data),
+				   GFP_KERNEL);
+	sreg->rdata->name = kstrdup(rdesc->name, GFP_KERNEL);
+	rval = of_get_address(np, 0, &ofsize, NULL);
+	if (rval) {
+		sreg->rdata->control_reg = be32_to_cpu(rval[0]);
+		sreg->rdata->vol_bit_shift = be32_to_cpu(rval[1]);
+	}
+	sreg->rdata->mfd = anatopmfd;
+	sreg->rdata->vol_bit_size = (int)ofsize;
+	rval = of_get_property(np, "min-bit-val", NULL);
+	if (rval)
+		sreg->rdata->min_bit_val = be32_to_cpu(*rval);
+	rval = of_get_property(np, "min-voltage", NULL);
+	if (rval)
+		sreg->rdata->min_voltage = be32_to_cpu(*rval);
+	rval = of_get_property(np, "max-voltage", NULL);
+	if (rval)
+		sreg->rdata->max_voltage = be32_to_cpu(*rval);
+
+	/* register regulator */
+	rdev = regulator_register(rdesc, &pdev->dev,
+				  initdata, sreg, pdev->dev.of_node);
+	platform_set_drvdata(pdev, rdev);
+
+	if (IS_ERR(rdev)) {
+		dev_err(&pdev->dev, "failed to register %s\n",
+			rdesc->name);
+		return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+
+int anatop_regulator_remove(struct platform_device *pdev)
+{
+	struct regulator_dev *rdev = platform_get_drvdata(pdev);
+	regulator_unregister(rdev);
+	return 0;
+}
+
+struct of_device_id __devinitdata of_anatop_regulator_match_tbl[] = {
+	{ .compatible = "fsl,anatop-regulator", },
+	{ /* end */ }
+};
+
+
+struct platform_driver anatop_regulator = {
+	.driver = {
+		.name	= "anatop_regulator",
+		.owner  = THIS_MODULE,
+		.of_match_table = of_anatop_regulator_match_tbl,
+	},
+	.probe	= anatop_regulator_probe,
+	.remove	= anatop_regulator_remove,
+};
+
+int anatop_regulator_init(void)
+{
+	return platform_driver_register(&anatop_regulator);
+}
+
+void anatop_regulator_exit(void)
+{
+	platform_driver_unregister(&anatop_regulator);
+}
+
+postcore_initcall(anatop_regulator_init);
+module_exit(anatop_regulator_exit);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("ANATOP Regulator driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/regulator/anatop-regulator.h b/include/linux/regulator/anatop-regulator.h
new file mode 100644
index 0000000..089d519
--- /dev/null
+++ b/include/linux/regulator/anatop-regulator.h
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * 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.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifndef __ANATOP_REGULATOR_H
+#define __ANATOP_REGULATOR_H
+#include <linux/regulator/driver.h>
+#include <linux/mfd/anatop.h>
+
+struct anatop_regulator {
+	struct regulator_desc *regulator;
+	struct regulator_init_data *initdata;
+	struct anatop_regulator_data *rdata;
+};
+
+
+struct anatop_regulator_data {
+	const char *name;
+
+	u32 control_reg;
+	struct anatop *mfd;
+	int vol_bit_shift;
+	int vol_bit_size;
+	int min_bit_val;
+	int min_voltage;
+	int max_voltage;
+};
+
+int anatop_register_regulator(
+		struct anatop_regulator *reg_data, int reg,
+		      struct regulator_init_data *initdata);
+
+#endif /* __ANATOP_REGULATOR_H */
-- 
1.7.8.3

Re: [PATCH v4 2/2] Regulator: Add Anatop regulator driver

[Mark Brown]

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

On Thu, Feb 09, 2012 at 04:51:26AM +0800, Ying-Chun Liu (PaulLiu) wrote:

Overall this is looking pretty good, a few issues but relatively minor.

> +	if (uv < anatop_reg->rdata->min_voltage
> +	    || uv > anatop_reg->rdata->max_voltage) {
> +		if (max_uV > anatop_reg->rdata->min_voltage)
> +			uv = anatop_reg->rdata->min_voltage;
> +		else
> +			return -EINVAL;
> +	}

This looks buggy (and is quite hard to follow).  The check for the
voltage being above the minimum voltage is fine but surely if the
voltage is too high then the first if statement will be true and max_uV
will be greater than the minimum voltage so the second if will be true.
This will cause us to choose the minimum voltage that the regulator can
do which is less than the minimum voltage requested.

You probably shouldn't be checking for the upper end of the range at all
here...

> +	if (anatop_reg->rdata->control_reg) {
> +		sel = (uv - anatop_reg->rdata->min_voltage) / 25000;
> +		val = anatop_reg->rdata->min_bit_val + sel;
> +		*selector = sel;
> +		pr_debug("%s: calculated val %d\n", __func__, val);

...instead check that selector is in range here.

> +		anatop_reg->rdata->mfd->write(anatop_reg->rdata->mfd,
> +					      anatop_reg->rdata->control_reg,
> +					      anatop_reg->rdata->vol_bit_shift,
> +					      anatop_reg->rdata->vol_bit_size,
> +					      val);

Just have a write() function in the MFD.

> +	memset(rdesc, 0, sizeof(struct regulator_desc));
> +	rdesc->name = kstrdup(of_get_property(np, "regulator-name", NULL),
> +			      GFP_KERNEL);

You should add binding documentation for the device since it's OF based.

> +	sreg->rdata->name = kstrdup(rdesc->name, GFP_KERNEL);

Can't you just point to rdesc->name?

> +	if (IS_ERR(rdev)) {
> +		dev_err(&pdev->dev, "failed to register %s\n",
> +			rdesc->name);
> +		return PTR_ERR(rdev);
> +	}

All your kstrdup()s need to be unwound in error and free cases.

> +int anatop_regulator_init(void)
> +{
> +	return platform_driver_register(&anatop_regulator);
> +}
> +
> +void anatop_regulator_exit(void)
> +{
> +	platform_driver_unregister(&anatop_regulator);
> +}
> +
> +postcore_initcall(anatop_regulator_init);
> +module_exit(anatop_regulator_exit);

These should be adjacent to the functions.

> +MODULE_AUTHOR("Freescale Semiconductor, Inc.");

Either omit this or put one or more humans as the author.

> +struct anatop_regulator {
> +       struct regulator_desc *regulator;
> +       struct regulator_init_data *initdata;
> +       struct anatop_regulator_data *rdata;
> +};

May as well just merge the regulator data in here - it's not ever used
except with a 1:1 relationship between them.  Could also directly
embed the desc and init_data, then you just have one allocation for the
data rather than a series to error check.

> +int anatop_register_regulator(
> +		struct anatop_regulator *reg_data, int reg,
> +		      struct regulator_init_data *initdata);

This looks like it's not defined any more so could be removed and since
you only appear to support OF the entire header could be merged into the
driver.

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

Re: [PATCH v4 1/2] mfd: Add anatop mfd driver

[Shawn Guo]

On Thu, Feb 09, 2012 at 04:51:25AM +0800, Ying-Chun Liu (PaulLiu) wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> 
> Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
> Anatop provides regulators and thermal.
> This driver handles the address space and the operation of the mfd device.
> 
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> ---
>  drivers/mfd/Kconfig        |    6 ++
>  drivers/mfd/Makefile       |    1 +
>  drivers/mfd/anatop-mfd.c   |  152 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/anatop.h |   39 +++++++++++
>  4 files changed, 198 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/mfd/anatop-mfd.c
>  create mode 100644 include/linux/mfd/anatop.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index cd13e9f..4f71627 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -830,6 +830,12 @@ config MFD_INTEL_MSIC
>  	  Passage) chip. This chip embeds audio, battery, GPIO, etc.
>  	  devices used in Intel Medfield platforms.
>  
> +config MFD_ANATOP
> +        bool "Support for Anatop"
> +	depends on SOC_IMX6Q
> +	help
> +	  Select this option to enable Anatop MFD driver.
> +
>  endmenu
>  endif
>  
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index b953bab..8e11060 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -112,3 +112,4 @@ obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
>  obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
>  obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
>  obj-$(CONFIG_MFD_S5M_CORE)	+= s5m-core.o s5m-irq.o
> +obj-$(CONFIG_MFD_ANATOP)	+= anatop-mfd.o
> diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
> new file mode 100644
> index 0000000..58c6054
> --- /dev/null
> +++ b/drivers/mfd/anatop-mfd.c
> @@ -0,0 +1,152 @@
> +/*
> + * Anatop MFD driver
> + *
> + * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> + * Copyright (C) 2012 Linaro
> + *
> + *  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.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *  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.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + */
> +
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/mfd/anatop.h>
> +
> +static u32 anatop_read(struct anatop *adata, u32 addr, int bit_shift, int bits)
> +{
> +	u32 val;
> +	int mask;

Nit: it may be nice to put a blank line here.

> +	if (bits == 32)
> +		mask = 0xff;

Shouldn't it be ~0?

> +	else
> +		mask = (1 << bits) - 1;
> +
> +	val = ioread32(adata->ioreg+addr);

Why not just readl()?  Nit: put space before and after '+'.

> +	val = (val >> bit_shift) & mask;

Nit: it may be nice to put a blank line here.

> +	return val;
> +}
> +
> +static void anatop_write(struct anatop *adata, u32 addr, int bit_shift,
> +			 int bits, u32 data)
> +{
> +	u32 val;
> +	int mask;
> +	if (bits == 32)
> +		mask = 0xff;
> +	else
> +		mask = (1 << bits) - 1;
> +
> +	val = ioread32(adata->ioreg+addr) & ~(mask << bit_shift);
> +	iowrite32((data << bit_shift) | val, adata->ioreg);

Same comments on anatop_read apply on anatop_write here.

> +}
> +
> +static const struct of_device_id of_anatop_regulator_match[] = {

A better naming of of_anatop_regulator_match, since it covers not only
regulator but also thermal as below?

> +	{
> +		.compatible = "fsl,anatop-regulator",
> +	},

Nit: since you only have one line here, it could be just:

	{ .compatible = "fsl,anatop-regulator", },

> +	{
> +		.compatible = "fsl,anatop-thermal",
> +	},
> +	{ },
> +};
> +
> +static int of_anatop_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	u64 ofaddr;
> +	u64 ofsize;
> +	void *ioreg;
> +	struct anatop *drvdata;
> +	int ret = 0;
> +	const __be32 *rval;
> +
---
> +	rval = of_get_address(np, 0, &ofsize, NULL);
> +	if (rval)
> +		ofaddr = be32_to_cpu(*rval);
> +	else
> +		return -EINVAL;
> +
> +	ioreg = ioremap(ofaddr, ofsize);
---

Above lines can just be of_iomap(np, 0);

> +	if (!ioreg)
> +		return -EINVAL;
> +	drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);
> +	if (!drvdata)
> +		return -EINVAL;
> +	drvdata->ioreg = ioreg;
> +	drvdata->read = anatop_read;
> +	drvdata->write = anatop_write;
> +	platform_set_drvdata(pdev, drvdata);
> +	of_platform_bus_probe(np, of_anatop_regulator_match, dev);

Nit: it may be nice to put a blank line here.

> +	return ret;

The 'ret' is used nowhere, so the above line can just be:

	return 0;

> +}
> +
> +static int of_anatop_remove(struct platform_device *pdev)
> +{

Nothing to do here?  At least iounmap?

> +	return 0;
> +}
> +
> +static const struct of_device_id of_anatop_match[] = {
> +	{
> +		.compatible = "fsl,imx6q-anatop",
> +	},

	{ .compatible = "fsl,imx6q-anatop", },

> +	{ },
> +};
> +
> +static struct platform_driver anatop_of_driver = {
> +	.driver = {
> +		.name = "anatop-mfd",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_anatop_match,
> +	},
> +	.probe		= of_anatop_probe,
> +	.remove		= of_anatop_remove,
> +};
> +
> +static int __init anatop_init(void)
> +{
> +	int ret;
> +	ret = platform_driver_register(&anatop_of_driver);
> +	return ret;

	return platform_driver_register(&anatop_of_driver);

> +}
> +
> +static void __exit anatop_exit(void)
> +{
> +	platform_driver_unregister(&anatop_of_driver);
> +}
> +
> +postcore_initcall(anatop_init);
> +module_exit(anatop_exit);
> +
> +MODULE_AUTHOR("Ying-Chun Liu (PaulLiu)");

Missing your email address here.

> +MODULE_DESCRIPTION("ANATOP MFD driver");
> +MODULE_LICENSE("GPL");

"GPL v2" for better?

> diff --git a/include/linux/mfd/anatop.h b/include/linux/mfd/anatop.h
> new file mode 100644
> index 0000000..4425538
> --- /dev/null
> +++ b/include/linux/mfd/anatop.h
> @@ -0,0 +1,39 @@
> +/*
> + * anatop.h - Anatop MFD driver
> + *
> + *  Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paulliu@debian.org>

It would nice to use a consistent email address through the patch.

Regards,
Shawn

> + *  Copyright (C) 2012 Linaro
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#ifndef __LINUX_MFD_ANATOP_H
> +#define __LINUX_MFD_ANATOP_H
> +
> +/**
> + * anatop - MFD data
> + * @ioreg: ioremap register
> + * @read: function to read bits from the device
> + * @write: function to write bits to the device
> + */
> +struct anatop {
> +	void *ioreg;
> +	u32 (*read) (struct anatop *adata, u32 addr, int bit_shift, int bits);
> +	void (*write) (struct anatop *adata, u32 addr, int bit_shift,
> +		       int bits, u32 data);
> +
> +};
> +
> +#endif /*  __LINUX_MFD_ANATOP_H */
> -- 
> 1.7.8.3
> 

Re: [PATCH v4 2/2] Regulator: Add Anatop regulator driver

[Shawn Guo]

On Thu, Feb 09, 2012 at 04:51:26AM +0800, Ying-Chun Liu (PaulLiu) wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> 
> Anatop is an integrated regulator inside i.MX6 SoC.
> There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
> And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
> This patch adds the Anatop regulator driver.
> 
> Signed-off-by: Nancy Chen <Nancy.Chen@freescale.com>
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Liam Girdwood <lrg@ti.com>
> ---
>  drivers/regulator/Kconfig                  |    6 +
>  drivers/regulator/Makefile                 |    1 +
>  drivers/regulator/anatop-regulator.c       |  204 ++++++++++++++++++++++++++++
>  include/linux/regulator/anatop-regulator.h |   49 +++++++
>  4 files changed, 260 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/regulator/anatop-regulator.c
>  create mode 100644 include/linux/regulator/anatop-regulator.h
> 
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 7a61b17..5fbcda2 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -335,5 +335,11 @@ config REGULATOR_AAT2870
>  	  If you have a AnalogicTech AAT2870 say Y to enable the
>  	  regulator driver.
>  
> +config REGULATOR_ANATOP
> +	tristate "ANATOP LDO regulators"
> +	depends on MFD_ANATOP
> +	help
> +	  Say y here to support ANATOP LDOs regulators.
> +
>  endif
>  
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 503bac8..8440871 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -48,5 +48,6 @@ obj-$(CONFIG_REGULATOR_AB8500)	+= ab8500.o
>  obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
>  obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
>  obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
> +obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
>  
>  ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
> diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
> new file mode 100644
> index 0000000..d800c88
> --- /dev/null
> +++ b/drivers/regulator/anatop-regulator.c
> @@ -0,0 +1,204 @@
> +/*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + */
> +
> +/*
> + * 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.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/anatop-regulator.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/regulator/of_regulator.h>
> +
> +static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
> +				  int max_uV, unsigned *selector)
> +{
> +	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
> +	u32 val, sel;
> +	int uv;
> +
> +	uv = min_uV;
> +	pr_debug("%s: uv %d, min %d, max %d\n", __func__,

I would suggest dev_dbg in device driver.

> +		 uv, anatop_reg->rdata->min_voltage,
> +		 anatop_reg->rdata->max_voltage);
> +
> +	if (uv < anatop_reg->rdata->min_voltage
> +	    || uv > anatop_reg->rdata->max_voltage) {
> +		if (max_uV > anatop_reg->rdata->min_voltage)
> +			uv = anatop_reg->rdata->min_voltage;
> +		else
> +			return -EINVAL;
> +	}
> +
> +	if (anatop_reg->rdata->control_reg) {
> +		sel = (uv - anatop_reg->rdata->min_voltage) / 25000;
> +		val = anatop_reg->rdata->min_bit_val + sel;
> +		*selector = sel;
> +		pr_debug("%s: calculated val %d\n", __func__, val);
> +		anatop_reg->rdata->mfd->write(anatop_reg->rdata->mfd,
> +					      anatop_reg->rdata->control_reg,
> +					      anatop_reg->rdata->vol_bit_shift,
> +					      anatop_reg->rdata->vol_bit_size,
> +					      val);
> +		return 0;
> +	} else {
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static int anatop_get_voltage_sel(struct regulator_dev *reg)
> +{
> +	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
> +	int selector;
> +	struct anatop_regulator_data *rdata = anatop_reg->rdata;
> +
> +	if (rdata->control_reg) {
> +		u32 val = rdata->mfd->read(rdata->mfd,
> +				     rdata->control_reg,
> +				     rdata->vol_bit_shift,
> +				     rdata->vol_bit_size);
> +		selector = val - rdata->min_bit_val;
> +		return selector;
> +	} else {
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static int anatop_list_voltage(struct regulator_dev *dev, unsigned selector)
> +{
> +	struct anatop_regulator *anatop_reg = rdev_get_drvdata(dev);
> +	int uv;
> +	struct anatop_regulator_data *rdata = anatop_reg->rdata;
> +
> +	uv = rdata->min_voltage + selector * 25000;
> +	pr_debug("vddio = %d, selector = %u\n", uv, selector);
> +	return uv;
> +}
> +
> +static struct regulator_ops anatop_rops = {
> +	.set_voltage     = anatop_set_voltage,
> +	.get_voltage_sel = anatop_get_voltage_sel,
> +	.list_voltage    = anatop_list_voltage,
> +};
> +
> +int anatop_regulator_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct regulator_desc *rdesc;
> +	struct regulator_dev *rdev;
> +	struct anatop_regulator *sreg;
> +	struct regulator_init_data *initdata;
> +	struct anatop *anatopmfd = dev_get_drvdata(pdev->dev.parent);
> +	const __be32 *rval;
> +	u64 ofsize;
> +
> +	initdata = of_get_regulator_init_data(dev, dev->of_node);
> +	sreg = devm_kzalloc(dev, sizeof(struct anatop_regulator), GFP_KERNEL);
> +	if (!sreg)
> +		return -EINVAL;
> +	rdesc = devm_kzalloc(dev, sizeof(struct regulator_desc), GFP_KERNEL);
> +	if (!rdesc)
> +		return -EINVAL;
> +	sreg->initdata = initdata;
> +	sreg->regulator = rdesc;
> +	memset(rdesc, 0, sizeof(struct regulator_desc));
> +	rdesc->name = kstrdup(of_get_property(np, "regulator-name", NULL),
> +			      GFP_KERNEL);

We can probably reuse the regulator's node name here to save property
regulator-name.

> +	rdesc->ops = &anatop_rops;
> +	rdesc->type = REGULATOR_VOLTAGE;
> +	rdesc->owner = THIS_MODULE;
> +	sreg->rdata = devm_kzalloc(dev,
> +				   sizeof(struct anatop_regulator_data),
> +				   GFP_KERNEL);
> +	sreg->rdata->name = kstrdup(rdesc->name, GFP_KERNEL);
> +	rval = of_get_address(np, 0, &ofsize, NULL);
> +	if (rval) {
> +		sreg->rdata->control_reg = be32_to_cpu(rval[0]);
> +		sreg->rdata->vol_bit_shift = be32_to_cpu(rval[1]);

I'm not sure you can (ab)use 'reg' property for bit shift.  We may
need to have a property for it.

> +	}
> +	sreg->rdata->mfd = anatopmfd;
> +	sreg->rdata->vol_bit_size = (int)ofsize;
> +	rval = of_get_property(np, "min-bit-val", NULL);
> +	if (rval)
> +		sreg->rdata->min_bit_val = be32_to_cpu(*rval);
> +	rval = of_get_property(np, "min-voltage", NULL);
> +	if (rval)
> +		sreg->rdata->min_voltage = be32_to_cpu(*rval);
> +	rval = of_get_property(np, "max-voltage", NULL);
> +	if (rval)
> +		sreg->rdata->max_voltage = be32_to_cpu(*rval);

We need a sensible binding document to understand those.  But at least,
shouldn't min-voltage and max-voltage be retrieved as the common
regulator binding documented in
Documentation/devicetree/bindings/regulator/regulator.txt?

> +
> +	/* register regulator */
> +	rdev = regulator_register(rdesc, &pdev->dev,
> +				  initdata, sreg, pdev->dev.of_node);
> +	platform_set_drvdata(pdev, rdev);
> +
> +	if (IS_ERR(rdev)) {
> +		dev_err(&pdev->dev, "failed to register %s\n",
> +			rdesc->name);
> +		return PTR_ERR(rdev);
> +	}
> +
> +	return 0;
> +}
> +
> +int anatop_regulator_remove(struct platform_device *pdev)
> +{
> +	struct regulator_dev *rdev = platform_get_drvdata(pdev);
> +	regulator_unregister(rdev);
> +	return 0;
> +}
> +
> +struct of_device_id __devinitdata of_anatop_regulator_match_tbl[] = {
> +	{ .compatible = "fsl,anatop-regulator", },
> +	{ /* end */ }
> +};
> +
> +

One blank line is enough.

> +struct platform_driver anatop_regulator = {
> +	.driver = {
> +		.name	= "anatop_regulator",
> +		.owner  = THIS_MODULE,
> +		.of_match_table = of_anatop_regulator_match_tbl,
> +	},
> +	.probe	= anatop_regulator_probe,
> +	.remove	= anatop_regulator_remove,
> +};
> +
> +int anatop_regulator_init(void)
> +{
> +	return platform_driver_register(&anatop_regulator);
> +}
> +
> +void anatop_regulator_exit(void)
> +{
> +	platform_driver_unregister(&anatop_regulator);
> +}
> +
> +postcore_initcall(anatop_regulator_init);
> +module_exit(anatop_regulator_exit);
> +
> +MODULE_AUTHOR("Freescale Semiconductor, Inc.");
> +MODULE_DESCRIPTION("ANATOP Regulator driver");
> +MODULE_LICENSE("GPL");

"GPL v2"?

> diff --git a/include/linux/regulator/anatop-regulator.h b/include/linux/regulator/anatop-regulator.h
> new file mode 100644
> index 0000000..089d519
> --- /dev/null
> +++ b/include/linux/regulator/anatop-regulator.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + */
> +
> +/*
> + * 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.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#ifndef __ANATOP_REGULATOR_H
> +#define __ANATOP_REGULATOR_H

Have a blank line here.

> +#include <linux/regulator/driver.h>
> +#include <linux/mfd/anatop.h>
> +
> +struct anatop_regulator {
> +	struct regulator_desc *regulator;
> +	struct regulator_init_data *initdata;
> +	struct anatop_regulator_data *rdata;
> +};
> +
> +

Drop one blank line here.

> +struct anatop_regulator_data {
> +	const char *name;
> +

Nit: drop this blank line.

> +	u32 control_reg;
> +	struct anatop *mfd;
> +	int vol_bit_shift;
> +	int vol_bit_size;
> +	int min_bit_val;
> +	int min_voltage;
> +	int max_voltage;
> +};
> +
It seems to me that anatop_regulator_data should be put before 
anatop_regulator.

Regards,
Shawn

> +int anatop_register_regulator(
> +		struct anatop_regulator *reg_data, int reg,
> +		      struct regulator_init_data *initdata);
> +
> +#endif /* __ANATOP_REGULATOR_H */
> -- 
> 1.7.8.3
> 

Re: [PATCH v4 2/2] Regulator: Add Anatop regulator driver

[Mark Brown]

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

On Fri, Feb 10, 2012 at 10:36:38PM -0800, Shawn Guo wrote:
> On Thu, Feb 09, 2012 at 04:51:26AM +0800, Ying-Chun Liu (PaulLiu) wrote:

> > +	rval = of_get_property(np, "min-voltage", NULL);
> > +	if (rval)
> > +		sreg->rdata->min_voltage = be32_to_cpu(*rval);
> > +	rval = of_get_property(np, "max-voltage", NULL);
> > +	if (rval)
> > +		sreg->rdata->max_voltage = be32_to_cpu(*rval);

> We need a sensible binding document to understand those.  But at least,
> shouldn't min-voltage and max-voltage be retrieved as the common
> regulator binding documented in
> Documentation/devicetree/bindings/regulator/regulator.txt?

Normally this would be a bad idea as the set of voltages that can safely
be used on a given board might differ from those which are supported by
the device.  However in this case you might be OK as this is all
internal to the SoC and so presumably won't vary from board to board.

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

Re: [PATCH v4 1/2] mfd: Add anatop mfd driver

[Samuel Ortiz]

Hi Paul,

I didn't get patch #2, so I don't get to see how the read/write functions ar
eused for example.

On Thu, Feb 09, 2012 at 04:51:25AM +0800, Ying-Chun Liu (PaulLiu) wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> 
> Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
> Anatop provides regulators and thermal.
> This driver handles the address space and the operation of the mfd device.
A few comments here:



> +static u32 anatop_read(struct anatop *adata, u32 addr, int bit_shift, int bits)
> +{
> +	u32 val;
> +	int mask;
> +	if (bits == 32)
> +		mask = 0xff;
> +	else
> +		mask = (1 << bits) - 1;
> +
> +	val = ioread32(adata->ioreg+addr);
> +	val = (val >> bit_shift) & mask;
> +	return val;
> +}
> +
> +static void anatop_write(struct anatop *adata, u32 addr, int bit_shift,
> +			 int bits, u32 data)
> +{
> +	u32 val;
> +	int mask;
> +	if (bits == 32)
> +		mask = 0xff;
> +	else
> +		mask = (1 << bits) - 1;
> +
> +	val = ioread32(adata->ioreg+addr) & ~(mask << bit_shift);
> +	iowrite32((data << bit_shift) | val, adata->ioreg);
> +}
Don't you need some sort of read/write atomic routine as well ? Locking would
be needed then...


> +static const struct of_device_id of_anatop_regulator_match[] = {
> +	{
> +		.compatible = "fsl,anatop-regulator",
> +	},
> +	{
> +		.compatible = "fsl,anatop-thermal",
> +	},
> +	{ },
> +};
> +
> +static int of_anatop_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	u64 ofaddr;
> +	u64 ofsize;
> +	void *ioreg;
> +	struct anatop *drvdata;
> +	int ret = 0;
> +	const __be32 *rval;
> +
> +	rval = of_get_address(np, 0, &ofsize, NULL);
> +	if (rval)
> +		ofaddr = be32_to_cpu(*rval);
> +	else
> +		return -EINVAL;
> +
> +	ioreg = ioremap(ofaddr, ofsize);
> +	if (!ioreg)
> +		return -EINVAL;
> +	drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);
> +	if (!drvdata)
> +		return -EINVAL;
> +	drvdata->ioreg = ioreg;
> +	drvdata->read = anatop_read;
> +	drvdata->write = anatop_write;
> +	platform_set_drvdata(pdev, drvdata);
> +	of_platform_bus_probe(np, of_anatop_regulator_match, dev);
> +	return ret;
> +}
So it seems that your driver here does nothing but extending your device tree
definition. Correct me if I'm wrong, aren't you trying to fix a broken device
tree definition here ?

Cheers,
Samuel.

-- 
Intel Open Source Technology Centre
http://oss.intel.com/

[PATCH v5 1/2] mfd: Add anatop mfd driver

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
Anatop provides regulators and thermal.
This driver handles the address space and the operation of the mfd device.

Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/mfd/Kconfig        |    6 ++
 drivers/mfd/Makefile       |    1 +
 drivers/mfd/anatop-mfd.c   |  144 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/anatop.h |   40 ++++++++++++
 4 files changed, 191 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mfd/anatop-mfd.c
 create mode 100644 include/linux/mfd/anatop.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index f147395..f787d17 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -830,6 +830,12 @@ config MFD_INTEL_MSIC
 	  Passage) chip. This chip embeds audio, battery, GPIO, etc.
 	  devices used in Intel Medfield platforms.
 
+config MFD_ANATOP
+        bool "Support for Anatop"
+	depends on SOC_IMX6Q
+	help
+	  Select this option to enable Anatop MFD driver.
+
 endmenu
 endif
 
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b953bab..8e11060 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -112,3 +112,4 @@ obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
 obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
 obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
 obj-$(CONFIG_MFD_S5M_CORE)	+= s5m-core.o s5m-irq.o
+obj-$(CONFIG_MFD_ANATOP)	+= anatop-mfd.o
diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
new file mode 100644
index 0000000..86e2b8e
--- /dev/null
+++ b/drivers/mfd/anatop-mfd.c
@@ -0,0 +1,144 @@
+/*
+ * Anatop MFD driver
+ *
+ * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
+ * Copyright (C) 2012 Linaro
+ *
+ *  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.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *  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.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/mfd/anatop.h>
+
+u32 anatop_get_bits(struct anatop *adata, u32 addr, int bit_shift, int bits)
+{
+	u32 val;
+	int mask;
+
+	if (bits == 32)
+		mask = ~0;
+	else
+		mask = (1 << bits) - 1;
+
+	spin_lock(&adata->reglock);
+	val = readl(adata->ioreg + addr);
+	spin_unlock(&adata->reglock);
+	val = (val >> bit_shift) & mask;
+
+	return val;
+}
+EXPORT_SYMBOL(anatop_get_bits);
+
+void anatop_set_bits(struct anatop *adata, u32 addr, int bit_shift,
+		     int bits, u32 data)
+{
+	u32 val;
+	int mask;
+	if (bits == 32)
+		mask = ~0;
+	else
+		mask = (1 << bits) - 1;
+
+	spin_lock(&adata->reglock);
+	val = readl(adata->ioreg + addr) & ~(mask << bit_shift);
+	writel((data << bit_shift) | val, adata->ioreg);
+	spin_unlock(&adata->reglock);
+}
+EXPORT_SYMBOL(anatop_set_bits);
+
+static const struct of_device_id of_anatop_subdevice_match[] = {
+	{ .compatible = "fsl,anatop-regulator", },
+	{ .compatible = "fsl,anatop-thermal", },
+	{ },
+};
+
+static int of_anatop_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	void *ioreg;
+	struct anatop *drvdata;
+
+	ioreg = of_iomap(np, 0);
+	if (!ioreg)
+		return -EINVAL;
+	drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);
+	if (!drvdata)
+		return -EINVAL;
+	drvdata->ioreg = ioreg;
+	spin_lock_init(&drvdata->reglock);
+	platform_set_drvdata(pdev, drvdata);
+	of_platform_bus_probe(np, of_anatop_subdevice_match, dev);
+
+	return 0;
+}
+
+static int of_anatop_remove(struct platform_device *pdev)
+{
+	struct anatop *drvdata;
+	drvdata = platform_get_drvdata(pdev);
+	iounmap(drvdata->ioreg);
+	return 0;
+}
+
+static const struct of_device_id of_anatop_match[] = {
+	{ .compatible = "fsl,imx6q-anatop", },
+	{ },
+};
+
+static struct platform_driver anatop_of_driver = {
+	.driver = {
+		.name = "anatop-mfd",
+		.owner = THIS_MODULE,
+		.of_match_table = of_anatop_match,
+	},
+	.probe		= of_anatop_probe,
+	.remove		= of_anatop_remove,
+};
+
+static int __init anatop_init(void)
+{
+	return platform_driver_register(&anatop_of_driver);
+}
+postcore_initcall(anatop_init);
+
+static void __exit anatop_exit(void)
+{
+	platform_driver_unregister(&anatop_of_driver);
+}
+module_exit(anatop_exit);
+
+MODULE_AUTHOR("Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>");
+MODULE_DESCRIPTION("ANATOP MFD driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/anatop.h b/include/linux/mfd/anatop.h
new file mode 100644
index 0000000..22c1007
--- /dev/null
+++ b/include/linux/mfd/anatop.h
@@ -0,0 +1,40 @@
+/*
+ * anatop.h - Anatop MFD driver
+ *
+ *  Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
+ *  Copyright (C) 2012 Linaro
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef __LINUX_MFD_ANATOP_H
+#define __LINUX_MFD_ANATOP_H
+
+#include <linux/spinlock.h>
+
+/**
+ * anatop - MFD data
+ * @ioreg: ioremap register
+ * @reglock: spinlock for register read/write
+ */
+struct anatop {
+	void *ioreg;
+	spinlock_t reglock;
+};
+
+extern u32 anatop_get_bits(struct anatop *, u32, int, int);
+extern void anatop_set_bits(struct anatop *, u32, int, int, u32);
+
+#endif /*  __LINUX_MFD_ANATOP_H */
-- 
1.7.9

[PATCH v5 2/2] Regulator: Add Anatop regulator driver

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Anatop is an integrated regulator inside i.MX6 SoC.
There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
This patch adds the Anatop regulator driver.

Signed-off-by: Nancy Chen <Nancy.Chen@freescale.com>
Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 .../bindings/regulator/anatop-regulator.txt        |   28 +++
 drivers/regulator/Kconfig                          |    6 +
 drivers/regulator/Makefile                         |    1 +
 drivers/regulator/anatop-regulator.c               |  203 ++++++++++++++++++++
 include/linux/regulator/anatop-regulator.h         |   40 ++++
 5 files changed, 278 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/anatop-regulator.txt
 create mode 100644 drivers/regulator/anatop-regulator.c
 create mode 100644 include/linux/regulator/anatop-regulator.h

diff --git a/Documentation/devicetree/bindings/regulator/anatop-regulator.txt b/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
new file mode 100644
index 0000000..9ed7326
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
@@ -0,0 +1,28 @@
+Anatop Voltage regulators
+
+Required properties:
+- compatible: Must be "fsl,anatop-regulator"
+- vol-bit-shift: Bit shift for the register
+- vol-bit-size: Number of bits used in the register
+- min-bit-val: Minimum value of this register
+- min-voltage: Minimum voltage of this regulator
+- max-voltage: Maximum voltage of this regulator
+
+Any property defined as part of the core regulator
+binding, defined in regulator.txt, can also be used.
+
+Example:
+
+	reg_vddpu: regulator-vddpu@140 {
+		compatible = "fsl,anatop-regulator";
+		regulator-name = "vddpu";
+		regulator-min-microvolt = <725000>;
+		regulator-max-microvolt = <1300000>;
+		regulator-always-on;
+		reg = <0x140>;
+		vol-bit-shift = <9>;
+		vol-bit-size = <5>;
+		min-bit-val = <1>;
+		min-voltage = <725000>;
+		max-voltage = <1300000>;
+	};
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7a61b17..5fbcda2 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -335,5 +335,11 @@ config REGULATOR_AAT2870
 	  If you have a AnalogicTech AAT2870 say Y to enable the
 	  regulator driver.
 
+config REGULATOR_ANATOP
+	tristate "ANATOP LDO regulators"
+	depends on MFD_ANATOP
+	help
+	  Say y here to support ANATOP LDOs regulators.
+
 endif
 
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 503bac8..8440871 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -48,5 +48,6 @@ obj-$(CONFIG_REGULATOR_AB8500)	+= ab8500.o
 obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
 obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
 obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
+obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
 
 ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
new file mode 100644
index 0000000..1f3a878
--- /dev/null
+++ b/drivers/regulator/anatop-regulator.c
@@ -0,0 +1,203 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * 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.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/anatop-regulator.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/regulator/of_regulator.h>
+
+static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
+				  int max_uV, unsigned *selector)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	u32 val, sel;
+	int uv;
+
+	uv = min_uV;
+	dev_dbg(&reg->dev, "%s: uv %d, min %d, max %d\n", __func__,
+		uv, anatop_reg->min_voltage,
+		anatop_reg->max_voltage);
+
+	if (uv < anatop_reg->min_voltage) {
+		if (max_uV > anatop_reg->min_voltage)
+			uv = anatop_reg->min_voltage;
+		else
+			return -EINVAL;
+	}
+
+	if (anatop_reg->control_reg) {
+		sel = (uv - anatop_reg->min_voltage) / 25000;
+		if (sel * 25000 + anatop_reg->min_voltage
+		    > anatop_reg->max_voltage)
+			return -EINVAL;
+		val = anatop_reg->min_bit_val + sel;
+		*selector = sel;
+		dev_dbg(&reg->dev, "%s: calculated val %d\n", __func__, val);
+		anatop_set_bits(anatop_reg->mfd,
+				anatop_reg->control_reg,
+				anatop_reg->vol_bit_shift,
+				anatop_reg->vol_bit_size,
+				val);
+		return 0;
+	} else {
+		return -ENOTSUPP;
+	}
+}
+
+static int anatop_get_voltage_sel(struct regulator_dev *reg)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	int selector;
+
+	if (anatop_reg->control_reg) {
+		u32 val = anatop_get_bits(anatop_reg->mfd,
+					  anatop_reg->control_reg,
+					  anatop_reg->vol_bit_shift,
+					  anatop_reg->vol_bit_size);
+		selector = val - anatop_reg->min_bit_val;
+		return selector;
+	} else {
+		return -ENOTSUPP;
+	}
+}
+
+static int anatop_list_voltage(struct regulator_dev *reg, unsigned selector)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	int uv;
+
+	uv = anatop_reg->min_voltage + selector * 25000;
+	dev_dbg(&reg->dev, "vddio = %d, selector = %u\n", uv, selector);
+
+	return uv;
+}
+
+static struct regulator_ops anatop_rops = {
+	.set_voltage     = anatop_set_voltage,
+	.get_voltage_sel = anatop_get_voltage_sel,
+	.list_voltage    = anatop_list_voltage,
+};
+
+int anatop_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct regulator_desc *rdesc;
+	struct regulator_dev *rdev;
+	struct anatop_regulator *sreg;
+	struct regulator_init_data *initdata;
+	struct anatop *anatopmfd = dev_get_drvdata(pdev->dev.parent);
+	const __be32 *rval;
+	u64 ofsize;
+
+	initdata = of_get_regulator_init_data(dev, dev->of_node);
+	sreg = devm_kzalloc(dev, sizeof(struct anatop_regulator), GFP_KERNEL);
+	if (!sreg)
+		return -EINVAL;
+	rdesc = devm_kzalloc(dev, sizeof(struct regulator_desc), GFP_KERNEL);
+	if (!rdesc)
+		return -EINVAL;
+	sreg->initdata = initdata;
+	sreg->regulator = rdesc;
+	memset(rdesc, 0, sizeof(struct regulator_desc));
+	rdesc->name = kstrdup(of_get_property(np, "regulator-name", NULL),
+			      GFP_KERNEL);
+	rdesc->ops = &anatop_rops;
+	rdesc->type = REGULATOR_VOLTAGE;
+	rdesc->owner = THIS_MODULE;
+	sreg->name = rdesc->name;
+	sreg->mfd = anatopmfd;
+	rval = of_get_address(np, 0, &ofsize, NULL);
+	if (rval)
+		sreg->control_reg = be32_to_cpu(*rval);
+	rval = of_get_property(np, "vol-bit-size", NULL);
+	if (rval)
+		sreg->vol_bit_size = be32_to_cpu(*rval);
+	rval = of_get_property(np, "vol-bit-shift", NULL);
+	if (rval)
+		sreg->vol_bit_shift = be32_to_cpu(*rval);
+	rval = of_get_property(np, "min-bit-val", NULL);
+	if (rval)
+		sreg->min_bit_val = be32_to_cpu(*rval);
+	rval = of_get_property(np, "min-voltage", NULL);
+	if (rval)
+		sreg->min_voltage = be32_to_cpu(*rval);
+	rval = of_get_property(np, "max-voltage", NULL);
+	if (rval)
+		sreg->max_voltage = be32_to_cpu(*rval);
+
+	/* register regulator */
+	rdev = regulator_register(rdesc, &pdev->dev,
+				  initdata, sreg, pdev->dev.of_node);
+	platform_set_drvdata(pdev, rdev);
+
+	if (IS_ERR(rdev)) {
+		dev_err(&pdev->dev, "failed to register %s\n",
+			rdesc->name);
+		kfree(rdesc->name);
+		return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+
+int anatop_regulator_remove(struct platform_device *pdev)
+{
+	struct regulator_dev *rdev = platform_get_drvdata(pdev);
+	regulator_unregister(rdev);
+	return 0;
+}
+
+struct of_device_id __devinitdata of_anatop_regulator_match_tbl[] = {
+	{ .compatible = "fsl,anatop-regulator", },
+	{ /* end */ }
+};
+
+struct platform_driver anatop_regulator = {
+	.driver = {
+		.name	= "anatop_regulator",
+		.owner  = THIS_MODULE,
+		.of_match_table = of_anatop_regulator_match_tbl,
+	},
+	.probe	= anatop_regulator_probe,
+	.remove	= anatop_regulator_remove,
+};
+
+int anatop_regulator_init(void)
+{
+	return platform_driver_register(&anatop_regulator);
+}
+postcore_initcall(anatop_regulator_init);
+
+void anatop_regulator_exit(void)
+{
+	platform_driver_unregister(&anatop_regulator);
+}
+module_exit(anatop_regulator_exit);
+
+MODULE_DESCRIPTION("ANATOP Regulator driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/regulator/anatop-regulator.h b/include/linux/regulator/anatop-regulator.h
new file mode 100644
index 0000000..7a9a05b
--- /dev/null
+++ b/include/linux/regulator/anatop-regulator.h
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * 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.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifndef __ANATOP_REGULATOR_H
+#define __ANATOP_REGULATOR_H
+
+#include <linux/regulator/driver.h>
+#include <linux/mfd/anatop.h>
+
+struct anatop_regulator {
+	struct regulator_desc *regulator;
+	struct regulator_init_data *initdata;
+	const char *name;
+	u32 control_reg;
+	struct anatop *mfd;
+	int vol_bit_shift;
+	int vol_bit_size;
+	int min_bit_val;
+	int min_voltage;
+	int max_voltage;
+};
+
+#endif /* __ANATOP_REGULATOR_H */
-- 
1.7.9

Re: [PATCH v4 1/2] mfd: Add anatop mfd driver

[Ying-Chun Liu (PaulLiu)]

(2012年02月21日 19:17), Samuel Ortiz wrote:
> Hi Paul,
> 
> I didn't get patch #2, so I don't get to see how the read/write functions ar
> eused for example.
> 

Hi Samuel,

Sorry for late reply.
I've sent out v5 today.

>> +	ioreg = ioremap(ofaddr, ofsize);
>> +	if (!ioreg)
>> +		return -EINVAL;
>> +	drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);
>> +	if (!drvdata)
>> +		return -EINVAL;
>> +	drvdata->ioreg = ioreg;
>> +	drvdata->read = anatop_read;
>> +	drvdata->write = anatop_write;
>> +	platform_set_drvdata(pdev, drvdata);
>> +	of_platform_bus_probe(np, of_anatop_regulator_match, dev);
>> +	return ret;
>> +}
> So it seems that your driver here does nothing but extending your device tree
> definition. Correct me if I'm wrong, aren't you trying to fix a broken device
> tree definition here ?
> 

The driver here ioremap the addresses for anatop chip in i.MX6Q SoC. The
addresses are shared by several regulators. Also thermal drivers are
also in the same range.

Here are examples of the anatop description in dts file

   anatop@020c8000 {
           compatible = "fsl,imx6q-anatop";
           reg = <0x020c8000 0x1000>;
           interrupts = <0 49 0x04 0 54 0x04 0 127 0x04>;
           #address-cells = <1>;
           #size-cells = <1>;
           reg_vddpu: regulator-vddpu@140 {
                   compatible = "fsl,anatop-regulator";
                   regulator-name = "vddpu";
                   regulator-min-microvolt = <725000>;
                   regulator-max-microvolt = <1300000>;
                   regulator-always-on;
                   reg = <0x140 1>;
                   vol-bit-shift = <9>;
                   vol-bit-size = <5>;
                   min-bit-val = <1>;
                   min-voltage = <725000>;
                   max-voltage = <1300000>;
           };
           reg_vddcore: regulator-vddcore@140 {
                   compatible = "fsl,anatop-regulator";
                   regulator-name = "vddcore";
                   regulator-min-microvolt = <725000>;
                   regulator-max-microvolt = <1300000>;
                   regulator-always-on;
                   reg = <0x140 1>;
                   vol-bit-shift = <0>;
                   vol-bit-size = <5>;
                   min-bit-val = <1>;
                   min-voltage = <725000>;
                   max-voltage = <1300000>;
           };
           ...

So both reg_vddpu and reg_vddcore are using the same address.

Yours Sincerely,
Paul

Re: [PATCH v5 2/2] Regulator: Add Anatop regulator driver

[Mark Brown]

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

On Thu, Mar 01, 2012 at 05:10:52PM +0800, Ying-Chun Liu (PaulLiu) wrote:

> +	if (IS_ERR(rdev)) {
> +		dev_err(&pdev->dev, "failed to register %s\n",
> +			rdesc->name);
> +		kfree(rdesc->name);
> +		return PTR_ERR(rdev);
> +	}
> +
> +	return 0;
> +}
> +
> +int anatop_regulator_remove(struct platform_device *pdev)
> +{
> +	struct regulator_dev *rdev = platform_get_drvdata(pdev);
> +	regulator_unregister(rdev);
> +	return 0;

Looks mostly good but this leaks rdesc->name which was allocated with
kstrdup() in the probe() function.

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

Re: [PATCH v5 1/2] mfd: Add anatop mfd driver

[Mark Brown]

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

On Thu, Mar 01, 2012 at 05:10:51PM +0800, Ying-Chun Liu (PaulLiu) wrote:

> +	spin_lock(&adata->reglock);
> +	val = readl(adata->ioreg + addr);
> +	spin_unlock(&adata->reglock);

Do you really need to take a lock for a single read operation from a
memory mapped register?  I'd expect this to be atomic in itself.  You
need to lock on read/modify/write cycles to make sure that you don't get
a read/read/modify/modify/write/write and misplace one of the modifies
but that's not an issue for an isolated read.

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

[PATCH v6 1/2] mfd: Add anatop mfd driver

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
Anatop provides regulators and thermal.
This driver handles the address space and the operation of the mfd device.

Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/mfd/Kconfig        |    6 ++
 drivers/mfd/Makefile       |    1 +
 drivers/mfd/anatop-mfd.c   |  142 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/anatop.h |   40 ++++++++++++
 4 files changed, 189 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mfd/anatop-mfd.c
 create mode 100644 include/linux/mfd/anatop.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index f147395..f787d17 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -830,6 +830,12 @@ config MFD_INTEL_MSIC
 	  Passage) chip. This chip embeds audio, battery, GPIO, etc.
 	  devices used in Intel Medfield platforms.
 
+config MFD_ANATOP
+        bool "Support for Anatop"
+	depends on SOC_IMX6Q
+	help
+	  Select this option to enable Anatop MFD driver.
+
 endmenu
 endif
 
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b953bab..8e11060 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -112,3 +112,4 @@ obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
 obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
 obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
 obj-$(CONFIG_MFD_S5M_CORE)	+= s5m-core.o s5m-irq.o
+obj-$(CONFIG_MFD_ANATOP)	+= anatop-mfd.o
diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
new file mode 100644
index 0000000..0307051
--- /dev/null
+++ b/drivers/mfd/anatop-mfd.c
@@ -0,0 +1,142 @@
+/*
+ * Anatop MFD driver
+ *
+ * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
+ * Copyright (C) 2012 Linaro
+ *
+ *  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.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *  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.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/mfd/anatop.h>
+
+u32 anatop_get_bits(struct anatop *adata, u32 addr, int bit_shift, int bits)
+{
+	u32 val;
+	int mask;
+
+	if (bits == 32)
+		mask = ~0;
+	else
+		mask = (1 << bits) - 1;
+
+	val = readl(adata->ioreg + addr);
+	val = (val >> bit_shift) & mask;
+
+	return val;
+}
+EXPORT_SYMBOL(anatop_get_bits);
+
+void anatop_set_bits(struct anatop *adata, u32 addr, int bit_shift,
+		     int bits, u32 data)
+{
+	u32 val;
+	int mask;
+	if (bits == 32)
+		mask = ~0;
+	else
+		mask = (1 << bits) - 1;
+
+	spin_lock(&adata->reglock);
+	val = readl(adata->ioreg + addr) & ~(mask << bit_shift);
+	writel((data << bit_shift) | val, adata->ioreg);
+	spin_unlock(&adata->reglock);
+}
+EXPORT_SYMBOL(anatop_set_bits);
+
+static const struct of_device_id of_anatop_subdevice_match[] = {
+	{ .compatible = "fsl,anatop-regulator", },
+	{ .compatible = "fsl,anatop-thermal", },
+	{ },
+};
+
+static int of_anatop_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	void *ioreg;
+	struct anatop *drvdata;
+
+	ioreg = of_iomap(np, 0);
+	if (!ioreg)
+		return -EINVAL;
+	drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);
+	if (!drvdata)
+		return -EINVAL;
+	drvdata->ioreg = ioreg;
+	spin_lock_init(&drvdata->reglock);
+	platform_set_drvdata(pdev, drvdata);
+	of_platform_bus_probe(np, of_anatop_subdevice_match, dev);
+
+	return 0;
+}
+
+static int of_anatop_remove(struct platform_device *pdev)
+{
+	struct anatop *drvdata;
+	drvdata = platform_get_drvdata(pdev);
+	iounmap(drvdata->ioreg);
+	return 0;
+}
+
+static const struct of_device_id of_anatop_match[] = {
+	{ .compatible = "fsl,imx6q-anatop", },
+	{ },
+};
+
+static struct platform_driver anatop_of_driver = {
+	.driver = {
+		.name = "anatop-mfd",
+		.owner = THIS_MODULE,
+		.of_match_table = of_anatop_match,
+	},
+	.probe		= of_anatop_probe,
+	.remove		= of_anatop_remove,
+};
+
+static int __init anatop_init(void)
+{
+	return platform_driver_register(&anatop_of_driver);
+}
+postcore_initcall(anatop_init);
+
+static void __exit anatop_exit(void)
+{
+	platform_driver_unregister(&anatop_of_driver);
+}
+module_exit(anatop_exit);
+
+MODULE_AUTHOR("Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>");
+MODULE_DESCRIPTION("ANATOP MFD driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/anatop.h b/include/linux/mfd/anatop.h
new file mode 100644
index 0000000..22c1007
--- /dev/null
+++ b/include/linux/mfd/anatop.h
@@ -0,0 +1,40 @@
+/*
+ * anatop.h - Anatop MFD driver
+ *
+ *  Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
+ *  Copyright (C) 2012 Linaro
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef __LINUX_MFD_ANATOP_H
+#define __LINUX_MFD_ANATOP_H
+
+#include <linux/spinlock.h>
+
+/**
+ * anatop - MFD data
+ * @ioreg: ioremap register
+ * @reglock: spinlock for register read/write
+ */
+struct anatop {
+	void *ioreg;
+	spinlock_t reglock;
+};
+
+extern u32 anatop_get_bits(struct anatop *, u32, int, int);
+extern void anatop_set_bits(struct anatop *, u32, int, int, u32);
+
+#endif /*  __LINUX_MFD_ANATOP_H */
-- 
1.7.9.1

[PATCH v6 2/2] Regulator: Add Anatop regulator driver

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Anatop is an integrated regulator inside i.MX6 SoC.
There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
This patch adds the Anatop regulator driver.

Signed-off-by: Nancy Chen <Nancy.Chen@freescale.com>
Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
---
 .../bindings/regulator/anatop-regulator.txt        |   28 +++
 drivers/regulator/Kconfig                          |    6 +
 drivers/regulator/Makefile                         |    1 +
 drivers/regulator/anatop-regulator.c               |  205 ++++++++++++++++++++
 include/linux/regulator/anatop-regulator.h         |   40 ++++
 5 files changed, 280 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/regulator/anatop-regulator.txt
 create mode 100644 drivers/regulator/anatop-regulator.c
 create mode 100644 include/linux/regulator/anatop-regulator.h

diff --git a/Documentation/devicetree/bindings/regulator/anatop-regulator.txt b/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
new file mode 100644
index 0000000..f19cfea
--- /dev/null
+++ b/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
@@ -0,0 +1,28 @@
+Anatop Voltage regulators
+
+Required properties:
+- compatible: Must be "fsl,anatop-regulator"
+- vol-bit-shift: Bit shift for the register
+- vol-bit-size: Number of bits used in the register
+- min-bit-val: Minimum value of this register
+- min-voltage: Minimum voltage of this regulator
+- max-voltage: Maximum voltage of this regulator
+
+Any property defined as part of the core regulator
+binding, defined in regulator.txt, can also be used.
+
+Example:
+
+	reg_vddpu: regulator-vddpu@140 {
+		compatible = "fsl,anatop-regulator";
+		regulator-name = "vddpu";
+		regulator-min-microvolt = <725000>;
+		regulator-max-microvolt = <1300000>;
+		regulator-always-on;
+		reg = <0x140 1>;
+		vol-bit-shift = <9>;
+		vol-bit-size = <5>;
+		min-bit-val = <1>;
+		min-voltage = <725000>;
+		max-voltage = <1300000>;
+	};
diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index 7a61b17..5fbcda2 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -335,5 +335,11 @@ config REGULATOR_AAT2870
 	  If you have a AnalogicTech AAT2870 say Y to enable the
 	  regulator driver.
 
+config REGULATOR_ANATOP
+	tristate "ANATOP LDO regulators"
+	depends on MFD_ANATOP
+	help
+	  Say y here to support ANATOP LDOs regulators.
+
 endif
 
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index 503bac8..8440871 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -48,5 +48,6 @@ obj-$(CONFIG_REGULATOR_AB8500)	+= ab8500.o
 obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
 obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
 obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
+obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
 
 ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
new file mode 100644
index 0000000..84d8f50
--- /dev/null
+++ b/drivers/regulator/anatop-regulator.c
@@ -0,0 +1,205 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * 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.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#include <linux/slab.h>
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/anatop-regulator.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/regulator/of_regulator.h>
+
+static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
+				  int max_uV, unsigned *selector)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	u32 val, sel;
+	int uv;
+
+	uv = min_uV;
+	dev_dbg(&reg->dev, "%s: uv %d, min %d, max %d\n", __func__,
+		uv, anatop_reg->min_voltage,
+		anatop_reg->max_voltage);
+
+	if (uv < anatop_reg->min_voltage) {
+		if (max_uV > anatop_reg->min_voltage)
+			uv = anatop_reg->min_voltage;
+		else
+			return -EINVAL;
+	}
+
+	if (anatop_reg->control_reg) {
+		sel = (uv - anatop_reg->min_voltage) / 25000;
+		if (sel * 25000 + anatop_reg->min_voltage
+		    > anatop_reg->max_voltage)
+			return -EINVAL;
+		val = anatop_reg->min_bit_val + sel;
+		*selector = sel;
+		dev_dbg(&reg->dev, "%s: calculated val %d\n", __func__, val);
+		anatop_set_bits(anatop_reg->mfd,
+				anatop_reg->control_reg,
+				anatop_reg->vol_bit_shift,
+				anatop_reg->vol_bit_size,
+				val);
+		return 0;
+	} else {
+		return -ENOTSUPP;
+	}
+}
+
+static int anatop_get_voltage_sel(struct regulator_dev *reg)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	int selector;
+
+	if (anatop_reg->control_reg) {
+		u32 val = anatop_get_bits(anatop_reg->mfd,
+					  anatop_reg->control_reg,
+					  anatop_reg->vol_bit_shift,
+					  anatop_reg->vol_bit_size);
+		selector = val - anatop_reg->min_bit_val;
+		return selector;
+	} else {
+		return -ENOTSUPP;
+	}
+}
+
+static int anatop_list_voltage(struct regulator_dev *reg, unsigned selector)
+{
+	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
+	int uv;
+
+	uv = anatop_reg->min_voltage + selector * 25000;
+	dev_dbg(&reg->dev, "vddio = %d, selector = %u\n", uv, selector);
+
+	return uv;
+}
+
+static struct regulator_ops anatop_rops = {
+	.set_voltage     = anatop_set_voltage,
+	.get_voltage_sel = anatop_get_voltage_sel,
+	.list_voltage    = anatop_list_voltage,
+};
+
+int anatop_regulator_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct regulator_desc *rdesc;
+	struct regulator_dev *rdev;
+	struct anatop_regulator *sreg;
+	struct regulator_init_data *initdata;
+	struct anatop *anatopmfd = dev_get_drvdata(pdev->dev.parent);
+	const __be32 *rval;
+	u64 ofsize;
+
+	initdata = of_get_regulator_init_data(dev, dev->of_node);
+	sreg = devm_kzalloc(dev, sizeof(struct anatop_regulator), GFP_KERNEL);
+	if (!sreg)
+		return -EINVAL;
+	rdesc = devm_kzalloc(dev, sizeof(struct regulator_desc), GFP_KERNEL);
+	if (!rdesc)
+		return -EINVAL;
+	sreg->initdata = initdata;
+	sreg->name = kstrdup(of_get_property(np, "regulator-name", NULL),
+			     GFP_KERNEL);
+	sreg->regulator = rdesc;
+	memset(rdesc, 0, sizeof(struct regulator_desc));
+	rdesc->name = sreg->name;
+	rdesc->ops = &anatop_rops;
+	rdesc->type = REGULATOR_VOLTAGE;
+	rdesc->owner = THIS_MODULE;
+	sreg->mfd = anatopmfd;
+	rval = of_get_address(np, 0, &ofsize, NULL);
+	if (rval)
+		sreg->control_reg = be32_to_cpu(*rval);
+	rval = of_get_property(np, "vol-bit-size", NULL);
+	if (rval)
+		sreg->vol_bit_size = be32_to_cpu(*rval);
+	rval = of_get_property(np, "vol-bit-shift", NULL);
+	if (rval)
+		sreg->vol_bit_shift = be32_to_cpu(*rval);
+	rval = of_get_property(np, "min-bit-val", NULL);
+	if (rval)
+		sreg->min_bit_val = be32_to_cpu(*rval);
+	rval = of_get_property(np, "min-voltage", NULL);
+	if (rval)
+		sreg->min_voltage = be32_to_cpu(*rval);
+	rval = of_get_property(np, "max-voltage", NULL);
+	if (rval)
+		sreg->max_voltage = be32_to_cpu(*rval);
+
+	/* register regulator */
+	rdev = regulator_register(rdesc, &pdev->dev,
+				  initdata, sreg, pdev->dev.of_node);
+	platform_set_drvdata(pdev, rdev);
+
+	if (IS_ERR(rdev)) {
+		dev_err(&pdev->dev, "failed to register %s\n",
+			rdesc->name);
+		kfree(sreg->name);
+		return PTR_ERR(rdev);
+	}
+
+	return 0;
+}
+
+int anatop_regulator_remove(struct platform_device *pdev)
+{
+	struct regulator_dev *rdev = platform_get_drvdata(pdev);
+	struct anatop_regulator *sreg = rdev_get_drvdata(rdev);
+	kfree(sreg->name);
+	regulator_unregister(rdev);
+	return 0;
+}
+
+struct of_device_id __devinitdata of_anatop_regulator_match_tbl[] = {
+	{ .compatible = "fsl,anatop-regulator", },
+	{ /* end */ }
+};
+
+struct platform_driver anatop_regulator = {
+	.driver = {
+		.name	= "anatop_regulator",
+		.owner  = THIS_MODULE,
+		.of_match_table = of_anatop_regulator_match_tbl,
+	},
+	.probe	= anatop_regulator_probe,
+	.remove	= anatop_regulator_remove,
+};
+
+int anatop_regulator_init(void)
+{
+	return platform_driver_register(&anatop_regulator);
+}
+postcore_initcall(anatop_regulator_init);
+
+void anatop_regulator_exit(void)
+{
+	platform_driver_unregister(&anatop_regulator);
+}
+module_exit(anatop_regulator_exit);
+
+MODULE_DESCRIPTION("ANATOP Regulator driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/regulator/anatop-regulator.h b/include/linux/regulator/anatop-regulator.h
new file mode 100644
index 0000000..7a9a05b
--- /dev/null
+++ b/include/linux/regulator/anatop-regulator.h
@@ -0,0 +1,40 @@
+/*
+ * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
+ */
+
+/*
+ * 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.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ */
+
+#ifndef __ANATOP_REGULATOR_H
+#define __ANATOP_REGULATOR_H
+
+#include <linux/regulator/driver.h>
+#include <linux/mfd/anatop.h>
+
+struct anatop_regulator {
+	struct regulator_desc *regulator;
+	struct regulator_init_data *initdata;
+	const char *name;
+	u32 control_reg;
+	struct anatop *mfd;
+	int vol_bit_shift;
+	int vol_bit_size;
+	int min_bit_val;
+	int min_voltage;
+	int max_voltage;
+};
+
+#endif /* __ANATOP_REGULATOR_H */
-- 
1.7.9.1

RE: [PATCH v6 1/2] mfd: Add anatop mfd driver

[Venu Byravarasu]

Why you did not made use of regmap?

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Ying-Chun Liu (PaulLiu)
> Sent: Friday, March 02, 2012 12:31 PM
> To: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org; linaro-dev@lists.linaro.org;
> patches@linaro.org; Ying-Chun Liu (PaulLiu); Samuel Ortiz; Mark Brown; Shawn
> Guo
> Subject: [PATCH v6 1/2] mfd: Add anatop mfd driver
> 
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> 
> Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
> Anatop provides regulators and thermal.
> This driver handles the address space and the operation of the mfd device.
> 
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/mfd/Kconfig        |    6 ++
>  drivers/mfd/Makefile       |    1 +
>  drivers/mfd/anatop-mfd.c   |  142
> ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/anatop.h |   40 ++++++++++++
>  4 files changed, 189 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/mfd/anatop-mfd.c
>  create mode 100644 include/linux/mfd/anatop.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index f147395..f787d17 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -830,6 +830,12 @@ config MFD_INTEL_MSIC
>  	  Passage) chip. This chip embeds audio, battery, GPIO, etc.
>  	  devices used in Intel Medfield platforms.
> 
> +config MFD_ANATOP
> +        bool "Support for Anatop"
> +	depends on SOC_IMX6Q
> +	help
> +	  Select this option to enable Anatop MFD driver.
> +
>  endmenu
>  endif
> 
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index b953bab..8e11060 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -112,3 +112,4 @@ obj-$(CONFIG_TPS65911_COMPARATOR)	+=
> tps65911-comparator.o
>  obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
>  obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
>  obj-$(CONFIG_MFD_S5M_CORE)	+= s5m-core.o s5m-irq.o
> +obj-$(CONFIG_MFD_ANATOP)	+= anatop-mfd.o
> diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
> new file mode 100644
> index 0000000..0307051
> --- /dev/null
> +++ b/drivers/mfd/anatop-mfd.c
> @@ -0,0 +1,142 @@
> +/*
> + * Anatop MFD driver
> + *
> + * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> + * Copyright (C) 2012 Linaro
> + *
> + *  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.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *  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.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + */
> +
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/mfd/anatop.h>
> +
> +u32 anatop_get_bits(struct anatop *adata, u32 addr, int bit_shift, int bits)
> +{
> +	u32 val;
> +	int mask;
> +
> +	if (bits == 32)
> +		mask = ~0;
> +	else
> +		mask = (1 << bits) - 1;
> +
> +	val = readl(adata->ioreg + addr);
> +	val = (val >> bit_shift) & mask;
> +
> +	return val;
> +}
> +EXPORT_SYMBOL(anatop_get_bits);
> +
> +void anatop_set_bits(struct anatop *adata, u32 addr, int bit_shift,
> +		     int bits, u32 data)
> +{
> +	u32 val;
> +	int mask;
> +	if (bits == 32)
> +		mask = ~0;
> +	else
> +		mask = (1 << bits) - 1;
> +
> +	spin_lock(&adata->reglock);
> +	val = readl(adata->ioreg + addr) & ~(mask << bit_shift);
> +	writel((data << bit_shift) | val, adata->ioreg);
> +	spin_unlock(&adata->reglock);
> +}
> +EXPORT_SYMBOL(anatop_set_bits);
> +
> +static const struct of_device_id of_anatop_subdevice_match[] = {
> +	{ .compatible = "fsl,anatop-regulator", },
> +	{ .compatible = "fsl,anatop-thermal", },
> +	{ },
> +};
> +
> +static int of_anatop_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	void *ioreg;
> +	struct anatop *drvdata;
> +
> +	ioreg = of_iomap(np, 0);
> +	if (!ioreg)
> +		return -EINVAL;
> +	drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);
> +	if (!drvdata)
> +		return -EINVAL;
> +	drvdata->ioreg = ioreg;
> +	spin_lock_init(&drvdata->reglock);
> +	platform_set_drvdata(pdev, drvdata);
> +	of_platform_bus_probe(np, of_anatop_subdevice_match, dev);
> +
> +	return 0;
> +}
> +
> +static int of_anatop_remove(struct platform_device *pdev)
> +{
> +	struct anatop *drvdata;
> +	drvdata = platform_get_drvdata(pdev);
> +	iounmap(drvdata->ioreg);
> +	return 0;
> +}
> +
> +static const struct of_device_id of_anatop_match[] = {
> +	{ .compatible = "fsl,imx6q-anatop", },
> +	{ },
> +};
> +
> +static struct platform_driver anatop_of_driver = {
> +	.driver = {
> +		.name = "anatop-mfd",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_anatop_match,
> +	},
> +	.probe		= of_anatop_probe,
> +	.remove		= of_anatop_remove,
> +};
> +
> +static int __init anatop_init(void)
> +{
> +	return platform_driver_register(&anatop_of_driver);
> +}
> +postcore_initcall(anatop_init);
> +
> +static void __exit anatop_exit(void)
> +{
> +	platform_driver_unregister(&anatop_of_driver);
> +}
> +module_exit(anatop_exit);
> +
> +MODULE_AUTHOR("Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>");
> +MODULE_DESCRIPTION("ANATOP MFD driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/anatop.h b/include/linux/mfd/anatop.h
> new file mode 100644
> index 0000000..22c1007
> --- /dev/null
> +++ b/include/linux/mfd/anatop.h
> @@ -0,0 +1,40 @@
> +/*
> + * anatop.h - Anatop MFD driver
> + *
> + *  Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> + *  Copyright (C) 2012 Linaro
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#ifndef __LINUX_MFD_ANATOP_H
> +#define __LINUX_MFD_ANATOP_H
> +
> +#include <linux/spinlock.h>
> +
> +/**
> + * anatop - MFD data
> + * @ioreg: ioremap register
> + * @reglock: spinlock for register read/write
> + */
> +struct anatop {
> +	void *ioreg;
> +	spinlock_t reglock;
> +};
> +
> +extern u32 anatop_get_bits(struct anatop *, u32, int, int);
> +extern void anatop_set_bits(struct anatop *, u32, int, int, u32);
> +
> +#endif /*  __LINUX_MFD_ANATOP_H */
> --
> 1.7.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 1/2] mfd: Add anatop mfd driver

[Ying-Chun Liu (PaulLiu)]

(2012年03月02日 15:07), Venu Byravarasu wrote:
> Why you did not made use of regmap?
> 

Hi Venu,

I cannot fully understand. Anatop is not SPI or I2C device. It is
internally embedded into the SoC. It is directly accessed by memory I/O.

Do you mean I should implement a regmap_bus which uses memory I/O and
then use regmap?

Regards,
Paul

>> -----Original Message-----
>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
>> owner@vger.kernel.org] On Behalf Of Ying-Chun Liu (PaulLiu)
>> Sent: Friday, March 02, 2012 12:31 PM
>> To: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org; linaro-dev@lists.linaro.org;
>> patches@linaro.org; Ying-Chun Liu (PaulLiu); Samuel Ortiz; Mark Brown; Shawn
>> Guo
>> Subject: [PATCH v6 1/2] mfd: Add anatop mfd driver
>>
>> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
>>
>> Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
>> Anatop provides regulators and thermal.
>> This driver handles the address space and the operation of the mfd device.
>>
>> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
>> Cc: Samuel Ortiz <sameo@linux.intel.com>
>> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
>> Cc: Shawn Guo <shawn.guo@linaro.org>
>> ---
>>  drivers/mfd/Kconfig        |    6 ++

RE: [PATCH v6 1/2] mfd: Add anatop mfd driver

[Venu Byravarasu]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 439 bytes --]

> I cannot fully understand. Anatop is not SPI or I2C device. It is
> internally embedded into the SoC. It is directly accessed by memory I/O.

Sorry, I missed this.

> Do you mean I should implement a regmap_bus which uses memory I/O and
> then use regmap?
> 
> Regards,
> Paul
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v6 1/2] mfd: Add anatop mfd driver

[Peter Korsgaard]

>>>>> "Y" == Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org> writes:

Hi,

 Y> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
 Y> Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
 Y> Anatop provides regulators and thermal.
 Y> This driver handles the address space and the operation of the mfd device.

 Y> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
 Y> Cc: Samuel Ortiz <sameo@linux.intel.com>
 Y> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
 Y> Cc: Shawn Guo <shawn.guo@linaro.org>
 Y> ---
 Y>  drivers/mfd/Kconfig        |    6 ++
 Y>  drivers/mfd/Makefile       |    1 +
 Y>  drivers/mfd/anatop-mfd.c   |  142 ++++++++++++++++++++++++++++++++++++++++++++
 Y>  include/linux/mfd/anatop.h |   40 ++++++++++++
 Y>  4 files changed, 189 insertions(+), 0 deletions(-)
 Y>  create mode 100644 drivers/mfd/anatop-mfd.c
 Y>  create mode 100644 include/linux/mfd/anatop.h

 Y> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
 Y> index f147395..f787d17 100644
 Y> --- a/drivers/mfd/Kconfig
 Y> +++ b/drivers/mfd/Kconfig
 Y> @@ -830,6 +830,12 @@ config MFD_INTEL_MSIC
 Y>  	  Passage) chip. This chip embeds audio, battery, GPIO, etc.
 Y>  	  devices used in Intel Medfield platforms.
 
 Y> +config MFD_ANATOP
 Y> +        bool "Support for Anatop"
 Y> +	depends on SOC_IMX6Q

The bool line should be indented with a <tab>, not spaces.

-- 
Bye, Peter Korsgaard

[PATCH v7 1/2] mfd: Add anatop mfd driver

[Ying-Chun Liu (PaulLiu)]

From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>

Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
Anatop provides regulators and thermal.
This driver handles the address space and the operation of the mfd device.

Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Shawn Guo <shawn.guo@linaro.org>
Cc: Venu Byravarasu <vbyravarasu@nvidia.com>
Cc: Peter Korsgaard <jacmet@sunsite.dk>
---
 drivers/mfd/Kconfig        |    6 ++
 drivers/mfd/Makefile       |    1 +
 drivers/mfd/anatop-mfd.c   |  142 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/anatop.h |   40 ++++++++++++
 4 files changed, 189 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mfd/anatop-mfd.c
 create mode 100644 include/linux/mfd/anatop.h

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index f147395..78593e8 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -830,6 +830,12 @@ config MFD_INTEL_MSIC
 	  Passage) chip. This chip embeds audio, battery, GPIO, etc.
 	  devices used in Intel Medfield platforms.
 
+config MFD_ANATOP
+	bool "Support for Anatop"
+	depends on SOC_IMX6Q
+	help
+	  Select this option to enable Anatop MFD driver.
+
 endmenu
 endif
 
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index b953bab..8e11060 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -112,3 +112,4 @@ obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
 obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
 obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
 obj-$(CONFIG_MFD_S5M_CORE)	+= s5m-core.o s5m-irq.o
+obj-$(CONFIG_MFD_ANATOP)	+= anatop-mfd.o
diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
new file mode 100644
index 0000000..0307051
--- /dev/null
+++ b/drivers/mfd/anatop-mfd.c
@@ -0,0 +1,142 @@
+/*
+ * Anatop MFD driver
+ *
+ * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
+ * Copyright (C) 2012 Linaro
+ *
+ *  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.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *  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.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License along
+ *  with this program; if not, write to the Free Software Foundation, Inc.,
+ *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ */
+
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/of_address.h>
+#include <linux/mfd/anatop.h>
+
+u32 anatop_get_bits(struct anatop *adata, u32 addr, int bit_shift, int bits)
+{
+	u32 val;
+	int mask;
+
+	if (bits == 32)
+		mask = ~0;
+	else
+		mask = (1 << bits) - 1;
+
+	val = readl(adata->ioreg + addr);
+	val = (val >> bit_shift) & mask;
+
+	return val;
+}
+EXPORT_SYMBOL(anatop_get_bits);
+
+void anatop_set_bits(struct anatop *adata, u32 addr, int bit_shift,
+		     int bits, u32 data)
+{
+	u32 val;
+	int mask;
+	if (bits == 32)
+		mask = ~0;
+	else
+		mask = (1 << bits) - 1;
+
+	spin_lock(&adata->reglock);
+	val = readl(adata->ioreg + addr) & ~(mask << bit_shift);
+	writel((data << bit_shift) | val, adata->ioreg);
+	spin_unlock(&adata->reglock);
+}
+EXPORT_SYMBOL(anatop_set_bits);
+
+static const struct of_device_id of_anatop_subdevice_match[] = {
+	{ .compatible = "fsl,anatop-regulator", },
+	{ .compatible = "fsl,anatop-thermal", },
+	{ },
+};
+
+static int of_anatop_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	void *ioreg;
+	struct anatop *drvdata;
+
+	ioreg = of_iomap(np, 0);
+	if (!ioreg)
+		return -EINVAL;
+	drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);
+	if (!drvdata)
+		return -EINVAL;
+	drvdata->ioreg = ioreg;
+	spin_lock_init(&drvdata->reglock);
+	platform_set_drvdata(pdev, drvdata);
+	of_platform_bus_probe(np, of_anatop_subdevice_match, dev);
+
+	return 0;
+}
+
+static int of_anatop_remove(struct platform_device *pdev)
+{
+	struct anatop *drvdata;
+	drvdata = platform_get_drvdata(pdev);
+	iounmap(drvdata->ioreg);
+	return 0;
+}
+
+static const struct of_device_id of_anatop_match[] = {
+	{ .compatible = "fsl,imx6q-anatop", },
+	{ },
+};
+
+static struct platform_driver anatop_of_driver = {
+	.driver = {
+		.name = "anatop-mfd",
+		.owner = THIS_MODULE,
+		.of_match_table = of_anatop_match,
+	},
+	.probe		= of_anatop_probe,
+	.remove		= of_anatop_remove,
+};
+
+static int __init anatop_init(void)
+{
+	return platform_driver_register(&anatop_of_driver);
+}
+postcore_initcall(anatop_init);
+
+static void __exit anatop_exit(void)
+{
+	platform_driver_unregister(&anatop_of_driver);
+}
+module_exit(anatop_exit);
+
+MODULE_AUTHOR("Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>");
+MODULE_DESCRIPTION("ANATOP MFD driver");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mfd/anatop.h b/include/linux/mfd/anatop.h
new file mode 100644
index 0000000..22c1007
--- /dev/null
+++ b/include/linux/mfd/anatop.h
@@ -0,0 +1,40 @@
+/*
+ * anatop.h - Anatop MFD driver
+ *
+ *  Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
+ *  Copyright (C) 2012 Linaro
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+#ifndef __LINUX_MFD_ANATOP_H
+#define __LINUX_MFD_ANATOP_H
+
+#include <linux/spinlock.h>
+
+/**
+ * anatop - MFD data
+ * @ioreg: ioremap register
+ * @reglock: spinlock for register read/write
+ */
+struct anatop {
+	void *ioreg;
+	spinlock_t reglock;
+};
+
+extern u32 anatop_get_bits(struct anatop *, u32, int, int);
+extern void anatop_set_bits(struct anatop *, u32, int, int, u32);
+
+#endif /*  __LINUX_MFD_ANATOP_H */
-- 
1.7.9.1

Re: [PATCH v7 1/2] mfd: Add anatop mfd driver

[Mark Brown]

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

On Sun, Mar 04, 2012 at 01:39:12AM +0800, Ying-Chun Liu (PaulLiu) wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> 
> Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
> Anatop provides regulators and thermal.
> This driver handles the address space and the operation of the mfd device.

Please stop sending new patches as followups to old versions, it tends
not to do the right thing in mail software.

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

Re: [PATCH v7 1/2] mfd: Add anatop mfd driver

[Shawn Guo]

On Sun, Mar 04, 2012 at 01:39:12AM +0800, Ying-Chun Liu (PaulLiu) wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> 
> Anatop is a mfd chip embedded in Freescale i.MX6Q SoC.
> Anatop provides regulators and thermal.
> This driver handles the address space and the operation of the mfd device.
> 
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>

A few trivial comments below, otherwise

Acked-by: Shawn Guo <shawn.guo@linaro.org>

> Cc: Venu Byravarasu <vbyravarasu@nvidia.com>
> Cc: Peter Korsgaard <jacmet@sunsite.dk>
> ---
>  drivers/mfd/Kconfig        |    6 ++
>  drivers/mfd/Makefile       |    1 +
>  drivers/mfd/anatop-mfd.c   |  142 ++++++++++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/anatop.h |   40 ++++++++++++
>  4 files changed, 189 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/mfd/anatop-mfd.c
>  create mode 100644 include/linux/mfd/anatop.h
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index f147395..78593e8 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -830,6 +830,12 @@ config MFD_INTEL_MSIC
>  	  Passage) chip. This chip embeds audio, battery, GPIO, etc.
>  	  devices used in Intel Medfield platforms.
>  
> +config MFD_ANATOP
> +	bool "Support for Anatop"

It might worth a more descriptive prompt, something like
"Support Freescale i.MX on-chip ANATOP controller"?

> +	depends on SOC_IMX6Q
> +	help
> +	  Select this option to enable Anatop MFD driver.

Ditto, a more descriptive help?

> +
>  endmenu
>  endif
>  
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index b953bab..8e11060 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -112,3 +112,4 @@ obj-$(CONFIG_TPS65911_COMPARATOR)	+= tps65911-comparator.o
>  obj-$(CONFIG_MFD_AAT2870_CORE)	+= aat2870-core.o
>  obj-$(CONFIG_MFD_INTEL_MSIC)	+= intel_msic.o
>  obj-$(CONFIG_MFD_S5M_CORE)	+= s5m-core.o s5m-irq.o
> +obj-$(CONFIG_MFD_ANATOP)	+= anatop-mfd.o
> diff --git a/drivers/mfd/anatop-mfd.c b/drivers/mfd/anatop-mfd.c
> new file mode 100644
> index 0000000..0307051
> --- /dev/null
> +++ b/drivers/mfd/anatop-mfd.c
> @@ -0,0 +1,142 @@
> +/*
> + * Anatop MFD driver
> + *
> + * Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> + * Copyright (C) 2012 Linaro
> + *
> + *  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.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *  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.
> + *
> + *  This program is distributed in the hope that it will be useful,
> + *  but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *  GNU General Public License for more details.
> + *
> + *  You should have received a copy of the GNU General Public License along
> + *  with this program; if not, write to the Free Software Foundation, Inc.,
> + *  51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + *
> + */
> +
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_address.h>
> +#include <linux/mfd/anatop.h>
> +
> +u32 anatop_get_bits(struct anatop *adata, u32 addr, int bit_shift, int bits)
> +{
> +	u32 val;
> +	int mask;

Shouldn't mask be also u32?  Then "u32 val, mask;"?
> +
> +	if (bits == 32)
> +		mask = ~0;
> +	else
> +		mask = (1 << bits) - 1;
> +
> +	val = readl(adata->ioreg + addr);
> +	val = (val >> bit_shift) & mask;
> +
> +	return val;
> +}
> +EXPORT_SYMBOL(anatop_get_bits);
> +
> +void anatop_set_bits(struct anatop *adata, u32 addr, int bit_shift,
> +		     int bits, u32 data)
> +{
> +	u32 val;
> +	int mask;

Ditto, with a blank line to be consistent with above function.

> +	if (bits == 32)
> +		mask = ~0;
> +	else
> +		mask = (1 << bits) - 1;
> +
> +	spin_lock(&adata->reglock);
> +	val = readl(adata->ioreg + addr) & ~(mask << bit_shift);
> +	writel((data << bit_shift) | val, adata->ioreg);
> +	spin_unlock(&adata->reglock);
> +}
> +EXPORT_SYMBOL(anatop_set_bits);
> +
> +static const struct of_device_id of_anatop_subdevice_match[] = {
> +	{ .compatible = "fsl,anatop-regulator", },
> +	{ .compatible = "fsl,anatop-thermal", },
> +	{ },
> +};
> +
> +static int of_anatop_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	void *ioreg;
> +	struct anatop *drvdata;
> +
> +	ioreg = of_iomap(np, 0);
> +	if (!ioreg)
> +		return -EINVAL;

return -EADDRNOTAVAIL;

> +	drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);
> +	if (!drvdata)
> +		return -EINVAL;

return -ENOMEM;

The bonus point is we can know the failure cause from error number,
with no need of error message.

Regards,
Shawn

> +	drvdata->ioreg = ioreg;
> +	spin_lock_init(&drvdata->reglock);
> +	platform_set_drvdata(pdev, drvdata);
> +	of_platform_bus_probe(np, of_anatop_subdevice_match, dev);
> +
> +	return 0;
> +}
> +
> +static int of_anatop_remove(struct platform_device *pdev)
> +{
> +	struct anatop *drvdata;
> +	drvdata = platform_get_drvdata(pdev);
> +	iounmap(drvdata->ioreg);
> +	return 0;
> +}
> +
> +static const struct of_device_id of_anatop_match[] = {
> +	{ .compatible = "fsl,imx6q-anatop", },
> +	{ },
> +};
> +
> +static struct platform_driver anatop_of_driver = {
> +	.driver = {
> +		.name = "anatop-mfd",
> +		.owner = THIS_MODULE,
> +		.of_match_table = of_anatop_match,
> +	},
> +	.probe		= of_anatop_probe,
> +	.remove		= of_anatop_remove,
> +};
> +
> +static int __init anatop_init(void)
> +{
> +	return platform_driver_register(&anatop_of_driver);
> +}
> +postcore_initcall(anatop_init);
> +
> +static void __exit anatop_exit(void)
> +{
> +	platform_driver_unregister(&anatop_of_driver);
> +}
> +module_exit(anatop_exit);
> +
> +MODULE_AUTHOR("Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>");
> +MODULE_DESCRIPTION("ANATOP MFD driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/anatop.h b/include/linux/mfd/anatop.h
> new file mode 100644
> index 0000000..22c1007
> --- /dev/null
> +++ b/include/linux/mfd/anatop.h
> @@ -0,0 +1,40 @@
> +/*
> + * anatop.h - Anatop MFD driver
> + *
> + *  Copyright (C) 2012 Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> + *  Copyright (C) 2012 Linaro
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
> + */
> +
> +#ifndef __LINUX_MFD_ANATOP_H
> +#define __LINUX_MFD_ANATOP_H
> +
> +#include <linux/spinlock.h>
> +
> +/**
> + * anatop - MFD data
> + * @ioreg: ioremap register
> + * @reglock: spinlock for register read/write
> + */
> +struct anatop {
> +	void *ioreg;
> +	spinlock_t reglock;
> +};
> +
> +extern u32 anatop_get_bits(struct anatop *, u32, int, int);
> +extern void anatop_set_bits(struct anatop *, u32, int, int, u32);
> +
> +#endif /*  __LINUX_MFD_ANATOP_H */
> -- 
> 1.7.9.1
> 

Re: [PATCH v7 1/2] mfd: Add anatop mfd driver

[Shawn Guo]

On Sun, Mar 04, 2012 at 01:39:12AM +0800, Ying-Chun Liu (PaulLiu) wrote:
...
> +static int of_anatop_probe(struct platform_device *pdev)

__devinit

> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	void *ioreg;
> +	struct anatop *drvdata;
> +
> +	ioreg = of_iomap(np, 0);
> +	if (!ioreg)
> +		return -EINVAL;
> +	drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);
> +	if (!drvdata)
> +		return -EINVAL;
> +	drvdata->ioreg = ioreg;
> +	spin_lock_init(&drvdata->reglock);
> +	platform_set_drvdata(pdev, drvdata);
> +	of_platform_bus_probe(np, of_anatop_subdevice_match, dev);
> +
> +	return 0;
> +}
> +
> +static int of_anatop_remove(struct platform_device *pdev)

__devexit

> +{
> +	struct anatop *drvdata;
> +	drvdata = platform_get_drvdata(pdev);
> +	iounmap(drvdata->ioreg);
> +	return 0;
> +}
> +

-- 
Regards,
Shawn

Re: [PATCH v7 1/2] mfd: Add anatop mfd driver

[Shawn Guo]

Sorry, one more missing ...

On Sun, Mar 04, 2012 at 01:39:12AM +0800, Ying-Chun Liu (PaulLiu) wrote:
...
> +static int of_anatop_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	void *ioreg;
> +	struct anatop *drvdata;
> +
> +	ioreg = of_iomap(np, 0);
> +	if (!ioreg)
> +		return -EINVAL;
> +	drvdata = devm_kzalloc(dev, sizeof(struct anatop), GFP_KERNEL);

sizeof(*drvdata) please.

Documentation/CodingStyle, Chapter 14:

The preferred form for passing a size of a struct is the following:

        p = kmalloc(sizeof(*p), ...);

The alternative form where struct name is spelled out hurts readability and
introduces an opportunity for a bug when the pointer variable type is changed
but the corresponding sizeof that is passed to a memory allocator is not.

> +	if (!drvdata)
> +		return -EINVAL;
> +	drvdata->ioreg = ioreg;
> +	spin_lock_init(&drvdata->reglock);
> +	platform_set_drvdata(pdev, drvdata);
> +	of_platform_bus_probe(np, of_anatop_subdevice_match, dev);
> +
> +	return 0;
> +}

-- 
Regards,
Shawn

Re: [PATCH v6 2/2] Regulator: Add Anatop regulator driver

[Shawn Guo]

On Fri, Mar 02, 2012 at 03:00:41PM +0800, Ying-Chun Liu (PaulLiu) wrote:
> From: "Ying-Chun Liu (PaulLiu)" <paul.liu@linaro.org>
> 
> Anatop is an integrated regulator inside i.MX6 SoC.
> There are 3 digital regulators which controls PU, CORE (ARM), and SOC.
> And 3 analog regulators which controls 1P1, 2P5, 3P0 (USB).
> This patch adds the Anatop regulator driver.
> 
> Signed-off-by: Nancy Chen <Nancy.Chen@freescale.com>
> Signed-off-by: Ying-Chun Liu (PaulLiu) <paul.liu@linaro.org>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Liam Girdwood <lrg@ti.com>
> Cc: Samuel Ortiz <sameo@linux.intel.com>
> Cc: Shawn Guo <shawn.guo@linaro.org>
> ---
>  .../bindings/regulator/anatop-regulator.txt        |   28 +++
>  drivers/regulator/Kconfig                          |    6 +
>  drivers/regulator/Makefile                         |    1 +
>  drivers/regulator/anatop-regulator.c               |  205 ++++++++++++++++++++
>  include/linux/regulator/anatop-regulator.h         |   40 ++++
>  5 files changed, 280 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/regulator/anatop-regulator.txt
>  create mode 100644 drivers/regulator/anatop-regulator.c
>  create mode 100644 include/linux/regulator/anatop-regulator.h
> 
> diff --git a/Documentation/devicetree/bindings/regulator/anatop-regulator.txt b/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
> new file mode 100644
> index 0000000..f19cfea
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/regulator/anatop-regulator.txt
> @@ -0,0 +1,28 @@
> +Anatop Voltage regulators
> +
> +Required properties:
> +- compatible: Must be "fsl,anatop-regulator"
> +- vol-bit-shift: Bit shift for the register
> +- vol-bit-size: Number of bits used in the register

A better name might be vol-bit-width, considering shift and width
are generally a couple.

> +- min-bit-val: Minimum value of this register
> +- min-voltage: Minimum voltage of this regulator
> +- max-voltage: Maximum voltage of this regulator
> +
> +Any property defined as part of the core regulator
> +binding, defined in regulator.txt, can also be used.
> +
> +Example:
> +
> +	reg_vddpu: regulator-vddpu@140 {
> +		compatible = "fsl,anatop-regulator";
> +		regulator-name = "vddpu";
> +		regulator-min-microvolt = <725000>;
> +		regulator-max-microvolt = <1300000>;
> +		regulator-always-on;
> +		reg = <0x140 1>;

The size cell is useless and confusing here.  I think we can just have
it like "reg = <0x140>;".

> +		vol-bit-shift = <9>;
> +		vol-bit-size = <5>;
> +		min-bit-val = <1>;
> +		min-voltage = <725000>;
> +		max-voltage = <1300000>;
> +	};
> diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
> index 7a61b17..5fbcda2 100644
> --- a/drivers/regulator/Kconfig
> +++ b/drivers/regulator/Kconfig
> @@ -335,5 +335,11 @@ config REGULATOR_AAT2870
>  	  If you have a AnalogicTech AAT2870 say Y to enable the
>  	  regulator driver.
>  
> +config REGULATOR_ANATOP
> +	tristate "ANATOP LDO regulators"

A more descriptive prompt?

> +	depends on MFD_ANATOP
> +	help
> +	  Say y here to support ANATOP LDOs regulators.

Ditto.

> +
>  endif
>  
> diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
> index 503bac8..8440871 100644
> --- a/drivers/regulator/Makefile
> +++ b/drivers/regulator/Makefile
> @@ -48,5 +48,6 @@ obj-$(CONFIG_REGULATOR_AB8500)	+= ab8500.o
>  obj-$(CONFIG_REGULATOR_DB8500_PRCMU) += db8500-prcmu.o
>  obj-$(CONFIG_REGULATOR_TPS65910) += tps65910-regulator.o
>  obj-$(CONFIG_REGULATOR_AAT2870) += aat2870-regulator.o
> +obj-$(CONFIG_REGULATOR_ANATOP) += anatop-regulator.o
>  
>  ccflags-$(CONFIG_REGULATOR_DEBUG) += -DDEBUG
> diff --git a/drivers/regulator/anatop-regulator.c b/drivers/regulator/anatop-regulator.c
> new file mode 100644
> index 0000000..84d8f50
> --- /dev/null
> +++ b/drivers/regulator/anatop-regulator.c
> @@ -0,0 +1,205 @@
> +/*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + */
> +
> +/*
> + * 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.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/driver.h>
> +#include <linux/regulator/anatop-regulator.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/regulator/of_regulator.h>

It may be good to have <linux/regulator/*> headers grouped/sorted
together, something like:

#include <linux/regulator/anatop-regulator.h>
#include <linux/regulator/driver.h>
#include <linux/regulator/of_regulator.h>

> +
> +static int anatop_set_voltage(struct regulator_dev *reg, int min_uV,
> +				  int max_uV, unsigned *selector)
> +{
> +	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
> +	u32 val, sel;
> +	int uv;
> +
> +	uv = min_uV;
> +	dev_dbg(&reg->dev, "%s: uv %d, min %d, max %d\n", __func__,
> +		uv, anatop_reg->min_voltage,
> +		anatop_reg->max_voltage);
> +
> +	if (uv < anatop_reg->min_voltage) {
> +		if (max_uV > anatop_reg->min_voltage)
> +			uv = anatop_reg->min_voltage;
> +		else
> +			return -EINVAL;
> +	}
> +
> +	if (anatop_reg->control_reg) {
> +		sel = (uv - anatop_reg->min_voltage) / 25000;
> +		if (sel * 25000 + anatop_reg->min_voltage
> +		    > anatop_reg->max_voltage)
> +			return -EINVAL;
> +		val = anatop_reg->min_bit_val + sel;
> +		*selector = sel;
> +		dev_dbg(&reg->dev, "%s: calculated val %d\n", __func__, val);
> +		anatop_set_bits(anatop_reg->mfd,
> +				anatop_reg->control_reg,
> +				anatop_reg->vol_bit_shift,
> +				anatop_reg->vol_bit_size,
> +				val);
> +		return 0;
> +	} else {
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static int anatop_get_voltage_sel(struct regulator_dev *reg)
> +{
> +	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
> +	int selector;
> +
> +	if (anatop_reg->control_reg) {
> +		u32 val = anatop_get_bits(anatop_reg->mfd,
> +					  anatop_reg->control_reg,
> +					  anatop_reg->vol_bit_shift,
> +					  anatop_reg->vol_bit_size);
> +		selector = val - anatop_reg->min_bit_val;
> +		return selector;
> +	} else {
> +		return -ENOTSUPP;
> +	}
> +}
> +
> +static int anatop_list_voltage(struct regulator_dev *reg, unsigned selector)
> +{
> +	struct anatop_regulator *anatop_reg = rdev_get_drvdata(reg);
> +	int uv;
> +
> +	uv = anatop_reg->min_voltage + selector * 25000;
> +	dev_dbg(&reg->dev, "vddio = %d, selector = %u\n", uv, selector);
> +
> +	return uv;
> +}
> +
> +static struct regulator_ops anatop_rops = {
> +	.set_voltage     = anatop_set_voltage,
> +	.get_voltage_sel = anatop_get_voltage_sel,
> +	.list_voltage    = anatop_list_voltage,
> +};
> +
> +int anatop_regulator_probe(struct platform_device *pdev)

static int __devinit

> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct regulator_desc *rdesc;
> +	struct regulator_dev *rdev;
> +	struct anatop_regulator *sreg;
> +	struct regulator_init_data *initdata;
> +	struct anatop *anatopmfd = dev_get_drvdata(pdev->dev.parent);
> +	const __be32 *rval;
> +	u64 ofsize;
> +
> +	initdata = of_get_regulator_init_data(dev, dev->of_node);

You already have a variable np being dev->of_node.

> +	sreg = devm_kzalloc(dev, sizeof(struct anatop_regulator), GFP_KERNEL);
> +	if (!sreg)
> +		return -EINVAL;
> +	rdesc = devm_kzalloc(dev, sizeof(struct regulator_desc), GFP_KERNEL);
> +	if (!rdesc)
> +		return -EINVAL;

Would something like the following be better?

	sreg = devm_kzalloc(dev, sizeof(*sreg) + sizeof(*rdesc), GFP_KERNEL);
	if (!sreg)
		return -ENOMEM;
	rdesc = (struct regulator_desc *)(sreg + 1);

> +	sreg->initdata = initdata;
> +	sreg->name = kstrdup(of_get_property(np, "regulator-name", NULL),
> +			     GFP_KERNEL);
> +	sreg->regulator = rdesc;
> +	memset(rdesc, 0, sizeof(struct regulator_desc));

sizeof(*rdesc)

> +	rdesc->name = sreg->name;
> +	rdesc->ops = &anatop_rops;
> +	rdesc->type = REGULATOR_VOLTAGE;
> +	rdesc->owner = THIS_MODULE;
> +	sreg->mfd = anatopmfd;
> +	rval = of_get_address(np, 0, &ofsize, NULL);
> +	if (rval)
> +		sreg->control_reg = be32_to_cpu(*rval);

With size cell removed, we can just read the "reg" property as an u32
with helper function of_property_read_u32().

> +	rval = of_get_property(np, "vol-bit-size", NULL);
> +	if (rval)
> +		sreg->vol_bit_size = be32_to_cpu(*rval);
> +	rval = of_get_property(np, "vol-bit-shift", NULL);
> +	if (rval)
> +		sreg->vol_bit_shift = be32_to_cpu(*rval);
> +	rval = of_get_property(np, "min-bit-val", NULL);
> +	if (rval)
> +		sreg->min_bit_val = be32_to_cpu(*rval);
> +	rval = of_get_property(np, "min-voltage", NULL);
> +	if (rval)
> +		sreg->min_voltage = be32_to_cpu(*rval);
> +	rval = of_get_property(np, "max-voltage", NULL);
> +	if (rval)
> +		sreg->max_voltage = be32_to_cpu(*rval);

All above can just use of_property_read_u32().

> +
> +	/* register regulator */
> +	rdev = regulator_register(rdesc, &pdev->dev,
> +				  initdata, sreg, pdev->dev.of_node);
> +	platform_set_drvdata(pdev, rdev);
> +
> +	if (IS_ERR(rdev)) {
> +		dev_err(&pdev->dev, "failed to register %s\n",
> +			rdesc->name);
> +		kfree(sreg->name);
> +		return PTR_ERR(rdev);
> +	}

Shouldn't the error checking go right after regulator_register() call?

> +
> +	return 0;
> +}
> +
> +int anatop_regulator_remove(struct platform_device *pdev)

static int __devexit

> +{
> +	struct regulator_dev *rdev = platform_get_drvdata(pdev);
> +	struct anatop_regulator *sreg = rdev_get_drvdata(rdev);
> +	kfree(sreg->name);
> +	regulator_unregister(rdev);
> +	return 0;
> +}
> +
> +struct of_device_id __devinitdata of_anatop_regulator_match_tbl[] = {

static

> +	{ .compatible = "fsl,anatop-regulator", },
> +	{ /* end */ }
> +};
> +
> +struct platform_driver anatop_regulator = {

static

> +	.driver = {
> +		.name	= "anatop_regulator",
> +		.owner  = THIS_MODULE,
> +		.of_match_table = of_anatop_regulator_match_tbl,
> +	},
> +	.probe	= anatop_regulator_probe,
> +	.remove	= anatop_regulator_remove,
> +};
> +
> +int anatop_regulator_init(void)

static int __init

> +{
> +	return platform_driver_register(&anatop_regulator);
> +}
> +postcore_initcall(anatop_regulator_init);
> +
> +void anatop_regulator_exit(void)

static void __exit

> +{
> +	platform_driver_unregister(&anatop_regulator);
> +}
> +module_exit(anatop_regulator_exit);
> +
> +MODULE_DESCRIPTION("ANATOP Regulator driver");
> +MODULE_LICENSE("GPL v2");

MODULE_AUTHOR?  Multiple people can be listed there.

> diff --git a/include/linux/regulator/anatop-regulator.h b/include/linux/regulator/anatop-regulator.h
> new file mode 100644
> index 0000000..7a9a05b
> --- /dev/null
> +++ b/include/linux/regulator/anatop-regulator.h
> @@ -0,0 +1,40 @@
> +/*
> + * Copyright (C) 2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + */
> +
> +/*
> + * 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.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, write to the Free Software Foundation, Inc.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#ifndef __ANATOP_REGULATOR_H
> +#define __ANATOP_REGULATOR_H
> +
> +#include <linux/regulator/driver.h>
> +#include <linux/mfd/anatop.h>
> +
> +struct anatop_regulator {
> +	struct regulator_desc *regulator;
> +	struct regulator_init_data *initdata;
> +	const char *name;
> +	u32 control_reg;
> +	struct anatop *mfd;
> +	int vol_bit_shift;
> +	int vol_bit_size;
> +	int min_bit_val;
> +	int min_voltage;
> +	int max_voltage;
> +};

Does this structure really need to be public?

> +
> +#endif /* __ANATOP_REGULATOR_H */
> -- 
> 1.7.9.1
> 

-- 
Regards,
Shawn

Re: [PATCH v6 2/2] Regulator: Add Anatop regulator driver

[Mark Brown]

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

On Sun, Mar 04, 2012 at 02:51:48PM +0800, Shawn Guo wrote:

> > +	sreg = devm_kzalloc(dev, sizeof(struct anatop_regulator), GFP_KERNEL);
> > +	if (!sreg)
> > +		return -EINVAL;
> > +	rdesc = devm_kzalloc(dev, sizeof(struct regulator_desc), GFP_KERNEL);
> > +	if (!rdesc)
> > +		return -EINVAL;

> Would something like the following be better?

> 	sreg = devm_kzalloc(dev, sizeof(*sreg) + sizeof(*rdesc), GFP_KERNEL);
> 	if (!sreg)
> 		return -ENOMEM;
> 	rdesc = (struct regulator_desc *)(sreg + 1);

No, that sort of pointer arithmetic would be much worse - it's harder to
read and more likely to break.  However, embedding the regulator_desc in
sreg would achieve the same result without the legibility issues.

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