[PATCH v3] regulator: max14577: fix set_mode clobbering enable on MAX77836 LDOs

[Jad Keskes <inasj268@gmail.com>]

So the PWRMD field in CNFG1_LDO is both the enable bit and the mode.
You can't change one without stepping on the other.

The problem is that enable() from the regulator core just writes
enable_mask (which is PWRMD_NORMAL).  If you'd called set_mode(LPM)
then disabled and re-enabled, the mode gets reset to NORMAL.  And
set_mode updates the register through the same field, so it can
accidentally enable a disabled regulator.

Fix it by storing the mode in per-regulator data.  A custom enable
writes whatever mode was last set.  set_mode only touches hardware
if the regulator is already on; otherwise it just caches the value.

Add of_map_mode while here so the initial mode can be wired from DT.

Signed-off-by: Jad Keskes <inasj268@gmail.com>
---
v3:
- Cache the desired mode in driver data instead of writing to the
  enable register directly, so enable() and set_mode() stop fighting
  over the same register field (caught by Mark Brown).
- Add custom enable/disable that respect the cached mode.
- Add of_map_mode while here.

Link: https://lore.kernel.org/all/81002c3a-f868-494c-83b1-9cf6214255b5@sirena.org.uk/

 drivers/regulator/max14577-regulator.c | 103 ++++++++++++++++++++++++-
 include/linux/mfd/max14577-private.h   |   3 +
 2 files changed, 102 insertions(+), 4 deletions(-)

diff --git a/drivers/regulator/max14577-regulator.c b/drivers/regulator/max14577-regulator.c
index c9d8d5e31cbd..378475368356 100644
--- a/drivers/regulator/max14577-regulator.c
+++ b/drivers/regulator/max14577-regulator.c
@@ -123,15 +123,89 @@ static const struct regulator_desc max14577_supported_regulators[] = {
 	[MAX14577_CHARGER] = MAX14577_CHARGER_REG,
 };
 
+struct max77836_ldo {
+	struct max14577	*max14577;
+	unsigned int	mode;
+};
+
+static int max77836_ldo_enable(struct regulator_dev *rdev)
+{
+	struct max77836_ldo *ldo = rdev_get_drvdata(rdev);
+
+	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+			MAX77836_CNFG1_LDO_PWRMD_MASK, ldo->mode);
+}
+
+static int max77836_ldo_disable(struct regulator_dev *rdev)
+{
+	return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+			MAX77836_CNFG1_LDO_PWRMD_MASK,
+			MAX77836_CNFG1_LDO_PWRMD_OFF);
+}
+
+static unsigned int max77836_ldo_get_mode(struct regulator_dev *rdev)
+{
+	struct max77836_ldo *ldo = rdev_get_drvdata(rdev);
+
+	switch (ldo->mode) {
+	case MAX77836_CNFG1_LDO_PWRMD_LPM:
+		return REGULATOR_MODE_IDLE;
+	case MAX77836_CNFG1_LDO_PWRMD_NORMAL:
+		return REGULATOR_MODE_NORMAL;
+	default:
+		return REGULATOR_MODE_INVALID;
+	}
+}
+
+static int max77836_ldo_set_mode(struct regulator_dev *rdev,
+				 unsigned int mode)
+{
+	struct max77836_ldo *ldo = rdev_get_drvdata(rdev);
+	unsigned int val;
+
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+		val = MAX77836_CNFG1_LDO_PWRMD_NORMAL;
+		break;
+	case REGULATOR_MODE_IDLE:
+		val = MAX77836_CNFG1_LDO_PWRMD_LPM;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ldo->mode = val;
+
+	/* Only touch hardware if the regulator is already on */
+	if (regulator_is_enabled_regmap(rdev))
+		return regmap_update_bits(rdev->regmap, rdev->desc->enable_reg,
+				MAX77836_CNFG1_LDO_PWRMD_MASK, val);
+
+	return 0;
+}
+
+static unsigned int max77836_ldo_of_map_mode(unsigned int mode)
+{
+	switch (mode) {
+	case REGULATOR_MODE_NORMAL:
+	case REGULATOR_MODE_IDLE:
+		return mode;
+	default:
+		return REGULATOR_MODE_INVALID;
+	}
+}
+
 static const struct regulator_ops max77836_ldo_ops = {
 	.is_enabled		= regulator_is_enabled_regmap,
-	.enable			= regulator_enable_regmap,
-	.disable		= regulator_disable_regmap,
+	.enable			= max77836_ldo_enable,
+	.disable		= max77836_ldo_disable,
 	.list_voltage		= regulator_list_voltage_linear,
 	.map_voltage		= regulator_map_voltage_linear,
 	.get_voltage_sel	= regulator_get_voltage_sel_regmap,
 	.set_voltage_sel	= regulator_set_voltage_sel_regmap,
-	/* TODO: add .set_suspend_mode */
+	.get_mode		= max77836_ldo_get_mode,
+	.set_mode		= max77836_ldo_set_mode,
+	.of_map_mode		= max77836_ldo_of_map_mode,
 };
 
 #define MAX77836_LDO_REG(num)	{ \
