On 06/11/2022 00:30, Bjorn Helgaas wrote: > 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? > Ok, sure. >> >> +/* 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@xxxxxxxx/ >> My opinion is the same. > > Yep, that's fine. But spell it "glue logic", not "blue logic" :) > Thanks, it was just a typo. >> >> +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()). > I understood your question. I think it was simply a way to generate log msg. In this case, is there a more proper print function that use to generate log msg? In addition, error return in artpec8_pcie_get_clk_resources() will be modified to devm_clk_bulk_get(). (according to Krzysztof's review comment.. ) >> >> + 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 Thank you for kindness reivew. Best regards, Wangseok Lee
