Re: [PATCH 1/2] MIPS: dts: correct gpio-keys names and properties

["Arınç ÜNAL" <arinc.unal@arinc9.com>]

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ç