Re: [PATCH v3 1/6] dmaengine: dw-edma: fix dw_edma_probe() can't be call globally

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

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