Re: [PATCH v2 01/16] dt-bindings: dma: qcom,bam: Add bam pipe lock

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 8/23/2024 2:37 PM, Krzysztof Kozlowski wrote:
On 22/08/2024 13:45, Md Sadre Alam wrote:


On 8/22/2024 11:57 AM, Krzysztof Kozlowski wrote:
On 21/08/2024 18:34, Md Sadre Alam wrote:


On 8/17/2024 2:38 PM, Krzysztof Kozlowski wrote:
On 15/08/2024 10:57, Md Sadre Alam wrote:
BAM having pipe locking mechanism. The Lock and Un-Lock bit
should be set on CMD descriptor only. Upon encountering a
descriptor with Lock bit set, the BAM will lock all other
pipes not related to the current pipe group, and keep
handling the current pipe only until it sees the Un-Lock
set.

Please wrap commit message according to Linux coding style / submission
process (neither too early nor over the limit):
https://elixir.bootlin.com/linux/v6.4-rc1/source/Documentation/process/submitting-patches.rst#L597
     Ok , will update in next patch.


Signed-off-by: Md Sadre Alam <quic_mdalam@xxxxxxxxxxx>
---

Change in [v2]

* Added initial support for dt-binding

Change in [v1]

* This patch was not included in [v1]

    Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml | 8 ++++++++
    1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
index 3ad0d9b1fbc5..91cc2942aa62 100644
--- a/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
+++ b/Documentation/devicetree/bindings/dma/qcom,bam-dma.yaml
@@ -77,6 +77,12 @@ properties:
          Indicates that the bam is powered up by a remote processor but must be
          initialized by the local processor.
+ qcom,bam_pipe_lock:

Please follow DTS coding style.
     Ok

+    type: boolean
+    description:
+      Indicates that the bam pipe needs locking or not based on client driver
+      sending the LOCK or UNLOK bit set on command descriptor.

You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.
     Ok, will update in next patch.

+
      reg:
        maxItems: 1
@@ -92,6 +98,8 @@ anyOf:
          - qcom,powered-remotely
      - required:
          - qcom,controlled-remotely
+  - required:
+      - qcom,bam_pipe_lock

Why is it here? What do you want to achieve?
     This property added to achieve locking/unlocking
     of BAM pipe groups for mutual exclusion of resources
     that can be used across multiple EE's

This explains me nothing. I am questioning the anyOf block. Why this is
the fourth method of controlling BAM? Anyway, if it is, then explain
this in commit msg.
    This is the BAM property for locking/unlocking the BAM pipes.That's
    why I kept in anyOf block.

You keep repeating the same. It's like poking me with the same comment
till I agree. I am done with this.

NAK. Provide proper rationale.
  Sorry, I misunderstood your review comment. Now as Mani suggested will
  keep this implementation in driver itself. Will drop the binding patch
  in next revision.

Best regards,
Krzysztof






[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux