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

From: Krzysztof Kozlowski
Date: Sat Jun 25 2022 - 16:25:44 EST


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@xxxxxxxxxx> 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@xxxxxxxxxx> a écrit :
>>>> gpio-keys children do not use unit addresses.
>>>>
>>>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
>>>>
>>>> ---
>>>>
>>>> See:
>>>>
>>>> https://lore.kernel.org/all/20220616005224.18391-1-krzysztof.kozlowski@xxxxxxxxxx/
>>>> ---
>>>> 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