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
