Re: [PATCH v7 1/2] dt-bindings: dma: Add bindings for intel LGM SOC

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

 



Hi Thomas,

On 11/11/2020 1:39 AM, Thomas Langer wrote:

-----Original Message-----
From: Reddy, MallikarjunaX <mallikarjunax.reddy@xxxxxxxxxxxxxxx>
Sent: Montag, 2. November 2020 15:42
To: Thomas Langer <tlanger@xxxxxxxxxxxxx>; dmaengine@xxxxxxxxxxxxxxx;
vkoul@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx
Cc: linux-kernel@xxxxxxxxxxxxxxx; Shevchenko, Andriy
<andriy.shevchenko@xxxxxxxxx>; chuanhua.lei@xxxxxxxxxxxxxxx; Kim,
Cheol Yong <Cheol.Yong.Kim@xxxxxxxxx>; Wu, Qiming <qi-
ming.wu@xxxxxxxxx>; malliamireddy009@xxxxxxxxx; peter.ujfalusi@xxxxxx;
Langer, Thomas <thomas.langer@xxxxxxxxx>
Subject: Re: [PATCH v7 1/2] dt-bindings: dma: Add bindings for intel
LGM SOC

This email was sent from outside of MaxLinear.


Hi Thomas,
Thanks for the review, my comments inline.

On 10/28/2020 3:24 AM, Thomas Langer wrote:
Hello Reddy,

I think "Intel" should always be written with a capital "I" (like in
the Subject, but except in the binding below)
OK.
+ compatible:
+  oneOf:
+   - const: intel,lgm-cdma
+   - const: intel,lgm-dma2tx
+   - const: intel,lgm-dma1rx
+   - const: intel,lgm-dma1tx
+   - const: intel,lgm-dma0tx
+   - const: intel,lgm-dma3
+   - const: intel,lgm-toe-dma30
+   - const: intel,lgm-toe-dma31
Bindings are normally not per instance.
What if next generation chip gets more DMA modules but has no other
changes in the HW block?
What is wrong with
    - const: intel,lgm-cdma
    - const: intel,lgm-hdma
and extra attributes to define the rx/tx restriction (or what do it
mean?)?
  From the driver code I saw that "toe" is also just of type "hdma"
and no further differences in code are done.
We had a discussion on the same in the previous patches and Rob
Herring
said Okay using Different compatibles.
below the snippet.
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+
  >>> + compatible:
  >>> +  anyOf:
  >>> +   - const: intel,lgm-cdma
  >>> +   - const: intel,lgm-dma2tx
  >>> +   - const: intel,lgm-dma1rx
  >>> +   - const: intel,lgm-dma1tx
  >>> +   - const: intel,lgm-dma0tx
  >>> +   - const: intel,lgm-dma3
  >>> +   - const: intel,lgm-toe-dma30
  >>> +   - const: intel,lgm-toe-dma31
  >> Please explain why you need so many different compatible strings.
  > This hw dma has 7 DMA instances.
  > Some for datapath, some for memcpy  and some for TOE.
  > Some for TX only, some for RX only, and some for TX/RX(memcpy and
ToE).
  >
  > dma TX/RX type we considered as driver specific data of each
instance and
  > used different compatible strings for each instance.
  > And also idea is in future if any driver specific data of any
particular
  > instance we can handle.
  >
  > Here if dma name and type(tx or rx) will be accepted as devicetree
  > attributes then we can move .name = "toe_dma31", & .type =
DMA_TYPE_MCPY
  > to devicetree. So that the compatible strings can be limited to
two.
  > intel,lgm-cdma & intel,lgm-hdma .

[Rob]
Different compatibles are okay if the instances are different and we
don't have properties to describe the differences.
Okay, but then explain what the differences are, that cannot be described
by other properties/attributes. In the driver code I cannot see anything,
except the "name". But for printouts in driver, "drv_dbg" or similar will
just use the node path for the instance.
On patch4 series we had the same discussion.
i will brief it here again.

This hw dma has 7 DMA instances, and each Some for TX only, some for RX only, and some for TX/RX.

dma TX/RX type we considered as driver specific data and it cant be used as dt property as per the previous reviewers.

So i moved it to driver specific data.

If type(tx or rx) will be accepted as devicetree attributes then we can move it to devicetree.

So as you said we can limit compatible strings can be limited to two. intel,lgm-cdma & intel,lgm-hdma .

One more advantage i see with this model is in future if any driver specific data of any particular instance we can handle easily.

For some of what you have in this binding, I think it should be part
of
the consumer cells.
++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
++
Best regards,
Thomas




[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