Re: [RFC PATCH 2/2] dt-bindings: add device tree binding for silex multipk

[Krzysztof Kozlowski <krzk@kernel.org>]

On 12/03/2025 10:54, Nipun Gupta wrote:
> Add binding documentation for Silex multipk device node with compatible
> string as 'silex,mutlipk'.

Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching. For bindings, the preferred subjects are
explained here:
https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html#i-for-patch-submitters

A nit, subject: drop second/last, redundant "bindings". The
"dt-bindings" prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18

> 
> Signed-off-by: Nipun Gupta <nipun.gupta@amd.com>
> ---
>  .../bindings/misc/silex,multipk.yaml          | 50 +++++++++++++++++++

Bindings are before users.

>  MAINTAINERS                                   |  1 +
>  2 files changed, 51 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/misc/silex,multipk.yaml
> 
> diff --git a/Documentation/devicetree/bindings/misc/silex,multipk.yaml b/Documentation/devicetree/bindings/misc/silex,multipk.yaml
> new file mode 100644
> index 000000000000..6951886734ca
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/misc/silex,multipk.yaml
> @@ -0,0 +1,50 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/misc/silex,multipk.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Silex MultiPK driver

Drop "driver" and describe hardware.

> +
> +maintainers:
> +  - Nipun Gupta <nipun.gupta@amd.com>
> +  - Praveen Jain <praveen.jain@amd.com>
> +
> +description: |
> +  Silex Multipk device handles the Asymmetric crypto operations. The
> +  driver provides interface to user-space to directly interact with the
> +  Silex MultiPK device.

Why this isn't in crypto?

> +
> +properties:
> +  compatible:
> +    const: silex,mutlipk

Unknown vendor prefix

Device name part is weirdly generic. How is this device exactly called?
Where is it used? Where is datasheet?

> +
> +  interrupts:
> +    maxItems: 1
> +
> +  reg:
> +    items:
> +      - description: PKI Queues memory region
> +      - description: PKI TRNG memory region
> +      - description: PKI reset memory region

reset? Like reset controller? Why is this here instead of using existing
reset framework?

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - iommus

You did not test your patches.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    multipk@20400000000 {
> +      compatible = "silex,multipk";
> +      interrupts = <0x00000000 0x0000009b IRQ_TYPE_LEVEL_HIGH>;

This is nowhere DTS coding style. See how other bindings do it.

> +      reg = <0x00000204 0x00000000 0x00000000 0x00010000>,
> +            <0x00000204 0x00020000 0x00000000 0x00000050>,
> +            <0x00000000 0xEC200340 0x00000000 0x00000004>;

lowercase hex, drop the padings of r0.

> +      iommus = <&smmu 0x25B>;

Lowercase hex

> +    };
Why is this patch a RFC? What is incomplete here?

RFC means patch is not ready so you will not get full review. Full
review will come once you send proper patch (and remember about
changelog and versioning - this is v1).

Best regards,
Krzysztof