Re: [PATCH v2 4/4] regulator: Add TI LM51772 support

["Paul Louvel" <paul.louvel@bootlin.com>]

On Tue Oct 27, 2026 at 11:01 AM CET, sashiko-bot wrote:
> Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
> - [High] regulator: lm51772: use logical AND in lm51772_get_conv_en()
> - [High] regulator: lm51772: fix current limit bounds and max_uA check
> - [High] regulator: lm51772: fix STAT_OFF and fault checks in IRQ callbacks
> - [Medium] regulator: lm51772: fix cached errors and IRQ order in lm51772_setup_irq()
> - [Medium] regulator: lm51772: allow OVP limits below 4V in lm51772_set_ov()
> - [Medium] regulator: lm51772: OVP updated after voltage ramp-up trips false OVP fault
> - [Medium] regulator: lm51772: state mismatch between priv->iset_pin and hardware default
> --
>
> commit 85d9f9bf5e7e89e913ab51cc149fc605a17772db
> Author: Paul Louvel <paul.louvel@bootlin.com>
>
> regulator: Add TI LM51772 support
>
> This commit adds a regulator driver for the TI LM51772 I2C buck-boost
> voltage regulator and its auxiliary 5 V LDO output.
>
>> diff --git a/drivers/regulator/lm51772-regulator.c b/drivers/regulator/lm51772-regulator.c
>> new file mode 100644
>> index 0000000000000..66f4541b50994
>> --- /dev/null
>> +++ b/drivers/regulator/lm51772-regulator.c
>
> [ ... ]
>
>> +static int lm51772_set_conv_en(struct regmap *regmap, bool enable)
>> +{
>> +	int ret;
>> +
>> +	ret = regmap_update_bits(regmap, LM51772_REG_USB_PD_CONTROL_0, LM51772_CONV_EN2,
>> +				 FIELD_PREP(LM51772_CONV_EN2, enable));
>> +	if (ret)
>> +		return ret;
>> +
>> +	return regmap_update_bits(regmap, LM51772_REG_MFR_SPECIFIC_D0, LM51772_CONV_EN,
>> +				  FIELD_PREP(LM51772_CONV_EN, enable));
>> +}
>> +
>> +static int lm51772_get_conv_en(struct regmap *regmap)
>> +{
>> +	unsigned int pd_ctrl, cfg0;
>> +	int ret;
>> +
>> +	ret = regmap_read(regmap, LM51772_REG_USB_PD_CONTROL_0, &pd_ctrl);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_read(regmap, LM51772_REG_MFR_SPECIFIC_D0, &cfg0);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return FIELD_GET(LM51772_CONV_EN2, pd_ctrl) || FIELD_GET(LM51772_CONV_EN, cfg0);
>> +}
>
> [Severity: High]
> Should lm51772_get_conv_en() use && instead of || here?
>
> In lm51772_reg_defaults (and at hardware reset), LM51772_REG_USB_PD_CONTROL_0
> is 0x01 (so LM51772_CONV_EN2 is 1) while LM51772_REG_MFR_SPECIFIC_D0 is 0x32
> (so LM51772_CONV_EN is 0), and lm51772_set_conv_en() must set both bits to
> enable the converter.
>
> When a consumer calls regulator_enable() on vout for the first time:
>
>   _regulator_enable()
>     _regulator_is_enabled()
>       lm51772_regulator_is_enabled()
>         lm51772_get_conv_en()
>
> Because lm51772_get_conv_en() uses ||, it returns 1 while LM51772_CONV_EN is
> still 0, causing _regulator_enable() to treat vout as already enabled and
> skip calling _regulator_do_enable().
>
> [ ... ]
>

No. It might need a comment in the code to clarify this.
Both bits are internally OR'ed, so whenever one of them is set, the converter is
enabled.
When enabling / disabling, we update both bits, just to be consistent.

