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

[Mark Brown]

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.

> +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.

> +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?

> +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?

> +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).

Attachment: signature.asc
Description: PGP signature