[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Tero Kristo]

vdd1 and vdd2 are now common regulators for twl4030 and twl6030. Also
added vdd3 as a new regulator for twl6030. twl6030 vdd1...vdd3 smps
regulator voltages can only be controlled through the smartreflex
voltage channel, thus the support for the voltage_get and set is
minimal and requires external controller.

Signed-off-by: Tero Kristo <t-kristo@ti.com>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@ti.com>
Cc: Samuel Ortiz <sameo@linux.intel.com>
Cc: Kevin Hilman <khilman@ti.com>
---
 drivers/mfd/twl-core.c            |   15 ++++++++++++++
 drivers/regulator/twl-regulator.c |   39 +++++++++++++++++++++++++++++++++++++
 include/linux/i2c/twl.h           |    5 ++-
 3 files changed, 57 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index fae5f76..c788e36 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -949,6 +949,21 @@ add_children(struct twl4030_platform_data *pdata, unsigned long features)
 	/* twl6030 regulators */
 	if (twl_has_regulator() && twl_class_is_6030() &&
 			!(features & TWL6025_SUBCLASS)) {
+		child = add_regulator(TWL6030_REG_VDD1, pdata->vdd1,
+					features);
+		if (IS_ERR(child))
+			return PTR_ERR(child);
+
+		child = add_regulator(TWL6030_REG_VDD2, pdata->vdd2,
+					features);
+		if (IS_ERR(child))
+			return PTR_ERR(child);
+
+		child = add_regulator(TWL6030_REG_VDD3, pdata->vdd3,
+					features);
+		if (IS_ERR(child))
+			return PTR_ERR(child);
+
 		child = add_regulator(TWL6030_REG_VMMC, pdata->vmmc,
 					features);
 		if (IS_ERR(child))
diff --git a/drivers/regulator/twl-regulator.c b/drivers/regulator/twl-regulator.c
index 0afc9e1a..55a0e4d 100644
--- a/drivers/regulator/twl-regulator.c
+++ b/drivers/regulator/twl-regulator.c
@@ -561,6 +561,32 @@ static struct regulator_ops twl4030smps_ops = {
 	.get_voltage	= twl4030smps_get_voltage,
 };
 
+static int twl6030coresmps_set_voltage(struct regulator_dev *rdev, int min_uV,
+	int max_uV, unsigned *selector)
+{
+	struct twlreg_info *info = rdev_get_drvdata(rdev);
+
+	if (info->set_voltage)
+		return info->set_voltage(info->data, min_uV);
+
+	return 0;
+}
+
+static int twl6030coresmps_get_voltage(struct regulator_dev *rdev)
+{
+	struct twlreg_info *info = rdev_get_drvdata(rdev);
+
+	if (info->get_voltage)
+		return info->get_voltage(info->data);
+
+	return 0;
+}
+
+static struct regulator_ops twl6030coresmps_ops = {
+	.set_voltage	= twl6030coresmps_set_voltage,
+	.get_voltage	= twl6030coresmps_get_voltage,
+};
+
 static int twl6030ldo_list_voltage(struct regulator_dev *rdev, unsigned index)
 {
 	struct twlreg_info	*info = rdev_get_drvdata(rdev);
@@ -918,6 +944,16 @@ static struct regulator_ops twlsmps_ops = {
 		}, \
 	}
 
+#define TWL6030_ADJUSTABLE_SMPS(label) { \
+	.desc = { \
+		.name = #label, \
+		.id = TWL6030_REG_##label, \
+		.ops = &twl6030coresmps_ops, \
+		.type = REGULATOR_VOLTAGE, \
+		.owner = THIS_MODULE, \
+		}, \
+	}
+
 #define TWL6030_ADJUSTABLE_LDO(label, offset, min_mVolts, max_mVolts) { \
 	.base = offset, \
 	.min_mV = min_mVolts, \
