| Message ID | 20220624170740.66271-1-krzysztof.kozlowski@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] MIPS: dts: correct gpio-keys names and properties | expand |
Hi Krzysztof, Le ven., juin 24 2022 at 19:07:39 +0200, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> a écrit : > gpio-keys children do not use unit addresses. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > See: > https://lore.kernel.org/all/20220616005224.18391-1-krzysztof.kozlowski@linaro.org/ > --- > arch/mips/boot/dts/img/pistachio_marduk.dts | 4 +-- > arch/mips/boot/dts/ingenic/gcw0.dts | 31 > +++++++++---------- > arch/mips/boot/dts/ingenic/rs90.dts | 18 +++++------ > arch/mips/boot/dts/pic32/pic32mzda_sk.dts | 9 ++---- > .../boot/dts/qca/ar9132_tl_wr1043nd_v1.dts | 6 ++-- > arch/mips/boot/dts/qca/ar9331_dpt_module.dts | 4 +-- > .../mips/boot/dts/qca/ar9331_dragino_ms14.dts | 6 ++-- > arch/mips/boot/dts/qca/ar9331_omega.dts | 4 +-- > .../qca/ar9331_openembed_som9331_board.dts | 4 +-- > arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts | 8 ++--- > 10 files changed, 37 insertions(+), 57 deletions(-) > > diff --git a/arch/mips/boot/dts/img/pistachio_marduk.dts > b/arch/mips/boot/dts/img/pistachio_marduk.dts > index a8708783f04b..a8da2f992b1a 100644 > --- a/arch/mips/boot/dts/img/pistachio_marduk.dts > +++ b/arch/mips/boot/dts/img/pistachio_marduk.dts > @@ -59,12 +59,12 @@ led-1 { > > keys { > compatible = "gpio-keys"; > - button@1 { > + button-1 { > label = "Button 1"; > linux,code = <0x101>; /* BTN_1 */ > gpios = <&gpio3 6 GPIO_ACTIVE_LOW>; > }; > - button@2 { > + button-2 { > label = "Button 2"; > linux,code = <0x102>; /* BTN_2 */ > gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; > diff --git a/arch/mips/boot/dts/ingenic/gcw0.dts > b/arch/mips/boot/dts/ingenic/gcw0.dts > index 4abb0318416c..5d33f26fd28c 100644 > --- a/arch/mips/boot/dts/ingenic/gcw0.dts > +++ b/arch/mips/boot/dts/ingenic/gcw0.dts > @@ -130,89 +130,86 @@ backlight: backlight { > > gpio-keys { > compatible = "gpio-keys"; > - #address-cells = <1>; > - #size-cells = <0>; Are you sure you can remove these? Looking at paragraph 2.3.5 of the DT spec, I would think they have to stay (although with #address-cells = <0>). Cheers, -Paul > - > autorepeat; > > - button@0 { > + button-0 { > label = "D-pad up"; > linux,code = <KEY_UP>; > linux,can-disable; > gpios = <&gpe 21 GPIO_ACTIVE_LOW>; > }; > > - button@1 { > + button-1 { > label = "D-pad down"; > linux,code = <KEY_DOWN>; > linux,can-disable; > gpios = <&gpe 25 GPIO_ACTIVE_LOW>; > }; > > - button@2 { > + button-2 { > label = "D-pad left"; > linux,code = <KEY_LEFT>; > linux,can-disable; > gpios = <&gpe 23 GPIO_ACTIVE_LOW>; > }; > > - button@3 { > + button-3 { > label = "D-pad right"; > linux,code = <KEY_RIGHT>; > linux,can-disable; > gpios = <&gpe 24 GPIO_ACTIVE_LOW>; > }; > > - button@4 { > + button-4 { > label = "Button A"; > linux,code = <KEY_LEFTCTRL>; > linux,can-disable; > gpios = <&gpe 29 GPIO_ACTIVE_LOW>; > }; > > - button@5 { > + button-5 { > label = "Button B"; > linux,code = <KEY_LEFTALT>; > linux,can-disable; > gpios = <&gpe 20 GPIO_ACTIVE_LOW>; > }; > > - button@6 { > + button-6 { > label = "Button Y"; > linux,code = <KEY_SPACE>; > linux,can-disable; > gpios = <&gpe 27 GPIO_ACTIVE_LOW>; > }; > > - button@7 { > + button-7 { > label = "Button X"; > linux,code = <KEY_LEFTSHIFT>; > linux,can-disable; > gpios = <&gpe 28 GPIO_ACTIVE_LOW>; > }; > > - button@8 { > + button-8 { > label = "Left shoulder button"; > linux,code = <KEY_TAB>; > linux,can-disable; > gpios = <&gpb 20 GPIO_ACTIVE_LOW>; > }; > > - button@9 { > + button-9 { > label = "Right shoulder button"; > linux,code = <KEY_BACKSPACE>; > linux,can-disable; > gpios = <&gpe 26 GPIO_ACTIVE_LOW>; > }; > > - button@10 { > + button-10 { > label = "Start button"; > linux,code = <KEY_ENTER>; > linux,can-disable; > gpios = <&gpb 21 GPIO_ACTIVE_LOW>; > }; > > - button@11 { > + button-11 { > label = "Select button"; > linux,code = <KEY_ESC>; > linux,can-disable; > @@ -223,7 +220,7 @@ button@11 { > gpios = <&gpd 18 GPIO_ACTIVE_HIGH>; > }; > > - button@12 { > + button-12 { > label = "Power slider"; > linux,code = <KEY_POWER>; > linux,can-disable; > @@ -231,7 +228,7 @@ button@12 { > wakeup-source; > }; > > - button@13 { > + button-13 { > label = "Power hold"; > linux,code = <KEY_PAUSE>; > linux,can-disable; > diff --git a/arch/mips/boot/dts/ingenic/rs90.dts > b/arch/mips/boot/dts/ingenic/rs90.dts > index 74fee7f01352..e8df70dd42bf 100644 > --- a/arch/mips/boot/dts/ingenic/rs90.dts > +++ b/arch/mips/boot/dts/ingenic/rs90.dts > @@ -52,53 +52,51 @@ backlight: backlight { > > keys@0 { > compatible = "gpio-keys"; > - #address-cells = <1>; > - #size-cells = <0>; > > - key@0 { > + key-0 { > label = "D-pad up"; > linux,code = <KEY_UP>; > gpios = <&gpc 10 GPIO_ACTIVE_LOW>; > }; > > - key@1 { > + key-1 { > label = "D-pad down"; > linux,code = <KEY_DOWN>; > gpios = <&gpc 11 GPIO_ACTIVE_LOW>; > }; > > - key@2 { > + key-2 { > label = "D-pad left"; > linux,code = <KEY_LEFT>; > gpios = <&gpb 31 GPIO_ACTIVE_LOW>; > }; > > - key@3 { > + key-3 { > label = "D-pad right"; > linux,code = <KEY_RIGHT>; > gpios = <&gpd 21 GPIO_ACTIVE_LOW>; > }; > > - key@4 { > + key-4 { > label = "Button A"; > linux,code = <KEY_LEFTCTRL>; > gpios = <&gpc 31 GPIO_ACTIVE_LOW>; > }; > > - key@5 { > + key-5 { > label = "Button B"; > linux,code = <KEY_LEFTALT>; > gpios = <&gpc 30 GPIO_ACTIVE_LOW>; > }; > > - key@6 { > + key-6 { > label = "Right shoulder button"; > linux,code = <KEY_BACKSPACE>; > gpios = <&gpc 12 GPIO_ACTIVE_LOW>; > debounce-interval = <10>; > }; > > - key@7 { > + key-7 { > label = "Start button"; > linux,code = <KEY_ENTER>; > gpios = <&gpd 17 GPIO_ACTIVE_LOW>; > diff --git a/arch/mips/boot/dts/pic32/pic32mzda_sk.dts > b/arch/mips/boot/dts/pic32/pic32mzda_sk.dts > index d7fa5d55dbf3..ab70637bbec5 100644 > --- a/arch/mips/boot/dts/pic32/pic32mzda_sk.dts > +++ b/arch/mips/boot/dts/pic32/pic32mzda_sk.dts > @@ -52,22 +52,19 @@ keys0 { > pinctrl-0 = <&user_buttons_s0>; > pinctrl-names = "default"; > > - #address-cells = <1>; > - #size-cells = <0>; > - > - button@sw1 { > + button-1 { > label = "ESC"; > linux,code = <1>; > gpios = <&gpio1 12 0>; > }; > > - button@sw2 { > + button-2 { > label = "Home"; > linux,code = <102>; > gpios = <&gpio1 13 0>; > }; > > - button@sw3 { > + button-3 { > label = "Menu"; > linux,code = <139>; > gpios = <&gpio1 14 0>; > diff --git a/arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts > b/arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts > index 7fccf6357225..f3dff4009ab5 100644 > --- a/arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts > +++ b/arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts > @@ -23,17 +23,15 @@ extosc: ref { > > gpio-keys { > compatible = "gpio-keys"; > - #address-cells = <1>; > - #size-cells = <0>; > > - button@0 { > + button-0 { > label = "reset"; > linux,code = <KEY_RESTART>; > gpios = <&gpio 3 GPIO_ACTIVE_LOW>; > debounce-interval = <60>; > }; > > - button@1 { > + button-1 { > label = "qss"; > linux,code = <KEY_WPS_BUTTON>; > gpios = <&gpio 7 GPIO_ACTIVE_LOW>; > diff --git a/arch/mips/boot/dts/qca/ar9331_dpt_module.dts > b/arch/mips/boot/dts/qca/ar9331_dpt_module.dts > index 7695d326df11..c857cd22f7db 100644 > --- a/arch/mips/boot/dts/qca/ar9331_dpt_module.dts > +++ b/arch/mips/boot/dts/qca/ar9331_dpt_module.dts > @@ -33,10 +33,8 @@ led-0 { > > gpio-keys { > compatible = "gpio-keys"; > - #address-cells = <1>; > - #size-cells = <0>; > > - button@0 { > + button-0 { > label = "reset"; > linux,code = <KEY_RESTART>; > gpios = <&gpio 11 GPIO_ACTIVE_LOW>; > diff --git a/arch/mips/boot/dts/qca/ar9331_dragino_ms14.dts > b/arch/mips/boot/dts/qca/ar9331_dragino_ms14.dts > index d38aa73f1a2e..40e4c5da0e65 100644 > --- a/arch/mips/boot/dts/qca/ar9331_dragino_ms14.dts > +++ b/arch/mips/boot/dts/qca/ar9331_dragino_ms14.dts > @@ -49,16 +49,14 @@ system { > > gpio-keys { > compatible = "gpio-keys"; > - #address-cells = <1>; > - #size-cells = <0>; > > - button@0 { > + button-0 { > label = "jumpstart"; > linux,code = <KEY_WPS_BUTTON>; > gpios = <&gpio 11 GPIO_ACTIVE_LOW>; > }; > > - button@1 { > + button-1 { > label = "reset"; > linux,code = <KEY_RESTART>; > gpios = <&gpio 12 GPIO_ACTIVE_LOW>; > diff --git a/arch/mips/boot/dts/qca/ar9331_omega.dts > b/arch/mips/boot/dts/qca/ar9331_omega.dts > index 11778abacf66..ed184d861d5f 100644 > --- a/arch/mips/boot/dts/qca/ar9331_omega.dts > +++ b/arch/mips/boot/dts/qca/ar9331_omega.dts > @@ -31,10 +31,8 @@ system { > > gpio-keys { > compatible = "gpio-keys"; > - #address-cells = <1>; > - #size-cells = <0>; > > - button@0 { > + button-0 { > label = "reset"; > linux,code = <KEY_RESTART>; > gpios = <&gpio 11 GPIO_ACTIVE_HIGH>; > diff --git > a/arch/mips/boot/dts/qca/ar9331_openembed_som9331_board.dts > b/arch/mips/boot/dts/qca/ar9331_openembed_som9331_board.dts > index e6622f8e8c2b..dc65ebd60bbc 100644 > --- a/arch/mips/boot/dts/qca/ar9331_openembed_som9331_board.dts > +++ b/arch/mips/boot/dts/qca/ar9331_openembed_som9331_board.dts > @@ -33,10 +33,8 @@ led-0 { > > gpio-keys { > compatible = "gpio-keys"; > - #address-cells = <1>; > - #size-cells = <0>; > > - button@0 { > + button-0 { > label = "reset"; > linux,code = <KEY_RESTART>; > gpios = <&gpio 11 GPIO_ACTIVE_HIGH>; > diff --git a/arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts > b/arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts > index c8290d36cfbe..5f424c2cd781 100644 > --- a/arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts > +++ b/arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts > @@ -49,22 +49,20 @@ led3g { > > gpio-keys { > compatible = "gpio-keys"; > - #address-cells = <1>; > - #size-cells = <0>; > > - button@0 { > + button-0 { > label = "wps"; > linux,code = <KEY_WPS_BUTTON>; > gpios = <&gpio 11 GPIO_ACTIVE_HIGH>; > }; > > - button@1 { > + button-1 { > label = "sw1"; > linux,code = <BTN_0>; > gpios = <&gpio 18 GPIO_ACTIVE_HIGH>; > }; > > - button@2 { > + button-2 { > label = "sw2"; > linux,code = <BTN_1>; > gpios = <&gpio 20 GPIO_ACTIVE_HIGH>; > -- > 2.34.1 >
On 24/06/2022 20:40, Paul Cercueil wrote: > Hi Krzysztof, > > Le ven., juin 24 2022 at 19:07:39 +0200, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> a écrit : >> gpio-keys children do not use unit addresses. >> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >> >> --- >> >> See: >> https://lore.kernel.org/all/20220616005224.18391-1-krzysztof.kozlowski@linaro.org/ >> --- >> arch/mips/boot/dts/img/pistachio_marduk.dts | 4 +-- >> arch/mips/boot/dts/ingenic/gcw0.dts | 31 >> +++++++++---------- >> arch/mips/boot/dts/ingenic/rs90.dts | 18 +++++------ >> arch/mips/boot/dts/pic32/pic32mzda_sk.dts | 9 ++---- >> .../boot/dts/qca/ar9132_tl_wr1043nd_v1.dts | 6 ++-- >> arch/mips/boot/dts/qca/ar9331_dpt_module.dts | 4 +-- >> .../mips/boot/dts/qca/ar9331_dragino_ms14.dts | 6 ++-- >> arch/mips/boot/dts/qca/ar9331_omega.dts | 4 +-- >> .../qca/ar9331_openembed_som9331_board.dts | 4 +-- >> arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts | 8 ++--- >> 10 files changed, 37 insertions(+), 57 deletions(-) >> >> diff --git a/arch/mips/boot/dts/img/pistachio_marduk.dts >> b/arch/mips/boot/dts/img/pistachio_marduk.dts >> index a8708783f04b..a8da2f992b1a 100644 >> --- a/arch/mips/boot/dts/img/pistachio_marduk.dts >> +++ b/arch/mips/boot/dts/img/pistachio_marduk.dts >> @@ -59,12 +59,12 @@ led-1 { >> >> keys { >> compatible = "gpio-keys"; >> - button@1 { >> + button-1 { >> label = "Button 1"; >> linux,code = <0x101>; /* BTN_1 */ >> gpios = <&gpio3 6 GPIO_ACTIVE_LOW>; >> }; >> - button@2 { >> + button-2 { >> label = "Button 2"; >> linux,code = <0x102>; /* BTN_2 */ >> gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; >> diff --git a/arch/mips/boot/dts/ingenic/gcw0.dts >> b/arch/mips/boot/dts/ingenic/gcw0.dts >> index 4abb0318416c..5d33f26fd28c 100644 >> --- a/arch/mips/boot/dts/ingenic/gcw0.dts >> +++ b/arch/mips/boot/dts/ingenic/gcw0.dts >> @@ -130,89 +130,86 @@ backlight: backlight { >> >> gpio-keys { >> compatible = "gpio-keys"; >> - #address-cells = <1>; >> - #size-cells = <0>; > > Are you sure you can remove these? Yes, from DT spec point of view, DT bindings and Linux implementation. However this particular change was not tested, except building. > > Looking at paragraph 2.3.5 of the DT spec, I would think they have to > stay (although with #address-cells = <0>). The paragraph 2.3.5 says nothing about regular properties (which can be also child nodes). It says about children of a bus, right? It's not related here, it's not a bus. Second, why exactly this one gpio-keys node is different than all other gpio-keys everywhere and than bindings? Why this one has to be incompatible/wrong according to bindings (which do not allow address-cells and nodes with unit addresses)? Best regards, Krzysztof
Hi Krzysztof, Le sam., juin 25 2022 at 21:58:08 +0200, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> a écrit : > On 24/06/2022 20:40, Paul Cercueil wrote: >> Hi Krzysztof, >> >> Le ven., juin 24 2022 at 19:07:39 +0200, Krzysztof Kozlowski >> <krzysztof.kozlowski@linaro.org> a écrit : >>> gpio-keys children do not use unit addresses. >>> >>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>> >>> --- >>> >>> See: >>> >>> https://lore.kernel.org/all/20220616005224.18391-1-krzysztof.kozlowski@linaro.org/ >>> --- >>> arch/mips/boot/dts/img/pistachio_marduk.dts | 4 +-- >>> arch/mips/boot/dts/ingenic/gcw0.dts | 31 >>> +++++++++---------- >>> arch/mips/boot/dts/ingenic/rs90.dts | 18 +++++------ >>> arch/mips/boot/dts/pic32/pic32mzda_sk.dts | 9 ++---- >>> .../boot/dts/qca/ar9132_tl_wr1043nd_v1.dts | 6 ++-- >>> arch/mips/boot/dts/qca/ar9331_dpt_module.dts | 4 +-- >>> .../mips/boot/dts/qca/ar9331_dragino_ms14.dts | 6 ++-- >>> arch/mips/boot/dts/qca/ar9331_omega.dts | 4 +-- >>> .../qca/ar9331_openembed_som9331_board.dts | 4 +-- >>> arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts | 8 ++--- >>> 10 files changed, 37 insertions(+), 57 deletions(-) >>> >>> diff --git a/arch/mips/boot/dts/img/pistachio_marduk.dts >>> b/arch/mips/boot/dts/img/pistachio_marduk.dts >>> index a8708783f04b..a8da2f992b1a 100644 >>> --- a/arch/mips/boot/dts/img/pistachio_marduk.dts >>> +++ b/arch/mips/boot/dts/img/pistachio_marduk.dts >>> @@ -59,12 +59,12 @@ led-1 { >>> >>> keys { >>> compatible = "gpio-keys"; >>> - button@1 { >>> + button-1 { >>> label = "Button 1"; >>> linux,code = <0x101>; /* BTN_1 */ >>> gpios = <&gpio3 6 GPIO_ACTIVE_LOW>; >>> }; >>> - button@2 { >>> + button-2 { >>> label = "Button 2"; >>> linux,code = <0x102>; /* BTN_2 */ >>> gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; >>> diff --git a/arch/mips/boot/dts/ingenic/gcw0.dts >>> b/arch/mips/boot/dts/ingenic/gcw0.dts >>> index 4abb0318416c..5d33f26fd28c 100644 >>> --- a/arch/mips/boot/dts/ingenic/gcw0.dts >>> +++ b/arch/mips/boot/dts/ingenic/gcw0.dts >>> @@ -130,89 +130,86 @@ backlight: backlight { >>> >>> gpio-keys { >>> compatible = "gpio-keys"; >>> - #address-cells = <1>; >>> - #size-cells = <0>; >> >> Are you sure you can remove these? > > Yes, from DT spec point of view, DT bindings and Linux implementation. > However this particular change was not tested, except building. > >> >> Looking at paragraph 2.3.5 of the DT spec, I would think they have >> to >> stay (although with #address-cells = <0>). > > The paragraph 2.3.5 says nothing about regular properties (which can > be > also child nodes). It says about children of a bus, right? It's not > related here, it's not a bus. I quote: "A DTSpec-compliant boot program shall supply #address-cells and #size-cells on all nodes that have children." The gpio-keys node has children nodes, therefore it should have #address-cells and #size-cells, there's no room for interpretation here. > Second, why exactly this one gpio-keys node is different than all > other > gpio-keys everywhere and than bindings? Why this one has to be > incompatible/wrong according to bindings (which do not allow > address-cells and nodes with unit addresses)? Nothing is different. I'm just stating that your proposed fix is invalid if we want to enforce compliance with the DT spec. Cheers, -Paul >
On 25/06/2022 22:15, Paul Cercueil wrote: > Hi Krzysztof, > > Le sam., juin 25 2022 at 21:58:08 +0200, Krzysztof Kozlowski > <krzysztof.kozlowski@linaro.org> a écrit : >> On 24/06/2022 20:40, Paul Cercueil wrote: >>> Hi Krzysztof, >>> >>> Le ven., juin 24 2022 at 19:07:39 +0200, Krzysztof Kozlowski >>> <krzysztof.kozlowski@linaro.org> a écrit : >>>> gpio-keys children do not use unit addresses. >>>> >>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>> >>>> --- >>>> >>>> See: >>>> >>>> https://lore.kernel.org/all/20220616005224.18391-1-krzysztof.kozlowski@linaro.org/ >>>> --- >>>> arch/mips/boot/dts/img/pistachio_marduk.dts | 4 +-- >>>> arch/mips/boot/dts/ingenic/gcw0.dts | 31 >>>> +++++++++---------- >>>> arch/mips/boot/dts/ingenic/rs90.dts | 18 +++++------ >>>> arch/mips/boot/dts/pic32/pic32mzda_sk.dts | 9 ++---- >>>> .../boot/dts/qca/ar9132_tl_wr1043nd_v1.dts | 6 ++-- >>>> arch/mips/boot/dts/qca/ar9331_dpt_module.dts | 4 +-- >>>> .../mips/boot/dts/qca/ar9331_dragino_ms14.dts | 6 ++-- >>>> arch/mips/boot/dts/qca/ar9331_omega.dts | 4 +-- >>>> .../qca/ar9331_openembed_som9331_board.dts | 4 +-- >>>> arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts | 8 ++--- >>>> 10 files changed, 37 insertions(+), 57 deletions(-) >>>> >>>> diff --git a/arch/mips/boot/dts/img/pistachio_marduk.dts >>>> b/arch/mips/boot/dts/img/pistachio_marduk.dts >>>> index a8708783f04b..a8da2f992b1a 100644 >>>> --- a/arch/mips/boot/dts/img/pistachio_marduk.dts >>>> +++ b/arch/mips/boot/dts/img/pistachio_marduk.dts >>>> @@ -59,12 +59,12 @@ led-1 { >>>> >>>> keys { >>>> compatible = "gpio-keys"; >>>> - button@1 { >>>> + button-1 { >>>> label = "Button 1"; >>>> linux,code = <0x101>; /* BTN_1 */ >>>> gpios = <&gpio3 6 GPIO_ACTIVE_LOW>; >>>> }; >>>> - button@2 { >>>> + button-2 { >>>> label = "Button 2"; >>>> linux,code = <0x102>; /* BTN_2 */ >>>> gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; >>>> diff --git a/arch/mips/boot/dts/ingenic/gcw0.dts >>>> b/arch/mips/boot/dts/ingenic/gcw0.dts >>>> index 4abb0318416c..5d33f26fd28c 100644 >>>> --- a/arch/mips/boot/dts/ingenic/gcw0.dts >>>> +++ b/arch/mips/boot/dts/ingenic/gcw0.dts >>>> @@ -130,89 +130,86 @@ backlight: backlight { >>>> >>>> gpio-keys { >>>> compatible = "gpio-keys"; >>>> - #address-cells = <1>; >>>> - #size-cells = <0>; >>> >>> Are you sure you can remove these? >> >> Yes, from DT spec point of view, DT bindings and Linux implementation. >> However this particular change was not tested, except building. >> >>> >>> Looking at paragraph 2.3.5 of the DT spec, I would think they have >>> to >>> stay (although with #address-cells = <0>). >> >> The paragraph 2.3.5 says nothing about regular properties (which can >> be >> also child nodes). It says about children of a bus, right? It's not >> related here, it's not a bus. > > I quote: > "A DTSpec-compliant boot program shall supply #address-cells and > #size-cells on all nodes that have children." And paragraph 2.2.3 says: "A unit address may be omitted if the full path to the node is unambiguous." You have address/size cells for nodes with children having unit addresses. If they don't unit addresses, you don't add address/size cells (with some exceptions). The paragraph 2.3.5 mentions "child device nodes" and these properties are not devices, although I agree that DT spec here is actually confusing. > > The gpio-keys node has children nodes, therefore it should have > #address-cells and #size-cells, there's no room for interpretation here. > >> Second, why exactly this one gpio-keys node is different than all >> other >> gpio-keys everywhere and than bindings? Why this one has to be >> incompatible/wrong according to bindings (which do not allow >> address-cells and nodes with unit addresses)? > > Nothing is different. I'm just stating that your proposed fix is > invalid if we want to enforce compliance with the DT spec. In such case, we rather enforce the compliance with the bindings. Best regards, Krzysztof
On 25.06.2022 23:25, Krzysztof Kozlowski wrote: > On 25/06/2022 22:15, Paul Cercueil wrote: >> Hi Krzysztof, >> >> Le sam., juin 25 2022 at 21:58:08 +0200, Krzysztof Kozlowski >> <krzysztof.kozlowski@linaro.org> a écrit : >>> On 24/06/2022 20:40, Paul Cercueil wrote: >>>> Hi Krzysztof, >>>> >>>> Le ven., juin 24 2022 at 19:07:39 +0200, Krzysztof Kozlowski >>>> <krzysztof.kozlowski@linaro.org> a écrit : >>>>> gpio-keys children do not use unit addresses. >>>>> >>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> >>>>> >>>>> --- >>>>> >>>>> See: >>>>> >>>>> https://lore.kernel.org/all/20220616005224.18391-1-krzysztof.kozlowski@linaro.org/ >>>>> --- >>>>> arch/mips/boot/dts/img/pistachio_marduk.dts | 4 +-- >>>>> arch/mips/boot/dts/ingenic/gcw0.dts | 31 >>>>> +++++++++---------- >>>>> arch/mips/boot/dts/ingenic/rs90.dts | 18 +++++------ >>>>> arch/mips/boot/dts/pic32/pic32mzda_sk.dts | 9 ++---- >>>>> .../boot/dts/qca/ar9132_tl_wr1043nd_v1.dts | 6 ++-- >>>>> arch/mips/boot/dts/qca/ar9331_dpt_module.dts | 4 +-- >>>>> .../mips/boot/dts/qca/ar9331_dragino_ms14.dts | 6 ++-- >>>>> arch/mips/boot/dts/qca/ar9331_omega.dts | 4 +-- >>>>> .../qca/ar9331_openembed_som9331_board.dts | 4 +-- >>>>> arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts | 8 ++--- >>>>> 10 files changed, 37 insertions(+), 57 deletions(-) >>>>> >>>>> diff --git a/arch/mips/boot/dts/img/pistachio_marduk.dts >>>>> b/arch/mips/boot/dts/img/pistachio_marduk.dts >>>>> index a8708783f04b..a8da2f992b1a 100644 >>>>> --- a/arch/mips/boot/dts/img/pistachio_marduk.dts >>>>> +++ b/arch/mips/boot/dts/img/pistachio_marduk.dts >>>>> @@ -59,12 +59,12 @@ led-1 { >>>>> >>>>> keys { >>>>> compatible = "gpio-keys"; >>>>> - button@1 { >>>>> + button-1 { >>>>> label = "Button 1"; >>>>> linux,code = <0x101>; /* BTN_1 */ >>>>> gpios = <&gpio3 6 GPIO_ACTIVE_LOW>; >>>>> }; >>>>> - button@2 { >>>>> + button-2 { >>>>> label = "Button 2"; >>>>> linux,code = <0x102>; /* BTN_2 */ >>>>> gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; >>>>> diff --git a/arch/mips/boot/dts/ingenic/gcw0.dts >>>>> b/arch/mips/boot/dts/ingenic/gcw0.dts >>>>> index 4abb0318416c..5d33f26fd28c 100644 >>>>> --- a/arch/mips/boot/dts/ingenic/gcw0.dts >>>>> +++ b/arch/mips/boot/dts/ingenic/gcw0.dts >>>>> @@ -130,89 +130,86 @@ backlight: backlight { >>>>> >>>>> gpio-keys { >>>>> compatible = "gpio-keys"; >>>>> - #address-cells = <1>; >>>>> - #size-cells = <0>; >>>> >>>> Are you sure you can remove these? >>> >>> Yes, from DT spec point of view, DT bindings and Linux implementation. >>> However this particular change was not tested, except building. >>> >>>> >>>> Looking at paragraph 2.3.5 of the DT spec, I would think they have >>>> to >>>> stay (although with #address-cells = <0>). >>> >>> The paragraph 2.3.5 says nothing about regular properties (which can >>> be >>> also child nodes). It says about children of a bus, right? It's not >>> related here, it's not a bus. >> >> I quote: >> "A DTSpec-compliant boot program shall supply #address-cells and >> #size-cells on all nodes that have children." > > And paragraph 2.2.3 says: > "A unit address may be omitted if the full path to the node is unambiguous." > > You have address/size cells for nodes with children having unit > addresses. If they don't unit addresses, you don't add address/size > cells (with some exceptions). > > The paragraph 2.3.5 mentions "child device nodes" and these properties > are not devices, although I agree that DT spec here is actually confusing. > >> >> The gpio-keys node has children nodes, therefore it should have >> #address-cells and #size-cells, there's no room for interpretation here. >> >>> Second, why exactly this one gpio-keys node is different than all >>> other >>> gpio-keys everywhere and than bindings? Why this one has to be >>> incompatible/wrong according to bindings (which do not allow >>> address-cells and nodes with unit addresses)? >> >> Nothing is different. I'm just stating that your proposed fix is >> invalid if we want to enforce compliance with the DT spec. > > In such case, we rather enforce the compliance with the bindings. > > Best regards, > Krzysztof I recall them to be unnecessary as well. I have a patch of mine applied identical to this: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=8c9f00d4b05134164e462f27b21c8295255ffa64 Also, I don't see any warnings with this patch applied: $ ARCH=mips CROSS_COMPILE=mipsel-linux-gnu- make clean dtbs -j$(nproc) SYNC include/config/auto.conf.cmd HOSTCC scripts/basic/fixdep HOSTCC scripts/kconfig/conf.o HOSTCC scripts/kconfig/confdata.o HOSTCC scripts/kconfig/expr.o LEX scripts/kconfig/lexer.lex.c YACC scripts/kconfig/parser.tab.[ch] HOSTCC scripts/kconfig/menu.o HOSTCC scripts/kconfig/preprocess.o HOSTCC scripts/kconfig/symbol.o HOSTCC scripts/kconfig/util.o HOSTCC scripts/kconfig/lexer.lex.o HOSTCC scripts/kconfig/parser.tab.o HOSTLD scripts/kconfig/conf HOSTCC scripts/dtc/dtc.o HOSTCC scripts/dtc/flattree.o HOSTCC scripts/dtc/fstree.o HOSTCC scripts/dtc/data.o HOSTCC scripts/dtc/livetree.o HOSTCC scripts/dtc/treesource.o HOSTCC scripts/dtc/srcpos.o HOSTCC scripts/dtc/checks.o HOSTCC scripts/dtc/util.o LEX scripts/dtc/dtc-lexer.lex.c YACC scripts/dtc/dtc-parser.tab.[ch] HOSTCC scripts/dtc/libfdt/fdt.o HOSTCC scripts/dtc/libfdt/fdt_ro.o HOSTCC scripts/dtc/libfdt/fdt_wip.o HOSTCC scripts/dtc/libfdt/fdt_sw.o HOSTCC scripts/dtc/libfdt/fdt_rw.o HOSTCC scripts/dtc/libfdt/fdt_strerror.o HOSTCC scripts/dtc/libfdt/fdt_empty_tree.o HOSTCC scripts/dtc/libfdt/fdt_addresses.o HOSTCC scripts/dtc/libfdt/fdt_overlay.o HOSTCC scripts/dtc/fdtoverlay.o HOSTCC scripts/dtc/dtc-lexer.lex.o HOSTCC scripts/dtc/dtc-parser.tab.o HOSTLD scripts/dtc/fdtoverlay HOSTLD scripts/dtc/dtc DTC arch/mips/boot/dts/ingenic/gcw0.dtb Have my acked-by. Acked-by: Arınç ÜNAL <arinc.unal@arinc9.com> Arınç
On Fri, Jun 24, 2022 at 07:07:39PM +0200, Krzysztof Kozlowski wrote: > gpio-keys children do not use unit addresses. > > Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> > > --- > > See: https://lore.kernel.org/all/20220616005224.18391-1-krzysztof.kozlowski@linaro.org/ > --- > arch/mips/boot/dts/img/pistachio_marduk.dts | 4 +-- > arch/mips/boot/dts/ingenic/gcw0.dts | 31 +++++++++---------- > arch/mips/boot/dts/ingenic/rs90.dts | 18 +++++------ > arch/mips/boot/dts/pic32/pic32mzda_sk.dts | 9 ++---- > .../boot/dts/qca/ar9132_tl_wr1043nd_v1.dts | 6 ++-- > arch/mips/boot/dts/qca/ar9331_dpt_module.dts | 4 +-- > .../mips/boot/dts/qca/ar9331_dragino_ms14.dts | 6 ++-- > arch/mips/boot/dts/qca/ar9331_omega.dts | 4 +-- > .../qca/ar9331_openembed_som9331_board.dts | 4 +-- > arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts | 8 ++--- > 10 files changed, 37 insertions(+), 57 deletions(-) applied to mips-next. Thomas.
diff --git a/arch/mips/boot/dts/img/pistachio_marduk.dts b/arch/mips/boot/dts/img/pistachio_marduk.dts index a8708783f04b..a8da2f992b1a 100644 --- a/arch/mips/boot/dts/img/pistachio_marduk.dts +++ b/arch/mips/boot/dts/img/pistachio_marduk.dts @@ -59,12 +59,12 @@ led-1 { keys { compatible = "gpio-keys"; - button@1 { + button-1 { label = "Button 1"; linux,code = <0x101>; /* BTN_1 */ gpios = <&gpio3 6 GPIO_ACTIVE_LOW>; }; - button@2 { + button-2 { label = "Button 2"; linux,code = <0x102>; /* BTN_2 */ gpios = <&gpio2 14 GPIO_ACTIVE_LOW>; diff --git a/arch/mips/boot/dts/ingenic/gcw0.dts b/arch/mips/boot/dts/ingenic/gcw0.dts index 4abb0318416c..5d33f26fd28c 100644 --- a/arch/mips/boot/dts/ingenic/gcw0.dts +++ b/arch/mips/boot/dts/ingenic/gcw0.dts @@ -130,89 +130,86 @@ backlight: backlight { gpio-keys { compatible = "gpio-keys"; - #address-cells = <1>; - #size-cells = <0>; - autorepeat; - button@0 { + button-0 { label = "D-pad up"; linux,code = <KEY_UP>; linux,can-disable; gpios = <&gpe 21 GPIO_ACTIVE_LOW>; }; - button@1 { + button-1 { label = "D-pad down"; linux,code = <KEY_DOWN>; linux,can-disable; gpios = <&gpe 25 GPIO_ACTIVE_LOW>; }; - button@2 { + button-2 { label = "D-pad left"; linux,code = <KEY_LEFT>; linux,can-disable; gpios = <&gpe 23 GPIO_ACTIVE_LOW>; }; - button@3 { + button-3 { label = "D-pad right"; linux,code = <KEY_RIGHT>; linux,can-disable; gpios = <&gpe 24 GPIO_ACTIVE_LOW>; }; - button@4 { + button-4 { label = "Button A"; linux,code = <KEY_LEFTCTRL>; linux,can-disable; gpios = <&gpe 29 GPIO_ACTIVE_LOW>; }; - button@5 { + button-5 { label = "Button B"; linux,code = <KEY_LEFTALT>; linux,can-disable; gpios = <&gpe 20 GPIO_ACTIVE_LOW>; }; - button@6 { + button-6 { label = "Button Y"; linux,code = <KEY_SPACE>; linux,can-disable; gpios = <&gpe 27 GPIO_ACTIVE_LOW>; }; - button@7 { + button-7 { label = "Button X"; linux,code = <KEY_LEFTSHIFT>; linux,can-disable; gpios = <&gpe 28 GPIO_ACTIVE_LOW>; }; - button@8 { + button-8 { label = "Left shoulder button"; linux,code = <KEY_TAB>; linux,can-disable; gpios = <&gpb 20 GPIO_ACTIVE_LOW>; }; - button@9 { + button-9 { label = "Right shoulder button"; linux,code = <KEY_BACKSPACE>; linux,can-disable; gpios = <&gpe 26 GPIO_ACTIVE_LOW>; }; - button@10 { + button-10 { label = "Start button"; linux,code = <KEY_ENTER>; linux,can-disable; gpios = <&gpb 21 GPIO_ACTIVE_LOW>; }; - button@11 { + button-11 { label = "Select button"; linux,code = <KEY_ESC>; linux,can-disable; @@ -223,7 +220,7 @@ button@11 { gpios = <&gpd 18 GPIO_ACTIVE_HIGH>; }; - button@12 { + button-12 { label = "Power slider"; linux,code = <KEY_POWER>; linux,can-disable; @@ -231,7 +228,7 @@ button@12 { wakeup-source; }; - button@13 { + button-13 { label = "Power hold"; linux,code = <KEY_PAUSE>; linux,can-disable; diff --git a/arch/mips/boot/dts/ingenic/rs90.dts b/arch/mips/boot/dts/ingenic/rs90.dts index 74fee7f01352..e8df70dd42bf 100644 --- a/arch/mips/boot/dts/ingenic/rs90.dts +++ b/arch/mips/boot/dts/ingenic/rs90.dts @@ -52,53 +52,51 @@ backlight: backlight { keys@0 { compatible = "gpio-keys"; - #address-cells = <1>; - #size-cells = <0>; - key@0 { + key-0 { label = "D-pad up"; linux,code = <KEY_UP>; gpios = <&gpc 10 GPIO_ACTIVE_LOW>; }; - key@1 { + key-1 { label = "D-pad down"; linux,code = <KEY_DOWN>; gpios = <&gpc 11 GPIO_ACTIVE_LOW>; }; - key@2 { + key-2 { label = "D-pad left"; linux,code = <KEY_LEFT>; gpios = <&gpb 31 GPIO_ACTIVE_LOW>; }; - key@3 { + key-3 { label = "D-pad right"; linux,code = <KEY_RIGHT>; gpios = <&gpd 21 GPIO_ACTIVE_LOW>; }; - key@4 { + key-4 { label = "Button A"; linux,code = <KEY_LEFTCTRL>; gpios = <&gpc 31 GPIO_ACTIVE_LOW>; }; - key@5 { + key-5 { label = "Button B"; linux,code = <KEY_LEFTALT>; gpios = <&gpc 30 GPIO_ACTIVE_LOW>; }; - key@6 { + key-6 { label = "Right shoulder button"; linux,code = <KEY_BACKSPACE>; gpios = <&gpc 12 GPIO_ACTIVE_LOW>; debounce-interval = <10>; }; - key@7 { + key-7 { label = "Start button"; linux,code = <KEY_ENTER>; gpios = <&gpd 17 GPIO_ACTIVE_LOW>; diff --git a/arch/mips/boot/dts/pic32/pic32mzda_sk.dts b/arch/mips/boot/dts/pic32/pic32mzda_sk.dts index d7fa5d55dbf3..ab70637bbec5 100644 --- a/arch/mips/boot/dts/pic32/pic32mzda_sk.dts +++ b/arch/mips/boot/dts/pic32/pic32mzda_sk.dts @@ -52,22 +52,19 @@ keys0 { pinctrl-0 = <&user_buttons_s0>; pinctrl-names = "default"; - #address-cells = <1>; - #size-cells = <0>; - - button@sw1 { + button-1 { label = "ESC"; linux,code = <1>; gpios = <&gpio1 12 0>; }; - button@sw2 { + button-2 { label = "Home"; linux,code = <102>; gpios = <&gpio1 13 0>; }; - button@sw3 { + button-3 { label = "Menu"; linux,code = <139>; gpios = <&gpio1 14 0>; diff --git a/arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts b/arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts index 7fccf6357225..f3dff4009ab5 100644 --- a/arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts +++ b/arch/mips/boot/dts/qca/ar9132_tl_wr1043nd_v1.dts @@ -23,17 +23,15 @@ extosc: ref { gpio-keys { compatible = "gpio-keys"; - #address-cells = <1>; - #size-cells = <0>; - button@0 { + button-0 { label = "reset"; linux,code = <KEY_RESTART>; gpios = <&gpio 3 GPIO_ACTIVE_LOW>; debounce-interval = <60>; }; - button@1 { + button-1 { label = "qss"; linux,code = <KEY_WPS_BUTTON>; gpios = <&gpio 7 GPIO_ACTIVE_LOW>; diff --git a/arch/mips/boot/dts/qca/ar9331_dpt_module.dts b/arch/mips/boot/dts/qca/ar9331_dpt_module.dts index 7695d326df11..c857cd22f7db 100644 --- a/arch/mips/boot/dts/qca/ar9331_dpt_module.dts +++ b/arch/mips/boot/dts/qca/ar9331_dpt_module.dts @@ -33,10 +33,8 @@ led-0 { gpio-keys { compatible = "gpio-keys"; - #address-cells = <1>; - #size-cells = <0>; - button@0 { + button-0 { label = "reset"; linux,code = <KEY_RESTART>; gpios = <&gpio 11 GPIO_ACTIVE_LOW>; diff --git a/arch/mips/boot/dts/qca/ar9331_dragino_ms14.dts b/arch/mips/boot/dts/qca/ar9331_dragino_ms14.dts index d38aa73f1a2e..40e4c5da0e65 100644 --- a/arch/mips/boot/dts/qca/ar9331_dragino_ms14.dts +++ b/arch/mips/boot/dts/qca/ar9331_dragino_ms14.dts @@ -49,16 +49,14 @@ system { gpio-keys { compatible = "gpio-keys"; - #address-cells = <1>; - #size-cells = <0>; - button@0 { + button-0 { label = "jumpstart"; linux,code = <KEY_WPS_BUTTON>; gpios = <&gpio 11 GPIO_ACTIVE_LOW>; }; - button@1 { + button-1 { label = "reset"; linux,code = <KEY_RESTART>; gpios = <&gpio 12 GPIO_ACTIVE_LOW>; diff --git a/arch/mips/boot/dts/qca/ar9331_omega.dts b/arch/mips/boot/dts/qca/ar9331_omega.dts index 11778abacf66..ed184d861d5f 100644 --- a/arch/mips/boot/dts/qca/ar9331_omega.dts +++ b/arch/mips/boot/dts/qca/ar9331_omega.dts @@ -31,10 +31,8 @@ system { gpio-keys { compatible = "gpio-keys"; - #address-cells = <1>; - #size-cells = <0>; - button@0 { + button-0 { label = "reset"; linux,code = <KEY_RESTART>; gpios = <&gpio 11 GPIO_ACTIVE_HIGH>; diff --git a/arch/mips/boot/dts/qca/ar9331_openembed_som9331_board.dts b/arch/mips/boot/dts/qca/ar9331_openembed_som9331_board.dts index e6622f8e8c2b..dc65ebd60bbc 100644 --- a/arch/mips/boot/dts/qca/ar9331_openembed_som9331_board.dts +++ b/arch/mips/boot/dts/qca/ar9331_openembed_som9331_board.dts @@ -33,10 +33,8 @@ led-0 { gpio-keys { compatible = "gpio-keys"; - #address-cells = <1>; - #size-cells = <0>; - button@0 { + button-0 { label = "reset"; linux,code = <KEY_RESTART>; gpios = <&gpio 11 GPIO_ACTIVE_HIGH>; diff --git a/arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts b/arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts index c8290d36cfbe..5f424c2cd781 100644 --- a/arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts +++ b/arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts @@ -49,22 +49,20 @@ led3g { gpio-keys { compatible = "gpio-keys"; - #address-cells = <1>; - #size-cells = <0>; - button@0 { + button-0 { label = "wps"; linux,code = <KEY_WPS_BUTTON>; gpios = <&gpio 11 GPIO_ACTIVE_HIGH>; }; - button@1 { + button-1 { label = "sw1"; linux,code = <BTN_0>; gpios = <&gpio 18 GPIO_ACTIVE_HIGH>; }; - button@2 { + button-2 { label = "sw2"; linux,code = <BTN_1>; gpios = <&gpio 20 GPIO_ACTIVE_HIGH>;
gpio-keys children do not use unit addresses. Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> --- See: https://lore.kernel.org/all/20220616005224.18391-1-krzysztof.kozlowski@linaro.org/ --- arch/mips/boot/dts/img/pistachio_marduk.dts | 4 +-- arch/mips/boot/dts/ingenic/gcw0.dts | 31 +++++++++---------- arch/mips/boot/dts/ingenic/rs90.dts | 18 +++++------ arch/mips/boot/dts/pic32/pic32mzda_sk.dts | 9 ++---- .../boot/dts/qca/ar9132_tl_wr1043nd_v1.dts | 6 ++-- arch/mips/boot/dts/qca/ar9331_dpt_module.dts | 4 +-- .../mips/boot/dts/qca/ar9331_dragino_ms14.dts | 6 ++-- arch/mips/boot/dts/qca/ar9331_omega.dts | 4 +-- .../qca/ar9331_openembed_som9331_board.dts | 4 +-- arch/mips/boot/dts/qca/ar9331_tl_mr3020.dts | 8 ++--- 10 files changed, 37 insertions(+), 57 deletions(-)