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,
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.

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