Re: [PATCH] dt-bindings: hwlock: Convert to dtschema

[Krzysztof Kozlowski <krzk@kernel.org>]

On 18/02/2025 17:09, Siddharth Menon wrote:
> From: BiscuitBobby <simeddon@gmail.com>
> 
> Convert the generic hwspinlock bindings to DT schema.

Thank you for your patch. There is something to discuss/improve.

Please run scripts/checkpatch.pl and fix reported warnings. After that,
run also `scripts/checkpatch.pl --strict` and (probably) fix more
warnings. Some warnings can be ignored, especially from --strict run,
but the code here looks like it needs a fix. Feel free to get in touch
if the warning is not clear.

Also you must use your full name, not nicknames.


> ---
>  This is my first time converting bindings to dt schema, please let me
>  know if I have overlooked anything.
>  .../devicetree/bindings/hwlock/hwlock.txt     | 59 -----------------
>  .../devicetree/bindings/hwlock/hwlock.yaml    | 65 +++++++++++++++++++
>  2 files changed, 65 insertions(+), 59 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.txt
>  create mode 100644 Documentation/devicetree/bindings/hwlock/hwlock.yaml
> 

You leave now incorrect paths in the kernel.

If you decide to convert the generic subsystem binding, you must take
extra care and change/fix/update/improve all bindings using it.


> diff --git a/Documentation/devicetree/bindings/hwlock/hwlock.yaml b/Documentation/devicetree/bindings/hwlock/hwlock.yaml
> new file mode 100644
> index 000000000000..2492fdad3c6e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwlock/hwlock.yaml
> @@ -0,0 +1,65 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/hwlock/hwlock.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Generic Hardware Lock (hwlock)
> +
> +description: |
> +  Generic bindings that are common to all the hwlock platform specific driver
> +  implementations.
> +  Please also look through the individual platform specific hwlock binding
> +  documentations for identifying any additional properties specific to that
> +  platform.
> +
> +maintainers:
> +  - Bjorn Andersson <andersson@kernel.org>
> +  - Rob Herring <robh@kernel.org>
> +  - Krzysztof Kozlowski <krzk+dt@kernel.org>
> +  - Conor Dooley <conor+dt@kernel.org>

Subsystem maintainer only.

> +
> +properties:
> +  $nodename:
> +    pattern: "^hwlock(@.*)?"

Why .* in the pattern if you do not anchor it with $?

> +
> +  "#hwlock-cells":
> +    description: |
> +      Specifies the number of cells needed to represent a specific lock.
> +    minimum: 1
> +
> +  hwlocks:
> +    description: |

Do not need '|' unless you need to preserve formatting.

> +      List of phandle to a hwlock provider node and an associated hwlock args
> +      specifier as indicated by #hwlock-cells. The list can have just a single
> +      hwlock or multiple hwlocks, with each hwlock represented by a phandle and
> +      a corresponding args specifier.

Missing type, here and  other places, unless this is already covered by
dtschema? But then why this binding is needed?

> +
> +  hwlock-names:
> +    description: |
> +      List of hwlock name strings defined in the same order as the hwlocks,
> +      with one name per hwlock. Consumers can use the hwlock-names to match
> +      and get a specific hwlock.

Hm? I don't think you understood the binding. The provider does not have
consumer properties. Read again original binding.

> +
> +patternProperties:
> +  "^hwlock@[0-9a-f]+$":
> +    type: object
> +    description: Hardware lock provider node

This makes no sense. hwlock within hwlock?

> +
> +required:
> +  - "#hwlock-cells"
> +
> +additionalProperties: true
> +
> +examples:
> +  # Example 1: A node using a single specific hwlock


Drop all examples, not really useful.

Best regards,
Krzysztof