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