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

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

[Jad Keskes]

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

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

[kernel test robot]

Hi Jad,

kernel test robot noticed the following build errors:

[auto build test ERROR on broonie-regulator/for-next]
[also build test ERROR on lee-mfd/for-mfd-next lee-leds/for-leds-next lee-mfd/for-mfd-fixes linus/master v7.1 next-20260616]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jad-Keskes/regulator-max14577-fix-set_mode-clobbering-enable-on-MAX77836-LDOs/20260617-024714
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
patch link:    https://lore.kernel.org/r/20260616170256.1659595-1-inasj268%40gmail.com
patch subject: [PATCH v3] regulator: max14577: fix set_mode clobbering enable on MAX77836 LDOs
config: powerpc64-randconfig-r071-20260617 (https://download.01.org/0day-ci/archive/20260617/202606170855.NFp0dIU9-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
smatch: v0.5.0-9185-gbcc58b9c
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260617/202606170855.NFp0dIU9-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202606170855.NFp0dIU9-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/regulator/max14577-regulator.c:208:3: error: field designator 'of_map_mode' does not refer to any field in type 'const struct regulator_ops'
     208 |         .of_map_mode            = max77836_ldo_of_map_mode,
         |         ~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +208 drivers/regulator/max14577-regulator.c

   197	
   198	static const struct regulator_ops max77836_ldo_ops = {
   199		.is_enabled		= regulator_is_enabled_regmap,
   200		.enable			= max77836_ldo_enable,
   201		.disable		= max77836_ldo_disable,
   202		.list_voltage		= regulator_list_voltage_linear,
   203		.map_voltage		= regulator_map_voltage_linear,
   204		.get_voltage_sel	= regulator_get_voltage_sel_regmap,
   205		.set_voltage_sel	= regulator_set_voltage_sel_regmap,
   206		.get_mode		= max77836_ldo_get_mode,
   207		.set_mode		= max77836_ldo_set_mode,
 > 208		.of_map_mode		= max77836_ldo_of_map_mode,
   209	};
   210	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Jad,

kernel test robot noticed the following build errors:

[auto build test ERROR on broonie-regulator/for-next]
[also build test ERROR on lee-mfd/for-mfd-next lee-leds/for-leds-next lee-mfd/for-mfd-fixes linus/master v7.1 next-20260616]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jad-Keskes/regulator-max14577-fix-set_mode-clobbering-enable-on-MAX77836-LDOs/20260617-024714
base:   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next
patch link:    https://lore.kernel.org/r/20260616170256.1659595-1-inasj268%40gmail.com
patch subject: [PATCH v3] regulator: max14577: fix set_mode clobbering enable on MAX77836 LDOs
config: parisc-randconfig-r073-20260617 (https://download.01.org/0day-ci/archive/20260617/202606170800.s4CQUJ3m-lkp@intel.com/config)
compiler: hppa-linux-gcc (GCC) 16.1.0
smatch: v0.5.0-9185-gbcc58b9c
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260617/202606170800.s4CQUJ3m-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202606170800.s4CQUJ3m-lkp@intel.com/

All errors (new ones prefixed by >>):

>> drivers/regulator/max14577-regulator.c:208:10: error: 'const struct regulator_ops' has no member named 'of_map_mode'
     208 |         .of_map_mode            = max77836_ldo_of_map_mode,
         |          ^~~~~~~~~~~
>> drivers/regulator/max14577-regulator.c:208:35: error: initialization of 'unsigned int (*)(struct regulator_dev *)' from incompatible pointer type 'unsigned int (*)(unsigned int)' [-Wincompatible-pointer-types]
     208 |         .of_map_mode            = max77836_ldo_of_map_mode,
         |                                   ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/regulator/max14577-regulator.c:208:35: note: (near initialization for 'max77836_ldo_ops.get_mode')
   drivers/regulator/max14577-regulator.c:187:21: note: 'max77836_ldo_of_map_mode' declared here
     187 | static unsigned int max77836_ldo_of_map_mode(unsigned int mode)
         |                     ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/regulator/max14577-regulator.c:208:35: warning: initialized field overwritten [-Woverride-init]
     208 |         .of_map_mode            = max77836_ldo_of_map_mode,
         |                                   ^~~~~~~~~~~~~~~~~~~~~~~~
   drivers/regulator/max14577-regulator.c:208:35: note: (near initialization for 'max77836_ldo_ops.get_mode')

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for MFD_STMFX
   Depends on [n]: HAS_IOMEM [=y] && I2C [=y] && OF [=n]
   Selected by [m]:
   - PINCTRL_STMFX [=m] && PINCTRL [=y] && I2C [=y] && HAS_IOMEM [=y]


vim +208 drivers/regulator/max14577-regulator.c

   197	
   198	static const struct regulator_ops max77836_ldo_ops = {
   199		.is_enabled		= regulator_is_enabled_regmap,
   200		.enable			= max77836_ldo_enable,
   201		.disable		= max77836_ldo_disable,
   202		.list_voltage		= regulator_list_voltage_linear,
   203		.map_voltage		= regulator_map_voltage_linear,
   204		.get_voltage_sel	= regulator_get_voltage_sel_regmap,
   205		.set_voltage_sel	= regulator_set_voltage_sel_regmap,
   206		.get_mode		= max77836_ldo_get_mode,
   207		.set_mode		= max77836_ldo_set_mode,
 > 208		.of_map_mode		= max77836_ldo_of_map_mode,
   209	};
   210	

--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Lee Jones]

On Tue, 16 Jun 2026, Jad Keskes wrote:

> 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 +

Acked-by: Lee Jones <lee@kernel.org>

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

-- 
Lee Jones