Re: [PATCH 2/2] regulator: histb: add voltage regulator driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] regulator: histb: set_voltage may apply voltage exceeding max_uV constraint
--

commit 800fca6de780a18cd0c58b4d9fc8681fea0944fb
Author: David Yang <mmyangfl@gmail.com>

regulator: histb: add voltage regulator driver
    
Add a driver for the voltage rails on HiSilicon STB SoCs which are
controlled by the PMOC block through a PWM signal, as described by the
hisilicon,histb-volt binding.

> diff --git a/drivers/regulator/histb-regulator.c b/drivers/regulator/histb-regulator.c
> new file mode 100644
> index 0000000000000..77073a646e493
> --- /dev/null
> +++ b/drivers/regulator/histb-regulator.c

[ ... ]

> +static int histb_regulator_set_voltage(struct regulator_dev *rdev, int min_uV,
> +				       int max_uV, unsigned int *selector)
> +{
> +	struct histb_regulator *priv = rdev_get_drvdata(rdev);
> +	unsigned int duty;
> +	u32 val;
> +
> +	if (min_uV >= priv->max_uV)
> +		duty = 0;
> +	else if (min_uV <= priv->min_uV)
> +		duty = HISTB_PWM_PERIOD;
> +	else
> +		/* rounding down keeps the output at or above the request */
> +		duty = DIV_ROUND_DOWN_ULL((u64)(priv->max_uV - min_uV) *
> +					  HISTB_PWM_PERIOD,
> +					  priv->max_uV - priv->min_uV);
> +
> +	val = FIELD_PREP(HISTB_DUTY, duty + 1) |
> +	      FIELD_PREP(HISTB_PERIOD, HISTB_PWM_PERIOD + 1);
> +	writel_relaxed(val, priv->base);
> +
> +	return 0;

[Severity: High]
Could this rounding logic result in applying an output voltage that exceeds
the max_uV limit?

Because a smaller duty cycle yields a higher physical voltage, rounding down
the duty cycle ensures the output voltage is at or above min_uV. However,
if the exact requested voltage range does not perfectly align with one of
the 240 PWM steps, the resulting physical voltage might exceed the strict
max_uV constraint requested by the consumer.

Should the driver verify if the achieved physical voltage violates max_uV
and return -EINVAL here instead of applying the setting and unconditionally
returning success?

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008155727.3801095-1-mmyangfl@gmail.com?part=2