diff mbox series

[v2,4/8] hw/arm/virt: Fix devicetree warning about the gpio-key node

Message ID 20220927100347.176606-5-jean-philippe@linaro.org
State New
Headers show
Series hw/arm/virt: Fix dt-schema warnings | expand

Commit Message

Jean-Philippe Brucker Sept. 27, 2022, 10:03 a.m. UTC
The node name of the gpio-key devicetree node should be "key-poweroff":

  gpio-keys: 'poweroff' does not match any of the regexes: '^(button|event|key|switch|(button|event|key|switch)-[a-z0-9-]+|[a-z0-9-]+-(button|event|key|switch))$', 'pinctrl-[0-9]+'
  From schema: linux/Documentation/devicetree/bindings/input/gpio-keys.yaml

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
---
 hw/arm/virt.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Peter Maydell Sept. 27, 2022, 11:56 a.m. UTC | #1
On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
<jean-philippe@linaro.org> wrote:
> The node name of the gpio-key devicetree node should be "key-poweroff":
>
>   gpio-keys: 'poweroff' does not match any of the regexes: '^(button|event|key|switch|(button|event|key|switch)-[a-z0-9-]+|[a-z0-9-]+-(button|event|key|switch))$', 'pinctrl-[0-9]+'
>   From schema: linux/Documentation/devicetree/bindings/input/gpio-keys.yaml
>
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

This restriction only went into the DT documentation in July
(kernel commit 5eb5652250).

Please don't retrospectively make perfectly valid working DTs
non-valid. I don't see any reason to change QEMU here.

More generally, the set of things you might want the
validator to warn about for a fresh new human-written DTB
doesn't necessarily correspond to the set of things you want
to enforce for a pre-existing code-generated DTB. For the
former it makes much more sense to impose "coding style"
and "naming convention" type rules.

thanks
-- PMM
Eric Auger Sept. 27, 2022, 12:48 p.m. UTC | #2
On 9/27/22 12:03, Jean-Philippe Brucker wrote:
> The node name of the gpio-key devicetree node should be "key-poweroff":
> 
>   gpio-keys: 'poweroff' does not match any of the regexes: '^(button|event|key|switch|(button|event|key|switch)-[a-z0-9-]+|[a-z0-9-]+-(button|event|key|switch))$', 'pinctrl-[0-9]+'
>   From schema: linux/Documentation/devicetree/bindings/input/gpio-keys.yaml
> 
> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric

