Date:   Wed, 25 Jan 2023 17:23:57 -0600
From:   Bjorn Helgaas <helgaas@kernel.org>
To:     Serge Semin <fancer.lancer@gmail.com>
Cc:     Serge Semin <Sergey.Semin@baikalelectronics.ru>,
        Gustavo Pimentel <gustavo.pimentel@synopsys.com>,
        Vinod Koul <vkoul@kernel.org>, Rob Herring <robh@kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>,
        Cai Huoqing <cai.huoqing@linux.dev>,
        Robin Murphy <robin.murphy@arm.com>,
        Jingoo Han <jingoohan1@gmail.com>, Frank Li <Frank.Li@nxp.com>,
        Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>,
        Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>,
        Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        caihuoqing <caihuoqing@baidu.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
        linux-pci@vger.kernel.org, dmaengine@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v9 24/27] dmaengine: dw-edma: Relax driver config settings
Message-ID: <20230125232357.GA1176625@bhelgaas>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230125144019.sn7kliw3qlwgtwzs@mobilestation>
Precedence: bulk

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