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

[Serge Semin <fancer.lancer@gmail.com>]

On Wed, Mar 09, 2022 at 11:42:33PM +0530, Manivannan Sadhasivam wrote:
> 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.

I see. Ok then.

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

What places are these? I see the only obstacle is the dw_edma_remove()
method. But it's easily fixable. Except that, everything else is more
or less straightforward (just a few methods need to have prototypes
converted to accepting dw_edma instead dw_edma_chip).

In order to make the code design more coherent, we need to split up
private data and device/platform info. As I see it dw_edma_chip is
nothing but a chip info data. The eDMA driver is supposed to mainly
use and pass it's private data, not the platform info. It will greatly
improve the code readability and maintainability. Such approach will
also prevent a temptation of adding new private data fields into the
dw_edma_chip structure since reaching the pointer to dw_edma will be
much easier that getting the dw_edma_chip data. In this case
dw_edma_chip will be something like i2c_board_info in i2c.

Ideally dw_edma_chip could be a temporarily defined device info, which
memory after the dw_edma_probe() method invocation could be freed. But
in order to implement that we'd need a bit more modifications
introduced.

Last but not least the approach suggested by me is easier to
implement, thus having easier review, easier backporting and causing
less potential bugs.

-Sergey

> 
> Thanks,
> Mani