Hey, On Wed, Mar 09, 2022 at 04:39:40PM +0300, Serge Semin wrote: > Hello Frank > > On Mon, Mar 07, 2022 at 04:47:45PM -0600, Frank Li wrote: > > "struct dw_edma_chip" contains an internal structure "struct dw_edma" that is > > used by the eDMA core internally. This structure should not be touched > > by the eDMA controller drivers themselves. But currently, the eDMA > > controller drivers like "dw-edma-pci" allocates and populates this > > internal structure then passes it on to eDMA core. The eDMA core further > > populates the structure and uses it. This is wrong! > > > > Hence, move all the "struct dw_edma" specifics from controller drivers > > to the eDMA core. > > Thanks for the patchset. Alas it has just drawn my attention on v3 > stage, otherwise I would have given to you my thoughts stright away on > v1. Anyway first of all a cover letter would be very much appropriate > to have a general notion about all the changes introduced in the set. > +1 for cover letter. > Secondly I've just been working on adding the eDMA platform support > myself, so you have been just about a week ahead of me submitting my > changes. Welcome to the ship :) We (me and my colleague) were also working on eDMA support for Qcom platform, so jumped in. > My work contains some of the modifications done by you (but > have some additional fixes too) so I'll need to rebase it on top of > your patchset when it's finished. Anyway am I understand correctly, > that you've also been working on the DW PCIe driver alteration so one > would properly initialize the eDMA-chip data structure? If so have you > sent the patchset already? Could you give me a link and add me to Cc > in the emailing thread? (That's where the cover letter with all the > info and related patchsets would be very helpful.) > https://lore.kernel.org/linux-pci/20220309120149.GB134091@thinkpad/T/#m979eb506c73ab3cfca2e7a43635ecdaec18d8097 But this patch got dropped in v3 as the ep support for imx driver has not landed yet. > Thirdly regarding this patch. Your modification is mainly correct, but > I would suggest to change the concept. Instead of needlessly converting > the code to using the dw_edma_chip structure pointer within the DW eDMA > driver, it would be much easier in modification and more logical to > keep using the struct dw_edma pointer. Especially seeing dw_edma > structure is going to be a pure private data. So to speak what I would > suggest is to have the next pointers setup: > I'm afraid that this will not work for all cases (unless I miss something). As Zhi Li pointed out, there are places where only chip pointer will be passed and we'd need to extract the private data (dw_edma) from it. Tbh I also considered your idea but because of the above mentioned issue and also referring to other implementations like gpiochip, I settled with Frank's idea of copying the fields. Thanks, Mani
