[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <b5f352e8-19a5-4c06-b9ba-44af791f6a81@kernel.org>
Date: Wed, 16 Oct 2024 11:00:58 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Raj Kumar Bhagat <quic_rajkbhag@...cinc.com>
Cc: ath12k@...ts.infradead.org, linux-wireless@...r.kernel.org,
Kalle Valo <kvalo@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley
<conor+dt@...nel.org>, Jeff Johnson <jjohnson@...nel.org>,
Bjorn Andersson <andersson@...nel.org>,
Konrad Dybcio <konradybcio@...nel.org>, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 01/22] dt-bindings: net: wireless: describe the ath12k
AHB module
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@...cinc.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
Powered by blists - more mailing lists