[PATCH v2 1/3] arm64: dts: rockchip: add "dcin" regulator for Radxa ROCK 5C

[Dragan Simic]

Hello Fukaumi,

On 2024-11-28 02:03, FUKAUMI Naoki wrote:
> On 11/28/24 09:39, Dragan Simic wrote:
>> On 2024-11-19 11:08, FUKAUMI Naoki wrote:
>>> add "dcin" label to vcc5v_dcin regulator and use it in vcc_sysin
>>> regulator.
>>> 
>>> Signed-off-by: FUKAUMI Naoki <naoki at radxa.com>
>>> ---
>>> Changes in v2:
>>> - none
>>> ---
>>>  arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 4 ++--
>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>> b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>> index 6da13b6b9a7b..b5460c179ef7 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>> @@ -88,7 +88,7 @@ vcc3v3_pcie2x1l2: regulator-vcc3v3_pcie2x1l2 {
>>>          vin-supply = <&vcc_sysin>;
>>>      };
>>> 
>>> -    vcc5v_dcin: regulator-vcc5v-dcin {
>>> +    vcc5v_dcin: dcin: regulator-vcc5v-dcin {
>> 
>> I just went through the associated part of the ROCK 5C schematic,
>> and I was unable to see any reasons why should we introduce "dcin"
>> as another alias here?
>> 
>> The root of the ROCK 5C's power tree is labeled "5v_dcin" in the
>> schematic, so renaming "vcc5v_dcin" to "5v_dcin" would make sense,
>> but I don't see why should we add another alias.
> 
> ROCK 5A has vcc12_dcin which supply power to vcc_sysin. both
> vcc5v_dcin on ROCK5C and vcc12v_dcin on ROCK 5C have additional "dcin"
> label for sharing .dtsi. (please check PATCH 3/3)

Hmm, I don't think that's the best way to achieve the shared nature
of the .dtsi file.  I think that an acceptable way to achieve that
would be to just rename "vcc5v_dcin" to "vbus_typec" (and rename
"regulator-vcc5v-dcin" to "regulator-vbus-typec" as well), because
"vbus_typec" is actually used in the ROCK 5A schematic.  Though, it
isn't used in the ROCK 5C schematic, but should still be obvious to
anyone wanting to correlate the DT and the schematic.

It should also be described better in the patch description, which
is simply too terse in its current form.  Also, this patch series
should have a cover letter that describes its purpose.

>>>          compatible = "regulator-fixed";
>>>          regulator-name = "vcc5v_dcin";

Here, "vcc5v_dcin" should also be renamed to "vbus_typec" in the
shared .dtsi file, which is rather obvious.

Also, the associated regulator-min-microvolt and regulator-max-
microvolt properties should be dropped in the shared .dtsi file
and moved to the ROCK 5A and 5C board .dts files, because the
ROCK 5A and 5C schematics indicate that different VBUS voltages
(12 V and 5 V, respectively) are used.

Finally, all these DT changes should belong to the patch in this
series that actually creates the shared .dtsi file.  I see no point
or value in having it performed in a separate patch.

>>>          regulator-always-on;
>>> @@ -170,7 +170,7 @@ vcc_sysin: regulator-vcc-sysin {
>>>          regulator-boot-on;
>>>          regulator-min-microvolt = <5000000>;
>>>          regulator-max-microvolt = <5000000>;
>>> -        vin-supply = <&vcc5v_dcin>;
>>> +        vin-supply = <&dcin>;
>>>      };
>>> 
>>>      vcca: regulator-vcca {