Re: [PATCH v1 5/7] regulator: lp872x: Remove platform_data based probing

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] regulator: lp872x: uninitialized stack variable general_config write
- [High] regulator: lp872x: ABI breakage due to changed GPIO connection ID
- [Medium] regulator: lp872x: unconditional 30ms sleep in probe path
--

commit ffba7b7b46bbae180cc6ce91332a272bc6093ec2
Author: Svyatoslav Ryhel <clamor95@gmail.com>
Subject: regulator: lp872x: Remove platform_data based probing

This commit removes platform_data-based probing from the lp872x regulator
driver, transitioning fully to Device Tree parsing directly in the probe
function. It also consolidates DVS initialization and drops the use of
of_regulator_match in favor of the core registration framework.

> diff --git a/drivers/regulator/lp872x.c b/drivers/regulator/lp872x.c
> index 1e16d7e8361a0..3d95ca7fa0246 100644
> --- a/drivers/regulator/lp872x.c
> +++ b/drivers/regulator/lp872x.c
[ ... ]
> @@ -700,110 +688,45 @@ static const struct regulator_desc lp8725_regulator_desc[] = {
>  
>  static int lp872x_init_dvs(struct lp872x *lp)
>  {
[ ... ]
> -	dvs->gpio = devm_gpiod_get_optional(lp->dev, "ti,dvs", pinstate);
[ ... ]
> +	lp->dvs_gpio = devm_gpiod_get_optional(dev, "dvs", lp->dvs_pin);

[Severity: High]
Does this change in connection ID break compatibility with existing Device
Trees?

The original code searched for ti,dvs-gpios by using the ti,dvs connection
ID. Changing this to dvs causes the GPIO framework to search for
dvs-gpios instead. Existing device trees using the documented
ti,dvs-gpios property will fail to find the GPIO, causing a fallback to
register-based DVS.

[ ... ]
> @@ -817,152 +740,77 @@ static const struct regmap_config lp872x_regmap_config = {
[ ... ]
>  static int lp872x_probe(struct i2c_client *cl)
>  {
[ ... ]
> +	u8 general_config;
>  	int ret;
[ ... ]
> +	/* Always set enable GPIO high. */
> +	lp->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_HIGH);
> +	if (IS_ERR(lp->enable_gpio))
> +		return dev_err_probe(dev, PTR_ERR(lp->enable_gpio),
> +				     "Failed to request gpio\n");
> +
> +	/* Each chip has a different enable delay. */
> +	if (lp->info->chipid == LP8720)
> +		usleep_range(LP8720_ENABLE_DELAY, 1.5 * LP8720_ENABLE_DELAY);
> +	else
> +		usleep_range(LP8725_ENABLE_DELAY, 1.5 * LP8725_ENABLE_DELAY);

[Severity: Medium]
Should this delay be conditional on whether an enable GPIO was actually
found?

In the previous implementation, if there was no enable GPIO, it returned
early and skipped the delay. Now, devm_gpiod_get_optional() returns NULL if
no GPIO is configured, but the delay runs unconditionally. This adds an
unnecessary 30ms sleep during boot for LP8725 platforms without an enable
GPIO.

>  
> -	ret = lp872x_config(lp);
> +	device_property_read_u8(dev, "ti,general-config", &general_config);
> +	if (device_property_read_bool(dev, "ti,update-config")) {
> +		ret = regmap_write(lp->regmap, LP872X_GENERAL_CFG, general_config);

[Severity: High]
Can uninitialized stack memory be written to the hardware here?

If a device tree specifies ti,update-config but omits ti,general-config,
device_property_read_u8() will fail and leave general_config untouched. 
Because general_config is declared on the stack without initialization, this
leads to uninitialized memory being written to the LP872X_GENERAL_CFG
register.

> +		if (ret)
>  			return ret;
> +	}
>  
> -	ret = lp872x_init_dvs(lp);
> +	ret = lp872x_init_dvs(lp);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006154431.318936-1-clamor95@gmail.com?part=5