Messages in this thread | | | Date | Wed, 25 Jan 2023 17:23:57 -0600 | From | Bjorn Helgaas <> | Subject | Re: [PATCH v9 24/27] dmaengine: dw-edma: Relax driver config settings |
| |
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
| |