linux-kernel - Re: [PATCH 1/6] dt: bindings: net: add bindings for QCN6122

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3dc712ae-b51f-4142-bbab-1eadbc27e60a@kernel.org>
Date: Wed, 29 Oct 2025 15:32:17 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: george.moussalem@...look.com, Johannes Berg <johannes@...solutions.net>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Jeff Johnson <jjohnson@...nel.org>
Cc: linux-wireless@...r.kernel.org, devicetree@...r.kernel.org,
 ath11k@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] dt: bindings: net: add bindings for QCN6122

On 29/10/2025 15:26, George Moussalem via B4 Relay wrote:
> From: George Moussalem <george.moussalem@...look.com>
> 
> QCN6122 is a PCIe based solution that is attached to and enumerated
> by the WPSS (Wireless Processor SubSystem) Q6 processor.

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

The prefix is never "dt:".


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

> 
> Though it is a PCIe device, since it is not attached to APSS processor
> (Application Processor SubSystem), APSS will be unaware of such a decice
> so it is registered to the APSS processor as a platform device(AHB).
> Because of this hybrid nature, it is called as a hybrid bus device as
> introduced by WCN6750. It has 5 CE and 8 DP rings.
> 
> QCN6122 is similar to WCN6750 and follows the same codepath as for
> WCN6750.
> 
> Signed-off-by: George Moussalem <george.moussalem@...look.com>
> ---
>  .../bindings/net/wireless/qcom,ath11k.yaml         | 57 +++++++++++++++++++++-
>  1 file changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml
> index c089677702cf17f3016b054d21494d2a7706ce5d..4b0b282bb9231c8bc496fed42e0917b9d7d106d2 100644
> --- a/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml
> +++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath11k.yaml
> @@ -21,12 +21,13 @@ properties:
>        - qcom,ipq6018-wifi
>        - qcom,wcn6750-wifi
>        - qcom,ipq5018-wifi
> +      - qcom,qcn6122-wifi

Why people keep adding to the end... previously ipq5018 added by
qualcom, did not even get any review.

Place it before wcn and let ipq5018 be outlier since this was broken
already.

>  
>    reg:
>      maxItems: 1
>  
>    interrupts:
> -    minItems: 32
> +    minItems: 13
>      maxItems: 52
>  
>    interrupt-names:
> @@ -87,6 +88,14 @@ properties:
>      items:
>        - const: wlan-smp2p-out
>  
> +  qcom,userpd:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    enum: [2, 3]
> +    description: instance ID of user PD (protection domain) in multi-PD
> +                 architectures to distinguish between multiple instances
> +                 of the same wifi chip used by QMI in its interface with
> +                 the firmware running on Q6.

Broken indentation. It is supposed to be two spaces. Look at this file -
why are you doing this completely different?

Anyway, please do not come with 2nd or 3rd property for this. We already
have such somewhere.

> +
>  required:
>    - compatible
>    - reg
> @@ -268,6 +277,31 @@ allOf:
>              - description: interrupt event for ring DP20
>              - description: interrupt event for ring DP21
>              - description: interrupt event for ring DP22
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,qcn6122-wifi
> +    then:
> +      required:
> +        - qcom,userpd
> +      properties:
> +        interrupts:
> +          items:
> +            - description: interrupt event for ring CE1
> +            - description: interrupt event for ring CE2
> +            - description: interrupt event for ring CE3
> +            - description: interrupt event for ring CE4
> +            - description: interrupt event for ring CE5
> +            - description: interrupt event for ring DP1
> +            - description: interrupt event for ring DP2
> +            - description: interrupt event for ring DP3
> +            - description: interrupt event for ring DP4
> +            - description: interrupt event for ring DP5
> +            - description: interrupt event for ring DP6
> +            - description: interrupt event for ring DP7
> +            - description: interrupt event for ring DP8
>  
>  examples:
>    - |
> @@ -467,3 +501,24 @@ examples:
>              iommus = <&apps_smmu 0x1c02 0x1>;
>          };
>      };
> +
> +  - |
> +    wifi1: wifi@...a040 {
> +        reg = <0x0b00a040 0x0>;
> +        compatible = "qcom,qcn6122-wifi";

Don't add examples if they differ by one property. Drop.

BR

Best regards,
Krzysztof