Re: [PATCH v2 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver

[Bjorn Helgaas <helgaas@kernel.org>]

On Fri, Jun 10, 2022 at 09:03:03AM +0900, Wangseok Lee wrote:
> On 06/04/2022 01:03, Bjorn Helgaas wrote:
> > In the subject, why do you tag this "axis"?  There's an existing
> > pcie-artpec6.c that uses the driver name ""artpec6-pcie" and the
> > subject line tag "artpec6".
> > 
> > This adds pcie-artpec8.c with driver name "artpec8-pcie", so the
> > obvious choice would be "artpec8".
> > 
> > I assume you evaluated the possibility of extending artpec6 to support
> > artpec8 in addition to the artpec6 and artpec7 it already supports?
>  
> "pcie-artpec6. c" supports artpec6 and artpec7 H/W.
> artpec8 can not be expanded because H/W configuration is
> completely different from artpec6/7.
> phy and sub controller are different.

Thanks for this detail.  Can you include this in the commit log next
time around in case anybody else has a similar question?

> >> +/* FSYS SYSREG Offsets */
> > 
> > The list below seems to inclue more than just register offsets.
> > 
>  
> Is it clear to change to "FSYS blue logic system registers" 
> like Jasper Nilsson`s comment?
> https://lore.kernel.org/all/20220607070332.GY18902@axis.com/
> My opinion is the same.

Yep, that's fine.  But spell it "glue logic", not "blue logic" :)

> >> +static int artpec8_pcie_get_clk_resources(struct platform_device *pdev,
> >> +                                       struct artpec8_pcie *artpec8_ctrl)
> >> +{
> >> +        struct device *dev = &pdev->dev;
> >> +
> >> +        artpec8_ctrl->pipe_clk = devm_clk_get(dev, "pipe_clk");
> >> +        if (IS_ERR(artpec8_ctrl->pipe_clk)) {
> >> +                dev_err(dev, "couldn't get pipe clock\n");
> >> +                return -EINVAL;
> >> +        }
> >> +
> >> +        artpec8_ctrl->dbi_clk = devm_clk_get(dev, "dbi_clk");
> >> +        if (IS_ERR(artpec8_ctrl->dbi_clk)) {
> >> +                dev_info(dev, "couldn't get dbi clk\n");
> >> +                return -EINVAL;
> >> +        }
> >> +
> >> +        artpec8_ctrl->slv_clk = devm_clk_get(dev, "slv_clk");
> >> +        if (IS_ERR(artpec8_ctrl->slv_clk)) {
> >> +                dev_err(dev, "couldn't get slave clock\n");
> >> +                return -EINVAL;
> >> +        }
> >> +
> >> +        artpec8_ctrl->mstr_clk = devm_clk_get(dev, "mstr_clk");
> >> +        if (IS_ERR(artpec8_ctrl->mstr_clk)) {
> >> +                dev_info(dev, "couldn't get master clk\n");
> > 
> > It'd be nice if the err/info messages matched the exact DT name:
> > "pipe_clk", "dbi_clk", slv_clk", etc.
> > 
>  
> I will fix it.
>  
> > Why are some of the above dev_err() and others dev_info() when you
> > return -EINVAL in all cases?
>  
> When property is not found, it just to return error.
> I will modify to return PTR_ERR.

Using PTR_ERR() looks like a good idea, since then you return the
actual error from devm_clk_get() instead of always returning -EINVAL.

But that wasn't my comment.  My comment was that it looks like these
should be all dev_err() (or all dev_info()).

> >> +        switch (mode) {
> >> +        case DW_PCIE_RC_TYPE:
> >> +                artpec8_pcie_writel(artpec8_ctrl->elbi_base, DEVICE_TYPE_RC,
> >> +                                PCIE_ARTPEC8_DEVICE_TYPE);
> >> +                ret = artpec8_add_pcie_port(artpec8_ctrl, pdev);
> >> +                if (ret < 0)
> > 
> > Are there positive return values that indicate success?  Most places
> > above you assume "ret != 0" means failure, so just curious why you
> > test "ret < 0" instead of just "ret".
>  
> There is no special reason, but it seems that the format used 
> in the existing dw driver is applied.

Fair enough.  "git grep -A2 add_pcie_port drivers/pci/controller/"
says all *_add_pcie_port() calls use the same pattern, so thanks for
following that.

Bjorn