Re: [PATCH v2 3/7] regulator: qcom-pm8008: Add PM8010 mode support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] regulator: qcom-pm8008: get_mode() reads status instead of control reg
--

commit b99c63e4d74725e0e0de61987ec984083cab186f
Author: Dhruvin Rajpura <drajpura@qti.qualcomm.com>

regulator: qcom-pm8008: Add PM8010 mode support

This commit implements set_mode and get_mode support for PM8010 LDOs using
the mode control and status registers. It allows clients to vote for LDO
modes, a feature that is not supported in the PM8008 LDOs.

> diff --git a/drivers/regulator/qcom-pm8008-regulator.c b/drivers/regulator/qcom-pm8008-regulator.c
> index b484c0f73eeac..ae5370770b2fc 100644
> --- a/drivers/regulator/qcom-pm8008-regulator.c
> +++ b/drivers/regulator/qcom-pm8008-regulator.c
[ ... ]
> @@ -134,6 +145,51 @@ static int pm8008_regulator_get_voltage_sel(struct regulator_dev *rdev)
>  	return regulator_map_voltage_linear_range(rdev, uV, INT_MAX);
>  }
>  
[ ... ]
> +static unsigned int pm8010_regulator_get_mode(struct regulator_dev *rdev)
> +{
> +	struct pm8008_regulator *preg = rdev_get_drvdata(rdev);
> +	unsigned int val;
> +	int ret;
> +
> +	ret = regmap_read(preg->regmap, preg->base + LDO_STATUS1_REG, &val);
> +	if (ret < 0)
> +		return REGULATOR_MODE_INVALID;
> +
> +	return (val & MODE_STATE_MASK) == MODE_STATE_NPM ?
> +		REGULATOR_MODE_NORMAL : REGULATOR_MODE_IDLE;
> +}

[Severity: Medium]
Does reading LDO_STATUS1_REG instead of the control register
(LDO_MODE_CTL1_REG) here break the regulator core's set_mode optimization
when the LDO is disabled?

When the LDO is disabled, the physical status register indicates an OFF state
(evaluating to 0), which incorrectly falls through to return
REGULATOR_MODE_IDLE.

If a consumer calls regulator_set_mode() with REGULATOR_MODE_IDLE while the
regulator is disabled, the core compares the requested mode with the returned
get_mode() value. Seeing a match, it skips writing the new mode to the control
register. Later, when the regulator is enabled, it powers up using the stale
mode in LDO_MODE_CTL1_REG (e.g., REGULATOR_MODE_NORMAL), completely ignoring
the consumer's earlier REGULATOR_MODE_IDLE request.

Should pm8010_regulator_get_mode() read LDO_MODE_CTL1_REG instead to
accurately reflect the configured state rather than the dynamic physical
status?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-pm8010_i2c_support-v2-0-b395d2b3f668@oss.qualcomm.com?part=3