>> +static int lm51772_set_ov(struct regulator_dev *rdev, int lim_uV)
>> +{
>> +	static const struct linear_range ovp_ranges[] = {
>> +		LINEAR_RANGE(4000000, 0, 0x18, 500000),
>> +		LINEAR_RANGE(16000000, 0x18, 0x3f, 1000000),
>> +	};
>> +	struct regmap *regmap = rdev_get_regmap(rdev);
>> +	struct device *dev = rdev_get_dev(rdev);
>> +	unsigned int sel;
>> +	bool found;
>> +	int ret;
>> +
>> +	ret = linear_range_get_selector_high_array(ovp_ranges, ARRAY_SIZE(ovp_ranges), lim_uV, &sel,
>> +						   &found);
>> +	if (ret || !found)
>> +		return -EINVAL;
>
> [Severity: Medium]
> Does checking !found in lm51772_set_ov() reject valid over-voltage protection
> thresholds when the target voltage plus lim_uV is below 4.0 V?
>
> Because vout supports output voltages down to 1.0 V while ovp_ranges starts
> at 4.0 V (4000000 uV), calling linear_range_get_selector_high_array() with
> lim_uV < 4000000 sets sel = 0 (4.0 V, the lowest available upper-bound OVP
> threshold), sets found = false, and returns 0.
>
> Checking !found causes lm51772_set_ov() to return -EINVAL instead of
> selecting 4.0 V, and since fixed_ovp is set on vout, _regulator_do_set_voltage()
> will also fail with -EINVAL whenever vout + lim_uV is under 4.0 V.
>

Yes, setting it to 4.0V should be the correct behavior.

>> +
>> +	dev_dbg(dev, "OVP set to selector 0x%02x\n", sel);
>> +
>> +	return regmap_write(regmap, LM51772_REG_MFR_SPECIFIC_D5, sel);
>> +}
>> +
>> +static int lm51772_regulator_set_over_voltage_protection(struct regulator_dev *rdev, int lim_uV,
>> +							 int severity, bool enable)
>> +{
>> +	int ret;
>> +
>> +	if (!enable)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (severity != REGULATOR_SEVERITY_PROT)
>> +		return -EOPNOTSUPP;
>> +
>> +	/* Use default OVP threshold when no limit is requested. */
>> +	if (!lim_uV)
>> +		return regmap_write(rdev_get_regmap(rdev), LM51772_REG_MFR_SPECIFIC_D5,
>> +				    LM51772_OVP_DEFAULT);
>> +
>> +	ret = regulator_get_voltage_rdev(rdev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return lm51772_set_ov(rdev, ret + lim_uV);
>> +}
>
> [Severity: Medium]
> When regulator-ov-protection-microvolt is configured and vout is raised to a
> new voltage above the previous absolute OVP threshold, can this trip a false
> hardware OVP fault during the voltage ramp?
>
> With fixed_ovp enabled on vout in lm51772_regulator_descs,
> _regulator_do_set_voltage() first writes the new voltage selector via
> ops->set_voltage_sel() and sleeps for fsleep(delay) while the output slews,
> and only afterwards invokes ops->set_over_voltage_protection():
>
>   _regulator_do_set_voltage()
>     ops->set_voltage_sel()
>     fsleep(delay)
>     handle_notify_limits(..., ops->set_over_voltage_protection, ...)
>       lm51772_regulator_set_over_voltage_protection()
>
> When stepping vout up (for example from 5 V to 20 V with a 1 V OVP margin,
> where LM51772_REG_MFR_SPECIFIC_D5 is initially set to 6 V), the converter
> ramps past 6 V during fsleep(delay) before LM51772_REG_MFR_SPECIFIC_D5 is
> updated to 21 V. Should the OVP threshold be raised before increasing the
> output voltage?
>
> [ ... ]
>

Described in last review.

>> +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);
>> +	struct regmap *regmap = rdev_get_regmap(rdev);
>> +	unsigned int current_uA;
>> +	unsigned int sel;
>> +
>> +	if (priv->iset_pin)
>> +		return -EOPNOTSUPP;
>> +
>> +	if (min_uA > LM51772_ILIM_MAX_UA || max_uA < LM51772_ILIM_MIN_UA)
>> +		return -EINVAL;
>> +
>> +	if (max_uA > LM51772_ILIM_MAX_UA)
>> +		max_uA = LM51772_ILIM_MAX_UA;
>> +
>> +	sel = LM51772_UA_TO_ILIM_SEL(priv->sense_res_micro_ohms, max_uA);
>> +	sel = clamp_val(sel, LM51772_ILIM_THR_MIN, LM51772_ILIM_THR_MAX);
>> +
>> +	/* Check if the selected value is still within the allowed range after computation. */\n> +	current_uA = LM51772_ILIM_SEL_TO_UA(priv->sense_res_micro_ohms, sel);
>> +	if (current_uA < min_uA)
>> +		return -EINVAL;
>
> [Severity: High]
> Can clamping sel up to LM51772_ILIM_THR_MIN cause
> lm51772_regulator_set_current_limit() to program a current limit higher than
> max_uA when a smaller sense resistor is used?
>
> For example, if shunt-resistor-micro-ohms is 5000 (where LM51772_ILIM_THR_MIN
> 0x0A corresponds to 1000000 uA) and the caller requests min_uA = 500000 and
> max_uA = 600000, LM51772_UA_TO_ILIM_SEL() computes sel = 6, which clamp_val()
> raises to 0x0A. Recomputing current_uA gives 1000000 uA, and because only
> current_uA < min_uA is checked (without checking current_uA > max_uA), the
> driver programs 1.0 A into LM51772_REG_ILIM_THRESHOLD, exceeding max_uA.
>
> Also, LM51772_ILIM_MIN_UA and LM51772_ILIM_MAX_UA hardcode the current range
> for a 10 mOhm resistor rather than scaling with priv->sense_res_micro_ohms,
> which rejects valid selectors when a different sense resistor is configured.
>
> [ ... ]
>

