[PATCH 2/4] regulator: add STw481x VMMC driver

[PATCH 2/4] regulator: add STw481x VMMC driver

[Linus Walleij]

The ST Microelectronics STw481x PMIC used for the Nomadik
has one single software-controlled regulator for VMMC.
This driver registers directly to the compatible string
as there is just one regulator.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
Hi Mark, I'm seeking an ACK for this driver eventually, to
take it through the ARM SoC tree with the dependency MFD
driver and the enablement patches.
---
 drivers/regulator/Kconfig        |   8 ++
 drivers/regulator/Makefile       |   1 +
 drivers/regulator/stw481x-vmmc.c | 195 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 204 insertions(+)
 create mode 100644 drivers/regulator/stw481x-vmmc.c

diff --git a/drivers/regulator/Kconfig b/drivers/regulator/Kconfig
index f1e6ad9..7e424fc 100644
--- a/drivers/regulator/Kconfig
+++ b/drivers/regulator/Kconfig
@@ -374,6 +374,14 @@ config REGULATOR_PALMAS
 	  on the muxing. This is handled automatically in the driver by
 	  reading the mux info from OTP.
 
+config REGULATOR_STW481X_VMMC
+	bool "ST Microelectronics STW481X VMMC regulator"
+	depends on MFD_STW481X
+	default y if MFD_STW481X
+	help
+	  This driver supports the VMMC regulator in the STw481x
+	  PMIC chips.
+
 config REGULATOR_TPS51632
 	tristate "TI TPS51632 Power Regulator"
 	depends on I2C
diff --git a/drivers/regulator/Makefile b/drivers/regulator/Makefile
index ba4a3cf..6d53bd4 100644
--- a/drivers/regulator/Makefile
+++ b/drivers/regulator/Makefile
@@ -52,6 +52,7 @@ obj-$(CONFIG_REGULATOR_PCF50633) += pcf50633-regulator.o
 obj-$(CONFIG_REGULATOR_RC5T583)  += rc5t583-regulator.o
 obj-$(CONFIG_REGULATOR_S2MPS11) += s2mps11.o
 obj-$(CONFIG_REGULATOR_S5M8767) += s5m8767.o
+obj-$(CONFIG_REGULATOR_STW481X_VMMC) += stw481x-vmmc.o
 obj-$(CONFIG_REGULATOR_TPS6105X) += tps6105x-regulator.o
 obj-$(CONFIG_REGULATOR_TPS62360) += tps62360-regulator.o
 obj-$(CONFIG_REGULATOR_TPS65023) += tps65023-regulator.o
