| Message ID | 20241216113052.15696-5-naoki@radxa.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: Refine dts for Radxa ROCK 5C | expand |
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@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 Best regards, Krzysztof
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@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 Thanks for pointing. (Please blame other dts too) Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. > Best regards, > Krzysztof >
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@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@gmail.com/ Is 'regulator-0v0' valid? Is 'regulator-12v0' invalid? How should we handle multiple 1v8/3v3/5v0 regulators? Are examples in fixed-regulator.yaml valid? 'regulator-name' should be '[0-9]v[0-9]' even if there is same name? Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. > Best regards, > Krzysztof >
Hi Krzysztof, Could you please reply to this email? (Not for me, but for everyone) Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. 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@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@gmail.com/ > > Is 'regulator-0v0' valid? > Is 'regulator-12v0' invalid? > > How should we handle multiple 1v8/3v3/5v0 regulators? > > Are examples in fixed-regulator.yaml valid? > 'regulator-name' should be '[0-9]v[0-9]' even if there is same name? > > Best regards, > > -- > FUKAUMI Naoki > Radxa Computer (Shenzhen) Co., Ltd. > >> Best regards, >> Krzysztof >> >
On 20/12/2024 07:51, FUKAUMI Naoki wrote: > Hi Krzysztof, > > Could you please reply to this email? > (Not for me, but for everyone) You have me how much time... 3 days to reply? > > Best regards, > > -- > FUKAUMI Naoki > Radxa Computer (Shenzhen) Co., Ltd. > > 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@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@gmail.com/ >> >> Is 'regulator-0v0' valid? Why would it be valid? Can you have regulator with 0 volts? >> Is 'regulator-12v0' invalid? Read the binding. I gave you very specific link. >> >> 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. Best regards, Krzysztof
On 12/22/24 05:15, Krzysztof Kozlowski wrote: > On 20/12/2024 07:51, FUKAUMI Naoki wrote: >> Hi Krzysztof, >> >> Could you please reply to this email? >> (Not for me, but for everyone) > > You have me how much time... 3 days to reply? sorry. >> Best regards, >> >> -- >> FUKAUMI Naoki >> Radxa Computer (Shenzhen) Co., Ltd. >> >> 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@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@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? >>> 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". >>> 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? Best regards, -- FUKAUMI Naoki Radxa Computer (Shenzhen) Co., Ltd. > Best regards, > Krzysztof >
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: >>> Hi Krzysztof, >>> >>> Could you please reply to this email? >>> (Not for me, but for everyone) >> >> You have me how much time... 3 days to reply? > > sorry. > >>> Best regards, >>> >>> -- >>> FUKAUMI Naoki >>> Radxa Computer (Shenzhen) Co., Ltd. >>> >>> 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@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@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? Best regards, Krzysztof
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 { compatible = "regulator-fixed"; enable-active-high; gpio = <&gpio0 RK_PC5 GPIO_ACTIVE_HIGH>; pinctrl-names = "default"; pinctrl-0 = <&pow_en>; - regulator-name = "pcie2x1l2_3v3"; + regulator-name = "vcc3v3_pcie2x1l2"; regulator-min-microvolt = <3300000>; regulator-max-microvolt = <3300000>; vin-supply = <&vcc_sysin>; @@ -421,7 +421,7 @@ &pcie2x1l2 { pinctrl-names = "default"; pinctrl-0 = <&pcie20x1_2_perstn_m0>; reset-gpios = <&gpio3 RK_PD1 GPIO_ACTIVE_HIGH>; - vpcie3v3-supply = <&pcie2x1l2_3v3>; + vpcie3v3-supply = <&vcc3v3_pcie2x1l2>; status = "okay"; };
Use consistent name with other regulators. No functional change. Fixes: 3ddf5cdb77e6 ("arm64: dts: rockchip: add Radxa ROCK 5C") Signed-off-by: FUKAUMI Naoki <naoki@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(-)