@@ -1019,6 +1055,9 @@ static struct twlreg_info twl_regs[] = {
 	/* 6030 REG with base as PMC Slave Misc : 0x0030 */
 	/* Turnon-delay and remap configuration values for 6030 are not
 	   verified since the specification is not public */
+	TWL6030_ADJUSTABLE_SMPS(VDD1),
+	TWL6030_ADJUSTABLE_SMPS(VDD2),
+	TWL6030_ADJUSTABLE_SMPS(VDD3),
 	TWL6030_ADJUSTABLE_LDO(VAUX1_6030, 0x54, 1000, 3300),
 	TWL6030_ADJUSTABLE_LDO(VAUX2_6030, 0x58, 1000, 3300),
 	TWL6030_ADJUSTABLE_LDO(VAUX3_6030, 0x5c, 1000, 3300),
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 08a82d3..f66c031 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -712,6 +712,9 @@ struct twl4030_platform_data {
 	struct regulator_init_data		*vaux1;
 	struct regulator_init_data		*vaux2;
 	struct regulator_init_data		*vaux3;
+	struct regulator_init_data		*vdd1;
+	struct regulator_init_data		*vdd2;
+	struct regulator_init_data		*vdd3;
 	/* TWL4030 LDO regulators */
 	struct regulator_init_data		*vpll1;
 	struct regulator_init_data		*vpll2;
@@ -720,8 +723,6 @@ struct twl4030_platform_data {
 	struct regulator_init_data		*vsim;
 	struct regulator_init_data		*vaux4;
 	struct regulator_init_data		*vio;
-	struct regulator_init_data		*vdd1;
-	struct regulator_init_data		*vdd2;
 	struct regulator_init_data		*vintana1;
 	struct regulator_init_data		*vintana2;
 	struct regulator_init_data		*vintdig;
-- 
1.7.4.1

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Mark Brown]

On Thu, Feb 23, 2012 at 01:05:09PM +0200, Tero Kristo wrote:

> +static int twl6030coresmps_set_voltage(struct regulator_dev *rdev, int min_uV,
> +	int max_uV, unsigned *selector)
> +{
> +	struct twlreg_info *info = rdev_get_drvdata(rdev);
> +
> +	if (info->set_voltage)
> +		return info->set_voltage(info->data, min_uV);
> +
> +	return 0;
> +}

This should be returning an error if it failed to set the voltage.
Since you're using min_uV as the "register value" you probably ought to
be returning that as the selector too and supplying a list_voltage()
which just passes the selector back in case something tries to use it
and gets confused.

> +static int twl6030coresmps_get_voltage(struct regulator_dev *rdev)
> +{
> +	struct twlreg_info *info = rdev_get_drvdata(rdev);
> +
> +	if (info->get_voltage)
> +		return info->get_voltage(info->data);
> +
> +	return 0;

Similarly here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120223/7a7b9d61/attachment-0001.sig>

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Tero Kristo]

On Thu, 2012-02-23 at 15:34 +0000, Mark Brown wrote:
> On Thu, Feb 23, 2012 at 01:05:09PM +0200, Tero Kristo wrote:
> 
> > +static int twl6030coresmps_set_voltage(struct regulator_dev *rdev, int min_uV,
> > +	int max_uV, unsigned *selector)
> > +{
> > +	struct twlreg_info *info = rdev_get_drvdata(rdev);
> > +
> > +	if (info->set_voltage)
> > +		return info->set_voltage(info->data, min_uV);
> > +
> > +	return 0;
> > +}
> 
> This should be returning an error if it failed to set the voltage.

Yea, I can change the return to -ENODEV for this and get_voltage.

> Since you're using min_uV as the "register value" you probably ought to
> be returning that as the selector too and supplying a list_voltage()
> which just passes the selector back in case something tries to use it
> and gets confused.

I was thinking at some point about adding a list_voltage for these
regulators, however I dropped that idea, because the regulators can
support a range of voltages (from min to max) with some stepping value.
But... if you propose that the list_voltage would just return the
current voltage back, wouldn't that also potentially confuse the user
more, as it can only see the single voltage and nothing else, maybe
making it to think that the regulator can only support one voltage
level?

> 
> > +static int twl6030coresmps_get_voltage(struct regulator_dev *rdev)
> > +{
> > +	struct twlreg_info *info = rdev_get_drvdata(rdev);
> > +
> > +	if (info->get_voltage)
> > +		return info->get_voltage(info->data);
> > +
> > +	return 0;
> 
> Similarly here.

I'll change that also.

-Tero

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Mark Brown]

On Fri, Feb 24, 2012 at 11:38:09AM +0200, Tero Kristo wrote:
> On Thu, 2012-02-23 at 15:34 +0000, Mark Brown wrote:

> > Since you're using min_uV as the "register value" you probably ought to
> > be returning that as the selector too and supplying a list_voltage()
> > which just passes the selector back in case something tries to use it
> > and gets confused.

> I was thinking at some point about adding a list_voltage for these
> regulators, however I dropped that idea, because the regulators can
> support a range of voltages (from min to max) with some stepping value.
> But... if you propose that the list_voltage would just return the
> current voltage back, wouldn't that also potentially confuse the user
> more, as it can only see the single voltage and nothing else, maybe
> making it to think that the regulator can only support one voltage
> level?

Yes, that'd be completely broken.  You'd need to just return the
selector back which would tell them that they had voltage control in
microvolt steps.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120224/d9f2bef4/attachment.sig>

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Tero Kristo]

On Fri, 2012-02-24 at 11:49 +0000, Mark Brown wrote:
> On Fri, Feb 24, 2012 at 11:38:09AM +0200, Tero Kristo wrote:
> > On Thu, 2012-02-23 at 15:34 +0000, Mark Brown wrote:
> 
> > > Since you're using min_uV as the "register value" you probably ought to
> > > be returning that as the selector too and supplying a list_voltage()
> > > which just passes the selector back in case something tries to use it
> > > and gets confused.
> 
> > I was thinking at some point about adding a list_voltage for these
> > regulators, however I dropped that idea, because the regulators can
> > support a range of voltages (from min to max) with some stepping value.
> > But... if you propose that the list_voltage would just return the
> > current voltage back, wouldn't that also potentially confuse the user
> > more, as it can only see the single voltage and nothing else, maybe
> > making it to think that the regulator can only support one voltage
> > level?
> 
> Yes, that'd be completely broken.  You'd need to just return the
> selector back which would tell them that they had voltage control in
> microvolt steps.

I still ain't quite sure how this would work, do you mean adding
something like this:

+static int twl6030smps_list_voltage(struct regulator_dev *rdev,
+                                   unsigned int selector)
+{
+       return selector;
+}

I believe this would fail still. I took a look at a few drivers that use
regulator_list_voltage(), but all of these seem to numerate voltages
based on regulator_count_voltages(), which will return -EINVAL for the
SMPS ones as the num_voltages is zero. Also, even if I defined
num_voltages here, I would be attempting to list_voltage for zero index,
returning zero, but this would be invalid voltage for the cpu obviously
(and is also out of range for the regulator min_voltage, and also
according to docs invalid return value for the function in the first
place.)

-Tero

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Mark Brown]

