Re: [PATCH v2 01/22] dt-bindings: net: wireless: describe the ath12k AHB module

[Krzysztof Kozlowski <krzk@kernel.org>]

On 16/10/2024 10:37, Raj Kumar Bhagat wrote:
> On 10/16/2024 12:32 PM, Krzysztof Kozlowski wrote:
>> On Tue, Oct 15, 2024 at 11:56:16PM +0530, Raj Kumar Bhagat wrote:
>>> Add device-tree bindings for the ATH12K module found in the IPQ5332
>>> device.
>>>
>>> Signed-off-by: Raj Kumar Bhagat <quic_rajkbhag@quicinc.com>
>>> ---
>>
>> That's a v2, what changed?
>>
>> Did you ignore entire review? Limited review follows because of that (I
>> am not going to do the same work twice).
>>
> 
> Review comments in version 1 are addressed.
> 
> Per-patch version log is not added here. But we have consolidated version log
> in the cover-letter. Will include per-patch version log from next version.

Hm? There is no such in cover letter. There is description, dependencies
and list of commits which indicates end (format-patch standard stuff).
> 
> Below are the version log for v2:
> - "qcom,board_id" property is dropped. This is not the direct dependency for Ath12k
>   AHB support, hence it can be taken up separately.
> - "qcom,bdf-addr" property is dropped in device-tree and moved to ath12k driver.
> - Currently we have only one compatible enum (qcom,ipq5332-wifi), hence conditional
>   if() check for defining the binding is removed.
> - "reserved-memory" node is dropped from example DTS.
> - "status" property is dropped in wifi node of example DTS.


>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: gcc_xo_clk
>>
>> Drop _clk, drop gcc_. Look how this clock is called *everywhere* else.
>>
> 
> Thanks, Based on other bindings example, will rename to "xo"

git grep gcc_xo_clk -> nothing like that!

...

>>> +
>>> +  memory-region:
>>> +    minItems: 1
>>
>> upper constraint
>>
>>> +    description:
>>> +      phandle to a node describing reserved memory (System RAM memory)
>>> +      used by ath12k firmware (see bindings/reserved-memory/reserved-memory.txt)
>>> +
>>> +  qcom,rproc:
>>> +    $ref: /schemas/types.yaml#/definitions/phandle
>>> +    description:
>>> +      DT entry of a WCSS node. WCSS node is the child node of q6 remoteproc driver.
>>> +      (see bindings/remoteproc/qcom,multipd-pil.yaml)
>>
>> DT nodes are not children of drivers. But other DT nodes. Explain why
>> this phandle is needed, what is it for.
>>
>> To me it looks like you incorrectly organized your nodes.
>>
> 
> This phandle is required by wifi driver (ath12k) to retrieve the correct remote processor
> (rproc_get_by_phandle()). Ath12k driver needs this rproc to interact with the remote
> processor (example: booting-up remote processor).

That's driver aspect. Why does the hardware needs it?

WiFi is the remote processor, so I would expect this being a child. Or
just drop entirely.

You keep using here arguments how you designed your drivers, which is
not valid. Sorry, fix your drivers... or use arguments in terms of hardware.


> 
> In next version, will correct the description based on existing bindings (qcom,ath11k.yaml).

Sorry, let's don't copy existing solutions just because they exist.



Best regards,
Krzysztof