From: Wangseok Lee <wangseok.lee@samsung.com> To: Bjorn Helgaas <helgaas@kernel.org> Cc: "robh+dt@kernel.org" <robh+dt@kernel.org>, "krzk+dt@kernel.org" <krzk+dt@kernel.org>, "kishon@ti.com" <kishon@ti.com>, "vkoul@kernel.org" <vkoul@kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "jesper.nilsson@axis.com" <jesper.nilsson@axis.com>, "lars.persson@axis.com" <lars.persson@axis.com>, "bhelgaas@google.com" <bhelgaas@google.com>, "linux-phy@lists.infradead.org" <linux-phy@lists.infradead.org>, "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>, "kw@linux.com" <kw@linux.com>, "linux-arm-kernel@axis.com" <linux-arm-kernel@axis.com>, "kernel@axis.com" <kernel@axis.com>, Moon-Ki Jun <moonki.jun@samsung.com>, Sang Min Kim <hypmean.kim@samsung.com>, Dongjin Yang <dj76.yang@samsung.com>, Yeeun Kim <yeeun119.kim@samsung.com> Subject: Re: [PATCH v2 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver Date: Mon, 13 Jun 2022 10:50:23 +0900 [thread overview] Message-ID: <20220613015023epcms2p70e6700a99042d4deb560e40ab5397001@epcms2p7> (raw) In-Reply-To: <20220610153020.GA597980@bhelgaas> 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@axis.com/ >> 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
WARNING: multiple messages have this Message-ID (diff)
From: Wangseok Lee <wangseok.lee@samsung.com> To: Bjorn Helgaas <helgaas@kernel.org> Cc: "robh+dt@kernel.org" <robh+dt@kernel.org>, "krzk+dt@kernel.org" <krzk+dt@kernel.org>, "kishon@ti.com" <kishon@ti.com>, "vkoul@kernel.org" <vkoul@kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "jesper.nilsson@axis.com" <jesper.nilsson@axis.com>, "lars.persson@axis.com" <lars.persson@axis.com>, "bhelgaas@google.com" <bhelgaas@google.com>, "linux-phy@lists.infradead.org" <linux-phy@lists.infradead.org>, "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>, "devicetree@vger.kernel.org" <devicetree@vger.kernel.org>, "lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>, "kw@linux.com" <kw@linux.com>, "linux-arm-kernel@axis.com" <linux-arm-kernel@axis.com>, "kernel@axis.com" <kernel@axis.com>, Moon-Ki Jun <moonki.jun@samsung.com>, Sang Min Kim <hypmean.kim@samsung.com>, Dongjin Yang <dj76.yang@samsung.com>, Yeeun Kim <yeeun119.kim@samsung.com> Subject: Re: [PATCH v2 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver Date: Mon, 13 Jun 2022 10:50:23 +0900 [thread overview] Message-ID: <20220613015023epcms2p70e6700a99042d4deb560e40ab5397001@epcms2p7> (raw) In-Reply-To: <20220610153020.GA597980@bhelgaas> 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@axis.com/ >> 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 -- linux-phy mailing list linux-phy@lists.infradead.org https://lists.infradead.org/mailman/listinfo/linux-phy
next prev parent reply other threads:[~2022-06-13 1:50 UTC|newest] Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <CGME20220603015431epcms2p6203908cebe6a320854136559a32b54cb@epcms2p6> 2022-06-03 1:54 ` [PATCH v2 0/5] Add support for Axis, ARTPEC-8 PCIe driver Wangseok Lee 2022-06-03 1:54 ` Wangseok Lee [not found] ` <CGME20220603015431epcms2p6203908cebe6a320854136559a32b54cb@epcms2p5> 2022-06-03 2:23 ` [PATCH v2 1/5] dt-bindings: pci: Add ARTPEC-8 PCIe controller Wangseok Lee 2022-06-03 2:23 ` Wangseok Lee 2022-06-06 10:12 ` Krzysztof Kozlowski 2022-06-06 10:12 ` Krzysztof Kozlowski [not found] ` <CGME20220603015431epcms2p6203908cebe6a320854136559a32b54cb@epcms2p8> 2022-06-08 3:30 ` Wangseok Lee 2022-06-08 3:30 ` Wangseok Lee 2022-06-08 3:30 ` [PATCH v2 2/5] dt-bindings: phy: Add ARTPEC-8 PCIe phy Wangseok Lee 2022-06-08 3:30 ` Wangseok Lee [not found] ` <CGME20220603015431epcms2p6203908cebe6a320854136559a32b54cb@epcms2p2> 2022-06-03 2:34 ` [PATCH v2 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver Wangseok Lee 2022-06-03 2:34 ` Wangseok Lee 2022-06-03 16:03 ` Bjorn Helgaas 2022-06-03 16:03 ` Bjorn Helgaas 2022-06-07 7:03 ` Jesper Nilsson 2022-06-07 7:03 ` Jesper Nilsson [not found] ` <CGME20220603160329epcas2p1bdb17f3c29513d82b156bae743d8e00d@epcms2p5> 2022-06-10 0:03 ` Wangseok Lee 2022-06-10 0:03 ` Wangseok Lee 2022-06-10 15:30 ` Bjorn Helgaas 2022-06-10 15:30 ` Bjorn Helgaas [not found] ` <CGME20220610153050epcas2p3b0d83f4f56ffe81a06aae73d8994a3d1@epcms2p7> 2022-06-13 1:50 ` Wangseok Lee [this message] 2022-06-13 1:50 ` Wangseok Lee 2022-06-06 10:23 ` Krzysztof Kozlowski 2022-06-06 10:23 ` Krzysztof Kozlowski [not found] ` <CGME20220603015431epcms2p6203908cebe6a320854136559a32b54cb@epcms2p7> 2022-06-03 2:38 ` [PATCH v2 4/5] phy: Add ARTPEC-8 PCIe PHY driver Wangseok Lee 2022-06-03 2:38 ` Wangseok Lee 2022-06-06 10:28 ` Krzysztof Kozlowski 2022-06-06 10:28 ` Krzysztof Kozlowski [not found] ` <CGME20220603015431epcms2p6203908cebe6a320854136559a32b54cb@epcms2p4> 2022-06-03 2:43 ` [PATCH v2 5/5] MAINTAINERS: Add maintainer for Axis " Wangseok Lee 2022-06-03 2:43 ` Wangseok Lee 2022-06-03 16:09 ` Bjorn Helgaas 2022-06-03 16:09 ` Bjorn Helgaas 2022-06-07 7:05 ` Jesper Nilsson 2022-06-07 7:05 ` Jesper Nilsson 2022-06-08 3:31 ` [PATCH v2 3/5] PCI: axis: Add ARTPEC-8 PCIe controller driver Wangseok Lee 2022-06-08 3:31 ` Wangseok Lee 2022-06-03 2:31 ` [PATCH v2 2/5] dt-bindings: phy: Add ARTPEC-8 PCIe phy Wangseok Lee 2022-06-03 2:31 ` Wangseok Lee 2022-06-06 10:14 ` Krzysztof Kozlowski 2022-06-06 10:14 ` Krzysztof Kozlowski 2022-06-08 4:14 ` [PATCH v2 4/5] phy: Add ARTPEC-8 PCIe PHY driver Wangseok Lee 2022-06-08 4:14 ` Wangseok Lee
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20220613015023epcms2p70e6700a99042d4deb560e40ab5397001@epcms2p7 \ --to=wangseok.lee@samsung.com \ --cc=bhelgaas@google.com \ --cc=devicetree@vger.kernel.org \ --cc=dj76.yang@samsung.com \ --cc=helgaas@kernel.org \ --cc=hypmean.kim@samsung.com \ --cc=jesper.nilsson@axis.com \ --cc=kernel@axis.com \ --cc=kishon@ti.com \ --cc=krzk+dt@kernel.org \ --cc=kw@linux.com \ --cc=lars.persson@axis.com \ --cc=linux-arm-kernel@axis.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-phy@lists.infradead.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=moonki.jun@samsung.com \ --cc=robh+dt@kernel.org \ --cc=vkoul@kernel.org \ --cc=yeeun119.kim@samsung.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.