Message-ID:
 <DS7PR19MB88836505C4CC48D3E62FBC0A9DFAA@DS7PR19MB8883.namprd19.prod.outlook.com>
Date: Wed, 29 Oct 2025 20:12:37 +0400
From: George Moussalem <george.moussalem@...look.com>
To: Krzysztof Kozlowski <krzk@...nel.org>,
 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 10/29/25 18:32, Krzysztof Kozlowski wrote:
> 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:".
> 
Will do in next version, thanks.

> 
> 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
> 

Will change accordingly.

>>
>> 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.
> 

it was exactly ipq5018 which got me thinking I should add it to the end.
Will alphabetically insert qcn6122. I could reorder the entire list as
well if you'd like (ipq6018 is also misplaced)

>>  
>>    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.

Would you mind pointing me to the property you're referring to? Do you
mean the QRTR ID as proposed in this RFC:
https://lore.kernel.org/linux-wireless/cover.1732506261.git.ionic@ionic.de/

if so, this wouldn't help this in this case. Although it's PCIe based,
PCI is not even enabled as the Q6 firmware itself takes care of that.

> 
>> +
>>  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.

Dropped in next version.
Thanks for the quick review!

> 
> BR
> 
> Best regards,
> Krzysztof

Powered by blists - more mailing lists