Re: [PATCH] Revert "power: regulator: Add vin-supply for GPIO and Fixed regulators"

[Quentin Schulz <quentin.schulz@cherry.de>]

On 11/3/25 11:36 AM, Jonas Karlman wrote:
> Hi Peng,
> 
> On 11/3/2025 9:51 AM, Peng Fan wrote:
>> On Sat, Nov 01, 2025 at 08:34:26PM +0000, Jonas Karlman wrote:
>>> Rockchip boards may depend on a working MMC regulator in SPL to
>>> successfully load FIT payload from MMC. Typically, these boards only
>>> include the vmmc-supply regulator and not its vin-supply in SPL control
>>> FDT.
>>>
>>> The commit f98d812e5353 ("power: regulator: Add vin-supply for GPIO and
>>> Fixed regulators") breaks loading FIT from MMC in SPL on some of these
>>> boards due to now requiring the vin-supply to be included in the SPL
>>> control FDT.
>>>
>>> The commit also strangely enables any found vin-supply in
>>> regulator_common_of_to_plat() and not when a regulator is enabled or as
>>> part of regulator_autoset().
>>>
>>> Revert the commit to fix FIT loading in SPL on broken boards.
>>>
>>> If a board needs to have its vin-supply enabled, two options come to
>>> mind:
>>>
>>> - Add regulator-always-on prop to the regulator in the -u-boot.dtsi for
>>>   any board.
>>>
>>> - Implement full support for reference counting of regulators and then
>>>   update the regulator-uclass to enable any found vin-supply when a
>>>   regulator is enabled.
>>>
>>> This reverts commit f98d812e5353408ef77a46bad1f1cdc793ff8a03.
>>>
>>> Reported-by: Dang Huynh <danct12@riseup.net>
>>> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
>>> ---
>>> drivers/power/regulator/regulator_common.c | 10 ----------
>>> drivers/power/regulator/regulator_common.h |  1 -
>>> 2 files changed, 11 deletions(-)
>>>
>>> diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c
>>> index 3ed713ce5019..685d8735fa5c 100644
>>> --- a/drivers/power/regulator/regulator_common.c
>>> +++ b/drivers/power/regulator/regulator_common.c
>>> @@ -44,16 +44,6 @@ int regulator_common_of_to_plat(struct udevice *dev,
>>> 			dev_read_u32_default(dev, "u-boot,off-on-delay-us", 0);
>>> 	}
>>>
>>> -	ret = device_get_supply_regulator(dev, "vin-supply", &plat->vin_supply);
>>> -	if (ret) {
>>> -		debug("Regulator vin regulator not defined: %d\n", ret);
>>> -		if (ret != -ENOENT)
>>
>> I understand it might be not proper to enable vin-supply in
>> function regulator_common_of_to_plat().
>> But I have a question, there is -ENOENT check in the original patch. If the
>> board does not have vin-supply, there should be no issue. Or I miss something?
> 
> As mentioned in the commit message the SPL control FDT does not contain
> the regulator references as a vin-supply, this result in -ENODEV to be
> returned. I.e. following where the vcc_io is not included in SPL control
> FDT.
> 
>    vcc_io: regulator@ {
>    }
>    vcc_sd: regulator@ {
>    	bootph-pre-ram;
>    	vin-supply = <&vcc_io>;
>    }
>    sdmmc: mmc@ {
>    	bootph-pre-ram;
>    	vmmc-supply = <&vcc_sd>;
>    }
> 

This needs to be fixed for SPL FDT for all impacted boards then 
(Whenever the patch adding the enabling of vin-supply is resent). We 
need the same bootph props for the node pointed at by vin-supply.

> With the change introduced in this commit the core gpio/fixed regulator
> behavior changed and this now result in unbootable boards.
> 
> Other reasons why I prefer a full revert instead of a workaround:
> - The implementation changed core gpio/fixed regulator function without
>    any review-by or acked-by.
> - The patch was quickly merged after only being on the list for 1 week,
>    the imx pull-request that included this did not even mention regulator
>    changes.
> - The implementation is not optimal, looking up vin-supply could be okay
>    but not failing because it is missing, and enabling it during probe

If the property is missing, then we shouldn't fail of course, since the 
property isn't required. If the property is there but it points to an 
empty node, it should fail? Since the vin-supply could be one that needs 
to do something (e.g. a gpio-regulator whose default state is off). If 
it's there but cannot be probed (e.g. gpio-regulator but the gpio 
controller of the enable GPIO isn't available in the stage), we should 
fail as well.

>    when we already have working enable count and regulator autoset to
>    handle any needed auto-enable.
> - The commit message does not give any insights on why this is needed.
> 
> My suggestion is that this is reverted and if needed it should can be
> submitted as a separated patch/series with a proper implementation.
> 

Agreed.

1) vin-supply isn't a regulator property. It is a fixed-regulator and 
gpio-regulator property so it should be handled in those drivers and not 
in the function common to all regulator drivers,
2) Parsing the property and mapping in *_of_to_plat is fine, but 
enabling should be done when enabling the regulator, not when parsing 
the FDT,

Cheers,
Quentin