diff --git a/drivers/regulator/stw481x-vmmc.c b/drivers/regulator/stw481x-vmmc.c
new file mode 100644
index 0000000..e53916e
--- /dev/null
+++ b/drivers/regulator/stw481x-vmmc.c
@@ -0,0 +1,195 @@
+/*
+ * Regulator driver for STw4810/STw4811 VMMC regulator.
+ *
+ * Copyright (C) 2013 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ *
+ * Author: Linus Walleij <linus.walleij@linaro.org>
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/mfd/stw481x.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/driver.h>
+#include <linux/regulator/of_regulator.h>
+
+static int stw481x_vmmc_enable(struct regulator_dev *reg)
+{
+	struct stw481x *stw481x = rdev_get_drvdata(reg);
+	int ret;
+
+	/* First disable the external VMMC if it's active */
+	ret = regmap_update_bits(stw481x->map, STW_CONF2,
+				 STW_CONF2_VMMC_EXT, 0);
+	if (ret)
+		return ret;
+
+	return regmap_update_bits(stw481x->map, STW_CONF1,
+				  0, STW_CONF1_PDN_VMMC);
+}
+
+static int stw481x_vmmc_disable(struct regulator_dev *reg)
+{
+	struct stw481x *stw481x = rdev_get_drvdata(reg);
+
+	return regmap_update_bits(stw481x->map, STW_CONF1,
+				  STW_CONF1_PDN_VMMC, 0);
+}
+
+static int stw481x_vmmc_is_enabled(struct regulator_dev *reg)
+{
+	struct stw481x *stw481x = rdev_get_drvdata(reg);
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(stw481x->map, STW_CONF1, &val);
+	if (ret)
+		return ret;
+	return !!(val & STW_CONF1_PDN_VMMC);
+}
+
+static int stw481x_vmmc_get_voltage_sel(struct regulator_dev *reg)
+{
+	struct stw481x *stw481x = rdev_get_drvdata(reg);
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(stw481x->map, STW_CONF1, &val);
+	if (ret)
+		return ret;
+	val &= STW_CONF1_VMMC_MASK;
+	return (val >> 1);
+}
+
+static int stw481x_vmmc_set_voltage_sel(struct regulator_dev *reg,
+					unsigned selector)
+{
+	struct stw481x *stw481x = rdev_get_drvdata(reg);
+
+	return regmap_update_bits(stw481x->map, STW_CONF1,
+				  STW_CONF1_VMMC_MASK,
+				  (selector << 1) & STW_CONF1_VMMC_MASK);
+}
+
+static const unsigned int stw481x_vmmc_voltages[] = {
+	1800000,
+	1800000,
+	2850000,
+	3000000,
+	1850000,
+	2600000,
+	2700000,
+	3300000,
+};
+
+static struct regulator_ops stw481x_vmmc_ops = {
+	.list_voltage = regulator_list_voltage_table,
+	.enable      = stw481x_vmmc_enable,
+	.disable     = stw481x_vmmc_disable,
+	.is_enabled  = stw481x_vmmc_is_enabled,
+	.get_voltage_sel = stw481x_vmmc_get_voltage_sel,
+	.set_voltage_sel = stw481x_vmmc_set_voltage_sel,
+};
+
+static struct regulator_desc vmmc_regulator = {
+	.name = "VMMC",
+	.id   = 0,
+	.ops  = &stw481x_vmmc_ops,
+	.type = REGULATOR_VOLTAGE,
+	.owner = THIS_MODULE,
+	.n_voltages = ARRAY_SIZE(stw481x_vmmc_voltages),
+	.volt_table = stw481x_vmmc_voltages,
+	.enable_time = 200, /* FIXME: look this up */
+};
+
+static ssize_t show_ctrl1(struct device *dev,
+			  struct device_attribute *attr, char *buf)
+{
+	int ret;
+	unsigned int val;
+	struct stw481x *stw481x = dev_get_platdata(dev);
+
+	ret = regmap_read(stw481x->map, STW_CONF1, &val);
+	if (ret)
+		return ret;
+
+	return sprintf(buf, "%#x\n", val);
+};
+
+static DEVICE_ATTR(ctrl1, S_IRUGO, show_ctrl1, NULL);
+
+static struct attribute *stw481x_vmmc_sysfs_entries[] = {
+	&dev_attr_ctrl1.attr,
+	NULL,
+};
+
+static struct attribute_group stw481x_vmmc_attr_group = {
+        .attrs  = stw481x_vmmc_sysfs_entries,
+};
+
+static int stw481x_vmmc_regulator_probe(struct platform_device *pdev)
+{
+	struct stw481x *stw481x = dev_get_platdata(&pdev->dev);
+	struct regulator_config config = { };
+	int ret;
+
+	/* Register VMMC regulator */
+	config.dev = &pdev->dev;
+	config.driver_data = stw481x;
+	config.of_node = pdev->dev.of_node;
+	config.init_data = of_get_regulator_init_data(&pdev->dev,
+						      pdev->dev.of_node);
+
+	stw481x->vmmc_regulator = regulator_register(&vmmc_regulator, &config);
+	if (IS_ERR(stw481x->vmmc_regulator)) {
+		dev_err(&pdev->dev,
+			"error initializing STw481x VMMC regulator\n");
+		return PTR_ERR(stw481x->vmmc_regulator);
+	}
+
+	ret = sysfs_create_group(&pdev->dev.kobj, &stw481x_vmmc_attr_group);
+	if (ret)
+		dev_err(&pdev->dev, "could not create sysfs group\n");
+
+	dev_info(&pdev->dev, "initialized STw481x VMMC regulator\n");
+	return 0;
+}
+
+static int stw481x_vmmc_regulator_remove(struct platform_device *pdev)
+{
+	struct stw481x *stw481x = dev_get_platdata(&pdev->dev);
+
+	regulator_unregister(stw481x->vmmc_regulator);
+	return 0;
+}
+
+static const struct of_device_id stw481x_vmmc_match[] = {
+	{ .compatible = "st,stw481x-vmmc", },
+        {},
+};
+
+static struct platform_driver stw481x_vmmc_regulator_driver = {
+	.driver = {
+		.name  = "stw481x-vmmc-regulator",
+		.owner = THIS_MODULE,
+	},
+	.probe = stw481x_vmmc_regulator_probe,
+	.remove = stw481x_vmmc_regulator_remove,
+};
+
+static __init int stw481x_vmmc_regulator_init(void)
+{
+	return platform_driver_register(&stw481x_vmmc_regulator_driver);
+}
+
+static __exit void stw481x_vmmc_regulator_exit(void)
+{
+	platform_driver_unregister(&stw481x_vmmc_regulator_driver);
+}
+
+subsys_initcall(stw481x_vmmc_regulator_init);
+module_exit(stw481x_vmmc_regulator_exit);
-- 
1.8.1.4

