[PATCH v5 04/12] arm64: dts: rockchip: Rename regulator for pcie2x1l2 for Radxa ROCK 5C

[Dragan Simic]

Hello Krzysztof,

On 2024-12-22 07:27, Krzysztof Kozlowski wrote:
> On 22/12/2024 04:18, FUKAUMI Naoki wrote:
>> On 12/22/24 05:15, Krzysztof Kozlowski wrote:
>>> On 20/12/2024 07:51, FUKAUMI Naoki wrote:
>>>> On 12/17/24 10:11, FUKAUMI Naoki wrote:
>>>>> On 12/16/24 22:38, Krzysztof Kozlowski wrote:
>>>>>> On 16/12/2024 12:30, FUKAUMI Naoki wrote:
>>>>>>> Use consistent name with other regulators. No functional change.
>>>>>>> 
>>>>>>> Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C")
>>>>>>> Signed-off-by: FUKAUMI Naoki <naoki at radxa.com>
>>>>>>> ---
>>>>>>> Changes in v5:
>>>>>>> - Reword commit message
>>>>>>> Changes in v4:
>>>>>>> - reword commit message
>>>>>>> Changes in v3:
>>>>>>> - none
>>>>>>> Changes in v2:
>>>>>>> - new
>>>>>>> ---
>>>>>>>    arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts | 6 +++---
>>>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>>> 
>>>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts 
>>>>>>> b/arch/
>>>>>>> arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>>>>> index 85589d1a6d3b..61d75ab503b2 100644
>>>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3588s-rock-5c.dts
>>>>>>> @@ -76,13 +76,13 @@ pwm-fan {
>>>>>>>            pwms = <&pwm3 0 60000 0>;
>>>>>>>        };
>>>>>>> -    pcie2x1l2_3v3: regulator-pcie2x1l2-3v3 {
>>>>>>> +    vcc3v3_pcie2x1l2: regulator-vcc3v3_pcie2x1l2 {
>>>>>> 
>>>>>> No, neither explained, nor correct. See DTS coding style.
>>>>>> 
>>>>>> Please use name for all fixed regulators which matches current 
>>>>>> format
>>>>>> recommendation: 'regulator-[0-9]v[0-9]'
>>>>>> 
>>>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/
>>>>>> tree/Documentation/devicetree/bindings/regulator/fixed-regulator.yaml?
>>>>>> h=v6.11-rc1#n46
>>>>> 
>>>>> 'regulator-[0-9]v[0-9]' is preferred, but 'regulator-[0-9a-z-]+' is 
>>>>> also
>>>>> permitted, right?
>>>>> 
>>>>> i.e. regulator-vcc3v3_pcie2x1l2 should be 
>>>>> regulator-vcc3v3-pcie2x1l2
>>>>> 
>>>>> 
>>>>> Or, should we revert below patch and use 'regulator-[0-9]v[0-9]'?
>>>>> 
>>>>>    https://lore.kernel.org/
>>>>> all/0ae40493-93e9-40cd-9ca9-990ae064f21a at gmail.com/
>>>>> 
>>>>> Is 'regulator-0v0' valid?
>>> 
>>> Why would it be valid? Can you have regulator with 0 volts?
>> 
>> There may be a .dtsi that is shared across multiple .dts, so some
>> regulators may not be able to set a specific voltage in the .dtsi.
>> 
>> How should I describe it?
> 
> How fixed voltage regulator can have non-specific voltage? Sorry, 
> that's
> impossible. Shared DTSI with a regulator means that regulator is 
> shared,
> so it cannot be flexible.
> 
>>>>> Is 'regulator-12v0' invalid?
>>> 
>>> Read the binding. I gave you very specific link.
>> 
>> 46|       - description: Preferred name is 'regulator-[0-9]v[0-9]'
>> 47|         pattern: '^regulator(-[0-9]+v[0-9]+|-[0-9a-z-]+)?$'
>> 
>> The "description" and "pattern" don't match. What you provided is a 
>> link
>> to the "description".
> 
> It's pretty obvious still...
> 
>>>>> How should we handle multiple 1v8/3v3/5v0 regulators?
>>> 
>>> Just add suffix. But usually more than one suffix, 
>>> vcc+3v3+pcie_2x1l2,
>>> means you created a very specific name.
>> 
>> So shouldn't we refer to the schematic?
> 
> So that's the argument you define 10 fixed, uncontrollable regulators 
> in
> DTS? What is the benefit of that exactly? Just unnecessary bloat in 
> DTS,
> in kernel memory and for probe time?

I'm quite surprised to hear you suggesting that some of the hardware
components should be omitted from a board dts(i) file.  Regardless of
some of the hardware components being fixed or adjustable, they should
all be described, for the sake of transcribing the board schematic into
the board dts(i) files accurately and completely.  Furthermore, some
fixed components may actually be needed in the board dts(i) files; for
example, a fixed regulator may be turned on at boot time, or turned on
and off at runtime.

Technically, we could omit the fixed regulators from the board dts(i)
files, _only_ if they aren't powered on and off at boot time or at
runtime, of course, but many more things could technically be omitted
as well from the dts(i) files if we'd follow that rule, which would
result in a huge per-board inconsistency, leading to a mess.