> ---
>  hw/arm/virt.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 8605f5058a..6805c57530 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -932,12 +932,12 @@ static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
>      qemu_fdt_add_subnode(fdt, "/gpio-keys");
>      qemu_fdt_setprop_string(fdt, "/gpio-keys", "compatible", "gpio-keys");
>  
> -    qemu_fdt_add_subnode(fdt, "/gpio-keys/poweroff");
> -    qemu_fdt_setprop_string(fdt, "/gpio-keys/poweroff",
> +    qemu_fdt_add_subnode(fdt, "/gpio-keys/key-poweroff");
> +    qemu_fdt_setprop_string(fdt, "/gpio-keys/key-poweroff",
>                              "label", "GPIO Key Poweroff");
> -    qemu_fdt_setprop_cell(fdt, "/gpio-keys/poweroff", "linux,code",
> +    qemu_fdt_setprop_cell(fdt, "/gpio-keys/key-poweroff", "linux,code",
>                            KEY_POWER);
> -    qemu_fdt_setprop_cells(fdt, "/gpio-keys/poweroff",
> +    qemu_fdt_setprop_cells(fdt, "/gpio-keys/key-poweroff",
>                             "gpios", phandle, 3, 0);
>  }
>
Rob Herring Oct. 13, 2022, 9:46 p.m. UTC | #3
On Tue, Sep 27, 2022 at 6:56 AM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Tue, 27 Sept 2022 at 11:12, Jean-Philippe Brucker
> <jean-philippe@linaro.org> wrote:
> > The node name of the gpio-key devicetree node should be "key-poweroff":
> >
> >   gpio-keys: 'poweroff' does not match any of the regexes: '^(button|event|key|switch|(button|event|key|switch)-[a-z0-9-]+|[a-z0-9-]+-(button|event|key|switch))$', 'pinctrl-[0-9]+'
> >   From schema: linux/Documentation/devicetree/bindings/input/gpio-keys.yaml
> >
> > Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>
>
> This restriction only went into the DT documentation in July
> (kernel commit 5eb5652250).

Fair enough.

> Please don't retrospectively make perfectly valid working DTs
> non-valid. I don't see any reason to change QEMU here.
>
> More generally, the set of things you might want the
> validator to warn about for a fresh new human-written DTB
> doesn't necessarily correspond to the set of things you want
> to enforce for a pre-existing code-generated DTB. For the
> former it makes much more sense to impose "coding style"
> and "naming convention" type rules.

I too would like to distinguish that, but haven't come up with a way
to do that in json-schema yet. The way schemas are applied
independently makes that a challenge. So far it's been low on the
priority list as any platforms with few enough warnings to get to 0
haven't been a problem to fix (in a few cases we do end up relaxing
the schemas).

On the flip side, even existing things eventually get updated for
coding style or evolving conventions. As long as we don't break ABIs,
the same should apply to DT.

Rob
Peter Maydell Oct. 14, 2022, 11:37 a.m. UTC | #4
On Thu, 13 Oct 2022 at 22:47, Rob Herring <robh+dt@kernel.org> wrote:
>
> On Tue, Sep 27, 2022 at 6:56 AM Peter Maydell <peter.maydell@linaro.org> wrote:
> > Please don't retrospectively make perfectly valid working DTs
> > non-valid. I don't see any reason to change QEMU here.
> >
> > More generally, the set of things you might want the
> > validator to warn about for a fresh new human-written DTB
> > doesn't necessarily correspond to the set of things you want
> > to enforce for a pre-existing code-generated DTB. For the
> > former it makes much more sense to impose "coding style"
> > and "naming convention" type rules.
>
> I too would like to distinguish that, but haven't come up with a way
> to do that in json-schema yet. The way schemas are applied
> independently makes that a challenge. So far it's been low on the
> priority list as any platforms with few enough warnings to get to 0
> haven't been a problem to fix (in a few cases we do end up relaxing
> the schemas).
>
> On the flip side, even existing things eventually get updated for
> coding style or evolving conventions. As long as we don't break ABIs,
> the same should apply to DT.

Yeah, but from QEMU's point of view pretty much the whole DT
*is* ABI, because we have no idea what the guest will be
doing with it, whether it is looking for things in the
"correct" way or if it happens to have hard-coded a node
name or similar: and the guest could be any of a wide
variety of operating systems or custom code, and including
pretty old versions of OSes as well as the latest-and-greatest.
This is different from Linux's handwritten device trees,
which only need to work with Linux and only with the associated
Linux version, at that. So the set of things I'm happy changing
tends to be more restricted than the set of things it's worth
cleaning up in the dt sources that ship with the kernel.

thanks
-- PMM
diff mbox series

Patch

diff --git a/hw/arm/virt.c b/hw/arm/virt.c
index 8605f5058a..6805c57530 100644
--- a/hw/arm/virt.c
+++ b/hw/arm/virt.c
@@ -932,12 +932,12 @@  static void create_gpio_keys(char *fdt, DeviceState *pl061_dev,
     qemu_fdt_add_subnode(fdt, "/gpio-keys");
     qemu_fdt_setprop_string(fdt, "/gpio-keys", "compatible", "gpio-keys");
 
-    qemu_fdt_add_subnode(fdt, "/gpio-keys/poweroff");
-    qemu_fdt_setprop_string(fdt, "/gpio-keys/poweroff",
+    qemu_fdt_add_subnode(fdt, "/gpio-keys/key-poweroff");
+    qemu_fdt_setprop_string(fdt, "/gpio-keys/key-poweroff",
                             "label", "GPIO Key Poweroff");
-    qemu_fdt_setprop_cell(fdt, "/gpio-keys/poweroff", "linux,code",
+    qemu_fdt_setprop_cell(fdt, "/gpio-keys/key-poweroff", "linux,code",
                           KEY_POWER);
-    qemu_fdt_setprop_cells(fdt, "/gpio-keys/poweroff",
+    qemu_fdt_setprop_cells(fdt, "/gpio-keys/key-poweroff",
                            "gpios", phandle, 3, 0);
 }