Messages in this thread | | | Subject | Re: [PATCH v7 1/2] dt-bindings: dma: Add bindings for intel LGM SOC | From | "Reddy, MallikarjunaX" <> | Date | Mon, 2 Nov 2020 22:41:54 +0800 |
| |
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 >
| |