diff mbox

[1/9] regulator: tps65217: Enable suspend configuration

Message ID 1466412218-5906-2-git-send-email-j-keerthy@ti.com (mailing list archive)
State New, archived
Headers show

Commit Message

J, KEERTHY June 20, 2016, 8:43 a.m. UTC
From: Russ Dill <Russ.Dill@ti.com>

This allows platform data to specify which power rails should be on or off
during RTC only suspend. This is necessary to keep DDR state while in RTC
only suspend.

Signed-off-by: Russ Dill <Russ.Dill@ti.com>
Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/regulator/tps65217-regulator.c | 74 +++++++++++++++++++++++++++++-----
 1 file changed, 65 insertions(+), 9 deletions(-)

Comments

Mark Brown June 21, 2016, 7:08 p.m. UTC | #1
On Mon, Jun 20, 2016 at 02:13:30PM +0530, Keerthy wrote:

> +static struct tps65217_regulator_data regulator_data[TPS65217_NUM_REGULATOR];

Why is this a static global?

> +		/* Store default strobe info */
> +		ret = tps65217_reg_read(tps, regulators[i].bypass_reg, &val);
> +
> +		regulator_data[i].strobe = val & regulators[i].bypass_mask;
> +

Not sure what this is doing...  I think this needs splitting up a bit,
it looks like it's a bit more than just adding the ops (which should be
generic things), that bit seems OK but there's these other bits in
there as well.
Keerthy June 22, 2016, 10:14 a.m. UTC | #2
On Wednesday 22 June 2016 12:38 AM, Mark Brown wrote:
> On Mon, Jun 20, 2016 at 02:13:30PM +0530, Keerthy wrote:
>
>> +static struct tps65217_regulator_data regulator_data[TPS65217_NUM_REGULATOR];
>
> Why is this a static global?
>
>> +		/* Store default strobe info */
>> +		ret = tps65217_reg_read(tps, regulators[i].bypass_reg, &val);
>> +
>> +		regulator_data[i].strobe = val & regulators[i].bypass_mask;
>> +
>
> Not sure what this is doing...  I think this needs splitting up a bit,
> it looks like it's a bit more than just adding the ops (which should be
> generic things), that bit seems OK but there's these other bits in
> there as well.

Okay. Let me explain a bit more here:

The TPS65217 has a pre-defined power-up / power-down sequence which in a 
typical application does not need
to be changed. However, it is possible to define custom sequences under 
I2C control. The power-up sequence is
defined by strobes and delay times. Each output rail is assigned to a 
strobe to determine the order in which the
rails are enabled.

Every regulator of tps65217 PMIC has sequence registers and every 
regulator has a default strobe value
and gets disabled when a particular power down sequence occurs.

So as to keep it on during suspend we write value 0 to strobe so that 
the regulator is out of all sequencers and is not impacted by any power 
down sequence. We are saving the default strobe value during probe so 
that when we want to regulator to be enabled during suspend we write 0 
to strobe and when we want it to get disabled during suspend we write 
the default saved strobe value.

Hence saving it in a static array and using it later in the ops 
functions to disable or enable regulator during suspend.

>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown June 22, 2016, 10:16 a.m. UTC | #3
On Wed, Jun 22, 2016 at 03:44:02PM +0530, Keerthy wrote:

> Hence saving it in a static array and using it later in the ops functions to
> disable or enable regulator during suspend.

Why a static array and not part of the dynamically allocated driver
data?
Keerthy June 22, 2016, 10:26 a.m. UTC | #4
On Wednesday 22 June 2016 03:46 PM, Mark Brown wrote:
> On Wed, Jun 22, 2016 at 03:44:02PM +0530, Keerthy wrote:
>
>> Hence saving it in a static array and using it later in the ops functions to
>> disable or enable regulator during suspend.
>
> Why a static array and not part of the dynamically allocated driver
> data?

Okay. That can be done.
I can introduce another integer pointer to struct tps65217 which 
currently holds the driver data.

I will allocate memory for TPS65217_NUM_REGULATOR strobes during 
regulator probe. Is this approach okay?

>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown June 23, 2016, 10:26 a.m. UTC | #5
On Wed, Jun 22, 2016 at 03:56:33PM +0530, Keerthy wrote:

> I will allocate memory for TPS65217_NUM_REGULATOR strobes during regulator
> probe. Is this approach okay?

It should be I think.  Some more comments or changelog explaining what's
going on with the sequencing would also help.
Keerthy June 23, 2016, 10:32 a.m. UTC | #6
On Thursday 23 June 2016 03:56 PM, Mark Brown wrote:
> On Wed, Jun 22, 2016 at 03:56:33PM +0530, Keerthy wrote:
>
>> I will allocate memory for TPS65217_NUM_REGULATOR strobes during regulator
>> probe. Is this approach okay?
>
> It should be I think.  Some more comments or changelog explaining what's
> going on with the sequencing would also help.