Yes. I should use a linear range array to describe which value this register can
take. Clamping was lazy and wrong.

>> +static int lm51772_irq_map_event(int irq, struct regulator_irq_data *rid, unsigned long *dev_mask)
>> +{
>> +	struct regmap *regmap = rid->data;
>> +	unsigned int status, reg, errors;
>> +	bool dev_off;
>> +	size_t bit;
>> +	int ret;
>> +
>> +	ret = regmap_read(regmap, LM51772_REG_STATUS_BYTE, &status);
>> +	if (ret)
>> +		return REGULATOR_FAILED_RETRY;
>> +
>> +	errors = lm51772_status2err(status);
>> +	rid->opaque = status;
>> +
>> +	dev_off = (status & LM51772_STAT_OFF) != 0;
>> +
>> +	ret = regmap_read(regmap, LM51772_REG_CLEAR_FAULTS, &reg);
>> +	if (ret)
>> +		return REGULATOR_FAILED_RETRY;
>> +
>> +	for (size_t i = 0; i < rid->num_states; i++) {
>> +		rid->states[i].errors = errors & rid->states[i].possible_errs;
>> +		rid->states[i].notifs = 0;
>> +
>> +		for_each_set_bit(bit, &rid->states[i].errors, BITS_PER_TYPE(rid->states[i].errors))
>> +			rid->states[i].notifs |= regulator_err2notif(BIT(bit));
>> +
>> +		if (dev_off)
>> +			rid->states[i].notifs |= REGULATOR_EVENT_DISABLE;
>> +
>> +		if (rid->states[i].errors || dev_off)
>> +			*dev_mask |= BIT(i);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +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;
>> +
>> +		return REGULATOR_ERROR_ON;
>> +	}
>> +
>> +	rid->opaque = 0;
>> +
>> +	return REGULATOR_ERROR_CLEARED;
>> +}
>
> [Severity: High]
> Are LM51772_STAT_OFF and rid->opaque handled as intended in
> lm51772_irq_map_event() and lm51772_irq_renable()?
>
> In lm51772_irq_map_event(), dev_off reflects LM51772_STAT_OFF for the main
> buck-boost converter (vout), including when vout is simply disabled in
> software. However, the loop applies dev_off to all regulators in
> rid->num_states, which sends REGULATOR_EVENT_DISABLE to the auxiliary vcc1
> LDO (and to vout on warning interrupts while vout is software-disabled).
>
> In lm51772_irq_renable(), comparing the raw status byte with
> rid->opaque == reg has two issues:
>
> 1. If an interrupt occurs when status is only LM51772_STAT_OFF (for example,
> an INPUT/VIN_UV event while vout is off), reading LM51772_REG_CLEAR_FAULTS
> does not clear LM51772_STAT_OFF, so rid->opaque == reg stays true and
> lm51772_irq_renable() returns REGULATOR_ERROR_ON in an infinite polling loop.
>

This is the intended behavior.
renable is only called when the pin act as nFLT. When the unit is off, the OFF
bit will be set, until the unit is enabled again.
In the mean time, the IRQ have to be disabled, so REGULATOR_ERROR_ON must be
returned. This is not an infinite loop if may be turned on again, otherwise,
this is a intended infinite loop.

> 2. If multiple status bits were set and one bit changes while other fault
> bits remain active in reg, rid->opaque == reg evaluates to false, skipping
> LM51772_REG_CLEAR_FAULTS and prematurely returning REGULATOR_ERROR_CLEARED.
>

