From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 4D7A33B3BFE for ; Wed, 17 Jun 2026 07:00:00 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781679601; cv=none; b=eJjB9N3RbBEA5uUn+sozuts7PAyLlEiVr9KunB9dkTmRhwIUdmX3UxExvNLkOoyayAaGc5B0eqGBRnZF3x4MNklwPurISRTlQWvpmLmkAsW0WkzFW/b9FwqX0ZI3KvIXqlqh5Vyma8pkb+8F9VR/AOuVaRqgici+1ATTEWd9G9s= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781679601; c=relaxed/simple; bh=GD1Mx3N9E7K1g7tQZirPUCYAUJ1DS/vwj2R9HYKfDcY=; h=Date:From:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=PGQzJZ8GwZD47TVTbv4v5tzDSxiteEoT4rwrialkfS7xS7lk7xGPd8SHp4Za2MaPj2ECfXXLINAS90o2LWrZ6nfnWYHSmmbmXjb9qeD3uXsNxE1qu4/k+HQREDSrKI09T2JIKPl27luqYIz+7NGVuUZdPThbjMau0QUc/319PXM= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=jduN31wS; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="jduN31wS" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 69D071F000E9; Wed, 17 Jun 2026 06:59:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1781679599; bh=Qj5FNo/fu5SIaSPKNaRwcKCyxw8+enHqBtSdXCdrG0I=; h=Date:From:To:Cc:Subject:References:In-Reply-To; b=jduN31wSZ0VcT6ckRg7+QvDSOuajBaqZGYEnIF0fFMsFYDxovDXimZXr9n2wrMDhk 8vDpNk2dvpQIFJZ+qeq8JV13cUqqG/ybN9Ny+VJsSd5oJ1bNFeBbae+/v1uE0uInkr pm4MGrlEv7k7w30KNCg0eGhiwcOU/9xnwF2uWBH/iQTbq/3pmF/kLP3KakgKRHQoN4 eb078DE4Nt7IJZfLn7pK4SHiaM+dPVI2UKs+bXKM74bUM+XktHMMwIu0JwhU9Ao13K N0Ky7RLL5Gmp6oiMn7CDpuPKo/4nQLs6oumG8AKegpYgmE0cK3+akRwJOkWfOJdNWU EsB7xfGYI5/4g== Date: Wed, 17 Jun 2026 07:59:55 +0100 From: Lee Jones To: Jad Keskes Cc: Mark Brown , Chanwoo Choi , Krzysztof Kozlowski , Liam Girdwood , linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] regulator: max14577: fix set_mode clobbering enable on MAX77836 LDOs Message-ID: <20260617065955.GA10056@google.com> References: <20260616170256.1659595-1-inasj268@gmail.com> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20260616170256.1659595-1-inasj268@gmail.com> 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 > --- > 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 > 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