Sure. I will do that. Thanks for the feedback.

>
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/regulator/tps65217-regulator.c b/drivers/regulator/tps65217-regulator.c
index adbe4fc..c15659c 100644
--- a/drivers/regulator/tps65217-regulator.c
+++ b/drivers/regulator/tps65217-regulator.c
@@ -28,7 +28,7 @@ 
 #include <linux/mfd/tps65217.h>
 
 #define TPS65217_REGULATOR(_name, _id, _of_match, _ops, _n, _vr, _vm, _em, \
-                           _t, _lr, _nlr) \
+			   _t, _lr, _nlr,  _sr, _sm)	\
 	{						\
 		.name		= _name,		\
 		.id		= _id,			\
@@ -45,6 +45,8 @@ 
 		.volt_table	= _t,			\
 		.linear_ranges	= _lr,			\
 		.n_linear_ranges = _nlr,		\
+		.bypass_reg	= _sr,			\
+		.bypass_mask	= _sm,			\
 	}						\
 
 static const unsigned int LDO1_VSEL_table[] = {
@@ -118,6 +120,42 @@  static int tps65217_pmic_set_voltage_sel(struct regulator_dev *dev,
 	return ret;
 }
 
+struct tps65217_regulator_data {
+	int strobe;
+};
+
+static struct tps65217_regulator_data regulator_data[TPS65217_NUM_REGULATOR];
+
+static int tps65217_pmic_set_suspend_enable(struct regulator_dev *dev)
+{
+	struct tps65217 *tps = rdev_get_drvdata(dev);
+	unsigned int rid = rdev_get_id(dev);
+
+	if (rid < TPS65217_DCDC_1 || rid > TPS65217_LDO_4)
+		return -EINVAL;
+
+	return tps65217_clear_bits(tps, dev->desc->bypass_reg,
+				   dev->desc->bypass_mask,
+				   TPS65217_PROTECT_L1);
+}
+
+static int tps65217_pmic_set_suspend_disable(struct regulator_dev *dev)
+{
+	struct tps65217 *tps = rdev_get_drvdata(dev);
+	unsigned int rid = rdev_get_id(dev);
+
+	if (rid < TPS65217_DCDC_1 || rid > TPS65217_LDO_4)
+		return -EINVAL;
+
+	if (!regulator_data[rid].strobe)
+		return -EINVAL;
+
+	return tps65217_set_bits(tps, dev->desc->bypass_reg,
+				 dev->desc->bypass_mask,
+				 regulator_data[rid].strobe,
+				 TPS65217_PROTECT_L1);
+}
+
 /* Operations permitted on DCDCx, LDO2, LDO3 and LDO4 */
 static struct regulator_ops tps65217_pmic_ops = {
 	.is_enabled		= regulator_is_enabled_regmap,
@@ -127,6 +165,8 @@  static struct regulator_ops tps65217_pmic_ops = {
 	.set_voltage_sel	= tps65217_pmic_set_voltage_sel,
 	.list_voltage		= regulator_list_voltage_linear_range,
 	.map_voltage		= regulator_map_voltage_linear_range,
+	.set_suspend_enable	= tps65217_pmic_set_suspend_enable,
+	.set_suspend_disable	= tps65217_pmic_set_suspend_disable,
 };
 
 /* Operations permitted on LDO1 */
@@ -138,41 +178,50 @@  static struct regulator_ops tps65217_pmic_ldo1_ops = {
 	.set_voltage_sel	= tps65217_pmic_set_voltage_sel,
 	.list_voltage		= regulator_list_voltage_table,
 	.map_voltage		= regulator_map_voltage_ascend,
+	.set_suspend_enable	= tps65217_pmic_set_suspend_enable,
+	.set_suspend_disable	= tps65217_pmic_set_suspend_disable,
 };
 
 static const struct regulator_desc regulators[] = {
 	TPS65217_REGULATOR("DCDC1", TPS65217_DCDC_1, "dcdc1",
 			   tps65217_pmic_ops, 64, TPS65217_REG_DEFDCDC1,
 			   TPS65217_DEFDCDCX_DCDC_MASK, TPS65217_ENABLE_DC1_EN,
-			   NULL, tps65217_uv1_ranges, 2),
+			   NULL, tps65217_uv1_ranges, 2, TPS65217_REG_SEQ1,
+			   TPS65217_SEQ1_DC1_SEQ_MASK),
 	TPS65217_REGULATOR("DCDC2", TPS65217_DCDC_2, "dcdc2",
 			   tps65217_pmic_ops, 64, TPS65217_REG_DEFDCDC2,
 			   TPS65217_DEFDCDCX_DCDC_MASK, TPS65217_ENABLE_DC2_EN,
 			   NULL, tps65217_uv1_ranges,
-			   ARRAY_SIZE(tps65217_uv1_ranges)),
+			   ARRAY_SIZE(tps65217_uv1_ranges), TPS65217_REG_SEQ1,
+			   TPS65217_SEQ1_DC2_SEQ_MASK),
 	TPS65217_REGULATOR("DCDC3", TPS65217_DCDC_3, "dcdc3",
 			   tps65217_pmic_ops, 64, TPS65217_REG_DEFDCDC3,
 			   TPS65217_DEFDCDCX_DCDC_MASK, TPS65217_ENABLE_DC3_EN,
-			   NULL, tps65217_uv1_ranges, 1),
+			   NULL, tps65217_uv1_ranges, 1, TPS65217_REG_SEQ2,
+			   TPS65217_SEQ2_DC3_SEQ_MASK),
 	TPS65217_REGULATOR("LDO1", TPS65217_LDO_1, "ldo1",
 			   tps65217_pmic_ldo1_ops, 16, TPS65217_REG_DEFLDO1,
 			   TPS65217_DEFLDO1_LDO1_MASK, TPS65217_ENABLE_LDO1_EN,
-			   LDO1_VSEL_table, NULL, 0),
+			   LDO1_VSEL_table, NULL, 0, TPS65217_REG_SEQ2,
+			   TPS65217_SEQ2_LDO1_SEQ_MASK),
 	TPS65217_REGULATOR("LDO2", TPS65217_LDO_2, "ldo2", tps65217_pmic_ops,
 			   64, TPS65217_REG_DEFLDO2,
 			   TPS65217_DEFLDO2_LDO2_MASK, TPS65217_ENABLE_LDO2_EN,
 			   NULL, tps65217_uv1_ranges,
-			   ARRAY_SIZE(tps65217_uv1_ranges)),
+			   ARRAY_SIZE(tps65217_uv1_ranges), TPS65217_REG_SEQ3,
+			   TPS65217_SEQ3_LDO2_SEQ_MASK),
 	TPS65217_REGULATOR("LDO3", TPS65217_LDO_3, "ldo3", tps65217_pmic_ops,
 			   32, TPS65217_REG_DEFLS1, TPS65217_DEFLDO3_LDO3_MASK,
 			   TPS65217_ENABLE_LS1_EN | TPS65217_DEFLDO3_LDO3_EN,
 			   NULL, tps65217_uv2_ranges,
-			   ARRAY_SIZE(tps65217_uv2_ranges)),
+			   ARRAY_SIZE(tps65217_uv2_ranges), TPS65217_REG_SEQ3,
+			   TPS65217_SEQ3_LDO3_SEQ_MASK),
 	TPS65217_REGULATOR("LDO4", TPS65217_LDO_4, "ldo4", tps65217_pmic_ops,
 			   32, TPS65217_REG_DEFLS2, TPS65217_DEFLDO4_LDO4_MASK,
 			   TPS65217_ENABLE_LS2_EN | TPS65217_DEFLDO4_LDO4_EN,
 			   NULL, tps65217_uv2_ranges,
-			   ARRAY_SIZE(tps65217_uv2_ranges)),
+			   ARRAY_SIZE(tps65217_uv2_ranges), TPS65217_REG_SEQ4,
+			   TPS65217_SEQ4_LDO4_SEQ_MASK),
 };
 
 static int tps65217_regulator_probe(struct platform_device *pdev)
@@ -181,7 +230,8 @@  static int tps65217_regulator_probe(struct platform_device *pdev)
 	struct tps65217_board *pdata = dev_get_platdata(tps->dev);
 	struct regulator_dev *rdev;
 	struct regulator_config config = { };
-	int i;
+	int i, ret;
+	unsigned int val;
 
 	if (tps65217_chip_id(tps) != TPS65217) {
 		dev_err(&pdev->dev, "Invalid tps chip version\n");
@@ -200,6 +250,12 @@  static int tps65217_regulator_probe(struct platform_device *pdev)
 
 		rdev = devm_regulator_register(&pdev->dev, &regulators[i],
 					       &config);
+
+		/* Store default strobe info */
+		ret = tps65217_reg_read(tps, regulators[i].bypass_reg, &val);
+
+		regulator_data[i].strobe = val & regulators[i].bypass_mask;
+
 		if (IS_ERR(rdev)) {
 			dev_err(tps->dev, "failed to register %s regulator\n",
 				pdev->name);