Re: [PATCH 2/4] regulator: add STw481x VMMC driver

[Mark Brown]

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

On Sun, Sep 01, 2013 at 10:00:59PM +0200, Linus Walleij wrote:

> Hi Mark, I'm seeking an ACK for this driver eventually, to
> take it through the ARM SoC tree with the dependency MFD
> driver and the enablement patches.

I can put it on a branch so it can be pulled into arm-soc - it makes
life easier for framework refactorings to have the driver in the
regulator tree as well.  Would that be OK?

> +static int stw481x_vmmc_enable(struct regulator_dev *reg)
> +{
> +	struct stw481x *stw481x = rdev_get_drvdata(reg);
> +	int ret;
> +
> +	/* First disable the external VMMC if it's active */
> +	ret = regmap_update_bits(stw481x->map, STW_CONF2,
> +				 STW_CONF2_VMMC_EXT, 0);
> +	if (ret)
> +		return ret;

This is a bit unusual - is there some definite relationship between the
two?

> +static int stw481x_vmmc_disable(struct regulator_dev *reg)
> +{
> +	struct stw481x *stw481x = rdev_get_drvdata(reg);
> +
> +	return regmap_update_bits(stw481x->map, STW_CONF1,
> +				  STW_CONF1_PDN_VMMC, 0);
> +}

Should use regulator_disable_regmap().

> +static int stw481x_vmmc_is_enabled(struct regulator_dev *reg)
> +{
> +	struct stw481x *stw481x = rdev_get_drvdata(reg);
> +	int ret;
> +	unsigned int val;
> +
> +	ret = regmap_read(stw481x->map, STW_CONF1, &val);
> +	if (ret)
> +		return ret;
> +	return !!(val & STW_CONF1_PDN_VMMC);
> +}

Should use regulator_is_enabled_regmap().

> +static int stw481x_vmmc_get_voltage_sel(struct regulator_dev *reg)
> +static int stw481x_vmmc_set_voltage_sel(struct regulator_dev *reg,
> +					unsigned selector)

Should use regulator_get/set_voltage_sel_regmap().

> +static ssize_t show_ctrl1(struct device *dev,
> +			  struct device_attribute *attr, char *buf)
> +{
> +	int ret;
> +	unsigned int val;
> +	struct stw481x *stw481x = dev_get_platdata(dev);
> +
> +	ret = regmap_read(stw481x->map, STW_CONF1, &val);
> +	if (ret)
> +		return ret;
> +
> +	return sprintf(buf, "%#x\n", val);
> +};
> +
> +static DEVICE_ATTR(ctrl1, S_IRUGO, show_ctrl1, NULL);

I'd suggest setting max_register (and possibly readable_reg()) in the
regmap config rather than adding specific sysfs files.  Unless there's
some pressing reason for this beyond debug?

> +	stw481x->vmmc_regulator = regulator_register(&vmmc_regulator, &config);
> +	if (IS_ERR(stw481x->vmmc_regulator)) {
> +		dev_err(&pdev->dev,
> +			"error initializing STw481x VMMC regulator\n");
> +		return PTR_ERR(stw481x->vmmc_regulator);
> +	}

I sent patches for a devm_regulator_register() yesterday - it'll be
going into -next after the merge window, though the conversion can
always happen later (I didn't convert most of the drivers yet anyway).

> +static __init int stw481x_vmmc_regulator_init(void)
> +{
> +	return platform_driver_register(&stw481x_vmmc_regulator_driver);
> +}
> +
> +static __exit void stw481x_vmmc_regulator_exit(void)
> +{
> +	platform_driver_unregister(&stw481x_vmmc_regulator_driver);
> +}
> +
> +subsys_initcall(stw481x_vmmc_regulator_init);
> +module_exit(stw481x_vmmc_regulator_exit);

You should be able to use module_platform_driver() with modern systems,
deferred probing should cope with most uses of subsys_initcall().  Not
the end of the world though.

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

Re: [PATCH 2/4] regulator: add STw481x VMMC driver

[Linus Walleij]

On Sun, Sep 1, 2013 at 10:13 PM, Mark Brown <broonie@kernel.org> wrote:
> On Sun, Sep 01, 2013 at 10:00:59PM +0200, Linus Walleij wrote:
>
>> Hi Mark, I'm seeking an ACK for this driver eventually, to
>> take it through the ARM SoC tree with the dependency MFD
>> driver and the enablement patches.
>
> I can put it on a branch so it can be pulled into arm-soc - it makes
> life easier for framework refactorings to have the driver in the
> regulator tree as well.  Would that be OK?

Sure but then you need patch 1/4 in there as well. I don't know if that
comment pertained to the situation prior to the v3.12 merge window
though, can I take this in now maybe or is there still a stir in the
regulator tree?

>> +static int stw481x_vmmc_enable(struct regulator_dev *reg)
>> +{
>> +     struct stw481x *stw481x = rdev_get_drvdata(reg);
>> +     int ret;
>> +
>> +     /* First disable the external VMMC if it's active */
>> +     ret = regmap_update_bits(stw481x->map, STW_CONF2,
>> +                              STW_CONF2_VMMC_EXT, 0);
>> +     if (ret)
>> +             return ret;
>
> This is a bit unusual - is there some definite relationship between the
> two?

They are mutally exclusive. You cannot have both at the same
time. The datasheet says:

0 Internal LDO VMMC is used
1 External VMMC is used

If it is set to 1, the internal VMMC is disconnected. So it's
like some internal mux. All designs I've seen use the internal
regulator though.

I move this to probe() and used regulator_enable_regmap()
instead.

> Should use regulator_disable_regmap().
> Should use regulator_is_enabled_regmap().
> Should use regulator_get/set_voltage_sel_regmap().

OK OK OK :-)

All too new stuff to me, the helpers are really convenient!

I only just rewrote the driver to use regmap and saved
some lines, with this I save even more, cool.

>> +static ssize_t show_ctrl1(struct device *dev,
>> +                       struct device_attribute *attr, char *buf)
>> +{
>> +     int ret;
>> +     unsigned int val;
>> +     struct stw481x *stw481x = dev_get_platdata(dev);
>> +
>> +     ret = regmap_read(stw481x->map, STW_CONF1, &val);
>> +     if (ret)
>> +             return ret;
>> +
>> +     return sprintf(buf, "%#x\n", val);
>> +};
>> +
>> +static DEVICE_ATTR(ctrl1, S_IRUGO, show_ctrl1, NULL);
>
> I'd suggest setting max_register (and possibly readable_reg()) in the
> regmap config rather than adding specific sysfs files.  Unless there's
> some pressing reason for this beyond debug?

This should've been just debug, and it's way simpler to do using
just regmap. Deleted.

>> +     stw481x->vmmc_regulator = regulator_register(&vmmc_regulator, &config);
>> +     if (IS_ERR(stw481x->vmmc_regulator)) {
>> +             dev_err(&pdev->dev,
>> +                     "error initializing STw481x VMMC regulator\n");
>> +             return PTR_ERR(stw481x->vmmc_regulator);
>> +     }
>
> I sent patches for a devm_regulator_register() yesterday - it'll be
> going into -next after the merge window, though the conversion can
> always happen later (I didn't convert most of the drivers yet anyway).

Hm nice, if we can merge the MFD patch into the regulator tree
I can rebase this on you branch for next and use that.

>> +static __init int stw481x_vmmc_regulator_init(void)
>> +{
>> +     return platform_driver_register(&stw481x_vmmc_regulator_driver);
>> +}
>> +
>> +static __exit void stw481x_vmmc_regulator_exit(void)
>> +{
>> +     platform_driver_unregister(&stw481x_vmmc_regulator_driver);
>> +}
>> +
>> +subsys_initcall(stw481x_vmmc_regulator_init);
>> +module_exit(stw481x_vmmc_regulator_exit);
>
> You should be able to use module_platform_driver() with modern systems,

Yeah ... and module_init() level works just fine this is just
some kind of artifact.

Yours,
Linus Walleij

Re: [PATCH 2/4] regulator: add STw481x VMMC driver

[Mark Brown]

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

On Fri, Sep 13, 2013 at 09:00:08PM +0200, Linus Walleij wrote:
> On Sun, Sep 1, 2013 at 10:13 PM, Mark Brown <broonie@kernel.org> wrote:

> > I can put it on a branch so it can be pulled into arm-soc - it makes
> > life easier for framework refactorings to have the driver in the
> > regulator tree as well.  Would that be OK?

> Sure but then you need patch 1/4 in there as well. I don't know if that

It shouldn't do, the Kconfig dependencies should stop it actually
getting built until the MFD is merged into the same tree I'd expect?
But if it is required then I'm fine with it going via arm-soc only, it's
just easier to avoid mistakes if there's no new drivers in other trees.

> comment pertained to the situation prior to the v3.12 merge window
> though, can I take this in now maybe or is there still a stir in the
> regulator tree?

There's still stuff going on - devm_regulator_register() is the main one
I'm thinking of immediately.

> They are mutally exclusive. You cannot have both at the same
> time. The datasheet says:
> 
> 0 Internal LDO VMMC is used
> 1 External VMMC is used
> 
> If it is set to 1, the internal VMMC is disconnected. So it's
> like some internal mux. All designs I've seen use the internal
> regulator though.
> 
> I move this to probe() and used regulator_enable_regmap()
> instead.

Great, that seems more idiomatic.

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

Re: [PATCH 2/4] regulator: add STw481x VMMC driver

[Linus Walleij]

On Fri, Sep 13, 2013 at 9:15 PM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Sep 13, 2013 at 09:00:08PM +0200, Linus Walleij wrote:
>> On Sun, Sep 1, 2013 at 10:13 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > I can put it on a branch so it can be pulled into arm-soc - it makes
>> > life easier for framework refactorings to have the driver in the
>> > regulator tree as well.  Would that be OK?
>
>> Sure but then you need patch 1/4 in there as well. I don't know if that
>
> It shouldn't do, the Kconfig dependencies should stop it actually
> getting built until the MFD is merged into the same tree I'd expect?

Yes you're right of course. It's just me being sensitive about
things like it's including yet non-existant headers etc. It is indeed
possible to apply it right off.

> But if it is required then I'm fine with it going via arm-soc only, it's
> just easier to avoid mistakes if there's no new drivers in other trees.

There's no need if we are convinced the MFD driver goes in
through the MFD tree.

The ARM SoC tree changes are only DT changes and those
could anyway live in through a stand-alone git the other day.

>> comment pertained to the situation prior to the v3.12 merge window
>> though, can I take this in now maybe or is there still a stir in the
>> regulator tree?
>
> There's still stuff going on - devm_regulator_register() is the main one
> I'm thinking of immediately.

OK better handle it there then. Feel free to apply when you're
happy with it.

Yours,
Linus Walleij