Re: [PATCH v9 24/27] dmaengine: dw-edma: Relax driver config settings

[Serge Semin]

 On Wed, Jan 25, 2023 at 05:23:57PM -0600, Bjorn Helgaas wrote:
> On Wed, Jan 25, 2023 at 05:40:19PM +0300, Serge Semin wrote:
> > On Tue, Jan 24, 2023 at 05:47:44PM -0600, Bjorn Helgaas wrote:
> 
> > > In the commit log, I think "forcibly selecting the DW eDMA driver from
> > > the DW PCIe RP/EP kconfig" actually refers to just the "DW eDMA PCIe"
> > > driver" not the "DW PCIe RP/EP driver," right?
> > 
> > Right.
> 
> Good.  I think it's worth updating the commit log to clear this up
> because there are several things with very similar names, so it's
> confusing enough already ;)
> 
> > > The undefined reference to dw_edma_probe() doesn't actually happen
> > > unless we merge 27/27 without *this* patch, right? 
> > 
> > Right.
> 
> Thanks, I got unreasonably focused on the "fix 'undefined reference'
> error" comment, wondering if we needed to identify a Fixes: commit, so
> this clears that up, too.
> 
> > > I would use "depends on
> > >      DW_EDMA" instead of adding if/endif around DW_EDMA_PCIE.
> > 
> > Could you explain why is the "depends on" operator more preferable
> > than if/endif? In this case since we have a single core kconfig from
> > which all the eDMA LLDD config(s) (except PCIE_DW for the reason
> > previously described) will surely depend on, using if/endif would
> > cause the possible new eDMA-capable LLDD(s) adding their kconfig
> > entries within the if-endif clause without need to copy the same
> > "depends on DW_EDMA" pattern over and over. That seems to look a bit
> > more maintainable than the alternative you suggest. Do you think
> > otherwise?
> 
> Only that "depends on" is much more common and I always try to avoid
> unusual constructs.  But I wasn't looking into the future and
> imagining several LLDDs with similar uses of "depends on DW_EDMA".
> Thanks for that perspective; with it, I think it's OK either way.
> 
> > > What do you think? 
> > 
> > What you described was the second option I had in mind for the update
> > to look like, but after all I decided to take a shorter path and
> > combine the modifications into a single patch. If you think that
> > splitting it up would make the update looking simpler then I'll do as
> > you suggest. But in that case Lorenzo will need to re-merge the
> > updated patchset v10.
> 

> It's a pretty trivial update, so I just did it myself.  The result is
> at https://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git/log/?h=pci/ctrl/dwc&id=ecadcaed4ef7
> 
> I split this patch and tweaked some commit messages for consistency
> (including the "DW eDMA PCIe driver" change above).  "git diff -b"
> with Lorenzo's current branch (95624672bb3e ("PCI: dwc: Add DW eDMA
> engine support")) is empty except for a minor comment change.  

Great! Thanks. Although I've already created v10 beforehand but didn't
submitted it yet waiting for your response. The split up patches look
exactly like yours.

In addition to that since I was going to re-send v10 I also took into
account your comments regarding the patch:
[PATCH v9 19/27] dmaengine: dw-edma: Use non-atomic io-64 methods
Link: https://lore.kernel.org/linux-pci/20230113171409.30470-20-Sergey.Semin@xxxxxxxxxxxxxxxxxxxx/
I've dropped unneeded modification and unpinned another fixes patch
which turned out to be a part of those modifications. So if you
re-based your pci/ctrl/dwc branch with that patch replaced with the
patches attached to this email it would have been great. Otherwise
it's ok to merge the series as is.

Note in the attached "non-atomic io-64" patch I've already replaced
the commit log with the your short version.

-Sergey

> 
> Bjorn