On Fri, Feb 24, 2012 at 03:16:38PM +0200, Tero Kristo wrote:

> I still ain't quite sure how this would work, do you mean adding
> something like this:

> +static int twl6030smps_list_voltage(struct regulator_dev *rdev,
> +                                   unsigned int selector)
> +{
> +       return selector;
> +}

Yes.

> I believe this would fail still. I took a look at a few drivers that use
> regulator_list_voltage(), but all of these seem to numerate voltages
> based on regulator_count_voltages(), which will return -EINVAL for the
> SMPS ones as the num_voltages is zero. Also, even if I defined
> num_voltages here, I would be attempting to list_voltage for zero index,
> returning zero, but this would be invalid voltage for the cpu obviously
> (and is also out of range for the regulator min_voltage, and also
> according to docs invalid return value for the function in the first
> place.)

Well, clearly some of the values won't actually be useful and you should
feel free to return error values for those or apply an offset or
something but the basic principle applies.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120224/f044e41f/attachment.sig>

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Tero Kristo]

On Fri, 2012-02-24 at 13:24 +0000, Mark Brown wrote:
> On Fri, Feb 24, 2012 at 03:16:38PM +0200, Tero Kristo wrote:
> 
> > I still ain't quite sure how this would work, do you mean adding
> > something like this:
> 
> > +static int twl6030smps_list_voltage(struct regulator_dev *rdev,
> > +                                   unsigned int selector)
> > +{
> > +       return selector;
> > +}
> 
> Yes.
> 
> > I believe this would fail still. I took a look at a few drivers that use
> > regulator_list_voltage(), but all of these seem to numerate voltages
> > based on regulator_count_voltages(), which will return -EINVAL for the
> > SMPS ones as the num_voltages is zero. Also, even if I defined
> > num_voltages here, I would be attempting to list_voltage for zero index,
> > returning zero, but this would be invalid voltage for the cpu obviously
> > (and is also out of range for the regulator min_voltage, and also
> > according to docs invalid return value for the function in the first
> > place.)
> 
> Well, clearly some of the values won't actually be useful and you should
> feel free to return error values for those or apply an offset or
> something but the basic principle applies.

So, do you want me to also change the num_voltages value for the
regulator from zero to be the same as max_uV, as we have this check
within regulator/core:

       if (!ops->list_voltage || selector >= rdev->desc->n_voltages)
                return -EINVAL;

This will also potentially make some code to iterate over regulator
voltages for ~1.5M times. I still don't think adding list_voltage for
the SMPS regulators makes any sense, unless either the API for
regulator_list_voltage is changed, or we change the control for these
regulators completely from set_voltage() based to set_voltage_sel()
based implementation.

-Tero

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Mark Brown]

On Fri, Feb 24, 2012 at 03:56:05PM +0200, Tero Kristo wrote:

> So, do you want me to also change the num_voltages value for the
> regulator from zero to be the same as max_uV, as we have this check
> within regulator/core:

>        if (!ops->list_voltage || selector >= rdev->desc->n_voltages)
>                 return -EINVAL;

