Re: [PATCH 5/5] regulator: Add TI LM51772 support

[Paul Louvel]

Hi Mark,

On Thu Oct 22, 2026 at 7:34 PM CEST, Mark Brown wrote:
> On Wed, Oct 21, 2026 at 12:27:46PM +0200, Paul Louvel wrote:
>
>> +static int lm51772_regulator_set_voltage_sel(struct regulator_dev *rdev, unsigned int selector)
>> +{
>> +	const struct lm51772_priv *priv = rdev_get_drvdata(rdev);
>> +	int ret;
>
>> +	/*
>> +	 * The LM51772 uses hardcoded OVP thresholds. Update the OVP threshold
>> +	 * to match the new voltage.
>> +	 */
>> +	if (priv->ovp_thres) {
>> +		ret = regulator_list_voltage_pickable_linear_range(rdev, selector);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		ret = lm51772_set_ov(rdev, ret + priv->ovp_thres);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return regulator_set_voltage_sel_pickable_regmap(rdev, selector);
>
> If the voltage is being lowered this will lower the threashold prior to
> lowering the voltage which could cause the protection to flag.  I guess
> we need to look to see if the voltage is being raised or lowered and do
> the change before or after as appropriate.
>

If new_uV > old_uV, and we set ovp after or before, that doesn't matter, because
the voltage only ramp up without getting a chance to be > ovp.
If new_uV < old_uV, if we set ovp before, the protection will flag, and if we
set ovp after, the protection might still flag because of the slew rate delay
(or put another way, there is no guarantee that the voltage will be low enough
so that the ovp don't trip).

My idea is to implement the .set_voltage_time() callback, to inform the core how
much time it will take for the regulator to reach the new voltage, to that we
fsleep() for the correct amount of time [1].
Then, the core could call .set_over_voltage_protection() if the driver enabled a
new 'fixed_ovp' flag:

--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -4014,6 +4014,16 @@ static int _regulator_do_set_voltage(struct regulator_dev *rdev,
 				     (void *)data);
 	}
 
+	if (old_uV != best_val) {
+		if (rdev->desc->fixed_ovp && ops->set_over_voltage_protection) {
+			ret = handle_notify_limits(
+				rdev, ops->set_over_voltage_protection,
+				&rdev->constraints->over_voltage_limits);
+			if (ret == -EOPNOTSUPP)
+				ret = 0;
+		}
+	}
+

[1]: https://elixir.bootlin.com/linux/v7.3/source/drivers/regulator/core.c#L4005

>> +static int lm51772_regulator_set_current_limit(struct regulator_dev *rdev, int min_uA, int max_uA)
>> +{
>> +	struct lm51772_priv *priv = rdev_get_drvdata(rdev);
>> +	u64 voltage_uV = ((u64)priv->sense_res_milliohms * max_uA) / 1000;
>
> Does this build on arm32 or similar things that need a libgcc division
> implmentation?  div_u64().
>
>> +	if (status_reg & LM51772_STAT_INPUT)
>> +		errors |= REGULATOR_ERROR_UNDER_VOLTAGE;
>
> That sounds like an error with the input so I'm not sure under voltage
> is the right error to report.

Since there is no error for input under voltage, I guess I can use
REGULATOR_ERROR_FAIL then ?

>
>> +static int lm51772_irq_map_event(int irq, struct regulator_irq_data *rid,
>> +			     unsigned long *dev_mask)
>> +{
>> +	struct regmap *regmap = rid->data;
>> +	unsigned int reg, errors;
>> +	int ret;
>> +
>> +	ret = regmap_read(regmap, LM51772_REG_STATUS_BYTE, &reg);
>> +	if (ret)
>> +		return REGULATOR_FAILED_RETRY;
>> +
>> +	errors = lm51772_status2err(reg);
>
> Does this do the right thing if multiple errors are flagged?
>

No.. should iterate with for_each_set_bit() on errors and then do
regulator_err2notif() on each bit.

>> +static int lm51772_irq_renable(struct regulator_irq_data *rid)
>> +{
>> +	struct regmap *regmap = rid->data;
>> +	unsigned int reg;
>> +	int ret;
>> +
>> +	ret = regmap_read(regmap, LM51772_REG_STATUS_BYTE, &reg);
>> +	if (ret)
>> +		return REGULATOR_FAILED_RETRY;
>> +
>> +	if (rid->opaque && rid->opaque == reg) {
>> +		ret = regmap_read(regmap, LM51772_REG_CLEAR_FAULTS, &reg);
>> +		if (ret)
>> +			return REGULATOR_FAILED_RETRY;
>
> Shoud _CLEAR_FAULTS be volatile?
>

Yes.

>> +static const struct reg_default lm51772_reg_defaults[] = {
>> +	{ LM51772_REG_CLEAR_FAULTS, 0x00 },	{ LM51772_REG_ILIM_THRESHOLD, 0x64 },
>> +	{ LM51772_REG_VOUT_TARGET1_LSB, 0x58 }, { LM51772_REG_VOUT_TARGET1_MSB, 0x02 },
>> +	{ LM51772_REG_STATUS_BYTE, 0x00 },	{ LM51772_REG_USB_PD_CONTROL_0, 0x01 },
>
> STATUS_BYTE is volatile so a default isn't terribly sensible (but
> harmless).

Thanks.
Best regards,
-- 
Paul Louvel, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com