| Message ID | 987e68d1c5e9a0cc56d730aec87246aa5ab8ea14.1725535770.git.dsimic@manjaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: dts: rockchip: Add a few aliases to the PineTab2 dtsi | expand |
On 05/09/2024 13:32, Dragan Simic wrote: > Sprinkle a few commonly used aliases onto the PineTab2 dtsi file, to improve > its readability a bit, to make it easier to refer to the actual nodes later, > if needed, and to add a bit more detail to some of the labels. > > No functional changes are introduced, which was validated by decompiling and > comparing all affected board dtb files before and after these changes. When > compared with the decompiled original dtb files, some of the phandles in the > updated dtb files have different values, and the updated dtb files contain > some additional phandles and additional symbols that come from the introduced > aliases, but they still effectively remain the same as the originals. > > Suggested-by: Diederik de Haas <didi.debian@cknow.org> > Signed-off-by: Dragan Simic <dsimic@manjaro.org> > --- Unused aliases do not improve readability, so for me this change is making code worse without valid reason. Best regards, Krzysztof
Hello Krzysztof, On 2024-09-05 13:39, Krzysztof Kozlowski wrote: > On 05/09/2024 13:32, Dragan Simic wrote: >> Sprinkle a few commonly used aliases onto the PineTab2 dtsi file, to >> improve >> its readability a bit, to make it easier to refer to the actual nodes >> later, >> if needed, and to add a bit more detail to some of the labels. >> >> No functional changes are introduced, which was validated by >> decompiling and >> comparing all affected board dtb files before and after these changes. >> When >> compared with the decompiled original dtb files, some of the phandles >> in the >> updated dtb files have different values, and the updated dtb files >> contain >> some additional phandles and additional symbols that come from the >> introduced >> aliases, but they still effectively remain the same as the originals. >> >> Suggested-by: Diederik de Haas <didi.debian@cknow.org> >> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >> --- > > Unused aliases do not improve readability, so for me this change is > making code worse without valid reason. Then why do we already have, for example, unused "rk817_charger: charger" aliases in quite a few board dts(i) files? If those are actually seen as redundant, we should remove all of them.
On 05/09/2024 13:43, Dragan Simic wrote: > Hello Krzysztof, > > On 2024-09-05 13:39, Krzysztof Kozlowski wrote: >> On 05/09/2024 13:32, Dragan Simic wrote: >>> Sprinkle a few commonly used aliases onto the PineTab2 dtsi file, to >>> improve >>> its readability a bit, to make it easier to refer to the actual nodes >>> later, >>> if needed, and to add a bit more detail to some of the labels. >>> >>> No functional changes are introduced, which was validated by >>> decompiling and >>> comparing all affected board dtb files before and after these changes. >>> When >>> compared with the decompiled original dtb files, some of the phandles >>> in the >>> updated dtb files have different values, and the updated dtb files >>> contain >>> some additional phandles and additional symbols that come from the >>> introduced >>> aliases, but they still effectively remain the same as the originals. >>> >>> Suggested-by: Diederik de Haas <didi.debian@cknow.org> >>> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >>> --- >> >> Unused aliases do not improve readability, so for me this change is >> making code worse without valid reason. > > Then why do we already have, for example, unused "rk817_charger: > charger" > aliases in quite a few board dts(i) files? If those are actually seen Ask contributors... > as > redundant, we should remove all of them. They are already there, so dropping them is close to unnecessary churn. And quite a lot of work to investigate the reason behind EACH label. Best regards, Krzysztof
On 2024-09-05 13:48, Krzysztof Kozlowski wrote: > On 05/09/2024 13:43, Dragan Simic wrote: >> Hello Krzysztof, >> >> On 2024-09-05 13:39, Krzysztof Kozlowski wrote: >>> On 05/09/2024 13:32, Dragan Simic wrote: >>>> Sprinkle a few commonly used aliases onto the PineTab2 dtsi file, to >>>> improve >>>> its readability a bit, to make it easier to refer to the actual >>>> nodes >>>> later, >>>> if needed, and to add a bit more detail to some of the labels. >>>> >>>> No functional changes are introduced, which was validated by >>>> decompiling and >>>> comparing all affected board dtb files before and after these >>>> changes. >>>> When >>>> compared with the decompiled original dtb files, some of the >>>> phandles >>>> in the >>>> updated dtb files have different values, and the updated dtb files >>>> contain >>>> some additional phandles and additional symbols that come from the >>>> introduced >>>> aliases, but they still effectively remain the same as the >>>> originals. >>>> >>>> Suggested-by: Diederik de Haas <didi.debian@cknow.org> >>>> Signed-off-by: Dragan Simic <dsimic@manjaro.org> >>>> --- >>> >>> Unused aliases do not improve readability, so for me this change is >>> making code worse without valid reason. >> >> Then why do we already have, for example, unused "rk817_charger: >> charger" >> aliases in quite a few board dts(i) files? If those are actually seen > > Ask contributors... That would be a lot of work for next to nothing. Actually, I believe those aliases were simply copied and pasted, at least when it comes to the "rk817_*" aliases that are unused without doubt. >> as >> redundant, we should remove all of them. > > They are already there, so dropping them is close to unnecessary churn. > And quite a lot of work to investigate the reason behind EACH label. Hmm, it's true that a lot of work would be needed. On the other hand, how about adding these aliases to the PineTab2 dtsi for the sake of consistency with the other similar board dts(i) files? For example, grepping for "rk817_charger", which is used as an alias in all other dts(i) files for boards that use RK817, currently omits the PineTab2 dtsi. I see that as a downside that this patch attempts to improve.
On Thu Sep 5, 2024 at 1:39 PM CEST, Krzysztof Kozlowski wrote: > On 05/09/2024 13:32, Dragan Simic wrote: > > Sprinkle a few commonly used aliases onto the PineTab2 dtsi file, to improve > > its readability a bit, to make it easier to refer to the actual nodes later, > > if needed, and to add a bit more detail to some of the labels. > > > > Suggested-by: Diederik de Haas <didi.debian@cknow.org> > > Signed-off-by: Dragan Simic <dsimic@manjaro.org> > > Unused aliases do not improve readability, so for me this change is > making code worse without valid reason. This came forth by a question from me to Dragan about a patch for another board which doesn't have a charger defined at all (yet). I actually have that patch (but not the HW) for a while (~1.5 year) now and I had used `rk817_charger: charger` for that, probably because I saw that being used everywhere else. Then I compared it to the PineTab2 and noticed it had only `charger`, so I asked "What should I use? With or without the alias?" In this case the inconsistency is causing confusion (with me). So: What should be used for that other/new board(s)? Cheers, Diederik
Am Donnerstag, 5. September 2024, 14:21:12 CEST schrieb Diederik de Haas: > On Thu Sep 5, 2024 at 1:39 PM CEST, Krzysztof Kozlowski wrote: > > On 05/09/2024 13:32, Dragan Simic wrote: > > > Sprinkle a few commonly used aliases onto the PineTab2 dtsi file, to improve > > > its readability a bit, to make it easier to refer to the actual nodes later, > > > if needed, and to add a bit more detail to some of the labels. > > > > > > Suggested-by: Diederik de Haas <didi.debian@cknow.org> > > > Signed-off-by: Dragan Simic <dsimic@manjaro.org> > > > > Unused aliases do not improve readability, so for me this change is > > making code worse without valid reason. > > This came forth by a question from me to Dragan about a patch for > another board which doesn't have a charger defined at all (yet). > I actually have that patch (but not the HW) for a while (~1.5 year) > now and I had used `rk817_charger: charger` for that, probably because > I saw that being used everywhere else. > > Then I compared it to the PineTab2 and noticed it had only `charger`, so > I asked "What should I use? With or without the alias?" > In this case the inconsistency is causing confusion (with me). > > So: What should be used for that other/new board(s)? As Krzysztof said, having a phandle that is never going to be used is somewhat pointless. Having a phandle defined for a node does not hurt anything, so having some in a board dts is not catastrophically bad, but there is no reason to add or remove unused ones for no reason - especially as it affects git blame . So in short, if you see an unused phandle in a dts _patch_, just point it out in review. Heiko
diff --git a/arch/arm64/boot/dts/rockchip/rk3566-pinetab2.dtsi b/arch/arm64/boot/dts/rockchip/rk3566-pinetab2.dtsi index db40281eafbe..04d98715ae6e 100644 --- a/arch/arm64/boot/dts/rockchip/rk3566-pinetab2.dtsi +++ b/arch/arm64/boot/dts/rockchip/rk3566-pinetab2.dtsi @@ -41,7 +41,7 @@ button-vol-down { }; }; - backlight: backlight { + backlight: backlight-dsi { compatible = "pwm-backlight"; pwms = <&pwm4 0 25000 0>; brightness-levels = <20 220>; @@ -551,20 +551,20 @@ regulator-state-mem { }; }; - charger { + rk817_charger: charger { monitored-battery = <&battery>; rockchip,resistor-sense-micro-ohms = <10000>; rockchip,sleep-enter-current-microamp = <300000>; rockchip,sleep-filter-current-microamp = <100000>; }; }; }; &i2c1 { clock-frequency = <400000>; status = "okay"; - touchscreen@5d { + touchscreen: touchscreen@5d { compatible = "goodix,gt911"; reg = <0x5d>; interrupt-parent = <&gpio0>; @@ -583,13 +583,13 @@ &i2c2 { pinctrl-0 = <&i2c2m1_xfer>; status = "okay"; - vcm@c { + vcm: vcm@c { compatible = "dongwoon,dw9714"; reg = <0x0c>; vcc-supply = <&vcc1v8_dvp>; }; - camera@36 { + camerab: camera@36 { compatible = "ovti,ov5648"; reg = <0x36>; pinctrl-names = "default"; @@ -619,7 +619,7 @@ &i2c5 { clock-frequency = <400000>; status = "okay"; - accelerometer@18 { + accelerometer: accelerometer@18 { compatible = "silan,sc7a20"; reg = <0x18>; interrupt-parent = <&gpio3>;
Sprinkle a few commonly used aliases onto the PineTab2 dtsi file, to improve its readability a bit, to make it easier to refer to the actual nodes later, if needed, and to add a bit more detail to some of the labels. No functional changes are introduced, which was validated by decompiling and comparing all affected board dtb files before and after these changes. When compared with the decompiled original dtb files, some of the phandles in the updated dtb files have different values, and the updated dtb files contain some additional phandles and additional symbols that come from the introduced aliases, but they still effectively remain the same as the originals. Suggested-by: Diederik de Haas <didi.debian@cknow.org> Signed-off-by: Dragan Simic <dsimic@manjaro.org> --- Notes: This patch wasn't tested on a PineTab2 before it was submitted to the mailing list, because I don't have that device, but Diederik has already agreed to test this patch on his PineTab2 and provide feedback. arch/arm64/boot/dts/rockchip/rk3566-pinetab2.dtsi | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)