> This will also potentially make some code to iterate over regulator
> voltages for ~1.5M times. I still don't think adding list_voltage for
> the SMPS regulators makes any sense, unless either the API for
> regulator_list_voltage is changed, or we change the control for these
> regulators completely from set_voltage() based to set_voltage_sel()
> based implementation.

Well, the important thing here is to fill in something useful for the
returned selector rather than just leaving it undefined.  Providing a
list_voltage() would be nice and make things more robust.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120224/61da7e25/attachment.sig>

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Tero Kristo]

On Fri, 2012-02-24 at 14:01 +0000, Mark Brown wrote:
> On Fri, Feb 24, 2012 at 03:56:05PM +0200, Tero Kristo wrote:
> 
> > So, do you want me to also change the num_voltages value for the
> > regulator from zero to be the same as max_uV, as we have this check
> > within regulator/core:
> 
> >        if (!ops->list_voltage || selector >= rdev->desc->n_voltages)
> >                 return -EINVAL;
> 
> > This will also potentially make some code to iterate over regulator
> > voltages for ~1.5M times. I still don't think adding list_voltage for
> > the SMPS regulators makes any sense, unless either the API for
> > regulator_list_voltage is changed, or we change the control for these
> > regulators completely from set_voltage() based to set_voltage_sel()
> > based implementation.
> 
> Well, the important thing here is to fill in something useful for the
> returned selector rather than just leaving it undefined.  Providing a
> list_voltage() would be nice and make things more robust.

Still, setting selector in this case does nothing, as it is immediately
overwritten by the regulator core by -1. This looks like a perfectly
acceptable way to implement a regulator, as everything checks for the
presence of list_voltage anyway.

If you really insist, I could probably make something that does a
list_voltage and shows min and max voltages for the regulator, but I
don't know about the usefulness of that.

-Tero

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Mark Brown]

On Fri, Feb 24, 2012 at 04:25:12PM +0200, Tero Kristo wrote:

> Still, setting selector in this case does nothing, as it is immediately
> overwritten by the regulator core by -1. This looks like a perfectly
> acceptable way to implement a regulator, as everything checks for the
> presence of list_voltage anyway.

Ah, so it is - we're fixing things up in the core.  I'd forgotten we did
that.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120224/b2efbe28/attachment.sig>

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Tero Kristo]

On Fri, 2012-02-24 at 14:34 +0000, Mark Brown wrote:
> On Fri, Feb 24, 2012 at 04:25:12PM +0200, Tero Kristo wrote:
> 
> > Still, setting selector in this case does nothing, as it is immediately
> > overwritten by the regulator core by -1. This looks like a perfectly
> > acceptable way to implement a regulator, as everything checks for the
> > presence of list_voltage anyway.
> 
> Ah, so it is - we're fixing things up in the core.  I'd forgotten we did
> that.

So, no need to add list_voltage to this, I just fix the return values
for the get / set and send the new version, am I right?

-Tero

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Mark Brown]

On Fri, Feb 24, 2012 at 04:42:12PM +0200, Tero Kristo wrote:
> On Fri, 2012-02-24 at 14:34 +0000, Mark Brown wrote:

> > Ah, so it is - we're fixing things up in the core.  I'd forgotten we did
> > that.

> So, no need to add list_voltage to this, I just fix the return values
> for the get / set and send the new version, am I right?

I guess.  Though actually given that there's no interaction with the
chip itself at all I'm not sure I understand why this is part of the
twl6030 driver rather than being a separate driver for whatever
implements the operations - it's not like the case with the other
regulators where there was some functionality implemented through the
twl6030 driver beyond straight passthrough.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120224/5ffa8938/attachment.sig>

[PATCH] regulator: twl6030: add support for vdd1, vdd2 and vdd3 regulators

[Tero Kristo]

On Fri, 2012-02-24 at 14:50 +0000, Mark Brown wrote:
> On Fri, Feb 24, 2012 at 04:42:12PM +0200, Tero Kristo wrote:
> > On Fri, 2012-02-24 at 14:34 +0000, Mark Brown wrote:
> 
> > > Ah, so it is - we're fixing things up in the core.  I'd forgotten we did
> > > that.
> 
> > So, no need to add list_voltage to this, I just fix the return values
> > for the get / set and send the new version, am I right?
> 
> I guess.  Though actually given that there's no interaction with the
> chip itself at all I'm not sure I understand why this is part of the
> twl6030 driver rather than being a separate driver for whatever
> implements the operations - it's not like the case with the other
> regulators where there was some functionality implemented through the
> twl6030 driver beyond straight passthrough.

Well, there are still some registers within twl6030 that could be
accessed through the control channel, it is just that the voltage
registers are no longer visible through this. We have the CFG_GRP,
CFG_TRANS, CFG_STATE and CFG_STEP registers visible here, even though we
are not accessing them right now.

-Tero