@@ -205,7 +279,6 @@ static int max14577_regulator_probe(struct platform_device *pdev)
 	}
 
 	config.dev = max14577->dev;
-	config.driver_data = max14577;
 
 	for (i = 0; i < supported_regulators_size; i++) {
 		struct regulator_dev *regulator;
@@ -217,6 +290,28 @@ static int max14577_regulator_probe(struct platform_device *pdev)
 			config.init_data = pdata->regulators[i].initdata;
 			config.of_node = pdata->regulators[i].of_node;
 		}
+
+		/*
+		 * LDOs need per-regulator driver data to store their mode.
+		 * The charger and safeout share the core MFD struct.
+		 */
+		if (dev_type == MAXIM_DEVICE_TYPE_MAX77836 &&
+		    (supported_regulators[i].id == MAX77836_LDO1 ||
+		     supported_regulators[i].id == MAX77836_LDO2)) {
+			struct max77836_ldo *ldo;
+
+			ldo = devm_kzalloc(&pdev->dev, sizeof(*ldo),
+					   GFP_KERNEL);
+			if (!ldo)
+				return -ENOMEM;
+
+			ldo->max14577 = max14577;
+			ldo->mode = MAX77836_CNFG1_LDO_PWRMD_NORMAL;
+			config.driver_data = ldo;
+		} else {
+			config.driver_data = max14577;
+		}
+
 		config.regmap = max14577_get_regmap(max14577,
 				supported_regulators[i].id);
 
diff --git a/include/linux/mfd/max14577-private.h b/include/linux/mfd/max14577-private.h
index dd51a37fa37f..5957e15b568e 100644
--- a/include/linux/mfd/max14577-private.h
+++ b/include/linux/mfd/max14577-private.h
@@ -350,6 +350,9 @@ enum max77836_pmic_reg {
 #define MAX77836_CNFG1_LDO_PWRMD_SHIFT		6
 #define MAX77836_CNFG1_LDO_TV_SHIFT		0
 #define MAX77836_CNFG1_LDO_PWRMD_MASK		(0x3 << MAX77836_CNFG1_LDO_PWRMD_SHIFT)
+#define MAX77836_CNFG1_LDO_PWRMD_OFF		(0x0 << MAX77836_CNFG1_LDO_PWRMD_SHIFT)
+#define MAX77836_CNFG1_LDO_PWRMD_LPM		(0x1 << MAX77836_CNFG1_LDO_PWRMD_SHIFT)
+#define MAX77836_CNFG1_LDO_PWRMD_NORMAL		(0x3 << MAX77836_CNFG1_LDO_PWRMD_SHIFT)
 #define MAX77836_CNFG1_LDO_TV_MASK		(0x3f << MAX77836_CNFG1_LDO_TV_SHIFT)
 
 /* LDO1/LDO2 CONFIG2 register */
-- 
2.54.0