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

[Bjorn Helgaas]

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.  

Bjorn