Yes and this is the intended behavior.
If a bit change we want to have an irq and map event again.
LM51772_REG_CLEAR_FAULTS will be called.

>> +
>> +static int lm51772_vout_of_parse(struct device_node *np, const struct regulator_desc *desc,
>> +				 struct regulator_config *cfg)
>> +{
>> +	u32 sense_res_micro_ohms, active_discharge_strength, pcm_window_low;
>> +	struct lm51772_priv *priv = cfg->driver_data;
>> +	int ret;
>
> [ ... ]
>
>> +	priv->iset_pin = of_property_read_bool(np, "ti,iset-pin");
>
> [Severity: Medium]
> What happens to priv->iset_pin if the optional regulators/vout DT node is
> absent?
>
> In lm51772_reg_defaults, LM51772_REG_MFR_SPECIFIC_D9 defaults to 0x2C, which
> has LM51772_SEL_ISET_PIN (BIT(5)) set to 1 (selecting the external ISET pin).
> However, priv->iset_pin is zero-initialized to false in lm51772_probe() and
> LM51772_SEL_ISET_PIN is only updated in lm51772_vout_of_parse().
>
> If the vout child node is omitted (or on non-DT instantiation via
> lm51772_id), regulator_of_get_init_data() does not invoke
> lm51772_vout_of_parse(), leaving LM51772_SEL_ISET_PIN set to 1 in hardware
> while priv->iset_pin is false in software. As a result,
> lm51772_regulator_get_current_limit() and
> lm51772_regulator_set_current_limit() will read and write the inactive
> LM51772_REG_ILIM_THRESHOLD register instead of returning -EOPNOTSUPP (or
> clearing LM51772_SEL_ISET_PIN).
>
> [ ... ]
>

Yes.

>> +static int lm51772_setup_irq(struct device *dev, struct regmap *regmap, int irq,
>> +			     struct regulator_dev *rdev[LM51772_N_REGS], bool nint)
>> +{
>> +	int common_errs = REGULATOR_ERROR_OVER_TEMP | REGULATOR_ERROR_OVER_TEMP_WARN |
>> +			  REGULATOR_ERROR_FAIL;
>> +	int per_rdev_errs[] = {
>> +		REGULATOR_ERROR_OVER_CURRENT | REGULATOR_ERROR_OVER_VOLTAGE_WARN,
>> +		0,
>> +	};
>> +	struct regulator_irq_desc irq_desc = {
>> +		.name = "lm51772-irq",
>> +		.data = regmap,
>> +		.map_event = lm51772_irq_map_event,
>> +	};
>> +	void *retp;
>> +
>> +	if (!nint) {
>> +		irq_desc.renable = lm51772_irq_renable;
>> +		irq_desc.irq_off_ms = 1000;
>> +		irq_desc.reread_ms = 200;
>> +	}
>> +
>> +	retp = devm_regulator_irq_helper(dev, &irq_desc, irq, 0, common_errs, per_rdev_errs, rdev,
>> +					 LM51772_N_REGS);
>> +	if (IS_ERR(retp))
>> +		return PTR_ERR(retp);
>> +
>> +	return regmap_update_bits(regmap, LM51772_REG_MFR_SPECIFIC_D1, LM51772_EN_NINT,
>> +				  FIELD_PREP(LM51772_EN_NINT, nint));
>> +}
>
> [Severity: Medium]
> When nint is true, how do cached error flags in rdev->cached_err get cleared?
>
> Because common_errs and per_rdev_errs are passed to
> devm_regulator_irq_helper(), init_rdev_errors() sets rdev->use_cached_err =
> true, and regulator_notifier_isr() latches any reported errors via
> rdev_flag_err(). However, rdev_clear_err() is only called from
> regulator_notifier_isr_work(), which is only scheduled when
> irq_desc.irq_off_ms is non-zero. With irq_off_ms left at 0 when nint is true,
> _regulator_get_error_flags() will permanently OR the latched rdev->cached_err
> flags into every subsequent regulator_get_error_flags() call even after the
> fault clears.
>

Will be fixed by keeping irq_off_ms, but set the renable callback to NULL when
nint is true.

> Also, should LM51772_EN_NINT be configured via regmap_update_bits() before
> calling devm_regulator_irq_helper() so the IRQ handler is not active while
> the pin is still in its default nFLT mode?

Would be better, yes.

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