Re: [PATCH v3 3/3] regulator: Add TI LM51772 support

[Paul Louvel]

Hi Mark,

On Fri Oct 30, 2026 at 5:21 PM CET, Mark Brown wrote:
>> Add driver for the TI LM51772 buck-boost voltage regulator.
>
>> @@ -0,0 +1,903 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * TI LM51772 I2C regulator driver
>> + *
>> + * Datasheet: SNVSC22D (September 2025)
>> + *
>> + * Copyright (c) 2026 Bootlin
>> + */
>
> Please make the entire comment block a C++ one so things look more
> intentional.
>
>> +static int lm51772_irq_map_event(int irq, struct regulator_irq_data *rid, unsigned long *dev_mask)
>> +{
>
>> +	ret = regmap_read(regmap, LM51772_REG_CLEAR_FAULTS, &reg);
>> +	if (ret)
>> +		return REGULATOR_FAILED_RETRY;
>
> This register appears to be clear on read so it should be marked as
> precious, otherwise debugfs will eat notifications.
>
>> +/*
>> + * For < 24V, voltages are set in 10mV monotonous steps.
>> + * For >= 24V, voltages are set in 20mV monotonous steps.
>> + */
>> +static const struct linear_range lm51772_voltage_ranges[] = {
>> +	REGULATOR_LINEAR_RANGE(LM51772_DIV10_MIN_UV, 0, 99, 0),
>> +	REGULATOR_LINEAR_RANGE(LM51772_DIV10_MIN_UV, 100, 2399, LM51772_DIV10_STEP_UV),
>
> Looks like this ought to cover 2400 too since there's overlap between
> the two ranges?  If we don't have it then the driver will get upset if
> it finds the hardware with that configuration when it starts.
>
>> +	  .active_discharge_mask = LM51772_FORCE_DISCH,
>> +	  .active_discharge_on = FIELD_PREP_CONST(LM51772_FORCE_DISCH, 1),
>> +	  .active_discharge_off = FIELD_PREP_CONST(LM51772_FORCE_DISCH, 0),
>
> It looks like this is an always on load on the output rather than
> something only enabled when the regulator has been disabled so we'll end
> up with an extra power drain for no reason.  Possibly we should have
> core support for hardware that needs such manual management, though the
> datasheet suggests DICHARG_CONFIG0/1 might be more appropriate for what
> the API is looking for here?

Yes indeed. I see no reason for the DISCHARGE_CONFIG0/1 to be disabled. At
first, I thought that the FORCE_DISH was necessary to get the discharge on DVS
ramp down.

By manual management, do you mean selecting between those three configuration
(force discharge, discharge only when disabled, disacharge only when disabled
and until a certain threshold) ?

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