diff mbox

[PATCHv2] Regulator: Add Anatop regulator driver

Message ID 1324458211-1612-1-git-send-email-paul.liu@linaro.org
State Superseded
Headers show

Commit Message

Paul Liu Dec. 21, 2011, 9:03 a.m. UTC
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

Comments

Mark Brown Dec. 22, 2011, 11:33 a.m. UTC | #1
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?
Mark Brown Dec. 24, 2011, 12:37 p.m. UTC | #2
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.
Paul Liu Dec. 27, 2011, 10:06 a.m. UTC | #3
(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
Mark Brown Dec. 27, 2011, 10:40 a.m. UTC | #4
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.
diff mbox

Patch

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 */