Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 22 Jul 2021 17:24:04 +0530
From:   Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     kishon@ti.com, lorenzo.pieralisi@arm.com, bhelgaas@google.com,
        robh@kernel.org, devicetree@vger.kernel.org,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-arm-msm@vger.kernel.org, hemantk@codeaurora.org,
        smohanad@codeaurora.org, bjorn.andersson@linaro.org,
        sallenki@codeaurora.org, skananth@codeaurora.org,
        vpernami@codeaurora.org, vbadigan@codeaurora.org
Subject: Re: [PATCH v6 2/3] PCI: dwc: Add Qualcomm PCIe Endpoint controller
 driver
Message-ID: <20210722115404.GB4446@workstation>
References: <20210714083316.7835-3-manivannan.sadhasivam@linaro.org>
 <20210714191509.GA1864706@bjorn-Precision-5520>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210714191509.GA1864706@bjorn-Precision-5520>
User-Agent: Mutt/1.9.4 (2018-02-28)
Precedence: bulk

On Wed, Jul 14, 2021 at 02:15:09PM -0500, Bjorn Helgaas wrote:
> Can you use the driver name for *this* driver in the subject instead
> of "dwc"?  Then we'll be able to identify changes related to this
> driver easily in the "git log --oneline" output.
> 

Okay sure. I usually do that for the drivers that got merged but don't
have any issues with driver prefix while under review.

> I'm not sure what that name should be -- you have the PCIE_QCOM_EP
> config symbol and "qcom-pcie-ep" as the driver.name.  Both seem
> possibly a little too generic.  We already have a "qcom" driver
> (drivers/pci/controller/dwc/pcie-qcom.c).  Is this for the same
> hardware in endpoint mode?
> 

Yes

> Will this driver support every endpoint device from Qualcomm, even
> future ones?  People often use a model or codename here (xgene,
> aardvark, armada, tegra, etc).  But I guess you can always use
> something more specific for future drivers if/when Qualcomm produces
> something that requires a different driver.
> 

Since pcie-qcom.c supports most of the RC IPs in recent SoCs, I can
assure that this driver can handle all EPs.

> On Wed, Jul 14, 2021 at 02:03:15PM +0530, Manivannan Sadhasivam wrote:
> > Add driver support for Qualcomm PCIe Endpoint controller driver based on
> > the Designware core with added Qualcomm specific wrapper around the
> > core. The driver support is very basic such that it supports only
> > enumeration, PCIe read/write, and MSI. There is no ASPM and PM support
> > for now but these will be added later.
> > 
> > The driver is capable of using the PERST# and WAKE# side-band GPIOs for
> > operation and written on top of the DWC PCI framework.
> > 

[...]

> > +static int qcom_pcie_establish_link(struct dw_pcie *pci)
> > +{
> 
> This is much more complicated than existing *_pcie_establish_link()
> functions.  Are other drivers doing work like this in different
> functions?
> 
> Please try to copy the same structure and function name conventions as
> other drivers.  This applies to all the functions here.  It makes
> maintenance much easier if code doing similar things looks similar.
> 

Sure thing. I can use the patterns for the callback functions as they
are the ones shared between all drivers.

Thanks,
Mani