> -----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. > > For some of what you have in this binding, I think it should be part > of > the consumer cells. > ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ > ++ > > > > Best regards, > > Thomas > >
